fix: make sticky session routing cost-aware - #2596
Merged
Merged
Conversation
Sticky session routing previously pinned each session to a provider via rendezvous hashing, which ignored the weighted-score algorithm entirely. ~1/N of sessions got pinned to whichever provider won the hash — including the most expensive one — bypassing price weight and provider priority. Now the session's provider is chosen with the normal weighted-score algorithm (price, priority, uptime, throughput) and then persisted per session via a Redis-backed store. Subsequent requests reuse the saved provider to keep upstream prompt caches warm; the pin only moves when the provider leaves the candidate list or its uptime drops below the session uptime threshold, at which point the session is re-scored and re-pinned. - Replace the rendezvous hash in getCheapestFromAvailableProviders with an injected SessionProviderStore (get/set), matching the existing providerDiscountResolver injection pattern. - Add createSessionProviderStore (Redis, keyed per org/model/session) and wire it into the real routing decision points; skip exploration and the org+model preferred-provider hysteresis while a session is pinned. - Add session.ttlSeconds and session.uptimeThreshold routing config, with API schema + docs updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR migrates session-sticky provider routing from stateless ChangesSession-sticky provider routing migration
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40654934a1
ℹ️ 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".
Comment on lines
+1971
to
+1974
| const sessionStickyEnabled = Boolean(sessionId) && routingCfg.session.enabled; | ||
| const createSessionStore = (modelId: string) => | ||
| sessionStickyEnabled && sessionId | ||
| ? createSessionProviderStore( |
There was a problem hiding this comment.
When a request has both a session id and an explicit provider model such as openai/<model>, this enables the Redis-backed session store before the direct-provider routing branch runs. That branch only has the requested provider in its candidate list, so applySessionSticky treats any existing pin to another healthy provider as unavailable and overwrites it; subsequent unpinned requests for the same org/model/session are then forced to the one-off explicit provider instead of staying on the weighted-best session pin. Gate the session store off for direct provider requests, or avoid writing the store from those constrained routing paths.
Useful? React with 👍 / 👎.
Add unit tests for the new weighted-pick-then-persist session stickiness: - get-cheapest selection (metrics-driven, deterministic fixtures): picks the weighted-score winner for a new session and persists it; keeps a session on its pinned provider even when a cheaper one is available; refreshes the pin's TTL on reuse; re-pins to the best provider when the pinned one's uptime drops below the session threshold; respects the exact-threshold boundary and a custom uptimeThreshold; ignores the store when stickiness is disabled; and reuses/abandons a pinned region correctly. - createSessionProviderStore (Redis-mocked): key scoping per org/model/session, get/set round-trip, ttl on write, region-less pins, and error swallowing on read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/src/chat/chat.ts (1)
2641-2649:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftApply session stickiness before collapsing regions.
createSessionProviderStore(...)persists bothproviderIdandregion, andpackages/actions/src/get-cheapest-from-available-providers.ts:413-459only reuses the pin when that exact region is still present incandidates. These paths callcollapseProvidersToBestRegionPerProvider(...)first, so a session pinned to (for example)alibaba/cn-beijingcan be reduced to a different Alibaba region before the sticky lookup even runs. The next request then looks like a cache miss and overwrites the stored region, which defeats the cache-warming affinity this feature is trying to preserve. Either run sticky selection before the per-provider region collapse, or make the persisted session contract provider-only if region affinity is not intended.Also applies to: 3277-3289, 3484-3495
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/gateway/src/chat/chat.ts` around lines 2641 - 2649, The session stickiness is applied after regions have been collapsed, causing pinned sessions (createSessionProviderStore / createSessionStore) to mismatch when collapseProvidersToBestRegionPerProvider reduces a provider's region; move the sticky selection call (the logic that reads/writes the session pin used by getCheapestFromAvailableProviders) so it runs before collapseProvidersToBestRegionPerProvider, ensuring the persisted region/provider pair is consulted against the full candidate set, or alternatively change createSessionProviderStore/createSessionStore to persist only providerId (not region) and update getCheapestFromAvailableProviders to treat stored pins as provider-only; update calls around getCheapestFromAvailableProviders and the helper collapseProvidersToBestRegionPerProvider accordingly (also apply same change at the other occurrences referenced).
🧹 Nitpick comments (3)
packages/shared/src/routing-config.spec.ts (1)
131-138: ⚡ Quick winAdd a session TTL upper-bound clamp test.
This test validates
ttlSecondsminimum but not its ceiling. Add a case (e.g.,ttlSeconds: 999999) to lock in the 86,400 max behavior and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/routing-config.spec.ts` around lines 131 - 138, Extend the "clamps session overrides into valid ranges" unit test for resolveRoutingConfig to also assert upper-bound clamping for session.ttlSeconds: pass a large ttlSeconds (e.g., 999999) in the overrides alongside providerDefaults, then expect resolved.session.ttlSeconds to equal 86400; keep the existing uptimeThreshold assertion (100) and ensure you call resolveRoutingConfig with the same shape used in the current test so the new assertion validates the max TTL behavior.apps/api/src/routes/routing-config.ts (1)
247-248: ⚡ Quick winUse the same numeric bounds in response schemas as request schemas.
These fields are bounded in
sessionSchema(Line 170-171) but unconstrained inresolvedConfigSchemaand/defaultsresponse schemas. Mirroring bounds here keeps the OpenAPI contract consistent for clients.Suggested schema tightening
session: z.object({ enabled: z.boolean(), - ttlSeconds: z.number(), - uptimeThreshold: z.number(), + ttlSeconds: z.number().int().min(1).max(86_400), + uptimeThreshold: z.number().min(0).max(100), }),session: z.object({ enabled: z.boolean(), - ttlSeconds: z.number(), - uptimeThreshold: z.number(), + ttlSeconds: z.number().int().min(1).max(86_400), + uptimeThreshold: z.number().min(0).max(100), }),Also applies to: 510-511
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/routes/routing-config.ts` around lines 247 - 248, The response schemas (resolvedConfigSchema and the /defaults response) currently declare ttlSeconds and uptimeThreshold as z.number() without bounds; match the numeric constraints used in sessionSchema (the same min/max used there) by replacing those unconstrained z.number() definitions with the identical bounded z.number().refine or z.number().min(...).max(...) expressions used in sessionSchema so the OpenAPI contract is consistent for ttlSeconds and uptimeThreshold across requests and responses.packages/actions/src/models.spec.ts (1)
581-608: ⚡ Quick winAdd an explicit uptime-threshold re-pin test.
This sticky suite validates missing-provider re-pin, but not the “saved provider uptime below
session.uptimeThreshold” invalidation path. Add one focused test to lock that behavior.Suggested test shape
+ it("re-pins when saved provider uptime drops below session threshold", async () => { + if (!modelWithMultipleProviders) { + return; + } + const availableProviders = modelWithMultipleProviders.providers.filter( + (p) => p.inputPrice !== undefined && p.outputPrice !== undefined, + ); + if (availableProviders.length <= 1) { + return; + } + + const pinned = availableProviders[0]; + const fallback = availableProviders[1]; + const store = createMemoryStore({ + providerId: pinned.providerId, + region: pinned.region, + }); + + const overrides = resolveRoutingConfig( + { session: { enabled: true, uptimeThreshold: 95 } }, + buildProviderPriorityDefaults(), + ); + + const metricsMap = new Map([ + [ + metricsKey(modelWithMultipleProviders.id, pinned.providerId, pinned.region), + { modelId: modelWithMultipleProviders.id, providerId: pinned.providerId, uptime: 70, averageLatency: 100, throughput: 100, totalRequests: 10 }, + ], + [ + metricsKey(modelWithMultipleProviders.id, fallback.providerId, fallback.region), + { modelId: modelWithMultipleProviders.id, providerId: fallback.providerId, uptime: 99.9, averageLatency: 100, throughput: 100, totalRequests: 10 }, + ], + ]); + + const result = await getCheapestFromAvailableProviders( + availableProviders, + modelWithMultipleProviders, + { sessionProviderStore: store, routingConfig: overrides, metricsMap }, + ); + + expect(result?.provider.providerId).not.toBe(pinned.providerId); + expect(store.value?.providerId).toBe(result?.provider.providerId); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/actions/src/models.spec.ts` around lines 581 - 608, Add a new unit test in the same "re-pins to the current best when the saved provider is gone" suite that specifically verifies re-pinning when the saved provider's uptime is below session.uptimeThreshold: create a memory store via createMemoryStore with providerId set to one of modelWithMultipleProviders.providers but set that provider's uptime to a value below the session.uptimeThreshold (or set session.uptimeThreshold high and the provider uptime low), call getCheapestFromAvailableProviders(availableProviders, modelWithMultipleProviders, { sessionProviderStore: store }), and assert that the returned result.metadata.selectionReason is "session-sticky", result.provider.providerId is not the saved providerId, and store.value?.providerId equals the new selected providerId; use modelWithMultipleProviders and availableProviders as in the existing test to locate where to add this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/content/features/routing.mdx`:
- Line 181: Edit the sentence that begins "On a session's **first** request..."
to remove the enumerated factors and instead refer to the generic algorithm;
specifically replace "— the same price-, priority-, uptime-, and
throughput-aware algorithm used for non-sticky requests." with "— the same
weighted smart-routing algorithm used for non-sticky requests." so the paragraph
reads that the provider is chosen by the same weighted smart-routing algorithm
and that choice is persisted for the session.
In `@apps/gateway/src/lib/preferred-provider.ts`:
- Around line 97-103: sessionRedisKey currently embeds the raw, user-controlled
sessionId into the Redis key which leaks identifiers and risks delimiter
collisions; change sessionRedisKey to hash the sessionId before composing the
key (e.g., compute a stable SHA-256/hex or HMAC digest of sessionId) and use
that digest (sessionHash) instead of the raw sessionId in
`session_provider:${orgId}:${modelId}:${sessionHash}`; import/use the project's
crypto utility (or Node's crypto) to compute the digest so keys remain
deterministic and safe.
In `@packages/actions/src/get-cheapest-from-available-providers.ts`:
- Around line 149-151: applySessionSticky currently uses
SessionProviderStore.get() followed by an unconditional set(), which allows a
race where two concurrent first requests both see a miss and both write
different providers; make the "first sticky pin" atomic by adding an atomic
claim method to SessionProviderStore (e.g., claim(providerId: string, region?:
string, ttlSeconds?: number): Promise<boolean>) and change applySessionSticky to
call claim() for the first pin instead of get()+set(); implement the Redis
backend to use SET key value NX EX ttl so only one caller wins the claim and
subsequent callers fall back to reading the existing provider via get() and/or
using set() only when replacing an already-held claim.
In `@packages/shared/src/routing-config.ts`:
- Around line 66-69: Update the docstring for the repin-condition in
routing-config.ts to reflect actual behavior: state that a sticky pin is broken
either when the pinned provider's uptime drops below the configured percentage
OR when the pinned provider becomes no longer eligible for selection; locate the
JSDoc/comment block associated with the repin-condition (the comment above
repinCondition/repin-condition constant or function) and change the wording so
it lists both triggers (uptime threshold and eligibility) and clarifies that
either condition will cause a re-score and potential re-pin.
- Around line 193-194: The ttlSeconds field in resolveRoutingConfig
(routing-config.ts) only enforces a minimum via Math.max and can exceed the
API's 86,400 upper bound; update the ttlSeconds normalization for cfg.ttlSeconds
to also clamp to 86,400 so non-API callers match the schema (i.e., apply both
min 1 and max 86400 when flooring cfg.ttlSeconds) and leave uptimeThreshold
clamping as-is.
---
Outside diff comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 2641-2649: The session stickiness is applied after regions have
been collapsed, causing pinned sessions (createSessionProviderStore /
createSessionStore) to mismatch when collapseProvidersToBestRegionPerProvider
reduces a provider's region; move the sticky selection call (the logic that
reads/writes the session pin used by getCheapestFromAvailableProviders) so it
runs before collapseProvidersToBestRegionPerProvider, ensuring the persisted
region/provider pair is consulted against the full candidate set, or
alternatively change createSessionProviderStore/createSessionStore to persist
only providerId (not region) and update getCheapestFromAvailableProviders to
treat stored pins as provider-only; update calls around
getCheapestFromAvailableProviders and the helper
collapseProvidersToBestRegionPerProvider accordingly (also apply same change at
the other occurrences referenced).
---
Nitpick comments:
In `@apps/api/src/routes/routing-config.ts`:
- Around line 247-248: The response schemas (resolvedConfigSchema and the
/defaults response) currently declare ttlSeconds and uptimeThreshold as
z.number() without bounds; match the numeric constraints used in sessionSchema
(the same min/max used there) by replacing those unconstrained z.number()
definitions with the identical bounded z.number().refine or
z.number().min(...).max(...) expressions used in sessionSchema so the OpenAPI
contract is consistent for ttlSeconds and uptimeThreshold across requests and
responses.
In `@packages/actions/src/models.spec.ts`:
- Around line 581-608: Add a new unit test in the same "re-pins to the current
best when the saved provider is gone" suite that specifically verifies
re-pinning when the saved provider's uptime is below session.uptimeThreshold:
create a memory store via createMemoryStore with providerId set to one of
modelWithMultipleProviders.providers but set that provider's uptime to a value
below the session.uptimeThreshold (or set session.uptimeThreshold high and the
provider uptime low), call getCheapestFromAvailableProviders(availableProviders,
modelWithMultipleProviders, { sessionProviderStore: store }), and assert that
the returned result.metadata.selectionReason is "session-sticky",
result.provider.providerId is not the saved providerId, and
store.value?.providerId equals the new selected providerId; use
modelWithMultipleProviders and availableProviders as in the existing test to
locate where to add this case.
In `@packages/shared/src/routing-config.spec.ts`:
- Around line 131-138: Extend the "clamps session overrides into valid ranges"
unit test for resolveRoutingConfig to also assert upper-bound clamping for
session.ttlSeconds: pass a large ttlSeconds (e.g., 999999) in the overrides
alongside providerDefaults, then expect resolved.session.ttlSeconds to equal
86400; keep the existing uptimeThreshold assertion (100) and ensure you call
resolveRoutingConfig with the same shape used in the current test so the new
assertion validates the max TTL behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cabd2858-9ad3-4a98-86bc-e251a183e7e9
📒 Files selected for processing (9)
apps/api/src/routes/routing-config.tsapps/docs/content/features/routing.mdxapps/docs/content/features/sessions.mdxapps/gateway/src/chat/chat.tsapps/gateway/src/lib/preferred-provider.tspackages/actions/src/get-cheapest-from-available-providers.tspackages/actions/src/models.spec.tspackages/shared/src/routing-config.spec.tspackages/shared/src/routing-config.ts
| #### How pinning works | ||
|
|
||
| When a session id is present, the provider (and region) is chosen **deterministically** using [rendezvous hashing](https://en.wikipedia.org/wiki/Rendezvous_hashing) over the available providers, rather than the weighted score. The same session always maps to the same provider as long as that provider remains available. | ||
| On a session's **first** request the provider is chosen by the normal weighted smart-routing score — the same price-, priority-, uptime-, and throughput-aware algorithm used for non-sticky requests. That choice is then **persisted for the session** and reused on every subsequent request, so the upstream prompt cache stays warm without bouncing the conversation between providers. |
Contributor
There was a problem hiding this comment.
Avoid listing only a subset of weighted factors as if exhaustive.
This sentence can be read as the full scoring formula, but weighted routing also conditionally includes latency/cache (and image price for image models). Prefer “same weighted smart-routing algorithm” without enumerating factors.
Suggested wording
-On a session's **first** request the provider is chosen by the normal weighted smart-routing score — the same price-, priority-, uptime-, and throughput-aware algorithm used for non-sticky requests. That choice is then **persisted for the session** and reused on every subsequent request, so the upstream prompt cache stays warm without bouncing the conversation between providers.
+On a session's **first** request the provider is chosen by the normal weighted smart-routing score — the same weighted algorithm used for non-sticky requests. That choice is then **persisted for the session** and reused on every subsequent request, so the upstream prompt cache stays warm without bouncing the conversation between providers.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| On a session's **first** request the provider is chosen by the normal weighted smart-routing score — the same price-, priority-, uptime-, and throughput-aware algorithm used for non-sticky requests. That choice is then **persisted for the session** and reused on every subsequent request, so the upstream prompt cache stays warm without bouncing the conversation between providers. | |
| On a session's **first** request the provider is chosen by the normal weighted smart-routing score — the same weighted algorithm used for non-sticky requests. That choice is then **persisted for the session** and reused on every subsequent request, so the upstream prompt cache stays warm without bouncing the conversation between providers. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/docs/content/features/routing.mdx` at line 181, Edit the sentence that
begins "On a session's **first** request..." to remove the enumerated factors
and instead refer to the generic algorithm; specifically replace "— the same
price-, priority-, uptime-, and throughput-aware algorithm used for non-sticky
requests." with "— the same weighted smart-routing algorithm used for non-sticky
requests." so the paragraph reads that the provider is chosen by the same
weighted smart-routing algorithm and that choice is persisted for the session.
Comment on lines
+97
to
+103
| function sessionRedisKey( | ||
| orgId: string, | ||
| modelId: string, | ||
| sessionId: string, | ||
| ): string { | ||
| return `session_provider:${orgId}:${modelId}:${sessionId}`; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid putting raw session IDs in Redis keys.
sessionId is user-controlled, so embedding it directly in session_provider:${orgId}:${modelId}:${sessionId} exposes that identifier in Redis keyspace inspection, monitoring, slow logs, and backups. Hash the session component before composing the key; that also removes delimiter-collision risk from ids containing :.
Suggested change
+import { createHash } from "node:crypto";
import { redisClient } from "`@llmgateway/cache`";
import { logger } from "`@llmgateway/logger`";
@@
+function hashKeyPart(value: string): string {
+ return createHash("sha256").update(value).digest("hex");
+}
+
function sessionRedisKey(
orgId: string,
modelId: string,
sessionId: string,
): string {
- return `session_provider:${orgId}:${modelId}:${sessionId}`;
+ return `session_provider:${orgId}:${modelId}:${hashKeyPart(sessionId)}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function sessionRedisKey( | |
| orgId: string, | |
| modelId: string, | |
| sessionId: string, | |
| ): string { | |
| return `session_provider:${orgId}:${modelId}:${sessionId}`; | |
| } | |
| import { createHash } from "node:crypto"; | |
| import { redisClient } from "`@llmgateway/cache`"; | |
| import { logger } from "`@llmgateway/logger`"; | |
| function hashKeyPart(value: string): string { | |
| return createHash("sha256").update(value).digest("hex"); | |
| } | |
| function sessionRedisKey( | |
| orgId: string, | |
| modelId: string, | |
| sessionId: string, | |
| ): string { | |
| return `session_provider:${orgId}:${modelId}:${hashKeyPart(sessionId)}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/gateway/src/lib/preferred-provider.ts` around lines 97 - 103,
sessionRedisKey currently embeds the raw, user-controlled sessionId into the
Redis key which leaks identifiers and risks delimiter collisions; change
sessionRedisKey to hash the sessionId before composing the key (e.g., compute a
stable SHA-256/hex or HMAC digest of sessionId) and use that digest
(sessionHash) instead of the raw sessionId in
`session_provider:${orgId}:${modelId}:${sessionHash}`; import/use the project's
crypto utility (or Node's crypto) to compute the digest so keys remain
deterministic and safe.
Comment on lines
+149
to
+151
| export interface SessionProviderStore { | ||
| get: () => Promise<SessionProviderEntry | null>; | ||
| set: (providerId: string, region?: string) => Promise<void>; |
Contributor
There was a problem hiding this comment.
Make the first sticky pin atomic.
applySessionSticky currently does get() and then unconditional set(). With the Redis backend in apps/gateway/src/lib/preferred-provider.ts also doing a plain SET, two concurrent first requests for the same session can both observe a miss, route to different providers, and let the last write win. That breaks the “one provider per session” invariant and can split prompt-cache warming across providers.
Suggested direction
export interface SessionProviderStore {
get: () => Promise<SessionProviderEntry | null>;
+ claim: (providerId: string, region?: string) => Promise<boolean>;
set: (providerId: string, region?: string) => Promise<void>;
}- await store.set(
- naturalResult.provider.providerId,
- naturalResult.provider.region,
- );
+ const claimed = await store.claim(
+ naturalResult.provider.providerId,
+ naturalResult.provider.region,
+ );
+ if (!claimed) {
+ const existing = await store.get();
+ if (existing) {
+ // reuse the already-pinned provider instead of overwriting it
+ }
+ }Using a Redis SET ... NX EX ... implementation for claim() is enough to remove the race.
Also applies to: 421-451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/actions/src/get-cheapest-from-available-providers.ts` around lines
149 - 151, applySessionSticky currently uses SessionProviderStore.get() followed
by an unconditional set(), which allows a race where two concurrent first
requests both see a miss and both write different providers; make the "first
sticky pin" atomic by adding an atomic claim method to SessionProviderStore
(e.g., claim(providerId: string, region?: string, ttlSeconds?: number):
Promise<boolean>) and change applySessionSticky to call claim() for the first
pin instead of get()+set(); implement the Redis backend to use SET key value NX
EX ttl so only one caller wins the claim and subsequent callers fall back to
reading the existing provider via get() and/or using set() only when replacing
an already-held claim.
Comment on lines
+66
to
+69
| * When the pinned provider's uptime drops below this percentage the session | ||
| * is re-scored and pinned to the current best provider instead. This is the | ||
| * only thing that breaks an established pin. | ||
| */ |
Contributor
There was a problem hiding this comment.
Update the repin-condition docstring to match behavior.
Lines 66-69 say uptime is the only repin trigger, but sticky re-pinning also occurs when the pinned provider is no longer eligible. This comment is now misleading.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/src/routing-config.ts` around lines 66 - 69, Update the
docstring for the repin-condition in routing-config.ts to reflect actual
behavior: state that a sticky pin is broken either when the pinned provider's
uptime drops below the configured percentage OR when the pinned provider becomes
no longer eligible for selection; locate the JSDoc/comment block associated with
the repin-condition (the comment above repinCondition/repin-condition constant
or function) and change the wording so it lists both triggers (uptime threshold
and eligibility) and clarifies that either condition will cause a re-score and
potential re-pin.
Comment on lines
+193
to
+194
| ttlSeconds: Math.max(1, Math.floor(cfg.ttlSeconds)), | ||
| uptimeThreshold: Math.max(0, Math.min(100, cfg.uptimeThreshold)), |
Contributor
There was a problem hiding this comment.
Clamp session.ttlSeconds to the same upper bound as API validation.
Line 193 only enforces a minimum. The API schema caps ttlSeconds at 86,400, so non-API callers of resolveRoutingConfig can still resolve oversized TTLs and drift from the declared contract.
Suggested fix
function clampSession(
cfg: Required<RoutingSessionConfig>,
): Required<RoutingSessionConfig> {
return {
enabled: Boolean(cfg.enabled),
- ttlSeconds: Math.max(1, Math.floor(cfg.ttlSeconds)),
+ ttlSeconds: Math.max(1, Math.min(86_400, Math.floor(cfg.ttlSeconds))),
uptimeThreshold: Math.max(0, Math.min(100, cfg.uptimeThreshold)),
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ttlSeconds: Math.max(1, Math.floor(cfg.ttlSeconds)), | |
| uptimeThreshold: Math.max(0, Math.min(100, cfg.uptimeThreshold)), | |
| function clampSession( | |
| cfg: Required<RoutingSessionConfig>, | |
| ): Required<RoutingSessionConfig> { | |
| return { | |
| enabled: Boolean(cfg.enabled), | |
| ttlSeconds: Math.max(1, Math.min(86_400, Math.floor(cfg.ttlSeconds))), | |
| uptimeThreshold: Math.max(0, Math.min(100, cfg.uptimeThreshold)), | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/src/routing-config.ts` around lines 193 - 194, The ttlSeconds
field in resolveRoutingConfig (routing-config.ts) only enforces a minimum via
Math.max and can exceed the API's 86,400 upper bound; update the ttlSeconds
normalization for cfg.ttlSeconds to also clamp to 86,400 so non-API callers
match the schema (i.e., apply both min 1 and max 86400 when flooring
cfg.ttlSeconds) and leave uptimeThreshold clamping as-is.
Add a case proving the pinned provider is the full weighted-score winner (price + uptime + throughput + priority), not merely the cheapest: a far cheaper provider with poor uptime loses to the more expensive one, and that more expensive provider is what gets pinned. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
steebchen
enabled auto-merge
June 8, 2026 22:01
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b39b32e8a
ℹ️ 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".
Comment on lines
+170
to
+171
| ttlSeconds: z.number().int().min(1).max(86_400).optional(), | ||
| uptimeThreshold: z.number().min(0).max(100).optional(), |
There was a problem hiding this comment.
When these new session fields are set through this API, the existing routing settings dashboard still loads row.session but serializes it back as only null or { enabled } on save (apps/ui/src/app/dashboard/[orgId]/[projectId]/settings/routing/_components/routing-config-client.tsx lines 451-454). In that workflow, an enterprise user who configured ttlSeconds or uptimeThreshold via API loses those overrides the next time they save any routing setting in the dashboard, because the PUT overwrites the whole session object.
Useful? React with 👍 / 👎.
steebchen
added this pull request to the merge queue
Jun 8, 2026
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jun 8, 2026
## Problem The activity-log "Routing Info" panel hides the per-provider scores for sticky-session requests. As of #2596, sticky-session routing scores providers with the normal weighted algorithm and only pins the *result* for the session — so the log already carries the real inferred scores under `selectionReason: "session-sticky"`. The dashboard still listed `session-sticky` in `SCORE_BYPASSED_SELECTION_REASONS`, a leftover from when sticky picks used rendezvous hashing and emitted hardcoded `0` placeholders, so those real scores were never displayed. ## Fix Remove `session-sticky` from `SCORE_BYPASSED_SELECTION_REASONS` so the inferred weighted scores that informed the sticky routing decision show up in the log. The existing all-zero fallback still hides them in the degenerate case where scoring couldn't run (e.g. no metrics available). ## Testing - `pnpm format` - `pnpm --filter ui build` + `tsc --noEmit` (clean) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Updated the visibility logic for Provider Scores to display them in additional scenarios where they are applicable. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Sticky session routing pinned each session to a provider via rendezvous hashing, which ignored the weighted-score routing algorithm entirely. Roughly 1/N of all sessions got pinned (for the session's life) to whichever provider won the hash — including the most expensive one — and the cheaper, higher-priority provider lost those sessions outright. The weighted-score path's price weight and provider priority only applied when there was no session id; they were completely bypassed once a session id was present.
Fix
Sticky routing now uses the exact same weighted-score algorithm (price, priority, uptime, throughput) to pick the best provider, and only the result is made sticky:
Changes
packages/actions— Replace the rendezvous hash (selectStickyProvider/hashToUnitInterval) ingetCheapestFromAvailableProviderswith an injectedSessionProviderStore(get/set), matching the existingproviderDiscountResolverinjection pattern. Exploration is skipped while sticky so the deterministic best is what gets pinned. TTL is refreshed on reuse.apps/gateway— AddcreateSessionProviderStore(Redis-backed, per org/model/session) inpreferred-provider.tsand wire it into the real routing decision points inchat.ts. The org+model preferred-provider hysteresis is skipped while a session is pinned (session stickiness takes precedence); region sub-selection and the metadata-only scoring call do not persist.packages/shared— Addsession.ttlSeconds(default 3600) andsession.uptimeThreshold(default 85) to the routing config, with clamping.apps/api— Extend the routing-config Zod schemas (override + resolved + defaults) for the new session fields.Testing
packages/actions/src/models.spec.ts— rewritten sticky-session tests: pins the weighted best, reuses it on the next request, matches the non-session weighted pick, no-pin when disabled, re-pins when the saved provider is gone.packages/shared/src/routing-config.spec.ts— new tests for session ttl/uptime defaults and clamping.pnpm build,pnpm format,pnpm lint, and the affected unit suites (64 tests) all pass.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests