microsoft / microsoft/vscode

Agent host: set GIT_OPTIONAL_LOCKS=0 on read-only git probes to avoid worktree-removal races

Open
#329,893 0 comments 0 reactions 1 assignee Claimed by @DonJayamanne View on GitHub
Dominant language
TypeScript
Stars
193k
Forks
42.4k
PR merge metrics
PR metrics pending

Description

## Summary

Our read-only git probes in the agent host (`git status` / `git diff`) run **without** `GIT_OPTIONAL_LOCKS=0`, so they opportunistically take `index.lock` in the working directory. For a **worktree-isolated** session the working directory is a linked worktree, and that lock lives in `.git/worktrees//`. When a session is disposed, `git worktree remove` deletes the working tree and then `rmdir`s the admin directory `.git/worktrees/` — which fails with **`Directory not empty`** if one of our probes re-created `index.lock` there concurrently.

We should set `GIT_OPTIONAL_LOCKS=0` on our **read-only** git invocations (status/diff), matching the existing repo idiom in the built-in git extension. This deterministically removes *our* contribution to the race, without any cross-service cancellation/quiesce plumbing.

## Observed failure

Agent Host E2E — **Claude** provider — failed in the Mocha `after each` teardown of the test `worktree materialization copies configured ignored files`:

```
AggregateError: Failed to dispose Agent Host E2E test resources: Failed to release Agent Host E2E test resources:
Error: git worktree exited with code 255: error: failed to delete '.git/worktrees/reply-exactly-materialized': Directory not empty
at .../out/vs/platform/agentHost/node/agentHostGitService.js:625:20
at ...
at Context. (.../out/vs/platform/agentHost/test/node/e2e/suites/agentHostE2ESuites.js:116:15)
```

The **test body passed**; only teardown failed. Conformance, Codex, and **Copilot** passed the same suite (Copilot runs the same test — both providers set `supportsWorktreeIncludeFilesE2E: true`).

## Root cause

`git worktree remove` deletes the working tree, then `rmdir`s `.git/worktrees/`. `Directory not empty` means a concurrent git process re-created a per-worktree file (`index.lock` → `index`) after git emptied it.

For this test the dominant, **controllable** writers are our own read-only probes, all of which take `index.lock` because none pass `GIT_OPTIONAL_LOCKS=0`:

- Git-state refresh (fire-and-forget on materialize/subscribe/turn-complete/watcher) → `git status -b --porcelain=v2` in the worktree (`src/vs/platform/agentHost/node/agentHostGitService.ts` `_computeSessionGitState`).
- Session subscription → branch changeset refresh → `computeSessionFileDiffs` → `git status --porcelain=v1` + `git diff --raw` in the worktree.
- `onTurnComplete` **unconditionally** schedules branch + session changeset recomputes (`agentHostChangesetService.ts`), also fire-and-forget git.

The prompt in the failing test is a trivial `Reply exactly "materialized".` (no tool calls), so the agent itself is unlikely to spawn git — pointing at our own probes as the writer. The Claude-fails / Copilot-passes split is a teardown-await asymmetry: Copilot `disposeSession` awaits SDK disconnect (draining fire-and-forget writers), while Claude aborts and races.

## Proposed change

Set `GIT_OPTIONAL_LOCKS=0` (via the existing `env` option on `_runGit`) on our **read-only** git invocations:

- `git status` / `git diff` in `_computeSessionGitState` (status `-b --porcelain=v2`)
- `computeSessionFileDiffs` (`status --porcelain=v1`, `diff --raw`)
- `getUntrackedPaths` / `captureWorkingTreeAsTree` status probes
- other read-only `diff` helpers

**Do NOT** apply it to mutating operations (`commitAll`, `restore`, `update-index`); the temp-index write paths already isolate via `GIT_INDEX_FILE`.

This is the existing house idiom — see `extensions/git/src/git.ts` (`const env = { GIT_OPTIONAL_LOCKS: '0' }`).

## Why a separate issue

The **immediate correctness fix** — making the worktree-removal retry backstop honest (it currently masks failures because `git worktree prune` exits 0 even when it fails to delete the admin dir, and success wrongly clears `_worktreeDeletionRetries`) — is being handled separately. `GIT_OPTIONAL_LOCKS=0` is the deterministic root-cause reduction for our own probes and is tracked here so it can land as a focused, low-risk change.

## Rejected alternatives

- **Quiesce / cancel fire-and-forget git on dispose** — over-engineered and non-deterministic as scoped: clearing coordinator maps is a no-op for in-flight git; a per-session git-state drain is mis-/under-scoped (unguarded re-trigger from `onTurnComplete`/watcher; keyed by `session.toString()` vs removal's `AgentSession.id`); and awaiting the Claude subprocess can't guarantee an orphaned/reparented `git` grandchild is gone.
- **`rm -rf` the admin dir / delete a live `index.lock`** — bypasses git bookkeeping and risks index corruption; races the same writer.

## Acceptance criteria

- [ ] Read-only agent-host git probes (`git status`, read-only `git diff`) run with `GIT_OPTIONAL_LOCKS=0`.
- [ ] Mutating git operations are unchanged (no `GIT_OPTIONAL_LOCKS=0`).
- [ ] Worktree materialization E2E (Claude + Copilot) is stable across repeated runs.

## Notes

- Follow-up (non-blocking): await the Claude SDK subprocess exit in `disposeSession` for symmetry with Copilot; add Windows/`EBUSY`/`EPERM` retryable signatures to worktree-removal error classification.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.