pfcp: relay UPF-initiated session reports, so a paged UE can be reached

[midwell]

What this fixes

A Session Report is the only message on N4 that the user-plane function originates, and a
downlink data report is the only way the SMF learns that an idle UE has traffic waiting. This
adapter handles neither — types 56 and 57 are commented out of the dispatcher, so a report falls
through to the default branch and is logged as an unknown message type:

        /*
                case pfcp.PFCP_SESSION_REPORT_REQUEST:
                        handler.HandlePfcpSessionReportRequest(msg)
                case pfcp.PFCP_SESSION_REPORT_RESPONSE:
                        handler.HandlePfcpSessionReportResponse(msg)
        */

There is no UPF→SMF direction here at all. The consequence is that any deployment putting this
adapter on N4 has no mobile-terminated reachability: a downlink packet for an idle UE never
becomes a page. It fails silently, twice over — the SMF never learns there is traffic waiting,
and the user plane's request is never answered, so it retransmits into nothing while holding
traffic it should either deliver or release.

What the relay does

• The dispatcher routes types 56 and 57, and the read path returns the peer address — without
  which a UPF-originated message can be neither answered nor attributed.
• The SMF's address is recorded from the SMF-initiated messages that already carry it (SmfIp),
  since a relayed report has no request of ours to answer. The claim is parsed where it arrives
  and refused there if it is not an IP, so a malformed one cannot erase an address that works.
• The report is renumbered into the adapter's own sequence space, and the user plane's own
  number is restored on the response. This is the part worth a second look: outstanding requests
  are held in one table keyed by this socket's own address, so every request the adapter sends
  shares a single sequence space. Forwarding a UPF's number into it is refused as a duplicate
  whenever an SMF-originated request happens to be in flight under the same number — and the
  report is then rejected for no reason but coincidence. With several user planes, whose counters
  are independent, two reports collide with each other directly and one origin is lost.
• A retransmitted report is relayed once. A user plane retransmits a request that has not
  been answered, and until the answer exists nothing here recognises the copy — a report has no
  transaction of its own until it is answered, and the response transaction that absorbs
  retransmissions is created only then. Each copy was therefore renumbered and forwarded as a
  report in its own right: a second downlink data notification, and a second page, for traffic the
  SMF was already being told about. A report from the same address and port under the same
  sequence number — the identity this adapter keys every other transaction by — is recognised as
  the exchange already in flight, and that relay answers it. Delivery is not weakened, though the
  claim is narrower than unaffected: a relay lost on the way to the SMF used to be recovered by
  whichever came first, the adapter's own resend or the peer's copy, and now only the adapter's
  schedule recovers it — the same notification, possibly a little later on that path. Once the
  exchange is answered, or rejected, the same number is a new report again.
• A report is relayed, and answered, only for the user planes the SMF has addressed through us
  — see below.
• A report that cannot be relayed is rejected rather than dropped, including when the SMF
  never answers, so the user plane always learns the outcome instead of retransmitting blind.
  The recorded origin is forgotten after 30 s, so an unanswered report costs nothing lasting.

Relaying only for known user planes

Added in the third commit, answering the review. The handler previously checked that the message
parsed, that the peer address was non-nil and that an SMF address was known — nothing about who
sent it. Anything that could reach the UDP port had a report relayed toward the SMF, and, because
a report that cannot be relayed is rejected rather than dropped, was answered too. Each answer
costs a resend transaction — a goroutine and a 15 s timer — keyed in ConsumerTable by the
peer's address, a key PutTransaction creates and nothing ever deletes. Until this branch the
adapter only ever sent requests, so that key was the socket's own address and there was one of
them; relaying is what made it peer-chosen.

The set of user planes is learned from UpNodeID, which the SMF names on every message it sends
through us, and a report's source address is matched against it. Deliberately not taken from
UpfCfg.UPFs: that table is filled only on the association-setup path, and an adapter restart
empties it while the sessions stay established. Nothing then refills it — the heartbeat responses
the SMF gets carry the user plane's own recovery timestamp rather than ours, and a heartbeat that
fails to send while the adapter is down is not counted as a miss, because that increment sits in
the success branch. An association-gated relay would therefore drop every report for the life of
the association while everything else kept working. Learning from ordinary traffic comes back
within one heartbeat, the same way the SMF's own address does.

