177 lines
8.1 KiB
Markdown
177 lines
8.1 KiB
Markdown
# Adversarial Bug Report: add-state-loop-lock
|
||
|
||
Adversarial probing of the `_loop_lock` implementation. Each attack vector is
|
||
hypothesized, then tested (or static-analyzed for non-testable cases). Verdict
|
||
shown against each.
|
||
|
||
## A1 — Concurrent `--approve --loop` race
|
||
|
||
**Hypothesis**: With 5 concurrent `--approve --loop` invocations on a halted
|
||
loop, more than one might pass the `status != "halted"` check before any of
|
||
them writes the cleared state, double-incrementing `resumed_count`.
|
||
|
||
**Test**: `/tmp/loop-lock-adv` — set status=halted, spawn 5 concurrent
|
||
`status.py --approve --loop` subprocesses simultaneously.
|
||
|
||
**Result**:
|
||
```
|
||
codes: [1, 1, 1, 1, 0]
|
||
outputs: 4× "ERROR: loop 'adv1' is in status 'running', not 'halted'..."
|
||
1× "Approved loop 'adv1'. Halt cleared. Resumed count: 1"
|
||
final state: status=running, resumed_count=1
|
||
```
|
||
|
||
**Verdict**: PASS — exactly one approve won; 4 others re-read inside the lock
|
||
and saw `status=running`, returning 1 with the "not halted" error. resumed_count
|
||
incremented exactly once. The lock serializes approves correctly.
|
||
|
||
## A2 — Concurrent `--pause-loop` race
|
||
|
||
**Hypothesis**: With 5 concurrent `--pause-loop` invocations on a running
|
||
loop, all 5 succeed (since pause is idempotent — `state["status"] != "paused"`
|
||
fails open). resumed_count shouldn't be touched by pause anyway.
|
||
|
||
**Test**: Spawn 5 concurrent `status.py --pause-loop adv1`.
|
||
|
||
**Result**: all 5 returned code 0 with "Paused loop..." message; final
|
||
state: status=paused (consistent). No `resumed_count` touched (pause
|
||
doesn't increment it).
|
||
|
||
**Verdict**: PASS (no race) — but note pause is idempotent and re-writes
|
||
paused state even when already paused. Each writer holds the lock
|
||
sequentially and re-writes the same value. Wasteful but consistent. Not a
|
||
bug.
|
||
|
||
## A3 — Lock release on mid-tick exception
|
||
|
||
**Hypothesis**: If the runner's `_gate` subprocess or any code inside the
|
||
`with _loop_lock` block raises, the OS-level flock is held forever, stalling
|
||
all future ticks and pause/approve commands.
|
||
|
||
**Test**: Monkeypatch `_gate` to raise `RuntimeError`, invoke
|
||
`cmd_tick(args)`, catch the exception. Verify a follow-up `_loop_lock`
|
||
acquire succeeds immediately (<1s elapsed).
|
||
|
||
**Result**:
|
||
```
|
||
caught: simulate gate crash
|
||
re-acquire elapsed: 2.5e-05 s
|
||
PASS — lock released on exception
|
||
```
|
||
|
||
**Verdict**: PASS — `finally` block in `_loop_lock` runs on exception exit
|
||
of the `with` body, releases the flock and closes the FD. No resource leak.
|
||
|
||
## A4 — Harness calling loop-control commands from inside a tick (theoretical deadlock)
|
||
|
||
**Hypothesis**: The runner holds `_loop_lock` across the harness subprocess
|
||
(Implement/Verify/Orchestrate). If the harness transitively invokes
|
||
`status.py --pause-loop` / `--resume-loop` / `--approve --loop` / `--check-gate`
|
||
(without `AUTOMATON_NO_LOOP_LOCK=1` env var — which is only set in the
|
||
runner's own `_gate` call, not in harness subprocess env), that nested
|
||
status.py would acquire `_loop_lock` → block waiting for the runner's parent
|
||
lock → runner waits for harness to return → harness waits for its
|
||
subprocess → subprocess waits for parent lock → DEADLOCK.
|
||
|
||
**Test**: Not run live (would hang the entire test session). Static analysis
|
||
of harness-integration contract:
|
||
- Harnesses invoked via `harness.command` are described in
|
||
`design/loops/technical.md` §8 as LLM-driven agents (opencode, aider, Pi
|
||
Dev, generic). They invoke `status.py` for task-level transitions
|
||
(`--transition`, `--can-edit`, `--task`, `--scope-check`) per the
|
||
`contracts/harness-integration.md` requirement. Task commands do NOT touch
|
||
`.state.lock` (only loop commands do).
|
||
- No known harness in scope (opencode/aider/Pi Dev) calls `--pause-loop` /
|
||
`--approve --loop` inside a tick. The orchestrator might inspect loop
|
||
state but doesn't write to it.
|
||
- The orchestrator prompt (`prompts/orchestrate.md` etc.) is invoked by the
|
||
runner AFTER the verify verdict is parsed; it's expected to call
|
||
`--transition <task>` based on the verdict, not loop commands.
|
||
|
||
**Severity**: LOW. Hypothetical; no known harness hits this. The
|
||
harness-integration contract should explicitly forbid harness invocations of
|
||
loop-control commands during a tick.
|
||
|
||
**Mitigation documented**: Per `_loop_lock`'s docstring and per SPEC D-L1
|
||
("ticks short; operator notices via `--loop-list` stale `last_tick_at`"),
|
||
ticks are expected to complete in seconds; an operator noticing a wedged tick
|
||
would `kill` the runner process, releasing the OS flock. The deadlock
|
||
surface area is small and mitigated by operator-wedge-detection.
|
||
|
||
**Recommendation**: Add a note to `contracts/harness-integration.md`
|
||
explicitly listing loop-control commands (`--pause-loop`, `--resume-loop`,
|
||
`--approve --loop`, `--check-gate`) as FORBIDDEN inside a tick's harness
|
||
subprocess. Not a blocker for this task — defer to a small docs-only follow-up.
|
||
|
||
## A5 — Manual `AUTOMATON_NO_LOOP_LOCK=1` disables all `status.py` locking
|
||
|
||
**Hypothesis**: An operator who sets `$AUTOMATON_NO_LOOP_LOCK=1` in their
|
||
shell and runs `--pause-loop` etc. bypasses the lock entirely, re-opening the
|
||
TOCTOU race that A1/A2 verified is closed.
|
||
|
||
**Test**: Not run live (requires manual env var setup; covered by code-level
|
||
audit). The env-var bypass is unconditional inside `_loop_lock` for the
|
||
status.py helper; there's no check that the bypass is actually being
|
||
invoked by a trusted caller.
|
||
|
||
**Severity**: LOW. Documented as an escape hatch in `_loop_lock`'s
|
||
docstring; only the runner sets it, and only in the `_gate` subprocess env
|
||
(scoped, not global). A malicious or careless shell user could
|
||
circumvent, but they're effectively "running alternative middleware" at
|
||
that point — no different from killing the runner.
|
||
|
||
**Verdict**: PASS — escape hatch is documented; same trust boundary as the
|
||
"shell user can override anything" assumption.
|
||
|
||
## A6 — `.state.lock` left on disk after crash
|
||
|
||
**Hypothesis**: If the runner is killed mid-tick (SIGKILL or power loss),
|
||
the `.state.lock` file is left on disk. A subsequent tick's `os.open`
|
||
re-uses the orphaned file (with `O_RDWR | O_CREAT`). The
|
||
`fcntl.flock` on the new FD succeeds (the previous flock was associated
|
||
with a now-closed FD; the kernel auto-releases flocks on FD close /
|
||
process exit). No wedged lock.
|
||
|
||
**Test**: Not run live (would require killing the runner mid-tick). Static
|
||
analysis: POSIX `flock` is per-FD-per-process; the OS auto-releases the
|
||
flock when the holding process exits. So orphaned `.state.lock` files are
|
||
dead bytes, not live locks.
|
||
|
||
**Verdict**: PASS — the orphan-file situation is benign. Documented in
|
||
`_loop_lock`'s docstring ("not garbage-collected").
|
||
|
||
## A7 — NFS loop dir causes different flock semantics
|
||
|
||
**Hypothesis**: If the project dir (and therefore `.automaton/loops/<n>/`)
|
||
is on an NFS mount, `fcntl.flock` semantics differ — flock may be
|
||
advisory-only or behave unpredictably.
|
||
|
||
**Test**: Not run live (no NFS available). Acknowledged in `_loop_lock`'s
|
||
docstring: "NFS caveat: `flock` semantics differ on NFS-mounted loop dirs.
|
||
The loop dir is documented to be local (project root or `~/.automaton`)."
|
||
|
||
**Verdict**: Documented assumption per SPEC "Risks" section; not a bug.
|
||
|
||
## A8 — Cyclomatic complexity of cmd_tick jumped with the indent
|
||
|
||
**Hypothesis**: Wrapping cmd_tick's body in `with _loop_lock(loop_path):`
|
||
plus re-read state inside increases cyclomatic complexity and re-indent
|
||
churn, making future maintenance error-prone.
|
||
|
||
**Test**: Not run live. Static analysis: the wrap is a single
|
||
context-manager level; the body retains its original structure inside.
|
||
Re-indent added 4 columns to all lines inside the with block (visible in
|
||
git diff), but no control-flow change beyond the re-read.
|
||
|
||
**Verdict**: PASS — function shape is preserved; the only new control flow
|
||
is the early-return on `state is None` retry inside the with block. The
|
||
.SMALL cost is offset by the correctness gain.
|
||
|
||
## Verdict
|
||
|
||
**No BLOCKERS found.** All hypotheses either verified-safe (A1, A2, A3, A6,
|
||
A8, A7), or theoretical-low-severity (A4, A5) with documented mitigations.
|
||
|
||
Recommend proceeding to doc_review. A4's recommendation (harness-contract
|
||
docs note about loop-control commands inside a tick) is a follow-up
|
||
improvement, not a blocker for v1.1. |