Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@
when metadata is missing or malformed, and revalidates it while holding the
per-name creation lock before removal.

### Generation-guarded gc cleanup and respawn

- Residual sweep and permanent respawn now compare the generation observed by
gc with the current metadata while holding the per-name creation lock.
Cleanup is skipped when another process replaced or edited the record after
gc's snapshot, preventing a stale pass from unlinking or respawning over the
replacement. Legacy generation-less records use exact metadata equality as a
conservative fallback. Bundled CLI respawns inherit the already-held
creation lock until the replacement publishes its socket.

### Attach-only CLI policy

- `pty attach --no-restart <ref>` attaches only to a currently running
Expand Down
11 changes: 9 additions & 2 deletions src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
validateName,
validateDisplayName,
acquireLock,
isLockOwnedByPid,
releaseLock,
updateTags,
setDisplayName,
Expand Down Expand Up @@ -1546,7 +1547,13 @@ async function cmdRun(
process.exit(1);
}

if (!acquireLock(name)) {
const delegatedOwner = Number(process.env.PTY_CREATION_LOCK_OWNER_PID);
const inheritedCreationLock =
Number.isSafeInteger(delegatedOwner) &&
isLockOwnedByPid(name, delegatedOwner);
// This is a one-hop control value for the CLI process, not session env.
delete process.env.PTY_CREATION_LOCK_OWNER_PID;
if (!inheritedCreationLock && !acquireLock(name)) {
console.error(
`Session "${name}" is being created by another process. Try again.`
);
Expand Down Expand Up @@ -1578,7 +1585,7 @@ async function cmdRun(
...(extraEnvOpt && Object.keys(extraEnvOpt).length > 0 ? { extraEnv: extraEnvOpt } : {}),
});
} finally {
releaseLock(name);
if (!inheritedCreationLock) releaseLock(name);
}

console.log(`Session "${name}" created.`);
Expand Down
122 changes: 99 additions & 23 deletions src/sessions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,50 @@ export async function cleanupRawCandidateGuarded(
}
}

function metadataMatchesObservation(
observed: SessionMetadata,
current: SessionMetadata,
): boolean {
if (observed.generation !== undefined || current.generation !== undefined) {
return observed.generation !== undefined &&
observed.generation === current.generation;
}
// Legacy records have no generation token. Exact structural equality is a
// conservative fallback: any intervening tag/launch/exit update makes the
// observation stale and suppresses cleanup.
return JSON.stringify(observed) === JSON.stringify(current);
}

function cleanupAllWhileLocked(name: string): void {
cleanupSocket(name);
try {
fs.unlinkSync(getMetadataPath(name));
} catch {}
try {
fs.unlinkSync(getEventsPath(name));
} catch {}
}

/** @internal Generation-CAS cleanup primitive; not part of client-api. */
export async function cleanupObservedSession(
session: SessionInfo,
): Promise<boolean> {
if (!session.metadata || !acquireLock(session.name)) return false;
try {
const current = readMetadata(session.name);
if (
!current ||
!metadataMatchesObservation(session.metadata, current)
) {
return false;
}
cleanupAllWhileLocked(session.name);
return true;
} finally {
releaseLock(session.name);
}
}