An unrecognised source is dropped, not rejected: it holds no session here, so it is owed no
answer, and answering it is what costs the state. The check runs before the no-SMF-address
rejection, so the only peers the adapter ever sends to unprompted are ones the SMF named. The
drop is logged with the peer and deliberately not rate-limited — the realistic case is a user
plane whose source address differs from its node ID, and that has to stay diagnosable; one Warn
is cheaper than the goroutine and 15 s timer an answer would cost.

What this is not: the set is learned from the same unauthenticated HTTP body that already names
the UPF every PFCP message is forwarded to, so it adds no new trust and removes none, and a
spoofed source that matches a real user plane still passes. N4 has no transport authentication;
this is recognition, not authentication, and it does not make the endpoint safe. Responses are
also not gated — one is matched to its outstanding request by sequence number, as every other
response type already is, so checking the sender there is a change to the transaction layer
rather than to the relay. Both belong with authenticating the endpoint, in their own change.

One behaviour this does change: a user plane whose configured node ID resolves to a different
address than it sends from will now lose MT paging. It fails visibly, with a log line naming the
peer, rather than silently.

The same limit applies to recognising a retransmission, which is by exchange identity and not by
content: two reports from a known user plane's address under one sequence number, inside the few
seconds an exchange is outstanding, are one exchange here and the second is dropped. A spoofed
source matching a real user plane already passes the gate above, and this is the same property
rather than a new one. Parsing the claimed SMF address is input validation on a field the adapter
already trusted — it keeps a malformed claim from erasing a working address, and it authenticates
nothing.

Two smaller things this made necessary

• A response sent to a user plane keeps its own error handler. The resend transaction ends in a
  timeout once the peer stops retransmitting — the ordinary outcome of a delivered response —
  and HandlePfcpSendError announced that as a message it had been unable to send.
• The retransmission check compared an error against a spelling that error never had
  ("Receive resend PFCP request" vs "receive resend PFCP request"), so every retransmission
  was logged as a read failure. Harmless while no user-plane-originated request was handled at
  all; reachable now, so it is replaced with a sentinel that readPfcpMessage returns unwrapped
  and the read loop compares directly.

Two behaviours added in review, worth knowing about

An address stops being authorised once the SMF stops naming its user plane. The set of user
planes reports may be relayed for is learned from SMF traffic, so nothing used to remove an entry:
a node that moved, or one that went away, left its old address authorised for the life of the
process, and whatever took that address over was relayed for as though the SMF had named it.
Addresses now carry the time the SMF last named the node and are dropped after 30 minutes of
silence. That is 180 of the SMF's heartbeat periods, and every message the SMF forwards names its
user plane, so a live node is renamed constantly and only one the SMF has stopped talking to
altogether ages out. A name that fails to resolve does not revoke the address — the node is
still being named, and a DNS blip is not a user plane that has gone. The age is tested where
reports arrive as well as on the sweep: the sweep runs on SMF traffic, so an SMF that has gone
quiet is both the case the lifetime exists for and the case in which nothing would sweep.

A report that cannot be numbered is rejected rather than misdelivered. The adapter's own
sequence range is finite, and taking the next number regardless would, once the counter came
round, replace an exchange still outstanding: the SMF's answer to the first report would go back
to the user plane that raised the second. The allocator now skips numbers still in use and
reports ErrRelaySequenceExhausted when the range is full, which the handler turns into a
rejection toward the originating user plane.

Testing

One test is a race test, and its numbers are worth having.
TestPutTransactionAdmitsOneSenderPerSequence puts eight senders under one sequence number.
Against the previous check-then-store shape it fails 13 runs in 200 by default and 63 in 200
under -race, admitting two to six senders where one may pass — so it catches the defect
intermittently rather than every time. TestTxTableRefusesASecondSenderUnderOneSequence pins the
contract deterministically, but does not distinguish the two shapes on its own, since both refuse
a sequential second insert.

linux/amd64, in the images CI pins:

• go test -race ./... green in golang:1.27.1-bookworm, the Dockerfile's pin
• golangci-lint run — 0 issues in golangci/golangci-lint:v2.13.2, the workflow's pin
• pre-commit run --all-files at the repo's own pins — gitleaks, gci, staticcheck, yamlfmt,
  reuse and the Go hooks — every hook passing

