mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 18:39:33 +01:00
fix: refuse a case folder move while a session is still being created in it (release review)
The rename lock is checked once, in the Session constructor, but a session
reaches the server's map (where the rename route builds its list of open
sessions) only later, after the create awaits the owner's layout lock and a
fresh settings read (and, for POST /api/sessions, inferLocalCaseName). A
rename in that gap neither listed nor stopped the session, moved the folder,
and the create then registered a session whose pane starts with
`cd "<old path>"`, skipping the 409 requiresStop / stopSessions guarantee.
- case-rename.ts keeps a record of sessions constructed but not yet
registered (`noteCaseSessionStarting` / `caseSessionSettled` /
`isCaseSessionStarting`), with a 30 s TTL for one that never registers
(a failed create, a one-shot prompt), swept on every note and check.
- The Session constructor records itself (local sessions only, as the
route ignores remote ones); registerSessionWithLayout()'s register
callback, the boot-restore registration and stop() settle it.
- The rename route answers 409 CONFLICT with renameCode BUSY ("A session is
starting in this case") for a folder move while one is recorded at or
under the folder. The check runs in the same tick that takes the lock, so
every interleaving is either refused here or by the constructor. An
alias-only rename is not refused.
A create whose path was validated before the move but whose Session is
constructed after it is still the ordinary check-then-use race of any
create path; CLAUDE.md now says so instead of "nothing starts mid-move".
Tests: unit (record, settle, stop, TTL), route (BUSY, then requiresStop once
registered; alias-only unaffected) and an end-to-end WebServer test that
holds the create between constructor and registration (it fails without
the route check: the folder moves under the session).
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
40158ce1ef
commit
f8029c2a9e
10 files changed
+238
-7
No files matched your search
@@ -0,0 +1,103 @@
|
||||
/**
|
||||
* @fileoverview A case rename racing a session create, against the real WebServer.
|
||||
*
|
||||
* A create constructs its `Session` (which checks the rename lock) and only puts it in the server's session
|
||||
* map later, after awaiting the owner's layout lock and a settings read. A rename that ran in that gap built its
|
||||
* list of open sessions without the new one, moved the folder, and the create then registered a session whose
|
||||
* pane would `cd` into a path that no longer existed. The rename now refuses the move while such a session is
|
||||
* still being created, and the server's registration is what lets go of it.
|
||||
*
|
||||
* The gap is widened by holding the settings read inside `TabLayoutService.sessionCreated()`
|
||||
* (`childrenFollowParent`), the await between the constructor and the registration callback.
|
||||
*
|
||||
* Port: 0 (ephemeral, read back from `server.boundPort`).
|
||||
*/
|
||||
import { existsSync, mkdirSync, writeFileSync } from 'node:fs';
|
||||
import { homedir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
|
||||
import { WebServer } from '../src/web/server.js';
|
||||
import { isCaseSessionStarting } from '../src/case-rename.js';
|
||||
import { safeRmHomeTree } from './mocks/test-helpers.js';
|
||||
|
||||
interface LayoutDeps {
|
||||
childrenFollowParent?: () => Promise<boolean>;
|
||||
}
|
||||
|
||||
let server: WebServer;
|
||||
const made: string[] = [];
|
||||
const url = (path: string) => `http://localhost:${server.boundPort}${path}`;
|
||||
const post = async (path: string, body: unknown) => {
|
||||
const res = await fetch(url(path), {
|
||||
method: 'POST',
|
||||
headers: { 'content-type': 'application/json' },
|
||||
body: JSON.stringify(body),
|
||||
});
|
||||
return { status: res.status, json: (await res.json()) as Record<string, any> };
|
||||
};
|
||||
const localCase = (name: string) => {
|
||||
const dir = join(homedir(), 'codeman-cases', name);
|
||||
mkdirSync(dir, { recursive: true });
|
||||
writeFileSync(join(dir, 'CLAUDE.md'), name);
|
||||
made.push(dir);
|
||||
return dir;
|
||||
};
|
||||
|
||||
beforeAll(async () => {
|
||||
server = new WebServer(0, false, true);
|
||||
await server.start();
|
||||
}, 60000);
|
||||
|
||||
afterAll(async () => {
|
||||
await server?.stop();
|
||||
for (const dir of made.splice(0)) safeRmHomeTree(dir);
|
||||
}, 60000);
|
||||
|
||||
describe('renaming a case while a session is being created in it', () => {
|
||||
it('refuses the move until the session is registered, then lists it like any open session', async () => {
|
||||
const dir = localCase('race-alpha');
|
||||
made.push(join(homedir(), 'codeman-cases', 'race-beta')); // only there if a rename got through
|
||||
const deps = (server as unknown as { tabLayouts: { deps: LayoutDeps } }).tabLayouts.deps;
|
||||
const original = deps.childrenFollowParent;
|
||||
let reached!: () => void;
|
||||
const atGap = new Promise<void>((resolve) => (reached = resolve));
|
||||
let release!: () => void;
|
||||
const gate = new Promise<void>((resolve) => (release = resolve));
|
||||
deps.childrenFollowParent = async () => {
|
||||
reached();
|
||||
await gate;
|
||||
return (await original?.()) === true;
|
||||
};
|
||||
try {
|
||||
const create = post('/api/sessions', { workingDir: dir, mode: 'shell' });
|
||||
await atGap; // constructed, not yet in the server's session map
|
||||
expect(isCaseSessionStarting(dir)).toBe(true);
|
||||
|
||||
// Without the refusal the rename moves the folder and then waits on the owner's layout lock, which the
|
||||
// held create owns: bound the wait so that shows up as a failure rather than a test timeout.
|
||||
const busy = await Promise.race([
|
||||
post('/api/cases/race-alpha/rename', { newName: 'race-beta', stopSessions: true }),
|
||||
new Promise<null>((resolve) => setTimeout(() => resolve(null), 5000)),
|
||||
]);
|
||||
expect(existsSync(dir), 'the folder moved under the session being created').toBe(true);
|
||||
expect(busy?.status).toBe(409);
|
||||
expect(busy?.json).toMatchObject({ errorCode: 'CONFLICT', renameCode: 'BUSY' });
|
||||
|
||||
deps.childrenFollowParent = original;
|
||||
release();
|
||||
const created = await create;
|
||||
expect(created.status).toBe(200);
|
||||
const id = created.json.data.session.id as string;
|
||||
expect(isCaseSessionStarting(dir)).toBe(false);
|
||||
|
||||
const listed = await post('/api/cases/race-alpha/rename', { newName: 'race-beta' });
|
||||
expect(listed.status).toBe(409);
|
||||
expect(listed.json).toMatchObject({ requiresStop: true, sessions: [{ id }] });
|
||||
expect(existsSync(dir)).toBe(true);
|
||||
await fetch(url(`/api/sessions/${id}`), { method: 'DELETE' });
|
||||
} finally {
|
||||
deps.childrenFollowParent = original;
|
||||
release();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,6 @@
|
||||
/**
|
||||
* @fileoverview src/case-rename.ts: the in-process lock, the retried folder move and Claude history
|
||||
* migration. Real filesystem under test/setup.ts's temp HOME. Port: N/A.
|
||||
* @fileoverview src/case-rename.ts: the in-process lock, the record of sessions still being created, the
|
||||
* retried folder move and Claude history migration. Real filesystem under test/setup.ts's temp HOME. Port: N/A.
|
||||
*/
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest';
|
||||
import { existsSync, mkdirSync, mkdtempSync, readFileSync, symlinkSync, writeFileSync } from 'node:fs';
|
||||
@@ -9,9 +9,12 @@ import { tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import {
|
||||
CaseRenameError,
|
||||
caseSessionSettled,
|
||||
claudeProjectDirName,
|
||||
isCasePathLocked,
|
||||
isCaseSessionStarting,
|
||||
moveCaseFolder,
|
||||
noteCaseSessionStarting,
|
||||
moveClaudeHistory,
|
||||
renameInOrder,
|
||||
siblingPath,
|
||||
@@ -78,6 +81,32 @@ describe('withCaseLock', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('sessions still being created', () => {
|
||||
it('counts a constructed session as starting in its folder until the server settles it', () => {
|
||||
const root = tmp();
|
||||
const dir = join(root, 'case');
|
||||
const session = new Session({ workingDir: join(dir, 'sub') });
|
||||
expect(isCaseSessionStarting(dir)).toBe(true);
|
||||
expect(isCaseSessionStarting(join(dir, 'sub'))).toBe(true);
|
||||
expect(isCaseSessionStarting(join(dir, 'other'))).toBe(false);
|
||||
expect(isCaseSessionStarting(`${dir}-2`)).toBe(false);
|
||||
caseSessionSettled(session);
|
||||
expect(isCaseSessionStarting(dir)).toBe(false);
|
||||
});
|
||||
|
||||
it('lets go of a session that is stopped, and of one never registered once a while has passed', async () => {
|
||||
const root = tmp();
|
||||
const stopped = new Session({ workingDir: root });
|
||||
await stopped.stop();
|
||||
expect(isCaseSessionStarting(root)).toBe(false);
|
||||
// a create that failed before registering, or a one-shot prompt that never registers
|
||||
const lost = {};
|
||||
noteCaseSessionStarting(lost, root, 1000);
|
||||
expect(isCaseSessionStarting(root, 1000 + 29_999)).toBe(true);
|
||||
expect(isCaseSessionStarting(root, 1000 + 30_000)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('moveCaseFolder', () => {
|
||||
it('moves a folder and its contents', async () => {
|
||||
const root = tmp();
|
||||
|
||||
@@ -22,7 +22,8 @@ import { createRouteTestHarness } from './_route-test-utils.js';
|
||||
import { registerCaseRoutes } from '../../src/web/routes/case-routes.js';
|
||||
import { dataPath } from '../../src/config/instance.js';
|
||||
import { SETTINGS_PATH, resolveCasesDir } from '../../src/web/route-helpers.js';
|
||||
import { claudeProjectDirName } from '../../src/case-rename.js';
|
||||
import { caseSessionSettled, claudeProjectDirName } from '../../src/case-rename.js';
|
||||
import { Session } from '../../src/session.js';
|
||||
import { safeRmHomeTree } from '../mocks/index.js';
|
||||
|
||||
const LINKED = () => dataPath('linked-cases.json');
|
||||
@@ -156,6 +157,28 @@ describe('POST /api/cases/:name/rename (a case under the cases directory)', () =
|
||||
expect(existsSync(join(CASES_DIR(), 'beta'))).toBe(true);
|
||||
});
|
||||
|
||||
it('refuses to move a folder a session is still being created in, since it can neither list nor stop it', async () => {
|
||||
const { app, ctx } = await createRouteTestHarness(registerCaseRoutes);
|
||||
const oldDir = localCase('alpha');
|
||||
// Constructed but not in ctx.sessions yet: the gap a create spends awaiting the layout lock and settings.
|
||||
const session = new Session({ workingDir: join(oldDir, 'sub') });
|
||||
try {
|
||||
const busy = await rename(app, 'alpha', { newName: 'beta', stopSessions: true });
|
||||
expect(busy.statusCode).toBe(409);
|
||||
expect(busy.json()).toMatchObject({ errorCode: 'CONFLICT', renameCode: 'BUSY' });
|
||||
expect(existsSync(oldDir)).toBe(true);
|
||||
expect(ctx.cleanupSession).not.toHaveBeenCalled();
|
||||
// once registered it is an open session like any other: listed, and stopped only on request
|
||||
(ctx.sessions as Map<string, unknown>).set(session.id, session);
|
||||
caseSessionSettled(session);
|
||||
const listed = await rename(app, 'alpha', { newName: 'beta' });
|
||||
expect(listed.statusCode).toBe(409);
|
||||
expect(listed.json()).toMatchObject({ requiresStop: true, sessions: [{ id: session.id }] });
|
||||
} finally {
|
||||
caseSessionSettled(session);
|
||||
}
|
||||
});
|
||||
|
||||
it('does not stop a remote session whose remote path equals the local folder', async () => {
|
||||
const { app, ctx } = await createRouteTestHarness(registerCaseRoutes);
|
||||
const dir = localCase('alpha');
|
||||
@@ -215,6 +238,23 @@ describe('POST /api/cases/:name/rename (a linked case)', () => {
|
||||
expect(ctx.cleanupSession).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('renames only the alias while a session is being created in the folder, which an alias rename leaves alone', async () => {
|
||||
const { app } = await createRouteTestHarness(registerCaseRoutes);
|
||||
const real = join(work, 'my-project');
|
||||
mkdirSync(real);
|
||||
writeFileSync(LINKED(), JSON.stringify({ proj: real }));
|
||||
const session = new Session({ workingDir: real });
|
||||
try {
|
||||
expect((await rename(app, 'proj', { newName: 'renamed' })).statusCode).toBe(200);
|
||||
expect((await rename(app, 'renamed', { newName: 'moved', renameFolder: true })).json()).toMatchObject({
|
||||
renameCode: 'BUSY',
|
||||
});
|
||||
expect(existsSync(real)).toBe(true);
|
||||
} finally {
|
||||
caseSessionSettled(session);
|
||||
}
|
||||
});
|
||||
|
||||
it('renames the real folder too, next to the old one, when asked', async () => {
|
||||
const { app } = await createRouteTestHarness(registerCaseRoutes);
|
||||
const real = join(work, 'my-project');
|
||||
|
||||
Reference in new issue
Block a user