refactor(jsonutil): centralize streaming atomic replacement - #2053
Open
MuskanPaliwal wants to merge 7 commits into
Open
refactor(jsonutil): centralize streaming atomic replacement#2053MuskanPaliwal wants to merge 7 commits into
jsonutil): centralize streaming atomic replacement#2053MuskanPaliwal wants to merge 7 commits into
Conversation
Sessions spawned outside a hooked terminal (e.g. by an external session host) never get a hook-cached export under .entire/tmp, so resolveAndValidateTranscript gave up: PrepareTranscript is only called when the transcript file already exists, and OpenCode has no SessionBaseDirProvider fallback search. OpenCode can materialize any session's transcript via opencode export, tracked or not. Add a TranscriptFetcher capability (agent can fetch a transcript on demand), implement it on OpenCodeAgent via fetchAndCacheExport, and consult it in resolveAndValidateTranscript after the initial stat fails, before the project-dir search fallback. Refs entireio#1876
…cached file runOpenCodeExportToFile wrote `opencode export` output straight into <repo>/.entire/tmp/<id>.json with O_TRUNC and removed the file when the export failed. That file is often the only local copy of the session: the turn-end hook writes it, and nothing condenses it into a checkpoint until the user commits. Both callers re-export over a possibly-populated path — PrepareTranscript on every turn end (cli/lifecycle.go) and FetchTranscript on attach — so a missing `opencode` binary, a rejected session, or a 30s timeout destroyed the transcript it was asked to refresh, and attach reported "transcript not found" for a session whose transcript it had just deleted. Stage the export in a sibling temp file and rename it over the target only on success, mirroring makeInstallTmpPath in cli/plugin_store.go. The temp name starts with "." and does not end in ".json" so .entire/tmp scanners (state files, pre-task files) skip it, and os.Rename replaces the destination on both POSIX and Windows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
detectAgentByTranscript probes every registered agent through resolveAndValidateTranscript, so making OpenCode a TranscriptFetcher meant any failed attach — of any agent — ran `opencode export <that-session-id>` as a side effect of detection: a subprocess with a 30s timeout ceiling, plus a MkdirAll of <repo>/.entire/tmp, during what reads as a read-only probe. agent.List() is alphabetical, so opencode was probed before pi and vogon. This is the same reason PrepareTranscript is gated behind an os.Stat (see the comment there about Cursor's 3s poll); agent-side materialization is strictly more expensive than a flush. Thread an explicit transcriptLookup through resolveAndValidateTranscript: lookupAllowFetch for the agent the user named, lookupLocalOnly while probing the others. Because detection no longer exports on the user's behalf, the transcript-not-found error now names the agents that could have, so someone who omitted or mistyped --agent gets the one-flag fix instead of "is the session ID correct?". TestAttach_TranscriptNotFound: 1.05s -> 0.13s. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
76c5d8e staged the export and renamed on success, but fetchAndCacheExport's json.Valid gate still ran AFTER the rename, so it only ever protected the non-zero-exit path. An `opencode export` that exits 0 with truncated or empty output still replaced the cached transcript — the exact failure the staging exists to prevent. That variant is worse than a missing file: attach's os.Stat branch accepts whatever is at the path and treats PrepareTranscript's failure as best-effort (attach.go:817), so a corrupt transcript is used silently, where a missing one falls through to a re-fetch. Move staging up into fetchAndCacheExport so the order is export → read → json.Valid → rename. The runner goes back to a plain write and gains the fsync that makes the caller's rename durable (see jsonutil.WriteFileAtomic: without it some filesystems surface the rename as complete while the file is still empty after a crash). A failed rename now keeps the validated export and names its path, instead of deleting a transcript we may be the last holder of. Because the export runner is the stubbed seam, every stub-based test now exercises staging: partial-write-then-error, truncated-with-exit-0, and empty-with-exit-0 all fail against 76c5d8e and pass here. The previous success-path test passed against the pre-fix code and pinned nothing; it is replaced. Windows: os.Rename there is MoveFileEx(MOVEFILE_REPLACE_EXISTING), which must delete the destination, and Go opens files without FILE_SHARE_DELETE — so a concurrent reader of the transcript makes the replace fail where the older in-place write succeeded. renameOverExisting retries briefly on a sharing violation and otherwise reports the contended file. POSIX rename(2) is unaffected, so isRenameContention is build-tagged to a constant false there. clean: .entire/tmp now holds transient files, so a name can vanish between listAllTempFiles' snapshot and deleteTempFiles' Remove. ENOENT is no longer counted as a deletion failure, which would have surfaced as a spurious "failed to delete N item(s)" and a non-zero exit from `entire clean --all`. Also fixes two pre-existing test-isolation bugs in this package, both of which wrote into the developer's own repository: TestPrepareTranscript_ErrorOnBrokenSymlink ran the real `opencode` binary there (its premise was wrong — os.Stat follows the link and reports ENOENT, so the guard never fires and it falls through to the export), and TestPrepareTranscript_AlwaysRefreshesTranscript staged into the real .entire/tmp for want of a t.Chdir. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MuskanPaliwal
marked this pull request as ready for review
August 19, 2026 10:49
MuskanPaliwal
changed the title
jsonutil): centralize streaming atomic replacement
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #1877. Until that PR merges, review the stacked range
3a39273b..7c62491a5.This change moves OpenCode’s staged transcript publication into
jsonutil.WriteFileAtomicStream. The previous implementation kept temporary-file creation, validation, rename retries, and recovery inside OpenCode even though most of that lifecycle is general atomic-write behavior. The shared implementation keeps publication ordered as produce, sync, close, validate, chmod, rename, and best-effort directory sync.Core Changes
WriteFileAtomicStreamnow owns the staging file from creation through publication. Producer and validator errors are returned unchanged. Any failure before publication leaves the existing destination untouched and makes a best-effort attempt to remove the staging file. If a completed and validated file cannot be renamed into place, the helper returns aPublishErrorand transfers ownership of the retained staging path to the caller so the output can be recovered.Windows replacement contention is handled inside
jsonutilrather than OpenCode. Only access-denied and sharing-violation failures are retried, with five total attempts and cancellation-aware waits. The follow-up review found a cancellation window between retries: cancellation could occur after a wait, but the next rename could still replace the destination. The retry loop now checks the context before every rename attempt, retaining the validated staging file if cancellation wins.OpenCode continues to translate publication failures into its existing actionable “export saved at …” message. Its package-specific staging, rename, and contention helpers have been removed because those responsibilities now belong to the shared atomic-write implementation.
API Notes
This adds the public
jsonutil.WriteFileAtomicStreamhelper and the publicjsonutil.PublishErrorownership contract. A caller receivingPublishErrorowns the validated file atStagedPathand must either recover or remove it.The existing
jsonutil.WriteFileAtomicfunction uses the same private engine but keeps its previous behavior. Rename failures do not exposePublishError, and its staging files are still cleaned up instead of being retained.Compatibility / Release Notes
OpenCode command invocation, timeout and error classification, mock export mode, transcript validation, attach behavior, transcript protocol, and declared capabilities are unchanged. Failed, partial, empty, invalid, and cancelled exports cannot replace an existing valid transcript.
No unrelated temporary-file or rename-based writers are migrated in this change because they may have different locking, permission, confinement, executable, or cross-filesystem requirements.
This branch is stacked on #1877 and should be rebased onto
mainafter that PR merges.Testing
mise run checkjsonutiland OpenCode