fix(opencode): clean sessionBusyStart on idle and on session.error#40
Merged
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 2 of the 3 latent bugs tracked in the #25 comment (surfaced by #28's characterization tests, now on
main).sessionBusyStart(aMap<sessionID, startTime>) grew unboundedly:deletelived inside the>10_000msidle 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;notifySessionIdlestill fires only if busy lasted >10s.session.errornever 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.errordeletes the entry.Guarantees
notifySessionIdleiff busy >10s;notifySessionErroralways; busy.set/ eventemit/audit.loguntouched.if (started)→if (started !== undefined)— closes a theoretical timestamp-0edge; behaviorally identical for all realDate.now()values.Testing
main's code, GREEN only with the fix (not tautological). Plus a busy→error→idle no-misfire regression test.bun test625/0 ·bunx tsc --noEmitclean · scope =event.ts+event.test.tsonlyIndependent off
main. No overlap with the open #33 wave-2 PRs (#34–39).Refs #25 (2 of 3 latent items; the pipeline.flush no-op follows in a separate PR).