fix(e2e): cancel a case's interrupt once it has been accounted for - #82
Open
aniruddhaadak80 wants to merge 1 commit into
Open
aniruddhaadak80 wants to merge 1 commit into
aniruddhaadak80 wants to merge 1 commit into
Conversation
The mid-stream interrupt was scheduled and never cancelled:
let interruptTimer: NodeJS.Timeout | undefined;
if (spec.interrupt && parentTs) {
interruptTimer = setTimeout(() => { postAsUser(...) }, spec.interrupt.afterMs);
}
...
if (interruptTimer === undefined) {
/* no-op */
}
That trailing block is the tell: the handle was read only to keep the
compiler quiet about an unused variable, and nothing ever cleared the timer.
afterMs is longer than a lot of replies take, so a case whose bot answers
first returns with the timer still armed and the runner moves on to the next
case. It then fires into the finished case's thread: a message the runner
never sent, a bot turn nobody asked for, and an "interrupt send failed" line
appended to an errors array whose CaseResult was already returned and written
to the report.
The scheduling moves to e2e/interrupt.ts and hands back a cancel(), called as
soon as the interrupt phase has read its result - the point after which the
interrupt can no longer belong to this case. run.ts calls main() at import, so
this is also what makes the behaviour testable at all; the new test drives the
fake timers directly.
The dead no-op goes with it.
This branch has not been deployed
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
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.
What this changes
The E2E harness schedules a mid-stream interrupt for a case and then never cancels it:
and 23 lines later, this:
That block is the tell. The handle was read only to stop the compiler complaining about an unused variable, and nothing in
e2e/ever callsclearTimeout—grepfor it acrosse2e/*.tsreturns nothing, and the only othersetTimeoutcalls are all awaited.afterMsis deliberately longer than a lot of replies take, so this fires whenever the bot answers before the delay:afterMs: 15000, the bot replies in 3s,runCasereturns with the timer still armed.main()starts case B.parentTsthread — a message the runner never sent and no case is watching. That starts an unplanned bot turn.thread_not_found, an archived thread), it pushesinterrupt send failed: …into case A'serrorsarray — but A'sCaseResultwas already returned and written toreport.jsonatrun.ts:386-389.So a fast reply leaks a real message into a real thread, and can append a failure to a result that has already been reported.
The scheduling moves into
e2e/interrupt.tsand hands back acancel(), called as soon as the interrupt phase has read its result — the point after which the interrupt can no longer belong to this case. The dead no-op goes with it.Why a new module
e2e/run.tscallsmain()at import (run.ts:397-400), so a test importing it would execute the whole harness against real Slack and thenprocess.exit. The scheduling is therefore extracted, which is what makes the behaviour testable at all.vitest.config.tsonly includesapp/**/*.test.ts, so the test lives atapp/e2e-interrupt.test.tsand imports from../e2e/interrupt.js, matching the existingapp/e2e-cases.test.tswhich already reaches intoe2e/.Verification
Commands run from the repository root, per AGENTS.md:
pnpm check-types— exit 0.pnpm test— 30 files, 440 tests, all passing. The four new ones are inapp/e2e-interrupt.test.ts, using fake timers:is sent once the delay has passed— the behaviour that already worked, pinned.is not sent at all once the case has cancelled it— the regression. Cancels, then advances 120s and asserts nothing was sent.reports a send that failed, and still only once— theinterrupt send failed: …message is preserved, so the change does not swallow a real failure.can be cancelled after it has already fired without throwing— makescancel()idempotent, since it is called on a path that may run after the timer fired.e2e/interrupt.tsexisted the suite failed to resolve its import.node node_modules/railway/dist/iac/bin.js— exit 0,"diagnostics": [].Two things I did not run, stated plainly rather than implied:
uv run pytestinagent/(AGENTS.md lists it). This change is TypeScript-only and touches no Python, but I did not execute it.pnpm --dir deployment/aws install/build/test. My diff is confined toe2e/andapp/, so it cannot affect the CDK deployment, but I did not run those steps either.I also have not run
pnpm e2eitself — it needsSLACK_USER_TOKENand a live bot, so the fix is verified at the scheduling layer rather than against a real interrupted thread.