From 208ff429159b2ef2a1e13054492c8d8fd83ca75b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 13 Oct 2026 20:22:06 +0000 Subject: [PATCH] Preserve caller cancellation in HTTP smoke probes --- docs/roadmap/remediation.md | 11 ++- internal/usecase/smoke_authentication.go | 6 +- internal/usecase/smoke_reachability.go | 12 ++- internal/usecase/smoke_tester_test.go | 102 ++++++++++++++++++----- 4 files changed, 104 insertions(+), 27 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 4e6e9ca..eeb8b4d 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Smoke Test Implementation Review Findings -Status: three open findings. Review completed 2026-10-13. +Status: one finding completed; two open. Review completed 2026-10-13. Reviewed implementation commits after `8fbd058` through `2f735aedf9b5dcc98164292db46ae1eab3e15cde`, against the @@ -16,7 +16,7 @@ below are reporting-correctness gaps, not reasons to redesign the feature. ## Finding 1: Preserve Caller Cancellation Independently Of Body Error Type -Priority: P2. Status: open. +Priority: P2. Status: completed 2026-10-13. Locations: [`finishHTTP`](../../internal/usecase/smoke_tester.go), lines 355–366, and [`runOpenRouterKey`](../../internal/usecase/smoke_authentication.go), @@ -47,6 +47,13 @@ that cancels the caller and returns a non-context I/O error. Its existing fake returns `context.Canceled`, which masks this defect. Check partial-report and context identity rather than exact error text; reuse existing timeout coverage. +Completion: HTTP probe failures now check the caller context at the failed-I/O +boundary before classification, preserving partial response evidence even for +opaque transport/body errors. Completed authentication is not revoked when body +closure subsequently cancels the caller. The new regressions failed before the +fix; `GOWORK=off go test -race ./internal/usecase .`, focused use-case vet, and +`git diff --check` passed. Existing configured-timeout coverage remains green. + ## Finding 2: Report The Actual Initial Inference Request Destination Priority: P2. Status: open. diff --git a/internal/usecase/smoke_authentication.go b/internal/usecase/smoke_authentication.go index dceb8fc..5187766 100644 --- a/internal/usecase/smoke_authentication.go +++ b/internal/usecase/smoke_authentication.go @@ -106,7 +106,7 @@ func (p authenticationProbe) runOpenRouterKey( } recognized, bodyErr := recognizedOpenRouterKeyResponse(response.Body) if bodyErr != nil { - outcome.failure = generation.ClassifyFailure(fmt.Errorf("%w: %w", llm.ErrTransport, bodyErr)) + outcome.failure = httpProbeFailure(ctx, bodyErr) return outcome, nil } if recognized { @@ -123,9 +123,9 @@ func (p authenticationProbe) runOpenRouterKey( if evidence.DispatchInvoked { if requestErr != nil { - outcome.failure = generation.ClassifyFailure(fmt.Errorf("%w: %w", llm.ErrTransport, requestErr)) + outcome.failure = httpProbeFailure(ctx, requestErr) } else { - outcome.failure = generation.ClassifyFailure(llm.ErrTransport) + outcome.failure = httpProbeFailure(ctx, nil) } return outcome, nil } diff --git a/internal/usecase/smoke_reachability.go b/internal/usecase/smoke_reachability.go index ffddf70..1726486 100644 --- a/internal/usecase/smoke_reachability.go +++ b/internal/usecase/smoke_reachability.go @@ -2,6 +2,7 @@ package usecase import ( "context" + "errors" "fmt" "net/http" "net/url" @@ -58,7 +59,7 @@ func (p reachabilityProbe) run( result.reachable = boolPointer(false) } if requestErr != nil { - result.failure = generation.ClassifyFailure(fmt.Errorf("%w: %w", llm.ErrTransport, requestErr)) + result.failure = httpProbeFailure(ctx, requestErr) if !evidence.DispatchInvoked { return result, fmt.Errorf("execute reachability request: %w", requestErr) } @@ -66,6 +67,15 @@ func (p reachabilityProbe) run( return result, nil } +// Failed I/O can lose context error identity. Check the caller at that boundary, +// without reclassifying a probe that already established its result. +func httpProbeFailure(ctx context.Context, err error) domain.ExecutionFailureKind { + if contextErr := ctx.Err(); contextErr != nil { + return generation.ClassifyFailure(contextErr) + } + return generation.ClassifyFailure(errors.Join(llm.ErrTransport, err)) +} + func endpointWithoutUserInfo(endpoint string) (string, error) { parsed, err := url.Parse(endpoint) if err != nil { diff --git a/internal/usecase/smoke_tester_test.go b/internal/usecase/smoke_tester_test.go index f7e93fb..87afd2c 100644 --- a/internal/usecase/smoke_tester_test.go +++ b/internal/usecase/smoke_tester_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "net/http" "strings" "testing" @@ -263,31 +264,84 @@ func TestSmokeTesterConfiguredTimeoutConcludesWithReport(t *testing.T) { } func TestSmokeTesterRetainsHTTPResponseEvidenceWhenCallerCancelsDuringBodyRead(t *testing.T) { + for _, readErr := range []error{context.Canceled, io.ErrUnexpectedEOF} { + t.Run(readErr.Error(), func(t *testing.T) { + runner, _ := newSmokeSelectionRunner(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + client := reachabilityClient(t, func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: cancelingSmokeBody{cancel: cancel, err: readErr}, + }, nil + }) + + report, err := NewSmokeTester(runner, client).Run(ctx, domain.SmokeTestRequest{ + Kind: domain.SmokeTestAuthentication, BackendID: "beta", APIKey: "synthetic-secret", + }) + if !errors.Is(err, context.Canceled) || report == nil || report.Completed || report.Failure != domain.ExecutionFailureCanceled { + t.Fatalf("partial report=%+v error=%v", report, err) + } + if report.Facts.ResponseReceived == nil || !*report.Facts.ResponseReceived || + report.Facts.EndpointReachable == nil || !*report.Facts.EndpointReachable || + report.Facts.CredentialsSupplied == nil || !*report.Facts.CredentialsSupplied || + report.Facts.CredentialsAccepted != nil || len(report.Attempts) != 1 || + report.Attempts[0].HTTPStatus == nil || *report.Attempts[0].HTTPStatus != http.StatusOK { + t.Fatalf("partial HTTP evidence = %+v", report) + } + if strings.Contains(fmt.Sprintf("%+v", report), "synthetic-secret") { + t.Fatalf("partial report retained credential: %+v", report) + } + }) + } +} + +func TestSmokeTesterRecognizesCallerCancellationWithOpaqueHTTPError(t *testing.T) { + for _, kind := range []domain.SmokeTestKind{domain.SmokeTestReachability, domain.SmokeTestAuthentication} { + t.Run(string(kind), func(t *testing.T) { + runner, _ := newSmokeSelectionRunner(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + client := reachabilityClient(t, func(*http.Request) (*http.Response, error) { + cancel() + return nil, io.ErrUnexpectedEOF + }) + report, err := NewSmokeTester(runner, client).Run(ctx, domain.SmokeTestRequest{ + Kind: kind, BackendID: "beta", + }) + if !errors.Is(err, context.Canceled) || report == nil || report.Completed || report.Failure != domain.ExecutionFailureCanceled { + t.Fatalf("partial report=%+v error=%v", report, err) + } + if len(report.Attempts) != 1 || report.Attempts[0].HTTPStatus != nil || + report.Facts.ResponseReceived == nil || *report.Facts.ResponseReceived || + report.Facts.CredentialsAccepted != nil { + t.Fatalf("partial HTTP evidence = %+v", report) + } + }) + } +} + +func TestSmokeTesterKeepsCompletedAuthenticationWhenCloseCancelsCaller(t *testing.T) { runner, _ := newSmokeSelectionRunner(t) ctx, cancel := context.WithCancel(context.Background()) + defer cancel() client := reachabilityClient(t, func(*http.Request) (*http.Response, error) { return &http.Response{ StatusCode: http.StatusOK, Header: make(http.Header), - Body: cancelingSmokeBody{cancel: cancel}, + Body: cancelOnCloseSmokeBody{ + ReadCloser: io.NopCloser(strings.NewReader(`{"data":{"label":"member","is_free_tier":false,"usage":1}}`)), + cancel: cancel, + }, }, nil }) - report, err := NewSmokeTester(runner, client).Run(ctx, domain.SmokeTestRequest{ - Kind: domain.SmokeTestAuthentication, BackendID: "beta", APIKey: "synthetic-secret", + Kind: domain.SmokeTestAuthentication, BackendID: "beta", }) - if !errors.Is(err, context.Canceled) || report == nil || report.Completed || report.Failure != domain.ExecutionFailureCanceled { - t.Fatalf("partial report=%+v error=%v", report, err) - } - if report.Facts.ResponseReceived == nil || !*report.Facts.ResponseReceived || - report.Facts.EndpointReachable == nil || !*report.Facts.EndpointReachable || - report.Facts.CredentialsSupplied == nil || !*report.Facts.CredentialsSupplied || - report.Facts.CredentialsAccepted != nil || len(report.Attempts) != 1 || - report.Attempts[0].HTTPStatus == nil || *report.Attempts[0].HTTPStatus != http.StatusOK { - t.Fatalf("partial HTTP evidence = %+v", report) - } - if strings.Contains(fmt.Sprintf("%+v", report), "synthetic-secret") { - t.Fatalf("partial report retained credential: %+v", report) + if !errors.Is(ctx.Err(), context.Canceled) || err != nil || report == nil || !report.Completed || + report.Failure != domain.ExecutionFailureNone || report.Facts.CredentialsAccepted == nil || !*report.Facts.CredentialsAccepted { + t.Fatalf("completed report=%+v error=%v context error=%v", report, err, ctx.Err()) } } @@ -317,16 +371,22 @@ func (a *smokeAdmissionSpy) Admit(context.Context, string) (func(), error) { type cancelingSmokeBody struct { cancel context.CancelFunc - read bool + err error } func (b cancelingSmokeBody) Read(buffer []byte) (int, error) { - if b.read { - return 0, context.Canceled - } - b.read = true b.cancel() - return copy(buffer, []byte(`{"data":{"label":"member","is_free_tier":false,"usage":1}}`)), context.Canceled + return copy(buffer, []byte(`{"data":{"label":"member","is_free_tier":false,"usage":1}}`)), b.err } func (cancelingSmokeBody) Close() error { return nil } + +type cancelOnCloseSmokeBody struct { + io.ReadCloser + cancel context.CancelFunc +} + +func (b cancelOnCloseSmokeBody) Close() error { + b.cancel() + return b.ReadCloser.Close() +}