Skip to content

Commit 6307c7a

Browse files
MayurK-cmdMayurK-cmd
authored
fix(usage): publish legacy migration without clobbering a concurrent winner (#1648)
* fix(usage): publish legacy migration without clobbering a concurrent winner * fix(usage): promote no-clobber publish to atomic-write, honest race-test names * fix(usage): wx fallback for link-less filesystems, quiet ENOENT already-migrated * test: colocate publishFileNoClobber tests, fold duplicate adoption check
1 parent b3cd841 commit 6307c7a

5 files changed

Lines changed: 197 additions & 15 deletions

File tree

‎.changeset/usage-migration-no-clobber.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@nanocollective/nanocoder": patch
3+
---
4+
5+
Fixed a race in the one-time `usage.json` legacy migration: two processes migrating at once could let the loser paste its stale copy over the winner's fresher file (via the `renameSync` copy fallback). Migration now publishes with a temp file plus an atomic hard link that raises `EEXIST` when a peer won, adopting the existing file untouched instead of overwriting it.

‎source/usage/storage.spec.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,8 @@ test('migration skips when new file already exists', t => {
148148
totalLifetime: 999,
149149
lastUpdated: Date.now(),
150150
};
151-
fs.writeFileSync(newFilePath, JSON.stringify(newData), 'utf-8');
151+
const newPayload = JSON.stringify(newData);
152+
fs.writeFileSync(newFilePath, newPayload, 'utf-8');
152153

153154
process.env.NANOCODER_CONFIG_DIR = legacyConfigDir;
154155

@@ -161,6 +162,11 @@ test('migration skips when new file already exists', t => {
161162
// Legacy file should still exist (wasn't touched)
162163
t.true(fs.existsSync(legacyFilePath));
163164
t.true(fs.existsSync(newFilePath));
165+
// And the existing file must survive byte-identical: adoption means
166+
// no write of any kind, not just preserved parsed values. (The
167+
// concurrent-publish path itself is covered by the `publishFileNoClobber`
168+
// unit tests in atomic-write.spec.ts.)
169+
t.is(fs.readFileSync(newFilePath, 'utf-8'), newPayload);
164170
});
165171

166172
test('migration handles missing legacy config directory gracefully', t => {

‎source/usage/storage.ts‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import * as fs from 'node:fs';
77
import * as path from 'node:path';
88
import {getAppDataPath, getConfigPath} from '@/config/paths';
99
import {MAX_DAILY_AGGREGATES, MAX_USAGE_SESSIONS} from '@/constants';
10-
import {atomicWriteFileSync} from '@/utils/atomic-write';
10+
import {atomicWriteFileSync, publishFileNoClobber} from '@/utils/atomic-write';
1111
import {formatError} from '@/utils/error-formatter';
1212
import {logInfo, logWarning} from '@/utils/message-queue';
1313
import type {DailyAggregate, SessionUsage, UsageData} from '../types/usage';
@@ -44,25 +44,40 @@ function getUsageFilePath(): string {
4444
fs.mkdirSync(appDataDir, {recursive: true});
4545
}
4646

47+
// No-clobber publish: if a concurrent process already migrated
48+
// (or wrote) usage data, newPath exists and we adopt it
49+
// untouched. The hard link inside is atomic and raises EEXIST
50+
// when the destination appeared concurrently, so two racers can
51+
// never both publish — the loser keeps the winner's fresher
52+
// file instead of overwriting it with this stale copy.
53+
let payload: Buffer;
4754
try {
48-
fs.renameSync(legacyPath, newPath);
49-
logInfo(`Migrated usage data to new location: ${newPath}`);
50-
} catch (renameError) {
51-
// Fallback if rename/move fails: copy then best-effort delete
52-
logWarning(
53-
`Could not move usage file (${formatError(
54-
renameError,
55-
)}), copying instead...`,
56-
);
57-
fs.copyFileSync(legacyPath, newPath);
55+
payload = fs.readFileSync(legacyPath);
56+
} catch (readError) {
57+
if ((readError as NodeJS.ErrnoException)?.code === 'ENOENT') {
58+
// The winner unlinked the legacy file first: migration
59+
// already happened — adopt the destination quietly
60+
// instead of logging a misleading failure warning.
61+
logInfo(
62+
`Legacy usage file already migrated by another process; using ${newPath}.`,
63+
);
64+
return newPath;
65+
}
66+
throw readError;
67+
}
68+
if (publishFileNoClobber(newPath, payload)) {
5869
try {
5970
fs.unlinkSync(legacyPath);
60-
logInfo(`Successfully migrated usage data to: ${newPath}`);
6171
} catch {
6272
logWarning(
6373
`Migrated usage data to new location, but could not remove old file at ${legacyPath}. You may want to manually delete it.`,
6474
);
6575
}
76+
logInfo(`Migrated usage data to new location: ${newPath}`);
77+
} else {
78+
logInfo(
79+
`Usage data already present at ${newPath}; keeping it and leaving legacy file in place.`,
80+
);
6681
}
6782

6883
return newPath;

‎source/utils/atomic-write.spec.ts‎

Lines changed: 69 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import * as path from 'path';
22
import test from 'ava';
33
import * as fs from 'fs/promises';
4-
import {atomicWriteFile} from './atomic-write';
4+
import {atomicWriteFile, publishFileNoClobber, setLinkSyncForTests} from './atomic-write';
55

66
async function createTempDir(): Promise<string> {
77
const tempDir = path.join(
@@ -78,3 +78,71 @@ test.serial('atomicWriteFile leaves no temp file behind on failure', async t =>
7878
await cleanupTempDir(tempDir);
7979
}
8080
});
81+
82+
test.serial('publishFileNoClobber publishes when the destination is absent', async t => {
83+
const tempDir = await createTempDir();
84+
try {
85+
const filePath = path.join(tempDir, 'usage.json');
86+
87+
t.true(publishFileNoClobber(filePath, '{"totalLifetime": 5}'));
88+
t.is(await fs.readFile(filePath, 'utf-8'), '{"totalLifetime": 5}');
89+
// No temp litter left behind.
90+
const dirListing = await fs.readdir(tempDir);
91+
t.false(dirListing.some(name => name.includes('.tmp')));
92+
} finally {
93+
await cleanupTempDir(tempDir);
94+
}
95+
});
96+
97+
test.serial('publishFileNoClobber keeps the existing file when a peer won the race', async t => {
98+
// The concurrent-publish window (destination appearing between the
99+
// exists check and the publish) cannot be staged single-threaded —
100+
// link(2) atomicity is the arbiter — so this covers the seam where the
101+
// old and new code differ: it fails against a copy-based implementation
102+
// (verified by mutation) and passes against the link-based one.
103+
const tempDir = await createTempDir();
104+
try {
105+
const filePath = path.join(tempDir, 'usage.json');
106+
await fs.writeFile(filePath, 'winner-data', 'utf-8');
107+
108+
t.false(publishFileNoClobber(filePath, 'stale-data'));
109+
t.is(await fs.readFile(filePath, 'utf-8'), 'winner-data');
110+
const dirListing = await fs.readdir(tempDir);
111+
t.false(dirListing.some(name => name.includes('.tmp')));
112+
} finally {
113+
await cleanupTempDir(tempDir);
114+
}
115+
});
116+
117+
test.serial('publishFileNoClobber falls back to exclusive write when hard links are unsupported', async t => {
118+
// Simulate FAT/exFAT or SMB/FUSE mounts where linkSync throws EPERM.
119+
const linkError = new Error(
120+
'operation not permitted, link',
121+
) as NodeJS.ErrnoException;
122+
linkError.code = 'EPERM';
123+
setLinkSyncForTests(() => {
124+
throw linkError;
125+
});
126+
try {
127+
const tempDir = await createTempDir();
128+
try {
129+
const filePath = path.join(tempDir, 'usage.json');
130+
131+
// Absent destination: the exclusive-create fallback publishes.
132+
t.true(publishFileNoClobber(filePath, 'fallback-data'));
133+
t.is(await fs.readFile(filePath, 'utf-8'), 'fallback-data');
134+
135+
// Present destination: EEXIST from the fallback likewise means a
136+
// peer won — content stays untouched.
137+
t.false(publishFileNoClobber(filePath, 'stale-data'));
138+
t.is(await fs.readFile(filePath, 'utf-8'), 'fallback-data');
139+
140+
const dirListing = await fs.readdir(tempDir);
141+
t.false(dirListing.some(name => name.includes('.tmp')));
142+
} finally {
143+
await cleanupTempDir(tempDir);
144+
}
145+
} finally {
146+
setLinkSyncForTests();
147+
}
148+
});

‎source/utils/atomic-write.ts‎

Lines changed: 89 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
import {randomUUID} from 'node:crypto';
2-
import {mkdirSync, renameSync, unlinkSync, writeFileSync} from 'node:fs';
2+
import {
3+
linkSync,
4+
mkdirSync,
5+
renameSync,
6+
unlinkSync,
7+
writeFileSync,
8+
} from 'node:fs';
39
import {rename, unlink, writeFile} from 'node:fs/promises';
410
import {dirname} from 'node:path';
511

@@ -55,3 +61,85 @@ export function atomicWriteJson(filePath: string, data: unknown): void {
5561
mkdirSync(dir, {recursive: true});
5662
atomicWriteFileSync(filePath, `${JSON.stringify(data, null, 2)}\n`);
5763
}
64+
65+
/**
66+
* Link primitive used by {@link publishFileNoClobber}, replaceable for
67+
* tests (e.g. to simulate filesystems without hard-link support).
68+
* Production code never touches this.
69+
* @internal
70+
*/
71+
type LinkSyncFn = (existingPath: string, newPath: string) => void;
72+
73+
let linkSyncImpl: LinkSyncFn = (existingPath, targetPath) =>
74+
linkSync(existingPath, targetPath);
75+
76+
/**
77+
* Test seam: replace the link primitive (e.g. with one that throws `EPERM`
78+
* to simulate FAT/exFAT or SMB/FUSE mounts). Call with no arguments to
79+
* restore the default.
80+
* @internal
81+
*/
82+
export function setLinkSyncForTests(impl?: LinkSyncFn): void {
83+
linkSyncImpl =
84+
impl ?? ((existingPath, targetPath) => linkSync(existingPath, targetPath));
85+
}
86+
87+
/**
88+
* Publish file content at `newPath` without ever overwriting an existing
89+
* destination. Returns true when this call published the file, false when a
90+
* destination was already present — i.e. a concurrent process won the race
91+
* and its (fresher) file is adopted untouched.
92+
*
93+
* The payload goes to a unique temp file in the destination directory
94+
* first and is published with a hard link, which is atomic and raises
95+
* EEXIST when the destination appeared concurrently. Because temp and
96+
* destination share a directory, a cross-device move can never occur, so —
97+
* unlike a `renameSync` + copy fallback — there is no path that overwrites
98+
* the winner's file with this stale copy.
99+
*/
100+
export function publishFileNoClobber(
101+
newPath: string,
102+
data: string | Buffer,
103+
): boolean {
104+
const tmpPath = `${newPath}.${process.pid}.${randomUUID()}.tmp`;
105+
try {
106+
writeFileSync(tmpPath, data, 'utf-8');
107+
try {
108+
linkSyncImpl(tmpPath, newPath);
109+
} catch (error) {
110+
const code = (error as NodeJS.ErrnoException)?.code;
111+
if (code === 'EEXIST') {
112+
// A concurrent process published first: adopt its file.
113+
return false;
114+
}
115+
if (
116+
code === 'EPERM' ||
117+
code === 'ENOTSUP' ||
118+
code === 'EOPNOTSUPP' ||
119+
code === 'ENOSYS'
120+
) {
121+
// Filesystem without hard-link support (FAT/exFAT, some
122+
// SMB/FUSE mounts): fall back to an exclusive create, which
123+
// still never overwrites — EEXIST from it likewise means a
124+
// peer won.
125+
try {
126+
writeFileSync(newPath, data, {flag: 'wx'});
127+
} catch (writeError) {
128+
if ((writeError as NodeJS.ErrnoException)?.code === 'EEXIST') {
129+
return false;
130+
}
131+
throw writeError;
132+
}
133+
return true;
134+
}
135+
throw error;
136+
}
137+
return true;
138+
} finally {
139+
try {
140+
unlinkSync(tmpPath);
141+
} catch {
142+
// Already consumed by a successful link, or never created.
143+
}
144+
}
145+
}

0 commit comments

Comments
 (0)