fix(session): prevent concurrent save failures - #447
Conversation
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed July 27, 2026, 10:06 AM ET / 14:06 UTC. ClawSweeper reviewWhat this changesThe branch introduces a shared UUID-suffixed temporary-path helper for atomic session and session-index writes, updates three persistence writers to use it, and adds concurrent-save regression coverage. Merge readinessThis PR remains necessary: current Priority: P2 Review scores
Verification
How this fits togetherACPX persists session records and a session index as JSON files through file-backed stores. Each save writes a temporary file and renames it into place; unique temporary names prevent simultaneous saves from interfering before the rename completes. flowchart LR
A[Session runtime or checkpoint] --> B[File-backed session store]
B --> C[Serialize record or index]
C --> D[Create unique temporary path]
D --> E[Write temporary JSON file]
E --> F[Atomic rename to destination]
F --> G[Readable session state]
Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the narrow shared-helper fix after normal maintainer review and completion of the remaining required checks, preserving the existing destination paths and atomic-rename behavior. Do we have a high-confidence way to reproduce the issue? Yes—source inspection provides a high-confidence reproduction path: hold Is this the best way to solve the issue? Yes—the UUID is confined to the private temporary pathname, preserves the destination path and atomic rename model, and centralizes the behavior so the three identified writers cannot drift apart. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9866c7608764. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
Co-authored-by: Henk ter Harmsel <henk@chaink.it>
d77504d to
2505217
Compare
|
Landed the atomic-write collision fix with contributor credit preserved. The failure came from using only PID plus millisecond time for temporary filenames: concurrent same-process writes could select the same path, and the first rename removed it from the later writer. The final patch centralizes UUID-suffixed same-directory temp paths across the public runtime store, session repository, and session index.
Thanks @henkterharmsel. |
2 participants
What Problem This Solves
Fixes an issue where concurrent saves of the same ACPX session could choose the same temporary pathname. One writer could rename the shared temporary file while another writer still expected it to exist, causing an
ENOENTfailure or allowing one save to overwrite another writer's temporary payload.Why This Change Was Made
Atomic-write temporary paths now include a per-write UUID in addition to the target path. All session-store callers use the same helper, so concurrent writes retain atomic rename semantics without sharing temporary files.
User Impact
Concurrent runtime and checkpoint updates can persist the same session reliably without intermittent temporary-file collisions.
Evidence
pnpm run checkpasses on Node 24.18.0 and pnpm 10.33.2.codex reviewand the project autoreview both reported no findings.Real behavior proof
Behavior or issue addressed: Concurrent saves through the real file session store must not share temporary paths or fail with
ENOENT.Real environment tested: Linux checkout of this branch using Node 24.18.0 and pnpm 10.33.2. The probe imported the production
createFileSessionStoreimplementation directly; it did not use Vitest, mocks, or snapshots.Exact steps or command run after this patch: Created a temporary ACPX state directory, froze
Date.now()to force every write into the same millisecond, and ran 50 rounds of eight concurrentstore.save(record)calls. It then loaded the session through the public store, parsed the destination JSON directly, and checked for leftover.tmpfiles.Evidence after fix:
Observed result after fix: All 400 concurrent writes completed without an exception. The final session was readable through the public API, its JSON was valid, and no temporary files remained.
What was not tested: I did not interfere with persistence timing on a production gateway, because doing so would risk live session state. Windows and macOS were not tested locally.
Testing boundary
The race was reproduced deterministically through the real file-session-store implementation with controlled filesystem timing. I did not attempt to force the timing window against a production gateway because that would require interfering with live session persistence.
AI-assisted disclosure
This PR is AI-assisted. I reviewed the implementation and understand that uniqueness is added only to the temporary pathname; the destination path and atomic rename behavior are unchanged.