Unit tests cover the bookkeeping: the address is remembered, an empty one does not erase it and
neither does one that is not an IP, what the getter hands out is a copy, the origin is taken
exactly once, two user planes using the same number stay distinguishable, a retransmission
resolves to the relay in flight and consumes no sequence number of its own, one whose entry has
expired is relayed afresh, the counter wraps inside its own range, and entries the SMF never
answered age out, and two ports at one address stay two peers.

Five mutations were checked, each failing exactly one test: storing an unparsed claim, handing out
the stored slice, never matching a retransmission, letting an expired entry be matched instead of
swept, and dropping the port from the peer's identity. One of those pins a property rather than a
fix — the getter handed out a copy before this round too, since it parsed on every call, and the
test is there so that is not quietly lost.

The source gate is covered at the config level — recorded and matched in both 4- and 16-octet
form, a source that was never named refused, an empty set refusing everything, an FQDN node ID
matched by the address it resolves to, and an unspecified address recorded as nothing. Making
IsKnownUpfAddr always return true fails three of them; making RecordUpfAddr record nothing
fails two. There is no test harness for pfcp/handler, so the gate's placement inside
HandlePfcpSessionReportRequest is not pinned by a test.

Verified end to end on a live deployment with enableUPFAdapter: true. A downlink packet to an
idle UE produced:

20:03:02.446  UPF -> adapter   Session Report Request  seq[1]
20:03:02.447  adapter -> SMF   Session Report Request  seq[8388608]   <- adapter's own range
20:03:02.456  SMF -> adapter   Session Report Response seq[8388608]   Cause: Request accepted
20:03:02.4560 Paging on N2
20:03:02.4572 adapter -> UPF   Session Report Response seq[1]         <- the peer's own number
20:03:02.4636 Service request

and the UE's user plane restored 18 ms after the page, where before the report was dropped. That
run predates the source gate and was taken in a topology where the user plane sends from the
address the SMF names it by; a paging run with the gate in place has not been done, so it is
verified by unit test and by reading, not on the wire.

The retransmission case is not on the wire either, and cannot be with this project's own user
plane: handleDigestReport sends the report with SendPFCPMsg rather than
sendPFCPRequestMessage, so the UPF transmits it once and never retries — an unanswered downlink
data report is simply lost there. Retransmitting an unanswered request is ordinary PFCP behaviour
for a user plane that does it, and the adapter sits on N4 for whatever is deployed behind it, so I
would rather it behave than rely on that. Stated here so the handling is not read as something a
run demonstrated.

Note for reviewers

The PfcpPort constant is duplicated in config rather than imported from udp, because udp
imports config. If you would rather have it moved somewhere both can import, say so and I will
restructure.

[Copilot]

Pull request overview

This PR adds end-to-end handling for UPF-originated PFCP Session Reports (including Downlink Data Reports) by relaying Session Report Requests to the SMF and returning the SMF’s response back to the originating UPF, enabling mobile-terminated reachability (paging) when the adapter sits on N4.

Changes:

• Extend the PFCP UDP receive path to surface the remote peer address and update the dispatcher/handlers to route Session Report Request/Response.
• Record the SMF address (from SMF-initiated messages) and introduce a relay sequence renumbering table to avoid sequence collisions between UPF-originated reports and the adapter/SMF transaction space.
• Add unit tests covering SMF address bookkeeping and relay-sequence mapping behavior (including multi-UPF collisions and wrap/aging behavior).

Reviewed changes

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

Show a summary per file

File	Description
upfadapter.go	Records the SMF address from incoming SMF-initiated HTTP messages to support relaying UPF-originated PFCP messages.
pfcp/udp/udp.go	Returns remote peer address from reads; introduces a sentinel resend error and updates dispatch signature accordingly.
pfcp/handler/handler.go	Implements Session Report Request relay to SMF, response return to UPF, and improved handling for response resend-window timeout logging.
pfcp/dispatcher.go	Routes Session Report Request/Response message types to the new handler logic and passes through remote peer address.
config/config.go	Adds SMF address storage and relay sequence/origin bookkeeping with an adapter-owned sequence range.
config/relay_test.go	Adds unit tests validating SMF address persistence and relay origin/sequence bookkeeping.

---

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

[midwell]

Thanks — both addressed in 5ee9e98.

The relay address. The concern is real, but I don't think a refusal is the right shape here. The SMF fills that field with its own first non-loopback address, which is the peer address in an ordinary deployment — but not in one where the SMF is multi-homed or reached through a proxy, and rejecting there would break the relay for a difference that is legitimate. So a claim that doesn't match the peer is now reported instead, once per change rather than per message, since every PFCP message the SMF sends arrives on this path.

Worth saying plainly, though: that doesn't make the endpoint safe, and I'd rather not imply it does. The same body already names UpNodeID, and the handler forwards PFCP to whatever it says — so anything that can reach this endpoint already directs traffic. Authenticating the endpoint is the fix for that, and it belongs in its own change rather than here.

The stale-relay log. Correct, and worse than ambiguous: the map is keyed by the adapter's own sequence while the line printed the user plane's, and several user planes can be waiting on the same number — so it named an entry nobody could look up. It now carries both numbers and the peer address.

One unrelated fix in the same push: govet's fieldalignment flags reportRelay under the v2.13.2 golangci-lint this repo now pins, so its fields are reordered.

[Copilot]

🟡 Changes recommended

The new SMF address recording and session-report relay paths introduce failure/duplication risks (invalid SMF address overwriting, nil-safety, and potential duplicate relays on UPF retransmits) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect sequence allocation, address tracking, and retransmission/error handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

pfcp/handler/handler.go:307

• The relay entry is already removed before this response transaction starts. If its first WriteToUDP fails, this callback only logs the error; the UPF receives neither the SMF response nor a rejection, and its next retry is treated as a new report because the deduplication entry is gone. Preserve the origin for this failure path and reject or otherwise complete the UPF transaction instead of silently losing it.

	return udp.PfcpEventData{LSEID: 0, ErrHandler: func(msg message.Message, err error) {
		logger.PfcpLog.Debugf("resend window closed for session report response seq[%d]: %v",
			msg.Sequence(), err)
	}}

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

[midwell]

@gab-arrobo Four comments on f523002, all four real and answered in 3e3d455. Reasoning is on
the individual threads; the short form:

• A claimed SMF address that is unspecified (0.0.0.0, ::) is refused, so one claim can no
  longer erase a working address. RecordUpfAddr already applied that test to a user plane's
  address; SetSmfAddr applies it too.
• The set of user planes the SMF has named is held per node identity now, so one that moves stops
  authorising the address it left. An address is dropped only once no identity remains at it,
  because the SMF may name the same user plane by FQDN in one message and by IP in another.
• A relayed report is tried under the next sequence number when the transaction table already
  holds one. NewTransaction keys a request by the adapter's own socket, so the SMF's numbering
  and the adapter's share one table; starting at the halfway point separated them by convention,
  and the convention ends once the SMF's counter reaches that half — which this code cannot see.
  udp exports ErrDuplicateSequence so the caller can tell that case from a send that failed.
• The origin of a relayed report is kept until its answer has been sent rather than dropped when
  the answer is claimed. The response transaction that absorbs retransmissions exists only after
  the send, and udp.Run dispatches each message on its own goroutine, so the gap was reachable —
  and a report relayed twice is a second downlink data notification and a second page, which is
  the defect this branch exists to remove. Claiming stays take-once, and only the path that
  claimed releases.

Each is pinned by a test that fails without it: I removed each fix in turn and watched its own
test die, rather than trusting one run of the suite.

Gates on the pushed tree: build, vet, go test -race ./..., golangci-lint v2.13.2 at 0 issues,
gofmt, and make check-reuse compliant — linux/amd64, in CI's own images.

Two limits worth naming rather than leaving to be discovered:

• The retry backstop is one-directional. It covers the report relayed toward the SMF, which is
  where the shared table is: a request is keyed by the adapter's own socket. The two sends back
  toward a user plane are responses, keyed by destination, under that peer's own sequence
  number — a much narrower collision domain — so they are left without a retry rather than given
  one nothing is known to need.
• An FQDN identity is compared byte for byte, so an SMF that varied the case of one hostname
  would look like two identities at one address rather than one that moved. That is the existing
  convention in this package — GetUpfFromNodeId compares FQDNs the same way — and this change
  neither introduces nor widens it.

[Copilot]

🟡 Changes recommended

Unresolved sequence-safety, address-expiry, and transaction-state issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

config/config.go:382

