Skip to content

fix: prevent connection sends from racing close - #1630

Draft
justin-layerv wants to merge 1 commit into
OpenNHP:mainfrom
justin-layerv:justin/upstream-connection-channel-race
Draft

fix: prevent connection sends from racing close#1630
justin-layerv wants to merge 1 commit into
OpenNHP:mainfrom
justin-layerv:justin/upstream-connection-channel-race

Conversation

@justin-layerv

Copy link
Copy Markdown
Contributor

What changed

  • Serialized channel-send registration with ConnectionData.Close and waited for in-flight senders before closing queues and signal channels.
  • Made concurrent Close calls atomic and kept channel fields stable after closure.
  • Guarded outbound, inbound, block, and timeout sends through the same lifecycle mechanism.
  • Routed the server's initial inbound packet through ForwardInboundPacket instead of a raw queue send.
  • Made endpoint connection routines distinguish a closed signal channel from a real timeout/block signal.
  • Added focused race-stress tests covering packet sends, timeout/block signals, concurrent close, and blocked senders.

Why

The previous IsClosed check was separate from the channel send. Close could run between them, close and nil the channel, and make the sender panic or race on the channel field. Concurrent Close calls could also both pass the check. In addition, a closed BlockSignal could be interpreted as a genuine request to block the peer.

The lifecycle gate prevents new sends once closure starts, and Close waits for already admitted sends to leave their select before it drains and closes channels. This removes process-killing teardown races without recovery or retry behavior and improves endpoint availability under connection churn and UDP flood pressure. It ports the repository-agnostic core of layervai/nhp#2691 to current OpenNHP.

Validation

  • go test -race -run 'TestConnectionData' -count=10 ./core (nhp module)
  • go test -race -count=1 ./... (nhp module)
  • go test -race -count=1 ./ac ./agent ./db (endpoints module)
  • go test -race -c ./server and go test -race -c ./relay (endpoints module)
  • go vet ./... in both modules

The 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/relay race builds succeed.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 46.29630% with 29 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
endpoints/server/udpserver.go 0.00% 7 Missing ⚠️
endpoints/ac/udpac.go 0.00% 6 Missing ⚠️
endpoints/agent/udpagent.go 0.00% 6 Missing ⚠️
endpoints/db/udpdevice.go 0.00% 6 Missing ⚠️
endpoints/relay/relay.go 0.00% 3 Missing ⚠️
nhp/core/udpconn.go 96.15% 1 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1630      +/-   ##
==========================================
+ Coverage   12.54%   12.98%   +0.44%     
==========================================
  Files          96       96              
  Lines       14526    14560      +34     
==========================================
+ Hits         1822     1891      +69     
+ Misses      12526    12492      -34     
+ Partials      178      177       -1     
Flag Coverage Δ
unittests 12.98% <46.29%> (+0.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
nhp/core/udpconn.go 85.71% <96.15%> (+67.04%) ⬆️
endpoints/relay/relay.go 19.66% <0.00%> (-0.08%) ⬇️
endpoints/ac/udpac.go 0.00% <0.00%> (ø)
endpoints/agent/udpagent.go 5.43% <0.00%> (-0.03%) ⬇️
endpoints/db/udpdevice.go 0.00% <0.00%> (ø)
endpoints/server/udpserver.go 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks Good - Code looks good

Reviewed the connection-close race fix across the core lifecycle gate and all five endpoint connection routines. The concurrency design is sound and the change is well-scoped.

Correctness of the lifecycle gate (nhp/core/udpconn.go)

  • Send-admission vs. close is properly serialized. channelSendMu guards both beginChannelSend's channelSendWg.Add(1) and Close's closed.CompareAndSwap(false, true). Once Close sets closed=true under the lock, no further Add can happen, so channelSendWg.Wait() (invoked after the unlock) never races a positive Add — this is correct sync.WaitGroup usage and avoids the "Add called concurrently with Wait" panic.
  • No deadlock. close(c.StopSignal) runs before Wait(), so any already-admitted sender blocked in its select unblocks via the <-c.StopSignal branch, releases its packet, and calls Done(). TestConnectionDataCloseUnblocksPendingSends covers exactly this.
  • Keeping channels non-nil after close is the right call. Removing the old c.SendQueue = nil (etc.) assignments is what makes the new _, ok := <-ch receive checks fire (ok=false) so the routines exit cleanly. Nil-ing them would have left those ok branches dead (a nil channel case never becomes ready). I confirmed no other code depends on the fields being nil after close.

Receive side

All readers that could observe a closed channel now detect it: BlockSignal in all five endpoints, SetTimeoutSignal in the four that read it (relay intentionally has no SetTimeoutSignal case — it uses time.After(TimeoutMs)), and SendQueue/RecvQueue already checked ok. This closes the real bug where a closed BlockSignal was interpreted as a genuine block request (e.g. s.AddBlockAddr(...)).

Other checks

  • Routing the server's initial inbound packet through ForwardInboundPacket (was a raw RecvQueue <- pkt) means every send now goes through the gate — I confirmed no ungated raw sends to these channels remain.
  • The race-stress tests are focused and meaningful; the zero-value &Device{} harness is safe because ReleasePoolPacket no-ops on non-PoolAllocated packets.

Minor (non-blocking) observations

  • Every packet forward now briefly acquires the per-connection channelSendMu. The critical section is tiny (a single Add/Load) and per-connection, so contention is negligible — just noting it for the hot path.
  • The c.TimeoutMs = ms write in SetTimeout is still read from the routine's time.After(...) case without synchronization, but that's pre-existing and out of scope for this PR.

Nice, careful fix with good test coverage.

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.

1 participant