Skip to content

test(cli): remove session cancellation timing races - #11299

Merged
markijbema merged 2 commits into
mainfrom
mark/stabilize-session-cancellation-tests
Jun 16, 2026
Merged

test(cli): remove session cancellation timing races#11299
markijbema merged 2 commits into
mainfrom
mark/stabilize-session-cancellation-tests

Conversation

@markijbema

@markijbema markijbema commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Why

The Windows unit job in PR #10388 failed in two session cancellation tests even though the corresponding Linux job passed. Looking across recent failed workflows showed both failures were intermittent and strongly correlated with loaded Windows runners:

  • stops quickly when aborted during retry backoff failed when interrupt cleanup exceeded an internal 2-second wall-clock deadline.
  • cancel with queued callers resolves all cleanly previously failed at its whole-test timeout and still used a fixed 50 ms sleep to assume the second caller had joined the active run.

Neither deadline is part of the behavior these tests need to prove. They make correctness depend on how quickly CI schedules Effect fibers and finalizers. Workflow reruns frequently pass without code changes, while the underlying cancellation assertions remain stable.

What changed

  • Remove the inner timeout around compaction fiber interruption. The test still waits for the published retry status before interrupting, then verifies that the fiber exits through interruption. The outer test timeout still protects against a genuine deadlock.
  • Replace Effect.sleep(50) with Effect.yieldNow before cancelling queued prompt callers. This gives the newly forked caller a scheduler turn without requiring it to complete within an arbitrary real-time window.

Why this preserves coverage

The behavioral assertions are unchanged:

  • retry backoff must be interruptible and terminate with an interrupt cause;
  • both callers sharing a prompt run must resolve successfully after cancellation;
  • both callers must receive the same assistant message.

This removes timing assumptions rather than weakening the expected cancellation semantics. The lower-level runner suite also covers queued cancellation using observable runner state, so the prompt integration test remains focused on end-to-end behavior.

Co-authored-by: kiloconnect[bot] <240665456+kiloconnect[bot]@users.noreply.github.com>
@markijbema markijbema self-assigned this Jun 16, 2026
@kilo-code-bot

kilo-code-bot Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • packages/opencode/test/session/compaction.test.ts - Removed scheduler-sensitive inner timeout on compaction interrupt verification
  • packages/opencode/test/session/prompt.test.ts - Replaced Effect.sleep(50) calls with Effect.yieldNow and waitFor + SessionStatus.Service readiness signals
Previous Review Summary (commit 670f2fa)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 670f2fa)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (2 files)
  • packages/opencode/test/session/compaction.test.ts - Removed scheduler-sensitive inner timeout on compaction interrupt verification
  • packages/opencode/test/session/prompt.test.ts - Replaced Effect.sleep(50) with Effect.yieldNow for cooperative scheduler sync
Other Observations (not in diff)

Several remaining Effect.sleep(50) / Effect.sleep(20) calls in prompt.test.ts (lines 93, 1290, 1695, 1737, 1971, 1974, 2004) follow the same anti-pattern that was fixed on line 1237. Consider replacing these with Effect.yieldNow or proper readiness signals in a follow-up to reduce further CI flakes.


Reviewed by deepseek-v4-pro-20260423 · 371,954 tokens

Review guidance: REVIEW.md from base branch main

Convert the remaining `Effect.sleep(50)` synchronization hacks in
`prompt.test.ts` to either `Effect.yieldNow` or `waitFor` against
`SessionStatus`, mirroring the pattern already used elsewhere in the
file. Same motivation as the previous commit: stop making correctness
depend on how quickly CI schedules fibers.

- "concurrent loop callers all receive same error result": yieldNow
  before resolving the gate, so the queued caller joins the run on a
  scheduler turn instead of a 50ms window.
- "loop waits while shell runs ..." / "shell completion resumes
  queued loop callers": yieldNow before asserting `llm.calls === 0`
  while the shell is still running. The shell is already gated on
  `waitFor("shell busy", ...)`.
- "cancel interrupts loop queued behind shell": waitFor shell busy
  before forking the loop, then yieldNow before cancelling.
- "shell rejects when another shell is already running": waitFor
  shell busy before issuing the second shell call.

The `Effect.sleep(20)` at line 93 is the poll interval inside the
`waitFor` helper itself and is left unchanged.
@markijbema
markijbema enabled auto-merge June 16, 2026 14:09
@markijbema
markijbema merged commit 8c36dde into main Jun 16, 2026
19 checks passed
@markijbema
markijbema deleted the mark/stabilize-session-cancellation-tests branch June 16, 2026 14:44
t7tran pushed a commit to t7tran/kilocode that referenced this pull request Aug 14, 2026
…n-cancellation-tests

test(cli): remove session cancellation timing races
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants