fix(tui): tear the sandbox down when Ctrl+Q quits twice - #1334
Open
poison-control wants to merge 1 commit into
Open
poison-control wants to merge 1 commit into
poison-control wants to merge 1 commit into
Conversation
The sidecar sends app.quit and exits in the same batch, so the command handler and run()'s finally both call quit(). The second cancel lands on the docker delete the first one's teardown is awaiting, and the sandbox container is left running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QE1brKHrPWcD8SHYooGBA1
poison-control
force-pushed
the
fix/sandbox-leak-on-quit
branch
from
September 18, 2026 16:18
de5338f to
cf18972
Compare
poison-control
marked this pull request as ready for review
September 18, 2026 16:21
Contributor
|
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.
Problem
Fixes #1079: Ctrl+Q leaves the scan's container running, so repeated sessions pile up orphans.
Ctrl+Q quits twice. The sidecar sends the quit command and exits in the same batch (
strix/interface/tui/internal/app/update.go:20):Both halves land on
GoTuiRuntime.quit():controller._quitcallsquit(), which cancelsscan_task.run_strix_scan'sfinallybegins teardown and suspends on the docker delete insidesession_manager.cleanup().wait_processreturns andrun()'sfinallycallsquit()again.scan_taskis still tearing down, soif not scan_task.done(): scan_task.cancel()fires a second time.That second cancel lands on the delete the teardown is awaiting.
cleanup()never finishes and the container stays up.4c1f00d fixed the same cancellation-path leak during staging. This one sits one step later, at teardown.
Fix
GoTuiRuntime.quit()becomes idempotent. The first caller owns the cancel and the wait as its own task, and later callers await that task instead of cancelling again. The await is shielded, since awaiting a task hands it the waiter's own cancellation, which is the thing being guarded against.Testing
test_quitting_twice_lets_the_sandbox_teardown_finish(tests/test_go_tui_runtime.py) drives the realGoTuiRuntime.quit()through the Ctrl+Q sequence: quit while the scan runs, then quit again while the teardown waits on the daemon. The teardown is held open on an event rather than a sleep, so the test does not depend on timing.Before:
After:
Checks:
make type-checkruns mypy, which is clean here, and pyright, which reports 864 errors acrossstrix/. That count is identical on this branch and on a cleanmain.The suite failure is
tests/test_pricing.py::test_resolves_common_bare_model_names. It fails the same way on a cleanmain(#1213, PR #1215) and this branch does not touch it.Note
run_strix_scan's teardown is open to the same shape from any other caller that cancels twice.cli.pyinstalls a SIGINT handler that callssys.exiton every signal, so a second Ctrl+C during teardown looks like it would interrupt the delete the same way. I did not reproduce that one and left it out to keep this change to the reported bug. Happy to send a follow-up that makes the teardown itself cancel-proof if you want it covered.