[Bug] jsonStore({ lock: true }) silently drops concurrent in-process updates: lock reentrancy is process-scoped, not owner-scoped

[yetval]

Summary

jsonStore({ lock: true }) silently loses writes when two or more update / updateOr / write calls on the same file overlap inside a single process. The second caller is handed the lock the first caller is still holding, both read the same value, both write, and one update disappears with no error and no warning. Locking across separate processes works correctly, so the defect is confined to the in-process reentrancy branch.

src/json-document-store.ts:94 passes allowReentrant: true on every locked JSON store operation, and the reentrancy check in src/sidecar-lock.ts:286-297 is keyed only on the target path plus "this process already holds it". There is no owner token and no AsyncLocalStorage scope, so an unrelated concurrent async task in the same process satisfies the reentrancy condition and is let straight through.

Environment

• Commit: 8cced2a (openclaw/fs-safe main at time of filing), package version 0.4.7
• Node: v26.0.0, macOS 26.3, APFS
• Surface: jsonStore from fs-safe/store with lock: true or any lock options object. update, updateOr and write are all affected.

Steps to reproduce

1. Clone openclaw/fs-safe at 8cced2a, install dependencies and build so dist/ exists.
2. Save this file as repro.mjs at the repo root:

// node repro.mjs
import fs from "node:fs";
import os from "node:os";
import path from "node:path";
import { spawn } from "node:child_process";
import { fileURLToPath } from "node:url";
import { setTimeout as delay } from "node:timers/promises";
import { jsonStore } from "./dist/store.js";

const self = fileURLToPath(import.meta.url);

async function bump(filePath, tag, log) {
  const store = jsonStore({ filePath, lock: true });
  await store.updateOr({ count: 0 }, async (prev) => {
    log.push(`${tag} read count=${prev.count}`);
    await delay(30);
    log.push(`${tag} write count=${prev.count + 1}`);
    return { count: prev.count + 1 };
  });
}

if (process.argv[2] === "child") {
  const log = [];
  await bump(process.argv[3], `child${process.argv[4]}`, log);
  console.log(log.map((line) => `  ${line}`).join("\n"));
  process.exit(0);
}

const dir = fs.mkdtempSync(path.join(os.tmpdir(), "fs-safe-repro-"));
const onDisk = (p) => fs.readFileSync(p, "utf8").replace(/\s+/g, " ").trim();

// CASE 1: three updaters in ONE process, staggered so each arrives while the
// previous one still holds the lock.
const inproc = path.join(dir, "inproc.json");
await jsonStore({ filePath: inproc, lock: true }).write({ count: 0 });
const log = [];
await Promise.all(
  [0, 1, 2].map(async (i) => {
    await delay(i * 10);
    await bump(inproc, `A${i}`, log);
  }),
);
console.log("CASE 1: three updaters, ONE process, jsonStore({ lock: true })");
console.log(log.map((line) => `  ${line}`).join("\n"));
console.log(`  on disk: ${onDisk(inproc)}    (expected count 3)`);

// CASE 2 (control): the same three updaters, one per process.
const xproc = path.join(dir, "xproc.json");
await jsonStore({ filePath: xproc, lock: true }).write({ count: 0 });
console.log("\nCASE 2 (control): three updaters, THREE processes, same options");
await Promise.all(
  [0, 1, 2].map(
    (i) =>
      new Promise((resolve) => {
        setTimeout(() => {
          spawn(process.execPath, [self, "child", xproc, String(i)], {
            stdio: "inherit",
          }).on("exit", resolve);
        }, i * 10);
      }),
  ),
);
console.log(`  on disk: ${onDisk(xproc)}    (expected count 3)`);

3. Run node repro.mjs.

Expected

Three updateOr calls against the same locked store increment the counter to 3, whether they originate in one process or three. docs/json-store.md:117 states that the whole read, run, write sequence runs inside one withLock call.

Actual

In one process the three calls all read count=0 and all write count=1. Final on-disk value is { "count": 1 }. Two updates are gone. No error is raised, no lock timeout, no diagnostic. The same three calls run one per process serialize correctly and produce { "count": 3 }.

At higher concurrency the loss is severe: 40 rounds of 10 staggered update() calls on a locked store lost 273 of 400 updates.

Second observable symptom, same root cause

Because the concurrent entrants are inside the atomic-write path at the same time, they also collide on the temp file and identity checks and intermittently surface a spurious FsSafeError with code path-mismatch at the caller. Observed across the same 400-update run on current main:

