Skip to content

[PR] fix: make multi-credential identity stable and complete - #6149

Open
Afloat16 wants to merge 1 commit into
ranxianglei:masterfrom
Afloat16:2026-11-10_stabilize-multi-credential-identity
Open

Afloat16 wants to merge 1 commit into
ranxianglei:masterfrom
Afloat16:2026-11-10_stabilize-multi-credential-identity

Conversation

@Afloat16

Copy link
Copy Markdown
Contributor

Problem

Fingerprints use the first recognized header. With Authorization plus an API-key header, reordering changes identity while changing a later credential does not. The same credential set can miss a 429 hold, while another set inherits or clears it. Pre-ingest header collection can also differ from forwarding order.

Changes

Normalize all recognized credentials with existing bearer/SDK-HMAC rules. Preserve the exact existing fingerprint when headers describe one credential. For distinct credentials, hash a sorted set of normalized header/value pairs so order/casing is stable and each credential plus its header role participates. Only the digest leaves the function.

Validation

  • 51 targeted tests passed across fingerprints, key-switch accounting, SDK-HMAC normalization and the actual governor; four new tests fail against unchanged source.
  • Governor regression verifies a hold survives reordering, does not block another credential set, cannot be cleared by that set's success, and clears when the original succeeds.
  • Controls preserve single-key aliases, empty-header behavior and signature-independent SDK-HMAC identity.
  • Final-code typecheck, npm production build with dist checks and git diff --check passed. Full suite remains with CI.

Checked fingerprint/header-order/composite history and previous Afloat16 submissions. No new configuration or persisted raw credential is introduced.

Base: e1e33a9.

Closes #6148

@ranxianglei ranxianglei changed the title fix: make multi-credential identity stable and complete [PR] fix: make multi-credential identity stable and complete Nov 10, 2026
@ranxianglei

Copy link
Copy Markdown
Owner

🤖 Powered by ework · qwen3.8-27b

(ai: vllm-qwen/qwen3.8-27b)

查重: 不重复。#6149 是 issue #6148 的唯一修复 PR —— 检索了 open/closed PR 与 issue,没有其它在途 PR 针对同一机制(credentialFingerprint 的多凭据身份);搜到的 #3659 / #5216 / #4637 均为无关主题。

结论(一句话): 针对真实根因的正确、收敛、有牙的修复;本地定向验证全绿;因改动落在凭据身份逻辑上,属人工评审档(§7.4 不可自动合并),建议 owner 批准、并在一次干净的全量 CI 后合并。

独立核验(非照抄作者描述)

  • 根因确认。 credentialFingerprint(src/cache-ledger.ts:654)此前以「第一个被识别的凭据 header」作为唯一身份载体 —— 既随 header 插入顺序变化,又漏掉其余凭据。它的四个消费面全部以该指纹为 key:429 governor(src/upstream-throttle.ts:379 经 holdKey)、缓存归属 kp(src/server/relay.ts:895、src/loop/core.ts:562)、keepalive 身份(src/cache-keepalive.ts:287)。
  • 修复走通真实 governor 状态机。 逐行 trace 了 noteUpstreamThrottle429/upstreamThrottleGate/noteUpstreamThrottleResolved:重排后仍命中同一 hold(hold 存活)、另一凭据集合既不继承也不能清除原 hold、只有原集合成功才解除。
  • 向后兼容证明。 凡被识别 header 归一化后同值(即所有单凭据请求,含 SDK-HMAC 单行、空/纯空白头、undefined)的输入,新旧摘要逐字节一致;仅当出现 ≥2 个不同值时才改变。输出形态不变(恒为 sha256: + 12 hex),下游按不透明串消费,不受内部格式变化影响。
  • 回归测试有牙。 把新测试文件对到修复前源码上跑:5 条中 4 条失败(顺序无关性、每凭据参与、多 header SDK-HMAC、governor 跨清),单凭据别名那条按设计通过 —— 与作者声明一致。
  • PR head 本地实测: 定向 66 项测试全过(fingerprint 家族 + throttle governor + scope + cache-capture);tsc --noEmit 干净;npm run build + dist 检查通过。

行为变更(按交付规范第 4 条显式披露)

对携带 ≥2 个不同凭据值的请求,指纹值发生变化:

  • 旧 = sha256(第一个被识别 header 的归一化值)
  • 新 = sha256("credential-set:" + 排序后的归一化 (header,value) 对集合 JSON)

这是修复本身的目的,属有意变更。单凭据请求完全不变(逐字节一致)。原始凭据从不落盘/外发,函数只输出摘要。多凭据采用「每个凭据 + 其 header 角色」的更细粒度身份 —— 对 throttle 而言是安全方向(只会把一个池拆成两个独立池,绝不会让一个池误继承另一个的 hold)。

剩余门槛

远程全量 CI。镜像里当前可见的那次运行大多为 cancelled(快门 gate/plan/test-import-graph 已 success,重型 test 矩阵未完成)。分支落后 master 52 个提交,但已核实这 52 个提交没有触碰本 PR 改动的三个文件,GitHub 亦报 mergeable: true,故 rebase / “Update branch” 后重跑应能干净转绿,无语义冲突。我未向外部贡献者分支 push(那会重写其提交历史,违反「外部 PR 只做增量、不替换」约定)。

建议标签(供参考,未代打):bug · p2 · scope/kernel。


结构化八问

  1. 是什么问题: 多凭据请求的身份指纹只取了第一个识别到的 header,导致身份随 header 顺序漂移、且漏掉其余凭据 —— 同一 origin 下,一个账号的 429 hold 会被另一个共享部分凭据的账号继承,甚至被后者成功时误清。
  2. 什么原因导致的: credentialFingerprint 以「第一个识别 header」作单一身份载体(first-header-wins),既不排序也不纳入全部凭据;而 governor / 缓存归属 / keepalive 全都以它为 key,于是顺序与完整性缺陷直接传导到 throttle 匹配。
  3. 影响范围与严重程度: 仅当请求同时携带 ≥2 个不同凭据值时触发(如 Authorization + x-api-key 双头且值不同);单凭据请求不受影响。严重度:p2 —— 能用但多凭据场景下 throttle 行为错误(跨继承/误清),会自愈、有界,非全量不可用,也不属缓存泄露类。
  4. 怎么修复的: 在指纹函数内归一化所有识别凭据(沿用 bearer 剥离 + SDK-HMAC 只取 Access key id 的既有规则);单凭据(含同值别名)保留原「仅值」指纹保证字节级兼容;≥2 个不同凭据则对「排序后的 (header,value) 对集合」做哈希,使顺序/大小写无关且每个凭据连同其 header 角色都参与;只输出摘要。
  5. 是否有回归问题: 无。单凭据路径与旧实现逐字节一致(bearer/x-api-key 统一、SDK-HMAC 单行、空/空白头、undefined 等既有断言全绿);输出形态不变,下游不透明消费不受影响;四个消费面语义一致;本地 66 项定向测试全过。唯一行为变化即第 4 条披露的多凭据指纹值。
  6. 是否破坏缓存: 不涉及缓存失效策略本身。kp(缓存归属键)对单凭据请求不变;多凭据请求改归「集合」桶 —— 是修正(旧归属本就漏了第二个凭据)而非破坏。throttle hold 为进程内瞬态状态,重启即重建,指纹变化只波及升级瞬间的极短窗口。
  7. 是否改了设计方向: 否。仍是 credentialFingerprint 这一单一事实源([bug][dsh] 429-hold + host retry ladder re-upload 51.2 MiB per retry (36/37 attempts failed) | 429-hold 与宿主重试叠加:每次重试重传 51.2 MiB,单步放大 2-5 倍 #5795 的设计前提不变),只是把「身份=第一个头」修正为「身份=完整凭据集合」;未动 kernel / wire 协议 / config schema / persistence 版本。
  8. 是否引入了新的配置: 否。无新增 env / config / 默认值。

收工判定: 代码正确、测试有牙、diff 干净(2 文件 +70/-4,外部提交质量达标)、根因到位 —— 具备合并条件;唯二待办:owner 批准(身份逻辑属人工档)+ 一次干净的全量 CI(rebase/update-branch 后重跑)。

@ranxianglei

Copy link
Copy Markdown
Owner

🤖 Powered by ework · qwen3.8-27b

(ai: vllm-qwen/qwen3.8-27b)

Status update — the full structured review for this PR is posted above (查重 conclusion + independent verification + Chinese 八问); this note just pins the current state so the thread isn't left open-ended.

Done & verified locally: root cause confirmed (credentialFingerprint, src/cache-ledger.ts:654 keyed identity on the first recognized credential header only); fix traced through all four consumers incl. the real 429 governor state machine; single-credential path proven byte-identical; regression teeth confirmed (4/5 new tests fail against pre-fix source); 66 targeted tests green, tsc --noEmit clean, production build + dist checks pass. Working tree clean; did not push to the contributor's branch.

Remaining gate + risk: one clean remote full-suite CI run. The mirror's visible run was mostly cancelled; branch is 52 commits behind master but those commits touch none of the three changed files and GitHub reports mergeable: true, so a rebase / “Update branch” + rerun should go green with no semantic conflict. Low risk of hidden cross-file interaction given zero file overlap.

Verdict: correct, well-scoped, well-tested root-cause fix — ready for owner merge after that CI run. Not auto-merge eligible because it changes credential-identity logic (human tier per §7.4). Behavior change disclosed in the review above (multi-cred fingerprint value changes by design; single-cred unchanged). Awaiting your go-ahead.

@Afloat16
Afloat16 force-pushed the 2026-11-10_stabilize-multi-credential-identity branch from ec9b08b to a10e38f Compare November 11, 2026 04:38
@Afloat16

Copy link
Copy Markdown
Contributor Author

Rebased the existing fix onto current master (33104b1) in a10e38f, preserving the original two-file patch. The resulting tree matches the clean merge result.

All 51 focused credential identity, key-switch, SDK HMAC, and throttle-scope tests pass on the rebased tree, along with npm run typecheck and npm run build. Fresh CI is at https://github.com/ranxianglei/billion-context/actions/runs/42960573788 . The full matrix and final repository-owner merge decision remain with the maintainers.

@ranxianglei

Copy link
Copy Markdown
Owner

🤖 Powered by ework · qwen3.8-27b

(ai: vllm-qwen/qwen3.8-27b)

Rebase verified independently — the patch is intact and CI is green. Everything checks out against live state, not just the reported numbers:

  • SHAs confirmed. Current head is a10e38f, and its parent is exactly 33104b1 = current master tip. So the branch sits directly on top of master and is genuinely clean/squash-mergeable — GitHub's transient behind flag is just post-force-push recompute lag, not a real divergence.
  • Patch preserved verbatim. The two changed files at the new head are byte-identical to the pre-rebase head. Against master the diff is still exactly the original shape: src/cache-ledger.ts (+11 net) + tests/credential-set-fingerprint.test.ts (+59) = 70 insertions / 4 deletions, whitespace clean. Nothing was lost or altered by the rebase.
  • CI green. On the linked run: full test matrix (ubuntu/windows × node 22/24, every shard) success; static-gates (both platforms), mysql-contract, architecture-docs, lane-guard, version-guard all success; 0 failed, 0 pending. The gated e2e lanes are in their default skipped state (opt-in via canary/env), which is the normal posture for a non-canary PR.

Because the patch is byte-for-byte the one I already reviewed, my prior verdict carries over unchanged: correct, well-scoped, well-tested root-cause fix (multi-credential identity no longer order-dependent or incomplete; single-credential path stays byte-identical). It remains not auto-merge eligible because it touches credential-identity logic (human tier per §7.4).

What remains: only your merge decision — no further code changes needed from me.

This branch has not been deployed

No deployments
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.

[Bug] Multiple authentication headers make throttle identity order-dependent and incomplete

2 participants