Skip to content

fix(lock): re-check the config under the lock; never break a lock mid-write (PHARN-03) - #197

Merged
PrzemekGalarowicz merged 1 commit into
mainfrom
claude/bold-archimedes-5czyyn
Sep 24, 2026
Merged

PrzemekGalarowicz merged 1 commit into
mainfrom
claude/bold-archimedes-5czyyn

Conversation

@PrzemekGalarowicz

Copy link
Copy Markdown
Member

What this changes

This fixes two holes in the single-writer project lock (.pharn.lock).

(a) Changes planned against an outdated config. add, update and remove load pharn.config.json before they take the lock, and update and the remove picker also show a confirm prompt in between. Reproduced: a remove picker was confirmed after a concurrent add had finished. The remove then wrote the config it had loaded earlier, so the added capability disappeared from the config while its files and records stayed on disk. Both commands exited 0.
→ Each of the three commands now calls assertConfigUnchanged as its first step inside the lock. If the config changed, it throws ProjectChangedError, a subclass of ProjectLockedError, so every existing catch site already reports it as a named refusal: exit 1, nothing written.

(b) A live lock broken while its owner is still writing it. tryCreate creates the lock file with O_EXCL and writes the payload in a second step, so a live lock is briefly empty. isStale(null) treated an empty file as stale, so a concurrent run could break a lock that was in use.
→ A lock file that cannot be parsed and is younger than MALFORMED_GRACE_MS (10 s) now counts as live. Older ones are still broken, so a corrupt lock cannot block the project for long.

Evidence from the review's race harness (8 processes started in the same millisecond, 150 rounds), outside the test suite:

Build Rounds with 2+ simultaneous lock holders
base 26/150
this PR 0/150

Docs: README.md ("One writer at a time") and CLAUDE.md.

Built with /pharn-dev-ship; stage artifacts are in .dev/features/lock-read-modify-write/. Results:

  • validate: exit 0
  • regress: no-regressions
  • verify: PASS

Type of change

  • feat — new stack option, wizard step, or command capability
  • fix — bug fix
  • docs — docs-only change
  • chore / refactor — tooling or internal restructure, no behavior change

Area(s) touched

lib/project-lock | lib/pharn-config | commands/add, update, remove | docs

Checklist

  • Read the existing file(s) before editing; followed the ESM .js-extension import convention.
  • Updated the matching tests. 8 new cases fail on the old source.
  • Updated the relevant docs.
  • Preserved the security invariants in src/lib/validate.ts.

Quality gates

  • npm run check passes locally (1303/1303 tests; non-root user, node 22).
  • npm run build succeeds (left to CI).
  • npm run test:coverage passes (left to CI).

Notes for the reviewer

  • Nine existing lock tests plant a malformed lock with a fresh timestamp, which would now count as live, so they now backdate it past the 10 s grace window. That matches what they are simulating: a lock left behind by a crashed or hand-edited run.
  • I also wrote a test that spawns real processes and races them for the lock. At a size CI can afford (6 processes × 12 rounds) it did not catch the race even on the old code, so I dropped it rather than ship a test that proves nothing.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc


Generated by Claude Code

…-write (PHARN-03)

Two ways the single-writer lock let a stale plan land:

(a) `add`/`update`/`remove` load pharn.config.json BEFORE taking the lock (and
    `update` / the `remove` picker prompt in between). A `remove` confirmed
    after a concurrent `add` rewrote its stale snapshot, dropping the added
    capability while its files and records stayed. Each now calls
    `assertConfigUnchanged` first inside the lock and refuses with
    `ProjectChangedError` (a `ProjectLockedError`: named refusal, exit 1,
    nothing written).

(b) `tryCreate` writes the payload after the O_EXCL create, so a live lock is
    briefly empty; `isStale(null)` read that as stale and a concurrent run
    broke it (8 processes x 150 rounds: 26 rounds with 2+ holders). An
    unparseable lock younger than MALFORMED_GRACE_MS (10 s) is now live
    (0/150 after the fix).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc
@coderabbitai

coderabbitai Bot commented Sep 24, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: fc814a99-efef-4549-9a18-03b16aa60815


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@PrzemekGalarowicz
PrzemekGalarowicz merged commit 71bc375 into main Sep 24, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants