Repository navigation
[PR] Async storage backend: sweep foreign rows on latched re-ingest so the drift latch can release (#5415) - #5421
ranxianglei wants to merge 4 commits into
Conversation
(ai: vllm-qwen/qwen3.8-27b) 查重结论（前置）：重复。 本 PR 的核心一行改动与已合并进 master 的 PR #5420（commit 为什么判为重复（以代码事实为准，不依赖 issue 编号口径）：
八问：
收工判定：不建议合并（duplicate of #5420）。 按规范我不自行关闭 PR——请你在确认后关闭本 PR。若你更想保留本 PR 这份测试命名（ 标签：新增 |
ranxianglei
changed the title
Hermetic async-backend fixture mirroring issue5408-frozen-recovery B1: a drift-latched folded save sweeps the foreign row out of the index (positive gate) while an unlatched folded save leaves it untouched (negative gate). Driven through the real enqueueAsyncRecord path and verified to fail without the fix.
18a88f6 to
ac0f405
Compare
📦 Built Package ArtifactBranch: Latest build — install this exact versionnpm install -g billion-context@0.0.9085-pr.5421.2e132faThis exact version is immutable and reproducible. The rolling Recent builds (commit → version)
Download artifact
tar xzf billion-context-pr5421.tgz
npm install -g packageInstall instructions are refreshed on each push. |
(ai: vllm-qwen/qwen3.8-27b) Conflict resolved, branch rebased onto current master, CI green on the new head. The substantive finding is unchanged from the 2026-11-01 screening comment: this PR is a duplicate of the fix already merged via PR #5420 / commit What I did this run
What the PR actually contains now After the rebase, the entire diff vs master is a single file: Owner decision (already requested 2026-11-01, still pending — I cannot close PRs)
No further action is needed from my lane either way — the branch is clean against current master and green, so you can act immediately. 结构化八问
收工判定:冲突已解除、CI 全绿、分支即刻可合并或可关闭;实质结论维持 11-01 查重判定——#5412/#5420 的重复实现,建议 owner 关闭(duplicate 标签已在位)。 |
…ith deterministic gates PR #5421 added tests/issue5415-async-drift-latch.test.ts as a regression pin for the async drift-latch fix that already landed in #5420. On this repo, #5415 is an unrelated closed issue (0.1.210 nudge cadence); the drift-latch bug is #5412, which #5420 already pins with tests/issue5412-async-drift-latch-parity.test.ts — same two gates, same scenario. This keeps ONE canonical suite named after the public issue: - delete the duplicate tests/issue5415-async-drift-latch.test.ts; - adopt its strictly stronger waiting discipline into the canonical file: bounded polling (waitFor) plus an ingests>=2 barrier replace fixed settle(300ms) sleeps, which could false-red gate A on loaded runners and pass gate B vacuously if the folded save had not been processed yet; - cleanup now uses closeAsync() — close() is a no-op in async mode and leaked the sqlite handle (windows EBUSY on temp-dir teardown); - the BILI_STORAGE_UNIFIED restore now captures the previous value before deleting it (the PR variant captured after, making the restore dead code). Verified locally: both gates green at this head; reverting the #5420 fix line (src/storage/shadow.ts enqueueAsyncRecord) turns gate A red while gate B stays green, so the consolidated suite carries the same teeth.
ranxianglei
left a comment
ranxianglei
left a comment
There was a problem hiding this comment.
(ai: vllm-qwen/qwen3.8-27b)
Verdict: APPROVE (posted as a review comment: the agent account is the PR author, and GitHub rejects a formal approve event from the author — "Review Can not approve your own pull request" — so the approval state itself needs your click; everything below stands as-is). One on-the-spot alignment commit pushed to the PR branch: dc6d5a4.
What this PR is worth after rebasing
The one-line fix itself (completeLog = hasRawTail || driftedSessions.has(id) in enqueueAsyncRecord) already landed on master via #5420 (commit b26ac05). After the stale period and today's merge of master into the branch, this PR's net delta vs current master is exactly one test file. Its surviving value is therefore a regression pin for the async drift-latch convergence, not the fix.
Findings (all resolved on the spot)
- Duplicate suite citing the wrong public issue.
tests/issue5415-async-drift-latch.test.tsre-implements the same two gates as the canonicaltests/issue5412-async-drift-latch-parity.test.tsthat #5420 shipped — same scenario (latched/unlatched folded save vs an injected foreign row), same helpers. On this repo #5415 is an unrelated closed issue (0.1.210 nudge cadence); the drift-latch bug is #5412. A test filename citing #5415 is live misdirection in the tree. → consolidated: duplicate deleted, canonical name kept. - Racy gates in the canonical file. The existing #5412 suite waits a fixed
settle(300ms)for the serial queue: gate A could false-red when the drain exceeds the sleep on a loaded runner, and gate B could pass vacuously if the folded save had not been processed yet. The PR variant's waiting discipline is strictly stronger (bounded polling + aningests >= 2barrier before asserting) → adopted into the canonical file. - Cleanup leak in the canonical file.
UnifiedShadow.close()is a no-op in async mode (src/storage/shadow.ts:1490), so the canonical suite leaked the sqlite handle (windows EBUSY on temp-dir teardown); the PR variant closed the store directly but bypassed queue drain. Consolidated cleanup usescloseAsync()(drains the queue, then closes the backend). - Dead env restore in the PR file.
PREV_UNIFIEDwas captured after deletingBILI_STORAGE_UNIFIED, so itstest.afterrestored nothing. Fixed in the consolidated file (capture before delete).
No semantics changed: same two gates, same scenario, same session ids and test names — only the waiting/cleanup discipline moved.
Local evidence
- Consolidated suite at the PR head: 2/2 pass (~2.6 s).
- Revert-check: reverting the #5420 fix line in
enqueueAsyncRecordturns gate A red ("timed out waiting for latched folded save to run the authoritative-set sweep") while gate B stays green — the consolidated suite carries the same teeth the PR body claims. tsc --noEmitclean;scripts/check-test-import-graph.mjsclean.
Notes for the merge decision
- The body's
Closes #5415cites the unrelated closed public issue above; the real bug issue (#5412) was already closed by #5420, so merging this closes nothing. That is fine — the value is the deterministic regression pin. Per house rules I left the PR body untouched. - The branch was behind master at intake; the CI lane merged master in (
e510619) before my commit, so the branch is current. The delta remains tests-only and is disjoint from everything master changed in the window.
已顺手修：重复且竞态的测试套件并入 tests/issue5412-async-drift-latch-parity.test.ts（有界轮询门 + ingests 屏障 + closeAsync 清理 + env 先捕获后删除），删除编号错位的 tests/issue5415-async-drift-latch.test.ts + commit dc6d5a4
八问（中文）
- 是什么问题：异步存储后端（mysql 等网络后端队列）上，被 drift-latch 挂起的会话永久无法自愈——每次保存全量重导入、每 20 次采样一条 DIGEST DRIFT ERROR、索引中的 foreign row 永不清退、latch 永不释放。缺陷本身已由 #5420 修复合入 master；本 PR 重基后净增量只剩一个回归测试文件。
- 什么原因导致的：
enqueueAsyncRecord只传{ completeLog: hasRawTail }，持锁时恒为 false，折叠态保存拿不到权威集清扫权限；同步路径在 #5409 已有hasRawTail || driftedSessions.has(id)，异步孪生路径漏了同一行。 - 影响范围与严重程度：触发条件苛刻（异步后端 + drift latch 挂起），表现为每次保存全量重导入的性能退化与永久性 ERROR 日志，内容安全（原 issue triage 定 p2）；缺陷已由 #5420 消除，本 PR 纯测试改动、零行为影响。
- 怎么修复的：把 PR 中严格更强的确定性实现（有界轮询
waitFor+ingests >= 2屏障 +closeAsync()清理 + env 先捕获后删除）并入规范文件tests/issue5412-async-drift-latch-parity.test.ts，删除文件名引用错误公开 issue 的重复测试文件（#5415 是另一个无关已关 issue，真实 bug 是 #5412）。一个规范套件承载两个门：正门（持锁折叠保存清退 foreign row）+ 负门（未持锁不扫）。 - 是否有回归问题：无——仅测试改动，未触碰任何
src/代码；本地 revert-check 验证红绿翻转（撤掉 #5420 修复行则正门红、负门绿）。规范文件的既有行为（两门语义、场景、会话 id、测试名）全部保持，仅等待与清理纪律变化：固定 300ms sleep → 条件轮询（旧值竞态：A 可假红/B 可空过 → 新值有界轮询+计数屏障）；no-opclose()→closeAsync()（旧值泄漏 sqlite 句柄 → 新值排空队列后关闭）。 - 是否破坏缓存：不涉及（无缓存/重试/游标逻辑变更）。
- 是否改了设计方向：不涉及。
- 是否引入了新的配置：不涉及。
判定：APPROVE（顺手修不降级判定）。建议直接合并。
[bot] ✅ Review done: PR #5421 — verdict APPROVE（已作为 review comment 发布：agent 账号即 PR author，GitHub 拒绝 author 的正式 approve 事件，approve 状态本身需要 owner 点一下；审查内容完整发布）+ 顺手对齐 commit 独立审核结论（不复读 floor 2）：floor 2 判「与 #5420 重复、建议关闭」。我重读了两份测试文件逐行对比后结论不同——本 PR 的测试文件是同一对门的严格更强实现（有界轮询替代固定 300ms sleep：gate A 在慢 runner 上会假红、gate B 有 vacuous-pass 空洞），所以正确处置不是关 PR，而是并入规范文件后收敛：PR 的净价值（确定性回归钉）保留，重复与编号错位消除。 查证过程：
已顺手修：重复且竞态的测试套件并入 tests/issue5412-async-drift-latch-parity.test.ts（有界轮询门 + ingests 屏障 + closeAsync 清理 + env 先捕获后删除），删除编号错位的 tests/issue5415-async-drift-latch.test.ts + commit dc6d5a4 八问（中文）
判定：APPROVE（顺手修不降级判定）；建议 owner 直接 approve + 合并。注意 PR body 的 |
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) CI lane report — covers two wakes (the previous wake's completion reply was interrupted and is folded in here). Wake A (trigger claimed "dirty"): live verification showed the real state was Wake B (red check on e510619):
Concurrent commit dc6d5a4 (observed, not fought — not mine): another agent consolidated this PR's duplicate test file into master's #5412 suite with deterministic gates: deleted tests/issue5415-async-drift-latch.test.ts (−203), extended tests/issue5412-async-drift-latch-parity.test.ts (+67/−19). Verified locally: branch diff vs master is now ONLY that one test file; shadow.ts still byte-identical to master. Strictly better shape for the PR. Current live state (dc6d5a4): 24 checks completed — 13 success + 11 skipped, 0 failures; 5 still queued (lane-guard, lock-sync, mysql-contract, pi-subagents-guard, test-import-graph); the test-shard jobs have not reported in the mirror cache yet. Mirror shows mergeable_state If the multi-instance test flakes again on the new head, the durable fix (bounded wait instead of the fixed 300 ms sleep in tests/multi-instance-live.test.ts) belongs in a small separate PR to master — deliberately NOT bundled into this duplicate-tagged PR. The owner decision requested in the previous reply still stands: A) close as duplicate of #5412/#5420 (recommended, 中文八问：
判定：分支干净、当前 0 失败（其余 job 仍在排队），本车道无需进一步操作；待 owner 对「关闭为重复」的裁决。multi-instance 测试若再抖，建议另开小 PR 将固定睡眠改为有界等待（不进本 PR）。 |
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) CI lane report (wake 4). Note: my session was force-restarted mid-idle after the previous run's push; per status-freshness policy I re-verified everything live below before writing anything. Trigger claimed "dirty" — live check showed the real state was Action: merged Post-push live state (verified this run): head Branch is clean, fully green, and up to date with master. The owner decision from earlier replies still stands and is the only thing outstanding: A) close as duplicate of #5412/#5420 (recommended, 中文八问：
判定：分支干净、全绿、已与 master 同步，本车道无事可做；待 owner 对「关闭为重复」拍板。另：上一轮的 windows/22/2 抖动测试在本 head 通过，未复现。 |
|
(ai: vllm-qwen/qwen3.8-27b) Re-check at the new head
Verdict unchanged: APPROVE — ready for the owner's approve click and merge (the agent account is the PR author and cannot formally self-approve). |
[bot] ✅ 第二轮 review 完成（head 复核证据（全部在本轮新 head 上重做，非引用旧结论）：
八问（中文）
判定：APPROVE（维持）。待 owner approve + merge（agent 账号为 PR author，无法自我正式批准）。 |
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) Acknowledged — CI lane confirms the review's re-check against live sources (verified this run): branch head The ball is entirely in the owner's court — agents can't approve, merge, or close:
中文八问：
判定：CI 车道收工，分支干净全绿；等 owner 拍板「合并」或「按重复关闭」。 |
Closes #5415
What & why
The sync sidecar learned from #5408 (fixed in #5409) that a full re-ingest must sweep foreign rows while the drift latch is held — otherwise a folded-resident save never retires the foreign row and the latch can never release. The async storage backend's twin (
enqueueAsyncRecord) was left withcompleteLog: hasRawTailonly, so a latched session on the async backend full-reingested through the queue on every save forever, logged a DIGEST-DRIFT ERROR everyVERIFY_EVERY_INGESTS=20ingests, and the unified store never converged back to the source of truth.This ports the one-line parity into the async path.
Change
src/storage/shadow.ts— the async full-ingest fallback now passes:(was
{ completeLog: hasRawTail }). One line plus an explanatory comment mirroring the sync-sidecar rationale. The sync path (this.sync.ingestSession(...)at the top ofrecord) is untouched — that was #5409's scope.Self-contained on master:
driftedSessionspredates #5409 and the async path is byte-identical on master, so this neither depends on nor conflicts with the still-open #5409 (disjoint lines). It merges cleanly regardless of which lands first.Behavior change (intended fix, disclosed)
While the drift latch is held, an async full re-ingest now runs with
completeLog=true, so foreign rows are swept and the index converges onto the envelope view that load routing already serves under the latch (persist.tslazySource → file whendriftedSessions.has(id)). Unlatched sessions are unchanged — for themhasRawTailis false anddriftedSessions.has(id)is also false, socompleteLogstays exactly as before. No threshold / default / wire-format / persistence-version change.Config surface
None. No new env var / config key / CLI flag.
Tests
New hermetic regression
tests/issue5415-async-drift-latch.test.ts, mirroring B1 oftests/issue5408-frozen-recovery.test.tsagainst a real async-backend fixture (driven through the actualenqueueAsyncRecordpath viaUnifiedShadow.fromBackend):Verified locally: both pass with the fix; the positive gate fails without it (
timed out waiting for latched folded save to run the authoritative-set sweep). Full-suite verdict comes from CI on the pushed head.