mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
Fourth review of the reboot-restore branch, and the third to find a defect in the previous round's fix. This one is the same shape as its predecessor: a counter keyed on one thing, compared against a set keyed on another. The generation counter was indexed by the entry's owner, while the in-flight set holds the caller doing the restoring. Those are the same person exactly when a user restores their own sessions, which is every case the tests covered. The route deliberately supports the other case: an admin may spend another user's entries. So when an admin restored Bob's sessions and Bob dismissed the banner, nothing matched, the entries came back, and a plan Bob had explicitly dismissed was re-armed for another twenty-four hours. Rather than reconcile the two key spaces, the counter is gone. `take()` now parks the entries it hands out, remembering which caller is spending them, and they stay parked until that restore ends. A dismiss filters the parked entries by `canAccess(entry.owner)` — the same predicate it already applies to the plan — so it reaches them wherever they are. `releaseFlight()` puts back only what is still parked. Expiry and a fresh boot plan unpark everything, for the same reason. There is one key space now, the entry's owner, and the spender is only ever used to tell two concurrent flights apart. That removes `generations`, `snapshotGenerations()`, `bump()`, `bumpAll()` and the argument threaded through the route. The discard grew the teardown it still lacked. A rebuild can fail after startInteractive() resolved, and a restored workspace still carries Codeman's hooks, so the CLI can post a hook event within milliseconds; the transcript watcher that starts from it, the attachment registry, the wait registry and the approvals inbox all outlive the listeners and would meet the retry, which reuses the session id by design. Its steps also run in reverse order now, so no live listener can reach a tracker that has already stopped, and the mux kill has its own guard, because stop() kills the pane in its last block after destroying four trackers. Tests. The run-summary test named an interval and asserted a map entry, so dropping stop() left it green; it now spies on stop(). Nothing pinned that before-spawn must precede setupSessionListeners, which reads the flag that phase restores, so swapping the two lines was silent; the ordering test now includes the listener setup. The retry assertion was a tautology and now asserts a different refs object. Both strengthened tests were verified by reverting their fix. Two new tests cover the admin-restores-another-owner cases this round was about. The server in the discard test is built once and stopped, since its constructor registers handlers on module-level watchers, and the workspace is removed through safeRmHomeTree. Refs #411 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
132 lines
5.4 KiB
TypeScript
132 lines
5.4 KiB
TypeScript
/**
|
|
* `WebServer.discardPartiallyBuiltSession()` against the real server object.
|
|
*
|
|
* The reboot-restore route calls this when a rebuild registers a session and
|
|
* then fails to start its pane. It has to be the exact inverse of
|
|
* `registerSessionWithLayout()` plus `setupSessionListeners()`, and it must NOT
|
|
* be the user-initiated delete: banking the session's token totals, demoting a
|
|
* pinned record or deleting the workspace's files would all be wrong for a
|
|
* session that never ran.
|
|
*
|
|
* These tests drive the real method rather than the route, because the route
|
|
* tests run against a mock context whose `discardPartiallyBuiltSession` is a
|
|
* one-line stub — an earlier version of this function left four registrations
|
|
* behind and every route test still passed.
|
|
*
|
|
* The retry assertion is the important one. `setupSessionListeners()` returns
|
|
* early when `sessionListenerRefs` still holds the session id, so a discard that
|
|
* leaves that entry makes the next attempt wire nothing at all, and the user
|
|
* gets a tab that never shows output.
|
|
*/
|
|
import { mkdirSync } from 'node:fs';
|
|
import { homedir } from 'node:os';
|
|
import { join } from 'node:path';
|
|
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest';
|
|
import { safeRmHomeTree } from './mocks/test-helpers.js';
|
|
|
|
import { WebServer } from '../src/web/server.js';
|
|
import { Session } from '../src/session.js';
|
|
import { TmuxManager } from '../src/tmux-manager.js';
|
|
|
|
/** Reach the private collections the discard is responsible for emptying. */
|
|
interface ServerInternals {
|
|
sessions: Map<string, Session>;
|
|
sessionListenerRefs: Map<string, unknown>;
|
|
runSummaryTrackers: Map<string, unknown>;
|
|
registerSessionWithLayout(session: Session): Promise<void>;
|
|
setupSessionListeners(session: Session): Promise<void>;
|
|
discardPartiallyBuiltSession(sessionId: string): Promise<void>;
|
|
}
|
|
|
|
const WORKSPACE = join(homedir(), '.codeman-test-discard');
|
|
const SESSION_ID = 'a1b2c3d4e5f60718';
|
|
|
|
let server: WebServer;
|
|
let internals: ServerInternals;
|
|
let mux: TmuxManager;
|
|
|
|
function buildSession(): Session {
|
|
return new Session({
|
|
id: SESSION_ID,
|
|
workingDir: WORKSPACE,
|
|
mode: 'claude',
|
|
name: 'rebuilt session',
|
|
mux,
|
|
useMux: true,
|
|
});
|
|
}
|
|
|
|
beforeAll(() => {
|
|
mkdirSync(WORKSPACE, { recursive: true });
|
|
// Test mode: no port is opened and no CLI is launched. One server for the file,
|
|
// stopped at the end: the constructor registers handlers on the module-level
|
|
// image, subagent, team and workflow watchers, and only stop() removes them.
|
|
server = new WebServer(0, false, true);
|
|
internals = server as unknown as ServerInternals;
|
|
mux = new TmuxManager();
|
|
});
|
|
|
|
afterEach(async () => {
|
|
await internals.discardPartiallyBuiltSession(SESSION_ID).catch(() => {});
|
|
});
|
|
|
|
afterAll(async () => {
|
|
await server.stop().catch(() => {});
|
|
safeRmHomeTree(WORKSPACE);
|
|
});
|
|
|
|
describe('discarding a session whose pane never started', () => {
|
|
it('takes the session back out of the server', async () => {
|
|
const session = buildSession();
|
|
await internals.registerSessionWithLayout(session);
|
|
await internals.setupSessionListeners(session);
|
|
expect(internals.sessions.has(SESSION_ID)).toBe(true);
|
|
|
|
await internals.discardPartiallyBuiltSession(SESSION_ID);
|
|
expect(internals.sessions.has(SESSION_ID)).toBe(false);
|
|
});
|
|
|
|
it('releases the listener registration, so a retry can wire itself again', async () => {
|
|
const first = buildSession();
|
|
await internals.registerSessionWithLayout(first);
|
|
await internals.setupSessionListeners(first);
|
|
expect(internals.sessionListenerRefs.has(SESSION_ID)).toBe(true);
|
|
|
|
const firstRefs = internals.sessionListenerRefs.get(SESSION_ID);
|
|
await internals.discardPartiallyBuiltSession(SESSION_ID);
|
|
expect(internals.sessionListenerRefs.has(SESSION_ID)).toBe(false);
|
|
|
|
// The retry reuses the id by design. `setupSessionListeners()` returns early
|
|
// while the refs are still there, so a session built now would run blind: no
|
|
// terminal output, no status updates, no exit broadcast. Asserting a DIFFERENT
|
|
// refs object is what distinguishes wiring the retry from finding the corpse
|
|
// of the first attempt still in place.
|
|
const retry = buildSession();
|
|
await internals.registerSessionWithLayout(retry);
|
|
await internals.setupSessionListeners(retry);
|
|
const retryRefs = internals.sessionListenerRefs.get(SESSION_ID);
|
|
expect(retryRefs).toBeDefined();
|
|
expect(retryRefs).not.toBe(firstRefs);
|
|
});
|
|
|
|
it('stops the run-summary tracker, whose interval would otherwise keep firing', async () => {
|
|
const session = buildSession();
|
|
await internals.registerSessionWithLayout(session);
|
|
await internals.setupSessionListeners(session);
|
|
const tracker = internals.runSummaryTrackers.get(SESSION_ID) as { stop: () => void };
|
|
expect(tracker).toBeDefined();
|
|
// Dropping the map entry is not enough: the tracker arms a setInterval in its
|
|
// constructor, and only stop() clears it, so a discard that merely forgot the
|
|
// entry would leave the timer running for the life of the process.
|
|
const stopped = vi.spyOn(tracker, 'stop');
|
|
|
|
await internals.discardPartiallyBuiltSession(SESSION_ID);
|
|
expect(stopped).toHaveBeenCalled();
|
|
expect(internals.runSummaryTrackers.has(SESSION_ID)).toBe(false);
|
|
});
|
|
|
|
it('does nothing at all for a session it never registered', async () => {
|
|
await expect(internals.discardPartiallyBuiltSession('never-existed')).resolves.toBeUndefined();
|
|
});
|
|
});
|