Repository navigation
[PR] fix: read the checkpoint tip-seq as one MAX(seq) aggregate, not a full-log scan - #5239
Merged
Merged
Conversation
…uires (#4599) The #5140 recovery test proves forkSnapshot reconstructs a non-retained parent's raw tail from the UNIFIED index — but it never pinned its own storage mode. Under the CI storage baseline (preload-storage-baseline.mjs sets BILI_STORAGE_UNIFIED=legacy for the broad corpus) no unified rows exist, so the reconstruction has no source and forkSnapshot fails closed with 409 SNAPSHOT_UNAVAILABLE. Deterministic, not a flake: reproduces 100% on master and any merged branch with BILI_STORAGE_UNIFIED=legacy, passes without it (default shadow). Pin full like the sibling persist tests and restore the ambient value afterwards.
…l-log scan Found while working #4739 (reported in its thread and in PR #4765's review notes): UnifiedShadow.sessionTipSeq / sessionTipSeqAsync materialized the session's entire message log (every row + one content-CAS read + media hydration per row) just to reduce it to max(seq)+1 — the checkpoint insert's seq_at. O(N) row materialization per checkpoint on the write path. Rewire both legs onto #4730's existing lastLiveSeq (SELECT MAX(seq) over live rows; store/backend/mysql already carry it end-to-end): identical value (same live-row filter, empty log -> tip 0), fail-fast error propagation unchanged, zero new interface surface. tests/storage-tipseq-narrow-read.test.ts pins both halves: tip == max live seq + 1 AND zero message rows materialized (proven failing on the old scan).
Contributor
📦 Built Package ArtifactBranch: Latest build — install this exact versionnpm install -g billion-context@0.0.7226-pr.5239.494b020This exact version is immutable and reproducible. The rolling Recent builds (commit → version)
Download artifact
tar xzf billion-context-pr5239.tgz
npm install -g packageInstall instructions are refreshed on each push. |
ranxianglei
changed the title
… under the global legacy test baseline its sidecar never opens and the index reconstruction gate can never pass
ci-fixes at 67f8e89 still leaves four Windows shards red, and all four deltas already exist on PR #5178. This ports the missing hunks so the convergence branch actually converges. 1. `test (windows-latest, 2x, 3)` — #5103. `UnifiedShadow` builds its db path as `${dir}/index.db`, so the open-failure warning reads `C:\...\sessions/index.db`. `issue5103` asserts the warning contains `path.join(dir, "index.db")` — `...\index.db` on win32 — so a mixed separator never matches. `path` is already imported at :30 and the db is opened through the same join, so POSIX output is byte-identical. 2. `test (windows-latest, 22, 4)` + the 4788 leg — #4739/#4788 teardown. The suites arm a real `SessionStorage` under `BILI_STORAGE_UNIFIED=full`, so the sidecar is still open at teardown. `cancelAll()` clears timers and the build map but never closes the shadow; Windows refuses to unlink a directory holding an open file (`EBUSY`/`EPERM` on `index.db`). Use `closeQuietly()` — cancel, then close — in the short-lived-store teardown. 3. `test (ubuntu-latest, 24, 3)` + `(windows-latest, 22, 2)` — #5155. Both cold-rebuild cases waited for the debounced durable flush with a fixed `setTimeout(20)`. A millisecond budget is a deadline a loaded runner can miss, and the miss is an honest `409 !== 200` from a one-turn-stale table. Await the settle itself: `afterNextFlush("conv-ptrs")` resolves once the armed save lands. 4. `test (ubuntu-latest, 22, 3)` — #4788 verdict deadlock. `afterNextFlush` parks a resolver in `flushWaiters`, and the only thing that drains it is the debounce timer — which is armed `unref?.()`. Once that timer is the last pending handle the loop drains, the promise never settles, and node reports `Promise resolution is still pending but the event loop has already resolved`. Keep the timer referenced exactly while a waiter exists (`ref()` on waiter registration; the host-upkeep default stays unref'd). Both view hunks and the two test-teardown hunks are byte-identical to the ones already on PR #5178, so this converges the branches instead of inventing a third variant. Verified locally: `npx tsc --noEmit` clean; issue4788 0/4 -> 4/4; issue4739 4/4; issue5103 5/5; inc-canonical-pointers 3/3. The `cancelAll` -> `closeQuietly` hunk has no local negative control: reverting it keeps Linux green because `EBUSY`/`EPERM` only fire where an open handle blocks unlink. That control can only run on a Windows lane — stated rather than papered over. The #4788 arm's own reader is confirmed instead: `cancelAll()` leaves `index.db`, `index.db-shm`, `index.db-wal` in `/proc/self/fd` while `closeQuietly()` leaves none (probed directly).
…e settle (#4599) The turn() helper arms scheduleSave whose debounce timer is unref by design (persist.ts: a pending save must not hold the process open at shutdown) and then awaits afterNextFlush. Nothing else in that await is refd, so in a node:test child whose only surviving handles are unref sockets the event loop drains before the 50 ms debounce fires and the settle promise never resolves: all four subtests fail with "Promise resolution is still pending but the event loop has already resolved" / cancelledByParent. Reproduced deterministically on master 44067a5 AND on pre-merge ci-fixes 67f8e89 (standalone run, exact CI preload chain), while the same file has passed some CI legs by ambient-liveness luck - a born-flake per section 7.2 (no environmental luck). The pin at 67f8e89 does not touch this path. Fix: arm a refd 25 ms keep-alive interval around the afterNextFlush await (cleared in finally) so the debounce fires deterministically on every host. Test-only diff; production settle semantics untouched.
…ts ref-tracking fix (#4599) PR #5262 fixes #4788 at the source: scheduleSave now leaves the debounce timer refd while flush waiters exist (and afterNextFlush refs a pending unrefd timer when a waiter registers), so the settle is deterministic for EVERY caller on every host. The 25 ms keep-alive interval added in a3019b0 only patched this one test file and is redundant now; removing it leaves one mechanism instead of two. The comment stays: without it a future reader re-adds the interval or deletes the persist.ts tracking as dead weight.
…uite (review fix #5239) The new suite's finally block called only cancelAll(), leaving the SessionStore's unified index.db connection open. On Windows an open SQLite file cannot be unlinked, so rmrf threw EBUSY and the subtest failed in teardown (CI run 41081054054, win22-s4/s5 family). closeQuietly is the documented short-lived-store close (cancels saves + closes the sidecar handle) and never throws.
Contributor
📦 Merged Build PublishedThis merge to master was published as npm install -g billion-context@0.0.931-master.33bc979 # this exact build
npm install -g billion-context@master # always the newest merged build
|
ranxianglei
pushed a commit
that referenced
this pull request
Oct 31, 2026
Louie0806
added a commit
to Louie0806/billion-context
that referenced
this pull request
Oct 31, 2026
…drifted-name-leak Resolve the three comment-only conflicts left by ranxianglei#5239 landing ci-fixes on master: the ci-fixes side carries the functional change without this PR's explanatory comments, so keep the longer (commented) side. No semantic change.
ranxianglei
pushed a commit
that referenced
this pull request
Oct 31, 2026
red-family fixes Conflicts resolved in favor of owner policy + master's improvements: - src/storage/shadow.ts: generic sidecar-failure log keeps the round-3 'REFUSING TO START' wording (owner policy: no silent legacy fallback); sessionTipSeq takes master's MAX(seq) aggregate (#5239/#897756a7) but drops the breaker-era assertLive() line (method removed with the 3-strike breaker, 2026-10-30 owner policy). - tests/plugin-fork.test.ts: both #5140 pins keep master's version (same BILI_STORAGE_UNIFIED=full pinning, clearer comments). - src/persist.ts auto-merge: #4788 debounce-timer ref/unref host fix is compatible with the D2 spill path (afterNextFlush waiters re-ref). Verified: tsc clean; storage-tipseq-narrow-read + issue4739 + issue4788 + inc-canonical-pointers 12/12; plugin-fork + issue5140 + issue5188 + multi-instance-live 62/62; integrity policy core (issue4816/durability/ review-fixes/integrity-cache/issue5103/issue4949) 29/29.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(ai: vllm-qwen/qwen3.8-27b)
Closes #5238
What was wrong
UnifiedShadow.sessionTipSeq/sessionTipSeqAsync(src/storage/shadow.ts) materialized the session's ENTIRE message log — every row plus one content-CAS read and media hydration per row — just to reduce it to max(seq)+1, the checkpoint insert'sseq_at(single caller: src/persist.ts). O(N) row materialization per checkpoint insert on the write path. Adjacent finding from #4739's work (reported in its thread and in PR #4765's review notes); same failure domain as #4739 but a different path (checkpoint writes, not ordinary loads), so it was left out of #5198's scope.Fix
Rewire both legs onto #4730's existing
lastLiveSeq(SELECT MAX(seq)over live rows — store/backend/mysql already carry it end-to-end): identical value semantics (same live-row filter; empty log → tip 0 as before), fail-fast error propagation unchanged (store/backend-level calls, no swallowing facade), zero new interface surface. ~6 lines in shadow.ts.Behavior changes (disclosed)
None observable: the returned tip value is byte-identical for every input (proven by test against the old algorithm's own input set). Only the I/O pattern changes: O(N) rows + N CAS reads + N hydrations → one aggregate query. No schema/config/kernel/wire changes (config-surface: N/A).
Test evidence
New tests/storage-tipseq-narrow-read.test.ts pins both halves of the contract: tip == max live seq + 1 AND zero message rows materialized (via the existing messageRowReads instrumentation; proven FAILING on the pre-fix code, which read all 300 seeded rows). Targeted local runs green: this suite 1/1, chain-checkpoint-step2/step3 26/26 (the checkpoint-insert path this feeds), tsc --noEmit clean. Full-suite verdict: CI on this head.
查重: 不重复 —— record issue #5238 carries the screening result (no existing issue/PR on sessionTipSeq / checkpoint tip-seq).