From ee7850cc9aa46fb6d9ed9d3ee1e7b9288b7ff3a4 Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Fri, 19 Jun 2026 16:53:38 -0700 Subject: [PATCH 01/18] docs(dev-plan): add reviewed plan for stoppable orphan sessions 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. --- .gitignore | 1 + .../20260619-bug-stoppable-orphan-session.md | 169 ++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 docs/dev_plans/20260619-bug-stoppable-orphan-session.md diff --git a/.gitignore b/.gitignore index 518816b..e097822 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ dist/ *.db secrets.env .deep-review/ +.review-plan/ diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md new file mode 100644 index 0000000..51896a0 --- /dev/null +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -0,0 +1,169 @@ +# Task: Stoppable orphaned recorder sessions (`onoats stop` + menu-bar rewiring) + +**Status**: Not Started +**Component**: recorder, macos +**Assigned to**: Varun Singh +**Priority**: High +**Branch**: bug/stoppable-orphan-session +**Created**: 2026-06-19 +**Completed**: (fill when done) + +## Objective + +Make any identity-verified live recorder session stoppable from the menu bar — including a GUI-started session orphaned by an app crash — by adding an identity-checked `onoats stop` CLI subcommand and routing the menu-bar Stop button through it, instead of hard-disabling Stop whenever the app lacks an in-memory `Process` handle. + +## Context + +**The incident (2026-06-19).** The Onoats menu-bar app crashed. The supervisor it had spawned (`onoats bot`, pid 70364) survived and reparented to launchd (PPID 1), still recording. On relaunch the app could only **Flush**, not **Stop**, leaving an unkillable-from-GUI session. Separately, the session's system-audio tap was delivering all-zero samples because Screen & System Audio Recording permission was denied — surfaced only as a passive 30 s watchdog warning. + +**Root cause of the unstoppable state.** Ownership ("did I start this?") is tracked *solely* by an in-memory `Process` handle (`RecorderModel.swift:76`). A crash destroys that handle. On relaunch, `refresh()` sees a live, identity-valid supervisor it has no handle for and classifies it `.running(ours: false)` (`RecorderModel.swift:254`); the Stop button is then hard-disabled via `.disabled(!ours)` (`OnoatsMenuBarApp.swift:71-72`). Flush still works because it shells out to the Python CLI, which performs its own identity-checked signalling (`resolve_flush_target` → marker + `ps` fingerprint) rather than relying on the handle. + +**Why the original design disabled Stop.** The comment at `RecorderModel.stop()` (`RecorderModel.swift:294-296`) states the deliberate rationale: "the safe identity-checked signalling lives in the Python CLI, not here." Stop was disabled for external sessions because no CLI seam existed to do it safely — `onoats flush` (SIGUSR1) was the only signal subcommand. This plan closes that gap: add the missing safe seam (`onoats stop`, SIGTERM) and let the GUI delegate to it exactly as `flush()` already does. The `quitApp()` comment (`RecorderModel.swift:304-313`) already documents the orphan hazard for graceful Quit; a *crash* bypasses `quitApp()` entirely, which is the unhandled path this plan addresses. + +## Requirements + +- `onoats stop` MUST reuse `resolve_flush_target` verbatim for identity verification before signalling — no weaker or duplicated check. PID-recycling defense matters *more* for SIGTERM than SIGUSR1 because SIGTERM kills by default; signalling a recycled foreign pid would terminate an unrelated process. +- `onoats stop` MUST send `SIGTERM` (graceful shutdown path, `runtime.py:1144`), giving the same drain semantics as the GUI's existing owned `p.terminate()`. (Assumption: SwiftUI `Process.terminate()` maps to SIGTERM — Foundation-documented behaviour, not repo-verifiable; named here rather than left implicit.) +- `onoats stop` MUST handle the identity-check→signal TOCTOU race exactly as `_cmd_flush` does (catch `ProcessLookupError`, treat as stale, unlink only when `stale=True`). +- The CLI command returns success on **signal delivery**, NOT on confirmed exit (parity with flush, which does not wait). The GUI MUST NOT interpret exit-0 as "stopped"; the stopped transition is driven by `refresh()` observing the supervisor **no longer alive** (`processAlive` → false; see correction below), NOT by exit code. +- The menu-bar Stop button MUST become enabled for verified `.running(ours: false)` sessions and route through `onoats stop` (mirroring `flush()`), while owned `.running(ours: true)` sessions keep the existing in-handle `p.terminate()` path. +- The external-stop GUI transition MUST NOT reuse the `.stopping` enum state, whose only exit is `handleExit` — which never fires for a handle-less external session. (See Architecture Decisions / Issue risk.) +- The double-stop guard MUST be a flag set **synchronously** in the button action (before the subprocess spawn) and gate `.disabled(stopRequested)` directly — NOT via the next 1 s poll tick. A redundant SIGTERM to a draining supervisor is harmless/idempotent (`runtime.py` ignores the second signal); the guard exists to prevent spawning a duplicate `onoats stop` subprocess, not for signal safety. +- Swift/Python pid-file parity (`test_native_contract_parity.py`) MUST remain green; no change to pid-file format. +- Final-flush-on-shutdown correctness MUST hold for the external-stop path (it shares the SIGTERM → `shutdown_event` path). NOTE: this is **inferred** from the shared path, not directly proven by `test_shutdown_drain.py` — that test asserts an EndFrame is *queued* before the terminal flush, not that it drains content. The content-bearing final flush was live-verified on 2026-06-10 (memory `shutdown-drain-final-segment-edge`); a content-bearing assertion SHOULD be added (see Testing Notes). +- No regression to `onoats flush`, `quitApp()`, or the zero-run watchdog. + +## Review Focus + +- **Signal-safety parity:** confirm `_cmd_stop` cannot signal an unverified/recycled pid — same guarantees as `_cmd_flush`. The only intended divergence is the signal number (SIGTERM vs SIGUSR1). +- **GUI state-machine soundness:** the external-stop path must converge to `.stopped` via polling and never wedge in a `.stopping`-like state with no clearing event. Trace every `refresh()` branch for a handle-less session through the drain window. NOTE the actual convergence driver is `processAlive` (`kill(0)` + `ps` fingerprint) returning false — i.e. process death — NOT pid-file removal per se (see Integration Seams correction). Verify there is no window where `processAlive` returns false while the drain is still in progress (which would flip the UI to `.stopped` prematurely). This holds because the recorder runs in the *same process* as the supervisor/pid-owner, which exits only after the full teardown `finally` block. Confirm it holds for **both** teardown branches: recorder-first SIGTERM drain, and capturer-first (`cli.py:519-522`, ErrorFrame → `_RECORDER_DRAIN_GRACE_SEC`=30 s force-cancel) — both keep the supervisor process alive until after drain, so `kill(0)` gates identically. +- **Enablement scope (DECIDED 2026-06-19):** Stop is enabled for *all* identity-verified live sessions, for parity with `flush` (which already reaches external sessions). The "crash-orphans only" alternative was rejected to avoid a controlling-tty heuristic the Swift side would have to compute and keep correct. +- **TOCTOU window:** residual race between `resolve_flush_target` returning and `os.kill(SIGTERM)` firing — confirm it is bounded and identical to flush's accepted residual risk. +- **Drain duration UX:** SIGTERM drain is not instant; verify the button cannot be spammed and the UI shows progress without faking a terminal state. + +## Implementation Checklist + +### Phase 1: `onoats stop` CLI subcommand (Python) + +**Impl files:** `src/onoats/cli.py` +**Test files:** `tests/test_cli.py` +**Test command:** `uv run pytest tests/test_cli.py tests/test_shutdown_drain.py -v` +**Validation cmd:** `uv run onoats stop --help` + +- Add `_cmd_stop(rest)` as a near-clone of `_cmd_flush` (`cli.py:1117-1166`): same `--data-dir` arg, same `resolve_flush_target` call, same stale-unlink + `ProcessLookupError` handling — the **only** change is `os.kill(pid, signal.SIGTERM)` instead of `SIGUSR1`, and the user-facing strings ("stop"/"SIGTERM"/"graceful shutdown"). +- Register `"stop": _cmd_stop` in the dispatch dict (anchor to the `"flush": _cmd_flush` entry, not an absolute line — `_HANDLERS` spans `cli.py:1300-1308`) and add `sub.add_parser("stop", help="Signal the running recorder to stop gracefully (drain + final flush).")` next to the `sub.add_parser("flush", ...)` line (`cli.py:1320-1326` block). Anchor inserts to these symbols; absolute line numbers drift as P1's own edits land. +- Prefer the near-clone over refactoring the already-shipped `_cmd_flush` in this PR. If drift between flush and stop is a concern, pin it with a parity test (Phase 1 tests) rather than extracting a shared helper that both stable+new paths depend on. +- Add `"stop"` to the parametrized subcommand tuple in `test_top_level_help_no_command` (`tests/test_cli.py:30`) — the existing assertion lists `flush` but not `stop`, and will keep passing while silently not guarding the new subcommand. +- Ensure `onoats stop --help` resolves without booting any service — the no-boot guarantee comes from `_cmd_stop`'s own local `argparse` + lazy import (mirroring `_cmd_flush` at `cli.py:1124-1130`), not from the top-level dispatch. + +### Phase 2: Menu-bar Stop rewiring (Swift — manual smoke, not /conduct-driven) + +**Impl files:** `native/onoats-menubar/Sources/RecorderModel.swift, native/onoats-menubar/Sources/OnoatsMenuBarApp.swift` + +- In `RecorderModel`, add a `stopExternal()` path that spawns `onoats stop` exactly as `flush()` does (`RecorderModel.swift:319-341`): subprocess, surface non-zero exit in a note (reuse the `flushNote` pattern), do NOT block on exit. +- Route the Stop button on the **`ours` value the `.running(ours:)` enum already carries** (single source — derived once in `refresh()` at `RecorderModel.swift:246` vs `:254`), NOT a fresh `proc != nil` read in the button action: `ours` → existing `p.terminate()`; `!ours` (verified external) → `stopExternal()`. Re-deriving `proc` independently in the action can disagree with the enum across a poll tick. Keep the in-handle path untouched for owned sessions. +- Drive the external-stop transition through `refresh()` polling only. Introduce a lightweight, cosmetic `stopRequested` flag that is **set synchronously in the button action** (before the subprocess spawn) and is the direct argument to `.disabled(stopRequested)` — do not wait for the next poll tick. Clear it **level-triggered inside `refresh()`**, placed **after** the `if let p = proc { … return }` early-return block (`RecorderModel.swift:243-252`) — i.e. at the `if alive`/`proc == nil` site (`:253`), NOT above it: `if !alive { stopRequested = false }`. Invariant: `stopRequested` is set only by `stopExternal()` and cleared only by `refresh()` observing `!alive`; no other writer. Do **not** set the `.stopping` enum case for external stops — it is cleared only by `handleExit`, which never fires without a `Process` handle. +- Enable the button for verified external sessions: change `.disabled(!ours)` to enable Stop for all verified sessions (enablement decision) and relabel `"Stop (external session)"` → `"Stop"`. +- Both the button-enable change and the `stopExternal()` handler MUST land in the **same commit** — enabling `.disabled` without the handler makes the button clickable with no action. +- Confirm `quitApp()` and `flush()` are unchanged in behaviour. +- **Smoke precondition (install step, not just merge):** `stopExternal()` execs the *installed* `onoats` resolved by `cliPath` (`RecorderModel.swift:321`), NOT the repo source — so an explicit install/refresh of the console script must happen between P1 and P2. Verify with the **bare installed binary** (`onoats stop --help`), NOT `uv run onoats stop --help` (which exercises repo source and can pass while the installed binary is stale). An older installed binary fails as a cosmetic "invalid choice" note, easily mis-diagnosed as a P2 bug. +- Manual smoke (no Swift unit harness — native Swift is not /conduct-runnable): (a) start via menu bar, kill the GUI to orphan the supervisor, relaunch, click Stop → supervisor drains and exits, menu returns to Stopped; (b) start, normal Stop still works; (c) double-click Stop → no duplicate `onoats stop` subprocess, no crash; (d) Flush still works mid-session; (e) during drain the menu shows "Stopping…" and does NOT flip to Stopped until the supervisor exits. + +### Phase 3: Zero-sample / denied-permission watchdog escalation (SEPARABLE — optional) + +**Impl files:** `native/onoats-menubar/Sources/RecorderModel.swift, src/onoats/cli.py` + +- Escalate the existing `zero-run-warning` ONOATS-EVENT (currently passive `status.set_warning`, surfaced only in the menu) to a macOS user notification so a denied Screen & System Audio Recording grant is loud instead of looking like a silent dead session. +- Keep the watchdog threshold/behaviour (`Resampler.swift` `zeroRunWarnSamples = 480_000`); only add the notification on the menu-bar side when `warning` transitions nil→set. +- This phase is independent of Phases 1–2 and can ship separately. It addresses the *other* half of the incident (silent permission denial) but does not affect stop-ability. **Sequencing:** its impl files (`RecorderModel.swift`, `cli.py`) overlap P1/P2; sequence P3 *after* P1+P2 merge — do not run it concurrently in an isolated worktree, which would cause textual merge conflicts (no logic dependency, just file overlap). + +## Technical Specifications + +### Files to Modify +- `src/onoats/cli.py` — add `_cmd_stop`; register subcommand + parser. Mirrors `_cmd_flush` (`cli.py:1117-1166`) and dispatch table (`_HANDLERS` at `cli.py:1300-1308`, `add_parser` calls at `cli.py:1320-1326`). +- `native/onoats-menubar/Sources/RecorderModel.swift` — add `stopExternal()` (clone of `flush()` at `:319-341`); branch `stop()`/button action by ownership; cosmetic stop-requested flag. +- `native/onoats-menubar/Sources/OnoatsMenuBarApp.swift` — enable Stop for verified external sessions; adjust label (`:71-72`). +- (Phase 3 only) zero-run notification wiring in `RecorderModel.swift`. + +### New Files to Create +- None. (New tests extend existing `tests/test_cli.py`.) + +### Architecture Decisions +- **Reuse `resolve_flush_target`, do not duplicate identity logic in Swift.** The original design deliberately centralises safe signalling in Python (`RecorderModel.swift:294-296`); duplicating the marker+`ps`-fingerprint resolver in Swift would risk drift against `test_native_contract_parity.py`. Routing Stop through a CLI subcommand keeps one source of truth and matches how `flush()` already works. +- **SIGTERM, not a new signal.** SIGTERM is already the graceful-shutdown trigger (`runtime.py:1144`, same as a single Ctrl-C) and is what the GUI's owned `p.terminate()` sends (Foundation maps `Process.terminate()` → SIGTERM — documented, not repo-verifiable; named in Requirements). No new runtime handler is needed; the external-stop path inherits the existing drain + final-flush. (Drain *content* correctness is inferred from the shared path and live-verified 2026-06-10, not unit-asserted — see Testing Notes.) +- **External-stop transition via polling, not `.stopping`.** `.stopping` is cleared only by `handleExit` (`RecorderModel.swift:389-412`), which fires off the `Process.terminationHandler` — absent for a handle-less external session. (For a `proc==nil` session `refresh()` would actually *overwrite* `.stopping` on the next tick rather than honour it, so the visible failure is a flickering/incorrect state, not a literal permanent wedge — either way `.stopping` is the wrong tool.) Instead, let `refresh()`'s existing `alive && proc==nil → .running(ours:false)` / `!alive → .stopped` logic drive convergence; the UI shows a cosmetic "Stopping…" affordance in the interim. +- **What drives `.stopped` is `processAlive` going false, not pid-file removal.** `refresh()` computes `alive = processAlive(pid)` via `kill(0)` + `ps` fingerprint (`RecorderModel.swift:175-189,229`) and sets `.stopped` on `!alive` (`:258`). Process **exit** alone flips it — even if the pid file momentarily lingers. Pid-file removal is *one* trigger, not the gating one. This matters because it means the GUI does NOT depend on the supervisor's status-before-pid-removal write ordering; correctness rests on the process actually exiting after drain. Verify there is no window where `kill(0)` fails while drain is still in progress. +- **CLI returns on signal delivery, not exit.** Like flush, `onoats stop` does not wait for the supervisor to finish draining (drain + capturer-group SIGTERM→grace→SIGKILL can take seconds — empirical, not pinned by a constant). Exit-0 means "signal sent." The GUI relies on `processAlive` → false for the authoritative stopped state. +- **Enablement scope = all verified sessions (DECIDED 2026-06-19).** Stop is enabled for *any* identity-verified live session (parity with `flush`, which already reaches external/terminal sessions). The "crash-orphans only" alternative (restrict by controlling tty / PPID==1) was rejected: it adds a heuristic the Swift side must compute and keep correct, for marginal protection of a deliberately foreground terminal session the same user could Ctrl-C anyway. +- **Rejected alternative — persist GUI ownership to re-adopt the orphan.** Writing a "GUI-started" marker the relaunched app could re-claim is more state to keep consistent and still fails if the marker write races the crash. The CLI-stop route works regardless of who started the session, so it strictly dominates. + +### Dependencies +- No new Python or Swift dependencies. Uses stdlib `signal`/`os.kill` (already imported in `cli.py`) and the existing subprocess-spawn pattern in `RecorderModel`. + +### Integration Seams + +| Seam | Writer (task) | Caller (task) | Contract | +|------|---------------|---------------|----------| +| Identity-checked stop signal | `resolve_flush_target` (`_vendor/pid.py`) | `_cmd_stop` (`cli.py`) | Resolver returns a verified pid or `stale`/refuse; caller signals ONLY a returned pid, unlinks ONLY when `stale=True`, treats `ProcessLookupError` as stale. | +| CLI stop ↔ GUI | `_cmd_stop` (`cli.py`) | `stopExternal()` (`RecorderModel.swift`) | CLI exit-0 = signal delivered, NOT stopped. GUI must derive stopped from `processAlive` → false via `refresh()`, never from exit code. | +| Drain → stopped detection | runtime SIGTERM handler (`runtime.py`/`dual.py`) | `refresh()` (`RecorderModel.swift`) | GUI keys "stopped" off `processAlive` (`kill(0)` + `ps` fingerprint) returning false, i.e. supervisor **exit** — NOT off pid-file absence. The supervisor's status-before-pid-removal write ordering (`dual.py`, writes status-stopped at ~`:570`, unlinks pid at ~`:584`) is real but is NOT what the GUI observes; the GUI never reads that ordering on the alive-path. | + +### Notes on the watchdog (context for Phase 3) +- Zero-run detection: `Resampler.swift` `zeroRunWarnSamples = 480_000` (30 s @ 16 kHz), one-shot, re-arms on real audio; emits `zero-run-warning`/`zero-run-clear` ONOATS-EVENTs. +- Supervisor drain: `cli.py` `_drain_capturer_stderr` parses ONOATS-EVENT and calls `status.set_warning(...)`. A denied tap has no preflight API — zero samples are the only signal. + +## Testing Notes + +### Test Approach +- [ ] Unit: `_cmd_stop` sends SIGTERM to a verified pid; mock `resolve_flush_target`. +- [ ] Unit (differential, defends "only the signal differs"): assert `_cmd_stop` sends SIGTERM and **NOT** SIGUSR1, AND `_cmd_flush` sends SIGUSR1 and **NOT** SIGTERM — parametrize so a copy-paste/shared-helper signal swap fails. (Mirror `test_flush_sends_sigusr1`, `test_cli.py:335`.) +- [ ] Unit: `_cmd_stop` refuses to signal on identity mismatch / missing fingerprint / no pid file; unlinks only when `stale=True`. Mirror **every** `test_flush_*` branch test (`test_cli.py:358-462` — no-pid-file, stale-dead-pid, foreign-marker, legacy-no-fingerprint, ps-probe-fails, recycled-identity-mismatch) 1:1 as `test_stop_*`; "behavioural twin" is only earned by branch parity, not asserted by one collapsed bullet. +- [ ] Unit: `_cmd_stop` handles `ProcessLookupError` (TOCTOU) as stale, returns non-zero, unlinks. +- [ ] Integration (highest-value, given SIGTERM's lethality): `stop` analogue of `test_flush_refuses_recycled_pid_identity_mismatch` (`test_cli.py:427`) — spawn a real foreign process on a recycled pid, run `onoats stop`, assert the foreign process is **still alive** (`proc.poll() is None`). Proves no SIGTERM reached an unrelated live pid. +- [ ] Unit: `stop` is in the dispatch table, in the `test_top_level_help_no_command` subcommand tuple (`test_cli.py:30`), and `onoats stop --help` resolves without booting a service. +- [ ] New regression: assert the graceful-shutdown teardown writes **status-stopped before unlinking the pid file** (the contract the GUI's predecessor reasoning relied on). NOTE: `test_status_file.py:241-245` already asserts this ordering *statically* (source-text `index()` check) — the new test MUST be **runtime/behavioural** (observe the actual write order during a real teardown), not a duplicate index check. Neither `test_shutdown_drain.py` (drives `stop_pipeline_for_shutdown` with a `FakeTask`, never raising SIGTERM nor exercising `_remove_pid_file`) nor the static check covers the runtime order. Use the real-teardown harness in `test_socket_supervisor.py`. +- [ ] New regression (or documented waiver): a **content-bearing** final-flush assertion on the SIGTERM path. `test_shutdown_drain.py` asserts an EndFrame is *queued*, not that it drains content; if not adding the assertion, record the memory `shutdown-drain-final-segment-edge` (live-verified 2026-06-10) as the confirming source in Findings. +- [ ] Regression: `test_native_contract_parity.py` still green (pid-file format unchanged). +- [ ] Manual (Swift): orphan-then-stop, normal-stop, double-stop, flush-still-works (see Phase 2). + +### Edge Cases Tested +- [ ] PID recycled to a foreign process between sessions → `resolve_flush_target` mismatch → no SIGTERM sent → foreign process survives (integration test above). +- [ ] Supervisor exits naturally during drain window → second Stop / refresh sees `processAlive` false → `.stopped`, no error. +- [ ] `ps` probe transiently fails → refuse to signal, do NOT unlink (no orphaning). +- [ ] External session stop drains slowly → UI shows "Stopping…", converges to `.stopped` when the supervisor exits (`processAlive` false), never wedges nor flips early. +- [ ] The Python half of "no early flip" — the supervisor process stays alive until drain+final-flush completes — is observable WITHOUT Swift via `test_socket_supervisor.py`'s teardown-timing harnesses (`_poll_pid_gone`, group-reap tests); assert pid liveness spans the whole drain rather than leaving the entire invariant to manual smoke. + +## Acceptance Criteria + +- `onoats stop` gracefully stops a verified session (owned or orphaned) and is a behavioural twin of `onoats flush` except for the signal. +- A GUI-started session orphaned by an app crash is stoppable from the menu bar after relaunch. +- Stop never signals an unverified or recycled pid (same guarantee as flush). +- Owned-session Stop, Flush, and Quit behaviours are unchanged. +- External-stop never wedges the GUI state machine; it converges to `.stopped` via polling on `processAlive` → false, and shows "Stopping…" (not Stopped) for the whole drain window. +- Content-bearing final flush on the SIGTERM path is either unit-asserted OR the waiver is recorded in `## Findings` with the `shutdown-drain-final-segment-edge` memory reference (no silent ship with neither). +- `uv run pytest` green (new + existing, incl. drain and parity tests); `ruff format` and `ruff check` clean. +- Manual smoke matrix in Phase 2 passes. +- Docs updated (README/AGENTS/CHANGELOG: new `onoats stop` subcommand). + + + +## Progress + +- [ ] Phase 1: `onoats stop` CLI subcommand (Python) +- [ ] Phase 2: Menu-bar Stop rewiring (Swift) +- [ ] Phase 3: Zero-sample watchdog escalation (separable) + +## Findings + +- (append findings here as work proceeds) + +## Issues & Solutions + +### Issue (anticipated): `.stopping` state has no clearing event for external sessions +- **Problem**: `.stopping` is exited only by `handleExit`, fired from `Process.terminationHandler`, which does not exist for a handle-less external session. Reusing `.stopping` for external stop yields incorrect/flickering state (for `proc==nil`, `refresh()` overwrites `.stopping` rather than honouring it). +- **Solution**: Drive external-stop convergence through `refresh()` polling on `processAlive` → false (process exit; not pid-file removal specifically); use a level-triggered cosmetic `stopRequested` flag (set synchronously in the button action, cleared in `refresh()` on `!alive`) instead of the enum state. +- **Files affected**: `native/onoats-menubar/Sources/RecorderModel.swift` + +## Final Results + +[Fill when complete] From ad5d7cc7a554d02ea7dc24d0b062cb32d8d4675e Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Fri, 19 Jun 2026 17:04:29 -0700 Subject: [PATCH 02/18] feat(cli): add identity-checked `onoats stop` (SIGTERM) subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 27 +++ CHANGELOG.md | 13 ++ README.md | 3 + .../20260619-bug-stoppable-orphan-session.md | 62 +++++- src/onoats/cli.py | 64 ++++++ tests/test_cli.py | 199 +++++++++++++++++- tests/test_socket_supervisor.py | 84 ++++++++ 7 files changed, 449 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9294853..1fbcc12 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -107,6 +107,33 @@ registry dep, not a git pin). `dual.main` and the supervisor route through it, so interactive/category handling can't drift. +## Identity-checked signalling (`onoats flush` / `onoats stop`) + +Both CLI signal subcommands share **one** identity gate — `resolve_flush_target` +(`_vendor/pid.py`) — and differ **only** in the signal sent: + +- `onoats flush` → **SIGUSR1** (continuation flush: rotate buffer, keep recording). +- `onoats stop` → **SIGTERM** (graceful shutdown: drain + final flush, then exit; + same trigger as a single Ctrl-C and the menu bar's owned `Process.terminate()`). + +Load-bearing invariants (pinned by `tests/test_cli.py`): + +- **Never signal an unverified or recycled pid.** The resolver validates the + marker, requires a cmdline fingerprint, probes liveness (`kill(0)`), and + compares the live `ps` cmdline against the stored one. Only a fully-verified + pid is signalled; unlink **only** when `stale=True`; treat `ProcessLookupError` + at signal time (TOCTOU) as stale. This matters **more** for `stop` than `flush` + — SIGTERM kills by default, so a blind signal to a recycled foreign pid would + terminate an unrelated process. A differential test asserts `stop` sends + SIGTERM-not-SIGUSR1 and `flush` sends SIGUSR1-not-SIGTERM, so a copy-paste + signal swap fails. +- **`stop` is a near-clone of `flush`, not a refactor.** Drift is pinned by tests + rather than a shared helper, keeping the shipped `flush` path untouched. +- Both return on **signal delivery**, not confirmed exit — a consumer must derive + "stopped" from the process actually exiting, never from the CLI exit code. +- **`onoats stop --help` resolves without booting a service** (local argparse + + lazy resolver import), like `flush`. + ## Reviewing a subprocess / process-boundary change When a change spawns a child process (`create_subprocess_*` / `Popen` / `exec`) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae18423..4660aed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,19 @@ no backdated tags exist). PR numbers `#1`–`#7` refer to this repository; older history predates the extraction and is cited by merge-commit SHA. Annotated tags exist from `v0.9.0` forward. +## [Unreleased] + +### Added +- `onoats stop` subcommand: signal the running recorder to stop gracefully + (SIGTERM → drain + final flush, then exit). It is a behavioural twin of + `onoats flush` and reuses the same identity gate (`resolve_flush_target`: + marker + cmdline-fingerprint + liveness), so it only ever signals the verified + recorder and never a recycled pid — which matters more than for flush because + SIGTERM kills by default. Returns on signal delivery, not confirmed exit; like + flush, `onoats stop --help` resolves without booting a service. (Phase 1 of the + stoppable-orphan-session fix: the menu-bar Stop rewiring that routes an + orphaned GUI session through this seam is a follow-up.) + ## [1.1.0] - 2026-06-12 First PyPI release (`pip install onoats` / `uv tool install onoats`). diff --git a/README.md b/README.md index e8e0e20..6e4a77a 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,9 @@ Other subcommands: ```bash onoats bot-single # legacy mic-only recorder onoats flush # tell the running recorder to rotate its buffer now +onoats stop # signal the running recorder to stop gracefully (drain + + # final flush); identity-checked like flush, so it only ever + # signals the verified recorder — never a recycled pid onoats devices # list audio input/output devices (PortAudio's view; under # the socket path it adds a note — the native capturer binds # the system default input / default-output tap instead) diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index 51896a0..cc35e1b 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -149,13 +149,71 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Progress -- [ ] Phase 1: `onoats stop` CLI subcommand (Python) +- [x] Phase 1: `onoats stop` CLI subcommand (Python) - [ ] Phase 2: Menu-bar Stop rewiring (Swift) - [ ] Phase 3: Zero-sample watchdog escalation (separable) ## Findings -- (append findings here as work proceeds) +### Phase 1 (2026-06-19) — `onoats stop` CLI subcommand shipped + +- **Implementation.** `_cmd_stop` (`src/onoats/cli.py`) is a near-clone of + `_cmd_flush`: identical `--data-dir` arg, identical `resolve_flush_target` + call, identical stale-unlink + `ProcessLookupError` handling. The *only* + behavioural divergence is `os.kill(pid, signal.SIGTERM)` (vs SIGUSR1) plus + user-facing strings. The resolver is reused verbatim — no weakened or + duplicated identity check. Registered in `_HANDLERS` (anchored to the `flush` + entry) and via `sub.add_parser("stop", …)`; `"stop"` added to the + `test_top_level_help_no_command` subcommand tuple. + +- **Tests (all green, `tests/test_cli.py`).** Every `test_flush_*` branch mirrored + 1:1 as `test_stop_*` (no-pid-file, stale-dead-pid, foreign-marker, + legacy-no-fingerprint, ps-probe-fails, recycled-identity-mismatch). The + recycled-pid case spawns a real `sleep` on the recycled pid and asserts it is + **still alive** after `onoats stop` (proves no SIGTERM reached an unrelated + live pid). A parametrized differential test asserts `stop` sends + SIGTERM-and-NOT-SIGUSR1 and `flush` sends SIGUSR1-and-NOT-SIGTERM, so a + copy-paste signal swap fails. Plus dispatch-table + `--help`-without-boot. + +- **Runtime write-order regression** (`tests/test_socket_supervisor.py`: + `test_graceful_teardown_writes_status_stopped_before_pid_unlink`). Drives the + real teardown producers (`runtime._write_pid_file` / `_write_status_running` / + `_write_status_stopped` / `_remove_pid_file` — the exact symbols dual.py's + `_run_shutdown` imports and calls) against a real filesystem and spies on the + pid-unlink boundary, observing that the on-disk status already reads + `running=False` at the instant the pid file is removed. This is the + **runtime** complement the plan asked for — distinct from the **static** + source-text index check in `test_status_file.py:243-245`. + - *Scope note (named, not hidden):* the producer **call order** inside + `dual._run_shutdown` is a nested closure unreachable without booting the heavy + STT/VAD stack, so it remains guarded by the static index check; this runtime + test proves the producers' on-disk effects are **synchronous + durable + + consistent** (status-stopped lands before pid removal, no async lag / no + pid-gone-while-status-running window). The plan's pointer to + "test_socket_supervisor.py's real-teardown harness" refers to that file's + teardown-timing harness; note its supervisor lifecycle tests substitute a + *fake* `run_onoats_dual`, so they do not themselves exercise the recorder's + pid/status writes — hence this dedicated producer-level runtime test. + +- **Content-bearing final flush on SIGTERM — WAIVER (per acceptance criteria).** + Not adding a new content-bearing unit assertion in Phase 1: the SIGTERM path + shares the existing `shutdown_event` → terminal-flush drain, and the + content-bearing final flush was **live-verified on 2026-06-10** (memory + `shutdown-drain-final-segment-edge`, recorded EDGE CLOSED after a 5b menu-bar + smoke). `test_shutdown_drain.py` asserts an EndFrame is *queued* before the + terminal flush; the content-drain itself needs a hardware/pipeline run, which + Phase 1 (CLI-only) does not boot. This waiver is the plan-sanctioned + alternative to a silent skip. + +- **Verification.** `uv run pytest` full suite: 278 passed. Target files + (`test_cli.py test_shutdown_drain.py test_socket_supervisor.py + test_native_contract_parity.py`): 112 passed. `uv run onoats stop --help` + resolves without booting a service. `ruff format` + `ruff check` clean. + +- **Remaining:** Phase 2 (Swift menu-bar Stop rewiring) requires building the + menu-bar app and a manual orphan-then-stop smoke on the user's machine — not + autonomously runnable; deferred. Phase 3 is separable/optional and sequenced + after P2. ## Issues & Solutions diff --git a/src/onoats/cli.py b/src/onoats/cli.py index d04214b..e0d1ba3 100644 --- a/src/onoats/cli.py +++ b/src/onoats/cli.py @@ -1166,6 +1166,65 @@ def _cmd_flush(rest: list[str]) -> int: return 0 +def _cmd_stop(rest: list[str]) -> int: + """Send SIGTERM to the running recorder so it shuts down gracefully. + + Near-clone of ``_cmd_flush``: the safe identity-checked signalling + (``resolve_flush_target`` → marker + cmdline fingerprint) is reused verbatim, + so a recycled foreign pid is never signalled. The PID-recycling guard matters + *more* here than for flush — SIGTERM's default disposition kills, so + signalling an unrelated pid would terminate it. The only behavioural change + from flush is the signal: SIGTERM (the graceful-shutdown trigger, + ``runtime.py`` — same as a single Ctrl-C / the GUI's owned + ``Process.terminate()``) instead of SIGUSR1. The recorder drains and writes a + final flush before exiting; the command returns on signal delivery, NOT on + confirmed exit. + """ + parser = argparse.ArgumentParser(prog="onoats stop") + parser.add_argument( + "--data-dir", + default=None, + help="Data dir override (else $ONOATS_DATA_DIR / XDG default).", + ) + args = parser.parse_args(rest) + data_dir = Path(args.data_dir) if args.data_dir else None + + from onoats._vendor.pid import resolve_flush_target + + pid_path = _pid_path(data_dir) + target = resolve_flush_target(pid_path) + if target.pid is None: + # Identity could not be confirmed. Drop a now-untrustworthy pid file so + # the next run starts clean, but never signal an unverified pid. + if target.stale: + try: + pid_path.unlink() + except OSError: + pass + print(f"onoats stop: {target.reason} (pid file {pid_path})", file=sys.stderr) + return 1 + pid = target.pid + try: + os.kill(pid, signal.SIGTERM) + except ProcessLookupError: + # Raced: the verified recorder exited between the identity check and + # the signal. Treat as stale rather than signalling a recycled pid. + try: + pid_path.unlink() + except OSError: + pass + print( + f"onoats stop: recorder pid {pid} is not running (stale pid file)", + file=sys.stderr, + ) + return 1 + except OSError as exc: + print(f"onoats stop: could not signal pid {pid}: {exc}", file=sys.stderr) + return 1 + print(f"onoats stop: sent SIGTERM to recorder pid {pid} (graceful shutdown)") + return 0 + + def _cmd_devices(rest: list[str]) -> int: """List audio input/output devices (reuses the device picker's enumeration).""" argparse.ArgumentParser(prog="onoats devices").parse_args(rest) @@ -1302,6 +1361,7 @@ def _cmd_status(rest: list[str]) -> int: "bot": _cmd_bot, "bot-single": _cmd_bot_single, "flush": _cmd_flush, + "stop": _cmd_stop, "convert": _cmd_convert, "devices": _cmd_devices, "status": _cmd_status, @@ -1321,6 +1381,10 @@ def _build_parser() -> argparse.ArgumentParser: sub.add_parser("bot", help="Dual-input recorder (mic + system loopback).") sub.add_parser("bot-single", help="Legacy single-input (mic-only) recorder.") sub.add_parser("flush", help="Signal the running recorder to rotate its buffer.") + sub.add_parser( + "stop", + help="Signal the running recorder to stop gracefully (drain + final flush).", + ) sub.add_parser("convert", help="Render pending/*.jsonl into markdown transcripts.") sub.add_parser("devices", help="List audio input/output devices.") sub.add_parser("status", help="Report recorder pid / running state + data dir.") diff --git a/tests/test_cli.py b/tests/test_cli.py index 0bb245f..a719b43 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -27,7 +27,16 @@ def test_top_level_help_no_command(capsys): out = capsys.readouterr().out assert rc == 0 assert "onoats" in out - for sub in ("init", "bot", "bot-single", "flush", "convert", "devices", "status"): + for sub in ( + "init", + "bot", + "bot-single", + "flush", + "stop", + "convert", + "devices", + "status", + ): assert sub in out @@ -459,3 +468,191 @@ def test_flush_refuses_recycled_pid_identity_mismatch(capsys, _isolate_env): finally: proc.terminate() proc.wait(timeout=5) + + +# --------------------------------------------------------------------------- +# stop — behavioural twin of flush, EXCEPT it sends SIGTERM (graceful shutdown) +# instead of SIGUSR1. The identity-checked signalling (resolve_flush_target → +# marker + cmdline fingerprint) is reused verbatim, so a recycled foreign pid is +# never signalled — which matters MORE here because SIGTERM kills by default. +# Every test_flush_* branch is mirrored 1:1 below. +# --------------------------------------------------------------------------- + + +def test_stop_sends_sigterm(monkeypatch, _isolate_env): + """Happy path: live process cmdline matches the stored fingerprint.""" + pid_path = cli._pid_path(_isolate_env) + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text("9001\nonoats-bot\nonoats bot\n0.0\n", encoding="utf-8") + + sent = {} + + def fake_kill(pid, sig): + # sig 0 is the liveness probe; record only the real signal. + if sig != 0: + sent["pid"] = pid + sent["sig"] = sig + + monkeypatch.setattr("os.kill", fake_kill) + # Live readback matches the stored 3rd-line fingerprint → identity confirmed. + monkeypatch.setattr("onoats._vendor.pid._live_ps_cmdline", lambda pid: "onoats bot") + rc = cli.main(["stop"]) + assert rc == 0 + assert sent["pid"] == 9001 + assert sent["sig"] == signal.SIGTERM + + +@pytest.mark.parametrize( + ("command", "want_sig", "reject_sig"), + [ + ("stop", signal.SIGTERM, signal.SIGUSR1), + ("flush", signal.SIGUSR1, signal.SIGTERM), + ], +) +def test_stop_and_flush_send_distinct_signals( + monkeypatch, _isolate_env, command, want_sig, reject_sig +): + """Differential guard: the ONLY intended divergence between stop and flush is + the signal number. ``stop`` MUST send SIGTERM and NOT SIGUSR1; ``flush`` MUST + send SIGUSR1 and NOT SIGTERM. A copy-paste/shared-helper signal swap (or a + handler accidentally calling the other) fails here.""" + pid_path = cli._pid_path(_isolate_env) + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text("9001\nonoats-bot\nonoats bot\n0.0\n", encoding="utf-8") + + sent = [] + + def fake_kill(pid, sig): + if sig != 0: # ignore the liveness probe + sent.append(sig) + + monkeypatch.setattr("os.kill", fake_kill) + monkeypatch.setattr("onoats._vendor.pid._live_ps_cmdline", lambda pid: "onoats bot") + + rc = cli.main([command]) + assert rc == 0 + assert sent == [want_sig], f"{command} must send exactly {want_sig!r}, got {sent!r}" + assert reject_sig not in sent, f"{command} must NOT send {reject_sig!r}" + + +def test_stop_no_pid_file(_isolate_env): + assert cli.main(["stop"]) == 1 + + +def test_stop_stale_dead_pid(monkeypatch, _isolate_env): + """A pid whose process is gone is stale: no signal, pid file removed.""" + pid_path = cli._pid_path(_isolate_env) + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text("12345\nonoats-bot\nonoats bot\n0.0\n", encoding="utf-8") + + def fake_kill(pid, sig): + raise ProcessLookupError() + + monkeypatch.setattr("os.kill", fake_kill) + assert cli.main(["stop"]) == 1 + assert not pid_path.exists() # stale file cleaned up + + +def test_stop_ignores_foreign_pid_marker(_isolate_env): + """A pid file without the onoats-bot marker is not stoppable.""" + pid_path = cli._pid_path(_isolate_env) + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text("9001\nsomething-else\ncmd\n0.0\n", encoding="utf-8") + assert cli.main(["stop"]) == 1 + + +def test_stop_refuses_legacy_pid_file_without_fingerprint(monkeypatch, _isolate_env): + """A marker-valid but fingerprint-less (legacy) pid file is not signalled.""" + pid_path = cli._pid_path(_isolate_env) + pid_path.parent.mkdir(parents=True, exist_ok=True) + # Two-line legacy format: pid + marker, no cmdline fingerprint. + pid_path.write_text("9001\nonoats-bot\n", encoding="utf-8") + + def fake_kill(pid, sig): # pragma: no cover - must never be called + raise AssertionError("stop must not signal a pid it cannot verify") + + monkeypatch.setattr("os.kill", fake_kill) + rc = cli.main(["stop"]) + assert rc == 1 + # No fingerprint to compare against → file is *not* treated as stale. + assert pid_path.exists() + + +def test_stop_keeps_pid_file_when_ps_probe_fails(monkeypatch, capsys, _isolate_env): + """A live recorder whose ``ps`` identity probe fails (ps missing/timeout) + must NOT be signalled and must NOT have its pid file deleted — a transient + probe failure is indeterminate, not proof the recorder is gone.""" + pid_path = cli._pid_path(_isolate_env) + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text("9001\nonoats-bot\nonoats bot\n0.0\n", encoding="utf-8") + + def fake_kill(pid, sig): + if sig == 0: + return # liveness probe succeeds → process is alive + raise AssertionError("stop must not signal an unverifiable pid") + + monkeypatch.setattr("os.kill", fake_kill) + # Identity probe fails (e.g. ps unavailable / timed out). + monkeypatch.setattr("onoats._vendor.pid._live_ps_cmdline", lambda pid: None) + + rc = cli.main(["stop"]) + err = capsys.readouterr().err.lower() + + assert rc == 1 + assert "could not verify" in err + # Live recorder's pid file must survive an indeterminate probe. + assert pid_path.exists() + + +def test_stop_refuses_recycled_pid_identity_mismatch(capsys, _isolate_env): + """Highest-value regression (given SIGTERM's lethality): a recycled pid + pointing at an unrelated *live* process must not be signalled. A blind + SIGTERM would terminate that foreign process; the identity check stops it. + + Integration-flavoured: exercises the real ``ps`` readback path. Skips on the + (rare) host without ``ps``/``sleep`` rather than misreporting the mismatch as + a "not running" stale path. + """ + import shutil + import subprocess + + if not shutil.which("ps") or not shutil.which("sleep"): + pytest.skip("requires ps and sleep for the live identity readback") + + # A real, unrelated live process whose cmdline will not match the stored + # fingerprint. SIGTERM would terminate it if stop signalled blindly. + proc = subprocess.Popen(["sleep", "30"]) + try: + pid_path = cli._pid_path(_isolate_env) + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text( + f"{proc.pid}\nonoats-bot\nonoats bot (this is not sleep)\n0.0\n", + encoding="utf-8", + ) + + rc = cli.main(["stop"]) + err = capsys.readouterr().err.lower() + + assert rc == 1 + assert "identity mismatch" in err + # The unrelated process must still be ALIVE — no SIGTERM was sent. + assert proc.poll() is None + finally: + proc.terminate() + proc.wait(timeout=5) + + +def test_stop_in_dispatch_table(): + """`stop` is wired into the subcommand dispatch table.""" + assert cli._HANDLERS["stop"] is cli._cmd_stop + + +def test_stop_help_resolves_without_booting(capsys): + """`onoats stop --help` resolves via _cmd_stop's own local argparse (lazy + resolver import), so it never boots a service.""" + with pytest.raises(SystemExit) as exc: + cli.main(["stop", "--help"]) + assert exc.value.code == 0 + out = capsys.readouterr().out.lower() + assert "usage" in out + assert "onoats stop" in out diff --git a/tests/test_socket_supervisor.py b/tests/test_socket_supervisor.py index 816b889..2d7585d 100644 --- a/tests/test_socket_supervisor.py +++ b/tests/test_socket_supervisor.py @@ -1964,3 +1964,87 @@ async def _bind_late(): assert rec is not None, "the extension must write the prestart waiting record" assert rec.running is True assert rec.warning and "permission prompt" in rec.warning + + +# --------------------------------------------------------------------------- +# Graceful-teardown write ordering (RUNTIME): status-stopped lands on disk +# BEFORE the pid file is unlinked. +# +# This is the contract `onoats stop` (and the menu bar's external-stop polling) +# rely on indirectly: the GUI keys "stopped" off the supervisor PROCESS exiting, +# but `onoats status` (and any pid-backstop reader) must never observe pid-gone +# while the status file still claims running. The producer call order is asserted +# STATICALLY in test_status_file.py (source-text index check). This test is the +# RUNTIME complement: it drives the real teardown producers +# (`onoats.runtime._write_pid_file` / `_write_status_running` / +# `_write_status_stopped` / `_remove_pid_file` — the exact symbols dual.py's +# `_run_shutdown` imports and calls) against a real filesystem and observes, AT +# THE INSTANT the pid file is unlinked, that the on-disk status already reads +# running=False. A producer whose status write lagged (non-durable / async) or a +# reorder at the producer-call level would flip the observed value and fail here +# — neither of which the static index check nor test_shutdown_drain.py (which +# drives a FakeTask and never exercises _remove_pid_file) can catch. +# --------------------------------------------------------------------------- + + +def test_graceful_teardown_writes_status_stopped_before_pid_unlink( + tmp_path, monkeypatch +): + from onoats import runtime + from onoats import status as status_file + + data_dir = tmp_path / "data" + data_dir.mkdir(parents=True, exist_ok=True) + + # Start-of-session: real pid file + real running status (pid FIRST, then + # status — the start half of the same ordering contract). + pid_path = runtime._write_pid_file(data_dir) + runtime._write_status_running(data_dir, audio_source="socket", stt_label="fake-stt") + assert pid_path.exists() + start_rec = status_file.read_status(data_dir) + assert start_rec is not None and start_rec.running is True + + # Instrument the unlink boundary: snapshot the on-disk status at the exact + # instant the pid file is removed. dual.py calls `_remove_pid_file` (this + # symbol) as the final teardown step; spying here observes the real order. + observed: dict = {} + real_remove = runtime._remove_pid_file + + def _spy_remove(p): + rec = status_file.read_status(data_dir) + observed["running_at_unlink"] = None if rec is None else rec.running + observed["pid_file_present_at_unlink"] = p.exists() + return real_remove(p) + + monkeypatch.setattr(runtime, "_remove_pid_file", _spy_remove) + + # Graceful teardown, in dual.py `_run_shutdown` order: status-stopped FIRST… + runtime._write_status_stopped(data_dir, exit_reason="graceful") + # …status-stopped must be durably on disk (synchronous atomic write) the + # moment it returns, before the pid is touched. + mid_rec = status_file.read_status(data_dir) + assert mid_rec is not None and mid_rec.running is False, ( + "status-stopped must be durable on disk before the pid file is unlinked" + ) + # …then remove the pid file (instrumented). + runtime._remove_pid_file(pid_path) + + # The unlink spy observed status already running=False — no window where a + # reader sees pid-gone while the status file still claims a live recorder. + assert observed["running_at_unlink"] is False, ( + "pid file was unlinked while the on-disk status still claimed running — " + "a reader could observe pid-gone + status-running (the exact disagreement " + "the ordering contract forbids)" + ) + assert observed["pid_file_present_at_unlink"] is True, ( + "the pid file should still exist at the moment _remove_pid_file is entered" + ) + + # Final on-disk state: pid gone, status durably stopped, start detail kept. + assert not pid_path.exists() + end_rec = status_file.read_status(data_dir) + assert end_rec is not None and end_rec.running is False + assert end_rec.exit_reason == "graceful" + assert end_rec.audio_source == "socket", ( + "stop must preserve start-of-session detail" + ) From b10c079def84ae19ae167da66a2ef7779975d19d Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Fri, 19 Jun 2026 17:36:27 -0700 Subject: [PATCH 03/18] refactor(dual): extract shutdown tail to `_finalize_shutdown_status` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../20260619-bug-stoppable-orphan-session.md | 37 ++++----- src/onoats/dual.py | 76 +++++++++++++------ tests/test_socket_supervisor.py | 61 ++++++++------- 3 files changed, 103 insertions(+), 71 deletions(-) diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index cc35e1b..c49d6fe 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -176,24 +176,25 @@ Make any identity-verified live recorder session stoppable from the menu bar — copy-paste signal swap fails. Plus dispatch-table + `--help`-without-boot. - **Runtime write-order regression** (`tests/test_socket_supervisor.py`: - `test_graceful_teardown_writes_status_stopped_before_pid_unlink`). Drives the - real teardown producers (`runtime._write_pid_file` / `_write_status_running` / - `_write_status_stopped` / `_remove_pid_file` — the exact symbols dual.py's - `_run_shutdown` imports and calls) against a real filesystem and spies on the - pid-unlink boundary, observing that the on-disk status already reads - `running=False` at the instant the pid file is removed. This is the - **runtime** complement the plan asked for — distinct from the **static** - source-text index check in `test_status_file.py:243-245`. - - *Scope note (named, not hidden):* the producer **call order** inside - `dual._run_shutdown` is a nested closure unreachable without booting the heavy - STT/VAD stack, so it remains guarded by the static index check; this runtime - test proves the producers' on-disk effects are **synchronous + durable + - consistent** (status-stopped lands before pid removal, no async lag / no - pid-gone-while-status-running window). The plan's pointer to - "test_socket_supervisor.py's real-teardown harness" refers to that file's - teardown-timing harness; note its supervisor lifecycle tests substitute a - *fake* `run_onoats_dual`, so they do not themselves exercise the recorder's - pid/status writes — hence this dedicated producer-level runtime test. + `test_shutdown_tail_writes_status_stopped_before_pid_unlink`, parametrized over + graceful + fatal-ErrorFrame branches). The shutdown tail that was inline in + `dual._run_shutdown` (a nested closure, unreachable without booting the STT/VAD + stack) was extracted to a module-level helper `dual._finalize_shutdown_status` + — a test-induced refactor done precisely so the **actual ordering logic** is + runtime-reachable. The test seeds a real pid file + running status, spies on + `dual._remove_pid_file`, drives the real helper, and asserts the on-disk status + already reads `running=False` at the instant the pid file is unlinked. Because + it now exercises dual.py's real call order (not a hand-sequenced copy), a + reorder of the status-stopped / pid-removal pair fails here at runtime. + - The **static** source-text index check (`test_status_file.py:243-245`) is now + redundant with this runtime test but kept (cheap, and still guards the + start-half `pid-write < status-running` order). The literals it greps + (`_write_status_stopped(`, `_remove_pid_file(pid_path)`, + `_write_pid_file(data_dir)`, `_write_status_running(`) all survive the + refactor in the correct source order, so it stays green. + - *Note:* the supervisor lifecycle tests in this file substitute a *fake* + `run_onoats_dual`, so they do not themselves exercise the recorder's + pid/status writes — hence this dedicated helper-level runtime test. - **Content-bearing final flush on SIGTERM — WAIVER (per acceptance criteria).** Not adding a new content-bearing unit assertion in Phase 1: the SIGTERM path diff --git a/src/onoats/dual.py b/src/onoats/dual.py index 229cc1c..d07fd19 100644 --- a/src/onoats/dual.py +++ b/src/onoats/dual.py @@ -46,11 +46,11 @@ SttPreflightError, stop_pipeline_for_shutdown, wait_or_force, - _remove_pid_file, _create_stt_service, log_stt_server_rss, _install_signal_handlers, _mark_status_rotation, + _remove_pid_file, _restore_terminal, _start_keypress_reader, _topic_pipeline_tasks, @@ -63,6 +63,45 @@ ) +def _finalize_shutdown_status( + data_dir: Path, + pid_path: Path, + *, + ended_by_error: bool, + old_terminal_settings: object | None = None, +) -> None: + """Single-writer shutdown tail: write status-stopped BEFORE removing the pid. + + Write ordering (status-file contract): status-stopped FIRST, then the pid is + removed — so ``onoats status`` (the pid backstop) never observes pid-gone + while the status file still claims running, and the menu bar's external-stop + polling never sees an inconsistent half-torn-down state. ``ended_by_error`` + (set in ``run_onoats_dual``'s outer run body) distinguishes a fatal-ErrorFrame + self-end from a graceful shutdown; the specific cause (capturer-crash vs the + recorder's own ErrorFrame, mic/system-audio denial) is enriched by the socket + supervisor, which alone knows the final rc. + + Extracted module-level (rather than left inline in ``_run_shutdown``) so the + write ordering is exercisable at runtime without booting the recorder stack — + see ``test_socket_supervisor.py``. Terminal restore + pid removal live here + too so the two shutdown call sites (``_shutdown_watcher`` and the outer + ``finally`` block) stay idempotent regardless of ordering. + """ + if ended_by_error: + _write_status_stopped( + data_dir, + exit_reason="fatal_error_frame", + last_error=( + "dual pipeline ended on a fatal ErrorFrame (capture branch " + "EOF / read-idle / framing failure)" + ), + ) + else: + _write_status_stopped(data_dir, exit_reason="graceful") + _restore_terminal(old_terminal_settings) + _remove_pid_file(pid_path) + + async def _shutdown_stt_service(stt_service, label: str) -> None: """Best-effort drain for STT services used by the dual-input path. @@ -558,30 +597,17 @@ async def _run_shutdown() -> None: await _shutdown_stt_service(system_stt, "system") await log_stt_server_rss("shutdown") - # Status file: record stop + failure reason BEFORE removing the pid file. - # Write ordering (status-file contract): status-stopped FIRST, then the - # pid is removed — so `onoats status` (pid backstop) never observes - # pid-gone while the status file still claims running. ``ended_by_error`` - # (set in the outer run body) distinguishes a fatal-ErrorFrame self-end - # from a graceful shutdown; the specific cause (capturer-crash vs the - # recorder's own ErrorFrame, mic/system-audio denial) is enriched by the - # socket supervisor, which alone knows the final rc. - if ended_by_error: - _write_status_stopped( - data_dir, - exit_reason="fatal_error_frame", - last_error=( - "dual pipeline ended on a fatal ErrorFrame (capture branch " - "EOF / read-idle / framing failure)" - ), - ) - else: - _write_status_stopped(data_dir, exit_reason="graceful") - # Restore terminal and remove PID file inside the single-writer - # shutdown path so the two call sites (_shutdown_watcher and the - # outer ``finally`` block) are truly idempotent regardless of ordering. - _restore_terminal(old_terminal_settings) - _remove_pid_file(pid_path) + # Single-writer shutdown tail: status-stopped BEFORE pid removal, plus + # terminal restore. Extracted to a module-level helper so the write + # ordering is exercisable at runtime without booting the recorder stack + # (see test_socket_supervisor.py); ``ended_by_error`` (set in the outer + # run body) selects the failure detail. + _finalize_shutdown_status( + data_dir, + pid_path, + ended_by_error=ended_by_error, + old_terminal_settings=old_terminal_settings, + ) logger.info("Shutdown: complete") async def _shutdown_watcher() -> None: diff --git a/tests/test_socket_supervisor.py b/tests/test_socket_supervisor.py index 2d7585d..21e829a 100644 --- a/tests/test_socket_supervisor.py +++ b/tests/test_socket_supervisor.py @@ -1967,29 +1967,35 @@ async def _bind_late(): # --------------------------------------------------------------------------- -# Graceful-teardown write ordering (RUNTIME): status-stopped lands on disk -# BEFORE the pid file is unlinked. +# Shutdown write ordering (RUNTIME): status-stopped lands on disk BEFORE the pid +# file is unlinked. # # This is the contract `onoats stop` (and the menu bar's external-stop polling) # rely on indirectly: the GUI keys "stopped" off the supervisor PROCESS exiting, # but `onoats status` (and any pid-backstop reader) must never observe pid-gone # while the status file still claims running. The producer call order is asserted # STATICALLY in test_status_file.py (source-text index check). This test is the -# RUNTIME complement: it drives the real teardown producers -# (`onoats.runtime._write_pid_file` / `_write_status_running` / -# `_write_status_stopped` / `_remove_pid_file` — the exact symbols dual.py's -# `_run_shutdown` imports and calls) against a real filesystem and observes, AT -# THE INSTANT the pid file is unlinked, that the on-disk status already reads -# running=False. A producer whose status write lagged (non-durable / async) or a -# reorder at the producer-call level would flip the observed value and fail here -# — neither of which the static index check nor test_shutdown_drain.py (which -# drives a FakeTask and never exercises _remove_pid_file) can catch. +# RUNTIME complement: it drives dual.py's real shutdown tail +# (`dual._finalize_shutdown_status` — the exact helper `run_onoats_dual`'s +# `_run_shutdown` calls, factored out of the closure precisely so it is +# runtime-reachable without booting the STT/VAD stack) against a real filesystem. +# It spies the pid-unlink boundary and observes, AT THE INSTANT the pid file is +# unlinked, that the on-disk status already reads running=False. A reorder of the +# status-stopped / pid-removal pair inside the helper, or a status write that +# lagged (non-durable / async), flips the observed value and fails here — neither +# of which the static index check nor test_shutdown_drain.py (which drives a +# FakeTask and never exercises _remove_pid_file) can catch. # --------------------------------------------------------------------------- -def test_graceful_teardown_writes_status_stopped_before_pid_unlink( - tmp_path, monkeypatch +@pytest.mark.parametrize( + ("ended_by_error", "expected_reason"), + [(False, "graceful"), (True, "fatal_error_frame")], +) +def test_shutdown_tail_writes_status_stopped_before_pid_unlink( + tmp_path, monkeypatch, ended_by_error, expected_reason ): + from onoats import dual from onoats import runtime from onoats import status as status_file @@ -2005,10 +2011,11 @@ def test_graceful_teardown_writes_status_stopped_before_pid_unlink( assert start_rec is not None and start_rec.running is True # Instrument the unlink boundary: snapshot the on-disk status at the exact - # instant the pid file is removed. dual.py calls `_remove_pid_file` (this - # symbol) as the final teardown step; spying here observes the real order. + # instant the pid file is removed. `_finalize_shutdown_status` calls + # `_remove_pid_file` (resolved in dual.py's namespace) as its final step, so + # patching the name there observes the helper's REAL ordering. observed: dict = {} - real_remove = runtime._remove_pid_file + real_remove = dual._remove_pid_file def _spy_remove(p): rec = status_file.read_status(data_dir) @@ -2016,18 +2023,15 @@ def _spy_remove(p): observed["pid_file_present_at_unlink"] = p.exists() return real_remove(p) - monkeypatch.setattr(runtime, "_remove_pid_file", _spy_remove) + monkeypatch.setattr(dual, "_remove_pid_file", _spy_remove) - # Graceful teardown, in dual.py `_run_shutdown` order: status-stopped FIRST… - runtime._write_status_stopped(data_dir, exit_reason="graceful") - # …status-stopped must be durably on disk (synchronous atomic write) the - # moment it returns, before the pid is touched. - mid_rec = status_file.read_status(data_dir) - assert mid_rec is not None and mid_rec.running is False, ( - "status-stopped must be durable on disk before the pid file is unlinked" + # Drive dual.py's actual shutdown tail (not a hand-sequenced copy of it). + dual._finalize_shutdown_status( + data_dir, + pid_path, + ended_by_error=ended_by_error, + old_terminal_settings=None, ) - # …then remove the pid file (instrumented). - runtime._remove_pid_file(pid_path) # The unlink spy observed status already running=False — no window where a # reader sees pid-gone while the status file still claims a live recorder. @@ -2040,11 +2044,12 @@ def _spy_remove(p): "the pid file should still exist at the moment _remove_pid_file is entered" ) - # Final on-disk state: pid gone, status durably stopped, start detail kept. + # Final on-disk state: pid gone, status durably stopped with the branch's + # exit reason, start-of-session detail preserved. assert not pid_path.exists() end_rec = status_file.read_status(data_dir) assert end_rec is not None and end_rec.running is False - assert end_rec.exit_reason == "graceful" + assert end_rec.exit_reason == expected_reason assert end_rec.audio_source == "socket", ( "stop must preserve start-of-session detail" ) From f245f7120432c3b27147e05d77b2ffbe3b46d1db Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Fri, 19 Jun 2026 19:22:54 -0700 Subject: [PATCH 04/18] =?UTF-8?q?docs(dev-plan):=20record=20orphan-recover?= =?UTF-8?q?y=20flows=20+=20stop=E2=86=92start=20race=20constraint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../20260619-bug-stoppable-orphan-session.md | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index c49d6fe..bd26c95 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -223,6 +223,54 @@ Make any identity-verified live recorder session stoppable from the menu bar — - **Solution**: Drive external-stop convergence through `refresh()` polling on `processAlive` → false (process exit; not pid-file removal specifically); use a level-triggered cosmetic `stopRequested` flag (set synchronously in the button action, cleared in `refresh()` on `!alive`) instead of the enum state. - **Files affected**: `native/onoats-menubar/Sources/RecorderModel.swift` +### Recommended orphan-recovery flows (UX decision, 2026-06-19 design discussion) + +Captured from a post-Phase-1 design discussion so Phase 2 (and any docs) pick a +sound flow rather than re-deriving it. Verified against current code +(`RecorderModel.refresh()`, `OnoatsMenuBarApp` button gating, `_write_pid_file`, +`_remove_pid_file`, `_rotate_flush`). + +- **Recommended flow — flush to checkpoint, keep recording, stop at the end.** + `onoats flush` (SIGUSR1) is a *continuation* flush (`_flush_continuation` → + `_rotate_flush(reason, continue_session=True)`, `dual.py:424-431`): it rotates + the current `.active/` buffer into `pending/` **and opens a fresh `.active/` + session in the same process** — capture never stops. So the orphan-recovery UX + is: Flush now (salvage everything so far as a clean segment, recording + continues), then Stop at the end of the call (graceful SIGTERM → final rotate → + exit). Nothing is lost; the live call is uninterrupted. + - *Ownership does NOT transfer on flush.* The orphan stays the orphan + (`.running(ours:false)`); flush opens a new **file**, not a new **process** or + a GUI-owned session. The only way to a GUI-owned session is stop → (wait for + exit) → start. + +### Issue (anticipated): external stop→start must not be chained immediately + +- **Problem**: `onoats stop` returns on **signal delivery, not exit**; the + graceful drain (STT drain → terminal flush → rotate → `_remove_pid_file`) takes + seconds. A `start` issued during that window makes the new recorder's + `_write_pid_file` **overwrite** the orphan's still-live pid (warn-and-overwrite, + `runtime.py:1029-1050`); the draining orphan then calls `_remove_pid_file`, + which **unlinks unconditionally** (`runtime.py:1057-1065`) — deleting the *new* + recorder's pid file. Net result: the new session runs with no pid file (invisible + to `onoats status`/`stop`/`flush` — a fresh uncontrollable orphan), plus two + processes capture the same mic/system audio concurrently during the overlap. +- **Why the menu is safe today**: the GUI cannot trigger this — `Start` is + rendered only in `.stopped`/`.failed` (`OnoatsMenuBarApp.swift:69-78`), and + `refresh()` keeps an alive handle-less orphan in `.running(ours:false)` until it + observes `processAlive` → false (`RecorderModel.swift:243-258`). So `Start` does + not exist during the drain; the state machine gates it behind real process exit. + The "no early flip" invariant (recorder shares the pid-owner's process, exits + only after teardown) is what keeps `kill(0)` true through the whole drain. +- **Constraint for Phase 2 / future work**: never chain `stop()` then `start()` + back-to-back (e.g. a "Restart" button) without first waiting for the + `.running → .stopped` transition (`processAlive` → false). A `flock`-style hard + single-instance lock on the pid file (refuse the second start until the first is + fully gone) is the durable fix and would make even the racy CLI sequence + (`onoats stop && onoats bot`) safe. **Out of scope for Phases 1–2** — separate, + start-path-touching change; recorded here so it is not lost. +- **Files affected (future)**: `src/onoats/runtime.py` (`_write_pid_file` / + `_remove_pid_file`), `native/onoats-menubar/Sources/RecorderModel.swift`. + ## Final Results [Fill when complete] From 5fb4e0f4c059b6e42ba6c7113597a7111abdbba1 Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Fri, 19 Jun 2026 19:42:03 -0700 Subject: [PATCH 05/18] =?UTF-8?q?fix(runtime):=20close=20stop=E2=86=92imme?= =?UTF-8?q?diate-start=20pid-file=20race?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- src/onoats/__main__.py | 5 +- src/onoats/cli.py | 4 +- src/onoats/dual.py | 5 +- src/onoats/runtime.py | 57 ++++++++++++++++++-- tests/test_socket_supervisor.py | 4 +- tests/test_status_file.py | 94 ++++++++++++++++++++++++++++++++- 6 files changed, 157 insertions(+), 12 deletions(-) diff --git a/src/onoats/__main__.py b/src/onoats/__main__.py index b00f00c..803a5cb 100644 --- a/src/onoats/__main__.py +++ b/src/onoats/__main__.py @@ -57,6 +57,7 @@ from onoats.runtime import ( # noqa: E402 BOT_NAME, PIPELINE_SAMPLE_RATE, + RecorderAlreadyRunningError, SHUTDOWN_CANCEL_TIMEOUT_SEC, SttPreflightError, _create_stt_service, @@ -385,7 +386,7 @@ async def _shutdown_watcher() -> None: finally: await _on_shutdown() _restore_terminal(_old_terminal_settings) - _remove_pid_file(pid_path) + _remove_pid_file(pid_path, owner_pid=os.getpid()) # --------------------------------------------------------------------------- @@ -434,7 +435,7 @@ def main(argv: list[str] | None = None) -> int: asyncio.run( run_onoats(interactive=args.interactive, locked_category=args.category) ) - except SttPreflightError as exc: + except (SttPreflightError, RecorderAlreadyRunningError) as exc: print(f"\n{exc}\n", file=sys.stderr) return 1 return 0 diff --git a/src/onoats/cli.py b/src/onoats/cli.py index e0d1ba3..c72e274 100644 --- a/src/onoats/cli.py +++ b/src/onoats/cli.py @@ -487,12 +487,12 @@ def _run_socket_supervisor(rest: list[str]) -> int: from loguru import logger - from onoats.runtime import SttPreflightError + from onoats.runtime import RecorderAlreadyRunningError, SttPreflightError from onoats.transports import SocketHandshakeError try: return _asyncio.run(_supervise_socket_session(rest)) - except SttPreflightError as exc: + except (SttPreflightError, RecorderAlreadyRunningError) as exc: # Mirror dual.main: actionable hint, not a traceback. print(f"\n{exc}\n", file=sys.stderr) return 1 diff --git a/src/onoats/dual.py b/src/onoats/dual.py index d07fd19..04f88a2 100644 --- a/src/onoats/dual.py +++ b/src/onoats/dual.py @@ -42,6 +42,7 @@ from onoats.runtime import ( # noqa: E402 BOT_NAME, PIPELINE_SAMPLE_RATE, + RecorderAlreadyRunningError, SHUTDOWN_CANCEL_TIMEOUT_SEC, SttPreflightError, stop_pipeline_for_shutdown, @@ -99,7 +100,7 @@ def _finalize_shutdown_status( else: _write_status_stopped(data_dir, exit_reason="graceful") _restore_terminal(old_terminal_settings) - _remove_pid_file(pid_path) + _remove_pid_file(pid_path, owner_pid=os.getpid()) async def _shutdown_stt_service(stt_service, label: str) -> None: @@ -758,7 +759,7 @@ def main(argv: list[str] | None = None) -> int: live_terminal=args.live_terminal, locked_category=args.category ) ) - except SttPreflightError as exc: + except (SttPreflightError, RecorderAlreadyRunningError) as exc: print(f"\n{exc}\n", file=sys.stderr) return 1 return rc diff --git a/src/onoats/runtime.py b/src/onoats/runtime.py index 82a6a6b..776919e 100644 --- a/src/onoats/runtime.py +++ b/src/onoats/runtime.py @@ -56,6 +56,21 @@ class SttPreflightError(RuntimeError): """ +class RecorderAlreadyRunningError(RuntimeError): + """Raised at startup when an identity-verified live recorder already owns the + pid file. + + Caught at the same CLI entrypoints as ``SttPreflightError`` so the user sees + an actionable hint, not a traceback. The existing recorder's pid file is left + intact — we refuse BEFORE overwriting it — which closes the + stop-then-immediate-start race: a second start can no longer clobber a + draining recorder's pid file (and the drainer can no longer later unlink the + second start's file). The flagged recorder is verified via the same identity + gate as ``onoats stop``/``flush`` (marker + cmdline fingerprint + liveness), + so a stale/recycled/foreign pid never blocks a legitimate start. + """ + + PIPELINE_SAMPLE_RATE = 16000 # Silero VAD requires 8kHz or 16kHz; 16kHz is standard @@ -1021,11 +1036,30 @@ def _own_ps_cmdline() -> str: def _write_pid_file(data_dir: Path) -> Path: - """Write the current process PID, identity marker, and cmdline fingerprint.""" + """Write the current process PID, identity marker, and cmdline fingerprint. + + Single-instance guard: refuses to start over a still-live, identity-verified + recorder (raising ``RecorderAlreadyRunningError``). The check reuses the same + identity gate as ``onoats stop``/``flush`` (``resolve_flush_target``: marker + + cmdline fingerprint + liveness), so a stale, recycled, or foreign pid does NOT + block a legitimate start — only a genuine live recorder does. This closes the + stop-then-immediate-start race: without it, a second start would overwrite a + draining recorder's pid file, and the drainer would later unlink THIS start's + file (see also the ownership check in ``_remove_pid_file``). + """ active_dir = data_dir / ".active" active_dir.mkdir(parents=True, exist_ok=True) pid_path = active_dir / PID_FILENAME + from onoats._vendor.pid import resolve_flush_target + + verified = resolve_flush_target(pid_path) + if verified.pid is not None and verified.pid != os.getpid(): + raise RecorderAlreadyRunningError( + f"An onoats recorder is already running (pid {verified.pid}). " + "Stop it first with `onoats stop`, then retry." + ) + existing = _read_pid_file(pid_path) if existing is not None: try: @@ -1054,8 +1088,25 @@ def _write_pid_file(data_dir: Path) -> Path: return pid_path -def _remove_pid_file(pid_path: Path) -> None: - """Remove the PID file on shutdown.""" +def _remove_pid_file(pid_path: Path, *, owner_pid: int | None = None) -> None: + """Remove the PID file on shutdown. + + When ``owner_pid`` is given, only unlink if the file STILL records that pid. + A recorder tearing down must never delete a pid file a NEWER recorder has + since overwritten with its own pid (the stop-then-immediate-start race) — that + would leave the new session running with no pid file, invisible to + ``status``/``stop``/``flush``. A file we can no longer read as a valid record + (``None`` — already gone, or foreign marker) falls through to the unlink, + preserving the prior best-effort cleanup for the non-race cases. + """ + if owner_pid is not None: + current = _read_pid_file(pid_path) + if current is not None and current != owner_pid: + logger.warning( + f"PID file {pid_path} now records pid {current}, not ours " + f"({owner_pid}) — a newer recorder owns it; not removing." + ) + return try: pid_path.unlink() logger.debug(f"PID file removed: {pid_path}") diff --git a/tests/test_socket_supervisor.py b/tests/test_socket_supervisor.py index 21e829a..d483cf7 100644 --- a/tests/test_socket_supervisor.py +++ b/tests/test_socket_supervisor.py @@ -2017,11 +2017,11 @@ def test_shutdown_tail_writes_status_stopped_before_pid_unlink( observed: dict = {} real_remove = dual._remove_pid_file - def _spy_remove(p): + def _spy_remove(p, **kwargs): rec = status_file.read_status(data_dir) observed["running_at_unlink"] = None if rec is None else rec.running observed["pid_file_present_at_unlink"] = p.exists() - return real_remove(p) + return real_remove(p, **kwargs) monkeypatch.setattr(dual, "_remove_pid_file", _spy_remove) diff --git a/tests/test_status_file.py b/tests/test_status_file.py index 6958a3a..d6465e4 100644 --- a/tests/test_status_file.py +++ b/tests/test_status_file.py @@ -241,7 +241,9 @@ def test_dual_wires_producers_at_start_rotation_stop(): # Write ordering: status-stopped MUST precede pid removal so the pid backstop # and the status file never disagree about a live recorder. stop_idx = src.index("_write_status_stopped(") - pid_rm_idx = src.index("_remove_pid_file(pid_path)") + # Match the call prefix only — the pid removal is ownership-checked + # (`_remove_pid_file(pid_path, owner_pid=...)`), so don't pin the closing paren. + pid_rm_idx = src.index("_remove_pid_file(pid_path") assert stop_idx < pid_rm_idx, "status-stopped must be written before pid removal" # And the start producer runs AFTER the pid file is written (pid first). @@ -501,3 +503,93 @@ def test_write_prestart_waiting_is_fresh_running_with_warning(tmp_path: Path): write_running(tmp_path, pid=2, audio_source="socket", stt_label="mlx") st = read_status(tmp_path) assert st.warning is None and st.pid == 2 + + +# --------------------------------------------------------------------------- +# (g) stop-then-immediate-start race: pid-file single-instance guard + +# ownership-checked removal. `onoats stop` returns on signal delivery, not exit, +# so a new `onoats bot` can launch while the old recorder is still draining. +# Without these guards the new start would overwrite the draining recorder's pid +# file, and the drainer would later unlink the NEW recorder's file (leaving it +# invisible to status/stop/flush). See runtime._write_pid_file / _remove_pid_file. +# --------------------------------------------------------------------------- + +import shutil # noqa: E402 +import subprocess # noqa: E402 + +from onoats._vendor.pid import PID_FILENAME # noqa: E402 +from onoats.runtime import ( # noqa: E402 + RecorderAlreadyRunningError, + _remove_pid_file, + _write_pid_file, +) + + +def _seed_pid_file(data_dir: Path, pid: int, *, cmdline: str = "onoats bot") -> Path: + pid_path = data_dir / ".active" / PID_FILENAME + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text(f"{pid}\nonoats-bot\n{cmdline}\n0.0\n", encoding="utf-8") + return pid_path + + +def test_write_pid_file_refuses_verified_live_recorder(tmp_path, monkeypatch): + """Start guard: a second start over an identity-verified LIVE recorder is + refused (RecorderAlreadyRunningError), and the existing pid file is left + intact — never overwritten.""" + if not shutil.which("sleep"): + pytest.skip("requires a real live process for the liveness check") + # A real, unrelated live process stands in for the still-draining recorder. + proc = subprocess.Popen(["sleep", "30"]) + try: + pid_path = _seed_pid_file(tmp_path, proc.pid) + # Make the identity readback match the stored fingerprint → verified live. + monkeypatch.setattr( + "onoats._vendor.pid._live_ps_cmdline", lambda pid: "onoats bot" + ) + with pytest.raises(RecorderAlreadyRunningError): + _write_pid_file(tmp_path) + # The draining recorder's pid file MUST survive the refused start. + assert pid_path.read_text(encoding="utf-8").startswith(f"{proc.pid}\n") + finally: + proc.terminate() + proc.wait(timeout=5) + + +def test_write_pid_file_overwrites_stale_dead_recorder(tmp_path): + """A stale pid file for a DEAD recorder does not block a legitimate start — + it is overwritten with the current process's pid (no false single-instance + refusal on a crashed predecessor).""" + import os as _os + + if not shutil.which("sleep"): + pytest.skip("requires spawning a process to obtain a real dead pid") + # Spawn then reap a process so its pid is genuinely dead (kill(0) raises + # ProcessLookupError) — a realistic crashed-predecessor pid, no mocking. + proc = subprocess.Popen(["sleep", "30"]) + proc.terminate() + proc.wait(timeout=5) + _seed_pid_file(tmp_path, proc.pid) + + pid_path = _write_pid_file(tmp_path) + # Overwritten with OUR pid — start proceeds. + assert pid_path.read_text(encoding="utf-8").startswith(f"{_os.getpid()}\n") + + +def test_remove_pid_file_skips_when_owned_by_newer_recorder(tmp_path): + """Ownership-checked removal: a draining recorder must NOT delete a pid file a + newer recorder has since overwritten with its own pid.""" + pid_path = _seed_pid_file(tmp_path, 55555) # the NEW recorder owns it now + # The OLD (draining) recorder, pid 12345, tears down and tries to remove. + _remove_pid_file(pid_path, owner_pid=12345) + assert pid_path.exists(), "new recorder's pid file must survive the old drainer" + # The rightful owner can remove it. + _remove_pid_file(pid_path, owner_pid=55555) + assert not pid_path.exists() + + +def test_remove_pid_file_unconditional_without_owner(tmp_path): + """Back-compat: with no owner_pid the removal is unconditional (the prior + best-effort behaviour for callers that don't pass ownership).""" + pid_path = _seed_pid_file(tmp_path, 999) + _remove_pid_file(pid_path) + assert not pid_path.exists() From ff12681ec025fde9c6e6f620b09f29fe0ffdae54 Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Fri, 19 Jun 2026 19:42:03 -0700 Subject: [PATCH 06/18] =?UTF-8?q?feat(menubar):=20Phase=202=20=E2=80=94=20?= =?UTF-8?q?Stop=20works=20for=20orphaned/external=20sessions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Sources/OnoatsMenuBarApp.swift | 27 ++++++-- .../Sources/RecorderModel.swift | 63 +++++++++++++++++++ 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/native/onoats-menubar/Sources/OnoatsMenuBarApp.swift b/native/onoats-menubar/Sources/OnoatsMenuBarApp.swift index 5eab2d0..676e382 100644 --- a/native/onoats-menubar/Sources/OnoatsMenuBarApp.swift +++ b/native/onoats-menubar/Sources/OnoatsMenuBarApp.swift @@ -23,8 +23,12 @@ struct OnoatsMenuBarApp: App { // A live capture warning (all-zero input — likely a denied grant or a // muted mic) changes the icon so the anomaly is visible without // opening the menu. Warning, not failure: the session keeps running. - case .running: return model.warning == nil - ? "waveform.circle.fill" : "waveform.badge.exclamationmark" + case .running: + // An external stop in flight reuses the draining glyph so the icon + // tracks the menu's "stopping…" affordance. + if model.stopRequested { return "ellipsis.circle" } + return model.warning == nil + ? "waveform.circle.fill" : "waveform.badge.exclamationmark" case .starting, .stopping: return "ellipsis.circle" case .failed: return "exclamationmark.triangle.fill" case .stopped: return "waveform.circle" @@ -68,8 +72,17 @@ struct MenuContent: View { switch model.state { case .running(let ours): - Button(ours ? "Stop" : "Stop (external session)") { model.stop() } - .disabled(!ours) + // Stop is enabled for ALL identity-verified live sessions (parity + // with Flush, which already reaches external sessions). Route on the + // `ours` value the enum already carries — owned sessions keep the + // in-handle `p.terminate()`; verified external/orphaned sessions go + // through `onoats stop` (the CLI's identity-checked SIGTERM). The + // cosmetic `stopRequested` flag (set synchronously inside + // `stopExternal`) disables the button for the external drain window. + Button("Stop") { + if ours { model.stop() } else { model.stopExternal() } + } + .disabled(model.stopRequested) Button("Flush") { model.flush() } case .starting, .stopping: Button("Start") {}.disabled(true) @@ -158,6 +171,12 @@ struct MenuContent: View { case .failed: return "Onoats: stopped" case .running(let ours): + // External stop in flight: show "stopping…" (not "Stopped") for the + // whole drain window — `refresh()` flips to `.stopped` only once the + // supervisor actually exits, so this never fakes a terminal state. + if model.stopRequested { + return "Onoats: stopping (draining)…" + } var line = "Onoats: recording" if let since = model.startTime { line += " since \(since.formatted(date: .omitted, time: .shortened))" diff --git a/native/onoats-menubar/Sources/RecorderModel.swift b/native/onoats-menubar/Sources/RecorderModel.swift index 5878baa..0c193c1 100644 --- a/native/onoats-menubar/Sources/RecorderModel.swift +++ b/native/onoats-menubar/Sources/RecorderModel.swift @@ -68,6 +68,18 @@ final class RecorderModel: ObservableObject { /// next Flush or Start. @Published var flushNote: String? + /// Cosmetic "external stop in flight" flag for a handle-less session. Set + /// synchronously by `stopExternal()` BEFORE the `onoats stop` subprocess + /// spawn, and the direct argument to the Stop button's `.disabled(...)` — so + /// a verified external/orphaned session can't be double-stopped while it + /// drains. Cleared level-triggered in `refresh()` once the supervisor is + /// actually gone (`!alive`). Invariant: ONLY `stopExternal()` sets it (plus a + /// spawn-failure reset so a failed launch doesn't wedge the button), ONLY + /// `refresh()` clears it on `!alive` — no other writer. We deliberately do + /// NOT use the `.stopping` enum case for external stops: `.stopping` is + /// cleared by `handleExit`, which never fires without a `Process` handle. + @Published var stopRequested = false + /// Valid `[stt].service` values — mirror of runtime.py /// `VALID_STT_SERVICES` (parity-checked by /// tests/test_native_contract_parity.py). @@ -250,6 +262,12 @@ final class RecorderModel: ObservableObject { // can't clobber an imminent .failed with .stopped. return } + // External-stop convergence (handle-less session): clear the cosmetic + // stopRequested flag once the supervisor is actually GONE — process exit + // (kill(0) false), NOT pid-file removal. Placed AFTER the owned-proc + // early-return above so an owned session's `.stopping` drain never + // touches it. Sole clearing site (see the `stopRequested` invariant). + if !alive { stopRequested = false } if alive { state = .running(ours: false) } else if case .failed = state { @@ -340,6 +358,51 @@ final class RecorderModel: ObservableObject { } } + /// Runs `onoats stop` for a session this app does NOT own — one started from + /// a terminal, or (the motivating case) a GUI-started session orphaned by an + /// app crash and seen as `running(ours: false)` on relaunch. Mirrors + /// `flush()`: the CLI does the identity-checked pid signalling (marker + + /// fingerprint + liveness → SIGTERM), so stopping a session we have no + /// `Process` handle for is safe and never signals a recycled pid. + /// + /// We do NOT block on exit and do NOT set the `.stopping` enum case (no + /// `handleExit` fires without a handle); convergence to `.stopped` is driven + /// by `refresh()` observing the supervisor exit (`processAlive` → false). The + /// cosmetic `stopRequested` flag is set HERE, synchronously, BEFORE the spawn + /// so a second `onoats stop` subprocess can't be launched mid-drain (the + /// button is `.disabled(stopRequested)`). + func stopExternal() { + stopRequested = true + flushNote = nil + let p = Process() + p.executableURL = URL(fileURLWithPath: cliPath) + p.arguments = ["stop"] + if let log = openLog() { + p.standardOutput = log + p.standardError = log + } + p.terminationHandler = { [weak self] proc in + Task { @MainActor in + if proc.terminationStatus != 0 { + // Non-zero rc means the CLI refused to signal (stale / identity + // mismatch): the supervisor is gone or unverifiable, so + // `refresh()` will observe `!alive` and clear stopRequested. + self?.flushNote = + "Stop failed (rc \(proc.terminationStatus)) — see onoats-bot.log" + } + } + } + do { + try p.run() + } catch { + // The subprocess never launched, so nothing is in flight — re-enable + // the button (the one writer other than refresh(), guarded to the + // spawn-failure path) so a missing/!executable CLI can be retried. + stopRequested = false + flushNote = "Stop spawn failed: \(error.localizedDescription)" + } + } + // --------------------------------------------------------------- settings // All settings write config.toml — the same file the CLI reads — so GUI // and terminal sessions share one source of truth. A change while a From e6fbbe45c9b60d980359274586092c3b2d917266 Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Fri, 19 Jun 2026 19:42:03 -0700 Subject: [PATCH 07/18] docs: record Phase 2 + pid-race fix (Codex review follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 15 ++++++ CHANGELOG.md | 22 ++++++-- .../20260619-bug-stoppable-orphan-session.md | 50 ++++++++++++++++++- 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 1fbcc12..d051b3f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,6 +134,21 @@ Load-bearing invariants (pinned by `tests/test_cli.py`): - **`onoats stop --help` resolves without booting a service** (local argparse + lazy resolver import), like `flush`. +Single-instance + pid-file ownership (`runtime.py`, pinned by `tests/test_status_file.py`): + +- **Start refuses to launch over a live recorder.** `_write_pid_file` runs the + same identity gate (`resolve_flush_target`) and raises + `RecorderAlreadyRunningError` (clean rc=1 at all CLI boundaries) when an + identity-verified recorder is already alive — a stale/recycled/foreign pid does + NOT block a legitimate start. This is the single-instance lock; there is no + separate `flock` file. +- **Pid removal is ownership-checked.** `_remove_pid_file(pid_path, owner_pid=…)` + unlinks only when the file still records that pid. Because `stop` returns on + signal delivery (not exit), a `stop`-then-immediate-`bot` could otherwise let a + draining recorder delete a NEWER recorder's pid file. Recorder teardown passes + `owner_pid=os.getpid()`; the GUI's menu gating (Start only in `.stopped`) + already prevents this from the app, so the guard protects the CLI/scripted path. + ## Reviewing a subprocess / process-boundary change When a change spawns a child process (`create_subprocess_*` / `Popen` / `exec`) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4660aed..5482720 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,9 +22,25 @@ Annotated tags exist from `v0.9.0` forward. marker + cmdline-fingerprint + liveness), so it only ever signals the verified recorder and never a recycled pid — which matters more than for flush because SIGTERM kills by default. Returns on signal delivery, not confirmed exit; like - flush, `onoats stop --help` resolves without booting a service. (Phase 1 of the - stoppable-orphan-session fix: the menu-bar Stop rewiring that routes an - orphaned GUI session through this seam is a follow-up.) + flush, `onoats stop --help` resolves without booting a service. +- Menu-bar **Stop now works for orphaned/external sessions**: a GUI-started + recorder orphaned by an app crash (seen as `running(ours: false)` on relaunch), + or any terminal-started session, can be stopped from the menu. Owned sessions + keep the in-handle `Process.terminate()`; verified external sessions route + through `onoats stop`. The menu shows "stopping (draining)…" for the whole + drain and flips to Stopped only when the supervisor actually exits (polled), + never faking a terminal state. (Completes the stoppable-orphan-session fix.) + +### Fixed +- **Stop→immediate-start pid-file race.** `onoats stop` returns on signal + delivery, not exit, so a new `onoats bot` launched during the old recorder's + drain could overwrite the draining recorder's pid file — and the drainer would + then unlink the *new* recorder's file, leaving it invisible to + `status`/`stop`/`flush`. Two guards close this: (1) start refuses to launch + over an identity-verified live recorder (`RecorderAlreadyRunningError`, same + identity gate as stop/flush — a stale/recycled/foreign pid never blocks a + legitimate start); (2) pid-file removal is ownership-checked, so a recorder + never deletes a pid file a newer recorder has since overwritten. ## [1.1.0] - 2026-06-12 diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index bd26c95..6f5189c 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -150,11 +150,59 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Progress - [x] Phase 1: `onoats stop` CLI subcommand (Python) -- [ ] Phase 2: Menu-bar Stop rewiring (Swift) +- [x] Phase 2: Menu-bar Stop rewiring (Swift) — impl complete + compiles clean (`make build/Onoats`); **manual orphan-then-stop smoke matrix still pending on the user's machine** - [ ] Phase 3: Zero-sample watchdog escalation (separable) ## Findings +### Phase 2 + pid-race hardening (2026-06-19) — Codex adversarial-review follow-up + +A Codex adversarial review of the Phase-1 branch returned **NO-SHIP** with two +[high] findings; both are now addressed (the user authorised pulling Phase 2 and +the deferred pid-race fix forward in response). + +- **[high] Orphaned GUI sessions still unstoppable from the menu bar → Phase 2 + implemented.** `RecorderModel.stopExternal()` clones `flush()` to exec + `onoats stop` for handle-less sessions; the Stop button now 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"`. Convergence to `.stopped` is driven by `refresh()` + polling `processAlive` → false; a cosmetic `stopRequested` flag (set + synchronously in `stopExternal` before the spawn, cleared only in `refresh()` + on `!alive`) gates `.disabled(...)` and drives the "stopping (draining)…" + affordance — `.stopping` enum is NOT used (no `handleExit` without a handle). + - *Deviation from the plan's strict invariant, named:* `stopExternal()` also + clears `stopRequested` in its **spawn-failure catch** (the subprocess never + launched, so nothing is in flight) — otherwise a missing/non-executable CLI + would wedge the button disabled forever (the supervisor stays alive, so + `refresh()` never sees `!alive`). This is the only writer besides `refresh()`, + guarded to the error path. + - **Verified:** `make build/Onoats` compiles clean (full module, real + `swiftc`). **NOT verified by me:** the manual orphan-then-stop smoke matrix + (Phase 2 (a)–(e)) — requires running/crashing/relaunching the signed app with + real TCC + audio on the user's machine. Smoke precondition still applies: + install/refresh the console script so the *installed* `onoats` (resolved by + `cliPath`) has the `stop` subcommand — verify with the bare installed binary + `onoats stop --help`, not `uv run`. + +- **[high] `onoats stop && onoats bot` could delete the new recorder's pid file + → fixed in Python.** Two guards in `runtime.py`: (1) `_write_pid_file` refuses + to start over an identity-verified live recorder (`RecorderAlreadyRunningError`, + reusing `resolve_flush_target` — a stale/recycled/foreign pid never blocks a + legitimate start), mapped to a clean non-zero exit at all three CLI boundaries + (`cli.py`, `dual.py`, `__main__.py`); (2) `_remove_pid_file(pid_path, owner_pid=…)` + unlinks only when the file still records our pid, so a draining recorder never + deletes a pid file a newer recorder has overwritten. Recorder call sites + (`dual._finalize_shutdown_status`, `__main__`) pass `owner_pid=os.getpid()`. + Regression tests in `test_status_file.py` (refuse-live, overwrite-stale-dead, + ownership-skip, back-compat-unconditional). This supersedes the "out of scope + for Phases 1–2" note under *Issues & Solutions* — the guard landed here rather + than as a separate `flock` change; the resolver-identity guard is equivalent + protection without a new lock file. + +- **Verification:** full `uv run pytest` suite **283 passed**; `ruff format` + + `ruff check` clean; `make build/Onoats` compiles. Codex re-review not re-run. + ### Phase 1 (2026-06-19) — `onoats stop` CLI subcommand shipped - **Implementation.** `_cmd_stop` (`src/onoats/cli.py`) is a near-clone of From 5227f7d5fd45479b3d8b5515fd670a443609530e Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Sat, 20 Jun 2026 04:49:16 -0700 Subject: [PATCH 08/18] fix(runtime): refuse start over an indeterminate-but-live recorder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/onoats/runtime.py | 51 ++++++++++++++++++++++++++++++++------- tests/test_status_file.py | 42 ++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 9 deletions(-) diff --git a/src/onoats/runtime.py b/src/onoats/runtime.py index 776919e..df3bdcf 100644 --- a/src/onoats/runtime.py +++ b/src/onoats/runtime.py @@ -1035,23 +1035,41 @@ def _own_ps_cmdline() -> str: return "" +def _pid_alive(pid: int) -> bool: + """True if ``pid`` exists. ``ProcessLookupError`` is the only positive proof + of death; any other error (``EPERM`` — owned by another user — or an odd + ``OSError``) is treated as alive, so a liveness guard fails *safe*.""" + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except OSError: + return True + return True + + def _write_pid_file(data_dir: Path) -> Path: """Write the current process PID, identity marker, and cmdline fingerprint. - Single-instance guard: refuses to start over a still-live, identity-verified - recorder (raising ``RecorderAlreadyRunningError``). The check reuses the same - identity gate as ``onoats stop``/``flush`` (``resolve_flush_target``: marker + - cmdline fingerprint + liveness), so a stale, recycled, or foreign pid does NOT - block a legitimate start — only a genuine live recorder does. This closes the - stop-then-immediate-start race: without it, a second start would overwrite a - draining recorder's pid file, and the drainer would later unlink THIS start's - file (see also the ownership check in ``_remove_pid_file``). + Single-instance guard: refuses to start over a still-live recorder (raising + ``RecorderAlreadyRunningError``). The check reuses the same identity gate as + ``onoats stop``/``flush`` (``resolve_flush_target``: marker + cmdline + fingerprint + liveness), so a *positively stale* pid — dead, or recycled to a + foreign process (identity mismatch) — does NOT block a legitimate start. But a + marker-valid pid file naming a LIVE process whose identity cannot be verified + (``ps`` probe failed, or a legacy fingerprint-less file) is treated the same + way ``flush``/``stop`` treat that indeterminate state — refuse, never + overwrite — so a transient probe failure can't spawn a second recorder over a + live one. This closes the stop-then-immediate-start race (and its degraded + variants): without it, a second start would overwrite a draining recorder's + pid file, and the drainer would later unlink THIS start's file (see also the + ownership check in ``_remove_pid_file``). """ active_dir = data_dir / ".active" active_dir.mkdir(parents=True, exist_ok=True) pid_path = active_dir / PID_FILENAME - from onoats._vendor.pid import resolve_flush_target + from onoats._vendor.pid import read_pid_record, resolve_flush_target verified = resolve_flush_target(pid_path) if verified.pid is not None and verified.pid != os.getpid(): @@ -1059,6 +1077,21 @@ def _write_pid_file(data_dir: Path) -> Path: f"An onoats recorder is already running (pid {verified.pid}). " "Stop it first with `onoats stop`, then retry." ) + # Indeterminate-but-live: the resolver refused (pid is None) WITHOUT declaring + # the file stale — i.e. it is neither a verified recorder nor a proven-dead / + # recycled-foreign pid, but a marker-valid file whose process is still alive + # and merely unverifiable (ps probe failed / legacy fingerprint-less). Refuse, + # exactly as flush/stop do for the same state; only ``stale=True`` is safe to + # clobber. + if verified.pid is None and not verified.stale: + rec = read_pid_record(pid_path) + if rec is not None and rec.pid != os.getpid() and _pid_alive(rec.pid): + raise RecorderAlreadyRunningError( + f"A recorder pid file names a live process (pid {rec.pid}) whose " + "identity could not be verified (ps probe failed / legacy pid " + "file) — refusing to start over a possibly-live recorder. Stop it " + "(`onoats stop`) or remove the stale pid file, then retry." + ) existing = _read_pid_file(pid_path) if existing is not None: diff --git a/tests/test_status_file.py b/tests/test_status_file.py index d6465e4..c4d538b 100644 --- a/tests/test_status_file.py +++ b/tests/test_status_file.py @@ -593,3 +593,45 @@ def test_remove_pid_file_unconditional_without_owner(tmp_path): pid_path = _seed_pid_file(tmp_path, 999) _remove_pid_file(pid_path) assert not pid_path.exists() + + +def test_write_pid_file_refuses_live_recorder_with_indeterminate_probe( + tmp_path, monkeypatch +): + """[high] regression (Codex re-review): a marker-valid pid file naming a LIVE + process whose identity can't be verified (ps probe returns None) must REFUSE + startup, not overwrite — the same indeterminate state flush/stop refuse to act + on. Without the guard a transient `ps` failure spawns a second recorder over a + live one.""" + if not shutil.which("sleep"): + pytest.skip("requires a real live process for the liveness check") + proc = subprocess.Popen(["sleep", "30"]) + try: + pid_path = _seed_pid_file(tmp_path, proc.pid) + # kill(0) succeeds (process alive) but the identity readback fails. + monkeypatch.setattr("onoats._vendor.pid._live_ps_cmdline", lambda pid: None) + with pytest.raises(RecorderAlreadyRunningError): + _write_pid_file(tmp_path) + # The live recorder's pid file MUST survive — never overwritten. + assert pid_path.read_text(encoding="utf-8").startswith(f"{proc.pid}\n") + finally: + proc.terminate() + proc.wait(timeout=5) + + +def test_write_pid_file_refuses_live_legacy_fingerprintless_pid(tmp_path): + """A legacy (2-line, fingerprint-less) pid file naming a LIVE process is + unverifiable → refuse startup rather than overwrite a possibly-live recorder.""" + if not shutil.which("sleep"): + pytest.skip("requires a real live process for the liveness check") + proc = subprocess.Popen(["sleep", "30"]) + try: + pid_path = tmp_path / ".active" / PID_FILENAME + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text(f"{proc.pid}\nonoats-bot\n", encoding="utf-8") # no line 3 + with pytest.raises(RecorderAlreadyRunningError): + _write_pid_file(tmp_path) + assert pid_path.exists() + finally: + proc.terminate() + proc.wait(timeout=5) From ab5e763bf8ea8c5768622ea387bdfd8989d74ef1 Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Sat, 20 Jun 2026 04:49:17 -0700 Subject: [PATCH 09/18] fix(menubar): re-enable Stop when external `onoats stop` exits non-zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../Sources/RecorderModel.swift | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/native/onoats-menubar/Sources/RecorderModel.swift b/native/onoats-menubar/Sources/RecorderModel.swift index 0c193c1..02d5563 100644 --- a/native/onoats-menubar/Sources/RecorderModel.swift +++ b/native/onoats-menubar/Sources/RecorderModel.swift @@ -72,12 +72,13 @@ final class RecorderModel: ObservableObject { /// synchronously by `stopExternal()` BEFORE the `onoats stop` subprocess /// spawn, and the direct argument to the Stop button's `.disabled(...)` — so /// a verified external/orphaned session can't be double-stopped while it - /// drains. Cleared level-triggered in `refresh()` once the supervisor is - /// actually gone (`!alive`). Invariant: ONLY `stopExternal()` sets it (plus a - /// spawn-failure reset so a failed launch doesn't wedge the button), ONLY - /// `refresh()` clears it on `!alive` — no other writer. We deliberately do - /// NOT use the `.stopping` enum case for external stops: `.stopping` is - /// cleared by `handleExit`, which never fires without a `Process` handle. + /// drains. Writers: `stopExternal()` sets it on entry and clears it on either + /// error path (spawn threw, OR the CLI exited non-zero = SIGTERM not + /// delivered) so a failed/stale-CLI stop can't wedge the sole Stop button; + /// `refresh()` clears it level-triggered once the supervisor is actually gone + /// (`!alive`) on the SUCCESS path. No other writer. We deliberately do NOT + /// use the `.stopping` enum case for external stops: `.stopping` is cleared + /// by `handleExit`, which never fires without a `Process` handle. @Published var stopRequested = false /// Valid `[stt].service` values — mirror of runtime.py @@ -384,9 +385,15 @@ final class RecorderModel: ObservableObject { p.terminationHandler = { [weak self] proc in Task { @MainActor in if proc.terminationStatus != 0 { - // Non-zero rc means the CLI refused to signal (stale / identity - // mismatch): the supervisor is gone or unverifiable, so - // `refresh()` will observe `!alive` and clear stopRequested. + // Non-zero rc means the CLI did NOT deliver SIGTERM — it + // refused (stale / identity mismatch), errored, or is a stale + // installed CLI lacking the `stop` subcommand (argparse rc 2). + // The supervisor may still be ALIVE, so re-enable the button: + // `refresh()` only clears stopRequested on `!alive`, and would + // otherwise leave the sole Stop control wedged until restart. + // A delivered SIGTERM (rc 0) leaves the flag set; refresh() + // clears it when the supervisor actually exits. + self?.stopRequested = false self?.flushNote = "Stop failed (rc \(proc.terminationStatus)) — see onoats-bot.log" } From 2a66d4aa652d436c89db01d2ded4bbf34c7ae39b Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Sat, 20 Jun 2026 04:49:17 -0700 Subject: [PATCH 10/18] docs: record Codex re-review round-2 fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 13 ++++--- .../20260619-bug-stoppable-orphan-session.md | 34 +++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5482720..eec3494 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,10 +37,15 @@ Annotated tags exist from `v0.9.0` forward. drain could overwrite the draining recorder's pid file — and the drainer would then unlink the *new* recorder's file, leaving it invisible to `status`/`stop`/`flush`. Two guards close this: (1) start refuses to launch - over an identity-verified live recorder (`RecorderAlreadyRunningError`, same - identity gate as stop/flush — a stale/recycled/foreign pid never blocks a - legitimate start); (2) pid-file removal is ownership-checked, so a recorder - never deletes a pid file a newer recorder has since overwritten. + over a live recorder (`RecorderAlreadyRunningError`) — both the identity-verified + case AND the indeterminate-but-live case (marker-valid pid file, process alive, + but the `ps` identity probe failed or the file is legacy/fingerprint-less), + matching how stop/flush refuse that same state; only a positively stale (dead or + recycled-to-foreign) pid is overwritten; (2) pid-file removal is + ownership-checked, so a recorder never deletes a pid file a newer recorder has + since overwritten. The menu's external Stop also re-enables itself if the + `onoats stop` subprocess fails or exits non-zero (e.g. a stale installed CLI), + rather than wedging the only Stop control until app restart. ## [1.1.0] - 2026-06-12 diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index 6f5189c..d515e52 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -155,6 +155,40 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Findings +### Codex re-review round 2 (2026-06-20) — degraded-path fixes + +The re-run Codex adversarial review (after the round-1 fixes) returned a fresh +NO-SHIP with two degraded-path findings; both fixed. + +- **[high] Start could overwrite a live recorder when identity probing is + indeterminate** (`runtime.py`). The round-1 start guard only refused on a + *verified* live recorder; if the existing recorder was live but the `ps` probe + failed (`resolve_flush_target` → `pid=None, stale=False`) it fell through to + warn-and-overwrite — the same indeterminate state `flush`/`stop` refuse to act + on. Fixed: `_write_pid_file` now also raises `RecorderAlreadyRunningError` when + a marker-valid pid file names a **live** process (`_pid_alive`, fail-safe) that + the resolver could neither verify nor declare stale (ps-probe-fail / legacy + fingerprint-less). Only `stale=True` (dead or recycled-to-foreign) is + overwritten. Regression tests (`test_status_file.py`): + `…_refuses_live_recorder_with_indeterminate_probe` (kill(0) ok + `_live_ps_cmdline` + → None) and `…_refuses_live_legacy_fingerprintless_pid`. + +- **[medium] Failed external stop wedged the GUI in "stopping"** (`RecorderModel.swift`). + `stopExternal()` cleared `stopRequested` only on a spawn *throw*, not on a + non-zero CLI *exit* — so a stale installed CLI lacking `stop` (argparse rc 2), + or an identity refusal while the process stays alive, left the sole Stop button + disabled until app restart. Fixed: 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`. + - *Honest gap:* Codex asked for a menu-model regression with a fake non-zero + `stop`. There is **no Swift unit-test harness** in this repo (no + `Package.swift`/XCTest; the menu app is built with a bare `swiftc` line) — the + project's standing decision is "native Swift = manual smoke only." So this is + covered by **manual smoke**, not an automated test: point `cliPath` at a CLI + without `stop` (or an `onoats` stub returning rc≠0) on a live external session, + click Stop, confirm the button re-enables and the menu leaves "stopping…". + Code compiles (`make build/Onoats`). + ### Phase 2 + pid-race hardening (2026-06-19) — Codex adversarial-review follow-up A Codex adversarial review of the Phase-1 branch returned **NO-SHIP** with two From 85b9556e6944c365305a7e5a862109a8be159ebb Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Sat, 20 Jun 2026 07:29:32 -0700 Subject: [PATCH 11/18] docs: deep-review polish (cross-refs, README clarity, plan split) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- README.md | 5 +++-- docs/dev_plans/20260619-bug-stoppable-orphan-session.md | 3 ++- native/onoats-menubar/Sources/RecorderModel.swift | 4 ++++ src/onoats/cli.py | 4 ++++ src/onoats/dual.py | 6 ++++-- 5 files changed, 17 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 6e4a77a..65ec022 100644 --- a/README.md +++ b/README.md @@ -108,8 +108,9 @@ Other subcommands: ```bash onoats bot-single # legacy mic-only recorder onoats flush # tell the running recorder to rotate its buffer now -onoats stop # signal the running recorder to stop gracefully (drain + - # final flush); identity-checked like flush, so it only ever +onoats stop # stop the running recorder gracefully: SIGTERM → drain + + # final flush, then EXIT (unlike flush, which keeps + # recording). Identity-checked like flush, so it only ever # signals the verified recorder — never a recycled pid onoats devices # list audio input/output devices (PortAudio's view; under # the socket path it adds a note — the native capturer binds diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index d515e52..3d4b715 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -150,7 +150,8 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Progress - [x] Phase 1: `onoats stop` CLI subcommand (Python) -- [x] Phase 2: Menu-bar Stop rewiring (Swift) — impl complete + compiles clean (`make build/Onoats`); **manual orphan-then-stop smoke matrix still pending on the user's machine** +- [x] Phase 2 (code): Menu-bar Stop rewiring (Swift) — impl complete + compiles clean (`make build/Onoats`) +- [ ] Phase 2 (manual smoke): orphan-then-stop matrix on the user's machine — **still pending** (requires running/crashing/relaunching the signed app with real TCC + audio) - [ ] Phase 3: Zero-sample watchdog escalation (separable) ## Findings diff --git a/native/onoats-menubar/Sources/RecorderModel.swift b/native/onoats-menubar/Sources/RecorderModel.swift index 02d5563..6bde8e7 100644 --- a/native/onoats-menubar/Sources/RecorderModel.swift +++ b/native/onoats-menubar/Sources/RecorderModel.swift @@ -335,6 +335,10 @@ final class RecorderModel: ObservableObject { /// like everything else). Deliberately works for EXTERNAL sessions too: /// the CLI does its own identity-checked pid signalling (marker + /// fingerprint), so flushing a terminal-started session from here is safe. + /// + /// Near-clone: `stopExternal()` mirrors this (it execs `onoats stop` instead + /// of `flush`, plus the `stopRequested` flag). Keep the two in sync — e.g. a + /// future `--data-dir` argument must be added to both. func flush() { flushNote = nil let p = Process() diff --git a/src/onoats/cli.py b/src/onoats/cli.py index c72e274..5072106 100644 --- a/src/onoats/cli.py +++ b/src/onoats/cli.py @@ -1120,6 +1120,10 @@ def _cmd_flush(rest: list[str]) -> int: This is the seam an integrating consumer's ``flush`` pass-through execs. It resolves the pid from ``/.active/onoats.pid`` (marker ``onoats-bot``) under the forwarded ``ONOATS_DATA_DIR`` root. + + Near-clone: ``_cmd_stop`` mirrors this verbatim except for the signal + (SIGTERM vs SIGUSR1). Keep the two in sync — a new error branch here must be + copied there (parity is pinned by tests, not a shared helper). """ parser = argparse.ArgumentParser(prog="onoats flush") parser.add_argument( diff --git a/src/onoats/dual.py b/src/onoats/dual.py index 04f88a2..bb91a32 100644 --- a/src/onoats/dual.py +++ b/src/onoats/dual.py @@ -85,8 +85,10 @@ def _finalize_shutdown_status( Extracted module-level (rather than left inline in ``_run_shutdown``) so the write ordering is exercisable at runtime without booting the recorder stack — see ``test_socket_supervisor.py``. Terminal restore + pid removal live here - too so the two shutdown call sites (``_shutdown_watcher`` and the outer - ``finally`` block) stay idempotent regardless of ordering. + too; this runs exactly once per session (the ``_on_shutdown`` de-dup guard + via ``shutdown_started``/``shutdown_complete`` serialises the two call sites — + ``_shutdown_watcher`` and the outer ``finally`` block), so it does not need to + be internally idempotent. """ if ended_by_error: _write_status_stopped( From b272ba4685365cdbfe10a3366f02db577f0684ec Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Mon, 22 Jun 2026 08:11:50 -0700 Subject: [PATCH 12/18] docs(dev-plan): record on-device orphan-then-stop smoke PASS 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). --- .../20260619-bug-stoppable-orphan-session.md | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index 3d4b715..47bd078 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -151,7 +151,7 @@ Make any identity-verified live recorder session stoppable from the menu bar — - [x] Phase 1: `onoats stop` CLI subcommand (Python) - [x] Phase 2 (code): Menu-bar Stop rewiring (Swift) — impl complete + compiles clean (`make build/Onoats`) -- [ ] Phase 2 (manual smoke): orphan-then-stop matrix on the user's machine — **still pending** (requires running/crashing/relaunching the signed app with real TCC + audio) +- [x] Phase 2 (manual smoke): orphan-then-stop **headline case verified live** on the user's machine (2026-06-22) — see Findings. Remaining matrix items (owned Stop, double-click guard, Flush mid-session, CLI-failure re-enable) not yet exercised. - [ ] Phase 3: Zero-sample watchdog escalation (separable) ## Findings @@ -294,10 +294,35 @@ the deferred pid-race fix forward in response). test_native_contract_parity.py`): 112 passed. `uv run onoats stop --help` resolves without booting a service. `ruff format` + `ruff check` clean. -- **Remaining:** Phase 2 (Swift menu-bar Stop rewiring) requires building the - menu-bar app and a manual orphan-then-stop smoke on the user's machine — not - autonomously runnable; deferred. Phase 3 is separable/optional and sequenced - after P2. +- **Remaining:** Phase 3 is separable/optional and sequenced after P2. + +### On-device smoke — headline orphan-then-stop verified (2026-06-22) + +The core bug (a crash-orphaned GUI session is unstoppable from the menu) is +**fixed and verified live** on the signed app with real TCC + audio. + +- **Smoke-procedure gotcha — Force Quit reaps the bundle capturer; it does NOT + produce an orphan.** First attempt force-quit `Onoats.app`; the capturer + (`Contents/MacOS/onoats-capturer`, a binary *inside* the bundle) was SIGKILL'd + (`rc=-9`) while the external supervisor (`~/.local/bin/onoats bot`) survived and + fail-loud-exited — so no orphan remained to Stop. Inferred mechanism: + LaunchServices reaps bundle-resident executables on app termination; the + out-of-bundle CLI supervisor is spared. A *genuine* GUI crash (segfault of only + the GUI process) leaves both supervisor and capturer alive. **To reproduce the + orphan, kill only the GUI process** (`kill -9 $(pgrep -f "MacOS/Onoats$")`), + never Force Quit. +- **Headline case PASS.** With the surgical kill, `onoats status` showed + `RUNNING` with a live capturer (orphan survived). Relaunch rendered + `running(ours:false)` with **Stop enabled** ("started outside the menu bar"). + Clicking Stop drove `stopExternal()` → `onoats stop` → identity-checked + **SIGTERM** → graceful drain. Live log confirmed the runtime invariant: STT + graceful close → **flush/rotate → pid-file removed → `Shutdown: complete`** (in + order), then `recorder exited; stopping capturer` — recorder-first, capturer + second, no `rc=-9`/`capturer-crash`/fail-loud. Final status: `not running` with + no exit-reason/last-error/supervisor-rc fields (clean graceful stop). +- **Not yet exercised (lower-risk matrix tail):** owned Start→Stop (unchanged + `p.terminate()` path, relabel only), double-click `stopRequested` guard, Flush + mid-session, and the CLI-failure re-enable (`cliPath`→non-zero exec). ## Issues & Solutions From 5572e755ed6065eb299ae32e48859e54d8acaa45 Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Mon, 22 Jun 2026 08:27:39 -0700 Subject: [PATCH 13/18] fix(runtime): close residual pid-file deletion window (atomic write + fail-closed removal) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 22 +++++-- CHANGELOG.md | 9 ++- .../20260619-bug-stoppable-orphan-session.md | 26 ++++++++ src/onoats/runtime.py | 61 ++++++++++++++----- tests/test_status_file.py | 55 +++++++++++++++++ 5 files changed, 149 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d051b3f..7291a07 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,12 +142,22 @@ Single-instance + pid-file ownership (`runtime.py`, pinned by `tests/test_status identity-verified recorder is already alive — a stale/recycled/foreign pid does NOT block a legitimate start. This is the single-instance lock; there is no separate `flock` file. -- **Pid removal is ownership-checked.** `_remove_pid_file(pid_path, owner_pid=…)` - unlinks only when the file still records that pid. Because `stop` returns on - signal delivery (not exit), a `stop`-then-immediate-`bot` could otherwise let a - draining recorder delete a NEWER recorder's pid file. Recorder teardown passes - `owner_pid=os.getpid()`; the GUI's menu gating (Start only in `.stopped`) - already prevents this from the app, so the guard protects the CLI/scripted path. +- **Pid writes are atomic.** `_write_pid_file` writes to a temp file and + `os.replace`s it into place (same dir → atomic rename), never truncating in + place — mirrors `onoats.status.write_status`. A concurrent reader (a draining + recorder's owner-checked removal) sees either the complete old record or the + complete new one, never an empty/partial file mid-write. +- **Pid removal is ownership-checked and fails closed.** + `_remove_pid_file(pid_path, owner_pid=…)` unlinks **only** when the file still + records exactly that pid. If it reads back as `None` (unreadable/foreign/already + gone) it is left in place — it must never be assumed to be our own benign + mid-write. Because `stop` returns on signal delivery (not exit), a + `stop`-then-immediate-`bot` could otherwise let a draining recorder delete a + NEWER recorder's pid file. Recorder teardown passes `owner_pid=os.getpid()`; the + GUI's menu gating (Start only in `.stopped`) already prevents this from the app, + so the guard protects the CLI/scripted path. A leftover invalid pid file is + self-healing: `status` reports no valid recorder and the next start atomically + replaces it. ## Reviewing a subprocess / process-boundary change diff --git a/CHANGELOG.md b/CHANGELOG.md index eec3494..ece0355 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,9 +41,12 @@ Annotated tags exist from `v0.9.0` forward. case AND the indeterminate-but-live case (marker-valid pid file, process alive, but the `ps` identity probe failed or the file is legacy/fingerprint-less), matching how stop/flush refuse that same state; only a positively stale (dead or - recycled-to-foreign) pid is overwritten; (2) pid-file removal is - ownership-checked, so a recorder never deletes a pid file a newer recorder has - since overwritten. The menu's external Stop also re-enables itself if the + recycled-to-foreign) pid is overwritten; (2) pid-file writes are atomic + (temp + `os.replace`, never a truncating in-place write) so a concurrent reader + never sees an empty/partial file; (3) pid-file removal is ownership-checked and + fails closed — a recorder unlinks only a file that still records *its own* pid, + and leaves an unreadable/foreign record in place rather than deleting a newer + recorder's (possibly in-progress) file. The menu's external Stop also re-enables itself if the `onoats stop` subprocess fails or exits non-zero (e.g. a stale installed CLI), rather than wedging the only Stop control until app restart. diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index 47bd078..ac31b9e 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -156,6 +156,32 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Findings +### Codex re-review round 3 (2026-06-22) — pid-file race residual + +A post-smoke Codex adversarial review returned NO-SHIP with one [high]: the +round-1/2 ownership-checked removal still had a deletion window. + +- **[high] Owner-checked pid cleanup could still delete a newer recorder's pid + file** (`runtime.py`). Two compounding gaps: (a) `_write_pid_file` wrote the pid + file **in place** with `write_text`, which truncates before writing — a reader + during that window sees an empty/partial file; (b) `_remove_pid_file(owner_pid=…)` + treated a `None` read (unparseable/empty) as "fall through to `unlink()`". So 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 fix + targeted. Verified from the code (in-place writer + `current is None` unlink); the + round-1/2 tests only covered a *fully written* newer pid file. Fixed: + - **Atomic write.** `_write_pid_file` now writes a temp file + `os.replace` + (same-dir atomic rename, `fsync` first), mirroring `onoats.status.write_status` + — no truncation window; a reader sees the complete old or complete new record. + - **Fail-closed removal.** `_remove_pid_file(owner_pid=…)` unlinks **only** when + the file still records exactly `owner_pid`; a `None`/foreign read is left in + place (logged), never deleted. A leftover invalid 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 (×3, parametrized) → file survives; and + `_write_pid_file` publishes via `os.replace` with no `*.tmp` residue. Full + suite: 289 passed; `ruff format`/`check` clean; static source-order check green. + ### Codex re-review round 2 (2026-06-20) — degraded-path fixes The re-run Codex adversarial review (after the round-1 fixes) returned a fresh diff --git a/src/onoats/runtime.py b/src/onoats/runtime.py index df3bdcf..c6cb10c 100644 --- a/src/onoats/runtime.py +++ b/src/onoats/runtime.py @@ -21,6 +21,7 @@ import platform import signal import sys +import tempfile import threading import time from pathlib import Path @@ -1111,10 +1112,30 @@ def _write_pid_file(data_dir: Path) -> Path: # readers can distinguish a freshly-started bot from one that # happens to have inherited a recycled pid (see onoats._vendor.pid). start_epoch = time.time() - pid_path.write_text( - f"{os.getpid()}\n{PID_MARKER}\n{cmdline}\n{start_epoch}\n", - encoding="utf-8", + payload = f"{os.getpid()}\n{PID_MARKER}\n{cmdline}\n{start_epoch}\n" + # Atomic replace (temp + os.replace in the SAME dir) — never truncate the pid + # file in place. A draining recorder's owner-checked `_remove_pid_file` reads + # this path concurrently; an in-place write_text would expose an empty/partial + # file mid-write, `read_pid_file` would return None, and the drainer would then + # delete this (newer) recorder's pid file. os.replace makes a concurrent reader + # see either the complete old record or the complete new one — never a partial. + # Mirrors the status-file writer idiom (onoats.status.write_status). + fd, tmp = tempfile.mkstemp( + dir=str(active_dir), prefix=".onoats-pid-", suffix=".tmp" ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(payload) + fh.flush() + os.fsync(fh.fileno()) + os.replace(tmp, pid_path) + except BaseException: + # Never leak a temp file on failure. + try: + os.unlink(tmp) + except FileNotFoundError: + pass + raise logger.debug( f"PID file written: {pid_path} (PID {os.getpid()}, cmdline={cmdline!r})" ) @@ -1124,21 +1145,31 @@ def _write_pid_file(data_dir: Path) -> Path: def _remove_pid_file(pid_path: Path, *, owner_pid: int | None = None) -> None: """Remove the PID file on shutdown. - When ``owner_pid`` is given, only unlink if the file STILL records that pid. - A recorder tearing down must never delete a pid file a NEWER recorder has - since overwritten with its own pid (the stop-then-immediate-start race) — that - would leave the new session running with no pid file, invisible to - ``status``/``stop``/``flush``. A file we can no longer read as a valid record - (``None`` — already gone, or foreign marker) falls through to the unlink, - preserving the prior best-effort cleanup for the non-race cases. + When ``owner_pid`` is given, unlink ONLY if the file still records exactly that + pid — fail closed. A recorder tearing down must never delete a pid file a NEWER + recorder has since taken over (the stop-then-immediate-start race), which would + leave the new session running with no pid file, invisible to + ``status``/``stop``/``flush``. We must also refuse to delete when the file reads + back as ``None``: paired with the atomic writer (``_write_pid_file`` uses + ``os.replace``, never a truncating in-place write) a ``None`` here is no longer + a benign mid-write of *our own* file, but either (a) a foreign/invalid record we + have no business removing, or (b) a file already gone — in both cases leaving it + is correct. A leftover invalid pid file is self-healing: ``status`` reports no + valid recorder and the next ``_write_pid_file`` atomically replaces it. """ if owner_pid is not None: current = _read_pid_file(pid_path) - if current is not None and current != owner_pid: - logger.warning( - f"PID file {pid_path} now records pid {current}, not ours " - f"({owner_pid}) — a newer recorder owns it; not removing." - ) + if current != owner_pid: + if current is None: + logger.debug( + f"PID file {pid_path} is unreadable/absent during owner-checked " + f"removal (owner {owner_pid}) — leaving in place (fail-closed)." + ) + else: + logger.warning( + f"PID file {pid_path} now records pid {current}, not ours " + f"({owner_pid}) — a newer recorder owns it; not removing." + ) return try: pid_path.unlink() diff --git a/tests/test_status_file.py b/tests/test_status_file.py index c4d538b..053b188 100644 --- a/tests/test_status_file.py +++ b/tests/test_status_file.py @@ -635,3 +635,58 @@ def test_write_pid_file_refuses_live_legacy_fingerprintless_pid(tmp_path): finally: proc.terminate() proc.wait(timeout=5) + + +@pytest.mark.parametrize( + "corrupt", + [ + "", # empty — a newer recorder mid-write (truncated) + "garbage-not-an-int\n", # unparseable first line + "12345\nWRONG-MARKER\nonoats bot\n0.0\n", # foreign / invalid marker + ], +) +def test_remove_pid_file_fail_closed_when_unreadable(tmp_path, corrupt): + """[high] regression (Codex adversarial review): owner-checked removal must + fail CLOSED. If the pid file reads back as None — an empty/partial file (a + newer recorder mid-write) or a foreign/invalid record — a draining recorder + must NOT unlink it. Pre-fix this fell through to ``pid_path.unlink()``, which + (paired with the old in-place truncating writer) could delete a newer + recorder's in-progress pid file, orphaning it (invisible to + status/stop/flush). Even with the atomic writer closing the truncation window, + removal stays fail-closed: a None read is never our own benign mid-write.""" + pid_path = tmp_path / ".active" / PID_FILENAME + pid_path.parent.mkdir(parents=True, exist_ok=True) + pid_path.write_text(corrupt, encoding="utf-8") + _remove_pid_file(pid_path, owner_pid=12345) + assert pid_path.exists(), ( + "fail-closed: a draining recorder must not delete an unreadable/foreign " + "pid file (it may be a newer recorder's in-progress record)" + ) + + +def test_write_pid_file_is_atomic_via_os_replace(tmp_path, monkeypatch): + """[high] regression (Codex adversarial review): the pid file is written via + temp + ``os.replace`` (atomic rename), never truncated in place. An in-place + ``write_text`` exposes an empty/partial file mid-write; a concurrent + owner-checked ``_remove_pid_file`` would then read None and delete the newer + recorder's file. Pin the atomic-replace contract and assert no temp residue.""" + import os as _os + + from onoats import runtime as _runtime + + replace_dests = [] + real_replace = _os.replace + + def _spy_replace(src, dst): + replace_dests.append(Path(dst)) + return real_replace(src, dst) + + monkeypatch.setattr(_runtime.os, "replace", _spy_replace) + pid_path = _write_pid_file(tmp_path) + + # Final record landed via os.replace into the real path — not written in place. + assert pid_path in replace_dests, "pid file must be published via os.replace" + assert pid_path.read_text(encoding="utf-8").startswith(f"{_os.getpid()}\n") + # No leaked temp files in the active dir. + leftovers = list((tmp_path / ".active").glob("*.tmp")) + assert not leftovers, f"atomic writer leaked temp residue: {leftovers}" From 7713caedb619cd16fe82e6c8a07056d07c3fc4e2 Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Mon, 22 Jun 2026 09:09:40 -0700 Subject: [PATCH 14/18] fix(runtime): atomic flock single-instance lock (close concurrent-start race) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 21 +++-- CHANGELOG.md | 30 ++++--- .../20260619-bug-stoppable-orphan-session.md | 60 ++++++++++--- src/onoats/__main__.py | 3 + src/onoats/dual.py | 4 + src/onoats/runtime.py | 84 ++++++++++++++++++- tests/test_status_file.py | 70 ++++++++++++++++ 7 files changed, 239 insertions(+), 33 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7291a07..ef55683 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -136,12 +136,21 @@ Load-bearing invariants (pinned by `tests/test_cli.py`): Single-instance + pid-file ownership (`runtime.py`, pinned by `tests/test_status_file.py`): -- **Start refuses to launch over a live recorder.** `_write_pid_file` runs the - same identity gate (`resolve_flush_target`) and raises - `RecorderAlreadyRunningError` (clean rc=1 at all CLI boundaries) when an - identity-verified recorder is already alive — a stale/recycled/foreign pid does - NOT block a legitimate start. This is the single-instance lock; there is no - separate `flock` file. +- **Start is gated by an atomic `flock` single-instance lock.** `_write_pid_file` + acquires an exclusive `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock` + (`_acquire_instance_lock`) **before** publishing the pid file, held for the + process lifetime. This is the primary gate and the only *atomic* one: of N + racing `onoats bot` starts exactly one wins, the rest raise + `RecorderAlreadyRunningError` (clean rc=1 at all CLI boundaries). The kernel + releases the lock when the holder exits — graceful OR crash/SIGKILL — so there is + no stale lock to reclaim; teardown also releases it explicitly + (`_release_instance_lock` in `dual._finalize_shutdown_status` / `__main__`) for a + prompt stop→start handoff. POSIX-only (no-op on Windows; macOS product). +- **Identity check is the secondary guard.** `_write_pid_file` still runs + `resolve_flush_target` and refuses an identity-verified (or indeterminate-but- + live) recorder — a stale/recycled/foreign pid does NOT block a legitimate start. + This catches a legacy/cross-version recorder that predates the lock; the `flock` + catches concurrent same-version starts the read-then-act identity check cannot. - **Pid writes are atomic.** `_write_pid_file` writes to a temp file and `os.replace`s it into place (same dir → atomic rename), never truncating in place — mirrors `onoats.status.write_status`. A concurrent reader (a draining diff --git a/CHANGELOG.md b/CHANGELOG.md index ece0355..9151774 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,19 +36,23 @@ Annotated tags exist from `v0.9.0` forward. delivery, not exit, so a new `onoats bot` launched during the old recorder's drain could overwrite the draining recorder's pid file — and the drainer would then unlink the *new* recorder's file, leaving it invisible to - `status`/`stop`/`flush`. Two guards close this: (1) start refuses to launch - over a live recorder (`RecorderAlreadyRunningError`) — both the identity-verified - case AND the indeterminate-but-live case (marker-valid pid file, process alive, - but the `ps` identity probe failed or the file is legacy/fingerprint-less), - matching how stop/flush refuse that same state; only a positively stale (dead or - recycled-to-foreign) pid is overwritten; (2) pid-file writes are atomic - (temp + `os.replace`, never a truncating in-place write) so a concurrent reader - never sees an empty/partial file; (3) pid-file removal is ownership-checked and - fails closed — a recorder unlinks only a file that still records *its own* pid, - and leaves an unreadable/foreign record in place rather than deleting a newer - recorder's (possibly in-progress) file. The menu's external Stop also re-enables itself if the - `onoats stop` subprocess fails or exits non-zero (e.g. a stale installed CLI), - rather than wedging the only Stop control until app restart. + `status`/`stop`/`flush`. Guards close this: (1) an **atomic `flock` + single-instance lock** — `_write_pid_file` takes an exclusive + `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock` before publishing the pid file, + so of N racing starts exactly one wins and the rest raise + `RecorderAlreadyRunningError`; the kernel releases it on exit (graceful or + crash), so there is no stale lock to reclaim, and a chained `onoats stop && + onoats bot` cleanly refuses until the drainer's process exits; (2) the identity + check (`resolve_flush_target`) remains as a secondary guard refusing a verified + or indeterminate-but-live recorder (legacy/cross-version), never blocking on a + stale/recycled/foreign pid; (3) pid-file writes are atomic (temp + `os.replace`, + never a truncating in-place write) so a concurrent reader never sees an + empty/partial file; (4) pid-file removal is ownership-checked and fails closed — + a recorder unlinks only a file that still records *its own* pid, and leaves an + unreadable/foreign record in place rather than deleting a newer recorder's + (possibly in-progress) file. The menu's external Stop also re-enables itself if + the `onoats stop` subprocess fails or exits non-zero (e.g. a stale installed + CLI), rather than wedging the only Stop control until app restart. ## [1.1.0] - 2026-06-12 diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index ac31b9e..4196867 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -156,6 +156,37 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Findings +### Codex re-review round 4 (2026-06-22) — atomic single-instance acquisition + +A fourth Codex adversarial pass returned NO-SHIP with one [high]: the pid guard +was still **check-then-replace**, so two concurrent starts could both launch. + +- **[high] Concurrent starts could both pass the guard and run** (`runtime.py`). + `_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 present 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). A pre-existing TOCTOU in + the single-instance model (not introduced by this branch), but the branch's + single-instance invariant claims to prevent exactly this. Fixed with the + durable lock the plan had deferred: + - **`flock` single-instance lock.** `_acquire_instance_lock` takes an exclusive + `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock` **before** publishing the pid + file; exactly one of N racing starts wins, the rest get + `RecorderAlreadyRunningError`. Held for the process lifetime via a + module-global fd; the kernel releases it on exit (graceful OR crash/SIGKILL), + so there is **no stale lock to reclaim** (the advantage over an `O_EXCL` + lockfile). Released explicitly on teardown (`dual._finalize_shutdown_status`, + `__main__`) for prompt handoff. No-op on Windows (POSIX-only; macOS product). + - The resolver-identity check stays as the **secondary** guard (catches a + legacy/cross-version recorder that predates the lock); the `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. Full suite: 291 passed; ruff clean; marker green. + ### Codex re-review round 3 (2026-06-22) — pid-file race residual A post-smoke Codex adversarial review returned NO-SHIP with one [high]: the @@ -256,10 +287,10 @@ the deferred pid-race fix forward in response). deletes a pid file a newer recorder has overwritten. Recorder call sites (`dual._finalize_shutdown_status`, `__main__`) pass `owner_pid=os.getpid()`. Regression tests in `test_status_file.py` (refuse-live, overwrite-stale-dead, - ownership-skip, back-compat-unconditional). This supersedes the "out of scope - for Phases 1–2" note under *Issues & Solutions* — the guard landed here rather - than as a separate `flock` change; the resolver-identity guard is equivalent - protection without a new lock file. + ownership-skip, back-compat-unconditional). **Superseded by round 4** — an + actual `flock` single-instance lock landed (see round-4 finding); the + resolver-identity guard is now the *secondary* (legacy/cross-version) check + behind the atomic lock, not the only guard. - **Verification:** full `uv run pytest` suite **283 passed**; `ruff format` + `ruff check` clean; `make build/Onoats` compiles. Codex re-review not re-run. @@ -395,15 +426,18 @@ sound flow rather than re-deriving it. Verified against current code not exist during the drain; the state machine gates it behind real process exit. The "no early flip" invariant (recorder shares the pid-owner's process, exits only after teardown) is what keeps `kill(0)` true through the whole drain. -- **Constraint for Phase 2 / future work**: never chain `stop()` then `start()` - back-to-back (e.g. a "Restart" button) without first waiting for the - `.running → .stopped` transition (`processAlive` → false). A `flock`-style hard - single-instance lock on the pid file (refuse the second start until the first is - fully gone) is the durable fix and would make even the racy CLI sequence - (`onoats stop && onoats bot`) safe. **Out of scope for Phases 1–2** — separate, - start-path-touching change; recorded here so it is not lost. -- **Files affected (future)**: `src/onoats/runtime.py` (`_write_pid_file` / - `_remove_pid_file`), `native/onoats-menubar/Sources/RecorderModel.swift`. +- **RESOLVED in round 4** — the `flock`-style hard single-instance lock landed. + `_write_pid_file` now acquires an exclusive `flock` (held until process exit) + before publishing the pid file, so the racy CLI sequence (`onoats stop && + onoats bot`) is now **safe**: the chained start cleanly refuses with + `RecorderAlreadyRunningError` until the draining recorder's process exits and the + kernel frees the lock (rather than clobbering the pid file). The GUI gating + (`Start` only in `.stopped`/`.failed`) remains as defence in depth, but is no + longer the *only* thing preventing the race. A future "Restart" button could now + retry-with-backoff against the lock instead of needing a hard wait. +- **Files affected**: `src/onoats/runtime.py` (`_acquire_instance_lock` / + `_release_instance_lock` / `_write_pid_file`), `src/onoats/dual.py` + + `src/onoats/__main__.py` (release on teardown). ## Final Results diff --git a/src/onoats/__main__.py b/src/onoats/__main__.py index 803a5cb..913e32d 100644 --- a/src/onoats/__main__.py +++ b/src/onoats/__main__.py @@ -64,6 +64,7 @@ stop_pipeline_for_shutdown, wait_or_force, _install_signal_handlers, + _release_instance_lock, _remove_pid_file, _restore_terminal, _start_keypress_reader, @@ -387,6 +388,8 @@ async def _shutdown_watcher() -> None: await _on_shutdown() _restore_terminal(_old_terminal_settings) _remove_pid_file(pid_path, owner_pid=os.getpid()) + # Release the single-instance lock (process-exit is the kernel backstop). + _release_instance_lock() # --------------------------------------------------------------------------- diff --git a/src/onoats/dual.py b/src/onoats/dual.py index bb91a32..d34db1c 100644 --- a/src/onoats/dual.py +++ b/src/onoats/dual.py @@ -51,6 +51,7 @@ log_stt_server_rss, _install_signal_handlers, _mark_status_rotation, + _release_instance_lock, _remove_pid_file, _restore_terminal, _start_keypress_reader, @@ -103,6 +104,9 @@ def _finalize_shutdown_status( _write_status_stopped(data_dir, exit_reason="graceful") _restore_terminal(old_terminal_settings) _remove_pid_file(pid_path, owner_pid=os.getpid()) + # Release the single-instance lock so a post-drain start can re-acquire the + # slot immediately; process exit is the kernel-guaranteed backstop. + _release_instance_lock() async def _shutdown_stt_service(stt_service, label: str) -> None: diff --git a/src/onoats/runtime.py b/src/onoats/runtime.py index c6cb10c..044dfa0 100644 --- a/src/onoats/runtime.py +++ b/src/onoats/runtime.py @@ -34,8 +34,9 @@ read_pid_file as _read_pid_file, ) -# termios/tty are Unix-only — guard for Windows compatibility +# termios/tty/fcntl are Unix-only — guard for Windows compatibility if sys.platform != "win32": + import fcntl import termios import tty @@ -1049,6 +1050,80 @@ def _pid_alive(pid: int) -> bool: return True +# Single-instance lock — an advisory ``flock`` held for the recorder's process +# lifetime. This is the ATOMIC instance-slot gate the pid file cannot be: the pid +# file is a readable data record written check-then-replace, but ``flock`` either +# acquires or fails with no window, and the kernel releases it automatically when +# the holder exits — graceful OR crash/SIGKILL — so there is never a stale lock to +# reclaim. The fd is kept open (module-global) for the whole process; closing it +# or process exit releases the lock. The lock file itself is never unlinked. +LOCK_FILENAME = "onoats.lock" +_instance_lock_fd: int | None = None + + +def _acquire_instance_lock(active_dir: Path) -> None: + """Atomically claim the single-instance slot; raise if another holds it. + + This is the gate that makes concurrent starts safe. ``_write_pid_file``'s + identity check is best-effort — it reads a possibly-absent pid file, so two + simultaneous starts with no valid pid file BOTH pass it. The ``flock`` here + lets exactly one win; the loser gets ``RecorderAlreadyRunningError`` before it + publishes a pid file or starts capturing. Held for the process lifetime via + the module-global fd; the kernel releases it on exit. As a bonus this enforces + the "no immediate stop→start" rule — ``stop`` returns on signal delivery, so a + chained start cleanly refuses until the draining recorder's process exits and + the kernel frees the lock. + + No-op on Windows (``flock`` unavailable); the pid-identity check remains the + only guard there. onoats is macOS-only in practice. + """ + global _instance_lock_fd + if sys.platform == "win32": + return + # One lock per process: drop any prior fd (e.g. a re-init inside a long-lived + # test process) before claiming a fresh slot. + if _instance_lock_fd is not None: + _release_instance_lock() + lock_path = active_dir / LOCK_FILENAME + fd = os.open(str(lock_path), os.O_RDWR | os.O_CREAT, 0o644) + try: + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError as exc: + os.close(fd) + # EWOULDBLOCK/EAGAIN → a live recorder already holds the slot. Read the + # pid file (if any) only to name it in the error — never to gate on. + from onoats._vendor.pid import read_pid_record + + rec = read_pid_record(active_dir / PID_FILENAME) + who = f" (pid {rec.pid})" if rec is not None else "" + raise RecorderAlreadyRunningError( + f"An onoats recorder is already running{who} and holds the " + "single-instance lock. Stop it first with `onoats stop`, then retry." + ) from exc + _instance_lock_fd = fd + + +def _release_instance_lock() -> None: + """Release the single-instance lock if held (no-op otherwise). + + Called on the teardown path for prompt release (so a post-drain start can + re-acquire the slot the instant we finish); process exit is the + kernel-guaranteed backstop for crash paths. + """ + global _instance_lock_fd + if _instance_lock_fd is None: + return + try: + fcntl.flock(_instance_lock_fd, fcntl.LOCK_UN) + except OSError: + pass + try: + os.close(_instance_lock_fd) + except OSError: + pass + _instance_lock_fd = None + + def _write_pid_file(data_dir: Path) -> Path: """Write the current process PID, identity marker, and cmdline fingerprint. @@ -1107,6 +1182,13 @@ def _write_pid_file(data_dir: Path) -> Path: except PermissionError: logger.warning("PID file exists, process may be running as different user") + # Atomic single-instance acquisition. The identity checks above are + # best-effort (they read a possibly-absent pid file); this flock is the gate + # that closes the concurrent-start race — two starts that both passed the + # checks above contend here, and exactly one proceeds to publish a pid file. + # Held until process exit (released on teardown / by the kernel on crash). + _acquire_instance_lock(active_dir) + cmdline = _own_ps_cmdline() # Wall-clock start_epoch is included as the 4th line so live-view # readers can distinguish a freshly-started bot from one that diff --git a/tests/test_status_file.py b/tests/test_status_file.py index 053b188..9ba498a 100644 --- a/tests/test_status_file.py +++ b/tests/test_status_file.py @@ -516,15 +516,30 @@ def test_write_prestart_waiting_is_fresh_running_with_warning(tmp_path: Path): import shutil # noqa: E402 import subprocess # noqa: E402 +import sys # noqa: E402 from onoats._vendor.pid import PID_FILENAME # noqa: E402 from onoats.runtime import ( # noqa: E402 + LOCK_FILENAME, RecorderAlreadyRunningError, + _acquire_instance_lock, + _release_instance_lock, _remove_pid_file, _write_pid_file, ) +@pytest.fixture(autouse=True) +def _release_instance_lock_after_each(): + """Keep the module-global single-instance lock from leaking between tests. + + A test that calls ``_write_pid_file``/``_acquire_instance_lock`` holds an + flock on its own ``tmp_path`` lock file; release it on teardown so the global + fd is reset (process-exit would release it anyway, but tests share a process).""" + yield + _release_instance_lock() + + def _seed_pid_file(data_dir: Path, pid: int, *, cmdline: str = "onoats bot") -> Path: pid_path = data_dir / ".active" / PID_FILENAME pid_path.parent.mkdir(parents=True, exist_ok=True) @@ -690,3 +705,58 @@ def _spy_replace(src, dst): # No leaked temp files in the active dir. leftovers = list((tmp_path / ".active").glob("*.tmp")) assert not leftovers, f"atomic writer leaked temp residue: {leftovers}" + + +def test_write_pid_file_refuses_concurrent_start_holding_instance_lock(tmp_path): + """[high] regression (Codex adversarial review round 4): the single-instance + guard must be ATOMIC, not check-then-replace. Two `onoats bot` starts racing + with no valid pid file both pass the best-effort identity check; the flock is + the gate that lets exactly ONE proceed. Simulate the race winner by holding + the flock (a separate open file description — POSIX flock conflicts even + within one process), then assert a second `_write_pid_file` refuses and does + NOT publish a pid file (so the loser can't run unrepresented).""" + import os as _os + + if sys.platform == "win32": + pytest.skip("flock single-instance lock is POSIX-only") + import fcntl as _fcntl + + active = tmp_path / ".active" + active.mkdir(parents=True, exist_ok=True) + lock_path = active / LOCK_FILENAME + holder = _os.open(str(lock_path), _os.O_RDWR | _os.O_CREAT, 0o644) + _fcntl.flock(holder, _fcntl.LOCK_EX | _fcntl.LOCK_NB) # instance 1 holds the slot + try: + with pytest.raises(RecorderAlreadyRunningError): + _write_pid_file(tmp_path) # instance 2 loses the race + assert not (active / PID_FILENAME).exists(), ( + "the start that lost the instance-lock race must not publish a pid file" + ) + finally: + _fcntl.flock(holder, _fcntl.LOCK_UN) + _os.close(holder) + + +def test_instance_lock_blocks_then_frees_on_release(tmp_path): + """The lock is exclusive while held and freed on release — so a post-drain + start can re-acquire the slot (the stop→start handoff).""" + import os as _os + + if sys.platform == "win32": + pytest.skip("flock single-instance lock is POSIX-only") + import fcntl as _fcntl + + active = tmp_path / ".active" + active.mkdir(parents=True, exist_ok=True) + _acquire_instance_lock(active) # we now hold the slot + # A concurrent acquirer (separate fd) is blocked while we hold it. + other = _os.open(str(active / LOCK_FILENAME), _os.O_RDWR) + try: + with pytest.raises(OSError): + _fcntl.flock(other, _fcntl.LOCK_EX | _fcntl.LOCK_NB) + # Release; the slot is now free for the next start. + _release_instance_lock() + _fcntl.flock(other, _fcntl.LOCK_EX | _fcntl.LOCK_NB) # must succeed now + _fcntl.flock(other, _fcntl.LOCK_UN) + finally: + _os.close(other) From 9968bbf92e2a63119216d31be535fe4c4efeb4cd Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Mon, 22 Jun 2026 13:49:36 -0700 Subject: [PATCH 15/18] fix(runtime): hoist single-instance lock before capturer spawn / device open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 28 ++++++----- CHANGELOG.md | 8 ++-- .../20260619-bug-stoppable-orphan-session.md | 30 ++++++++++++ src/onoats/__main__.py | 5 +- src/onoats/cli.py | 12 +++++ src/onoats/dual.py | 17 +++++-- src/onoats/runtime.py | 30 +++++++----- tests/conftest.py | 22 +++++++++ tests/test_socket_supervisor.py | 46 +++++++++++++++++++ tests/test_status_file.py | 14 ++---- 10 files changed, 170 insertions(+), 42 deletions(-) create mode 100644 tests/conftest.py diff --git a/AGENTS.md b/AGENTS.md index ef55683..8930c25 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -134,18 +134,24 @@ Load-bearing invariants (pinned by `tests/test_cli.py`): - **`onoats stop --help` resolves without booting a service** (local argparse + lazy resolver import), like `flush`. -Single-instance + pid-file ownership (`runtime.py`, pinned by `tests/test_status_file.py`): +Single-instance + pid-file ownership (`runtime.py`, pinned by +`tests/test_status_file.py` + `tests/test_socket_supervisor.py`): -- **Start is gated by an atomic `flock` single-instance lock.** `_write_pid_file` - acquires an exclusive `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock` - (`_acquire_instance_lock`) **before** publishing the pid file, held for the - process lifetime. This is the primary gate and the only *atomic* one: of N - racing `onoats bot` starts exactly one wins, the rest raise - `RecorderAlreadyRunningError` (clean rc=1 at all CLI boundaries). The kernel - releases the lock when the holder exits — graceful OR crash/SIGKILL — so there is - no stale lock to reclaim; teardown also releases it explicitly - (`_release_instance_lock` in `dual._finalize_shutdown_status` / `__main__`) for a - prompt stop→start handoff. POSIX-only (no-op on Windows; macOS product). +- **Start is gated by an atomic `flock` single-instance lock, acquired before any + capture side effect.** `_acquire_instance_lock` takes an exclusive + `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock`. It is hoisted to the EARLIEST + point in each entrypoint — the socket supervisor takes it *before spawning the + capturer* (`_supervise_socket_session`), `run_onoats_dual` *before opening + PortAudio* — so a losing concurrent start raises `RecorderAlreadyRunningError` + (clean rc=1 at all CLI boundaries) **before** it touches CoreAudio/TCC/a device, + never after. Acquisition is **idempotent** (already-held → no-op), so the later + `_write_pid_file` call (the universal backstop for `python -m onoats`) is a no-op + in the hoisted paths. This is the primary gate and the only *atomic* one: of N + racing starts exactly one wins. The lock is held for the **whole process + lifetime**; the kernel releases it on exit (graceful OR crash/SIGKILL) — there is + no stale lock to reclaim and **no teardown release call** (releasing during + shutdown would free the slot while the supervisor is still tearing down its + capturer). POSIX-only (no-op on Windows; macOS product). - **Identity check is the secondary guard.** `_write_pid_file` still runs `resolve_flush_target` and refuses an identity-verified (or indeterminate-but- live) recorder — a stale/recycled/foreign pid does NOT block a legitimate start. diff --git a/CHANGELOG.md b/CHANGELOG.md index 9151774..bef8cc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,10 +37,12 @@ Annotated tags exist from `v0.9.0` forward. drain could overwrite the draining recorder's pid file — and the drainer would then unlink the *new* recorder's file, leaving it invisible to `status`/`stop`/`flush`. Guards close this: (1) an **atomic `flock` - single-instance lock** — `_write_pid_file` takes an exclusive - `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock` before publishing the pid file, + single-instance lock** acquired before any capture side effect — the socket + supervisor takes an exclusive `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock` + *before spawning the capturer*, and `run_onoats_dual` *before opening PortAudio*, so of N racing starts exactly one wins and the rest raise - `RecorderAlreadyRunningError`; the kernel releases it on exit (graceful or + `RecorderAlreadyRunningError` **before touching CoreAudio/TCC/a device**; held + for the whole process lifetime and released by the kernel on exit (graceful or crash), so there is no stale lock to reclaim, and a chained `onoats stop && onoats bot` cleanly refuses until the drainer's process exits; (2) the identity check (`resolve_flush_target`) remains as a secondary guard refusing a verified diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index 4196867..11cb61c 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -156,6 +156,36 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Findings +### Codex re-review round 5 (2026-06-22) — lock acquired too late (before-capture hoist) + +A fifth pass acknowledged the round-4 atomic lock closed the pid-overwrite race +but returned NO-SHIP: the lock was acquired **too late** to be the advertised +single-instance *start* gate. + +- **[high] Socket starts could spawn a second capturer before losing the lock** + (`cli.py`). The lock 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. So two concurrent socket starts could both spawn capturers and touch + hardware; the loser failed only *after* those side effects (duplicate permission + prompts, device contention, transient double capture). Fixed by **hoisting + 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. + - **Removed the explicit teardown release** (`_finalize_shutdown_status`, + `__main__`) added in round 4. The lock is now held for the whole process + lifetime and freed by the kernel on exit. Releasing during shutdown was itself + a latent bug: it would free the slot while the socket supervisor is still + tearing down its capturer, letting a chained start spawn a second capturer into + a not-yet-released device. An autouse fixture (`tests/conftest.py`) resets the + process-global lock between tests (pytest shares one process). + - **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 green. + ### Codex re-review round 4 (2026-06-22) — atomic single-instance acquisition A fourth Codex adversarial pass returned NO-SHIP with one [high]: the pid guard diff --git a/src/onoats/__main__.py b/src/onoats/__main__.py index 913e32d..e03fdaa 100644 --- a/src/onoats/__main__.py +++ b/src/onoats/__main__.py @@ -64,7 +64,6 @@ stop_pipeline_for_shutdown, wait_or_force, _install_signal_handlers, - _release_instance_lock, _remove_pid_file, _restore_terminal, _start_keypress_reader, @@ -388,8 +387,8 @@ async def _shutdown_watcher() -> None: await _on_shutdown() _restore_terminal(_old_terminal_settings) _remove_pid_file(pid_path, owner_pid=os.getpid()) - # Release the single-instance lock (process-exit is the kernel backstop). - _release_instance_lock() + # The single-instance lock is held for the process lifetime; the kernel + # releases it on exit (see runtime._release_instance_lock). # --------------------------------------------------------------------------- diff --git a/src/onoats/cli.py b/src/onoats/cli.py index 5072106..661d2b4 100644 --- a/src/onoats/cli.py +++ b/src/onoats/cli.py @@ -528,6 +528,8 @@ async def _supervise_socket_session(rest: list[str]) -> int: from loguru import logger + from onoats.runtime import _acquire_instance_lock + # 0. One canonical data-dir resolution per session. Every status write in # this supervisor — the early-failure records below AND the recorder's own # stamps (passed down through _run_recorder_with_capturer → run_onoats_dual) @@ -544,6 +546,16 @@ async def _supervise_socket_session(rest: list[str]) -> int: ) return 1 + # Claim the single-instance slot BEFORE spawning the capturer. The capturer is + # the expensive, externally-visible side effect (a CoreAudio process tap, a TCC + # prompt, device acquisition); a losing concurrent start must refuse here — + # raising RecorderAlreadyRunningError (caught by _run_socket_supervisor → rc=1) + # — rather than spawn a second capturer and only fail after touching hardware. + # Idempotent + held for the process lifetime; the recorder's later acquires + # (run_onoats_dual / _write_pid_file) are no-ops, and the kernel frees the lock + # on exit (so the slot stays ours through the capturer teardown in `finally`). + _acquire_instance_lock(data_dir / ".active") + # 1. Private, supervisor-owned socket dir (0700). mkdtemp already creates the # dir 0700 and owner-only; a fresh per-generation dir means any stale socket # from a prior generation lives at a path the new recorder never references diff --git a/src/onoats/dual.py b/src/onoats/dual.py index d34db1c..5477be2 100644 --- a/src/onoats/dual.py +++ b/src/onoats/dual.py @@ -47,11 +47,11 @@ SttPreflightError, stop_pipeline_for_shutdown, wait_or_force, + _acquire_instance_lock, _create_stt_service, log_stt_server_rss, _install_signal_handlers, _mark_status_rotation, - _release_instance_lock, _remove_pid_file, _restore_terminal, _start_keypress_reader, @@ -104,9 +104,10 @@ def _finalize_shutdown_status( _write_status_stopped(data_dir, exit_reason="graceful") _restore_terminal(old_terminal_settings) _remove_pid_file(pid_path, owner_pid=os.getpid()) - # Release the single-instance lock so a post-drain start can re-acquire the - # slot immediately; process exit is the kernel-guaranteed backstop. - _release_instance_lock() + # The single-instance lock is intentionally NOT released here: it is held for + # the whole process lifetime and the kernel frees it on exit. Releasing now + # would free the slot while the socket supervisor is still tearing down its + # capturer (see runtime._release_instance_lock). async def _shutdown_stt_service(stt_service, label: str) -> None: @@ -399,6 +400,14 @@ async def run_onoats_dual( # callers (the PortAudio path) resolve here as before. data_dir = data_dir if data_dir is not None else onoats_data_dir() + # Claim the single-instance slot BEFORE any capture side effect (PortAudio + # device open below / socket-transport build). In socket mode the supervisor + # already holds it (acquired before spawning the capturer), so this is an + # idempotent no-op; in the PortAudio path this is the early gate. A losing + # concurrent start raises RecorderAlreadyRunningError here, before it touches + # an audio device. Held for the process lifetime (kernel releases on exit). + _acquire_instance_lock(data_dir / ".active") + from onoats.config import load_config cfg = load_config() diff --git a/src/onoats/runtime.py b/src/onoats/runtime.py index 044dfa0..6ec8cd9 100644 --- a/src/onoats/runtime.py +++ b/src/onoats/runtime.py @@ -1080,10 +1080,13 @@ def _acquire_instance_lock(active_dir: Path) -> None: global _instance_lock_fd if sys.platform == "win32": return - # One lock per process: drop any prior fd (e.g. a re-init inside a long-lived - # test process) before claiming a fresh slot. + # Idempotent: one lock per process. If we already hold it, this is a nested + # acquire (the socket supervisor takes it before spawning the capturer, then + # the recorder's _write_pid_file/run_onoats_dual call it again) — return + # without re-acquiring, so there is never a release-then-reacquire gap. if _instance_lock_fd is not None: - _release_instance_lock() + return + active_dir.mkdir(parents=True, exist_ok=True) lock_path = active_dir / LOCK_FILENAME fd = os.open(str(lock_path), os.O_RDWR | os.O_CREAT, 0o644) try: @@ -1106,9 +1109,13 @@ def _acquire_instance_lock(active_dir: Path) -> None: def _release_instance_lock() -> None: """Release the single-instance lock if held (no-op otherwise). - Called on the teardown path for prompt release (so a post-drain start can - re-acquire the slot the instant we finish); process exit is the - kernel-guaranteed backstop for crash paths. + Deliberately NOT called on the normal recorder teardown path: the lock is held + for the whole process lifetime and the kernel releases it on exit (graceful OR + crash). Releasing during shutdown would free the slot while the socket + supervisor is still tearing down its capturer — letting a chained start spawn a + second capturer into a device the old one hasn't finished releasing. Provided + for explicit lifecycle control in tests (and any future caller that genuinely + owns the whole start→stop span). """ global _instance_lock_fd if _instance_lock_fd is None: @@ -1182,11 +1189,12 @@ def _write_pid_file(data_dir: Path) -> Path: except PermissionError: logger.warning("PID file exists, process may be running as different user") - # Atomic single-instance acquisition. The identity checks above are - # best-effort (they read a possibly-absent pid file); this flock is the gate - # that closes the concurrent-start race — two starts that both passed the - # checks above contend here, and exactly one proceeds to publish a pid file. - # Held until process exit (released on teardown / by the kernel on crash). + # Single-instance acquisition (universal backstop). The capture entrypoints + # acquire this EARLY — the socket supervisor before spawning the capturer, + # run_onoats_dual before opening PortAudio — so by the time we publish a pid + # file the lock is already held and this call is an idempotent no-op. It stays + # here so any entrypoint that reaches pid publication without an earlier + # acquire (e.g. `python -m onoats`) is still gated. Held until process exit. _acquire_instance_lock(active_dir) cmdline = _own_ps_cmdline() diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..3eaf035 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,22 @@ +"""Shared pytest fixtures. + +The single-instance lock (`onoats.runtime._instance_lock_fd`) is held for the +recorder's whole process lifetime — in production the kernel releases it on exit, +so there is no teardown release call. But pytest runs every test in ONE process, +so a test that acquires the lock (directly, via `_write_pid_file`, or by driving +the socket supervisor) would otherwise leak the held fd into the next test and, +because acquisition is idempotent (already-held → no-op), silently suppress the +next test's acquire. Release it after every test so each starts from a clean slot. +""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture(autouse=True) +def _release_instance_lock_after_each(): + yield + from onoats.runtime import _release_instance_lock + + _release_instance_lock() diff --git a/tests/test_socket_supervisor.py b/tests/test_socket_supervisor.py index d483cf7..50c09fd 100644 --- a/tests/test_socket_supervisor.py +++ b/tests/test_socket_supervisor.py @@ -626,6 +626,52 @@ def test_unspawnable_capturer_bin_fails_loud(sup_env, log_sink, monkeypatch): assert log_sink.warned() +def test_held_instance_lock_blocks_capturer_spawn(sup_env, monkeypatch): + """[high] regression (Codex adversarial review round 5): the single-instance + lock is acquired BEFORE the capturer is spawned. With the slot already held, + the supervisor must refuse (rc=1) WITHOUT ever calling + ``create_subprocess_exec`` — a start that lost the race must not spawn a second + CoreAudio process tap / trigger a TCC prompt / contend for the device and only + fail afterwards. Acquiring inside ``_write_pid_file`` (post capturer-spawn) was + too late; this pins the hoisted acquisition.""" + import asyncio as _asyncio + import fcntl as _fcntl + import os as _os + import sys as _sys + + if _sys.platform == "win32": + pytest.skip("flock single-instance lock is POSIX-only") + + from onoats.runtime import LOCK_FILENAME + + data_dir, _pending = sup_env + active = data_dir / ".active" + active.mkdir(parents=True, exist_ok=True) + # Instance 1 holds the slot (separate open file description → flock conflicts + # even within this one process). + holder = _os.open(str(active / LOCK_FILENAME), _os.O_RDWR | _os.O_CREAT, 0o644) + _fcntl.flock(holder, _fcntl.LOCK_EX | _fcntl.LOCK_NB) + + spawned = {"n": 0} + real_exec = _asyncio.create_subprocess_exec + + async def _spy_exec(*a, **k): + spawned["n"] += 1 + return await real_exec(*a, **k) + + monkeypatch.setattr(_asyncio, "create_subprocess_exec", _spy_exec) + + try: + rc = _run_supervisor_bounded([]) + assert rc == 1, "a start that lost the instance-lock race must exit rc=1" + assert spawned["n"] == 0, ( + "the capturer must NOT be spawned when the instance lock is already held" + ) + finally: + _fcntl.flock(holder, _fcntl.LOCK_UN) + _os.close(holder) + + def test_recorder_handshake_failure_maps_to_clean_nonzero(log_sink, monkeypatch): """A controlled recorder launch failure must be rc=1, not a traceback. diff --git a/tests/test_status_file.py b/tests/test_status_file.py index 9ba498a..55cfae3 100644 --- a/tests/test_status_file.py +++ b/tests/test_status_file.py @@ -528,16 +528,10 @@ def test_write_prestart_waiting_is_fresh_running_with_warning(tmp_path: Path): _write_pid_file, ) - -@pytest.fixture(autouse=True) -def _release_instance_lock_after_each(): - """Keep the module-global single-instance lock from leaking between tests. - - A test that calls ``_write_pid_file``/``_acquire_instance_lock`` holds an - flock on its own ``tmp_path`` lock file; release it on teardown so the global - fd is reset (process-exit would release it anyway, but tests share a process).""" - yield - _release_instance_lock() +# The single-instance lock is released after every test by an autouse fixture in +# tests/conftest.py (the lock is process-lifetime in production; pytest shares one +# process, so tests must reset it). `_release_instance_lock` is imported above for +# the explicit acquire/release-cycle test below. def _seed_pid_file(data_dir: Path, pid: int, *, cmdline: str = "onoats bot") -> Path: From 8d300160640bcb69254e4d45d36e07f446b3f55b Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Mon, 22 Jun 2026 15:30:00 -0700 Subject: [PATCH 16/18] fix(runtime): bot-single lock parity + compare-and-unlink stale cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 16 +++-- CHANGELOG.md | 9 ++- .../20260619-bug-stoppable-orphan-session.md | 31 +++++++++ src/onoats/__main__.py | 21 ++++-- src/onoats/cli.py | 60 ++++++++++------ tests/test_cli.py | 68 +++++++++++++++++++ 6 files changed, 170 insertions(+), 35 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8930c25..8872daa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -121,7 +121,10 @@ Load-bearing invariants (pinned by `tests/test_cli.py`): - **Never signal an unverified or recycled pid.** The resolver validates the marker, requires a cmdline fingerprint, probes liveness (`kill(0)`), and compares the live `ps` cmdline against the stored one. Only a fully-verified - pid is signalled; unlink **only** when `stale=True`; treat `ProcessLookupError` + pid is signalled; unlink **only** when `stale=True`, and even then + **compare-and-unlink** (`_compare_and_unlink_stale_pid` → `_remove_pid_file` + ownership check) — never blindly, or a fresh recorder that won the lock and wrote + its pid in the resolve→cleanup window would be deleted; treat `ProcessLookupError` at signal time (TOCTOU) as stale. This matters **more** for `stop` than `flush` — SIGTERM kills by default, so a blind signal to a recycled foreign pid would terminate an unrelated process. A differential test asserts `stop` sends @@ -142,11 +145,12 @@ Single-instance + pid-file ownership (`runtime.py`, pinned by `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock`. It is hoisted to the EARLIEST point in each entrypoint — the socket supervisor takes it *before spawning the capturer* (`_supervise_socket_session`), `run_onoats_dual` *before opening - PortAudio* — so a losing concurrent start raises `RecorderAlreadyRunningError` - (clean rc=1 at all CLI boundaries) **before** it touches CoreAudio/TCC/a device, - never after. Acquisition is **idempotent** (already-held → no-op), so the later - `_write_pid_file` call (the universal backstop for `python -m onoats`) is a no-op - in the hoisted paths. This is the primary gate and the only *atomic* one: of N + PortAudio*, and `run_onoats` (`bot-single` / `python -m onoats`) *before even + importing the native deps* — so a losing concurrent start raises + `RecorderAlreadyRunningError` (clean rc=1 at all CLI boundaries) **before** it + touches CoreAudio/TCC/a device, never after. Acquisition is **idempotent** + (already-held → no-op), so the later `_write_pid_file` call (a backstop) is a + no-op in the hoisted paths. This is the primary gate and the only *atomic* one: of N racing starts exactly one wins. The lock is held for the **whole process lifetime**; the kernel releases it on exit (graceful OR crash/SIGKILL) — there is no stale lock to reclaim and **no teardown release call** (releasing during diff --git a/CHANGELOG.md b/CHANGELOG.md index bef8cc3..cbf6936 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,8 +39,9 @@ Annotated tags exist from `v0.9.0` forward. `status`/`stop`/`flush`. Guards close this: (1) an **atomic `flock` single-instance lock** acquired before any capture side effect — the socket supervisor takes an exclusive `flock(LOCK_EX|LOCK_NB)` on `.active/onoats.lock` - *before spawning the capturer*, and `run_onoats_dual` *before opening PortAudio*, - so of N racing starts exactly one wins and the rest raise + *before spawning the capturer*, `run_onoats_dual` *before opening PortAudio*, and + `run_onoats` (`bot-single` / `python -m onoats`) *before importing the native + deps*, so of N racing starts exactly one wins and the rest raise `RecorderAlreadyRunningError` **before touching CoreAudio/TCC/a device**; held for the whole process lifetime and released by the kernel on exit (graceful or crash), so there is no stale lock to reclaim, and a chained `onoats stop && @@ -52,7 +53,9 @@ Annotated tags exist from `v0.9.0` forward. empty/partial file; (4) pid-file removal is ownership-checked and fails closed — a recorder unlinks only a file that still records *its own* pid, and leaves an unreadable/foreign record in place rather than deleting a newer recorder's - (possibly in-progress) file. The menu's external Stop also re-enables itself if + (possibly in-progress) file; (5) `stop`/`flush` stale cleanup is + **compare-and-unlink** (not a blind `unlink`) so a fresh recorder that won the + lock and wrote its pid in the resolve→cleanup window is never deleted. The menu's external Stop also re-enables itself if the `onoats stop` subprocess fails or exits non-zero (e.g. a stale installed CLI), rather than wedging the only Stop control until app restart. diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index 11cb61c..805505b 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -156,6 +156,37 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Findings +### Codex re-review round 6 (2026-06-22) — entrypoint parity + stale-cleanup TOCTOU + +A sixth pass confirmed the main socket/dual lifecycle was sound but found two more +process-control holes; both fixed. + +- **[high] `bot-single` / `python -m onoats` still set up capture before claiming + the lock** (`__main__.py`). `run_onoats()` imported native deps (pyaudio via + `LocalAudioTransport` / `audio_devices`), resolved the data dir, then did + `select_input_device()` + crash recovery + pipeline build — all *before* + `_write_pid_file` (its only lock 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 same `run_onoats`, so both entrypoints are + now gated consistently with socket mode. Regression + (`test_cli.py::test_bot_single_held_lock_fails_before_device_selection`): a held + lock → rc=1 with `select_input_device` never called. +- **[high] `stop`/`flush` stale cleanup could delete a newer live recorder's pid + file** (`cli.py`). When `resolve_flush_target` returned `stale=True`, the command + **blindly** `unlink`ed 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` then deletes that fresh file, orphaning the live recorder from + `status`/`stop`/`flush`. Pre-existing (cloned from the shipped `flush`), now + fixed in the shared path: a new `_compare_and_unlink_stale_pid` helper 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; a fresh/foreign + pid is left in place. Applied to all four stale/dead-pid unlink sites (`stop` + and `flush`, the `stale` and `ProcessLookupError` branches). Regression + (`test_cli.py::test_stop_stale_cleanup_preserves_freshly_written_pid`): a fresh + pid written during resolution survives. Full suite: 294 passed; ruff + marker + green. + ### Codex re-review round 5 (2026-06-22) — lock acquired too late (before-capture hoist) A fifth pass acknowledged the round-4 atomic lock closed the pid-overwrite race diff --git a/src/onoats/__main__.py b/src/onoats/__main__.py index e03fdaa..6d0e15c 100644 --- a/src/onoats/__main__.py +++ b/src/onoats/__main__.py @@ -60,6 +60,7 @@ RecorderAlreadyRunningError, SHUTDOWN_CANCEL_TIMEOUT_SEC, SttPreflightError, + _acquire_instance_lock, _create_stt_service, stop_pipeline_for_shutdown, wait_or_force, @@ -122,6 +123,20 @@ async def run_onoats( The classifier still extracts summary/tags/action_items but the category is overridden. """ + # ---------------------------------------------------------------- + # Step 1: Resolve the data dir + claim the single-instance slot FIRST — + # before importing native deps (pyaudio via LocalAudioTransport / + # audio_devices) or ANY capture setup. A losing concurrent `onoats bot-single` + # (or `python -m onoats`) raises RecorderAlreadyRunningError here having + # touched nothing — the same before-capture gate the socket supervisor and + # run_onoats_dual apply. Idempotent; held for the process lifetime (kernel + # releases on exit). + # ---------------------------------------------------------------- + from onoats._vendor.store import onoats_data_dir + + data_dir = onoats_data_dir() + _acquire_instance_lock(data_dir / ".active") + from pipecat.audio.vad.silero import SileroVADAnalyzer from pipecat.processors.audio.vad_processor import VADProcessor from pipecat.pipeline.runner import PipelineRunner @@ -131,16 +146,10 @@ async def run_onoats( LocalAudioTransportParams, ) - from onoats._vendor.store import onoats_data_dir from onoats.config.audio_devices import select_input_device from onoats.processors.silence_detector import SilenceDetector from onoats.processors.transcript_buffer import TranscriptBuffer - # ---------------------------------------------------------------- - # Step 1: Resolve the data dir (XDG-aware; ONOATS_DATA_DIR wins) - # ---------------------------------------------------------------- - data_dir = onoats_data_dir() - # ---------------------------------------------------------------- # Step 2: Select audio device (input only — silent recorder) # ---------------------------------------------------------------- diff --git a/src/onoats/cli.py b/src/onoats/cli.py index 661d2b4..137ff63 100644 --- a/src/onoats/cli.py +++ b/src/onoats/cli.py @@ -1126,6 +1126,26 @@ def _cmd_convert(rest: list[str]) -> int: return convert_main(rest) +def _compare_and_unlink_stale_pid(pid_path: Path, expected_pid: int | None) -> None: + """Remove a stale/dead pid file ONLY if it still records ``expected_pid``. + + A blind ``unlink`` during stale cleanup can delete a NEWER recorder's pid + file: ``stop``/``flush`` resolve an old dead/recycled pid as stale, but in the + window before the unlink a fresh recorder may have won the single-instance + lock and written its own pid file — a blind unlink would then orphan that live + recorder from ``status``/``stop``/``flush``. Reuse the recorder's own + fail-closed ownership check (``_remove_pid_file`` unlinks only on an exact pid + match; a ``None``/foreign read is left in place). ``expected_pid is None`` (the + file was unparseable when we resolved it) → skip entirely; a future start + atomically replaces it (self-healing). + """ + if expected_pid is None: + return + from onoats.runtime import _remove_pid_file + + _remove_pid_file(pid_path, owner_pid=expected_pid) + + def _cmd_flush(rest: list[str]) -> int: """Send SIGUSR1 to the running recorder so it rotates its buffer. @@ -1146,18 +1166,21 @@ def _cmd_flush(rest: list[str]) -> int: args = parser.parse_args(rest) data_dir = Path(args.data_dir) if args.data_dir else None - from onoats._vendor.pid import resolve_flush_target + from onoats._vendor.pid import read_pid_record, resolve_flush_target pid_path = _pid_path(data_dir) + # Capture the pid the file records BEFORE resolving, so stale cleanup can + # compare-and-unlink (never delete a newer recorder's freshly-written file). + prior_rec = read_pid_record(pid_path) target = resolve_flush_target(pid_path) if target.pid is None: # Identity could not be confirmed. Drop a now-untrustworthy pid file so - # the next run starts clean, but never signal an unverified pid. + # the next run starts clean, but never signal an unverified pid — and only + # if it STILL records the stale pid we resolved (compare-and-unlink). if target.stale: - try: - pid_path.unlink() - except OSError: - pass + _compare_and_unlink_stale_pid( + pid_path, prior_rec.pid if prior_rec else None + ) print(f"onoats flush: {target.reason} (pid file {pid_path})", file=sys.stderr) return 1 pid = target.pid @@ -1166,10 +1189,7 @@ def _cmd_flush(rest: list[str]) -> int: except ProcessLookupError: # Raced: the verified recorder exited between the identity check and # the signal. Treat as stale rather than signalling a recycled pid. - try: - pid_path.unlink() - except OSError: - pass + _compare_and_unlink_stale_pid(pid_path, pid) print( f"onoats flush: recorder pid {pid} is not running (stale pid file)", file=sys.stderr, @@ -1205,18 +1225,21 @@ def _cmd_stop(rest: list[str]) -> int: args = parser.parse_args(rest) data_dir = Path(args.data_dir) if args.data_dir else None - from onoats._vendor.pid import resolve_flush_target + from onoats._vendor.pid import read_pid_record, resolve_flush_target pid_path = _pid_path(data_dir) + # Capture the pid the file records BEFORE resolving, so stale cleanup can + # compare-and-unlink (never delete a newer recorder's freshly-written file). + prior_rec = read_pid_record(pid_path) target = resolve_flush_target(pid_path) if target.pid is None: # Identity could not be confirmed. Drop a now-untrustworthy pid file so - # the next run starts clean, but never signal an unverified pid. + # the next run starts clean, but never signal an unverified pid — and only + # if it STILL records the stale pid we resolved (compare-and-unlink). if target.stale: - try: - pid_path.unlink() - except OSError: - pass + _compare_and_unlink_stale_pid( + pid_path, prior_rec.pid if prior_rec else None + ) print(f"onoats stop: {target.reason} (pid file {pid_path})", file=sys.stderr) return 1 pid = target.pid @@ -1225,10 +1248,7 @@ def _cmd_stop(rest: list[str]) -> int: except ProcessLookupError: # Raced: the verified recorder exited between the identity check and # the signal. Treat as stale rather than signalling a recycled pid. - try: - pid_path.unlink() - except OSError: - pass + _compare_and_unlink_stale_pid(pid_path, pid) print( f"onoats stop: recorder pid {pid} is not running (stale pid file)", file=sys.stderr, diff --git a/tests/test_cli.py b/tests/test_cli.py index a719b43..07f0ea3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -656,3 +656,71 @@ def test_stop_help_resolves_without_booting(capsys): out = capsys.readouterr().out.lower() assert "usage" in out assert "onoats stop" in out + + +def test_stop_stale_cleanup_preserves_freshly_written_pid(monkeypatch, _isolate_env): + """[high] regression (Codex adversarial review round 6): stale cleanup must + COMPARE-and-unlink. A blind unlink would delete a NEWER recorder's pid file + written in the window between stale resolution and cleanup (the new recorder + won the single-instance lock and published its pid). Simulate that racing + write inside resolve_flush_target; the stale cleanup must NOT delete it.""" + import os as _os + + from onoats._vendor.pid import FlushTarget + + pid_path = cli._pid_path(_isolate_env) + pid_path.parent.mkdir(parents=True, exist_ok=True) + # The old, dead pid `stop` resolves first (valid marker → parseable). + pid_path.write_text("999999\nonoats-bot\nonoats bot\n0.0\n", encoding="utf-8") + + fresh_pid = _os.getpid() # a live pid standing in for the new recorder + + def _racing_resolve(p): + # The new recorder won the lock and published its pid in the window + # between our prior-read and the stale cleanup. + p.write_text(f"{fresh_pid}\nonoats-bot\nonoats bot\n123.0\n", encoding="utf-8") + return FlushTarget(pid=None, reason="stale (dead pid)", stale=True) + + monkeypatch.setattr("onoats._vendor.pid.resolve_flush_target", _racing_resolve) + rc = cli.main(["stop"]) + assert rc == 1 + # The freshly-written pid file MUST survive — never deleted by stale cleanup. + assert pid_path.exists(), "stale cleanup must not delete a newer recorder's file" + assert pid_path.read_text(encoding="utf-8").startswith(f"{fresh_pid}\n") + + +def test_bot_single_held_lock_fails_before_device_selection(monkeypatch, _isolate_env): + """[high] regression (Codex adversarial review round 6): `bot-single` must + claim the single-instance lock BEFORE any capture setup. With the slot already + held, run_onoats must fail (rc=1) without ever calling select_input_device — + a losing start must not enumerate/touch audio before discovering it lost.""" + import fcntl as _fcntl + import os as _os + import sys as _sys + + if _sys.platform == "win32": + pytest.skip("flock single-instance lock is POSIX-only") + + from onoats.runtime import LOCK_FILENAME + + active = _isolate_env / ".active" + active.mkdir(parents=True, exist_ok=True) + holder = _os.open(str(active / LOCK_FILENAME), _os.O_RDWR | _os.O_CREAT, 0o644) + _fcntl.flock(holder, _fcntl.LOCK_EX | _fcntl.LOCK_NB) + + selected = {"n": 0} + monkeypatch.setattr( + "onoats.config.audio_devices.select_input_device", + lambda **k: selected.__setitem__("n", selected["n"] + 1), + ) + try: + from onoats.__main__ import main as single_main + + rc = single_main([]) + assert rc == 1, "a losing bot-single start must exit rc=1" + assert selected["n"] == 0, ( + "device selection must not run when the instance lock is already held" + ) + finally: + _fcntl.flock(holder, _fcntl.LOCK_UN) + _os.close(holder) From 7a7d02801f41e29044bec162097b65206da9d8cf Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Mon, 22 Jun 2026 15:49:58 -0700 Subject: [PATCH 17/18] fix: legacy-recorder identity preflight in lock + Stop-button wedge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- AGENTS.md | 16 +- .../20260619-bug-stoppable-orphan-session.md | 34 ++++ .../Sources/RecorderModel.swift | 31 +++- src/onoats/runtime.py | 149 ++++++++++-------- tests/test_socket_supervisor.py | 51 ++++++ 5 files changed, 206 insertions(+), 75 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8872daa..357aa9c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -156,11 +156,17 @@ Single-instance + pid-file ownership (`runtime.py`, pinned by no stale lock to reclaim and **no teardown release call** (releasing during shutdown would free the slot while the supervisor is still tearing down its capturer). POSIX-only (no-op on Windows; macOS product). -- **Identity check is the secondary guard.** `_write_pid_file` still runs - `resolve_flush_target` and refuses an identity-verified (or indeterminate-but- - live) recorder — a stale/recycled/foreign pid does NOT block a legitimate start. - This catches a legacy/cross-version recorder that predates the lock; the `flock` - catches concurrent same-version starts the read-then-act identity check cannot. +- **Identity preflight is the secondary guard, and it runs INSIDE the lock.** + `_acquire_instance_lock` calls `_refuse_if_live_recorder` (`resolve_flush_target` + + the indeterminate-but-live refusal) immediately after taking the `flock`, so + both guards fire at the same hoisted, before-capture point. This catches a live + legacy/cross-version recorder that holds no `flock` (an older build) — without + it, such a start would acquire the `flock` and spawn the capturer before + refusing late in `_write_pid_file`. A stale/recycled/foreign pid does NOT block a + legitimate start. The `flock` catches concurrent same-version starts the + read-then-act identity check cannot; the identity preflight catches the legacy + recorder the `flock` cannot. On Windows (no `flock`) the preflight is the only + guard. - **Pid writes are atomic.** `_write_pid_file` writes to a temp file and `os.replace`s it into place (same dir → atomic rename), never truncating in place — mirrors `onoats.status.write_status`. A concurrent reader (a draining diff --git a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md index 805505b..cc2ce79 100644 --- a/docs/dev_plans/20260619-bug-stoppable-orphan-session.md +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -156,6 +156,40 @@ Make any identity-verified live recorder session stoppable from the menu bar — ## Findings +### Codex round 7 + `/code-review` (2026-06-22) — legacy-recorder preflight + Stop-button wedge + +Codex round 7 (NO-SHIP) and a parallel multi-angle `/code-review --fix` converged +on two issues; both fixed. + +- **[high] A live LEGACY/cross-version recorder was detected only after capturer + spawn** (`runtime.py` / `cli.py`). 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). Now both guards + (atomic `flock` + identity preflight) fire at the same hoisted, before-capture + point in every entrypoint; `_write_pid_file`'s duplicate check was removed (it + now relies on the lock it already calls). This 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** + (`RecorderModel.swift`). `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` (the pid we asked to stop) and clearing `stopRequested` + when that pid is gone OR a *different* pid is live. Compiles clean + (`make build/Onoats`); manual-smoke as usual for Swift. +- **Skipped (documented, not bugs):** atomic-write duplication with + `status.py::write_status` (extracting a shared helper would modify shipped, + out-of-scope code); `_cmd_stop`/`_cmd_flush` near-clone (a deliberate convention + — AGENTS.md: "near-clone, not a refactor", parity pinned by tests); + `_pid_alive`/`_compare_and_unlink_stale_pid` thin-wrapper nits (intentional + clarity). Full suite: 295 passed; ruff + marker green. + ### Codex re-review round 6 (2026-06-22) — entrypoint parity + stale-cleanup TOCTOU A sixth pass confirmed the main socket/dual lifecycle was sound but found two more diff --git a/native/onoats-menubar/Sources/RecorderModel.swift b/native/onoats-menubar/Sources/RecorderModel.swift index 6bde8e7..a1d22fa 100644 --- a/native/onoats-menubar/Sources/RecorderModel.swift +++ b/native/onoats-menubar/Sources/RecorderModel.swift @@ -80,6 +80,13 @@ final class RecorderModel: ObservableObject { /// use the `.stopping` enum case for external stops: `.stopping` is cleared /// by `handleExit`, which never fires without a `Process` handle. @Published var stopRequested = false + // The recorder pid `stopExternal()` asked to stop. `refresh()` clears + // `stopRequested` once THAT pid is gone — OR once a DIFFERENT pid is live (a + // new session replaced the one we stopped before we observed its exit), so a + // fresh session never inherits a stuck-disabled Stop button. nil when no + // external stop is in flight, or when the pid couldn't be read at stop time + // (then refresh falls back to the plain `!alive` clear). + private var stopRequestedForPid: Int32? /// Valid `[stt].service` values — mirror of runtime.py /// `VALID_STT_SERVICES` (parity-checked by @@ -264,11 +271,19 @@ final class RecorderModel: ObservableObject { return } // External-stop convergence (handle-less session): clear the cosmetic - // stopRequested flag once the supervisor is actually GONE — process exit - // (kill(0) false), NOT pid-file removal. Placed AFTER the owned-proc - // early-return above so an owned session's `.stopping` drain never - // touches it. Sole clearing site (see the `stopRequested` invariant). - if !alive { stopRequested = false } + // stopRequested flag once the supervisor we asked to stop is GONE — + // process exit (kill(0) false), NOT pid-file removal — OR once a DIFFERENT + // pid is live (a new session started before we observed the stopped one's + // exit; without this the new session's Stop button stays wedged disabled). + // Placed AFTER the owned-proc early-return above so an owned session's + // `.stopping` drain never touches it. Sole clearing site for the success + // path (see the `stopRequested` invariant). + let stoppedTargetGone = + stopRequestedForPid != nil && pid?.pid != stopRequestedForPid + if !alive || stoppedTargetGone { + stopRequested = false + stopRequestedForPid = nil + } if alive { state = .running(ours: false) } else if case .failed = state { @@ -378,6 +393,10 @@ final class RecorderModel: ObservableObject { /// button is `.disabled(stopRequested)`). func stopExternal() { stopRequested = true + // Remember which recorder we're stopping so refresh() can clear the flag + // if a DIFFERENT session appears (see `stopRequestedForPid`). nil-safe: if + // the pid can't be read, refresh falls back to the plain `!alive` clear. + stopRequestedForPid = readPid(under: Self.resolveDataDir())?.pid flushNote = nil let p = Process() p.executableURL = URL(fileURLWithPath: cliPath) @@ -398,6 +417,7 @@ final class RecorderModel: ObservableObject { // A delivered SIGTERM (rc 0) leaves the flag set; refresh() // clears it when the supervisor actually exits. self?.stopRequested = false + self?.stopRequestedForPid = nil self?.flushNote = "Stop failed (rc \(proc.terminationStatus)) — see onoats-bot.log" } @@ -410,6 +430,7 @@ final class RecorderModel: ObservableObject { // the button (the one writer other than refresh(), guarded to the // spawn-failure path) so a missing/!executable CLI can be retried. stopRequested = false + stopRequestedForPid = nil flushNote = "Stop spawn failed: \(error.localizedDescription)" } } diff --git a/src/onoats/runtime.py b/src/onoats/runtime.py index 6ec8cd9..546e04b 100644 --- a/src/onoats/runtime.py +++ b/src/onoats/runtime.py @@ -1061,48 +1061,93 @@ def _pid_alive(pid: int) -> bool: _instance_lock_fd: int | None = None +def _refuse_if_live_recorder(pid_path: Path) -> None: + """Raise ``RecorderAlreadyRunningError`` if the pid file names a live recorder. + + The ``flock`` catches a concurrent SAME-version start, but a recorder from an + OLDER build holds no flock — so this no-write identity preflight (the same gate + ``flush``/``stop`` use: ``resolve_flush_target`` + the indeterminate-but-live + refusal) catches a live legacy/cross-version recorder the flock cannot. Run it + right after acquiring the flock and BEFORE any capture side effect, so a start + over a live orphan refuses before spawning the capturer / opening a device. + """ + from onoats._vendor.pid import read_pid_record, resolve_flush_target + + verified = resolve_flush_target(pid_path) + if verified.pid is not None and verified.pid != os.getpid(): + raise RecorderAlreadyRunningError( + f"An onoats recorder is already running (pid {verified.pid}). " + "Stop it first with `onoats stop`, then retry." + ) + # Indeterminate-but-live: a marker-valid file whose process is still alive but + # unverifiable (ps probe failed / legacy fingerprint-less). Refuse, exactly as + # flush/stop do; only ``stale=True`` is safe to clobber. + if verified.pid is None and not verified.stale: + rec = read_pid_record(pid_path) + if rec is not None and rec.pid != os.getpid() and _pid_alive(rec.pid): + raise RecorderAlreadyRunningError( + f"A recorder pid file names a live process (pid {rec.pid}) whose " + "identity could not be verified (ps probe failed / legacy pid " + "file) — refusing to start over a possibly-live recorder. Stop it " + "(`onoats stop`) or remove the stale pid file, then retry." + ) + + def _acquire_instance_lock(active_dir: Path) -> None: """Atomically claim the single-instance slot; raise if another holds it. - This is the gate that makes concurrent starts safe. ``_write_pid_file``'s - identity check is best-effort — it reads a possibly-absent pid file, so two - simultaneous starts with no valid pid file BOTH pass it. The ``flock`` here - lets exactly one win; the loser gets ``RecorderAlreadyRunningError`` before it - publishes a pid file or starts capturing. Held for the process lifetime via - the module-global fd; the kernel releases it on exit. As a bonus this enforces - the "no immediate stop→start" rule — ``stop`` returns on signal delivery, so a - chained start cleanly refuses until the draining recorder's process exits and - the kernel frees the lock. - - No-op on Windows (``flock`` unavailable); the pid-identity check remains the - only guard there. onoats is macOS-only in practice. + Two layered guards, BOTH run here so a losing start refuses before any capture + side effect (the call sites hoist this ahead of capturer spawn / device open): + + 1. ``flock(LOCK_EX|LOCK_NB)`` — the atomic gate. Of N concurrent SAME-version + starts exactly one wins; the rest raise ``RecorderAlreadyRunningError``. + 2. ``_refuse_if_live_recorder`` — a no-write identity preflight that catches a + live LEGACY/cross-version recorder (which holds no flock). Without this, a + start over a live legacy orphan would acquire the flock and proceed to spawn + the capturer, only refusing later in ``_write_pid_file``. + + Held for the process lifetime via the module-global fd; the kernel releases it + on exit (so there is no stale lock, and a chained ``stop`` then ``start`` + refuses until the draining recorder's process exits). On Windows ``flock`` is + unavailable, so the identity preflight is the only guard (onoats is macOS-only + in practice). """ global _instance_lock_fd - if sys.platform == "win32": - return - # Idempotent: one lock per process. If we already hold it, this is a nested - # acquire (the socket supervisor takes it before spawning the capturer, then - # the recorder's _write_pid_file/run_onoats_dual call it again) — return - # without re-acquiring, so there is never a release-then-reacquire gap. + # Idempotent: one lock per process. A nested acquire (the socket supervisor + # takes it before spawning the capturer, then the recorder's + # run_onoats_dual/_write_pid_file call it again) returns without re-acquiring — + # no release-then-reacquire gap, and the identity preflight runs exactly once. if _instance_lock_fd is not None: return active_dir.mkdir(parents=True, exist_ok=True) + pid_path = active_dir / PID_FILENAME + if sys.platform == "win32": + # No flock available — the identity preflight is the only guard. + _refuse_if_live_recorder(pid_path) + return lock_path = active_dir / LOCK_FILENAME fd = os.open(str(lock_path), os.O_RDWR | os.O_CREAT, 0o644) try: fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) except OSError as exc: os.close(fd) - # EWOULDBLOCK/EAGAIN → a live recorder already holds the slot. Read the - # pid file (if any) only to name it in the error — never to gate on. + # EWOULDBLOCK/EAGAIN → a live SAME-version recorder holds the slot. Read + # the pid file (if any) only to name it in the error — never to gate on. from onoats._vendor.pid import read_pid_record - rec = read_pid_record(active_dir / PID_FILENAME) + rec = read_pid_record(pid_path) who = f" (pid {rec.pid})" if rec is not None else "" raise RecorderAlreadyRunningError( f"An onoats recorder is already running{who} and holds the " "single-instance lock. Stop it first with `onoats stop`, then retry." ) from exc + # We hold the flock. Now refuse a live LEGACY recorder (no flock) before any + # capture side effect — releasing the flock we just took if we must refuse. + try: + _refuse_if_live_recorder(pid_path) + except BaseException: + os.close(fd) + raise _instance_lock_fd = fd @@ -1134,47 +1179,29 @@ def _release_instance_lock() -> None: def _write_pid_file(data_dir: Path) -> Path: """Write the current process PID, identity marker, and cmdline fingerprint. - Single-instance guard: refuses to start over a still-live recorder (raising - ``RecorderAlreadyRunningError``). The check reuses the same identity gate as - ``onoats stop``/``flush`` (``resolve_flush_target``: marker + cmdline - fingerprint + liveness), so a *positively stale* pid — dead, or recycled to a - foreign process (identity mismatch) — does NOT block a legitimate start. But a - marker-valid pid file naming a LIVE process whose identity cannot be verified - (``ps`` probe failed, or a legacy fingerprint-less file) is treated the same - way ``flush``/``stop`` treat that indeterminate state — refuse, never - overwrite — so a transient probe failure can't spawn a second recorder over a - live one. This closes the stop-then-immediate-start race (and its degraded - variants): without it, a second start would overwrite a draining recorder's - pid file, and the drainer would later unlink THIS start's file (see also the - ownership check in ``_remove_pid_file``). + Single-instance enforcement lives in ``_acquire_instance_lock`` (the flock + + ``_refuse_if_live_recorder`` identity preflight), which the capture entrypoints + call EARLY — before any capture side effect — and which this function also + calls as a backstop. By the time we publish a pid file we are the sole + instance and any pid file on disk is stale/dead, so the atomic replace below is + safe. The write itself is atomic (temp + ``os.replace``) and paired with the + ownership-checked ``_remove_pid_file`` so a draining recorder never deletes a + newer recorder's file. """ active_dir = data_dir / ".active" active_dir.mkdir(parents=True, exist_ok=True) pid_path = active_dir / PID_FILENAME - from onoats._vendor.pid import read_pid_record, resolve_flush_target - - verified = resolve_flush_target(pid_path) - if verified.pid is not None and verified.pid != os.getpid(): - raise RecorderAlreadyRunningError( - f"An onoats recorder is already running (pid {verified.pid}). " - "Stop it first with `onoats stop`, then retry." - ) - # Indeterminate-but-live: the resolver refused (pid is None) WITHOUT declaring - # the file stale — i.e. it is neither a verified recorder nor a proven-dead / - # recycled-foreign pid, but a marker-valid file whose process is still alive - # and merely unverifiable (ps probe failed / legacy fingerprint-less). Refuse, - # exactly as flush/stop do for the same state; only ``stale=True`` is safe to - # clobber. - if verified.pid is None and not verified.stale: - rec = read_pid_record(pid_path) - if rec is not None and rec.pid != os.getpid() and _pid_alive(rec.pid): - raise RecorderAlreadyRunningError( - f"A recorder pid file names a live process (pid {rec.pid}) whose " - "identity could not be verified (ps probe failed / legacy pid " - "file) — refusing to start over a possibly-live recorder. Stop it " - "(`onoats stop`) or remove the stale pid file, then retry." - ) + # Single-instance acquisition + identity preflight (universal backstop). The + # capture entrypoints acquire this EARLY — the socket supervisor before + # spawning the capturer, run_onoats_dual / run_onoats before opening a device — + # so by the time we publish a pid file the lock is already held and this call + # is an idempotent no-op. It stays here so any entrypoint that reaches pid + # publication without an earlier acquire is still gated. ``_acquire_instance_lock`` + # raises ``RecorderAlreadyRunningError`` for a concurrent (flock) OR live legacy + # (identity) recorder, so by here we are the sole instance and any pid file on + # disk is stale/dead — safe to atomically replace. Held until process exit. + _acquire_instance_lock(active_dir) existing = _read_pid_file(pid_path) if existing is not None: @@ -1189,14 +1216,6 @@ def _write_pid_file(data_dir: Path) -> Path: except PermissionError: logger.warning("PID file exists, process may be running as different user") - # Single-instance acquisition (universal backstop). The capture entrypoints - # acquire this EARLY — the socket supervisor before spawning the capturer, - # run_onoats_dual before opening PortAudio — so by the time we publish a pid - # file the lock is already held and this call is an idempotent no-op. It stays - # here so any entrypoint that reaches pid publication without an earlier - # acquire (e.g. `python -m onoats`) is still gated. Held until process exit. - _acquire_instance_lock(active_dir) - cmdline = _own_ps_cmdline() # Wall-clock start_epoch is included as the 4th line so live-view # readers can distinguish a freshly-started bot from one that diff --git a/tests/test_socket_supervisor.py b/tests/test_socket_supervisor.py index 50c09fd..8b3da53 100644 --- a/tests/test_socket_supervisor.py +++ b/tests/test_socket_supervisor.py @@ -672,6 +672,57 @@ async def _spy_exec(*a, **k): _os.close(holder) +def test_live_legacy_recorder_blocks_capturer_spawn(sup_env, monkeypatch): + """[high] regression (Codex round 7 / code-review): a LIVE recorder that holds + no flock (e.g. an older build that predates the lock) must still be refused + BEFORE the capturer is spawned. The identity preflight now runs inside + `_acquire_instance_lock` (`_refuse_if_live_recorder`), not late in + `_write_pid_file` after the capturer has touched CoreAudio/TCC. Seed a + marker-valid pid file naming a live process (no flock held); the supervisor + must refuse (rc=1) without calling `create_subprocess_exec`.""" + import asyncio as _asyncio + import shutil as _shutil + import subprocess as _subprocess + import sys as _sys + + if _sys.platform == "win32": + pytest.skip("flock single-instance lock is POSIX-only") + if not _shutil.which("sleep"): + pytest.skip("requires a real live process") + + from onoats._vendor.pid import PID_FILENAME + + data_dir, _pending = sup_env + active = data_dir / ".active" + active.mkdir(parents=True, exist_ok=True) + proc = _subprocess.Popen(["sleep", "30"]) # a live process to name in the file + try: + (active / PID_FILENAME).write_text( + f"{proc.pid}\nonoats-bot\nonoats bot\n0.0\n", encoding="utf-8" + ) + # Identity readback matches the stored fingerprint → resolves as verified + # live (no flock held, so only the identity preflight can refuse). + monkeypatch.setattr( + "onoats._vendor.pid._live_ps_cmdline", lambda pid: "onoats bot" + ) + spawned = {"n": 0} + real_exec = _asyncio.create_subprocess_exec + + async def _spy_exec(*a, **k): + spawned["n"] += 1 + return await real_exec(*a, **k) + + monkeypatch.setattr(_asyncio, "create_subprocess_exec", _spy_exec) + rc = _run_supervisor_bounded([]) + assert rc == 1, "a start over a live legacy recorder must exit rc=1" + assert spawned["n"] == 0, ( + "the capturer must NOT be spawned when a live recorder already exists" + ) + finally: + proc.terminate() + proc.wait(timeout=5) + + def test_recorder_handshake_failure_maps_to_clean_nonzero(log_sink, monkeypatch): """A controlled recorder launch failure must be rc=1, not a traceback. From fcb30b33f9573cad55f2dae3c9d684ffdf7e84fb Mon Sep 17 00:00:00 2001 From: Varun Singh <382354+vr000m@users.noreply.github.com> Date: Tue, 23 Jun 2026 17:59:21 -0700 Subject: [PATCH 18/18] fix(menubar): clear Stop latch on same-pid recycle (start-epoch discriminator) --- .../Sources/RecorderModel.swift | 54 +++++++++++++------ 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/native/onoats-menubar/Sources/RecorderModel.swift b/native/onoats-menubar/Sources/RecorderModel.swift index a1d22fa..c7d9575 100644 --- a/native/onoats-menubar/Sources/RecorderModel.swift +++ b/native/onoats-menubar/Sources/RecorderModel.swift @@ -80,13 +80,16 @@ final class RecorderModel: ObservableObject { /// use the `.stopping` enum case for external stops: `.stopping` is cleared /// by `handleExit`, which never fires without a `Process` handle. @Published var stopRequested = false - // The recorder pid `stopExternal()` asked to stop. `refresh()` clears - // `stopRequested` once THAT pid is gone — OR once a DIFFERENT pid is live (a - // new session replaced the one we stopped before we observed its exit), so a - // fresh session never inherits a stuck-disabled Stop button. nil when no - // external stop is in flight, or when the pid couldn't be read at stop time - // (then refresh falls back to the plain `!alive` clear). + // The recorder pid `stopExternal()` asked to stop, paired with that + // recorder's start epoch (pid-file line 4). `refresh()` clears `stopRequested` + // once THAT recorder is gone — its pid is no longer live — OR once a DIFFERENT + // recorder is live: a different pid, OR the SAME pid number with a different + // start epoch (a same-pid recycle — without the epoch check the new session + // would inherit a stuck-disabled Stop button). nil when no external stop is in + // flight, or when the pid couldn't be read at stop time (then refresh falls + // back to the plain `!alive` clear). private var stopRequestedForPid: Int32? + private var stopRequestedForStartEpoch: Double? /// Valid `[stt].service` values — mirror of runtime.py /// `VALID_STT_SERVICES` (parity-checked by @@ -169,7 +172,7 @@ final class RecorderModel: ObservableObject { /// Pid-file read, mirroring `_vendor/pid.py`: line 1 pid, line 2 must be /// the "onoats-bot" identity marker, else the file is ignored. - private func readPid(under dataDir: URL) -> (pid: Int32, cmdline: String)? { + private func readPid(under dataDir: URL) -> (pid: Int32, cmdline: String, startEpoch: Double)? { let path = dataDir.appendingPathComponent(".active/onoats.pid") guard let text = try? String(contentsOf: path, encoding: .utf8) else { return nil } let lines = text.trimmingCharacters(in: .whitespacesAndNewlines) @@ -177,9 +180,17 @@ final class RecorderModel: ObservableObject { .map { $0.trimmingCharacters(in: .whitespaces) } guard lines.count >= 2, lines[1] == "onoats-bot", let pid = Int32(lines[0]) else { return nil } - // Line 3 is the recorder's `ps -o command=` self-fingerprint (empty - // for legacy pid files) — mirror of _vendor/pid.py PidRecord. - return (pid, lines.count >= 3 ? lines[2] : "") + // Line 3 is the recorder's `ps -o command=` self-fingerprint (empty for + // legacy pid files) — mirror of _vendor/pid.py PidRecord. Line 4 is the + // wall-clock start epoch: the ONLY on-disk discriminator between two + // same-version recorders that land on the SAME pid number (identical + // cmdline), used by refresh() to clear `stopRequested` on a same-pid + // recycle. 0 for legacy/incomplete files. + return ( + pid, + lines.count >= 3 ? lines[2] : "", + lines.count >= 4 ? (Double(lines[3]) ?? 0) : 0 + ) } /// Cached fingerprint verdicts so the 1 s poll doesn't spawn `ps` every @@ -278,11 +289,19 @@ final class RecorderModel: ObservableObject { // Placed AFTER the owned-proc early-return above so an owned session's // `.stopping` drain never touches it. Sole clearing site for the success // path (see the `stopRequested` invariant). + // A DIFFERENT recorder is live: a different pid, OR the same pid number + // recycled onto a new session (distinguished by start epoch — two + // same-version recorders share a cmdline, so the epoch is the only + // discriminator). Either way the recorder we stopped is gone and the new + // one must not inherit a stuck-disabled Stop button. let stoppedTargetGone = - stopRequestedForPid != nil && pid?.pid != stopRequestedForPid + stopRequestedForPid != nil + && (pid?.pid != stopRequestedForPid + || pid?.startEpoch != stopRequestedForStartEpoch) if !alive || stoppedTargetGone { stopRequested = false stopRequestedForPid = nil + stopRequestedForStartEpoch = nil } if alive { state = .running(ours: false) @@ -393,10 +412,13 @@ final class RecorderModel: ObservableObject { /// button is `.disabled(stopRequested)`). func stopExternal() { stopRequested = true - // Remember which recorder we're stopping so refresh() can clear the flag - // if a DIFFERENT session appears (see `stopRequestedForPid`). nil-safe: if - // the pid can't be read, refresh falls back to the plain `!alive` clear. - stopRequestedForPid = readPid(under: Self.resolveDataDir())?.pid + // Remember which recorder we're stopping (pid + start epoch) so refresh() + // can clear the flag if a DIFFERENT session appears — including a same-pid + // recycle (see `stopRequestedForPid`). nil-safe: if the pid can't be read, + // refresh falls back to the plain `!alive` clear. + let stopping = readPid(under: Self.resolveDataDir()) + stopRequestedForPid = stopping?.pid + stopRequestedForStartEpoch = stopping?.startEpoch flushNote = nil let p = Process() p.executableURL = URL(fileURLWithPath: cliPath) @@ -418,6 +440,7 @@ final class RecorderModel: ObservableObject { // clears it when the supervisor actually exits. self?.stopRequested = false self?.stopRequestedForPid = nil + self?.stopRequestedForStartEpoch = nil self?.flushNote = "Stop failed (rc \(proc.terminationStatus)) — see onoats-bot.log" } @@ -431,6 +454,7 @@ final class RecorderModel: ObservableObject { // spawn-failure path) so a missing/!executable CLI can be retried. stopRequested = false stopRequestedForPid = nil + stopRequestedForStartEpoch = nil flushNote = "Stop spawn failed: \(error.localizedDescription)" } }