Skip to content

Make session saves atomic and recover Windows sharing conflicts - #158

Merged
Maximooch merged 3 commits into
mainfrom
fix/session-atomic-save
Oct 28, 2026
Merged

Maximooch merged 3 commits into
mainfrom
fix/session-atomic-save

Conversation

@Maximooch

@Maximooch Maximooch commented Oct 28, 2026 •

Copy link
Copy Markdown
Owner

Rapid or overlapping SessionManager saves could share the same temporary filename and fail at os.replace on Windows. A reader holding the destination can also make Windows return access denied even though the conflict is temporary. This change makes session/index replacement atomic and serializes store operations without treating every permission error as retryable.

Independent of the ACP stack, based on current main de6b141. Addresses the storage blocker observed while validating #155.

Changes

  • Write session, backup, and index content to unique sibling files, flush/fsync, close, then atomically replace the destination. Backup restoration uses the same helper.
  • Share a reentrant lock between managers of the same directory within one process, covering save/index/cache operations.
  • Merge locally changed index entries into the latest saved index instead of replacing another manager's entries with a stale snapshot.
  • Retry Windows sharing/lock violations only. For ambiguous WinError 5, a non-mutating DELETE-access probe must confirm a sharing conflict or recovered access; read-only attributes and continuing ACL denials are not retried. Six attempts maximum, with 310ms total backoff.
  • Preserve previous files on replacement failure, remove only the current attempt's temporary file, and retain dirty cache state. An index failure after a successful session write returns False rather than reporting a complete save.
  • Add Windows/Linux Python 3.11/3.12 storage CI.

Validation

  • Native Windows reproduction on unchanged main: holding a reader open makes save_session return False with WinError 5 at the deterministic .json.temp -> .json replacement. The Windows DELETE-access probe identifies this as sharing violation 32.
  • 31 passed on Windows Python 3.11 with locked dependencies: real sharing-conflict recovery, real read-only denial with no retry, concurrent same/different session writers, complete JSON observed by readers, unique temporary paths, bounded retry exhaustion, failed serialization/fsync/replacement/index writes, backup recovery, and existing save/goal/persistence contracts.
  • 92 passed, 2 Windows-only skips on Linux with locked dependencies, additionally covering session-view and archive regressions.
  • 85 ACP integration tests passed on an isolated copy of committed ACP snapshot a52a872 with the storage patch applied, including the terminal/configuration test. The active ACP checkout was not edited.
  • A broader Windows session-view run had one existing failure in test_session_index_rebuilds_goal_after_index_checkpoint_failure; it also fails on unchanged main. The Linux counterpart passes.
  • Focused lint/format and git diff --check pass; no lockfile or dependencies changed.

Limits

Locks coordinate managers within one process; cross-process revision merging is not provided. Each file is atomic, but session and index are separate writes. A crash or failed index write may leave the index behind the session, for the existing stale-index recovery path to rebuild. The code does not chmod destinations or delete them to bypass permission errors.

No merge, deployment, or running-runtime replacement.

Integration revision

Final head: c347531295e20a6ea84614ce07ae7a602559298a on fix/session-atomic-save. It includes the base fix 4748531910a3cbead75f16769c9259afadb7f3e1 and a follow-up for sharing conflicts that clear before the diagnostic probe. The concurrent reader test uses Windows delete-sharing handles to test atomic contents independently of intentional sharing blockers; the held-reader and real read-only-denial regressions remain.

All four new storage CI jobs (Windows/Linux, Python 3.11/3.12) pass. The final code also passes all 85 ACP integration tests on the isolated snapshot. SessionManager.save_session(session=None) -> bool is unchanged; no ACP source files were modified.

All CI jobs for head 3d2eb2016e47defcea4e1ddc7be89d58595844d4 completed successfully, including both Windows and both Linux storage jobs and the existing runtime/provider, TUI, documentation, and dependency-security jobs. The external CodeRabbit review was skipped by its service.

Test accounting follow-up

Commit c347531295e20a6ea84614ce07ae7a602559298a corrects the concurrency regression to assert unique temporary-file creation, not unique source paths per replacement attempt. A deterministic injected sharing conflict proves a retry reuses its original temporary file, with at most six attempts per write. All concurrency, JSON integrity, index consistency, and cleanup checks remain. This commit changes tests only. Local revalidation: 31 Windows tests passed; 29 Linux tests passed with 2 Windows-only skips. All CI jobs also passed at this new head, including Windows/Linux Python 3.11/3.12 storage contracts.

@vercel

vercel Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
penguin Ready Ready Preview Oct 28, 2026 6:32am UTC

@coderabbitai

coderabbitai Bot commented Oct 28, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@Maximooch
Maximooch merged commit 60c9bad into main Oct 28, 2026
15 checks passed

This branch was successfully deployed

1 active deployment
Preview — c3475312 Deployed Oct 28, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant