🤖 feat: reuse the earlier import when a CoderTemplate Update is retried

docs/reference/aggregated-api-behavior.md

@@ -149,7 +149,7 @@ For a `CoderTemplate` with `spec.files`, the server waits for Coder to finish im
 - **Update** with changed files waits the same way before making the new version active. Metadata changes in the same request (`displayName`, `description`, `icon`) are applied only after that. If the template changed in Coder during the wait, the Update returns `409 Conflict` and changes nothing.
 - **Create without `spec.files`** does not wait.
 
-If the import fails, times out, or the request is cancelled, Create creates no template and Update changes nothing (neither the source nor the metadata). The uploaded file and template version stay in Coder; they are not deleted or cancelled.
+If the import fails, times out, or the request is cancelled, Create creates no template and Update changes nothing (neither the source nor the metadata). The exception is an import that finishes just before a timeout; see [The 34-second write budget](#the-34-second-write-budget). The uploaded file and template version stay in Coder; they are not deleted or cancelled.
 
 ### The 34-second write budget
 
@@ -161,13 +161,33 @@ When the budget runs out:
 
 - The client gets `504 Gateway Timeout`. The message is usually `request did not complete within requested timeout - context deadline exceeded`, but it can also be the server's own template import timeout message.
 - Usually, Create creates no template and Update does not activate the new version. But if the import finishes just before the deadline, Coder can still create the template or activate the version while the client gets the `504`. The final state after a `504` is not certain, so re-read the template with `kubectl get` before you retry.
-- If the upload or the version creation had already finished, the file or the template version stays in Coder. If the request timed out while still waiting for the import, the import keeps running and can still succeed, but nothing uses it.
+- If the upload or the version creation had already finished, the file or the template version stays in Coder. If the request timed out while still waiting for the import, the import keeps running and can still succeed. For Create, nothing uses it. For Update, a retry with the same files picks it up (see [Update retries](#update-retries)).
 
-!!! warning "Retries are not idempotent"
-    Each retry creates another template version and starts another import. Coder reuses an identical uploaded file, but not the version. If the import takes longer than the budget, every retry times out again, even after an earlier import has succeeded. An Update that timed out while waiting for the import never activates its version later. If your client gave up before the server answered, re-read the template before retrying.
+!!! warning "Create retries are not idempotent"
+    Each Create retry creates another template version and starts another import. Coder reuses an identical uploaded file, but not the version. If the import takes longer than the budget, every retry times out again, even after an earlier import has succeeded. If your client gave up before the server answered, re-read the template before retrying.
+
+#### Update retries
+
+An Update with changed files names its template version after the template and the exact source: `k8s-` followed by 20 hex digits. It uploads the files first; Coder returns the existing file for identical bytes from the same Coder user. Then it looks for the latest attempt with that name. An attempt counts only if it was built from that same uploaded file:
+
+| Latest attempt with that name | What the Update does |
+| --- | --- |
+| None | Creates the version with that name. |
+| Pending or running, same file | Waits for it. No new version. |
+| Succeeded, not archived, same file | Activates it. |
+| Failed, canceled, canceling, or archived, or built from a different file | Creates the next attempt, named `<name>-2`, then `<name>-3`, and so on. |
+
+Retrying the same Update therefore converges on one import. If the import takes longer than the budget, each retry waits for that same import and gets `504` while it runs. The retry that is waiting when it succeeds, or the first retry after that, activates it and returns `200`. This also works after the aggregated API server restarts, because the name is computed from the request. If two requests create the same attempt at the same time, Coder rejects the second one, and that request waits for the first one's version.
+
+- A retry must send the same files. The name also covers files in the active version that `spec.files` does not list, so if the active version changes between retries, a new import starts.
+- `kubectl apply` re-reads the template on each run, so running it again is a valid retry. A client that sends an old `resourceVersion` again gets `409 Conflict` once the template has changed, for example after a late activation.
+- Finding the attempt takes a few lookups per request, at most 48. If that is not enough, the request fails with `503 Service Unavailable`; the next request starts over. If the request's deadline passes during the lookups, it fails with `504`. In both cases no template version or import is created, but the uploaded file can remain in Coder.
+- Retries converge only while the `k8s-…` versions are not renamed or otherwise changed outside coder-k8s. Renaming one can make a later retry start another import.
+- Any Coder user who can edit the template can create or rename a version, so the name alone is not trusted. A version with that name but other source, or one created by another Coder user (whose upload has its own file ID), is never reused.
+- Versions created before this behavior have random names and are never reused.
 
 !!! tip "Keep template imports fast"
-    Imports that take longer than the budget cannot complete through this API today. Keep the import well under 34 seconds. Follow [issue #117](https://github.com/coder/coder-k8s/issues/117) for changes to this behavior.
+    Create cannot complete an import that takes longer than the budget. Update can, through retries. Keep imports well under 34 seconds where you can. Follow [issue #117](https://github.com/coder/coder-k8s/issues/117) for changes to this behavior.
 
 ### Tuning
 
@@ -178,7 +198,7 @@ Set these environment variables on the `coder-k8s` Deployment:
 | `CODER_K8S_TEMPLATE_BUILD_WAIT_TIMEOUT` | `25m` | Upper limit for the import wait. Must be greater than `0`, at most `30m`, and at least `CODER_K8S_TEMPLATE_BUILD_BACKOFF_AFTER`. Values above the 34-second budget are allowed but do not extend the wait. |
 | `CODER_K8S_TEMPLATE_BUILD_BACKOFF_AFTER` | `2m` | Poll at the initial interval for this long, then back off. `0` turns backoff off, so the interval never grows. Must be `0` or more and at most the wait timeout. |
 | `CODER_K8S_TEMPLATE_BUILD_INITIAL_POLL_INTERVAL` | `2s` | Poll interval before backoff. Must be greater than `0`. |
-| `CODER_K8S_TEMPLATE_BUILD_MAX_POLL_INTERVAL` | `10s` | Backoff doubles the interval up to this value. Must be at least the initial poll interval. |
+| `CODER_K8S_TEMPLATE_BUILD_MAX_POLL_INTERVAL` | `10s` | Backoff doubles the interval up to this value. Must be greater than `0` and at least the initial poll interval. |
 
 The aggregated API server's request timeout defaults to `30m`. Neither that timeout nor `CODER_K8S_TEMPLATE_BUILD_WAIT_TIMEOUT` can extend a write request beyond the 34-second budget. The wait fails if the version build ends `failed` or `canceled`, or if the budget or the wait timeout runs out.
 

internal/aggregated/storage/storage_test.go

@@ -2903,6 +2903,8 @@ func (s *mockCoderServerState) handleRequest(t *testing.T, w http.ResponseWriter
 	case r.Method == http.MethodDelete && hasSegments(segments, "api", "v2", "templates") && len(segments) == 4:
 		s.handleDeleteTemplate(w, segments[3])
 		return
+	case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "templates") && len(segments) == 6 && segments[4] == "versions":
+		s.handleGetTemplateVersionByName(w, segments[3], segments[5])
 	case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "templateversions") && len(segments) == 4:
 		s.handleGetTemplateVersion(w, segments[3])
 		return
@@ -3090,6 +3092,15 @@ func (s *mockCoderServerState) handleUploadFile(w http.ResponseWriter, r *http.R
 		return
 	}
 
+	// Like coderd (files.go, GetFileByHashAndCreator), identical bytes from the same user return the existing
+	// file ID with 200; the mock has a single user.
+	for existingID, existing := range s.filesByID {
+		if bytes.Equal(existing, fileData) {
+			writeJSON(w, http.StatusOK, codersdk.UploadResponse{ID: existingID})
+			return
+		}
+	}
+
 	fileID := uuid.New()
 	s.filesByID[fileID] = fileData
 
@@ -3146,6 +3157,24 @@ func (s *mockCoderServerState) handleCreateTemplateVersion(w http.ResponseWriter
 		return
 	}
 
+	// Like coderd, keep a requested name and generate one otherwise; names are unique per template, and a
+	// duplicate answers 409 with a "name" validation error (coderd/templateversions.go, Coder v2.37.2).
+	templateVersionName := request.Name
+	if templateVersionName == "" {
+		templateVersionName = fmt.Sprintf("template-version-%d", len(s.templateVersionsByID)+1)
+	}
+	if request.TemplateID != uuid.Nil {
+		for _, existing := range s.templateVersionsByID {
+			if existing.TemplateID != nil && *existing.TemplateID == request.TemplateID && existing.Name == templateVersionName {
+				writeJSON(w, http.StatusConflict, codersdk.Response{
+					Message:     fmt.Sprintf("A template version with name %q already exists for this template.", templateVersionName),
+					Validations: []codersdk.ValidationError{{Field: "name", Detail: "This value is already in use and should be unique."}},
+				})
+				return
+			}
+		}
+	}
+
 	now := time.Now().UTC()
 	initialStatus := s.nextTemplateVersionInitialStatus
 	if initialStatus == "" {
@@ -3156,7 +3185,7 @@ func (s *mockCoderServerState) handleCreateTemplateVersion(w http.ResponseWriter
 		OrganizationID: s.organization.ID,
 		CreatedAt:      now,
 		UpdatedAt:      now,
-		Name:           fmt.Sprintf("template-version-%d", len(s.templateVersionsByID)+1),
+		Name:           templateVersionName,
 		Message:        request.Message,
 		Job: codersdk.ProvisionerJob{
 			FileID: request.FileID,
@@ -3310,6 +3339,30 @@ func (s *mockCoderServerState) handleDeleteTemplate(w http.ResponseWriter, templ
 	writeJSON(w, http.StatusOK, map[string]string{"message": "template deleted"})
 }
 
+// handleGetTemplateVersionByName serves GET /api/v2/templates/{template}/versions/{name}. Like coderd it does
+// not advance simulated imports; only by-ID polls do.
+func (s *mockCoderServerState) handleGetTemplateVersionByName(w http.ResponseWriter, templateIDSegment, name string) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	templateID, err := uuid.Parse(templateIDSegment)
+	if err != nil {
+		writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("invalid template id %q", templateIDSegment))
+		return
+	}
+	if _, ok := s.templatesByID[templateID]; !ok {
+		writeCoderError(w, http.StatusNotFound, "template not found")
+		return
+	}
+	for _, version := range s.templateVersionsByID {
+		if version.TemplateID != nil && *version.TemplateID == templateID && version.Name == name {
+			writeJSON(w, http.StatusOK, version)
+			return
+		}
+	}
+	writeCoderError(w, http.StatusNotFound, "template version not found")
+}
+
 func (s *mockCoderServerState) handleGetTemplateVersion(w http.ResponseWriter, templateVersionIDSegment string) {
 	s.mu.Lock()
 	defer s.mu.Unlock()

internal/aggregated/storage/template.go

@@ -681,30 +681,12 @@ func (s *TemplateStorage) Update(
 				return nil, false, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", err))
 			}
 
-			uploadResponse, err := sdk.Upload(ctx, codersdk.ContentTypeZip, bytes.NewReader(zipBytes))
-			if err != nil {
-				return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name)
-			}
-			if uploadResponse.ID == uuid.Nil {
-				return nil, false, fmt.Errorf("assertion failed: uploaded file ID must not be nil")
-			}
-
-			org, err := sdk.OrganizationByName(ctx, currentTemplate.Spec.Organization)
-			if err != nil {
-				return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name)
-			}
-
-			newVersion, err := sdk.CreateTemplateVersion(ctx, org.ID, codersdk.CreateTemplateVersionRequest{
-				TemplateID:    templateID,
-				StorageMethod: codersdk.ProvisionerStorageMethodFile,
-				FileID:        uploadResponse.ID,
-				Provisioner:   codersdk.ProvisionerTypeTerraform,
-			})
+			// Retries with the same source reuse the attempt an earlier request started (#117).
+			newVersion, err := ensureTemplateVersionForUpdate(
+				ctx, sdk, currentTemplate.Spec.Organization, templateID, name, zipBytes,
+			)
 			if err != nil {
-				return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name)
-			}
-			if newVersion.ID == uuid.Nil {
-				return nil, false, fmt.Errorf("assertion failed: new template version ID must not be nil")
+				return nil, false, err
 			}
 
 			if waitErr := waitForTemplateVersionBuild(ctx, sdk, newVersion.ID); waitErr != nil {