carlos / platform Public

console/awsinv: fix infra-pass review round (Critical + 2 Important)

Critical: the real all-denied deny path (awsinv.Inventory) signalled with
a bare errors.New that neither awsinv.IsAuthShaped nor isAuthShapedError
could classify (both start from errors.As against smithy.APIError, which
this shape never satisfies), so the console's auth latch never actually
fired against production AWS — only against a hand-made fake. Fixed at
the awsinv end: exported sentinel awsinv.ErrNoGrant, returned from
Inventory instead of the inline error, and IsAuthShaped now also matches
it via errors.Is. The console's latch test now uses the real sentinel
instead of a fake API error shape production never emits.

Important 1: a latched pass logged 'disabled for this process' every
tick forever. StartInfraPass now recognizes the errInfraDisabled
sentinel and returns instead of looping — the latch is permanent for the
process, so the ticker goroutine has nothing left to do. A direct caller
(the future refresh door) still gets the error back from infraPass
itself.

Important 2: infraDisabled was a plain bool despite being reached from
loadInventory (an HTTP handler) and, in a later task, a refresh door —
both read/write outside the single-goroutine ticker cepull.go's
'no concurrent caller' reasoning assumed. Now atomic.Bool, proven race-
clean under -race.

Also: dropped the now-redundant '|| isAuthShapedError(err)' in infraPass
(awsinv.IsAuthShaped's code list is a strict superset once it also
matches ErrNoGrant); replaced the hand-rolled fakeAPIError test type with
smithy.GenericAPIError, the same fake cepull_test.go already uses in this
package; and the transient-failure test now uses a real
ThrottlingException-coded smithy error rather than a plain non-API error.

Deferred per review: the missing 'unreadable'-state test and the
unreachable s.cfg.Releases == nil guard in putInventory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 2 months ago 48b9f29ffe011c0b8beda38e48aecbfa0b7f5c70 parent b3674ac

5 files changed, +91 −31

internal/awsinv/awsinv.go +16 −1
internal/awsinv/awsinv_test.go +18 −1
internal/console/infrapass.go +26 −10
internal/console/infrapass_test.go +19 −15
internal/console/server.go +12 −4

diff --git a/internal/awsinv/awsinv.go b/internal/awsinv/awsinv.go

index cdb733f58..ab25d647c 100644

--- a/internal/awsinv/awsinv.go

+++ b/internal/awsinv/awsinv.go

