Skip to content

fix(ac): recover UDP handler panics - #1628

Draft
justin-layerv wants to merge 1 commit into
OpenNHP:mainfrom
justin-layerv:justin/recover-ac-udp-handler-panics
Draft

fix(ac): recover UDP handler panics#1628
justin-layerv wants to merge 1 commit into
OpenNHP:mainfrom
justin-layerv:justin/recover-ac-udp-handler-panics

Conversation

@justin-layerv

Copy link
Copy Markdown
Contributor

Summary

  • contain panics at the AC's input-driven UDP handler boundary
  • keep the panic log scoped to AC identity and NHP message type, with a stack trace
  • move AOP wait-group ownership to the goroutine wrapper so panic recovery cannot strand shutdown
  • protect temporary TCP/UDP access handlers and their terminator goroutines with the same recovery seam
  • add panic, wait-group release, happy-path, and nil-config regression tests

Why

The server now contains per-message handler panics, but the access controller still launches packet-driven goroutines without a top-level recovery boundary. A single unexpected panic in AOP or temporary-access handling therefore terminates nhp-acd, dropping every in-flight access operation on that controller.

The recovery boundary turns that failure into one dropped request with an actionable stack trace. It does not hide ordinary returned errors and does not recover long-running lifecycle routines outside the packet-handler scope.

This ports the portable process-safety portion of LayerV nhp#1655 while intentionally omitting LayerV-specific CloudWatch alarms.

Impact

Malformed input or an internal handler bug can no longer crash the AC process through these packet-facing goroutines. Shutdown still waits for every in-flight AOP handler, including one that panics.

Validation

  • cd endpoints && go test ./ac
  • cd endpoints && go test -race ./ac
  • git diff --check

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
endpoints/ac/msghandler.go 0.00% 3 Missing ⚠️
endpoints/ac/udpac.go 78.57% 3 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1628      +/-   ##
==========================================
+ Coverage   12.54%   12.60%   +0.06%     
==========================================
  Files          96       96              
  Lines       14526    14540      +14     
==========================================
+ Hits         1822     1833      +11     
- Misses      12526    12528       +2     
- Partials      178      179       +1     
Flag Coverage Δ
unittests 12.60% <64.70%> (+0.06%) ⬆️

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

Files with missing lines Coverage Δ
endpoints/ac/msghandler.go 0.00% <0.00%> (ø)
endpoints/ac/udpac.go 2.31% <78.57%> (+2.31%) ⬆️

... and 1 file 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

Focused, defensive fix that adds a top-level panic-recovery seam to the AC's input-driven UDP handler goroutines. I reviewed it against security, correctness, breaking-change, performance, and code-quality criteria and found no blocking issues.

What I verified

Correctness of WaitGroup ownership (the trickiest part):

  • HandleUdpACOperations had its defer a.wg.Done() removed, and ownership moved to the runUDPHandler wrapper. HandleUdpACOperations has exactly one caller (recvMessageRoutine via runUDPHandler), so there is no risk of a leaked Add or a double Done.
  • a.wg.Add(1) is still called before go, so Wait() cannot race ahead of the spawn.

Defer ordering:

  • In runUDPHandler, recoverUDPHandler is deferred after wg.Done(), so (LIFO) the panic is recovered before the WaitGroup is released. Shutdown correctly waits for even a panicking AOP handler, matching the PR's stated intent.
  • In tcpTempAccessHandler/udpTempAccessHandler, the order is Close then recoverUDPHandler then wg.Done(); the panic is contained before Done().
  • tempConnTerminator gets recovery but intentionally no wg.Done() — consistent with it being launched via a bare go with no matching wg.Add().

Nil-safety:

  • recoverUDPHandler guards a != nil && a.config != nil && a.config.ACId != "", falling back to unknown. The TestRecoverUDPHandlerNilConfigIsSafe case exercises this path directly.

Tests cover the four meaningful behaviors: panic recovery + wg release, happy path, and nil-config safety.

Minor, non-blocking observations

  • recvMessageRoutine itself (the dispatch loop) is not wrapped in recovery, so a panic there (rather than in a spawned handler) would still crash the process. This is outside the PR's stated scope (packet-handler goroutines, not lifecycle routines), and the loop only reads a channel and dispatches — the risky parsing happens inside the now-protected HandleUdpACOperations — so risk is low. Worth a follow-up, not this PR.
  • p := ppd in recvMessageRoutine is slightly redundant now that the value is passed positionally, but it is harmless and arguably clearer.
  • debug.Stack() in the error log is appropriate for an actionable trace; just noting it lands in error-level logs.

Nice, surgical port of the process-safety portion of the upstream change. Note: I could not run go test ./ac in this environment due to sandbox restrictions, but the PR reports go test / go test -race passing, and the code resolves cleanly against the referenced symbols.

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