You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat: the network-requested PDU session modification procedure - #667
This PR's change is twenty-six commits: e03f589 and 17ffda0, then fixes from Copilot's three reviews: a2b92d4, 7eae8da and d9a56f4; 2e4d32f, 9aca322 and 09d9258; 50cc561, dca35c0 and 25df40d; 2703b00, which gives the asynchronous tests a bound a loaded runner can meet; c10e539 and 070998d, which align the deferral with the design of record; e796994, f0122f1, bbef581 and dd21dd0 from Copilot's fourth review; 08eae11, which keeps a paging failure from being read as the modification's delivery failure; 3e53512 from Copilot's fifth review; 62d1516 from its sixth; 4b0bf40, d6719f8, 802d073 and c9b0f14 from its seventh; and 50a7223 from its eighth. It sits on main. Part of the four-PR split of #623.
When policy changes a running session, TS 23.502 subclause 4.3.3.2 and TS 24.501 subclause 6.3.2 have the SMF program the user plane, tell the UE what changed, and wait 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 SmStatePfcpModify, sends the N1 command and the N2 transfer together, and arms T3591.
The UE's Modification Complete 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 the update, with its own cause.
An unanswered command is retransmitted and abandoned when the retransmissions run out. The default is four, the count TS 24.501 subclause 6.3.2.5 a) specifies, and it can be configured. The pending update is discarded, the session settles back to Active so a later modification can be attempted, and the abandonment is 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 whose transfer fails is reverted, not left programmed. The update is discarded and the user plane is put back. If putting the user plane back itself fails, the session is marked for release instead: it would then be running parameters the UE was never told about.
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, because they are per session and shared with the rules that remain.
A retransmission is sent only while its procedure is current. Stopping T3591 does not wait for an expiry that is already due, so an acknowledgement arriving just as the timer fires could be followed by one more command for a procedure that had already ended (the race Copilot raised on #666). Abandonment already guarded against a superseded timer; retransmission now does too, checking under SMLock, both before the command is built and again before it is handed to the AMF, that the timer is still the session's T3591. The acknowledgement stops T3591 under the same lock, so the check is exact. The tests cannot compile against the unguarded send, since the guard is what they call; each check was instead removed in turn, and the tests fail without it.
Rebased twice: onto #664, then onto main after #666 and #659 merged. Where #664 and this PR both changed the failure path of ApplyModification, #664's guarantee is kept: a failed user-plane programming clears the pending UPF entry along with the state, and #664's test now asserts that directly. The second rebase had no conflicts, but two comments needed rewriting. #659's no-tunnel guard in HandleSMPolicyUpdateNotify, and #659's test of it, both said the guard had to be there because BuildPfcpParam, just below it, would dereference the tunnel while holding SMLock. In this PR the build has moved into ApplyModification, and both BuildPfcpParam and the send check for a missing tunnel. So without the guard, the modification fails at the send and its failure path undoes what it recorded, with the same answer. The guard stays as the cheaper refusal, and both comments now say so. With the guard removed, #659's no-tunnel tests still pass, which shows the second refusal holds by itself.
Rebased onto main at 1e759cd, without the merge. The previous push merged main into the branch (0d467a1), and GitHub cannot rebase a branch that carries a merge. The branch is linear on main again, and its content is the previous head's plus main's. The only conflicts were #686's interface{} to any rename, in two signatures this PR changes.
The radio's answer to the command is not handled here; that is the next PR.
A policy decision that arrives while the UE has not answered is held, and runs after the procedure ends (d9a56f4). Applying it at once replaced the pending update and stopped the first T3591. The UE's answer to the first Command, which carries no procedure transaction identity and so cannot say which Command it answers, then committed an update the UE was never sent. TS 24.501 subclause 6.3.2.5 has no network-against-network collision case, so the SMF runs one procedure at a time:
The held decision gets the same 200 as an applied one. Refusing it would lose the change, because the PCF does not resend.
Each held decision runs as a procedure of its own once the one before it ends: the UE completes or rejects it, T3591 abandons it, or programming it fails.
After a delivery failure, the next decision waits until the revert has its answer.
The update is computed when the decision is applied, against what is committed by then (TS 29.512 subclause 4.2.6.1).
Each modification carries a generation, so a procedure whose UE answered before its transfer call returned leaves the session's state and timer to the next one.
A revert that fails keeps its release mark (a2b92d4). The FSM handler returned Init for every outcome other than a successful revert, and HandleEvent applied that over the SmStatePfcpRelease the producer had just set. HandlePduSessN1N2TransFailInd now also reports whether the failure was a modification's. (Superseded by 08eae11: that failure is never a modification's.)
An abandoned modification stops its T3591 (7eae8da). The abandonment dropped the handle without stopping the timer. After a delivery failure, the timer then fired for its whole retransmission sequence: nothing was sent, but its goroutine ran on.
A held decision runs as a task in the session's queue (2e4d32f). Started on a goroutine from the transaction that ended its predecessor, it could move the session to SmStatePfcpModify before that transaction's state machine set Active again, after which the user plane's answer was never delivered. A session task is a transaction the SMF starts itself: it runs in the session's slot, after whatever the session is doing and before whatever arrives later.
No modification starts while the UE may still answer a copy of the last one (9aca322). The UE answers every copy of a Command it receives, and a network-requested Command carries no PTI, so a late answer to a retransmission cannot be told from an answer to the next Command. For assigned PTIs, TS 24.501 subclause 6.3.2.3 NOTE 5 has the UE keep the PTI for at least T3591, and this applies the network-side counterpart. After a procedure whose Command was retransmitted, decisions are held for one T3591 interval of that procedure. A Command sent once delays nothing.
A delivery failure reverts only the modification it belongs to (09d9258). The revert checks the generation its caller read, and abandons, in one hold of the lock, so a modification started in between cannot be discarded as though it were the one whose delivery failed.
A modification answer for another PDU session is ignored (50cc561). The Complete and Command Reject now check the PDU session identity they carry, as the release request does, and ignore a mismatch, which is what TS 24.501 subclause 7.3.2 d) asks.
A deleted rule is not removed twice in one request (dca35c0). When a decision deletes only the default rule, both the deletion loop and the release-only branch marked it. The release-only branch now skips a rule already removed.
A Command that AMF re-discovery sent twice is treated as retransmitted (25df40d). The transfer that got no HTTP answer may still have been delivered, so the UE can answer two copies. The procedure's end then waits out the same T3591 interval as after a retransmission.
A held decision is answered "204 No Content" (c10e539). TS 29.512 subclause 4.2.3.2 NOTE describes exactly this: an SMF with a colliding procedure in progress can delay the update of SM policies, answer 204, and process the request when the procedure is finished. A decision applied at once is answered as before.
An answer that arrives before the current Command was sent belongs to an earlier one (070998d). A modification is pending from the moment it starts, but its Command reaches the AMF only after the user plane has answered. A Complete or Command Reject in that gap is a late answer to an earlier Command, a retransmitted copy or one AMF re-discovery delivered twice, and it is ignored. Taken as the new procedure's answer, it committed or discarded an update the UE was never sent.
T3591 is cancelled, not stopped and waited for, under SMLock (e796994). Since #679, Timer.Stop waits for the timer's goroutine, and every caller of StopT3591 holds the lock both callbacks take. T3591's abandonment calls it as well, under the same lock. Timer.Cancel is Stop without the wait, and Stop is now Cancel followed by the wait. A late callback is harmless: both check, under SMLock, that their timer is still the session's T3591.
A newer policy decision does not overtake an older held one (f0122f1). A decision is held whenever others are held, not only while a modification is in progress, because the oldest is started as a session task that a notification already in the queue would otherwise run ahead of.
A failed transfer that was attempted counts as possibly delivered (bbef581), and opens the same quiet interval as a duplicated one. A transfer that failed before anything was sent does not.
Removing a session ends its network-requested modification (dd21dd0). Release and a replacement establishment both go through RemoveSMContextLocked, which now cancels T3591 and drops the held decisions.
The AMF's transfer failure notification is not taken as a modification's delivery failure (08eae11). The AMF sends it 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. Earlier commits here took it as the Command's delivery failure whenever a modification was pending. That reverted a modification on its way to the UE and skipped the paging failure's own handling. The notification is handled as main handles it again, except that a pending modification now keeps the session Active (802d073 below), and a Command that never reaches the UE is left to T3591.
A Command Reject with nothing pending is ignored (62d1516). A reject can answer a copy of a Command whose procedure has already ended, typically in the quiet interval after a retransmission. It went down the abandonment path: it was counted in smf_pdu_session_modification_abandoned_total and logged at error although nothing was abandoned, and it set the session back to Active from whatever state it had moved on to. A Complete in the same position already changed nothing, having nothing to commit.
A transaction takes the session's slot when it finds it free (4b0bf40). Two transactions arriving together for an idle session could both run. This is a gap in main's transaction state machine, which every transaction goes through: TxnCtxtPost found the slot free and let the transaction run, but only TxnCtxtRun took it, and TxnEnd freed it before the queued transaction it started had taken it. A held decision is queued as a session task when the quiet interval ends, on a timer, so it is one more way to meet the gap. Run beside an update from the AMF, its modification could have the session put back to Active while it waited for the user plane, whose answer is delivered only in SmStatePfcpModify. The slot is now taken in the hold of the lock that finds it free, and handed straight to the next queued transaction. The change is self-contained in fsm/txnFsm.go. Its tests drive the stages in the order that exposes the gap rather than racing goroutines.
A modification answer whose PTI is not the Command's is ignored (d6719f8). The Complete and the Command Reject checked the PDU session identity but not the PTI, so an answer carrying another one could stop T3591 and commit or discard the pending update. Every Command this SMF sends carries the core-initiated PTI, and the UE echoes the Command's. TS 24.501 subclause 7.3.1 a) has such an answer refused with 5GSM STATUS cause 47, which this SMF cannot send, so it is ignored.
A paging failure leaves a pending modification answerable (802d073). The state machine returned SmStateInit after every paging failure, over whatever state the producer had left. The AMF reports a paging failure when its paging gives up, which says nothing about a Command already sent: T3591 retransmits it and the UE can still answer. In SmStateInit that answer met EmptyEventHandler, and the modification was abandoned although the UE had applied it. With a modification pending and the session Active, the session now stays Active. A paging failure with nothing pending, or one that started a release after a PFCP timeout, still ends in SmStateInit, as on main.
An earlier version of this description, and the comment on HandlePduSessN1N2TransFailInd, said that a Command arriving while the AMF pages can end the paging and fail it. That is wrong for this AMF: it answers such a Command 409 unless it carries a paging priority, and the SMF sets none. 802d073 corrects the comment. 08eae11's change does not rest on it: the notification answers only the paging transfer, never the Command.
The T3591 retransmission test no longer races its own timer (c9b0f14). Its stub returned at once, so on a delayed runner the 10 ms timer could abandon the procedure before the checks were made, and panic on the fixture. The stub now waits for the checks, and the timer is joined before the stub is restored.
T3591's abandonment runs in the session's queue (50a7223). Its last expiry abandoned on the timer's goroutine, outside the transaction queue, and put the session back to Active. A transaction in the slot that waits for the user plane's answer without holding SMLock is a paging failure's own modification: it waits in SmStatePfcpModify, and that answer is delivered only in that state, so the transaction and everything queued behind it waited for good. The N1 and N2 handlers hold SMLock across their waits, so the abandonment simply queued behind them. The abandonment is now queued as a session task, as held decisions are, and the check that the expiring timer is still the session's runs inside the task, so an expiry superseded while it waited does nothing. TestT3591AbandonsInTheSessionSlot fails against c9b0f14's callback.go (nothing is queued, and it times out after 10 s) and passes with the change. It pins the queueing, not the hang: it stubs the queue and does not park a real transaction in SmStatePfcpModify.
With T3591 disabled by configuration and a UE that never answers, the procedure stays pending, as disabling the timer asks, so decisions held behind it accumulate.
An in-flight procedure and its held decisions do not survive an SMF restart. The pending update is persisted but the procedure that owns it is not, and held decisions, already answered 200, are lost. A follow-up change restores or abandons the procedure on restore, and keeps held decisions durably.
Release-only is still inferred from a decision with no valid rules, so deleting one dedicated rule also tears down the default rule. That predates this PR, and it is removed with the NGAP side's inference in a change of its own.
A Complete or Reject that arrives with nothing pending, or with a PTI the Command did not carry, is ignored. TS 24.501 subclause 7.3.1 a) has the network answer it with 5GSM STATUS cause 47, which needs a STATUS message path the SMF does not have.
A PDU Session Release Complete is checked for neither its PDU session identity nor its PTI, which TS 24.501 subclause 7.3.1 a) also covers. That predates this PR.
The PTI check holds because the SMF never answers a UE request with a Modification Command. If it ever does, the expected PTI has to be the one of the procedure in progress.
After a paging failure with no modification pending, the session is left in SmStateInit, where an update for it is not handled, as on main.
A panic inside a transaction leaves the session's slot held, so later transactions for it queue for good. That predates this PR.
T3591's abandonment now waits for the session's slot, so a slow transaction in it delays the abandonment, and with it the next held decision, by as long as that transaction takes. The UE's answer in the meantime is handled normally.
A UPF-restoration sweep (markReleasedAndBuild) moves a session to SmStateRelease from its own goroutine, outside the slot, which could strand a paging failure's transaction the same way. It needs a UPF restart concurrent with a paging failure, and predates this PR.
A UE-requested release and a user-plane re-establishment do not abort a pending network modification, which TS 24.501 subclause 6.3.2.5 items c and g ask for. Held decisions are dropped if the session is no longer active when they come to run.
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-files on 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.
Rebased onto main at 29dea54 (own commits 646bead and 29dea54), with #668 and #669 re-landed on it. No conflicts. Two comments, one in the code and one in its test, needed rewriting, because this PR changes the premise #659 wrote them on. That is in the description, under Rebased twice.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Rollback and abandonment can leave the UPF divergent or blocked, overlapping modifications can corrupt pending policy, and failed-revert state is overwritten.
Implements the end-to-end network-requested PDU session modification procedure, including UE acknowledgement, T3591 retransmission, abandonment, and delivery-failure handling.
Changes:
Adds modification completion/rejection handling and T3591 lifecycle management.
Adds PFCP rule withdrawal and attempted rollback on delivery failure.
Expands FSM routing and tests for failure, timeout, and race scenarios.
File
Description
producer/callback.go
Implements modification, retransmission, abandonment, and rollback flows.
producer/pdu_session.go
Handles delivery failures and in-flight acknowledgements.
producer/n1n2_data_handler.go
Processes modification completion and rejection messages.
fsm/handler.go
Routes modification events and preserves post-failure state.
context/sm_context.go
Adds modification state, timer data, and locked policy commits.
producer/t3591_retransmission_test.go
Tests retransmission race guards.
producer/modification_timeout_test.go
Verifies pending-UPF cleanup after failures.
producer/modification_not_sent_test.go
Updates no-tunnel test documentation.
producer/modification_failure_test.go
Tests rollback, failure, and rule withdrawal behavior.
producer/abandon_modification_test.go
Tests abandonment and timer supersession.
context/commit_policy_test.go
Tests policy commit safety and transient-state persistence.
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>
A reject answering a copy of a Command whose procedure has already ended,
typically in the quiet interval after a retransmission, fell through to
the abandonment path: it reported an abandonment that did not happen and
put the session back to Active from whatever state it had moved on to.
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>
Retransmission stub causes premature timer expiry and panic
producer/t3591_retransmission_test.go:69
The retransmission stub returns immediately, so T3591 can reach its final expiry after only 50 ms. If a loaded runner delays the test goroutine after the first callback, abandonment enters Active with this fixture's nil PDUAddress and panics on the timer goroutine. It can also make the running assertion fail before the test stops the timer. Block the callback until the assertions finish, then cancel, unblock, and join the timer before restoring the stub.
Two transactions arriving together for an idle session could both run.
TxnCtxtPost found the slot free and let the transaction run, but the
transaction took the slot only later, in TxnCtxtRun; and TxnEnd freed the
slot before the queued transaction it started had taken it, with the same
result for one arriving in between. The gap is in the state machine every
transaction goes through.
A held decision is queued as a session task when the quiet interval ends,
on a timer rather than behind another transaction, so it can meet an
update from the AMF on an idle session. Run together, the update could
put the session back to Active while the decision's modification waited
for the user plane, whose answer is delivered only in SmStatePfcpModify.
The slot is now taken in the hold of the lock that finds it free, and
handed straight from a transaction that ends to the one queued behind it.
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 Complete and the Command Reject checked the PDU session identity they
carry but not the procedure transaction identity. Every Command the
network sends carries the core-initiated PTI, so an answer with another
one answers no procedure the network is running, yet it could stop T3591
and commit or discard the pending update. TS 24.501 subclause 7.3.1 a)
has it refused with 5GSM STATUS cause 47; the SMF has no STATUS path, so
it is ignored, as an answer for another PDU session already is.
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 state machine moved a session to SmStateInit after a paging failure,
whatever the producer had left it in. The AMF reports a paging failure
when its paging gives up, which says nothing about a Command already
sent: T3591 retransmits it, and the UE's answer can still come. In
SmStateInit that Complete or Command Reject met EmptyEventHandler while
T3591 ran on, and the modification was abandoned although the UE had
applied it.
With a network-requested modification pending and the session still
Active, the paging failure now leaves it Active. A paging failure that
started a release, or one with no modification pending, still ends in
SmStateInit as before.
The doc comment on HandlePduSessN1N2TransFailInd, and the comment on
the test for it, also lose a mechanism they claimed: that a Command
arriving while the AMF pages can end the paging and fail it. The AMF
answers such a Command 409 unless it carries a paging priority, and the
SMF sets none.
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 stub retransmission returned at once, so the 10 ms timer ran on. A
runner that delayed the test by a few expiries saw T3591 abandon the
procedure first: the abandonment moved the fixture to Active and
panicked on its nil PDU address on the timer's goroutine, or the check
read false while it should still have held.
The stub now waits until the checks are made, and the timer is stopped
and joined before the stub is restored.
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>
Copilot's latest review also flagged, in its summary only, that TestAT3591ExpiryRetransmitsOnlyWhileItsTimerIsCurrent races its own timer. Fixed in c9b0f14: the stub now waits until the checks are made, and the timer is stopped and joined before the stub is restored. With a 100 ms stall added after the first expiry, the old test panicked on the fixture's nil PDU address as described, and the new one passes.
T3591's last expiry abandoned the modification on the timer's goroutine,
outside the session's transaction queue. The abandonment puts the session
back to Active, and a transaction in the session's slot can be waiting
for the user plane's answer in SmStatePfcpModify: a paging failure's own
modification, for one. That answer is delivered only in
SmStatePfcpModify, so the transaction, and everything queued behind it,
waited for good.
The abandonment is now queued as a session task, as held decisions are.
The check that the expiring timer is still the session's T3591 runs
inside the task, so an expiry superseded while it waited stays harmless.
The comments that placed the abandonment on the timer's goroutine, and
that had StopT3591 called from inside its cancellation callback, now
describe the queue.
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>
midwell
added a commit
to midwell/smf
that referenced
this pull request
Oct 15, 2026
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
midwell
added a commit
to midwell/smf
that referenced
this pull request
Oct 15, 2026
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
midwell
added a commit
to midwell/smf
that referenced
this pull request
Oct 16, 2026
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
midwell
added a commit
to midwell/smf
that referenced
this pull request
Oct 16, 2026
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
midwell
added a commit
to midwell/smf
that referenced
this pull request
Oct 16, 2026
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
midwell
added a commit
to midwell/smf
that referenced
this pull request
Oct 16, 2026
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
midwell
added a commit
to midwell/smf
that referenced
this pull request
Oct 16, 2026
The previous commit has every queued transaction wait for an owed
revert before processing. That is a check followed, some frames later,
by the handler's own PFCP exchange, so a revert that became owed in
between, from outside the queue, would not be seen, and the two
exchanges would wait on the session's single, uncorrelated response
channel at once.
Nothing sets one from outside the queue. T3591's last expiry (queued
since the eighth review of omec-project#667) and the corrective modification after
a partial rejection (queued since omec-project#668) already run as session tasks,
and the NAS and NGAP handlers already run inside a transaction. This
commit changes no behaviour: it says so in the comments that described
the old arrangement, and adds two tests that pin it.
Raised in review of this PR.
The two-rule fixture's uplink session AMBR is now 150 Mbps rather than
100. Nothing reads the value, but the T3591 retransmission tests this
is stacked on use "100 Mbps" twice, and a third occurrence trips
goconst.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>
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
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.
This PR's change is twenty-six commits:
e03f589and17ffda0, then fixes from Copilot's three reviews:a2b92d4,7eae8daandd9a56f4;2e4d32f,9aca322and09d9258;50cc561,dca35c0and25df40d;2703b00, which gives the asynchronous tests a bound a loaded runner can meet;c10e539and070998d, which align the deferral with the design of record;e796994,f0122f1,bbef581anddd21dd0from Copilot's fourth review;08eae11, which keeps a paging failure from being read as the modification's delivery failure;3e53512from Copilot's fifth review;62d1516from its sixth;4b0bf40,d6719f8,802d073andc9b0f14from its seventh; and50a7223from its eighth. It sits onmain. Part of the four-PR split of #623.When policy changes a running session, TS 23.502 subclause 4.3.3.2 and TS 24.501 subclause 6.3.2 have the SMF program the user plane, tell the UE what changed, and wait 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.
ApplyModificationprograms the user plane, moves the session intoSmStatePfcpModify, sends the N1 command and the N2 transfer together, and arms T3591.An unanswered command is retransmitted and abandoned when the retransmissions run out. The default is four, the count TS 24.501 subclause 6.3.2.5 a) specifies, and it can be configured. The pending update is discarded, the session settles back to Active so a later modification can be attempted, and the abandonment is 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 whose transfer fails is reverted, not left programmed. The update is discarded and the user plane is put back. If putting the user plane back itself fails, the session is marked for release instead: it would then be running parameters the UE was never told about.
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, because they are per session and shared with the rules that remain.
A retransmission is sent only while its procedure is current. Stopping T3591 does not wait for an expiry that is already due, so an acknowledgement arriving just as the timer fires could be followed by one more command for a procedure that had already ended (the race Copilot raised on #666). Abandonment already guarded against a superseded timer; retransmission now does too, checking under
SMLock, both before the command is built and again before it is handed to the AMF, that the timer is still the session's T3591. The acknowledgement stops T3591 under the same lock, so the check is exact. The tests cannot compile against the unguarded send, since the guard is what they call; each check was instead removed in turn, and the tests fail without it.Rebased twice: onto #664, then onto
mainafter #666 and #659 merged. Where #664 and this PR both changed the failure path ofApplyModification, #664's guarantee is kept: a failed user-plane programming clears the pending UPF entry along with the state, and #664's test now asserts that directly. The second rebase had no conflicts, but two comments needed rewriting. #659's no-tunnel guard inHandleSMPolicyUpdateNotify, and #659's test of it, both said the guard had to be there becauseBuildPfcpParam, just below it, would dereference the tunnel while holdingSMLock. In this PR the build has moved intoApplyModification, and bothBuildPfcpParamand the send check for a missing tunnel. So without the guard, the modification fails at the send and its failure path undoes what it recorded, with the same answer. The guard stays as the cheaper refusal, and both comments now say so. With the guard removed, #659's no-tunnel tests still pass, which shows the second refusal holds by itself.Rebased onto
mainat1e759cd, without the merge. The previous push mergedmaininto the branch (0d467a1), and GitHub cannot rebase a branch that carries a merge. The branch is linear onmainagain, and its content is the previous head's plusmain's. The only conflicts were #686'sinterface{}toanyrename, in two signatures this PR changes.The radio's answer to the command is not handled here; that is the next PR.
A policy decision that arrives while the UE has not answered is held, and runs after the procedure ends (
d9a56f4). Applying it at once replaced the pending update and stopped the first T3591. The UE's answer to the first Command, which carries no procedure transaction identity and so cannot say which Command it answers, then committed an update the UE was never sent. TS 24.501 subclause 6.3.2.5 has no network-against-network collision case, so the SMF runs one procedure at a time:A revert that fails keeps its release mark (
a2b92d4). The FSM handler returned Init for every outcome other than a successful revert, andHandleEventapplied that over theSmStatePfcpReleasethe producer had just set.HandlePduSessN1N2TransFailIndnow also reports whether the failure was a modification's. (Superseded by08eae11: that failure is never a modification's.)An abandoned modification stops its T3591 (
7eae8da). The abandonment dropped the handle without stopping the timer. After a delivery failure, the timer then fired for its whole retransmission sequence: nothing was sent, but its goroutine ran on.A held decision runs as a task in the session's queue (
2e4d32f). Started on a goroutine from the transaction that ended its predecessor, it could move the session toSmStatePfcpModifybefore that transaction's state machine set Active again, after which the user plane's answer was never delivered. A session task is a transaction the SMF starts itself: it runs in the session's slot, after whatever the session is doing and before whatever arrives later.No modification starts while the UE may still answer a copy of the last one (
9aca322). The UE answers every copy of a Command it receives, and a network-requested Command carries no PTI, so a late answer to a retransmission cannot be told from an answer to the next Command. For assigned PTIs, TS 24.501 subclause 6.3.2.3 NOTE 5 has the UE keep the PTI for at least T3591, and this applies the network-side counterpart. After a procedure whose Command was retransmitted, decisions are held for one T3591 interval of that procedure. A Command sent once delays nothing.A delivery failure reverts only the modification it belongs to (
09d9258). The revert checks the generation its caller read, and abandons, in one hold of the lock, so a modification started in between cannot be discarded as though it were the one whose delivery failed.A modification answer for another PDU session is ignored (
50cc561). The Complete and Command Reject now check the PDU session identity they carry, as the release request does, and ignore a mismatch, which is what TS 24.501 subclause 7.3.2 d) asks.A deleted rule is not removed twice in one request (
dca35c0). When a decision deletes only the default rule, both the deletion loop and the release-only branch marked it. The release-only branch now skips a rule already removed.A Command that AMF re-discovery sent twice is treated as retransmitted (
25df40d). The transfer that got no HTTP answer may still have been delivered, so the UE can answer two copies. The procedure's end then waits out the same T3591 interval as after a retransmission.A held decision is answered "204 No Content" (
c10e539). TS 29.512 subclause 4.2.3.2 NOTE describes exactly this: an SMF with a colliding procedure in progress can delay the update of SM policies, answer 204, and process the request when the procedure is finished. A decision applied at once is answered as before.An answer that arrives before the current Command was sent belongs to an earlier one (
070998d). A modification is pending from the moment it starts, but its Command reaches the AMF only after the user plane has answered. A Complete or Command Reject in that gap is a late answer to an earlier Command, a retransmitted copy or one AMF re-discovery delivered twice, and it is ignored. Taken as the new procedure's answer, it committed or discarded an update the UE was never sent.T3591 is cancelled, not stopped and waited for, under
SMLock(e796994). Since #679,Timer.Stopwaits for the timer's goroutine, and every caller ofStopT3591holds the lock both callbacks take. T3591's abandonment calls it as well, under the same lock.Timer.CancelisStopwithout the wait, andStopis nowCancelfollowed by the wait. A late callback is harmless: both check, underSMLock, that their timer is still the session's T3591.A newer policy decision does not overtake an older held one (
f0122f1). A decision is held whenever others are held, not only while a modification is in progress, because the oldest is started as a session task that a notification already in the queue would otherwise run ahead of.A failed transfer that was attempted counts as possibly delivered (
bbef581), and opens the same quiet interval as a duplicated one. A transfer that failed before anything was sent does not.Removing a session ends its network-requested modification (
dd21dd0). Release and a replacement establishment both go throughRemoveSMContextLocked, which now cancels T3591 and drops the held decisions.The AMF's transfer failure notification is not taken as a modification's delivery failure (
08eae11). The AMF sends it 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. Earlier commits here took it as the Command's delivery failure whenever a modification was pending. That reverted a modification on its way to the UE and skipped the paging failure's own handling. The notification is handled asmainhandles it again, except that a pending modification now keeps the session Active (802d073below), and a Command that never reaches the UE is left to T3591.A Command Reject with nothing pending is ignored (
62d1516). A reject can answer a copy of a Command whose procedure has already ended, typically in the quiet interval after a retransmission. It went down the abandonment path: it was counted insmf_pdu_session_modification_abandoned_totaland logged at error although nothing was abandoned, and it set the session back to Active from whatever state it had moved on to. A Complete in the same position already changed nothing, having nothing to commit.A transaction takes the session's slot when it finds it free (
4b0bf40). Two transactions arriving together for an idle session could both run. This is a gap inmain's transaction state machine, which every transaction goes through:TxnCtxtPostfound the slot free and let the transaction run, but onlyTxnCtxtRuntook it, andTxnEndfreed it before the queued transaction it started had taken it. A held decision is queued as a session task when the quiet interval ends, on a timer, so it is one more way to meet the gap. Run beside an update from the AMF, its modification could have the session put back to Active while it waited for the user plane, whose answer is delivered only inSmStatePfcpModify. The slot is now taken in the hold of the lock that finds it free, and handed straight to the next queued transaction. The change is self-contained infsm/txnFsm.go. Its tests drive the stages in the order that exposes the gap rather than racing goroutines.A modification answer whose PTI is not the Command's is ignored (
d6719f8). The Complete and the Command Reject checked the PDU session identity but not the PTI, so an answer carrying another one could stop T3591 and commit or discard the pending update. Every Command this SMF sends carries the core-initiated PTI, and the UE echoes the Command's. TS 24.501 subclause 7.3.1 a) has such an answer refused with 5GSM STATUS cause 47, which this SMF cannot send, so it is ignored.A paging failure leaves a pending modification answerable (
802d073). The state machine returnedSmStateInitafter every paging failure, over whatever state the producer had left. The AMF reports a paging failure when its paging gives up, which says nothing about a Command already sent: T3591 retransmits it and the UE can still answer. InSmStateInitthat answer metEmptyEventHandler, and the modification was abandoned although the UE had applied it. With a modification pending and the session Active, the session now stays Active. A paging failure with nothing pending, or one that started a release after a PFCP timeout, still ends inSmStateInit, as onmain.An earlier version of this description, and the comment on
HandlePduSessN1N2TransFailInd, said that a Command arriving while the AMF pages can end the paging and fail it. That is wrong for this AMF: it answers such a Command 409 unless it carries a paging priority, and the SMF sets none.802d073corrects the comment.08eae11's change does not rest on it: the notification answers only the paging transfer, never the Command.The T3591 retransmission test no longer races its own timer (
c9b0f14). Its stub returned at once, so on a delayed runner the 10 ms timer could abandon the procedure before the checks were made, and panic on the fixture. The stub now waits for the checks, and the timer is joined before the stub is restored.T3591's abandonment runs in the session's queue (
50a7223). Its last expiry abandoned on the timer's goroutine, outside the transaction queue, and put the session back to Active. A transaction in the slot that waits for the user plane's answer without holdingSMLockis a paging failure's own modification: it waits inSmStatePfcpModify, and that answer is delivered only in that state, so the transaction and everything queued behind it waited for good. The N1 and N2 handlers holdSMLockacross their waits, so the abandonment simply queued behind them. The abandonment is now queued as a session task, as held decisions are, and the check that the expiring timer is still the session's runs inside the task, so an expiry superseded while it waited does nothing.TestT3591AbandonsInTheSessionSlotfails againstc9b0f14'scallback.go(nothing is queued, and it times out after 10 s) and passes with the change. It pins the queueing, not the hang: it stubs the queue and does not park a real transaction inSmStatePfcpModify.Not in this PR:
92f227e). Here, T3591 expiry, the UE's reject and a radio refusal leave the user plane programmed, and the revert after a delivery failure sends an empty modification. The two PRs need to merge together.SmStateInit, where an update for it is not handled, as onmain.markReleasedAndBuild) moves a session toSmStateReleasefrom its own goroutine, outside the slot, which could strand a paging failure's transaction the same way. It needs a UPF restart concurrent with a paging failure, and predates this PR.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