automaton/tasks/add-state-loop-lock/CODE_REVIEW.md

# Code Review: add-state-loop-lock

Reviewed implementation against `tasks/add-state-loop-lock/SPEC.md`.

## SPEC coverage

| Requirement | Status |
|-------------|--------|
| R1 — `_loop_lock` context manager with POSIX/Windows branches, blocking acquire, FD lifecycle in `finally` | ✓ (added in `status.py` AND `loop-runner.py`) |
| R2 — Wrap `_write_state_loop` callsites in `status.py` (not `--create-loop`) | ✓ (`cmd_pause_loop`, `cmd_resume_loop`, `cmd_approve_loop`, `cmd_check_gate`); `--create-loop` per D-L3 left unwrapped |
| R3 — Wrap read-modify-write in `cmd_tick`'s step 10; lock must cover the `--check-gate` subprocess decision and the state write | ✓ — runner holds `_loop_lock` from before `_gate` through step 10's write. The `_gate` subprocess is invoked with `AUTOMATON_NO_LOOP_LOCK=1` so its own `_loop_lock` no-ops (avoids self-deadlock on the parent's held flock) |
| R4 — Idempotence: early-returns inside the `with` release cleanly (try/finally inside the context manager, not caller) | ✓ — the `finally` block in `_loop_lock` checks `acquired` and unlocks; safe on early returns |
| R5 — Lock file location is per-loop dir | ✓ — `lock_file = loop_path / ".state.lock"` |
| R6 — Stdlib only (fcntl/msvcrt/contextlib/sys/os) | ✓ — `import contextlib`, conditional `import fcntl` (POSIX) / `import msvcrt` (Windows), `os.open`, `os.close` |
| R7 — Existing atomic write (`_write_state_loop` tmp-then-replace) retained | ✓ — `_write_state_loop` untouched; lock is coarse mutex on top |

## Deviations from SPEC (with rationale)

1. **SPEC R2 listed `cmd_check_gate` as a callsite to wrap, but R3 said the runner must hold the lock across the gate subprocess.** These contradict: if both wrap, runner holds flock, spawns `--check-gate`, subprocess tries to flock the SAME file → deadlock. Resolved by introducing D-L6 (env-var bypass). `cmd_check_gate` acquires `_loop_lock` — but when `$AUTOMATON_NO_LOOP_LOCK=1` is set in the subprocess env (the runner sets it ONLY for the `--check-gate` subprocess's env), `_loop_lock` becomes a no-op. Standalone CLI invocations don't set the env var, so they lock normally and still serialize against `--pause-loop` etc.

   This deviates from the SPEC wording by adding an env-var mechanism not listed in the SPEC, but the SPEC's stated intent ("the lock acquired by the runner blocks the *runner's own* subsequent subprocess read... cannot lock the subprocess itself. This is acceptable: the lock scope we control is the parent runner's read-modify-write; a concurrent tick would block on `.state.lock` at the parent-runner level") is preserved exactly. The env var is the mechanism that achieves the SPEC's stated intent without deadlock.

2. **SPEC R3 mentioned loop-runner.py callsite line 684 for `_write_state_loop`.** The actual line is 690 in the pre-task tree (787 in the post-task tree). The cmd_tick wrapping covers all four `_write_state_loop` callsites in the runner (the early `_ensure_worktree` write at line 244, the `_halt_loop` writes for context-floor and verifier-fail, and the final step-10 state write). All are inside `cmd_tick`'s `with _loop_lock` block, so they're all covered by the single outer lock.

3. **`_read_state_loop` is invoked before the lock in `cmd_tick`** (to fast-fail untracked loops without paying the lock cost), then re-read inside the lock. This is **not** a race — the unlocked read only determines whether the loop is untracked; subsequent decisions re-read fresh under the lock. Documented in cmd_tick's docstring.

## Helpers audit (avoiding nested `_loop_lock`)

- `_halt_loop` (status.py:1718): does write + log + `_disable_schedule`. None re-acquire the lock. Called from `cmd_check_gate` while the lock is held — safe.
- `_halt_loop` (loop-runner.py:122): same shape, but doesn't call `_disable_schedule` (runner is short-lived per tick; OS schedule unit is best-effort disabled elsewhere). Called from `cmd_tick` while the lock is held — safe.
- `_ensure_worktree`: does subprocess `git` + state write. Doesn't lock. Called from `cmd_tick` inside `_loop_lock` — safe.
- `_disable_schedule` / `_enable_schedule` (status.py): now called OUTSIDE the `_loop_lock` block (after the `with` exits) in all three commands — keeps the critical section tight. They invoke OS shells (launchctl, cron, schtasks) and don't touch `.state.loop`. Safe.

## Cross-script duplication

`_loop_lock` is duplicated across `status.py` and `loop-runner.py`. This is consistent with the existing convention (`_read_state_loop`, `_write_state_loop`, `_read_loop_config`, etc. are all duplicated across the two scripts; the design doc explicitly says "no cross-script imports"). `status.py`'s version adds the env-var bypass; `loop-runner.py`'s does not (the runner is the lock holder, never the bypass consumer).

## Race-window closure confirmation

Scenarios the lock closes:

1. Two scheduler firings of the same loop → second runner blocks at `_loop_lock` until first finishes step 10. ✓
2. Concurrent `--pause-loop` and runner tick → pause blocks at the runner's lock; pause resumes after tick releases. ✓
3. Concurrent `--approve --loop` and runner tick → same as #2.
4. Concurrent `--check-gate` (CLI) and `--pause-loop` (CLI) → both acquire the lock, serialize. ✓
5. Concurrent `--check-gate` invoked from runner (env var set) and `--pause-loop` → runner holds the lock; pause blocks at runner's lock. ✓
6. Concurrent `--approve --loop` from harness (no env var) and a runner tick → harness's approve blocks at runner's lock. ✓ (Test 7 covers this scenario.)

## Edge cases verified

- Untracked loop: cmd_tick returns early before acquiring the lock — no `.state.lock` is created for untracked loops on tick.
- Empty `.state.loop`: not possible — `_read_state_loop` returns None on JSON decode failure; treat as untracked.
- `.state.lock` file pre-existing from a previous crash: `_loop_lock` opens with `O_RDWR | O_CREAT` — re-uses existing file. Idempotent.
- Loop dir deleted mid-hold: `BrokenPipeError`/`OSError` from writes would surface; documented as acceptable per SPEC.

## Test review

- `TestSerializeConcurrent`: relies on a `threading.Lock` to append enter/exit times safely. Good. Could be flaky on extremely slow CI; threshold is `last_enter >= first_exit` which is monotonic — not a wall-clock assertion. Robust.
- `TestPerLoop`: 1.0s upper bound on B's acquire while A holds a different lock. Could be flaky on a heavily loaded box, but 1s is generous. Acceptable.
- `TestNoLockOnCreate`: tests D-L3 — `--create-loop` does NOT create `.state.lock`; first `--check-gate` does. Excellent regression guard.
- `TestPauseSerializedWithConcurrentHolder`: relies on `--pause-loop`'s subprocess spawning (~100ms Python startup) plus the holder's 100ms hold. Asserts `"Paused loop"` is in the output. Doesn't strictly assert wall-clock > 100ms (the comment admits this). The monotonic ordering check (results["code"] is 0) plus the implicit blocking-on-flock suffice as a smoke test. Could be tightened to assert `results["elapsed"] >= 0.05` (the holder held for >=0.1s, minus subprocess startup), but the smoke-level assertion is adequate for v1.1.
- `TestRunnerHoldsLockAcrossStateWrite`: cleaner than the SPEC's "Skip if it grows flaky" suggestion — uses `threading.Event` synchronization rather than wall-clock delays for the critical assertions; wall-clock sleeps only to allow the approve subprocess to spin up. Robust. The key assertion is `assert "approve_out" not in results` BEFORE `tick_can_finish.set()` — proves the approve subprocess is blocked on flock while the tick is mid-flight.

## Test count

- Baseline: 440 passed (post-`fix-harness-command-template`).
- New: +7 in `tests/test_state_loop_lock.py`.
- Final: **447 passed**, 0 regressions.

## Verdict

PASS. Proceed to bug_find.