Skip to content

fix(sse): session affinity misses Gemini and legacy shapes, and hashes whole payloads - #12892

Closed
MumuTW wants to merge 5 commits into
cheaper-inference:mainfrom
MumuTW:MumuTW/session-affinity-edge-tests
Closed

MumuTW wants to merge 5 commits into
cheaper-inference:mainfrom
MumuTW:MumuTW/session-affinity-edge-tests

Conversation

@MumuTW

@MumuTW MumuTW commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

extractSessionAffinityKey falls back to hashing the request's first input text when no session id is present. Three problems in that fallback, all on the path every sessionless request takes.

Unbounded stringify on a hot path

extractTextForSessionHash ended both its array branch and its object branch with JSON.stringify(value):

return parts.length > 0 ? parts.join("\n") : JSON.stringify(value);
...
if (value && typeof value === "object") return JSON.stringify(value);

A multimodal message whose parts are one text and one base64 image stringifies the image as well. A body with no recognisable text field stringifies the whole body. Only the first 4096 characters are ever hashed, so the remainder is built and discarded. Extraction now accumulates against SESSION_HASH_TEXT_LIMIT and stops at the boundary.

Colliding affinity keys

Because no-text payloads were stringified rather than rejected, {input: []} and {input: {}} produced stable hashes — every request of that shape pinned to the same connection. Unrecognised shapes now return null: no affinity, which is the honest answer for a body carrying no user text.

Shapes that never got affinity

The fallback read input and messages only. These returned null and were never pinned:

shape example
Google Gemini {contents: [{role: "user", parts: [{text}]}]}
legacy completions {prompt} — OpenAI legacy, Anthropic /v1/complete, Ollama
root-level text {query}, {instruction}

It reads those now, and reads text / content / prompt / parts off object values rather than stringifying them.

Tests

tests/unit/session-affinity-edge-cases.test.ts — 36 cases over array and object bodies, multimodal payloads, the 4096 truncation boundary, Gemini's format, and the empty-container collisions.

  • 13 of the 36 fail on main, pass with this change
  • the 34 cases in the existing session-affinity-generic-7274, combo-sessionless-pin-3825, codex-failover, session-affinity-combo-timeout-eviction and codex-session-affinity-reset-aware-5903 suites still pass
  • eslint with the repo's suppressions is clean on auth.ts; typecheck:core reports nothing in auth.ts

Follows #11746, which hardened the key extraction itself; this is the text-extraction layer underneath it.

https://claude.ai/code/session_015E2idg9mo8qE2M9F5Myvnt

…s whole payloads

`extractSessionAffinityKey` falls back to hashing the request's first input text
when no session id is present. Three problems in that fallback, all on the path
every sessionless request takes:

**Unbounded stringify.** `extractTextForSessionHash` ended both its array and its
object branch with `JSON.stringify(value)`. A multimodal message whose parts are
one text and one base64 image stringifies the image too; a request body with no
recognisable text field stringifies the entire body. The hash only ever consumes
the first 4096 characters, so the rest of that string is built and thrown away.
It now accumulates against `SESSION_HASH_TEXT_LIMIT` and stops there.

**Colliding keys.** Because no-text payloads were stringified rather than
rejected, `{input: []}` and `{input: {}}` produced stable hashes — so every
request of that shape pinned to the same connection. Unrecognised shapes now
return null, which means no affinity, which is the honest answer.

**Shapes that got no affinity at all.** The fallback read `input` and `messages`
only. Google Gemini's `contents[].parts[].text`, the legacy completions `prompt`
(OpenAI legacy, Anthropic `/v1/complete`, Ollama), and root-level `query` /
`instruction` all returned null, so that traffic was never pinned. It reads them
now, and reads `text` / `content` / `prompt` / `parts` off object values instead
of stringifying them.

`tests/unit/session-affinity-edge-cases.test.ts` covers the fallback: 36 cases
over array and object bodies, multimodal payloads, the 4096 truncation boundary,
Gemini's format, and the empty-container collisions. 13 of them fail on main.

Claude-Session: https://claude.ai/code/session_015E2idg9mo8qE2M9F5Myvnt
@MumuTW
MumuTW requested a review from diegosouzapw as a code owner September 7, 2026 02:49
@MumuTW

MumuTW commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

CI triage after the retrigger — none of the 4 failing jobs appear related to this PR:

  • Build: the runner received a shutdown signal ~7 min in (The runner has received a shutdown signal → The operation was canceled), the same cancellation as the previous run. Looks like infra instability rather than a compile error (the earlier run compiled successfully before being killed).
  • Lint: fails on npm audit with 19 vulnerabilities (incl. the critical Next.js advisories). Upstream main's latest CI run fails this same job identically — baseline, not from this diff.
  • Quality Gates (Extended): also fails identically on upstream main's latest CI — baseline.
  • Unit Tests (2/8): I re-ran the full shard locally on this branch (TEST_SHARD=2/8 npm run test:unit:ci:shard): 4343 tests, 0 failures. Likely a resource/timeout flake on the runner.

Pushed an empty commit to retrigger once more; if Build keeps getting killed, a maintainer re-run would be appreciated. The Lint/Quality Gates failures track upstream main and should be ignorable for this PR.

@MumuTW

MumuTW commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Hi @diegosouzapw,

Quick note on the "fork rejects maintainer pushes" you hit on #11685 / #11702 / #11749: those three were opened with "Allow edits by maintainers" unchecked on my side — sorry for the extra work. It is enabled on #12892 and #13408, and I'll keep it on for everything going forward, so a rebase-and-merge from your end should just work.

While I'm here: would you consider giving me triage access on the repo? Since July I've landed ~37 PRs here (task-aware routing / auto-combo fitness, request-body heap fixes, the ratchet shrink-banking lane, the larger file decompositions) and filed ~36 issues. Most of what I file comes with a repro, and I end up doing the follow-up on stale ones anyway. With triage I could label and link my own issues to their PRs, close obsolete reports, and mark superseded PRs, without pulling you in for housekeeping. I'm not asking for write, and happy to keep going through PR review as now.

Thanks for the fast turnaround on the August batch.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks for this — but it looks like your own earlier PR #11746 ("harden session affinity key
extraction", merged into release/v3.8.51 on 2026-08-28) already ships this exact fix: no more
unbounded JSON.stringify, Gemini contents/parts support, bounded text extraction, and the
same exported helper shape in sessionAffinityPin.ts. This PR was opened 10 days later from an
older main fork that predates #11746. I ran your own new test file
(session-affinity-edge-cases.test.ts, all 36 cases) unmodified against the current release
tip: 34/36 already pass. The 2 remaining are just a design difference #11746 itself settled on
(reject an oversized session_id outright vs. cap-and-hash it) — not a real gap. Recommending we
close this one as superseded by #11746 rather than merge; let me know if you'd rather revisit
the cap-vs-reject choice for huge explicit session IDs as a small separate PR against the
current tip.

Triage note: this is the review recommendation — the close itself happens only after the maintainer's per-PR sign-off (and, where a superseding PR is named, after it has landed). Nothing is being closed by this comment.

@MumuTW

MumuTW commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Agreed — keeping this closed as superseded by #11746. I am not opening a follow-up for the cap-vs-reject difference. That choice is already settled on the release tip, and I do not have a case where rejecting an oversized explicit session id is the wrong behavior.

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.

2 participants