Repository navigation
[PR] fix(#5236): sessions rm must not full-VACUUM under a live foreign writer - #5330
ranxianglei wants to merge 2 commits into
Conversation
ranxianglei
changed the title
📦 Built Package ArtifactBranch: Latest build — install this exact versionnpm install -g billion-context@0.0.9077-pr.5330.ebe6adfThis exact version is immutable and reproducible. The rolling Recent builds (commit → version)
Download artifact
tar xzf billion-context-pr5330.tgz
npm install -g packageInstall instructions are refreshed on each push. |
|
(ai: vllm-qwen/qwen3.8-27b) CI 状态说明:首跑三条红全部为继承性 master 失败,与本 PR diff 无关——
|
(ai: vllm-qwen/qwen3.8-27b) 查重:不重复——与 #4612/#5236 同属「活的外写者下 full-VACUUM 饿死共驻实例」机制家族,但 #4612 只覆盖了 gc 侧三个守卫位(runGc CLI 拒绝、admin 面板 409、store.gc 内部);本 PR 修的是 owner 裁决 3a6ab62 点位审计里唯一漏网的第四处 runRm,是本家族的收尾修复,跟踪 issue 即 #5236。 标签:bug、scope/storage、p2(存储车道 CLI 缺陷;特定并发条件下共驻实例写入被饿死,有 --no-vacuum 绕行,无数据丢失 → p2 非 p1)。 代码审查结论(独立重审,非复读 issue):
CI 现状(head f97b257,已 merge origin/ci-fixes 0371089):
八问:
收工判定:代码与测试达到可合并标准;当前两条红均为继承性 master 门禁失败(根因与修法证据已交 #4599 哨兵),待 ci-fixes 收敛后重跑转绿即可合并——本 PR 侧无需再做任何改动。 |
…reign writer holds index.db After owner ruling 3a6ab62 (a live second writer never blocks startup), per-operation guards are the sole defense against an O(db-size) exclusive VACUUM starving a co-resident instance. storage gc kept its #4612 up-front refusal (CLI + panel endpoint); sessions rm's default full vacuum had none — while a proxy held index.db it could win the exclusive lock and starve that proxy's writes across the whole rewrite. runRm now checks store.unifiedActiveWriter() (same source of truth as runGc's refusal) before the shrink: a live foreign writer ⇒ skip ONLY the shrink with an actionable pid-named note; the deletion is a short transaction and stays committed (same graceful shape as the pre-existing SQLITE_BUSY path). No-writer and --no-vacuum paths unchanged. All four full-VACUUM sites are now guarded: runGc, runRm, admin.ts panel endpoint (#4612 409), store.gc.
f97b257 to
24bfdb0
Compare
…ES (PR #5330 red CI) The suite spawns one bare `node -e` dummy child (live PID holder for the <db>.lock fixtures); SPAWN_RE matched it but it was never registered, so the pretest tripwire aborted all 11 test shards before any test ran (run 42962029584). Concurrency reviewed per #1248: no ports bound, no network I/O, mkdtemp-isolated per test — same shape as the #4612/#5046-B entries; appended last so existing slot indices are untouched.
(ai: vllm-qwen/qwen3.8-27b) CI lane attempt 1/3 — conflict resolved AND the red-CI root cause found and fixed this run. Summary of everything done: 1. Conflict resolution. The branch ( 2. Red-CI root cause (found while waiting for the rerun). All 11 3. Fix. Independent labeled commit Live status honestly stated: the mirror check-cache still reflects the previous head (poller lag); the fresh run on 八问(中文结构化结论)
判定:冲突已清( |
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) Status update (full run summary is in my previous comment above): conflict resolved via rebase onto master tip |
[bot] 🏷 (ai: vllm-qwen/qwen3.8-27b) CI lane run — verified live, no action was needed this time. The "merge state: dirty" in the trigger is stale mirror data predating last run's rebase; live sources say otherwise: Verified in THIS run (not from memory):
So this run: zero git operations, zero pushes, zero new commits — the blockers were already cleared by last run ( 八问(中文结构化结论)
判定:PR 当前唯一待办 = 最后一个 test 分片跑完出结果(0 failed / 1 in_progress);若绿即可合并,若红下轮按签名分账。本轮无需 owner 决策。 |
[ework] CI fix verified green ✓ |
2 similar comments
[ework] CI fix verified green ✓ |
[ework] CI fix verified green ✓ |
Refs #5236 — post-merge supplement to PR #5213 / owner ruling
3a6ab628(via PR #5294).What
bili sessions rm <id>defaults to a full VACUUM after the deletion. While another bili process (e.g. a running proxy) holds index.db, that full VACUUM can win the exclusive write lock and starve the other instance's writes across the whole O(db-size) rewrite — exactly the hazard class #4612 fixed forstorage gc --vacuum full. After the owner ruling 3a6ab62 (2026-10-31: a live foreign writer never blocks startup), per-operation guards are the sole defense line — and runRm was missing its guard while runGc kept its (#4612).Found while closing out #5236 on current master (caaab21): the ruling's commit message names runGc's
store.unifiedActiveWriter()guard as the surviving enforcement point; a call-site audit shows four full-VACUUM sites, three already guarded (runGc CLI refusal, admin.ts panel 409, store.gc internal), one not (runRm).Fix
runRm checks
store.unifiedActiveWriter()before the shrink (same source of truth as runGc's refusal): live foreign writer ⇒ skip ONLY the shrink up front, with an actionable pid-named note (shrink skipped: another process (pid N) is actively writing index.db — … or run 'bili storage gc --vacuum incremental'). The deletion itself is a short transaction and stays committed — same graceful shape as the pre-existing SQLITE_BUSY path in the same function. No-writer and--no-vacuumpaths unchanged. JSON lane carriesvacuumed=false+vacuumNote(fields pre-existed; no schema change).Behavior change (old → new)
sessions rm <id>with a live external writer attempted the full VACUUM anyway — either it won the exclusive lock and starved the co-resident instance's writes for the whole rewrite (minutes on multi-GB dbs), or it lost the race with SQLITE_BUSY and skipped the shrink with a generic "database busy" note.Tests
New
tests/issue5236-rm-vacuum-writer-guard.test.ts: positive gate (skip + pid-named note + deletion-committed proof + json honesty) and both negative gates per #3617 discipline (no writer ⇒ vacuum runs; dead holder ⇒ vacuum runs). Local: typecheck clean, suite 3/3 green on caaab21. Full-suite verdict = CI on this head.Single-module scope (
src/storage-maintenance.ts+ one new test file); no config surface, no kernel/, no wire changes, no version touch.