Repository navigation
[PR] fix(storage): refs ledger diffs instead of full-replace per save (#5744) - #5755
Conversation
The refs table (one row per message the session has ever seen) was maintained by DELETE-all plus one-row INSERT on every save, so a steady-state delta save of a few new messages into a long session still executed O(full history) native SQLite statements synchronously on the event loop - the 96.5% StatementSync.run profile behind #5744. Now: an in-memory diff against one indexed read of the stored set. Vanished raw_ids are deleted (no ghost rows survive a save, so the review-#2690 replace-all contract still holds), untouched survivors are skipped, and added/moved rows go out as multi-row upsert batches with a bounded parameter count - first-ingest and compaction-renumber storms cost ~one batch per 160 rows instead of H native calls. Bench (H=3100, K=4/turn): delta lane 3223 -> 82 stmts/save, 34.8 -> 4.6 ms; folded-resident lane 3855 -> 714; steady-state refs work is now constant in history length. Regression suite pins end-state parity, the refs statement budget, and a ghost-free reopen.
📦 Built Package ArtifactBranch: Latest build — install this exact versionnpm install -g billion-context@0.0.8529-pr.5755.5b71e44This exact version is immutable and reproducible. The rolling Recent builds (commit → version)
Download artifact
tar xzf billion-context-pr5755.tgz
npm install -g packageInstall instructions are refreshed on each push. |
ranxianglei
changed the title
(ai: vllm-qwen/qwen3.8-27b) 查重：不重复。 检索 open+closed issue/PR，无并行 PR 做同一「refs 表 replace-all → 增量 diff」机制；其它在途 storage PR（#5757 sidecar 迁移 / #5679 drift 诊断 / #5676 嵌套折叠账本错位 / #5421 async 后端漂移锁释放）机制各不相同。本 PR 源自 #5744。 审核对象： GitHub PR #5755，head 结构化八问：
测试有效性（有牙）：新套件 instrument 了 CI 判定（权威）：push head 一个非阻塞 follow-up： 判定：可合。 方向对（疏而非堵，打的是 O(H) 写的生产者）、行为零回归、测试有牙、CI 全绿。 |
(ai: vllm-qwen/qwen3.8-27b) CI 车道收尾确认(本轮):独立复核了 round-3 判定——全绿,零代码改动。证据链(本轮逐条重查过,不是转述):
八问:
判定:head 4732e77 全 CI 绿,本车道收工,等待人工合并。 |
… refs diff relies on (#5744) The incremental upsert is correct under one producer-guaranteed invariant that was nowhere stated: a surviving raw_id's ord never increases between saves (appends take higher ords; the only renumberers #395/#2695 both go through pruneRefsToLiveIds, which preserves survivor key order and only drops ids). If that ever breaks, the new ord crosses a live neighbour's ord, hits the (session_id, ord) PK mid-batch, and rolls the whole ingest back. State the precondition at the write site so a future upward-reordering producer is caught in review instead of silently disabling persistence. Also drop a redundant /1 in the DELETE chunk size (no behavior change).
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) |
ranxianglei
left a comment
ranxianglei
left a comment
There was a problem hiding this comment.
(ai: vllm-qwen/qwen3.8-27b)
(Posted as a comment: GitHub forbids the APPROVE event on a self-authored PR, so this carries the approval verdict.)
APPROVE — correct for every current producer; end-state byte-identical to the old replace-all; measured 7–14× per-save store-time cut; real tests with teeth. One latent robustness point surfaced + a small spot-fix pushed to the branch.
What I verified (independent read of the diff, not the issue's claims)
- End-state parity. The new path deletes vanished
raw_ids, skips unchanged survivors, and upserts added/moved rows. For any input the final row set equalsinput.refsexactly — same as the old DELETE-all + re-insert. Checked by reasoning over append / downward-renumber / empty-store / first-save shapes and confirmed by the new parity test, which asserts every stored row equals the legacybyRawmap. - PK safety (the crux).
refsenforces BOTHPRIMARY KEY (session_id, ord)andUNIQUE (session_id, raw_id)(src/storage/schema.ts:189). The upsert targetsON CONFLICT(session_id, raw_id)while rewritingord, so the load-bearing question is whether a surviving row'sordcan ever increase or cross a live neighbour'sord. It cannot today:ordis thebyRawkey order (src/storage/ingest.ts:372), and the only two renumberers — native-compaction archive (#395) and zombie reap (#2695) — both route throughpruneRefsToLiveIds(src/session.ts:1814), which iterates existing keys in order keeping only live ids (preserving survivor order, dropping pruned ones, appending theREF_FLOOR_RAW_IDpin last). New messages append at higher ords. So the monotonic-downward shape the incremental write assumes holds by construction, for all known producers. - Batch math / safety. DELETE ≤ 699 ids + 1 session_id = 700 params; INSERT 140 rows × 5 = 700 params — both under SQLite's 999 bind-param cap. SQL lists are placeholder-only (no value interpolation) → no injection surface. Everything runs inside the existing
BEGIN IMMEDIATEtxn (store.ts:854) → atomic, identical rollback / 3-strike self-disable semantics as before (graceful degradation, never corruption). - Scope. No schema change, no config surface, no
kernel/change, no wire change. Two focused files.
Finding (non-blocking — addressed on the spot)
The incremental write silently depends on that monotonic-ord invariant, which was stated nowhere near the code. If a future producer could reorder survivors upward, the upsert would trip the (session_id, ord) PK mid-batch and roll the whole ingest back (storage self-disable) — the old delete-all was permutation-proof against exactly that case. I deliberately did not add a defensive fallback: that is a design call, and dead code guarding an unreachable path conflicts with house rules. Instead the precondition is now documented at the write site so the constraint is caught in review the moment a producer ever changes.
Tests
Real and deterministic (hermetic mkdtemp, BILI_STORAGE_UNIFIED=legacy pin, no fixed ports). They pin end-state parity vs the legacy map, the statement budget (≤3 steady / ≤6 renumber vs the old H+1), and a ghost check across an open/close cycle. Gap: no non-monotonic negative case — acceptable given the invariant holds by construction, which the added comment now makes explicit for the next reader.
已顺手修：补上增量 refs diff 依赖的「存活行 ord 只降不升」前置不变量注释（此前无处声明；未来若出现向上重排的 producer，会在 batch 内撞 (session_id, ord) PK、回滚整个 ingest、触发存储自禁用）＋去掉 DELETE 分块大小里多余的 /1（无行为变化）。commit 5f81abd
八问
- 是什么问题：长会话每次保存对 refs 账本做全表 DELETE + 逐行 INSERT，O(全历史) 条同步 SQL 卡死事件循环（#5744 上报实例
/__bili/health停 17–30s）。 - 什么原因：refs 段在 delta 与 full 两条路径都整表重写——即使本轮只新增几条，也把全部 ~H 行重新落盘。
- 影响范围 / 严重程度：所有长会话插件车道的 delta 保存路径；事件循环饥饿 + health 延迟。属性能/可用性退化，非数据丢失。
- 怎么修：一次索引读入存量集 → 内存 diff → 只写变化的行（消失的批量
IN(…)删、未动跳过、新增/移动的行按 ≤700 参数/语句批量 upsert）。 - 回归风险：终态字节级等价旧 replace-all。唯一收窄是「ord 只降不升」这一前置不变量——当前两个 renumber producer（#395/#2695 均经
pruneRefsToLiveIds）都满足，故无现实回归；若未来被破坏，失败模式是 ingest 回滚（存储自禁用）而非数据损坏。高危模式逐项过：语义漂移无、fail-fast 未变（txn 原子性保留）、阈值/时序/输出格式不变。 - 是否破坏缓存：不涉及（纯存储层写入路径，不动 prefix cache / 重试 / 增量游标 / 去重键）。
- 是否改设计方向：不涉及（沿用同一「入站数组为权威」契约，仅把 store 腿从 O(H) 降到 O(Δ)）。
- 是否引入新配置：无。
Verdict：APPROVE. Merge once CI is green on head 5f81abd.
✅ Review done — PR #5755 (head |
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) PR #5755 审核完成 —— 判定：可合。（完整结构化八问见上方评论，此处为摘要） 做了什么：
一个非阻塞 follow-up： 结论：可合。 |
|
(ai: vllm-qwen/qwen3.8-27b) Re-review on head Verdict unchanged: APPROVE — ready to merge. |
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) 独立复核了新 head
一条非阻塞技术注记（for the record，不影响批准）：注释把 PK 安全归因于「monotonic-ord」，但实际的主要保证是 delete 先于 upsert——被消费的 survivor 保留其精确 ord，而入站数组 ord 唯一，移位/新增的行落不到任何存活行的 ord 上，所以即便某条 ord 增大也不会撞 判定维持：APPROVE —— ready to merge。 你已拍板，我这边独立核实无新增风险。 |
…5744) The refs leg went incremental in a438f40, but two sibling legs still paid O(session size) on every save: - ccr_entries: full DELETE-all + re-INSERT of every entry per save. The release path dropped refcounts to zero, so the content blobs themselves were destroyed and re-created every turn (measured: 50-entry store -> ~180 extra content statements/save at H=3000). - blocks: summary/one strings were re-hashed on every save even when the stored row was byte-identical, and the upsert + refcount bump/release dance ran unconditionally for all blocks. Now: one indexed read of the stored set per leg, in-memory diff, untouched rows skipped entirely; only new/changed rows are written (multi-row batches), vanished ones deleted in chunked IN(...) batches. Content hashes are precomputed once in finishPlan (BlockRow summaryHash/oneHash, CcrEntryRow contentHash) so the CAS probes reuse them. End-state semantics unchanged: the incoming plan remains authoritative. Measured (UnifiedStore + planIngest, H=3000, 24 turns, K=2 msgs/turn): delta fast path 66.3 -> 15.6 ms/save mean (steady state ~15 statements/save at any history size) folded-resident 67.9 -> 21.3 ms/save mean, 4294 -> 837 stmts/save (before-fix the refs leg alone was 3025 inserts/save on the delta lane) MySQL backend gets the same diff treatment for parity (its ingestSession kept the old full-replace shape).
…age warn (#5744) Behavior change (warning triggers only, no data/wire effect): - server logRequestCost: the slow-session warning fired on message count (>1000 msgs). A 3k-msg session takes ~1s normally yet 14s on a checkpoint turn while a 13-msg session can take 39s behind a busy writer - count is not the failure signal. It now fires on wall time (local > 1000ms); the info line still carries the count. - shadow record(): new [storage] slow-ingest warning (wall > 500ms) that names the ingest path taken (delta vs full fallback) and the session, so a stall pins to its producer instead of surfacing as anonymous StatementSync.run self-time.
#5744) - New issue5744-ingest-diff suite: steady-state statement budget for the ccr/blocks legs (floored both ways so a broken meter cannot pass), add/change/delete end-state parity for ccr entries including blob reclaim, changed-block old-blob release, and delta-vs-full convergence (clearing the watermark and re-saving must reproduce the byte-identical stored set). - issue5744-incremental-refs: instrument the shadow's own store instance instead of the standalone handle - records write through the shadow's connection, so the old meter always read zero and the budget asserts were vacuous. Floors added alongside the caps.
…est warn (#5744) The intermittent multi-second stall spikes reported alongside the O(history) write amplification (same ~3k msgs saving in 0.8s vs 14.5s) could not be attributed from the log: was it per-row statement cost, or a periodic wal_checkpoint(PASSIVE) burst? The slow-ingest warn line now carries the answer: - maybeCheckpointWal times the tick and keeps the last (busy, log, checkpointed) triple plus its duration; - new read-only UnifiedStore.walDiagnosticsSuffix() reports the current -wal file size plus that triple (null when there is no WAL file; never throws into the caller); - the warn line appends it, e.g. 'took=14233ms, 58.7 MiB WAL lastCheckpoint=(busy=0 log=98211 checkpointed=97800 took=13980ms)' — a multi-second tick inside the window identifies a checkpoint burst; an absent/took~0ms tick points at statement cost; - regression test pins the suffix shape once a tick has fired (20 ingests = one WAL_CHECKPOINT_EVERY_INGESTS cycle).
|
(ai: vllm-qwen/qwen3.8-27b) Verdict: APPROVE — ready to merge once the in-flight test shards land green. This pass re-reviewed the NEW delta over the already-approved refs leg: 3 commits extending the same incremental treatment to the ccr_entries and blocks legs ( Correctness, per leg:
Tests (would they catch regressions): yes — real teeth, hermetic (mkdtemp, pinned env, file-based store, no fixed ports). The new suite pins (a) steady-state statement budgets (ccr 1..2 / blocks 1..3 per save vs old ~4H/~6B), (b) ccr add/change/delete landing exactly the incoming set with dense ords + honest refcounts + dropped-blob reclaim, (c) a changed block releasing its old summary blob, and (d) diff-path vs forced-full-path byte-identity (clearing the watermark makes the next save take the full path and must reproduce the diff path\u2019s table state exactly). Notably, the delta also FIXES a latent bug in the pre-existing refs test: it instrumented the standalone Structured eight-question conclusion (中文):
行为变更披露(policy #4): Non-blocking observations (deliberately left untouched, with reasons — not filed per owner policy 2026-11-01):
CI snapshot (honest): mirror cache shows |
✅ Re-review on new head |
ranxianglei
left a comment
ranxianglei
left a comment
There was a problem hiding this comment.
评审结论:建议可合并(需人审)
逐行读完 diff 并在本地完整复验。这个 PR 消掉的是 #5744 里占 ~97% 的主成本(refs / ccr_entries / blocks 三张台账每轮全删全插),机制成立,测试是"真断言"。
本地验证(在 head 8d282181a 上复跑)
| 批次 | 结果 |
|---|---|
新增 issue5744-incremental-refs + issue5744-ingest-diff + PR 改过的 issue4739-storage-read-snapshot + issue5403-watermark-fastpath |
14 项:12 pass / 2 skip(env-gated)/ 0 fail |
| 相关既有套件 fold-reconcile×3、issue3919-shadow-drift、issue5627、bounded-storage×2、fix-401、pins-storage-refactor、golden-storage-render | 130 项:124 pass / 6 fail |
| 那 6 个失败 | pristine master(c45ac21)上原样复现 → pre-existing,与本 PR 无关(golden-render×4:nudge/tsStamps 时间敏感;#5627×2:深嵌套 RangeError) |
npm run typecheck |
exit 0 |
| CI(head 8d28218) | 29/29 SUCCESS |
负向对照(证明计量器真在计量):把 src/ 整体换回 master 版本、保留新测试 → 7 项里 4 项变红,断言信息正是特征性的 turn 1: 103 refs statements for +2 rows(旧代码 ~H+1 条语句,新代码常量级)。换回修复版 7/7 复绿。预算断言不是恒真的。
语义核对(不只看测试)
- refs diff 的身份键选对了:按
raw_id(内核契约永不复用)对账,ord 位移被 monotonic-ord 前置条件罩住。核对了代码里仅有的两个重编号生产者(native-compaction #395、zombie reap #2695)都走pruneRefsToLiveIds(src/session.ts:1814):保序、只删、追加在尾 → 幸存者 ord 只降不升,新增永远更大号。在该前提下多行 upsert 不会撞(session_id, ord)PK(论证:a_i<b_i 且均严格递增,写入时目标 ord 必空)。注释把该前置条件写成了 load-bearing contract 并指明未来出现"向上重排"生产者时要加 delete-all fallback —— 防御姿势正确。 - ccr_entries:变更/消失的 key 先删行再 releaseContent(#4242 redirect-before-release 顺序保留),再插入 changed+new;不变 key 零 CAS 往返——正是之前讨论的"CAS blob 每轮销毁重建"浪费点,消掉了;CCR disarm 那一轮(key 全灭)= 全量删除+释放,语义与旧行为一致。
- blocks:planner 带预计算 sha256(store 信任 planner 哈希是 #3133 既有契约,同进程同字节,无新增风险);整行相等则整写跳过,不等就落回完整 upsert = 最坏退化成旧行为,不会更差。#4608 边界保持(CASE/IF(-1))在 skip 判定里同样考虑了("incoming -1 视为未变")。
- MySQL 孪生车道镜像对齐(批量参数上限 700 < SQLite 保守上限 999)。
- 观测:slow-ingest warn 从消息数改墙钟(>500ms)+
path=delta|full+ WAL 大小/checkpoint 三元组(只读诊断)——直接服务于 #5744 尖峰的后续归因,好。
本评审直接修的小问题(已在分支追加提交并推送)
BlockRow接口新增字段一带缩进被写成 12 空格(其余 4 空格)——纯格式,已归一化:commit5c6efce4d(chore(storage))。typecheck + 新套件在推送前复跑全绿。CI 会对新 head 重跑,预期不变。
不阻塞的遗留建议
📦 Merged Build PublishedThis merge to master was published as npm install -g billion-context@0.0.1074-master.83bddaa # this exact build
npm install -g billion-context@master # always the newest merged build
|
ranxianglei
left a comment
ranxianglei
left a comment
There was a problem hiding this comment.
更正:评审中"不阻塞的遗留建议"第二条归属有误
我在评审里写「folded-resident(lastMessagesFolded=true 恒走 full 路径)属 #5756/#5772 范围」——这句不对,更正如下:
#5756/#5772 实际覆盖的:full 路径里 historian sweep 的"已退役行重打戳+媒体重扫"部分(本 PR 之外、#5772 单独 lazy 化)。
folded-resident 恒走 full 路径的剩余大头不在任何 open PR 范围内(在 master 代码上核对):
store.ts:1053起的 messages 腿:对 plan 内每条消息执行putContent(SELECT refcount + UPDATE/INSERT)+INSERT OR IGNORE INTO messages+ 幂等场景的releaseContent(再一条 UPDATE)——full 路径 plan=整段日志,即每轮 ≈4 条语句 × H 行,两 PR 均未触碰;store.ts:928的staleRows全表 SELECT(无 seq 下界,每轮读 H 行),#5772 只省了写、没省这个读。
所以 folded-resident(以及任何 delta-fallback)会话在 #5755+#5772 合并后仍每轮 O(H),主导项是 messages 腿的 CAS 反复 bump/release。建议另立 issue 跟踪(方向:messages 腿按水位/identity 跳过未变行 + staleRows 读加 seq 下界),不与本 PR 捆绑。
第一条遗留建议(sessions 行整包 JSON blob 重写,另立 issue)经复核成立,维持原建议:每次 save 无条件重写六个 JSON blob 列(store.ts:984-990),其中 ccr_json/metadata_json 在大会话为 MB 级、每轮全量序列化,1 条语句、O(blob 字节),量级明显小于三张台账,属序列化侧,另立 issue 跟踪。
Resolve the single textual conflict in src/storage/store.ts, where both sides independently replaced the O(B^2) Array.some() stale-block sweep with an O(1) Set lookup (master via #5744, this branch via #5755). Kept master's incomingBlockIds form: behaviorally byte-identical, already shipped and reviewed on master, so the merged hunk matches what v0.1.213 already carries. All other files auto-merged cleanly.
…-value guard, media-refs carry-forward filter & cloud prefix families, oversize refusal messages that name the real fix, dsh durable lines with readable numbers, website streamKeepAliveMs typo, 5755 ord-crossing guard kept with regression tests - upstream-throttle: a saturated hold map (64/64 account-wide, no model-scoped hold to evict) silently dropped the next model-scoped 429 — now it warns once and the send is documented as ungoverned (ranxianglei#5199/ranxianglei#5889) - knobs: network.throttle429Scope matches its value EXACTLY; a typo now warns once and falls back to credential instead of silently doing so (ranxianglei#5199) - kernel media-refs: block summaries written by older builds can carry refs looksEphemeralSignedUrl would reject today; the carry-forward into merged blocks now applies the same filter so a dead signed URL cannot survive forever (ranxianglei#5865 review). EPHEMERAL_QUERY_PARAM also gains the Huawei OBS x-obs-*, Tencent COS x-cos-* and Baidu BCE x-bce-* prefix families. - native-intercept/dsh-native oversize refusals now say what to DO (restart host / fix the model window declaration), and dsh durable lines format numbers with thousands separators (ranxianglei#2924/ranxianglei#4068) - website: streamKeepaliveMs → streamKeepAliveMs (the real knob name) across main.js, docs/index.html, config-reference/process.yaml - CONFIGURATION.md/.zh-CN.md: throttle429Scope exact-match + model-string aliasing caveat documented - store/persist: ranxianglei#5755 refs ord-crossing guard (touching rows stay in the old-refs set until the new-refs commit lands) kept as defense-in-depth with a regression test kernel-change: 0.0.186 → 0.0.187 (media-refs carry-forward filter + cloud prefix families; compress.ts starvation-bound comment corrected)
What's wrong
#5744: on long sessions every save executed O(full history) native SQLite statements synchronously on the event loop — 96.5% of self time in node:sqlite
StatementSync.runon the reporter's instance,/__bili/healthstalling 17–30 s.The
refstable holds one row per message a session has ever seen (the byRaw ledger). It was maintained by DELETE-all + one-row INSERT on every save, so a delta save of 4 new messages into a 3k-message session re-wrote ~3,000 ref rows even though zero history rows changed. That section runs on BOTH the delta and the full path (ingestSession, oneBEGIN IMMEDIATEtransaction), so the #3133 watermark fast path — which made the plan O(new messages) — never made the store leg cheap. Replacing it wholesale was deliberate (review #2690: blind upserts hit the(session_id, ord)PK or leave ghost rows after native-compaction / #2695 renames-and-renumbers), so this keeps the same "incoming array is authoritative" contract while making it incremental.The change
src/storage/store.ts(ingestSession, refs section):SELECT raw_id, ord, ref, ref_num … WHERE session_id = ?) — a single range scan, µs-scale even at 3k rows.IN(…)statement (ghost rows can no longer outlive a single save — blocking-2 drift from Unified SQLite storage: corpus pins + parity core + shadow dry-run + drop-in read path (#2671 Phases 1 & 1.5) #2690 stays impossible); untouched survivors are skipped; added/moved rows go out as multi-row upsert batches capped at 700 bound parameters (~160 rows/statement), so first-ingest of a long session and compaction-renumber storms also cost ~one batch per 160 rows instead of H native calls.No schema change, no new config surface, no kernel/ changes. End state after any save is byte-identical to what replace-all produced.
Measurement (same harness, before → after)
K=4 new msgs/turn, ~850 B msgs, real
UnifiedStore+planIngest:Lane A refs work went from ~H statements (97% of all per-save statements) to a constant ~2; lane A's cost is now flat in history length. Lane B's remainder is the
completeLogsweep (per-row tombstone UPDATE + media cleanup over the non-windowed tail) — a larger view/scan design question tracked separately, plus the WAL-checkpoint cadence noted in #5744.Tests
tests/issue5744-incremental-refs.test.ts: pins (1) end-state parity with the legacy byRaw map across appends and a compaction-shaped renumber (pruned ids vanish, survivors shift ords down, no ghosts across an open/close cycle) and (2) the statement budget — steady saves touching ≤3 refs-table statements regardless of stored set size, renumber storm ≤6 (old code: H+1).storage-ingest-watermark,storage-backend-contract,issue3301-shape-flip-reanchor,issue3919-shadow-drift-forensics,media-cas,storage-batched-verification(57 pass / 0 fail / 1 pre-existing skip).tsc --noEmitclean.