feat: complete the PDU session modification procedure (refusal, T3591, RAN answer, realignment)

[midwell]

What

Completes the PDU session modification procedure in the SMF:

• A UE-requested modification is answered instead of dropped, and a colliding one is disregarded.
• A network-requested modification is retransmitted and abandoned on a timer (T3591).
• The radio access network's answer is consumed, including when it establishes some flows and refuses others.
• A modification the UE never acknowledged is discarded rather than recorded.
• A guaranteed bit rate configured in one direction only is honoured rather than silently dropped.

Why

producer/n1n2_data_handler.go dispatched only PDUSessionReleaseRequest and PDUSessionReleaseComplete, with no default branch. A PDU SESSION MODIFICATION REQUEST (0xC9) was decoded, debug-logged and dropped: the SMF answered UpdateSmContext 200 with no N1 or N2 content, the UE retransmitted until T3581 expired, and the procedure was abandoned. In a packet capture that reads as a UE fault, which is why it went unnoticed for a long time.

The network-requested direction had a matching gap. The command was sent once; nothing retransmitted it and nothing gave up on it. On a satellite link, where a fade routinely outlasts several seconds, an unacknowledged modification left the SMF recording parameters the UE was never running.

Verified against a real gNB stack

Not only unit-tested. Run end to end on a single-node cluster with gnbsim over NGAP:

Case	Result
Network-requested modification	Command delivered, gNB response decoded at +22 ms, UE acknowledged at +204 ms, T3591 stopped
Unacknowledged (fade)	Four retransmissions at exactly 16.0 s, abandoned on the fifth expiry, session kept its previous parameters, no release to the UPF
Whole rejection by the gNB	Abandoned with path=ran_whole_rejection, timer stopped, session left active
Partial rejection	Two flows requested, one refused; classified as partial, corrective modification sent withdrawing the refused flow; gNB saw two modify requests, UE two commands
Collision	UE request arriving during the network's own procedure disregarded per TS 24.501 §6.3.2.5 item d; no reject, network procedure untouched

Timer selection and abandonment are observable: smf_nas_timer_resolution_total{timer,source,value} and smf_pdu_session_modification_abandoned_total{path,cause}.

Three things worth explaining up front

These are the questions I would ask reading this.

Why refuse a UE-requested modification instead of implementing it

Implementing the UE-initiated procedure means policy evaluation of UE-proposed QoS rules, an SM policy association update toward the PCF (consumer/sm_policy.go has only create and delete), and an authorization model for what a UE may ask for. That is a much larger change and is not what unblocks anything here.

Refusing is a complete, conformant behaviour rather than a placeholder. TS 24.501 §6.4.2.4 specifies a MODIFICATION REJECT for a request the SMF does not accept, and the UE has a defined response to it. Decoding the message and dropping it is the only option that is not conformant.

Why 5GSM cause 32 and not 31

Both are permitted by §6.4.2.4.1, and the difference is what the UE does next. Under §6.4.2.4.3 item a, 32 "service option not supported" makes a conformant UE start a back-off timer without the network sending a Back-off timer value IE — its configured SM Retry Timer, or twelve minutes. 31 falls under item b, where back-off depends on an explicit IE, so choosing it without one replaces a silent drop with a fast rejection loop.

The back-off is modification-scoped and does not suppress session establishment, so a terminal that loses its session inside the window re-establishes normally. Congestion-control causes (26, 67, 69) do suppress establishment, which is a further reason not to reach for those.

The honest trade: with 32 the UE chooses the back-off duration, not the network. 31 with an explicit IE would give the network that control. 32 is kept because the semantics are the durable part — the network is declining a capability, not deferring a request.

One implementation note: github.com/omec-project/nas/v2 has no constant for 32. Its Cause5GSM list runs from 31 (0x1f) straight to 34 (0x22), so 32 and 33 are unnamed despite both being assigned in table 9.11.4.2.1. It is defined in smferrors with that citation rather than left a bare literal. Happy to send it to the NAS repository instead.

Why T3591 abandons to the previous configuration rather than releasing

§6.3.2.5 item a permits either: on the fifth expiry the SMF "may continue to use the previous configuration of the PDU session or initiate the network-requested PDU session release procedure".

Continuing is chosen because on a satellite link abandonment is an ordinary outcome — a fade can outlast the whole retransmission sequence. Releasing would turn a QoS change that did not land into a dropped session for a site that is working. The cost is that nothing re-drives the change, which is why abandonment is counted and logged at error level rather than being a debug line.

Prerequisite: the ngap optional-tag fix

Resolved. PDUSessionResourceModifyResponseTransfer generated the DL and UL NG-U UP TNL Information fields without the aper optional tag, so the codec treated them as always present and the SMF could not decode a QoS-only modification response from a conformant gNB — the ordinary case for this procedure. Fixed in omec-project/ngap#118 and released in v2.1.4, which this PR now requires. The two tests that documented the broken state are replaced by one that encodes with a conformant peer's shape and decodes with the shipped type, in both the tunnel-omitted and tunnel-present forms; it fails against v2.1.3 and passes from v2.1.4.

PDUSessionResourceModifyResponseTransfer is generated with DL and UL NG-U UP TNL Information lacking the aper optional tag, although TS 38.413 makes both OPTIONAL. A modification that changes QoS without moving the tunnel — the ordinary case, and what this change produces — has the gNB omit them. Encoder and decoder here are consistent with each other, so every self-test passes; a conformant peer's three-byte message is rejected with align Bit is not zero.

The defect is systematic: across ngapType, 674 non-CHOICE pointer fields carry the tag and all 30 pointer-to-CHOICE fields inside SEQUENCEs carry none, with no counter-example either way. HandoverCommandTransfer, HandoverRequestAcknowledgeTransfer and PathSwitchRequestAcknowledgeTransfer are affected too, so handover and path switch have the same latent failure.

