[Arieg419]

Issue for this PR

Closes #60447

Type of change

• Bug fix

What does this PR do?

Recover transient shared snapshot-index contention on v1. Retry only Git's index-lock collision, with six interruptible backoffs totaling 3.15 seconds plus command time. Keep the other process's lock intact, and return no capture when staging or write-tree fails instead of returning a stale tree.

The real contention test also exposed uncaught stdin EPIPE errors when Git exited before reading its pathspec. Keep the stdin error listener through stream closure, preserve exit codes/stderr and typed write failures, and settle premature closure without hanging. Caller-managed stdin stays open when requested.

How did you verify your code works?

Reproduced transient contention, stale-tree reporting, and the standalone small-stdin/early-exit crash before fixing them. Final local checks with Bun 1.4.4:

• Core process/spawner suites: 52 passed, 189 assertions.
• Snapshot, processor and snapshot/tool integration suites: 76 passed, 1 existing skip, 828 assertions.
• Both package bun typecheck commands passed; git diff --check passed.
• Independent correctness review reran all seven new contracts successfully. Both reviews have no unresolved Critical or Important findings: correctness, integration.

Tests use real Git/child processes and temporary fixtures on macOS. Windows was not executed locally. This is bounded retry, not a cross-process transaction across all snapshot operations. Existing ignored-file error handling is outside this change. No active coordination files are affected.

Screenshots / recordings

Nonvisual exemption: process and snapshot control flow only; no GUI or styling changes.

Draft head 6a9f25e93d7618d09fa5c233a89154b25165d104 is 0 commits behind freshly fetched origin/v1 and GitHub reports MERGEABLE/BLOCKED. Required test and typecheck workflows are action_required, awaiting maintainer approval. The publisher has read-only upstream permission. This is not a CI-pass or readiness claim.

Checklist

• I have tested my changes locally
• I have not included unrelated changes in this PR
• Reproductions and regression coverage include lock ownership, exhaustion, stale captures and stdin errors
• Both independent reviews posted and findings resolved
• Required hosted CI passed on the current head

[Arieg419]

Independent GPT-6.1 Sol correctness review. Local paths are redacted; findings and verification limits are preserved.

Snapshot contention correctness review

Verdict: no remaining Critical or Important source finding in the reviewed four-file diff. This is a source/contract review, not PR readiness or CI approval.

Checkout: isolated tool-snapshot worktree
Base: 006d837 (origin/v1)
Reviewed diff SHA256: 69401ffa1fce70b163918afe4f87b76694d444b784921439aab2f689927638f5
Owned scope: packages/opencode/src/snapshot/index.ts, packages/opencode/test/snapshot/snapshot.test.ts, packages/core/src/cross-spawn-spawner.ts, packages/core/test/process/process.test.ts. Lifecycle processor/race tests are not part of this PR review.

Correctness conclusions

• Git retries are limited to exit128 plus index.lock plus C-locale File exists. Seven attempts use six interruptible waits totaling3.15s, excluding command time. Other fatal errors do not enter the retry branch. LC_ALL applies to each Git subprocess; no global locale/config modification.
• No code deletes another process's index lock. A real external Git holder is used by tests; bounded exhaustion leaves both its lock and process alive.
• Failed candidate listing or git add now propagates false, causing track to return undefined before write-tree. Failed write-tree also returns undefined with diagnostics. The required clean-filter contract proves no stale previous tree and exactly one unrelated-fatal invocation using a marker, without the earlier brittle3s timing assertion.
• The stdin expansion addresses an observed EPIPE crash rather than hiding test failures. A per-writable typed-error listener remains until native close. NodeSink runs with endOnDone:false and explicit ending is enabled only when caller configuration requested it. Pending end settles on finish, typed error, or typed premature-close failure. The callback removes its finish/close listeners on settlement/interruption; the error keeper is removed on that exact writable's close. There is no global handler or unconditional error swallowing.
• Caller-managed endOnDone:false remains open over multiple writes. Producer failures and active write failures remain typed failures. Early child exit retains exit128 and stderr. Cancellation remains interruptible; no finalizer waits for close before the process finalizer can kill the child, so no close/kill dependency cycle was introduced.

Findings resolved during review

