diff --git a/docs/contributing/go-code.md b/docs/contributing/go-code.md index c8979109f7..408838b4c1 100644 --- a/docs/contributing/go-code.md +++ b/docs/contributing/go-code.md @@ -374,11 +374,13 @@ if errors.As(err, &te) && te.Timeout() { Context deadline and cancellation errors represent intentional cancellation by the caller (e.g., a request timeout set by the application, a user-initiated cancel). They should never be classified as transient or retried — the caller chose to stop waiting, and retrying re-creates the same deadline. -**Always guard against context errors before checking `Timeout()`:** +**Do not use `errors.Is(err, context.DeadlineExceeded)` to exclude context errors.** Go's `net/http` Client.Timeout error unwraps to `context.DeadlineExceeded` from an internal context even when the caller's context is still live, so `errors.Is` also matches genuine transport timeouts. The same false positive appears once an intermediate type implements `Unwrap` (for example `mintclient.retryableError`). That misclassification shipped in #7234 and was the third occurrence of this pitfall (#6424, #6425, #7240). + +**Always use `ctxerr.IsDeadlineExceededOrCanceled` (which checks `ctx.Err()`) before checking `Timeout()`:** ```go -// CORRECT — context errors are excluded before the Timeout() check. -if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { +// CORRECT — only the caller's own context expiry is excluded. +if ctxerr.IsDeadlineExceededOrCanceled(ctx, err) { return false } var te interface{ Timeout() bool } @@ -387,9 +389,9 @@ if errors.As(err, &te) && te.Timeout() { } ``` -See [`forge.IsTransient`](../../internal/forge/forge.go) for the canonical example of the correct pattern. +See [`ctxerr.IsDeadlineExceededOrCanceled`](../../internal/ctxerr/ctxerr.go) for the canonical helper, and [`forge.IsTransient`](../../internal/forge/forge.go) for it used in retry classification. -**When reviewing PRs:** Flag any `Timeout() bool` interface assertion without a preceding `errors.Is(err, context.DeadlineExceeded)` guard as a medium-severity finding. The fix is to add the context-error check before the `Timeout()` check. +**When reviewing PRs:** Flag any `Timeout() bool` interface assertion without a preceding `ctxerr.IsDeadlineExceededOrCanceled` (or `ctx.Err() != nil`) guard as a medium-severity finding. Flag any new `errors.Is(err, context.DeadlineExceeded)` call site that does not go through `ctxerr` as a medium-severity finding. The fix is to call the helper. ### Template map iteration diff --git a/internal/appsetup/appsetup.go b/internal/appsetup/appsetup.go index cd824bb435..a38c250a71 100644 --- a/internal/appsetup/appsetup.go +++ b/internal/appsetup/appsetup.go @@ -11,7 +11,6 @@ import ( "crypto/x509" "encoding/json" "encoding/pem" - "errors" "fmt" "html" "net" @@ -25,6 +24,7 @@ import ( "strings" "time" + "github.com/fullsend-ai/fullsend/internal/ctxerr" "github.com/fullsend-ai/fullsend/internal/forge" ghTypes "github.com/fullsend-ai/fullsend/internal/forge/github" "github.com/fullsend-ai/fullsend/internal/mintcore" @@ -989,7 +989,9 @@ func (s *Setup) ensureInstalled(ctx context.Context, org, slug string) error { if err := s.waitForAppReady(ctx, ghExt, slug); err != nil { // waitForAppReady returns ctx.Err() for parent cancellation, so // context errors propagate directly without a separate guard. - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + // Use ctxerr rather than errors.Is: a transport timeout can + // unwrap to DeadlineExceeded while this ctx is still live. + if ctxerr.IsDeadlineExceededOrCanceled(ctx, err) { return err } s.ui.StepWarn(fmt.Sprintf("App readiness check failed: %v", err)) diff --git a/internal/cli/run.go b/internal/cli/run.go index 0036a8dee0..cbd81d5746 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -30,6 +30,7 @@ import ( "github.com/fullsend-ai/fullsend/internal/binary" "github.com/fullsend-ai/fullsend/internal/config" + "github.com/fullsend-ai/fullsend/internal/ctxerr" "github.com/fullsend-ai/fullsend/internal/envfile" "github.com/fullsend-ai/fullsend/internal/evalmeasure" "github.com/fullsend-ai/fullsend/internal/fetch" @@ -5191,11 +5192,14 @@ func remintAgentTokenForPostScript(ctx context.Context, h *harness.Harness, mint } _, cleanup, err := mintAgentToken(remintCtx, role, mintURL, forgePlatform, printer) if err != nil { - if errors.Is(err, context.DeadlineExceeded) { + if ctxerr.IsDeadlineExceededOrCanceled(remintCtx, err) { // Distinct from a genuine mint rejection: the client's own // retry schedule (see mintclient.MaxMintDuration) did not get // to run to completion within remintForPostScriptTimeout, so // this is a truncated retry, not a confirmed failure. + // Check remintCtx, not err, because net/http Client.Timeout + // and mintclient.retryableError unwrap to DeadlineExceeded + // even when this remint context is still live (#7240). printer.StepWarn(fmt.Sprintf("Refreshing agent token for post-script timed out after %s; continuing with existing token", remintForPostScriptTimeout)) } else { printer.StepWarn("Failed to refresh agent token for post-script: " + err.Error() + "; continuing with existing token") diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 2db8d9be7b..88588a348f 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -5784,6 +5784,46 @@ func TestRemintAgentTokenForPostScript_DeadlineExceededGetsDistinctWarning(t *te assert.Equal(t, "existing_token", h.RunnerEnv["PUSH_TOKEN"], "RunnerEnv must be untouched when remint times out") } +// remintHTTPTimeoutErr mimics net/http Client.Timeout (Unwrap to +// DeadlineExceeded + Timeout() bool) wrapped the way mintclient.retryableError +// does. errors.Is against DeadlineExceeded matches it even when the remint +// context is still live — the #7240 misclassification. +type remintHTTPTimeoutErr struct { + error +} + +func (e *remintHTTPTimeoutErr) Timeout() bool { return true } +func (e *remintHTTPTimeoutErr) Unwrap() error { return e.error } + +// TestRemintAgentTokenForPostScript_HTTPTimeoutIsNotDeadlineWarning proves a +// transport timeout that unwraps to DeadlineExceeded is reported as a generic +// remint failure, not as truncation by remintForPostScriptTimeout, when the +// remint context itself is still live. +func TestRemintAgentTokenForPostScript_HTTPTimeoutIsNotDeadlineWarning(t *testing.T) { + origMint := statusMintToken + defer func() { statusMintToken = origMint }() + statusMintToken = func(_ context.Context, _ mintclient.MintRequest) (*mintclient.MintResult, error) { + return nil, &remintHTTPTimeoutErr{error: context.DeadlineExceeded} + } + + t.Setenv("REPO_FULL_NAME", "org/my-repo") + t.Setenv("GH_TOKEN", "") + t.Setenv("PUSH_TOKEN", "") + t.Setenv("PUSH_TOKEN_SOURCE", "") + + h := &harness.Harness{Role: "coder", RunnerEnv: map[string]string{"PUSH_TOKEN": "existing_token"}} + + var buf bytes.Buffer + printer := ui.New(&buf) + + cleanup := remintAgentTokenForPostScript(context.Background(), h, "https://mint.example.com", "", printer) + defer cleanup() + + assert.Contains(t, buf.String(), "Failed to refresh agent token for post-script", "a nested HTTP timeout must use the generic failure message") + assert.NotContains(t, buf.String(), "timed out", "must not misreport a transport timeout as remint-context truncation") + assert.Equal(t, "existing_token", h.RunnerEnv["PUSH_TOKEN"], "RunnerEnv must be untouched when remint fails") +} + func TestRemintAgentTokenForPostScript_ErrorIsNonFatal(t *testing.T) { origMint := statusMintToken defer func() { statusMintToken = origMint }() diff --git a/internal/ctxerr/ctxerr.go b/internal/ctxerr/ctxerr.go new file mode 100644 index 0000000000..5193e25cf6 --- /dev/null +++ b/internal/ctxerr/ctxerr.go @@ -0,0 +1,28 @@ +// Package ctxerr provides a single predicate for detecting whether an +// operation failed because the caller's own context was canceled or +// exceeded its deadline. +// +// Prefer this over errors.Is against context.DeadlineExceeded. Go's +// net/http Client.Timeout error unwraps to context.DeadlineExceeded +// from an internal context even when the caller's context is still +// live, so errors.Is cannot tell a caller-context expiry from a +// transport timeout. The same false positive appears once an +// intermediate type (for example mintclient.retryableError) implements +// Unwrap. See #6424, #6425, and #7240. +package ctxerr + +import "context" + +// IsDeadlineExceededOrCanceled reports whether err is non-nil and ctx +// itself is done (canceled or past its deadline). +// +// A nil err is never a context failure. A live ctx is never reported +// as expired, even when err wraps context.DeadlineExceeded or +// implements Timeout() bool — those are nested timeouts (for example +// net/http Client.Timeout), not this call's context. +func IsDeadlineExceededOrCanceled(ctx context.Context, err error) bool { + if err == nil { + return false + } + return ctx.Err() != nil +} diff --git a/internal/ctxerr/ctxerr_test.go b/internal/ctxerr/ctxerr_test.go new file mode 100644 index 0000000000..e28a14eb09 --- /dev/null +++ b/internal/ctxerr/ctxerr_test.go @@ -0,0 +1,150 @@ +package ctxerr + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +// httpTimeoutErr mimics net/http's Client.Timeout error: it unwraps to +// context.DeadlineExceeded and implements Timeout() bool. errors.Is +// against DeadlineExceeded matches it even when the caller's context +// is still live — the false positive this helper exists to prevent. +type httpTimeoutErr struct { + err error +} + +func (e *httpTimeoutErr) Error() string { + return e.err.Error() + " (Client.Timeout exceeded while awaiting headers)" +} + +func (e *httpTimeoutErr) Unwrap() error { return e.err } + +func (e *httpTimeoutErr) Timeout() bool { return true } + +// retryableWrapper mimics mintclient.retryableError: Unwrap only, no +// Timeout() method. After Unwrap is added, errors.Is against +// DeadlineExceeded matches a wrapped transport timeout. +type retryableWrapper struct { + error +} + +func (e *retryableWrapper) Unwrap() error { return e.error } + +func TestIsDeadlineExceededOrCanceled(t *testing.T) { + t.Parallel() + + cancelledCtx, cancel := context.WithCancel(context.Background()) + cancel() + + expiredCtx, expire := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + t.Cleanup(expire) + + liveDE := context.DeadlineExceeded + wrappedDE := fmt.Errorf("request failed: %w", context.DeadlineExceeded) + httpTimeout := &httpTimeoutErr{err: context.DeadlineExceeded} + retryableTimeout := &retryableWrapper{error: httpTimeout} + + tests := []struct { + name string + ctx context.Context + err error + want bool + }{ + { + name: "nil error with live context", + ctx: context.Background(), + err: nil, + want: false, + }, + { + name: "nil error with cancelled context", + ctx: cancelledCtx, + err: nil, + want: false, + }, + { + name: "generic error with live context", + ctx: context.Background(), + err: errors.New("mint rejected"), + want: false, + }, + { + name: "generic error with cancelled context is still context-done", + ctx: cancelledCtx, + err: errors.New("mint rejected"), + want: true, + }, + { + name: "DeadlineExceeded with live context is not this call's expiry", + ctx: context.Background(), + err: liveDE, + want: false, + }, + { + name: "wrapped DeadlineExceeded with live context is not this call's expiry", + ctx: context.Background(), + err: wrappedDE, + want: false, + }, + { + name: "http Client.Timeout wrapping DeadlineExceeded with live context", + ctx: context.Background(), + err: httpTimeout, + want: false, + }, + { + name: "retryable Unwrap of http Client.Timeout with live context", + ctx: context.Background(), + err: retryableTimeout, + want: false, + }, + { + name: "Canceled with live context is not this call's expiry", + ctx: context.Background(), + err: context.Canceled, + want: false, + }, + { + name: "DeadlineExceeded with expired context", + ctx: expiredCtx, + err: liveDE, + want: true, + }, + { + name: "wrapped DeadlineExceeded with expired context", + ctx: expiredCtx, + err: wrappedDE, + want: true, + }, + { + name: "http Client.Timeout with expired context is this call's expiry", + ctx: expiredCtx, + err: httpTimeout, + want: true, + }, + { + name: "Canceled with cancelled context", + ctx: cancelledCtx, + err: context.Canceled, + want: true, + }, + { + name: "DeadlineExceeded with cancelled context", + ctx: cancelledCtx, + err: liveDE, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, IsDeadlineExceededOrCanceled(tt.ctx, tt.err)) + }) + } +} diff --git a/internal/fetch/fetch.go b/internal/fetch/fetch.go index 3b15ad75f1..60895a613c 100644 --- a/internal/fetch/fetch.go +++ b/internal/fetch/fetch.go @@ -21,6 +21,7 @@ import ( "syscall" "time" + "github.com/fullsend-ai/fullsend/internal/ctxerr" "github.com/fullsend-ai/fullsend/internal/netutil" ) @@ -329,12 +330,13 @@ func portAllowed(port string, allowed []string) bool { // and worth retrying. Context cancellation and deadline errors are never // transient — the caller chose to stop waiting. // -// We check ctx.Err() rather than errors.Is(err, context.DeadlineExceeded) -// because http.Client.Timeout wraps context.DeadlineExceeded from an -// internal context even when the caller's context is still active. Such -// per-request timeouts on slow servers are transient and worth retrying. +// We use ctxerr.IsDeadlineExceededOrCanceled (ctx.Err()) rather than +// inspecting err for the DeadlineExceeded sentinel because +// http.Client.Timeout wraps DeadlineExceeded from an internal context +// even when the caller's context is still active. Such per-request +// timeouts on slow servers are transient and worth retrying. func isTransientRequestError(ctx context.Context, err error) bool { - if ctx.Err() != nil { + if ctxerr.IsDeadlineExceededOrCanceled(ctx, err) { return false } // HTTP client timeout (e.g. net/http.Client.Timeout exceeded) and diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 16c39c1e19..e1568e8b91 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -10,6 +10,8 @@ import ( "io" "strings" "time" + + "github.com/fullsend-ai/fullsend/internal/ctxerr" ) // ConfigRepoName is the conventional name for the org-level fullsend @@ -143,9 +145,16 @@ func IsNotFork(err error) bool { // - HTTP client/network timeouts // - unexpected connection closures (io.EOF, io.ErrUnexpectedEOF) // +// ctx is the caller's context for the operation that produced err. +// Caller-context cancellation and deadline expiry are never transient; +// nested timeouts (net/http Client.Timeout wrapping DeadlineExceeded +// from an internal context) are, because the caller's context is still +// live. Callers must pass the same ctx used for the operation — using +// context.Background() cannot distinguish the two. +// // Callers can use this to decide whether retrying an operation is // worthwhile before falling back to a log-and-continue strategy. -func IsTransient(err error) bool { +func IsTransient(ctx context.Context, err error) bool { if err == nil { return false } @@ -166,7 +175,9 @@ func IsTransient(err error) bool { // they reflect caller intent, not a server-side failure. // context.DeadlineExceeded implements Timeout() bool (returning // true), so this guard must come before the Timeout() check. - if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + // Use ctxerr rather than errors.Is: net/http Client.Timeout + // unwraps to DeadlineExceeded from an internal context. + if ctxerr.IsDeadlineExceededOrCanceled(ctx, err) { return false } // HTTP client timeout (e.g. net/http.Client.Timeout exceeded). diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 6795cc8ae3..e1fa3c7452 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -22,6 +22,7 @@ import ( "time" "unicode/utf8" + "github.com/fullsend-ai/fullsend/internal/ctxerr" "github.com/fullsend-ai/fullsend/internal/forge" "golang.org/x/crypto/nacl/box" ) @@ -258,7 +259,7 @@ func (c *LiveClient) do(ctx context.Context, method, path string, body any) (*ht if err != nil { // If the caller's context is done, propagate immediately // — retrying is pointless when the parent has cancelled. - if ctx.Err() != nil { + if ctxerr.IsDeadlineExceededOrCanceled(ctx, err) { return nil, fmt.Errorf("http %s %s: %w", method, path, err) } // HTTP client timeout (Client.Timeout exceeded): retry @@ -374,17 +375,17 @@ func isRetryable(resp *http.Response) (bool, []byte) { // isTimeoutError reports whether err is an HTTP client timeout (e.g. // Client.Timeout exceeded) as opposed to a caller-context cancellation -// or deadline. It checks ctx.Err() internally so callers do not need -// to guard against context errors before calling this function. +// or deadline. It uses ctxerr so callers do not need to guard against +// context errors before calling this function. // // The context check is necessary because Go's net/http client timeout // wraps context.DeadlineExceeded internally, making error-only // introspection unable to distinguish caller deadlines from transport -// timeouts. Checking the caller's context disambiguates: if ctx.Err() -// is non-nil, the caller's context expired; otherwise, any Timeout() +// timeouts. Checking the caller's context disambiguates: if ctx itself +// is done, the caller's context expired; otherwise, any Timeout() // error is a transport-level timeout worth retrying. func isTimeoutError(ctx context.Context, err error) bool { - if ctx.Err() != nil { + if ctxerr.IsDeadlineExceededOrCanceled(ctx, err) { return false } var te interface{ Timeout() bool } diff --git a/internal/forge/transient_test.go b/internal/forge/transient_test.go index fd6b441670..b1ebaba95e 100644 --- a/internal/forge/transient_test.go +++ b/internal/forge/transient_test.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -32,106 +33,175 @@ func (e *fakeTimeoutErr) Error() string { return "timeout error" } func (e *fakeTimeoutErr) Timeout() bool { return e.timeout } func (e *fakeTimeoutErr) Temporary() bool { return e.timeout } +// unwrapTimeoutErr mimics net/http Client.Timeout: Unwrap to +// DeadlineExceeded plus Timeout() bool. errors.Is against the +// DeadlineExceeded sentinel matches it even when ctx is still live. +type unwrapTimeoutErr struct { + error +} + +func (e *unwrapTimeoutErr) Timeout() bool { return true } +func (e *unwrapTimeoutErr) Unwrap() error { return e.error } + func TestIsTransient(t *testing.T) { t.Parallel() + cancelledCtx, cancel := context.WithCancel(context.Background()) + cancel() + + expiredCtx, expire := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + t.Cleanup(expire) + + live := context.Background() + httpTimeout := &unwrapTimeoutErr{error: context.DeadlineExceeded} + tests := []struct { name string + ctx context.Context err error want bool }{ { name: "nil error", + ctx: live, err: nil, want: false, }, { name: "ErrNonFastForward", + ctx: live, err: ErrNonFastForward, want: true, }, { name: "wrapped ErrNonFastForward", + ctx: live, err: fmt.Errorf("commit failed: %w", ErrNonFastForward), want: true, }, { name: "transient reporter true", + ctx: live, err: &fakeTransientErr{transient: true}, want: true, }, { name: "transient reporter false", + ctx: live, err: &fakeTransientErr{transient: false}, want: false, }, { name: "wrapped transient reporter", + ctx: live, err: fmt.Errorf("api call: %w", &fakeTransientErr{transient: true}), want: true, }, { - name: "context.DeadlineExceeded is not transient", + name: "DeadlineExceeded with live context is a nested timeout", + ctx: live, + err: context.DeadlineExceeded, + want: true, + }, + { + name: "wrapped DeadlineExceeded with live context is a nested timeout", + ctx: live, + err: fmt.Errorf("timed out: %w", context.DeadlineExceeded), + want: true, + }, + { + name: "DeadlineExceeded with expired context is not transient", + ctx: expiredCtx, err: context.DeadlineExceeded, want: false, }, { - name: "wrapped context.DeadlineExceeded is not transient", + name: "wrapped DeadlineExceeded with expired context is not transient", + ctx: expiredCtx, err: fmt.Errorf("timed out: %w", context.DeadlineExceeded), want: false, }, { - name: "context.Canceled is not transient", + name: "http Client.Timeout wrapping DeadlineExceeded with live context", + ctx: live, + err: httpTimeout, + want: true, + }, + { + name: "http Client.Timeout wrapping DeadlineExceeded with expired context", + ctx: expiredCtx, + err: httpTimeout, + want: false, + }, + { + name: "context.Canceled with live context is not transient", + ctx: live, err: context.Canceled, want: false, }, { - name: "wrapped context.Canceled is not transient", + name: "wrapped context.Canceled with live context is not transient", + ctx: live, err: fmt.Errorf("canceled: %w", context.Canceled), want: false, }, + { + name: "context.Canceled with cancelled context is not transient", + ctx: cancelledCtx, + err: context.Canceled, + want: false, + }, { name: "timeout error", + ctx: live, err: &fakeTimeoutErr{timeout: true}, want: true, }, { name: "non-timeout error with Timeout method", + ctx: live, err: &fakeTimeoutErr{timeout: false}, want: false, }, { name: "io.EOF", + ctx: live, err: io.EOF, want: true, }, { name: "wrapped io.EOF", + ctx: live, err: fmt.Errorf("read body: %w", io.EOF), want: true, }, { name: "io.ErrUnexpectedEOF", + ctx: live, err: io.ErrUnexpectedEOF, want: true, }, { name: "ErrNotFound is not transient", + ctx: live, err: ErrNotFound, want: false, }, { name: "ErrForbidden is not transient", + ctx: live, err: ErrForbidden, want: false, }, { name: "ErrBranchProtected is not transient", + ctx: live, err: ErrBranchProtected, want: false, }, { name: "generic error is not transient", + ctx: live, err: errors.New("something broke"), want: false, }, @@ -139,7 +209,7 @@ func TestIsTransient(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tt.want, IsTransient(tt.err)) + assert.Equal(t, tt.want, IsTransient(tt.ctx, tt.err)) }) } } diff --git a/internal/gcp/client.go b/internal/gcp/client.go index 580d009a1b..9f1edb4c0f 100644 --- a/internal/gcp/client.go +++ b/internal/gcp/client.go @@ -16,6 +16,8 @@ import ( "syscall" "time" + "github.com/fullsend-ai/fullsend/internal/ctxerr" + "golang.org/x/oauth2" "golang.org/x/oauth2/google" ) @@ -220,16 +222,17 @@ func defaultRetryDelay(attempt int) time.Duration { // TLS handshake timeouts, unexpected connection closures, and network // timeouts. // -// We check ctx.Err() rather than errors.Is(err, context.DeadlineExceeded) -// because http.Client.Timeout wraps context.DeadlineExceeded from an -// internal context even when the caller's context is still active. Such -// per-request timeouts on slow servers are transient and worth retrying. +// We use ctxerr.IsDeadlineExceededOrCanceled (ctx.Err()) rather than +// inspecting err for the DeadlineExceeded sentinel because +// http.Client.Timeout wraps DeadlineExceeded from an internal context +// even when the caller's context is still active. Such per-request +// timeouts on slow servers are transient and worth retrying. // See internal/fetch/fetch.go isTransientRequestError for the same // rationale. func isRetryableTransportError(ctx context.Context, err error) bool { // If the caller's context is done, this is intentional // cancellation — not a transient failure. - if ctx.Err() != nil { + if ctxerr.IsDeadlineExceededOrCanceled(ctx, err) { return false } // HTTP client timeout (e.g. net/http.Client.Timeout exceeded) diff --git a/internal/harness/compose.go b/internal/harness/compose.go index a9bbec3f79..447f127cdd 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/fullsend-ai/fullsend/internal/ctxerr" "github.com/fullsend-ai/fullsend/internal/fetch" "github.com/fullsend-ai/fullsend/internal/forge" "github.com/fullsend-ai/fullsend/internal/gitfetch" @@ -1671,7 +1672,7 @@ func fetchBaseScriptOrDir(ctx context.Context, field, baseURLDir, relPath string if err == nil { return dep, contentPath, nil } - if !isTransientFetchError(err) { + if !isTransientFetchError(ctx, err) { return Dependency{}, "", err } // Transient tree-fetch failure — fall through to single-file fetch. @@ -1825,7 +1826,7 @@ func fetchBaseSkill(ctx context.Context, field, baseURLDir, skillPath string, al dep, dirPath, err := fetchBaseSkillDir(ctx, field, skillDirURL, skillFileURL, skillPath, allowedBy, allowlist, opts) if err != nil && staleFallback != nil { - if !isTransientFetchError(err) { + if !isTransientFetchError(ctx, err) { return Dependency{}, "", err } staleFallback.Warning = fmt.Sprintf("using stale cached content (re-fetch failed: %s)", err) @@ -1835,8 +1836,16 @@ func fetchBaseSkill(ctx context.Context, field, baseURLDir, skillPath string, al } // isTransientFetchError returns true for errors that indicate a temporary -// network issue where serving stale cached content is appropriate. -func isTransientFetchError(err error) bool { +// network issue where serving stale cached content is appropriate. Unlike +// most ctxerr.IsDeadlineExceededOrCanceled call sites, this classifier does +// not distinguish caller-context expiry from a nested timeout (for example +// a git-fetch HTTP client's own Client.Timeout unwrapping to +// context.DeadlineExceeded): both mean the re-fetch didn't finish in time, +// and stale cache is an acceptable fallback either way. +func isTransientFetchError(ctx context.Context, err error) bool { + if ctxerr.IsDeadlineExceededOrCanceled(ctx, err) { + return true + } if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { return true } @@ -2024,7 +2033,7 @@ func fetchBaseDir(ctx context.Context, kind baseDirKind, field, baseURLDir, dirP dep, dirPath, err := fetchBaseDirTree(ctx, kind, field, dirURL, keyURL, dirPath, allowedBy, allowlist, opts) if err != nil && staleFallback != nil { - if !isTransientFetchError(err) { + if !isTransientFetchError(ctx, err) { return Dependency{}, "", err } staleFallback.Warning = fmt.Sprintf("using stale cached content (re-fetch failed: %s)", err) diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index ba96287589..8f236f0e5b 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/fullsend-ai/fullsend/internal/fetch" "github.com/fullsend-ai/fullsend/internal/gitfetch" @@ -6111,22 +6112,30 @@ func TestFetchBaseSkill_TreeFetchErrorWithToken(t *testing.T) { } func TestIsTransientFetchError(t *testing.T) { + cancelledCtx, cancel := context.WithCancel(context.Background()) + cancel() + expiredCtx, expire := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) + t.Cleanup(expire) + live := context.Background() + tests := []struct { name string + ctx context.Context err error transient bool }{ - {"context deadline", fmt.Errorf("git fetch: %w", context.DeadlineExceeded), true}, - {"context canceled", fmt.Errorf("git fetch: %w", context.Canceled), true}, - {"transient error type", &gitfetch.TransientError{Err: fmt.Errorf("connection refused")}, true}, - {"wrapped transient", fmt.Errorf("gitfetch: %w", &gitfetch.TransientError{Err: fmt.Errorf("no such host")}), true}, - {"auth error", fmt.Errorf("authentication failed"), false}, - {"generic error", fmt.Errorf("something went wrong"), false}, - {"404 error", fmt.Errorf("not found"), false}, + {"context deadline with expired context", expiredCtx, fmt.Errorf("git fetch: %w", context.DeadlineExceeded), true}, + {"context canceled with cancelled context", cancelledCtx, fmt.Errorf("git fetch: %w", context.Canceled), true}, + {"context deadline with live context is still transient (nested timeout)", live, fmt.Errorf("git fetch: %w", context.DeadlineExceeded), true}, + {"transient error type", live, &gitfetch.TransientError{Err: fmt.Errorf("connection refused")}, true}, + {"wrapped transient", live, fmt.Errorf("gitfetch: %w", &gitfetch.TransientError{Err: fmt.Errorf("no such host")}), true}, + {"auth error", live, fmt.Errorf("authentication failed"), false}, + {"generic error", live, fmt.Errorf("something went wrong"), false}, + {"404 error", live, fmt.Errorf("not found"), false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.transient, isTransientFetchError(tt.err)) + assert.Equal(t, tt.transient, isTransientFetchError(tt.ctx, tt.err)) }) } } diff --git a/pkg/behaviourtest/steps/cleanup.go b/pkg/behaviourtest/steps/cleanup.go index cef4bbedd9..c9a0ee91a1 100644 --- a/pkg/behaviourtest/steps/cleanup.go +++ b/pkg/behaviourtest/steps/cleanup.go @@ -32,7 +32,7 @@ func cleanupRetry(logf func(string, ...any), desc string, fn func() error) error if lastErr == nil { return nil } - if !forge.IsTransient(lastErr) { + if !forge.IsTransient(context.Background(), lastErr) { return lastErr } if attempt < cleanupMaxAttempts-1 {