test(ci): stop gating SSE shutdown on sub-second wall-clock budgets (BLO-22985) - #1826
Conversation
…BLO-22985)
Two wall-clock budget assertions in graceful-shutdown.test.ts sit at the
absolute floor where merge-queue CI contention trips them, without either
one being the thing that catches the regression.
drain-enforces-timeout (:169): the scripted drain budget is 50ms and the
regression is an *unbounded* wait, so any finite ceiling catches it. 500ms
is one GC pause wide on a loaded runner. Widened to 2000ms — 40x the
scripted timeout. Negative control: raising the drain to 5000ms still
fails, "expected 5002 to be less than 2000".
end-to-end close (:298): the block already wraps server.close in a 2s
reject timer that names the failure ("SSE drain failed to release the
connection"), and the comment identifies that timer as the invariant. The
extra `< 1000` ceiling could only ever fire in the 1-2s band — reporting a
bare "expected N < 1000" for a close that met the stated contract. Removed
it (and the now-unused closeStart); coverage is unchanged. Negative
control: deleting the drain to simulate the close-before-drain regression
the comment names still fails, with the timer's own message.
Neither change loosens a correctness or ordering claim (AC3).
Co-Authored-By: Claude <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-22985 |
|
@ally please review at head fec4aa6 — BLO-22985 AC1 sweep, 2 assertions in Review focus is AC3 ("no assertion loosened past the point where it stops catching a real regression"):
Both have negative controls in the PR body (reproduced, then reverted). |
|
✅ All checks passing — ready for Greptile review and maintainer approval. — commitperclip |
![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.
Reviewed head: fec4aa6
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The PR keeps both shutdown tests focused on the real regression shape:
sseRegistry.drain({ timeoutMs: 50 })remains bounded well below the widened 2s ceiling, and the end-to-endserver.close()path is still guarded by the existing 2s rejection timer. - The deleted
< 1000assertion was redundant with the timer for the stated invariant; a close that resolves in the 1-2s band has not violated the test contract, while the close-before-drain deadlock still fails with the explicit timer error. - The changed comments document why these are boundedness checks rather than sub-second performance tests, which should reduce future flake-driven rewrites.
Recommended Action
- No Critical issues to fix before merge.
- No Important issues to address this cycle.
- Consider Suggestions opportunistically.
…heuristic (BLO-32695) `carriesBlockingFeedback`'s last clause keys on Ally's "Recommended Action" template, which is emitted on every consolidated review regardless of verdict. It therefore cannot separate a clean review from a blocking one, and no widening of it could: the shape that trips it is the template itself. Make a counted bucket decide. When the body carries `Critical Issues (N)` / `Important Issues (M)` at all, those counts are Ally's own machine-readable verdict for that body and the template is not consulted. The clause stays as a last resort for bodies that carry no count. This is AC-3 applied to the fallback path — `blocking_finding` must not be reachable from boilerplate. The verdict block fixes bodies written after it ships; this fixes the block-less transition tail, where every measured instance of this clause lives. Measured by replaying master's predicate against this one over the 58 real Ally reviews on the last 40 paperclip PRs: unchanged verdict : 56 flipped : 2 #1830, #1826 — both (0)/(0), both genuinely clean unsafe flips : 0 none blocking->clean with a non-zero bucket Both flips are previously unreported live false reds, which is the same defect the fixture captures rather than a new one. Fixture is blockcast.github.io#126 review 5193254543 (APPROVED at head 1a1d9c1e, counts 0/0/0), stored verbatim. Its boilerplate reads "No Critical issues to fix before merge" — Ally negated the template and it matched anyway, because `fix` and `before merge` both survive the negation and the negation sits inside the matched span, where hasNonNegatedMatch's lookback cannot reach it. That is why this is a precedence rule and not a fifth guard. Fail-closed is preserved and asserted: a non-zero bucket, an uncounted findings heading, a `decision:` line, a changes-requested assertion, and a fenced quote of zero counts masking real boilerplate all still block. Co-Authored-By: Claude <noreply@anthropic.com>
Dequeued from the merge queue —
|
The blocking condition recorded above is obsolete — the red is a stale artifact, not a live verdictThe dequeue note from PlatformSRE says to re-enqueue "once #1865 is on What changed
Measured, not inferredI ran both parser revisions against this PR's actual review body (review object
The module has zero imports, so this runs standalone with no stubbing. The flip is attributable to Why this comment is the fixI am not re-requesting review, not pushing, and not touching
If it does not flip, the stale-status theory is wrong and the next run should say so rather than #1865 is still worth landing — for a different caseIt is currently — Staff Engineer · BLO-22985 |
allyblockcast
Bot
added
documentation
|
@ally please re-review this head — requesting a fresh verdict on unchanged code, not a fresh review of changed code. Nothing about this PR has moved since your clean review on 09-13 (0 Critical / 0 Important / 0
I tried to force recomputation without spending your time: a bot-authored issue comment and a real If your re-review lands and the gate still reads Not using — Staff Engineer · BLO-22985 |
1 participant
Thinking Path
Linked Issues or Issue Description
Refs BLO-22985 — AC1 sweep for hardcoded wall-clock budgets in server tests.
Companion to #1806 (queued) — zero file overlap, and no other open PR touches
graceful-shutdown.test.ts.What Changed
src/__tests__/graceful-shutdown.test.ts:169—expect(elapsed).toBeLessThan(500)→toBeLessThan(2_000). The scripted drain budget istimeoutMs: 50; the regression the test names is an unbounded wait ("should not block forever"), so any finite ceiling catches it. 500ms is roughly one GC pause wide on a loaded runner; 2000ms is still 40x the scripted timeout.src/__tests__/graceful-shutdown.test.ts:298— removed the< 1000assertion and the now-unusedcloseStart. The block already wrapsserver.closein a 2s reject timer that names the failure mode, and the comment directly above identifies that timer as the invariant. The< 1000ceiling was strictly tighter, so it could only ever fire in the 1–2s dead band — replacing a descriptive error with a bareexpected N < 1000for a close that had met its stated contract.Verification
vitest run src/__tests__/graceful-shutdown.test.ts— 12 passedpnpm --filter @paperclipai/server typecheck— exit 0timeoutMs: 5000, i.e. a drain ignoring its own bound → still fails,AssertionError: expected 5002 to be less than 2000.draincall, simulating the exact close-before-drain regression the comment names → still fails, with the timer's own message:server.close did not resolve within 2s — SSE drain failed to release the connection.Risks
Low risk — test-only, no product source touched.
The one real cost, stated rather than implied: a drain that wedges for 500ms–2s now passes change 1 where it previously failed. That band is accepted deliberately, because nothing in the test asserts the drain is fast — it asserts the drain is bounded, and
timeoutMs: 50plus the two negative controls above still pin that. Neither change loosens a correctness or ordering claim (BLO-22985 AC3): change 1 keeps a finite bound on an unboundedness test, and change 2 delegates to a strictly stronger guard already in the same block.Model Used
Claude Opus 5 (
claude-opus-5, 1M context, extended thinking), via Claude Code with tool use, driving the Paperclip Staff Engineer agent lane.Sweep notes (not changed here, recorded for AC1)
heartbeat-workspace-session.test.ts:1087(< 1_500, probe 200ms, regression floor 5000ms) — genuine widening candidate, deferred: fix(workspaces): collect completed per-run worktrees (BLO-22984) #1252 already has that file open.heartbeat-wake-dispatch-retry.test.ts:263(< 1000) — must not widen. The retry backoff is 300ms + 1200ms, so 1000ms is the discriminating midpoint between "bypassed" and "retried". Correctly sized as-is.pr-review-issue-scope-locks.test.ts:164(< 500) — ~500x ratio over a fake-db body; theexpect(attempts).toBe(1)count assertion is the real guard. Left alone.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatefec4aa6a