mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 02:29:33 +01:00
fix(cases): serialize every write to linked-cases.json
Five routes changed the linked-cases registry: link, unlink, rename, and a
create or clone into a custom folder. Each read the file, checked it and wrote
it back on its own, so two requests that both read before either wrote lost
an entry. Two clones that claimed one name and finished together both passed
the name check, both answered success, and the later write replaced the
earlier entry. The same race made "a name linked while the clone ran is a
409" in test/routes/case-clone-custom-folder.test.ts fail about half the time.
- New src/web/linked-cases-writer.ts, mutateLinkedCases(), shaped like
mutateSettingsFile(): mutations run one at a time, each write is a temp
file renamed over the registry (through a symlink, keeping its mode), and
a file that is not a JSON object is refused (409 CONFLICT) instead of being
read as {} and overwritten.
- All five writers go through it, with their name checks inside the
callback, so exactly one of two requests for one name wins. Rename also
re-checks that the old name still points at the old folder, and drops its
manual registry undo, since a failed write no longer changes the file.
- Tests: test/linked-cases-writer.test.ts (writer behaviour, and concurrent
link/unlink through the routes with slowed writes, which fail on the old
code). Tests that forced a failed registry write with chmod 444 or a
directory now inject the failure at the rename, or expect the refusal.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
2aa6016d33
commit
5929584ea4
8 files changed
+443
-80
No files matched your search
@@ -8,7 +8,7 @@
|
||||
* to start there. Each test's folders sit in a fresh mkdtemp dir, and everything is removed through
|
||||
* safeRmHomeTree, so a run can only ever delete what it created.
|
||||
*/
|
||||
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import {
|
||||
existsSync,
|
||||
mkdirSync,
|
||||
@@ -19,6 +19,7 @@ import {
|
||||
symlinkSync,
|
||||
writeFileSync,
|
||||
} from 'node:fs';
|
||||
import fs from 'node:fs/promises';
|
||||
import { homedir } from 'node:os';
|
||||
import { basename, join } from 'node:path';
|
||||
import { createRouteTestHarness } from './_route-test-utils.js';
|
||||
@@ -40,7 +41,7 @@ beforeEach(() => {
|
||||
if (!sandboxed()) {
|
||||
throw new Error('case-custom-path-routes.test.ts deletes the linked-cases registry: run it via npm test');
|
||||
}
|
||||
// Recursive: the rollback tests turn the registry into a directory.
|
||||
// Recursive, so a test that left anything at the registry's path cannot leak into the next one.
|
||||
safeRmHomeTree(LINKED());
|
||||
// Resolved, because the server answers with the symlink-resolved folder (macOS temp is under /private).
|
||||
workDir = realpathSync(mkdtempSync(join(homedir(), 'case-custom-path-')));
|
||||
@@ -134,20 +135,36 @@ describe('POST /api/cases with a custom path', () => {
|
||||
|
||||
it('undoes what it created when registering fails (a new folder is removed entirely)', async () => {
|
||||
const { app } = await createRouteTestHarness(registerCaseRoutes);
|
||||
mkdirSync(LINKED(), { recursive: true }); // writeFile onto a directory fails
|
||||
const res = await create(app, { name: 'doomed', path: join(work(), 'doomed') });
|
||||
// The registry is replaced by renaming a temp file over it: fail that rename.
|
||||
const realRename = fs.rename;
|
||||
const spy = vi.spyOn(fs, 'rename').mockImplementation(async (from, to) => {
|
||||
if (to === LINKED()) throw Object.assign(new Error('permission denied'), { code: 'EACCES' });
|
||||
return realRename(from, to);
|
||||
});
|
||||
let res;
|
||||
try {
|
||||
res = await create(app, { name: 'doomed', path: join(work(), 'doomed') });
|
||||
} finally {
|
||||
spy.mockRestore();
|
||||
}
|
||||
expect(res.statusCode).toBe(500);
|
||||
expect(existsSync(join(work(), 'doomed'))).toBe(false);
|
||||
expect(linked()).toEqual({});
|
||||
});
|
||||
|
||||
it('undoes only the scaffold inside an empty folder the user picked, leaving the folder', async () => {
|
||||
it('undoes only the scaffold inside an empty folder the user picked when the registry is refused', async () => {
|
||||
const { app } = await createRouteTestHarness(registerCaseRoutes);
|
||||
mkdirSync(join(work(), 'picked'));
|
||||
mkdirSync(LINKED(), { recursive: true });
|
||||
// A registry that does not parse is never written over (that would unlink every other case).
|
||||
const torn = '{\n "kept": "/somewhere"\n}tail"\n}';
|
||||
writeFileSync(LINKED(), torn);
|
||||
const res = await create(app, { name: 'picked', path: join(work(), 'picked') });
|
||||
expect(res.statusCode).toBe(500);
|
||||
expect(res.statusCode).toBe(409);
|
||||
expect(res.json().errorCode).toBe('CONFLICT');
|
||||
expect(res.json().error).toContain('is not a valid JSON object');
|
||||
expect(existsSync(join(work(), 'picked'))).toBe(true);
|
||||
expect(readdirSync(join(work(), 'picked'))).toEqual([]);
|
||||
expect(readFileSync(LINKED(), 'utf8')).toBe(torn);
|
||||
});
|
||||
|
||||
it('a request without a path still creates under the cases directory, as before', async () => {
|
||||
|
||||
@@ -6,7 +6,6 @@
|
||||
*/
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import {
|
||||
chmodSync,
|
||||
existsSync,
|
||||
mkdirSync,
|
||||
mkdtempSync,
|
||||
@@ -36,6 +35,21 @@ let claudeCfg = '';
|
||||
const made: string[] = [];
|
||||
|
||||
const linked = (): Record<string, string> => (existsSync(LINKED()) ? JSON.parse(readFileSync(LINKED(), 'utf8')) : {});
|
||||
/**
|
||||
* The registry is replaced by renaming a temp file over it, which a read-only file does not stop, so a
|
||||
* failed registry write is injected at that rename. `before` runs first (another writer landing then).
|
||||
*/
|
||||
const failRegistryWrite = (before?: () => Promise<unknown>, alsoFail?: (from: string) => boolean) => {
|
||||
const realRename = fs.rename;
|
||||
return vi.spyOn(fs, 'rename').mockImplementation(async (from, to) => {
|
||||
if (to === LINKED()) {
|
||||
if (before) await before();
|
||||
throw Object.assign(new Error('permission denied'), { code: 'EACCES' });
|
||||
}
|
||||
if (alsoFail?.(String(from))) throw Object.assign(new Error('resource busy'), { code: 'EBUSY' });
|
||||
return realRename(from, to);
|
||||
});
|
||||
};
|
||||
const settings = (): Record<string, unknown> => JSON.parse(readFileSync(SETTINGS_PATH, 'utf8'));
|
||||
const rename = (app: Awaited<ReturnType<typeof createRouteTestHarness>>['app'], name: string, payload: object) =>
|
||||
app.inject({ method: 'POST', url: `/api/cases/${name}/rename`, payload });
|
||||
@@ -278,8 +292,13 @@ describe('POST /api/cases/:name/rename (a linked case)', () => {
|
||||
writeFileSync(LINKED(), JSON.stringify({ proj: real }));
|
||||
writeFileSync(SETTINGS_PATH, JSON.stringify({ caseOrder: ['a', 'proj', 'b'], lastUsedCase: 'proj' }));
|
||||
// the registry reads fine but cannot be written: the rename must undo the folder move
|
||||
chmodSync(LINKED(), 0o444);
|
||||
const res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true });
|
||||
const spy = failRegistryWrite();
|
||||
let res;
|
||||
try {
|
||||
res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true });
|
||||
} finally {
|
||||
spy.mockRestore();
|
||||
}
|
||||
expect(res.statusCode).toBe(500);
|
||||
expect(existsSync(real)).toBe(true);
|
||||
expect(linked()).toEqual({ proj: real });
|
||||
@@ -294,14 +313,9 @@ describe('POST /api/cases/:name/rename (a linked case)', () => {
|
||||
mkdirSync(real);
|
||||
writeFileSync(LINKED(), JSON.stringify({ proj: real }));
|
||||
writeFileSync(SETTINGS_PATH, JSON.stringify({ caseOrder: ['a', 'proj', 'b'], lastUsedCase: 'proj' }));
|
||||
chmodSync(LINKED(), 0o444);
|
||||
// Another writer (a settings save) lands after the rename wrote settings.json and before the
|
||||
// registry write fails.
|
||||
const realWriteFile = fs.writeFile;
|
||||
const spy = vi.spyOn(fs, 'writeFile').mockImplementation(async (file, data, options) => {
|
||||
if (file === LINKED()) await mutateSettingsFile((current) => ({ ...current, agentSkillEnabled: true }));
|
||||
return realWriteFile(file, data, options);
|
||||
});
|
||||
const spy = failRegistryWrite(() => mutateSettingsFile((current) => ({ ...current, agentSkillEnabled: true })));
|
||||
try {
|
||||
const res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true });
|
||||
expect(res.statusCode).toBe(500);
|
||||
@@ -351,13 +365,8 @@ describe('POST /api/cases/:name/rename (a linked case)', () => {
|
||||
const real = join(work, 'my-project');
|
||||
mkdirSync(real);
|
||||
writeFileSync(LINKED(), JSON.stringify({ proj: real }));
|
||||
chmodSync(LINKED(), 0o444);
|
||||
const moved = join(work, 'renamed');
|
||||
const realRename = fs.rename;
|
||||
const spy = vi.spyOn(fs, 'rename').mockImplementation(async (from, to) => {
|
||||
if (from === moved) throw Object.assign(new Error('resource busy'), { code: 'EBUSY' });
|
||||
return realRename(from, to);
|
||||
});
|
||||
const spy = failRegistryWrite(undefined, (from) => from === moved);
|
||||
try {
|
||||
const res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true });
|
||||
expect(res.statusCode).toBe(500);
|
||||
@@ -373,9 +382,14 @@ describe('POST /api/cases/:name/rename (a linked case)', () => {
|
||||
const real = join(work, 'my-project');
|
||||
mkdirSync(real);
|
||||
writeFileSync(LINKED(), JSON.stringify({ proj: real }));
|
||||
chmodSync(LINKED(), 0o444);
|
||||
(ctx.sessions as Map<string, unknown>).set('s1', { id: 's1', name: 'w1', workingDir: real });
|
||||
const res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true, stopSessions: true });
|
||||
const spy = failRegistryWrite();
|
||||
let res;
|
||||
try {
|
||||
res = await rename(app, 'proj', { newName: 'renamed', renameFolder: true, stopSessions: true });
|
||||
} finally {
|
||||
spy.mockRestore();
|
||||
}
|
||||
expect(res.statusCode).toBe(500);
|
||||
expect(res.json().stoppedSessions).toBe(1);
|
||||
expect(ctx.cleanupSession).toHaveBeenCalledWith('s1', true, 'case_rename');
|
||||
|
||||
@@ -54,6 +54,12 @@ vi.mock('node:fs/promises', () => ({
|
||||
throw err;
|
||||
}),
|
||||
writeFile: vi.fn(async () => {}),
|
||||
// What the linked-cases writer needs to replace the registry atomically (temp file + rename).
|
||||
realpath: vi.fn(async (p: string) => p),
|
||||
mkdir: vi.fn(async () => undefined),
|
||||
chmod: vi.fn(async () => {}),
|
||||
rename: vi.fn(async () => {}),
|
||||
rm: vi.fn(async () => {}),
|
||||
},
|
||||
}));
|
||||
|
||||
@@ -704,6 +710,7 @@ describe('case-routes', () => {
|
||||
});
|
||||
// linked-cases.json doesn't exist yet
|
||||
mockedReadFile.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' }));
|
||||
mockedStat.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' }));
|
||||
|
||||
const res = await harness.app.inject({
|
||||
method: 'POST',
|
||||
|
||||
Reference in new issue
Block a user