From 4aa416802857036f0a2fc70c25ab86cc5f0ecefe Mon Sep 17 00:00:00 2001 From: Lesquel Date: Sun, 17 May 2026 14:48:36 -0500 Subject: [PATCH] fix(opencode): clean sessionBusyStart on idle and on session.error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two latent bugs (issue #25) surfaced by #28's characterization tests. The sessionBusyStart Map grew unboundedly: - Bug 2: the delete was gated inside the >10_000ms idle check, so a quick busy→idle cycle never removed the entry. The threshold must gate only the notification, not the cleanup. Now: on idle, always delete the entry if present; notify only if busy lasted >10s. - Bug 1: session.error never deleted the entry — an errored (no longer busy) session left a stale entry that a later idle for a reused ID could misfire on. Now: session.error deletes the entry. Notification semantics are byte-preserved: notifySessionIdle still fires iff busy >10s; notifySessionError still always fires with the same error extraction; busy .set / event emit / audit unchanged. Also tightened the idle guard `if (started)` → `if (started !== undefined)` (closes a theoretical timestamp-0 edge). The #28 characterization tests that asserted the buggy behavior were flipped to assert the fix (genuinely RED on the old code), plus a busy→error→idle no-misfire regression test. bun test 625/0, tsc clean. Independent fresh-context review: APPROVE, no issues (notification semantics quoted main-vs-branch identical; tests verified non-tautological). Refs #25 --- src/integrations/opencode/hooks/event.test.ts | 45 +++++++++++++------ src/integrations/opencode/hooks/event.ts | 9 ++-- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/integrations/opencode/hooks/event.test.ts b/src/integrations/opencode/hooks/event.test.ts index 65a66a3..07e7af9 100644 --- a/src/integrations/opencode/hooks/event.test.ts +++ b/src/integrations/opencode/hooks/event.test.ts @@ -105,7 +105,7 @@ describe("session.status busy→idle under 10s threshold", () => { expect(notifications.calls.filter(c => c.kind === "idle")).toHaveLength(0) }) - test("sessionBusyStart entry is deleted when under threshold", async () => { + test("sessionBusyStart entry is deleted even when under threshold (no leak)", async () => { const sessionBusyStart = new Map() const notifications = makeNotifications() const hook = createEventHook(notifications, sessionBusyStart, makeClient(), makeAudit()) @@ -115,11 +115,9 @@ describe("session.status busy→idle under 10s threshold", () => { await hook(makeStatusEvent(sessionID, "idle")) - // The entry must be cleaned up regardless of threshold outcome - // ACTUAL behavior: the entry is NOT deleted when under threshold (the code - // only deletes inside the `if (started && Date.now() - started > 10_000)` block) - // This is a characterization test for the real behavior: - expect(sessionBusyStart.has(sessionID)).toBe(true) // NOT cleaned up when under threshold + // The entry must ALWAYS be cleaned up on idle, regardless of whether the + // notification threshold was crossed (threshold only gates notification, not cleanup). + expect(sessionBusyStart.has(sessionID)).toBe(false) // cleaned up regardless of threshold }) }) @@ -215,10 +213,7 @@ describe("session.error behavior", () => { expect(errorCalls[0]!.error).toBe("flat string error") }) - // ─── LATENT BUG CHARACTERIZATION TEST ───────────────────────────────────── - // The audit flagged that sessionBusyStart is NOT cleaned up on session.error. - // This test asserts the ACTUAL current behavior (NOT fixed here — see final summary). - test("CHARACTERIZATION: sessionBusyStart is NOT cleaned up on session.error", async () => { + test("sessionBusyStart is cleaned up on session.error (errored session no longer busy)", async () => { const sessionBusyStart = new Map() const notifications = makeNotifications() const hook = createEventHook(notifications, sessionBusyStart, makeClient(), makeAudit()) @@ -229,11 +224,35 @@ describe("session.error behavior", () => { await hook(makeStatusEvent(sessionID, "busy")) expect(sessionBusyStart.has(sessionID)).toBe(true) // sanity check - // Now fire session.error — the code at event.ts:39-45 does NOT touch sessionBusyStart + // Now fire session.error — the errored session is no longer busy; entry must be removed await hook(makeErrorEvent(sessionID, "crashed")) - // ACTUAL behavior: entry remains in the map (possible memory leak) - expect(sessionBusyStart.has(sessionID)).toBe(true) + // FIXED behavior: entry is removed on error to prevent stale-ID misfire on reuse + expect(sessionBusyStart.has(sessionID)).toBe(false) + }) + + test("busy→error→idle (reused ID) does NOT misfire notifySessionIdle", async () => { + // Regression: without the error-cleanup fix, a session that errors and then + // idles (e.g. after a restart with the same ID) could see a stale busy-start + // from before the error and fire a spurious idle notification >10s later. + const sessionBusyStart = new Map() + const notifications = makeNotifications() + const hook = createEventHook(notifications, sessionBusyStart, makeClient(), makeAudit()) + + const sessionID = "sess-reuse" + + // Mark busy far in the past (would trip the >10s threshold) + sessionBusyStart.set(sessionID, Date.now() - 30_000) + + // Session errors — must clean the entry + await hook(makeErrorEvent(sessionID, "crashed")) + expect(sessionBusyStart.has(sessionID)).toBe(false) + + // Same ID goes idle — should NOT fire idle notification (no busy start present) + await hook(makeStatusEvent(sessionID, "idle")) + + const idleCalls = notifications.calls.filter(c => c.kind === "idle") + expect(idleCalls).toHaveLength(0) // no spurious notification }) }) diff --git a/src/integrations/opencode/hooks/event.ts b/src/integrations/opencode/hooks/event.ts index d766d93..1201bf0 100644 --- a/src/integrations/opencode/hooks/event.ts +++ b/src/integrations/opencode/hooks/event.ts @@ -29,14 +29,17 @@ export function createEventHook( sessionBusyStart.set(sessionID, Date.now()) } else if (sessionID && status?.type === "idle") { const started = sessionBusyStart.get(sessionID) - if (started && Date.now() - started > 10_000) { - sessionBusyStart.delete(sessionID) - await notifications.notifySessionIdle(client, sessionID) + if (started !== undefined) { + sessionBusyStart.delete(sessionID) // always clean on idle; threshold only gates notification + if (Date.now() - started > 10_000) { + await notifications.notifySessionIdle(client, sessionID) + } } } } if (event.type === "session.error" && sessionID) { + sessionBusyStart.delete(sessionID) // errored session is no longer busy; prevent stale-ID misfire const errorObj = props?.error as Record | undefined const error = (errorObj?.data as Record)?.message as string ||