🤖 fix: honor aggregated API delete preconditions

[ThomasK33]

Summary

Honor DeleteOptions.Preconditions for aggregated CoderTemplate and CoderWorkspace deletes. A supplied UID or resourceVersion that differs from the fetched object now returns 409 Conflict before admission or backend mutation. Deletes without preconditions, or with matching values, keep their existing behavior.

Fixes #108. Workspace version derivation remains unfixed in #109. This does not implement #107's leaf-name checks or #105's import-readiness work, and does not restore quickstart support.

Implementation and review focus

• Reuse Kubernetes' vendored precondition checker and error mapping. Explicit empty values are compared, not treated as absent.
• Keep namespace, canonical-identity and cross-organization privacy checks first. Compare the converted fetched snapshot, run admission, then mutate the same fetched backend ID.
• Add a shared matrix for both storage entrypoints, zero-mutation assertions, and checks that failed preconditions never invoke admission. Workspace deletion remains asynchronous.

Validation and limits

Candidate: 202821b7f9dd804157c2c182163d40c104b30667.

• Tests fail on the unchanged baseline for the ignored preconditions, then pass with the fix. All 12 final worker checks passed, including vendor verification, full tests/build/lint, docs checks, govulncheck and Trivy filesystem scanning at HIGH/CRITICAL severity.
• A fresh owned KIND/Coder v2.37.2 run was bound from source revision through binary, image and running pod: 64/64 steps passed, with 15/15 expected HTTP statuses. Both resources rejected wrong values with the original objects intact, and matching UID/RV/both succeeded on separate fixtures. Canonical repeated apply, alias rejection and LIST/GET behavior stayed intact. Owned resources were cleaned up.
• Templates: a real update retained the same ID, advanced the exposed resourceVersion, and an old-version DELETE returned 409.
• Workspaces: the wrong-RV test used an explicit synthetic value. This proves equality checking only. Tested builds, rename, TTL and autostart changes did not advance the workspace RV in Coder v2.37.2 (🤖 fix: make workspace resource versions reflect backend changes #109). UID protects object identity, not changes to the same object. Neither precondition check is an atomic backend compare-and-swap.
• The earlier exploratory run retained three failed steps: a rate-limit response, the disproved workspace-version assumption, and a harness assertion error. Those receipts remain unchanged. The final run was paced, used the final SHA, and had no retries. Other DeleteOptions behavior was not exercised.

Recorded evidence

Final screenshot:

e2e-accelerated-timing.webm

The video is a post-hoc terminal render with idle gaps trimmed: 27.48 seconds from a 380-second capture. It is not real-time playback. The raw cast is retained. The recorded-timing export failed; its failure receipt is retained. Both evidence packets passed hash verification: 438 initial and 274 final artifacts.

---

📋 Implementation Plan

Enforce DELETE preconditions on aggregated objects (#108)

Goal and boundary

Honor metav1.DeleteOptions.Preconditions (UID and resourceVersion) on CoderTemplate and CoderWorkspace deletes so a delete carrying a UID or resourceVersion that differs from the fetched object is rejected with a Kubernetes-compatible error before any Coder mutation. Deletes without preconditions, and deletes whose preconditions match the fetched object, behave as today.

Out of scope: leaf-name case handling (#107), template-import readiness (#105), quickstart/installer, any new version scheme, compare-and-swap subsystem, persistence of Kubernetes metadata, adjacent cleanup.

Baseline behaviour (main 3f65df0c, tree 6c49783c)

• Both delete handlers discard the options: internal/aggregated/storage/template.go:755 and internal/aggregated/storage/workspace.go:653 take _ *metav1.DeleteOptions. The apiserver handler passes the decoded options through unchanged (vendor/k8s.io/apiserver/pkg/endpoints/handlers/delete.go:169); only the generic registry Store enforces preconditions itself (vendor/k8s.io/apiserver/pkg/registry/generic/registry/store.go:629-641), which these custom storages do not use.
• Exposed identity fields come from the converters: UID = <backend ID> and resourceVersion = strconv.FormatInt(UpdatedAt.UnixNano(), 10) (internal/aggregated/convert/template.go:32-33, internal/aggregated/convert/workspace.go:36-37). The same converters build the object handed to admission deleteValidation (template.go:799, workspace.go:692), so the fetched object already carries the UID/RV that a precondition must be compared against.
• Backend mutations are ID-bound: sdk.DeleteTemplate(ctx, template.ID) (template.go:804) and sdk.CreateWorkspaceBuild(ctx, workspace.ID, {Transition: delete}) (workspace.go:698). Neither Coder API accepts a version/CAS argument.
• Observed on a case-folding storage simulation (main and ef29d3a5): a template delete with a mismatched UID precondition deleted the canonical template (err=nil, deleted=true); the workspace path has the same shape and needs its own regression.

Precondition policy (smallest supported)

• UID: compare preconditions.UID with the fetched object's UID (types.UID(<backend ID>)). Mismatch → 409 Conflict with the standard message Precondition failed: UID in precondition: <x>, UID in object meta: <y>. The subsequent mutation targets that same fetched backend ID rather than resolving the request name again.
• resourceVersion: compare preconditions.ResourceVersion with the fetched object's converted RV (UpdatedAt.UnixNano() string). Mismatch → 409 Conflict (Precondition failed: ResourceVersion in precondition: …). This compares the supplied value with the fetched snapshot, not an atomic Kubernetes storage transaction. A mismatch detects a different exposed value, not every backend change. Template metadata updates changed that value in the live test. On Coder 2.37.2, tested workspace builds, rename, TTL and autostart changes did not advance it; matching workspace RVs therefore do not protect against those same-object changes (tracked in 🤖 fix: make workspace resource versions reflect backend changes #109). Check and delete are also not atomic: there is no CAS on the Coder delete APIs, so a change between the fetch and the backend call is not detected. UID protects identity, not edits to the same object.
• Reuse, not reinvention: call the vendored storage.Preconditions{UID, ResourceVersion}.Check(key, obj) (vendor/k8s.io/apiserver/pkg/storage/interfaces.go:138-165) on the converted fetched object and map its InvalidObjError through storageerrors.InterpretDeleteError(err, aggregationv1alpha1.Resource(<resource>), name) (vendor/k8s.io/apiserver/pkg/storage/errors/storage.go:97-108), which yields the same apierrors.NewConflict the generic registry produces. nil options or nil preconditions → no check (Check returns nil for a nil receiver).
• Ordering: namespace resolution → name parsing → backend fetch → canonical/opacity identity checks (unchanged) → convert once → precondition check → admission deleteValidation on the same converted object → backend delete by the fetched ID. Precondition failures therefore never reach upload/build/delete calls and cannot disclose anything about objects that the identity checks already made opaque (NotFound still wins for cross-organization requests because it is returned before the precondition check).
• Result object: unchanged (metav1.Status success; workspace delete keeps deleted=false because deletion is asynchronous).

Changes (one bounded PR)

File	Change	Size
internal/aggregated/storage/template.go	name the options parameter; after TemplateByName, obj := convert.TemplateToK8s(...); if err := checkDeletePreconditions(options, obj, resource, name); reuse obj for admission and the watch event	~10 lines
internal/aggregated/storage/workspace.go	same after requireFetchedWorkspaceIdentity; the converted pre-delete object is used for the check and admission; the post-build snapshot logic stays	~10 lines
internal/aggregated/storage/errors.go (existing file)	checkDeletePreconditions(options *metav1.DeleteOptions, obj runtime.Object, resource schema.GroupResource, name string) error wrapping storage.Preconditions.Check + InterpretDeleteError; nil-safe; assertion on nil obj	~20 lines
internal/aggregated/storage/storage_test.go / new delete_preconditions_test.go	mock: count DELETE /api/v2/templates/* and POST /api/v2/workspaces/*/builds (the existing mutations() recorder already captures every non-GET request); test matrix below	~150 lines
docs/how-to/deploy-aggregated-apiserver.md	short "Delete preconditions" subsection: UID/RV supported, Conflict on mismatch, RV = backend updated_at (refetch on Conflict), non-atomic window	~12 lines

No API type, codegen, manifest, dependency or RBAC change. Estimated production delta ≈ 40 lines; risk: low (additive checks before existing mutations; ordinary deletes unchanged). Clients holding an RV from before a backend updated_at change receive Conflict and must refetch. This is an intended stale-snapshot rejection, not an atomicity guarantee. Only UID/resourceVersion preconditions are in scope; this change does not claim support for every DeleteOptions feature.

Test matrix (red → green, real rest.Storage entrypoints, both resources)

Case	Expected	Backend calls
options == nil	delete proceeds (template deleted=true; workspace delete build, deleted=false)	1 mutation
options.Preconditions == nil / empty Preconditions{}	proceeds	1
UID matches fetched ID	proceeds	1
UID mismatch (RED today)	apierrors.IsConflict, message names both UIDs; object still present (template hasTemplate true; workspace buildTransitions empty)	0
RV matches converted UpdatedAt.UnixNano()	proceeds	1
RV differs from the fetched value (RED on baseline; synthetic mismatch in unit tests)	Conflict, no mutation	0
UID and RV both match	proceeds	1
UID match + RV mismatch / UID mismatch + RV match	Conflict, no mutation	0
explicitly supplied empty UID / empty RV against a nonempty fetched value	Conflict, no mutation (not treated as absent)	0
cross-organization workspace / alias organization with any preconditions	existing opaque NotFound / BadRequest unchanged, precondition never evaluated, no disclosure of the real UID	0
missing object with preconditions	NotFound (fetch fails first)	0
failed preconditions with an admission callback	Conflict; callback is not called	0
deleteValidation rejection with matching preconditions	admission error; callback runs once	0

Gate: retain native failing-test receipts on unchanged main for the two RED cases per resource, then green for the whole matrix plus the existing storage/watch suites.

Phases and quality gates

1. Red tests (both resources, matrix above) on 3f65df0c; receipt of native failures.
2. Implementation (errors.go helper, two call sites, doc subsection); gate: focused + ./internal/aggregated/... green, make lint.
3. Full gates on the exact candidate: make verify-vendor, make build, make test, make lint, make docs-check, govulncheck, Trivy fs; attributed local commits and an exact candidate patch series; any later source or documentation commit requires final-head checks again.
4. Owned Kind dogfood (existing proven harness logic with explicit destination assertions; new cluster/tag/clone bound to the candidate SHA via vcs.revision and running-pod imageID): bootstrap CNPG/control plane, canonical template + workspace as today, then:
   • preconditions through the live API (kubectl proxy + curl -X DELETE -H "Content-Type: application/json" -d '{"preconditions":{"uid":"<x>"}}' against /apis/aggregation.coder.com/v1alpha1/namespaces/coder/codertemplates/<canonical>): wrong UID → HTTP 409 with the original backend object intact; matching UID, matching RV and both matching → 200 on separate disposable fixtures. Template success means absent from Coder; workspace success means a delete build with transition delete. Exercise mismatch combinations. Preconditions must be in the explicit request body; the tested kubectl delete -f path did not send them even with metadata.uid;
   • use separate disposable fixtures for destructive success cases. For templates, retain a real RV, make a controlled backend change, verify the same ID exposes a different RV, then show the old-RV DELETE fails and leaves that object intact. For workspaces, use an explicitly synthetic wrong RV to prove equality checking only; do not present it as proof of protection against real updates. Preserve the earlier failed same-ID workspace-version-change assumption as a limitation tracked in 🤖 fix: make workspace resource versions reflect backend changes #109. Keep all attempts and legitimate conflicts; do not hide them with blind retries. Pace template reads and updates to stay within the observed file-download rate limit;
   • the original identity flow (canonical apply ×2, alias rejections, LIST→GET) re-run to prove it is intact.
     Evidence: terminal transcript, screenshots and video of the actual CLI (playback labelled), native exits, discovery, backend counts, cleanup receipts (own cluster/tag/kubeconfig/session only).

Race and limits statement (for docs and PR)

Precondition checks run against the object fetched in the same request, and the mutation targets that same backend ID. This avoids a second name lookup between the comparison and mutation. A resourceVersion precondition detects a mismatch against the fetched exposed value. It does not detect changes that leave that value unchanged, or changes between the fetch and backend call (no CAS in the Coder delete APIs). Deletion of workspaces remains asynchronous.

---

Generated with xum • Model: coder:openai/gpt-6-astra • Thinking: xhigh

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	🔄 Running since 2026-09-22T14:41:39.007730Z	202821b	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-22T15:00:33.570094Z	202821b	Manual request
🔒 Security Review	✅ Completed 2026-09-22T14:51:48.223670Z	202821b	Manual request

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[ThomasK33]

@codex security review

[chatgpt-codex-connector]

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 202821b7f9

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

• You comment "@codex security review"
• A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

[ThomasK33]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 202821b7f9

ℹ️ 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".

[ThomasK33]

🤖 Delivery verified

Landed through the required squash merge queue as f9a78c162780629431a1e3b86ac7ea062d6c11ea. The landed tree matches reviewed source 202821b7f9dd804157c2c182163d40c104b30667; parent, attribution and main ancestry were verified.

1. Validation: 12 worker checks and 5 maintainer checks passed on the reviewed source. The isolated, SHA-bound runtime run passed all 64 steps and 15 expected HTTP outcomes. Screenshot and labelled video evidence are in the PR body.
2. Reviews: Separate explicit normal and security verdicts are clean; zero review threads remain. A fresh independent final assessor recommended ready with tracked follow-ups. Four assessments completed against the six-assessment limit. The loop stopped because the evidence established readiness, not because the limit was exhausted.
3. Merge admission: Run 35745413208 passed all six required checks, including actual full Kind E2E, not the PR skip notice.
4. After merge: Main CI and docs/deployment succeeded. The publisher output matches OCI index sha256:0a8bb932f5854028bed7c1a64253124d12f485f3dd622d0ffb3db75d7bdc1098; both linux/amd64 and linux/arm64 image configurations identify the exact landed commit. Index, child-manifest and configuration hashes were checked.

Issue #108 is closed. This implements fetched-snapshot UID/resourceVersion equality, not atomic deletion or general workspace change detection. Issue #109 remains open for workspace version representation; #107 remains open for canonical leaf names; #105 remains open for import readiness. The maintainer desk owns those follow-ups, with #107 next. No quickstart-support claim is added.

---

Generated with xum • Model: coder:openai/gpt-6-astra • Thinking: xhigh