Conversation
…not race
`render()` called `createRoot(container)` and `root.render(...)` outside this
suite's `act()` helper, then relied on a later `flushReact()` to catch up.
`flushReact()` is a single fixed pass — one microtask plus one `setTimeout(0)`
— so the first paint depends on the scheduler getting its turn inside that
one window.
On a CPU-starved runner it does not, and the assertion reads
`container.textContent === ""`:
AssertionError: expected '' to contain 'Pause'
ui/src/components/AgentActionButtons.test.tsx:175
That ejected the merge group for #1787 on 2026-09-21 with no defect in the PR
under test. Merge groups were ejecting at 17/33 on that day, and the ejectors
are timing-sensitive assertions across unrelated suites — the signature of
hypervisor CPU steal (BLO-35090), not of any diff.
This suite is the outlier: Inbox, Routines, CompanyInvites and ProjectDetail
all render inside `act` already. The local `act()` wraps `flushSync`, so
moving the render inside it flushes the first paint before `render()` returns
and the assertion no longer races the scheduler. Same class of fix as #1962,
which replaced a wall-clock deadline with an attempt budget.
Honest limit: the flake reproduces only under contention, so this is verified
as "3/3 pass, no new tsc errors, and the scheduling dependency is removed by
construction" — not by reproducing the red locally and watching it go green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-35090 |
|
@allyblockcast please review current HEAD of #1987. Exact HEAD: f0d3916 Report current Critical Issues and Important Issues counts; if clear, include exactly one standalone line: Reviewed head: f0d3916 Focus: AgentActionButtons render wrapped in act and first-paint race coverage. |
…e's 150ms budget
`GatewayState.upstreamTimeoutMs` is a single global budget, not per-upstream,
so the 150 ms this test set to trip the deliberately-hanging `stuck` upstream
was also the deadline for the healthy `alpha`. With `failureThreshold: 1`, one
`alpha` response slower than 150 ms opens its breaker for the rest of the test
and `tools/list` returns `[]`:
AssertionError: expected [] to deeply equal [ 'alpha__search' ]
packages/mcp-gateway/src/server.test.ts:1445
That ejected the merge group for #1952 on 2026-09-21. A local in-process
upstream is normally single-digit ms, but 150 ms is not much margin on a
runner losing ~50% of its cycles to hypervisor steal (BLO-35090).
Reproduced rather than inferred: squeezing the budget to 1 ms so the healthy
upstream also misses it produces that exact assertion locally.
`hanging` never answers at all, so raising the budget to 1 s does not weaken
what the test checks — the breaker still opens — it only costs wall-clock.
The `elapsed` assertion becomes proportional to the budget for the same
reason: an absolute 1000 ms bound re-introduces the steal sensitivity being
removed.
Verified: 52/52 in the file. Non-vacuity checked by re-squeezing the budget to
1 ms with the fix in place — still red, so the test has not been made blind.
Note the probe now trips the elapsed bound (`expected 64 to be less than 5`)
before reaching the tools assertion.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kkroo
changed the title
| @allyblockcast please review current HEAD of #1987. Exact HEAD: e91f74b. Report current Critical Issues and Important Issues counts; if clear, include exactly one standalone line: Reviewed head: e91f74b |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. All three ran degraded (nested CLI unavailable in this Kubernetes Job pod); prompts applied directly over the diff and exact-head file reads.
Reviewed head: e91f74b
Test-only, 2 files, +40/-12, no source change. Both fixes are correct and I verified the mechanism behind each rather than taking the description's word for it.
Critical Issues (0)
Important Issues (0)
I checked the one thing that could have made this an Important finding and it does not hold. expect(elapsed).toBeLessThan(UPSTREAM_TIMEOUT_MS * 5) is 5000 ms, and packages/mcp-gateway/vitest.config.ts sets no testTimeout, so under a bare vitest run the 5 s default would abort the test before the bound could ever fail. But CI does not run bare: scripts/run-vitest-stable.mjs:69 passes --testTimeout=30000, leaving 6x headroom. The assertion is enforceable in the environment this PR exists to fix.
Suggestions (2)
- [comments]
packages/mcp-gateway/src/server.test.ts:1429— the comment tells a future reader that "squeezing this to 1 ms reproduces that exact assertion locally", where "that exact assertion" is theexpected [] to deeply equal [ 'alpha__search' ]named six lines above. That was true against the old absolutetoBeLessThan(1000)bound, but it is no longer true against the proportional bound this PR introduces: withUPSTREAM_TIMEOUT_MS = 1, the elapsed check becomes< 5and trips first. The PR description already says so precisely (expected 64 to be less than 5). Someone following the comment will see a different failure and may conclude the repro is stale.- Suggest matching the description's wording: the 1 ms probe now trips the elapsed bound before reaching the tools assertion, which is still a valid non-vacuity check, just of a different assertion.
- [tests]
packages/mcp-gateway/src/server.test.ts:1452— the* 5multiplier only clears the bare-vitest 5 s default by exactly zero margin, so the bound is unreachable for anyone runningpnpm --filter @paperclipai/mcp-gateway testdirectly.* 3would keep the same steal-immunity (elapsed should land near 1 s) while staying enforceable locally too. Non-blocking — CI is the lane that matters here.
Strengths
- The mcp-gateway diagnosis is right, and it is not the obvious one.
upstreamTimeoutMsis a single global budget (server.test.ts:358,opts?.timeoutMs ?? 60_000) applied to every upstream, so the 150 ms intended for the deliberately-hungstuckwas also healthyalpha's deadline — and withfailureThreshold: 1a single slowalpharesponse opened its breaker for the rest of the test, yielding exactly the[]the CI failure reported. Raising the budget cannot weaken the test becausecreateHangingUpstream(:255) never callsres.end(), sostucktrips its breaker at any budget. - The UI fix removes the race by construction rather than by widening a window. The old
root.render()ran outside any flush, landing on DefaultLane and depending on the Scheduler'sMessageChanneltask beatingflushReact's singlesetTimeout(0)— an ordering that is genuinely not guaranteed under CPU starvation, which matches the observedexpected '' to contain 'Pause'(empty textContent, i.e. nothing mounted at all). InsideflushSyncthe mount lands on SyncLane and is flushed beforerender()returns. I confirmed the asserted text is reachable in that first synchronous paint:PauseandClear errorcome from theagentprop anduseMutationstate (AgentActionButtons.tsx:93,118,209), with nouseQueryfetch gating them. - Call-site conversion is complete. All three
render(call sites (:148,:164,:183) are awaited; no barerender(remains, which a floating-promise regression would have made silent. - Cleanup still holds. Moving
root = createRoot(container)inside theactcallback keepsafterEach's unmount working, including if the render throws after assignment. - Scope is correctly drawn, which I checked rather than assumed. The sibling tight budgets at
:1849(timeoutMs: 200) and:1866(timeoutMs: 150) drivecreateGateway, which takes a single upstream — there is no healthy upstream sharing the global budget, so they are not exposed to this failure mode and correctly went untouched. Theirelapsed < 3000bounds sit 15-30x above their budgets. - The comments are long for test code and earn it. Both encode the non-obvious causal fact (global-not-per-upstream budget; scheduler lane) whose absence is precisely what would let someone re-tighten these values in six months.
- Non-vacuity was probed, not assumed — re-squeezing the budget with the fix in place still goes red.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
|
Merge-queue ejection detected for PR #1987. The merge-group run failed and GitHub may have removed the PR from the queue and dropped auto-merge. Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR. Run: https://github.com/Blockcast/paperclip/actions/runs/35903530339 |
|
Merge-queue ejection detected for PR #1987. The merge-group run failed and GitHub may have removed the PR from the queue and dropped auto-merge. Inspect the merge-group jobs, fix or rerun the failing checks, then re-enqueue the PR. Run: https://github.com/Blockcast/paperclip/actions/runs/36219010052 |
|
Re-enqueued at The merge group was ejected at 05:56Z as 🤖 Generated with Claude Code |
This branch has not been deployed
2 participants
Thinking Path
What Changed
ui/src/components/AgentActionButtons.test.tsx—render()now doescreateRoot+root.renderinside the suite'sact()helper (which wrapsflushSync), and is awaited at its three call sites. Previously the first paint happened outside any flush and relied on a laterflushReact()— one microtask plus onesetTimeout(0)— to catch up.packages/mcp-gateway/src/server.test.ts— the aggregate-gateway timeout goes 150 ms → 1 s, namedUPSTREAM_TIMEOUT_MS, and theelapsedassertion becomes proportional to it instead of an absolute 1000 ms.Verification
Both failures were reproduced locally before being fixed, not inferred.
AgentActionButtons:175expected '' to contain 'Pause'server.test.ts:1445expected [] to deeply equal [ 'alpha__search' ]AgentActionButtons.test.tsx— 3/3 pass;tsc -p ui/tsconfig.jsonsurfaces no new errors in the file.packages/mcp-gateway/src/server.test.ts— 52/52 pass.expected 64 to be less than 5) before reaching the tools assertion.Limit worth stating. The
AgentActionButtonsflake only reproduces under contention, so for that one I removed the scheduling dependency by construction rather than watching a red turn green. The mcp-gateway one I did reproduce directly.Why the mcp-gateway change does not weaken the test
GatewayState.upstreamTimeoutMsis one global budget, not per-upstream, so the 150 ms meant for the deliberately-hangingstuckupstream was also the healthyalpha's deadline. WithfailureThreshold: 1, a single slowalpharesponse opens its breaker for the rest of the test.hangingnever answers at all, so a larger budget still trips its breaker — it only costs wall-clock.Risks
Low — test-only, no source change in either file.
AgentActionButtons: the render is now forced synchronous viaflushSync. No React warning appears in the run. A future test wanting to assert a pre-paint state could not do so throughrender(); none does.mcp-gateway: that one test now takes ~1 s longer, since it waits out the larger budget before the breaker opens.Model Used
Claude Opus 5 (1M context), extended thinking, with tool use.
Checklist
🤖 Generated with Claude Code