Skip to content

fix: multi-instance shared-state hardening — activePack round-trip + affinity clobber guard (#1724) - #1737

Merged
ranxianglei merged 3 commits into
masterfrom
2026-09-30_multi-instance-store-hardening
Sep 30, 2026
Merged

ranxianglei merged 3 commits into
masterfrom
2026-09-30_multi-instance-store-hardening

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Summary

Part of #1724 (multi-instance shared ~/.local/state/billion-context/ restart race). This PR lands batch 1 of the agreed plan — the two safe, independently-valuable persistence fixes. The drain barrier (Track 1) and web-UI scoping (Track 2) follow as separate PRs; state partitioning (Track 3) stays deferred to owner design.

Fix 1 — meta.activePack round-trip (src/persist.ts)

The observed symptom — a dsh-web session's /acp panel flipping pack: lean → pack: default after an auto-update restart — has a root cause independent of the race: buildRecord() persists the whole meta via spread (meta: { ...session.meta }, so activePack reaches disk), but buildSession() reconstructed meta as only {protocol, upstreamOrigin, label, title} and dropped activePack. Every reload therefore reset it to undefined, and the panel's ?? "default" fell through. This fires on every restart, including clean single-instance ones.

  • Added activePack?: string to the persisted-meta type (it was already written to disk; the type just did not declare it).
  • buildSession() now reads activePack back from the grouped meta.

Fix 2 — prefix-affinity cross-instance clobber guard (src/affinity-persist.ts)

prefix-affinity.json was written whole-file last-writer-wins with no staleness guard (unlike the session files, which carry the #405 counter guard). Two coexisting instances — or a new process hydrating before an old process's final flush — would each overwrite the other's chains, silently dropping them (the issue's risk #2, chain fork). A pure monotonic generation watermark cannot order two processes (both hydrate the same base), so this generalizes #405's "keep fresher" policy to a store that has no global counter:

  • On every snapshot write, the current on-disk entries are read and unioned with our in-memory snapshot, keyed by sessionId; per id the deeper/fresher chain wins (depth, tie → lastSeen).
  • Chains present on only one side are preserved — neither instance's chains are lost.
  • A single info line logs when the merge kept disk-only chains or resolved a same-depth fork, turning silent loss into visible evidence.
  • A disk read failure degrades to the previous plain-snapshot behavior.

Behavior changes (flagged)

  • activePack now survives a proxy restart (old: reset to default/absent on every reload → new: preserved). This is the intended fix for the reported symptom.
  • prefix-affinity.json write is now a read-merge-write union, not a blind snapshot. The on-disk file may retain chains created by other instances; a same-depth fork emits one info log line. Minor benign side effect: a chain TTL-evicted in one process can linger on disk until the next hydrate re-filters it (affinity hints self-heal via TTL).

No wire/message-shape change. No config surface added (no new field/env/flag). No acp-kernel pin change.

Tests

  • tests/persist-active-pack.test.ts (new): asserts meta.activePack survives a persist→reload round-trip (fails before Fix 1).
  • tests/affinity-persist.test.ts: two new cases — a sibling instance's disk-only chain survives our flush (clobber prevention), and a fresher on-disk copy is not rolled back by a stale in-memory write. Both fail without Fix 2.

Pre-flight

  • npm run typecheck — clean
  • npm test — 3335 tests, 0 fail (17 skipped = e2e-gated)
  • npm run build — success

Part of #1724 (not closing — drain barrier + web scoping remain).

@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-30_multi-instance-store-hardening (8670ea1)

Option A — Install this exact build (recommended)

npm install -g billion-context@0.0.1688-pr.1737.c1b2602

This exact version is immutable and reproducible. The rolling pr tag tracks the newest test build across all PRs.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr1737.tgz
npm install -g package

Install instructions are refreshed on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

收到,开始按 PR Review 规范审 #1737(batch 1:activePack 读回 + affinity clobber 守卫)。已核对原 issue #1724 的三轨计划,方向一致。接下来:① 逐行审两个 fix 的代码与既有行为清点;② 验证回归测试在改动前确实失败、改动后通过;③ 检查分支落后 master 4 个提交是否有热文件冲突。CI 目前 9/13 绿,4 个 test matrix job 还在跑。稍后给完整结论。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review complete for PR #1737 (head 8670ea13, after my two hardening fixes pushed below).

① Direction vs #1724 — correct. Both fixes target real root causes of the multi-instance shared-state race, not symptoms: Fix 1 removes the mechanism that produces the pack flip (the reader dropping activePack on every reload — fires on clean single-instance restarts too), Fix 2 removes the last-writer-wins clobber on prefix-affinity.json. Root-cause litmus passes: if the same defect resurfaced via another reload path or another instance topology, both fixes still hold.

② Regression inventory (existing behaviors audited):

  • buildRecord() meta spread → preserved unchanged; only the reader side changed.
  • Session.meta has exactly 5 fields (protocol/upstreamOrigin/label/title/activePack); buildSession now round-trips all five — no other field was being silently dropped.
  • activePack consumers: stamp point src/server.ts:2047 (per-request latest-wins, so a stale persisted value only persists until the next request overwrites it) + display-only readers (web/client.ts:688, web/sessions-data.ts:429, plugin.ts:880, acp-status.ts:50, all ?? "default"). No wire/message-shape impact. ✅
  • Affinity call sites (server.ts:691 boot hydrate, :805 shutdown flush, :2061/:2084 debounced schedule): single writer of the file confirmed; hydrate path untouched. ✅
  • Read-failure degradation: disk read error → previous plain-snapshot behavior, unchanged. ✅
  • No config surface added, no kernel pin change, no persistence format change (file schema identical; write policy changed as flagged).

③ Code review — found two gaps, fixed directly on the branch (commit 8670ea13, +80/-18):

  • A. Disk-entry validation weaker than importSnapshot. readDiskEntries() only checked sessionId+depth; a corrupted same-depth entry lacking lastSeen would win the merge (m.lastSeen >= undefined → false keeps disk) replace our valid chain, and persist the garbage back. Fixed: filter now mirrors importSnapshot's full field validation (tailHash string / itemHashes string[] / lastSeen number).

  • B. Monotonic file growth. The union prunes nothing, so within one process lifetime prefix-affinity.json grows by one entry per chain that churns through the LRU cap (256 × ~1–8KB itemHashes each); TTL-expired entries also linger until the next hydrate. Pre-PR code kept the file ≤256 always. Fixed: new normalizeForWrite() applied before every write — drops TTL-expired entries and caps at MAX_TRACKED_SESSIONS keeping most-recently-seen, both mirroring importSnapshot's own semantics (hence those two consts are now exported from src/prefix-affinity.ts). Also refactored so the info log line reports post-normalization counts (accurate evidence, not pre-prune numbers).

    ⚠️ Behavior changes from my commit (old→new): file size unbounded-within-lifetime → always ≤256 entries; TTL-expired chains lingered until next hydrate → pruned at next write (refines the side-effect you already flagged in the PR body — they no longer linger past a write); malformed disk entries could win same-depth merges → rejected. All three are strict improvements in the direction this PR itself states.

④ Test validity — real teeth. Verified pre/post on separate worktrees: on master + PR's tests, exactly the 3 new tests fail (union-clobber, stale-rollback, activePack round-trip); on the PR head all pass. My commit adds 2 more cases (write-time cap/TTL pruning with 305 pad entries + 1 expired; malformed-entry rejection) — 34/34 green across the three affected suites. Not tautologies: each asserts specific surviving/evicted session ids and file contents.

⑤ Diff cleanliness: 4 files, small, every line on-topic. My commit stays within the same 2 source files + their test file. Clean.

Stale base: branch is 4 commits behind master (subagent-sidecar work, #1702/#1722) but those commits touch only src/server.ts/src/stream.ts/src/subagent-sessions.ts + e2e tests — zero overlap with this PR's files, no conflicts expected when it lands. Rebase optional.

Open question: in #1724 floor 8 the agreed batch 1 had three items — ① activePack read-back, ② affinity clobber guard, ③ fork-clobber visibility extending #405's freshness guard to session files (making silent message-loss visible). This PR delivers ①② only, and ③ isn't mentioned as deferred anywhere in the body. Where did ③ go — dropped, folded into Track 3, or a follow-up? If it's still owed, say so explicitly so it doesn't evaporate.

CI at 8670ea13: 9/13 success (e2e, hermetic registry e2e, version-guard, build-artifact, all 5 client lanes); 4 test-matrix jobs (ubuntu/windows × node 22/24) still in_progress — no failures seen. Local: typecheck clean, target suites 34/34, build ok.

Verdict: mergeable once the 4 matrix jobs go green and the ③ question gets an answer (it doesn't block the code — both shipped fixes stand independently).

中文摘要:审查了多实例共享状态加固 PR——两个修复都打到了根因(activePack 读取端丢失、affinity 文件最后写者覆盖),我直接在分支上补了两个加固(磁盘条目完整校验、写入时按 store 自身上限做 TTL/LRU 收敛,修复了 union 导致的文件无限增长),测试前后对照证明有效,矩阵任务跑完且 ③(session 文件 fork 可见性)着落说明后即可合并。

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 更新:head 8670ea13 的 CI 已全部通过 —— 13/13 success, 0 failures(含最后的 windows-latest × node 24 矩阵任务)。代码侧无阻塞项;剩余待办只有上面那条问题:#1724 batch-1 的 ③(session 文件 fork-clobber 可见性 / #405 扩展)的去向说明。说明后即可合并。

@ranxianglei
ranxianglei merged commit f8c6aca into master Sep 30, 2026
13 checks passed
ranxianglei pushed a commit that referenced this pull request Sep 30, 2026
The 7-day TTL contradicted the month/year-level single-session promise:
a chain idle over a week lost its recognition, forking the resumed
conversation into a fresh session on the next request. Chains are now
permanent — they leave the table only under LRU capacity pressure
(raised 256 -> 1024, ~8MB worst case):

- drop the TTL gate in peekChain() and the sweep in resolve()/
  findResumeParent(); lastSeen stays as the LRU/freshness tie-break
- importSnapshot() no longer drops aged entries (400-day-old chains
  reattach); malformed-entry defenses unchanged
- note for #1737: its normalizeForWrite TTL/256 re-application should
  be dropped on rebase; the union + deeper-depth-wins merge survives
AEmbers pushed a commit to AEmbers/billion-context that referenced this pull request Sep 30, 2026
…ianglei#1724)

Record in TECHNICAL-NOTES (en/zh-CN) that all instances share one state dir whose control plane is lane-aware but whose data plane is not partitioned — no per-session owner, no per-lane isolation. State the two consequences (cross-instance session visibility in every instance's web UI; the restart drain race and its ranxianglei#1737/ranxianglei#1742/ranxianglei#405 mitigations) and the current security posture: only the admin loopback gate + filesystem permissions, i.e. no per-session authorization. Sufficient single-user, insufficient multi-user (must partition). Issue direction ranxianglei#5. Docs-only.
@ranxianglei
ranxianglei deleted the 2026-09-30_multi-instance-store-hardening branch October 15, 2026 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant