Skip to content

add non-durable atomic write option#9

Open
sallyom wants to merge 1 commit intoopenclaw:mainfrom
sallyom:oc-issue-73655
Open

add non-durable atomic write option#9
sallyom wants to merge 1 commit intoopenclaw:mainfrom
sallyom:oc-issue-73655

Conversation

@sallyom
Copy link
Copy Markdown

@sallyom sallyom commented May 6, 2026

Supports the OpenClaw fix for gateway stalls reported in openclaw/openclaw#73655, where frequent session-store writes held the session-write lock across fsync and caused long lock holds, event-loop starvation, polling stalls, cron timeouts, and slow session/status commands. This adds an opt-out for expensive fsync work on atomic text/JSON writes so hot, reconstructible metadata can keep atomic replace behavior without blocking latency-sensitive paths.
openclaw PR from before fs-safe: openclaw/openclaw#76881

Signed-off-by: sallyom <somalley@redhat.com>
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 6, 2026

Codex review: needs real behavior proof before merge.

Summary
The PR adds a default-durable durable?: boolean option to async writeTextAtomic and writeJson, with tests and an Unreleased changelog entry.

Reproducibility: not applicable. as a fs-safe bug reproduction; this is a new public API option. Source inspection does verify that current main has no durable option and always requests both fsync steps for async text/JSON atomic writes.

Real behavior proof
Needs real behavior proof before merge: The PR has related OpenClaw context but no after-fix terminal output, logs, screenshot, recording, or linked artifact showing this fs-safe branch running. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Maintainers need to decide the public durability API/docs contract and require contributor real behavior proof; this is not a safe automated repair-only path.

Security
Cleared: The diff touches TypeScript helper code, tests, and changelog only, with no workflow, dependency, package metadata, secret, permission, or supply-chain changes.

Review findings

  • [P3] Document the new durability option in public docs — src/text-atomic.ts:7-13
Review details

Best possible solution:

Land a narrow fs-safe API update that keeps durability enabled by default, exposes the opt-out only where maintainers want that contract, documents the tradeoff, and gives OpenClaw a released version to consume.

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

Not applicable as a fs-safe bug reproduction; this is a new public API option. Source inspection does verify that current main has no durable option and always requests both fsync steps for async text/JSON atomic writes.

Is this the best way to solve the issue?

Mostly yes: defaulting durable to true and mapping durable: false to the existing replaceFileAtomic fsync flags is a narrow implementation. The remaining blockers are public API/durability acceptance, docs for the tradeoff, and branch-specific real behavior proof.

Full review comments:

  • [P3] Document the new durability option in public docs — src/text-atomic.ts:7-13
    The PR adds a safety-relevant public option, but the shipped docs/atomic.md and docs/json.md option lists still describe only the old call shapes. Users should not have to inspect the TypeScript source to learn what durability is being skipped or that the default remains durable.
    Confidence: 0.78

Overall correctness: patch is correct
Overall confidence: 0.78

Acceptance criteria:

  • pnpm test test/json.test.ts
  • pnpm build
  • pnpm check

What I checked:

  • Current main always fsyncs text atomic writes: WriteTextAtomicOptions has only mode, dirMode, and trailingNewline, and writeTextAtomic passes syncTempFile: true and syncParentDir: true. (src/text-atomic.ts:3, 55327c893031)
  • Current main does not thread durability through JSON writes: writeJson accepts only mode, trailingNewline, and dirMode before forwarding to writeTextAtomic. (src/json.ts:292, 55327c893031)
  • Lower-level primitive already has fsync controls: replaceFileAtomic conditionally runs temp-file and parent-directory fsync from syncTempFile and syncParentDir, so the PR maps a public helper option onto existing primitive behavior. (src/replace-file.ts:51, 55327c893031)
  • PR diff surface: The PR modifies TypeScript helper code, focused JSON tests, and CHANGELOG.md; it does not change dependencies, workflows, package metadata, release scripts, or generated artifacts. (src/text-atomic.ts:7, e1d870061e81)
  • Real behavior proof missing: The PR body links related OpenClaw context, but the PR body and comments do not include terminal output, logs, screenshots, recordings, or artifacts showing this fs-safe branch running after the change. (e1d870061e81)
  • Ownership history: Blame and local history for the central atomic/JSON files point to steipete as the original and recent maintainer for this surface. (src/text-atomic.ts:1, cbe59d156a0d)

Likely related people:

  • steipete: Current blame for src/text-atomic.ts and the conditional fsync path points to Peter Steinberger, and local history shows recent work on the root JSON and durable queue helpers in the same area. (role: introduced behavior and recent maintainer; confidence: high; commits: cbe59d156a0d, 91f7b74ad653; files: src/text-atomic.ts, src/json.ts, src/replace-file.ts)

Remaining risk / open question:

  • The new durable: false option is a public durability policy knob, so maintainers should explicitly accept the API contract and caller documentation for opting out of fsync.
  • The PR does not yet include after-fix proof from this fs-safe branch; tests and linked OpenClaw context are supplemental only.

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

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