fix(json): retry on transient File changed during read race condition#84285
fix(json): retry on transient File changed during read race condition#84285samson1357924 wants to merge 4 commits into
Conversation
|
Codex review: passed. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. source-reproducible: current main reads pairing JSON through one-shot PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review detailsBest possible solution: Land one bounded async JSON read retry path; prefer the Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main reads pairing JSON through one-shot Is this the best way to solve the issue? Mostly yes: the PR is a narrow bounded retry that preserves parse and non-transient failures, but the cleaner long-term solution is the dependency-level retry in openclaw/fs-safe#19. Label justifications:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against b33deb41594e. |
723e179 to
45da87f
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Tiny Shellbean Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
@clawsweeper automerge |
|
🦞✅ Source: Why human review is needed: Recommended next action: I added |
…ly, no-unnecessary-type-assertion)
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
ClawSweeper 🐠 reef update Thanks for the work here. ClawSweeper could not write to the source branch, so it opened a replacement PR rather than letting the fix drift. attribution still points back here. Why replacement: ClawSweeper could not update the source PR branch directly; GitHub did not grant sufficient push rights to the bot for that branch.
fish notes: model gpt-5.5, reasoning high; reviewed against 00602a1. |
Summary
JsonFileReadError: File changed during readoccurs whenpaired.jsonis atomically rewritten (temp file + rename) while another component reads it.verifyStableReadTargetdetects the file changed between stat and open, and throws immediately.Fix
Replace the three async JSON read functions in
src/infra/json-files.ts—readJson,readJsonIfExists,tryReadJson— with bounded retry logic for the transient file-changed race:Key design decisions
Cause‑chain walking — The error from
@openclaw/fs-safeisJsonFileReadErrorwhose message is"Failed to read JSON file: …", while the original"File changed during read"error is nested inerr.cause. TheisFileChangedDuringReadcheck recursively followserr.causeto detect the true race, unlike the naïveerr.message.includesapproach that would never match.Bounded retry — Retry up to 3 attempts with exponential backoff (50 ms → 100 ms). Only retry on
File changed during readerrors — parse errors, ENOENT, and other errors are not retried.tryReadJsonre‑routed throughreadJsonIfExists— The underlying@openclaw/fs-safetryReadJsonImplswallows all errors internally and returnsnull, which prevents the retry wrapper from ever detecting a race. OurtryReadJsonnow delegates toreadJsonIfExists(which properly propagates errors on race conditions), so the retry wrapper can intercept them. Parse errors and file‑not‑found are still handled gracefully by the outer catch.Strict reader types preserved —
readJson<T>andreadJsonFileStrict<T>retainPromise<T>(notPromise<T | null>), maintaining SDK contract compatibility per ClawSweeper's [P2] finding.Real behavior proof
Behavior or issue addressed: Gateway startup triggers
JsonFileReadError: Failed to read JSON file: paired.jsonwhenverifyStableReadTargetdetects the file changed between stat and open (race condition with atomic write). Error propagates as unhandled error in request handler.Real environment tested: OpenClaw 2026.5.18 (npm install, Oracle VPS, Ubuntu 24.04, Node.js v24.15.0). The paired.json file is rewritten by
openclaw logs --followor device pairing operations during gateway startup.Exact steps or command run after this patch: (1) Backed up the original
dist/json-files-1SmAauRO.js. (2) Applied the patch with the cause‑chain‑aware retry andtryReadJsonre‑routing. (3) Verified JS syntax withnode --check. (4) Ran unit tests — 15/15 passed including new retry-success and retry-exhaustion coverage. (5) The gateway was restarted and has been running without errors for 1.5+ hours.Evidence after fix:
Before (error on every restart with concurrent device operations):
After (1.5+ hours of runtime — zero JsonFileReadError errors):
Unit tests (15/15 passed, including retry coverage):
Observed result after fix: The
JsonFileReadError: File changed during readerror no longer appears on gateway startup or operation. The cause‑chain‑aware retry mechanism absorbs the transient race condition without propagating to the request handler.What was not tested: Sync read paths (
readJsonSync,tryReadJsonSync) — less prone to races in practice. Non-Linux platforms (Windows rename semantics differ). Extreme contention (3+ concurrent writes) — retries exhaust after 3 attempts and the original error path activates.Related
Fixes #83657