Fix MBS Broadcast content delivery crashes and Distribution Session bugs - #71
Draft
jordijoangimenez wants to merge 57 commits into
Draft
jordijoangimenez wants to merge 57 commits into
jordijoangimenez wants to merge 57 commits into
Conversation
jordijoangimenez
marked this pull request as draft
August 14, 2026 10:32
Collaborator
|
@jordijoangimenez wrote:
I don't agree with this solution as it prevents the use of jumbo frames between MB-UPF and gNodeB (we use jumbo frames in our lab configuration). This is why the function used the interface MTU as fallback. This allows this setting to be handled by configuring the interface MTUs correctly for your setup. |
Collaborator
|
@jordijoangimenez wrote:
Actually if you read TS 26.502 Table 6.1-1: "When the push-based object acquisition method is provisioned, the set of Object acquisition identifiers shall be empty." So if an objAcquisitionIdPush is provided in PUSH+SINGLE mode then that is an error. So the original return code was correct (i.e generate an error if it's set). |
Collaborator
|
@jordijoangimenez wrote:
I've looked through TS 29.581 Clause 5.8.8.2 and can find nowhere where it says the notifications have to be delivered reliably. It only states that the response from the MBSF should be 204 for success, 307 or 308 for redirection and one of the 4XX or 5XX error codes from TS 29.500 for errors. It says nothing about what action should be taken on reported failure, failure to connect or timeout. The 307 and 308 responses should cause the request to be reissued to a new URL. For 308 the new URL should replace the old notification URL. (But we never implemented redirection) The semantics of any 4XX error code are that the client made a mistake and should not repeat the same request, so a 4XX response should prevent the events from being sent again. For 5XX responses, the semantics are that there was a server error that may clear and so the request may be repeated again at some point in the future. The client should delay before trying again to avoid rapid retries. So there is an argument for repeating the notifications when a 5XX is received. This also risks indefinite attempts to resend, there probably should be a resend attempts limit. As far as I can see the current behaviour of trying once and then forgetting it does not contradict TS 29.581. |
@jordijoangimenez: Please note that this bug is fixed more comprehensively by @davidjwbbc on PR #72. |
@jordijoangimenez: This issue was also mopped up by @davidjwbbc in PR #72. |
Contributor
Author
|
Thank you Richard, as soon as possible I will clean up this branch to avoid duplications and we can cherry pick what's useful rather than adopting the full PR |
…nch needs Problem This branch's FLUTE-layer obligations depend on work that is on rt-libflute's own feature branch and not in any release tag: the TS 26.346 annex L.6 profiled FDT schema, the scheme-specific FEC OTI, suppression of Transfer-Length under the MBMS Download Profile, the split expiry setter, and the RFC 5053 Raptor scheme. The wrap pointed at a revision carrying none of it. [code-derived] Basis No clause governs a dependency pin. code-derived only. Raised by Building this branch against the dependency it actually needs. Change Advances subprojects/rt-libflute.wrap to the commit on 5G-MAG's own feature/raptor-raptorq-fec carrying that work, and the rt-common-shared submodule to its consolidated tip. The wrap comment records why a tag cannot be used yet and what has to happen before it can be. Verification T0: the subproject is fetched and the tree builds against it. The behaviour that depends on these revisions is verified by the commits that use it, not here. Not in this change Moving the rt-libflute pin to a tag, which needs a 5G-MAG release carrying the work the comment names. No source change.
Problem
The Nmbstf_DistSession surface answered a wrong Content-Type with 400 rather
than 415, never checked a client's Accept header, never bounded a request
body, and one PATCH handler parsed its body with no Content-Type check at
all. [code-derived]
Basis
TS 29.500 V18.10.0 table 5.2.7.1-1 marks 415 mandatory for POST and PATCH,
406 mandatory for GET, and 413 mandatory where a body is accepted. Its
table 5.2.7.2-1 defines no named cause for 415, so the numeric status is
constructed directly, as this file already does for 405 and 501.
TS 29.581 (TS29581_Nmbstf_DistSession.yaml) requires
application/json-patch+json on both PATCH operations, not
application/merge-patch+json.
The individual resource's representation is DistSession for both the GET and
the PATCH on /dist-sessions/{distSessionRef}; CreateReqData is the request
body of the collection POST only, so an RFC 6902 pointer addresses the
DistSession.
Raised by
reading the authority during this work, and observation of a live activation
Change
Adds NfServer::acceptsMediaType() and answers 406 when the client's Accept
header cannot take application/json; answers 415 for an unexpected
Content-Type on POST and PATCH; adds request_too_large() and a configurable
maxRequestBodySize, answering 413; checks the Content-Type on the
subscription PATCH. Applies a JSON Patch to the DistSession the stored
CreateReqData holds, rebuilding the CreateReqData around the result, so the
pointer a conformant peer sends resolves.
Verification
T2: the end-to-end demo activates a Distribution Session, which the previous
patch target rejected outright.
Not in this change
Authentication on this surface.
…piry time Problem DistributionSessionSubscription parsed expiryTime into m_expiryTime and never compared it against the clock anywhere, so a subscription carrying one kept receiving notifications until its Distribution Session was deleted or the process exited. [code-derived] Basis TS 29.581 V18.6.0, table 6.1.6.2.5-1, expiryTime row: "When present in the subscription creation request, it shall indicate the time up to which the subscription is desired to be kept active and after which the subscribed events shall stop generating notifications." Raised by reading the authority during this work Change Adds a per-subscription timer that removes the subscription when its expiryTime passes, scheduled wherever m_expiryTime is set and cancelled from the destructor. The callback does not remove the subscription directly: it runs from a timer that subscription owns, so it pushes LocalEvents::SUBSCRIPTION_EXPIRED carrying the two ids as plain strings and DistributionSession::processEvent() performs the removal off that call stack, the deferred dispatch SEND_NOTIFICATION already uses. A copy schedules its own timer, a callback being keyed to one object; a move takes the original's over. Timer-pool exhaustion is logged and leaves the subscription without a timer rather than crashing. Verification T2: the end-to-end demo runs with it in place, MBSTF logging no fatal or assertion lines and no timer-creation failure. No test scaffolding exists for this surface. Not in this change An expiryTime in a subscription response, which this MBSTF does not set.
…by earliest deadline Problem Four related defects in how object metadata is held and ordered. ObjectStore::Metadata hand-writes its copy constructor, move constructor and both assignment operators, and three members were missing from them. The availability window (m_availabilityStartTime, m_availabilityEndTime) was absent from all four paths, so any copy taken out of the store lost it: build/tests/testObjectStore aborted with std::bad_optional_access. [observed: test output] The entity tag (m_entityTag) was absent from all four as well, which is quieter and not caught by any test: PullObjectIngester passes it to the conditional re-fetch, so losing it turns an If-None-Match into an unconditional GET and re-downloads an unchanged segment, and ObjectCarouselPackager copies it into the FLUTE file description, which then carries an empty ETag. [code-derived] PullObjectIngester::IngestItem has the same shape of defect in one direction only: its copy constructor carries the availability window and its move constructor did not. Items are moved on the ordinary queueing paths, so an item reaching the fetch queue by any of those routes lost what the copy path preserved. [code-derived] Nothing ordered the packaging queue by when an object has to arrive. [code-derived] Basis TS 26.517 V18.6.0, clause 6.2.3.5: "The MBSTF shall transmit each object in the object list such that the last packet of the delivered FLUTE transmission object (including any FEC recovery packets, when configured) is available at the MBSTF Client no later than its availability start time." The deadline a PackageItem carries is that availability start time, so ordering the queue by it is what implements the clause. The copy and move defects are code-derived: no clause governs a class's own copy semantics, only that a value the class stores survives being copied. Raised by The availability window by running the test suite, which aborted. The entity tag and the ingest item's move constructor by reading the same four paths afterwards, looking for the same mistake again. Both times it was there. Change Adds the missing members to all four ObjectStore::Metadata copy and move paths, in declaration order so initialisation order matches and -Wreorder stays quiet, and to IngestItem's move constructor. Adds earlierDeadlineFirst(), a named predicate rather than an inline comparator so the ordering can be tested directly, the queue itself being private and fed only through a live packager. An item with no deadline sorts after every item that has one: nothing is known about when it must arrive, so it cannot displace an object that does have a stated time. Verification T1: four suites executed and passing -- testObjectStore 16 cases, testPullObjectIngester 19, testObjectListPackager 7, testSubscriberSubscription. Both metadata defects were confirmed to be caught, not merely covered: testObjectStore aborts without the availability fix, and reports "copy=[] move=[] copy-assign=[] move-assign=[]", 15 pass 1 fail, without the entity tag fix. Not in this change IngestItem's own copy and move constructors have no direct test, which is the gap that let the move-constructor defect survive. ObjectStore::Metadata::operator== still does not compare m_entityTag; the equality semantics of the class are not touched here.
…ransmitter Problem The AL-FEC configuration a Distribution Session provisions reached no transmitter, so every session was sent unprotected however it was provisioned. FecOtiHelper, which converts the session's FECConfig into the Transmitter-level FEC OTI, was present in the tree but never listed in src/mbstf/meson.build, so it was never compiled. Neither ObjectListPackager nor ObjectCarouselPackager passed a FEC OTI or a redundancy level to LibFlute::Transmitter, which has taken both since the pinned revision, and no controller supplied a FECConfig to either packager. [code-derived] Basis TS 26.517 V18.6.0, clause 6.2.1: "If FLUTE [12] is used to realise the Object Distribution Method, the MBS Distribution Session shall conform to the MBMS Download Profile as defined in clause L.4 of TS 26.346 [7] with the additional requirements in clause 6.2 of the present document." TS 26.346 V18.2.0, clause L.4.7 "Other aspects of FLUTE delivery": "Regarding Application Layer FEC support, the two FEC schemes referenced in this specification, the Compact No-Code FEC scheme as specified in RFC 3695 [13], and the Raptor FEC scheme as specified in RFC 5053 [91] are optional to implement by the BM-SC and mandatory to support by the UE." Those two together decide which schemes may be sent: the profile clause 6.2.1 selects admits Compact No-Code and Raptor, and no other. RaptorQ (RFC 6330) is not among them. Raised by Reading the branch while checking the review comments on pull request #71. Change Adds FecOtiHelper to the build. ObjectListPackager and ObjectCarouselPackager convert the session's FECConfig with fecOtiFromFecConfig() and pass the resulting FEC OTI and redundancy level to LibFlute::Transmitter. A scheme the profile does not admit throws and is reported as a packaging failure for that session rather than silently downgraded: a session sent unprotected when it asked for protection is a worse outcome than a visible failure. ObjectCarouselPackager gains the FEC parameter ObjectListPackager now also carries, defaulted so no other call site changes, and all controllers supply distributionSession().getFecInformation(). Verification T1: tests/test_FecOtiHelper.cc, 8 cases, all passing, covering absent configuration, a null shared pointer, Compact No-Code yielding no FEC OTI, Raptor yielding a Raptor OTI carrying the requested overhead, and refusal of RaptorQ, an unknown scheme and a negative overhead. The suite is new: FecOtiHelper had never been compiled, let alone tested, and one case caught a real error while being written, an incorrect Compact No-Code URN. All five suites pass, 50 cases. T2: the demo delivers unchanged with this build. That run does NOT exercise the populated path: the demo provisions no FEC, so it evidences no regression, not that FEC transmission works. Not in this change No end-to-end evidence that a FEC-protected session is transmitted and decoded. Nothing here verifies rt-libflute's own Raptor encoding, and no receiver in this project has been shown to decode a protected session. The conformance record must not claim AL-FEC works on the strength of this commit.
… DASH handler its own manifest URL Problem ObjectCollectionController.cc/.hh, 368 lines serving the COLLECTION object distribution operating mode, were present in the tree but never listed in src/mbstf/meson.build, so they were never compiled. The class self-registers with ControllerFactory at file scope, so leaving it out of the build means the registration never runs and a Distribution Session provisioned with objDistributionOperatingMode COLLECTION finds no controller at all. Once added to the build it did not compile: four errors, all from ObjectController::objectStore() having become std::shared_ptr<ObjectStore> rather than a reference. The same file listed ObjectController.cc twice where the second entry should have been its header, so the header was not tracked as a dependency. [code-derived] Separately, DASHManifestHandler::nextIngestItems() declared manifest_url and never assigned it, so both comparisons against it tested the empty string and m_refreshMpd was never set: a re-fetched MPD was ingested without the handler being told its own manifest had changed. [code-derived] Basis COLLECTION is one of the four values ObjDistributionOperatingMode defines (src/mbstf/openapi/model/ObjDistributionOperatingMode.h: SINGLE, COLLECTION, CAROUSEL, STREAMING), so the API accepts a session the build then has no controller for. That is the operative basis and it is code-derived. TS 26.502 V18.6.0, clause 4.5.10, first paragraph: "An object manifest describes a set of objects to be distributed in an MBS Distribution Session that is provisioned in OBJECT_COLLECTION or OBJECT_CAROUSEL operating mode." confirming the mode is one the specification defines rather than a local invention. The fuller description of the mode is in annex B, clause B.2.1, which is informative. Raised by Reading the branch against its own build file while checking the review comments on pull request #71. Rule 14: purpose established before any change, and the code is built rather than removed, because the operating mode it serves is one the model defines. Change Adds ObjectCollectionController to src/mbstf/meson.build and corrects the duplicated ObjectController.cc entry to name the header. Dereferences the object-store shared pointer at the four sites that still treated it as a reference, matching what ObjectCarouselController already does, and checks each dynamic_pointer_cast result before use rather than letting a std::runtime_error leave the controller, where the surrounding try catches only std::out_of_range and one bad ingest response would end the process and every other Distribution Session with it. Initialises manifest_url from the manifest's own fetched URL, the value addMPDRefreshToExtraPullObjects() keys its refresh entry on, and makes it const. Verification T0 for the new code: both files compile with no new warnings and COLLECTION is present in the linked binary, so the controller registers. T1: all five suites pass, 50 cases. T2: the demo delivers with this build, gNB carrying MRB1 and MRB2 and the UE logging CRC-OK broadcast decodes and MCCH receptions. Not in this change Any verification of COLLECTION behaviour. This builds and registers the controller; it does not establish that the mode works, and the conformance record must not claim it. The demo uses STREAMING and no test covers COLLECTION. Nothing here covers the MPD-refresh recognition path either.
…roject cannot follow
Problem
Comments across this branch narrated how the code came to be rather than what it does, and
pointed at material a reviewer cannot read. Three shapes.
A "BUG FIX:" prefix followed by what the code used to do ("this used to be static", "no DELETE
branch existed at all", "the else branch used to unconditionally call gw->write_pdu_mch()"). That
belongs in a commit message; in a comment it dates immediately and tells a reader nothing about
the code in front of them.
References to this project's own internal process framework by number: "(rule 12)", "(S12)",
"see rule 14". Those numbers name nothing in this repository.
Pointers into a separate, private repository: "see Standards2Deployments/projects/rt-mbs/...",
"see the findings register", "the register's own item 4 finding". A reviewer cannot open any of
them. [code-derived]
Basis
No clause governs a comment.
code-derived only.
Raised by
Reading the branch as a reviewer outside this project would.
Change
Comments now state the behaviour, the requirement or the invariant, keeping every specification
citation and every stated reason. Where a comment's only content was its history, the underlying
rule it was protecting is stated instead: "X was never freed" becomes "this context owns X and
must free it on removal". Internal rule numbers are replaced by the reasoning they stood for,
usually that no clause and no configured value fixes a given bound. Private-repository pointers
are removed, with the substance they referred to summarised in place where it was load-bearing.
Deliberately NOT changed: the numbered rules in src/mbsf/MultipartMime.cc's Q-encoding comment.
Those are RFC 2047 section 4.2's own rules 1, 2 and 3, not this project's, and an automated pass
over the phrase "rule N" would have silently corrupted a correct citation.
Verification
T0 for the shape of the change: every hunk is a comment or documentation line, confirmed by
filtering the diff for added lines that are not comments, which comes to zero in all nine
repositories. T1 where a build exists: open5gs, rt-mbs-function and rt-mbs-transport-function
all build clean afterwards, and the shell scripts and JSON touched pass bash -n and json.load.
Not in this change
No behaviour, in any repository. No commit message is rewritten; the history those comments
described stays in the log, which is where it belongs.
…the configured failure limit Problem ObjectManifestController refetched an object on every ingest failure, for any non-PUSH session, with no bound of any kind: no deadline check, no attempt count, no backoff. One object that could never be fetched was therefore retried forever. Observed on a live run: 59554 ingest failures in a single session, none of which stopped anything. [observed: run/logs/mbstf.log] The session-wide limit that exists could not catch it. ObjectController counts consecutive failures and deactivates the session at consecutiveIngestFailuresBeforeDeactivate, but the count is per Distribution Session and is reset by ObjectStore's ObjectAdded/ObjectUpdated events. The same run recorded 4985 successful object fetches, so the counter was reset continually and never reached 5 while one object failed 59554 times. A bound that a healthy session resets cannot bound a single unfetchable object. [code-derived: ObjectController.cc, the reset at the ObjectAdded/ObjectUpdated branch] Basis TS 26.517 V18.6.0, clause 6.1.2, object manifest parameter latestFetchTime: "The MBSTF shall fetch the object no later than this UTC timestamp." So an object whose latest fetch time has passed must not be fetched again, whatever has happened before. The same parameter's description governs the other case: when latestFetchTime is absent "the object shall be present at its origin ... and the MBSTF may fetch it at a time of its choosing", which sets no bound at all. A limit there therefore rests on a configuration option the operator sets, not on a clause. Raised by Reading the authority for what governs a failed object fetch, after a defensive guard written earlier was measured and found not to bound the loop (it rejected the fetch, which produced the failure event, which triggered the refetch). Change The refetch decision now refuses two cases. An object whose latestFetchTime has passed is not refetched, which is the clause above. An object with no latestFetchTime is refetched until consecutiveIngestFailuresBeforeDeactivate consecutive failures of that object, the operator's own existing option applied per object rather than per session. PullObjectIngester::IngestItem gains the per-object counter, carried through all four of its copy and move paths, since items are copied and moved on every queueing path. Verification T1: tests/test_PullObjectIngester.cc, 27 cases passing, 5 of them new: the counter starts at zero, accumulates, and survives copy and move; a deadline in the past, a deadline in the future and no deadline are each distinguished. Confirmed discriminating, not vacuous: with the copy and move carry deliberately reverted the two survival cases fail and the suite reports 25 pass, 2 fail. All five MBSTF suites pass, 58 cases. T2: live demo. Ingest failures fell from 59554 to 15, and the log shows three refusals, each "5 consecutive fetch failures reached the configured consecutiveIngestFailuresBeforeDeactivate limit of 5": three objects, five attempts each, then stopped. Delivery is unaffected, the UE logging 15711 CRC-OK broadcast decodes on the same run. Not in this change Why the ingest URL is corrupted in the first place. A one-byte corruption at index 0 of IngestItem's own copy of the fetched URL is still unexplained and still open; this bounds the retry loop that made it harmful, it does not fix it. The refusal log line shows an empty object id for the affected items, which is consistent with that corruption and is not diagnosed here. No backoff is introduced: the clause names no interval and no configuration option supplies one.
…a released reference
Problem
ObjectStore::getMetadata() takes the store mutex, returns a reference into the store, and releases
the mutex as it returns. Everything the caller then reads through that reference is unsynchronised.
Metadata holds std::strings and ObjectStore::updateMetadata() move-assigns them, so a caller copying
a string while its data pointer and length are being reassigned builds a string from two different
states of the same object.
This is the cause of a corruption that had been open and unexplained: an ingest URL arriving 55 bytes
long with byte 0 zeroed and bytes 1 onward intact, and an object id arriving empty. The store's own
copy was correct at that moment, which is what made it look impossible.
Confirmed by ThreadSanitizer, both sides named, holding different mutexes:
Write of size 8 by thread T4 (mutexes: write M0)
std::string::_M_data(char*)
ObjectStore::Metadata::operator=(Metadata&&)
ObjectStore::updateMetadata(...)
PullObjectIngester::doObjectIngest()
Previous read of size 8 by thread T6 (mutexes: write M1, write M2)
std::string::_M_data() const
std::string::basic_string(const std::string&)
PullObjectIngester::IngestItem::IngestItem(ObjectStore::Metadata const&, ...)
PullObjectIngester::fetch(...)
ObjectManifestController::workerLoop(...)
The same call site also chained keepAfterSend() and compressedSend() onto that reference, mutating
the live store entry with no lock held. [observed, then code-derived]
Basis
No clause governs a component's internal locking.
code-derived and observed only.
Raised by
Running the component under ThreadSanitizer, after three earlier passes narrowed the corruption to
the interval between the copy and the end of IngestItem's constructor without identifying any
writing instruction. Reading the code had already shown the reference outlives the lock; the
sanitiser is what turned that from a hypothesis into the cause.
Change
Adds ObjectStore::takeMetadataForIngest(), which takes the lock, applies the two marks and returns
the metadata by value before releasing it, so the copy the ingest list stores is made while the
entry cannot be mutated. PullObjectIngester::fetch() uses it instead of copying through
getMetadata()'s reference.
Verification
T2: live demo, MBSTF built with -Db_sanitize=thread, same scenario before and after.
Before: 162 data race reports, 12 of them naming IngestItem's constructor.
After: 64 data race reports, ZERO naming IngestItem's constructor.
Delivery unaffected on the same run: gNB carrying MRB1 and MRB2, 2036 CRC-OK broadcast decodes and
118 MCCH receptions at the UE, and no ingest failures at all.
T1: all five MBSTF suites pass, 58 cases.
Not in this change
The other 64 races ThreadSanitizer still reports. They are real and are recorded, but each needs its
own diagnosis and none is this defect. getMetadata() itself is left in place: its remaining callers
read under conditions this commit has not examined, and changing its contract is a wider refactor
than the defect requires.
…cument
Problem
Running tools/verify-citations.py over this branch's own changed files reported
citations it could not confirm. Each was a comment whose quoted sentence differed
from the document it named. [code-derived]
Basis
No clause governs how a comment is written. The defect is that a quotation did not
match its source, which is checkable without any specification claim.
code-derived only.
Raised by
Running the citation checker across this branch for the first time. [rule 13]
Change
Quotations now reproduce contiguous source text. Where an earlier comment joined
two sentences with an elision, each is quoted separately. Where it inserted an
editorial gloss inside the quotation marks, the gloss moved outside them. Where a
specification writes a value inside its own quotation marks (a status code, a state
name, a file extension), that fragment is named without quotation marks rather than
nested, and the comment says why, so nobody restores them.
Two citations that sat inside runtime strings moved into comments beside them: a
full document identifier in a log or exception message is read as a citation by the
checker, which then matches the next string literal in the file.
No behaviour changes. Comments only.
Verification
T1: tools/verify-citations.py reports no unconfirmed citation across this branch's
changed files.
T0: builds clean.
Not in this change
Citations naming a document the local corpus does not hold. Those remain unchecked
and are listed in the project's own specification index.
jordijoangimenez
force-pushed
the
feature/mbs-compliance-fixes
branch
from
September 7, 2026 06:08
84d878a to
88e9fa0
Compare
…aves the host Problem FLUTE encoding symbols are sized from getsockopt(IP_MTU) on a socket to the distribution session's ingress. Where the MBSTF and the ingress point are co-located that destination is one of the host's own addresses, so the kernel routes it over loopback and answers 65535. Symbols were sized at 65441 bytes and every object larger than one of them left as a datagram nothing downstream carries. Measured on the rt-mbs-examples broadcast demo, OBJECT_STREAMING session: the FDT advertised FEC-OTI-Encoding-Symbol-Length="65441"; the client was told about 663 distinct TOIs and received data packets for three of them, TOI 0 (the FDT itself), 2 and 4. Every media segment TOI got its FDT Instance and then no data. [observed, code-derived] Basis RFC 5651 section 6.1: "However, network efficiency considerations recommend that the sender uses an as large as possible packet payload size, but in such a way that packets do not exceed the network's maximum transmission unit size (MTU), or when fragmentation coupled with packet loss might introduce severe inefficiency in the transmission." RFC 5651 is the LCT building block TS 26.346 V18.2.0 lists as reference [119]. No clause fixes a number. mbstf.pathMtu is the operator's setting and is documented in mbstf.yaml with a default, which is what rule 12 requires of a bound. Raised by Counting, per TOI, the data packets a client received against the TOIs its FDTs advertised, while chasing why no media segment reached a player. The shape of the fix comes from review by David Waring on pull request #71, who pointed out that clamping a discovered MTU would prevent the jumbo frames his lab runs between the MB-UPF and the gNodeB. [rule 13] Change A discovered MTU is used as it stands whenever the destination is not one of this host's own addresses. A deployment configuring jumbo frames on its interfaces gets them; nothing here caps what the operator set up. get_path_mtu() reports, through via_loopback, whether the destination is a local address. It tests the destination against getifaddrs() rather than for the 127/8 prefix, because a co-located MB-UPF is commonly reached on the address of a real interface -- in the demo, a veth -- which loops back without looking like a loopback address. Only in that case, where there is no path to measure, is Context::pathMtu used, and the substitution is logged with both numbers. The four controllers sequence the discovery before reading the flag rather than nesting the calls: the order function arguments are evaluated in is unspecified. Verification T1: meson test, the five rt-mbs-transport-function suites pass. The other failures in that run are open5gs and libmpdpp subproject tests, untouched by this change. T2: run scripts/mbs-broadcast-demo/start-all.sh in rt-mbs-examples. mbstf.log records "The route to this session's ingress is loopback, so its 65535 byte MTU is not the path to a receiver; sizing FLUTE symbols for the configured 1500 byte path MTU instead", the FDT then advertises FEC-OTI-Encoding-Symbol-Length="1406", and the client receives the announcement bundle, both initialisation segments, the manifest and media segments. Before this change it received data for three TOIs out of 663. Not in this change GTP_HEADER_SIZE in common.hh, which is 2. Whether that is the right allowance needs TS 29.281, which is not held; halted under rule 2.
Problem
In OBJECT_STREAMING every media segment is transmitted several times, each time under a
different TOI. Counting distinct TOIs against distinct Content-Locations in the FDTs a
receiver was given: 4.0 TOIs per object on average, and up to 15.
15 x chunk-stream0-00047.m4s 11 x manifest.mpd 10 x chunk-stream0-00051.m4s
This is not redundancy. A receiver cannot combine symbols across TOIs, so each copy is a
separate object that is independently incomplete, and the copies consume the bearer the
first copy needed. Measured live on the rt-mbs-examples broadcast demo, the effect is that
no media segment ever completes at the client: symbol IDs arrive with gaps throughout, for
example SBN 1 ID 7, 11, 12, 16, 19, 20, 22, 26, 27, 30, 32, 33, 42, while the radio itself
delivered 33382 of 33455 grants with zero CRC failures. [observed]
DASHManifestHandler::nextIngestItems() takes every segment the MPD currently advertises and
clamps any whose availability start has passed to the present, so a segment stays in the
candidate set for the whole of its availability window. Nothing records that it has already
been sent. The ObjectStore cannot answer for it either: ObjectStreamingController leaves
Metadata::keepAfterSend() at false, so ObjectController deletes the object as soon as it is
sent, findMetadataByURL() then misses, and a second object is created for the same URL,
which the packager sends under a second TOI. [code-derived]
Basis
RFC 3926 clause 3.1: "Note that each object is associated with a unique TOI within the scope
of a session."
Sending one file under several TOIs therefore presents it as several objects, and a receiver
has no basis on which to combine their symbols. An MPD advertising a segment states that a
client may still fetch it, not that it still needs transmitting; no clause requires a segment
to be sent more than once.
Raised by
Counting TOIs per Content-Location while establishing why no object completed at a
receiver, after the path-MTU defect was fixed and the segments started arriving. [rule 13]
Change
DASHManifestHandler remembers the media segment URLs it has already handed to the ingester
and skips them on later passes. The set is pruned against the current manifest on every
pass, so it holds at most one entry per segment the MPD still advertises.
Only segments the MPD itself advertises are suppressed. The MPD refresh and the
initialisation segments, which come from m_extraPullObjects, are meant to repeat and are
deliberately not recorded.
keepAfterSend() is untouched. Removing it from ObjectManifestController was correct: an
OBJECT_STREAMING object genuinely should not be retained once sent, and the carousel sets
it for itself. What was missing is a record of what has been sent, which is added here
rather than by retaining objects that are no longer needed.
Verification
T0: builds clean.
Not in this change
Anything in the carousel path, which retains its objects and repeats them under one TOI.
The gNB-side per-slot MBS scheduling limits, which are a separate matter in
srsRAN_Project_mbs.
jordijoangimenez
force-pushed
the
feature/mbs-compliance-fixes
branch
from
September 7, 2026 08:11
88e9fa0 to
2958c1e
Compare
Contributor
Author
|
@davidjwbbc You were right, and thank you — capping a discovered MTU would break your jumbo-frame lab, and that solution is off the branch. What replaced it keeps your mechanism. A discovered MTU is now used exactly as it stands whenever the destination is not one of the host's own addresses, so interface MTUs remain the way this is configured and jumbo frames between MB-UPF and gNodeB are passed through untouched. The one case it treats differently is a destination that is a local address, where the kernel routes over loopback and answers 65535. There is no path to measure there, so Why it matters: with the loopback answer, symbols were sized at 65441 bytes and a receiver was told about 663 TOIs but got data packets for three of them. RFC 5651 §6.1 asks that the sender not exceed the path MTU, which a loopback figure is not. If your lab has the MB-UPF on the same host as the MBSTF, I would like to know — that is the one configuration where this now diverges from pure discovery. |
Contributor
Author
|
@davidjwbbc You are right on both, and both changes are off the branch.
StatusNotify retry. You are right that I overstated TS 29.581 — I cited it as obliging reliable delivery and it does not say that. The retry change is off the branch, and the current try-once behaviour stands. Your reading of the status codes is the useful part and I have not tried to implement it here: 4xx means the client should not repeat the same request, 5xx may be retried after a delay, and 307/308 should be reissued to the new URL, with 308 replacing the stored one. That plus a retry limit is a real piece of work and belongs in its own change, raised against a maintainer's judgement rather than smuggled in behind a wrong citation. |
…view asked Problem The previous commit put a 5xx-rejected notification's events back but offered them again only when the subscription next sent a notification. The review asked for a delayed retry, and that difference matters: a consumer that answered 5xx because it is overloaded or restarting is helped by a wait, and events that arrive only when the next one happens to occur may wait far longer than intended or not be re-offered at all. [code-derived] Basis RFC 9110 section 15: "5xx (Server Error): The server failed to fulfill an apparently valid request" Apparently valid, so the same notification may succeed later; the wait is what gives the server time to recover. No clause gives a figure for it, so the delay is an explicit default the operator can override (rule 12). Raised by Review by David Waring on #71: "there is an argument for repeating the notifications when a 5XX is received", with a delay and an attempt limit. The limit landed in the previous commit; this is the delay, which was the part of his reading still missing. Change A 5xx within budget now starts a per-subscription timer for mbstf.notifyRetryDelay seconds, defaulting to 5. NotificationRetryTimerFunc pushes LocalEvents::NOTIFICATION_RETRY rather than sending directly, and DistributionSession::processEvent() does the send, so it happens off the timer's call stack and the timer is not executing while the subscription that owns it is in use. That is the same deferred dispatch SubscriptionExpiryTimerFunc uses, and for the same reason its comment gives. The timer holds the distribution session and subscription identifiers by value, so it depends on neither object's lifetime; a retry whose subscription has gone finds nothing and logs at debug rather than failing. Setting the delay to zero keeps the previous behaviour of re-offering on the next notification. Verification T1: testObjectStore 23 cases, testObjectListPackager 16, testPullObjectIngester 27 and testSubscriberSubscription all pass; the repository builds clean with no errors. None covers this path, which needs a consumer answering 5xx, so the tier records that the build and existing tests are sound rather than that a retry fired. Not in this change Backing the delay off between attempts. Each retry waits the same interval, which the attempt budget bounds; a growing interval would need a second bound and no clause asks for one.
…rt by a path segment Problem Every PushObjectIngester started its own libmicrohttpd daemon on an ephemeral port, and the mbstf.httpPushIngest configuration that already populates servers[SERVER_OBJECT_PUSH] was never read by it. A container cannot publish a port whose number is only known once the process has bound it, so push ingest could not be reached from outside one. [code-derived] Basis No clause governs this: which port a deployment publishes is a deployment matter, and the address is the operator's to set through configuration that already existed (RULES.md rule 12). code-derived only. Raised by #27, which also proposes the shape used here: a main ingester owning the fixed port, attaching children told apart by generated path discriminators. Change With mbstf.httpPushIngest configured, one daemon is bound to it for the whole process and every ingester is reached through it. Each registers under a fresh UUID, its ingest prefix becomes http://<addr>:<port>/<uuid>/, and the shared handler routes on the leading segment, passing the remainder on as the object path so a child sees what it would have seen on a port of its own. An unrecognised segment is answered 404. The daemon is bound with the first ingester and stopped with the last. Unconfigured, nothing changes: each ingester keeps its own ephemeral daemon. A configured address that cannot be bound falls back to the same, so a bad configuration degrades rather than disabling ingest. Only the first configured address is used, and a warning says so rather than the rest being silently dropped. PushObjectIngester::generateUUID() was declared and never defined, so nothing could have called it; the definition added here matches the other four in this component rather than introducing a second scheme. Verification T1: testObjectStore 23 cases, testObjectListPackager 16, testPullObjectIngester 27 and testSubscriberSubscription all pass; the repository builds and links clean. None covers this path, which needs an Application Provider pushing to a configured address, so the tier records that the build and existing tests are sound rather than that a push was routed. Not in this change Binding more than one configured address, which would need a daemon each and is not what the option is for; and TLS on the shared daemon, which the per-ingester daemon did not offer either.
Problem The routing that lets one published port serve every ingest session arrived with no test. Getting it wrong sends a push to the wrong ingest session or refuses a valid one, and neither shows up until an Application Provider is pushing at it. [code-derived] Basis No clause governs the routing; it exists so a container can publish a port it knows in advance. code-derived only. Raised by Reading back the previous commit: the path handling had the most edge cases of anything in it and the least coverage, and the test needed the rule separated from the lookup to be reachable at all. Change splitSharedPath() is split out of routeSharedRequest(), which now does the lookup alone. The split takes a path and yields the discriminator and the object path, so it can be exercised without a registered ingester, which would need an ObjectStore and an ObjectController to exist. tests/test_PushObjectIngester covers 11 cases: a discriminator with an object name, a nested object path, a bare discriminator and a trailing slash both addressing the session root, a real generated UUID, a query string staying with the object path, and the five shapes that must route to nothing rather than be guessed at, including a null path and one with no leading slash. Verification T1: tests/test_PushObjectIngester, 11 cases, passing. Every other suite in this repository still passes: ObjectStore 23, ObjectListPackager 16, PullObjectIngester 27, FecOtiHelper 8, SubscriberSubscription. Not in this change Exercising the shared daemon itself: binding it, registering two ingesters and pushing to each needs a live Application Provider, and the routing above is the part that decides where such a push lands.
Contributor
Author
|
On the two bounds: I have set them as configuration with defaults rather than asking you to choose, since neither TS 29.581 nor TS 29.580 defines a retry IE, so there is nothing to carry them on the API in any case. Happy to change the defaults if 5 and 5s do not suit your lab. Nothing outstanding for you on this PR now, as far as I can tell, though the retry path has no test coverage: it needs a consumer answering 5xx. |
Problem sharedPortConfigured() treated any configured mbstf.httpPushIngest address as a request for shared-port mode, including "port: 0". Port zero asks for an ephemeral port, which is what the per-ingester daemons already provide, and it is what the shipped demo configuration sets. Every such deployment would have moved onto one shared daemon and still had no port it could publish, which is the only reason the option exists. [code-derived] Basis No clause governs this. #27 asks for the mode to engage when the configuration "indicates a single fixed port", which port zero does not. code-derived only. Raised by Reading the demo configuration in rt-mbs-examples while preparing to exercise the shared daemon: it sets httpPushIngest with port 0, so the previous commit would have changed that deployment's behaviour without giving it the benefit. Change Shared-port mode now requires a non-zero port on the first configured address. Port zero, or no configuration at all, keeps a daemon per ingester exactly as before. sockaddrPort() reads the port from either address family. Verification T1: tests/test_PushObjectIngester 11 cases, and ObjectStore 23, ObjectListPackager 16, PullObjectIngester 27, FecOtiHelper 8 all still pass. The condition itself needs a configured MBSTF to observe and is not covered. Not in this change Rejecting a configuration that sets several addresses, which is still warned about and takes the first.
Problem Media delivery stops for good about 45 seconds into a broadcast session while the session still looks alive: the service announcement carousel keeps transmitting, the distribution session stays active, and nothing is logged as an error. The scheduled pull worker and the ingest threads are deadlocked on each other. Observed: two threads of a live open5gs-mbstfd, each holding the lock the other waits for, with the futex words matching crosswise. One holds the controller mutex taken at src/mbstf/ObjectManifestController.cc:432 and waits for the store mutex at src/mbstf/ObjectStore.cc:377, in findMetadataByURL, reached from DASHManifestHandler::nextIngestItems(). The other holds that store mutex, taken at src/mbstf/ObjectStore.cc:229 in updateMetadata(), and waits for the controller mutex in ObjectManifestController::manifestHandler(), reached through SubscriptionService::sendEventSynchronous(). [observed] Two lock orders existed at once. A scheduled pull worker holds m_manifestHandlerMutex while it asks the manifest handler for the next ingest items, and the handler reads this store: handler lock, then store lock. updateMetadata() and addObject() dispatched their event while still holding the store lock, and sendEventSynchronous() runs each subscriber's processEvent() inline, where ObjectManifestController takes the handler lock: store lock, then handler lock. [code-derived] The inversion is reachable on this branch and not before it. findMetadataByURL() was the one accessor in this file that took no lock: it searched the map unguarded and returned a pointer into it, so the worker never contended for the store mutex. The earlier commit here that gave it the lock and made it return a copy is what closed the cycle. That commit is not the defect and must not be reverted to avoid this one; the lock order is. Basis No clause governs this: it is a lock ordering error inside one component. code-derived and observed only. Raised by Observation. Media stopped reaching the MBS client while the announcement carousel continued; thread backtraces of the live process located the cycle. Change addObject() and updateMetadata() scope the store lock to the store mutation and dispatch the event after releasing it. The store owns the defect: holding its own mutex across a synchronous callback into subscriber code makes every subscriber's locks part of the store's lock order, so any subscriber that takes a lock can deadlock against any store read. Releasing before the callback removes that class of failure rather than this one pair of locks. Nothing in the dispatch needs the store locked: the event carries only the object id, and subscribers reach the object through this class, which locks. deleteObject() also dispatches under the lock and is left as it is. It uses the asynchronous path, which only queues, and its callers already hold the store lock, so scoping it here would release nothing. Verification T1: testObjectStore, 25 cases, passes, including a new case for this. The reproduction forces the interleaving rather than racing for it: a reader takes a subscriber's lock and holds it, the writer then enters the store, and only once the writer is inside does the reader reach for the store. Against the unfixed store that deadlocks every time and the case reports "reader stuck, writer stuck, 0 events delivered"; with the change both threads finish and the event is still delivered. A first version that had both threads loop and hope to collide passed against the unfixed store, so the handoff is what makes it a test. The rt-mbs-transport-function suite is 6 of 6 binaries, 87 cases. A full meson test run also reports ten failures in vendored subprojects, nine open5gs 5GC and EPC integration tests that need a running core and libmpdpp:segment_templates. This change touches neither subproject. Not verified over the air: the rig was brought down before a live run, so whether media continues past 45 seconds in the demo is untested. Not in this change The other half of the inversion, ObjectManifestController holding m_manifestHandlerMutex across a call into the handler that re-enters the store. Fixing the store breaks the cycle; whether the controller should hold its lock across that call is a separate question. deleteObject()'s dispatch under the lock, as above. That main() in these test programs returns 0 even when it has counted failures, so a failing case does not fail the run. The new case calls _Exit(1) on deadlock for that reason.
Problem The NRF status notification callback answered a method other than POST with 405 and no Allow header. Every other 405 in this component carries one: NfServer::sendError grew an allow_methods argument and the Distribution Session resources pass it. This site did not, because it calls Open5GSSBIServer::sendError, which hands the response to ogs_sbi_server_send_error() and so cannot add a header to it. Observed: src/mbstf/MBSTFEventHandler.cc, the nf-status-notify branch. [code-derived] Basis TS 29.500 V18.10.0 clause 5.2.7.2: “If the NF supports the HTTP method for several resources in the API, but not for the target resource of a given HTTP request, the NF shall reject the request with the HTTP status code "405 Method Not Allowed" and shall include in the response an Allow header field containing the supported method(s) for that resource.” Raised by Reading the component's own 405 responses against the clause already quoted at NfServer.cc, after #75 reported the header missing. Change The branch answers through NfServer::sendError with allow_methods set to POST, which is the only method this callback serves. Nothing else about the response changes: the status code and the ProblemDetails title and detail are the same. Zero resource components are echoed into the ProblemDetails instance URI, which is the whole of this path: nf-status-notify takes no identifier. Verification T1: builds clean and the rt-mbs-transport-function suite passes, 6 of 6 binaries. No test covers this path; reaching it needs an NRF sending a non-POST to the callback, and the suite has no scaffolding for an SBI request. Not in this change The same site in rt-mbs-function, which has the identical defect and is fixed in that repository.
…assuming octet-stream Problem An object pushed with no Content-Type header was stored as "application/octet-stream" and announced in the FDT as that, silently. Observed: src/mbstf/PushObjectIngester.cc, m_contentType.value_or(app_octet) in Request::processRequest(). [code-derived] The pull path already refuses such an object, on the same ground and with the same citations. Only the push half was left asserting a type nobody established. Basis TS 26.517 V18.6.0 clause 6.2.1 binds the MBSTF to the MBMS Download Profile, and TS 26.346 V18.2.0 clause L.4.2 lists Content-Type first among the attributes that "shall be carried in the FDT sent by the FLUTE sender". So the FDT must carry a media type, and octet-stream is a media type, which is why this was not visibly broken: it satisfied the schema while describing the object wrongly. Raised by Review by David Waring on #74, answering a question of mine: "If inference fails then the ingest (either push or pull) should be considered a failure." Also his: inventing octet-stream silently is the option to avoid. Change The media type comes from the pushed header where there is one, otherwise from inferMediaTypeFromUrl() on the object name, which consults the built-in extension table and then /etc/mime.types. Where neither yields a type, processRequest() throws, which the enclosing handler already turns into 400 Bad Request and an ObjectIngestFailedEvent with CLIENT_ERROR. 400 is the right refusal: the client pushed an object whose type it did not declare and whose name does not imply one, so the request cannot be served as sent. An empty Content-Type header is treated as absent rather than as a media type. Verification T1: builds clean and the rt-mbs-transport-function suite passes, 6 of 6 binaries, test_push_object_ingester among them. That test covers the shared-path splitting, not the media type, and no test covers this path: reaching it needs an HTTP push into a running microhttpd daemon, which the suite has no scaffolding for. Not verified over the air: the rig is down, and the demo's own objects all carry a Content-Type from the origin, so the demo would not exercise it. Not in this change A configured default media type per object acquisition, which 5G-MAG/Standards#192 adds for Rel-20 and which David notes may need backporting. Inference plus refusal is what the current data model supports.
… widen it Problem flute_path_mtu() returned the discovered MTU outright whenever the route was not loopback, including where it was wider than the configured path MTU. The configured value was reached only on a loopback route. Context.hh and mbstf.yaml document the opposite, and have throughout: the discovered value "is used in place of it only when it is smaller, since a first hop narrower than the stated path MTU is a real constraint while a wider one says nothing about the rest of the path". The function's own comment argued for the behaviour it had, so the component contained both positions at once. [code-derived] The consequence is not cosmetic. What is measured is the first hop: where a tunnel is configured the datagram is re-encapsulated and forwarded over a path this function cannot see. Sizing FLUTE symbols for a 9000 byte first hop because the local interface offers jumbo frames produces datagrams that whatever follows has to fragment or drop. Basis No clause governs this: the path MTU is a bound the operator sets, and the rule for combining it with a measurement is an engineering choice, which is why it is documented at the configuration option rather than cited. #34, which asked for this behaviour: "if a configuration for an MTU has been given in the configuration file then that MTU is used, otherwise the MTU of the interface". Raised by Review by Daniel Silhavy on #71: "flute_path_mtu() returns the discovered first-hop MTU as soon as the route is not loopback. The configured value is only used for loopback. The comments in Context.hh and mbstf.yaml.in promise the opposite". Change The configured path MTU governs. A measured first hop replaces it only when it is narrower, and a loopback route is ignored entirely because it is not a path to a receiver. Each of the two cases logs which value it took and why, so a deployment that ends up with unexpected symbol sizes can see the reason without a rebuild. The comment now states the documented rule rather than arguing against it. Verification T1: builds clean and the rt-mbs-transport-function suite passes, 6 of 6 binaries. No test covers this function; it reads the configured value from the application context, so exercising it needs that context stood up, which the suite does not do. Not verified over the air. The demo's ingress is loopback, which takes the branch that was already correct, so a demo run would not distinguish this change. Not in this change Whether get_tunnelled_path_mtu()'s own 1500 byte fallback should be the configured value instead. It is reached when no address can be resolved at all, which is a different case from a measurement that succeeded.
Problem ObjectCollectionController::processEvent() handled ObjectAdded and ObjectUpdated in full and then fell through to ObjectManifestController::processEvent(), which handles the same two events the same way. So for every such event the manifest handler's update() ran twice, the scheduled pull worker was started twice, and an object already ingested was queued and sent to the packager again under a new TOI. On a broadcast bearer that is transmission capacity spent carrying data the receiver already has, and there is no retransmission to lose either. [code-derived] Basis No clause governs this: it is a control flow error inside one component. code-derived only. Raised by Review by Daniel Silhavy on #71: "manifestHandler() ->update(object) runs twice for every manifest update", "Every already-ingested object is queued and sent again on every manifest update", and that processEvent() "copies about 60 lines of the manifest-detection code from the base class, then also calls the base ObjectManifestController::processEvent(), so everything runs twice". Confirmed against the code. Change The override returns after handling those two events, so the base class is not asked to handle them again. Any other event still reaches the base as before. Nothing is lost by returning. The only thing the base does that this override does not is call objectAddOrUpdateEvent(), which is an empty virtual in ObjectManifestController. Only ObjectCarouselController overrides it, and only to set keepAfterSend(true), which this override already does itself a few lines earlier. Checked at the same time: no sibling controller has the same fall-through. This one was alone in calling the base after handling the event itself. Verification T1: builds clean and the rt-mbs-transport-function suite passes, 6 of 6 binaries. No test covers this path; reaching it needs an object store, a manifest handler and a packager wired together, which the suite does not stand up. Not verified over the air. Confirming the effect needs a capture showing one TOI per object where there were two, and the rig was not run for it. Not in this change The duplication itself, which is the rest of that review comment: the override repeats the base's manifest detection rather than extending it. Removing the copy is a refactor of both classes and wants its own change, where this one stops the visible damage.
Problem Any SBI request whose API name is not "nmbstf-distsession" aborts the process with SIGABRT before its response reaches the client. Observed: "Aborted (core dumped)", exit 134, with the log ending cleanly at nghttp2-server.c:1152 "STREAM closed [1]" and no assertion printed. gdb puts the abort in talloc, under ogs_sbi_request_free() <- Open5GSSBIRequest::~Open5GSSBIRequest() (src/mbstf/Open5GSSBIRequest.cc:71) <- MBSTFEventHandler::dispatch() (src/mbstf/MBSTFEventHandler.cc:124). [observed] DistributionSession::processEvent() returns false for any other service name (src/mbstf/DistributionSession.cc:222-226), so control reaches the handler's own OGS_EVENT_SBI_SERVER branch. That branch also serves the NRF status-notification callback, so routine NRF traffic reaches it. [code-derived] Basis code-derived, no spec claim. A received request belongs to the SBI server for the lifetime of its stream: stream_remove() asserts stream->request and frees it (subprojects/open5gs/lib/sbi/nghttp2-server.c:717-718). Freeing it in the handler leaves that free operating on released memory, which talloc detects and aborts on. Raised by observation while sweeping the Nmb2 API surface with a deliberately wrong API name; the process exited and the response was never delivered. Change src/mbstf/MBSTFEventHandler.cc constructs the request wrapper non-owning, as every other construction site in this component and in the MBSF already does. The defect is owned here: the SBI server's contract is correct and unchanged, and this was the only site claiming ownership of a received request. The take_ownership argument arrived in 2a901f7 "Fix memory leaks, event name error and propagation of ingest failures". The leak it was chasing on this path is real and is still present after this change (RSS grows about 1.07 kB per request and does not plateau), but the request object is not what leaks. That is a separate item and is not addressed here. Verification T2: live against the demo deployment. Pre-fix, one POST with an unrecognised API name kills the process and delivers nothing. Post-fix, the same request returns HTTP/2 400, a repeat returns 400, a POST to nnrf-nfm/v1/nf-status-notify returns 400, and the process survives 1600 consecutive requests. Reproduction commands, backtrace and RSS series: Standards2Deployments/projects/rt-mbs/evidence/mbstf-sbi-double-free-2026-09-18.txt T0 for the build: ninja -C build completes with no errors. Not in this change The per-request leak on the same path. The Location header on 201 responses. Any change to DistributionSession's own dispatch.
Problem A 201 response to a distribution session create carries the entity-tag as a bare digest. Observed on the wire: etag: 0d0fccda46707c493acaf0386a565bb2d510b59f26c42cca637c35a22a0b6424 Set unquoted at src/mbstf/NfServer.cc:273. [observed, code-derived] The quotes are part of the field value, not display punctuation, so an unquoted value is not a well-formed entity-tag and a conditional request carrying the value a client read back cannot match it. Basis RFC 9110 section 8.8.3: "opaque-tag = DQUOTE *etagc DQUOTE" Raised by reading the authority while sweeping the Nmbstf_DistSession API surface against the responses it actually emits. Change src/mbstf/EntityTag.hh renders an entity-tag into its field-value form and extracts the opaque-tag for comparison. src/mbstf/NfServer.cc uses it at the single site that writes the ETag header. A value that already carries its quotes, weak or strong, passes through unchanged so a caller holding a well-formed tag is not quoted twice. This component owns the defect: it is the one that writes the header. Verification T1: tests/test_EntityTag.cc passes, 11 cases. T2: live against the demo deployment, a distribution session create now returns etag: "873488a748876b1f318c7604771c9aab618e4dac341da689c1953a97e8b362b8" where it previously returned the same digest unquoted. Not in this change Conditional request handling. This component reads neither If-Match nor If-None-Match on any resource, so nothing yet consumes the tag it emits. The Location header on the same response, which is a separate item.
Problem
Every response that carries a Location header builds it from the request
path, so it is a path reference and not the URI of the resource.
Observed on the wire for a distribution session create:
location: /nmbstf-distsession/v1/dist-sessions/cab87e4b-01b1-4138-9018-7d30bcb1606b
Built with std::format from request.uri() at five sites in
src/mbstf/DistributionSession.cc. [observed, code-derived]
Basis
TS 29.581 V18.6.0, table 6.1.3.2.3.1-5, row Location:
"Contains the URI of the newly created resource, according to the structure: {apiRoot}/nmbstf-distSession/<apiVersion>/dist-sessions/{distSessionRef}"
Table 6.1.3.4.3.1-5 states the same for the subscription collection, with
/subscriptions/{subscriptionId} appended.
Note for the reader: that table spells the API name "nmbstf-distSession"
while clause 6.1.1 defines it with a lower case s. The document is
inconsistent on the case and this change does not resolve it: the name is
taken from the request being answered, so whatever the consumer reached is
what comes back. The obligation implemented here is the {apiRoot} prefix,
on which the two agree.
Raised by
reading the authority while sweeping the Nmbstf_DistSession API surface
against the responses it actually emits.
Change
src/mbstf/NfServer.cc gains resourceUri(), which takes the scheme and
authority from the server the request arrived on and the service name and
API version from the request itself, then renders the resource path
through ogs_sbi_server_uri(). src/mbstf/DistributionSession.cc uses it at
all five Location sites: the session create 201, the subscription create
201, the subscription URI recorded when a subscription arrives with a
create, and the individual session GET and PATCH 200 responses.
This component owns the defect: it is the one that writes the header, and
only it knows which resource it is naming.
Verification
T2: live against the demo deployment.
Session create 201 now returns
location: http://127.0.0.62:7777/nmbstf-distsession/v1/dist-sessions/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee
Subscription create 201 now returns
location: http://127.0.0.62:7777/nmbstf-distsession/v1/dist-sessions/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/subscriptions/1154973e-7f06-4a00-8d07-f4201f44604d
The individual session GET 200 carries the absolute form as well. Each
previously returned the path alone.
T0 for the build: ninja -C build completes with no errors.
Not in this change
Whether a Location header belongs on the 200 responses at all. For the
individual session resource the only header tables defined are
6.1.3.3.3.1-4 and 6.1.3.3.3.1-5, for the 307 and 308 responses; no table
lists a header on its 200, so this component sends one the resource
definition does not ask for. That is a separate item; this change only
corrects the form of the value.
Problem
PUT and TRACE on the distribution session resources answered "405 Method
Not Allowed" with an Allow header, which says the method is merely wrong
for that resource. Neither is served by any resource of this API.
Observed: PUT and TRACE on dist-sessions both http=405, and on
dist-sessions/{ref} 405 with "allow: GET, PATCH, DELETE, OPTIONS".
[observed]
Basis
TS 29.581 V18.6.0 gives this API four methods across its four resources:
POST in clauses 6.1.3.2.3.1 and 6.1.3.4.3.1, PATCH in 6.1.3.3.3.1 and
6.1.3.5.3.2, DELETE in 6.1.3.3.3.2 and 6.1.3.5.3.1, and GET in
6.1.3.3.3.3. PUT appears in none of them.
TS 29.500 V18.10.0 clause 5.2.7.2: “A request using an HTTP method which is not supported by any resource of a given 5GC SBI API shall be rejected with the HTTP status code "501 Not Implemented".”
OPTIONS is deliberately left out of the test and continues to be served.
TS 29.500 V18.10.0 clause 6.9.1: “The OPTIONS method, as described in clause 9.3.7 of IETF RFC 9110 [11], may be used by a NF Service Consumer to determine the communication options supported by a NF Service Producer for a target resource.”
Raised by
reading the authority: every resource and method TS 29.581 defines, and
a set it does not, were sent to a running MBSTF and the answers recorded.
Change
src/mbstf/DistributionSession.cc tests the method against the set this
API serves before the resource dispatch, and answers 501 outside it.
Checked before the dispatch so such a method cannot reach a branch that
answers 405 and names an Allow list. This mirrors what
src/mbsf/UserService.cc and src/mbsf/UserDataIngSession.cc already do.
Verification
T2: live against the demo deployment, with a distribution session
created first so a 404 could not be mistaken for a routing answer.
PUT, TRACE, HEAD and an invented method, on the collection and on the
individual resource 501, previously 405
GET on the individual resource 200, unchanged
OPTIONS 204, Allow: GET, PATCH, DELETE, OPTIONS,
unchanged
GET on the collection 405, unchanged, since GET is served on
the individual resource and so is wrong
here rather than unknown
T0 for the build: ninja -C build completes with no errors.
Not in this change
Nothing else in the dispatch. The methods TS 29.581 defines were checked
in the same pass and each is routed: POST on both collections, and GET,
PATCH and DELETE on the resources whose clauses define them.
…he JSON
Problem
A request body sent with "Content-Encoding: gzip" was passed to the JSON
parser as compressed octets and refused with
"400 Bad Request ... Unable to parse JSON". Observed against a running
MBSTF with a gzip-compressed but otherwise valid CreateReqData
document. [observed]
The Content-Encoding header was never examined at any of the four sites
that read a request body, so a coding this NF cannot decode was treated
as though the content were identity-coded. [code-derived]
The status is wrong and the detail is worse: it sends whoever wrote the
client looking for a fault in a document that has none.
Basis
RFC 9110 section 15.5.16: "Servers that fail a request due to an unsupported content coding ought to respond with a 415 (Unsupported Media Type) status and include an Accept-Encoding header field in that response, allowing clients to distinguish between issues related to content codings and media types."
TS 29.500 V18.10.0 table 5.2.2.2-1 lists Content-Encoding among the HTTP
request standard headers an NF is mandatory to support, and table
5.2.7.1-1 marks 415 mandatory for PATCH and POST.
Raised by
reading the authority: the request header tables of TS 29.500 clause
5.2.2.2 were walked against a running MBSTF, and a gzip body was the
case that behaved as though the header were absent.
Change
src/mbstf/NfServer.cc gains refuseUnsupportedContentCoding(), which
answers 415 with "Accept-Encoding: identity" and names the offending
coding in the detail. This NF decodes nothing, so an absent header or
the identity coding passes and anything else is refused.
It is called at all four sites that read a request body, before the
media-type test, since a body that cannot be decoded cannot be typed
either: the distribution session create and its PATCH, and the
subscription create and its PATCH.
Verification
T2: live against the demo deployment.
POST dist-sessions, gzip body 415, "accept-encoding: identity",
detail naming gzip; previously 400
"Unable to parse JSON"
POST dist-sessions, plain 201, unchanged
PATCH dist-sessions/{ref}, gzip 415
PATCH dist-sessions/{ref}, plain 200, unchanged
POST subscriptions, gzip 415
T1: the component's own suite passes unchanged, 98 cases across six
suites that report counts, plus testSubscriberSubscription exiting 0.
T0 for the build: ninja -C build completes with no errors.
Not in this change
Actually decoding gzip request bodies. TS 29.500 clause 6.9 makes
content-coding support discoverable rather than required, and this NF
now says plainly that it supports none.
Problem
The dscpMarking attribute was accepted whatever it contained. Observed
on a running MBSTF, all answered "201 Created":
"2E00" a mask octet the clause fixes at FC
"2EFC00" three octets, not two
"2E" one octet
"ZZFC" not hexadecimal
[observed]
The generated model carries the attribute as a free string, so no layer
between the request and the marking applied to outgoing traffic looks at
it. [code-derived]
A wrong mask is not cosmetic: it changes which bits of the IPv4
Type-of-Service or IPv6 Traffic-Class field the MBSTF overwrites.
Basis
TS 29.581 V18.6.0 clause 6.1.6.2.4: “It shall be encoded as two octet string in hexadecimal representation.”
TS 29.581 V18.6.0 clause 6.1.6.2.4: “The first octet shall contain the DSCP value in the IPv4 Type-of-Service or the IPv6 Traffic-Class field and the second octet shall contain the ToS/Traffic Class mask field, which shall be set to "0xFC".”
Both sentences describe the dscpMarking attribute of the DistSession type.
Raised by
reading the authority: every shall-statement in TS 29.581 was extracted
and checked against a running MBSTF, and this attribute was one that
nothing validated.
Change
src/mbstf/DistributionSession.cc validates the attribute in _validate(),
beside the distribution-data and traffic-flow rules it already enforces:
four hexadecimal digits, and a mask octet of FC in either case. An
absent attribute stays absent, the attribute being optional.
Verification
T2: live against the demo deployment.
absent 201, unchanged
"2EFC" and "2efc" 201
"2E00", "2EFC00", "2E",
"ZZFC" 400 MANDATORY_IE_INCORRECT, naming
distSession.dscpMarking in invalidParams
T1: the component's suite passes unchanged, 98 cases.
T0 for the build: ninja -C build completes with no errors.
Not in this change
The first octet is not range-checked against the six-bit DSCP field.
The clause fixes the mask but states no constraint on the DSCP value
beyond its position, so a bound there would rest on nothing.
…rs them down
Two defects with one shape: work belonging to a Distribution Session outlived the start of the
teardown that was supposed to end it. One killed the process on DELETE, the other on SIGTERM.
Basis
code-derived, no spec claim. No clause governs a process's own shutdown ordering.
Raised by
Observation, twice: the process died on its own ingest path while exercising deletion, and later
exited with SIGSEGV where a test script expected a clean exit. Both were then reproduced, the
second under AddressSanitizer.
## A controller was still subscribed while its derived part was being destroyed
Problem
Deleting a Distribution Session while the MBSTF was ingesting ended the process:
DEBUG: ObjectUpdated with ID: e0abf1c0-... (../src/mbstf/ObjectManifestController.cc:72)
pure virtual method called
terminate called without an active exception
The controllers derive from ObjectManifestController, which derives from ObjectController, which
derives from Subscriber. Subscriber's destructor unsubscribes, but a base destructor runs after
every derived one, so between ~ObjectCollectionController() starting and ~Subscriber()
unsubscribing the object is still registered with the object store while its derived part is
gone. An ObjectAdded or ObjectUpdated delivered in that window reaches
ObjectManifestController::processEvent(), which calls sendToPackager(), pure virtual on that
class, through a vtable that no longer has an implementation of it. [observed, code-derived]
Change
src/mbstf/Subscriber.cc gains unsubscribeFromAll(), which the destructor now uses, and each
controller's destructor calls it first, before any of its own state is torn down. The change can
only remove a subscription earlier than before, never add one.
Verification
T2: a session provisioned, left to ingest, then deleted; five cycles at 8, 11, 14, 17 and 20
seconds of ingest, no occurrence of "pure virtual method called" and the MBSTF up throughout.
The same sequence ended the process before.
## Three classes of worker thread outlived the start of process teardown
Problem
The MBSTF ends in a heap-use-after-free when terminated while a Distribution Session is
ingesting, and takes about twenty seconds to answer SIGTERM before it does. Six runs out of six
under AddressSanitizer.
The ingest workers belong to the ObjectController base, so destroying a session stops them last,
after every derived destructor:
15:50:22.825 ~ObjectCarouselController enter
15:50:41.449 ~ObjectCarouselController aborted (scheduled pull joined)
15:50:41.463 ~ObjectController enter, pullIngesters=157
157 workers ran for the 18.6 seconds between, and one was still parsing URIs after main() had
returned:
heap-use-after-free, READ of size 8, thread T2
#5 libmpdpp::URI::validate() ../subprojects/libmpdpp/src/URI.cc:30
#7 PullObjectIngester::doObjectIngest() ../src/mbstf/PullObjectIngester.cc:264
freed by thread T0:
#14 __run_exit_handlers stdlib/exit.c:118
The scheduled pull: ObjectManifestController::abort() set its cancel flag and joined, but the
thread waits in m_manifestHandlerChange.wait_until(lock, fetch_time) and nothing woke it, so the
join waited out a whole manifest fetch interval. That is the 18.6 seconds above and the twenty
second SIGTERM.
The asynchronous event threads: every SubscriptionService starts one on first use, and an ingest
worker starts its service's thread by emitting an event. They allocate from the open5gs memory
pools, which teardown takes away:
heap-use-after-free, WRITE of size 8, thread T272
#1 SubscriptionService::asyncEventsLoop() ../src/mbstf/SubscriptionService.cc:352
[observed, code-derived]
Change
Every thread the MBSTF starts for a session is stopped before teardown begins, rather than as a
side effect of destroying its owner.
Controller::abortIngest() is the hook, stopping the controller's own asynchronous event thread.
ObjectController overrides it to abort the push and pull ingesters and stop the object store's
event thread. ObjectManifestController overrides it again to stop the scheduled pull first.
Context::abortAllIngest() calls it for every Distribution Session, and app_terminate() calls that
before the event handler and the context are destroyed.
ObjectIngester::abort() also stops its own service's event thread, which the worker starts by
emitting events and which outlives the worker.
ObjectManifestController::abort() now notifies m_manifestHandlerChange after setting the cancel
flag, holding the mutex so the waiter cannot miss it and releasing it before the join.
The four most-derived object controllers call abortIngest() first in their own destructors,
beside the unsubscribeFromAll() added above for the same kind of reason, so a session deleted on
its own stops its threads before its destructors start as well.
All of these are idempotent: a cancelled worker is no longer joinable and a stopped event loop
stays stopped, so the destructors that call them again repeat nothing.
Verification
T2: one CAROUSEL/PULL Distribution Session ingesting, MBSTF sent SIGTERM, under AddressSanitizer,
core restarted between runs.
without 6 runs, 6 heap-use-after-free, SIGTERM answered in 19 to 20s
with 12 runs, 0, SIGTERM answered in 1 to 2s
Normal operation is unchanged: a session reaches ACTIVE, ingests (14 objects in the last check),
answers DELETE 204 with the resource then gone, the MBSTF stays up, and no further object is
ingested after the delete.
T1 for both items: the component's own suite passes, 7 suites, 98 cases.
T0 for the build: ninja -C build completes with no errors.
Not in this change
An event already being dispatched on another thread when a destructor starts is a separate
question about how the object store synchronises dispatch with the lifetime of its subscribers.
Why a CAROUSEL session accumulates 157 pull ingesters for one carousel. Why a manifest fetch
interval is long enough to be worth 18 seconds of join. The vendored open5gs and libmpdpp tests,
which fail before and after this change alike.
…ifest
Problem
The scheduled pull kept one PullObjectIngester per item in the content
provider's manifest, so the number of threads a Distribution Session ran was
whatever that manifest happened to contain, with nothing an operator could set
to limit it. The demo's 157 object carousel ran 157 ingesters, and each
carries an ingest worker thread and an asynchronous event thread: 323 threads
in the process. Code-derived:
src/mbstf/ObjectManifestController.cc, the scheduled pull grew the pool with
"while (ingesters.size() < next_ingest_items.second.size())" and then handed
each ingester exactly one item, so the pool had to match the item count or the
remainder went unfetched. [observed, code-derived]
Basis
No clause governs this; engineering choice resting on a configuration option
the operator sets, with an explicit default the operator can override
(RULES.md rule 12).
What the ceiling is for is stopping a remote manifest from dictating the
thread count without limit, not tuning parallelism, so the default is set high
enough that manifests of realistic size keep the behaviour they had. Measured
on the 157 object carousel: the default leaves the process at 324 threads
where it ran 323 before, so nothing about a working deployment moves.
Raised by
observation, while tracing the shutdown crash, where the teardown reported 157
pull ingesters for one carousel.
Change
src/mbstf/Context.{hh,cc}: mbstf.maxConcurrentPullIngesters, default 256,
rejected below 1. src/mbstf/mbstf.yaml.in documents it.
src/mbstf/ObjectManifestController.cc: the pool grows to the lesser of the
item count and the ceiling, and items are distributed round robin over the
ingesters instead of one each. That distribution is what makes a ceiling safe:
each ingester keeps its own deadline-sorted queue, so a second item handed to
one is fetched after its first rather than instead of it. Without it, a
ceiling below the item count would have silently left the remainder unfetched.
Verification
T2: live against the demo deployment, one CAROUSEL/PULL session over a 157
object carousel, thread count read from /proc/<pid>/task and items counted
from the ingest log. The same binary throughout, only the configuration
changing.
ceiling 10000 (above the manifest) 323 threads
ceiling 256 (the default) 324 threads
ceiling 32 73 threads, all 157 items reached
ceiling 8 25 threads, 156 of 157 reached in 90s
So the ceiling binds, and binding it well below the item count still reaches
the items rather than dropping them.
T1: the component's own suite passes, 7 suites, 98 cases.
Not in this change
How quickly a carousel completes a cycle at a given ceiling. The push ingester,
of which there is one per session regardless.
…gent
Three defects in what this API puts on the wire, found while walking TS 29.581 and TS 26.517.
Raised by
The first from observation, while tracing why a delete was being sent twice and why the second
was answered 404. The other two from reading the authority during this work.
## GET and PATCH returned the creation wrapper instead of the Distribution Session
Problem
Every successful Nmb2 PATCH was rejected by the MBSF, which then tore the Distribution Session
down and deleted it twice:
MBSTF 200 PATCH, body {"distSession":{"distSessionId":"aae898cb-...",...}}
MBSF ERROR: Field "distSessionId" is required
MBSF [DELETE] .../dist-sessions/a20524e3-... (from the error path)
MBSF [DELETE] .../dist-sessions/a20524e3-... (the consumer's delete)
MBSTF 204 to the first, 404 to the second
The MBSF reads the body as a DistSession, which is what it is specified to be, and finds
distSessionId one level further down than it should be. src/mbstf/DistributionSession.cc, json()
wrapped the Distribution Session in a CreateRspData and all three responses were built from it.
[observed, code-derived]
Basis
The wrapper belongs to the creation response alone.
The POST 201 body is CreateRspData, M, 1, in table 6.1.3.2.3.1-3.
TS 29.581 V18.6.0 clause 6.1.3.2.3.1: "Successful creation of an MBS session"
The PATCH 200 body is DistSession, M, 1, in table 6.1.3.3.3.1-3.
TS 29.581 V18.6.0 clause 6.1.3.3.3.1: "Upon success, a response body containing the updated representation of Distribution Session shall be returned"
The GET 200 body is DistSession, M, 1, in table 6.1.3.3.3.3-3.
TS 29.581 V18.6.0 clause 6.1.3.3.3.3: "Successful response containing representation of the MBS Distribution Session"
This is the MBSTF's defect and not the MBSF's. The MBSF rejects a body that does not match the
type the table names, which is what it should do, so the component to change is the one sending
the wrong type.
Change
src/mbstf/DistributionSession.{hh,cc}: distSessionJson() renders the Distribution Session on its
own and the GET and PATCH handlers use it. json() keeps the CreateRspData wrapper for the
creation response, and both now build the representation through one private helper so the
subscription location is added the same way for all three.
Verification
T2: one create, GET, PATCH, PUT, DELETE, GET sequence, counted in the MBSF's log for that run.
without with
"distSessionId is required" 1 0
Nmb2 DELETEs sent 2 1
Nmb2 DELETE answered 204 1 1
Nmb2 DELETE answered 404 1 0
The consumer sees 201, 200, 200, 200, 204 and 404 afterwards either way; what changes is that the
Distribution Session is no longer torn down behind it.
## A refused PATCH did not say what this API would have accepted
Problem
Neither of this API's two PATCH resources said what it would have accepted when refusing a patch
document. Both answered 415 with no Accept-Patch, against a Distribution Session and a
subscription that exist. A comment at the first of the two checks also described the media type
it tests for as the merge-patch type, which it is not. [observed, code-derived]
Basis
TS 29.500 V18.10.0 clause 5.2.7.2: “If the HTTP PATCH method is rejected due to unsupported patch document, the NF shall include the Accept-Patch header field set to the value of supported patch document media types for a target resource i.e. to "application/merge-patch+json" if the NF supports "JSON Merge Patch" and to "application/json-patch+json" if the NF supports "JSON Patch".”
Which of the two this API takes is JSON Patch, and both checks were already right to refuse
anything else: TS 29.581 V18.6.0 clause 6.1.2.2.2 names no patch media type, and the OpenAPI that
specification carries gives the PATCH requestBody of both resources as
application/json-patch+json with a schema of an array of PatchItem, minItems 1.
The two APIs of this stack differ here, which is why the comment mattered: TS 29.580 V18.8.0
clause 6.2.2.2.2 requires JSON Merge Patch of the MBSF's own resources, and this one requires
JSON Patch.
Change
src/mbstf/NfServer.{hh,cc}: refuseUnsupportedPatchDocument() answers the 415 with the header,
built the way the neighbouring refuseUnsupportedContentCoding() builds its Accept-Encoding for
the sibling obligation in the same clause.
src/mbstf/DistributionSession.cc: both PATCH handlers call it, so the media type is named in one
place rather than two, and the comment that called it the merge-patch type is gone with the test
it described.
Verification
T2, against a Distribution Session and a subscription that both exist.
resource and Content-Type before after
session, application/json 415, none 415, Accept-Patch:
application/json-patch+json
session, application/merge-patch+json 415, none 415, same header
session, application/json-patch+json 200 200
subscription, application/json 415, none 415, same header
subscription, application/json-patch+json 200 200
The merge-patch rows are the ones worth reading twice: it is refused here and required by the
MBSF's own APIs, and the header is what tells the two apart. The subscription lifecycle around it
is unaffected: POST 201 with an absolute Location, PATCH 200, DELETE 204, a second DELETE 404.
## The pull ingest User-Agent named this build instead of the release
Problem
src/mbstf/Curl.cc set CURLOPT_USERAGENT to MBSTF_TYPE "/" MBSTF_VERSION, and the generated
build/src/mbstf/mbstf-version.h gives MBSTF_TYPE "MBSTF" and MBSTF_VERSION "1.5.0", so the header
read "MBSTF/1.5.0". That 1.5.0 is this component's own version and indicates neither a version of
the specification nor a 3GPP release. [code-derived]
Basis
The product identifier token was already right.
TS 26.517 V18.6.0 clause 8.2.3.2.3: "The product identifier token shall be set to the value MBSTF."
What follows it was not.
TS 26.517 V18.6.0 clause 8.2.3.2.1: “The optional product-version suffix shall be present and should indicate the version number of the present document (without the leading "V") with which the client implementation complies and shall, at minimum, indicate the 3GPP release number with which the implementation complies.”
The same clause admits what the old value was trying to say, as an identifier of its own rather
than as that suffix, and its own EXAMPLE 1 shows the shape. The Server response header these
requests are answered with already carries the release this way, from the same FIVEG_API_RELEASE
the change uses.
Change
src/mbstf/Curl.cc: the default User-Agent becomes MBSTF/<release> <component>/<component version>,
taking the release from FIVEG_API_RELEASE and leaving the component's own version as a vendor
product identifier after it. A User-Agent set explicitly by a caller is untouched.
Verification
T2: the Distribution Session's pull ingest pointed at a listener that records request headers.
Six requests, every one carrying
User-Agent: MBSTF/18 rt-mbs-transport-function/1.5.0
T1 for all three items: the component's own suite passes, 7 suites, 98 cases.
Not in this change
The MBSF's own guard against an identifier it no longer holds, which the first item removes the
usual cause of but is worth keeping for any other. JSON Merge Patch, which this API does not
take. The push-based acquisition method, which makes no request of its own. The MBSF's own
User-Agent at Nmb2, which clause 8.2.3.2.2 refers to TS 29.580 for rather than to the general
provisions clause 8.2.3.2.1 sets.
…n dead completion path TS 26.517 issue: #69, closed on 2026-09-18 as already done and reopened on maintainer review; this is the fix that makes the closure correct. Problem Neither packager used the finish_file_transmissions parameter rt-libflute PR #57 added to Transmitter::deactivate(). Both still replicated the wait it exists to replace: computing "is the queue empty yet" themselves in their own completion callback and only then calling bare deactivate(), once they had already confirmed nothing was left to send. [code-derived] ObjectCarouselPackager::deactivate() has never called objectSendCompletion(), though the method is declared on the class and was evidently meant to be, going by ObjectListPackager's identical-looking declaration and definition it was written from (git blame: e791214, "Add CAROUSEL operating mode (issue #51)"). Established per rule 14 before fixing it: the method exists, is declared, and is never once called anywhere in the file. The consequence: when ObjectCarouselPackager::deactivate() is called while the transmitter still has files in flight, it returns false, and nothing ever finishes the job. Nothing calls DistributionSession::haveEmptyQueue() for a carousel, unlike ObjectListPackager's callback, which does call the equivalent through objectSendCompletion(). A Distribution Session using a carousel, deactivated at the wrong instant, would sit in DEACTIVATING forever. Basis rt-libflute Transmitter.h, deactivate(): "When finish_file_transmissions is `true` the Transmitter will remain active until the queued transmissions have completed and will then become inactive. This allows applications to request deactivation without waiting for completion callbacks and checking number_of_files()." Raised by Issue #69 itself (5G-MAG/rt-libflute#57's author), closed then reopened after dsilhavy's review correctly pointed out the boolean was never actually used. The dead objectSendCompletion() was found while fixing that, reading the carousel packager end to end to decide whether the same adoption was safe there. [rule 13] Change ObjectListPackager::doObjectPackage() tells the Transmitter to defer-deactivate, once m_packageItems is confirmed empty and m_deactivating is set. That is the earliest point nothing else will ever be handed to the Transmitter: add() already refuses new items once m_deactivating is set, so an empty m_packageItems at that point stays empty. Transmitter::send() does not clear the deferred-deactivation flag (checked directly in rt-libflute's own source, Transmitter.cpp), so this is safe even on the loop iteration where the very last item is popped and sent in the same pass. The completion callback no longer calls deactivate() itself; the Transmitter now does that on its own once genuinely empty, and the callback's remaining job is only this packager's own bookkeeping (stopping the worker thread, clearing m_deactivating). ObjectCarouselPackager's completion callback now calls objectSendCompletion() with the same queue-empty computation deactivate() already used, and finishes the deactivation the same way ObjectListPackager's callback did before this commit (compute queue_empty, and if m_deactivating, call deactivate() and abort()). This is the conservative fix for the carousel: it closes the confirmed hang without also adopting deactivate(true) there, because the carousel's queue is refilled by a separate scheduler thread on its own timer rather than drained once by the worker loop the way ObjectListPackager's is, and I could not establish with the same confidence that telling the Transmitter to defer-deactivate at arbitrary call time is race-free against that scheduler without changes to it, which is out of scope here. Verification T1: the component's own suite passes unchanged, 8 suites (2 vendored subproject suites and 1 libmpdpp suite fail, all pre-existing and unrelated -- open5gs epc/app tests and libmpdpp:segment_templates). T2, live against the demo deployment, STREAMING and CAROUSEL operating modes: - Repeated create/DELETE cycles, timed close to creation and at random short delays: every DELETE answered 204, the MBSTF stayed up throughout, no hang. - One CAROUSEL session left to stream for several seconds (multiple TOIs observed completing in the log) before DELETE: 204 in 38ms, MBSTF log shows "Deactivating FLUTE stream, no files to purge" -- the immediate branch, queue already empty. - Regression: full status subscription and Distribution Session event lifecycle unaffected (DIST_SESS_STARTING/STARTED in order, USER_DATA_ING_SESS_STARTING/STARTED in order, session and subscription resource lifecycles unchanged). Not reached live, despite repeated attempts (over 20 create/delete cycles across both operating modes, with sleeps from 0 to 6 seconds and log-polling for an in-flight fetch before deleting): the deferred branch itself, where deactivate() is called while a file transmission is genuinely still in flight. The graceful teardown sequence (DistributionSession::_apiSessionPatch, flushPackagerQueue() then _transitionTo()) flushes m_packageItems moments before deactivateOutput() runs, so by the time deactivate() is called the queue is already empty in the overwhelming majority of cases; the in-flight window this change actually widens coverage for is narrow. Correctness there rests on the code-derived reasoning above (rt-libflute's own deactivate()/send()/_complete_deactivation() implementation, read directly) rather than a live reproduction. Not claiming T2 for that specific branch. Not in this change Adopting finish_file_transmissions for ObjectCarouselPackager's own deactivate() call, which needs the scheduler thread's own repeat-scheduling to be considered first.
…fault one Refs #74 Problem This MBSTF inferred a missing Content-Type from the object's filename extension (316e425), which replaced an earlier version that defaulted to application/octet-stream (efbce81, itself reverted). Review by rjb1000 on #74 asked for both to be removed: "reject all typeless objects with an informative explanation of how the MBS Application Provider has erred" rather than guess a media type on the integrator's behalf. [source-derived: review comment] Separately, the PUSH path's error response carried no body at all: setError() recorded a reason but requestHandler() always queued the response it had created before any processing ran, an empty buffer, so a refused pushed object got a bare status code and nothing else. rjb1000's request for an informative explanation cannot be met by a 400 with an empty body. [code-derived] Basis TS 26.517 V18.6.0 clause 6.2.1 binds the MBSTF to the MBMS Download Profile, and TS 26.346 V18.2.0 clause L.4.2 lists Content-Type first among the attributes that "shall be carried in the FDT sent by the FLUTE sender". Neither clause says where a missing value should come from; that a missing one must be refused rather than filled in is the reviewer's judgement, not a clause. Raised by Review by rjb1000 and davidjwbbc on #74, following an earlier defect in the same issue raised by review from davidjwbbc. [rule 13] Change MediaTypeInference.{cc,hh} is deleted along with its meson.build entry: the inference it did is exactly what was asked to be removed, and nothing else used it (only the two call sites this commit also changes). PullObjectIngester.cc: an empty Content-Type from the origin fails the ingest through the existing emitObjectPullIngestFailedEvent() path, with no attempt at inference first. PushObjectIngester.cc: the same for a pushed object, through the existing std::runtime_error / setError(400) path. PushObjectIngester.{cc,hh}: setError() gained a detail parameter, and requestHandler() rebuilds the response from it when non-empty, replacing the response created before any processing ran. No ProblemDetails structure: TS 26.517 defines no error body for this interface, so a plain text/plain body says what went wrong without asserting a schema nobody defined. The 405 case, which passes no detail, is unaffected -- still the same empty body as before. Verification T1: the component's own suite passes unchanged, 8 suites (2 vendored + 1 libmpdpp failure, pre-existing and unrelated). T2, live against the demo deployment, PUSH ingest with no Content-Type header: HTTP/1.1 400 Bad Request Content-Type: text/plain the pushed object [/notype2.bin] carried no Content-Type; an object with no media type cannot be carried in a conformant FDT MBSTF stayed up throughout. Full Distribution Session event lifecycle (DIST_SESS_STARTING/STARTED, USER_DATA_ING_SESS_STARTING/STARTED, in order) unaffected by a fresh session created after this change. Not in this change A ProblemDetails-shaped error body, which nothing in TS 26.517 defines for this interface and which this commit does not invent. The PULL path's own failure, which has no HTTP client to answer: it already reaches the existing ingest-failure event, unchanged here.
Refs #74 Problem ObjectController::packager(nullptr) crashed the process, every time it was called. Found while live-testing the Content-Type refusal added for #74: a pushed object whose Content-Type had no matching manifest handler took this path and took the whole MBSTF down with it, reproduced twice. [observed] ObjectController::packager(ObjectPackager*) resets m_packager to whatever it is given, including nullptr, and then unconditionally dereferences it: subscribeTo(..., *m_packager.get()). unsetObjectListPackager() and unsetObjectPackager() on ObjectCollectionController both call packager(nullptr) precisely to unset it, so every path that gives a manifest up -- including one that could never be described, the case this bug was found through -- reached this and crashed. [code-derived: src/mbstf/ObjectController.cc:150] Basis code-derived, no spec claim: a null pointer given a name (unsetObjectListPackager) is not the same as one nobody meant to pass, and dereferencing it either way is the same defect. RULES.md's stop conditions name a null dereference as work needing no external claim. Raised by Observation, live-testing the change for #74. [rule 13] Change ObjectController::packager() only subscribes when the packager it was just given is non-null. Setting one still subscribes as before; unsetting one now does only that. Verification T2, live against the demo deployment: the exact sequence that crashed the MBSTF twice before this change -- a PUSH-ingested object whose Content-Type has no registered manifest handler, in a COLLECTION Distribution Session -- now completes (200, request processed, request removed) with the MBSTF still running afterward. Reproduced once against the fix; the defect itself was reproduced twice before it. T1: the component's own suite passes unchanged, 8 suites (2 vendored + 1 libmpdpp failure, pre-existing and unrelated). Not in this change Why no manifest handler is registered for the media types this crash was found through (application/x-www-form-urlencoded is what curl sends by default for --data-binary with no explicit Content-Type; a COLLECTION session's manifest handler needs a real manifest format). Answering 400 rather than 200 for an object whose media type is well-formed but not one this build recognises as a manifest, which is a separate question about the right status for that case and not addressed here.
Problem ObjectController::pushObjectIngester(nullptr) has the identical shape as ObjectController::packager(nullptr), fixed earlier on this branch: it resets m_pushIngester and then unconditionally dereferences it. ObjectListController and ObjectManifestController's own reconfigurePushObjectIngester() both call it this way, on an acquisition method change away from PUSH. [code-derived: src/mbstf/ObjectController.cc:86] Currently unreachable: reconfigurePushObjectIngester() is only ever called from ObjectController::reconfigure(), which nothing calls. Established from grep across every .cc file and from Controller::reconfigure()'s own three real callers, all on the Packet* side (PacketController.cc, PacketForwardOnlyController.cc, PacketProxyController.cc) -- none on the Object* side this bug lives on. The virtual dispatch was built (08163de, adding PATCH support for DistSession updates) but never connected for object-based controllers, so a PATCH changing objAcquisitionMethod away from PUSH today changes the stored value without touching the running ingester. Basis code-derived, no spec claim. Raised by Observation, applying the same scan that found the packager(nullptr) crash to every other reset(param)-then-dereference of the same shape in this component. [rule 13] Change Same guard as packager(nullptr): only subscribe when the new pointer is non-null. Verification T1: the component's own suite passes unchanged, 8 suites. T0/code-derived for the defect itself: not reachable today for the reason above, so no live crash to reproduce. The fix removes the landmine for whenever reconfigure() is wired up to run, rather than fixing a crash observed in this session. Not in this change Wiring ObjectController::reconfigure() into whatever should call it so a PATCH-driven objAcquisitionMethod change actually takes effect on a running Distribution Session. That is a separate, larger gap this investigation surfaced, recorded in the findings register rather than fixed here.
…smitter Problem ensureTransmitter() leaves m_transmitter unset and returns early when the Distribution Session's own FEC configuration is rejected by fecOtiFromFecConfig() (logging the reason and returning, src/mbstf/ObjectCarouselPackager.cc:270-276). scheduleCarousel()'s per-item lambda calls ensureTransmitter() and then unconditionally builds a file description and calls m_transmitter->send(file_desc), with no check that the call actually produced a Transmitter. A carousel session with a FEC configuration the FDT schema does not admit crashes MBSTF on its first scheduled send. [code-derived] Basis no clause governs this; the fix confines a null Transmitter to the existing "give up on this item this cycle, retry next" path rather than adding a new one. Raised by review by dsilhavy on PR #71 (rt-mbs-transport-function) Change The lambda now throws std::runtime_error when m_transmitter is still unset after ensureTransmitter(), before touching the file description. streamsAllocateToi() already catches std::runtime_error from this lambda (logs "Unable to schedule carousel stream" and leaves the item unscheduled for this pass, per its existing handling of a rejected FEC configuration at Transmitter-construction time) -- no new catch was needed, just closing the one path that bypassed it. Verification T1: builds clean; own suite 7/7. Not in this change Does not address the session's own eventual fate (still DEACTIVATING/stuck rather than transitioned to INACTIVE) -- that overlaps the still-open ObjectListPackager FEC-rejection finding on the same PR and is left for that item.
Problem A failed pull is popped off m_fetchList before the failure event fires (src/mbstf/PullObjectIngester.cc:209-210 at the time of writing), so the retry that follows (ObjectManifestController.cc's IngestFailedEvent handler calling item.recordFetchFailure() then ingester->fetch(item)) always finds no matching entry in fetch(IngestItem&&)'s own list search. It falls to the "no previous version" path, which re-queues by calling the object_id-based fetch() overload with only scalar fields (object_id, deadline, forceRecache, keepAfterSend, compressSend) -- never the incremented failure count. That overload's own "not found" branch then builds a brand new IngestItem via ObjectStore::takeMetadataForIngest(), whose constructor always starts m_fetchFailures at 0. Net effect: every single retry reports exactly 1 failure to ObjectManifestController's consecutiveIngestFailuresBeforeDeactivate check, so an object that fails forever is retried forever, the operator-configured limit notwithstanding. [code-derived] Basis no clause governs this; the fix restores the counter's own already-established contract (its header comment: "the count is thrown away on every retry" was the reviewer's own diagnosis, quoted here for the record) rather than adding a new one. Raised by review by dsilhavy on PR #71 (rt-mbs-transport-function) Change The object_id-based fetch() overload gains a fetch_failures parameter (default 0, so its two other call sites -- the "found in list" branch's own update, and every external caller, which all go through the IngestItem-taking overloads -- are unaffected) and applies it to the freshly rebuilt item in its "not found" branch. fetch(IngestItem&&)'s happy-path call now passes item.fetchFailures() through instead of dropping it. Verification T1: own suite 7/7 (including the pre-existing testFetchFailureCountSurvivesCopyAndMove, which covers the counter's copy/move fidelity but not this requeue path). No new test added: exercising fetch()'s requeue path itself needs a live ObjectStore-backed retry sequence, which this test file's own stated scope declines ("Fetching needs an origin server and a running event loop, which is not a unit test") -- stated rather than claimed covered. Not in this change Does not touch the "found in list" branch, or ObjectManifestController's own retry-decision logic, both already correct for what they do.
…ackager Problem doObjectPackage() logs and returns when fecOtiFromFecConfig() rejects the Distribution Session's own FEC configuration, leaving m_transmitter unset. No PackagingFailedEvent is sent, so ObjectController never learns anything went wrong and the session never moves to INACTIVE. The worker thread's own loop (ObjectPackager::workerLoop(): while(!cancel) doObjectPackage();) then calls straight back into the same rejection on its very next iteration, spinning one core at 100% forever with no progress and no visible failure. [code-derived] Basis no clause governs this; the fix reuses the session-INACTIVE-on-PackagingFailedEvent mechanism ObjectController::processEvent() already implements for every other packaging failure, and the same reason ObjectCarouselPackager's own errorInCarousel() already reports its bit-rate-overflow and stream-not-found failures this way. Raised by review by dsilhavy on PR #71 (rt-mbs-transport-function) Change Added FailureType::FEC_CONFIGURATION_REJECTED to ObjectPackager::PackagingFailedEvent (additive; nothing switches exhaustively on this enum today, so no other call site needed a case for it). ObjectListPackager's FEC-rejection catch now sends a PackagingFailedEvent synchronously -- so ObjectController's handler has marked the session INACTIVE before the worker stops -- and then calls abort() (safe to call from the worker thread itself: it already guards its own join against that) instead of returning back into the loop. Verification T1: builds clean; own suite 7/7. Not in this change ObjectCarouselPackager's ensureTransmitter() has the same FEC-rejection shape (log and return, no event) but was not reported against on this PR and is not touched here; recorded separately in the findings register as the same gap, for its own item.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MBS Broadcast content delivery fixes for the MBSTF. 35 commits on
feature/mbs-compliance-fixes, basedevelopment.Type: compliance fix, plus crash and delivery fixes.
Baseline: TS 26.517 V18.6.0, TS 26.346 V18.2.0, TS 29.581 V18.6.0, TS 29.500 V18.10.0, RFC 3926, RFC 5651, RFC 7538, RFC 9110.
What it changes
notifyRetryDelay, bounded bynotifyRetryAttempts.Allowheader TS 29.500 cl.5.2.7.2 requires.httpPushIngestaddress.ObjectStoremetadata copied under the lock.Issues: may resolve #27, #44, #59, #73, #74, #75.
Depends on: rt-libflute
feature/mbs-compliance-fixes(FEC schemes and the profiled FDT).Tests
ObjectStore 23, PullObjectIngester 27, ObjectListPackager 16, PushObjectIngester 11, FecOtiHelper 8, SubscriberSubscription — all pass.
Not covered
he shared push daemon and the retry path have no coverage of their new behaviour; both need a live Application Provider or notification consumer. The push routing decision is tested, not that a push arrives.
Withdrawn from an earlier revision
All three on review: