Skip to content

[PR] fix: await unified snapshot writes and reset each debounce batch - #6186

Merged
ranxianglei merged 1 commit into
ranxianglei:masterfrom
Afloat16:2026-11-10_snapshot-write-lifecycle
Nov 11, 2026
Merged

ranxianglei merged 1 commit into
ranxianglei:masterfrom
Afloat16:2026-11-10_snapshot-write-lifecycle

Conversation

@Afloat16

Copy link
Copy Markdown
Contributor

Fixes #6178

Problem

Both prefix-affinity and session-order await persistUnifiedNow during shutdown, but that helper returns void while the KV read/modify/write is still queued. Graceful exit or restart can therefore proceed before the snapshot becomes durable. The unified path also leaves pendingSince set to the first batch forever, so after its max-delay expires later mutations schedule immediate writes instead of respecting the debounce window.

Changes

Return the serialized KV promise to flush callers and reset the pending clock when the current batch takes ownership of its write. Later mutations receive a fresh debounce budget even if an older write is still settling. Both writers retain their existing merge, bounds and failure-demotion behavior.

Validation

  • Rebased onto master e3220adc0f5412af83163666ca70a6afa05e3247 (package version 0.1.213). The intervening [PR] thirdparty-scan: flag dsh-live-compact as a known conflict (#5984) #6166 changes only third-party plugin detection, its tests, announcements and configuration docs; none of this patch's source or test files changed. The complete patch is byte-identical to the locally validated patch on 33104b1f9545caeb0344193009917ceef9969682 and applies cleanly.
  • Negative control on 33104b1f9545caeb0344193009917ceef9969682: 4 new regression cases still fail before the production fix.
  • Independent source tree based on 33104b1f9545caeb0344193009917ceef9969682 (92d15c3002258392fdc5f4007027f57bb703f2f2): 28/28 focused tests passed, with no failures, skips or cancellations.
  • Full project TypeScript checking and the standard npm run build passed on the temporary integration tree e186774d3428c85e2a4974c6e609b2a7381d6153 containing the seven independently submitted fixes on 33104b1f9545caeb0344193009917ceef9969682. The repository compiler options and include set are unchanged.
  • git diff --check passed. Local runtime: Node.js 24.22.1 on Linux. Full cross-platform remote CI remains pending.

The open immutable-journal work in #5480 changes legacy file publication; its unified helper still returns void. This patch fixes the shared async batch lifecycle without changing storage format, configuration or kernel code.

@ranxianglei

Copy link
Copy Markdown
Owner

🤖 Powered by ework · qwen3.8-27b

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

查重 (dedup): Not a duplicate. Scanned all open [PR] threads — no other PR targets #6178 (the async write-lifecycle defect). Nearest neighbor is #5480 (immutable-journal publication), which touches the same two source files but a different mechanism; coordination note below.

Labels: triage = bug, scope/storage, p1 (data-loss class: graceful restart can lose recent affinity/order snapshot mutations). The label API is not available in this agent environment, so the labels are reported here instead of applied.

Independent verification (redone locally at head 851e3bb, not taken from the description):

  1. Bug reality — confirmed at master e3220adc: persistUnifiedNow() in both src/affinity-persist.ts:166 and src/order-persist.ts:164 returned void while queueing the KV read-modify-write onto kvChain; the only callers awaiting it are the shutdown/restart paths (src/server.ts:1757-1758, src/restart.ts:488-489,498-499), whose await was therefore a no-op. Separately, pendingSince was only cleared on the legacy sync path (writeSnapshotLegacy's finally) — nothing reset it on the unified path, so once the first batch's max-delay expired, budget = persistMaxDelayMs() - (now - pendingSince) went negative and every later mutation scheduled a zero-delay write (per-mutation RMW instead of the documented ~500ms / 5s cadence from /bili/url/重启agent、重启bili、开新会话分支啥的都有概率丢失压缩 #1834's comment block). Corroboration: the third writer in the same shutdown sequence, flushResignPending() (src/apig-resign.ts:388), already does await pendingChain correctly — these two writers were the exceptions.
  2. Negative control (mine) — at the PR head I reverted only the two production files to their parent version and reran the new suite: 4/4 fail, each on exactly the assertion named in the test; restoring the fix: 4/4 pass.
  3. Adjacent suites — tests/affinity-persist.test.ts, tests/order-persist.test.ts, tests/affinity-unified-store.test.ts, tests/order-unified-store.test.ts: 24/24 pass on the PR head.
  4. Typecheck / CI — local tsc --noEmit clean on head; live check-run state for 851e3bb verified green (all completed: success/skipped, no pending, no failures). The API's mergeable_state: blocked is the review/approval gate, not a CI failure.
  5. Rebase claim — verified: the PR head's parent is exactly the current master tip e3220adc0f.

Test quality: both new cases have teeth and assert both directions of the gate (#3617 discipline): the durability case gates on deferred store writes and asserts shutdown does NOT proceed while any queued write is pending (including the flush's own write); the budget case asserts the second write does NOT fire early AND still fires at the normal debounce. Deterministic: mocked timer/clock APIs, deferred gates, env knobs saved/restored in finally.

Behavior changes (old → new, disclosed):

  • Steady-state unified-write cadence under sustained traffic: one immediate RMW per mutation after the first max-delay expired → the documented debounce/max-delay budget again (~500ms quiesce, 5s cap). This restores the intended /bili/url/重启agent、重启bili、开新会话分支啥的都有概率丢失压缩 #1834 behavior; the constants themselves are unchanged.
  • Graceful exit/restart: flushPrefixAffinity() / flushOrderSkeletons() now genuinely wait for the whole kvChain to settle (previously the await resolved immediately). Exit may take longer by the in-flight write duration; still bounded by the existing withExitDeadline wrap ([PR] storage: move six JSON sidecar states onto the unified sqlite store (#5753) #5757) — a wedged volume cannot hold the exit hostage.
  • Failure demotion (store error → kvReady=false + one-time warning + legacy-file fallback) is unchanged; the catch still resolves the chain as before, so flush never throws.

Coordination note (#5480): the open immutable-journal PR touches the same src/affinity-persist.ts / src/order-persist.ts plus their four test files, and per the author its copy of the helper still returns void. Whichever of the two merges second must rebase and carry both semantics (journal publication + durable-await/budget-reset). Flagging for merge ordering — not a defect in this PR.

Auto-merge gate (§7.4): eligible — pure fix:, one mechanism mirrored across the two sibling writers, reproducing regression tests, no config/wire/persistence-format/kernel/message-id changes, clean minimal diff, references Fixes #6178. Merging itself stays human-only.


结构化八问:

  1. 是什么问题:统一存储上的两个快照写者(prefix-affinity、session-order)把 KV 写入排队为异步链,但关机/重启路径的 await 对这条队列是空操作——优雅退出可能丢掉最近几秒的状态;同时去抖时钟每批完成后不复位,持续流量下之后每次变更都触发立即写库。
  2. 什么原因导致的:同一异步生命周期上的两个独立缺陷:(a) persistUnifiedNow() 返回 void,调用方的 await 没有可等的 promise;(b) pendingSince 只在 legacy 同步路径清除,统一路径永远钉死在第一批的时间戳上,预算算出负数 → 定时器延迟被钳到 0。
  3. 影响范围与严重程度:所有启用统一存储(默认)的实例。触发条件:优雅退出/重启与在途/排队写入重叠(健康卷上窗口≈毫秒级,慢卷更大)→ 最近的 affinity/order 变更丢失 → 重启后匿名会话可能孤儿化、以零压缩状态重新分叉(全量重发、缓存未命中,即 0.1.63 应用在DeepSeek Harness超出 固定阈值没进行压缩 #351 模式);持续流量场景 → 每次变更一次 RMW 的写放大。严重度 p1(数据丢失类;丢失的是辅助元数据而非会话正文)。
  4. 怎么修复的:让写者兑现其异步契约——返回串行化的 kvChain promise,使 flush 调用方真正等到全部排队写入落定;批次接管写入时清空 pendingSince,让后续变更获得全新的去抖预算。合并/限幅/失败降级逻辑一律不动。
  5. 是否有回归问题:逐项审过——去抖调度(有意的节奏恢复,已披露)、flush 持久性(有意变更,已披露)、失败降级(不变)、合并/限幅(未触碰)、legacy 文件路径(未触碰)、hydrate 流程(未触碰)、退出时限约束(保留)。无 fail-fast↔吞错反转、无阈值/时序漂移(除已披露项)、无输出格式变化。未发现回归。
  6. 是否破坏缓存:不涉及 prompt-cache、重试或增量游标策略;间接为正(减少重启导致的 affinity 丢失 → 减少冷启动重发)。
  7. 是否改了设计方向:不改。同一架构,补全既有统一写者的异步生命周期契约(与 flushResignPending 已有的正确模式对齐)。
  8. 是否引入了新的配置:无。

结论:可合。 修复最小且正确,回归测试钉得住原缺陷(本地负对照 4/4 失败 → 修复后 4/4 通过),CI 全绿。合并时请留意与 #5480 的顺序交互:后合的一方需要 rebase 并保留双方语义。

@ranxianglei ranxianglei added bug Something isn't working p1 scope/storage labels Nov 11, 2026
@ranxianglei

Copy link
Copy Markdown
Owner

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

Follow-up from the issue-side triage (#6178):

  • Labels applied (the earlier pass could not reach the label API): bug, scope/storage, p1 — judged from this PR's own diff (fixes a p1 data-loss defect in the storage layer).
  • Re-verified today at head 851e3bb766: targeted persistence suites 28/28 pass; negative control (revert only the two production files) → all 4 new regression cases fail; restoring the fix → 4/4 pass. tsc --noEmit clean. Remote CI still green: 30/30 checks completed, 0 failed / 0 pending.
  • Base drift: master has moved past the PR base e3220adc; no intervening commit touches src/affinity-persist.ts, src/order-persist.ts, or the new test file — the diff applies cleanly, so GitHub's "behind" flag is base-sha staleness only.
  • The merge-ordering note vs [PR] fix(persistence): publish concurrent snapshots through immutable journals #5480 stands as flagged above.

@ranxianglei

Copy link
Copy Markdown
Owner

🤖 Powered by ework · qwen3.8-27b

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

Status confirmed against live state (not from memory):

Nothing new to add to the review. Verdict stands: ready to merge — the only remaining steps are yours (merge is human-only), with the standing caveat that whichever of #6186 / #5480 lands second must rebase and carry both semantics.

@ranxianglei
ranxianglei merged commit ca1e6c8 into ranxianglei:master Nov 11, 2026
84 of 120 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

📦 Merged Build Published

This merge to master was published as 0.0.1174-master.ca1e6c8 (rolling master tag).

npm install -g billion-context@0.0.1174-master.ca1e6c8  # this exact build
npm install -g billion-context@master  # always the newest merged build

npm view billion-context@master prints the current version at any time.

ranxianglei pushed a commit that referenced this pull request Nov 12, 2026
master moved past the previous sync (dc041dd) by the web handoff-render
(#6260) + anthropic trailing-system (#5432/#5441) + snapshot-write-lifecycle
(#6186) landings. None of those commits modify src/plugin.ts or
src/agent/fork-adopt.ts (master's version of both == merge-base), so this
merge is clean and preserves the #5123/#4266 union resolution untouched.
ranxianglei pushed a commit that referenced this pull request Nov 12, 2026
Master advanced past the dc041dd merged earlier (web handoff rendering,
anthropic trailing-system tails #5432/#5441, snapshot write lifecycle #6186,
handoff-render #6260). Clean auto-merge — none of those commits touch the
side-lane surface (handle.ts / side-request.ts / side-lane tests), so no
conflicts and no logic change.
ranxianglei pushed a commit to john08281/billion-context that referenced this pull request Nov 12, 2026
… round 9)

Master advanced 33104b1 -> c26c31a (ranxianglei#6260, ranxianglei#5441, ranxianglei#6186, ranxianglei#5774,
ranxianglei#6166 et al.). Clean auto-merge, no conflicts: the new window's
blockOps work (ranxianglei#5765/ranxianglei#6166) and this PR's decide-lane keys land in
different regions of every shared file.

Also completes the round-8 docs regeneration that was left unstaged:
the committed head carried the decide-lane keys in the compress.yaml
seed but not in the generated output. Re-ran
node tools/gen-config-docs.mjs generate on the merged seed (341
entries; i18n ok: 256 keys, EN/ZH equal) — generated output now
matches the seed in docs/configuration[.zh-CN].md, website/main.js
and website/docs/index.html (blockOps + decide-lane keys, both
languages).

Verified: tsc --noEmit clean; unused-exports gate clean; targeted
suites green (nudge-decide + issue3004 selfheal + config-surface-guard
+ nudge-model-decided e2e). Full-suite verdict = remote CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Unified snapshot flushes return before durability and reuse an expired debounce budget

2 participants