fix: make crash-orphaned recorder sessions stoppable from the menu bar - #24
Merged
Conversation
Add identity-checked `onoats stop` + menu-bar Stop rewiring plan so a crash-orphaned recorder session is stoppable from the GUI (currently only Flush works). Twice-reviewed via /review-plan (16 + 10 findings incorporated); review marker CI-gated. Ignore .review-plan/ scratch dir.
Phase 1 of the stoppable-orphan-session fix. `onoats stop` is a near-clone of `onoats flush` that reuses `resolve_flush_target` verbatim and differs only in the signal sent: SIGTERM (graceful drain + final flush, then exit) instead of SIGUSR1. The identity gate (marker + cmdline fingerprint + liveness + recycle check) is unchanged, so a recycled or unverified pid is never signalled — which matters more here than for flush, since SIGTERM kills by default. Returns on signal delivery, not confirmed exit; `--help` resolves without booting a service. Tests (tests/test_cli.py): every test_flush_* branch mirrored 1:1 as test_stop_*, including a recycled-pid live-survival regression (spawns a real foreign process on the recycled pid and asserts it stays alive after stop) and a parametrized differential test pinning stop=SIGTERM-not-SIGUSR1 and flush=SIGUSR1-not-SIGTERM so a copy-paste signal swap fails. Runtime regression (tests/test_socket_supervisor.py): drives the real teardown producers and observes status-stopped is durable on disk before the pid file is unlinked — the runtime complement to the existing static source-order check. Content-bearing final flush on SIGTERM: waiver recorded in the plan's Findings citing the 2026-06-10 live verification (memory shutdown-drain-final-segment-edge), per the acceptance criteria. Docs: README, AGENTS.md, CHANGELOG. Phase 2 (Swift menu-bar Stop rewiring) is a manual follow-up.
The status-stopped → restore-terminal → pid-removal tail was inline in `run_onoats_dual._run_shutdown`, a nested closure unreachable without booting the STT/VAD stack — so the runtime write-order regression added in the previous commit could only drive the producers in a hand-sequenced copy of dual.py's order, not dual.py's order itself. Extract that tail verbatim to a module-level `_finalize_shutdown_status` (same status-stopped-before-pid-removal ordering, same ended_by_error branch) and call it from the closure. The runtime test in test_socket_supervisor.py now drives the real helper and spies the pid-unlink boundary, so a reorder of the status-stopped / pid-removal pair fails at runtime — parametrized over the graceful and fatal-ErrorFrame branches. Behaviour is unchanged; the static source-order check in test_status_file.py stays green (the grepped literals survive in order). Full suite: 279 passed.
…aint Capture the post-Phase-1 design discussion below the reviewed marker so Phase 2 doesn't re-derive it: - Recommended orphan-recovery flow: flush (continuation — salvage + keep recording) then stop at the end; ownership does not transfer on flush. - Anticipated Issue: external stop→start must not be chained immediately. stop returns on signal delivery not exit, so a start during the drain window clobbers the orphan's pid file and the draining orphan then unlinks the NEW recorder's pid file (warn-and-overwrite + unconditional unlink), leaving an invisible session + double capture. The menu is safe today because Start is gated behind the .running→.stopped transition; the constraint is for any future chained "Restart" / CLI use. A flock-style single-instance lock is the durable fix, out of scope for Phases 1-2. Plan body only (below the marker); contract untouched.
`onoats stop` returns on signal delivery, not exit, so a new `onoats bot` launched during the old recorder's drain window could overwrite the draining recorder's pid file — and the drainer would then unlink the NEW recorder's file unconditionally, leaving it invisible to status/stop/flush plus overlapping capture. (Codex adversarial-review [high] finding.) Two guards: - `_write_pid_file` refuses to start over an identity-verified live recorder, raising `RecorderAlreadyRunningError` (reuses resolve_flush_target, so a stale/recycled/foreign pid never blocks a legitimate start). Mapped to a clean rc=1 at all three CLI boundaries (cli.py, dual.py, __main__.py) — the single-instance lock, no separate flock file. - `_remove_pid_file(pid_path, owner_pid=...)` unlinks only when the file still records our pid; recorder teardown passes owner_pid=os.getpid() so a drainer never deletes a newer recorder's pid file. Regression tests (test_status_file.py): refuse-live, overwrite-stale-dead, ownership-skip, back-compat-unconditional. The static dual.py source-order check now matches the call prefix (ownership kwarg). Full suite: 283 passed.
A GUI-started recorder orphaned by an app crash is seen as `running(ours: false)` on relaunch; Stop was hard-disabled, so the incident's session was unstoppable from the menu. (Codex adversarial-review [high].) - RecorderModel.stopExternal() clones flush() to exec `onoats stop` for handle-less sessions (the CLI does the identity-checked SIGTERM, so it's safe without a Process handle). Does not block on exit; does NOT set the .stopping enum (no handleExit fires without a handle). - The Stop button routes on the `ours` value the .running(ours:) enum carries — owned → p.terminate(), verified external → stopExternal() — is enabled for all verified sessions, and relabelled "Stop". - Cosmetic stopRequested flag: set synchronously in stopExternal() before the spawn (gates .disabled so no double-stop subprocess), cleared level-triggered in refresh() on !alive (sole clearing site), plus a spawn-failure reset so a missing CLI can't wedge the button. Drives the "stopping (draining)…" status + icon; convergence to .stopped is polled on processAlive→false, never faked. Both the enablement change and the handler land together. Compiles clean (`make build/Onoats`); manual orphan-then-stop smoke matrix pending on-device.
CHANGELOG (Stop-for-external + the stop→start race fix), AGENTS.md (single-instance + pid-ownership invariants), and the dev plan (Phase 2 ticked with the on-device smoke caveat; Findings for both Codex [high] findings, including the named spawn-failure deviation from the strict stopRequested invariant). Plan body only — reviewed contract untouched.
Codex re-review [high]: the round-1 start guard only blocked a *verified* live recorder. If the existing recorder was live but the `ps` identity probe failed (resolve_flush_target → pid=None, stale=False), `_write_pid_file` fell through to warn-and-overwrite — the exact indeterminate state flush/stop refuse to act on, so a transient probe failure (or a legacy fingerprint-less pid file) could spawn a second recorder over a live one and corrupt the older session's ownership. `_write_pid_file` now also raises RecorderAlreadyRunningError when a marker-valid pid file names a live process (`_pid_alive`, fail-safe: only ProcessLookupError proves death) that the resolver could neither verify nor declare stale. Only stale=True (dead or recycled-to-foreign) is overwritten. Regressions (test_status_file.py): refuse-on-indeterminate-probe (kill(0) ok + _live_ps_cmdline→None) and refuse-on-live-legacy-fingerprintless. Full suite: 285 passed.
Codex re-review [medium]: stopExternal() cleared stopRequested only on a spawn *throw*, not on a non-zero CLI *exit*. A stale installed CLI lacking the `stop` subcommand (argparse rc 2), or an identity refusal while the process stays alive, left the sole Stop button disabled until app restart — the motivating orphaned session unstoppable again. The terminationHandler now clears stopRequested on any non-zero exit (SIGTERM not delivered → re-enable). A delivered SIGTERM (rc 0) still leaves it set, cleared by refresh() on !alive. Compiles clean (make build/Onoats). No Swift unit harness exists (no Package.swift/XCTest — bare swiftc build), so per the project's "native = manual smoke" standing decision this is covered by manual smoke, not an automated test (documented in the plan Findings).
CHANGELOG (indeterminate-but-live start refusal + external-stop re-enable) and dev-plan Findings (both round-2 [high]/[medium] fixes, plus the honest note that the Swift stop-failure regression is manual-smoke-only — no XCTest harness in-repo). Plan body only; reviewed contract untouched.
Minor non-blocking findings from /deep-review: - Cross-reference the near-clone pairs so the maintenance coupling is visible from both sides: `_cmd_flush` ↔ `_cmd_stop` (cli.py) and `flush()` ↔ `stopExternal()` (RecorderModel.swift). - Reword `_finalize_shutdown_status` docstring: it runs exactly once via the `_on_shutdown` de-dup guard, so drop the inaccurate "idempotent" claim. - README: name the SIGTERM → drain → EXIT semantic so `stop` reads as distinct from `flush` (which keeps recording). - Plan Progress: split Phase 2 into [x] code / [ ] manual smoke so "done" isn't read at a glance while the on-device smoke is still pending. Comment/doc-only; no behaviour change. Swift compiles; ruff clean.
Headline case verified live (2026-06-22) on the signed app with real TCC + audio: a crash-orphaned session (running(ours:false)) is now stoppable from the menu via identity-checked SIGTERM, draining in the correct runtime order (STT close -> flush/rotate -> pid removed -> complete; recorder-first then capturer). Ticks the Phase 2 manual-smoke box. Also records the smoke-procedure gotcha: Force Quit reaps the in-bundle onoats-capturer (rc=-9) and fail-loud-exits the supervisor, so it does NOT produce an orphan; a genuine GUI crash (or a surgical kill of only the GUI process) is required to reproduce the stoppable-orphan scenario. Plan body only (below the reviewed marker); contract untouched (check_review_markers.py: pass).
… fail-closed removal) Codex adversarial-review round 3 [high]: the ownership-checked pid cleanup still had a window that could orphan a newly started recorder. Two compounding gaps: - _write_pid_file wrote the pid file IN PLACE (write_text truncates-then-writes), so a concurrent reader could observe an empty/partial file mid-write. - _remove_pid_file(owner_pid=...) treated a None read (unparseable/empty) as 'fall through to unlink()'. A draining recorder reading a NEWER recorder's mid-write file as None would delete it — re-opening the exact orphan-the-new- recorder failure the round-1 ownership check was meant to close. The round-1/2 tests only covered a fully-written newer pid file. Fixes: - Atomic write: temp file + os.replace (same-dir atomic rename, fsync first), mirroring onoats.status.write_status. A reader sees the complete old or complete new record — never a partial. - Fail-closed removal: unlink ONLY when the file still records exactly owner_pid; a None/foreign read is left in place (logged), never deleted. A leftover invalid pid file is self-healing (status -> no valid recorder; next start atomically replaces it). Regressions (test_status_file.py): _remove_pid_file fail-closed on empty/garbage/ foreign-marker content (x3 parametrized); _write_pid_file publishes via os.replace with no temp residue. Full suite: 289 passed; ruff clean; marker + static source-order checks green. Docs: AGENTS.md, CHANGELOG, dev-plan round-3 finding.
…rt race) Codex adversarial-review round 4 [high]: the pid guard was check-then-replace — _write_pid_file resolved the existing pid file (best-effort identity check) then later published via os.replace, with no atomic step tying the liveness check to ownership of the instance slot. Two 'onoats bot' starts racing with no valid pid file both pass resolve_flush_target, both proceed, and the later os.replace wins; the loser keeps running but is unrepresented by the pid file (invisible to status/stop/flush, double capture). Pre-existing TOCTOU, but the branch's single-instance invariant claims to prevent exactly this. Fix — the durable lock the plan had deferred: - _acquire_instance_lock takes an exclusive flock(LOCK_EX|LOCK_NB) on .active/onoats.lock BEFORE publishing the pid file; of N racing starts exactly one wins, the rest raise RecorderAlreadyRunningError. Held for the process lifetime via a module-global fd; the kernel releases it on exit (graceful OR crash/SIGKILL) — no stale lock to reclaim (the advantage over an O_EXCL lockfile). Released explicitly on teardown (dual._finalize_shutdown_status, __main__) for prompt stop->start handoff. No-op on Windows (POSIX-only). - The resolver-identity check stays as the SECONDARY guard (catches a legacy/ cross-version recorder predating the lock); flock is the primary atomic gate. This also enforces the plan's 'no immediate stop->start' rule — a chained start refuses until the drainer's process exits. Regressions (test_status_file.py): a held flock makes a second _write_pid_file refuse and publish no pid file; the lock is exclusive while held and frees on release. An autouse fixture releases the module-global lock between tests. Full suite: 291 passed; ruff clean; marker + static source-order checks green. Docs: AGENTS.md, CHANGELOG, dev-plan round-4 finding + resolves the previously 'out of scope' flock note.
…ce open Codex adversarial-review round 5 [high]: the round-4 flock closed the pid-overwrite race but was acquired too late to be the single-instance START gate. It lived inside _write_pid_file, but socket mode reaches that only AFTER _supervise_socket_session has already spawned the native capturer (CoreAudio process tap, TCC prompt, device acquisition) and waited for its sockets. Two concurrent socket starts could both spawn capturers and touch hardware; the loser failed only after those side effects (duplicate prompts, device contention, transient double capture). Fix — hoist acquisition before any capture side effect: - _acquire_instance_lock is now called in _supervise_socket_session BEFORE create_subprocess_exec spawns the capturer, and at the top of run_onoats_dual BEFORE PortAudio device open. Acquisition is idempotent (already-held -> no-op), so the nested acquires (supervisor -> recorder -> _write_pid_file backstop, all same data_dir) never release-then-reacquire. _write_pid_file keeps the acquire as the universal backstop for 'python -m onoats'. - Removed the explicit teardown release (dual._finalize_shutdown_status, __main__) added in round 4 — itself a latent bug: it freed the slot while the supervisor was still tearing down its capturer, letting a chained start spawn a second capturer into a not-yet-released device. The lock is now held for the whole process lifetime; the kernel frees it on exit (graceful or crash). An autouse fixture (tests/conftest.py) resets the process-global lock between tests. Regression (test_socket_supervisor.py): with the lock held, the supervisor refuses rc=1 and create_subprocess_exec is NEVER called — the capturer is not spawned. Full suite: 292 passed; ruff clean; marker + static checks green. Docs: AGENTS.md, CHANGELOG, dev-plan round-5 finding.
Codex adversarial-review round 6 — two more process-control holes, both fixed. [high] bot-single / python -m onoats set up capture before claiming the lock. run_onoats() imported native deps (pyaudio via LocalAudioTransport / audio_devices), resolved the data dir, then ran select_input_device() + crash recovery + pipeline build — all before _write_pid_file (its only acquisition). Hoisted: run_onoats() now resolves data_dir and calls _acquire_instance_lock FIRST, before even the heavy imports, so a losing start refuses having touched nothing. _cmd_bot_single routes through this run_onoats, so both entrypoints are gated consistently with socket mode. [high] stop/flush stale cleanup could delete a newer live recorder's pid file. When resolve_flush_target returned stale=True, the command blindly unlinked the pid file. Race: stop resolves an old dead/recycled pid as stale; before the unlink a fresh recorder wins the lock and writes its pid; stop deletes that fresh file, orphaning the live recorder. Pre-existing (cloned from the shipped flush), now fixed in the shared path: new _compare_and_unlink_stale_pid captures the pid the file recorded BEFORE resolution and reuses the round-3 fail-closed _remove_pid_file(owner_pid=...) — unlink only on an exact match. Applied to all four stale/dead-pid unlink sites (stop + flush, stale + ProcessLookupError). Regressions (test_cli.py): a held lock makes bot-single fail rc=1 before select_input_device; a fresh pid written during stop's stale resolution survives. Full suite: 294 passed; ruff + marker green. Docs: AGENTS.md, CHANGELOG, dev-plan round-6 finding.
Codex round 7 (NO-SHIP) + a parallel /code-review --fix converged on two issues. [high] A live LEGACY/cross-version recorder was detected only after capturer spawn. The early flock blocks a concurrent SAME-version start, but a recorder from an older build holds no flock — so the socket supervisor acquired the flock and spawned the capturer, refusing only later inside _write_pid_file's identity check (after CoreAudio/TCC). Fixed by moving the identity preflight INTO the lock: extracted _refuse_if_live_recorder and call it inside _acquire_instance_lock right after the flock (releasing the flock if it must refuse). Both guards (atomic flock + identity preflight) now fire at the same hoisted, before-capture point in every entrypoint; _write_pid_file's duplicate check was removed. Also resolves the review's 'identity guard scattered across call sites' altitude finding — the lock owns both guards. Regression (test_socket_supervisor.py): a marker-valid LIVE pid file with no held flock → supervisor refuses rc=1, create_subprocess_exec never called. [medium] The menu-bar Stop button could wedge disabled. stopRequested cleared only on !alive; if a NEW external session started before refresh() observed the stopped one exit, the new session inherited a stuck-disabled Stop. Fixed by tracking stopRequestedForPid and clearing stopRequested when that pid is gone OR a different pid is live. Compiles clean (make build/Onoats); Swift manual-smoke. Skipped (documented, not bugs): atomic-write dup with status.py (shipped, out-of-scope), _cmd_stop/_cmd_flush near-clone (deliberate per AGENTS.md), thin-wrapper nits. Full suite: 295 passed; ruff + marker green.
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.
Problem
When the Onoats menu-bar GUI crashes, its
onoats botsupervisor is reparented to launchd and survives (running(ours:false)on relaunch). The menu's Stop was hard-disabled for that state, so the orphaned session was unstoppable from the GUI — only Flush worked. The recording would run indefinitely until manuallykilled.Fix
Phase 1 —
onoats stopCLI (identity-checked SIGTERM). A near-clone ofonoats flushreusingresolve_flush_targetverbatim, differing only in the signal: SIGTERM (graceful drain + final flush, then exit) vs SIGUSR1. Identity gate (marker + cmdline fingerprint + liveness + recycle check) unchanged, so a recycled/foreign pid is never signalled — matters more here since SIGTERM kills by default.Phase 2 — menu-bar Stop rewiring (Swift).
RecorderModel.stopExternal()execsonoats stopfor handle-less (orphaned/external) sessions; the Stop button routes on theoursflag (owned →Process.terminate(), verified-external →stopExternal()) and is enabled for all verified sessions. A cosmeticstopRequestedflag drives "stopping (draining)…" and gates.disabled; clears level-triggered inrefresh()on!alive, plus a spawn-failure / non-zero-exit reset so a broken CLI can't wedge the button.Hardening — single-instance + pid-file lifecycle. Five guards, hardened across five adversarial rounds:
flocksingle-instance lock, acquired before any capture side effect. The socket supervisor takesflock(LOCK_EX|LOCK_NB)on.active/onoats.lockbefore spawning the capturer,run_onoats_dualbefore opening PortAudio, andrun_onoats(bot-single/python -m onoats) before importing native deps. Of N racing starts exactly one wins; the rest raiseRecorderAlreadyRunningErrorbefore touching hardware. Idempotent; held for the whole process lifetime (kernel frees it on exit → no stale lock, and a chainedonoats stop && onoats botrefuses until the drainer exits).os.replace, never a truncating in-place write._remove_pid_file(owner_pid=…)unlinks only on an exact pid match;None/foreign left in place.stop/flushnever blindlyunlinka stale pid file; they remove it only if it still records the resolved stale pid, so a fresh recorder that won the lock mid-window isn't deleted.Verification
uv run pytest).ruff format+ruff checkclean. Review-marker + static source-order checks green.flock), r5 (lock acquired too late → hoisted before capturer spawn / device open), r6 (bot-single entrypoint parity + stop/flush stale-cleanup TOCTOU). Plus/security-review(clean) and/deep-review(0 Critical / 0 Important).RUNNING, live capturer); relaunch showedrunning(ours:false)with Stop enabled; Stop drovestopExternal()→onoats stop→ SIGTERM → graceful drain in the correct runtime order (STT close → flush/rotate → pid removed → complete; recorder-first then capturer; norc=-9/fail-loud). Final status: cleannot running.Smoke-procedure note (in the dev plan)
Force Quit does not reproduce the orphan — it reaps the in-bundle
onoats-capturer(rc=-9), which fail-loud-exits the supervisor. A genuine GUI crash (segfault) / surgicalkill -9of only the GUI process is required.Scope / follow-ups
🤖 Generated with Claude Code