Fail closed on a missing provider key instead of sending an unauthenticated request - #2566
Conversation
…icated request An `ai-sdk:*` engine with no resolved API key built its provider factory with no `apiKey` at all, so the SDK omitted the Authorization header and the gateway answered 401 "Missing Authentication header". `classifyProviderError` coded that `http_401` — a transport failure naming the wrong cause — and a scheduled job retried the same doomed unauthenticated request on every tick. Clips' `video-view-notifier` failed this way 18/18 times in the 8h after the last deploy, on an app configured for `ai-sdk:openrouter` with no OPENROUTER_API_KEY. `builder-engine` and `anthropic-engine` already failed closed here; this engine was the only one that did not. Also stop reaping in-process background automations at the tight 45s post-claim stale window. That window exists only to reach a durable successor sooner, but a self-claimed automation run carries no `dispatch_payload` and has no successor to reach, so an early reap killed still-working scheduled jobs that nothing could then recover. Those rows now get the 90s background window, and the recovery path reports `not_redispatchable` instead of `payload_missing`, which read as data loss in production forensics for the one case where nothing was ever lost.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
There was a problem hiding this comment.
Builder reviewed your changes — looks good ✅
Review Details
Code Review Summary
PR #2566 addresses two reliability failures in the core agent runtime. First, AISDKEngine now fails closed with missing_credentials before constructing a provider or sending a request when a required provider key is absent, while preserving keyless custom baseUrl gateways and providers such as Ollama. Second, stale-run recovery distinguishes in-process background automations from HTTP-redispatchable runs: payload-less runs receive the wider background stale window and are reported as not_redispatchable rather than implying payload loss.
This is a high-risk change because it affects credential handling and background-run recovery, but the implementation is consistent with the existing engine error contract and run-store recovery flow. The regression tests cover missing keys, explicit keys, custom gateways, and both stale-window variants. Independent reviews found no confirmed bugs or compatibility issues.
✅ Good patterns: fail-closed behavior avoids unauthenticated provider requests; the baseUrl exemption is explicit; stale recovery semantics are documented and tested; diagnostic naming now reflects the actual condition.
🧪 Browser testing: Skipped — PR only modifies backend/config/docs/tests, no UI impact

Follow-up measurement of the 2026-07-31 reliability wave (#2556, deployed 17:41Z) found the fixes largely worked, and surfaced two things that did not.
What moved (matched 7.75h windows either side of the deploy, 13 prod app DBs)
unknown(the 182-run baseline bucket)no_progressdispatch_payload_missingrun_timeoutunknownWhat did not move — fixed here
Clips
video-view-notifier: still 18/18 failing. The "skip gateway-rejected Builder credentials" fix could not help, because this never went through Builder credential selection. The app runsai-sdk:openrouterand its production environment has noOPENROUTER_API_KEY.AISDKEngineresolved no key, omittedconfig.apiKeyentirely, and the SDK sent a request with no Authorization header — hence the literal upstreamMissing Authentication header.classifyProviderErrorcoded ithttp_401, a transport error naming the wrong cause, and the job retried it every 30 minutes forever. The deploy only changed the classification (unknown→http_401); the outage was identical.builder-engineandanthropic-engineboth already fail closed withmissing_credentialsin this situation.ai-sdk-enginewas the only engine that did not. It now does, and names the env var it wants. A configuredbaseUrlstays exempt, since a self-hosted gateway may legitimately need no key.This makes the failure diagnosable and stops the retry loop. It does not by itself make that job succeed — the clips app still needs a key or a different engine, which is app configuration, not code.
New
stale_runcode, 73 runs. Not a regression so much as a relocation:runBackgroundAutomationnow self-claims its run intobackground-processing, which subjects it to the 45s post-claim stale window. That tight window exists to reach a durable successor sooner — but a self-claimed automation carries nodispatch_payload, soattemptStaleRunRecoverycan never build a successor for it. Reaping early was therefore pure loss: it killed still-working scheduled jobs (they died at 45–185s) that nothing could recover. Those rows now get the 90s background window; genuine HTTP workers keep the tight one, where an early reap does buy something.The recovery path also reported this as
payload_missing, which reads as "we are losing payloads" for the one case where nothing was ever lost — every HTTP-dispatched run writes its payload in the same INSERT that creates the row, so a payload-less row was simply never HTTP-dispatched. Renamednot_redispatchable.Verification
Four tests, each confirmed to fail against the pre-fix code and pass with it (guard temporarily reverted, tests re-run, then restored).
pnpm guards— all passtsc --noEmit -p packages/core— cleanvitest run src/agent/run-store src/agent/engine— 427 passed / 21 filesNot addressed
dispatchanddesignscheduled jobs had 0 runs in either window, so the 100%-failure baseline for them could not be re-measured here; their last runs were 07-28/07-29 and showed the oldno_progress/background_worker_never_startedcodes.