2x  path-mismatch: path changed during read
12x path-mismatch: unable to resolve opened file path
3x  path-mismatch: path mismatch

These errors disappear entirely once the reentrancy branch stops admitting unrelated callers, which is what points them at the same root cause rather than at a separate atomic-write defect.

Root cause

src/json-document-store.ts:83-99:

async function withOptionalLock<R>(run: () => Promise<R>): Promise<R> {
  if (!locks || !lockOptions) {
    return await run();
  }
  return await locks.withLock(
    {
      targetPath: adapter.filePath,
      staleMs: lockOptions.staleMs,
      timeoutMs: lockOptions.timeoutMs,
      retry: lockOptions.retry,
      staleRecovery: lockOptions.staleRecovery,
      allowReentrant: true,
      payload: () => ({ pid: process.pid, createdAt: new Date().toISOString() }),
    },
    run,
  );
}

src/sidecar-lock.ts:286-297:

const held = state.held.get(normalizedTargetPath);
if (held && options.allowReentrant) {
  held.count += 1;
  const release = () =>
    releaseHeldLock(state, normalizedTargetPath, held).then(() => undefined);
  return { lockPath, normalizedTargetPath, release, [Symbol.asyncDispose]: release };
}

The condition is "some code in this process holds this path". It carries no notion of which logical owner or call chain holds it. A second, unrelated async task therefore matches, bumps held.count, and enters the critical section concurrently with the first.

Worth noting for anyone reaching for the obvious objection that reentrancy is needed for nesting: jsonStore does not nest its own locks. update, updateOr and write (src/json-document-store.ts:105-121) all call the internal unlocked read / write helpers inside a single withOptionalLock. So allowReentrant: true is not required by the library's own structure here.

Documentation

Not filing this as a flat documentation contradiction. docs/json-store.md:116 is careful and correct: it says concurrent updaters "from different processes serialize cleanly", and that case does work. The softer gap is docs/json-store.md:146, which says the default managerKey means "two jsonStore calls on the same file share lock state automatically". Sharing lock state reads as an assurance that in-process calls serialize, when in practice sharing the manager is exactly what routes the second caller into the reentrancy branch and lets it through.

Two directions, maintainer's call

This is a semantics decision rather than an obvious patch, which is why it is an issue and not a PR.

1. Drop allowReentrant: true at src/json-document-store.ts:94. One line, and it makes concurrent in-process updaters serialize correctly (verified below). The cost is that a caller who re-enters the same store from inside its own update callback moves from "works" to "blocks until the lock timeout". Verified on the current build: such a nested call succeeds today, and after removing the flag it fails with file_lock_timeout. That may be the right trade, since losing a write silently is worse than a loud timeout, but it is a behavior change for any existing caller doing that.
2. Make reentrancy owner-scoped rather than process-scoped, via AsyncLocalStorage or an explicit owner token threaded through SidecarLockAcquireOptions, so the reentrancy branch fires only for the same logical holder. Preserves nesting and fixes the data loss, at the cost of more machinery in sidecar-lock.ts.

Novelty

Checked open and closed issues and pull requests on this repo. Nothing covers this. The only open pull request, #59, refactors sidecar-lock.ts and moves the allowReentrant?: boolean declaration into a new sidecar-lock-types.ts, but it does not change the reentrancy condition and does not touch src/json-document-store.ts, so the defect survives it.

Real behavior proof

Behavior addressed: jsonStore({ lock: true }) silently discards concurrent in-process updates because lock reentrancy is scoped to the process rather than to the logical owner.
Real environment tested: real fs-safe runtime driven through the public jsonStore API from dist/store.js, writing to a real temp directory on APFS, at commit 8cced2a on Node v26.0.0 / macOS 26.3.
Exact steps or command run after this patch: node repro.mjs (source above), then the same script against a build with the single allowReentrant: true line removed from json-document-store.
Evidence after fix:

# OBSERVED on origin/main 8cced2ab62640577005974cc7cf06136dbb1867a
CASE 1: three updaters, ONE process, jsonStore({ lock: true })
  A0 read count=0
  A1 read count=0
  A2 read count=0
  A0 write count=1
  A1 write count=1
  A2 write count=1
  on disk: { "count": 1 }    (expected count 3)

CASE 2 (control): three updaters, THREE processes, same options
  child0 read count=0
  child0 write count=1
  child2 read count=1
  child2 write count=2
  child1 read count=2
  child1 write count=3
  on disk: { "count": 3 }    (expected count 3)

