diff --git a/src/mbsf/DistributionSessionInfo.cc b/src/mbsf/DistributionSessionInfo.cc index 0001753..74bd33f 100644 --- a/src/mbsf/DistributionSessionInfo.cc +++ b/src/mbsf/DistributionSessionInfo.cc @@ -162,6 +162,21 @@ CJson DistributionSessionInfo::json(bool as_request = false) const return m_mbsDistributionSessionInfo->toJSON(as_request); } +namespace { +// Compares two optional> fields, treating "absent" and "present but null" as the +// same "no value" state so a field that's merely re-sent unchanged doesn't spuriously trip up as +// "changed". +template +bool optionalPtrFieldsEqual(const std::optional> &a, const std::optional> &b) +{ + bool a_has = a.has_value() && a.value(); + bool b_has = b.has_value() && b.value(); + if (a_has != b_has) return false; + if (!a_has) return true; + return *a.value() == *b.value(); +} +} + std::shared_ptr &DistributionSessionInfo::updateMBSDistributionSessionInfo( std::shared_ptr new_mbs_dist_session_infos) { @@ -175,12 +190,34 @@ std::shared_ptr &DistributionSessionInfo::updateMBSD m_mbsDistributionSessionInfo->setTgtServAreas(std::move(new_mbs_dist_session_infos->getTgtServAreas())); + // TS 26.502 clause 4.5.6 lists these among the parameters the MBS Application + // Provider may update at any time, alongside mbsServInfo/mbsFSAId/tgtServAreas + // above -- not gated on INACTIVE like the block below. + m_mbsDistributionSessionInfo->setExtTgtServAreas(std::move(new_mbs_dist_session_infos->getExtTgtServAreas())); + m_mbsDistributionSessionInfo->setNrRedCapUeInfo(std::move(new_mbs_dist_session_infos->getNrRedCapUeInfo())); + // -------------------------------------------------------------------- // 2. Conditional updates – only when the session is INACTIVE // -------------------------------------------------------------------- std::optional > dist_session_state = m_mbsDistributionSessionInfo->getMbsDistSessState(); + bool is_inactive = dist_session_state.has_value() && dist_session_state.value()->getValue() == DistSessionState::VAL_INACTIVE; + + // BUG FIX: objDistrInfo/pckDistrInfo (and hence e.g. objAcqIds) were previously only ever + // copied across below when the Distribution Session is INACTIVE -- if PATCHed while + // ESTABLISHED/ACTIVE the request was accepted (200) but the change was silently dropped, so + // e.g. a PATCH narrowing objAcqIds would echo back the OLD array. These fields are only + // mutable while INACTIVE, so reject the request outright instead of silently no-op'ing it. + if (!is_inactive) { + if (!optionalPtrFieldsEqual(m_mbsDistributionSessionInfo->getObjDistrInfo(), new_mbs_dist_session_infos->getObjDistrInfo()) || + !optionalPtrFieldsEqual(m_mbsDistributionSessionInfo->getPckDistrInfo(), new_mbs_dist_session_infos->getPckDistrInfo())) { + throw ModelException( + "objDistrInfo/pckDistrInfo cannot be modified while the MBS Distribution Session is not INACTIVE", + "MBSDistributionSessionInfo", "objDistrInfo", + fiveg_mag_reftools::ProblemCause::MODIFICATION_NOT_ALLOWED); + } + } - if (dist_session_state.has_value() && dist_session_state.value()->getValue() == DistSessionState::VAL_INACTIVE) { + if (is_inactive) { // ----- Max Continuous Bit Rate ----- m_mbsDistributionSessionInfo->setMaxContBitRate(std::move(new_mbs_dist_session_infos->getMaxContBitRate())); @@ -202,18 +239,12 @@ std::shared_ptr &DistributionSessionInfo::updateMBSD // ----- Traffic Marking Info ----- m_mbsDistributionSessionInfo->setTrafficMarkingInfo(std::move(new_mbs_dist_session_infos->getTrafficMarkingInfo())); - // ----- External Target Service Areas ----- - m_mbsDistributionSessionInfo->setExtTgtServAreas(std::move(new_mbs_dist_session_infos->getExtTgtServAreas())); - // ----- Multiplexed Service Flag ----- m_mbsDistributionSessionInfo->setMultiplexedServFlag(std::move(new_mbs_dist_session_infos->getMultiplexedServFlag())); // ----- Restricted Flag ----- m_mbsDistributionSessionInfo->setRestrictedFlag(std::move(new_mbs_dist_session_infos->getRestrictedFlag())); - // ----- NR RedCap UE Info ----- - m_mbsDistributionSessionInfo->setNrRedCapUeInfo(std::move(new_mbs_dist_session_infos->getNrRedCapUeInfo())); - // ----- Associated Session Id ----- m_mbsDistributionSessionInfo->setAssociatedSessionId(std::move(new_mbs_dist_session_infos->getAssociatedSessionId())); } diff --git a/src/mbsf/MBSMFMBSSession.cc b/src/mbsf/MBSMFMBSSession.cc index c6bec1e..0f22d86 100644 --- a/src/mbsf/MBSMFMBSSession.cc +++ b/src/mbsf/MBSMFMBSSession.cc @@ -248,6 +248,16 @@ bool MBSMFMBSSession::processEvent(Open5GSEvent &MBSMFEvent) } UserDataIngSession::setMBSSessionFlag(*ids); } else if (mbsf_event->result == OGS_ERROR) { + // BUG FIX: the fallback call below used to run unconditionally, so even + // when a specific cause was already matched and handled just above (e.g. + // the registered 403 MBS_DIST_SESSION_ALREADY_CREATED), it was immediately + // overwritten by a second, generic INBOUND_SERVER_ERROR call with no + // problem_detail -- meaning the client only ever saw the generic 502-class + // error regardless of what MB-SMF actually reported. Track whether a cause + // (specific or the "no cause string" generic-with-detail case) was already + // handled and only fall through to the bare generic call as a genuine + // last resort (no problem_details at all, or an unregistered cause string). + bool cause_handled = false; if (mbsf_event->problem_details) { cJSON *problem = OpenAPI_problem_details_convertToJSON((OpenAPI_problem_details_t*)mbsf_event->problem_details); CJson problem_detail(problem, true); @@ -256,12 +266,16 @@ bool MBSMFMBSSession::processEvent(Open5GSEvent &MBSMFEvent) MBSProblemCause::lookup(std::string(mbsf_event->problem_details->cause)); if (cause.has_value()) { UserDataIngSession::setMBSSessionFailureFlag(*ids, cause.value(), problem_detail); + cause_handled = true; } } else { UserDataIngSession::setMBSSessionFailureFlag(*ids, ProblemCause::INBOUND_SERVER_ERROR, problem_detail); + cause_handled = true; } } - UserDataIngSession::setMBSSessionFailureFlag(*ids, ProblemCause::INBOUND_SERVER_ERROR); + if (!cause_handled) { + UserDataIngSession::setMBSSessionFailureFlag(*ids, ProblemCause::INBOUND_SERVER_ERROR); + } } else { UserDataIngSession::setMBSSessionFailureFlag(*ids, ProblemCause::INBOUND_SERVER_ERROR); } diff --git a/src/mbsf/MBSNcgi.cc b/src/mbsf/MBSNcgi.cc index fdd5cda..3546df0 100644 --- a/src/mbsf/MBSNcgi.cc +++ b/src/mbsf/MBSNcgi.cc @@ -81,7 +81,9 @@ mb_smf_sc_ncgi_t *MBSNcgi::populateNcgi() { mb_smf_sc_ncgi_t *ncgi = mb_smf_sc_ncgi_new(); - mb_smf_sc_ncgi_set_plmn_id(ncgi, mcc, mnc); + // Use the length-aware setter: mcc()/mnc() alone lose the MNC's actual digit + // count (2 vs 3), which a plain numeric value under 100 cannot distinguish. + mb_smf_sc_ncgi_set_plmn_id_len(ncgi, mcc, mnc, mbs_plmn_id->mncLen()); uint64_t cell_id = nrCellId(); ncgi->nr_cell_id = static_cast(cell_id) & ((1ULL << 36) - 1); ncgi->nid = nid(); diff --git a/src/mbsf/MBSPlmnId.hh b/src/mbsf/MBSPlmnId.hh index 6f6c78e..b1aa734 100644 --- a/src/mbsf/MBSPlmnId.hh +++ b/src/mbsf/MBSPlmnId.hh @@ -56,6 +56,10 @@ public: uint16_t mcc(); uint16_t mnc(); + // The MNC's actual digit count (2 or 3), taken directly from the source + // string rather than guessed from mnc()'s numeric value -- a 3-digit MNC + // under 100 (e.g. "001") is otherwise indistinguishable from a 2-digit one. + uint8_t mncLen() const {return static_cast(getMnc().length());}; private: std::shared_ptr m_plmnId; diff --git a/src/mbsf/Nmb2Build.cc b/src/mbsf/Nmb2Build.cc index ecc4a10..17a98fc 100644 --- a/src/mbsf/Nmb2Build.cc +++ b/src/mbsf/Nmb2Build.cc @@ -242,7 +242,8 @@ ogs_sbi_request_t *Nmb2Build::buildNmb2DistSessionPatch(void *context, void *dat std::shared_ptr context_data_ptr(ing_session->getDistributionSessionInfoData(session_ids->second->second)); DistSessionState req_state; if (context_data_ptr->needsUpdate) { - status_item.path = (char *)"/distSession"; + // RFC 6901: the whole document is addressed by the empty JSON Pointer "". + status_item.path = (char *)""; std::shared_ptr dist_session = build_nmb2_create_dist_session(ing_session, context_data_ptr); std::string sess_id(context_data_ptr->mbstfDistSessionId); @@ -250,7 +251,20 @@ ogs_sbi_request_t *Nmb2Build::buildNmb2DistSessionPatch(void *context, void *dat dist_session->setDistSessionId(sess_id); UserDataIngSession::addToRegistry(sess_id, session_ids->second); - patch_val = dist_session->toJSON(true); + // BUG FIX (found live, 2026-08-11): MBSTF stores the PATCH target for + // /dist-sessions/{id} as a CreateReqData (see DistributionSession.cc, which patches + // distributionSessionReqData() -- a CreateReqData, not a bare DistSession), and + // CreateReqData::fromJSON() (invoked by its applyJSONPatch() for an empty-path + // add/replace) requires its value to be a full CreateReqData document -- i.e. an + // object with a "distSession" property wrapping the DistSession fields, not the + // DistSession's own JSON directly. Sending dist_session->toJSON() unwrapped, as this + // used to, made every needsUpdate PATCH (a content/session change, not just a state + // change -- see the stateUpdate branch below for that) fail with "Mandatory + // Information Element Missing: distSession: Field \"distSession\" is required", + // silently breaking updates to any already-created distribution session. + CJson wrapped_patch_val = CJson::newObject(); + wrapped_patch_val.set("distSession", dist_session->toJSON(true)); + patch_val = wrapped_patch_val; const auto &state = dist_session->getDistSessionState(); if (state) req_state = *state; } else if (context_data_ptr->stateUpdate) { @@ -264,6 +278,19 @@ ogs_sbi_request_t *Nmb2Build::buildNmb2DistSessionPatch(void *context, void *dat req_state = want_state; } patch_val = req_state.toJSON(); + // BUG FIX (found live, 2026-08-10): MBSTF's actual PATCH target for + // /dist-sessions/{id} is CreateReqData (see DistributionSession::_apiSessionPatch(), + // which patches distributionSessionReqData(), a CreateReqData), and CreateReqData's + // generated applyPatch() (CreateReqData.cc) only recognises paths under its own + // "/distSession" property -- it does NOT expose DistSession's fields at the top + // level. The previous "/distSessionState" (no wrapper) path was rejected by MBSTF + // with "Runtime Error: Unknown path in JSON Patch", which meant every state-only + // PATCH (activate/deactivate) silently failed and triggered a rollback -- including + // the one that activates the built-in "USER SERVICE ANNOUNCEMENT CHANNEL" session, + // i.e. the real MBS-4-MC broadcast Service Announcement carousel never got updated + // with newly-provisioned services. Confirmed against CreateReqData.cc's path_prefix + // dispatch: it matches "/distSession" then delegates the remainder ("/distSessionState") + // to the nested DistSession object, which does recognise it (see DistSession.cc). status_item.path = (char *)"/distSession/distSessionState"; } } diff --git a/src/mbsf/TrackingAreaIdentity.cc b/src/mbsf/TrackingAreaIdentity.cc index eabd369..1ed00df 100644 --- a/src/mbsf/TrackingAreaIdentity.cc +++ b/src/mbsf/TrackingAreaIdentity.cc @@ -86,7 +86,9 @@ mb_smf_sc_tai_t *TrackingAreaIdentity::populateTai() { tracking_area = tac(); n_id = nid(); - return mb_smf_sc_tai_new(mcc, mnc, tracking_area, n_id); + // Use the length-aware constructor: mcc()/mnc() alone lose the MNC's actual + // digit count (2 vs 3), which a plain numeric value under 100 cannot distinguish. + return mb_smf_sc_tai_new_len(mcc, mnc, mbs_plmn_id->mncLen(), tracking_area, n_id); } uint32_t TrackingAreaIdentity::tac() { @@ -124,12 +126,9 @@ uint32_t TrackingAreaIdentity::tac() { uint64_t* TrackingAreaIdentity::nid() { const std::optional &nid = getNid(); if (!nid.has_value()) return nullptr; - uint64_t value = 0; - for (char ch : nid.value()) { - if (std::isdigit(static_cast(ch))) { - value = value * 10 + (ch - '0'); - } - } + // TS 29.571 Nid is an 11-character hex string (44-bit SNPN Network Id) -- + // parse as base 16, matching the correct sibling implementation MBSNcgi::nid(). + uint64_t value = std::stoull(nid.value(), nullptr, 16); uint64_t *result = static_cast(std::malloc(sizeof(uint64_t))); if (result != nullptr) { diff --git a/src/mbsf/UserDataIngSession.cc b/src/mbsf/UserDataIngSession.cc index 1ef6a3f..821076b 100644 --- a/src/mbsf/UserDataIngSession.cc +++ b/src/mbsf/UserDataIngSession.cc @@ -495,6 +495,26 @@ bool UserDataIngSession::processEvent(Open5GSEvent &event) ogs_debug("Patch Request Parsed JSON: %s", txt.c_str()); } + // Reject actPeriods/actPeriodsRepRule given together, mirroring the + // mutual-exclusion check validate_state_setting_options() already + // enforces on POST (TS 29.580 clause 6: the two are mutually exclusive). + // This is a PUT-only fix: PATCH on this resource is intentionally not + // implemented yet (returns 404 above), so it is not affected. + try { + MBSUserDataIngSession update_model(user_data_ing_sess_update, true); + if (update_model.getActPeriods() && update_model.getActPeriodsRepRule()) { + std::map invalid_params; + invalid_params["actPeriods"] = "actPeriods cannot be present if actPeriodsRepRule is present"; + invalid_params["actPeriodsRepRule"] = "actPeriodsRepRule cannot be present if actPeriods is present"; + ogs_assert(true == NfServer::sendError(stream, ProblemCause::OPTIONAL_IE_INCORRECT, 3, message, + app_meta, api, std::nullopt, std::nullopt, std::nullopt, invalid_params)); + return true; + } + } catch (ModelException &ex) { + send_model_error(ex, stream, 3, message, app_meta, api, "Problem with UserDataIngSession update", "Validating UserDataIngSession update"); + return true; + } + try { std::shared_ptr user_data_ing_sess = find(user_data_ing_session_id); user_data_ing_sess->processUserDataIngSessionUpdate(stream_id, request_ctx, user_data_ing_sess_update); @@ -514,6 +534,18 @@ bool UserDataIngSession::processEvent(Open5GSEvent &event) ogs_assert(true == Open5GSSBIServer::sendResponse(stream, *response)); } catch (const std::out_of_range &e) { send_invalid_user_data_ing_session_err(e, stream, 3, message, app_meta, api, user_data_ing_session_id); + } catch (ModelException &ex) { + // BUG FIX (found live, 2026-08-11): processUserDataIngSessionUpdate() + // (via updateMBSDistributionSessionInfo()) throws a ModelException for + // a genuinely invalid update -- e.g. PATCHing objDistrInfo/pckDistrInfo + // while the Distribution Session isn't INACTIVE (correctly rejected, + // not a bug in itself) -- but nothing here caught it, so it propagated + // all the way out of the SBI request handler uncaught and crashed the + // entire MBSF process via std::terminate(), taking down every other + // active session with it over a single bad client request. Convert it + // to a proper error response instead, same pattern as the + // actPeriods/actPeriodsRepRule validation above. + send_model_error(ex, stream, 3, message, app_meta, api, "Problem with UserDataIngSession update", "Applying UserDataIngSession update"); } return true; @@ -1074,7 +1106,10 @@ void UserDataIngSession::updateContexts(ogs_pool_id_t stream_id, const std::shar .ssm_port = port, .request = request, .streamId = stream_id, - .tsi = tsi + .tsi = tsi, + // See createMbsSession()'s comment: captured here (an + // instance method, has "this") rather than looked up later. + .userServType = mbsUserService() ? mbsUserService()->getMBSUserServiceType() : std::string{} }); addToDistributionSessionInfos(key, ctx_data); createMbsSession(ctx_data); @@ -1132,9 +1167,20 @@ void UserDataIngSession::userServiceAnnChannelDistributionSessionInfo() const std::optional &dest_ipv4_addr = dest_ip_addr->getIpv4Addr(); const std::optional> &dest_ipv6_addr = dest_ip_addr->getIpv6Addr(); std::shared_ptr ssm_data(new Ssm(*ssm_val)); - static std::random_device rd; - static std::uniform_int_distribution ud(32768, 65535); - in_port_t port = ud(rd); + // BUG FIX: this used to draw a fresh random port (ud(rd), the same + // generator the regular per-content-session branch above uses, where a + // new port every session is genuinely correct) for the Service + // Announcement channel too -- but the announcement channel is meant to + // be a single, fixed, well-known channel a client can bootstrap from + // static configuration (see mbsf.yaml's userServiceAnnouncement.ssmPort, + // and rt-mbs-client.conf's matching mbsf_client.announcement_channel). + // Confirmed live: with the random port, MBSTF ended up transmitting the + // real FLUTE carousel on some other, unpredictable port every run (e.g. + // 41873), while any client bootstrapped from the configured ssmPort + // (3000) filtered every real packet out silently, since it never matches + // -- ssmPort was already correctly plumbed through Context (see + // Context::userServiceAnnSsmPort()), it just wasn't used here. + in_port_t port = static_cast(App::self().context()->userServiceAnnSsmPort()); uint64_t tsi = 0; if (info->getDistrMethod()->getValue() == DistributionMethod::VAL_OBJECT) { tsi = 1; @@ -1151,7 +1197,11 @@ void UserDataIngSession::userServiceAnnChannelDistributionSessionInfo() .ssm_port = port, .request = nullptr, .streamId = 0, - .tsi = tsi + .tsi = tsi, + // This is the built-in Service Announcement carousel + // channel (see this method's name) -- MBS-4-MC Service + // Announcement is inherently a broadcast delivery, always. + .userServType = std::string("BROADCAST") }); addToDistributionSessionInfos(key, ctx_data); nmbstfDiscoverOnly(ctx_data); @@ -1227,12 +1277,44 @@ void UserDataIngSession::processUserDataIngSessionUpdate(ogs_pool_id_t stream_id // update std::shared_ptr update_info = sess_info_update.value(); - // Copy old MBS Dist Session Id + // TS 29.580 clause 5.3.2.4.2: mbsSessionId, mbsDistSessionId and + // locationDependent shall never be updated after provisioning -- + // restore all three from the stored value before comparing/applying, + // not just mbsDistSessionId. update_info->setMbsDistSessionId(info->getMbsDistSessionId()); - - if (*update_info != *info) { + update_info->setMbsSessionId(info->getMbsSessionId()); + update_info->setLocationDependent(info->getLocationDependent()); + + // BUG FIX (found live, 2026-08-11): mbsDistSessState is included in + // MBSDistributionSessionInfo::operator!=, so a PUT that changes ONLY the + // state (activate/deactivate -- the common case, since this API has no + // separate lightweight state-only endpoint) always took the needsUpdate + // branch below, which rebuilds and PATCHes the ENTIRE MBSTF distribution + // session, instead of the purpose-built, lightweight stateUpdate path (see + // setDistSessionState() / buildNmb2DistSessionPatch()'s + // "/distSession/distSessionState" branch) meant for exactly this. Compare + // with state normalised out first, so pure state changes are classified + // as stateUpdate, not needsUpdate; a change to anything else still counts + // as needsUpdate regardless of whether state also changed (the needsUpdate + // path's rebuilt DistSession already carries the new state along with it). + const auto orig_update_state = update_info->getMbsDistSessState(); + update_info->setMbsDistSessState(info->getMbsDistSessState()); + bool content_changed = (*update_info != *info); + update_info->setMbsDistSessState(orig_update_state); + + if (content_changed) { context_data->needsUpdate = true; context_data->distributionSessionInfo->updateMBSDistributionSessionInfo(update_info); + } else if (orig_update_state != info->getMbsDistSessState()) { + context_data->stateUpdate = true; + info->setMbsDistSessState(orig_update_state); + // buildNmb2DistSessionPatch()'s stateUpdate branch reads the wanted + // state off context_data->info, which is normally the same object as + // this loop's info -- set both explicitly rather than relying on that + // aliasing. + if (context_data->info && context_data->info != info) { + context_data->info->setMbsDistSessState(orig_update_state); + } } } update_dist_sess_infos.erase(key_in_update); @@ -1620,6 +1702,24 @@ bool UserDataIngSession::handleMbstfDiscover(ogs_sbi_nf_instance_t *nf_instance, bool UserDataIngSession::createMbsSession(const std::shared_ptr &context_data) { + // BUG FIX: this function used to unconditionally build a brand new MBSMFMBSSession (and the + // underlying mb_smf_sc_mbs_session_new_ipv4()/_ipv6() C session object) on every call, only + // guarding the *assignment* to context_data->MBSSession ("if (!context_data->MBSSession)" + // below) rather than the work itself. isMBSSessionCreated() only flips to true once the + // underlying MB-SMF session genuinely reaches CREATED state -- if MBSTF ever rejects the + // distribution session (e.g. a malformed request), that never happens, and + // userServiceAnnChannelDistributionSessionInfo()'s periodic check + // ("if (!isMBSSessionCreated(key)) createMbsSession(...)") called this again, immediately, + // every single loop iteration, forever: no backoff, no bound. Confirmed live -- MBSF spun at + // the workerLoop's tick rate (dozens of iterations/second) reconstructing the C session object + // and re-notifying MB-SMF each time, until it crashed. Skip entirely once a session object for + // this context already exists; the caller's own retry-driving state (MBSSessionStatus, + // receivedMBSTFResponse) is what should progress it from here, not another blind rebuild. + if (context_data->MBSSession) { + ogs_debug("createMbsSession: MBS Session already exists for this context, not recreating"); + return true; + } + const auto &ssm_ptr = context_data->ssm; if (!ssm_ptr) ogs_error("Unable to get SSM from Context Data"); @@ -1707,7 +1807,21 @@ bool UserDataIngSession::createMbsSession(const std::shared_ptrsetTunnelRequest(true); mb_smf_mbs_session->setTmgiRequest(true); - mb_smf_mbs_session->setServiceType(MBS_SERVICE_TYPE_MULTICAST); + // BUG FIX (found live, 2026-08-10): this unconditionally sent MULTICAST to the SMF/MB-SMF + // regardless of the parent MBS User Service's own servType. SMF's Nmbsmf handler + // (n4mb-handler.c) only triggers the Namf_MBSBroadcast context-create call -- the step + // that actually drives NGAP Broadcast Session Setup to the gNB -- "if the service type is + // broadcast service" (TS 23.247 cl.7.3.1 step 2); for MULTICAST it correctly does nothing + // here (multicast UE-join uses a separate, currently-unimplemented Namf_MBSCommunication + // procedure instead). So every BROADCAST User Service ended up silently treated as + // MULTICAST at the MB-SMF boundary: PFCP/N4mb and MBSTF FLUTE transmission all completed + // normally, but NGAP never reached the gNB, no MRB was ever created for the new session, + // and content had no bearer to travel over -- dropped after leaving the UPF with no + // visible error anywhere. Confirmed via UserService::getMBSUserServiceType(), which reads + // the real value ("BROADCAST"/"MULTICAST") straight from the User Service's own servType. + mb_smf_mbs_session->setServiceType( + ogs_strcasecmp(context_data->userServType.c_str(), "BROADCAST") == 0 + ? MBS_SERVICE_TYPE_BROADCAST : MBS_SERVICE_TYPE_MULTICAST); if (!context_data->MBSSession) context_data->MBSSession = mb_smf_mbs_session; mb_smf_mbs_session->setCallback(UserDataIngDistSessId(context_data->ingSessionId, context_data->distSessionInfoKey)); populate_mb_smf_mbs_session(context_data, mb_smf_mbs_session); @@ -2072,8 +2186,20 @@ void UserDataIngSession::setMBSSessionDeleted(const UserDataIngDistSessId &ids) } ing_sess->m_deleteRequests.clear(); if (context_data->markForDeletion) { - removeFromRegistry(ids.second); - ing_sess->removeDistributionSessionInfo(ids.first); + // BUG FIX (found live, 2026-08-12): these used to pass ids.second (the + // distSessionInfoKey, e.g. "AP_MBS_SESSION_1") to removeFromRegistry() -- which + // is keyed by the real MBSTF-assigned distribution session ID -- and ids.first + // (the ingSessionId, a UUID) to removeDistributionSessionInfo() -- which is keyed + // by distSessionInfoKey. Both erase()-by-wrong-key calls silently no-op (no + // exception, no log), so s_distSessionIdRegistry and m_distributionSessionInfos + // were never actually cleared here. Confirmed live: this left the deleted + // session's MbsSessionId (and thus its SSM address) permanently registered in + // Context::m_mbsSessionIds (see Context::addMbsSessionId's "Attempt to insert + // duplicate" warning), reproducible on every create+delete+recreate cycle with + // the same SSM, even minutes apart. Fixed to use context_data's own, correctly + // populated fields instead of the mismatched ids pair. + removeFromRegistry(context_data->mbstfDistSessionId); + ing_sess->removeDistributionSessionInfo(context_data->distSessionInfoKey); } App::self().context()->deleteUserDataIngSession(ing_sess->m_UserDataIngSessionId); } @@ -2141,15 +2267,38 @@ void UserDataIngSession::setMBSTFDistSessionDeletedFlag(const std::string &dist_ ogs_debug("Deleting MBS Session for Dist Session %s", dist_session_id.c_str()); context_data->MBSSession->deleteSession(); } - //if (context_data->markForDeletion) { - //removeFromRegistry(dist_session_id); - //ing_sess->removeDistributionSessionInfo(ids->first); - //return; - //} + // BUG FIX (found live, 2026-08-12): this whole cleanup was commented out, and the + // original draft passed ids->first (the ingSessionId, a UUID) to + // removeDistributionSessionInfo() -- which is keyed by distSessionInfoKey (e.g. + // "AP_MBS_SESSION_1") -- so even re-enabled as-is it would have silently no-op'd (see + // the identical bug fixed in setMBSSessionDeleted() above). With nothing here to ever + // erase this session from s_distSessionIdRegistry/m_distributionSessionInfos or drop the + // UserDataIngSession's last shared_ptr reference, a deleted MBS User Service's + // MbsSessionId (and its SSM address) stayed registered in Context::m_mbsSessionIds + // forever once its Distribution Session's deletion was confirmed via this path. + // Confirmed live via Context::addMbsSessionId's "Attempt to insert duplicate" warning, + // reproducible on every create+delete+recreate cycle using the same SSM. dist_session_id + // (this function's own parameter) is already the correct registry key -- no field + // lookup needed for that one. + if (context_data->markForDeletion) { + removeFromRegistry(dist_session_id); + ing_sess->removeDistributionSessionInfo(context_data->distSessionInfoKey); + return; + } } - //if (ing_sess->checkIfAllMBSTFDistSessionDeleted()) { - // App::self().context()->deleteUserDataIngSession(ing_sess->m_UserDataIngSessionId); - //} + // NOT calling deleteUserDataIngSession() here (found live, 2026-08-12): setMBSSessionDeleted() + // is the authoritative "whole ingest session torn down" trigger -- it already sends the + // deferred DELETE HTTP response(s) queued in m_deleteRequests before destroying the session, + // gated on checkIfAllMBSSessionDeletionsReceived(). A normal delete fires BOTH this function + // (MBSTF distribution session deleted) and setMBSSessionDeleted() (MB-SMF MBS Session deleted) + // for the same logical session; re-enabling this call as well raced the two paths and + // destroyed the UserDataIngSession from here first, before setMBSSessionDeleted() could send + // its queued response -- confirmed live: "User Data Ingest Session deleted before 1 pending + // responses sent" followed by the portal's DELETE request timing out after 30s waiting for a + // response that could now never be sent. checkIfAllMBSTFDistSessionDeleted() still runs, for + // its s_distSessionIdRegistry cleanup side effect, but its own deleteUserDataIngSession() call + // stays commented out (see the function itself) for the same reason. + ing_sess->checkIfAllMBSTFDistSessionDeleted(); } void UserDataIngSession::populateAndSendError(UserDataIngDistSessId *ids, const std::optional &cause, const std::optional &problem_detail_json) diff --git a/src/mbsf/UserDataIngSession.hh b/src/mbsf/UserDataIngSession.hh index 58b904a..c966c4f 100644 --- a/src/mbsf/UserDataIngSession.hh +++ b/src/mbsf/UserDataIngSession.hh @@ -124,6 +124,14 @@ public: reftools::mbsf::DistSessionState last_reported_state; std::shared_ptr distSession = nullptr; std::shared_ptr sdp = nullptr; + // BUG FIX (found live, 2026-08-10): captured here, at construction time in the owning + // UserDataIngSession instance method (which has "this" and so can call + // mbsUserService()), rather than looked up later inside the static createMbsSession() via + // locate(ingSessionId) -- that lookup raced against this object's own registration into + // the id->instance map and always lost (this ContextData is built and createMbsSession() + // is invoked on it *before* the constructing UserDataIngSession finishes registering + // itself), so it always silently fell back to "MULTICAST". See createMbsSession(). + std::string userServType = std::string{}; }; UserDataIngSession(fiveg_mag_reftools::CJson &json, bool as_request); diff --git a/src/mbsf/UserService.cc b/src/mbsf/UserService.cc index 0fe2a5b..d6f50a5 100644 --- a/src/mbsf/UserService.cc +++ b/src/mbsf/UserService.cc @@ -226,7 +226,16 @@ bool UserService::processEvent(Open5GSEvent &event) if (resource0 == "mbs-user-services") { std::string method(message.method()); const char *ptr_resource1 = message.resourceComponent(1); - if (method == OGS_SBI_HTTP_METHOD_POST) { + // BUG FIX: this POST branch used to match any "mbs-user-services" prefix regardless + // of what followed it, so a request actually meant for a sub-resource (e.g. a + // misrouted or malformed "/mbs-user-services/{id}/ingest-sessions") would be parsed + // as if it were a brand-new MBSUserService creation body instead -- confirmed live: + // that body is missing fields MBSUserService requires (e.g. extServiceIds), and + // checkAndSetUserServiceAnnouncementChannel() below constructs a raw MBSUserService + // from it with no try/catch, so the resulting ModelException was uncaught and took + // the whole process down. A real POST to create a user service has no resource1 at + // all (mbs-user-services is a collection endpoint); only match that case here. + if (method == OGS_SBI_HTTP_METHOD_POST && !ptr_resource1) { ogs_debug("POST response: status = %i", message.resStatus()); std::shared_ptr user_service; ogs_debug("Request body: %s", request.content()); @@ -249,11 +258,23 @@ bool UserService::processEvent(Open5GSEvent &event) return true; } - if(!checkAndSetUserServiceAnnouncementChannel(mbs_user_service, true)) { - static const char *err = "MBSF cannot handle User Service Announcement channel without local configuration."; - ogs_error("%s", err); + // BUG FIX: checkAndSetUserServiceAnnouncementChannel() constructs a raw MBSUserService + // straight from the request body (fiveg_mag_reftools::ModelException on any missing + // required field, e.g. extServiceIds) with no try/catch of its own -- confirmed live: + // an uncaught ModelException here calls std::terminate() and takes the whole MBSF + // process down, rather than the 400 Bad Request a malformed client body should get. + try { + if(!checkAndSetUserServiceAnnouncementChannel(mbs_user_service, true)) { + static const char *err = "MBSF cannot handle User Service Announcement channel without local configuration."; + ogs_error("%s", err); + ogs_assert(true == NfServer::sendError(stream, OGS_SBI_HTTP_STATUS_BAD_REQUEST, 1, message, + app_meta, api, "Bad MBSF User Service", err)); + return true; + } + } catch (const std::exception &ex) { + ogs_error("Malformed MBS User Service in request body: %s", ex.what()); ogs_assert(true == NfServer::sendError(stream, OGS_SBI_HTTP_STATUS_BAD_REQUEST, 1, message, - app_meta, api, "Bad MBSF User Service", err)); + app_meta, api, "Bad MBSF User Service", ex.what())); return true; } diff --git a/src/mbsf/UserServiceAnnChannel.cc b/src/mbsf/UserServiceAnnChannel.cc index 68248f8..54687e4 100644 --- a/src/mbsf/UserServiceAnnChannel.cc +++ b/src/mbsf/UserServiceAnnChannel.cc @@ -162,7 +162,19 @@ void UserServiceAnnChannel::workerLoop() m_announcementChannelRunning = true; #define CHECK_CANCEL_MS 100 + // BUG FIX (found live, 2026-08-11): sendMbstfRequests() used to be called exactly once + // (guarded by requested_mbstf_dist_session, which was never reset), and the wait below had + // no deadline of its own -- if MBSTF's response to that single request was ever lost + // (confirmed live via gdb: this worker thread stuck forever in the wait_for() below, with + // a real, established TCP connection to MBSTF sitting idle -- some transient SBI/SCP + // hiccup around the same moment, not a deadlock or a bug in the wait itself), the + // announcement channel's distribution session would never be created and NO Service + // Announcement content would ever be pushed, for the lifetime of the MBSF process, with no + // way to recover short of restarting it. Retry after a bounded number of wait iterations + // instead of waiting on the same request forever. +#define MBSTF_DIST_SESSION_RETRY_AFTER_ITERATIONS (5000 / CHECK_CANCEL_MS) /* 5 seconds */ bool requested_mbstf_dist_session = false; + unsigned mbstf_dist_session_wait_iterations = 0; std::lock_guard lock(*m_announcementChannelMutex); while (true) { @@ -181,9 +193,16 @@ void UserServiceAnnChannel::workerLoop() ogs_debug("Request creation of USAC MBSTF Dist Session"); m_userServiceAnnChannelDataIngSession->sendMbstfRequests(); requested_mbstf_dist_session = true; + mbstf_dist_session_wait_iterations = 0; } if (!m_userServiceAnnChannelDataIngSession->hasMbstfResponded(USER_SERVICE_ANN_CHANNEL)) { + if (++mbstf_dist_session_wait_iterations >= MBSTF_DIST_SESSION_RETRY_AFTER_ITERATIONS) { + ogs_warn("No response from MBSTF for USAC Dist Session after %u ms -- retrying", + mbstf_dist_session_wait_iterations * CHECK_CANCEL_MS); + requested_mbstf_dist_session = false; + continue; + } // dist session not present, wait for change ogs_debug("Wait for USAC MBSTF Dist Session"); m_announcementChannelChange.wait_for(*m_announcementChannelMutex, std::chrono::milliseconds(CHECK_CANCEL_MS)); diff --git a/src/mbsf/mbsf.yaml.in b/src/mbsf/mbsf.yaml.in index 53553b1..3a0ab21 100644 --- a/src/mbsf/mbsf.yaml.in +++ b/src/mbsf/mbsf.yaml.in @@ -31,6 +31,13 @@ mbsf: mbsUserServiceMaxAge: 60 mbsUserDataIngestSessionMaxAge: 60 + # NOTE: rt-mbs-function does not implement a real MBS-5 (TS 26.517 cl.9.2) discovery API or any + # MBS-4-MC bootstrap signalling, so a client has no protocol-level way to learn where the + # announcement channel actually is. If rt-mbs-client's mbsf_client.announcement_channel config + # (rt-mbs-client.conf) is used to bootstrap it automatically instead of a manual ManualActivate + # call, its multicast_address/port must match ssmDestinationAddress/ssmPort below, and its tsi + # must match the announcement channel's TSI (hardcoded to 1 in UserDataIngSession.cc -- see + # g_next_tsi there, which starts real content sessions at 2 to leave 1 free for this). userServiceAnnouncement: announcementRepetitionTime: 10000 ssmPort: 3000 diff --git a/subprojects/rt-common-shared b/subprojects/rt-common-shared index 1c6ffdf..25f848f 160000 --- a/subprojects/rt-common-shared +++ b/subprojects/rt-common-shared @@ -1 +1 @@ -Subproject commit 1c6ffdf20e6b10b932402d66e2214dd88f6bde43 +Subproject commit 25f848fdf07cf918756b685bc9bd881b1d50aecf