fix(ci): space the cold-connect canary so each sample meets a cold database - #214
fix(ci): space the cold-connect canary so each sample meets a cold database#214wmadden-electric wants to merge 2 commits into
Conversation
…tabase The canary returned a bug-gone verdict on 2026-08-09 (run 31330072181, 14/14 successes) — the forcing signal to delete `withConnectionRetry` — while runs on main minutes either side still saw the rejection. Six live runs since have all rejected on their first sample, at 575-588ms. It sampled back to back, and counted every success as evidence. Across 20 recent runs (63 samples) the successes came back in a median of 122ms against 546ms for rejections, which matches the documented ~0.4-0.6s fast-reject: the quick ones were reaching an already-warm upstream and testing nothing. Raising MIN_BUG_GONE_SAMPLES would not have helped. Its arithmetic assumes independent samples at a fixed rate, and the data says otherwise — 8 of those 20 runs rejected on sample #0 while others ran 8, 10 and 14 clean, so the outcome is dominated by whole-run conditions. More samples from a run that is already sailing through are just more clean samples. The doc comment now says so. Samples are spaced 60s apart, matching the sibling cold-start canary, which adopted the same spacing for the same family of bug. The pause sits between samples only — never between provisioning a database and connecting to it, which would warm the thing under test. Cost is small in practice because the loop still stops at the first rejection, the normal outcome today; only a fully clean run pays all 13 intervals, and the run has its own 15 minute budget below the job timeout. The sampling loop moves into the classify module so the spacing, the early stop and the budget stop are unit-tested against a virtual clock instead of waited out. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughThe cold-connect canary now uses an asynchronous collector with 60-second sample spacing and a 15-minute runtime limit. The collector stops on rejection or timeout and continues successful sampling until the required threshold. Tests cover timing, termination, logging, and inconclusive results. The deployment workflow timeout is 20 minutes. Documentation describes warm-database false positives and confirmation of bug-gone verdicts. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/cold-connect-canary-classify.ts`:
- Around line 180-192: Update the sampling loop in
scripts/cold-connect-canary-classify.ts:180-192 to use a >= deadline check, skip
any interval wait that cannot fit within the remaining maxRunMs budget, and
recheck the deadline after sleep before invoking sample. Update the boundary
assertion in scripts/cold-connect-canary-classify.test.ts:158-165 so the
virtual-clock case collects four samples and three waits without starting a
fifth sample.
In `@scripts/cold-connect-canary.ts`:
- Around line 63-65: Validate the runtime controls before starting the canary:
require COLD_CONNECT_SAMPLE_INTERVAL_MS and COLD_CONNECT_MAX_RUN_MS to be finite
positive numbers, and COLD_CONNECT_SAMPLES to be a positive safe integer. Route
invalid values through the existing non-blocking canary error path and prevent
startup until configuration is valid, while preserving the current
environment-variable defaults.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7b4ec42e-d661-4299-ae08-b5d12da27741
📒 Files selected for processing (5)
.github/workflows/e2e-deploy.ymlgotchas.mdscripts/cold-connect-canary-classify.test.tsscripts/cold-connect-canary-classify.tsscripts/cold-connect-canary.ts
Addresses two review findings, both real. The sampling loop could begin a full interval with a second of budget left and then sample on the far side of it, overrunning by more than the interval itself. It now declines to start a wait the budget cannot cover, which also makes a recheck after the sleep redundant. The budget test asserts the new boundary — four samples and three waits against a 200s budget — rather than just "fewer than 14". A misspelt numeric override became NaN, and NaN quietly disabled the things it named: `setTimeout(NaN)` fires at once, so the spacing vanished, and every comparison against a NaN budget is false, so the run never stopped itself. The three knobs are now validated up front and refuse to start, routed through the same non-blocking exit-0-with-a-warning path as every other way this canary fails to reach a verdict. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Both findings were real and are fixed in 7ed2a42. Run budget. The loop checked the budget, then started a full interval, then sampled — so with a second of budget left it would still wait 60s and sample on the far side, overrunning by more than the interval itself. It now declines to start a wait the budget cannot cover. That also makes the suggested recheck after the sleep redundant: if Override validation. Worth flagging how badly this one bit: @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --no-ext-diff --format=fuller --stat 7ed2a42f
git diff --no-ext-diff --unified=80 7ed2a42f^ 7ed2a42f -- \
scripts/cold-connect-canary-classify.ts \
scripts/cold-connect-canary-classify.test.ts \
scripts/cold-connect-canary.ts \
scripts/cold-connect-canary.test.ts 2>/dev/null || true
for f in \
scripts/cold-connect-canary-classify.ts \
scripts/cold-connect-canary-classify.test.ts \
scripts/cold-connect-canary.ts \
scripts/cold-connect-canary.test.ts
do
if [ -f "$f" ]; then
echo "===== AST OUTLINE: $f ====="
ast-grep outline "$f" --items all || true
fi
done
echo "===== Numeric override and collector references ====="
rg -n -C 5 --type ts \
'collectColdConnectSamples|MIN_BUG_GONE_SAMPLES|intervalMs|maxRunMs|Number\(|isFinite|Number\.isFinite|setTimeout|sleep' \
scriptsLength of output: 59587 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "===== PR metadata ====="
gh pr view 214 --repo prisma/composer --json baseRefName,headRefName,commits,files --jq \
'{baseRefName, headRefName, commits: [.commits[].oid], files: [.files[].path]}'
base="$(git merge-base HEAD "origin/$(gh pr view 214 --repo prisma/composer --json baseRefName --jq .baseRefName)")"
echo "===== Merge base: $base ====="
git diff --no-ext-diff --stat "$base"...HEAD
git diff --no-ext-diff --unified=50 "$base"...HEAD -- \
scripts/cold-connect-canary.ts \
scripts/cold-connect-canary-classify.ts \
scripts/cold-connect-canary-classify.test.ts \
.github/workflows/e2e-deploy.yml \
gotchas.md
echo "===== runCanary and final exit paths ====="
sed -n '200,310p' scripts/cold-connect-canary.ts
echo "===== Changed-file references ====="
rg -n -C 3 --type ts \
'positiveNumber\s*\(|COLD_CONNECT_(SAMPLES|SAMPLE_INTERVAL_MS|MAX_RUN_MS)|collectColdConnectSamples\s*\(' \
scriptsLength of output: 59577
Commit
I did not execute tests because the review sandbox does not run repository code.
|
1 participant
On 2026-08-09 the cold-connect canary returned a
bug-goneverdict — 14 of 14 successes on run 31330072181 — and printed the full instruction to deletewithConnectionRetryand the canary itself. It was wrong. Runs onmainminutes either side still saw the rejection, and six live runs since have all rejected on their first sample at 575–588ms.Why it was wrong
The canary sampled back to back and counted every success as evidence. Across 20 recent runs (63 samples):
That latency split is the tell. A rejection takes ~546ms, matching the "direct endpoint fast-rejects ~0.4–0.6s" already documented in
gotchas.md. A 122ms success is a connect to something already warm — it never met a cold database, so it tested nothing, yet it counted towards "the bug is gone".Why raising the sample count was not the fix
My first instinct was to raise
MIN_BUG_GONE_SAMPLESfrom 14 to about 26. That was wrong, and it's worth writing down why so nobody tries it again.The threshold's justification — "at a conservative 20% rejection rate, 0.8^14 ≈ 4.4% chance of a lucky streak" — assumes every sample is an independent draw at a fixed rate. The data contradicts that. Eight of twenty runs rejected on their very first sample, while others ran 8, 10 and 14 samples clean. The outcome is dominated by conditions that hold for a whole run, so adding twelve more samples to a run that is already sailing through just collects twelve more clean samples. The doc comment on
MIN_BUG_GONE_SAMPLESnow records this rather than leaving the misleading arithmetic in place.The change
Samples are spaced 60 seconds apart, so each one has a chance of meeting a genuinely cold database. This is the sibling cold-start canary's approach:
SAMPLE_INTERVAL_MSthere is also 60s, adopted for the same family of bug, and pergotchas.mdthat canary goes further and refuses to count a sample it cannot confirm was cold.The pause sits between samples only. Putting one between provisioning a database and connecting to it would warm the very thing under test — there's a comment saying so, because it is an easy mistake to make later.
The cost is smaller than it looks. The loop still stops at the first rejection, which is the normal outcome today, so a typical run finishes in a couple of minutes. Only a fully clean run pays all 13 intervals. The script has its own 15-minute budget so it stops and still reports, with
timeout-minutesraised from 3 to 20 as headroom for that — the same arrangement the sibling job documents.60s is adopted from the sibling rather than derived from measurement here. The comment says so, and says it is the first number to re-derive if false verdicts persist.
Verification
The sampling loop moves into the classify module so it can be tested offline against a virtual clock, rather than waited out. Four new tests cover: no pause before the first sample, one pause between each pair after, stopping at the first rejection, continuing past
minSampleswhile everything succeeds, and stopping on the run budget. The budget test also asserts the resulting short run classifies asinconclusive, which is why stopping early can never manufacture the forcing signal.Reverting the
samples.length > 0guard fails two of them, so they catch the regression they exist for.Six live runs against the real API confirm the wiring end to end: correct verdict, exit 0, project torn down each time. All six rejected on sample #0, so the spacing path itself is covered by the unit tests rather than by a live run.
🤖 Generated with Claude Code