# EXPECTED, identical inputs, same build with `allowReentrant: true` removed
CASE 1: three updaters, ONE process, jsonStore({ lock: true })
  A0 read count=0
  A0 write count=1
  A1 read count=1
  A1 write count=2
  A2 read count=2
  A2 write count=3
  on disk: { "count": 3 }    (expected count 3)

CASE 2 (control): three updaters, THREE processes, same options
  child0 read count=0
  child0 write count=1
  child2 read count=1
  child2 write count=2
  child1 read count=2
  child1 write count=3
  on disk: { "count": 3 }    (expected count 3)

# Volume run, 40 rounds x 10 staggered update() calls on a locked store
# on origin/main 8cced2ab62640577005974cc7cf06136dbb1867a
40 rounds x 10 staggered update() calls, lock: true
  updates requested: 400
  updates lost:      273
  errors raised to the caller:
    2x path-mismatch: path changed during read
    12x path-mismatch: unable to resolve opened file path
    3x path-mismatch: path mismatch

# same volume run, same build with `allowReentrant: true` removed
40 rounds x 10 staggered update() calls, lock: true
  updates requested: 400
  updates lost:      0
  errors raised to the caller:
    (none)

# nested call semantics, the trade-off in option 1, lock timeoutMs 1000
# store.write() called from inside store.update()'s own callback
on origin/main:                             nested store call inside update(): OK, on disk { "count": 1 }
with `allowReentrant: true` removed:        nested store call inside update(): file_lock_timeout: file lock timeout for /private/var/folders/.../n.json

Observed result after fix: on current main three overlapping in-process updateOr calls all read count=0 and the file ends at { "count": 1 } instead of { "count": 3 }, while the identical three calls split across three processes end at { "count": 3 }; removing the process-scoped reentrancy flag makes the in-process case match the cross-process case exactly and eliminates the 273 lost updates and all path-mismatch errors in the volume run.
What was not tested: Windows and Linux (macOS 26.3 / APFS only), worker_threads rather than plain async tasks, configureFsSafeLocks global defaults other than the built-in ones, and the low-level acquireFileLock / withSidecarLock API used directly by callers who deliberately pass allowReentrant: true themselves. No implementation of the owner-scoped option was built or measured; the only alternative build exercised was the one-line flag removal.

[clawsweeper]

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve. Reviewed July 26, 2026, 12:30 PM ET / 16:30 UTC.

Summary
This issue remains necessary: the reported current-main path can admit unrelated in-process async callers to the same JSON-store critical section, allowing silent lost updates. The report also identifies a real compatibility decision around nested store calls, so it needs maintainer direction rather than an automatic one-line repair.

Reproducibility: yes. the supplied reproduction is high-confidence from source and includes a real current-main runtime transcript: staggered same-process updates take the reentrant branch and overwrite one another. This review did not independently execute the script, so the structured status is source-reproducible rather than reproduced.

Root-cause cluster
Relationship: canonical
Canonical: #60
Summary: This issue is the canonical report for the JSON-store in-process reentrancy defect; the open sidecar-lock refactor overlaps the implementation area but does not yet address the reported behavior.

Members:

• partial_overlap: feat: 0.5.0 API surface — archive policy, publication mechanics, sync locks, secret create-only, Root.walk, cleanup receipts #59 - It refactors the sidecar-lock surface that contains the reentrancy option, but the supplied PR context does not change the JSON-store opt-in or ownerless reentrancy condition.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Maintainer decision needed

Question	Recommendation
Should locked JSON stores preserve nested same-call-chain operations through owner-scoped reentrancy, or intentionally disallow them so every nested store operation waits and times out?	Use owner-scoped reentrancy: Preserve valid nested same-call-chain operations while forcing unrelated concurrent async callers to queue behind the lock.

Why: Both directions prevent the silent data loss, but they establish different compatibility contracts for existing callers that invoke a store again inside an update callback.

Next step
A maintainer must choose the compatibility contract for nested locked store calls before a repair can safely be implemented.

Review details

Best possible solution:

Define reentrancy as belonging only to the same logical async owner, then add regression coverage for overlapping in-process updates and nested same-owner store calls; retain a documented fail-closed behavior for unrelated callers.

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

Yes, the supplied reproduction is high-confidence from source and includes a real current-main runtime transcript: staggered same-process updates take the reentrant branch and overwrite one another. This review did not independently execute the script, so the structured status is source-reproducible rather than reproduced.

