Repository navigation
fix(edge): end an exec session whose read loop or input writer panics - #378
Merged
Merged
Conversation
panicStart closed the un-wired conn and then called failStart. A panic in that close, or in the record failStart writes first, stopped the exit there: the session stayed open and registered and no exec_end went out, which is the failure #376 fixed for the first panic. Both steps are deferred now. The record runs first and on its own, so its Once is spent before failStart reaches it and a write that panics can't cost the close and the exec_end that follow it. failStart runs last, so it still closes the session and sends the exec_end when the conn's Close panicked. The second panic carries on to recoverSession as before.
A panic in readLoop closed the session and sent nothing, so the controller kept a session the agent had already dropped. The read loop is the one place a live session's exec_end comes from, and its panic exit now sends it too, with the fixed reason "exec session failed: internal error", ahead of the same deferred close its normal exit runs. A session ending writes no audit record, so this exit writes none. It sends nothing when the controller's exec_end or a tunnel drop ended the session. That takes a flag of its own: once a session is live the input writer closes it as well and leaves the exec_end to the read loop, so "closed" no longer means the other side ended it the way it does during the bring-up. EndExec and closeAllExecSessions set the flag through endByPeer. recoverSession is now the read loop's outermost defer. The close sat outside it, so a panic in the close took the agent down on either exit.
A panic in inputWriter was logged and that was all. The session stayed registered on its exec slot with nothing left to write its stdin or run its resizes. The writer now closes the session when it panics. It sends no exec_end of its own: the close closes the conn under the read loop, and the read loop's exit sends the session's one exec_end, the same way a failed write already ends a session. A second one from here would be a duplicate.
StartExec's caller only learns of the hijacked conn when StartExec returns it. A panic after the dial left the conn open until its finalizer ran, and the edge bring-up's recover had nothing to close. StartExec now closes the conn it dialled and panics again with the same value, so the caller's recover still handles it.
The CHANGELOG entry for the panicking bring-up said it sends the controller an exec_end without saying when it doesn't. The Drydock integration page said "that exec_end" straight after naming the controller's own, and didn't name the fixed reason a panicked bring-up reports.
The read loop and input writer panic tests ran those loops on the test's own goroutine, so a loop that never returned hung its test instead of failing it. That turned the read loop's `err != nil` mutant from killed into timed out: with the check negated the loop spins on EOF forever. They now run through runToEnd, which waits readTimeout for the loop to return and fails the test if it hasn't, or if a panic got out of it.
auditStartError kept the record to one with a sync.Once, and a Once counts a call that panicked as done. So when an exit's write panicked before it wrote anything, the retry from the panic exit was a no-op and the session closed with no error record. The record is now marked written only after APIRequest returns, under a mutex held across the write. A write that panicked is tried again by the next call, and a write that returned still can't be followed by a second record. That changes what the panic exit can lean on. It ran the record and then failStart, counting on the spent Once to make failStart's own write a no-op. With a retry there, a sink that panics every time would stop failStart before its close and exec_end. failStart is split into the record and endStart, and the panic exit defers each on its own: the record once, then endStart whatever the record did.
The bring-up panic tests read the exec_end and checked its reason, and would still have passed with a second frame queued behind it. Each case that expects exactly one now sends a pong after it and requires that to be the next frame. Adds the case that was missing: a bring-up that panics after its context was cancelled, with the session not yet closed, writes the one record and sends no exec_end.
scttbnsn
requested review from
ALARGECOMPANY and
biggest-littlest
as code owners
October 8, 2026 18:45
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDocker exec startup now closes its connection before propagating a panic. Edge exec sessions now close after input-writer panics and report read-loop panics with a fixed Suggested labels: Priority: ⬇️ Low Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable issue remains identified for this change; it is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
the
second-opinion
Contributor
|
@greptileai Review exact head |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This was referenced Oct 8, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #376. That one made a panicking exec bring-up end its session properly. Two reviews of it turned up the same gap in the other two session goroutines, plus a hole in the fix itself.
exec_endafterexec_ready, so the controller kept a session that looked live. It now sends one, reasonexec session failed: internal error, unless the controller or a tunnel drop already ended the session.exec_end, same as when a write fails.sync.Once, which counts a write that panicked as done, so the retry wrote nothing. It's now a flag set after the write returns.StartExec. A panic after the dial left the hijacked conn to the finalizer. It's closed, then the panic continues to the caller.Ownership once a session is live can't use the closed flag, because the input writer closes the session too. There's a
peerEndedflag for that, set by the controller'sexec_endand by tunnel teardown.A session ending writes no audit record today, so these exits don't add one.
Known and left: a read-loop panic landing at the same instant as a controller
exec_endcan still send one strayexec_end. Closing that needs the session lock held across the send. Drydock drops anexec_endfor a session it has already deleted.Tests: each fix has a test that failed first.
go test -race ./...passes, 20 runs each on the new tests, golangci-lint clean.Changelog
exec_endwith reasonexec session failed: internal error, unless the controller already ended the session or the tunnel dropped.exec_endreporting to the read loop.StartExecto close a dialed connection if startup panics, then re-panic with the original value.exec_endframes.go test -race ./...passes, new tests passed 20 runs, and golangci-lint is clean.Concerns
exec_endand send one strayexec_end; Drydock drops it if the session was already deleted.