Skip to content

Commit 917fbb0

Browse files
fix(deploy): reap stuck 'building' deploys with no provider_id (sweep #5) (#86)
A deployments row stuck at status='building' with an EMPTY provider_id is never reaped: the deploy_status_reconcile sweep skips empty-provider_id rows unconditionally (correct for a build in flight), so a deploy whose api goroutine DIED before UpdateDeploymentProviderID (crash mid-runDeploy) keeps the row 'building' forever and permanently consumes the team's deployments_apps tier cap. Fix: in the empty-provider_id branch, if status='building' AND the row is older than a 15m grace window, reap it to 'failed' via a guarded UPDATE (double-guarded on status='building' AND empty provider_id, so a row that raced and acquired a provider_id is a no-op). This frees the tier cap; the idempotent deploy_failure_autopsy job then emits the deploy.failed audit -> failure email. Fresh (<15m) builds are still left alone — 15m is well beyond the normal ~30-90s build. - add createdAt to activeDeployment + created_at to listActiveDeployments SELECT - add stuckBuildingGrace (15m) + stuckBuildingReapMessage consts - add guarded reapStuckBuilding helper (does NOT reuse updateStatus, which doesn't gate on provider_id) - log jobs.deploy_status_reconcile.stuck_building_reaped + reaped counter Coverage block: Symptom: building deploy w/ empty provider_id never reaped; tier cap leak Enumeration: rg 'if d.providerID == ""' / grep listActiveDeployments SELECT callers Sites found: 1 (single sweep loop + its SELECT/scan) Sites touched: 1 Coverage test: TestDeployStatusReconciler_Work_StuckBuildingReaped (+ Fresh/ ReapFailed/StuckDeploying/RowWithProviderIDUnaffected) Live verified: awaiting post-merge auto-deploy + worker /healthz SHA gate (rule 14) 100% patch coverage (Work/listActiveDeployments/reapStuckBuilding all 100.0%); jobs package 97.2% (>=95%). make gate green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 1b71277 commit 917fbb0

3 files changed

Lines changed: 263 additions & 36 deletions

File tree

internal/jobs/deploy_lifecycle_coverage_test.go

Lines changed: 178 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -237,10 +237,10 @@ func TestDeployNamespaceFromProviderID(t *testing.T) {
237237
}{
238238
{"app-abc", "instant-deploy-abc"},
239239
{"app-1234", "instant-deploy-1234"},
240-
{"app-", ""}, // empty appID after prefix
241-
{"instant-stack-xyz", ""}, // foreign prefix
242-
{"", ""}, // nothing
243-
{"appabc", ""}, // missing hyphen
240+
{"app-", ""}, // empty appID after prefix
241+
{"instant-stack-xyz", ""}, // foreign prefix
242+
{"", ""}, // nothing
243+
{"appabc", ""}, // missing hyphen
244244
}
245245
for _, tc := range cases {
246246
if got := deployNamespaceFromProviderID(tc.in); got != tc.want {
@@ -297,7 +297,7 @@ func TestDeployStatusReconciler_Work_NoActiveRows(t *testing.T) {
297297
defer db.Close()
298298

299299
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
300-
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status"}))
300+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}))
301301

302302
w := NewDeployStatusReconciler(db, newFakeDeployStatusK8s())
303303
if err := w.Work(context.Background(), fakeRiverJob[DeployStatusReconcileArgs]()); err != nil {
@@ -347,14 +347,18 @@ func TestDeployStatusReconciler_Work_FullSweep(t *testing.T) {
347347
idHealthy := uuid.New()
348348
idFailed := uuid.New()
349349

350+
// idBlank gets a FRESH created_at so the empty-provider_id row stays
351+
// "skipped" (a build still in flight), not reaped by the stuck-building
352+
// path — that path is exercised in its own dedicated tests.
353+
now := time.Now().UTC()
350354
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
351-
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status"}).
352-
AddRow(idBlank, "", "building").
353-
AddRow(idForeign, "instant-stack-zzz", "building").
354-
AddRow(idStopped, "app-stopped", "building").
355-
AddRow(idSame, "app-same", "building").
356-
AddRow(idHealthy, "app-healthy", "building").
357-
AddRow(idFailed, "app-failed", "deploying"))
355+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}).
356+
AddRow(idBlank, "", "building", now).
357+
AddRow(idForeign, "instant-stack-zzz", "building", now).
358+
AddRow(idStopped, "app-stopped", "building", now).
359+
AddRow(idSame, "app-same", "building", now).
360+
AddRow(idHealthy, "app-healthy", "building", now).
361+
AddRow(idFailed, "app-failed", "deploying", now))
358362

359363
k8s := newFakeDeployStatusK8s()
360364
// app-same: deployment with all-zero status → building
@@ -413,12 +417,12 @@ func TestDeployStatusReconciler_Work_AutopsyCapDeferred(t *testing.T) {
413417

414418
const n = maxAutopsiesPerTick + 1 // one beyond the cap → at least 1 deferred
415419
k8s := newFakeDeployStatusK8s()
416-
rows := sqlmock.NewRows([]string{"id", "provider_id", "status"})
420+
rows := sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"})
417421
ids := make([]uuid.UUID, n)
418422
for i := 0; i < n; i++ {
419423
ids[i] = uuid.New()
420424
appID := "appfail" + uuid.NewString()[:8]
421-
rows.AddRow(ids[i], "app-"+appID, "deploying")
425+
rows.AddRow(ids[i], "app-"+appID, "deploying", time.Now().UTC())
422426
k8s.objs["instant-deploy-"+appID+"|app-"+appID] = &appsv1.Deployment{
423427
Status: appsv1.DeploymentStatus{
424428
Conditions: []appsv1.DeploymentCondition{{
@@ -481,8 +485,8 @@ func TestDeployStatusReconciler_Work_K8sGetFailed(t *testing.T) {
481485

482486
idA := uuid.New()
483487
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
484-
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status"}).
485-
AddRow(idA, "app-broken", "building"))
488+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}).
489+
AddRow(idA, "app-broken", "building", time.Now().UTC()))
486490

487491
k8s := newFakeDeployStatusK8s()
488492
k8s.errOn["instant-deploy-broken|app-broken"] = errors.New("network blip")
@@ -507,8 +511,8 @@ func TestDeployStatusReconciler_Work_UpdateFailed(t *testing.T) {
507511

508512
id := uuid.New()
509513
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
510-
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status"}).
511-
AddRow(id, "app-borked", "building"))
514+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}).
515+
AddRow(id, "app-borked", "building", time.Now().UTC()))
512516

513517
k8s := newFakeDeployStatusK8s()
514518
k8s.objs["instant-deploy-borked|app-borked"] = &appsv1.Deployment{
@@ -539,8 +543,8 @@ func TestDeployStatusReconciler_listActiveDeployments_ScanError(t *testing.T) {
539543

540544
// Return a row whose id is not a UUID → Scan fails.
541545
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
542-
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status"}).
543-
AddRow("not-a-uuid", "app-x", "building"))
546+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}).
547+
AddRow("not-a-uuid", "app-x", "building", time.Now().UTC()))
544548

545549
w := NewDeployStatusReconciler(db, newFakeDeployStatusK8s())
546550
if werr := w.Work(context.Background(), fakeRiverJob[DeployStatusReconcileArgs]()); werr == nil {
@@ -559,6 +563,154 @@ func TestComputeNewStatus_ForeignProviderID(t *testing.T) {
559563
}
560564
}
561565