The compatibility direction is counter-intuitive and matters for sequencing: against a real gNB the fix makes the SMF more interoperable; against an unpatched omec-based peer it makes it less so, because gnbsim and SD-Core currently interwork on those procedures precisely by being wrong in the same way. PDUSessionResourceSetupResponseTransfer is not among the affected types, so registration and session establishment are unaffected either way.

There is a canary test in context/ngap_modify_handler_test.go that builds a conformant peer's message and fails once the dependency is fixed, telling you to drop it.

Incidental fixes

Adding callers to code that previously had none surfaced five latent panics and one silent data loss. Each is upstream code; what this change contributes is a path that reaches it. The pattern is consistent enough to state once rather than five times: this codebase assumes an establishment flow, and every path that reaches the same code from a modification finds something that was never nil before.

• CommitSmPolicyDecision indexed SmPolicyUpdates[0] unguarded. A retransmitted MODIFICATION COMPLETE arriving with nothing pending would have taken the SMF down. This one fired in production during testing, on the guard.
• BuildPfcpParam dereferenced smContext.Tunnel without a nil check.
• The AMF-derived Timer this branch ports panicked when stopped twice, despite a comment saying it would hang. A timer stopped by an incoming message can race an abort, so Stop is idempotent here.
• Four sites in context/datapath.go index SmPolicyUpdates[0] unguarded, and reverting an undelivered modification reaches all four, because it discards the pending update and then rebuilds the user plane. Every revert would have crashed.
• CreatePccRuleQer dereferenced SelectedSessionRule().AuthSessAmbr, which is nil when a modification adds a PCC rule without touching session rules — the ordinary case for an application function adding a flow mid-session.
• CommitSessionRulesUpdate cleared the session's active rule on every modification that carried session rules. GetSessionRulesUpdate names an active rule only when the rule is new, so on a modification it names none, and the commit assigned that nil over the committed one. Establishment set it, the first modification wiped it, and anything afterwards needing the session AMBR from committed state found nothing. Two of the failures above trace to this single line.

Two further corrections on the modification path itself:

• The NGAP modify request carried exactly one QoS flow, taken from the session's default flow indication, so a modification adding dedicated flows asked the radio about none of them — the UE was told about every flow and the radio about one. It now iterates the policy delta, as the establishment path in the same file always has.
• HandlePduSessN1N2TransFailInd is shared with establishment and drops the downlink data path. Right for a session that was never usable; wrong for one that was working before a change nobody could deliver. A modification is now reverted instead.

Found reviewing this branch, after it was working

Each of these was in code this change adds, none was caught by the tests already here, and two are
on the corrective path a partial rejection takes — the path with the least prior exercise. They are
listed separately from the incidental fixes above because those are upstream code this change
merely reaches, and these are mine.

The corrective modification asked the radio to modify the default flow. It carries only
deletions, so it produced no add-or-modify items and fell into the fallback written for a
modification that names no flows. Those are opposite cases: an update naming nothing wants the
radio told about the default flow; an update naming only deletions wants the radio told nothing,
because the flows it withdraws were never established there. The cost was a radio reconfiguration
per partial rejection to re-assert a flow nobody asked about.

A T3591 expiry already in flight could abandon the modification that had superseded it. Stopping
a timer cannot recall an expiry; the abort takes SMLock and queues behind whatever holds it, which
is most likely the acknowledgement that just finished the procedure — and it then discards whatever
is pending by then, which after a partial rejection is the corrective started in that window. The
window is narrow and it is exactly the satellite case: a UE acknowledging at the fifth expiry after
a long fade. The abort now checks it is still the session's timer.

ApplyModification left the session in SmStatePfcpModify with an uncommitted update when the
user plane refused. Upstream's path had the same gap, but this is a new function and the corrective
modification reaches it, so its failure contract is ours: it now puts both back.

revertModification claimed to release the session when the user plane could not be restored,
and only assigned a state label that nothing acts on. Releasing properly means releaseTunnel plus a
read of SBIPFCPCommunicationChan plus RemoveSMContext plus notifying the AMF, and this runs on a
background goroutine — a sixth reader of that single-slot channel could consume another
transaction's response while cleaning up after a rare double failure, which is worse than the
divergence it repairs. It now reports what it actually does and says why it stops there.

Known limitations

Collision sub-item i is unreachable. TS 24.501 §6.3.2.5 item d has the network consume the URSP rule enforcement reports IE from a colliding request before ignoring the rest. nas/v2 does not decode that IE — no field on the message, no mention of URSP in the module — so sub-item ii is the whole of the reachable behaviour.

Upstream's single-slot pending update is untouched. SmPolicyUpdates is replaced in place, so a modification starting while another is uncommitted overwrites it. This change consumes that slot more than before but does not restructure it.

Two concurrent PFCP operations on one session still race. SBIPFCPCommunicationChan is a single-slot rendezvous with five existing readers; the corrective modification adds a sixth caller that can overlap a transaction for about the 50 ms its exchange takes. It cannot be closed by holding SMLock across the exchange, because BuildAndSendQosN1N2TransferMsg takes that lock itself. Closing it properly means correlating PFCP requests with responses.

A guaranteed rate is not programmed at establishment. newQER.GBR is set only on the update
path; CreatePccRuleQer, which the establishment datapath uses, has no GBR branch. So a guaranteed
rate that a modification applies disappears when the UE re-attaches. It is unobservable until the
policy path can carry one, so it is stated rather than fixed here.

