Skip to content

VC-834: background Change Set/rail/venue/diff reads run read-only git (stacked on #885 — merge after it) - #909

Merged
hussainph merged 3 commits into
mainfrom
volli/VC-834-background-change-set-rail-venue-git-status-read
Oct 19, 2026
Merged

hussainph merged 3 commits into
mainfrom
volli/VC-834-background-change-set-rail-venue-git-status-read

Conversation

@hussainph

Copy link
Copy Markdown
Owner

Stacked on #885 (VC-292) — merge after it. This branch starts at #885's head (ae3ecdfec), so until #885 merges the diff also shows its commits. VC-834's own change is the last commit, VC-834: background worktree reads run read-only git.

Defect

git status holds the checkout's index.lock for its whole run, untracked scan included, so it can write back the stat data it refreshed (git-status(1), "BACKGROUND REFRESH"). The orphan scan already avoids this with readOnlyGitAsync (--no-optional-locks). The reads that run most often did not:

  • changeSetSnapshot / changeSetPaths: refetched within ~1 s of any write in the worktree
  • getWorktreeStatus: the rail, on every mount and watch event
  • venueSnapshot: also runs on the Main checkout that Board Sessions share
  • diffStat

So an agent's own git add/commit, or a pre-commit hook's git add after a formatter rewrote files, could race Volli's background status -uall and fail with index.lock: File exists.

Verified locally with real git 2.48.1 (not only by reading code). I paused a plain git status --porcelain=v2 -z -uall mid-way through its untracked scan. While it was paused, .git/index.lock existed and a concurrent git add -u failed with Unable to create '…/index.lock': File exists. With --no-optional-locks there was no lock and the same git add succeeded.

Fix

Each of the five entry points wraps its runner once in the existing readOnlyGitAsync (scan.ts). Every git call they make, including the helpers they pass git to (comparison-ref resolution, the sequencer probe), leads with --no-optional-locks. User-initiated mutations (commit.ts, sync.ts, worktree add/remove) and dirty.ts are untouched: the confirmed-delete probe is out of scope per the ticket, and the scan's probe was already read-only. No retries were added.

Test-only: scriptedGit gains an opt-in scriptedReadOnlyGit variant that strips the leading switch before the handler and the recorded calls see the args. That lets the existing suites keep dispatching on the verb and asserting exact argv unchanged.

How the regression test proves it

packages/host-core/src/worktree/read-only-reads.test.ts:

  1. argv (the scan.test.ts:171 shape): for changeSetSnapshot, changeSetPaths, getWorktreeStatus, venueSnapshot and diffStat (both modes), every recorded arg list starts with --no-optional-locks.
  2. real git: the reads run serialized against a real repo. Right before each status they issue, a tracked file is made stat-dirty (same bytes, new mtime). That is exactly what a plain status refreshes under index.lock and writes back. The test asserts all five status invocations leave .git/index byte-identical. A control test shows a plain status does rewrite it, so the fixture can see a lock being taken.

On the base (fix stashed), 7 of 8 fail (the control passes); with the fix, 8/8 pass.

The ticket asked for a concurrent-git add race test. I swapped it for this deterministic write check because a timing race would be flaky in CI. The concurrency itself was shown by hand, as described above.

For the maintainer to decide

  • Porcelain git diff still takes index.lock briefly. builtin/diff.c's refresh_index_quietly() locks and writes the index at the end of a working-tree diff whenever it found stat-dirty entries. It is gated by diff.autoRefreshIndex, not by --no-optional-locks: I checked this in git master and saw it on 2.48.1. That window is much shorter than status's (refresh + write, not the untracked walk), but it is the same symptom. Closing it needs either diff.autoRefreshIndex=false on the read runner (localGitArgs admits no -c today) or a move to plumbing diff-index. Both are outside this ticket's scope, so they are not done here. The real-git test measures only status and says why.
  • trim.ts trackedChangeRefusal (status --porcelain inside a trim) is in the ticket's evidence list but not in its proposed fix, so it is unchanged.
  • Open PR VC-659: adopt stacked ticket branches; name why a start's attach failed #879 adds a status --porcelain read in reconcile.ts (uncommittedChanges) that should take the read-only form too. It is not on this branch's base.

Checks run locally

  • vp test run on: read-only-reads, change-set, status, venue, diff, read, ensure, snapshot, git-regressions, git-parity, scan, sync, publish, watch, worktree-runtime, handlers/host-handlers, agent-commands, agent-dispatch. All green.
  • vp check: pass. vp run --filter @volli/host-core typecheck: pass.
  • None of the touched source files is on a 100% coverage list.

…cks)

The Change Set snapshot and path set, the rail status, the venue snapshot
and diffStat ran a plain git status, which holds the checkout's index.lock
for its whole untracked scan. These run in the background within a second
of any write, so an agent's own git add/commit (or a pre-commit hook's
git add) in the same checkout could fail with 'index.lock: File exists'.
Each entry point now wraps its runner in readOnlyGitAsync, the orphan
scan's existing wrapper. Mutations (commit, sync, worktree add/remove)
are unchanged.
@coderabbitai

coderabbitai Bot commented Oct 18, 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: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: d4a9c826-d930-49ef-8a4b-e7d9bef5911b
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Resolve agent-commands.test.ts: keep main's dropped sync seam
({ gitAsync } only) and this branch's scriptedReadOnlyGit. Move the two
new VC-802 conflicts fixtures that drive changeSetPaths onto
scriptedReadOnlyGit, since their handlers dispatch on the verb.
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.

1 participant