566+
// ─── stuck-building reaper (sweep finding #5) ─────────────────────────────────
567+
568+
// TestDeployStatusReconciler_Work_StuckBuildingReaped pins the core finding-#5
569+
// fix: a deployments row stuck at status="building" with an EMPTY provider_id
570+
// for longer than stuckBuildingGrace (api goroutine died before the build was
571+
// created) is reaped to "failed" via the guarded UPDATE — freeing the team's
572+
// deployments_apps tier cap. No k8s Get happens (nothing to poll without a
573+
// provider_id); the only DB write is the guarded reap UPDATE.
574+
func TestDeployStatusReconciler_Work_StuckBuildingReaped(t *testing.T) {
575+
db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherRegexp))
576+
if err != nil {
577+
t.Fatalf("sqlmock.New: %v", err)
578+
}
579+
defer db.Close()
580+
581+
id := uuid.New()
582+
stale := time.Now().UTC().Add(-stuckBuildingGrace - time.Minute) // > grace
583+
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
584+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}).
585+
AddRow(id, "", "building", stale))
586+
587+
// The guarded reap UPDATE: status→failed, double-guarded on the prior
588+
// status='building' AND empty provider_id.
589+
mock.ExpectExec(`UPDATE deployments\s+SET status = \$1`).
590+
WithArgs(deployStatusFailed, stuckBuildingReapMessage, id, deployStatusBuilding).
591+
WillReturnResult(sqlmock.NewResult(0, 1))
592+
593+
// k8s provider must NOT be consulted for an empty-provider_id row.
594+
k8s := newFakeDeployStatusK8s()
595+
w := NewDeployStatusReconciler(db, k8s)
596+
if werr := w.Work(context.Background(), fakeRiverJob[DeployStatusReconcileArgs]()); werr != nil {
597+
t.Fatalf("Work: %v", werr)
598+
}
599+
if len(k8s.callLog) != 0 {
600+
t.Errorf("empty-provider_id reap must not call GetDeployment, got calls: %v", k8s.callLog)
601+
}
602+
if err := mock.ExpectationsWereMet(); err != nil {
603+
t.Errorf("unmet expectations: %v", err)
604+
}
605+
}
606+
607+
// TestDeployStatusReconciler_Work_StuckBuildingFreshNotReaped pins the
608+
// safety boundary: a FRESH (< stuckBuildingGrace) "building" row with an empty
609+
// provider_id is a build still in flight — it MUST be left alone (skipped), no
610+
// UPDATE, so a legitimately-in-progress build is never killed early.
611+
func TestDeployStatusReconciler_Work_StuckBuildingFreshNotReaped(t *testing.T) {
612+
db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherRegexp))
613+
if err != nil {
614+
t.Fatalf("sqlmock.New: %v", err)
615+
}
616+
defer db.Close()
617+
618+
id := uuid.New()
619+
fresh := time.Now().UTC().Add(-30 * time.Second) // well inside grace
620+
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
621+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}).
622+
AddRow(id, "", "building", fresh))
623+
// No ExpectExec — any UPDATE is an unmet/unexpected expectation failure.
624+
625+
w := NewDeployStatusReconciler(db, newFakeDeployStatusK8s())
626+
if werr := w.Work(context.Background(), fakeRiverJob[DeployStatusReconcileArgs]()); werr != nil {
627+
t.Fatalf("Work: %v", werr)
628+
}
629+
if err := mock.ExpectationsWereMet(); err != nil {
630+
t.Errorf("a fresh empty-provider_id build must not be reaped: %v", err)
631+
}
632+
}
633+
634+
// TestDeployStatusReconciler_Work_StuckBuildingReapFailed covers the reap
635+
// UPDATE error branch: the failure is logged and counted but the sweep does
636+
// not abort (fail-open) — Work returns nil.
637+
func TestDeployStatusReconciler_Work_StuckBuildingReapFailed(t *testing.T) {
638+
db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherRegexp))
639+
if err != nil {
640+
t.Fatalf("sqlmock.New: %v", err)
641+
}
642+
defer db.Close()
643+
644+
id := uuid.New()
645+
stale := time.Now().UTC().Add(-stuckBuildingGrace - time.Minute)
646+
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
647+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}).
648+
AddRow(id, "", "building", stale))
649+
mock.ExpectExec(`UPDATE deployments\s+SET status = \$1`).
650+
WithArgs(deployStatusFailed, stuckBuildingReapMessage, id, deployStatusBuilding).
651+
WillReturnError(errors.New("deadlock"))
652+
653+
w := NewDeployStatusReconciler(db, newFakeDeployStatusK8s())
654+
if werr := w.Work(context.Background(), fakeRiverJob[DeployStatusReconcileArgs]()); werr != nil {
655+
t.Fatalf("Work must isolate a reap-UPDATE failure (fail-open), got: %v", werr)
656+
}
657+
if err := mock.ExpectationsWereMet(); err != nil {
658+
t.Errorf("unmet expectations: %v", err)
659+
}
660+
}
661+
662+
// TestDeployStatusReconciler_Work_StuckDeployingEmptyProviderNotReaped guards
663+
// that the reaper is scoped to status="building" ONLY: an empty-provider_id
664+
// row in "deploying" (an unusual but possible interim state) is NOT reaped even
665+
// when old — the guard is `d.status == deployStatusBuilding`.
666+
func TestDeployStatusReconciler_Work_StuckDeployingEmptyProviderNotReaped(t *testing.T) {
667+
db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherRegexp))
668+
if err != nil {
669+
t.Fatalf("sqlmock.New: %v", err)
670+
}
671+
defer db.Close()
672+
673+
id := uuid.New()
674+
stale := time.Now().UTC().Add(-stuckBuildingGrace - time.Hour)
675+
mock.ExpectQuery(`FROM deployments\s+WHERE status IN`).
676+
WillReturnRows(sqlmock.NewRows([]string{"id", "provider_id", "status", "created_at"}).
677+
AddRow(id, "", "deploying", stale))
678+
// No ExpectExec — a non-building status must be skipped, never reaped.
679+
680+
w := NewDeployStatusReconciler(db, newFakeDeployStatusK8s())
681+
if werr := w.Work(context.Background(), fakeRiverJob[DeployStatusReconcileArgs]()); werr != nil {
682+
t.Fatalf("Work: %v", werr)
683+
}
684+
if err := mock.ExpectationsWereMet(); err != nil {
685+
t.Errorf("a non-building empty-provider_id row must not be reaped: %v", err)
686+
}
687+
}
688+
689+
// TestReapStuckBuilding_RowWithProviderIDUnaffected exercises the guarded
690+
// helper directly: the UPDATE is double-guarded on empty provider_id, so a row
691+
// that raced and acquired a provider_id between SELECT and UPDATE matches zero
692+
// rows (RowsAffected 0) and the helper returns nil — a safe no-op.
693+
func TestReapStuckBuilding_RowWithProviderIDUnaffected(t *testing.T) {
694+
db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherRegexp))
695+
if err != nil {
696+
t.Fatalf("sqlmock.New: %v", err)
697+
}
698+
defer db.Close()
699+
700+
id := uuid.New()
701+
mock.ExpectExec(`UPDATE deployments\s+SET status = \$1`).
702+
WithArgs(deployStatusFailed, stuckBuildingReapMessage, id, deployStatusBuilding).
703+
WillReturnResult(sqlmock.NewResult(0, 0)) // guard matched nothing
704+
705+
w := NewDeployStatusReconciler(db, newFakeDeployStatusK8s())
706+
if err := w.reapStuckBuilding(context.Background(), id); err != nil {
707+
t.Fatalf("reapStuckBuilding no-op must not error: %v", err)
708+
}
709+
if err := mock.ExpectationsWereMet(); err != nil {
710+
t.Errorf("unmet expectations: %v", err)
711+
}
712+
}
713+
562714
// ─── deploy_failure_autopsy.go ────────────────────────────────────────────────
563715

