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.