Skip to content

feat(auth): bounded fail-open for auth, enrollment and spend gates - #1278

Closed
makosblade wants to merge 1 commit into
fail-open/2-original-modelfrom
fail-open/3-auth-enrollment-spend
Closed

makosblade wants to merge 1 commit into
fail-open/2-original-modelfrom
fail-open/3-auth-enrollment-spend

Conversation

@makosblade

@makosblade makosblade commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Extends ROUTER_DEPENDENCY_FAIL_OPEN (PR 2) from routing/session reads to the request gates that sit in front of them. Everything is a last-known-good copy of a read that already succeeded in this process; nothing here creates a snapshot from nothing.

  • auth.StaleAPIKeyCache: the LRU cache retains a 30s copy of each positive key; InvalidateInstallation clears it too, so a revoked installation never falls open.
  • auth.Service.VerifyAPIKeyWithDependencyFailOpen: runs the normal lookup through the bounded DB dependency; only an infrastructural failure on a prepared request falls back to the stale copy. ErrInvalidToken/ErrInvalidPrefix are returned unchanged.
  • auth.Service.ListSubscriptionAccountsForRequest: same shape for enrollment (30s snapshot per api key).
  • middleware.BillingFailOpenCache behind server.Features.BillingFailOpen: balance, api-key cap and org monthly cap reuse a recent successful read during a prepared outage; unprepared requests keep today's strict behavior.
  • The auth middleware now begins request preparation, so auth, enrollment, spend gates and routing share one 12s/1s/250ms budget (BeginPreparation is idempotent).
  • auth receives the preparer/starter from main.go because requestcontext already imports auth.

Still strict: unknown keys, cold caches, explicit invalidation, the switch being off, and cold startup readiness (PR 4).

Validation

  • make precommit check-agent-guides check-docs inference-boundary (all green except the two pre-existing local cc-statusline stamp assertions, which also fail on main in this environment)
  • go test -race ./internal/auth ./internal/server/middleware ./internal/requestcontext ./internal/proxy
  • New: internal/server/middleware/fail_open_test.go — stale key served / cold cache / switch off / revoked installation; enrollment snapshot; each billing gate prepared vs unprepared vs cold.
  • Also fixes a pre-existing race in TestFireMarkUsedRecoversFromPanic (unsynchronized log buffer read by require.Eventually).

Depends on #1275.

🤖 Generated with Weave Router


Summary by cubic

Extends ROUTER_DEPENDENCY_FAIL_OPEN from routing reads to the auth, enrollment, and billing spend gates, so a bounded database outage no longer blocks already-authorized requests.

  • Auth middleware now starts the same bounded preparation as routing, sharing one 12s/1s/250ms budget.
  • Auth serves a 30s last-known-good API key copy when the database read fails; revoked installations clear it too.
  • Enrollment reuses a 30s subscription snapshot, and the balance, api-key cap, and org monthly cap gates reuse a 30s successful read.
  • Unknown tokens, cold caches, the switch being off, and unprepared requests still fail closed.
  • Also fixes a pre-existing race in TestFireMarkUsedRecoversFromPanic by synchronizing the log buffer.

Written for commit 7b7fe82. Summary will update on new commits.

Review in cubic

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread internal/auth/service.go
Comment thread internal/auth/service.go
Comment thread internal/server/middleware/balance_check.go
Comment thread internal/auth/service.go

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

20 issues found across 19 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/server/middleware/billing_fail_open_cache.go">

<violation number="1" location="internal/server/middleware/billing_fail_open_cache.go:21">
P2: Each successful read adds a key to a plain map, while expiration is removed only when that same key is requested again. Long-lived API-key and installation churn therefore leaves expired snapshots resident indefinitely; add a bounded LRU or periodic/explicit cleanup.</violation>

<violation number="2" location="internal/server/middleware/billing_fail_open_cache.go:37">
P1: During a sustained prepared database outage, each fallback request re-arms its cache entry for another 30 seconds because the gates save the fallback result after replacing the error with nil. Preserve the original expiry or refresh the entry only after a fresh successful read, so fail-open remains bounded to the last known-good read.</violation>
</file>

<file name="cmd/router/main.go">

<violation number="1" location="cmd/router/main.go:539">
P1: When `ROUTER_DEPENDENCY_FAIL_OPEN=false`, an auth read failure can still authenticate from the stale cache. Leave the starter nil when fail-open is disabled so `PrepareRequest` does not mark the request as prepared.</violation>

<violation number="2" location="cmd/router/main.go:547">
P1: With fail-open enabled, every unknown or otherwise invalid key can mark the shared database dependency unhealthy because the auth method passes logical auth errors to the starter's finish callback. Classify auth denials separately and report only infrastructure failures to dependency health, otherwise an invalid request briefly takes valid-key traffic offline.</violation>
</file>

<file name="internal/server/middleware/org_monthly_spend_cap.go">

<violation number="1" location="internal/server/middleware/org_monthly_spend_cap.go:58">
P2: When an organization has multiple installations, a successful cap read for one installation is not reused for another during a prepared outage because the cache is keyed by installation ID while the read is organization-scoped. Key both the `getOrg` and `setOrg` operations by `orgID`.</violation>

<violation number="2" location="internal/server/middleware/org_monthly_spend_cap.go:72">
P1: During a sustained database outage, each request served from the stale snapshot refreshes its TTL here, so the org cap can remain fail-open indefinitely instead of expiring 30 seconds after the last successful read. Track whether the result came from the live read and update the cache only for live successful reads.</violation>
</file>

<file name="internal/requestcontext/fail_open.go">

<violation number="1" location="internal/requestcontext/fail_open.go:183">
P3: The new `StartDependency` function was inserted between `Start`'s doc comment and the `Start` method, so Go now attaches the "Start bounds a prerequisite call..." comment to `StartDependency` and leaves the exported `*Preparation.Start` method with no godoc at all. This violates the repo convention that every exported symbol carries a godoc starting with the symbol name. Move the `StartDependency` block (with its own comment) above the "// Start bounds..." comment, or place it after `Start`, so `Start` keeps its documentation.</violation>
</file>

<file name="internal/server/middleware/balance_check.go">

<violation number="1" location="internal/server/middleware/balance_check.go:78">
P2: When billing Postgres hangs, this fallback runs only after `CheckBalance` returns, but the billing read is not wrapped in the shared database dependency starter. Wrap the billing read with `StartDependency` and finish it before applying the cache so prepared outages fall back within the intended bounded budget.</violation>

<violation number="2" location="internal/server/middleware/balance_check.go:78">
P1: When the balance row is missing, this fallback converts `billing.ErrBalanceRowMissing` into a successful cached read before the 402 path runs. Exclude `ErrBalanceRowMissing` from fail-open fallback so only infrastructure failures can use the snapshot.</violation>

<violation number="3" location="internal/server/middleware/balance_check.go:116">
P1: During a continuing billing outage, every request served from the fallback refreshes the 30-second TTL at this line. Write the balance snapshot only after a live successful read, not after a cache hit, or fail-open can continue indefinitely under traffic.</violation>
</file>

<file name="internal/auth/subscription.go">

<violation number="1" location="internal/auth/subscription.go:113">
P2: After an auth read fails and a stale key is returned, this early return skips the warm enrollment snapshot. Check the prepared cache when `startDependency` reports the already-latched database failure instead of returning before the fallback path.</violation>

<violation number="2" location="internal/auth/subscription.go:120">
P2: Every successful enrollment read adds an entry that is never removed, so high-cardinality or churned API keys grow this process map without bound and retain encrypted token data. Replace this map with a bounded expirable/LRU cache, or add eviction and expired-entry cleanup.</violation>

<violation number="3" location="internal/auth/subscription.go:120">
P3: Use the service's injected clock for snapshot expiry instead of calling `time.Now()` directly, so cache behavior remains deterministic and follows the auth package's clock convention.</violation>
</file>

<file name="internal/auth/service.go">

<violation number="1" location="internal/auth/service.go:734">
P1: When the database dependency is in its cooldown, `startDependency` returns `startErr` before the live read runs. These early returns bypass the stale auth and enrollment caches, so subsequent prepared requests fail until cooldown expires; route dependency-start failures through the same prepared stale-cache fallback.</violation>

<violation number="2" location="internal/auth/service.go:750">
P1: During a prepared outage, derived-auth BYOK keys are returned without `resolveUpstreamSecrets`. This sends the stored private/client credential or an empty WIF credential downstream instead of the required short-lived upstream credential. Resolve stale external keys exactly like the normal cache-hit path.</violation>
</file>

<file name="internal/server/middleware/fail_open_test.go">

<violation number="1" location="internal/server/middleware/fail_open_test.go:82">
P3: These tests rely on wall-clock TTL expiry to decide the served path: the positive LRU is created with a 1ms TTL and the test sleeps 5ms, assuming the expirable LRU's background janitor has already evicted the positive entry. That assumption is load-bearing only in the "switch off" subtest, where a still-cached positive entry would make the second probe return 200 from cache instead of the asserted 503, and it silently weakens TestWithAuthServesRecentKeyDuringDatabaseOutage: that test passes even if the positive entry is what serves the key, so it does not strictly prove the stale copy stood in during the outage. Make the stale-path isolation deterministic (e.g. assert the repo was actually queried, or control eviction explicitly) instead of relying on sleep vs janitor timing.</violation>
</file>

<file name="internal/auth/apikey_cache.go">

<violation number="1" location="internal/auth/apikey_cache.go:99">
P2: Every positive `Set` adds to an unbounded stale map, and expired entries are cleaned only when that exact hash is requested. Use a capacity-bounded TTL cache or periodic cleanup so API-key churn cannot retain entries and grow process memory without bound.</violation>

<violation number="2" location="internal/auth/apikey_cache.go:99">
P1: When an auth read started before installation invalidation completes afterward, `Set` captures the new generation and re-adds the pre-invalidation result. Carry the generation captured before the database read into the cache write, or otherwise reject results from reads that began before invalidation.</violation>

<violation number="3" location="internal/auth/apikey_cache.go:99">
P1: When the positive LRU evicts an `ra_` entry, a prepared database outage returns it through the stale path as a routing credential. Restrict stale snapshots to `ScopeRouting` and re-check the scope before returning the fallback.</violation>
</file>

<file name="internal/server/middleware/api_key_spend_cap.go">

<violation number="1" location="internal/server/middleware/api_key_spend_cap.go:74">
P1: When a prepared request falls back during a sustained outage, `err` is cleared before this block, so `setKey` refreshes the stale snapshot's 30-second TTL. Repeated traffic can therefore keep the old spend result alive indefinitely; track fallback use and cache only live successful reads.</violation>
</file>

Re-trigger cubic


func (c *BillingFailOpenCache) setBalance(key string, value billing.CheckResult) {
c.mu.Lock()
c.balances[key] = billingCacheEntry[billing.CheckResult]{value: value, expiresAt: time.Now().Add(billingFailOpenTTL)}

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: During a sustained prepared database outage, each fallback request re-arms its cache entry for another 30 seconds because the gates save the fallback result after replacing the error with nil. Preserve the original expiry or refresh the entry only after a fresh successful read, so fail-open remains bounded to the last known-good read.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/server/middleware/billing_fail_open_cache.go, line 37:

<comment>During a sustained prepared database outage, each fallback request re-arms its cache entry for another 30 seconds because the gates save the fallback result after replacing the error with nil. Preserve the original expiry or refresh the entry only after a fresh successful read, so fail-open remains bounded to the last known-good read.</comment>

<file context>
@@ -0,0 +1,84 @@
+
+func (c *BillingFailOpenCache) setBalance(key string, value billing.CheckResult) {
+	c.mu.Lock()
+	c.balances[key] = billingCacheEntry[billing.CheckResult]{value: value, expiresAt: time.Now().Add(billingFailOpenTTL)}
+	c.mu.Unlock()
+}
</file context>
Fix with cubic

Comment thread cmd/router/main.go
billingFailOpen = middleware.NewBillingFailOpenCache()
}
authSvc := auth.NewService(repo.Installations, repo.APIKeys, repo.ExternalAPIKeys, repo.Users, cache, userCache, time.Now).
WithDependencyPreparation(prepareDependencies, startDatabaseDependency).

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: With fail-open enabled, every unknown or otherwise invalid key can mark the shared database dependency unhealthy because the auth method passes logical auth errors to the starter's finish callback. Classify auth denials separately and report only infrastructure failures to dependency health, otherwise an invalid request briefly takes valid-key traffic offline.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/router/main.go, line 547:

