Repository navigation
Conversation
This was referenced Sep 24, 2026
midwell
force-pushed
the
split/ue-requested-modification
branch
3 times, most recently
from
September 30, 2026 19:48
e74bd2f to
c6fe258
Compare
midwell
force-pushed
the
split/ue-requested-modification
branch
5 times, most recently
from
October 7, 2026 10:56
38c22f9 to
abb921a
Compare
TS 23.502 subclause 4.3.3.2 and TS 24.501 subclause 6.3.2: when policy changes a running session, the SMF programs the user plane, tells the UE what changed, and waits for the UE to acknowledge it. Until now the SMF sent the command and treated it as done. The procedure is now driven end to end. ApplyModification programs the user plane, moves the session into PfcpModify, sends the N1 command and the N2 transfer together, and arms T3591. The UE's Modification Complete is what commits the update: committing on the acknowledgement rather than on the send keeps the SMF's record in step with what the UE is actually running, so the next modification computes its delta against the parameters in force. A Modification Command Reject discards it with its own cause. An unanswered command is retransmitted, and abandoned when the retransmissions run out -- the pending update discarded, the session settled back to Active so a later modification can be attempted, and the abandonment counted with the path and cause that produced it. Abandonment is idempotent and ignores a superseded timer, because a procedure can be answered while its last retransmission is in flight. A command that cannot be delivered is reverted rather than released. The delivery-failure indication is shared with establishment, where dropping the data path is right because the session was never usable; for a modification it is wrong, and turned a routine delivery failure into an outage. Where putting the user plane back itself fails, the session is marked for release instead, because it is then running parameters the UE was never told about -- and the FSM is told which of the two happened. A rule the update deletes is withdrawn from the user plane as well: its PDR and FAR are marked for removal, in both directions. The QERs are deliberately kept, being per session and shared with the rules that remain. The radio's answer to the command is not handled here; that follows. Rebased onto main after omec-project#664 merged, keeping its guarantee through this branch's rewrite of the policy-update path. A modification that fails now returns the session to active through abandonPendingModify, so the pending user-plane entry is cleared too, where ApplyModification only reset the state. omec-project#664's test now records the entry the way a real send does, and fails without this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
An acknowledgement that arrived just as T3591 fired could be followed by one more PDU session modification command, for a procedure that had already ended. Stopping T3591 does not wait for an expiry that is already due: the timer's goroutine selects between the stop signal and its ticker, and Go picks at random when both are ready. Abandonment was already guarded against this, because abandonIfCurrent ignores an expiry for a timer that is no longer the session's. Retransmission was not. The retransmission now sends only while its own timer is still the session's T3591, and it checks that under SMLock at each of the send's two holds of the lock: before building the command, and again in the hold that starts the transfer. The second check is what makes it exact. The acknowledgement stops T3591 under SMLock, so it either lands before that check, which then fails, or waits until the command is on its way, in which case the retransmission really did precede it. A check-then-send in the timer callback would not have closed the gap, because the send takes SMLock itself. Neither would making Stop wait, because callers stop the timer under the lock the callback takes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
When a modification cannot be delivered and putting the user plane back fails too, the session runs parameters the UE was never told about, and revertModification marks it SmStatePfcpRelease. The FSM handler for the delivery-failure indication then returned SmStateInit for every case that was not a successful revert, and HandleEvent applies whatever the handler returns, so the mark was overwritten one frame after it was made. HandlePduSessN1N2TransFailInd now reports whether the failure was a modification's as well as whether it was reverted. A modification whose revert failed keeps SmStatePfcpRelease; the AN-release path, which is not a modification, still ends in Init as it always has. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
abandonModificationLocked dropped the session's handle on T3591 without stopping the timer. Most abandonments stop it themselves before they get there, but a delivery-failure indication reverts a modification whose timer is still armed, and that timer went on firing for its whole retransmission sequence: each retransmission was refused by the currency check, and the final abandonment found a different timer and did nothing. Nothing reached the UE, but a goroutine per reverted modification ran on for up to five T3591 intervals, and it read the retransmission seam while tests were replacing it. The handle is now dropped by StopT3591, which also clears NwModificationPending as this did. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A second policy notification for a session whose network-requested modification was still waiting for the UE started a second procedure over the first. It replaced the pending update and stopped the first T3591, and the UE's answer to the first Command then committed the second update, which the UE had never been sent; the user plane by then carried the changes of both. TS 24.501 subclause 6.3.2.5 gives the network no way to run two at once: a network-requested Command carries no procedure transaction identity, so the UE's answer cannot say which Command it answers. Such a decision is now held, and the PCF answered as for an applied one. Each held decision is applied as a procedure of its own once the one before it ends: the UE completing or rejecting it, T3591 abandoning it, or a failure to program it. A modification whose delivery failed starts the next one only after its revert has its answer, since the revert is a PFCP exchange on the session's one response channel. The update is computed when the decision is applied, against what the session has committed by then, because a decision is a change to the policy in force (TS 29.512 subclause 4.2.6.1). The UE can answer before the transfer call returns, and a held decision can then start before that call has finished. Each modification now carries a generation, and the first one's tail leaves the session's state and timer alone once a later one has started. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A held decision started on a goroutine of its own, from the handler of the transaction that ended the modification before it. That transaction's state machine had not finished: HandleEvent applies the handler's returned state after the handler has released SMLock, and a modification started in between moved the session to SmStatePfcpModify only for HandleEvent to put it back to Active. The user plane's answer is delivered only in SmStatePfcpModify, so the held modification then waited for it for good. Work the SMF starts for a session can now be queued in that session's transaction queue as a SessionTask, which runs in the session's slot after whatever the session is doing and without a state machine of its own. A held decision is queued this way. The fsm package installs the queue at start, since the producer cannot import it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
…e's copy The UE answers every copy of a PDU SESSION MODIFICATION COMMAND it receives, and a network-requested Command carries the PTI value "No procedure transaction identity assigned" (TS 24.501 subclause 6.2.1), so the UE cannot recognise a retransmission and the SMF cannot tell which Command an answer belongs to. After a retransmitted Command was answered, a late answer to another copy of it could arrive once the next held decision had started: it stopped that procedure's T3591 and committed, or discarded, an update the UE had not answered. For assigned PTIs, TS 24.501 subclause 6.3.2.3 NOTE 5 has the UE keep the PTI for at least the default T3591 so that retransmissions stay distinguishable. This is the network-side counterpart for the unassigned one: once a procedure whose Command was retransmitted ends, no network-requested modification starts for one T3591 interval of that procedure. A decision arriving meanwhile is held, and the held ones start when the interval is over. An answer arriving within it finds nothing pending and changes nothing. A procedure whose Command was sent once has no copy to wait for and is not delayed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
The failure indication reads whether a modification is pending in one hold of SMLock and reverts it in another. T3591 can abandon the modification in between, and a held decision can start, which the revert then discarded as though it were the modification whose delivery had failed. The indication now reads the modification's generation with the pending flag, and the revert checks both and abandons in a single hold of the lock, doing nothing if a later modification owns the session or none is pending. The transfer-failure path in ApplyModification passes its own generation. abandonModification had no caller left after that, outside tests, and is removed; its tests abandon through abandonModificationLocked, which every abandonment path uses. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
The Modification Complete and Command Reject handlers acted on the current procedure without checking the PDU session identity the message carries, so an answer naming another session stopped this session's T3591 and committed or discarded its pending update. TS 24.501 subclause 7.3.2 d) has the network ignore a 5GSM message whose PDU session identity does not match an existing PDU session; both now compare it with this context's, as the release request already does, and ignore a mismatch before touching any procedure state. Raised in review of this PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A decision that deletes only rules carries no valid PCC rule, so the builder infers release-only and releases its fallback rule, "default". Since the deletion loop in this PR, a decision deleting the default rule had it marked for removal twice, by the loop and by the release-only branch, and each Remove PDR and Remove FAR went out twice in one request. The release-only branch now skips a rule the loop has already removed. Release-only also tears down the default rule when the decision deletes a different one. That predates this PR and is removed, with the NGAP side's inference, in a change of its own. Raised in review of this PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
…tted The Command's transfer re-discovers the AMF and retries against another one when a transfer gets no HTTP answer, and the transfer that got no answer may still have been delivered. The UE then held two copies of a Command the SMF counted as sent once, and answers both; the first answer could start the next held modification and the second then commit it. The quiet interval after a retransmitted Command did not apply, because T3591 had retransmitted nothing. SendN1N2TransferCountingSends is SendN1N2TransferWithRediscovery reporting how many transfers it sent, and the existing function, used by the other callers, wraps it. When the Command's transfer sent more than one, its procedure's end waits out one T3591 interval, as after a retransmission. The count is read from the function's returns by inspection; the test drives the Command through the real builder with the transfer reporting two sends. Raised in review of this PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
The held-decision tests waited 2 seconds for work the session runs asynchronously, as a queued task or after a quiet interval. Under the race detector on a loaded machine that was not enough: a held decision started after the test had given up and restored its stubs, failing the test and racing its cleanup. They now wait up to 10 seconds; the bound only decides how long a real failure takes to be reported. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
TS 29.512 subclause 4.2.3.2 NOTE: "When there is an ongoing procedure that collisions with the update of SM policies ... the SMF, based on operator policies, can delay the update of SM policies and return a "204 No Content" response code. In this case the SMF will process the request when the procedure is finished." A decision held behind a modification is exactly that case, and it was answered 200 as if it had been applied. A decision applied at once is answered as before. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A modification is pending from the moment it starts, but its Command reaches the AMF only after the user plane has answered. A Modification Complete or Command Reject arriving in that gap cannot be the new procedure's -- the UE has nothing of it to answer -- so it is a late answer to an earlier Command, a retransmitted copy or one AMF re-discovery delivered twice. Taken as the new procedure's, it ended that procedure and committed or discarded an update the UE was never sent. NwModificationUnsent is set when a modification starts and cleared in the hold of SMLock that hands its Command to the AMF, so the UE's answer, which needs the lock, is never judged in between. Both answers are ignored while it is set. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
Timer.Stop now waits for the timer's goroutine (omec-project#679), and documents that a caller must not hold a lock the callbacks take, nor call it from a callback. StopT3591 did both: every caller holds SMLock, which both T3591 callbacks take, and T3591's own abandonment reaches it from inside the cancellation callback. So every abandonment waited out Stop's five-second timeout on itself, and an acknowledgement that met a callback waiting for SMLock stalled the same way. Timer.Cancel is Stop without the wait, for exactly those callers, and Stop is now Cancel followed by the wait. StopT3591 cancels. A late callback is harmless because both check, under SMLock, that their timer is still the session's T3591, and StopT3591 drops the handle in the same hold. Raised in review of this PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A held decision is started as a session task when the modification before it ends. A notification that was already in the session's transaction queue runs before that task, and found nothing pending, so it was applied at once; the older held decision then ran after it and overwrote it with older policy. A decision is now held whenever others are held, not only while a modification is in progress or the quiet interval runs, and starting the oldest is requested there too in case nothing in progress will. A second request is harmless: the task that runs later finds a modification pending and stops. Raised in review of this PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A transfer that got no HTTP answer and could not be retried against another AMF returns an error after one send. That Command may still have reached the UE, whose answer can then arrive after the next modification has started and be taken as that one's. The quiet interval after a duplicated Command now applies here as well. A failure that proves nothing was delivered is not told apart: waiting needlessly only delays the next modification by one T3591 interval. Raised in review of this PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A replacement establishment purges the old context through RemoveSMContextLocked, as release does, but nothing there stopped T3591. The timer went on retransmitting the old context's Command -- built from its retained policy, to the UE's new session with the same PDU session ID -- and later abandoned it. Removal now cancels T3591 and drops its handle, so both callbacks find their timer superseded, and drops the decisions held for the session. Raised in review of this PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
The AMF sends a transfer failure notification only for a transfer that asked for one, and the only transfer that asks is the downlink data paging one: a modification's Command does not. So the notification is never the Command's delivery failure. Taken as one whenever a modification was pending, it reverted a modification on its way to the UE and skipped the paging failure's own handling. The AMF can produce that case itself: a Command that arrives while it is paging can end the paging and fail the paging transfer. The handler and its state machine step are back to what main has. A Command that never reaches the UE is left to T3591, and a transfer that fails outright is still reverted where it fails, now the revert's only cause. Tests that drove the revert through the notification call it directly. Raised in review of this PR. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
From review. BuildPfcpParam asked for the rules the pending update deletes inside the loop over data paths. The answer depends only on the update, so it is now read once, before the loop. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A modification is answered twice: the UE acknowledges the command, and the radio reports what it actually established. Only the UE's answer was read, so a radio that refused the modification, or built part of it, changed nothing. The session's record, the UE and the user plane then disagreed, and a downlink packet classified onto a flow the radio never built went to a QoS flow with no radio bearer and was dropped rather than falling back. The response transfer is decoded into the flows the radio established and the flows it refused, and the three shapes are answered differently. Everything established: nothing to do. Nothing established: the modification is abandoned, and the session settles back on the parameters it had. Some established: what the radio built is what the session has, so the pending update is pruned to those flows before it is committed -- committing the whole of it would record flows that do not exist -- and the refused ones are withdrawn from the UE and the user plane by an ordinary corrective modification. TS 23.502 subclause 4.3.3.2 calls for a further procedure rather than a retry of the one that was partly accepted, which is what a deletion through ApplyModification is. The two answers are unordered, and the order where the UE is first has to be handled too: the completion then commits the update whole, so the radio's answer keeps the committed update and builds the correction from that instead. Both refusal paths are covered -- the response naming every flow as refused, and the failure message refusing the modification in its entirety -- because they arrive at different handlers. A corrective modification asks for no release: the flows it withdraws are the ones the radio refused, which it never built, so asking it to release them names flows it does not have. Policy decisions held behind a modification (see the previous PR) wait for the correction too. A correction is committed to when the UE's completion finds a partial rejection, so the session is marked pending from there, before its goroutine starts anything: a notification arriving meanwhile is held, and the corrected modification's tail leaves the session to it. With nothing to correct, the completion starts the next held decision, as does the radio refusing the whole modification. The correction runs as a task in the session's queue, as a held decision does since the previous PR: started on a goroutine from the UE's completion, it could move the session to SmStatePfcpModify before that completion's state machine set Active again, after which the user plane's answer to the correction was never delivered. The radio's answer is part of the modification, so a held policy decision waits for it as well as for the UE's: the answer can still call for a correction after the UE has completed (TS 23.502 clause 4.3.3.2), and a modification started before it would discard what the completion kept for it. The wait is bounded by one T3591 interval from the UE's completion; a radio that never answers then stops holding decisions, and its correction, if one was owed, is given up as it was before. A correction after a Command that was retransmitted, or sent twice by AMF re-discovery, waits out the same interval a held decision does, and its Command counts as not yet sent while it waits, so a late answer to another copy of the corrected Command is ignored rather than taken as the correction's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
A PDU SESSION MODIFICATION REQUEST was decoded, logged at debug and dropped. The UE is left retransmitting a request nothing will ever answer, and TS 24.501 subclause 6.4.2 expects an answer either way -- so the session ends up in the one state the specification does not describe. The SMF does not implement UE-requested modification, and saying so is an answer: the request is refused with a MODIFICATION REJECT carrying the cause, attached to the UpdateSmContext response per TS 23.502 subclause 4.3.3.2 step 3a. The UE stops asking and knows why, instead of retrying into silence. One case is not refused. TS 24.501 subclause 6.3.2.5 item d: a UE request for a session the network is already modifying is disregarded, not rejected -- the network's own procedure carries on as if the request had never arrived, so no reject, no state change, and T3591 keeps running. Refusing there would answer a procedure the UE is entitled to have ignored, and the back-off that cause 32 asks for would be applied to a session that is being modified perfectly well. The shared activeSmContext fixture now carries an empty tunnel. An active session always has one, and since omec-project#659 a policy update refuses a session without one as being released, so this change's tests of the network-initiated procedure would otherwise stop at that refusal. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
midwell
force-pushed
the
split/ue-requested-modification
branch
from
October 14, 2026 12:00
abb921a to
7e4e226
Compare
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
1 participant
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.
Stacked on #668. This PR's own change is the last commit,
7e4e226; the commits below it belong to #668 and will drop out of this diff as it merges. Part of the four-PR split of #623.A PDU SESSION MODIFICATION REQUEST from the UE was decoded, logged at debug and dropped. The UE was left retransmitting a request nothing would ever answer, and TS 24.501 subclause 6.4.2 expects an answer either way.
The SMF does not implement UE-requested modification, and saying so is an answer. The request is refused with a PDU SESSION MODIFICATION REJECT carrying the cause, attached to the UpdateSMContext response (TS 23.502 subclause 4.3.3.2, step 3a). The UE stops asking and knows why, instead of retrying into silence.
One case is not refused. Under TS 24.501 subclause 6.3.2.5 d), a UE request for a session the network is already modifying is ignored, and the network's procedure proceeds "as if no PDU SESSION MODIFICATION REQUEST message was received from the UE". So there is no reject, no state change, and T3591 keeps running. Refusing there would answer a request the network is required to ignore, and would apply the back-off that cause 32 asks for to a session that is being modified correctly.
A known limitation inside that case. Sub-item i) of the same clause has the network act on the URSP rule enforcement reports IE of the request it otherwise ignores.
github.com/omec-project/nas/v2does not decode that IE:PDUSessionModificationRequesthas no field for it. So no report can reach the SMF, and only sub-item ii) is implemented here. Handling the reports needs the IE added to the NAS library first.The shared
activeSmContexttest fixture now carries an empty tunnel. An active session always has one, and since #659 a policy update refuses a session without one as being released. Four of this change's tests go through that path, so without the tunnel they stopped at the refusal:TestNetworkInitiatedModificationSendsTheCommandAndArmsT3591,TestNetworkProcedureMarksTheSessionSoAUeRequestCollidesandTestUeRequestDuringThePfcpRoundTripIsAlreadyACollisionfailed.TestFailedNetworkModificationDoesNotLeaveTheSessionLookingBusypassed for the wrong reason: its assertions hold under the refusal too, so its stubbed send was never reached. It is reached now; replacing that stub with a panic shows it.Verification. Each fix in this PR has a test that fails without it; every fix was reverted in turn to confirm its test catches it.
GOWORK=off pre-commit run --all-fileson darwin/arm64 with Go 1.26.0 at this commit, all nine hooks passing as pinned in.pre-commit-config.yaml:go test -race ./... -count=20,staticcheck,gci, golangci-lint v2.13.2, gitleaks v8.30.1, reuse v6.2.0, yamlfmt v0.21.0 and the whitespace hooks.🤖 Generated with Claude Code