mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(session): merge-time fixes for the dead-pane resume pin (#467)
- test/setup.ts strips CLAUDE_CONFIG_DIR (pinned in test-env-isolation), so transcript-fixture tests such as session-custom-model-restart no longer go red on a machine that exports it for a separate Claude account (#255). - The vanished-tmux-session branch of _setupOrAttachMuxSession() relaunches the CLI through createSession() just like a failed respawn, so it now takes the same resume pin. A genuinely new session is unaffected. - After a dead-pane respawn of a fallback-chain CLI, _claudeSessionId names the conversation the walk actually pinned instead of the chain tail, which the walk may have passed over for lack of a transcript. - _claudeConfigDir() trims the override like claudeProjectsDir() does. - The remote-reattach test is labelled as documentation, since the pin builder's own remote guard would make it pass either way. - CLAUDE.md: the create-path pin persists through toState() as resumeSessionId, and the end of the walk adds no pin rather than clearing the launch seed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+39
-12
@@ -1875,29 +1875,40 @@ export class Session extends EventEmitter {
|
||||
respawnPaneOptions: import('./mux-interface.js').RespawnPaneOptions;
|
||||
createSessionOptions: import('./mux-interface.js').CreateSessionOptions;
|
||||
spawnErrLabel: string;
|
||||
}): Promise<{ isRestored: boolean }> {
|
||||
}): Promise<{ isRestored: boolean; respawnedResumeId?: string; respawnedDeadPane: boolean }> {
|
||||
const mux = this._mux!;
|
||||
|
||||
// Verify stale mux session — tmux may have been destroyed (e.g., killed externally)
|
||||
// Verify stale mux session — tmux may have been destroyed (e.g., killed externally).
|
||||
// A session that HAD a mux session relaunches its CLI below just like a failed
|
||||
// respawn does (tmux kill-server, a tmux crash, an external kill-session), so
|
||||
// its transcript collides with the bare `--session-id` the same way. A
|
||||
// genuinely new session starts with `_muxSession` null and never sets this.
|
||||
let muxSessionVanished = false;
|
||||
if (this._muxSession && !mux.muxSessionExists(this._muxSession.muxName)) {
|
||||
console.log('[Session] Stale mux session detected (tmux gone):', this._muxSession.muxName);
|
||||
this._muxSession = null;
|
||||
muxSessionVanished = true;
|
||||
}
|
||||
|
||||
// Check if session exists but pane is dead (remain-on-exit keeps it alive)
|
||||
// Respawn the pane instead of creating a whole new session — preserves tmux scrollback
|
||||
let needsNewSession = false;
|
||||
let respawnedDeadPane = false;
|
||||
let respawnedResumeId: string | undefined;
|
||||
if (this._muxSession && mux.isPaneDead(this._muxSession.muxName)) {
|
||||
console.log('[Session] Dead pane detected, respawning:', this._muxSession.muxName);
|
||||
// Confirmed dead — safe to resolve/pin now (see `_pinOmpRespawnId()`).
|
||||
// `options.respawnPaneOptions` was built eagerly before this dead-pane
|
||||
// check ran, so it still carries the pre-pin ompConfig; rebuild it.
|
||||
this._pinOmpRespawnId();
|
||||
const newPid = await mux.respawnPane(await this._buildRespawnPaneOptionsWithResumePin());
|
||||
const respawnOptions = await this._buildRespawnPaneOptionsWithResumePin();
|
||||
const newPid = await mux.respawnPane(respawnOptions);
|
||||
if (!newPid) {
|
||||
console.error('[Session] Failed to respawn pane, will create new session');
|
||||
needsNewSession = true;
|
||||
} else {
|
||||
respawnedDeadPane = true;
|
||||
respawnedResumeId = respawnOptions.resumeSessionId;
|
||||
this._pendingEnvUnsets.clear();
|
||||
// Wait a moment for the respawned process to fully start
|
||||
await new Promise((resolve) => setTimeout(resolve, MUX_STARTUP_DELAY_MS));
|
||||
@@ -1931,8 +1942,9 @@ export class Session extends EventEmitter {
|
||||
// `this.id` while the CLI resumes the chain tail. The response viewer,
|
||||
// Read My Mind and the unified-list alias map all read `_claudeSessionId`
|
||||
// until the next first-hand hook, so the two have to name the same
|
||||
// conversation.
|
||||
if (needsNewSession) {
|
||||
// conversation. The vanished-tmux-session branch above relaunches for the
|
||||
// same reason and takes the same pin.
|
||||
if (needsNewSession || muxSessionVanished) {
|
||||
const pinned = (await this._buildRespawnPaneOptionsWithResumePin()).resumeSessionId;
|
||||
if (pinned) {
|
||||
options.createSessionOptions.resumeSessionId = pinned;
|
||||
@@ -1980,7 +1992,7 @@ export class Session extends EventEmitter {
|
||||
throw spawnErr;
|
||||
}
|
||||
|
||||
return { isRestored };
|
||||
return { isRestored, respawnedResumeId, respawnedDeadPane };
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -2165,9 +2177,10 @@ export class Session extends EventEmitter {
|
||||
* claude prints "No conversation found" into the scrollback of a session that
|
||||
* is brand new, and `wrapWithNice()` prefixes only the FIRST branch of the
|
||||
* rendered `a || b`, so the branch that actually runs loses its priority for
|
||||
* the life of the session. Falling off the end of the walk therefore pins
|
||||
* nothing, which is the right answer: with no transcript anywhere there is
|
||||
* nothing for the bare `--session-id <this.id>` to collide with.
|
||||
* the life of the session. Falling off the end of the walk therefore adds
|
||||
* no pin (the options keep any launch seed they already carried), which is
|
||||
* the right answer: with no transcript anywhere there is nothing for the
|
||||
* bare `--session-id <this.id>` to collide with.
|
||||
*
|
||||
* The create route pre-validates a resume id for the same reason, though it
|
||||
* additionally requires the transcript be substantial — here mere existence
|
||||
@@ -2213,7 +2226,10 @@ export class Session extends EventEmitter {
|
||||
|
||||
/** The session's Claude config dir when it has been relocated (#255), else undefined. */
|
||||
private _claudeConfigDir(): string | undefined {
|
||||
return this._envOverrides?.CLAUDE_CONFIG_DIR;
|
||||
// Trimmed like `claudeProjectsDir()` trims the process-wide override: the
|
||||
// envOverrides schema validates keys only, and a whitespace-only value would
|
||||
// otherwise resolve to a relative path and read "no transcript" for everything.
|
||||
return this._envOverrides?.CLAUDE_CONFIG_DIR?.trim() || undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -2537,7 +2553,7 @@ export class Session extends EventEmitter {
|
||||
// If mux wrapping is enabled, create or attach to a mux session
|
||||
if (this._useMux && this._mux) {
|
||||
try {
|
||||
const { isRestored } = await this._setupOrAttachMuxSession({
|
||||
const { isRestored, respawnedResumeId, respawnedDeadPane } = await this._setupOrAttachMuxSession({
|
||||
// Single source of truth shared with reattachRemote() (COD-108).
|
||||
respawnPaneOptions: this._buildRespawnPaneOptions(),
|
||||
createSessionOptions: {
|
||||
@@ -2584,7 +2600,18 @@ export class Session extends EventEmitter {
|
||||
// persisted chain's tail is that conversation, reported first-hand by
|
||||
// the CLI's own hook, so it outranks every fallback here. A NEW pane has
|
||||
// an empty chain and falls through to the resume/alias fallbacks.
|
||||
restoredConversation = isRestored ? this._claudeSessionChain[this._claudeSessionChain.length - 1] : undefined;
|
||||
//
|
||||
// A dead-pane respawn is NOT that case for a CLI whose relaunch the resume
|
||||
// pin walk governs (`launch.chain === 'fallback'`): the CLI did stop, and
|
||||
// the walk may have passed over a chain tail with no transcript behind it,
|
||||
// so the conversation is whatever the respawn actually resumed. Undefined
|
||||
// there means the pane launched unpinned, which the fallbacks below name.
|
||||
const pinGovernsRespawn = respawnedDeadPane && getCli(this.mode)?.launch.chain === 'fallback';
|
||||
restoredConversation = pinGovernsRespawn
|
||||
? respawnedResumeId
|
||||
: isRestored
|
||||
? this._claudeSessionChain[this._claudeSessionChain.length - 1]
|
||||
: undefined;
|
||||
this._claudeSessionId =
|
||||
restoredConversation ||
|
||||
this._resumeSessionId ||
|
||||
|
||||
@@ -82,6 +82,32 @@ function failingRespawnMux() {
|
||||
return { mux: mux as unknown as TerminalMultiplexer, calls };
|
||||
}
|
||||
|
||||
/**
|
||||
* A mux whose tmux lost the WHOLE session (tmux kill-server, a crash, an
|
||||
* external kill-session), not just the pane. `_setupOrAttachMuxSession()` drops
|
||||
* its stale handle and goes straight to `createSession()`, relaunching the CLI
|
||||
* exactly as the failed-respawn fallback does.
|
||||
*/
|
||||
function vanishedSessionMux() {
|
||||
const calls: CreateSessionOptions[] = [];
|
||||
const respawns: RespawnPaneOptions[] = [];
|
||||
const mux = {
|
||||
isAvailable: () => true,
|
||||
muxSessionExists: () => false,
|
||||
isPaneDead: () => false,
|
||||
setAttached: () => {},
|
||||
respawnPane: async (options: RespawnPaneOptions) => {
|
||||
respawns.push(options);
|
||||
return 4242;
|
||||
},
|
||||
createSession: async (options: CreateSessionOptions) => {
|
||||
calls.push(options);
|
||||
return muxSession('codeman-recreated');
|
||||
},
|
||||
};
|
||||
return { mux: mux as unknown as TerminalMultiplexer, calls, respawns };
|
||||
}
|
||||
|
||||
const CONVERSATION = 'aaaabbbb-cccc-dddd-eeee-ffff00001111';
|
||||
|
||||
let configDir: string;
|
||||
@@ -258,6 +284,58 @@ describe('pinning a conversation onto a relaunch', () => {
|
||||
}
|
||||
});
|
||||
|
||||
it('pins the create path when tmux lost the whole session, not just the pane', async () => {
|
||||
// The stale-session branch nulls the handle and never sets the failed-respawn
|
||||
// flag, so without its own pin the relaunch carried the bare launch line and
|
||||
// met the same `--session-id ... already in use` refusal.
|
||||
giveTranscript(CONVERSATION);
|
||||
const { mux, calls, respawns } = vanishedSessionMux();
|
||||
const session = localSession({ claudeSessionChain: [CONVERSATION] }, mux);
|
||||
|
||||
await session.startInteractive();
|
||||
try {
|
||||
expect(respawns).toHaveLength(0);
|
||||
expect(calls).toHaveLength(1);
|
||||
expect(calls[0].resumeSessionId).toBe(CONVERSATION);
|
||||
expect(session.claudeSessionId).toBe(CONVERSATION);
|
||||
} finally {
|
||||
await session.stop();
|
||||
}
|
||||
});
|
||||
|
||||
it('leaves a genuinely new session unpinned on the create path', async () => {
|
||||
// No mux handle to begin with, so the stale-session flag is never set and
|
||||
// the create options keep their original shape.
|
||||
const { mux, calls } = vanishedSessionMux();
|
||||
const session = localSession({ muxSession: undefined }, mux);
|
||||
giveTranscript(session.id);
|
||||
|
||||
await session.startInteractive();
|
||||
try {
|
||||
expect(calls).toHaveLength(1);
|
||||
expect(calls[0].resumeSessionId).toBeUndefined();
|
||||
} finally {
|
||||
await session.stop();
|
||||
}
|
||||
});
|
||||
|
||||
it('names the conversation the dead-pane respawn actually resumed', async () => {
|
||||
// The chain tail has no transcript, so the walk degrades to the session id.
|
||||
// The session must then report that id, not the chain tail claude never
|
||||
// opened: the response viewer, Read My Mind and the alias map all read it.
|
||||
const { mux, calls } = recordingMux();
|
||||
const session = localSession({ claudeSessionChain: [CONVERSATION] }, mux);
|
||||
giveTranscript(session.id);
|
||||
|
||||
await session.startInteractive();
|
||||
try {
|
||||
expect(calls[0].resumeSessionId).toBe(session.id);
|
||||
expect(session.claudeSessionId).toBe(session.id);
|
||||
} finally {
|
||||
await session.stop();
|
||||
}
|
||||
});
|
||||
|
||||
it('pins nothing for a remote session, whose conversation lives elsewhere', async () => {
|
||||
// The dead-pane respawn is reached by every session shape, unlike
|
||||
// `restartCli()` whose route refuses remote. A local id pinned onto a
|
||||
@@ -311,9 +389,13 @@ describe('pinning a conversation onto a relaunch', () => {
|
||||
expect(calls[0].resumeSessionId).toBeUndefined();
|
||||
});
|
||||
|
||||
it('pins nothing for a remote reattach, which relaunches no CLI', async () => {
|
||||
it('documents that a remote reattach carries no pin', async () => {
|
||||
// `reattachRemote()` re-runs the remote session command, which attaches to
|
||||
// the durable remote tmux with the agent still running inside it.
|
||||
// ⚠️ Documentation, not a regression guard: `reattachRemote()` only runs for
|
||||
// a remote session, and the pin builder refuses remote sessions on its own,
|
||||
// so this would still pass if `reattachRemote()` were switched to the pinned
|
||||
// builder. The builder's remote guard is what the test above pins.
|
||||
giveTranscript(CONVERSATION);
|
||||
const { mux, calls } = recordingMux();
|
||||
const session = localSession(
|
||||
|
||||
@@ -45,6 +45,12 @@ delete process.env.CODEMAN_GESTURE;
|
||||
// operator who exports it (exactly who the feature is for) would otherwise see the
|
||||
// root-install byte-identity assertions fail.
|
||||
delete process.env.CODEMAN_BASE_URL;
|
||||
// CLAUDE_CONFIG_DIR (#255) relocates Claude's whole tree, transcripts included, and
|
||||
// `claudeProjectsDir()` reads it before it ever looks at `homedir()`. A developer who runs
|
||||
// Codeman against a separate Claude account exports exactly this, and every test that writes
|
||||
// a transcript fixture under the temp HOME's `~/.claude/projects` then reads "no transcript"
|
||||
// (found at merge of #467: test/session-custom-model-restart.test.ts went red).
|
||||
delete process.env.CLAUDE_CONFIG_DIR;
|
||||
|
||||
// Instance selection is PROCESS-WIDE and is what `src/config/instance.ts` derives
|
||||
// both the data dir and the tmux socket from, so a shell that exports any of these
|
||||
|
||||
@@ -34,6 +34,7 @@ const STRIPPED_ENV_VARS: Array<[name: string, why: string]> = [
|
||||
['CODEMAN_INSTANCE', 'moves the data dir to ~/.codeman-<name> and the tmux socket to codeman-<name>'],
|
||||
['CODEMAN_DATA_DIR', 'ABSOLUTE override: bypasses the temp HOME and points the suite at a real data dir'],
|
||||
['CODEMAN_TMUX_SOCKET', 'renames the socket resolveTmuxSocketName() returns'],
|
||||
['CLAUDE_CONFIG_DIR', 'relocates the Claude tree, so transcript fixtures under the temp HOME read as missing'],
|
||||
];
|
||||
|
||||
const SETUP_SOURCE = readFileSync(fileURLToPath(new URL('./setup.ts', import.meta.url)), 'utf-8');
|
||||
|
||||
Reference in New Issue
Block a user