fix(coderd): show correct deletion time in dormancy notification - #26488
Conversation
The dormancy notification's "will be automatically deleted in X" sentence rendered the dormancy threshold instead of the auto-delete duration. A 30-day threshold rendered as "4 weeks" even when auto-delete was 90 days; a 60-day threshold rendered as "1 month" with a 7-day auto-delete. Render the countdown from the auto-delete setting, and skip the deletion sentence entirely when auto-delete is disabled so the notification no longer promises a deletion that will never happen.
7686a27 to
b03984c
Compare
There was a problem hiding this comment.
Worth testing the new branch in workspaces_test.go?
There was a problem hiding this comment.
Good call. I added similar test coverage here.
| // Auto-delete must be configured for the body to render a | ||
| // deletion timeline. | ||
| if tmpl.TimeTilDormantAutoDelete > 0 { | ||
| deleteTime := dbtime.Now().Add(time.Duration(tmpl.TimeTilDormantAutoDelete)) |
There was a problem hiding this comment.
Rather than generating the delete time with now + offset, we could use the updated DeletingAt returned from UpdateWorkspaceDormantDeletingAt via wsNew (mutating ws as we do with DormantAt). In practice this only saves us a very very minor data race between template updates and this loop and a bit of code duplication, so it isn't a big deal.
There was a problem hiding this comment.
Good thinking here! This is an even bigger win because it eliminates any race and removes a DB query for the template.
There was a problem hiding this comment.
Ah didn't realize the template was only needed for this - nice!
Eventually it might be nice to tighten up the time handling in our state transitions. As it stands now, a coder instance that's stopped for 30 days with dormant=7day and delete=7days template will have its workspaces immediately set to dormant when it starts back up but the deletion would be queued for now+7 days (at least, I think this is true). This is perhaps surprising (it would be to me), perhaps intentional, and certainly not a big deal. It would be nice if the transitions worked more like a discrete time simulation, where the reference time wasn't time.Now() but the time of the last item in the causal chain. Applying this idea to dormancy, we could pass ws.LastUsedAt (which is used to gate the transition) to UpdateWorkspaceDormantDeletingAt rather than dbtime.Now(). I'm not advocating for this change here btw (it might break other assumptions), it's just that all the Now() calls give me the ick. :)
There was a problem hiding this comment.
Very fair point on all the ad-hoc timestamps. It's a recurring pain point I see in many tests, and a theme in flakes I've fixed over time.
The dormancy notification previously quoted a deletion deadline derived from a cached template snapshot, separate from the deadline the database had just recorded on the workspace itself. Bind the notification to the workspace's recorded deadline instead.
…ration to 000527 main landed 000526_boundary_log_owner; bump to avoid the collision.
The dormancy notification's "will be automatically deleted in X" sentence rendered the dormancy threshold instead of the auto-delete duration. A 30-day threshold rendered as "4 weeks" even when auto-delete was 90 days; a 60-day threshold rendered as "1 month" with a 7-day auto-delete. Render the countdown from the auto-delete setting, and skip the deletion sentence entirely when auto-delete is disabled so the notification no longer promises a deletion that will never happen.