@@ -21,6 +21,18 @@ import (

// otherwise hang on IMDS.

const credentialProbe = 5 * time.Second

+// ErrNoGrant is Inventory's sentinel for "every read was denied and

+// nothing was discovered" — this deployment has no inventory IAM grant.

+// A sentinel, not a wrapped original error, because by the time Inventory

+// decides this the individual mapping failures have already been turned

+// into Partial STRINGS (looksDenied scans those, not error values) — no

+// error object survives assembly to wrap, so a sentinel is the only way to

+// keep this one rule in one place. IsAuthShaped matches it via errors.Is,

+// same as it matches a denied smithy code, so the console's latch fires on

+// both the construction-time denial (a bad DescribeRegions call) and this

+// assembly-time one.

+var ErrNoGrant = errors.New("awsinv: every read was denied — this deployment has no inventory grant")

// Provider is the AWS implementation of infraview.Provider.

type Provider struct {

factory Factory

@@ -175,7 +187,7 @@ func (p *Provider) Inventory(ctx context.Context) (infraview.Inventory, error) {

}

if len(inv.Resources) == 0 && authDenied {

- return inv, errors.New("awsinv: every read was denied — this deployment has no inventory grant")

+ return inv, ErrNoGrant

}

return inv, nil

}

@@ -184,6 +196,9 @@ func (p *Provider) Inventory(ctx context.Context) (infraview.Inventory, error) {

// console classifies THIS package's construction errors by the same rule it

// classifies Cost Explorer's. One rule, one list of codes.

func IsAuthShaped(err error) bool {

+ if errors.Is(err, ErrNoGrant) {

+ return true

+ }

var apiErr smithy.APIError

if !errors.As(err, &apiErr) {

return false

diff --git a/internal/awsinv/awsinv_test.go b/internal/awsinv/awsinv_test.go

index 0a2f1847b..865f6a314 100644

--- a/internal/awsinv/awsinv_test.go

+++ b/internal/awsinv/awsinv_test.go

@@ -115,9 +115,26 @@ func TestAuthShapedWithNoResourcesIsAnError(t *testing.T) {

}

p := newWithFactory(factory, "eu-west-1", []string{"eu-west-1"}, "")

- if _, err := p.Inventory(context.Background()); err == nil {

+ _, err := p.Inventory(context.Background())

+ if err == nil {

t.Fatal("want an error when every read is denied and nothing was discovered")

}

+ if !errors.Is(err, ErrNoGrant) {

+ t.Fatalf("err = %v, want it to satisfy errors.Is(err, ErrNoGrant) — this is what the console's latch matches on", err)

+ }

+// IsAuthShaped must also recognize ErrNoGrant itself, not just a denied

+// smithy code — Inventory's assembly-time denial (this test's own sibling,

+// TestAuthShapedWithNoResourcesIsAnError) has no surviving smithy.APIError

+// to classify by the time it reaches the caller, only this sentinel.

+func TestIsAuthShapedRecognizesErrNoGrant(t *testing.T) {

+ if !IsAuthShaped(ErrNoGrant) {

+ t.Fatal("IsAuthShaped(ErrNoGrant) = false, want true")

+ }

+ if IsAuthShaped(errors.New("some other error")) {

+ t.Fatal("IsAuthShaped(plain error) = true, want false")

+ }

}

// The same denial, but something WAS discovered: the inventory is returned

diff --git a/internal/console/infrapass.go b/internal/console/infrapass.go

index 3f3c12b9b..e59bc8d64 100644

--- a/internal/console/infrapass.go

+++ b/internal/console/infrapass.go

@@ -36,6 +36,14 @@ const infraPassInterval = time.Hour

// restarts.

const infraPassTimeout = 2 * time.Minute

+// errInfraDisabled is infraPass's sentinel for "this process already

+// latched off" — StartInfraPass matches it with errors.Is to stop ticking

+// rather than log it hourly forever: the latch is permanent for the

+// process, so there is nothing left for the goroutine to do. A direct

+// caller (the refresh door, a later task) still gets the error back from

+// infraPass itself; only the ticker's own logging loop is silenced.

+var errInfraDisabled = errors.New("console: infra inventory disabled for this process")

// StartInfraPass runs infraPass immediately and then hourly until ctx is

// done — StartEconomicsPass's shape (economicspass.go:38).

func (s *Server) StartInfraPass(ctx context.Context) {

@@ -46,6 +54,9 @@ func (s *Server) StartInfraPass(ctx context.Context) {

defer t.Stop()

for {

if err := s.infraPass(ctx); err != nil {

+ if errors.Is(err, errInfraDisabled) {

+ return // permanent for the process — nothing left to tick for

+ }

log.Printf("console: infra pass: %v", err)

}

select {

@@ -62,8 +73,8 @@ func (s *Server) resolveInventory(ctx context.Context) (infraview.Inventory, err

if s.inventory != nil {

return s.inventory(ctx)

}

- if s.infraDisabled {

- return infraview.Inventory{}, errors.New("console: infra inventory disabled for this process")

+ if s.infraDisabled.Load() {

+ return infraview.Inventory{}, errInfraDisabled

}

s.infraOnce.Do(func() {

p, err := awsinv.New(ctx)

@@ -72,14 +83,14 @@ func (s *Server) resolveInventory(ctx context.Context) (infraview.Inventory, err

// Anything else means this process cannot build a provider at

// all, which is equally permanent for this process.

log.Printf("console: infra inventory unavailable: %v", err)

- s.infraDisabled = true

+ s.infraDisabled.Store(true)

return

}

s.infraProvider = p

})

if s.infraProvider == nil {

- s.infraDisabled = true

- return infraview.Inventory{}, errors.New("console: no infrastructure provider")

+ s.infraDisabled.Store(true)

+ return infraview.Inventory{}, errInfraDisabled

}

return s.infraProvider.Inventory(ctx)

}

@@ -87,16 +98,21 @@ func (s *Server) resolveInventory(ctx context.Context) (infraview.Inventory, err

// infraPass takes one inventory and stores it. An auth-shaped failure

// latches; everything else is returned for the caller to log and retry.

func (s *Server) infraPass(ctx context.Context) error {

- if s.infraDisabled {

- return errors.New("console: infra inventory disabled for this process")

+ if s.infraDisabled.Load() {

+ return errInfraDisabled

}

ctx, cancel := context.WithTimeout(ctx, infraPassTimeout)

defer cancel()

inv, err := s.resolveInventory(ctx)

if err != nil {

- if awsinv.IsAuthShaped(err) || isAuthShapedError(err) {

- s.infraDisabled = true

+ // awsinv.IsAuthShaped alone covers this seam: it matches both a

+ // denied smithy code AND awsinv.ErrNoGrant (the "every read was

+ // denied" assembly-time sentinel, which carries no smithy.APIError

+ // to classify by construction), so isAuthShapedError's narrower,

+ // smithy-only check can never turn a false into a true here.

+ if awsinv.IsAuthShaped(err) {

+ s.infraDisabled.Store(true)

return fmt.Errorf("console: infra inventory not granted (disabled for this process): %w", err)

}

return fmt.Errorf("console: infra inventory: %w", err)

@@ -140,7 +156,7 @@ func (s *Server) loadInventory(ctx context.Context) (*infraview.Inventory, strin

body, _, err := s.cfg.Releases.Get(ctx, infraview.InventoryKey)

switch {

case errors.Is(err, os.ErrNotExist):

- if s.infraDisabled {

+ if s.infraDisabled.Load() {

return nil, "ungranted"

}

return nil, "never"

diff --git a/internal/console/infrapass_test.go b/internal/console/infrapass_test.go

index 459df2f1f..13e57ddd3 100644

--- a/internal/console/infrapass_test.go

+++ b/internal/console/infrapass_test.go

@@ -3,26 +3,15 @@ package console

import (

"context"

"encoding/json"

- "errors"

"testing"

"time"

"github.com/aws/smithy-go"

+ "github.com/carlosframework/platform/internal/awsinv"

"github.com/carlosframework/platform/internal/infraview"

)

-// fakeAPIError is the same four-method smithy.APIError shape used by

-// internal/awsinv's fakeAPIErr (rest_test.go) — the minimum a caller needs

-// to satisfy errors.As(err, &apiErr) and drive isAuthShapedError/

-// awsinv.IsAuthShaped off ErrorCode() alone.

-type fakeAPIError struct{ code string }

-func (e *fakeAPIError) Error() string { return e.code }

-func (e *fakeAPIError) ErrorCode() string { return e.code }

-func (e *fakeAPIError) ErrorMessage() string { return e.code }

-func (e *fakeAPIError) ErrorFault() smithy.ErrorFault { return smithy.FaultClient }

// The pass writes the snapshot where the page reads it, and loadInventory

// reads back exactly what was written.

func TestInfraPassWritesSnapshot(t *testing.T) {

@@ -56,12 +45,22 @@ func TestInfraPassWritesSnapshot(t *testing.T) {

// An auth-shaped failure latches the pass off for the process after ONE

// attempt: "this deployment has no inventory grant" is a permanent answer,

// not something to re-ask AWS every hour forever.

+//

+// The fake returns awsinv.ErrNoGrant — the REAL error value

+// awsinv.Provider.Inventory produces when every read comes back denied and

+// nothing was discovered (internal/awsinv/awsinv.go) — not a hand-made

+// smithy.APIError. A prior version of this test used a fake AccessDenied

+// smithy error, an error shape the real seam never actually emits (the

+// per-mapping denials are folded into Partial strings well before

+// assembly decides anything, so no smithy.APIError survives to the

+// caller); that let the pass pass its tests while the real deny path

+// never latched at all. Using the real sentinel is what proves the fix.

func TestInfraPassLatchesOnAuthShapedFailure(t *testing.T) {

fx := newConsoleFixture(t)

calls := 0

fx.srv.inventory = func(context.Context) (infraview.Inventory, error) {

calls++

- return infraview.Inventory{}, &fakeAPIError{code: "AccessDenied"}

+ return infraview.Inventory{}, awsinv.ErrNoGrant

}

if err := fx.srv.infraPass(context.Background()); err == nil {

t.Fatal("want an error from the denied pass")

@@ -78,13 +77,18 @@ func TestInfraPassLatchesOnAuthShapedFailure(t *testing.T) {

}

// A transient failure is NOT the latch: CE throttles under load and a blip

-// must not silently kill inventory until the next redeploy.

+// must not silently kill inventory until the next redeploy. Uses a real

+// smithy.GenericAPIError coded ThrottlingException — the same fake type

+// cepull_test.go uses in this package — so this proves a genuine AWS

+// throttle survives errors.As(&smithy.APIError{}) and still fails

+// awsinv.IsAuthShaped's code check, rather than merely proving a plain

+// non-API error fails the type assertion outright.

func TestInfraPassRetriesTransientFailure(t *testing.T) {

fx := newConsoleFixture(t)

calls := 0

fx.srv.inventory = func(context.Context) (infraview.Inventory, error) {

calls++

- return infraview.Inventory{}, errors.New("Throttling: rate exceeded")

+ return infraview.Inventory{}, &smithy.GenericAPIError{Code: "ThrottlingException"}

}

_ = fx.srv.infraPass(context.Background())

diff --git a/internal/console/server.go b/internal/console/server.go

index ea262b028..d9952f180 100644

--- a/internal/console/server.go

+++ b/internal/console/server.go

@@ -623,10 +623,18 @@ type Server struct {

// real path lazily constructs an awsinv.Provider once. infraDisabled

// latches an auth-shaped failure off for the process — a deployment

// with no inventory IAM grant answers "ungranted" once, not hourly.

- inventory func(ctx context.Context) (infraview.Inventory, error)

- infraOnce sync.Once

- infraProvider infraview.Provider

- infraDisabled bool

+ // Unlike ceDisabled above, this one IS reached from more than the

+ // single-goroutine hourly ticker: loadInventory reads it to serve an

+ // HTTP handler, and the refresh door (a later task) calls infraPass

+ // directly from another — so it is atomic.Bool, not a plain bool.

+ inventory func(ctx context.Context) (infraview.Inventory, error)

+ infraOnce sync.Once

+ infraProvider infraview.Provider

+ infraDisabled atomic.Bool

+ // infraRefreshing single-flights concurrent refresh-door callers

+ // against EACH OTHER only — it says nothing about the ticker, which is

+ // why infraDisabled above needs its own atomicity and this field

+ // cannot stand in for that fix.

infraRefreshing atomic.Bool

// shipSem bounds how many `carlos ship` uploads (apicli.go,

// handleAPIShip) may run AT ONCE, as opposed to s.cli.ship, which