fix(usage): serialize addSession to prevent lost updates on concurrent writes - #1201
fix(usage): serialize addSession to prevent lost updates on concurrent writes#1201aakash-env wants to merge 1 commit into
Conversation
will-lamerton
left a comment
There was a problem hiding this comment.
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.tsalready exportsatomicWriteFileSyncwith the same signature, plus temp-file cleanup on failure. The private copy here omits that, so a failedrenameSyncorphansusage.json.<uuid>.tmppermanently. Import the shared one. (source/stats/storage.tsopen-codes a third variant - consolidating all three would be genuinely useful.)- Spec scope creep: the
writeUsageData handles write errors gracefullyrewrite and the newclearUsageData handles unlink errors gracefullytest 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.
fca4f0f to
6ae4a38
Compare
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 onusage.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, thenfs.renameSyncso concurrent readers never see empty or truncated JSON.In-process lock— serialize read-modify-write with a Promise-chain lock (
withUsageLock), same pattern assession-manager.ts.addSessionis async — now returnsPromise<void>and runs insidewithUsageLock.Changeset + test — patch changeset, plus a concurrency regression test that fires several sessions with
Promise.all.Testing