fix(core): isolate asynchronous packet ownership

[Kevin-layerV]

Summary

• give asynchronous transaction sends an independent sender-owned packet so transaction teardown cannot recycle bytes during a socket write
• preserve EncryptedPktCh delivery for response packets derived from PrevParserData
• add deterministic real-UDP packet-lifetime and response-channel exact-once regressions under the race detector

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Refactoring
• CI/CD improvement

Related Issues

Found while validating NHP REG/RAK response handling through asynchronous packet channels. No wire format or protocol message shape changes.

Testing

• Unit tests added/updated
• Manual testing performed
• All existing tests pass

Validated locally:

• cd nhp && go test -race ./...
• cd endpoints && go test -race ./agent ./relay
• focused packet-lifetime race regression, 50 repetitions
• focused response-channel identity/exact-once race regressions, 50 repetitions
• go vet ./..., go build ./..., and golangci-lint 2.7.2 in both modules
• gofmt, goimports, and git diff --check

The full endpoints suite on unmodified upstream main has two local Go 1.26/environment baselines: a POST redirect assertion expects 301 while net/http returns 307, and server KBS initialization writes under /opt. The changed agent/relay packages and the complete NHP module pass under -race; upstream CI will remain authoritative for the full matrix.

Checklist

• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No new warnings introduced

[Kevin-layerV]

@claude /simplify

[github-actions]

✅ Looks Good - Code looks good

Thorough review of the packet-ownership fix. This is a well-constructed, correct concurrency fix with strong regression coverage. No blocking issues found.

What the change does (and why it's correct)

The bug being fixed (a genuine use-after-free / data race):
In the base code, a transaction request's mad.BasePacket was marked KeepAfterSend = true and handed to the asynchronous physical sender via ForwardOutboundPacket → SendQueue → SendPacket, which does netConn.Write(pkt.Content). That same BasePacket is retained by the LocalTransaction. On connection close / device stop / timeout, LocalTransaction.Run's deferred t.mad.Destroy() calls ReleasePoolPacket(BasePacket) — niling Content and returning the buffer to the pool. If teardown lands while the socket write is in flight, the write races on / reads a recycled buffer (empty or corrupted datagram). The new endpoints/agent/packet_lifetime_test.go reproduces exactly this window deterministically.

The fix — independent sender-owned packet (nhp/core/packet.go, device.go):
clonePacketForSend allocates a fresh pool packet, copies HeaderType + Content, and leaves KeepAfterSend = false. The sender now owns and releases the clone after the write, while the transaction keeps exclusive ownership of BasePacket. I verified:

• The transaction never re-sends BasePacket over the wire (transaction.go only uses t.mad as PrevAssemblerData for response crypto), so dropping KeepAfterSend = true is safe — BasePacket is released solely by t.mad.Destroy().
• Destroy() (initiator.go:443) releases only BasePacket; the clone has a separate Buf, so there is no double-free between the transaction and the sender.
• The outboundPacketOwned flag + deferred release correctly plugs the panic window between clonePacketForSend and ForwardOutboundPacket (AddLocalTransaction). outboundPacketOwned == true implies mad != nil; it's cleared after handoff, and ForwardOutboundPacket takes ownership (enqueue → drained by SendPacket/Close flush, or released on closed conn), so no leak and no double-release on any path.
• clonePacketForSend bounds-checks len(Content) against PacketBufferSize before the copy.

Response-channel fix (initiator.go):
Moving mad.encryptedPktCh = md.EncryptedPktCh out of the PrevParserData == nil branch is safe and backward-compatible: deriveMsgAssemblerData never sets encryptedPktCh, so this doesn't overwrite anything, and no existing production caller sets both PrevParserData and EncryptedPktCh (the only EncryptedPktCh user, agent/knock.go, is a new-initiator path). For all current PrevParserData callers md.EncryptedPktCh is nil, so behavior is unchanged; the change only enables response-side callers to divert encrypted bytes instead of silently falling through to SendQueue. Covered by encrypted_channel_test.go's exact-once assertion.

Minor, non-blocking notes

• Each transaction request now incurs an extra pool allocation + Content memcpy. This is negligible and scoped to transaction requests only — a reasonable price for eliminating the shared-ownership race.
• packet_lifetime_test.go uses a log.Info function-field override as a blocking pre-write seam. It's clever and effective; just note it depends on the exact "Send [" log prefix in UdpAgent.SendPacket, so a future log-message change would silently defang the test rather than fail it.

Nice work — the ownership boundaries are now clean and the race-detector regressions document both contracts well.

[github-actions]

✅ Looks Good - Code looks good

Thorough, well-motivated fix for a real data-race in asynchronous packet ownership, backed by deterministic race-detector regressions. I traced the ownership paths end-to-end and it holds up.

What the fix does (and why it's correct)

• nhp/core/device.go / packet.go — Previously a transaction request set mad.BasePacket.KeepAfterSend = true and handed the same pooled Packet to the physical sender. Transaction teardown (mad.Destroy() → ReleasePoolPacket) could recycle that buffer while netConn.Write(pkt.Content) was still reading it, producing an empty/garbled datagram and racing under load. The new clonePacketForSend gives the sender an independently-owned pool packet (KeepAfterSend=false, released after the write) while the transaction retains and destroys BasePacket. Clean ownership split.

  • Verified LocalTransaction.Run (nhp/core/transaction.go) never retransmits BasePacket — it only awaits response/timeout/stop then Destroys — so dropping KeepAfterSend on the base packet is safe.
  • The outboundPacketOwned flag correctly gates the deferred release on the error/panic path and is cleared after ForwardOutboundPacket; the clone and BasePacket are distinct buffers, so a concurrent teardown cannot double-free.
  • The clone copies HeaderType + Content and AllocatePoolPacket supplies Buf/PoolAllocated — complete for everything the senders (udpagent.go, relay.go) consume.

• nhp/core/initiator.go — Hoisting mad.encryptedPktCh = md.EncryptedPktCh out of the new-initiator branch fixes responses derived from PrevParserData, which previously dropped the channel (never set by deriveMsgAssemblerData) and silently fell through to the socket SendQueue. Backward compatible: only changes behavior when a caller explicitly sets EncryptedPktCh.

Minor (non-blocking) suggestions

1. endpoints/agent/packet_lifetime_test.go installs a custom global logger Info hook via SetGlobalLogger and never restores it, so later tests in the package inherit the silent logger + hook. It's harmless (the closed releaseSend/enteredSend channels return immediately for any subsequent Send [ log), but consider restoring the prior global logger in a t.Cleanup for hygiene.
2. Each transaction send now incurs an extra pool allocation + content copy for the clone. Pool-backed and released promptly, so acceptable given the correctness win — just noting it.

No security, correctness, or breaking-change concerns found. Nice work isolating the seam deterministically rather than relying on load to reproduce.