Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions docs/contributing/go-code.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -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

Expand Down
6 changes: 4 additions & 2 deletions internal/appsetup/appsetup.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"crypto/x509"
"encoding/json"
"encoding/pem"
"errors"
"fmt"
"html"
"net"
Expand All @@ -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"
Expand Down Expand Up @@ -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))
Expand Down
6 changes: 5 additions & 1 deletion internal/cli/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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")
Expand Down
40 changes: 40 additions & 0 deletions internal/cli/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 }()
Expand Down
28 changes: 28 additions & 0 deletions internal/ctxerr/ctxerr.go
Original file line number Diff line number Diff line change
@@ -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
}
150 changes: 150 additions & 0 deletions internal/ctxerr/ctxerr_test.go
Original file line number Diff line number Diff line change
@@ -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))
})
}
}
12 changes: 7 additions & 5 deletions internal/fetch/fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"syscall"
"time"

"github.com/fullsend-ai/fullsend/internal/ctxerr"
"github.com/fullsend-ai/fullsend/internal/netutil"
)

Expand Down Expand Up @@ -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
Expand Down
15 changes: 13 additions & 2 deletions internal/forge/forge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] authorization-tier

The PR widens the surface of an exported function (forge.IsTransient) by adding a required ctx parameter — a larger authorization footprint than issue #7240 explicitly requested (it only asked to migrate call sites to a new helper). This repo's COMMITS.md breaking-change criteria are framed around user-visible CLI/API/config behavior, not internal Go package signatures, and the fix is not achievable without the signature change; the PR body discloses it explicitly ('forge.IsTransient now takes ctx').

Suggested fix: No action needed — confirming this was a deliberate, disclosed choice, which it is. No BREAKING CHANGE marker required.

// 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
}
Expand All @@ -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).
Expand Down
Loading
Loading