Skip to content

Commit 3fd9b30

Browse files
committed
🤖 fix: align template build wait timeout semantics
Address Codex review by aligning aggregated API request timeout defaults with template build waits and preserving timeout/cancel semantics when mapping build wait errors. - set aggregated API request timeout default to 30m - distinguish internal build wait expiry from request lifecycle cancellation/deadline - keep internal wait expiry as BadRequest while returning Timeout for canceled/deadline contexts - add coverage for request-timeout mapping and config defaults - document request-timeout relationship in aggregated deployment guide --- _Generated with `mux` • Model: `openai:gpt-5.3-codex` • Thinking: `xhigh` • Cost: `$0.57`_ <!-- mux-attribution: model=openai:gpt-5.3-codex thinking=xhigh costs=0.57 -->
1 parent b462966 commit 3fd9b30

5 files changed

Lines changed: 198 additions & 8 deletions

File tree

‎docs/how-to/deploy-aggregated-apiserver.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,8 @@ The wait behavior is configurable via environment variables on the
103103
- `CODER_K8S_TEMPLATE_BUILD_INITIAL_POLL_INTERVAL` (default: `2s`)
104104
- `CODER_K8S_TEMPLATE_BUILD_MAX_POLL_INTERVAL` (default: `10s`)
105105

106+
- Aggregated API request timeout defaults to `30m`; keep it at or above the build wait timeout.
107+
106108
Behavior:
107109

108110
- Polls at the initial interval for the first 2 minutes.

‎internal/aggregated/storage/storage_test.go‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -509,6 +509,110 @@ func TestTemplateStorageUpdateReturnsBadRequestWhenTemplateVersionBuildWaitTimes
509509
}
510510
}
511511

