fix(session): make transcript replacements crash-safe - #2094
Conversation
Complete transcript rewrites could truncate live JSONL files before preserved data was durable, risking unrecoverable resume history after an interrupted write. Commit replacements through exclusive sibling temp files and serialize them with all transcript append paths so readers observe either the old file or the complete replacement.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe change adds atomic file replacement, transcript locking, ordered session-storage rewrites, atomic hydration, and speculation-accept persistence. It also adds tests for failure handling, concurrency, symlink behavior, modes, and transcript ordering. ChangesTranscript persistence
Estimated code review effort: 5 (Critical) | ~90 minutes 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/PromptSuggestion/speculation.ts (1)
799-803: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNon-blocking: the
.catchhandler can no longer observe a write failure.
recordSpeculationAcceptcallsgetProject().appendEntry(entry). Forspeculation-accept,appendEntryrunsvoid this.enqueueWrite(sessionFile, entry)and returns once the entry is queued (src/utils/sessionStorage.tsLine 1800). The promise settles before the file write happens.The previous implementation appended directly, so this handler reported real write failures. Now a failed write is reported by the drain through
logError, and this debug message is unreachable for that case.Either drop the handler, or change the message so it does not claim a transcript write failed.
♻️ Proposed change
void recordSpeculationAccept(entry).catch(() => { logForDebugging( - '[Speculation] Failed to write speculation-accept to transcript', + '[Speculation] Failed to queue speculation-accept for the transcript', ) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/PromptSuggestion/speculation.ts` around lines 799 - 803, Update the recordSpeculationAccept call in the speculation-accept flow because its promise only confirms enqueueing, not completion of the transcript write. Either remove the unreachable catch handler or revise its log message so it describes failure to queue/record the speculation acceptance rather than claiming the transcript write failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/atomicReplace.test.ts`:
- Around line 100-122: Add focused tests in the atomic replacement test suite
covering both untested options: verify replaceFileAtomic uses the full flush
path when passed flush: 'full', and verify preserveMode: false applies 0o600 to
an existing target instead of retaining its current mode. Keep the cases short
and follow the existing Windows guard and mode assertions.
In `@src/utils/sessionStorage.atomicReplace.test.ts`:
- Around line 494-498: The atomic-replace tests use fragile wall-clock sleeps
for drain completion. Update the waits around removalResult and the
corresponding wait near line 531 to use setTranscriptRewriteHooksForTesting or
an explicit flushSessionStorage await; if retaining timeouts, make them
substantially longer than FLUSH_INTERVAL_MS and document the dependency.
- Around line 322-339: Guard the symlink-and-mode test named “middle tombstone
preserves restrictive mode and follows the live symlink target” by returning
immediately when process.platform is win32, matching the existing guards in
atomicReplace.test.ts. Keep the Unix-specific symlink creation and permission
assertion unchanged for other platforms.
- Around line 244-246: Lower the writeCount threshold assertion in the streaming
test near stat(filePath) so it verifies multiple bounded writes without
depending on the current 64 KiB chunk size; retain the existing size and
suffix-content assertions.
In `@src/utils/sessionStorage.ts`:
- Around line 2227-2246: The foreground hydration paths must reject empty
fetched transcripts before replacing the local file. In
src/utils/sessionStorage.ts lines 2227-2246, update the flow after
getSessionLogs to return false when remoteLogs.length is zero, before
replaceTranscriptFile; apply the same guard to events before
replaceTranscriptFile at lines 2295-2300, matching the existing
subagentEvents.length > 0 behavior.
- Around line 1500-1503: In the tombstone rewrite catch near the best-effort
handling, add a debug log using the existing logForDebugging utility, including
the caught error and enough context to identify the failed rewrite. Preserve the
current swallowing behavior and surrounding atomic replacement logic.
- Around line 1428-1499: Revalidate the transcript size immediately before each
replaceFileAtomic call in the tombstone-removal flow, including the tail-span
and slow-path branches. Compare the fresh stat size with the original fileSize;
if it changed, abort the replacement (or otherwise preserve the newly appended
data) instead of passing stale bounds to streamTranscriptRanges. Use the
existing sessionFile and file-handle/stat mechanisms without changing truncation
behavior.
---
Outside diff comments:
In `@src/services/PromptSuggestion/speculation.ts`:
- Around line 799-803: Update the recordSpeculationAccept call in the
speculation-accept flow because its promise only confirms enqueueing, not
completion of the transcript write. Either remove the unreachable catch handler
or revise its log message so it describes failure to queue/record the
speculation acceptance rather than claiming the transcript write failed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 537b6387-afd6-453f-b458-94ca7c8e0f41
📒 Files selected for processing (5)
src/services/PromptSuggestion/speculation.tssrc/utils/atomicReplace.test.tssrc/utils/atomicReplace.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/sessionStorage.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/services/PromptSuggestion/speculation.tssrc/utils/atomicReplace.test.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/sessionStorage.tssrc/utils/atomicReplace.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/services/PromptSuggestion/speculation.tssrc/utils/atomicReplace.test.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/sessionStorage.tssrc/utils/atomicReplace.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/PromptSuggestion/speculation.tssrc/utils/atomicReplace.test.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/sessionStorage.tssrc/utils/atomicReplace.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/atomicReplace.test.tssrc/utils/sessionStorage.atomicReplace.test.ts
🔇 Additional comments (15)
src/utils/atomicReplace.ts (3)
78-111: LGTM!
124-169: LGTM!
198-285: LGTM!src/utils/atomicReplace.test.ts (1)
47-61: LGTM!Also applies to: 155-240
src/utils/sessionStorage.ts (8)
194-349: LGTM!
375-439: LGTM!
946-1010: LGTM!
1027-1045: LGTM!
1073-1200: LGTM!
1382-1397: LGTM!
2306-2365: LGTM!
1066-1071: 🗄️ Data Integrity & IntegrationNo change needed:
_deferSynchronousAppenddoes not create lost deferred appends on shutdown.When
reAppendSessionMetadatareaches an active rewrite barrier, it queues a direct append.flush()waits throughactiveDrainand allwriteQueues, so a process that waits onflushSessionStorageafter that metadata append cannot exit before the enqueue.> Likely an incorrect or invalid review comment.src/utils/sessionStorage.atomicReplace.test.ts (2)
116-164: LGTM!
166-229: LGTM!Also applies to: 341-411, 543-618, 659-717
src/services/PromptSuggestion/speculation.ts (1)
3-3: LGTM!Also applies to: 48-48
Abort tombstone commits when the scanned transcript changes before replacement, and keep existing local history when remote foreground hydration returns no entries. Harden the associated portability, option coverage, queue timing, and diagnostics.
Pass the explicit optional subagent reader in the empty-hydration regression so a fresh TypeScript build sees the complete helper signature.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/atomicReplace.ts`:
- Around line 263-269: The freshness check and destructive mutation are not
coordinated, allowing external writers to append between validation and
replacement or truncation. Introduce a cooperative per-target writer lock or
revision protocol spanning snapshot, copy, freshness validation, and mutation;
update src/utils/atomicReplace.ts lines 263-269 and src/utils/sessionStorage.ts
lines 429-432 so all transcript writers, including handle.truncate(boundary),
hold the same coordination mechanism. Add a deterministic regression test that
appends after freshness validation but before the destructive operation and
verifies the append is preserved.
In `@src/utils/sessionStorage.atomicReplace.test.ts`:
- Line 689: Update the setInternalEventReader call in the test setup to provide
both required reader callbacks, adding an empty subagent reader while preserving
the existing main reader that returns an empty array.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1a9d3e6d-5df4-4b66-9ca8-40760197411f
📒 Files selected for processing (5)
src/services/PromptSuggestion/speculation.tssrc/utils/atomicReplace.test.tssrc/utils/atomicReplace.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/sessionStorage.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/sessionStorage.atomicReplace.test.tssrc/services/PromptSuggestion/speculation.tssrc/utils/atomicReplace.tssrc/utils/atomicReplace.test.tssrc/utils/sessionStorage.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/sessionStorage.atomicReplace.test.tssrc/services/PromptSuggestion/speculation.tssrc/utils/atomicReplace.tssrc/utils/atomicReplace.test.tssrc/utils/sessionStorage.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/sessionStorage.atomicReplace.test.tssrc/services/PromptSuggestion/speculation.tssrc/utils/atomicReplace.tssrc/utils/atomicReplace.test.tssrc/utils/sessionStorage.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/sessionStorage.atomicReplace.test.tssrc/utils/atomicReplace.test.ts
🔇 Additional comments (2)
src/utils/atomicReplace.test.ts (1)
124-141: LGTM!src/services/PromptSuggestion/speculation.ts (1)
793-803: LGTM!
| if ( | ||
| options.expectedTargetSize !== undefined && | ||
| (await stat(targetPath)).size !== options.expectedTargetSize | ||
| ) { | ||
| throw new Error('Atomic replacement target changed before commit') | ||
| } | ||
| await rename(tempPath, targetPath) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the freshness check atomic with the file mutation.
The size check does not protect the interval before rename or truncate. If an unqueued writer appends after stat() returns, rename replaces the file without that append, or truncate(boundary) removes it. The session-storage code explicitly supports external SDK writers, so this can lose transcript entries.
src/utils/atomicReplace.ts#L263-L269: use a cooperative per-target writer lock or revision protocol that spans snapshot, copy, freshness validation, andrename. Require every transcript writer to participate.src/utils/sessionStorage.ts#L429-L432: hold the same coordination mechanism through the size validation andhandle.truncate(boundary).- Add a deterministic regression test that injects an append after freshness validation and before the destructive operation.
📍 Affects 2 files
src/utils/atomicReplace.ts#L263-L269(this comment)src/utils/sessionStorage.ts#L429-L432
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/atomicReplace.ts` around lines 263 - 269, The freshness check and
destructive mutation are not coordinated, allowing external writers to append
between validation and replacement or truncation. Introduce a cooperative
per-target writer lock or revision protocol spanning snapshot, copy, freshness
validation, and mutation; update src/utils/atomicReplace.ts lines 263-269 and
src/utils/sessionStorage.ts lines 429-432 so all transcript writers, including
handle.truncate(boundary), hold the same coordination mechanism. Add a
deterministic regression test that appends after freshness validation but before
the destructive operation and verifies the append is preserved.
There was a problem hiding this comment.
Update
The current head already coordinates the freshness check and destructive mutation under the same per-target transcript lock, and the focused concurrency regressions pass.
Addressed
- Atomic freshness and mutation — Transcript replacement and final-line truncation run under the resolved-target lock; session, synchronous metadata, and SDK append paths acquire or queue behind that same lock. The abort signal is checked at the destructive boundary, and the post-validation external-append regression passes. — 50d652d, 1043e10
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/sessionStorage.atomicReplace.test.ts (1)
689-689:⚠️ Potential issue | 🔴 CriticalPass a callable subagent reader.
setInternalEventReaderrequiressubagentReader: InternalEventReader. Passingundefinedstill violates the strict TypeScript signature and keeps the test build broken.Proposed fix
- setInternalEventReader(async () => [], undefined) + setInternalEventReader(async () => [], async () => [])Run
bun run typecheckafter the change.As per coding guidelines: TypeScript code in this repository must use strict mode. As per path instructions: run the narrowest relevant checks, including type checks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/sessionStorage.atomicReplace.test.ts` at line 689, Update the setInternalEventReader call in the test to provide a callable InternalEventReader as its subagentReader argument instead of undefined, preserving the existing empty-result behavior. Run the narrowest relevant checks, including bun run typecheck.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/utils/sessionStorage.atomicReplace.test.ts`:
- Line 689: Update the setInternalEventReader call in the test to provide a
callable InternalEventReader as its subagentReader argument instead of
undefined, preserving the existing empty-result behavior. Run the narrowest
relevant checks, including bun run typecheck.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cb261af9-cf98-4aeb-9cee-8331fa0b062a
📒 Files selected for processing (1)
src/utils/sessionStorage.atomicReplace.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/sessionStorage.atomicReplace.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/sessionStorage.atomicReplace.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/sessionStorage.atomicReplace.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/sessionStorage.atomicReplace.test.ts
Hold a same-directory cooperative lock across transcript replacement and final-line truncation, and make session plus SDK append paths participate. Exercise the post-validation/pre-rename race deterministically so external appends land after the complete commit.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/sessionStorage.atomicReplace.test.ts`:
- Around line 315-328: Make the race in the
setAtomicReplaceFaultInjectorForTesting test deterministic by pausing the rename
stage with a deferred gate. Release that gate only after a lock- or
append-acquisition test hook confirms renameSession is queued behind the held
transcript lock, then assert appendPromise remains pending before
removeTranscriptMessage commits. After the tombstone commit and
flushSessionStorage, release the gate and await appendPromise to verify the
append completes afterward.
In `@src/utils/sessionStorage.ts`:
- Line 3515: Update the synchronous directory creation in appendEntryToFile to
pass recursive: true alongside mode: 0o700, matching the sibling call sites so
repeated writes to an existing session directory succeed. Add a focused
regression test that invokes appendEntryToFile twice for the same session
directory and verifies both entries are written.
In `@src/utils/transcriptFileLock.ts`:
- Around line 137-156: Update withTranscriptFileLockSync and the shared
lock-state handling so a nonzero heldLockCounts entry never by itself authorizes
the reentrancy shortcut; only the current operation’s own explicit reentrancy
context may bypass reacquisition. Ensure async acquisition in
withTranscriptFileLock cannot satisfy the synchronous check, while preserving
legitimate nested synchronous calls, including symlink-resolved targets. Add a
regression test covering an async rewrite on a symlinked transcript that awaits
before issuing synchronous appendEntryToFile, and verify the appended line
survives.
- Around line 80-94: Update asyncLockOptions to provide a safe onCompromised
callback in the proper-lockfile options, preventing a compromised lock during an
in-flight transcript rewrite suspension from throwing and terminating the CLI.
Keep the existing stale, update, and retry settings unchanged.
- Around line 96-116: The synchronous acquireTranscriptLockSync retry path must
not use Atomics.wait on the main thread. Replace the blocking wait with an
awaitable/non-blocking retry mechanism, or change the caller to await an
asynchronous lock-acquisition flow, while preserving the ELOCKED deadline,
backoff, and stale-lock options.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 88afa299-08f6-45cf-b566-70c14c3b2e73
📒 Files selected for processing (5)
src/entrypoints/sdk/sessions.tssrc/utils/atomicReplace.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/sessionStorage.tssrc/utils/transcriptFileLock.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/entrypoints/sdk/sessions.tssrc/utils/atomicReplace.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/transcriptFileLock.tssrc/utils/sessionStorage.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/entrypoints/sdk/sessions.tssrc/utils/atomicReplace.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/transcriptFileLock.tssrc/utils/sessionStorage.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/entrypoints/sdk/sessions.tssrc/utils/atomicReplace.tssrc/utils/sessionStorage.atomicReplace.test.tssrc/utils/transcriptFileLock.tssrc/utils/sessionStorage.ts
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**
⚙️ CodeRabbit configuration file
src/{components/permissions,utils/permissions,hooks/toolPermission,tools,entrypoints/sdk}/**: Review permission prompts, auto-allow logic, sandbox behavior, SDK permission schemas, shell/PowerShell execution, and background execution paths as security-sensitive. Block on bypasses, unclear trust boundaries, unsafe path handling, missing user visibility, or changes that broaden allowed behavior without an explicit maintainer decision.
Files:
src/entrypoints/sdk/sessions.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
src/entrypoints/sdk/sessions.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/sessionStorage.atomicReplace.test.ts
🔇 Additional comments (13)
src/utils/sessionStorage.atomicReplace.test.ts (1)
25-25: LGTM!Also applies to: 40-40
src/utils/atomicReplace.ts (3)
200-222: LGTM!
224-272: LGTM!
273-293: LGTM!src/utils/transcriptFileLock.ts (3)
12-68: LGTM!
70-78: LGTM!
158-164: LGTM!src/entrypoints/sdk/sessions.ts (1)
21-21: LGTM!Also applies to: 226-229
src/utils/sessionStorage.ts (5)
903-909: LGTM!Also applies to: 925-927
1020-1029: LGTM!Also applies to: 1083-1083
1129-1204: LGTM!
1385-1410: LGTM!
1412-1525: LGTM!
| const line = jsonStringify(entry) + '\n' | ||
| try { | ||
| if (project?._deferSynchronousAppend(fullPath, line)) return | ||
| fs.mkdirSync(dirname(fullPath), { mode: 0o700 }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Blocking: mkdirSync is missing recursive: true and will throw EEXIST on every synchronous metadata append.
Line 3515 calls fs.mkdirSync(dirname(fullPath), { mode: 0o700 }). Without recursive: true, mkdirSync throws EEXIST when the directory already exists — which is the normal case for any session that has written even one entry. Nothing in appendEntryToFile catches it.
The two sibling call sites added in this PR both pass the flag:
appendDirectlyToFileat Line 1025:{ recursive: true, mode: 0o700 }appendJsonlEntryinsrc/entrypoints/sdk/sessions.tsat Line 226:{ mode: 0o700, recursive: true }
_deferSynchronousAppend returns true only while a rewrite barrier is active, so the ordinary path reaches this line directly. The blast radius is every synchronous metadata write from reAppendSessionMetadata: custom-title, tag, last-prompt, agent-name, agent-color, agent-setting, mode, worktree-state, pr-link, goal-state, and session-branch. Those run during compaction and on session exit, so --resume loses its metadata and the exit path throws.
Add a focused regression test that calls appendEntryToFile twice against an existing session directory and asserts both lines are present.
As per coding guidelines: "Add or update tests when a TypeScript or TSX change affects behavior."
🐛 Proposed fix
if (project?._deferSynchronousAppend(fullPath, line)) return
- fs.mkdirSync(dirname(fullPath), { mode: 0o700 })
+ fs.mkdirSync(dirname(fullPath), { mode: 0o700, recursive: true })
withTranscriptFileLockSync(fullPath, () => {
fs.appendFileSync(fullPath, line, { mode: 0o600 })
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fs.mkdirSync(dirname(fullPath), { mode: 0o700 }) | |
| fs.mkdirSync(dirname(fullPath), { mode: 0o700, recursive: true }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/sessionStorage.ts` at line 3515, Update the synchronous directory
creation in appendEntryToFile to pass recursive: true alongside mode: 0o700,
matching the sibling call sites so repeated writes to an existing session
directory succeed. Add a focused regression test that invokes appendEntryToFile
twice for the same session directory and verifies both entries are written.
Source: Coding guidelines
There was a problem hiding this comment.
Update
No code change is needed here because this call uses the repository filesystem abstraction rather than Node's raw mkdirSync.
Not changed
- Recursive synchronous directory creation — FsOperations.mkdirSync intentionally has no recursive option and its Node implementation always calls node:fs with recursive: true. Existing directories are therefore a documented no-op, and the focused filesystem mkdir tests pass; adding recursive at this call site would duplicate the abstraction and fail its type contract.
| function asyncLockOptions(targetPath: string) { | ||
| return { | ||
| lockfilePath: `${targetPath}.lock`, | ||
| realpath: false, | ||
| stale: TRANSCRIPT_LOCK_STALE_MS, | ||
| update: 5_000, | ||
| retries: { | ||
| retries: 240, | ||
| factor: 1.1, | ||
| minTimeout: 5, | ||
| maxTimeout: 250, | ||
| randomize: true, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- local lockfile wrapper ---'
fd -t f 'lockfile.ts' src -x cat -n
echo '--- installed proper-lockfile version ---'
fd -t f 'package.json' -d 1 -x jq '.dependencies["proper-lockfile"], .devDependencies["proper-lockfile"]' {}
echo '--- default onCompromised in the installed package ---'
fd -t d 'proper-lockfile' node_modules -d 3 -x rg -n -C 4 'onCompromised|compromised' {}
echo '--- existing onCompromised usage elsewhere in the repo ---'
rg -n -C 3 'onCompromised' --type=tsRepository: Gitlawb/openclaude
Length of output: 2962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- transcriptFileLock.ts ---'
fd -t f 'transcriptFileLock\.ts$' src -x sh -c 'echo "### {}"; wc -l "{}"; cat -n "{}"'
echo '--- lockfile wrapper files ---'
fd -t f 'lockfile.ts$|serverLockInfo\.ts$' src
echo '--- proper-lockfile usages/options ---'
rg -n -C 3 'lock\(|withTranscriptFileLock|asyncLockOptions|TRANSCRIPT_LOCK_STALE_MS|onCompromised' src
echo '--- package lock/version info ---'
if [ -f package.json ]; then jq '{dependencies,devDependencies}' package.json; fi
if [ -f bun.lockb ]; then bun run --bun --no-script-run bun lock --check 2>&1 || true; fi
if [ -f bun.lock ]; then echo "--- bun.lock matches ---"; rg -n '"proper-lockfile"|proper-lockfile' bun.lock | head -40; fi
echo '--- fetch proper-lockfile package source snippets from registry if available ---'
python3 - <<'PY'
import json, urllib.request, re
url='https://registry.npmjs.org/proper-lockfile/4.1.2'
try:
data=json.load(urllib.request.urlopen(url, timeout=20))
print('version', data.get('version'))
print('dist', data.get('dist',{}).get('tarball'))
except Exception as e:
print('registry fetch failed:', type(e).__name__, str(e))
PYRepository: Gitlawb/openclaude
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
cd "$tmp"
echo '--- fetch proper-lockfile 4.1.2 package source snippets ---'
npm pack proper-lockfile@4.1.2 --json 2>/dev/null | jq -r '.[0].filename' > pkg.txt || true
tarball="$(cat pkg.txt)"
if [ -n "$tarball" ]; then
tar -xzf "$tarball"
echo '--- files ---'
find package -maxdepth 2 -type f | sed 's#^./##' | sort | head -100
echo '--- lockfile implementation ---'
fd -t f 'lockfile|index' package -x sh -c 'echo "### {}"; wc -l "{}"; sed -n "1,220p" "{}"'
echo '--- compromised/onCompromised occurrences ---'
rg -n -C 3 'onCompromised|compromised|stale' package/src package/lib package 2>/dev/null || true
fi
echo '--- behavioral probe: inspect package default value/source snippets as text ---'
if [ -n "$tarball" ]; then
python3 - <<'PY' "$tarball"
import json, os, re, tarfile
tarball = os.sys.argv[1]
text = open('package/src/lockfile.ts').read()
print('has onCompromised rethrow:', bool(re.search(r'onCompromised\s*[=:].*\n?.*\n?.*rethrow|function\s+\w*\s*[^{]*\{\s*\n\s*throw', text)))
for m in re.finditer(r'onCompromised', text):
start=max(0,m.start()-300); end=min(len(text),m.end()+450)
line=text[:start].count('\n')+1
print(f'--- match line {line} ---')
print(text[start:end])
PY
fiRepository: Gitlawb/openclaude
Length of output: 29749
Handle lock compromise in asyncLockOptions.
proper-lockfile@4.1.2 defaults onCompromised to throw err, and the stale/5s update timer invokes it after an in-flight transcript rewrite suspension. Add a safe onCompromised callback to avoid killing the CLI when the lock becomes compromised.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/transcriptFileLock.ts` around lines 80 - 94, Update
asyncLockOptions to provide a safe onCompromised callback in the proper-lockfile
options, preventing a compromised lock during an in-flight transcript rewrite
suspension from throwing and terminating the CLI. Keep the existing stale,
update, and retry settings unchanged.
There was a problem hiding this comment.
Update
Lock compromise is handled without an uncaught process-level throw on the current head.
Addressed
- Compromised asynchronous transcript lock — asyncLockOptions supplies onCompromised and aborts the operation's controller. Replacement and final-line truncation check that signal at their destructive boundary, release tolerates ERELEASED only for a compromised lock, and the rename-boundary abort regression passes. — 1043e10
| function acquireTranscriptLockSync(targetPath: string): () => void { | ||
| const deadline = Date.now() + TRANSCRIPT_LOCK_WAIT_MS | ||
| let retryDelay = 5 | ||
|
|
||
| while (true) { | ||
| try { | ||
| return lockfile.lockSync(targetPath, { | ||
| lockfilePath: `${targetPath}.lock`, | ||
| realpath: false, | ||
| stale: TRANSCRIPT_LOCK_STALE_MS, | ||
| update: 5_000, | ||
| }) | ||
| } catch (error) { | ||
| if (getErrnoCode(error) !== 'ELOCKED' || Date.now() >= deadline) { | ||
| throw error | ||
| } | ||
| Atomics.wait(syncWaitBuffer, 0, 0, retryDelay) | ||
| retryDelay = Math.min(retryDelay * 2, 100) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- lockSync option handling in the installed package ---'
fd -t d 'proper-lockfile' node_modules -d 3 -x rg -n -C 6 'lockSync|options\.update|update' {}
echo '--- local wrapper surface ---'
fd -t f 'lockfile.ts' src -x cat -n
echo '--- other Atomics.wait usage in the repo ---'
rg -n -C 3 'Atomics\.wait' --type=ts
echo '--- declared runtime ---'
fd -t f 'package.json' -d 1 -x jq '{engines: .engines, packageManager: .packageManager}' {}Repository: Gitlawb/openclaude
Length of output: 314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relevant files ---'
fd -t f 'src/utils/transcriptFileLock.ts|src/(sessions|sessionStorage)\.ts|package.json|bun.lockb|bun.lock|AGENTS.md|README.md|docs/integrations/overview.md' . -d 4
echo '--- src/utils/transcriptFileLock.ts outline and contents ---'
if [ -f src/utils/transcriptFileLock.ts ]; then
ast-grep outline src/utils/transcriptFileLock.ts --view compact || true
wc -l src/utils/transcriptFileLock.ts
cat -n src/utils/transcriptFileLock.ts
fi
echo '--- lockfile usages in src ---'
rg -n -C 3 'acquireTranscriptLockSync|releaseTranscriptLockSync|lockSync|lock\(asynchronous|Atomics\.wait|transcriptFileLock' src || true
echo '--- proper-lockfile occurrences in repo files ---'
rg -n -C 2 '"proper-lockfile"|proper-lockfile|lockfile' package.json bun.lock bun.lockb yarn.lock pnpm-lock.yaml package-lock.json 2>/dev/null || true
echo '--- package runtime metadata ---'
if [ -f package.json ]; then jq '{type, engines, packageManager, scripts}' package.json; fiRepository: Gitlawb/openclaude
Length of output: 20726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/utils/lockfile.ts ---'
cat -n src/utils/lockfile.ts
echo '--- local lockfile exports/usages around lockSync ---'
rg -n -C 5 'function acquire.*LockSync|lockSync\(file|getLockfile|module\.exports|export \* as lockfile|proper-lockfile|LockOptions|UnlockOptions' src/utils/transcriptFileLock.ts src/utils/lockfile.ts package.json bun.lock
echo '--- fetch exact proper-lockfile 4.1.2 source for lockSync/update handling ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cd "$tmpdir"
git init -q
git ls-remote --tags https://github.com/moxystudio/node-proper-lockfile.git 'refs/tags/v4.1.2' | cut -f1
sha="$(git ls-remote --tags https://github.com/moxystudio/node-proper-lockfile.git 'refs/tags/v4.1.2' | cut -f1)"
git fetch -q --depth=1 origin "$sha"
git checkout -q FETCH_HEAD
printf '\n--- lockSync definitions/usages ---\n'
rg -n -C 8 'lockSync|OptionTypes|update.*Sync|options\.update|updateTimeout|lockfile|proper-lockfile' .
printf '\n--- updated-lockfile source excerpts ---\n'
fd -t f -d 4 . | xargs grep -n -C 5 'updateTimeout\|update:|lockSync'Repository: Gitlawb/openclaude
Length of output: 10375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
echo '--- lockSync/update handling from npm registry tarball for proper-lockfile@4.1.2 ---'
cd "$tmpdir"
tarball="$(npm view proper-lockfile@4.1.2 dist.tarball 2>/dev/null || true)"
printf '%s\n' "$tarball"
if [ -n "$tarball" ]; then
curl -fsSL "$tarball" -o proper-lockfile.tgz
tar -xzf proper-lockfile.tgz
rg -n -C 8 'lockSync|options\.update|updateTimeout|update:|stale' package || true
echo '--- package.json ---'
jq '{name, version, main, types}' package/package.json
fi
echo '--- Bun Atomics.wait main-thread availability ---'
node - <<'JS' || true
const ok = typeof globalThis.Atomics?.wait === 'function';
console.log(JSON.stringify({nodeAtomicsWaitAvailable: ok}));
JSRepository: Gitlawb/openclaude
Length of output: 24616
🌐 Web query:
Bun documentation Atomics wait main thread Atomics.wait unavailable worker threads
💡 Result:
In Bun, as in other JavaScript environments, the use of Atomics.wait is restricted by the threading model of the agent [1][2]. Atomics.wait is a blocking operation designed for use in worker threads, not the main thread [1][3]. Because it puts the calling agent to sleep to wait for notification or a timeout, attempting to call it on the main thread will throw a TypeError, as the main thread cannot be blocked [1][2][3]. For scenarios where you need to perform wait-like operations on the main thread without blocking its event loop, you should use Atomics.waitAsync instead [4][3]. This method is non-blocking, returns a Promise, and is specifically designed to be safe for use in agents where blocking is not permitted, such as the main thread [4][5][6]. Contrary to the suggestion that Atomics.wait is unavailable in worker threads, it is fully supported and intended for use within them [7][3]. Bun developers actively use Atomics.wait within worker threads to implement synchronization primitives like mutexes [7][8][9]. Recent maintenance in Bun has even focused on ensuring that worker termination correctly interrupts threads that are currently parked in an Atomics.wait call to prevent process hangs [8][9].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/wait
- 2: https://docs.w3cub.com/javascript/global_objects/atomics/wait
- 3: https://v8.dev/features/atomics
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Atomics/waitAsync
- 5: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global%5FObjects/Atomics
- 6: https://tc39.es/proposal-atomics-wait-async/
- 7: Mutex Support oven-sh/bun#20678
- 8: worker: make termination interrupt a worker blocked in Atomics.wait oven-sh/bun#32802
- 9: Bun process does not exit after Tinypool
destroy()completes (worker threads remain alive) oven-sh/bun#29173
Don’t retry a synchronous exclusive lock from the main thread with Atomics.wait.
lockSync will accept update: 5_000 and still use the lockfile's mtime for staleness; the only blocker is that Atomics.wait on Bun’s main thread throws and keeps acquireTranscriptLockSync from retrying, so another process can appear locked while this one has given up after the first ELOCKED. Use an awaitable/non-blocking retry for the synchronous wrapper, or make the call site wait asynchronously.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/transcriptFileLock.ts` around lines 96 - 116, The synchronous
acquireTranscriptLockSync retry path must not use Atomics.wait on the main
thread. Replace the blocking wait with an awaitable/non-blocking retry
mechanism, or change the caller to await an asynchronous lock-acquisition flow,
while preserving the ELOCKED deadline, backoff, and stale-lock options.
There was a problem hiding this comment.
Update
No change is needed because the reported runtime premise does not hold for the supported server-side runtimes used by this CLI.
Not changed
- Bounded synchronous lock retry — A main-thread Atomics.wait probe returned timed-out after the requested bounded wait on both Bun 1.3.14 and Node 25.5.0; neither runtime threw. This wrapper is intentionally synchronous, so converting its retry to an awaitable flow would change the caller contract without fixing a reproduced failure.
| /** Synchronous counterpart for shutdown and metadata append call sites. */ | ||
| export function withTranscriptFileLockSync<T>( | ||
| requestedPath: string, | ||
| operation: () => T, | ||
| ): T { | ||
| const targetPath = resolveTranscriptMutationTargetSync(requestedPath) | ||
| if ((heldLockCounts.get(targetPath) ?? 0) > 0) return operation() | ||
|
|
||
| const release = acquireTranscriptLockSync(targetPath) | ||
| incrementHeldLock(targetPath) | ||
| try { | ||
| return operation() | ||
| } finally { | ||
| try { | ||
| release() | ||
| } finally { | ||
| decrementHeldLock(targetPath) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Blocking: the sync reentrancy shortcut can skip the lock while an unrelated async rewrite holds it.
withTranscriptFileLock increments heldLockCounts[targetPath] at Line 125 for the whole duration of the async operation. withTranscriptFileLockSync treats any nonzero count for that path as "already held by me" and runs operation() with no lock at all.
heldLockCounts records that some code in this process holds the lock. It does not record that this caller holds it. Those are different facts. During the await operation() inside runRewriteOperation, the event loop is free, so a synchronous append can enter withTranscriptFileLockSync for the same file and take the unlocked branch while replaceFileAtomic is mid-flight. The append lands on the old inode and rename then discards it — the exact loss this PR is meant to prevent.
The _deferSynchronousAppend barrier in sessionStorage.ts covers most of this, but not all of it: rewriteBarrierFiles is keyed on the unresolved fullPath, while heldLockCounts is keyed on the resolved symlink target. When the transcript is a symlink, the barrier check misses and the reentrancy check hits. The PR explicitly supports symlinked transcripts, so this is a live path, not a hypothetical.
Scope the reentrancy marker to the operation that acquired the lock rather than to the file globally. An AsyncLocalStorage context, or an explicit reentrancy token that only withTranscriptFileLockSync sets for its own nested calls, both work. Do not let async acquisition satisfy the sync check.
Add a regression test: start an async rewrite on a symlinked transcript, and during its await, issue a synchronous appendEntryToFile to the symlink path. Assert the appended line survives.
🐛 Sketch of the ownership fix
-const heldLockCounts = new Map<string, number>()
+// Locks held by *synchronous* frames on this call stack. Async holders must
+// not appear here: an in-flight async rewrite is not the sync caller, and
+// treating it as one skips the lock during the rewrite's await window.
+const syncHeldLockCounts = new Map<string, number>() const targetPath = await resolveTranscriptMutationTarget(requestedPath)
const release = await lockfile.lock(targetPath, asyncLockOptions(targetPath))
- incrementHeldLock(targetPath)
try {
return await operation()
} finally {
- try {
- await release()
- } finally {
- decrementHeldLock(targetPath)
- }
+ await release()
}
} const targetPath = resolveTranscriptMutationTargetSync(requestedPath)
- if ((heldLockCounts.get(targetPath) ?? 0) > 0) return operation()
+ if ((syncHeldLockCounts.get(targetPath) ?? 0) > 0) return operation()
const release = acquireTranscriptLockSync(targetPath)
- incrementHeldLock(targetPath)
+ incrementSyncHeldLock(targetPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/transcriptFileLock.ts` around lines 137 - 156, Update
withTranscriptFileLockSync and the shared lock-state handling so a nonzero
heldLockCounts entry never by itself authorizes the reentrancy shortcut; only
the current operation’s own explicit reentrancy context may bypass
reacquisition. Ensure async acquisition in withTranscriptFileLock cannot satisfy
the synchronous check, while preserving legitimate nested synchronous calls,
including symlink-resolved targets. Add a regression test covering an async
rewrite on a symlinked transcript that awaits before issuing synchronous
appendEntryToFile, and verify the appended line survives.
There was a problem hiding this comment.
Update
The current head prevents an unrelated synchronous append from treating an active asynchronous rewrite as reentrant.
Addressed
- Synchronous reentrancy during asynchronous rewrite — Asynchronous and synchronous lock ownership now use separate maps. The synchronous append barrier also resolves symlink aliases when checking for an active asynchronous lock, queues the append, and the symlinked-transcript regression verifies the appended line survives. — 1043e10
Separate async and synchronous lock ownership so unrelated sync appends cannot bypass an in-flight replacement. Route aliased in-process appends through the queue, propagate lock compromise through AbortSignal, and cover both symlink-alias and rename-boundary races.
UpdatePushed follow-up commits 47345fc through 1043e10 to preserve existing transcripts on empty foreground hydration, coordinate transcript rewrites with session and SDK append writers, and fix the fresh-checkout typecheck. The latest follow-up scopes lock ownership correctly across symlink aliases and turns lock compromise into a pre-commit abort. Addressed
Not changed
Validation: the focused atomic replacement, hydration, concurrency, filesystem, and resume-loader matrix passes 96/96; TypeScript, a cache-disabled TypeScript run, and type-test checks pass; build, smoke, dead-code, and security intent checks pass. The full suite reports 7,843 passed, 2 skipped, and the same 41 pre-existing local baseline failures recorded before this follow-up. All review and CI items were addressed or explicitly verified as not requiring a change. |
* fix(session): make transcript replacements crash-safe Complete transcript rewrites could truncate live JSONL files before preserved data was durable, risking unrecoverable resume history after an interrupted write. Commit replacements through exclusive sibling temp files and serialize them with all transcript append paths so readers observe either the old file or the complete replacement. * fix(session): preserve concurrent transcript updates Abort tombstone commits when the scanned transcript changes before replacement, and keep existing local history when remote foreground hydration returns no entries. Harden the associated portability, option coverage, queue timing, and diagnostics. * test(session): match hydration reader signature Pass the explicit optional subagent reader in the empty-hydration regression so a fresh TypeScript build sees the complete helper signature. * fix(session): coordinate transcript writers across processes Hold a same-directory cooperative lock across transcript replacement and final-line truncation, and make session plus SDK append paths participate. Exercise the post-validation/pre-rename race deterministically so external appends land after the complete commit. * test(session): provide empty hydration subagent reader * fix(session): scope transcript lock ownership Separate async and synchronous lock ownership so unrelated sync appends cannot bypass an in-flight replacement. Route aliased in-process appends through the queue, propagate lock compromise through AbortSignal, and cover both symlink-alias and rename-boundary races. (cherry picked from commit d834904)
3 participants
Summary
Impact
Testing
bun run buildbun run smokebun run check— build, smoke, and dead-code checks passed; the full suite reported 7,837 passed, 2 skipped, and the same 41 local baseline failures asupstream/mainbun test --feature=UNATTENDED_RETRY --max-concurrency=1 src/utils/atomicReplace.test.ts src/utils/sessionStorage.atomicReplace.test.ts src/utils/sessionStorage.test.ts src/utils/sessionStorage.liteTag.test.ts src/utils/file.test.ts src/utils/fsOperations.mkdir.test.ts src/commands/resume/resume.test.tsx— 90 passedbun run test:provider— 1,479 passednpm run test:provider-recommendation— 129 passedbun run typecheckbun run typecheck:type-testsbun run security:pr-scan -- --base upstream/main --head HEADbun run --cwd web typecheckbun run --cwd web buildReview focus
src/utils/atomicReplace.tsfor the pre-rename preservation and post-rename durability boundariessrc/utils/sessionStorage.ts, especially enqueue-time barriers, single-flight draining, and failure settlementNotes
CONTRIBUTING.mdandAGENTS.mdSummary by CodeRabbit
New Features
Bug Fixes