Conversation
A CoderTemplate Update with changed spec.files created a new template
version on every request. When the import outlasted the request, each
retry started another import and none of them ever became active.
Update now names the version after the target template and the exact
source zip ("k8s-" + 20 hex digits of a hash). Before creating a
version it looks up the latest attempt with that name. A pending,
running or succeeded attempt is reused, so a retry waits on or promotes
it, also after a server restart. A failed, canceled or archived attempt
is skipped and the next attempt gets "<name>-<n>". If another request
creates the same name first, Coder answers 409 and the lookup runs again
instead of failing.
Finding the latest attempt uses an exponential probe and a binary
search, so skipped attempts cost O(log n) lookups. Each request stops
after 48 lookups (503) or when its context ends (504), and creates
nothing in that case. Create is unchanged.
Part of #117
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
Change-Id: Id70c6acd7411b73e7bf26b71c035d896c2e209d8
Signed-off-by: Thomas Kosiewski <tk@coder.com>
Update retries with the same files now wait on or activate the version an earlier request started, and a failed attempt gets the next name. Document that for Update only; Create retries still import again. Also point the Template builds summary to the 34-second budget section for imports that finish just before a timeout, and say that the maximum poll interval must be greater than 0. Part of #117 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_ Change-Id: I810f0c355814752dbe79f8d80d956d1fdbcf83de Signed-off-by: Thomas Kosiewski <tk@coder.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
The docs-quality job flagged "resends" in the Update retries section. Part of #117 Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_ Change-Id: I9a3fa5d2fb48bc71da74cc62fba2d7a6cbdbb3be
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3c3d68d0e
ℹ️ 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".
… file Coder lets callers choose or change template version names, so a version with the derived name could hold other source. Reusing it on the name alone could wait on and activate the wrong source. Update now uploads the source first (Coder returns the existing file ID for the same bytes from the same user) and reuses an attempt only when its job was built from that file ID. A name match with another file counts as unusable, so the next attempt name is used. The lookup bound and context handling are unchanged. The mock deduplicates uploads like coderd. Part of #117 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_ Change-Id: I631bb729f759c21b3cb1e0eb88ebd9a6f8ba8a83 Signed-off-by: Thomas Kosiewski <tk@coder.com>
List canceling with the unusable attempt states, and explain that a version with the derived name is reused only when it was built from the same uploaded file, so versions with other source or from another Coder user are never reused. Part of #117 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_ Change-Id: I94f127c0633e1ab2b842f22c29a03b1f9361b4cd Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8576174bed
ℹ️ 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".
| if version.Archived || version.Job.FileID != fileID { | ||
| return false |
There was a problem hiding this comment.
Fresh evidence beyond the earlier source-mismatch report is that CreateTemplateVersionRequest accepts import-affecting inputs such as ProvisionerTags and UserVariableValues, while this predicate verifies only the deduplicated file ID. A caller using the same Coder user can create or rename a version built from that same file but with different variable values or provisioner routing; a retry then treats it as its own and can activate build results that the Kubernetes Update would never have created. Validate all relevant creation inputs or use an immutable ownership marker, and cover the same-file/different-input case.
AGENTS.md reference: AGENTS.md:L107-L108
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and not fixable with the current Coder API. Coder v2.37.2 does not expose a template version's provisioner type or user variable values (codersdk.ProvisionerJob has no provisioner field, and the job input is decoded to TemplateVersionID/WorkspaceBuildID/Error only), and the expected tags depend on those variables. So there is no fail-closed check that an earlier attempt was built with the same inputs. This PR is deferred until Coder offers a request-scoped idempotency key or exposes the job's creation inputs. Leaving this thread open.
A lookup that fails with 503 or 504 creates no template version or import, but the file it uploaded first can remain in Coder. Say that instead of "creates nothing". Retries converge only while the k8s-... versions are not renamed or changed outside coder-k8s: renaming one breaks the contiguous attempt names, and a later retry starts another import. Document that and add a test that pins the behavior: one extra import, and only the new attempt built from the retry's own upload is activated. Part of #117 _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_ Change-Id: I2be9fbd85d5b2f0efa24fc4a323a73a77324a86b Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
Deferred, and converted to draft. The last review found that reuse checks the version name and the uploaded file, but not the other inputs that affect an import: provisioner type, user variable values and provisioner tags. A version built from the same file with different inputs could be waited on and activated. Coder v2.37.2 does not expose the provisioner type or the user variable values of a template version, and the expected tags depend on those variables, so there is no fail-closed way to prove that an earlier attempt matches the request. Anything that trusts the name and file alone changes the contract this PR claims. What stays on this branch for later: the naming and lookup code, the file-ID check against name squatting, the tests (including out-of-band rename and squatting), and the docs. The latest commit (7123a09) fixes the two smaller findings; it does not fix the input-equivalence finding, which stays open.
Generated with |
This branch has not been deployed
1 participant
Part of #117.
What changed
Retrying the same
CoderTemplateUpdate now waits for, or activates, the template version an earlier attempt started, instead of starting a new import every time. Before this, an import that outlasted the 34-second write budget could never become active through the API: each retry created another version, timed out again, and left one more unused version behind.How it works:
k8s-plus 20 hex digits of a hash over the template ID and the exact source zip. Attempt n ≥ 2 is<name>-<n>. The name is recomputed from the request, so it also works after a restart. Two templates with identical files never share a version.TemplateVersionByName; an exponential probe plus binary search, about 2·log₂(n) lookups).409with anamevalidation error and creates no job. The Update looks up again and waits on the winner's version, never returning AlreadyExists.503, nothing created). A deadline or cancellation during the lookups returns504, nothing created. There is no permanent cap on attempts.docs/reference/aggregated-api-behavior.mdgets an "Update retries" section and keeps the "not idempotent" warning for Create. It also folds in two small wording fixes from the #128 review.Why only Update
Coder has no API to look up a version that is not attached to a template yet, so reuse on Create can only be best-effort. That part is deferred. The Coder behaviors this PR relies on (name rules, the duplicate-name
409and its single transaction, lookup by name) were checked in the Coder v2.37.2 source first, and the409was confirmed live.Tests
Review round 2 added the file check:
TestTemplateStorageUpdateNeverReusesSquattedNamebuilds a version with the exact derived name from other source (succeeded, pending, running); the Update creates<name>-2and never touches the squatter. Removing the file check fails all of its cases. A live KIND check on Coder v2.37.2 (real attempt renamed away, squatter created with the derived name) confirmed that repeatedkubectl applyactivated<name>-2built from the right file and never the squatter.template_update_reuse_test.goadds 19 tests, all passing: name derivation; timeout then retry on the same and on a restarted server (exactly oneCreateTemplateVersion); a succeeded attempt promoted with no new version and no upload; failed, canceled, being-canceled and archived attempts each lead to-2; 40 earlier failures lead to-41in ≤12 lookups; a lost duplicate-name race; lost responses (version created or promotion applied, reply dropped); no sharing across templates; the 48-lookup limit (503); cancellation mid-scan (504, no writes). The fullinternal/aggregated/...suite passes.Mutations: always creating a new version, treating the
409as an error, raising the lookup limit, dropping the template ID from the name, a linear scan, treating archived as reusable, and dropping the in-flight context mapping each make at least one test fail. One more mutation (no context check before each lookup) is equivalent to the last one and changes no behavior.Gates on the head:
make test,make test-integration,make build,make verify-vendor,make lint,make docs-check, all exit 0.Live KIND run against Coder v2.37.2
Recorded on an image built from the same code before a docs-only rebase (the pod's image ID matched the built image).
Step 2: Coder itself rejects a second version with the same name on one template (
409with anamevalidation error), and the rejected request creates no import job.Step 3: an Update to a source that takes about 102 s to import, applied repeatedly with
kubectl apply:504,504, then200. Coder ends with one new version,k8s-aa38de892b445b2e6016, now active, and one new import job.Step 4: an import that fails. Two applies return
400and createk8s-6130…and thenk8s-6130…-2.Accelerated replay of the recorded run (14 s; idle gaps trimmed):
live-kind-run-accelerated.webm
Not verified
409path is covered by a unit test and by the live Coder409in step 2.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh• Cost:$1680.17