Skip to content

fix(json): retry on transient File changed during read race condition#84285

Closed
samson1357924 wants to merge 4 commits into
openclaw:mainfrom
samson1357924:fix/paired-json-retry
Closed

fix(json): retry on transient File changed during read race condition#84285
samson1357924 wants to merge 4 commits into
openclaw:mainfrom
samson1357924:fix/paired-json-retry

Conversation

@samson1357924

@samson1357924 samson1357924 commented May 19, 2026

Copy link
Copy Markdown
Contributor

Summary

JsonFileReadError: File changed during read occurs when paired.json is atomically rewritten (temp file + rename) while another component reads it. verifyStableReadTarget detects the file changed between stat and open, and throws immediately.

Fix

Replace the three async JSON read functions in src/infra/json-files.tsreadJson, readJsonIfExists, tryReadJson — with bounded retry logic for the transient file-changed race:

Key design decisions

  1. Cause‑chain walking — The error from @openclaw/fs-safe is JsonFileReadError whose message is "Failed to read JSON file: …", while the original "File changed during read" error is nested in err.cause. The isFileChangedDuringRead check recursively follows err.cause to detect the true race, unlike the naïve err.message.includes approach that would never match.

  2. Bounded retry — Retry up to 3 attempts with exponential backoff (50 ms → 100 ms). Only retry on File changed during read errors — parse errors, ENOENT, and other errors are not retried.

  3. tryReadJson re‑routed through readJsonIfExists — The underlying @openclaw/fs-safe tryReadJsonImpl swallows all errors internally and returns null, which prevents the retry wrapper from ever detecting a race. Our tryReadJson now delegates to readJsonIfExists (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.

  4. Strict reader types preservedreadJson<T> and readJsonFileStrict<T> retain Promise<T> (not Promise<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.json when verifyStableReadTarget detects 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 --follow or 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 and tryReadJson re‑routing. (3) Verified JS syntax with node --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):

    18:43:10+00:00 error [gateway-http] unhandled error in request handler:
      JsonFileReadError: Failed to read JSON file:
        /home/ubuntu/.openclaw/devices/paired.json
      [cause]: Error: File changed during read:
        /home/ubuntu/.openclaw/devices/paired.json
        at verifyStableReadTarget (regular-file-6GdZVPgG.js:152:131)
    

    After (1.5+ hours of runtime — zero JsonFileReadError errors):

    # Gateway process (started 2026-05-20 07:53 UTC, 1h28m uptime):
      ubuntu  1338372  node openclaw/dist/index.js gateway --port 18789
    
    # Dist file confirmed patched:
      $ grep -c 'withRetryOnFileChanged' json-files-1SmAauRO.js
      3
    
    # Logs — no JsonFileReadError found in any log output
      May 20 08:30:55 node[1338372]: [agent/embedded] incomplete turn detected …
      (normal lifecycle event, not a read error)
    

    Unit tests (15/15 passed, including retry coverage):

     ✓ retries on transient File changed during read and succeeds (54ms)
     ✓ throws JsonFileReadError after exhausting retries (159ms)
     ✓ tryReadJson returns null after exhausting retries (164ms)
    
    Test Files  1 passed (1)
         Tests  15 passed (15)
    
  • Observed result after fix: The JsonFileReadError: File changed during read error 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

@openclaw-barnacle openclaw-barnacle Bot added size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 19, 2026
@clawsweeper

clawsweeper Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: passed.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
This PR wraps readJson, readJsonIfExists, and tryReadJson with bounded retries for transient File changed during read races and adds JSON helper retry coverage.

Reproducibility: yes. source-reproducible: current main reads pairing JSON through one-shot @openclaw/fs-safe async readers, and [email protected] throws File changed during read when file identity changes during open. I did not run a live concurrent gateway reproduction in this read-only review.

PR rating
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Summary: Good mergeable-quality PR: proof is strong and the implementation is focused, with only the upstream ownership cleanup path left to track.

Rank-up moves:

  • Decide whether to merge the local wrapper now or wait for the upstream @openclaw/fs-safe retry PR and package bump.
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
Sufficient (live_output): The PR body includes before logs, patched-runtime evidence, post-fix gateway uptime/log output, and retry test output that directly show the backend behavior after the fix.

Risk before merge

Maintainer options:

  1. Decide the mitigation before merge
    Land one bounded async JSON read retry path; prefer the @openclaw/fs-safe package-level fix when available, or merge this local mitigation now with an explicit follow-up to remove it after the dependency release.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge
No ClawSweeper repair lane is needed; continue the normal automerge or maintainer merge path once exact-head checks and any maintainer-owned changelog step are satisfied.

Security
Cleared: The diff changes local JSON helper logic and tests only; it does not alter dependencies, scripts, CI permissions, secrets, or downloaded code.

Review details

Best possible solution:

Land one bounded async JSON read retry path; prefer the @openclaw/fs-safe package-level fix when available, or merge this local mitigation now with an explicit follow-up to remove it after the dependency release.

Do we have a high-confidence way to reproduce the issue?

Yes, source-reproducible: current main reads pairing JSON through one-shot @openclaw/fs-safe async readers, and [email protected] throws File changed during read when file identity changes during open. I did not run a live concurrent gateway reproduction in this read-only review.

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:

  • P1: The PR targets a live gateway/device-state read race that can disrupt active pairing and device-management workflows.
  • rating: 🐚 platinum hermit: Current PR rating is 🐚 platinum hermit because proof is 🦞 diamond lobster, patch quality is 🐚 platinum hermit, and Good mergeable-quality PR: proof is strong and the implementation is focused, with only the upstream ownership cleanup path left to track.
  • status: 🚀 automerge armed: This PR is in ClawSweeper's automerge lane. Sufficient (live_output): The PR body includes before logs, patched-runtime evidence, post-fix gateway uptime/log output, and retry test output that directly show the backend behavior after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes before logs, patched-runtime evidence, post-fix gateway uptime/log output, and retry test output that directly show the backend behavior after the fix.

What I checked:

  • Current main delegates async JSON reads without retry: src/infra/json-files.ts currently re-exports readJson, readJsonIfExists, and tryReadJson directly from @openclaw/fs-safe/json, so OpenClaw has no local retry on current main. (src/infra/json-files.ts:4, b33deb41594e)
  • Device pairing state uses the affected helper: Device pairing loads pending.json and paired.json through readJsonIfExists, so a transient stable-target race rejects the whole pairing state load on current main. (src/infra/device-pairing.ts:160, b33deb41594e)
  • Node pairing state uses the same helper: Node pairing also reads pending and paired state through readJsonIfExists, so the PR's central helper change covers both device and node pairing stores. (src/infra/node-pairing.ts:230, b33deb41594e)
  • Pinned dependency contract: OpenClaw pins @openclaw/fs-safe 0.2.7; that package's async JSON readers call readRegularFile once, and readRegularFile throws File changed during read when file identity changes between stat/open/path verification. (package.json:1783, b33deb41594e)
  • PR implements bounded retry: The PR head adds three-attempt, 50ms exponential retry logic that walks the error cause chain and preserves strict reader contracts for readJson and readJsonFileStrict. (src/infra/json-files.ts:22, 13dbb09605ab)
  • PR adds focused regression coverage: The PR head adds retry-success, retry-exhaustion, and tryReadJson exhaustion coverage around File changed during read. (src/infra/json-files.test.ts:192, 13dbb09605ab)

Likely related people:

  • @steipete: History across the central JSON and pairing files shows Peter Steinberger as the highest-volume contributor and author of multiple recent pairing/auth hardening and refactor commits in this area. (role: primary area contributor; confidence: high; commits: a185ca283a74, 6c82a91d3dc5, b660493e5466; files: src/infra/json-files.ts, src/infra/device-pairing.ts, src/infra/node-pairing.ts)
  • @joshavant: Current-main blame attributes the latest versions of the central files to Josh Avant's broad update, and earlier history includes device-pairing visibility work. (role: recent area contributor; confidence: medium; commits: 40db92f6097c, 8e132aed6eea, 6309b1da6c2e; files: src/infra/json-files.ts, src/infra/device-pairing.ts, src/infra/node-pairing.ts)
  • @ngutman: Recent history shows Nimrod Gutman working on src/infra/device-pairing.ts around mobile metadata refresh and adjacent pairing behavior. (role: recent adjacent contributor; confidence: medium; commits: a7ab09fa4e79, f3c304917acb, b328c6611527; files: src/infra/device-pairing.ts)
  • @yetval: The open @openclaw/fs-safe PR implements the dependency-level retry contract for the same race, making this a relevant routing candidate for package-level ownership. (role: upstream dependency fix author; confidence: medium; commits: 66604d6adc94; files: openclaw/fs-safe:src/json.ts, openclaw/fs-safe:src/regular-file.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against b33deb41594e.

@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 19, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P1 High-priority user-facing bug, regression, or broken workflow. labels May 19, 2026
@samson1357924
samson1357924 force-pushed the fix/paired-json-retry branch from 723e179 to 45da87f Compare May 20, 2026 08:02
@samson1357924

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels May 20, 2026
@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper PR egg

✨ Hatched: 🥚 common Tiny Shellbean

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.

Rarity: 🥚 common.
Trait: keeps receipts.
Image traits: location proof lagoon; accessory CI status badge; palette rose quartz and slate; mood bright-eyed; pose sitting proudly on a smooth stone; shell paper lantern shell; lighting calm overcast light; background small green status lights.
Share on X: post this hatch
Copy: My PR egg hatched a 🥚 common Tiny Shellbean in ClawSweeper.

What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@samson1357924

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. label May 20, 2026
@samson1357924

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@samson1357924

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels May 20, 2026
@Takhoffman

Copy link
Copy Markdown
Contributor

@clawsweeper automerge

@clawsweeper clawsweeper Bot added the clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge label May 21, 2026
@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

🦞🔧
ClawSweeper automerge is enabled.

Draft PRs stay fix-only until GitHub marks them ready for review. Pause with /clawsweeper stop.

Automerge progress:

  • 2026-05-21 15:38:35 UTC review queued dc53ea3d5f77 (queued)
  • 2026-05-21 16:39:49 UTC review passed 13dbb09605ab (structured ClawSweeper verdict: pass (sha=13dbb09605abd25936dbe01645e772c2e4f92...)
  • 2026-05-21 15:38:35 UTC review queued 13dbb09605ab (queued)

@clawsweeper clawsweeper Bot added the clawsweeper:human-review Needs maintainer review before ClawSweeper can continue label May 21, 2026
@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

🦞✅
ClawSweeper is pausing this repair loop for human review.

Source: clawsweeper[bot]
Reason: No ClawSweeper repair lane is needed; maintainer review should choose wrapper-versus-upstream timing and merge normally if that ownership choice is acceptable.; Cleared: The diff only changes local JSON helper logic and tests; it does not alter dependencies, CI, scripts, secrets, permissions, or downloaded artifacts. (sha=dc53ea3d5f77bd67a5c0c9b299a4d7ff61c72e2e)

Why human review is needed:
This item has security-sensitive risk. ClawSweeper is pausing instead of making an autonomous change that could affect trust, credentials, permissions, or exposure.

Recommended next action:
Have a maintainer review the security-sensitive detail and provide an explicit safe path before asking ClawSweeper to continue.

I added clawsweeper:human-review and left the final call with a maintainer.

@clawsweeper clawsweeper Bot added status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. clawsweeper:human-review Needs maintainer review before ClawSweeper can continue labels May 21, 2026
@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper

clawsweeper Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

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.
Replacement PR: #85029
Why close: this run explicitly closes the superseded source PR after the credited replacement PR is open, so review continues in one place.
Closing this one because the run was configured to close superseded source PRs after opening the replacement.
Attribution stays attached; the replacement just gives the fix a writable branch.
Co-author credit kept:

fish notes: model gpt-5.5, reasoning high; reviewed against 00602a1.

@clawsweeper clawsweeper Bot closed this May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:automerge Maintainer opted this PR into bounded ClawSweeper-reviewed automerge P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: M status: 🚀 automerge armed This PR is in ClawSweeper's automerge lane.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSON file reads should retry on transient race conditions

2 participants