fix(coderd): show correct deletion time in dormancy notification (backport #26488) - #27895
Conversation
Code-only backport of #26488 to release/2.34. The stored notification body is unchanged; the timeTilDormant label now carries the auto-delete countdown from the workspace's deleting_at instead of the dormancy threshold. When auto-delete is disabled the label falls back to generic wording so the body no longer promises a deletion that will not happen. (cherry picked from #26488, without migration 000527)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3648ccec50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6482d8a957
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 6 findings (1 P2, 3 P3, 1 Nit, 1 Note), COMMENT. Review Finding inventoryFinding inventory, PR #27895Findings
Contested and acknowledgedNone yet. Round logRound 1Netero-only (P2 present, pre-panel gate). LOC 159 additions, below Law threshold, Law not spawned. 1 P2, 3 P3, 1 Nit, 1 Note. Orchestrator verified each finding against the head worktree: About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
First-pass review only. These are mechanical findings from a single reviewer (Netero); the full review panel has not looked at this PR yet and will review once these are addressed.
The backport reasoning is unusually well documented, and the tests earn their keep: Netero reverted only the two production files, kept the new tests, and all four dormancy subtests failed (actual: "59 seconds from now", actual: "now"). No new test is vacuous. Propagating wsNew.DeletingAt onto ws instead of re-fetching is the right call, and dropping the GetTemplateByID hop removes an error path that previously swallowed the notification.
One P2, three P3, one nit, one note.
The P2 is the fallback wording, which Codex raised on the lifecycle-executor site and you defended in an already-resolved thread. I verified the mechanics of that defense: coderd/notifications/render/gotmpl.go:23 does set missingkey=invalid, and migration 000311 does render the deletion sentence unconditionally, so omitting the label is genuinely not available without a migration. What the defense does not cover is the wording itself, which needs no migration to change. An agent cannot accept "we tell every auto-delete-disabled deployment that their workspace will be deleted" as permanent; that is a human decision. Either reword the fallback so it does not assert a deadline, or have a human explicitly accept the wording.
Netero on the golden fixture, which is my favourite kind of finding: "The whole approach of this backport is to splice prose into a fixed sentence, and the golden render is the only place that sentence is visible."
coderd/workspaces.go:1511
P3 [CRF-3] The PR removed one instance of a slog.Error(err)-instead-of-the-real-error bug and left its sibling in the same function. (Netero)
The deleted
tmplErrblock loggedslog.Error(err)rather thanslog.Error(tmplErr). The surviving initiator block has the identical defect: at line 1511,erris theUpdateWorkspaceDormantDeletingAterror, which is nil there because the check at lines 1496-1502 returns when it is not. So whenGetUserByIDfails, the log says "failed to fetch the user that marked the workspace as dormant" with no cause, and the notification is skipped (line 1516 gates oninitiatorErr == nil).
Verified in the diff and in the head worktree. The deleted hunk carried slog.Error(err) and the surviving block still does. One correction implies every instance: slog.Error(initiatorErr). Outside the diff hunk, but it is the same defect this PR just deleted a copy of, so fixing it here is cheap and nobody will come back for it.
🤖
coderd/notifications/testdata/rendered-templates/webhook/TemplateWorkspaceDormant.json.golden:23
P3 [CRF-4] Nothing renders the assembled sentence, so the fallback's fit inside the stored body is unverified. (Netero)
The whole approach of this backport is to splice prose into a fixed sentence, and the golden render is the only place that sentence is visible. The fixture label is
"24 hours", a value no enqueue site can now produce: the code emits eitherhumanize.Time(deleting_at)or the fallback clause. The new tests assert label values only, not rendered bodies.
Confirmed: coderd/notifications/notifications_test.go:795 still sets "timeTilDormant": "24 hours". The new tests assert label equality, which is exactly the check that cannot catch a sentence that reads wrong. Setting the fixture label to the fallback string (or adding a second case) and regenerating the goldens would make the rendered sentence reviewable, which is also what makes CRF-1 concrete for whoever decides the wording.
🤖
🤖 This review was automatically generated with Coder Agents.
Emyrk
left a comment
There was a problem hiding this comment.
Two non-blocking notes inline. Verified the label sources (deleting_at from UpdateWorkspaceDormantDeletingAt's RETURNING, both paths), that all TemplateWorkspaceDormant producers are covered, and that migration 000527 cleanly supersedes this on upgrade.
Coder Agents on behalf of @Emyrk.
Emyrk
left a comment
There was a problem hiding this comment.
only nits, nothing large or important
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Code-only backport of #26488 to
release/2.34.The dormancy notification's "will be automatically deleted in X" sentence rendered the dormancy threshold (
time_til_dormant) instead of the auto-delete duration (time_til_dormant_autodelete). A 30-day threshold rendered as "4 weeks" even when auto-delete was configured to fire much sooner or later, so users were told the wrong deletion date.Unlike #26488, this backport contains no migration. Adding migration 000527 to the 2.34 line would break the migration ordering for deployments that later upgrade. Instead, the stored notification body is left untouched and the existing
timeTilDormantlabel is populated with the correct value:deleting_at, whichUpdateWorkspaceDormantDeletingAtalready computes atomically fromtime_til_dormant_autodelete.deleting_atis unset and the sentence cannot be omitted without a body change, so the label falls back to generic wording: "...will be automatically deleted in line with your template's auto-deletion policy if it remains inactive."Upgrading to >= 2.35.1 later applies migration 000527 and the
timeTilDeleterename as usual; this patch is fully superseded at that point.Implementation notes
coderd/autobuild/lifecycle_executor.go: propagatewsNew.DeletingAtontowsafter the dormancy UPDATE and humanize it into thetimeTilDormantlabel.coderd/workspaces.go(putWorkspaceDormant): usenewWorkspace.DeletingAtfor the label; the template fetch that fed the wrong duration is removed.timeTilDormantbecause the 2.34 notification body (migration 000311) references it; renaming would require a data migration, which this backport deliberately avoids.TestNotifications/DormancyAutoDelete(lifecycle executor) andTestWorkspaceNotifications/Dormant/InitiatorNotOwnerWithAutoDelete(API path), plus fallback-wording assertions in the existing no-auto-delete tests. Both use a 35-day auto-delete sohumanize.Timedeterministically renders "1 month from now".This PR was generated by Coder Agents on behalf of @ibetitsmike.