From da1eb2793dbe6c87f70b442465dc26097be97f47 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 15:13:53 +0200 Subject: [PATCH 1/3] fix(cli): treat cancelled context as terminal in engine-version fan-out queryMajorEngineVersionsWithClient downgraded every per-engine error to a warning, so a cancelled context or expired deadline produced a partial version map with a nil error and callers treated it as a complete query. The loop now checks ctx.Err() before each engine and after each failed fetch, returning nil plus the context error instead of continuing. The caller's context is checked rather than the wrapped API error so SDK-internal timeouts (e.g. credential-fetch deadlines) still degrade to the pre-existing per-engine warning. Closes #1325 --- cmd/multi_service_engine_versions.go | 8 ++ ...i_service_engine_versions_paginate_test.go | 93 +++++++++++++++++++ 2 files changed, 101 insertions(+) diff --git a/cmd/multi_service_engine_versions.go b/cmd/multi_service_engine_versions.go index 85c1bef00..a79737580 100644 --- a/cmd/multi_service_engine_versions.go +++ b/cmd/multi_service_engine_versions.go @@ -218,7 +218,15 @@ func queryMajorEngineVersionsWithClient(ctx context.Context, rdsClient RDSMajorV engines := []string{"mysql", "postgres", "aurora-mysql", "aurora-postgresql"} for _, engine := range engines { + if err := ctx.Err(); err != nil { + return nil, err + } if err := fetchMajorEngineVersionsForEngine(ctx, rdsClient, engine, versionInfo); err != nil { + // Cancelled caller context is terminal (issue #1325); check ctx.Err(), + // not the wrapped API error, so SDK-internal timeouts stay warnings. + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } log.Printf("Warning: Failed to describe major engine versions for %s: %v", engine, err) } } diff --git a/cmd/multi_service_engine_versions_paginate_test.go b/cmd/multi_service_engine_versions_paginate_test.go index c8ad2296a..8c60bc400 100644 --- a/cmd/multi_service_engine_versions_paginate_test.go +++ b/cmd/multi_service_engine_versions_paginate_test.go @@ -151,3 +151,96 @@ func TestFetchMajorEngineVersionsForEngine_PaginationCapError(t *testing.T) { assert.Equal(t, maxEngineVersionPages, mock.calls, "must stop exactly at the cap") } + +// cancelOnFirstQueryRDSMock cancels the context from inside the first API call +// and returns the SDK-shaped error a cancelled request produces, so the fan-out +// loop sees both a real ctx cancellation and a wrapped context error. +type cancelOnFirstQueryRDSMock struct { + cancel context.CancelFunc + enginesQueried []string + wrap func(error) error +} + +func (m *cancelOnFirstQueryRDSMock) DescribeDBMajorEngineVersions( + _ context.Context, + params *awsrds.DescribeDBMajorEngineVersionsInput, + _ ...func(*awsrds.Options), +) (*awsrds.DescribeDBMajorEngineVersionsOutput, error) { + m.enginesQueried = append(m.enginesQueried, aws.ToString(params.Engine)) + m.cancel() + err := error(context.Canceled) + if m.wrap != nil { + err = m.wrap(err) + } + return nil, err +} + +// TestQueryMajorEngineVersionsWithClient_CtxCancelIsTerminal asserts that a +// cancelled context stops the per-engine fan-out instead of being downgraded to +// a warning: the loop must return the error and must not keep querying the +// remaining engines (issue #1325). +func TestQueryMajorEngineVersionsWithClient_CtxCancelIsTerminal(t *testing.T) { + tests := []struct { + name string + wrap func(error) error + }{ + {name: "bare context.Canceled"}, + { + // The AWS SDK wraps transport errors; the mock cancels the real + // context, so detection via ctx.Err() works regardless of wrapping. + name: "SDK-wrapped context.Canceled", + wrap: func(err error) error { + return fmt.Errorf("operation error RDS: DescribeDBMajorEngineVersions, %w", err) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + mock := &cancelOnFirstQueryRDSMock{cancel: cancel, wrap: tt.wrap} + + result, err := queryMajorEngineVersionsWithClient(ctx, mock) + + require.ErrorIs(t, err, context.Canceled, + "a cancelled context must not be reported as a successful query") + assert.Nil(t, result, + "partial version info must not be handed back as if it were complete") + assert.Equal(t, []string{"mysql"}, mock.enginesQueried, + "cancellation must stop the fan-out after the first engine") + }) + } +} + +// TestQueryMajorEngineVersionsWithClient_CtxDeadlineIsTerminal asserts the same +// for an expired deadline (issue #1325): it fails fast before the first call. +func TestQueryMajorEngineVersionsWithClient_CtxDeadlineIsTerminal(t *testing.T) { + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Minute)) + defer cancel() + + mock := &cancelOnFirstQueryRDSMock{cancel: func() {}} + + _, err := queryMajorEngineVersionsWithClient(ctx, mock) + + require.ErrorIs(t, err, context.DeadlineExceeded) + assert.Empty(t, mock.enginesQueried, + "an expired deadline must not issue API calls") +} + +// TestQueryMajorEngineVersionsWithClient_CtxAlreadyCancelled asserts that a +// context cancelled before the call fails fast without spending a single API +// call (issue #1325). +func TestQueryMajorEngineVersionsWithClient_CtxAlreadyCancelled(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + mock := &cancelOnFirstQueryRDSMock{cancel: func() {}} + + _, err := queryMajorEngineVersionsWithClient(ctx, mock) + + require.ErrorIs(t, err, context.Canceled) + assert.Empty(t, mock.enginesQueried, + "an already-cancelled context must not issue API calls") +} From 7d9bfc0127e87da80bc8c8aadf833093b0a4e1ee Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 15:24:12 +0200 Subject: [PATCH 2/3] fix(cli): correct misspellings and redundant conversion flagged by CI lint The golangci-lint misspell and unconvert checks flagged the #1325 test additions (British "cancelled" spellings, error() conversion of context.Canceled). No behavior change. Refs #1325 --- cmd/multi_service_engine_versions.go | 2 +- ...ulti_service_engine_versions_paginate_test.go | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/cmd/multi_service_engine_versions.go b/cmd/multi_service_engine_versions.go index a79737580..4cbd4c251 100644 --- a/cmd/multi_service_engine_versions.go +++ b/cmd/multi_service_engine_versions.go @@ -222,7 +222,7 @@ func queryMajorEngineVersionsWithClient(ctx context.Context, rdsClient RDSMajorV return nil, err } if err := fetchMajorEngineVersionsForEngine(ctx, rdsClient, engine, versionInfo); err != nil { - // Cancelled caller context is terminal (issue #1325); check ctx.Err(), + // Canceled caller context is terminal (issue #1325); check ctx.Err(), // not the wrapped API error, so SDK-internal timeouts stay warnings. if ctxErr := ctx.Err(); ctxErr != nil { return nil, ctxErr diff --git a/cmd/multi_service_engine_versions_paginate_test.go b/cmd/multi_service_engine_versions_paginate_test.go index 8c60bc400..56d6a0113 100644 --- a/cmd/multi_service_engine_versions_paginate_test.go +++ b/cmd/multi_service_engine_versions_paginate_test.go @@ -153,7 +153,7 @@ func TestFetchMajorEngineVersionsForEngine_PaginationCapError(t *testing.T) { } // cancelOnFirstQueryRDSMock cancels the context from inside the first API call -// and returns the SDK-shaped error a cancelled request produces, so the fan-out +// and returns the SDK-shaped error a canceled request produces, so the fan-out // loop sees both a real ctx cancellation and a wrapped context error. type cancelOnFirstQueryRDSMock struct { cancel context.CancelFunc @@ -168,7 +168,7 @@ func (m *cancelOnFirstQueryRDSMock) DescribeDBMajorEngineVersions( ) (*awsrds.DescribeDBMajorEngineVersionsOutput, error) { m.enginesQueried = append(m.enginesQueried, aws.ToString(params.Engine)) m.cancel() - err := error(context.Canceled) + err := context.Canceled if m.wrap != nil { err = m.wrap(err) } @@ -176,7 +176,7 @@ func (m *cancelOnFirstQueryRDSMock) DescribeDBMajorEngineVersions( } // TestQueryMajorEngineVersionsWithClient_CtxCancelIsTerminal asserts that a -// cancelled context stops the per-engine fan-out instead of being downgraded to +// canceled context stops the per-engine fan-out instead of being downgraded to // a warning: the loop must return the error and must not keep querying the // remaining engines (issue #1325). func TestQueryMajorEngineVersionsWithClient_CtxCancelIsTerminal(t *testing.T) { @@ -205,7 +205,7 @@ func TestQueryMajorEngineVersionsWithClient_CtxCancelIsTerminal(t *testing.T) { result, err := queryMajorEngineVersionsWithClient(ctx, mock) require.ErrorIs(t, err, context.Canceled, - "a cancelled context must not be reported as a successful query") + "a canceled context must not be reported as a successful query") assert.Nil(t, result, "partial version info must not be handed back as if it were complete") assert.Equal(t, []string{"mysql"}, mock.enginesQueried, @@ -229,10 +229,10 @@ func TestQueryMajorEngineVersionsWithClient_CtxDeadlineIsTerminal(t *testing.T) "an expired deadline must not issue API calls") } -// TestQueryMajorEngineVersionsWithClient_CtxAlreadyCancelled asserts that a -// context cancelled before the call fails fast without spending a single API +// TestQueryMajorEngineVersionsWithClient_CtxAlreadyCanceled asserts that a +// context canceled before the call fails fast without spending a single API // call (issue #1325). -func TestQueryMajorEngineVersionsWithClient_CtxAlreadyCancelled(t *testing.T) { +func TestQueryMajorEngineVersionsWithClient_CtxAlreadyCanceled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -242,5 +242,5 @@ func TestQueryMajorEngineVersionsWithClient_CtxAlreadyCancelled(t *testing.T) { require.ErrorIs(t, err, context.Canceled) assert.Empty(t, mock.enginesQueried, - "an already-cancelled context must not issue API calls") + "an already-canceled context must not issue API calls") } From cb6e9e344fa2b5946c40a2df9963c8ced9422c5a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 8 Oct 2026 02:43:21 +0200 Subject: [PATCH 3/3] fix(cli): reject cancellation after the final engine fetch Return the caller context error and discard lifecycle data when the last successful RDS response arrives after cancellation. Pin the four-engine success case with a regression that fails on the previous source. --- cmd/multi_service_engine_versions.go | 3 ++ ...i_service_engine_versions_paginate_test.go | 44 ++++++++++++++----- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/cmd/multi_service_engine_versions.go b/cmd/multi_service_engine_versions.go index 4cbd4c251..891dc88e0 100644 --- a/cmd/multi_service_engine_versions.go +++ b/cmd/multi_service_engine_versions.go @@ -231,6 +231,9 @@ func queryMajorEngineVersionsWithClient(ctx context.Context, rdsClient RDSMajorV } } + if err := ctx.Err(); err != nil { + return nil, err + } return versionInfo, nil } diff --git a/cmd/multi_service_engine_versions_paginate_test.go b/cmd/multi_service_engine_versions_paginate_test.go index 56d6a0113..97c6f5f4b 100644 --- a/cmd/multi_service_engine_versions_paginate_test.go +++ b/cmd/multi_service_engine_versions_paginate_test.go @@ -152,9 +152,7 @@ func TestFetchMajorEngineVersionsForEngine_PaginationCapError(t *testing.T) { "must stop exactly at the cap") } -// cancelOnFirstQueryRDSMock cancels the context from inside the first API call -// and returns the SDK-shaped error a canceled request produces, so the fan-out -// loop sees both a real ctx cancellation and a wrapped context error. +// The mock cancels the caller context, including when returning a wrapped SDK error. type cancelOnFirstQueryRDSMock struct { cancel context.CancelFunc enginesQueried []string @@ -175,10 +173,7 @@ func (m *cancelOnFirstQueryRDSMock) DescribeDBMajorEngineVersions( return nil, err } -// TestQueryMajorEngineVersionsWithClient_CtxCancelIsTerminal asserts that a -// canceled context stops the per-engine fan-out instead of being downgraded to -// a warning: the loop must return the error and must not keep querying the -// remaining engines (issue #1325). +// Caller cancellation must remain terminal even when the SDK wraps the error. func TestQueryMajorEngineVersionsWithClient_CtxCancelIsTerminal(t *testing.T) { tests := []struct { name string @@ -229,9 +224,6 @@ func TestQueryMajorEngineVersionsWithClient_CtxDeadlineIsTerminal(t *testing.T) "an expired deadline must not issue API calls") } -// TestQueryMajorEngineVersionsWithClient_CtxAlreadyCanceled asserts that a -// context canceled before the call fails fast without spending a single API -// call (issue #1325). func TestQueryMajorEngineVersionsWithClient_CtxAlreadyCanceled(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -244,3 +236,35 @@ func TestQueryMajorEngineVersionsWithClient_CtxAlreadyCanceled(t *testing.T) { assert.Empty(t, mock.enginesQueried, "an already-canceled context must not issue API calls") } + +type cancelOnFinalSuccessRDSMock struct { + cancel context.CancelFunc + enginesQueried []string +} + +func (m *cancelOnFinalSuccessRDSMock) DescribeDBMajorEngineVersions( + _ context.Context, + params *awsrds.DescribeDBMajorEngineVersionsInput, + _ ...func(*awsrds.Options), +) (*awsrds.DescribeDBMajorEngineVersionsOutput, error) { + engine := aws.ToString(params.Engine) + m.enginesQueried = append(m.enginesQueried, engine) + if engine == "aurora-postgresql" { + m.cancel() + } + return &awsrds.DescribeDBMajorEngineVersionsOutput{ + DBMajorEngineVersions: []rdstypes.DBMajorEngineVersion{rdsMajorVersion(engine, "8.0")}, + }, nil +} + +func TestQueryMajorEngineVersionsWithClient_CtxCanceledOnFinalSuccess(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + mock := &cancelOnFinalSuccessRDSMock{cancel: cancel} + + result, err := queryMajorEngineVersionsWithClient(ctx, mock) + + assert.ErrorIs(t, err, context.Canceled) + assert.Nil(t, result, "cancellation must discard the accumulated lifecycle data") + assert.Equal(t, []string{"mysql", "postgres", "aurora-mysql", "aurora-postgresql"}, mock.enginesQueried) +}