fix(ci): canary teardown safety-net slug pattern (was reversed) - #2125
Merged
Conversation
[Molecule-Platform-Evolvement-Manager]
## What was broken
`canary-staging.yml`'s teardown safety-net step filtered candidate
slugs with `f'e2e-{today}-canary-'`. But `test_staging_full_saas.sh`
emits canary slugs as `e2e-canary-${date}-${RUN_ID_SUFFIX}` — date
SECOND, mode FIRST. Full-mode slugs are the other way around
(`e2e-${date}-${RUN_ID_SUFFIX}`), and the canary workflow seems to
have been copy-pasted from there without re-checking the slug
generator.
Net effect: the safety-net step ran on every cancelled / failed
canary, hit the CP, got the org list, filtered to zero matches,
and exited cleanly. Every cancelled canary EC2 leaked until the
once-an-hour `sweep-stale-e2e-orgs.yml` cron eventually caught it
(120-min default age threshold means ≥1h leak in the worst case).
## Today's incident
Canary run 24966995140 cancelled at 21:03Z. EC2
`tenant-e2e-canary-20260426-canary-24966` still running 1h25m
later, manually terminated by the CEO. Three earlier cancellations
today (16:04Z, 19:26Z, 20:02Z) hit the same gap — visible as the
hourly canary failure pattern in #2090.
## Fix
- Filter prefix corrected to `e2e-canary-${today}-` (mode FIRST,
date SECOND) to match the actual slug emitter.
- Added per-run scoping (`-canary-${GITHUB_RUN_ID}-` suffix) when
GITHUB_RUN_ID is set, mirroring the e2e-staging-saas.yml safety
net's per-run scoping that was added after the 2026-04-21
cross-run cleanup incident — guards against a queued canary's
safety-net step deleting an in-flight different canary's slug
while the queue's `cancel-in-progress: false` lets two reach the
teardown step concurrently.
- Added a comment block tracing the bug + the prior incident so
the next maintainer doesn't re-introduce the same mistake.
## Test plan
- [x] Manual trace: today's slug `e2e-canary-20260426-canary-24966...`
now matches `e2e-canary-20260426-canary-24966` prefix
- [x] YAML parses
- [ ] Next canary cancellation cleans up automatically
## Companion PR
The PRIMARY symptom (TLS-timeout failures, not the leaked EC2)
traces to a separate bug in `molecule-controlplane`: tunnel/DNS
creation errors are logged-and-continued rather than failing
provision. PR coming separately.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
HongmingWang-Rabbit
requested a review
from hongmingwang-moleculeai
as a code owner
April 26, 2026 21:44
HongmingWang-Rabbit
enabled auto-merge
April 26, 2026 21:44
Contributor
Author
Self-review (5-axis pass)Correctness: ✓ Verified by manual trace — today's slug Tests: Inline trace + commit message documents the slug-emitter contract — there's no unit test surface for a workflow YAML's embedded Python filter, but the inline comment block covers the regression case so the next maintainer can verify by inspection. Architecture: ✓ Mirrors the per-run-scoping pattern from Security: N/A — admin-token usage unchanged; no expansion of what the script can delete. Performance: N/A — same single-curl + python filter; trivially fast. FYI
Approval pending external reviewer (can't self-approve as PR author). Auto-merge already armed via earlier |
3 tasks
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
…+ panic recovery)
CTO-mandated regression test for the core#2125 family
("fix(workspace-server): http client timeouts, panic recovery, and
error checks"). The fix landed in main with NO test, despite
introducing 4 production-affecting behavior changes:
(1) http.DefaultClient → &http.Client{Timeout: 10*time.Second} in
cmd/server/cp_config.go:refreshEnvFromCP
(2) inline defer-recover blocks in 6 long-lived background
goroutines (cache sweepers, rate-limit cleanup, session cache,
A2A SSE idle watcher, terminal bridges, importer provision
goroutine)
(3) ctx propagation in discovery.go's queryPeerMaps (use
QueryRowContext)
(4) deferred tx.Rollback in workspace.go's Create handler
The CTO's spec asks for tests of the timeout and panic-recovery
facets. This PR lands:
NEW TestRefreshEnvFromCP_ClientTimeoutFiresOnSlowUpstream — proves
the 10s client.Timeout actually fires on a slow upstream. Spins up
an httptest server that delays 12s (longer than the 10s timeout);
asserts refreshEnvFromCP returns an error within 11s AND the error
mentions timeout/deadline. Without the timeout, the test would
block for 12s+ AND the function would return success — the
elapsed-time bound proves the timeout fired, not the server.
Skipped in -short mode (10s wall cost, acceptable for CI).
NEW TestRecoverPanic_RecoversFromPanicInGoroutine + the
NoopOnNormalReturn companion — proves the canonical #2125
panic-recovery pattern. The inline blocks (duplicated across 6 call sites) were
refactored into a single helper in
session_auth.go. The tests assert:
- a goroutine that defers recoverPanic and then panics does NOT
crash the test process
- the recovered value is logged with the caller's prefix (so
operators can grep for the specific goroutine)
- the goroutine's deferred epilogue (e.g. close(done)) still
runs after the panic
- recoverPanic is a no-op on a normal return (no spurious
'PANIC' log)
Refactor note: the other 5 goroutines that #2125 wrapped
(terminal.go × 3, a2a_proxy.go × 1, bundle/importer.go × 1,
ratelimit.go × 1, mcp_ratelimit.go × 1) still use the inline
pattern. The refactor to recoverPanic could be applied to those
in a follow-up — kept this PR focused on the test mandate. The
new recoverPanic helper is in session_auth.go (the package that
already had the most inline recover blocks) and the test in
panic_recovery_test.go covers the contract for ALL future call
sites.
Error-path coverage (CTO spec (c)) is already provided by the
existing TestRefreshEnvFromCP_NonOKPropagates (CP 500 propagates
as error) and TestRefreshEnvFromCP_RejectsOversizedValue
(oversized value rejected, no env poisoning) — no new test
needed.
Test results:
ok workspace-server/cmd/server 10.016s
ok workspace-server/internal/middleware 0.100s
go vet clean.
Refs: molecule-core#2615, core#2125
Co-Authored-By: Claude <noreply@anthropic.com>
HongmingWang-Rabbit
pushed a commit
that referenced
this pull request
Jun 12, 2026
…TP panic-recovery test + refactor 2 more sites to recoverPanic) CR2 review on PR #2625 (REQUEST_CHANGES, 5-axis) flagged two gaps in the panic-recovery coverage that this commit fixes: (1) The helper was only wired into session_auth.go's cache sweeper. The other 2 same-package #2125 call sites (ratelimit.go bucket cleanup, mcp_ratelimit.go bucket cleanup) still used the inline `defer func() { if r := recover(); ... }()` pattern. Refactored both to use the testable `recoverPanic(prefix)` helper. With this change, the helper has 3 in-production call sites in the middleware package (session_auth sweeper, ratelimit cleanup, mcp_ratelimit cleanup) — the contract test in panic_recovery_test.go now covers real production paths, not just one call site. (2) The CTO spec for the original regression asked for "a regression that exercises the real HTTP panic-recovery path (minimal Gin/router stack with the production recovery middleware, a handler that panics, and assertions for 500/ status shape and no process crash)". The previous test exercised a synthetic copy of the goroutine pattern. Added TestHTTPPanicRecovery_HandlerPanicReturns500 that: - wires gin.Recovery() middleware (the standard project-wide recovery that would be the right shape for a follow-up) - registers a /panic handler that panics - asserts the response is 500 (not connection-reset / crash) - asserts the engine survives (a /after-panic follow-up request still returns 200 with the expected body) - wraps the test in an outer recover() that fails the test if a panic escapes gin.Recovery() at the process level Honest gap documented in the test doc: core#2125 only added per-goroutine recover() wrappers — it did NOT introduce a project-wide HTTP-panic recovery middleware. So this test documents the gap and the right shape for a follow-up. The assertions serve as the regression gate: if a future change accidentally re-introduces a process-crash on a handler panic, the 500/200/process-alive assertions will fail. Build clean, vet clean, all tests pass: ok workspace-server/internal/middleware 0.099s ok workspace-server/cmd/server 0.019s go vet clean. The 3 cross-package #2125 sites (terminal.go × 3, a2a_proxy.go × 1, bundle/importer.go × 1) remain inline — the cycle risk between internal/middleware and internal/handlers/internal/bundle (and the cross-package import churn) makes a same-package helper the right scope. Documented in the test doc + this commit body. Refs: molecule-core#2615, core#2125, agent-reviewer-cr2 REQUEST_CHANGES on PR #2620 Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[Molecule-Platform-Evolvement-Manager]
What was broken
`canary-staging.yml`'s teardown safety-net step filtered with `f'e2e-{today}-canary-'`. But `test_staging_full_saas.sh` emits canary slugs as `e2e-canary-${date}-${RUN_ID_SUFFIX}` — date SECOND, mode FIRST. Full-mode slugs are the other way around (`e2e-${date}-${RUN_ID_SUFFIX}`); the canary workflow seems to have been copy-pasted from there without re-checking the slug generator.
Net effect: the safety-net step ran on every cancelled / failed canary, hit the CP, got the org list, filtered to zero matches, and exited cleanly. Every cancelled canary EC2 leaked until the once-an-hour `sweep-stale-e2e-orgs.yml` cron eventually caught it (120-min default age threshold = ≥1h leak in the worst case).
Today's incident
Canary run 24966995140 cancelled at 21:03Z. EC2 `tenant-e2e-canary-20260426-canary-24966` still running 1h25m later, terminated manually. Three earlier cancellations today (16:04Z, 19:26Z, 20:02Z) hit the same gap — visible as the canary failure cluster in #2090.
Fix
Test plan
Companion PR
The PRIMARY symptom (TLS-timeout failures, not the leaked EC2) traces to a separate bug in `molecule-controlplane`: tunnel/DNS creation errors are logged-and-continued rather than failing provision. CP PR coming separately.
🤖 Generated with Claude Code