Locks from killed runs are never reclaimed, permanently blocking a feature from review

[youhaowei]

Summary

A clawpatch review that dies without unwinding (SIGKILL, terminal closed, CI job cancelled, OOM) leaves its per-feature lock file behind. Nothing ever reclaims it. Every later run that selects that feature fails with feature locked: <id>, and because reviewCommand treats the claim failure as fatal, the whole run exits 7 — even when every other feature reviewed cleanly.

The lock file records the information needed to detect this and does not use it:

{
  "lockedByRunId": "20260728T051054-7e273e",
  "lockedAt": "2026-07-28T05:12:08.397Z",
  "hostname": "some-host.local",
  "pid": 29345
}

What we hit

A review of 12 features reviewed 11 and then failed:

clawpatch review feature-error index=3 total=12 feature=feat_config_89dc9a6aee elapsed=0s error=feature locked: feat_config_89dc9a6aee
clawpatch review failed run=20260728T053559-069b53 errors=1
error: feature locked: feat_config_89dc9a6aee

The holder was a run killed earlier that day. Inspecting the lock directory found five stale locks, the oldest two weeks old; all five pids were dead, and all five recorded the local hostname. The eleven successful reviews were still charged for, and the one feature that mattered most for that branch was the one blocked.

Why clean-locks is not the answer

clean-locks exists, but cleanLocksCommand (app.js:232-249) clears every lock unconditionally — no liveness check, no age check:

for (const feature of features) {
    if (feature.lock === null) continue;
    await writeFeature(loaded.paths, { ...feature, status: ..., lock: null, ... });
}

So it cannot be run automatically or from a wrapper: doing so while another review is in flight would steal that run's locks and let two processes review the same feature concurrently. It is only safe when the operator knows nothing else is running — which means recovery from a crashed run is a manual step that the user has to first diagnose from an exit 7.

Suggested fix

Treat a lock as stale when it is provably not held, using what the lock already records:

• hostname matches the current host and pid is not alive → reclaim it automatically when claiming.
• Otherwise (different host, or process alive) → leave it, and keep the current conflict error.

That makes the common single-machine case self-healing without weakening the cross-machine guarantee. A lockedAt-based TTL would be a reasonable secondary backstop for the cross-host case, ideally configurable.

Two smaller improvements that would help independently:

1. Give clean-locks a safe mode — --stale-only, applying the liveness rule above, so it can be run automatically by wrappers and CI.
2. Don't discard a whole run for one locked feature. A lock conflict on one feature is closer to a per-feature error than a fatal run error; reporting it alongside the successful reviews (and exiting non-zero) would preserve the work already paid for.

Reproduction

1. Start clawpatch review on a repo with several features.
2. kill -9 the process while a feature is claimed.
3. Run clawpatch review again — it fails with feature locked: <id> and exit 7, indefinitely, until clean-locks is run manually.

Environment

clawpatch 0.7.1 (latest at time of filing), macOS, node global install.

[clawsweeper]

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Reviewed August 1, 2026, 9:42 PM ET / August 2, 2026, 01:42 UTC.

Summary
Keep open: current main still treats any existing feature lock as an immediate conflict and marks the review run failed, while the full cleanup command clears every lock without checking host or PID. The explicitly linked open repair PR at #170 owns this bounded fix, so a second repair branch would duplicate active work.

Reproducibility: yes. —source establishes the reported path: an existing lock file produces an immediate lock-conflict error, and the review command treats that error as fatal. This read-only review did not run the destructive SIGKILL fixture itself.

Next step
The open linked PR already owns the narrow repair; do not queue a duplicate fix PR while it is awaiting review and merge.

Review details

Best possible solution:

Review and land the linked PR with focused stale-lock regression coverage, preserving live local and remote locks; after it merges, the issue can close through the linked relationship.

Do we have a high-confidence way to reproduce the issue?

Yes—source establishes the reported path: an existing lock file produces an immediate lock-conflict error, and the review command treats that error as fatal. This read-only review did not run the destructive SIGKILL fixture itself.

Is this the best way to solve the issue?

Yes—the linked PR's same-host, dead-PID reclamation is the narrowest maintainable repair because it preserves locks held by live processes and unknown remote hosts.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4a5028c27f01.

Label changes

Label justifications:

• P1: A killed review process can block later review runs for an affected feature until an operator intervenes.
• impact:other: The issue affects review-workflow availability and recovery rather than data loss, message delivery, session state, or provider routing.

Evidence reviewed

What I checked:

• Current lock claim has no stale-lock check: Current main creates the lock file atomically and returns feature locked immediately on an existing file; it does not inspect the recorded hostname or PID before rejecting the claim. (src/state.ts:84, 4a5028c27f01)
• Lock conflicts fail the review run: The review loop records claim failures as errors, and any error other than finding-validation drops makes the run status failed and throws the first error. (src/review.ts:155, 4a5028c27f01)
• Current cleanup is indiscriminate: cleanLocksCommand clears every feature lock and then deletes every lock file, without a local-host or PID liveness test. (src/app.ts:269, 4a5028c27f01)
• Released behavior remains unchanged on main: The lock implementation is attributed by blame to the v0.7.1 release commit, and git log v0.7.1..HEAD -- over the lock, review, CLI, and specification paths returned no subsequent changes. git tag --contains confirms that commit shipped as v0.7.1. (src/state.ts:84, f6e00010cc2a)
• Open PR owns the repair: The linked PR explicitly closes this issue and describes the same narrow rule: reclaim only a dead PID on the current host while retaining live and remote locks; its supplied real CLI fixture proof also covers clean-locks --stale-only. (df92a88b798f)
• Maturity scorecard unavailable in this checkout: The required shortlist helper could not locate qa/maturity-scores.yaml, so no stable-maturity label is asserted.

Likely related people:

• Peter Steinberger: Current lock claim, cleanup, and review-path lines are attributed to the v0.7.1 release commit, which is the available provenance for the current implementation. (role: current lock-path contributor; confidence: medium; commits: f6e00010cc2a; files: src/state.ts, src/app.ts, src/review.ts)

How this review workflow works

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.