Repository navigation
fix(cli): skip C++ static teardown on interrupt so Metal exits clean - #1001
Merged
Merged
Conversation
silverstein
force-pushed
the
fix/998-metal-teardown
branch
from
September 16, 2026 04:34
19f972a to
f129c81
Compare
`minutes watch` aborted with SIGABRT on Ctrl-C while a transcription was still in flight: ggml-metal-device.m:608: GGML_ASSERT([rsets->data count] == 0) failed The watch handler released the lock and then called `std::process::exit(0)` from ctrlc's signal thread while the worker thread still held a live WhisperContext. `exit()` runs `__cxa_finalize`, ggml's Metal device destructor runs with buffers still registered in the residency-set collection, and `ggml_metal_rsets_free` asserts. Upstream says as much directly above the assert: "most likely you haven't deallocated all Metal resources before exiting". A signal handler cannot fix this by dropping the context, because the decode that owns it is running on another thread. The desktop app already solved the same class of bug for its worker subprocess in #229 with a hard exit, but that helper is macOS-gated and private to the Tauri binary, so the CLI never got it. Add `exit_without_cxx_teardown` to minutes-core and use it from the four CLI interrupt paths that exit: watch, record, dictate, and the graceful-stop handler. Non-macOS targets keep the ordinary exit, since the aborting teardown is Metal's and no equivalent exit-time assertion has been shown for the CUDA, HIP or Vulkan backends. The helper deliberately does not flush stdout first. An earlier revision of this change did, on the reasoning that `_exit` skips the flushing that `exit()` performs. That was a regression on every platform: `Stdout::flush` takes the process-wide stdout lock, so a thread already blocked writing into a stalled pipe holds that lock and the force-quit hangs instead of quitting. Rust's own exit cleanup sidesteps this with `try_lock`. A force-quit that cannot quit is worse than the buffered bytes it saves, and these handlers report through stderr, which is unbuffered. `exit_without_cxx_teardown_does_not_block_on_a_held_stdout_lock` guards that: it re-executes the test binary, holds the stdout lock on another thread, and asserts the child still exits. It fails after a 30s hang if a blocking flush returns. Unlike the Metal abort, it needs no GPU backend, so it runs everywhere CI builds. Verified on macOS 26.6 (M-series) with `--features metal`: the reproduction aborted with exit 134 every time before the change, and exits 0 with no assert on every run after it. Refs #998 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
silverstein
force-pushed
the
fix/998-metal-teardown
branch
from
September 16, 2026 05:22
f129c81 to
e1346cd
Compare
This was referenced Sep 16, 2026
silverstein
added a commit
that referenced
this pull request
Sep 17, 2026
Codex found three things. One was already fixed before the review ran on a stale ref: the orphaned #[test], repaired in 19b7e0c. The other two were real. **Force quit could discard completed dictation output.** `minutes dictate --stdout` writes each finished utterance with println! and nothing flushed it. Piped stdout is block buffered and the macOS force quit takes _exit, so an utterance the user watched complete could never reach the pipe. Flushing at the exit is not the fix: #1001 removed exactly that flush because taking the process-wide stdout lock on the emergency path can hang forever behind a thread stuck writing to a stalled pipe. So the flush moves to the write instead, which is correct regardless of how the process ends and makes the stream arrive per utterance rather than in one lump. **The guard was fragile in three ways.** It sliced byte windows out of UTF-8 source, which panics if an edge lands mid-character, and this file has plenty of non-ASCII in its strings. It accepted the helper name anywhere in the window, including a comment, even if the real call was a plain exit. And it asserted only that some sites matched, so a rename that stopped matching would drop a call site out of the guard silently, which is the same way #1008 removed them. Now line-based, asserting the exact count of each shape, and requiring the safe exit to appear before any plain one. The watch site had only 94 bytes of slack under the old byte window; lines remove that cliff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
silverstein
added a commit
that referenced
this pull request
Sep 17, 2026
) * fix(cli): restore the interrupt teardown skip that #1008 reverted #1001 fixed the Ctrl-C abort reported in #998 by routing every interrupt path through exit_without_cxx_teardown, so a force quit does not run __cxa_finalize while an interrupted transcription still holds a live whisper context. #1008 reverted all four call sites. It was not deliberate. #1008 never mentions the exit path; it was built on a copy of crates/cli/src/main.rs that predated #1001 and overwrote the file wholesale. The helper survived in minutes-core with its own test, so the tree still compiled, still passed, and quietly went back to aborting. Nothing caught it. exit_without_cxx_teardown is pub, so losing its only callers raised no dead-code warning, and no test named the call sites. The reporter of #998 has been retesting against a main that had silently dropped its own fix, which is worse than never having shipped one. Restores all four: the three graceful force-quit paths and the watch Ctrl-C handler. Adds a source-level guard asserting each force-quit site and the watch handler reach the teardown-skipping exit, in the style of the existing overlay guards. A released `minutes watch` that dies on SIGABRT when the user presses Ctrl-C is worth a test that fails loudly when the call sites go missing again. Refs #998, #944, #1001. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(cli): the teardown guard was matching its own source Spelled literally, the needle appears twice: once at the call site it guards and once in the test that looks for it. The window after the test's own copy contains no exit call, so the guard failed on itself and would have gone green only by deleting the thing it protects. Built from halves now, so the full string never appears in this file. The same applies to the watch banner, which passed only because find returns the first match and the handler happens to sit above the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): repair the #[test] attribute I orphaned The guard was inserted between the next test's #[test] and its fn, which put two attributes on the guard and left the existing test as a plain function. That is why CI reported a duplicated attribute on the changed lines and a never-used function nearby: the interrupt test had silently stopped being a test. Third time this session that inserting between an attribute or doc comment and the item it belongs to has broken something. Worth doing the read-back every time rather than trusting the anchor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(cli): address the review of this branch Codex found three things. One was already fixed before the review ran on a stale ref: the orphaned #[test], repaired in 19b7e0c. The other two were real. **Force quit could discard completed dictation output.** `minutes dictate --stdout` writes each finished utterance with println! and nothing flushed it. Piped stdout is block buffered and the macOS force quit takes _exit, so an utterance the user watched complete could never reach the pipe. Flushing at the exit is not the fix: #1001 removed exactly that flush because taking the process-wide stdout lock on the emergency path can hang forever behind a thread stuck writing to a stalled pipe. So the flush moves to the write instead, which is correct regardless of how the process ends and makes the stream arrive per utterance rather than in one lump. **The guard was fragile in three ways.** It sliced byte windows out of UTF-8 source, which panics if an edge lands mid-character, and this file has plenty of non-ASCII in its strings. It accepted the helper name anywhere in the window, including a comment, even if the real call was a plain exit. And it asserted only that some sites matched, so a rename that stopped matching would drop a call site out of the guard silently, which is the same way #1008 removed them. Now line-based, asserting the exact count of each shape, and requiring the safe exit to appear before any plain one. The watch site had only 94 bytes of slack under the old byte window; lines remove that cliff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: silverstein <silverstein@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
silverstein
added a commit
that referenced
this pull request
Sep 23, 2026
`cmd_talk`'s handler was the one Ctrl-C path still calling `std::process::exit(130)` directly. It landed in #1006 alongside voice-live, in parallel with #1001, so it never picked up the helper the other four interrupt paths now use. Talk transcribes remotely, so no whisper context is live in that process today and this is not a crash anyone can hit. It is the inconsistency that matters: a force-quit is the wrong place to depend on a remote transcription assumption holding, and RFC 0007 calls this phase 1. Also routes the #1001 regression test through `crate::engine_process::command`. `crates/core/clippy.toml` disallows `std::process::Command::new` so subprocess construction stays auditable, and that test violated it. The same correction is on a voice-continuity branch as bdf3f23; making it here keeps `main` correct whichever lands first, and an identical change merges cleanly. That violation reached `main` because clippy never runs with `--all-targets`, so no lint is enforced in test code at all. Filed separately as #1009. Refs #998, #1009 Co-authored-by: silverstein <silverstein@users.noreply.github.com>
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.
What
minutes watchaborts with SIGABRT on Ctrl-C when a transcription is still in flight, reported in #998:Why it happens
The watch handler at
crates/cli/src/main.rs:7217releases the lock and callsstd::process::exit(0)from ctrlc's signal thread while the worker thread still holds a liveWhisperContext.exit()runs__cxa_finalize, ggml's Metal device destructor runs with that context's buffers still registered in the residency-set collection, andggml_metal_rsets_freeasserts.That reading is not inference. The reporter's stack has
__cxa_finalize_rangesat frame 5 and_pthread_startat frame 12, which is the signal thread calling exit, and upstream states the cause directly above the assert:A signal handler cannot fix this by dropping the context, because the decode that owns it is running on another thread.
The fix
We already solved this class of bug once. #229 hit the same
__cxa_finalize_rangesabort in the desktop worker subprocess and fixed it with a hard exit, but that helper was macOS-gated and private to the Tauri binary, so the CLI's interrupt paths never got it.This adds
exit_without_cxx_teardowntominutes-core, with explicit stdout and stderr flushing (_exitwill not flush), and uses it from the four CLI interrupt paths that exit: watch, record, dictate, and the graceful-stop handler. Non-macOS targets keep the ordinary exit, since the aborting teardown is Metal's.The Tauri copy is deliberately left in place rather than pointed at the core one. That dedup is tempting, but
tauri/src-tauri/src/main.rsis covered in full by the Apple Speech authority golden hash, and that hash exists precisely so edits to that file are deliberate. I had the delegation written and backed it out: re-sealing a hash that guards an XPC authority boundary in order to tidy a ten-line helper is the wrong trade, and it belongs in a change that is actually about that file.Adversarial review found a defect in the first revision
Worth recording, because the bug was in the fix rather than in the original code.
The first revision flushed stdout before
_exit, reasoning that_exitskips the flushingexit()does. That was a regression on every platform, including the non-macOS ones this change otherwise leaves alone.Stdout::flushtakes the process-wide stdout lock, so a thread already blocked writing into a stalled pipe (sayminutes dictatepiped to a consumer that stopped reading) holds that lock, and the force-quit blocks on it forever. Rust's own exit cleanup avoids exactly this withtry_lock.Verified standalone before changing anything: with the flush, the process hung; without it, it exited 23 immediately. A crash turned into an unkillable hang is a worse trade than the one it was meant to fix.
The flush is gone, and
exit_without_cxx_teardown_does_not_block_on_a_held_stdout_locknow guards it: re-execute the test binary, hold the stdout lock on another thread, assert the child still exits. Confirmed it fails (30s hang) when the flush is put back and passes when it is removed, so it catches the defect rather than merely passing. It needs no GPU backend, so unlike the Metal abort it runs everywhere CI builds.Verification
Reproduced and verified on macOS 26.6, Apple Silicon, built
--features metal, in an isolatedHOMEso no real meeting data was touched. Interrupt during an in-flight decode:cargo fmt --all -- --checkandcargo clippy --all --no-default-features -- -D warningsare clean.What this does not cover
Two things, stated plainly rather than left for someone to discover.
No regression guard is possible in the current CI matrix. Nothing in CI builds with
metal, so this abort cannot reproduce there and no test in this PR would catch a regression. The honest fix is a small macOS Metal smoke job that startswatch, interrupts it mid-decode, and asserts the exit status is not a signal. Filing that separately.The desktop half of #998 is not closed. The reporter also saw the abort on desktop app shutdown. The app's explicit clean-exit route already skips teardown via
finish_clean_exit, so the remaining exposure is any path where the run loop returns frommaininstead, which still runs destructors. That needs its own audit, so this isRefs #998rather thanFixes.🤖 Generated with Claude Code