Skip to content

[Fix] Subtask e2e suite can inherit a cancelled delayed mock stream from the previous test - #1074

Draft
zoomote[bot] wants to merge 1 commit into
mainfrom
fix/subtask-delayed-stream-e2e-34zldbjo9f55t
Draft

[Fix] Subtask e2e suite can inherit a cancelled delayed mock stream from the previous test#1074
zoomote[bot] wants to merge 1 commit into
mainfrom
fix/subtask-delayed-stream-e2e-34zldbjo9f55t

Conversation

@zoomote

@zoomote zoomote Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Created by Roomote. Follow up by mentioning @zoomote, in the web UI, or in Discord.

Related GitHub Issue

Split out from #835 (test-stabilization portion only; no production telemetry changes). Independent from main — does not depend on any of the other split PRs.

Description

Stabilizes the delayed-stream subtask e2e tests in apps/vscode-e2e, test-only.

The race being prevented: the API-hang child fixture delayed its mocked response with aimock's flat latency: 15_000. aimock applies flat latency to every SSE chunk and never observes client disconnects (writeSSEStream only honors the fixture's own interruption signal). When the test cancels the child's in-flight request, the mock keeps the stream pending server-side for chunks × latency (~75s for this 5-chunk response) and then flushes the remaining chunks to the dead socket. A streamed request opened by the next test can interleave with that late flush — so one test's unfinished delayed request leaked into the next test.

The fix, in two parts:

  1. fixtures/subtasks.ts — the fixture now delays only the first chunk via streamingProfile: { ttft: SUBTASK_API_HANG_RESPONSE_LATENCY_MS }. With no flat latency set, chunks after the first get ~0 delay, so the server-side pending window is exactly one named constant instead of an unbounded chunks × latency. Observable pre-cancel behavior is identical (no bytes for 15s), so the test scenario is preserved.
  2. suite/subtasks.test.ts — the API-hang test captures the request's aimock journal timestamp (journal entries are stamped when the request is received, before the delayed write begins) and, in finally, drains via waitForDelayedSubtaskStreamDrain: it waits only the remaining time until timestamp + SUBTASK_API_HANG_RESPONSE_LATENCY_MS + 500ms grace.

Why this synchronization point is reliable: the deadline is anchored to the journal timestamp of the exact request this test created (scenario-unique marker + parent-marker exclusion, matched before cancellation, so it can only be the original hung request — never a stale or resumed one). The wait is bounded by one latency window plus grace, so it cannot hide a genuine hang; it no-ops if the delayed request never reached the mock. This replaces the arbitrary fixed sleep(latency) / blanket per-test sleep(1_500) teardown approach with an explicit, bounded drain.

Production files deliberately untouched: all of src/, webview-ui/, packages/ — including every telemetry behavior from #835 (consent, circuit breaker, shutdown, task aggregation, tool attribution, model-cache). bedrock-mock-server.ts was also left untouched: the subtask suite routes through aimock, not the bedrock mock, and aimock's journal already exposes the needed synchronization point.

Test Procedure

  • TEST_FILE=subtasks.test.js xvfb-run -a pnpm --filter @roo-code/vscode-e2e test:ci:mock — 11/11 passing, run twice consecutively; the API-hung test completes in a deterministic ~15.8s (one latency window).
  • npx tsc -p apps/vscode-e2e/tsconfig.json --noEmit — clean.
  • pnpm --dir apps/vscode-e2e exec eslint --prune-suppressions --max-warnings=0 src/fixtures/subtasks.ts src/suite/subtasks.test.ts — clean, no suppression-count increases.
  • Full repo pnpm test: zoo-code (src) 7190 passed. One unrelated pre-existing flake in webview-ui (SlashCommandsSettings.spec.tsx › "refreshes commands after creating new command") failed under the full parallel run but passes in isolation; that package is untouched by this diff.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Visual Snapshot (UI changes only): If a user would notice this change at a glance (layout, theme tokens, brand elements, empty/error states), I've added or updated a *.visual.tsx snapshot in webview-ui/. See webview-ui/AGENTS.md → "When a UI change needs a snapshot".
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Visual Snapshots

N/A — test-only change, no UI surface.

Videos (interaction / animation only)

N/A.

Documentation Updates

  • No documentation updates are required.
  • Yes, documentation updates are required. (Please describe what needs to be updated or link to a PR in the docs repository).

Additional Notes

Success criterion: repeated runs of the subtask suite do not inherit delayed mock activity from the preceding test — verified with two consecutive clean mock-mode runs.

Get in Touch

edelauna

The API-hang subtask fixture used aimock's flat latency, which applies
per SSE chunk and is never interrupted by client disconnects, so a
cancelled delayed stream stayed pending server-side for chunks x latency
and could flush into the next test's traffic.

- delay only the first chunk via streamingProfile.ttft so the pending
  window is exactly the shared SUBTASK_API_HANG_RESPONSE_LATENCY_MS
- anchor the post-test drain to the request's aimock journal timestamp
  and wait out only the remainder of that bounded window
@zoomote

zoomote Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

No code issues found. See task

Reviewed both changed files in context and verified the core claims against the installed aimock 1.35.0 internals:

  • streamingProfile: { ttft } is a supported fixture option and is passed through to writeSSEStream; with no fixture-level or LLMock-level flat latency (confirmed runTest.ts sets none, and aimock defaults it to 0), chunks after the first get ~0 delay, so the pending window really is one ttft.
  • Journal entries are stamped with Date.now() in journal.add, which runs before writeSSEStream starts the ttft delay, so the drain deadline anchored to the journal timestamp is correct and bounded.
  • The marker match (child marker + parent-marker exclusion, first journal entry) can only identify this test's hung request; the suite has no mocha retries and the journal is per-run, so no stale-anchor risk.
  • The drain in finally is bounded (one window + 500ms grace, ample since the post-ttft flush is a synchronous chunk loop) and no-ops when the delayed request never reached the mock.

Reviewed b3f3347

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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.

1 participant