chore: improve build deadline code#19203
Conversation
deansheather
commented
Aug 6, 2025
- Adds/improves a lot of comments to make the autostop calculation code clearer
- Changes the behavior of the enterprise template schedule store to match the behavior of the workspace TTL endpoint when the new TTL is zero
- Fixes a bug in the workspace TTL endpoint where it could unset the build deadline, even though a max_deadline was specified
- Adds a new constraint to the workspace_builds table that enforces the deadline is non-zero and below the max_deadline if it is set
- Adds CHECK constraint enum generation to scripts/dbgen, used for testing the above constraint
- Adds Dean and Danielle as CODEOWNERS for the autostop calculation code
- Adds/improves a lot of comments to make the autostop calculation code clearer - Changes the behavior of the enterprise template schedule store to match the behavior of the workspace TTL endpoint when the new TTL is zero - Fixes a bug in the workspace TTL endpoint where it could unset the build deadline, even though a max_deadline was specified - Adds a new constraint to the workspace_builds table that enforces the deadline is non-zero and below the max_deadline if it is set - Adds CHECK constraint enum generation to scripts/dbgen, used for testing the above constraint - Adds Dean and Danielle as CODEOWNERS for the autostop calculation code
| @@ -0,0 +1,239 @@ | |||
| package main | |||
There was a problem hiding this comment.
Moved all the constraint stuff to it's own file because it's quite large.
There was a problem hiding this comment.
Also refactored it into a reusable function rather than pasting the same code three times. I'll admit it's not the prettiest but it's probably good enough.
DanielleMaywood
left a comment
There was a problem hiding this comment.
Looks good to me 
ssncferreira
left a comment
There was a problem hiding this comment.
Overall LGTM, just a couple of nits, suggestions, and questions.
Thanks for adding the extra comments, they’re super helpful! 🙌
| // Deadline is intended as a cost saving measure, not as a hard policy. It is | ||
| // derived from either the workspace's TTL or the template's TTL, depending on | ||
| // the template's policy, to ensure workspaces are stopped when they are idle. | ||
| // | ||
| // MaxDeadline is intended as a compliance policy. It is derived from the | ||
| // template's autostop requirement to cap workspace uptime and effectively force | ||
| // people to update often. | ||
| // | ||
| // Note that only the build's CURRENT deadline property influences automation in | ||
| // the autobuild package. As stated above, the MaxDeadline property is only used | ||
| // to cap the value of a build's deadline. |
There was a problem hiding this comment.
🤩 really nice comment, thanks!
| // If the build has a max_deadline set, we still need to | ||
| // abide by it. It will either be zero (our target), or a | ||
| // value. | ||
| Deadline: build.MaxDeadline, |
There was a problem hiding this comment.
Not sure I follow the reasoning behind this change 🤔
According to the above comment:
// If autostop has been disabled, we want to remove the deadline from the
// existing workspace build (if there is one).
if !dbTTL.Valid {
So, shouldn't deadline be time.Time{},?
There was a problem hiding this comment.
Actually, if I understood correctly, Deadline is derived from TTL and MaxDeadline is derived from Autostop. In that case, when autostop is disabled, shouldn’t just MaxDeadline be unset? 🤔
There was a problem hiding this comment.
This code is correct, but I'll update the comment to make it clearer.
// Use the max_deadline as the new build deadline. It will
// either be zero (our target), or a non-zero value that we
// need to abide by anyway due to template policy.
//
// Previously, we would always set the deadline to zero,
// which was incorrect behavior. When max_deadline is
// non-zero, deadline must be set to a non-zero value that
// is less than max_deadline.
//
// Disabling TTL autostop (at a workspace or template level)
// does not trump the template's autostop requirement.
//
// Refer to the comments on schedule.CalculateAutostop for
// more information.
There was a problem hiding this comment.
A new build with ttl: 0, autostop_requirement: enabled will end up with the deadlines of deadline: required_autostop_time, max_deadline: required_autostop_time (e.g. equal deadline and max_deadline).
So this matches that behavior.
|
@deansheather, should we cherry-pick this to previous release? |
|
No, I don't think so |