🔁 feat: Retry OpenID Discovery After Startup Failures - #15899
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93fb3a1e99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async function registerOpenIdStrategies() { | ||
| const config = await setupOpenId(); | ||
| if (!config) { | ||
| return false; |
There was a problem hiding this comment.
Move OpenID registration behavior out of /api
This newly added helper puts behavioral logic in the legacy CJS layer: it calls discovery, branches on its result, conditionally constructs/registers a strategy, and determines success for the retry policy. Move this behavior into packages/api and inject the strategy/discovery dependencies so this file only wires configuration and calls the TypeScript module, as required for edits to existing /api files.
AGENTS.md reference: AGENTS.md:L53-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 92b2725. The discovery result check, the token-reuse branch, strategy registration and error handling now live in registerOpenIdWithRetry (packages/api/src/auth/openid/retry.ts). socialLogins.js only passes in setupOpenId, a registerJwtStrategy callback, reuseTokens and the raw config sources. Covered by retry.spec.ts; socialLogins.spec.js checks the wiring.
| startupAttempts: z.number().int().min(0).max(100).optional(), | ||
| /** Milliseconds between startup and background attempts. Default 5000. */ | ||
| retryDelayMs: z.number().int().min(100).max(3_600_000).optional(), |
There was a problem hiding this comment.
Define the retry defaults in configSchema
Both new fields are optional without schema defaults, while 3 and 5000 are independently hard-coded in retry.ts and repeated in the example YAML. Consequently getConfigDefaults() cannot provide these defaults and the three definitions can drift; resolve the legacy environment override separately, but make configSchema the canonical definition of the effective defaults.
AGENTS.md reference: AGENTS.md:L66-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 92b2725. openIdDiscoverySchema in config.ts defines the defaults (1 and 5000). registration.openidDiscovery uses openIdDiscoverySchema.partial(), so an unset YAML field stays unset and the env var can still fill it. resolveOpenIdDiscovery falls back to schema.shape[field].parse(undefined), and retry.ts no longer hard-codes either value.
| const DEFAULT_STARTUP_ATTEMPTS = 3; | ||
| const DEFAULT_RETRY_DELAY_MS = 5000; |
There was a problem hiding this comment.
Preserve the existing one-attempt startup default
When an OpenID deployment omits both new settings and its provider is unavailable, this default changes startup from one discovery request followed by immediate continuation to three requests separated by two five-second waits. That adds at least ten seconds to every affected restart, contrary to the repository requirement that a new configurable lever preserve existing default behavior; default to one startup attempt and leave additional blocking retries opt-in.
AGENTS.md reference: AGENTS.md:L66-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 92b2725. The default is now one startup attempt, so startup takes no longer than it did before. Blocking retries at startup are opt-in via startupAttempts, and retries after startup only run in the background with an unref'd timer. Covered by 'makes one startup attempt by default, then recovers in the background'.
| function parseRetryDelay(value: string | number | undefined): number { | ||
| const delay = Number(value); | ||
| if (value == null || value === '' || !Number.isFinite(delay) || delay <= 0) { | ||
| return DEFAULT_RETRY_DELAY_MS; | ||
| } | ||
| return delay; |
There was a problem hiding this comment.
Enforce the retry-delay bounds for environment values
When OPENID_DISCOVERY_RETRY_DELAY_MS supplies the fallback, any finite positive value bypasses the 100..3,600,000 bounds enforced for the equivalent YAML field. For example, 1 creates an indefinite millisecond retry loop against an unavailable provider, while a value above Node's timer range is coerced to roughly 1 ms; apply the same bounds before scheduling the timer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 92b2725. Env values are checked against the same field schema as the YAML. A value that is out of bounds, not a whole number or unparseable is logged and replaced by the default. Covered by the table test for 1, 99999999999, soon, -1 and 1.5.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Continues #15617 by @insoln. Their three commits are kept as-is (rebased onto
dev) so the work stays credited to them. The commits on top addlibrechat.yamlsettings, move strategy registration intopackages/api, and makeconfigSchemathe single place the defaults are defined.Summary
What breaks: LibreChat tries OpenID discovery once at startup. If the identity provider or the network is down at that moment, the OpenID strategy is never registered, and OpenID login keeps failing until someone restarts the server.
After this change:
openidstrategy (andopenidJwtwhen token reuse is on) is registered without a restart.Configuration. Each field resolves separately:
librechat.yamlfirst, then the env var, then theopenIdDiscoverySchemadefault. An env value outside the schema bounds is logged and ignored.librechat.yaml0= background retries only)registration.openidDiscovery.startupAttemptsOPENID_DISCOVERY_RETRY_ATTEMPTSregistration.openidDiscovery.retryDelayMsOPENID_DISCOVERY_RETRY_DELAY_MSMechanism
Change Type
Testing
packages/api:npx jest src/auth/openid/retry.spec.ts, 11/11 pass. Covers:0startup attempts1,99999999999,soon,-1,1.5) falling back to the defaultsapi:npx jest server/socialLogins.spec.js, 6/6 pass. Covers the wiring passed to the TS module and the existing session and token-reuse behavior.packages/data-provider:npx tsc --noEmitis clean.packages/api:tsc --noEmitshows no errors outside the existingsrc/agentsProviderstype mismatch.