fix: prevent transaction sends from racing teardown - #1629
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Looks Good - Code looks good
This is a well-engineered fix for a genuine send on closed channel panic (process-terminating) that occurred when FindLocalTransaction/FindRemoteTransaction returned a transaction pointer whose Run cleanup then closed the destination channel before the caller's send. The approach is sound and I found no security, correctness, or breaking-change issues.
Why the fix is correct
- Lifecycle signal instead of channel close. Data channels (
NextPacketCh,NextMsgCh,ExternalMsgCh) are no longer closed on teardown; instead adonechannel is closed under the same mutex that removes the transaction from its lookup map (nhp/core/transaction.go:153-155and239-241). This makes the lookup→send sequence effectively atomic w.r.t. teardown: a stale caller either delivers or falls through to<-t.doneand getsErrTransactionClosed, never a panic and never a permanent block (bothRunselects always terminate via timeout/stop, sodoneis always eventually closed). - No consumer relied on the close. I confirmed via grep that the only readers of these channels are the single-shot
selectreceives inRun()— norangeor, okreads — so leaving them open for GC is safe. - Packet leak closed on the failure path. The local-packet forwarders now call
ReleasePoolPacket(pkt)whenSendPacketreturns an error (previously the send panicked; a released packet on the error path is correct since it was never delivered — no double-free, as the success path still hands ownership toRun).
Bonus latent-bug fix
Moving ExternalMsgCh initialization into newLocalTransaction (rather than creating it inside Run) also closes a pre-existing nil-channel window: AddLocalTransaction starts Run in a goroutine, so a caller that found the transaction and sent to ExternalMsgCh before Run scheduled would previously block on a nil channel. Nice catch.
Minor / informational (no action required)
sendMessageRoutine(endpoints/server/udpserver.go:932) dropsmdon theErrTransactionClosedpath without falling through toSendMsgToPacket. This is correct — the transaction is already dead so the response is undeliverable — and it's not a resource leak because a responseMsgDataonsendMsgChcarries onlyMessagebytes, not a pooledExternalPacket.- The new
transaction_test.gorace-stress tests are thorough (post-done send, concurrent send/exit races with and without a receiver, blocked-sender liveness, and cleanup ordering) and cover exactly the panic scenario being fixed.
Verified: all transaction constructions now go through the constructors (only nhp/core/device.go builds them), so done is never nil in production; done is closed exactly once per Run; and IsClosed() on the bare ConnectionData in the cleanup-ordering test is safe (atomic bool zero value).
Thanks for the clear PR description and the linked upstream context.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1629 +/- ##
==========================================
+ Coverage 12.54% 12.68% +0.13%
==========================================
Files 96 96
Lines 14526 14582 +56
==========================================
+ Hits 1822 1849 +27
- Misses 12526 12553 +27
- Partials 178 180 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
What changed
donesignals and safe send helpers for local and remote transactions.Why
FindLocalTransactionandFindRemoteTransactioncould return a transaction immediately before itsRuncleanup closed the destination channel. The caller then sent through that stale pointer and panicked withsend on closed channel, terminating the process. A plain pre-send closed check cannot make the lookup/send sequence atomic.The new helpers select between delivery and an immutable lifecycle signal. The data channels are left for garbage collection, while map removal and lifecycle closure share the lookup mutex. A stale caller now receives
ErrTransactionClosedinstead of panicking or remaining blocked.This improves server and endpoint availability during timeout, disconnect, and reconnect races. It ports the repository-agnostic core of layervai/nhp#1096 to current OpenNHP.
Validation
go test -race -run 'Test(Remote|Local)Transaction' -count=10 ./core(nhpmodule)go test -race -count=1 ./...(nhpmodule)go test -race -count=1 ./ac ./agent ./db(endpointsmodule)go test -race -c ./server(endpointsmodule)go vet ./...in both modulesThe full endpoint test sweep also reaches two unrelated current-main failures: relay expects a 301/400 redirect but receives 307, and server test initialization attempts to create
/opt/confidential-containers. The changed endpoint packages pass and the server race build succeeds.