A stale answer from the radio cannot be told from a timely one while a newer modification is in flight. The session records that it is waiting for the radio's answer, and one arriving outside that window is ignored — which covers an answer to a modification that has been abandoned. What it cannot cover is an answer to the previous modification arriving while a new one is outstanding: the response carries no identity beyond the session, so at this interface the two are the same message.

Tests

Unit tests cover the refusal and its causes, the collision exception, T3591 resolution and expiry, commit-or-discard, the three shapes of a radio access network answer, the realignment, and the abandonment counters. They drive the real handlers rather than reimplementing them — the refusal tests encode a genuine 0xC9 with the NAS library and post it through HandleUpdateN1Msg, and the response tests hold SMLock the way the update path holds it.

That last detail is not incidental. A deadlock survived a full green suite because every test called the inner handler directly, where the lock is free, while production calls it under a defer-held lock. There is now a test that holds the lock the way the real caller does and fails by timing out.

Every guard is mutation-verified. Removing the 0xC9 case reproduces the original defect and the test reports it as "the response carries no N1 SM message". The four fixes in the section above are each verified the same way — restoring the both-directions GBR requirement, removing the delete-only detection, dropping the state restoration, and removing the superseded-timer check each fail a test.

pre-commit run --all-files is clean end to end on darwin — gitleaks v8.30.1, gci v0.14.0, staticcheck v0.8.1, go test -race, golangci-lint v2.13.2, yamlfmt v0.21.0 and reuse v6.2.0, the versions .pre-commit-config.yaml pins. One caveat worth naming rather than leaving to be discovered: go test -race ./producer/ fails intermittently on RetrieveUPFNodeByNodeID, which reproduces on upstream main without this branch — NewUPF publishes into the pool before initialising it. Targeted -race over the packages this branch touches is clean.

Deletions are expressed as a QosFlowToReleaseList now, which an earlier version of this section said they never were. The corrective modification after a partial rejection is the one exception, and deliberately so: its deletions are the flows the radio refused, which were never established there.

🤖 Generated with Claude Code

https://claude.ai/code/session_01GyNr6vp6JaxxzPyVcuHXTf

[gab-arrobo]

Why 5GSM cause #32 and not #31

