Skip to content

refactor(windows): share the atomic-replace retry instead of one writer owning it - #1946

Merged
lidge-jun merged 1 commit into
devfrom
codex/win-030-atomic-replace
Aug 18, 2026
Merged

lidge-jun merged 1 commit into
devfrom
codex/win-030-atomic-replace

Conversation

@lidge-jun

Copy link
Copy Markdown
Owner

Summary

  • src/config.ts:102-123 knew that Windows can refuse rename with EBUSY, EPERM or EACCES while a scanner or sync client still holds the target, and retried twice (25ms then 50ms). Nothing else did. Two durable publishers called renameSync directly: src/codex/prompt-journal.ts, which publishes a journal carrying full config.toml bytes — losing that publish is what breaks journal restore — and src/lib/config-ownership.ts, which publishes the uninstall manifest.
  • Neither corrupts anything on failure; they throw rather than publish a partial file. But under a real-time scanner holding the target they turn a momentary hold into a user-visible failure, and the tolerance to survive it already existed one module away.
  • The loop moves to src/lib/windows-atomic-replace.ts rather than becoming an export of config.ts: config-ownership.ts is one of the callers and config.ts already imports config-ownership.ts (line 47), so the obvious placement would close an import cycle. config.ts re-exports the names because they are part of its public surface.
  • The envelope is unchanged at two retries. Widening it without evidence trades a rare failure for a routine stall; the counters in the child PR are how that argument gets made from data instead.

Third of a stacked chain implementing devlog/_plan/260817_windows_stability_program/ (phase 030). Targets #1945; retarget to dev once the parents land.

Verification

  • bun run typecheck — clean
  • Full suite in 60-file batches (Bun 1.3.14 panics near 3.5GB on a monolithic --isolate run over 809 files): batches 1-2 green at 647 and 1032 pass; remaining batches running. One pre-existing failure in tests/codex-app-server-processes.test.ts ("a defaulted read is memoized") reproduces on clean origin/dev and is unrelated to this change.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • ^dev$
  • ^preview$

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: aaf758a3-9363-480e-903e-3ca839138575

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Aug 18, 2026
@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Aug 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_regression_test — Behavior changed under src/ or gui/src/ without a test change. Add focused coverage or obtain test-exception-approved.

@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed.

Hygiene

Deterministic PR hygiene checks passed.

@github-actions
github-actions Bot marked this pull request as draft August 18, 2026 00:29
@lidge-jun
lidge-jun marked this pull request as ready for review August 18, 2026 00:31
@github-actions
github-actions Bot marked this pull request as draft August 18, 2026 00:33
@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Aug 18, 2026
@github-actions
github-actions Bot marked this pull request as ready for review August 18, 2026 00:39
@lidge-jun
lidge-jun force-pushed the codex/win-030-atomic-replace branch from 5315f76 to ff4408a Compare August 18, 2026 01:17
@lidge-jun
lidge-jun force-pushed the codex/win-020-wrapper-killer branch from a8cef7d to a3169db Compare August 18, 2026 01:17
@lidge-jun
lidge-jun force-pushed the codex/win-030-atomic-replace branch from ff4408a to c23031a Compare August 18, 2026 01:22
…er owning it

src/config.ts:102-123 knew that Windows can refuse rename with EBUSY, EPERM or
EACCES while a scanner or sync client still holds the target, and retried twice
(25ms then 50ms). Nothing else did. Eight durable publishers called renameSync
directly:

- src/codex/prompt-journal.ts, whose journal carries full config.toml bytes --
  losing that publish is what breaks journal restore;
- src/lib/config-ownership.ts, the uninstall manifest;
- src/claude/agents-inject.ts, the generated agent definitions;
- src/lab/automation/persistence.ts and config-persistence.ts;
- src/lab/ledger/purge.ts, the rewritten ledger;
- src/storage/cleanup.ts, both the satellite backup (1094) and the
  restore-pending state file (2438);
- src/tray/windows.ts, the tray's owned-file publisher.

None corrupts anything on failure; they throw rather than publish a partial
file. But under a real-time scanner holding the target they turn a momentary
hold into a user-visible failure, and the tolerance to survive it already
existed one module away.

Three renameSync calls in src/storage/cleanup.ts are deliberately NOT converted.
1639 and 2616 move directories between staging and trash, and 1667 moves one
back on rollback: these relocate a directory rather than publishing a temp file
over a destination. Windows directory-move failures are a different problem with
a different fix, and the callers already handle them.

The loop moves to src/lib/windows-atomic-replace.ts rather than becoming an
export of config.ts: config-ownership.ts is one of the callers and config.ts
already imports config-ownership.ts, so the obvious placement would close an
import cycle. config.ts re-exports renameAtomicFile because callers use it;
renameAtomicFileAsync stays internal, as it was before.

The envelope is unchanged at two retries, and tests/windows-atomic-replace.test.ts
now pins it: which codes count as transient, that POSIX never retries, and that
the bound is two rather than hopeful. The extracted module had no test of its
own before -- config.test.ts exercises it only through atomicWriteFile -- so an
accidental widening would have gone unnoticed. That matters because the next
commit adds counters specifically to decide whether widening is justified.

Verification: bun run typecheck clean; bun test over claude-agents-inject,
windows-atomic-replace, config, storage-cleanup and windows-tray.
@lidge-jun
lidge-jun force-pushed the codex/win-030-atomic-replace branch from c23031a to c5c6644 Compare August 18, 2026 01:25
@lidge-jun

Copy link
Copy Markdown
Owner Author

The keyring ubuntu job on this PR has stalled twice on its Install Secret Service dependencies (Linux) step (apt), once for 25+ minutes before I cancelled it, and again on the rerun. The same job passes on the sibling PRs in this stack (#1944, #1945, #1947) against nearly identical trees, and keyring macos / keyring windows pass here.

Nothing in this PR touches keyring code, CI configuration, or dependencies — it moves a rename retry loop into src/lib/windows-atomic-replace.ts and converts eight callers. Treating the stall as runner-side rather than a real failure; happy to rerun once more if a maintainer prefers.

@lidge-jun
lidge-jun changed the base branch from codex/win-020-wrapper-killer to dev August 18, 2026 08:43
@lidge-jun

Copy link
Copy Markdown
Owner Author

Validation before merge: scratch-worktree merge onto dev (post-#1945) — windows-popup-fix + config suites 158/0, tsc --noEmit clean.

@lidge-jun
lidge-jun merged commit c140758 into dev Aug 18, 2026
59 of 63 checks passed
@lidge-jun
lidge-jun deleted the codex/win-030-atomic-replace branch August 19, 2026 09:18
agentHits pushed a commit to agentHits/opencodex that referenced this pull request Sep 17, 2026
…-replace

refactor(windows): share the atomic-replace retry instead of one writer owning it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant