mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
fix(omp): resolve and pin the respawn session id only at actual respawn time
findLatestOmpSessionId()'s newest-mtime pin ran eagerly inside _buildRespawnPaneOptions(), which startInteractive() calls unconditionally on every boot-recovery reattach — before anything checks whether the pane is actually dead. With two omp tabs in the same case dir, this could pin an ALIVE pane's session onto whichever sibling's file happened to be newest on disk, purely as a side effect of building options that might never lead to a respawn (reported in Ark0N/Codeman#353 review). Move resolution out of the eager builder into _pinOmpRespawnId(), called explicitly only where a respawn is actually confirmed: the dead-pane branch in _setupOrAttachMuxSession() and reattachRemote(). Add resolveAndClaimOmpSessionId(), which verifies each candidate's own file header (cwd) rather than trusting the mangled-directory match alone, and tracks claimed ids in a process-wide registry so two ambiguous resolutions can't both pick the same sibling's conversation.
This commit is contained in:
timkjr
+43
-44
@@ -57,7 +57,7 @@ import {
|
||||
type SessionRemote,
|
||||
type SessionDocker,
|
||||
} from './types.js';
|
||||
import { findLatestOmpSessionId } from './utils/omp-session-resolver.js';
|
||||
import { resolveAndClaimOmpSessionId } from './utils/omp-session-resolver.js';
|
||||
import { probeDockerCliVersion } from './docker-hosts.js';
|
||||
import { probeRemoteCliVersion } from './remote-hosts.js';
|
||||
import type { TerminalMultiplexer, MuxSession } from './mux-interface.js';
|
||||
@@ -1514,7 +1514,11 @@ export class Session extends EventEmitter {
|
||||
let needsNewSession = false;
|
||||
if (this._muxSession && mux.isPaneDead(this._muxSession.muxName)) {
|
||||
console.log('[Session] Dead pane detected, respawning:', this._muxSession.muxName);
|
||||
const newPid = await mux.respawnPane(options.respawnPaneOptions);
|
||||
// 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(this._buildRespawnPaneOptions());
|
||||
if (!newPid) {
|
||||
console.error('[Session] Failed to respawn pane, will create new session');
|
||||
needsNewSession = true;
|
||||
@@ -1605,6 +1609,9 @@ export class Session extends EventEmitter {
|
||||
return false;
|
||||
}
|
||||
|
||||
// Confirmed the mux session (and thus the pane) exists but this reattach
|
||||
// is about to respawn it — safe to resolve/pin now.
|
||||
this._pinOmpRespawnId();
|
||||
const newPid = await mux.respawnPane(this._buildRespawnPaneOptions());
|
||||
if (!newPid) {
|
||||
console.error('[Session] reattachRemote: respawnPane failed for', this._muxSession.muxName);
|
||||
@@ -1637,21 +1644,16 @@ export class Session extends EventEmitter {
|
||||
piConfig: this._piConfig,
|
||||
grokConfig: this._grokConfig,
|
||||
deepSeekConfig: this._deepSeekConfig,
|
||||
// Respawning a dead pane means the CLI process exited (crash, idle
|
||||
// respawn, or the user's own /exit) but this is still the same
|
||||
// conversation from the user's perspective — unlike a brand-new
|
||||
// `createSession` call, defaulting to continuation here is the honest
|
||||
// behavior. `--continue` alone is ambiguous the moment ANY other omp
|
||||
// conversation has touched this directory more recently (ours resumed
|
||||
// elsewhere, a second Codeman session opened here, ...) since it just
|
||||
// picks the newest session file — so resolve and PIN the exact id the
|
||||
// pane that just died was writing to, once. The dead pane's file is
|
||||
// already fully flushed at this point, so "newest file" here is
|
||||
// unambiguous by construction; every later respawn then reuses the
|
||||
// pinned id instead of re-guessing. Only when the session already
|
||||
// carries an explicit resumeSessionId does this skip straight past it
|
||||
// (that one always wins in buildOmpCommand regardless).
|
||||
ompConfig: this._resolvedOmpRespawnConfig(),
|
||||
// OMP resolution/pinning does NOT happen here. This object is built
|
||||
// EAGERLY — including on every boot-recovery reattach, before anyone
|
||||
// knows whether the pane is actually dead — so resolving here mutated
|
||||
// `_ompConfig`/`_claudeSessionId` even for a pane that was simply being
|
||||
// reattached to, not respawned; with two omp tabs in the same case dir
|
||||
// that mis-pinned the ALIVE session onto whichever file happened to be
|
||||
// newest on disk (reported live in the Ark0N/Codeman#353 review). The
|
||||
// real pin now happens in `_pinOmpRespawnId()`, called by callers ONLY
|
||||
// once they've confirmed an actual respawn is about to happen.
|
||||
ompConfig: this._ompConfig,
|
||||
resumeSessionId: this._resumeSessionId,
|
||||
envOverrides: this._envOverrides,
|
||||
effort: this._effort,
|
||||
@@ -1671,23 +1673,18 @@ export class Session extends EventEmitter {
|
||||
* "newest file on disk" is safe here specifically. Non-omp modes and a
|
||||
* session that already carries an explicit id pass through untouched.
|
||||
*/
|
||||
private _resolvedOmpRespawnConfig(): OmpConfig | undefined {
|
||||
if (this.mode !== 'omp') return this._ompConfig;
|
||||
if (this._ompConfig?.resumeSessionId) return this._ompConfig;
|
||||
// Resolving-and-pinning is only correct when a mux session ALREADY exists for
|
||||
// this Session object — a dead-pane respawn, or a boot-recovery reattach (the
|
||||
// constructor sets _muxSession from persisted state before startInteractive()
|
||||
// ever runs there). A genuinely brand-new session (Run OMP -> POST
|
||||
// /api/quick-start -> a fresh Session with no muxSession in its create config)
|
||||
// has _muxSession still null at this point. Without this guard, the eager
|
||||
// `respawnPaneOptions: this._buildRespawnPaneOptions()` in startInteractive()
|
||||
// mutates this._ompConfig via the side effect below BEFORE
|
||||
// createSessionOptions.ompConfig is even read in the SAME object literal, so a
|
||||
// fresh "Run OMP" click silently inherited whatever omp conversation happened
|
||||
// to be newest on disk for this working directory instead of starting clean
|
||||
// (reported live 2026-08-27).
|
||||
if (!this._muxSession) return this._ompConfig;
|
||||
const resolvedId = findLatestOmpSessionId(this.workingDir);
|
||||
private _pinOmpRespawnId(): void {
|
||||
if (this.mode !== 'omp') return;
|
||||
if (this._ompConfig?.resumeSessionId) return;
|
||||
// Callers MUST call this only immediately before an ACTUAL respawn (a
|
||||
// confirmed-dead pane, or a genuine remote reattach) — never while merely
|
||||
// building options that might not lead to a respawn. A fresh "Run OMP"
|
||||
// click has no _muxSession yet and must never inherit whatever omp
|
||||
// conversation happens to be newest on disk for this working directory
|
||||
// (reported live 2026-08-27, fixed in 13a19f79); this guard keeps that
|
||||
// fix intact now that resolution has moved out of the eager options build.
|
||||
if (!this._muxSession) return;
|
||||
const resolvedId = resolveAndClaimOmpSessionId(this.workingDir);
|
||||
if (resolvedId) {
|
||||
this._ompConfig = { ...this._ompConfig, resumeSessionId: resolvedId };
|
||||
// Alias omp's own session uuid to this Codeman id — see the
|
||||
@@ -1695,14 +1692,15 @@ export class Session extends EventEmitter {
|
||||
// (generically-named) mechanism that folds a Past-Sessions row back
|
||||
// into its live/persisted session instead of duplicating it.
|
||||
this._claudeSessionId = resolvedId;
|
||||
return this._ompConfig;
|
||||
return;
|
||||
}
|
||||
// Nothing on disk yet (the dying process never got far enough to write a
|
||||
// session file) — fall back to the CLI's own "most recent" heuristic.
|
||||
// Nothing unclaimed on disk (the dying process never got far enough to
|
||||
// write a session file, or a sibling already claimed the only candidate)
|
||||
// — fall back to the CLI's own "most recent" heuristic.
|
||||
console.warn(
|
||||
`[Session] OMP: no session file found under ${this.workingDir} to pin --resume on respawn; falling back to ambiguous --continue`
|
||||
);
|
||||
return { ...this._ompConfig, continueSession: true };
|
||||
this._ompConfig = { ...this._ompConfig, continueSession: true };
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -1968,10 +1966,11 @@ export class Session extends EventEmitter {
|
||||
});
|
||||
|
||||
// Set claudeSessionId — when resuming, the Claude conversation ID is the
|
||||
// resumed one. `_resolvedOmpRespawnConfig()` (called above while building
|
||||
// respawnPaneOptions) may have JUST aliased this to omp's own session
|
||||
// uuid — that already-resolved id must win over the generic
|
||||
// `this.id` fallback, or this line clobbers it back to the Codeman id
|
||||
// resumed one. `_pinOmpRespawnId()` (called just above, inside
|
||||
// `_setupOrAttachMuxSession()`'s dead-pane branch) may have JUST aliased
|
||||
// this to omp's own session uuid — that already-resolved id must win
|
||||
// over the generic `this.id` fallback, or this line clobbers it back
|
||||
// to the Codeman id
|
||||
// on every single respawn.
|
||||
this._claudeSessionId = this._resumeSessionId || this._ompConfig?.resumeSessionId || this.id;
|
||||
|
||||
@@ -2394,7 +2393,7 @@ export class Session extends EventEmitter {
|
||||
|
||||
/**
|
||||
* A brand-new omp session (never yet respawned, so
|
||||
* {@link _resolvedOmpRespawnConfig} has never run) has no captured
|
||||
* {@link _pinOmpRespawnId} has never run) has no captured
|
||||
* omp-native session id: `_claudeSessionId` still defaults to this
|
||||
* session's OWN Codeman id from the constructor. Until something aliases
|
||||
* it, the omp history scan's row for this exact conversation (keyed by
|
||||
@@ -2407,7 +2406,7 @@ export class Session extends EventEmitter {
|
||||
private _maybeCaptureOmpSessionId(): void {
|
||||
if (this.mode !== 'omp' || this._claudeSessionId !== this.id) return;
|
||||
try {
|
||||
const resolvedId = findLatestOmpSessionId(this.workingDir);
|
||||
const resolvedId = resolveAndClaimOmpSessionId(this.workingDir);
|
||||
if (resolvedId) {
|
||||
this._claudeSessionId = resolvedId;
|
||||
this._ompConfig = { ...this._ompConfig, resumeSessionId: resolvedId };
|
||||
|
||||
@@ -15,7 +15,7 @@
|
||||
* @module utils/omp-session-resolver
|
||||
*/
|
||||
|
||||
import { readdirSync, statSync } from 'node:fs';
|
||||
import { closeSync, openSync, readdirSync, readSync, statSync } from 'node:fs';
|
||||
import { homedir } from 'node:os';
|
||||
import { join, sep } from 'node:path';
|
||||
|
||||
@@ -92,3 +92,95 @@ export function findLatestOmpSessionId(workingDir: string): string | null {
|
||||
}
|
||||
return newestId;
|
||||
}
|
||||
|
||||
/**
|
||||
* The session header line is always near the top of the file (the
|
||||
* transcript's own "second line" — see omp-transcript.ts), so identifying a
|
||||
* file never needs reading the whole thing (up to multi-MB, per that same
|
||||
* module's size cap). Bounded read only.
|
||||
*/
|
||||
const HEADER_READ_BYTES = 8 * 1024;
|
||||
|
||||
function readOmpSessionHeader(filePath: string): { id: string; cwd: string } | null {
|
||||
let raw: string;
|
||||
try {
|
||||
const fd = openSync(filePath, 'r');
|
||||
try {
|
||||
const buf = Buffer.alloc(HEADER_READ_BYTES);
|
||||
const bytesRead = readSync(fd, buf, 0, HEADER_READ_BYTES, 0);
|
||||
raw = buf.toString('utf-8', 0, bytesRead);
|
||||
} finally {
|
||||
closeSync(fd);
|
||||
}
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
for (const line of raw.split('\n')) {
|
||||
if (!line) continue;
|
||||
let entry: unknown;
|
||||
try {
|
||||
entry = JSON.parse(line);
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
if (!entry || typeof entry !== 'object') continue;
|
||||
const e = entry as Record<string, unknown>;
|
||||
if (e.type === 'session' && typeof e.id === 'string' && typeof e.cwd === 'string') {
|
||||
return { id: e.id, cwd: e.cwd };
|
||||
}
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Process-wide registry of OMP session ids already pinned to a live Codeman
|
||||
* session. Two omp tabs in the same case dir (`w1-foo`, `w2-foo`) resolve
|
||||
* against the SAME directory on disk — without this, both could pick the
|
||||
* newest file and alias onto each other's conversation (found in upstream PR
|
||||
* review, Ark0N/Codeman#353). Never released: this holds at most a handful of
|
||||
* short ids per real omp conversation ever pinned in this process's lifetime,
|
||||
* immaterial memory even after weeks of uptime — correctness here matters
|
||||
* more than reclaiming it.
|
||||
*/
|
||||
const claimedOmpSessionIds = new Set<string>();
|
||||
|
||||
/**
|
||||
* Safe variant of {@link findLatestOmpSessionId} for callers where two omp
|
||||
* sessions CAN share the same case directory — a dead-pane respawn, a
|
||||
* boot-recovery reattach, or a first-idle capture — instead of the narrower
|
||||
* cases where "newest file" is unambiguous by construction. Verifies each
|
||||
* candidate's own header `cwd` against `workingDir` (mangling is a lossy
|
||||
* one-way transform — see {@link mangleOmpWorkingDir} — so trusting the
|
||||
* filename-derived id alone isn't enough) and skips any id a sibling session
|
||||
* has already claimed. Claims the id it returns so a concurrent caller
|
||||
* resolving the same directory in the same tick can't double-claim it.
|
||||
*/
|
||||
export function resolveAndClaimOmpSessionId(workingDir: string): string | null {
|
||||
const dir = join(resolveOmpHome(), 'agent', 'sessions', mangleOmpWorkingDir(workingDir));
|
||||
let entries: string[];
|
||||
try {
|
||||
entries = readdirSync(dir);
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
|
||||
let newestMtime = -Infinity;
|
||||
let newestId: string | null = null;
|
||||
for (const entry of entries) {
|
||||
if (!OMP_SESSION_FILE_PATTERN.test(entry)) continue;
|
||||
const filePath = join(dir, entry);
|
||||
let mtimeMs: number;
|
||||
try {
|
||||
mtimeMs = statSync(filePath).mtimeMs;
|
||||
} catch {
|
||||
continue;
|
||||
}
|
||||
if (mtimeMs <= newestMtime) continue;
|
||||
const header = readOmpSessionHeader(filePath);
|
||||
if (!header || header.cwd !== workingDir || claimedOmpSessionIds.has(header.id)) continue;
|
||||
newestMtime = mtimeMs;
|
||||
newestId = header.id;
|
||||
}
|
||||
if (newestId) claimedOmpSessionIds.add(newestId);
|
||||
return newestId;
|
||||
}
|
||||
|
||||
@@ -1,18 +1,23 @@
|
||||
/**
|
||||
* @fileoverview Pins the "Run OMP always resumes" bug found live 2026-08-27.
|
||||
* @fileoverview Pins the "Run OMP always resumes" bug found live 2026-08-27,
|
||||
* and its follow-on fix for the sibling-aliasing bug found in upstream PR
|
||||
* review (Ark0N/Codeman#353).
|
||||
*
|
||||
* Session._resolvedOmpRespawnConfig() resolves-and-pins the newest on-disk omp
|
||||
* conversation as a side effect on `this._ompConfig`. That is correct when
|
||||
* reattaching to an ALREADY-TRACKED mux session (a dead-pane respawn, or a
|
||||
* boot-recovery reattach — the constructor sets `_muxSession` from persisted
|
||||
* state before startInteractive() ever runs there). It is wrong for a
|
||||
* genuinely brand-new session: startInteractive() computes
|
||||
* `respawnPaneOptions: this._buildRespawnPaneOptions()` EAGERLY in the same
|
||||
* object literal that builds `createSessionOptions.ompConfig: this._ompConfig`,
|
||||
* so the resolve-and-pin side effect ran and poisoned `this._ompConfig` before
|
||||
* that field was even read — a fresh "Run OMP" click in a working directory
|
||||
* with any prior omp history silently launched `--resume <old-id>` instead of
|
||||
* a clean `omp` invocation.
|
||||
* Session._pinOmpRespawnId() resolves-and-pins the newest on-disk omp
|
||||
* conversation as a side effect on `this._ompConfig`. That is correct ONLY
|
||||
* immediately before an ACTUAL respawn (a confirmed-dead pane, or a genuine
|
||||
* remote reattach) — never while merely building options that might not
|
||||
* lead to one. It used to run eagerly inside `_buildRespawnPaneOptions()`,
|
||||
* which startInteractive() calls unconditionally (including for a genuinely
|
||||
* brand-new session, and for a boot-recovery reattach to a pane that turns
|
||||
* out to still be alive): a fresh "Run OMP" click in a working directory
|
||||
* with any prior omp history silently launched `--resume <old-id>` instead
|
||||
* of a clean `omp` invocation, and — with two omp tabs in the same case dir
|
||||
* — a live pane's `_ompConfig`/`claudeSessionId` could get mis-pinned to
|
||||
* whichever sibling's file happened to be newest on disk, even though
|
||||
* nothing was actually being respawned. Resolution now happens only inside
|
||||
* `_pinOmpRespawnId()`, called by a caller that has already confirmed a
|
||||
* real respawn is happening.
|
||||
*/
|
||||
import { mkdirSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { homedir } from 'node:os';
|
||||
@@ -35,7 +40,11 @@ describe('OMP: fresh session vs. reattach must not share resumeSessionId resolut
|
||||
function seedOmpSessionFile(id: string) {
|
||||
mkdirSync(workingDir, { recursive: true });
|
||||
mkdirSync(sessionDir, { recursive: true });
|
||||
writeFileSync(join(sessionDir, `2026-08-27T17-31-08-001Z_${id}.jsonl`), '{}');
|
||||
// resolveAndClaimOmpSessionId() verifies the file's own header (not just
|
||||
// the filename), mirroring the real `omp` session-file shape — the
|
||||
// header's `cwd` must match `workingDir` for the candidate to count.
|
||||
const header = `${JSON.stringify({ type: 'session', id, cwd: workingDir })}\n`;
|
||||
writeFileSync(join(sessionDir, `2026-08-27T17-31-08-001Z_${id}.jsonl`), header);
|
||||
}
|
||||
|
||||
it('a brand-new session (no prior mux session) never inherits an on-disk conversation', async () => {
|
||||
@@ -56,8 +65,13 @@ describe('OMP: fresh session vs. reattach must not share resumeSessionId resolut
|
||||
expect(session.claudeSessionId).toBe(session.id);
|
||||
});
|
||||
|
||||
it('a reattach to an existing tracked mux session still resolves and pins the real id', async () => {
|
||||
seedOmpSessionFile('real-omp-uuid');
|
||||
it('a plain reattach to an existing mux session (pane still alive) does NOT pin', async () => {
|
||||
// Regression for the sibling-aliasing bug: pinning must never be a side
|
||||
// effect of merely building respawn options for a pane that might still
|
||||
// be alive (isPaneDead is unconditionally false under IS_TEST_MODE,
|
||||
// which is what a real "just reattaching, nothing died" boot recovery
|
||||
// looks like from Session's perspective).
|
||||
seedOmpSessionFile('sibling-conversation-id');
|
||||
|
||||
const muxSession: MuxSession = {
|
||||
sessionId: 'placeholder',
|
||||
@@ -81,7 +95,35 @@ describe('OMP: fresh session vs. reattach must not share resumeSessionId resolut
|
||||
await session.startInteractive();
|
||||
const state = session.toState();
|
||||
|
||||
expect(state.ompConfig?.resumeSessionId).toBe('real-omp-uuid');
|
||||
expect(state.ompConfig?.resumeSessionId).toBeUndefined();
|
||||
expect(session.claudeSessionId).toBe(session.id);
|
||||
});
|
||||
|
||||
it('_pinOmpRespawnId() resolves and pins the real id once a respawn is confirmed', () => {
|
||||
seedOmpSessionFile('real-omp-uuid');
|
||||
|
||||
const muxSession: MuxSession = {
|
||||
sessionId: 'placeholder',
|
||||
muxName: 'codeman-deadbeef',
|
||||
pid: 1,
|
||||
createdAt: Date.now(),
|
||||
workingDir,
|
||||
mode: 'omp',
|
||||
attached: false,
|
||||
};
|
||||
|
||||
const session = new Session({
|
||||
workingDir,
|
||||
mode: 'omp',
|
||||
mux: new TmuxManager(),
|
||||
useMux: true,
|
||||
muxSession,
|
||||
});
|
||||
sessions.push(session);
|
||||
|
||||
(session as unknown as { _pinOmpRespawnId(): void })._pinOmpRespawnId();
|
||||
|
||||
expect(session.toState().ompConfig?.resumeSessionId).toBe('real-omp-uuid');
|
||||
expect(session.claudeSessionId).toBe('real-omp-uuid');
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user