Merge pull request #1132 from ComposioHQ/feat/issue-1035
fix(core): skip liveness check on spawning sessions to prevent race condition
This commit is contained in:
commit
d9ff7315d4
|
|
@ -135,6 +135,44 @@ describe("check (single session)", () => {
|
|||
expect(plugins.runtime.isAlive).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not kill a spawning session even when runtimeHandle IS persisted in metadata (#1035)", async () => {
|
||||
vi.mocked(plugins.runtime.isAlive).mockResolvedValue(false);
|
||||
|
||||
const lm = setupCheck("app-1", {
|
||||
session: makeSession({
|
||||
status: "spawning",
|
||||
runtimeHandle: { id: "app-1", runtimeName: "mock", data: {} },
|
||||
metadata: {},
|
||||
}),
|
||||
// runtimeHandle IS in metadata — this is the production scenario
|
||||
});
|
||||
|
||||
await lm.check("app-1");
|
||||
|
||||
expect(lm.getStates().get("app-1")).toBe("working");
|
||||
expect(plugins.runtime.isAlive).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("does not kill a spawning session when agent reports exited activity (#1035)", async () => {
|
||||
vi.mocked(plugins.agent.getActivityState).mockResolvedValue({
|
||||
state: "exited" as ActivityState,
|
||||
timestamp: new Date(),
|
||||
});
|
||||
|
||||
const lm = setupCheck("app-1", {
|
||||
session: makeSession({
|
||||
status: "spawning",
|
||||
runtimeHandle: { id: "app-1", runtimeName: "mock", data: {} },
|
||||
metadata: {},
|
||||
}),
|
||||
});
|
||||
|
||||
await lm.check("app-1");
|
||||
|
||||
// Should transition to working, not killed
|
||||
expect(lm.getStates().get("app-1")).toBe("working");
|
||||
});
|
||||
|
||||
it("still probes a working session when it relies on a synthesized runtime handle", async () => {
|
||||
vi.mocked(plugins.runtime.isAlive).mockResolvedValue(false);
|
||||
|
||||
|
|
|
|||
|
|
@ -281,3 +281,31 @@ describe("deleteSession retry loop", () => {
|
|||
expect(deleteCallCount).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("spawning session liveness (#1035)", () => {
|
||||
it("does not call runtime.isAlive for spawning sessions, preventing false 'killed' status", async () => {
|
||||
// Write a session in "spawning" status with a persisted runtime handle
|
||||
writeMetadata(sessionsDir, "app-spawn", {
|
||||
worktree: "/tmp/ws",
|
||||
branch: "main",
|
||||
status: "spawning",
|
||||
project: "my-app",
|
||||
agent: "opencode",
|
||||
runtimeHandle: JSON.stringify(makeHandle("rt-spawn")),
|
||||
});
|
||||
|
||||
// Make isAlive return false — if it were called, the session would become "killed"
|
||||
vi.mocked(ctx.mockRuntime.isAlive).mockResolvedValue(false);
|
||||
|
||||
const sm = createSessionManager({ config, registry: mockRegistry });
|
||||
const sessions = await sm.list();
|
||||
const spawning = sessions.find((s) => s.id === "app-spawn");
|
||||
|
||||
// isAlive must NOT have been called for the spawning session
|
||||
expect(ctx.mockRuntime.isAlive).not.toHaveBeenCalled();
|
||||
|
||||
// Status must remain "spawning", not "killed"
|
||||
expect(spawning).toBeDefined();
|
||||
expect(spawning!.status).toBe("spawning");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -371,11 +371,12 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan
|
|||
|
||||
// Track activity state across steps so stuck detection can run after PR checks
|
||||
let detectedIdleTimestamp: Date | null = null;
|
||||
const hasPersistedRuntimeIdentity =
|
||||
typeof session.metadata["runtimeHandle"] === "string" ||
|
||||
typeof session.metadata["tmuxName"] === "string";
|
||||
// Never probe runtime/agent liveness while a session is still spawning —
|
||||
// tmux may not be initialized and the agent process hasn't started yet.
|
||||
// A persisted runtimeHandle alone is insufficient to gate on because spawn
|
||||
// writes it to metadata before leaving "spawning" status (#1035).
|
||||
const canProbeRuntimeIdentity =
|
||||
hasPersistedRuntimeIdentity || session.status !== SESSION_STATUS.SPAWNING;
|
||||
session.status !== SESSION_STATUS.SPAWNING;
|
||||
|
||||
// 1. Check if runtime is alive
|
||||
if (session.runtimeHandle && canProbeRuntimeIdentity) {
|
||||
|
|
@ -417,7 +418,7 @@ export function createLifecycleManager(deps: LifecycleManagerDeps): LifecycleMan
|
|||
const activityState = await agent.getActivityState(session, config.readyThresholdMs);
|
||||
if (activityState) {
|
||||
if (activityState.state === "waiting_input") return "needs_input";
|
||||
if (activityState.state === "exited") return "killed";
|
||||
if (activityState.state === "exited" && canProbeRuntimeIdentity) return "killed";
|
||||
|
||||
if (
|
||||
(activityState.state === "idle" || activityState.state === "blocked") &&
|
||||
|
|
|
|||
|
|
@ -881,13 +881,16 @@ export function createSessionManager(deps: SessionManagerDeps): OpenCodeSessionM
|
|||
plugins: ReturnType<typeof resolvePlugins>,
|
||||
handleFromMetadata: boolean,
|
||||
): Promise<void> {
|
||||
// Check runtime liveness first — regardless of session status.
|
||||
// This fixes #1081: terminal statuses (merged, done, etc.) should not force
|
||||
// activity to "exited" if the agent process is still alive.
|
||||
// Check runtime liveness first — for all statuses except "spawning".
|
||||
// Skip spawning sessions because tmux may not be fully initialized yet,
|
||||
// and a false-negative from isAlive() would permanently mark the session
|
||||
// as "killed" (see #1035).
|
||||
// This also fixes #1081: terminal statuses (merged, done, etc.) should not
|
||||
// force activity to "exited" if the agent process is still alive.
|
||||
// Fabricated handles (constructed as fallback for external sessions) should
|
||||
// NOT override status to "killed" — we don't know if the session ever had
|
||||
// a tmux session, and we'd clobber meaningful statuses like "pr_open".
|
||||
if (handleFromMetadata && session.runtimeHandle && plugins.runtime) {
|
||||
if (handleFromMetadata && session.runtimeHandle && plugins.runtime && session.status !== "spawning") {
|
||||
try {
|
||||
const alive = await plugins.runtime.isAlive(session.runtimeHandle);
|
||||
if (!alive) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue