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/AGENTS.md b/AGENTS.md index 9294853..357aa9c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -107,6 +107,83 @@ 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`, 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 + 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`. + +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, 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*, 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 + shutdown would free the slot while the supervisor is still tearing down its + capturer). POSIX-only (no-op on Windows; macOS product). +- **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 + 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 When a change spawns a child process (`create_subprocess_*` / `Popen` / `exec`) diff --git a/CHANGELOG.md b/CHANGELOG.md index ae18423..cbf6936 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,52 @@ 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. +- 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`. 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*, `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 && + 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; (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. + ## [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..65ec022 100644 --- a/README.md +++ b/README.md @@ -108,6 +108,10 @@ Other subcommands: ```bash onoats bot-single # legacy mic-only recorder onoats flush # tell the running recorder to rotate its buffer now +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 # 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 new file mode 100644 index 0000000..cc2ce79 --- /dev/null +++ b/docs/dev_plans/20260619-bug-stoppable-orphan-session.md @@ -0,0 +1,539 @@ +# 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 + +- [x] Phase 1: `onoats stop` CLI subcommand (Python) +- [x] Phase 2 (code): Menu-bar Stop rewiring (Swift) — impl complete + compiles clean (`make build/Onoats`) +- [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 + +### 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 +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 +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 +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 +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 +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 +[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). **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. + +### 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_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 + 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 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 + +### 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` + +### 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. +- **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 + +[Fill when complete] 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..c7d9575 100644 --- a/native/onoats-menubar/Sources/RecorderModel.swift +++ b/native/onoats-menubar/Sources/RecorderModel.swift @@ -68,6 +68,29 @@ 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. 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 + // 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 /// tests/test_native_contract_parity.py). @@ -149,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) @@ -157,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 @@ -250,6 +281,28 @@ 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 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). + // 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 + || pid?.startEpoch != stopRequestedForStartEpoch) + if !alive || stoppedTargetGone { + stopRequested = false + stopRequestedForPid = nil + stopRequestedForStartEpoch = nil + } if alive { state = .running(ours: false) } else if case .failed = state { @@ -316,6 +369,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() @@ -340,6 +397,68 @@ 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 + // 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) + 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 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?.stopRequestedForPid = nil + self?.stopRequestedForStartEpoch = nil + 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 + stopRequestedForPid = nil + stopRequestedForStartEpoch = nil + 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 diff --git a/src/onoats/__main__.py b/src/onoats/__main__.py index b00f00c..6d0e15c 100644 --- a/src/onoats/__main__.py +++ b/src/onoats/__main__.py @@ -57,8 +57,10 @@ from onoats.runtime import ( # noqa: E402 BOT_NAME, PIPELINE_SAMPLE_RATE, + RecorderAlreadyRunningError, SHUTDOWN_CANCEL_TIMEOUT_SEC, SttPreflightError, + _acquire_instance_lock, _create_stt_service, stop_pipeline_for_shutdown, wait_or_force, @@ -121,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 @@ -130,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) # ---------------------------------------------------------------- @@ -385,7 +395,9 @@ 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()) + # The single-instance lock is held for the process lifetime; the kernel + # releases it on exit (see runtime._release_instance_lock). # --------------------------------------------------------------------------- @@ -434,7 +446,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 d04214b..137ff63 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 @@ -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 @@ -1114,12 +1126,36 @@ 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. 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( @@ -1130,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 @@ -1150,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, @@ -1166,6 +1202,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 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 — and only + # if it STILL records the stale pid we resolved (compare-and-unlink). + if target.stale: + _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 + 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. + _compare_and_unlink_stale_pid(pid_path, pid) + 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 +1397,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 +1417,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/src/onoats/dual.py b/src/onoats/dual.py index 229cc1c..5477be2 100644 --- a/src/onoats/dual.py +++ b/src/onoats/dual.py @@ -42,15 +42,17 @@ from onoats.runtime import ( # noqa: E402 BOT_NAME, PIPELINE_SAMPLE_RATE, + RecorderAlreadyRunningError, SHUTDOWN_CANCEL_TIMEOUT_SEC, SttPreflightError, stop_pipeline_for_shutdown, wait_or_force, - _remove_pid_file, + _acquire_instance_lock, _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 +65,51 @@ ) +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; 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( + 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, owner_pid=os.getpid()) + # 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: """Best-effort drain for STT services used by the dual-input path. @@ -353,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() @@ -558,30 +613,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: @@ -732,7 +774,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..546e04b 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 @@ -33,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 @@ -56,6 +58,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 @@ -1020,12 +1037,172 @@ 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 + + +# 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 _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. + + 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 + # 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 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(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 + + +def _release_instance_lock() -> None: + """Release the single-instance lock if held (no-op otherwise). + + 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: + 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.""" + """Write the current process PID, identity marker, and cmdline fingerprint. + + 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 + # 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: try: @@ -1044,18 +1221,65 @@ 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})" ) 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, 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 != 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() logger.debug(f"PID file removed: {pid_path}") 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_cli.py b/tests/test_cli.py index 0bb245f..07f0ea3 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,259 @@ 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 + + +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) diff --git a/tests/test_socket_supervisor.py b/tests/test_socket_supervisor.py index 816b889..8b3da53 100644 --- a/tests/test_socket_supervisor.py +++ b/tests/test_socket_supervisor.py @@ -626,6 +626,103 @@ 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_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. @@ -1964,3 +2061,92 @@ 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 + + +# --------------------------------------------------------------------------- +# 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 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. +# --------------------------------------------------------------------------- + + +@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 + + 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. `_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 = dual._remove_pid_file + + 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, **kwargs) + + monkeypatch.setattr(dual, "_remove_pid_file", _spy_remove) + + # 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, + ) + + # 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 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 == expected_reason + assert end_rec.audio_source == "socket", ( + "stop must preserve start-of-session detail" + ) diff --git a/tests/test_status_file.py b/tests/test_status_file.py index 6958a3a..55cfae3 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,254 @@ 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 +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, +) + +# 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: + 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() + + +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) + + +@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}" + + +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)