Skip to content

fix(context): an allocator with no pool refuses the work instead of panicking - #712

Merged
gab-arrobo merged 2 commits into
omec-project:mainfrom
grewelltech:fix/allocator-with-no-pool
Oct 26, 2026
Merged

gab-arrobo merged 2 commits into
omec-project:mainfrom
grewelltech:fix/allocator-with-no-pool

Conversation

@bgrewell

@bgrewell bgrewell commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

The defect

An IPAllocator can be reached with nothing behind it, in two ways:

  • A DNN with no usable ueSubnet. buildSnssaiSmfInfo (context/context.go:372-379) builds an allocator only when the IP domain carries a ueSubnet that parses; it logs a warning otherwise and stores the DNN regardless, with UeIPAllocator nil. The first PDU session on that DNN reaches smContext.DNNInfo.UeIPAllocator.Allocate(...) (producer/pdu_session.go:219) on a nil receiver.
  • A session restored from MongoDB. IPAllocator has only unexported fields, so an SMContext stores as "dnnInfo":{"UeIPAllocator":{},…} and comes back with g and ipNetwork nil — UnmarshalJSON rebuilds the PFCP context, the tunnel and the log tags, but not the allocator. Releasing such a session reaches Release and dereferences the pool that is not there.

Both panic. The second one is the broad case: it takes every session whose address the SMF allocated — any deployment running enable_ue_ip_alloc: false with enableDBStore set — once the SMF has restarted. Transaction.StartTxnLifeCycle recovers, logs the stack and fails the transaction, so the process survives and what is lost is that session's release and the address it held.

The fix

Allocate answers an error, which HandlePDUSessionSMContextCreate already turns into IpAllocError. Release, BlockIp and ReserveStaticIps have nothing to do and return; Release says so at WARN and skips the release: there is no pool to return the address to. After a restart the running pool never recorded a restored session's address, so it may already be free there. That is the unreconciled-pool defect below.

What it deliberately leaves alone

  • The pool is never reconciled with live sessions. An allocator is built with an empty in-use set and nothing feeds existing sessions back into it, so a restart or a polled configuration change can hand out an address another session is using. Separate defect, larger fix, not folded in here.
  • Refusing a DNN with an unusable ueSubnet at config load. Arguably better than storing it, but it changes when an operator finds out; with this change the session fails cleanly either way.

Verification

  • go test ./... passes. Reverting only context/ip_allocator.go makes TestReleaseUeIpAddr_OnRestoredContext panic, so the test holds the fix.
  • Tests added: every allocator method against a nil and a restored allocator (context/ip_allocator_test.go), and a round trip through ToBsonM + UnmarshalJSON followed by ReleaseUeIpAddr (context/db_test.go).
  • golangci-lint v2.13.2 with the repo's .golangci.yml: 0 issues. gofmt clean. Commit is signed off.

Relation to the open PRs

Independent of #711 — it touches none of the same lines and can land before or after it. It is the follow-up promised in that PR's review thread, where Copilot raised this panic as if #711 introduced it: #711 only widens which sessions reach it, because a static address on a UEIP-capable UPF no longer ends up UpfProvided.

@bgrewell
bgrewell requested a review from a team October 26, 2026 20:55
…anicking

An IPAllocator can be reached with nothing behind it, in two ways.
buildSnssaiSmfInfo stores a DNN whose ipDomain names no ueSubnet, or one
that does not parse, with UeIPAllocator nil; the first session on that DNN
then calls Allocate on a nil receiver. And an SMContext read back from
MongoDB under enableDBStore carries an IPAllocator that marshalled as {} --
the type has only unexported fields, and UnmarshalJSON rebuilds the PFCP
context, the tunnel and the log tags but not the allocator -- so its pool
and network come back nil. Releasing such a session reaches Release and
dereferences the missing pool.

Both panic today. The second one takes every session whose address the SMF
allocated, which is any deployment running enable_ue_ip_alloc: false with
enableDBStore set, once the SMF has restarted; the transaction lifecycle
recovers, so what is lost is the release rather than the process.

Allocate now answers an error, which HandlePDUSessionSMContextCreate
already turns into IpAllocError -- the only honest outcome for a DNN with
no addresses to give. Release, BlockIp and ReserveStaticIps have nothing to
do and return, and Release says so at WARN: it cannot return the address to
the pool the running configuration holds, so the address stays marked used
there.

What this does not do is reconcile that pool with the sessions already
using it. An allocator is built empty and nothing feeds the live sessions
back into it, so a restart or a configuration change can hand out an
address another session holds. That is a separate defect and a larger fix.
Refusing a DNN with an unusable ueSubnet at config load, rather than
storing it, is also left alone: it changes when an operator finds out, and
this change makes the session fail cleanly either way.

Tests: every allocator method on a nil and on a restored allocator, and a
round trip through ToBsonM and UnmarshalJSON followed by ReleaseUeIpAddr,
which panics without the guards.

Signed-off-by: Benjamin Grewell <bgrewell@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The guards preserve initialized-pool behavior and cover both panic paths; only a nonblocking warning-wording correction remains.

1 open issue
What changed in this PR

This PR prevents missing-pool IP allocators from panicking during session creation and release, including sessions restored from MongoDB.

Changes:

  • Return an allocation error and safely skip pool operations when the allocator is uninitialized.
  • Log a warning when an address cannot be released.
  • Add regression tests for nil allocators and release after database serialization.
File Description
context/​ip_allocator.go Guard missing-pool operations and warn when release is skipped.
context/​ip_allocator_test.go Test allocator methods with nil and empty receivers.
context/​db_test.go Test restored-session release and address clearing.

Review effort: Balanced

Comment thread context/ip_allocator.go Outdated
The warning claimed the address stays marked used, which is wrong for the
case it mostly serves: an allocator is built empty and only pre-reserves
static addresses, so after a restart the running pool never recorded a
restored session's address and may already hand it out. For a DNN with no
usable ueSubnet there is no pool at all. The warning now reports only that
the release was skipped; reconciling the pool with live sessions stays out
of scope.

Signed-off-by: Benjamin Grewell <bgrewell@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The focused guards preserve initialized-pool behavior and address the identified panic paths with regression coverage.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@gab-arrobo
gab-arrobo merged commit 2678bc5 into omec-project:main Oct 26, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants