diff --git a/internal/jobs/customer_restore_runner.go b/internal/jobs/customer_restore_runner.go index e12606e..031c84c 100644 --- a/internal/jobs/customer_restore_runner.go +++ b/internal/jobs/customer_restore_runner.go @@ -419,8 +419,16 @@ func (w *CustomerRestoreRunnerWorker) processRestore(parentCtx context.Context, return false } - // Finalize. - if _, updErr := w.db.ExecContext(parentCtx, ` + // Finalize on a FRESH bounded context, not parentCtx — mirrors the + // customer_backup_runner fix (P2-W4, BugBash 2026-05-18). pg_restore has + // already succeeded above, so the data is durably in place; if parentCtx + // were cancelled by a worker shutdown (rolling deploy / node drain), the + // UPDATE would fail, markRestoreFailed would run, and a SUCCESSFUL restore + // would be recorded as 'failed'. A detached context lets the row reach + // 'ok' even mid-shutdown. + finalizeCtx, finalizeCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer finalizeCancel() + if _, updErr := w.db.ExecContext(finalizeCtx, ` UPDATE resource_restores SET status = 'ok', finished_at = now() WHERE id = $1 diff --git a/internal/jobs/deploy_success_email_test.go b/internal/jobs/deploy_success_email_test.go index 6c1e2ba..d26ad93 100644 --- a/internal/jobs/deploy_success_email_test.go +++ b/internal/jobs/deploy_success_email_test.go @@ -111,17 +111,23 @@ func TestEventEmail_BuildDeploySuccess_NoEmailReturnsFalse(t *testing.T) { func TestLifecycle_RenderDeployHealthy_SurfacesURL(t *testing.T) { const url = "https://my-app.deployment.instanode.dev" subject, html, text := renderDeployHealthy(map[string]string{ - "app_name": "my-app", + "app_name": "6fffcc21", "env": "production", "app_url": url, "time_to_healthy_seconds": "31", }) - if !strings.Contains(subject, "my-app") { - t.Errorf("subject %q should name the app", subject) + // app_name is an opaque hex slug, so it must NOT appear in the subject as + // a prose name (bug #23) — the subject is generic and the URL identifies + // the app. The slug appears in the body only as a labeled identifier. + if strings.Contains(subject, "6fffcc21") { + t.Errorf("subject %q must not render the opaque app_id slug as a name", subject) } if !strings.Contains(html, url) { t.Errorf("html body should contain the live URL %q", url) } + if !strings.Contains(html, "6fffcc21") { + t.Errorf("html body should show the app_id as an identifier") + } if !strings.Contains(text, url) { t.Errorf("text body should contain the live URL %q", url) } @@ -145,11 +151,15 @@ func TestLifecycle_RenderDeployHealthy_NoURLFallsBackToDashboard(t *testing.T) { // and links to the dashboard. func TestLifecycle_RenderDeployCreated_NoURLPromise(t *testing.T) { subject, html, _ := renderDeployCreated(map[string]string{ - "app_name": "my-app", + "app_name": "6fffcc21", "env": "production", }) - if !strings.Contains(subject, "my-app") { - t.Errorf("subject %q should name the app", subject) + // Opaque slug must not appear as a prose name in the subject (bug #23). + if strings.Contains(subject, "6fffcc21") { + t.Errorf("subject %q must not render the opaque app_id slug as a name", subject) + } + if !strings.Contains(html, "6fffcc21") { + t.Errorf("started email should still show the app_id as an identifier in the body") } if !strings.Contains(html, dashboardURL) { t.Errorf("started email should link to the dashboard") diff --git a/internal/jobs/event_email_forwarder.go b/internal/jobs/event_email_forwarder.go index c689725..653a5ad 100644 --- a/internal/jobs/event_email_forwarder.go +++ b/internal/jobs/event_email_forwarder.go @@ -658,10 +658,18 @@ batchLoop: "audit_id", row.ID, "kind", row.Kind, "error", supErr, - "note", "fail-closed: skipping send, cursor NOT advanced — retries when DB recovers", + "note", "fail-closed: halting batch, cursor NOT advanced — this row + remainder retried when DB recovers", ) - skipped++ - continue + // MUST halt the whole batch, not `continue`. The cursor is + // advanced per-row inline (and redisEventCursorStore.write is an + // unconditional Set, not a max), so a `continue` would let a LATER + // sendable row in this same batch advance the watermark past this + // held row — stranding it behind the cursor forever and silently + // dropping a legitimate transactional email. `break batchLoop` + // leaves the cursor at the last successfully-advanced row so this + // row is re-fetched next tick — mirrors the SendClassTransient halt. + transient++ + break batchLoop } if supErr != nil { // Bounce/spam lookup failure — fail-OPEN: treat as "not diff --git a/internal/jobs/event_email_forwarder_test.go b/internal/jobs/event_email_forwarder_test.go index a8b7fc3..783a2ea 100644 --- a/internal/jobs/event_email_forwarder_test.go +++ b/internal/jobs/event_email_forwarder_test.go @@ -704,6 +704,53 @@ func TestEventForwarder_UnsubscribeCheckError_FailsClosed(t *testing.T) { } } +// TestEventForwarder_UnsubscribeFailClosed_HaltsBatch is the multi-row +// regression for bug #23 (bug bash 2026-06-02): a fail-CLOSED row followed by +// a sendable row in the SAME batch must NOT let the sendable row advance the +// cursor past the held row. The pre-fix code used `continue` (not `break +// batchLoop`); because the cursor is advanced per-row inline via an +// unconditional Set, the second row's send moved the watermark past row 1, +// stranding it forever — silently dropping a legitimate transactional email +// during a transient unsubscribe-lookup DB blip. The single-row test above +// can't catch this (continue and break are indistinguishable with one row). +func TestEventForwarder_UnsubscribeFailClosed_HaltsBatch(t *testing.T) { + db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherRegexp)) + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer db.Close() + + held := time.Date(2026, 5, 13, 19, 0, 0, 0, time.UTC) + sendable := held.Add(time.Minute) // sorts AFTER the held row (ASC order) + mock.ExpectQuery(`SELECT[\s\S]+FROM audit_log`). + WillReturnRows(sqlmock.NewRows(auditRowsCols). + AddRow("held-row", "team-h", auditKindOnboardingClaimed, "", "x", []byte(`{}`), held, "blip@example.com"). + AddRow("sendable-row", "team-s", auditKindOnboardingClaimed, "", "y", []byte(`{}`), sendable, "ok@example.com")) + + provider := &fakeProvider{sendFn: func(_ context.Context, _ email.EventEmail) error { return nil }} + cursor := &memCursor{} + w := newEventEmailForwarderWorkerForTest(db, cursor, provider) + // failNext fails ONLY the first hasSuppression call — i.e. the held row. + w.suppression = &memSuppression{ + suppressedEmails: map[string]bool{}, + failNext: fmt.Errorf("simulated unsubscribe DB blip: %w", errUnsubscribeLookupFailed), + } + + if err := w.Work(context.Background(), fakeJobLocal[EventEmailForwarderArgs]()); err != nil { + t.Fatalf("Work: %v", err) + } + + // The batch must HALT on the held row: the sendable row is never reached, + // so the provider is not called and the cursor never advances past (or to) + // either row. Next tick re-fetches from the un-advanced cursor. + if got := provider.callCount(); got != 0 { + t.Errorf("expected 0 SendEvent calls (batch halts on fail-closed row), got %d — a later row advanced past the held row", got) + } + if cursor.c.ID != "" { + t.Errorf("cursor.ID = %q; want \"\" — fail-closed must halt the batch, never let a later row advance the cursor past the held row", cursor.c.ID) + } +} + // TestEventForwarder_NoopProvider_AdvancesCursor — wiring a real // email.NoopProvider through the forwarder is the integration check that // the SendClassSkippedNoTemplate path advances cursors. If this regresses, diff --git a/internal/jobs/lifecycle_emails.go b/internal/jobs/lifecycle_emails.go index 9213886..879d726 100644 --- a/internal/jobs/lifecycle_emails.go +++ b/internal/jobs/lifecycle_emails.go @@ -183,15 +183,25 @@ var ( `

Your deployment is now permanent — it will no longer expire automatically{{ if .Source }} (changed via {{ .Source }}){{ end }}.

It'll keep serving traffic until you delete it explicitly.

`)) + // NOTE: AppName here is the app_id — an opaque 8-char hex slug + // (generateAppID), NOT a human-readable name. So it is rendered ONLY as a + // labeled `App` identifier in a cell, never interpolated into prose + // as if it were a name ("Your deployment 6fffcc21 is live" reads as + // gibberish). This mirrors the deploy-TTL builders, which deliberately do + // NOT copy app_id → deploy_name for the same reason. bodyDeployCreated = template.Must(template.New("b_dcreated").Parse( - `

Your deployment{{ if .AppName }} {{ .AppName }}{{ end }} has started building{{ if .Env }} in the {{ .Env }} environment{{ end }}.

+ `

Your deployment has started building{{ if .Env }} in the {{ .Env }} environment{{ end }}.

+{{ if .AppName }} + +
App{{ .AppName }}
{{ end }}

We're building the image and rolling it out now — this usually takes under a minute. You'll get a second email with the live URL the moment it's serving traffic.

Watch the build live from your dashboard.

`)) bodyDeployHealthy = template.Must(template.New("b_dhealthy").Parse( - `

Your deployment{{ if .AppName }} {{ .AppName }}{{ end }} is live{{ if .Env }} in the {{ .Env }} environment{{ end }} and serving traffic.

-{{ if .AppURL }} - + `

Your deployment is live{{ if .Env }} in the {{ .Env }} environment{{ end }} and serving traffic.

+{{ if or .AppURL .AppName }}
URL{{ .AppURL }}
+ {{ if .AppURL }}{{ end }} + {{ if .AppName }}{{ end }} {{ if .TimeToHealthy }}{{ end }}
URL{{ .AppURL }}
App{{ .AppName }}
Build time{{ .TimeToHealthy }}s
{{ end }}

Manage, redeploy, or set a custom domain from your dashboard.

`)) @@ -724,8 +734,9 @@ func renderDeployMadePermanent(params map[string]string) (string, string, string // live URL yet. The "started" email links to the dashboard; the live URL // arrives in the separate deploy.healthy email. func renderDeployCreated(params map[string]string) (string, string, string) { - name := orDefault(params["app_name"], "your app") - subject := "Deploying " + name + " on instanode" + // app_name is the opaque app_id slug — used as an identifier, never as a + // prose name in the subject (see bodyDeployCreated NOTE). + subject := "Your instanode deployment has started" heading := "Your deployment has started" body := renderBody(bodyDeployCreated, viewDeployCreated{ AppName: params["app_name"], Env: params["env"], @@ -734,9 +745,14 @@ func renderDeployCreated(params map[string]string) (string, string, string) { Title: subject, Heading: heading, Body: body, CTALabel: "Watch the build", CTAURL: dashboardURL, }) + textBody := "Your deployment has started building." + if id := params["app_name"]; id != "" { + textBody += " App: " + id + "." + } + textBody += " This usually takes under a minute — you'll get a follow-up email with the live URL once it's serving traffic." text := lifecycleText(lifecycleTextView{ - Heading: heading, - Body: "Your deployment " + name + " has started building. This usually takes under a minute — you'll get a follow-up email with the live URL once it's serving traffic.", + Heading: heading, + Body: textBody, CTALabel: "Watch the build", CTAURL: dashboardURL, }) return subject, html, text @@ -746,8 +762,10 @@ func renderDeployCreated(params map[string]string) (string, string, string) { // success counterpart to renderDeployFailed: the app is live, so the email // leads with the URL and the CTA opens the running app. func renderDeployHealthy(params map[string]string) (string, string, string) { - name := orDefault(params["app_name"], "your app") - subject := name + " is live on instanode" + // app_name is the opaque app_id slug — shown as an identifier in the body, + // never as a prose name in the subject (see bodyDeployHealthy NOTE). The + // live URL identifies the app for the reader. + subject := "Your instanode deployment is live" heading := "Your app is live 🚀" body := renderBody(bodyDeployHealthy, viewDeployHealthy{ AppName: params["app_name"], @@ -765,10 +783,13 @@ func renderDeployHealthy(params map[string]string) (string, string, string) { Title: subject, Heading: heading, Body: body, CTALabel: ctaLabel, CTAURL: cta, }) - textBody := "Your deployment " + name + " is live and serving traffic." + textBody := "Your deployment is live and serving traffic." if url := params["app_url"]; url != "" { textBody += " It's available at " + url + "." } + if id := params["app_name"]; id != "" { + textBody += " App: " + id + "." + } text := lifecycleText(lifecycleTextView{ Heading: heading, Body: textBody,