1. Initial focused lock tests failed with uncaught Node EPIPE on both ambient Bun1.4.2 and pinned1.4.4. A holder double-end was guarded, but failure persisted. Writer then independently reproduced it in AppProcess tiny-stdin/early-exit calls and fixed the actual stream-listener lifetime. Broad-suite success alone was not treated as a rebuttal.
2. The first explicit-end wrapper awaited finish/error only. Native stdin.destroy during pending end can emit close alone. Writer captured that real Node event sequence and added typed premature-close settlement plus listener cleanup. This closes the identified hang path.
3. Unrelated-fatal test previously inferred no retry from a3s timeout against3.15s backoff. It now checks a required filter's invocation marker exactly equals x.

Independently executed verification

Pinned Bun1.4.4, isolated HOME/OPENCODE_TEST_HOME/XDG config/data/state/cache, inert local tests only:

• New snapshot3contracts:3pass,0fail,10assertions; exit0. opencode-snapshot-review-final-20261030.log (local diagnostic)
• New stdin4contracts after the final close guard:4pass,0fail,108assertions; exit0. opencode-snapshot-core-review-close-final-20261030.log (local diagnostic)
• Effect interruption probe: an inner Effect.catch does not convert Effect.interrupt into success; Exit remains Failure with Interrupt cause. This supports the snapshot helper's unchanged interruption handling.
• git diff --check: exit0.

Also inspected writer's current core process/spawner output:52pass,0fail,189assertions across2files. The writer reports a fresh broad snapshot/lifecycle run after the close guard:76pass,1skip,0fail,828assertions across3files. Lifecycle fixtures are QA-only and remain outside the PR scope.

Limits and remaining checks

• macOS local verification only; Windows behavior was reviewed but not executed here.
• No live OpenCode runtime, user configuration, historical session database or research path was modified. Tests ran against temporary fixtures. No commits or external writes were made by this reviewer.
• The earlier duplicate-workspace package-identity typecheck failure was resolved in task-owned dependency setup. Writer reports exit0 for both package typechecks, and both final logs contain only tsgo --noEmit with no diagnostics: opencode-tool-snapshot-final-typecheck-20261030.log (local diagnostic) and opencode-core-process-final-typecheck-20261030.log. (local diagnostic) These are writer-run checks; the focused tests above were independently rerun by this reviewer.
• This patch retries a shared index; it does not create a cross-process transaction spanning every snapshot command. Existing ignored-file drop/check-ignore failure handling predates the diff and was not expanded here.

[Arieg419]

Independent GPT-6.1 Sol integration review. Local paths are redacted; findings and verification limits are preserved.

Snapshot and stdin integration review

Reviewer: GPT-6.1 Sol routing/usage worker, independent of the snapshot implementation.
Review date: 2026-10-30.
Role: simplicity, integration, lifecycle and portability review.
Base: 006d837c5022ea5278162307c04c78412cfae9f3 (OpenCode v1).
Reviewed diff SHA-256: 69401ffa1fce70b163918afe4f87b76694d444b784921439aab2f689927638f5.

Result

No Critical or Important findings in the four-file diff. No source edits made.

Evidence considered

• packages/opencode/src/snapshot/index.ts: retry requires exit 128 plus both the specific index.lock path fragment and File exists; LC_ALL=C makes that diagnostic comparison stable. Seven total attempts have six waits of 50, 100, 200, 400, 800 and 1600 milliseconds (3150 milliseconds total sleep). Unrelated errors return immediately. The code does not remove another process's lock or kill its holder. The pre-existing conversion of process errors into a diagnostic GitResult remains unchanged.
• track now stops after unsuccessful staging or write-tree instead of returning stale or empty tree evidence. This is a small change at the acquisition boundary, without changing snapshot storage identity or introducing a new locking scheme.
• packages/core/src/cross-spawn-spawner.ts: stdin setup occurs once per spawned handle. The late-error listener owns the Writable until close and then removes itself. Finish/close listeners for the explicit end phase are cleaned up by the Effect callback. Existing caller-managed endOnDone=false semantics remain open; the end phase is added only for endOnDone=true. Producer failures and typed stdin write failures remain observable rather than being broadly swallowed.
• New tests use real temporary child processes and Git locks, exercise eventual release and persistent contention, retain another holder, verify captured content, and verify unrelated filter failures are not retried. Windows has an explicit cmd.exe branch for early-child diagnostics; other process fixtures use the current Bun executable. Git filter commands execute through Git's shell contract. The timeout margins are bounded and no portability defect was identified by source review.

Verification limits

This review inspected source, call sites and regression assertions. It did not rerun the author's suites or independently measure Windows execution. Root supplied current evidence of 52 core tests, 76 OpenCode tests with one skip, and both package typechecks passing; those counts are author/root evidence, not new runs by this reviewer. Existing snapshot helper behavior outside the changed track/staging path was not expanded into a separate audit.