564716
// TestExtractPodFailure_TerminatedCurrentState exercises the
@@ -1558,7 +1710,7 @@ func TestDeploymentReminderWorker_FullSweep(t *testing.T) {
15581710
// row B: CAS wins, past expiry → floor branch + audit emit
15591711
rows := sqlmock.NewRows(reminderCandidateCols).
15601712
AddRow("deploy-A", teamID, "appA", "https://a.deployment.instanode.dev",
1561-
pastExpiry, 0, "auto_24h", "owner@example.com").
1713+
pastExpiry, 0, "auto_24h", "owner@example.com").
15621714
AddRow("deploy-B", teamID, "appB", "", // empty app_url → deployURL fallback
15631715
pastExpiry, 1, "auto_24h", nil) // nil email → audited_no_owner branch
15641716
mock.ExpectQuery(`SELECT d.id::text, d.team_id::text, d.app_id, d.app_url`).
@@ -2289,11 +2441,11 @@ func TestNewRazorpayOrphanCanceler_UnconfiguredReturnsNil(t *testing.T) {
22892441

22902442
// TestRazorpayOrphanCanceler_CancelSubscription_Fake exercises the
22912443
// production CancelSubscription wrapper logic through the test seam:
2292-
// - empty subID is a no-op
2293-
// - whitespace subID is a no-op
2294-
// - non-empty subID hits the SDK
2295-
// - SDK error fragment is mapped to "terminal" success
2296-
// - SDK error not in the terminal set is propagated
2444+
// - empty subID is a no-op
2445+
// - whitespace subID is a no-op
2446+
// - non-empty subID hits the SDK
2447+
// - SDK error fragment is mapped to "terminal" success
2448+
// - SDK error not in the terminal set is propagated
22972449
func TestRazorpayOrphanCanceler_CancelSubscription_Fake(t *testing.T) {
22982450
// Empty + whitespace.
22992451
sdk := &fakeCancelSDKCov{}

0 commit comments

Comments
 (0)