• This loop scans every outstanding relay for every incoming report, while holding the global relay mutex. With a slow SMF or a burst of reports, N in-flight entries makes each report O(N), producing O(N²) comparisons and serializing all report handling; that can delay the paging notification the relay is meant to deliver. Keep a keyed (peer address/port, UPF sequence) index for deduplication and use a separate expiry mechanism rather than a full-map scan on every allocation.

	for held, relay := range reportRelays {
		if now.Sub(relay.recorded) > reportRelayLifetime {
			delete(reportRelays, held)
			// Both numbers, and the peer: the map is keyed by the adapter's own sequence while the
			// user plane is waiting on its own, and several user planes can be waiting on the same

pfcp/handler/handler.go:254

• The four-attempt bound can reject a valid paging-triggering report even though the relay range still has millions of free sequence numbers. Four legitimate SMF-initiated requests can occupy the consecutive high-half values that this allocator tries, causing all four SendPfcp calls to return ErrDuplicateSequence and the report to be rejected for a transient collision. Continue searching until a free sequence is found (with an exhaustion bound over the whole relay range) instead of treating four collisions as exhaustion.

	for attempt := 1; attempt <= relaySendAttempts; attempt++ {
		relaySeq, fresh := config.RelayReportSequence(upfAddr, upfSeq, time.Now())
		if !fresh {
			// A retransmission of a report still being relayed. Relaying it again would raise a

pfcp/handler/handler.go:325

• This new response path creates a ConsumerTable entry keyed by the UPF's address/port, but removeTransaction only deletes the sequence entry and never removes an empty peer table. Consequently, every distinct source address (or source port, which the gate allows) that receives a report response is retained for the process lifetime; moving UPFs or changing NAT ports can make this map grow without bound. Clean up empty peer tables with concurrency-safe ownership, or otherwise bound this peer-keyed state.

	if err := udp.SendPfcp(response, upfAddr, reportResponseEventData()); err != nil {

pfcp/handler/handler.go:343

• This new response path relies on the response transaction to absorb retransmissions, but Transaction.Start creates one 15-second timer before its resend loop (pfcp/udp/transaction.go:120) and never resets it after ReceiveResendRequest. A report retransmitted near that deadline is answered once more and then the transaction is removed immediately; a later copy is re-relayed as a new report because this handler has already forgotten the relay entry, which can produce duplicate paging. Reset or extend the response timer when a retransmission is received.

// reportResponseEventData reports the end of a response's resend window for what it is.
// A response transaction holds the message so a retransmitted report is answered again,
// and closes with a timeout once the peer stops asking -- the ordinary outcome of a
// delivered response. HandlePfcpSendError would announce that as a message it was unable
// to send, and a false negative on this path is what made the original defect so hard to
// find.
func reportResponseEventData() udp.PfcpEventData {
	return udp.PfcpEventData{LSEID: 0, ErrHandler: func(msg message.Message, err error) {
		logger.PfcpLog.Debugf("resend window closed for session report response seq[%d]: %v",
			msg.Sequence(), err)
	}}

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved issues remain in expiry enforcement, sequence allocation, and transaction namespace coordination.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

config/config.go:472

• This check only considers reportRelays, while udp.PutTransaction inserts ordinary SMF-originated requests into the same local-address transaction table. If a relay occupies a sequence first and the SMF later sends an ordinary request with that sequence, PutTransaction rejects the ordinary request as ErrDuplicateSequence; unlike this path, that request cannot be renumbered, so the HTTP transaction fails. The relay IDs need a namespace unavailable to ordinary requests, or the two transaction namespaces must be coordinated in both directions.

		if _, taken := reportRelays[reportRelaySeq]; taken {
			continue
		}

		reportRelays[reportRelaySeq] = reportRelay{upfAddr: upfAddr, upfSeq: upfSeq, recorded: now}

upfadapter.go:63

• This warning claims the adapter will relay to SmfIp, but config.SetSmfAddr now rejects malformed and unspecified claims on the following lines. A bad claim therefore produces a misleading warning (and may leave the adapter with no destination at all); validate the claim before emitting this message or describe that it is only a claim being validated.

		if current := config.SmfAddr(); current == nil || current.IP.String() != udpPodMsg.SmfIp {
			logger.AppLog.Warnf("message claims SMF address [%s] but arrived from [%s]; relaying to the claimed address",
				udpPodMsg.SmfIp, host)

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved peer-state cleanup, relay-scan performance, and test-isolation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

config/config.go:464

• This loop scans every outstanding report while holding the global relay mutex for every incoming report, even when the peer/sequence is not present. Under a burst with many reports in flight, handlers serialize and the work per report grows linearly with outstanding state, which can delay relays and paging. Keep a keyed retransmission index (with separate expiry/sequence occupancy) or otherwise avoid the full-map scan.

config/relay_test.go:520

• This test does not isolate reportRelays and leaves the repeated entry created at line 538 in the package-global map. Neighboring tests use withReportRelays; without isolation, a later test using the same peer/sequence can become order-dependent and falsely classify its first report as a retransmission. Add the test isolation helper at the start.

func TestTakeReportRelayKeepsTheEntryUntilItIsForgotten(t *testing.T) {
	upfAddr := &net.UDPAddr{IP: net.ParseIP("10.42.0.220"), Port: PfcpPort}
	now := time.Now()

upfadapter.go:61

• The guard compares the stored, canonical net.IP back to the original text. Equivalent IPv6 spellings (for example, compressed versus expanded form) therefore look changed on every request, so a persistent peer/claim mismatch emits a warning for every PFCP message instead of once per change. Compare parsed addresses with net.IP.Equal here.

		if current := config.SmfAddr(); current == nil || current.IP.String() != udpPodMsg.SmfIp {

upfadapter.go:63

• This warning is emitted before SetSmfAddr validates the claim. For a malformed or unspecified SmfIp, the log says the adapter is relaying to that address even though SetSmfAddr rejects it and preserves the previous destination, which makes the diagnostic misleading during exactly the failure this validation is meant to handle. Validate the claim before logging this message, or have the setter report whether it accepted the address.

		if current := config.SmfAddr(); current == nil || current.IP.String() != udpPodMsg.SmfIp {
			logger.AppLog.Warnf("message claims SMF address [%s] but arrived from [%s]; relaying to the claimed address",
				udpPodMsg.SmfIp, host)

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved transaction-lifecycle, authorization, cleanup, and error-reporting issues remain, including one critical finding.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

Previously missed (1) — in code that hasn't changed since the last review.

config/config.go:472

• This sweep holds the global relay mutex while scanning every outstanding entry on each report to expire entries and find a (peer, UPF sequence) match. With the 30-second retention window, admission is O(number of in-flight reports) and all UPFs serialize behind the scan, so a burst can turn relay handling into a CPU/latency bottleneck. Keep a secondary dedup index keyed by peer plus UPF sequence and use an expiry queue/heap (or another bounded cleanup structure) instead of a full-map scan.

config/config.go:434

• When the SMF goes quiet, this branch only returns false; it does not remove the stale address/node records or report the released host to udp.ForgetConsumer. Since RecordUpfAddr is the only sweep that performs that release, a peer that was answered before aging out leaves an empty ConsumerTable entry (and the authorization indexes) indefinitely, so the new 30-minute bound does not bound per-peer state in the quiet-SMF case. Add a cleanup path for age-based expiry (or a periodic sweep/callback) that can release the corresponding UDP state without holding this read lock.

	for node := range upfAddrs[ip.String()] {
		if now.Sub(upfNodeAddrs[node].seen) <= upfAddrLifetime {
			return true
		}
	}

pfcp/handler/handler.go:213

• Authorization is enforced only after readPfcpMessage has already run findTransaction for requests. If a UPF is deauthorized while its response transaction is still present (or the lookup races ForgetConsumer), a retransmitted Session Report is consumed there, causing that transaction to resend without reaching this IsKnownUpfAddr check. That violates the drop-on-unknown behavior; perform the source check before duplicate lookup for Session Report requests or make lookup authorization-aware.

	if !config.IsKnownUpfAddr(upfAddr.IP) {

pfcp/handler/handler.go:335

• Deauthorization can remove the UPF's ConsumerTable while this SMF response is in flight. Because SendPfcp is allowed to run for the stored origin without rechecking authorization, it can recreate a table for an address that has already been released; after the 15-second response transaction is removed, the empty table is never reclaimed. Repeated moves with in-flight reports can therefore reintroduce the per-peer leak this cleanup is meant to prevent. Coordinate this send with deauthorization or add cleanup for recreated tables.

	if err := udp.SendPfcp(response, upfAddr, reportResponseEventData()); err != nil {

pfcp/handler/handler.go:353

• This callback receives both the normal response-window timeout and an actual WriteToUDP failure, but logs both only at Debug level. A closed socket or other send error can therefore leave the UPF without a response and be invisible at normal log levels; preserve the quiet timeout handling while reporting transport failures as errors (and, if appropriate, retrying them).

func reportResponseEventData() udp.PfcpEventData {
	return udp.PfcpEventData{LSEID: 0, ErrHandler: func(msg message.Message, err error) {
		logger.PfcpLog.Debugf("resend window closed for session report response seq[%d]: %v",
			msg.Sequence(), err)
	}}

pfcp/udp/udp.go:130

• This makes duplicate-sequence handling asymmetric: the relay retries when it loses the LoadOrStore, but ordinary SMF-initiated requests still return this error directly from ForwardPfcpMsgToUpf. If a report reserves a relay ID first and an SMF request happens to use that same sequence concurrently, the legitimate SMF request is rejected even though the relay could have moved to another ID. Coordinate allocation so a relay cannot claim an ID needed by an SMF request, or add an equivalent retry/reservation path for ordinary requests.

	if _, loaded := txTable.LoadOrStore(tx.SequenceNumber, tx); loaded {
		return fmt.Errorf("insert tx error: %w %d", ErrDuplicateSequence, tx.SequenceNumber)

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate relay correctness, cleanup, expiry, and scalability issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

config/config.go:499

• This allocator only avoids sequence numbers already present in reportRelays; it does not reserve the shared udp.ConsumerTable slot atomically with ordinary SMF requests. If an SMF-originated request chooses this number after the relay entry is created, its normal SendPfcp path reaches PutTransaction, gets ErrDuplicateSequence, and ForwardPfcpMsgToUpf has no retry path, so an unrelated control-plane request fails because a report is in flight. Coordinate relay allocation with the common transaction table (or otherwise isolate/reserve the sequence space) so the collision cannot reject the ordinary request.

	candidate, free := nextFreeRelaySequence(reportRelays, reportRelaySeq, relaySequenceFloor, relaySequenceCeil)

config/config.go:476

• This is a lazy expiry sweep: an old reportRelay is deleted only when another report calls RelayReportSequence. If an unanswered burst is followed by silence, every origin remains in reportRelays indefinitely, so the claimed 30-second bound does not hold and a burst can permanently retain a large map. Add a timer/expiry queue or otherwise clean entries independently of new reports.

	for held, relay := range reportRelays {
		if now.Sub(relay.recorded) > reportRelayLifetime {
			delete(reportRelays, held)

pfcp/handler/handler.go:23

• The fixed limit can reject a report even when a usable relay sequence exists: four consecutive candidates may be occupied by legitimate SMF-originated transactions while the fifth is free, but the loop stops and sends RequestRejected. Since the allocator already skips occupied numbers and only reports true exhaustion for a full range, retry until a bounded full-range scan (or otherwise choose a free candidate) instead of treating four collisions as exhaustion.

// relaySendAttempts bounds how many sequence numbers a single report may try before it is
// rejected. Each retry costs one map insertion, and a peer numbering requests fast enough to
// take several of the adapter's numbers in a row is not a peer more attempts would help.
const relaySendAttempts = 4

pfcp/handler/handler.go:335

• This creates the SendingResponse transaction used to absorb UPF retransmissions, but Transaction.Start stops after NumOfResend (three) resend events rather than keeping the entry for the full 15-second response window. A fourth retransmission within that window is then dispatched as a new Session Report after ForgetReportRelay has run, causing a second SMF notification/page; retain the response transaction for the whole window or keep the relay deduplication entry until it expires.

	if err := udp.SendPfcp(response, upfAddr, reportResponseEventData()); err != nil {

• Files reviewed: 10/10 changed files
• Comments generated: 3
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A critical stale-claim error path and two moderate bookkeeping/test-isolation issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

config/relay_test.go:526

• This test mutates the package-global relay map and sequence counter but does not call withReportRelays, and its final repeated claim is never forgotten. Later tests inherit that live claim and counter value, making outcomes depend on test order and leaving state behind for repeated runs. Isolate the test with withReportRelays(t) (or explicitly clean the final claim).

pfcp/handler/handler.go:384

• This defer drops the relay claim even when udp.SendPfcp returns an error before registering a response transaction (for example, when the server is unavailable or the sequence is duplicated). A retransmitted report can then find neither the claim nor a response transaction and be relayed to the SMF again, producing a duplicate downlink notification/page. Keep the claim on the error path and forget it only after SendPfcp succeeds.

	defer config.ForgetReportRelay(relaySeq)

• Files reviewed: 12/12 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unresolved critical relay-lifecycle and sequence-collision issues can duplicate reports or fail normal N4 traffic.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

pfcp/handler/handler.go:448

• rejectSessionReport has the same asynchronous contract: a nil error only means the response was registered, not delivered. If its initial UDP write fails, startTxLifeCycle removes the response transaction after this function has forgotten the claim; a UPF retransmission then creates a second relay and page. Coordinate claim cleanup with the actual send result rather than treating SendPfcp returning nil as delivery.

	config.ForgetReportRelay(relaySeq)

pfcp/handler/handler.go:420

• This callback runs for both a normal response-window timeout and an actual initial WriteToUDP failure, but it always logs “resend window closed” at Debug level. A response that never left the socket is therefore indistinguishable from a delivered response whose peer stopped retransmitting and may be invisible at the normal log level; distinguish send failures from timeout and log the former as an operational error.

	return udp.PfcpEventData{LSEID: 0, ErrHandler: func(msg message.Message, err error) {
		logger.PfcpLog.Debugf("resend window closed for session report response seq[%d]: %v",
			msg.Sequence(), err)
	}}

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

[midwell]

Real, and fixed at 6be9a83. The deadline was started once outside the loop, so it measured from the first answer rather than from the last copy: a peer still retransmitting when it expired outlived the transaction that was absorbing those copies, and the next one arrived at the handler as a new request — a second downlink data notification for traffic already reported. Each resend now stops the timer, drains it if it had already fired, and resets it.

One limit does not change, and the commit says so: the loop is still capped at NumOfResend, so a fourth copy meets no transaction whatever the timing. This is about the window each of the three is given, which they previously shared.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical authorization-expiry race and moderate stale-transaction bypass block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Authorization expiry bypasses relay generation check · New
• This new response path relies on the SendingResponse transaction to absorb retransmissions, but…

Resolved since last review (3)

• PutTransaction is shared by ordinary SMF→UPF requests as well as relayed reports, but only…
• udp.SendPfcp returns as soon as the response transaction is registered; the actual WriteToUDP…
• The retry only handles the relay side of a sequence collision. The same ConsumerTable is used by…

[Copilot]

Copilot review overview

🟡 Changes recommended

Resolve the critical sequence-reservation race and the IPv6 address comparison issue.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Prevent relay sequence reuse from misrouting stale SMF responses · New
• This new response path relies on the SendingResponse transaction to absorb retransmissions, but…

Resolved since last review (1)

• Authorization expiry bypasses relay generation check

[midwell]

Checked the arithmetic and it does not reach. The relay range is the upper half of the 24-bit space, 0x800000–0xFFFFFF, and the allocator advances one number per relayed report — so coming back to a given number takes 8 388 608 relays. The outbound transaction it would have to still be alive for lives at most NumOfResend × ResendRequestTimeOutPeriod, which is 3 × 3 s. That is roughly 930 000 relayed session reports per second sustained for nine seconds, on one adapter.

The claim table's own reuse guard is what makes the ordinary case safe: nextFreeRelaySequence skips any number still claimed, so a number is only reusable once its claim has been dropped or aged out. What your case adds is that a released claim frees the number while the outbound transaction may still be in flight — true, and the window is the transaction's remaining lifetime against 8.4 million allocations. I would rather leave that than make response matching carry a generation, which is a change to how every N4 response is matched for a race that needs six orders of magnitude more traffic than this adapter sees.

[gab-arrobo]

@midwell,
Should I understand that Copilot's comment is incorrect/inaccurate?

[midwell]

@gab-arrobo
Copilot is in theory not wrong, but the issue will never happen as it takes 8.4 million requests in a 9 second period to reach the limit. That is 8.4 million concurrent sessions all notifying at once.

The code change is only about 15-20 lines, so that's no problem. What hurts is the test. A race test up to 8M would take around 2 seconds on my Mac. With pre-commit's count=20 it would realistically be around 30s-120s added test time. It could be solved by making relaySequenceFloor/relaySequenceCeil variables so the test range could be made smaller.

What do you recommend, shall I add the guard and fixes to make it testable or leave it with a comment?

[gab-arrobo]

@midwell, I think we can leave it as-is because the pre-commit hooks do not run in the GHA. The pre-commit hooks are currently used only in local environments and the idea is to make things easier for developers to test their changes.
Does it mean this PR is ready to merge?