Skip to content

fix(usage): serialize addSession to prevent lost updates on concurrent writes - #1201

Open
aakash-env wants to merge 1 commit into
Nano-Collective:mainfrom
aakash-env:fix/usage-add-session-concurrency
Open

fix(usage): serialize addSession to prevent lost updates on concurrent writes#1201
aakash-env wants to merge 1 commit into
Nano-Collective:mainfrom
aakash-env:fix/usage-add-session-concurrency

Conversation

@aakash-env

Copy link
Copy Markdown

Summary of changes

addSession() had a race when multiple sessions finished at the same time (for example parallel subagents). Each caller did a non-atomic read-modify-write on usage.json, so a later write could replace an earlier one using a stale baseline and drop usage data.

Changes

Atomic writes — write to ${filePath}.${uuid}.tmp, then fs.renameSync so concurrent readers never see empty or truncated JSON.
In-process lock— serialize read-modify-write with a Promise-chain lock (withUsageLock), same pattern as session-manager.ts.
addSession is async — now returns Promise<void> and runs inside withUsageLock.
Changeset + test — patch changeset, plus a concurrency regression test that fires several sessions with Promise.all.

Testing

  • pnpm run test:ava source/usage/storage.spec.ts — 35 tests passed
  • pnpm run test:types — 0 errors
  • pnpm run test:knip— no unused exports
  • pnpm run test:lint— Biome clean

@will-lamerton will-lamerton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes. The in-process lock can't fix the described bug, the new test doesn't detect it, and the module has no production callers.

1. addSession has zero production callers. source/usage/storage.ts is a test-only island - every export is referenced only by its own spec. Its sole consumer source/usage/tracker.ts was deleted in dde4431 as "never wired". The live path is source/stats/ (record.ts -> stats/storage.ts). So the premise (parallel subagents dropping usage data) isn't reachable today. If concurrent ledger writes are the real concern, source/stats/storage.ts is the file to harden.

2. The lock is a no-op - the read-modify-write is fully synchronous. readUsageData/writeUsageData are existsSync/readFileSync/JSON.parse/writeFileSync/renameSync with no await in the critical section, so two addSession bodies can never interleave in one process. withUsageLock just adds a microtask hop around code that was already atomic in-process. The genuine race is cross-process (multiple CLI instances, daemon, scheduler all share usage.json), and a module-level promise chain does nothing there. That needs a real lockfile (O_EXCL / proper-lockfile).

3. The regression test passes on the unfixed code. I checked out this branch, neutralised withUsageLock so fn() runs unserialized, and ran the spec: 36/36 pass, including addSession serializes concurrent calls without lost updates. Follows from (2) - Promise.all over synchronous bodies never interleaves.

4. clearUsageData resets the lock, reintroducing the race. usageWriteLock = Promise.resolve() discards the pending chain, so a subsequent addSession no longer waits on an in-flight one. It's also not itself under the lock, so it can unlink mid-cycle and have the in-flight write resurrect the data.

Non-blocking:

  • source/utils/atomic-write.ts already exports atomicWriteFileSync with the same signature, plus temp-file cleanup on failure. The private copy here omits that, so a failed renameSync orphans usage.json.<uuid>.tmp permanently. Import the shared one. (source/stats/storage.ts open-codes a third variant - consolidating all three would be genuinely useful.)
  • Spec scope creep: the writeUsageData handles write errors gracefully rewrite and the new clearUsageData handles unlink errors gracefully test are unrelated to concurrency. The chmod change is defensible on its own (0o444 doesn't stop root, so it can false-pass in containers) but belongs in its own PR.
  • No fsync, so rename gives atomic visibility but not crash durability. Fine for usage stats, just narrower than the changeset implies.

Suggested path: either retarget at source/stats/storage.ts where the code actually runs, or split this - land the shared atomicWriteFileSync reuse, drop the promise lock, and treat cross-process safety separately with a test that spawns competing writers.

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.

2 participants