Skip to content

fix: shrink withSessionLock to state-mutating sections (#970) - #972

Merged
ranxianglei merged 1 commit into
masterfrom
2026-09-19_shrink-session-lock
Sep 20, 2026
Merged

ranxianglei merged 1 commit into
masterfrom
2026-09-19_shrink-session-lock

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Closes #970

What changed

withSessionLock was held from prepare() through the entire upstream response stream (src/server.ts). Every concurrent request sharing a session id — Claude Code's main turn plus all background subagents, which do not carry distinct session ids — serialized behind one slow upstream stream, producing the 1-2+ minute first-token stalls in #970.

The lock now covers only the fast state-mutating sections:

  • Outer lock (src/server.ts): prepare + preflight only; returns {body, prepared} and releases before forward() runs.
  • Inside forward(), each discrete mutation re-acquires the lock just long enough:
    • markNativeCompactionBoundary (native compact completion)
    • refreshFolded (post-compress processTurn + session.state =)
    • JSON response rewrites (openai / responses / anthropic)
    • executeProxyTool in the streaming compress loop (src/loop/core.ts) and the Responses JSON path (src/compress-loop-responses.ts)
    • rememberPluginMessages after forward
  • Read-only proxy-tool executions are deliberately unlocked (they skip MUTATING_PROXY_TOOLS and run synchronously, hence atomically). The plugin-MCP tool path was already locked.
  • withSessionLock signature widened to accept sync callbacks (src/session.ts).

Net effect: a slow upstream delays only its own request; requests queued behind it proceed concurrently.

Why this shape

The lock exists to protect session.state from interleaved processTurn / stream-rewriter mutations. On Node's single-threaded runtime, only sections spanning an await/yield suspension point can interleave — exactly the sections locked above. Wire shape is unchanged: same outbound bodies, same streams, same injection points, reasoned in both compression modes (full suite + real-client E2E below).

Regression test

tests/session-lock-concurrency.test.ts: two concurrent Anthropic-wire requests on one session against a mock upstream holding its first byte for 400ms. Asserts the second upstream arrival lands within half the hold window (pre-fix code waited the full ~400ms), plus state integrity (message-ref ids never re-issued under concurrent prepares) and a follow-up sequential request. Verified to FAIL on pre-fix src/ and pass with the fix.

Pre-flight (rebased onto current master)

  • npm run typecheck — clean
  • npm test — 1900 pass / 0 fail / 2 skipped
  • npm run build — success
  • Real-client E2E through the built dist (tests/e2e/e2e-codex-fake.test.ts, deterministic fake Responses upstream): overflow scenario (compression really fires inside real codex, bulk folded, sentinels retained) + under-window control (no compression) — both pass.

Out of scope (remaining items from #970, need owner decision)

  • Separate subagent sessions (distinct session id per subagent conversation) — identity/session-binding territory.
  • Preserve upstream cache prefix (keep bili injections out of the stable prefix so subagent first requests stop landing at cache hit 0%).
  • Upstream flash first-byte latency itself (upstream queueing vs connection layer — the blind TCP tunnel churn noted in the issue).

This PR fixes the lock-chain mechanism that turned those slow first bytes into head-of-line blocking for every request on the session.

@github-actions

github-actions Bot commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

📦 Built Package Artifact

Branch: 2026-09-19_shrink-session-lock (16dac72)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-972

Each push to this PR publishes a new version under the pr-972 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr972.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

[bot] 🏷 Received — this is the follow-up to the #970 lock-scope fix (already triaged + implemented in-thread there). Verifying now: pulling the branch, reviewing the diff against current master, re-running the full pre-flight (typecheck / tests / build) locally, and checking CI. Will report the verdict shortly.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review complete — verified, CI fully green, ready to merge. This is the #970 lock-scope fix from my earlier session; per protocol I did not redo it but independently re-verified every claim.

Verification performed (all on head 9462eb5):

  1. Freshness — branch is up to date with current master (b6c2ef3); no stale base. Single commit, clean diff: 5 files, +175/−41, no version bump, no lockfile churn, no unrelated lines.
  2. Code review of the lock shape — walked every mutation point in forward():
    • Outer lock now covers prepare+preflight only and returns {body, prepared} (src/server.ts:1556-1684); forward() runs unlocked; the preflight-failure path correctly writes the error response then returns null (no dangling forward).
    • Re-locked sections confirmed in place: markNativeCompactionBoundary, refreshFolded (made async, whole body inside the lock so no suspension point sits mid-mutation), the three JSON rewrites, executeProxyTool at src/loop/core.ts:576 + src/compress-loop-responses.ts:216, and post-forward rememberPluginMessages.
    • The "read-only tools stay unlocked" claim checks out: that path (surfaceReadonlyJson, src/compress-loop-responses.ts:144-181) is pre-existing and runs synchronously — no await ⇒ atomic on Node. The new locks wrap everything conservatively, which is safe either way.
    • withSessionLock widening to sync callbacks (src/session.ts:297) keeps the try/finally release-on-throw structure intact.
    • Interleaving reasoning holds: the only read-modify-write spanning suspension points is refreshFolded's processTurn+state writeback, and it's now atomic under the lock; concurrent prepares dedupe by content-hash raw ids, so no ref re-issue window opens.
  3. Local pre-flight (re-ran, not trusting the PR text): npm run typecheck clean · npm test 1900 pass / 0 fail / 2 skipped · npm run build success.
  4. Regression test bites both ways — temporarily restored pre-fix src/ (keeping the new test): it FAILS exactly as designed (second request reached the upstream 451ms after the first — it queued behind …), and passes with the fix. Restored clean afterward.
  5. CI — all 8 check runs green on the head, including the real-codex-vs-deterministic-fake-upstream E2E (zero tokens). mergeable_state was blocked only while the two Windows test jobs were still running; both have since passed.

Verdict: scope is right (fixes the lock-chain mechanism, leaves the three out-of-scope items for your decision), both compression modes reasoned, evidence is real. No changes needed — merge when you're ready. (Merging itself stays with you: I can't merge PRs.)

中文摘要:独立复核了锁缩小方案(所有跨 await 的状态变更段均已加锁、只读工具路径为既有同步实现、回归测试在旧代码上确实失败/新代码上通过),本地 typecheck/全量测试(1900 通过)/构建全部复跑通过,CI 八项检查(含真实 codex E2E)全绿,无需修改,可以合并。

…requests on one session stop head-of-line-blocking (#970)
@ranxianglei
ranxianglei force-pushed the 2026-09-19_shrink-session-lock branch from 9462eb5 to 16dac72 Compare September 19, 2026 16:33
@ranxianglei
ranxianglei merged commit f57828e into master Sep 20, 2026
7 of 8 checks passed
@ranxianglei
ranxianglei deleted the 2026-09-19_shrink-session-lock 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.

Subagent first token stalls 1-2min: withSessionLock covers full forward + slow flash upstream first byte

1 participant