/** Return one bounded, read-only observation of the session registry.
*
* This function deliberately performs no lifecycle work: it does not create
Expand Down Expand Up @@ -981,8 +1025,9 @@ export async function gc(
}

try {
await respawnPermanent(s.name, s.metadata!, decision.newBookkeeping);
respawned.push({ name: s.name, ptyfileReread });
if (await respawnPermanent(s.name, s.metadata!, decision.newBookkeeping)) {
respawned.push({ name: s.name, ptyfileReread });
}
} catch (err: any) {
respawnFailed.push({ name: s.name, error: err?.message ?? String(err) });
}
Expand All @@ -1007,8 +1052,11 @@ export async function gc(
kept.push(s.name);
continue;
}
if (!dryRun) cleanupAll(s.name);
removed.push(s.name);
if (dryRun) {
removed.push(s.name);
} else if (await cleanupObservedSession(s)) {
removed.push(s.name);
}
}

return {
Expand Down Expand Up @@ -1208,12 +1256,14 @@ function classifyFlapping(
*
* Lazy-imports `spawn.ts` so the `sessions.ts ↔ spawn.ts` cycle doesn't
* bite at module-init time. After spawn, appends a `session_respawn`
* event to the session's event log so consumers see the restart. */
async function respawnPermanent(
* event to the session's event log so consumers see the restart.
*
* @internal Generation-CAS primitive; not part of client-api. */
export async function respawnPermanent(
name: string,
metadata: SessionMetadata,
bookkeepingOverlay: Record<string, string> = {},
): Promise<void> {
): Promise<boolean> {
let command = metadata.command;
let args = metadata.args;
let displayCommand = metadata.displayCommand;
Expand Down Expand Up @@ -1262,22 +1312,36 @@ async function respawnPermanent(
delete tags["strategy.status"];
}

// Wipe stale socket/pid/events before respawn so spawnDaemon doesn't
// trip over leftovers from the dead daemon. Metadata is recreated by
// spawnDaemon.
cleanupAll(name);

const { spawnDaemon } = await import("./spawn.ts");
await spawnDaemon({
name, command, args, displayCommand, cwd, tags,
...(displayName ? { displayName } : {}),
...(metadata.rows !== undefined ? { rows: metadata.rows } : {}),
...(metadata.cols !== undefined ? { cols: metadata.cols } : {}),
...(metadata.ephemeral !== undefined ? { ephemeral: metadata.ephemeral } : {}),
...(metadata.isolateEnv ? { isolateEnv: true } : {}),
...(extraEnv && Object.keys(extraEnv).length > 0 ? { extraEnv } : {}),
...(exactEnv ? { env: exactEnv } : {}),
});
// Serialize compare-and-swap cleanup + replacement creation under the same
// per-name lock. If the observed generation changed while gc was planning,
// this tick is stale and must not touch the replacement.
if (!acquireLock(name)) return false;
try {
const current = readMetadata(name);
if (!current || !metadataMatchesObservation(metadata, current)) {
return false;
}

// Wipe stale socket/pid/events before respawn so spawnDaemon doesn't trip
// over leftovers from the dead daemon. Keep the creation lock held until
// the replacement has published its socket.
cleanupAllWhileLocked(name);

const { spawnDaemon } = await import("./spawn.ts");
await spawnDaemon({
name, command, args, displayCommand, cwd, tags,
creationLockOwnerPid: process.pid,
...(displayName ? { displayName } : {}),
...(metadata.rows !== undefined ? { rows: metadata.rows } : {}),
...(metadata.cols !== undefined ? { cols: metadata.cols } : {}),
...(metadata.ephemeral !== undefined ? { ephemeral: metadata.ephemeral } : {}),
...(metadata.isolateEnv ? { isolateEnv: true } : {}),
...(extraEnv && Object.keys(extraEnv).length > 0 ? { extraEnv } : {}),
...(exactEnv ? { env: exactEnv } : {}),
});
} finally {
releaseLock(name);
}

// Best-effort event; respawn already succeeded if we got here.
try {
Expand All @@ -1287,6 +1351,7 @@ async function respawnPermanent(
ts: new Date().toISOString(),
});
} catch {}
return true;
}

/**
Expand Down Expand Up @@ -1519,6 +1584,17 @@ function getLockPath(name: string): string {
return path.join(getSessionDir(), `${name}.lock`);
}

/** @internal Verify an explicitly delegated creation lock without acquiring it. */
export function isLockOwnedByPid(name: string, ownerPid: number): boolean {
if (!Number.isSafeInteger(ownerPid) || ownerPid <= 0) return false;
try {
return parseInt(fs.readFileSync(getLockPath(name), "utf-8").trim(), 10) === ownerPid &&
isProcessAlive(ownerPid);
} catch {
return false;
}
}

/**
* Acquire an exclusive lock for a session name. Prevents concurrent
* `pty run` calls from racing to create the same session.
Expand Down
11 changes: 11 additions & 0 deletions src/spawn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ export interface SpawnDaemonOptions {
* path can't express it (a bundled consumer that hits the fallback also
* isn't the operator-restart context this guards). */
scrubEnv?: string[];
/** @internal PID owning an already-held per-name creation lock. Used only
* when a lifecycle operation must keep its CAS lock across CLI fallback. */
creationLockOwnerPid?: number;
}

/** Default time we wait for a daemon's Unix socket to appear after
Expand Down Expand Up @@ -236,9 +239,17 @@ function spawnViaCli(options: SpawnDaemonOptions): Promise<void> {
}
cliArgs.push("--", options.command, ...options.args);

const env = { ...process.env };
if (options.creationLockOwnerPid !== undefined) {
env.PTY_CREATION_LOCK_OWNER_PID = String(options.creationLockOwnerPid);
} else {
delete env.PTY_CREATION_LOCK_OWNER_PID;
}

const result = spawnSync("pty", cliArgs, {
stdio: ["ignore", "pipe", "pipe"],
encoding: "utf-8",
env,
});
if (result.error !== undefined) {
const err = result.error as NodeJS.ErrnoException;
Expand Down
Loading
Loading