Skip to content

Commit 63db29c

Browse files
ThomasK33ThomasK33
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 63db29c

5 files changed

Lines changed: 224 additions & 9 deletions

File tree

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,10 @@ 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+
108+
- `CODER_K8S_TEMPLATE_BUILD_WAIT_TIMEOUT` values above `30m` are rejected, because they cannot exceed the API request timeout.
109+
106110
Behavior:
107111

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

‎internal/aggregated/storage/storage_test.go‎

Lines changed: 116 additions & 1 deletion
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, "")
@@ -534,12 +638,23 @@ func TestLoadTemplateVersionBuildWaitConfigFromEnvDefaults(t *testing.T) {
534638
}
535639

536640
func TestLoadTemplateVersionBuildWaitConfigFromEnvInvalid(t *testing.T) {
537-
t.Setenv(templateVersionBuildWaitTimeoutEnv, "1m")
641+
t.Setenv(templateVersionBuildWaitTimeoutEnv, "31m")
538642
t.Setenv(templateVersionBuildBackoffAfterEnv, "2m")
539643
t.Setenv(templateVersionBuildInitialPollIntervalEnv, "500ms")
540644
t.Setenv(templateVersionBuildMaxPollIntervalEnv, "10s")
541645

542646
_, err := loadTemplateVersionBuildWaitConfigFromEnv()
647+
if err == nil {
648+
t.Fatal("expected max wait timeout validation to fail")
649+
}
650+
if !strings.Contains(err.Error(), templateVersionBuildWaitTimeoutEnv) {
651+
t.Fatalf("expected error to mention %s, got %v", templateVersionBuildWaitTimeoutEnv, err)
652+
}
653+
654+
t.Setenv(templateVersionBuildWaitTimeoutEnv, "1m")
655+
t.Setenv(templateVersionBuildBackoffAfterEnv, "2m")
656+
657+
_, err = loadTemplateVersionBuildWaitConfigFromEnv()
543658
if err == nil {
544659
t.Fatal("expected invalid wait config to fail")
545660
}

‎internal/aggregated/storage/template.go‎

Lines changed: 68 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ const (
3232
defaultTemplateVersionBuildBackoffAfter = 2 * time.Minute
3333
defaultTemplateVersionBuildInitialPoll = 2 * time.Second
3434
defaultTemplateVersionBuildMaxPollInterval = 10 * time.Second
35+
// MaxTemplateVersionBuildWaitTimeout keeps template build waits within aggregated API request deadlines.
36+
MaxTemplateVersionBuildWaitTimeout = 30 * time.Minute
3537
)
3638

3739
type templateVersionBuildWaitConfig struct {
@@ -50,6 +52,8 @@ var (
5052
_ rest.GracefulDeleter = (*TemplateStorage)(nil)
5153
_ rest.Scoper = (*TemplateStorage)(nil)
5254
_ rest.SingularNameProvider = (*TemplateStorage)(nil)
55+
56+
errTemplateVersionBuildWaitTimeoutExceeded = errors.New("template version build wait timeout exceeded")
5357
)
5458

5559
// TemplateStorage provides codersdk-backed CoderTemplate objects.
@@ -687,7 +691,14 @@ func mapTemplateVersionBuildWaitError(waitErr error, templateName string) error
687691

688692
var timeoutErr *templateVersionBuildWaitTimeoutError
689693
if errors.As(waitErr, &timeoutErr) {
690-
return apierrors.NewBadRequest(timeoutErr.Error())
694+
switch {
695+
case errors.Is(timeoutErr.cause, errTemplateVersionBuildWaitTimeoutExceeded):
696+
return apierrors.NewBadRequest(timeoutErr.Error())
697+
case errors.Is(timeoutErr.cause, context.DeadlineExceeded), errors.Is(timeoutErr.cause, context.Canceled):
698+
return apierrors.NewTimeoutError(timeoutErr.Error(), 0)
699+
default:
700+
return apierrors.NewInternalError(timeoutErr)
701+
}
691702
}
692703

693704
return coder.MapCoderError(waitErr, aggregationv1alpha1.Resource("codertemplates"), templateName)
@@ -709,7 +720,15 @@ func waitForTemplateVersionBuild(ctx context.Context, sdk *codersdk.Client, vers
709720
return waitConfigErr
710721
}
711722

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

715734
pollInterval := waitConfig.initialPoll
@@ -721,12 +740,17 @@ func waitForTemplateVersionBuild(ctx context.Context, sdk *codersdk.Client, vers
721740
version, err := sdk.TemplateVersion(waitCtx, versionID)
722741
if err != nil {
723742
if waitCtx.Err() != nil {
743+
timeoutCause, timeoutCauseErr := templateVersionBuildWaitTimeoutCause(ctx, waitCtx)
744+
if timeoutCauseErr != nil {
745+
return timeoutCauseErr
746+
}
747+
724748
return &templateVersionBuildWaitTimeoutError{
725749
versionID: versionID,
726-
waitTimeout: waitConfig.waitTimeout,
750+
waitTimeout: effectiveWaitTimeout,
727751
lastStatus: lastStatus,
728752
lastError: lastError,
729-
cause: waitCtx.Err(),
753+
cause: timeoutCause,
730754
}
731755
}
732756
return fmt.Errorf("fetch template version %q: %w", versionID.String(), err)
@@ -771,21 +795,47 @@ func waitForTemplateVersionBuild(ctx context.Context, sdk *codersdk.Client, vers
771795

772796
select {
773797
case <-waitCtx.Done():
774-
if waitCtx.Err() == nil {
775-
return fmt.Errorf("assertion failed: wait context finished without an error")
798+
timeoutCause, timeoutCauseErr := templateVersionBuildWaitTimeoutCause(ctx, waitCtx)
799+
if timeoutCauseErr != nil {
800+
return timeoutCauseErr
776801
}
802+
777803
return &templateVersionBuildWaitTimeoutError{
778804
versionID: versionID,
779-
waitTimeout: waitConfig.waitTimeout,
805+
waitTimeout: effectiveWaitTimeout,
780806
lastStatus: lastStatus,
781807
lastError: lastError,
782-
cause: waitCtx.Err(),
808+
cause: timeoutCause,
783809
}
784810
case <-time.After(pollInterval):
785811
}
786812
}
787813
}
788814

815+
func templateVersionBuildWaitTimeoutCause(requestCtx, waitCtx context.Context) (error, error) {
816+
if requestCtx == nil {
817+
return nil, fmt.Errorf("assertion failed: request context must not be nil")
818+
}
819+
if waitCtx == nil {
820+
return nil, fmt.Errorf("assertion failed: wait context must not be nil")
821+
}
822+
823+
waitErr := waitCtx.Err()
824+
if waitErr == nil {
825+
return nil, fmt.Errorf("assertion failed: wait context finished without an error")
826+
}
827+
828+
if requestErr := requestCtx.Err(); requestErr != nil {
829+
return requestErr, nil
830+
}
831+
832+
if errors.Is(waitErr, context.DeadlineExceeded) {
833+
return errTemplateVersionBuildWaitTimeoutExceeded, nil
834+
}
835+
836+
return waitErr, nil
837+
}
838+
789839
func loadTemplateVersionBuildWaitConfigFromEnv() (templateVersionBuildWaitConfig, error) {
790840
waitTimeout, waitTimeoutErr := parseDurationEnvOrDefault(templateVersionBuildWaitTimeoutEnv, defaultTemplateVersionBuildWaitTimeout)
791841
if waitTimeoutErr != nil {
@@ -799,6 +849,16 @@ func loadTemplateVersionBuildWaitConfigFromEnv() (templateVersionBuildWaitConfig
799849
)
800850
}
801851

852+
if waitTimeout > MaxTemplateVersionBuildWaitTimeout {
853+
return templateVersionBuildWaitConfig{}, fmt.Errorf(
854+
"assertion failed: %s (%s) must be <= %s (%s)",
855+
templateVersionBuildWaitTimeoutEnv,
856+
waitTimeout,
857+
"aggregated API request timeout",
858+
MaxTemplateVersionBuildWaitTimeout,
859+
)
860+
}
861+
802862
backoffAfter, backoffAfterErr := parseDurationEnvOrDefault(templateVersionBuildBackoffAfterEnv, defaultTemplateVersionBuildBackoffAfter)
803863
if backoffAfterErr != nil {
804864
return templateVersionBuildWaitConfig{}, backoffAfterErr

‎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 aligned with template build wait limits.
42+
defaultRequestTimeout = storage.MaxTemplateVersionBuildWaitTimeout
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)