From 8e7c7bbf24fb17ee58283fa08006efc58195a94e Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Wed, 23 Sep 2026 03:47:19 +0800 Subject: [PATCH] =?UTF-8?q?fix(reaper):=20deliver=20pre-reap=20warnings=20?= =?UTF-8?q?for=20real=20=E2=80=94=20and=20never=20fake=20a=20delivery?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The §18 warning path had no delivery channel at all: no Warner implementation existed, `felis reaper` passed nil, and maybeWarn still stamped warned_3d_at/ warned_1d_at and counted `warned=N`. So every owned server was silently reaped 15 days after its last join with no notice, and the operator's only feedback said warnings were sent. Two changes close that: - Honest stamps: warned_* now records a DELIVERED notice. A nil Warner logs `warning suppressed — no warner wired` and does NOT stamp; a delivery error logs and retries on the next daily run (bounded by the warning window). The stamps are no longer burned by notices nobody received. - A real channel: mail.SendNotice (the second and last message shape the mail package sends) plus a mailWarner that resolves the owner's VERIFIED email and mails the notice through the configured [smtp] relay. `felis reaper` wires it when [smtp] is set (same password_ref convention as felis-api) and prints exactly what happens when it is not. Plumbing so the in-cluster CronJob can actually reach the relay: the reaper pod gets the optional FELIS_SMTP_PASSWORD env (same Secret as felis-api), and the "configure email" screen now refreshes the minecraft-namespace mirrors of felis-smtp AND felis-config (a secretKeyRef is namespace-local, and the config mirror is what carries [smtp] into the reaper's own config). `felis setup`'s replica list gains felis-smtp for fresh installs. Tests: the delivered/retried/suppressed matrix in internal/reaper (the old "stamp advances on failure" contract is deliberately replaced), the notice message shape, the warner's resolve/send/failure paths, and the CronJob's optional-secret env. docs/troubleshooting.md §10 now states the real semantics. --- cmd/felis/reaper.go | 77 +++++++++++++++++++++++++++++ cmd/felis/reaper_test.go | 46 +++++++++++++++++ cmd/felis/setup.go | 7 +++ cmd/felis/systemservers.go | 9 ++-- cmd/felis/tui_smtp.go | 65 +++++++++++++++++++++--- docs/troubleshooting.md | 23 +++++++++ internal/mail/mail.go | 33 ++++++++++++- internal/mail/mail_test.go | 27 ++++++++++ internal/platform/workloads.go | 14 ++++++ internal/platform/workloads_test.go | 13 +++++ internal/reaper/reaper.go | 32 +++++++++--- internal/reaper/reaper_test.go | 56 ++++++++++++++++++--- 12 files changed, 375 insertions(+), 27 deletions(-) diff --git a/cmd/felis/reaper.go b/cmd/felis/reaper.go index 6d4842b..14b6a07 100644 --- a/cmd/felis/reaper.go +++ b/cmd/felis/reaper.go @@ -2,6 +2,8 @@ package main import ( "context" + "database/sql" + "errors" "flag" "fmt" "io" @@ -14,6 +16,8 @@ import ( "felis.lolicon.best/internal/apis/felis/v1alpha1" "felis.lolicon.best/internal/backup" "felis.lolicon.best/internal/config" + "felis.lolicon.best/internal/mail" + "felis.lolicon.best/internal/platform" "felis.lolicon.best/internal/reaper" "felis.lolicon.best/internal/store" corev1 "k8s.io/api/core/v1" @@ -81,6 +85,47 @@ func cmdReaper(args []string, stdout, stderr io.Writer) int { Archiver: archiver, } + // Pre-reap warnings go out by email when [smtp] is configured (the same + // relay and password_ref convention felis-api uses); without it the channel + // stays nil and the reaper logs each suppressed warning instead of stamping + // it, so a later SMTP setup still gets to warn. The owner must have a + // VERIFIED address — that flag is what proves the mailbox. + if cfg.SMTP.Host != "" { + passRef := cfg.SMTP.PasswordRef + if passRef == "" { + passRef = platform.SMTPPasswordEnv + } + password := os.Getenv(passRef) + if cfg.SMTP.Username != "" && password == "" { + fmt.Fprintf(stderr, "felis reaper: warning: [smtp] username is set but credentials env %s is empty — warning emails will fail AUTH\n", passRef) + } + db := drv.DB() + r.Warner = &mailWarner{ + lookupEmail: func(ctx context.Context, ownerID string) (string, error) { + var email string + switch err := db.QueryRowContext(ctx, + `SELECT email FROM users + WHERE id = $1 AND email_verified = true AND COALESCE(email, '') <> ''`, + ownerID).Scan(&email); { + case errors.Is(err, sql.ErrNoRows): + return "", fmt.Errorf("owner %s has no verified email", ownerID) + case err != nil: + return "", err + } + return email, nil + }, + notifier: &mail.SMTP{ + Host: cfg.SMTP.Host, + Port: cfg.SMTP.Port, + From: cfg.SMTP.From, + Username: cfg.SMTP.Username, + Password: password, + }, + } + } else { + fmt.Fprintln(stderr, "felis reaper: [smtp] not configured — pre-reap warnings are logged and NOT marked sent") + } + sum, err := r.RunOnce(ctx) if err != nil { fmt.Fprintf(stderr, "felis reaper: %v\n", err) @@ -91,6 +136,38 @@ func cmdReaper(args []string, stdout, stderr io.Writer) int { return 0 } +// mailWarner delivers a pre-reap notice to the owner's verified email — the +// only channel this build can reach. Unowned owners and owners who never proved +// a mailbox yield an error; the reaper retries such notices on its next run and +// never lets them block the reap (red line ⑤). +type mailWarner struct { + lookupEmail func(ctx context.Context, ownerID string) (string, error) + notifier noticeNotifier +} + +// noticeNotifier is the slice of mail.SMTP the warner needs (injected in tests). +type noticeNotifier interface { + SendNotice(ctx context.Context, email, subject, body string) error +} + +func (w *mailWarner) Warn(ctx context.Context, ownerID, server, remaining string) error { + email, err := w.lookupEmail(ctx, ownerID) + if err != nil { + return fmt.Errorf("resolve owner email: %w", err) + } + subject := fmt.Sprintf("Felis: 服务器 %s 将在 %s 后回收 · server reaped in %s", server, remaining, remaining) + body := fmt.Sprintf( + "Felis 世界回收提醒 / world-reaper notice\r\n"+ + "\r\n"+ + "服务器 / Server: %s\r\n"+ + "距回收 / Time left: %s\r\n"+ + "\r\n"+ + "闲置的服务器会先自动备份，再释放世界；有人加入游戏即可重置倒计时。\r\n"+ + "Idle servers are backed up and then released; any join resets the countdown.\r\n", + server, remaining) + return w.notifier.SendNotice(ctx, email, subject, body) +} + // reaperConfig derives the reaper's retention windows from felis.toml. The 15d // idle deadline is fixed by §18; only the warning offsets, retention, and the // store soft-cap are configurable (§24). diff --git a/cmd/felis/reaper_test.go b/cmd/felis/reaper_test.go index 5dbd4f3..040f65f 100644 --- a/cmd/felis/reaper_test.go +++ b/cmd/felis/reaper_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "os" "path/filepath" "strings" @@ -75,3 +76,48 @@ func TestResolveWorldDir(t *testing.T) { } }) } + +// The pre-reap warner resolves the owner's VERIFIED email and hands the notice +// to the mailer. Every failure (no verified address, relay refusal) returns an +// error so the reaper retries on its next run instead of stamping a notice +// nobody received. +func TestMailWarner(t *testing.T) { + lookup := func(email string, err error) func(context.Context, string) (string, error) { + return func(context.Context, string) (string, error) { return email, err } + } + + n := &captureNotifier{} + w := &mailWarner{lookupEmail: lookup("owner@example.net", nil), notifier: n} + if err := w.Warn(context.Background(), "u1", "survival", "3d"); err != nil { + t.Fatalf("Warn: %v", err) + } + if n.email != "owner@example.net" || !strings.Contains(n.subject, "survival") || !strings.Contains(n.subject, "3d") { + t.Fatalf("notice envelope = (%q, %q)", n.email, n.subject) + } + if !strings.Contains(n.body, "survival") || !strings.Contains(n.body, "3d") { + t.Fatalf("body missing server/remaining:\n%s", n.body) + } + + w = &mailWarner{lookupEmail: lookup("", errors.New("owner u2 has no verified email")), notifier: n} + if err := w.Warn(context.Background(), "u2", "survival", "3d"); err == nil || !strings.Contains(err.Error(), "verified email") { + t.Fatalf("unverified owner = %v, want the lookup error surfaced", err) + } + + w = &mailWarner{lookupEmail: lookup("owner@example.net", nil), notifier: &captureNotifier{err: errors.New("relay down")}} + if err := w.Warn(context.Background(), "u1", "survival", "3d"); err == nil || !strings.Contains(err.Error(), "relay down") { + t.Fatalf("relay failure = %v, want it surfaced", err) + } +} + +type captureNotifier struct { + email, subject, body string + err error +} + +func (n *captureNotifier) SendNotice(_ context.Context, email, subject, body string) error { + if n.err != nil { + return n.err + } + n.email, n.subject, n.body = email, subject, body + return nil +} diff --git a/cmd/felis/setup.go b/cmd/felis/setup.go index 0c6eea9..1d88c2c 100644 --- a/cmd/felis/setup.go +++ b/cmd/felis/setup.go @@ -246,6 +246,13 @@ func provisionSystemServers(ctx context.Context, cfg *config.Config, out io.Writ naming.ForwardingSecretName, naming.ForwardingSecretKey, "forwarding-secret", "minecraft ns"), ensureSecretReplica(ctx, cl, controlNS, cfg.K8s.Namespace, "felis-config", "felis.toml", "config", "minecraft ns"), + // The reaper's pre-reap warning emails authenticate with the same relay + // password felis-api uses; the reaper pod runs in the minecraft namespace, + // where a secretKeyRef resolves only against a local mirror. Skipped while + // the relay is not configured yet — the "configure email" screen refreshes + // both mirrors when it applies. + ensureSecretReplica(ctx, cl, controlNS, cfg.K8s.Namespace, + "felis-smtp", "password", "smtp", "minecraft ns"), // The build namespace needs the same token: the build Job's fetch // initContainer reads the submission context from the internal face. Best // effort — a deployment that only installs the control plane simply never diff --git a/cmd/felis/systemservers.go b/cmd/felis/systemservers.go index c2d05d2..97c4a47 100644 --- a/cmd/felis/systemservers.go +++ b/cmd/felis/systemservers.go @@ -478,11 +478,12 @@ func phaseOrPending(p v1alpha1.Phase) string { // beside the control plane — so without this replica the secretKeyRef would dangle and // wedge the pod in CreateContainerConfigError. // -// Two Secrets need it, for different reasons: the service token (the login limbo and -// the build Pod's context fetch — both authenticate to the felis-api internal face) -// and the Velocity modern-forwarding secret (every backend — it is how a backend knows +// Three Secrets need it, for different reasons: the service token (the login limbo and +// the build Pod's context fetch — both authenticate to the felis-api internal face), +// the Velocity modern-forwarding secret (every backend — it is how a backend knows // a login really came from the proxy, and so that the player's UUID is Mojang-verified -// rather than offline-derived). +// rather than offline-derived), and the SMTP relay password (the reaper's pre-reap +// warning emails; the felis-config mirror is what carries [smtp] into its pod). // // It is create-if-absent: an existing replica is left untouched so a hand-rotated // value in the workload namespace is never clobbered (to rotate, delete the replica diff --git a/cmd/felis/tui_smtp.go b/cmd/felis/tui_smtp.go index ff7e729..1ad81e5 100644 --- a/cmd/felis/tui_smtp.go +++ b/cmd/felis/tui_smtp.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "os" "strconv" "strings" @@ -338,17 +339,23 @@ func applySMTPConfig(ctx context.Context, in smtpInputs) error { if err := applyFelisConfigSecret(ctx); err != nil { return err } + // Refresh the workload-namespace copies too (the reaper's warning path): the + // OTP path is already live in the control namespace, so a replica miss is + // reported but not fatal. + if err := replicateSMTPToWorkloadNamespace(ctx, in.password); err != nil { + fmt.Fprintf(os.Stderr, "felis setup: warning: email is configured, but refreshing the workload copies failed (pre-reap warning emails may stay suppressed): %v\n", err) + } if err := kubectl(ctx, "-n", "felis", "rollout", "restart", "deployment/felis-api"); err != nil { return err } return kubectl(ctx, "-n", "felis", "rollout", "status", "deployment/felis-api", "--timeout=180s") } -// applySMTPSecret creates (or replaces) the felis-smtp Secret the felis-api -// Deployment injects the relay password from. Rendered in-process and piped to -// `kubectl apply` — the password is never a command-line arg, so it never -// appears in the host process table. -func applySMTPSecret(ctx context.Context, password string) error { +// smtpSecretManifest renders the felis-smtp Secret (in the control namespace, +// via the caller's apply) the felis-api Deployment injects the relay password +// from. Rendered in-process and piped to `kubectl apply` — the password is +// never a command-line arg, so it never appears in the host process table. +func smtpSecretManifest(password string) ([]byte, error) { secret := &corev1.Secret{ TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "Secret"}, ObjectMeta: metav1.ObjectMeta{Name: platform.SMTPSecretName, Namespace: "felis"}, @@ -359,7 +366,53 @@ func applySMTPSecret(ctx context.Context, password string) error { } manifest, err := yaml.Marshal(secret) if err != nil { - return fmt.Errorf("render smtp secret: %w", err) + return nil, fmt.Errorf("render smtp secret: %w", err) + } + return manifest, nil +} + +func applySMTPSecret(ctx context.Context, password string) error { + manifest, err := smtpSecretManifest(password) + if err != nil { + return err } return kubectlWithInput(ctx, manifest, "apply", "-f", "-") } + +// replicateSMTPToWorkloadNamespace refreshes the workload-namespace (minecraft) +// copies of felis-smtp and felis-config after email is reconfigured. The +// reaper's CronJob runs there and resolves both by local reference — a +// secretKeyRef is namespace-local, and `felis setup` creates the felis-config +// replica create-if-absent, so without this refresh a later SMTP change would +// never reach the pre-reap warning emails. Deliberately OVERWRITES both: these +// are mirrors of the control-namespace sources, and a stale mirror is exactly +// the failure this closes. +func replicateSMTPToWorkloadNamespace(ctx context.Context, password string) error { + cfg, err := config.Load(hostSetupConfigPath) + if err != nil { + return err + } + ns := cfg.K8s.Namespace + if ns == "" || ns == "felis" { + return nil + } + smtpManifest, err := smtpSecretManifest(password) + if err != nil { + return err + } + if err := kubectlWithInput(ctx, smtpManifest, "-n", ns, "apply", "-f", "-"); err != nil { + return fmt.Errorf("replicate %s to %s: %w", platform.SMTPSecretName, ns, err) + } + manifest, err := kubectlOutput(ctx, + "-n", ns, "create", "secret", "generic", "felis-config", + "--from-file=felis.toml="+podSetupConfigPath, + "--dry-run=client", "-o", "yaml", + ) + if err != nil { + return fmt.Errorf("render felis-config for %s: %w", ns, err) + } + if err := kubectlWithInput(ctx, manifest, "-n", ns, "apply", "-f", "-"); err != nil { + return fmt.Errorf("replicate felis-config to %s: %w", ns, err) + } + return nil +} diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 7c274d3..98a821e 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -504,6 +504,29 @@ So a missing backup never results in a deleted world. [GO-TESTED: - CRD missing → logs `reaper: CRD missing, skipping`, skipped. - Idle `≤ 15d` → not yet eligible. +### Pre-reap warnings (the `warn_before` offsets) + +An OWNED server inside a warning window gets an email notice (`3d`/`1d` before +the deadline, `warn_before` from `[archive]`) to the owner's **verified** email — +the same `[smtp]` relay felis-api uses. The `warned_3d_at` / `warned_1d_at` +stamps record a **delivered** notice: + +- No `[smtp]` configured (or owner has no verified address): the run logs + `reaper: warning suppressed — no warner wired` / a delivery error and does + NOT stamp. Nothing is falsely recorded as sent, and the day SMTP is + configured the pending warning can still go out. +- Delivery failure (relay down): logged and retried on the next daily run — + bounded by the warning window, since the reap removes the candidate anyway. +- `warned=` in the run output counts DELIVERED notices, not attempts. + +The reaper runs in the minecraft namespace and reads the **mirrors** of +`felis-smtp` and `felis-config` there (a `secretKeyRef` is namespace-local). The +installed `felis setup`'s "configure email" screen refreshes both mirrors when it +applies, so configuring SMTP after install is enough; a manual edit of the +control-namespace Secret alone is not. [GO-TESTED: the delivered/retried/ +suppressed matrix in `internal/reaper`; live-drilled end to end against a local +SMTP sink.] + ### Genuine false-delete risk vectors - **Stale `last_active_at`.** The keep-alive is `RecordJoin`, called from the diff --git a/internal/mail/mail.go b/internal/mail/mail.go index 7b7a246..8a10beb 100644 --- a/internal/mail/mail.go +++ b/internal/mail/mail.go @@ -1,8 +1,9 @@ // Package mail is the SMTP implementation of the api.OTPMailer seam: it // delivers the email one-time codes the passwordless doors mint (onboarding, // email login, op-login) through the relay configured in felis.toml [smtp]. -// It is deliberately tiny — one message shape, stdlib net/smtp — because the -// only mail Felis ever sends is a six-digit code. +// It is deliberately tiny — two message shapes, stdlib net/smtp — because the +// only mail Felis ever sends is a six-digit code plus the reaper's pre-deletion +// notice (SendNotice). // // TLS posture: port 465 dials implicit TLS; any other port dials plaintext and // upgrades via STARTTLS when the relay advertises it. AUTH is attempted only @@ -53,6 +54,23 @@ func (s *SMTP) SendOTP(ctx context.Context, email, code string) error { return c.Quit() } +// SendNotice mails one operator-composed notice to email — the reaper's +// pre-deletion warning is its only caller. Subject and body are the caller's; +// the body is CRLF-normalized so a multi-line string renders as one text/plain +// message. Delivery errors surface exactly like SendOTP's, so the caller can +// retry on its own cadence. +func (s *SMTP) SendNotice(ctx context.Context, email, subject, body string) error { + c, err := s.connect(ctx) + if err != nil { + return err + } + defer c.Close() + if err := s.deliver(c, email, notice(s.From, email, subject, body, time.Now())); err != nil { + return err + } + return c.Quit() +} + // Ping proves the configured relay will actually ACCEPT mail from this sender, // by running a complete transaction — connect, (STARTTLS,) AUTH, MAIL FROM, // RCPT TO, DATA — and delivering a short self-test message to From itself. The @@ -208,3 +226,14 @@ func selfTest(from string, now time.Time) []byte { b.WriteString("Sent by `felis setup` when the SMTP relay was configured. / 由 `felis setup` 配置 SMTP 时发出。\r\n") return []byte(b.String()) } + +// notice renders an operator notice: the shared header block plus the caller's +// body, CRLF-normalized so every line obeys RFC 5322 regardless of which line +// endings the caller's format string produced. +func notice(from, to, subject, body string, now time.Time) []byte { + body = strings.ReplaceAll(strings.ReplaceAll(body, "\r\n", "\n"), "\n", "\r\n") + if !strings.HasSuffix(body, "\r\n") { + body += "\r\n" + } + return []byte(headers(from, to, subject, now) + body) +} diff --git a/internal/mail/mail_test.go b/internal/mail/mail_test.go index 93f6635..935e2f6 100644 --- a/internal/mail/mail_test.go +++ b/internal/mail/mail_test.go @@ -58,6 +58,33 @@ func TestSelfTestCarriesNoCode(t *testing.T) { } } +// TestNoticeShape pins the second message shape — the reaper's pre-deletion +// warning: CRLF throughout even when the caller's body used bare LFs, a +// Q-encoded subject when it carries non-ASCII, and the caller's text rendered +// verbatim between the header block and the wire. +func TestNoticeShape(t *testing.T) { + now := time.Date(2026, 7, 20, 12, 0, 0, 0, time.UTC) + msg := string(notice("felis@example.net", "player@example.org", + "Felis: 服务器 survival 将回收", "line one\nline two\n", now)) + + if strings.Contains(strings.ReplaceAll(msg, "\r\n", ""), "\n") { + t.Error("notice contains a bare LF; every line must end CRLF") + } + headers, body, ok := strings.Cut(msg, "\r\n\r\n") + if !ok { + t.Fatal("notice has no blank line between headers and body") + } + if !strings.Contains(headers, "To: player@example.org") { + t.Errorf("headers missing To:\n%s", headers) + } + if !strings.Contains(headers, "Subject: =?utf-8?") { + t.Errorf("non-ASCII subject must be Q-encoded:\n%s", headers) + } + if !strings.Contains(body, "line one\r\nline two\r\n") { + t.Errorf("body must be CRLF-normalized verbatim text:\n%q", body) + } +} + // fakeRelay speaks just enough SMTP for net/smtp, answering 250 to MAIL FROM // and RCPT TO but dataVerdict at end-of-DATA. That split is the entire point: // relays which validate sender identity (Fastmail among them) accept MAIL FROM diff --git a/internal/platform/workloads.go b/internal/platform/workloads.go index 72a6d1b..1459e29 100644 --- a/internal/platform/workloads.go +++ b/internal/platform/workloads.go @@ -536,6 +536,20 @@ func reaperCronJob(p Params) *batchv1.CronJob { "--config", configFilePath, "--worlds-root", worldsMountPath, }, + // The [smtp] relay password for pre-reap warning emails — same optional + // Secret felis-api reads. Namespace caveat: a secretKeyRef is + // namespace-local, so this resolves against the minecraft-ns felis-smtp + // mirror that the "configure email" screen refreshes (the felis-config + // mirror it also refreshes is what puts [smtp] in this pod's config). + // Absent Secret ⇒ empty env ⇒ the reaper logs suppressed warnings + // instead of stamping them (never a failed pod). + Env: []corev1.EnvVar{ + {Name: SMTPPasswordEnv, ValueFrom: &corev1.EnvVarSource{SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{Name: SMTPSecretName}, + Key: SMTPSecretPasswordKey, + Optional: boolPtr(true), + }}}, + }, VolumeMounts: []corev1.VolumeMount{ {Name: configVolume, MountPath: configMountPath, ReadOnly: true}, {Name: worldsVolume, MountPath: worldsMountPath, ReadOnly: true}, diff --git a/internal/platform/workloads_test.go b/internal/platform/workloads_test.go index 102df9a..b4f1f92 100644 --- a/internal/platform/workloads_test.go +++ b/internal/platform/workloads_test.go @@ -729,6 +729,19 @@ func TestReaperCronJob_Shape(t *testing.T) { t.Errorf("reaper image = %q, want FelisImage %q", c.Image, p.FelisImage) } + // The relay password for pre-reap warning emails: same optional Secret as + // felis-api, resolved against the minecraft-ns mirror. Optional so an install + // without SMTP still starts (the reaper then logs suppressed warnings). + smtpEnv := envVar(c.Env, SMTPPasswordEnv) + if smtpEnv == nil || smtpEnv.ValueFrom == nil || smtpEnv.ValueFrom.SecretKeyRef == nil { + t.Fatalf("reaper must wire %s from a secretKeyRef", SMTPPasswordEnv) + } + if ref := smtpEnv.ValueFrom.SecretKeyRef; ref.Name != SMTPSecretName || ref.Key != SMTPSecretPasswordKey { + t.Errorf("reaper %s ref = %s/%s, want %s/%s", SMTPPasswordEnv, ref.Name, ref.Key, SMTPSecretName, SMTPSecretPasswordKey) + } else if ref.Optional == nil || !*ref.Optional { + t.Errorf("reaper %s secretKeyRef must be optional", SMTPPasswordEnv) + } + // config: Secret, mounted read-only (it carries the DB URL). cfgVol := volumeByName(ps.Volumes, configVolume) if cfgVol == nil || cfgVol.Secret == nil || cfgVol.Secret.SecretName != configSecretName { diff --git a/internal/reaper/reaper.go b/internal/reaper/reaper.go index c90e975..a891796 100644 --- a/internal/reaper/reaper.go +++ b/internal/reaper/reaper.go @@ -203,7 +203,10 @@ type Cluster interface { } // Warner delivers an impending-reap notice. It is optional and best-effort: a -// nil Warner or a delivery error never blocks a reap (red line ⑤). +// nil Warner or a delivery error never blocks a reap (red line ⑤). Warn returns +// nil only when the notice was handed to the delivery channel; an error (or a +// nil Warner) leaves warned_* unstamped, so the next daily run retries instead +// of silently burning the owner's only warning. type Warner interface { Warn(ctx context.Context, ownerID, server, remaining string) error } @@ -433,9 +436,11 @@ func (r *Reaper) ensureCapacity(ctx context.Context, now time.Time, sum *Summary // maybeWarn sends at most one impending-reap notice per run, honoring §18's // elif precedence (earliest unsent warning first). Unowned servers are never -// warned but are still reaped at the deadline (red line ⑤). A warner delivery -// failure is logged but the warned_* stamp still advances so the notice is not -// retried forever; a real join (RecordJoin) is what clears the stamps. +// warned but are still reaped at the deadline (red line ⑤). The warned_* stamp +// records a DELIVERED notice: a nil Warner or a delivery error is logged and +// leaves the stamp untouched, so the next run retries — bounded by the warning +// window, since the reap itself removes the candidate. A real join (RecordJoin) +// clears the stamps when a player renews. func (r *Reaper) maybeWarn(ctx context.Context, now time.Time, idle time.Duration, offs []time.Duration, c Candidate, sum *Summary) { if c.OwnerID == "" { return @@ -449,10 +454,21 @@ func (r *Reaper) maybeWarn(ctx context.Context, now time.Time, idle time.Duratio if !c.warnedAt(tier).IsZero() { continue // already sent this tier } - if r.Warner != nil { - if err := r.Warner.Warn(ctx, c.OwnerID, c.Name, formatRemaining(offs[i])); err != nil { - r.log().Warn("reaper: warn delivery failed (best-effort)", "server", c.Name, "err", err) - } + if r.Warner == nil { + // No delivery channel is wired at all. Do not stamp: an operator who + // wires one later must still be able to warn, and a stamp here would + // have recorded a notice nobody received. Logged every run so silence + // is never mistaken for delivery. + r.log().Warn("reaper: warning suppressed — no warner wired", + "server", c.Name, "owner", c.OwnerID, "remaining", formatRemaining(offs[i])) + return + } + if err := r.Warner.Warn(ctx, c.OwnerID, c.Name, formatRemaining(offs[i])); err != nil { + // Best-effort: the reap still proceeds on schedule, but the stamp + // stays empty so the next daily run retries the delivery instead of + // permanently suppressing the owner's only notice. + r.log().Warn("reaper: warn delivery failed; will retry next run", "server", c.Name, "err", err) + return } if err := r.Store.MarkWarned(ctx, c.Name, tier, now); err != nil { r.log().Error("reaper: mark warned failed", "server", c.Name, "err", err) diff --git a/internal/reaper/reaper_test.go b/internal/reaper/reaper_test.go index 6824d90..a0d2011 100644 --- a/internal/reaper/reaper_test.go +++ b/internal/reaper/reaper_test.go @@ -472,6 +472,8 @@ func TestWarningsDerivedFromNonDefaultDeadline(t *testing.T) { // past 7d but unowned -> never warned (red line ⑤) Candidate{Name: "e", OwnerID: "", LastActiveAt: idleBy(8 * Day)}, ) + rw := &recordingWarner{} + r.Warner = rw sum := mustRun(t, r) if sum.WorldsReaped != 0 { @@ -480,6 +482,9 @@ func TestWarningsDerivedFromNonDefaultDeadline(t *testing.T) { if sum.Warned != 2 { t.Fatalf("Warned = %d, want 2 (a:3d, b:1d)", sum.Warned) } + if len(rw.sent) != 2 { + t.Fatalf("deliveries = %d, want 2 (a:3d, b:1d)", len(rw.sent)) + } if cl.deletePVCCalls != 0 { t.Fatalf("a warning path deleted a PVC") } @@ -494,20 +499,48 @@ func TestWarningsDerivedFromNonDefaultDeadline(t *testing.T) { } } -// Red line ⑤ (best-effort): a Warner delivery error does not abort the run, and -// the warned_* stamp still advances (a real join, not a failed warn, is what -// resets the clock). -func TestWarningBestEffortOnDeliveryFailure(t *testing.T) { +// Red line ⑤ (best-effort) with delivery honesty: a Warner failure does not +// abort the run, and it does NOT stamp — the stamp records a DELIVERED notice, +// so the next daily run retries (the warning window bounds the retries, and the +// reap clears the candidate either way). +func TestWarningDeliveryRetriedAfterFailure(t *testing.T) { r, st, _, _ := newReaper(DefaultConfig(), Candidate{Name: "h", OwnerID: "u-h", LastActiveAt: idleBy(13 * Day)}) r.Warner = failWarner{} sum := mustRun(t, r) - if sum.Warned != 1 { - t.Fatalf("Warned = %d, want 1 despite delivery failure", sum.Warned) + if sum.Warned != 0 { + t.Fatalf("Warned = %d, want 0 (nothing was delivered)", sum.Warned) + } + if !st.byName["h"].Warned3dAt.IsZero() { + t.Fatal("a failed delivery must not stamp warned_3d_at") + } + + // Next run with a working channel: the SAME warning goes out and stamps. + rw := &recordingWarner{} + r.Warner = rw + sum = mustRun(t, r) + if sum.Warned != 1 || len(rw.sent) != 1 { + t.Fatalf("retry: Warned=%d sent=%d, want 1/1", sum.Warned, len(rw.sent)) } if st.byName["h"].Warned3dAt.IsZero() { - t.Fatalf("warned_3d_at not stamped after best-effort warn") + t.Fatal("a delivered warning must stamp warned_3d_at") + } +} + +// A nil Warner suppresses the warning WITHOUT stamping it: nothing was sent, so +// nothing is recorded as sent — and the day a channel is wired, the owner can +// still be warned. +func TestWarningSuppressedWithoutWarner(t *testing.T) { + r, st, _, _ := newReaper(DefaultConfig(), + Candidate{Name: "n", OwnerID: "u-n", LastActiveAt: idleBy(13 * Day)}) + + sum := mustRun(t, r) + if sum.Warned != 0 { + t.Fatalf("Warned = %d, want 0 with no warner wired", sum.Warned) + } + if !st.byName["n"].Warned3dAt.IsZero() || !st.byName["n"].Warned1dAt.IsZero() { + t.Fatal("a suppressed warning must not stamp either tier") } } @@ -517,6 +550,15 @@ func (failWarner) Warn(context.Context, string, string, string) error { return errors.New("smtp unavailable") } +// recordingWarner captures deliveries so the threshold tests exercise the real +// deliver-then-stamp path. +type recordingWarner struct{ sent []string } + +func (w *recordingWarner) Warn(_ context.Context, ownerID, server, remaining string) error { + w.sent = append(w.sent, ownerID+"/"+server+"/"+remaining) + return nil +} + // §26 capacity: when the store is over its cap, the oldest backup is evicted // early (destructive — audited) to make room, then the reap proceeds. func TestCapacityEvictsOldestThenReaps(t *testing.T) {