512+
func TestTemplateStorageUpdateReturnsTimeoutWhenRequestContextTimesOut(t *testing.T) {
513+
server, state := newMockCoderServer(t)
514+
defer server.Close()
515+
516+
templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL))
517+
baseCtx := namespacedContext("control-plane")
518+
519+
createObj := &aggregationv1alpha1.CoderTemplate{
520+
ObjectMeta: metav1.ObjectMeta{Name: "acme.request-timeout-template"},
521+
Spec: aggregationv1alpha1.CoderTemplateSpec{
522+
Organization: "acme",
523+
Files: map[string]string{"main.tf": "resource \"null_resource\" \"initial\" {}"},
524+
},
525+
}
526+
createdObj, err := templateStorage.Create(baseCtx, createObj, rest.ValidateAllObjectFunc, nil)
527+
if err != nil {
528+
t.Fatalf("expected template create to succeed: %v", err)
529+
}
530+
createdTemplate := createdObj.(*aggregationv1alpha1.CoderTemplate)
531+
532+
t.Setenv(templateVersionBuildWaitTimeoutEnv, "5s")
533+
t.Setenv(templateVersionBuildBackoffAfterEnv, "0s")
534+
t.Setenv(templateVersionBuildInitialPollIntervalEnv, "10ms")
535+
t.Setenv(templateVersionBuildMaxPollIntervalEnv, "20ms")
536+
537+
state.setNextCreatedTemplateVersionStatus(codersdk.ProvisionerJobPending)
538+
539+
updateCtx, cancel := context.WithTimeout(baseCtx, 120*time.Millisecond)
540+
defer cancel()
541+
542+
desiredTemplate := createdTemplate.DeepCopy()
543+
desiredTemplate.Spec.Files = map[string]string{"main.tf": "resource \"null_resource\" \"updated\" {}"}
544+
545+
updatedObj, created, updateErr := templateStorage.Update(
546+
updateCtx,
547+
desiredTemplate.Name,
548+
testUpdatedObjectInfo{obj: desiredTemplate},
549+
nil,
550+
rest.ValidateAllObjectUpdateFunc,
551+
false,
552+
nil,
553+
)
554+
if updateErr == nil {
555+
t.Fatal("expected update to fail when request context times out")
556+
}
557+
if updatedObj != nil {
558+
t.Fatalf("expected nil updated object on request timeout, got %T", updatedObj)
559+
}
560+
if created {
561+
t.Fatal("expected update created=false")
562+
}
563+
if apierrors.IsBadRequest(updateErr) {
564+
t.Fatalf("expected non-BadRequest error for request context timeout, got %v", updateErr)
565+
}
566+
if !apierrors.IsTimeout(updateErr) {
567+
t.Fatalf("expected timeout error for request context timeout, got %v", updateErr)
568+
}
569+
assertTopLevelStatusError(t, updateErr)
570+
}
571+
572+
func TestMapTemplateVersionBuildWaitErrorMapsRequestContextFailuresToTimeout(t *testing.T) {
573+
tests := []struct {
574+
name string
575+
cause error
576+
}{
577+
{
578+
name: "deadline exceeded",
579+
cause: context.DeadlineExceeded,
580+
},
581+
{
582+
name: "canceled",
583+
cause: context.Canceled,
584+
},
585+
}
586+
587+
for _, tt := range tests {
588+
tt := tt
589+
t.Run(tt.name, func(t *testing.T) {
590+
t.Parallel()
591+
592+
mappedErr := mapTemplateVersionBuildWaitError(
593+
&templateVersionBuildWaitTimeoutError{
594+
versionID: uuid.New(),
595+
waitTimeout: time.Minute,
596+
lastStatus: codersdk.ProvisionerJobRunning,
597+
cause: tt.cause,
598+
},
599+
"acme.template",
600+
)
601+
602+
if mappedErr == nil {
603+
t.Fatal("expected mapped timeout error")
604+
}
605+
if apierrors.IsBadRequest(mappedErr) {
606+
t.Fatalf("expected timeout error, got BadRequest: %v", mappedErr)
607+
}
608+
if !apierrors.IsTimeout(mappedErr) {
609+
t.Fatalf("expected timeout error, got %v", mappedErr)
610+
}
611+
assertTopLevelStatusError(t, mappedErr)
612+
})
613+
}
614+
}
615+
512616
func TestLoadTemplateVersionBuildWaitConfigFromEnvDefaults(t *testing.T) {
513617
t.Setenv(templateVersionBuildWaitTimeoutEnv, "")
514618
t.Setenv(templateVersionBuildBackoffAfterEnv, "")

‎internal/aggregated/storage/template.go‎

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ var (
5050
_ rest.GracefulDeleter = (*TemplateStorage)(nil)
5151
_ rest.Scoper = (*TemplateStorage)(nil)
5252
_ rest.SingularNameProvider = (*TemplateStorage)(nil)
53+
54+
errTemplateVersionBuildWaitTimeoutExceeded = errors.New("template version build wait timeout exceeded")
5355
)
5456

5557
// TemplateStorage provides codersdk-backed CoderTemplate objects.
@@ -687,7 +689,14 @@ func mapTemplateVersionBuildWaitError(waitErr error, templateName string) error
687689

688690
var timeoutErr *templateVersionBuildWaitTimeoutError
689691
if errors.As(waitErr, &timeoutErr) {
690-
return apierrors.NewBadRequest(timeoutErr.Error())
692+
switch {
693+
case errors.Is(timeoutErr.cause, errTemplateVersionBuildWaitTimeoutExceeded):
694+
return apierrors.NewBadRequest(timeoutErr.Error())
695+
case errors.Is(timeoutErr.cause, context.DeadlineExceeded), errors.Is(timeoutErr.cause, context.Canceled):
696+
return apierrors.NewTimeoutError(timeoutErr.Error(), 0)
697+
default:
698+
return apierrors.NewInternalError(timeoutErr)
699+
}
691700
}
692701

693702
return coder.MapCoderError(waitErr, aggregationv1alpha1.Resource("codertemplates"), templateName)
@@ -709,7 +718,15 @@ func waitForTemplateVersionBuild(ctx context.Context, sdk *codersdk.Client, vers
709718
return waitConfigErr
710719
}
711720

712-
waitCtx, cancel := context.WithTimeout(ctx, waitConfig.waitTimeout)
721+
effectiveWaitTimeout := waitConfig.waitTimeout
722+
if requestDeadline, hasRequestDeadline := ctx.Deadline(); hasRequestDeadline {
723+
remaining := time.Until(requestDeadline)
724+
if remaining > 0 && remaining < effectiveWaitTimeout {
725+
effectiveWaitTimeout = remaining
726+
}
727+
}
728+
729+
waitCtx, cancel := context.WithTimeout(ctx, effectiveWaitTimeout)
713730
defer cancel()
714731

715732
pollInterval := waitConfig.initialPoll
@@ -721,12 +738,17 @@ func waitForTemplateVersionBuild(ctx context.Context, sdk *codersdk.Client, vers
721738
version, err := sdk.TemplateVersion(waitCtx, versionID)
722739
if err != nil {
723740
if waitCtx.Err() != nil {
741+
timeoutCause, timeoutCauseErr := templateVersionBuildWaitTimeoutCause(ctx, waitCtx)
742+
if timeoutCauseErr != nil {
743+
return timeoutCauseErr
744+
}
745+
724746
return &templateVersionBuildWaitTimeoutError{
725747
versionID: versionID,
726-
waitTimeout: waitConfig.waitTimeout,
748+
waitTimeout: effectiveWaitTimeout,
727749
lastStatus: lastStatus,
728750
lastError: lastError,
729-
cause: waitCtx.Err(),
751+
cause: timeoutCause,
730752
}
731753
}
732754
return fmt.Errorf("fetch template version %q: %w", versionID.String(), err)
@@ -771,21 +793,47 @@ func waitForTemplateVersionBuild(ctx context.Context, sdk *codersdk.Client, vers
771793

772794
select {
773795
case <-waitCtx.Done():
774-
if waitCtx.Err() == nil {
775-
return fmt.Errorf("assertion failed: wait context finished without an error")
796+
timeoutCause, timeoutCauseErr := templateVersionBuildWaitTimeoutCause(ctx, waitCtx)
797+
if timeoutCauseErr != nil {
798+
return timeoutCauseErr
776799
}
800+
777801
return &templateVersionBuildWaitTimeoutError{
778802
versionID: versionID,
779-
waitTimeout: waitConfig.waitTimeout,
803+
waitTimeout: effectiveWaitTimeout,
780804
lastStatus: lastStatus,
781805
lastError: lastError,
782-
cause: waitCtx.Err(),
806+
cause: timeoutCause,
783807
}
784808
case <-time.After(pollInterval):
785809
}
786810
}
787811
}
788812

813+
func templateVersionBuildWaitTimeoutCause(requestCtx, waitCtx context.Context) (error, error) {
814+
if requestCtx == nil {
815+
return nil, fmt.Errorf("assertion failed: request context must not be nil")
816+
}
817+
if waitCtx == nil {
818+
return nil, fmt.Errorf("assertion failed: wait context must not be nil")
819+
}
820+
821+
waitErr := waitCtx.Err()
822+
if waitErr == nil {
823+
return nil, fmt.Errorf("assertion failed: wait context finished without an error")
824+
}
825+
826+
if requestErr := requestCtx.Err(); requestErr != nil {
827+
return requestErr, nil
828+
}
829+
830+
if errors.Is(waitErr, context.DeadlineExceeded) {
831+
return errTemplateVersionBuildWaitTimeoutExceeded, nil
832+
}
833+
834+
return waitErr, nil
835+
}
836+
789837
func loadTemplateVersionBuildWaitConfigFromEnv() (templateVersionBuildWaitConfig, error) {
790838
waitTimeout, waitTimeoutErr := parseDurationEnvOrDefault(templateVersionBuildWaitTimeoutEnv, defaultTemplateVersionBuildWaitTimeout)
791839
if waitTimeoutErr != nil {

‎internal/app/apiserverapp/apiserverapp.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ const (
3838
// DefaultSecureServingPort is the secure serving port used by aggregated-apiserver mode.
3939
DefaultSecureServingPort = 6443
4040
serverName = "coder-k8s-aggregated-apiserver"
41+
// defaultRequestTimeout keeps API request lifetimes above template build wait defaults.
42+
defaultRequestTimeout = 30 * time.Minute
4143
)
4244

4345
// Options configures aggregated-apiserver bootstrap behavior.
@@ -192,6 +194,7 @@ func NewRecommendedConfig(
192194
recommendedConfig.RuleResolver = authz
193195
recommendedConfig.EffectiveVersion = apiservercompatibility.DefaultBuildEffectiveVersion()
194196
recommendedConfig.SkipOpenAPIInstallation = true
197+
recommendedConfig.RequestTimeout = defaultRequestTimeout
195198

196199
definitionNamer := apiserveropenapi.NewDefinitionNamer(scheme)
197200
recommendedConfig.OpenAPIConfig = genericapiserver.DefaultOpenAPIConfig(getOpenAPIDefinitions, definitionNamer)

‎internal/app/apiserverapp/apiserverapp_test.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,39 @@ func TestInstallAPIGroupRegistersDiscovery(t *testing.T) {
128128
}
129129
}
130130

131+
func TestNewRecommendedConfigSetsExtendedRequestTimeout(t *testing.T) {
132+
t.Helper()
133+
134+
scheme := NewScheme()
135+
if scheme == nil {
136+
t.Fatal("expected non-nil scheme")
137+
}
138+
codecs := serializer.NewCodecFactory(scheme)
139+
140+
listener, err := net.Listen("tcp", "127.0.0.1:0")
141+
if err != nil {
142+
t.Fatalf("create test listener: %v", err)
143+
}
144+
defer func() {
145+
_ = listener.Close()
146+
}()
147+
148+
secureServingOptions := genericoptions.NewSecureServingOptions()
149+
secureServingOptions.Listener = listener
150+
secureServingOptions.BindPort = 0
151+
secureServingOptions.ServerCert.CertDirectory = ""
152+
secureServingOptions.ServerCert.PairName = ""
153+
154+
recommendedConfig, err := NewRecommendedConfig(scheme, codecs, secureServingOptions)
155+
if err != nil {
156+
t.Fatalf("build recommended config: %v", err)
157+
}
158+
159+
if got, want := recommendedConfig.RequestTimeout, defaultRequestTimeout; got != want {
160+
t.Fatalf("expected request timeout %s, got %s", want, got)
161+
}
162+
}
163+
131164
func TestBuildClientProviderDefersMissingCoderConfigAsServiceUnavailable(t *testing.T) {
132165
t.Parallel()
133166

0 commit comments

Comments
 (0)