feat: a timer for NAS procedures, and T3591 resolved for satellite access by midwell · Pull Request #666 · omec-project/smf

midwell · 2026-09-24T14:17:21Z

The first of four pull requests that complete the network-requested PDU session modification procedure, split out of #623. This one adds only T3591 (TS 24.501 subclause 10.3, table 10.3.2): the timer, its configuration and its reporting. Nothing uses it yet; the procedure that does is the next PR.

Why a timer type of its own rather than a time.Timer at each site. The procedure needs three things from it: an expiry count, a callback per expiry plus one at the end, and a Stop that is safe to call twice. A procedure answered while its last retransmission is in flight stops a timer that has already fired, and that must not panic.

The value depends on satellite access. Under TS 24.501 subclause 4.23.4, when the AMF indicates the extended NAS timer for access through a satellite NG-RAN cell, the SMF uses table 10.3.2's longer value. The value is resolved from configuration with a documented fallback, so an operator who configures neither gets the specification's terrestrial default rather than zero. A counter of where each value came from is added here for the next PR to report through. Once sessions report it, a deployment can tell a configured value from a defaulted one, which is the first question asked when a timer looks wrong.

The stack, each PR adding one commit on top of the one before:

this PR: the T3591 timer;
feat: the network-requested PDU session modification procedure #667, the network-requested modification procedure, which arms it;
feat: act on the radio's answer to a modification, not only the UE's #668, acting on the radio's answer to a modification;
feat: answer a UE-requested modification instead of dropping it #669, answering a UE-requested modification instead of dropping it.

Each is rebased onto current main, after #657, #658 and #660, and each commit passes the gate on its own.

Verification. pre-commit run --all-files on darwin/arm64 with Go 1.26.0, 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

Copilot

Copilot review overview

🟡 Changes recommended

Stop can return before a concurrently selected expiry callback is suppressed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Stop allows timer callbacks after returning · New

What changed in this PR

Adds foundational T3591 timer support for NAS procedures, including satellite-aware configuration, defaults, and telemetry.

Changes:

Adds a retry-counting NAS timer.
Adds terrestrial/satellite T3591 resolution and configuration defaults.
Adds timer and modification-abandonment metrics.

File	Summary
`metrics/telemetry.go`	Registers NAS timer and modification metrics.
`factory/t3591_defaults_test.go`	Tests configuration defaulting.
`factory/factory.go`	Applies T3591 defaults.
`factory/config.go`	Defines T3591 configuration.
`context/timer.go`	Implements retry timing; contains a critical stop/callback race.
`context/timer_test.go`	Tests timer expiry and stopping.
`context/nas_timer.go`	Resolves T3591 values and retry settings.
`context/nas_timer_test.go`	Tests timer resolution and defaults.

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

Copilot

Copilot review overview

🔵 Needs a closer look

Stop can still allow a concurrent expiry callback after stopping the timer.

Review effort: Lite
Findings: None

Resolved since last review (1)

Stop allows timer callbacks after returning

gab-arrobo

Here are a few changes/improvements as suggested by Go linter.

Also, is comment #666 (comment) incorrect? Or should it be addressed?

…cess The network-requested PDU session modification procedure needs T3591 (TS 24.501 subclause 10.3): the SMF retransmits the command when the UE does not answer, and gives the procedure up when the retransmissions run out. This is that timer, its configuration and its reporting, with nothing using it yet. The timer is its own type rather than a time.Timer per site, because the procedure needs three things from it that a bare timer does not give: an expiry count, a callback per expiry and one at the end, and a Stop that can be called twice. A procedure that is answered while its last retransmission is in flight stops a timer that has already fired, and that must not panic. The value is resolved from configuration, with a documented fallback, and it depends on whether the UE reaches the network through a satellite NG-RAN cell. TS 24.501 subclause 4.23.4 has the AMF indicate the extended NAS-SM timer for that access, and table 10.3.2 gives T3591 a longer value there. An operator who configures neither gets the specification's terrestrial default rather than zero. A counter of where each value came from is added here for the procedure to report through, so a deployment can tell a configured value from a defaulted one, which is the question asked first when a timer looks wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>

…ys what it guarantees The counters use atomic.Int32 instead of int32 fields updated through atomic.AddInt32 and friends, as suggested in review. Stop's comment promised that no expiry follows it, and that was not true. Stop signals the timer's goroutine and returns without waiting. An expiry that is already due can still be delivered, because the goroutine's select picks at random when the stop signal and the ticker are both ready. The comment now says so, and puts the check where it can work: in the caller, which has to confirm that its timer is still the current one, under the lock it holds when it calls Stop. Waiting in Stop for a callback in progress is not an alternative. Callers stop the timer under the same lock their callbacks take, so Stop would wait for a callback that is waiting for it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>

Copilot

Copilot review overview

🟢 Approval recommended

No unresolved review issues were identified.

Review effort: Lite
Findings: None

gab-arrobo

@midwell, please take a look at this comment. Thanks!

context/timer.go is the AMF's timer, ported so that both network functions retransmit on the same semantics. It keeps the free5GC notice the AMF's copy carries, as Apache-2.0 section 4(c) requires, but it had nothing marking what the port changes: the idempotent Stop, the typed atomics, and Stop's documented guarantee. Section 4(b) asks for that notice. It goes above the original one, the order this repository uses where a free5GC file carries a modifier's notice too: ONF's line above free5GC's in producer/sm_pfcp_handling.go, for instance. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Edvin Lindqvist <edvin.lindqvist@forsway.com>

midwell requested a review from a team September 24, 2026 14:17

midwell mentioned this pull request Sep 24, 2026

feat: the network-requested PDU session modification procedure #667

Merged

gab-arrobo force-pushed the split/t3591-timer branch from f517367 to 38f2365 Compare September 25, 2026 18:20

gab-arrobo requested a lite review from Copilot September 25, 2026 18:20

Copilot started reviewing on behalf of gab-arrobo September 25, 2026 18:20

View session

Copilot AI reviewed Sep 25, 2026

View reviewed changes

Comment thread context/timer.go

midwell mentioned this pull request Sep 25, 2026

fix: a policy update programs each rule on its own PDRs, and an abandoned one is put back #673

Open

gab-arrobo requested a lite review from Copilot September 25, 2026 22:44

gab-arrobo force-pushed the split/t3591-timer branch from 38f2365 to ca5990e Compare September 25, 2026 22:44

Copilot started reviewing on behalf of gab-arrobo September 25, 2026 22:44

View session

Copilot AI reviewed Sep 25, 2026

View reviewed changes

gab-arrobo reviewed Sep 25, 2026

View reviewed changes

midwell and others added 2 commits September 30, 2026 08:38

gab-arrobo requested a lite review from Copilot September 30, 2026 15:38

gab-arrobo force-pushed the split/t3591-timer branch from 2c8ab45 to 2973cc2 Compare September 30, 2026 15:38

Copilot started reviewing on behalf of gab-arrobo September 30, 2026 15:38

View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

gab-arrobo reviewed Sep 30, 2026

View reviewed changes

Comment thread context/timer.go

gab-arrobo approved these changes Sep 30, 2026

View reviewed changes

gab-arrobo merged commit c213f43 into omec-project:main Sep 30, 2026
12 checks passed

midwell mentioned this pull request Oct 17, 2026

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

Closed

Navigation Menu

Repository navigation

feat: a timer for NAS procedures, and T3591 resolved for satellite access - #666

feat: a timer for NAS procedures, and T3591 resolved for satellite access#666
gab-arrobo merged 3 commits into
omec-project:mainfrom
midwell:split/t3591-timer

midwell commented Sep 24, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

gab-arrobo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

gab-arrobo left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

midwell commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

gab-arrobo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

gab-arrobo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

midwell commented Sep 24, 2026 •

edited

Loading