fix(ci): distinguish unreachable from stale in /buildinfo verify step - #2402
Conversation
The /buildinfo verify step (PR #2398) was treating "no /buildinfo response" the same as "tenant returned wrong SHA" — both bumped MISMATCH_COUNT and hard-failed the workflow. First post-merge run on staging caught a real edge case: ephemeral E2E tenants (slug e2e-20260430-...) get torn down by the E2E teardown trap between CP's healthz_ok snapshot and the verify step running, so the verify step would dial into DNS that no longer resolves and hard-fail on a benign condition. The bug class we actually care about is STALE (tenant up + serving old code, the #2395 root). UNREACHABLE post-redeploy is almost always a benign teardown race; real "tenant up but unreachable" is caught by CP's own healthz monitor + the alert pipeline, so double-counting it here was making this workflow flaky on every staging push that overlapped E2E. Wire: - Split MISMATCH_COUNT into STALE_COUNT + UNREACHABLE_COUNT. - STALE → hard-fail the workflow (the bug class we're guarding). - UNREACHABLE → ::warning::, don't fail. Reachable-mismatch still hard-fails. - Job summary surfaces both lists separately so on-call can tell at a glance which class fired. Mirror in redeploy-tenants-on-main.yml for shape parity (prod has fewer ephemeral tenants but identical asymmetry would be a gratuitous fork). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
left a comment
There was a problem hiding this comment.
Five-axis review. The fix targets a real flake I observed firsthand on c06e2fe (#2396 merge): the verify step hard-failed on e2e-20260430-25181492800-1 despite the underlying SSM redeploy working. The split into STALE/UNREACHABLE is the right move.
Correctness. The control flow is sound:
- STALE > 0 →
exit 1(the bug class we care about — #2395 root) - UNREACHABLE > 0 →
::warning::only - "all matched" notice fires only when both counts are 0
- Both staging.yml + main.yml mirror the same logic
I worked through the four state combinations (STALE × UNREACHABLE, both 0/non-0) and each produces the right exit code + summary content. No off-by-ones in the count math.
Readability. The staging.yml inline comment is exemplary — it names the bug class (#2395), names the noise source (e2e-* ephemeral churn at 5-10/hour), and explicitly justifies the soft-warn. The main.yml short-form comment that defers to staging is appropriate.
Architecture. Same logic duplicated across redeploy-tenants-on-{staging,main}.yml. The PR comment ("asymmetry would be a gratuitous fork") justifies the choice — both should behave identically. Optional: if either workflow grows another verify branch, lift this into a reusable workflow (.github/workflows/verify-tenant-buildinfo.yml) called from both — it's ~50 lines of bash that needs to evolve in lockstep, which is exactly the maintenance hazard a composite action solves.
Security. YAML script change, no secrets, no auth surface, no new external calls.
Performance. Same N curls per tenant. No change.
One real concern — flag for follow-up, not a blocker:
The PR body says "Real 'tenant up but unreachable' is caught by CP's own healthz monitor + the post-redeploy alert." I grepped the workflow tree for healthz-monitoring workflows and only found these two files — so the monitor presumably lives in molecule-controlplane. Optional: could you link the CP healthz monitor + alert in the PR body so future on-call has the load-bearing assumption captured? If that monitor is misconfigured / paged-down, this soft-warn loses real outage detection.
Optional / Consider: a sanity-floor would close the residual gap without re-introducing the e2e-* flake. Something like:
TOTAL=${#SLUGS[@]}
if [ $UNREACHABLE_COUNT -gt $((TOTAL / 2)) ]; then
echo "::error::More than 50% of tenants unreachable post-redeploy — likely real outage, not teardown race."
exit 1
fiThis catches the "all tenants crash on the new image" case while still soft-warning the typical 1-of-N e2e churn. Not a blocker — the canary-verify step would also catch a mass failure on the canary tenant first. Just belt-and-suspenders.
Verdict: approve. Real fix for a real flake, mirrored correctly across both workflows, well-commented. CI green (including Shellcheck which is the relevant gate for this kind of YAML/bash change).
Self-review of #2403 caught a regression: with a 1-tenant fleet (the exact case the original #2402 fix targeted), the new floor would re-introduce the flake. Trace: TOTAL=1, UNREACHABLE=1, $((1/2))=0 if 1 -gt 0 → TRUE → exit 1 The 50%-rule only meaningfully distinguishes "real outage" from "teardown race" when the fleet is large enough that "half down" is statistically meaningful. With 1-3 tenants, canary-verify is the actual gate (it runs against the canary first and aborts the rollout if the canary fails to come up). Gate the floor on TOTAL_VERIFIED >= 4. Truth table: TOTAL UNREACHABLE RESULT 1 1 soft-warn (original e2e flake case) 4 2 soft-warn (exactly half) 4 3 hard-fail (75% — real outage) 10 6 hard-fail (60% — real outage) Mirrored across staging.yml + main.yml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Follow-up to #2398. The first post-merge run on staging surfaced a real edge case: ephemeral E2E tenants get torn down between CP's
healthz_oksnapshot and the verify step running, so the step dials into DNS that no longer resolves and hard-fails the workflow on a benign teardown race.The bug class we actually care about is STALE (tenant up + serving old code — the #2395 root). UNREACHABLE post-redeploy is almost always benign churn from
e2e-*ephemeral tenants. Real "tenant up but unreachable" is caught by CP's own healthz monitor + alert pipeline, so double-counting it here was making this workflow flaky on every staging push that overlapped an E2E run.How
MISMATCH_COUNTintoSTALE_COUNT+UNREACHABLE_COUNT.STALE→::error::+exit 1(the bug class we're guarding).UNREACHABLE→::warning::, don't fail.Evidence
Run 25181884522 — verify step caught
e2e-20260430-25181492800-1as unreachable and hard-failed, even though that tenant was already torn down by the E2E suite. Expected behavior post-fix: warn + pass.Test plan
🤖 Generated with Claude Code