scalesets: distinguish a GitHub 403 from a 401

apiserver/controllers/controllers.go

@@ -33,6 +33,7 @@ import (
 	"github.com/cloudbase/garm/apiserver/params"
 	"github.com/cloudbase/garm/auth"
 	"github.com/cloudbase/garm/config"
+	internalErrors "github.com/cloudbase/garm/internal/errors"
 	"github.com/cloudbase/garm/metrics"
 	runnerParams "github.com/cloudbase/garm/params"
 	"github.com/cloudbase/garm/runner" //nolint:typecheck
@@ -109,6 +110,12 @@ func handleError(ctx context.Context, w http.ResponseWriter, err error) {
 	case errors.Is(err, gErrors.ErrNotFound):
 		w.WriteHeader(http.StatusNotFound)
 		apiErr.Error = "Not Found"
+	// Checked before the unauthorized case below: a ForbiddenError reports as an
+	// UnauthorizedError too, so that callers asking only "was this refused?" keep
+	// working, and the broader case would otherwise swallow it.
+	case errors.Is(err, &internalErrors.ForbiddenError{}):
+		w.WriteHeader(http.StatusForbidden)
+		apiErr.Error = "Forbidden"
 	case errors.Is(err, gErrors.ErrUnauthorized):
 		w.WriteHeader(http.StatusUnauthorized)
 		apiErr.Error = "Not Authorized"

internal/errors/errors.go

@@ -102,3 +102,44 @@ func RunnerIsTerminal(s params.RunnerStatus) bool {
 		return false
 	}
 }
+
+// NewForbiddenError returns a ForbiddenError carrying the message the forge
+// replied with.
+func NewForbiddenError(msg string, a ...any) error {
+	return &ForbiddenError{
+		msg: fmt.Sprintf(msg, a...),
+	}
+}
+
+// ForbiddenError is returned when a forge answers a request with 403 rather
+// than 401. Both mean the request was refused, but only 401 implies the
+// credentials are wrong: GitHub also answers 403 for a secondary rate limit or
+// for SSO enforcement on an organization, neither of which says anything about
+// the credentials, and both of which clear on their own or with an operator
+// action that is not a credential rotation.
+//
+// It reports as a runnerErrors.UnauthorizedError so that callers which only
+// ask "was this refused?" keep working unchanged. Callers that need to act on
+// the difference — retrying a transient refusal instead of declaring the
+// credentials dead — can single it out with errors.Is(err, &ForbiddenError{}),
+// which must be checked before the broader unauthorized case.
+type ForbiddenError struct {
+	msg string
+}
+
+func (e *ForbiddenError) Error() string {
+	return e.msg
+}
+
+func (e *ForbiddenError) Is(target error) bool {
+	if target == nil {
+		return false
+	}
+
+	switch target.(type) {
+	case *ForbiddenError, *runnerErrors.UnauthorizedError:
+		return true
+	default:
+		return false
+	}
+}

internal/errors/errors_test.go

@@ -0,0 +1,93 @@
+// Copyright 2026 Cloudbase Solutions SRL
+//
+//	Licensed under the Apache License, Version 2.0 (the "License"); you may
+//	not use this file except in compliance with the License. You may obtain
+//	a copy of the License at
+//
+//	     http://www.apache.org/licenses/LICENSE-2.0
+//
+//	Unless required by applicable law or agreed to in writing, software
+//	distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+//	WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+//	License for the specific language governing permissions and limitations
+//	under the License.
+package errors
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	runnerErrors "github.com/cloudbase/garm-provider-common/errors"
+)
+
+func TestForbiddenErrorIdentity(t *testing.T) {
+	forbidden := NewForbiddenError("forbidden while calling %s", "https://example.com")
+
+	tests := []struct {
+		name   string
+		target error
+		want   bool
+	}{
+		{
+			name:   "matches itself, so callers can single out a 403",
+			target: &ForbiddenError{},
+			want:   true,
+		},
+		{
+			// The compatibility guarantee: callers that only ask "was this
+			// refused?" must keep working after 403 stopped returning
+			// ErrUnauthorized outright.
+			name:   "still reports as unauthorized",
+			target: runnerErrors.ErrUnauthorized,
+			want:   true,
+		},
+		{
+			name:   "is not a not-found",
+			target: runnerErrors.ErrNotFound,
+			want:   false,
+		},
+		{
+			name:   "is not a bad request",
+			target: runnerErrors.ErrBadRequest,
+			want:   false,
+		},
+		{
+			name:   "does not match a nil target",
+			target: nil,
+			want:   false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			require.Equal(t, tt.want, errors.Is(forbidden, tt.target))
+		})
+	}
+}
+
+func TestForbiddenErrorKeepsItsMessage(t *testing.T) {
+	err := NewForbiddenError("forbidden while calling %s: %q", "https://example.com", "rate limited")
+
+	require.EqualError(t, err, `forbidden while calling https://example.com: "rate limited"`)
+}
+
+func TestForbiddenErrorSurvivesWrapping(t *testing.T) {
+	// Callers report failures with %w up the stack, so the distinction has to
+	// survive being wrapped or it is useless where it is actually read.
+	wrapped := fmt.Errorf("removing runner from github: %w", NewForbiddenError("forbidden"))
+
+	require.True(t, errors.Is(wrapped, &ForbiddenError{}))
+	require.True(t, errors.Is(wrapped, runnerErrors.ErrUnauthorized))
+}
+
+func TestUnauthorizedErrorIsNotForbidden(t *testing.T) {
+	// The inverse of the compatibility guarantee: a plain 401 must not be
+	// mistaken for a 403, or the retry decision inverts.
+	err := runnerErrors.NewUnauthorizedError("unauthorized while calling https://example.com")
+
+	require.True(t, errors.Is(err, runnerErrors.ErrUnauthorized))
+	require.False(t, errors.Is(err, &ForbiddenError{}))
+}

