From cb505dd8e7b0ab80497e252893701d45882490c6 Mon Sep 17 00:00:00 2001 From: aakashsharma7 Date: Sun, 6 Sep 2026 11:15:32 +0530 Subject: [PATCH] fix(usage): serialize addSession to prevent lost updates on concurrent writes Serialize read-modify-write calls to usage.json using an in-process Promise-chain lock and write to a temporary file before atomically renaming into place. This prevents concurrent session completions (such as simultaneous subagent runs) from overwriting each other's session usage data and lifetime totals. --- .changeset/fix-add-session-concurrency.md | 5 + source/usage/storage.spec.ts | 118 +++++++++++++--------- source/usage/storage.ts | 69 ++++++++++--- 3 files changed, 132 insertions(+), 60 deletions(-) create mode 100644 .changeset/fix-add-session-concurrency.md diff --git a/.changeset/fix-add-session-concurrency.md b/.changeset/fix-add-session-concurrency.md new file mode 100644 index 000000000..39cee8620 --- /dev/null +++ b/.changeset/fix-add-session-concurrency.md @@ -0,0 +1,5 @@ +--- +'@nanocollective/nanocoder': patch +--- + +Fix race condition in `addSession()` by introducing atomic temp-file writes and serialized in-process locking to prevent lost updates on concurrent session completions. diff --git a/source/usage/storage.spec.ts b/source/usage/storage.spec.ts index 85574a74f..e01d6a2b6 100644 --- a/source/usage/storage.spec.ts +++ b/source/usage/storage.spec.ts @@ -377,10 +377,10 @@ test('writeUsageData handles write errors gracefully', t => { // addSession Tests // ============================================================================ -test('addSession adds new session to empty data', t => { +test('addSession adds new session to empty data', async t => { const session = createMockSession(); - addSession(session); + await addSession(session); const data = readUsageData(); t.is(data.sessions.length, 1); @@ -388,12 +388,12 @@ test('addSession adds new session to empty data', t => { t.is(data.totalLifetime, session.tokens.total); }); -test('addSession adds session to beginning of list', t => { +test('addSession adds session to beginning of list', async t => { const session1 = createMockSession('provider1', 'model1', 1000); const session2 = createMockSession('provider2', 'model2', 2000); - addSession(session1); - addSession(session2); + await addSession(session1); + await addSession(session2); const data = readUsageData(); t.is(data.sessions.length, 2); @@ -401,32 +401,32 @@ test('addSession adds session to beginning of list', t => { t.is(data.sessions[1]!.id, session1.id); }); -test('addSession updates total lifetime tokens', t => { +test('addSession updates total lifetime tokens', async t => { const session1 = createMockSession('provider1', 'model1', 1000); const session2 = createMockSession('provider2', 'model2', 2000); - addSession(session1); - addSession(session2); + await addSession(session1); + await addSession(session2); const data = readUsageData(); t.is(data.totalLifetime, 3000); }); -test('addSession limits sessions to MAX_SESSIONS (100)', t => { +test('addSession limits sessions to MAX_SESSIONS (100)', async t => { // Add 101 sessions for (let i = 0; i < 101; i++) { const session = createMockSession(`provider-${i}`, `model-${i}`, 100); - addSession(session); + await addSession(session); } const data = readUsageData(); t.is(data.sessions.length, 100); // Should be limited to 100 }); -test('addSession creates daily aggregate', t => { +test('addSession creates daily aggregate', async t => { const session = createMockSession(); - addSession(session); + await addSession(session); const data = readUsageData(); t.is(data.dailyAggregates.length, 1); @@ -439,12 +439,12 @@ test('addSession creates daily aggregate', t => { t.is(aggregate!.totalTokens, session.tokens.total); }); -test('addSession updates existing daily aggregate', t => { +test('addSession updates existing daily aggregate', async t => { const session1 = createMockSession('provider1', 'model1', 1000); const session2 = createMockSession('provider2', 'model2', 2000); - addSession(session1); - addSession(session2); + await addSession(session1); + await addSession(session2); const data = readUsageData(); const today = new Date().toISOString().split('T')[0]; @@ -455,14 +455,14 @@ test('addSession updates existing daily aggregate', t => { t.is(aggregate!.totalTokens, 3000); }); -test('addSession tracks provider stats in daily aggregate', t => { +test('addSession tracks provider stats in daily aggregate', async t => { const session1 = createMockSession('openai', 'gpt-4', 1000); const session2 = createMockSession('openai', 'gpt-3.5', 2000); const session3 = createMockSession('anthropic', 'claude', 3000); - addSession(session1); - addSession(session2); - addSession(session3); + await addSession(session1); + await addSession(session2); + await addSession(session3); const data = readUsageData(); const today = new Date().toISOString().split('T')[0]; @@ -473,14 +473,14 @@ test('addSession tracks provider stats in daily aggregate', t => { t.is(aggregate!.providers.anthropic, 3000); }); -test('addSession tracks model stats in daily aggregate', t => { +test('addSession tracks model stats in daily aggregate', async t => { const session1 = createMockSession('openai', 'gpt-4', 1000); const session2 = createMockSession('openai', 'gpt-4', 2000); const session3 = createMockSession('openai', 'gpt-3.5', 3000); - addSession(session1); - addSession(session2); - addSession(session3); + await addSession(session1); + await addSession(session2); + await addSession(session3); const data = readUsageData(); const today = new Date().toISOString().split('T')[0]; @@ -491,19 +491,45 @@ test('addSession tracks model stats in daily aggregate', t => { t.is(aggregate!.models['gpt-3.5'], 3000); }); -test('addSession limits daily aggregates to MAX_DAILY_AGGREGATES (30)', t => { +test('addSession limits daily aggregates to MAX_DAILY_AGGREGATES (30)', async t => { // Create sessions with different dates for (let i = 0; i < 35; i++) { const session = createMockSession(); // Modify timestamp to be i days ago session.timestamp = Date.now() - i * 24 * 60 * 60 * 1000; - addSession(session); + await addSession(session); } const data = readUsageData(); t.is(data.dailyAggregates.length, 30); // Should be limited to 30 }); +test('addSession serializes concurrent calls without lost updates', async t => { + const session1 = createMockSession('provider1', 'model1', 1000); + const session2 = createMockSession('provider2', 'model2', 2000); + const session3 = createMockSession('provider3', 'model3', 3000); + + // Simulate simultaneous session completions + await Promise.all([ + addSession(session1), + addSession(session2), + addSession(session3), + ]); + + const data = readUsageData(); + t.is(data.sessions.length, 3); + t.is(data.totalLifetime, 6000); + + const today = new Date().toISOString().split('T')[0]; + const aggregate = data.dailyAggregates.find(agg => agg.date === today); + t.truthy(aggregate); + t.is(aggregate!.sessions, 3); + t.is(aggregate!.totalTokens, 6000); + t.is(aggregate!.providers.provider1, 1000); + t.is(aggregate!.providers.provider2, 2000); + t.is(aggregate!.providers.provider3, 3000); +}); + // ============================================================================ // getTodayAggregate Tests // ============================================================================ @@ -513,19 +539,19 @@ test('getTodayAggregate returns null when no data exists', t => { t.is(aggregate, null); }); -test('getTodayAggregate returns null when no sessions today', t => { +test('getTodayAggregate returns null when no sessions today', async t => { // Add a session from yesterday const session = createMockSession(); session.timestamp = Date.now() - 24 * 60 * 60 * 1000; - addSession(session); + await addSession(session); const aggregate = getTodayAggregate(); t.is(aggregate, null); }); -test('getTodayAggregate returns today aggregate', t => { +test('getTodayAggregate returns today aggregate', async t => { const session = createMockSession('openai', 'gpt-4', 1000); - addSession(session); + await addSession(session); const aggregate = getTodayAggregate(); @@ -547,12 +573,12 @@ test('getLastNDaysAggregate returns zero when no data exists', t => { t.is(result.avgTokensPerDay, 0); }); -test('getLastNDaysAggregate calculates totals for last 7 days', t => { +test('getLastNDaysAggregate calculates totals for last 7 days', async t => { // Add sessions for the past 5 days for (let i = 0; i < 5; i++) { const session = createMockSession('openai', 'gpt-4', 1000); session.timestamp = Date.now() - i * 24 * 60 * 60 * 1000; - addSession(session); + await addSession(session); } const result = getLastNDaysAggregate(7); @@ -562,19 +588,19 @@ test('getLastNDaysAggregate calculates totals for last 7 days', t => { t.is(result.avgTokensPerDay, Math.round(5000 / 7)); }); -test('getLastNDaysAggregate filters out older sessions', t => { +test('getLastNDaysAggregate filters out older sessions', async t => { // Add 3 sessions within last 7 days for (let i = 0; i < 3; i++) { const session = createMockSession('openai', 'gpt-4', 1000); session.timestamp = Date.now() - i * 24 * 60 * 60 * 1000; - addSession(session); + await addSession(session); } // Add 2 sessions older than 7 days for (let i = 8; i < 10; i++) { const session = createMockSession('openai', 'gpt-4', 1000); session.timestamp = Date.now() - i * 24 * 60 * 60 * 1000; - addSession(session); + await addSession(session); } const result = getLastNDaysAggregate(7); @@ -584,12 +610,12 @@ test('getLastNDaysAggregate filters out older sessions', t => { t.is(result.totalSessions, 3); }); -test('getLastNDaysAggregate handles different day ranges', t => { +test('getLastNDaysAggregate handles different day ranges', async t => { // Add sessions for the past 10 days for (let i = 0; i < 10; i++) { const session = createMockSession('openai', 'gpt-4', 1000); session.timestamp = Date.now() - i * 24 * 60 * 60 * 1000; - addSession(session); + await addSession(session); } const result7 = getLastNDaysAggregate(7); @@ -601,12 +627,12 @@ test('getLastNDaysAggregate handles different day ranges', t => { t.is(result30.totalTokens, 10000); }); -test('getLastNDaysAggregate calculates average correctly', t => { +test('getLastNDaysAggregate calculates average correctly', async t => { // Add 10 sessions over 5 days for (let i = 0; i < 10; i++) { const session = createMockSession('openai', 'gpt-4', 500); session.timestamp = Date.now() - (i % 5) * 24 * 60 * 60 * 1000; - addSession(session); + await addSession(session); } const result = getLastNDaysAggregate(7); @@ -621,10 +647,10 @@ test('getLastNDaysAggregate calculates average correctly', t => { // clearUsageData Tests // ============================================================================ -test('clearUsageData removes usage file', t => { +test('clearUsageData removes usage file', async t => { // Add some data first const session = createMockSession(); - addSession(session); + await addSession(session); // Verify data exists let data = readUsageData(); @@ -644,9 +670,9 @@ test('clearUsageData handles non-existent file', t => { t.notThrows(() => clearUsageData()); }); -test('clearUsageData is idempotent', t => { +test('clearUsageData is idempotent', async t => { // Add and clear data - addSession(createMockSession()); + await addSession(createMockSession()); clearUsageData(); // Clear again @@ -661,7 +687,7 @@ test('clearUsageData is idempotent', t => { // Integration Tests // ============================================================================ -test('complete usage tracking flow', t => { +test('complete usage tracking flow', async t => { // Start with empty data let data = readUsageData(); t.is(data.sessions.length, 0); @@ -671,9 +697,9 @@ test('complete usage tracking flow', t => { const session2 = createMockSession('anthropic', 'claude', 2000); const session3 = createMockSession('openai', 'gpt-3.5', 1500); - addSession(session1); - addSession(session2); - addSession(session3); + await addSession(session1); + await addSession(session2); + await addSession(session3); // Verify sessions added data = readUsageData(); diff --git a/source/usage/storage.ts b/source/usage/storage.ts index d354750f5..800c77d01 100644 --- a/source/usage/storage.ts +++ b/source/usage/storage.ts @@ -3,6 +3,7 @@ * Persists usage statistics to the app data directory */ +import * as crypto from 'node:crypto'; import * as fs from 'node:fs'; import * as path from 'node:path'; import {getAppDataPath, getConfigPath} from '@/config/paths'; @@ -114,6 +115,43 @@ export function readUsageData(): UsageData { } } +/** + * Write data to a temp file then atomically rename into place. + */ +function atomicWriteFileSync(filePath: string, data: string): void { + const tmpPath = `${filePath}.${crypto.randomUUID()}.tmp`; + try { + fs.writeFileSync(tmpPath, data, 'utf-8'); + fs.renameSync(tmpPath, filePath); + } catch (error) { + try { + if (fs.existsSync(tmpPath)) { + fs.unlinkSync(tmpPath); + } + } catch { + // Ignore cleanup errors + } + throw error; + } +} + +/** Serializes read-modify-write of usage.json to prevent lost updates from concurrent session completion */ +let usageWriteLock: Promise = Promise.resolve(); + +async function withUsageLock(fn: () => Promise): Promise { + const prev = usageWriteLock; + let release!: () => void; + usageWriteLock = new Promise(r => { + release = r; + }); + await prev; + try { + return await fn(); + } finally { + release(); + } +} + export function writeUsageData(data: UsageData): void { try { ensureAppDataDir(); @@ -121,7 +159,7 @@ export function writeUsageData(data: UsageData): void { data.lastUpdated = Date.now(); const filePath = getUsageFilePath(); - fs.writeFileSync(filePath, JSON.stringify(data, null, 2), 'utf-8'); + atomicWriteFileSync(filePath, JSON.stringify(data, null, 2)); } catch (error) { logWarning('Failed to write usage data:', true, { context: {error}, @@ -129,24 +167,26 @@ export function writeUsageData(data: UsageData): void { } } -export function addSession(session: SessionUsage): void { - const data = readUsageData(); +export async function addSession(session: SessionUsage): Promise { + return withUsageLock(async () => { + const data = readUsageData(); - // Add session to the beginning (most recent first) - data.sessions.unshift(session); + // Add session to the beginning (most recent first) + data.sessions.unshift(session); - // Keep only last MAX_USAGE_SESSIONS - if (data.sessions.length > MAX_USAGE_SESSIONS) { - data.sessions = data.sessions.slice(0, MAX_USAGE_SESSIONS); - } + // Keep only last MAX_USAGE_SESSIONS + if (data.sessions.length > MAX_USAGE_SESSIONS) { + data.sessions = data.sessions.slice(0, MAX_USAGE_SESSIONS); + } - // Update lifetime total - data.totalLifetime += session.tokens.total; + // Update lifetime total + data.totalLifetime += session.tokens.total; - // Update daily aggregate - updateDailyAggregate(data, session); + // Update daily aggregate + updateDailyAggregate(data, session); - writeUsageData(data); + writeUsageData(data); + }); } function updateDailyAggregate(data: UsageData, session: SessionUsage): void { @@ -229,6 +269,7 @@ export function getLastNDaysAggregate(days: number): { * Clear all usage data */ export function clearUsageData(): void { + usageWriteLock = Promise.resolve(); try { const filePath = getUsageFilePath(); if (fs.existsSync(filePath)) {