Is this the best way to solve the issue?

Unclear until maintainers choose the nested-operation contract. A one-line removal is a valid containment fix, but owner-scoped reentrancy is the narrower long-term solution if nested same-call-chain store operations are supported behavior.

AGENTS.md: found and applied where relevant.

Remaining risk / open question:

• Removing allowReentrant in the JSON-store wrapper would prevent silent concurrent writes but changes existing nested store calls from succeeding to waiting until lock timeout.
• Adding owner-scoped reentrancy needs a carefully bounded async-context or explicit-owner design so direct lock callers, nested operations, and lock release accounting remain fail-closed.

Codex review notes: model internal, reasoning high; reviewed against 8cced2ab6264.

Label changes

Label changes:

• add P0: The report demonstrates silent loss of persisted JSON updates under the public locking option, including substantial loss in a concurrent volume run.
• add impact:data-loss: Overlapping locked updates can complete without error while discarding prior callers' state changes.
• add issue-rating: 🦞 diamond lobster: Current issue advisory state selects this label.
• add clawsweeper:source-repro: Current issue advisory state selects this label.
• add clawsweeper:no-new-fix-pr: Current issue advisory state selects this label.
• add clawsweeper:needs-maintainer-review: Current issue advisory state selects this label.
• add clawsweeper:needs-product-decision: Current issue advisory state selects this label.

Label justifications:

• P0: The report demonstrates silent loss of persisted JSON updates under the public locking option, including substantial loss in a concurrent volume run.
• impact:data-loss: Overlapping locked updates can complete without error while discarding prior callers' state changes.

Evidence reviewed

What I checked:

• Current-main JSON-store call path: The report identifies src/json-document-store.ts:83-99 as routing every locked store operation through one optional lock wrapper and passing allowReentrant: true at line 94. At the supplied current-main SHA, that makes this the central surface for update, updateOr, and write. (src/json-document-store.ts:94, 8cced2ab6264)
• Reentrancy condition lacks a logical owner: The reported src/sidecar-lock.ts:286-297 branch reuses a held lock by normalized target path whenever reentrancy is enabled, incrementing the held count without an async-context or owner identity check. That is consistent with concurrent same-process callers entering one critical section. (src/sidecar-lock.ts:286, 8cced2ab6264)
• Concrete runtime reproduction: The issue includes a real macOS/APFS run against the supplied current-main SHA: three staggered in-process updateOr calls end at count 1, while the equivalent three-process control ends at count 3; a larger run reports 273 lost updates out of 400 requests. The report also shows that removing the JSON-store reentrancy flag serializes the in-process case, while changing nested-call behavior. (8cced2ab6264)
• Related work does not yet own the fix: Open pull request feat: 0.5.0 API surface — archive policy, publication mechanics, sync locks, secret create-only, Root.walk, cleanup receipts #59 refactors the sidecar-lock surface, but the supplied discussion states it does not change the reentrancy condition or the JSON-store call that enables it. It is adjacent work, not a merged fix for this issue. (src/sidecar-lock.ts, 3f77406e8ba7)

Likely related people:

• steipete: Authored the open refactor that directly touches the sidecar-lock API surface implicated by this report; that makes them a useful routing candidate for the reentrancy contract decision, although the branch is not merged. (role: adjacent lock-system contributor; confidence: low; commits: 3f77406e8ba7; files: src/sidecar-lock.ts, src/sidecar-lock-types.ts)

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.

[steipete]

Confirmed on current main (8cced2ab6264) with a deterministic in-process three-writer test: the first writer entered the locked update, then two unrelated followers started while it remained inside the callback. All three calls completed, but the final document was { "count": 1 }; the expected result was { "count": 3 }. Running the same overlap through the public jsonStore adapter also reproduced the secondary path-mismatch failure reported here.

The root cause matches the report: reentrancy is process-wide and keyed only by the normalized target path, so unrelated same-process callers can enter the same critical section. This behavior originates in b9434cd, which enabled reentrancy for JSON-store operations without a logical-owner identity.

Thanks, @yetval, for the unusually precise report and proof. We’ll coordinate the fix with the in-flight 0.5.0 lock work in #59 rather than patching this path ad hoc.

[clawsweeper]

ClawSweeper status: review started.

I am starting a fresh review of this issue: [Bug] jsonStore({ lock: true }) silently drops concurrent in-process updates: lock reentrancy is process-scoped, not owner-scoped This is item 1/1 in the current shard. Shard 0/1.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.