Repository navigation
fix(edge): audit and close a typed exec bring-up that panics - #376
Merged
Merged
Conversation
The controller's exec_end landing while a create or start round trip is in flight closes the session and fails the round trip. failStart then sent an exec_end back, while the same cancel landing between the round trips went through abortStart and sent none. A tunnel drop mid round trip did the same toward a connection that was already gone. failStart now takes the abortStart path when the bring-up's context has ended or the session is already closed: whoever ended it owns the exec_end. It checks the context as well as the closed flag because Close cancels the context before it sets the flag. The error record is written either way.
A panic inside bringUpExec was recovered and logged, and that was all: no audit record, the session stayed registered and kept its exec slot, and the controller never got an exec_end. It was the one bring-up exit #367 left without a record. The bring-up now recovers on its own and takes the failed-start exit: one api_request error record under exec_start, the session closed, and an exec_end with a fixed reason unless the session was already ended. A conn the daemon handed over that activate hadn't taken yet is closed too. The record sits behind a sync.Once so a panic after an exit already wrote it doesn't write a second, and recoverSession stays as the outer defer so a panic in the teardown itself still can't reach the agent.
scttbnsn
requested review from
ALARGECOMPANY and
biggest-littlest
as code owners
October 8, 2026 16:10
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 23 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
scttbnsn
added a commit
that referenced
this pull request
Oct 8, 2026
…#378) * fix(edge): finish a panicking exec bring-up's exit when it panics too 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. * fix(edge): send exec_end when a live exec session's read loop panics 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. * fix(edge): close an exec session whose input writer panics 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. * fix(docker): close the dialled conn when StartExec panics 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. * docs: qualify the exec_end a failed exec start sends 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. * test(edge): bound the session goroutine runs in the panic tests 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. * fix(edge): retry a failed-start record whose write panicked 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. * test(edge): pin a panicking exec bring-up to one exec_end 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.
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
1 participant
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.
A typed exec start on the edge whose bring-up panicked left no trace.
recoverSessionlogged the panic and did nothing else, so no audit record was written, the session kept its exec slot, and the controller waited for anexec_endthat never came. #367 made every other failing bring-up exit write a record. This was the one left.bringUpExecnow takes the failed-start exit: oneapi_requestrecord (exec_start, outcomeerror), the session closed, and anexec_endwith a fixed reason. The panic value stays in the agent log.StartExecreturned butactivatehadn't taken yet is closed too. Only the bring-up held it, soClosecouldn't reach it.sync.Once, so a panic after an exit already wrote its record can't write a second.One wire change, in its own commit: a start that fails after the controller's own
exec_end(or a tunnel drop) already ended the session no longer sends anexec_endback. That's whatabortStartalready did in the other cancel windows. Drydock ignores anexec_endfor a session it has deleted, so nothing on the controller depended on it.Not changed, from the same review notes:
deniedexec_startstill doesn't name the limit. That needs a field on the audit record and the export schema.Tests:
TestBringUpExecPanicFailsTheStart(create, start, resize) failed with no record before the fix.go test -race ./...passes, golangci-lint is clean.