Skip to content

fix: adjust workspace claims to be initiated by users#20179

Merged
SasSwart merged 2 commits into
mainfrom
jjs/internal-934
Oct 8, 2025
Merged

fix: adjust workspace claims to be initiated by users#20179
SasSwart merged 2 commits into
mainfrom
jjs/internal-934

Conversation

@SasSwart
Copy link
Copy Markdown
Contributor

@SasSwart SasSwart commented Oct 6, 2025

The prebuilds user never initiates a workspace claim autonomously. A claim can only happen when a user attempts to create a workspace. When listing prebuild provisioner jobs, it would not make sense to see jobs related to users who are creating workspaces and have gotten a prebuilt workspace. When cleaning up an overwhelmed provisioner queue, we should not delete claims as they have humans waiting for them and are not part of the thundering herd.

Therefore, this PR ensures that provisioner jobs that claim workspaces are considered to be initiated by the user, not the prebuilds system.

@SasSwart SasSwart changed the title fix: workspace claims are initiated by users fix: adjust workspace claims to now initiated by users Oct 6, 2025
@SasSwart SasSwart changed the title fix: adjust workspace claims to now initiated by users fix: adjust workspace claims to be initiated by users Oct 6, 2025
@SasSwart SasSwart marked this pull request as ready for review October 6, 2025 12:15
Copy link
Copy Markdown
Contributor

@ssncferreira ssncferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, it makes sense for the initiator to be the user who actually claims the prebuild rather than the system user 👍

I only have one concern: after this change, existing data in the database will have mixed information. Older workspace_build rows will have the prebuilds system user as the initiator, while new ones will have the user who claimed the workspace. This means the data becomes inconsistent in how the initiator is represented.

I don’t think this will be an issue in practice, but it’s definitely something we should document in case we add future database queries or analytics that rely on this field. Maybe a short code comment or note in the schema would help make that clear?

@johnstcn
Copy link
Copy Markdown
Member

johnstcn commented Oct 6, 2025

existing data in the database will have mixed information. Older workspace_build rows will have the prebuilds system user as the initiator, while new ones will have the user who claimed the workspace. This means the data becomes inconsistent in how the initiator is represented.

I can see an argument for creating a migration to update the existing schema, as the initiator ID shows up in the following tables:

  • workspace_builds
  • provisioner_jobs
  • audit_logs (e.g. search for resource_type:workspace_build username:prebuilds).

I'd suggest keeping it out of scope of this PR.

The fact that this change hasn't required any updates to existing tests is mildly concerning though.

@ssncferreira
Copy link
Copy Markdown
Contributor

The fact that this change hasn't required any updates to existing tests is mildly concerning though.

Agree 💯 I don't think we test the initiator ID 😕 so it might actually be a good idea to also add a test to cover this new change!

@SasSwart
Copy link
Copy Markdown
Contributor Author

SasSwart commented Oct 7, 2025

We have a thorough test suite on prebuild claiming already.
I've added assertions to it to ensure that the initiator is set correctly in each case.

@SasSwart SasSwart requested a review from ssncferreira October 7, 2025 11:43
Copy link
Copy Markdown
Contributor

@ssncferreira ssncferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thank you for adding the tests.
Regarding the possible migration to update the existing schema, we can do that on a follow-up PR or create a separate issue.

Copy link
Copy Markdown
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@SasSwart
Copy link
Copy Markdown
Contributor Author

SasSwart commented Oct 8, 2025

I've created the follow-up issue here:
#20207

@SasSwart SasSwart merged commit 544f155 into main Oct 8, 2025
25 checks passed
@SasSwart SasSwart deleted the jjs/internal-934 branch October 8, 2025 08:40
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants