Skip to content

test(cli): make goal fork controls test deterministic - #14681

Merged
marius-kilocode merged 1 commit into
mainfrom
fix/flaky-goal-controls-test
Sep 30, 2026
Merged

marius-kilocode merged 1 commit into
mainfrom
fix/flaky-goal-controls-test

Conversation

@marius-kilocode

@marius-kilocode marius-kilocode commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

What Problem This Solves

The goal metadata/fork controls test failed with All fibers interrupted without error in Windows unit shard 6/6, run 36688682656, on a PR that does not change goal code. This test should check metadata, forks, transcript identity, and model selection without depending on response startup or cleanup speed.

Why This Change Was Made

The test has two unsynchronized lifecycle boundaries:

  • llm.wait(n) is signaled when the test server records a request, before it sends the response. It does not prove that the client has consumed the stream or published its step-start part. The test could fork and issue controls while stream startup was still in progress.
  • Even /goal with empty arguments cancels an active goal. Cancellation interrupts the autonomous runLoop work fiber registered by SessionRunState, but its caller can return before the prompt waiter releases its queue slot. The next control or resume did not wait for that cleanup.

Each resume now subscribes before starting the turn and waits for that session's published step-start part. Each control waits for SessionDrain.wait before reading the result or starting another control. These barriers establish client-side readiness and prompt cleanup without changing the hanging response fixture.

No sleeps, retries, larger existing timeouts, or skip gates were added. The existing 5.2-second final wait remains a negative assertion across the real 5-second goal repetition interval, not a readiness wait. No product code or behavior changed.

Causal limit: the exact Windows interruption did not reproduce locally. Effect produces the reported message when it squashes an Interrupt-only cause, but the CI log does not identify the foreground operation that received it. The intended cancellation target is the autonomous work fiber. The new barriers remove the two demonstrated ordering gaps, but do not prove the precise source of the original escaped interruption. Windows execution is now verified below; precise attribution of the original failure remains unconfirmed.

User Impact

Test-only synchronization hardening. Runtime goal behavior, model selection, transcript handling, and fork metadata behavior are unchanged. No changeset is needed.

Evidence

The branch started at freshly fetched origin/main, commit 0fd06048f7a60ed9fe5d082313a88a57be5af868.

Check Result
Unchanged target test, 50 fresh processes 50 passed, 0 failed
Revised target test, 100 fresh processes in four concurrent groups of 25 100 passed, 0 failed
bun test ./test/kilocode/session/goal.test.ts 96 passed, 0 failed
Windows CI unit (windows, 6/6) Passed; goal file 96 passed, 0 failed, 0 skipped
Target test in Windows JUnit artifact Passed in 5.92242 seconds, 39 assertions
bun run typecheck from packages/opencode Passed
Focused lint for goal.test.ts 0 errors, 10 pre-existing warnings
git diff --check Passed

All PR checks passed in run 36694537453, including all six Windows shards, Linux, macOS, and the HttpApi exerciser. Typecheck and annotation checks also passed. The affected Windows job actually executed the CLI tests, rather than replaying a cached test task. Its unit-windows-6-1 JUnit artifact contains the named target test as a passing testcase and reports all 96 goal tests passing.

Focused repeat command in each fresh process: bun test ./test/kilocode/session/goal.test.ts -t "preserves metadata and paused forks".

--rerun-each 50 was also attempted, but the shared preload's afterAll removes the process-wide test data after the first repetition. Repetitions 2-50 then failed with an unrelated missing log file. Those failures are excluded from the comparison above; fresh processes avoid that harness limitation.

Local verification ran on macOS with Bun 1.4.0-canary.1. Additional diagnostic runs under concurrent load, after the preceding permission-rejection test, and with Bun 1.3.14 also did not reproduce the original interruption. Windows CI now confirms the revised test passes on the affected runner. There is no measured before/after reduction in the original failure rate, and a passing CI run alone does not establish the precise cause of the original interruption.

@marius-kilocode
marius-kilocode marked this pull request as ready for review September 30, 2026 09:31
@marius-kilocode
marius-kilocode enabled auto-merge (squash) September 30, 2026 09:31
@kilo-code-bot

kilo-code-bot Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

The change is test-only synchronization hardening for goal.test.ts. The new resume helper subscribes to MessageV2.Event.PartUpdated (filtered on step-start) before issuing the command and joins the fiber with an explicit 15s barrier, and control now waits on SessionDrain.wait after each command. Both barriers are asserted against published signals rather than wall-clock sleeps, matching the fixtures guidance in packages/opencode/test/AGENTS.md. The removed llm.wait(n) calls were the racy readiness checks. No product code is touched; the forked subscription fiber is a child of the test fiber and is cleaned up by the test scope.

Files Reviewed (1 file)
  • packages/opencode/test/kilocode/session/goal.test.ts

Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0

Review guidance: REVIEW.md from base branch main

@marius-kilocode
marius-kilocode merged commit 0e7988a into main Sep 30, 2026
31 checks passed
@marius-kilocode
marius-kilocode deleted the fix/flaky-goal-controls-test branch September 30, 2026 10:41
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