<comment>With fail-open enabled, every unknown or otherwise invalid key can mark the shared database dependency unhealthy because the auth method passes logical auth errors to the starter's finish callback. Classify auth denials separately and report only infrastructure failures to dependency health, otherwise an invalid request briefly takes valid-key traffic offline.</comment>

<file context>
@@ -524,7 +524,27 @@ func main() {
+		billingFailOpen = middleware.NewBillingFailOpenCache()
+	}
 	authSvc := auth.NewService(repo.Installations, repo.APIKeys, repo.ExternalAPIKeys, repo.Users, cache, userCache, time.Now).
+		WithDependencyPreparation(prepareDependencies, startDatabaseDependency).
 		WithEncryptor(encryptor).
 		WithInstallationChangeNotifier(notifier).
</file context>
Fix with cubic

Comment thread cmd/router/main.go
prepared, _, _ := requestcontext.BeginPreparation(ctx, failOpenHealth, dependencyLimits)
return prepared
})
startDatabaseDependency := auth.DependencyStarter(func(ctx context.Context) (context.Context, func(error), error) {

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: When ROUTER_DEPENDENCY_FAIL_OPEN=false, an auth read failure can still authenticate from the stale cache. Leave the starter nil when fail-open is disabled so PrepareRequest does not mark the request as prepared.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/router/main.go, line 539:

<comment>When `ROUTER_DEPENDENCY_FAIL_OPEN=false`, an auth read failure can still authenticate from the stale cache. Leave the starter nil when fail-open is disabled so `PrepareRequest` does not mark the request as prepared.</comment>

<file context>
@@ -524,7 +524,27 @@ func main() {
+		prepared, _, _ := requestcontext.BeginPreparation(ctx, failOpenHealth, dependencyLimits)
+		return prepared
+	})
+	startDatabaseDependency := auth.DependencyStarter(func(ctx context.Context) (context.Context, func(error), error) {
+		return requestcontext.StartDependency(ctx, requestcontext.DependencyDatabase)
+	})
</file context>
Fix with cubic

}