util/github/scalesets/client.go

@@ -23,6 +23,7 @@ import (
 	"github.com/google/go-github/v84/github"
 
 	runnerErrors "github.com/cloudbase/garm-provider-common/errors"
+	internalErrors "github.com/cloudbase/garm/internal/errors"
 	"github.com/cloudbase/garm/metrics"
 	"github.com/cloudbase/garm/params"
 	"github.com/cloudbase/garm/runner/common"
@@ -111,8 +112,19 @@ func (s *ScaleSetClient) Do(req *http.Request) (*http.Response, error) {
 		return nil, runnerErrors.NewBadRequestError("bad request while calling %s: %q", req.URL.String(), string(body))
 	case 409:
 		return nil, runnerErrors.NewConflictError("conflict while calling %s: %q", req.URL.String(), string(body))
-	case 401, 403:
-		return nil, runnerErrors.ErrUnauthorized
+	case 401:
+		// The credentials were rejected outright. Keep the body: it is the only
+		// thing that says whether the token expired, was revoked, or was never
+		// valid for this resource.
+		return nil, runnerErrors.NewUnauthorizedError(
+			fmt.Sprintf("unauthorized while calling %s: %q", req.URL.String(), string(body)))
+	case 403:
+		// Not the same as 401. GitHub answers 403 for a secondary rate limit and
+		// for SSO enforcement as well as for a genuine permission problem, so
+		// collapsing it into ErrUnauthorized tells callers the credentials are
+		// dead when they are usually fine and the refusal is temporary.
+		return nil, internalErrors.NewForbiddenError(
+			"forbidden while calling %s: %q", req.URL.String(), string(body))
 	default:
 		return nil, fmt.Errorf("request to %s failed with status code %d: %q", req.URL.String(), resp.StatusCode, string(body))
 	}

util/github/scalesets/client_test.go

@@ -0,0 +1,132 @@
+// Copyright 2026 Cloudbase Solutions SRL
+//
+//	Licensed under the Apache License, Version 2.0 (the "License"); you may
+//	not use this file except in compliance with the License. You may obtain
+//	a copy of the License at
+//
+//	     http://www.apache.org/licenses/LICENSE-2.0
+//
+//	Unless required by applicable law or agreed to in writing, software
+//	distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+//	WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+//	License for the specific language governing permissions and limitations
+//	under the License.
+package scalesets
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	runnerErrors "github.com/cloudbase/garm-provider-common/errors"
+	internalErrors "github.com/cloudbase/garm/internal/errors"
+)
+
+// doAgainstStatus dispatches one request through ScaleSetClient.Do against a
+// server that answers with the given status and body.
+func doAgainstStatus(t *testing.T, status int, body string) error {
+	t.Helper()
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(status)
+		_, _ = w.Write([]byte(body))
+	}))
+	t.Cleanup(srv.Close)
+
+	req, err := http.NewRequest(http.MethodDelete, srv.URL, nil)
+	require.NoError(t, err)
+
+	cli := &ScaleSetClient{httpClient: srv.Client()}
+	resp, err := cli.Do(req)
+	if resp != nil {
+		_ = resp.Body.Close()
+	}
+	return err
+}
+
+func TestDoMapsRefusalStatuses(t *testing.T) {
+	tests := []struct {
+		name string
+		// status GitHub answered with.
+		status int
+		// isForbidden is whether the caller can tell this apart as a 403.
+		isForbidden bool
+	}{
+		{
+			// 401 is the only status that says the credentials themselves were
+			// rejected.
+			name:        "unauthorized is not forbidden",
+			status:      http.StatusUnauthorized,
+			isForbidden: false,
+		},
+		{
+			// GitHub answers 403 for a secondary rate limit or SSO enforcement,
+			// which say nothing about the credentials.
+			name:        "forbidden is distinguishable",
+			status:      http.StatusForbidden,
+			isForbidden: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := doAgainstStatus(t, tt.status, "some detail")
+
+			require.Error(t, err)
+			// Both remain unauthorized, so callers that only ask whether the
+			// request was refused are unaffected by the split.
+			require.True(t, errors.Is(err, runnerErrors.ErrUnauthorized))
+			require.Equal(t, tt.isForbidden, errors.Is(err, &internalErrors.ForbiddenError{}))
+		})
+	}
+}
+
+func TestDoKeepsRefusalDetail(t *testing.T) {
+	// Both refusals used to return a bare sentinel, so an operator reading the
+	// log had no way to tell a dead credential from a rate limit. Every other
+	// status branch reports the URL and body; these now do too.
+	tests := []struct {
+		name   string
+		status int
+		body   string
+	}{
+		{name: "unauthorized", status: http.StatusUnauthorized, body: "bad credentials"},
+		{name: "forbidden", status: http.StatusForbidden, body: "secondary rate limit"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := doAgainstStatus(t, tt.status, tt.body)
+
+			require.ErrorContains(t, err, tt.body)
+		})
+	}
+}
+
+func TestDoLeavesOtherStatusesAlone(t *testing.T) {
+	tests := []struct {
+		name   string
+		status int
+		target error
+	}{
+		{name: "not found", status: http.StatusNotFound, target: runnerErrors.ErrNotFound},
+		{name: "bad request", status: http.StatusBadRequest, target: runnerErrors.ErrBadRequest},
+		{name: "conflict", status: http.StatusConflict, target: &runnerErrors.ConflictError{}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := doAgainstStatus(t, tt.status, "detail")
+
+			require.ErrorIs(t, err, tt.target)
+			require.False(t, errors.Is(err, runnerErrors.ErrUnauthorized))
+		})
+	}
+}
+
+func TestDoPassesSuccessThrough(t *testing.T) {
+	require.NoError(t, doAgainstStatus(t, http.StatusOK, "ok"))
+}