I think this description has to be updated (similarly to what i did in #619) because when you prepend # to a number, GitHub assumes you are referencing a pull request or issue

[Copilot]

Pull request overview

This PR completes the SMF’s PDU Session Modification procedure handling end-to-end, covering UE-initiated modification refusal, network-initiated modification retransmission/abandonment via T3591, consumption of RAN modify responses (including partial acceptance + realignment), and correctness fixes uncovered when modification paths started exercising previously establishment-only code paths.

Changes:

• Add full N1/N2 handling for PDU Session Modification (refusal path, completion/command-reject handling, RAN modify response/failure handling, partial-rejection realignment).
• Introduce and wire up T3591 resolution, retransmission, and abandonment reporting/metrics.
• Harden policy commit/revert and datapath/QoS handling to avoid panics and silent state divergence on modification and failure paths (including one-direction GBR support).

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
smferrors/errors.go	Adds 5GSM cause #32 constant and maps a modification-refusal error cause.
qos/session_rule.go	Prevents clearing the active session rule on modifications; adds logging.
qos/session_rule_active_test.go	Tests active session rule retention/replacement behavior.
qos/prune.go	Adds pruning utilities to remove refused flows and dependent PCC rules from a pending update.
qos/prune_test.go	Unit tests for pruning/refused-flow handling and commit behavior.
producer/pdu_session.go	Changes N1N2 transfer failure handling to revert modifications instead of dropping sessions.
producer/n1n2_data_handler.go	Adds UE modification refusal, modification complete/command-reject handling, default-case logging, and N2 modify response/failure handlers + realignment goroutine.
producer/modify_response_shapes_test.go	Tests handling of “all/none/some flows established” RAN responses and delivery-failure behavior.
producer/modification_refusal_test.go	Extensive tests for UE-request refusal, collision behavior, timer interaction, and deadlock regression.
producer/modification_failure_test.go	Tests revert/discard behavior on failures in PFCP/N1N2 stages and state restoration.
producer/callback.go	Refactors network-initiated modification to use ApplyModification; adds seams; implements T3591 start/expiry/abandon and revert logic.
producer/abandon_modification_test.go	Tests abandonment behavior, idempotence, and superseded-timer expiry protection.
metrics/telemetry.go	Adds Prometheus counters for NAS timer resolution and modification abandonment.
factory/t3591_defaults_test.go	Tests config defaulting behavior for the new T3591 block.
factory/factory.go	Ensures T3591 defaults are applied during config init.
factory/config.go	Introduces T3591 config block and TimerValue schema (enable/expireTime/maxRetryTimes).
context/timer.go	Adds/ports a retransmission Timer with idempotent Stop.
context/timer_test.go	Tests timer retransmit/cancel, Stop idempotence, and Stop-before-expiry behavior.
context/sm_context.go	Adds session fields for extended NAS-SM timers, T3591 resolution, pending modification state, and locked commit helper; adds StopT3591 helper.
context/session_rule_guard_test.go	Tests SelectedSessionRule nilability and committed-rule selection behavior.
context/ngap_modify_request_test.go	Tests NGAP modify request flow-list construction (added flows, default fallback, delete-only correction behavior).
context/ngap_modify_handler.go	Adds NGAP modify response/failure decoders and pending realignment model.
context/ngap_modify_handler_test.go	Tests modify response classification and includes canary tests around upstream NGAP optional-tag behavior.
context/ngap_build.go	Fixes NGAP modify request to iterate policy deltas (and omit add/modify list for delete-only correction); adds helpers.
context/nas_timer.go	Adds T3591 resolution logic (default vs satellite vs config) and effective retry logic.
context/nas_timer_test.go	Tests timer resolution sources and EffectiveT3591 behavior.
context/gsm_build.go	Adds builder for PDU Session Modification Reject using request PTI/session ID and mapped cause.
context/gbr_test.go	Tests one-directional GBR handling.
context/datapath.go	Adds nil/pending-update guards, avoids nil derefs in session-rule access, and factors GBR building logic.
context/commit_policy_test.go	Tests commit/discard behavior safety and ensures in-flight modification state isn’t persisted.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[midwell]

Rebased onto current main and dropped the local cause constant — head 84ebacd.

smferrors.Cause5GSMServiceOptionNotSupported existed only because the NAS library had no name for cause 32: its Cause5GSM list ran from 31 straight to 34. nas v2.2.1 fills that gap, and main already requires it, so the local definition was a duplicate of a public constant with the same value. The refusal tests assert against the library constant now too, so nothing names this cause locally any more.

The ngap commit also lost its dependency half in the rebase — main already requires v2.1.4 — so it is now just the test change, and its message says so.

Mutation-checked both: swapping the mapping to cause 31 fails both refusal tests, and the modify-response test still fails against ngap v2.1.3.

[gab-arrobo]

@midwell please fix lint issues. Thanks!

[midwell]

@gab-arrobo Lint is fixed in c7b200e — the lint job is green on that head, and so is the rest of the run.

CI's golangci-lint moved to v2.13.2, which flagged eight goconst findings, all in this change's own tests. The session rule id, session AMBR and unparsable QoS id are now named in context, rule-2 in qos, and the two producer tests use the testSupi the package already defines rather than repeating the IMSI literal.

[midwell]

Rebased onto main at 7b3ff95 and added two commits.

The rebase brought #633's smferrors/keys_test.go, and this branch failed it twice. Both failures were real, so thanks — they were worth having.

• "ModificationNotSupported" is in ErrorCause and not in ErrorType. That is legitimate rather than an omission: the refusal answers a UE-requested modification inside the UpdateSmContext response with a MODIFICATION REJECT carrying a 5GSM cause, per TS 23.502 subclause 4.3.3.2 step 3a, and never builds a PostSmContexts problem-details body — the same shape as the release path's InvalidPDUSessionIdentity. Recorded in causeOnlyKeys with that reason.
• The cause was computed into a local and then handed to the builder, which TestNoErrorKeyIsComputed refuses. The refusal is now a function taking the cause as a parameter, and each switch arm passes its own literal. Behaviour is unchanged and the tests say so rather than the diff: the three refusal tests pass unaltered, including the sub-cases asserting 43 for an inactive or unknown identity against 32 for the refusal.

Also added: the guaranteed bit rate is now programmed at establishment, not only on the policy-update path. CreateDedicatedQosQer set the QER's GBR and CreatePccRuleQer had no GBR branch at all, so a guarantee configured in policy reached the UPF on a policy edit and was dropped the next time the session was established — it works, then disappears when the UE re-attaches, which reads as intermittent rather than as unimplemented. BuildGBR is reused so both paths answer the same way. Deliberately no session-level fallback there, unlike the maximum rate: the session AMBR is a ceiling over every flow while a guarantee is a commitment to one. Two tests, the first verified by removing the new branch and watching it report the guarantee dropped.

This became worth doing now because the producing end has landed: pcf#390 and webconsole#565 are what let a policy carry a guaranteed rate at all, so the branch is no longer code nothing exercises.

Gates on the pushed tree: make test green across all 15 packages, golangci-lint v2.13.2 0 issues, make check-reuse compliant.

[gab-arrobo]

@midwell please resolve conflicts and rebase PR. Thanks!

[midwell]

@gab-arrobo Rebased onto a51bd70; head 68e6321, conflicts resolved.

The only conflict was metrics/telemetry.go, and it was additive on both sides: #626's
upfRestoration/upfUnrestored and this branch's nasTimer/modAbandon are added to the same
struct, initialiser and register(). I kept both, and took labelUpf for the sessProfile
labels rather than the literal this branch had. Nothing else needed resolving — git range-diff
against the pre-rebase head shows the other 24 commits unchanged.

Two things I checked rather than assumed, since main moved under this branch:

• Echo the request's PTI in the PDU session establishment and release rejects #636's PTI echo. The modification refusal reads the PTI out of the decoded
  PDU SESSION MODIFICATION REQUEST and passes it to the reject builder, so it does not depend
  on smContext.Pti and does not overwrite it. Both of Echo the request's PTI in the PDU session establishment and release rejects #636's stores are intact.
• Fixes after restoration #637's response-channel gating. ApplyModification moves the session to
  SmStatePfcpModify before it sends, which is the state
  HandlePfcpSessionModificationResponse gates both its success and failure sends on, and
  SendPfcpSessionModifyReq is the waiter. So the new modification paths pair a send with a
  receive the way the gating expects.

One new commit on top: goconst at v2.13.2 counts a literal across the whole package, and the
"default" PDR key is written by n1n2_data_handler_test.go and, since #626,
restoration_wire_test.go. This branch's modify-response test is the third writer, which trips
the linter, so it names the key in its own file and leaves the other two alone.

One thing to be aware of, and it is not from this branch: go test -race ./producer/ has a
race on pristine main. I reproduced it on a51bd70 with nothing of mine present, in 3 of
4 runs, failing in TestOutstandingRequestsNeverExceedTheBound or
TestASweepThatCouldNotLookAtAnySessionDoesNotReportNothingToRestore depending on the run. The
pair is:

• context.NewUPF stores the UPF into upfPool before it initialises UPFStatus, NodeID
  and the ID generators (context/upf.go:221 versus :224 onward), and
• RetrieveUPFNodeByNodeID ranges that pool and reads curUPF.NodeID with no lock
  (context/upf.go:332), which waitForAssociation does on the restoration goroutine
  (producer/restoration.go:980).

So a reader can observe a UPF that is in the pool but not yet initialised. In the tests the
reader is the goroutine TestASecondRestartSupersedesTheRestorationInProgress leaves running;
in a deployment the writer is a configuration update creating UP nodes while a restoration is
polling. CI does not hit it — the pipeline is green on main, and unit-tests is green on this
pull request — so it is a local-only symptom today, but the ordering is real. Publishing the UPF into the pool only once it is initialised
would close it. Happy to send that as its own pull request if useful.

Gates on the pushed tree: golangci-lint v2.13.2 0 issues, make check-reuse compliant,
and make test green on all 15 packages other than producer, whose only failure is the race
above.

[Copilot]

🟡 Changes recommended

Critical unresolved issues remain in rollback, timer synchronization, policy commits, and modification-state handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

producer/callback.go:666

• BuildPfcpParam mutates the in-memory PDR/QER objects before sendPfcpSessionModifyReq confirms the update (CreateDedicatedQosQer allocates QERs and assigns them to PDRs). On failure, this discards the policy and rebuilds from no pending update, but it never restores those mutated PDR/QER fields or removes the allocated QERs; the supposed revert can resend the failed QERs and leak them instead of restoring the previous configuration. Snapshot/clone the data path or explicitly roll back these mutations before constructing the revert.

	pfcpParam := BuildPfcpParam(smContext)

producer/callback.go:583

• An explicit T3591 disable returns here after clearing NwModificationPending, but it leaves the already-programmed update in SmPolicyUpdates with no terminal commit, discard, or rollback path. If the UE never answers, the session remains indefinitely inconsistent; a later modification can overwrite the single pending slot, and a delivery-failure indication is misclassified as an establishment because it sees the flag as false. Disabling retransmission still needs a defined abandonment/rollback policy.

	enabled, maxRetries := smfContext.EffectiveT3591(factory.SmfConfig.Configuration.T3591)
	if !enabled {
		smContext.SubPduSessLog.Warnf("T3591 is disabled by configuration; an unacknowledged modification will be neither retransmitted nor abandoned")
		smContext.NwModificationPending = false
	}

producer/n1n2_data_handler.go:325

• The command-reject path has the same correlation gap: it abandons the current update without checking the message PDU Session ID/PTI or whether the current network procedure is the one being rejected. A stale reject can call abandonModificationUnderLock and discard a newer pending update. Correlate the reject before stopping T3591 and discarding state.

			cause := m.PDUSessionModificationCommandReject.GetCauseValue()
			smContext.SubPduSessLog.Warnf("PDUSessionSMContextUpdate, N1 Msg PDU Session Modification Command Reject received, 5GSM cause %d", cause)
			smContext.StopT3591()
			// The UE will not apply the parameters it was given. This is an abandonment with its
			// own cause rather than a timeout, and it is reported on the same path as one, so that

qos/prune.go:78

• The corrective update keeps the original full SmPolicyDecision, but ApplyModification passes it to BuildPfcpParam, whose CreateDedicatedQosQer iterates SmPolicyDecision.GetQosDecs() rather than the delta. A refused flow therefore remains programmed/recreated in PFCP even after it is removed from add/mod; the UE is withdrawn while the UPF still enforces it. Build the corrective decision from the pruned policy or make the PFCP builder consume the delta.

		SmPolicyDecision: u.SmPolicyDecision,

• Files reviewed: 31/31 changed files
• Comments generated: 20
• Review effort level: Lite

[midwell]

@gab-arrobo Twenty comments on the last push. Six are answered in 8be7a79, on top of your
rebase; the rest I have worked through against the code, and this is where each stands, so none
of them is left ambiguous while I finish them.

Fixed in 8be7a79, each with a test that fails against the old code:

• The T3591 value a restored session does not carry. Arming with it gives
  panic: non-positive interval for NewTicker — reproduced, not reasoned about — so the first
  network-initiated modification after a restart ended the process. Resolved on arming and kept.
• An identifier that cannot be a QoS flow identifier reaching the radio as one: 257 narrows to
  QFI 1 inside GetQosFlowIdFromQosId, so the range check downstream could never see it.
• The timer armed after the state change released SMLock, and abandonIfCurrent checking the
  timer under one acquisition and abandoning under another.
• The realignment marker outliving the procedure it belonged to.

Verified real, and being fixed next — these belong together, because they are one theme: a
modification that is given up on leaves the user plane holding it.

• Abandonment on T3591 expiry, whole RAN rejection or a UE reject discards the pending update and
  settles the session, but does not put the user plane back. revertModification already does
  exactly that for a delivery failure; the abandonment paths should go through the same rebuild,
  with their own path and cause on the metric.
• The PFCP-modify failure path, for the same reason, and the second-failure path that only
  relabels the state.
• The expiry closure retransmits without checking that its procedure is still current, and reads
  the pending update and the tunnel outside the lock.
• A stale failure indication reverting a newer procedure, and a delayed MODIFICATION COMPLETE
  committing one.

Verified real, and the shape of the fix is not what the comment suggests — worth saying
before I push it:

• RemoveFlows moving a refused mod entry into the corrective del does tear down an
  established flow. The radio refused a change to that flow, so the session should keep it on
  its previous parameters rather than lose it. Since CommitQosFlowDescUpdate never applies
  mod, those previous parameters are still the committed ones, which is what the corrective
  update should carry. Same for the PCC rules.
• The refused-flow lookup keying on the update's map key rather than QosData.QosId is a real
  inconsistency with the builder, which derives the QFI from QosId. The fix is to read it from
  the same field in both places.
• A session rule whose name is in update.del leaves ActiveSessRule nil for a different reason
  than a modification does, and the branch that preserves the active rule keeps a rule that has
  been deleted. Clearing it when its name is in del is right.

Pre-existing, and now load-bearing: CommitQosFlowDescUpdate, CommitPccRulesUpdate and
CommitSessionRulesUpdate all carry // Mod rules — TODO from the original 5G QoS commit, so a
modification of an existing rule is sent and then dropped from committed state. This branch is
what makes that reachable on the modification path, so I am treating it as mine to close rather
than as inherited.

Gates on the pushed tree: make test green across all 16 packages, golangci-lint v2.13.2 at 0
issues, make check-reuse compliant.

[Copilot]

🟡 Changes recommended

Unresolved findings affect PFCP rollback, procedure correlation, timer synchronization, state transitions, and deletion handling.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (14)

context/datapath.go:136

• Returning an empty params object here does not make the nil-tunnel path safe: ApplyModification and revertModification still unconditionally call SendPfcpSessionModifyReq, whose first line dereferences smContext.Tunnel. Thus the teardown race this guard claims to cover still panics; propagate a no-tunnel error and skip the PFCP send instead.

	if pccRuleUpdate != nil {
		addRules := pccRuleUpdate.GetAddPccRuleUpdate()

		for name, rule := range addRules {

producer/callback.go:576

• This callback runs on the timer goroutine without SMLock, but BuildAndSendQosN1N2TransferMsg reads SmPolicyUpdates while the acknowledgement path commits/discards that slice under SMLock. A stop cannot recall a tick already selected, so an acknowledgement can race this build and cause -race reports or a retransmission built from an empty/new update after the original procedure finished. Check that this timer is still current and synchronize/snapshot the modification before constructing the retransmission (without holding the lock across the nested send lock).

			smContext.SubPduSessLog.Warnf("T3591 expired (%d of %d), retransmitting PDU session modification command",
				expireTimes, maxRetries)
			if err := sendQosN1N2TransferMsg(smContext); err != nil {
				smContext.SubPduSessLog.Errorf("retransmitting the modification command failed: %v", err)

producer/callback.go:497

• BuildPfcpParam mutates Tunnel by creating QERs/PDRs before sendPfcpSessionModifyReq runs. If PFCP fails, this branch discards the policy update and restores only the FSM state; the attempted rules remain in the in-memory datapath even though the UPF never accepted them, so a later modification can send stale rules or duplicate them. Roll back the datapath mutation as part of this failure path, rather than only discarding the policy slice.

		if discardErr := smContext.CommitSmPolicyDecisionLocked(false); discardErr != nil {
			smContext.SubPduSessLog.Errorf("discarding the unprogrammed modification failed: %v", discardErr)
		}
		smContext.ChangeState(smfContext.SmStateActive)

producer/callback.go:681

• The discard-first rebuild does not restore the previous user-plane configuration: after abandonModification removes SmPolicyUpdates, BuildPfcpParam has no policy delta to recreate dedicated/session QERs and ActivateUlDlTunnel falls back to default PDR creation, while PDRs/QERs installed by the failed modification remain. Thus an N1/N2 delivery failure can leave the UPF enforcing the modification the UE never received. Preserve the pre-modification PFCP state or construct an explicit inverse update before discarding it.

// The discard comes first and does the heavy lifting: the pending policy update was never
// committed, so once it is dropped the session's policy state already describes the
// pre-modification session, and rebuilding the PFCP parameters from it yields exactly the rules
// that were in force. Nothing is snapshotted and nothing is copied.

producer/callback.go:594

• An explicit T3591 disable clears NwModificationPending even though the command and SmPolicyUpdates[0] are still awaiting a UE response. A colliding UE request will therefore no longer be disregarded, and an N1/N2 failure indication will take the establishment path and can drop a working session. Keep the procedure marked pending until an acknowledgement or an explicit rollback settles it.

	if !enabled {
		smContext.SubPduSessLog.Warnf("T3591 is disabled by configuration; an unacknowledged modification will be neither retransmitted nor abandoned")
		smContext.NwModificationPending = false
	}

producer/callback.go:618

• The PFCP request has already programmed the new PDR/QER state before the UE can acknowledge it. Discarding only SmPolicyUpdates does not roll those in-memory or UPF rules back, so a command reject, whole RAN rejection, or T3591 expiry leaves the UE/session policy on the old configuration while the user plane enforces the new one. This helper needs a PFCP rollback (or a design that delays programming) before claiming that abandonment keeps the previous parameters.

func abandonModificationLocked(smContext *smfContext.SMContext) {
	if err := smContext.CommitSmPolicyDecisionLocked(false); err != nil {
		smContext.SubPduSessLog.Errorf("discarding the abandoned modification failed: %v", err)
	}

producer/callback.go:622

• Dropping the timer pointer without stopping it leaves the ticker goroutine running when abandonment follows a command that was already sent (for example, an N1/N2 failure indication). That goroutine can retransmit after the policy update has been reverted, potentially using a later procedure's state. Stop the timer before clearing its handle.

	smContext.T3591 = nil

producer/n1n2_data_handler.go:915

• The corrective update contains QosFlowUpdate.del/PccRuleUpdate.del, but BuildPfcpParam only emits removals for its release-only case and otherwise chooses an arbitrary valid PCC rule and rebuilds it. Consequently this delete-only realignment withdraws the refused flow from the UE while leaving its PDR/QER installed at the UPF; the next packet can still be classified onto the flow that the RAN refused. The corrective PFCP path must translate the deletion delta into removePDR/removeQER (and related FAR removal) before committing it.

// Both are corrected by one ordinary modification carrying the deletions. TS 23.502 clause
// 4.3.3.2 calls for a further procedure rather than a retry of the one that was partly accepted,
// and a deletion *is* such a procedure — so it goes through ApplyModification like any other
// instead of this function rebuilding the user plane and sending N1N2 itself. It did both by hand

producer/n1n2_data_handler.go:966

• If NAS encoding fails here, the helper simply returns and the caller still completes the UpdateSmContext request with a 200 response but no N1 message. That reintroduces the original dropped-modification behavior on an error path and leaves the UE retrying until T3581; propagate the error to the transaction instead of silently returning. Apply the same propagation to the temporary-file error below.

	buf, err := context.BuildGSMPDUSessionModificationRejectWithCause(pduSessionID, pti, cause)
	if err != nil {
		smContext.SubPduSessLog.Errorf("PDUSessionSMContextUpdate, build GSM PDUSessionModificationReject failed: %+v", err)

		return

producer/n1n2_data_handler.go:312

• This is now the success path for every acknowledged network modification, but the underlying commit still ignores existing-entry updates in QosFlowsUpdate.mod and SessRulesUpdate.mod (qos/qos_flow.go:583 and qos/session_rule.go:67). A change to an existing QoS or session rule is therefore programmed in PFCP and sent to the UE, then omitted from SmPolicyData; the next PCF delta is computed against stale values and can resend or overwrite the change. Persist the mod entries before treating this procedure as committed.

			if err := smContext.CommitSmPolicyDecisionLocked(true); err != nil {

producer/n1n2_data_handler.go:288

• A late or replayed MODIFICATION COMPLETE is accepted unconditionally. After T3591 abandonment or a command rejection, it can arrive after a newer network modification has populated SmPolicyUpdates; this branch then stops the newer timer and commits its update even though the message acknowledges the old command, and it can consume the newer realignment marker. Guard the completion with a procedure-generation/pending check before stopping or committing.

		case nas.MsgTypePDUSessionModificationComplete:
			smContext.SubPduSessLog.Infoln("PDUSessionSMContextUpdate, N1 Msg PDU Session Modification Complete received")

producer/n1n2_data_handler.go:288

• This branch accepts every PDU SESSION MODIFICATION COMPLETE without checking the message's PDU Session ID or PTI. A stale response delivered to this SM context can stop T3591 and commit or discard whichever update is currently pending, including a newer procedure. Validate the response identity (the network command uses PTI 0) before mutating session state, as the release-request path already validates its session ID.

		case nas.MsgTypePDUSessionModificationComplete:
			smContext.SubPduSessLog.Infoln("PDUSessionSMContextUpdate, N1 Msg PDU Session Modification Complete received")

producer/pdu_session.go:901

• The pending flag is read, the lock is released, and only then is the rollback started. A concurrent modification completion or newer policy procedure can commit/replace the pending update in that gap, after which revertModification discards or rebuilds the wrong procedure. Correlate the failure indication with the procedure (for example with a generation/update identity) and revalidate it while holding the lock before reverting.

	smContext.SMLock.Lock()
	modifying := smContext.NwModificationPending
	smContext.SMLock.Unlock()
	if modifying {
		smContext.SubPduSessLog.Warnf("the modification could not be delivered to the UE; reverting it and leaving the session on its previous parameters")
		revertModification(smContext, "n1n2_transfer_failure_indication")

qos/prune.go:91

• The corrective update retains the original full SmPolicyDecision. ApplyModification passes it to BuildPfcpParam, and CreateDedicatedQosQer iterates that decision's complete QoS-data and PCC-rule sets, so a partial-rejection correction can program the refused flows again in the UPF; only the NAS delta contains their deletion. Build the corrective decision from the accepted flows or make PFCP construction consume the delta/deletion maps.

		SmPolicyDecision: u.SmPolicyDecision,

• Files reviewed: 33/33 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

Note

Copilot was unable to run its full agentic suite in this review.

Pull request overview

Copilot reviewed 42 out of 42 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

qos/prune.go:1

• flow can be nil if an upstream producer inserts a nil *models.QosData into the add/mod maps, and flow.GetQosId() would panic. Add a nil guard for flow before calling methods on it (similar to the nil handling already present in the PCC rule loop).

[midwell]

Fixed at 2c97a56 — a plain conditional. You are right that the path is reached routinely: a retransmitted PDU SESSION MODIFICATION COMPLETE arrives with nothing pending, which is exactly when this line runs.

[gab-arrobo]

@midwell, it is possible to break this PR into smaller pieces/PRs?

[Copilot]

🟡 Changes recommended

Rejection, rollback, stale-answer, and policy-commit paths can leave the UPF and committed SMF state inconsistent.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (8)

producer/n1n2_data_handler.go:912

• At this point ApplyModification has already programmed PFCP before sending the command. abandonModificationUnderLock only discards the pending policy and changes state; it does not send a compensating PFCP modification. A whole RAN rejection therefore leaves the UPF enforcing the rejected policy while the SMF returns to the previous one. Roll back the user plane before settling this procedure (outside this lock or through a lock-safe rollback path).

	case result.WhollyRejected():
		smContext.SubPduSessLog.Warnf("radio access network established none of the modified flows %v", result.RejectedQFIs)
		smContext.StopT3591()
		abandonModificationUnderLock(smContext, "ran_whole_rejection", "no_flow_established")

producer/n1n2_data_handler.go:968

• The whole-rejection failure path has the same missing user-plane rollback as the response path: PFCP was applied before the command, but abandonModificationUnderLock only removes the pending record. A PDU_RES_MOD_FAIL consequently leaves the UPF on the failed modification while the SMF/UE state is discarded. Restore the pre-modification PFCP state before abandoning this procedure.

	if committed := smContext.CommittedBeforeRanAnswer; committed != nil {
		smContext.CommittedBeforeRanAnswer = nil
		correctCommittedModification(smContext, committed,
			context.ModifyResponse{RejectedQFIs: flowsCarriedBy(committed)})

		return nil
	}

	abandonModificationUnderLock(smContext, "ran_whole_rejection", fmt.Sprintf("ngap_cause_present_%d", cause.Present))

producer/n1n2_data_handler.go:304

• Every decoded Modification Complete reaches StopT3591 and commit without verifying that a modification is still pending for this session. A late duplicate after T3591 abandonment can therefore clear a newer timer and commit its pending update even though the UE never acknowledged it (and the PDU session ID is not checked either). Gate this branch on the active procedure identity/state before mutating policy or timer state.

			smContext.StopT3591()

context/ngap_build.go:547

• This now puts GetModified() QoS entries on the NGAP request, but qos.CommitQosFlowDescUpdate still has no implementation for its mod map (qos/qos_flow.go:583-585). After a successful UE completion, the update is popped without changing the committed QoS data, so later policy deltas are computed from stale parameters. Implement the committed-state update for modified flows before exposing them on this procedure.

	// The establishment path in this file has always built its list by iterating the policy delta.
	// This does the same, over both the added and the modified flows, since a modification may do
	// either. The single default flow remains the fallback for an update that names no flows at

qos/prune.go:36

• RemoveFlows treats add and mod identically, but QosFlowFailedToAddOrModifyList can report a failed modification of an already established QFI. Removing that mod entry and its PCC rule makes the corrective command withdraw the pre-existing flow instead of retaining or restoring its previous parameters. Convert only newly added flows directly to deletions; modified flows need their committed value restored or retained.
  context/datapath.go:431
• Returning nil when no policy update is pending makes the rollback unable to reconstruct committed policy: revertModification discards SmPolicyUpdates before calling BuildPfcpParam, so these QER builders have no policy to derive from. BuildPfcpParam then only reprocesses the default PDR and leaves newly added or modified PDRs in the UPF, even when the caller treats the revert as successful. Rebuild from committed policy or a pre-modification snapshot instead of treating an empty pending update as an empty data plane.

func (dpNode *DataPathNode) CreatePccRuleQer(smContext *SMContext, qosData string, tcData string) (*QER, error) {
	// Nothing pending. Reachable whenever the user plane is rebuilt after the pending update has
	// been discarded — which is exactly what reverting an undelivered modification does — and
	// indexing here would take the SMF down.
	if len(smContext.SmPolicyUpdates) == 0 {
		logger.PduSessLog.Warnf("no pending SM policy update while building QERs for UE [%s]; nothing to program",
			smContext.Supi)
		return nil, nil

producer/callback.go:663

• The timer identity check exists only in cancelFunc; an expiry callback can already be running when StopT3591 or a newer procedure replaces smContext.T3591. It then calls sendQosN1N2TransferMsg unconditionally and can retransmit after the UE completed, or build the newer pending update from an old timer. Guard retransmissions with the current timer/procedure generation and coordinate that check with the send.

	var timer *smfContext.Timer
	timer = smfContext.NewTimer(smContext.T3591Value, maxRetries,
		func(expireTimes int32) {
			smContext.SubPduSessLog.Warnf("T3591 expired (%d of %d), retransmitting PDU session modification command",
				expireTimes, maxRetries)
			if err := sendQosN1N2TransferMsg(smContext); err != nil {
				smContext.SubPduSessLog.Errorf("retransmitting the modification command failed: %v", err)

qos/prune.go:73

• A PCC rule can reference more than one QoS data entry, but this deletes the entire rule as soon as any referenced QFI is refused. With one accepted and one refused flow under the same PCC rule, the corrective update removes policy enforcement for the accepted flow too. Remove only the refused reference (or retain the rule when other references remain) rather than putting the whole rule in del.

• Files reviewed: 42/42 changed files
• Comments generated: 4
• Review effort level: Lite

[midwell]

Yes — split into eight, four of which are open now and stand alone.

The four that need nothing else, in no particular order:

• 657 — three ways a mid-session policy change could take the SMF down: a nil session rule dereferenced, five unguarded indexings of the pending update, and a modification wiping the session's active rule.
• 658 — a guaranteed bit rate dropped at establishment, and discarded unless both directions were configured.
• 659 — PFCP sends and dispatches that reported success without sending, and a Fatalln on a truncated adapter response.
• 660 — the NGAP modify request naming one flow, never naming a release, and narrowing QoS identifiers before range-checking them.

Those four are worth taking on their own account: each fixes something reachable today on the path a PCF policy update already takes, without any of the new procedure.

The remaining four are the feature itself, and they are stacked because each genuinely needs the one before:

1. the T3591 timer, its configuration and its reporting, with nothing using it yet;
2. the network-requested procedure — send the command, commit on the UE's acknowledgement, retransmit and abandon, revert one that cannot be delivered;
3. the radio's answer — decode it, act on whole and partial rejection, correct a partial rejection with an ordinary modification;
4. UE-requested modifications — refuse what the SMF does not implement, disregard one that collides with a network modification.

They also need 657 and 660: the revert path rebuilds the user plane with no pending update, which is what 657 guards, and the procedure calls the builder 660 fixes. I will open them as those merge, rather than stack four pull requests on unmerged bases — say the word if you would rather see them now.

The branches for all eight are pushed. I checked the split by merging the eight back together and diffing against this branch rebased onto current main: the only differences are in test files, where shared fixtures moved to the earliest pull request that needs them. No production code differs. Each of the eight builds, passes the full suite, and passes pre-commit run --all-files on its own.

This branch stays as it is until the pieces are merged, so nothing is lost if the split stalls.