if cache != nil {
cache.setOrg(installation.ID, result)

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: During a sustained database outage, each request served from the stale snapshot refreshes its TTL here, so the org cap can remain fail-open indefinitely instead of expiring 30 seconds after the last successful read. Track whether the result came from the live read and update the cache only for live successful reads.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/server/middleware/org_monthly_spend_cap.go, line 72:

<comment>During a sustained database outage, each request served from the stale snapshot refreshes its TTL here, so the org cap can remain fail-open indefinitely instead of expiring 30 seconds after the last successful read. Track whether the result came from the live read and update the cache only for live successful reads.</comment>

<file context>
@@ -52,6 +68,9 @@ func WithOrgMonthlySpendCap(svc *billing.Service) gin.HandlerFunc {
 		}
 
+		if cache != nil {
+			cache.setOrg(installation.ID, result)
+		}
 		if result.LimitReached() {
</file context>
Fix with cubic

subscriptionExempt := proxy.RequestPresentsCoveringSubscription(c.Request.Context(), c.Request.Header, c.FullPath())

result, err := svc.CheckBalance(c.Request.Context(), orgID)
if err != nil && cache != nil && requestcontext.PreparationFrom(c.Request.Context()) != nil {

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1: When the balance row is missing, this fallback converts billing.ErrBalanceRowMissing into a successful cached read before the 402 path runs. Exclude ErrBalanceRowMissing from fail-open fallback so only infrastructure failures can use the snapshot.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/server/middleware/balance_check.go, line 78:

<comment>When the balance row is missing, this fallback converts `billing.ErrBalanceRowMissing` into a successful cached read before the 402 path runs. Exclude `ErrBalanceRowMissing` from fail-open fallback so only infrastructure failures can use the snapshot.</comment>

<file context>
@@ -64,6 +75,11 @@ func WithBalanceCheck(svc *billing.Service, minBalanceMicros int64) gin.HandlerF
 		subscriptionExempt := proxy.RequestPresentsCoveringSubscription(c.Request.Context(), c.Request.Header, c.FullPath())
 
 		result, err := svc.CheckBalance(c.Request.Context(), orgID)
+		if err != nil && cache != nil && requestcontext.PreparationFrom(c.Request.Context()) != nil {
+			if cached, ok := cache.getBalance(orgID); ok {
+				result, err = cached, nil
</file context>
Suggested change
if err != nil && cache != nil && requestcontext.PreparationFrom(c.Request.Context()) != nil {
if err != nil && !errors.Is(err, billing.ErrBalanceRowMissing) && cache != nil && requestcontext.PreparationFrom(c.Request.Context()) != nil {
Fix with cubic

return nil, errors.New("subscription accounts are not configured")
}
callCtx, finish, startErr := startDependency(ctx)
if startErr != nil {

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: After an auth read fails and a stale key is returned, this early return skips the warm enrollment snapshot. Check the prepared cache when startDependency reports the already-latched database failure instead of returning before the fallback path.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/auth/subscription.go, line 113:

<comment>After an auth read fails and a stale key is returned, this early return skips the warm enrollment snapshot. Check the prepared cache when `startDependency` reports the already-latched database failure instead of returning before the fallback path.</comment>

<file context>
@@ -98,6 +98,41 @@ func (s *Service) ListSubscriptionAccounts(ctx context.Context, apiKeyID string)
+		return nil, errors.New("subscription accounts are not configured")
+	}
+	callCtx, finish, startErr := startDependency(ctx)
+	if startErr != nil {
+		return nil, startErr
+	}
</file context>
Fix with cubic

c.mu.Lock()
if instID != "" {
preGen = c.invalidationGen[instID]
c.stale[keyHash] = staleAPIKeyEntry{entry: entry, expiresAt: time.Now().Add(c.staleTTL)}

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Every positive Set adds to an unbounded stale map, and expired entries are cleaned only when that exact hash is requested. Use a capacity-bounded TTL cache or periodic cleanup so API-key churn cannot retain entries and grow process memory without bound.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/auth/apikey_cache.go, line 99:

<comment>Every positive `Set` adds to an unbounded stale map, and expired entries are cleaned only when that exact hash is requested. Use a capacity-bounded TTL cache or periodic cleanup so API-key churn cannot retain entries and grow process memory without bound.</comment>

<file context>
@@ -79,6 +96,13 @@ func (c *LRUAPIKeyCache) Set(keyHash string, entry CachedKey) {
 	c.mu.Lock()
 	if instID != "" {
 		preGen = c.invalidationGen[instID]
+		c.stale[keyHash] = staleAPIKeyEntry{entry: entry, expiresAt: time.Now().Add(c.staleTTL)}
+		staleHashes, ok := c.staleByInstallation[instID]
+		if !ok {
</file context>
Fix with cubic

// Start bounds a prerequisite call. Call finish exactly once with its result.
// A nil state preserves the legacy context and does not change error handling.
// StartDependency starts a bounded operation when request preparation is active.
func StartDependency(ctx context.Context, dependency Dependency) (context.Context, func(error), error) {

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: The new StartDependency function was inserted between Start's doc comment and the Start method, so Go now attaches the "Start bounds a prerequisite call..." comment to StartDependency and leaves the exported *Preparation.Start method with no godoc at all. This violates the repo convention that every exported symbol carries a godoc starting with the symbol name. Move the StartDependency block (with its own comment) above the "// Start bounds..." comment, or place it after Start, so Start keeps its documentation.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/requestcontext/fail_open.go, line 183:

<comment>The new `StartDependency` function was inserted between `Start`'s doc comment and the `Start` method, so Go now attaches the "Start bounds a prerequisite call..." comment to `StartDependency` and leaves the exported `*Preparation.Start` method with no godoc at all. This violates the repo convention that every exported symbol carries a godoc starting with the symbol name. Move the `StartDependency` block (with its own comment) above the "// Start bounds..." comment, or place it after `Start`, so `Start` keeps its documentation.</comment>

<file context>
@@ -179,6 +179,15 @@ func (p *Preparation) Fail(dependency Dependency, err error) error {
 // Start bounds a prerequisite call. Call finish exactly once with its result.
 // A nil state preserves the legacy context and does not change error handling.
+// StartDependency starts a bounded operation when request preparation is active.
+func StartDependency(ctx context.Context, dependency Dependency) (context.Context, func(error), error) {
+	preparation := PreparationFrom(ctx)
+	if preparation == nil {
</file context>
Fix with cubic

finish(err)
if err == nil {
s.subscriptionMu.Lock()
s.subscriptionCache[apiKeyID] = subscriptionCacheEntry{accounts: accounts, expiresAt: time.Now().Add(30 * time.Second)}

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: Use the service's injected clock for snapshot expiry instead of calling time.Now() directly, so cache behavior remains deterministic and follows the auth package's clock convention.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/auth/subscription.go, line 120:

<comment>Use the service's injected clock for snapshot expiry instead of calling `time.Now()` directly, so cache behavior remains deterministic and follows the auth package's clock convention.</comment>

<file context>
@@ -98,6 +98,41 @@ func (s *Service) ListSubscriptionAccounts(ctx context.Context, apiKeyID string)
+	finish(err)
+	if err == nil {
+		s.subscriptionMu.Lock()
+		s.subscriptionCache[apiKeyID] = subscriptionCacheEntry{accounts: accounts, expiresAt: time.Now().Add(30 * time.Second)}
+		s.subscriptionMu.Unlock()
+		return accounts, nil
</file context>
Fix with cubic


require.Equal(t, http.StatusOK, authProbe(t, svc, routerToken, nil).Code, "warm the cache with a successful read")
// The positive LRU expires almost immediately; only the outage copy remains.
time.Sleep(5 * time.Millisecond)

@cubic-dev-ai cubic-dev-ai Bot Sep 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: These tests rely on wall-clock TTL expiry to decide the served path: the positive LRU is created with a 1ms TTL and the test sleeps 5ms, assuming the expirable LRU's background janitor has already evicted the positive entry. That assumption is load-bearing only in the "switch off" subtest, where a still-cached positive entry would make the second probe return 200 from cache instead of the asserted 503, and it silently weakens TestWithAuthServesRecentKeyDuringDatabaseOutage: that test passes even if the positive entry is what serves the key, so it does not strictly prove the stale copy stood in during the outage. Make the stale-path isolation deterministic (e.g. assert the repo was actually queried, or control eviction explicitly) instead of relying on sleep vs janitor timing.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/server/middleware/fail_open_test.go, line 82:

<comment>These tests rely on wall-clock TTL expiry to decide the served path: the positive LRU is created with a 1ms TTL and the test sleeps 5ms, assuming the expirable LRU's background janitor has already evicted the positive entry. That assumption is load-bearing only in the "switch off" subtest, where a still-cached positive entry would make the second probe return 200 from cache instead of the asserted 503, and it silently weakens TestWithAuthServesRecentKeyDuringDatabaseOutage: that test passes even if the positive entry is what serves the key, so it does not strictly prove the stale copy stood in during the outage. Make the stale-path isolation deterministic (e.g. assert the repo was actually queried, or control eviction explicitly) instead of relying on sleep vs janitor timing.</comment>

<file context>
@@ -0,0 +1,231 @@
+
+	require.Equal(t, http.StatusOK, authProbe(t, svc, routerToken, nil).Code, "warm the cache with a successful read")
+	// The positive LRU expires almost immediately; only the outage copy remains.
+	time.Sleep(5 * time.Millisecond)
+
+	repo.lookupErr = errors.New("connection refused")
</file context>
Fix with cubic

Signed-off-by: Drew Bailey <drew@workweave.ai>
@makosblade
makosblade force-pushed the fail-open/3-auth-enrollment-spend branch from ac274d9 to 7b7fe82 Compare September 11, 2026 23:37

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7b7fe82. Configure here.

if cached, ok := cache.getBalance(orgID); ok {
result, err = cached, nil
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Spend gates skip the shared DB budget

Medium Severity

Balance, API-key cap, and org monthly cap call the billing service on the raw request context and only inspect PreparationFrom after the read fails. They never call StartDependency, so they ignore the 250ms/1s database budget that auth now starts. A hung billing read can consume the remaining 12s preparation deadline before the snapshot is used, cancelling the request context for everything downstream.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7b7fe82. Configure here.

staleHashes = make(map[string]struct{}, 1)
c.staleByInstallation[instID] = staleHashes
}
staleHashes[keyHash] = struct{}{}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale key map grows without bound

Medium Severity

LRUAPIKeyCache.Set writes every positive key into an unbounded stale map. Expired entries are removed only from GetStale, which runs on outage fallback, not on the healthy path. The 30s copy therefore retains CachedKey values, including BYOK material, for the process lifetime. subscriptionCache has the same write-only growth.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7b7fe82. Configure here.

@makosblade makosblade closed this Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant