Skip to content

feat: bili daemon subcommand — per-session dynamic-port proxy (#518) - #523

Open
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-04_bili-daemon-subcommand
Open

feat: bili daemon subcommand — per-session dynamic-port proxy (#518)#523
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-04_bili-daemon-subcommand

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

What

New thin subcommand bili daemon for agent-side plugins to start an isolated proxy per session (native mode A from #513):

bili daemon --parent-pid <agent-pid>
# stdout (single line): {"origin":"http://127.0.0.1:<port>","port":<port>,"pid":<pid>,"logPath":"…"}
# exit 0 on success; non-zero + stderr explanation on failure

Design (per issue spec)

  • Reuses ensureProxyRunning — same launch-token handshake, health polling, and launcher 端口 TOCTOU(探测-释放-重bind 无重试):可静默挂上陌生 bili 或 20s 挂死;且被启动代理强制 --debug 制造 12MB/天日志 #407 EADDRINUSE retry chain; new fresh option skips the attach step so sessions never share an instance.
  • Dynamic port: exported allocateDynamicPort() does an ephemeral bind-0 probe; an explicit --port/ACP_PORT is still preferred first, with automatic fallback on conflict (findFreePort now reuses it).
  • Per-session result file: parent creates tmp/bili-daemon-<uuid>.json, passes it via BILI_RESULT_FILE; child writes it atomically after binding (alongside the usual global instance file, which stays last-writer-wins for MCP-shell/install discovery). Parent polls ONLY its own file — parallel daemons can't clobber each other, and the poll loop no longer needs any new dep surface.
  • Auto-reaping: passes BILI_PARENT_PID=<host pid> and reuses the existing Windows 上 launcher 子代理从无优雅退出:stopProxy 直接 TerminateProcess,SIGBREAK-flush 全史 0 次触发,防抖窗状态+进行中轮被拦腰 #414 watcher. Triple-state semantics: --parent-pid flag > inherited BILI_PARENT_PID env > no watcher at all (+ stderr warning). It deliberately does NOT default to the daemon CLI's own pid — the CLI exits right after printing JSON, which would suicide the proxy within one 2s tick.
  • Output contract: single-line JSON {origin, port, pid, logPath} on stdout, exit 0. Failure → stderr + exit 1. Uses process.exitCode (not process.exit) so stdout flushes before exit.
  • Server side: announceListening also writes the per-session file when BILI_RESULT_FILE is set; finishShutdown unlinks it with an ownership guard (only if the record's instanceId is ours), covering the case where the parent died before startup completed.

Files

  • src/launcher.tsLaunchOptions.{fresh,resultFile,parentPid}, allocateDynamicPort(), spawnDaemonProxy(), runDaemon()
  • src/server.ts — per-session result-file write/cleanup
  • src/cli.tsdaemon command + --parent-pid (validated positive integer, exit 2 otherwise)
  • tests/daemon.test.ts (new) — 8 unit tests, deps-injection style like the launcher tests: fresh-skips-attach vs control, env plumbing (all three parentPid states), resultFile-only polling (decoy global instance), timeout path, dynamic port allocation, runDaemon stdout/exitCode/tmpdir-cleanup, env fallback + no-parent warning
  • tests/e2e-daemon.test.ts (new) — real processes: spawn node --import tsx src/index.ts daemon --parent-pid <dummy>, parse the JSON line, hit /__bili/health, kill the dummy parent and assert the proxy self-exits within the watcher window (~2s observed)
  • tests/cli-parseargs.test.ts — parse/validation cases for the new command
  • README.md — short Per-session proxy (bili daemon) section

Pre-flight

  • npm run typecheck — clean
  • npm test — 958/959 pass. The single failure (resolveClientCommand: codex/claude resolve to themselves) is pre-existing and environment-specific: this sandbox has a real /usr/bin/codex on PATH, so PATH resolution returns the absolute path. Verified by running the same test on pristine master in a clean worktree — identical failure. Unrelated to this change.
  • npm run build — clean; smoke-tested the built artifact: node dist/index.js daemon --parent-pid <pid> → handshake JSON, health OK, proxy reaped ~2s after parent kill.

No version bump (content branch); no CHANGELOG entry (release territory).

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Review started. Pulling the branch (2026-09-04_bili-daemon-subcommand, head e515720), running pre-flight (typecheck / test / build), and verifying each claim in the description — including the per-session result-file race story, the parent-pid triple-state semantics, and the "pre-existing failure" claim. Will report findings here shortly.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 ework agent · qwen3.8-27b

[bot] 🏷 Review complete — LGTM. Implementation matches the spec point-for-point, and every pre-flight claim checked out under independent verification. Details below.

What I verified (not just trusted)

Pre-flight, re-run independently on the PR head:

  • npm run typecheck — clean
  • npm test — 958/959. The single failure is resolveClientCommand: codex/claude resolve to themselves (tests/launcher.test.ts:715). Confirmed pre-existing & environment-specific: I reproduced it on a pristine worktree at the merge base (09ef208) — identical failure. Root cause: that test calls resolveClientCommand("codex", { PATH: "/usr/bin" }) expecting the bare-name fallback, but resolveOnPath probes the real filesystem (fs.existsSync), and this environment happens to have genuine codex/claude binaries resolvable from /usr/bin. Neither the function nor the test is touched by this PR. Your claim stands.
  • npm run build — clean.

End-to-end smoke on the built artifact (node dist/index.js daemon --parent-pid <pid>):

  • stdout was exactly one line: {"origin":"http://127.0.0.1:34501","port":34501,"pid":…,"logPath":"/tmp/bili-proxy-34501.log"}, exit 0
  • post-print exit latency: 10 ms — the process.exitCode (not exit) choice works; nothing keeps the event loop alive (no auto-update timer on this path, logFd closed, child unref'd). No undici keep-alive hang.
  • /__bili/health{"ok":true,…}
  • killed the host pid → proxy self-exited after 1916 ms, matching the 2 s watcher tick
  • per-session tmp/bili-daemon-*.json cleaned up (verified zero leftovers across repeated full-suite runs)

Design assessment (why the approach holds):

  • The per-session result file solves a real concurrency defect: with N parallel daemons polling the single global instance file, last-writer-wins means daemon A's parent can read daemon B's token/origin. Polling only your own UUID-named file removes the coupling entirely, and keeping the global write preserves MCP-shell/install discovery. Correct layering — the fix lives at the handshake surface, not by adding locks.
  • fresh skipping probeExistingInstance is the right isolation guarantee (a per-session proxy must never attach to another session's writer over the shared sessions dir).
  • Triple-state parent-pid (flag > inherited env > none+warning, deliberately not defaulting to the CLI's own pid) is correct — the CLI exits milliseconds later, so defaulting would suicide every proxy within one 2 s tick. The unit test pins all three states.
  • Ownership-guarded cleanup on both sides (CLI unlink after handshake; finishShutdown unlink guarded by instanceId) is race-safe in both orderings.
  • findFreePort now delegating to the exported allocateDynamicPort keeps launcher behavior identical while sharing the probe — good dedup.
  • Two-compression-modes check (§6): the change is lifecycle-only (spawn/handshake/file cleanup); no wire rewrite, tool injection, acp_summary, preflight, or nudge paths touched → no pluginMode/proxyMode interaction.
  • Dup screening: no other issue/PR implements bili daemon. Related open PRs fix(launcher): isolate launches on a random free port; drop proxy attach (#446) #465/fix(launcher): eliminate port TOCTOU via child-reported handshake bind; stop forcing --debug (#401) #480/fix(launcher): fail fast when the spawned proxy child exits pre-bind #481 are distinct launcher-behavior work (fix(launcher): fail fast when the spawned proxy child exits pre-bind #481's own body notes most of fix(launcher): eliminate port TOCTOU via child-reported handshake bind; stop forcing --debug (#401) #480 already landed in master) — no scope collision here.
  • Compliance: no version bump, no acp-kernel change, no as any/@ts-ignore, no <acp> literals added. The ungated e2e-daemon.test.ts is fine to run in plain npm test — it's hermetic (local node processes, loopback only, ~4 s, no upstream/network), unlike the token-costing codex suite which stays gated.

Minor findings (all non-blocking, your call)

  1. DRY nitfinishShutdown's result-file cleanup (src/server.ts:475-480) re-implements what clearProxyInstanceFile(instanceId, file?) already does (same read → instanceId guard → unlink → swallow). Since the helper already takes an optional path, it could be clearProxyInstanceFile(instanceId, resultFile), dropping the two new imports (readProxyInstanceFile, isProxyInstanceFile) from server.ts.
  2. README typo — "Concurrency-safe: daemons handshakes through a per-session result file" → "daemon handshakes go through …" (subject/verb).
  3. Help-text indent drift — the diff re-indents several unrelated --help lines by one extra space (bili update, bili plugin install, the --passthrough/--no-* option rows). Purely cosmetic, but the help block ends up internally inconsistent.
  4. Theoretical hardeningBILI_RESULT_FILE/BILI_LAUNCH_TOKEN aren't in INHERITED_PROXY_VARS. Every realistic path is safe because both are always explicitly assigned (runDaemon always sets resultFile; launchToken is always freshly generated), but a non-daemon ensureProxyRunning call inside a process whose env carried a stale BILI_RESULT_FILE would leak it to the child. Cheap defense-in-depth: treat BILI_* vars as stripped-and-reassigned in the spawn env.
  5. Observation, pre-existing hazard class (not introduced here) — the poll-loop fallback (src/launcher.ts:1730-1738) trusts the preferred origin when the handshake record is missing/stale. With per-session files the record is absent until our child writes, so in a tight race where another bili proxy binds the very ephemeral port we just released, a parent could report a foreign origin as its own. Needs rare kernel port reuse plus sub-second timing, and the same class existed pre-PR via the global file. If you want the daemon contract airtight: skip the fallback entirely when opts.resultFile is set (that fallback exists for legacy/broken-state-dir cases the daemon path doesn't need).

None of these rise to a separate issue per the reporting bar — they're nits/theoretical, so reported here only.

Verdict: ready to merge from my side. If you'd like, I can push a follow-up commit addressing items 1–3 (one-line fixes each); otherwise ship as-is and handle them whenever.

@ranxianglei

Copy link
Copy Markdown
Owner Author

Review vs master 3cd85b6 (post v0.1.89, kernel 0.0.56): ⚠️ design-pending (#518) + drift found — needs rework before it can land.

Sound core (kept for the rework):

  • Per-session handshake file so daemon parents poll ONLY their own token — concurrent daemons never clobber. This composes cleanly with the post-fix: instance registry lost-update — per-instance marker files (#527) #528 marker-file instance registry (<state>/instances/<id>.json replaced the single instances.json); the handshake idea is if anything MORE natural there.
  • --fresh skipping probeExistingInstance (per-session isolation, no attach) and --parent-pid watchdog semantics are right.

Drift vs master:

  1. Duplicate port allocator: the PR adds allocateDynamicPort() — master already has pickEphemeralPort() (src/launcher.ts:1436, landed v0.1.84 via fix(launcher): spawn on an ephemeral port when no --port is given (#446) #526/sessions 存储三重膨胀:叶块 blockContents 逐字节重复 50%、零 GC、每次启动两遍全量同步解析(实测 387MB/+60MB 每天/2s 起步) #401) doing exactly this. Drop and reuse.
  2. Instance-layer drift: server.ts handshake writes were authored against the pre-fix: instance registry lost-update — per-instance marker files (#527) #528 single-instance-file world (atomicWriteInstanceFile(info, file?) now takes an optional file param; the registry is the directory listing). The auto-merge compiles but the write/read paths must be re-validated against registerInstanceAndWarn/marker files.
  3. src/server.ts has a real textual conflict (announce/instance region moved twice since).

Given #524 stacks on this, sequence: family decision (#513) → rework #523 → rebase #524 on it.

(tracking: #518 / family #513)

Thin launcher for agent-side plugins to start an isolated proxy per session:
always a fresh instance (never attaches), dynamic port via ephemeral bind-0
probe with EADDRINUSE fallback, per-session handshake through BILI_RESULT_FILE
so parallel daemons never clobber each other, and #414 parent-gone reaping via
BILI_PARENT_PID (triple-state: flag > env > no watcher + warning). Prints one
JSON line {origin,port,pid,logPath} to stdout on success; exit non-zero +
stderr on failure.

- launcher.ts: LaunchOptions +fresh/resultFile/parentPid, exported
  allocateDynamicPort, spawnDaemonProxy/runDaemon
- server.ts: announceListening also writes the per-session result file when
  BILI_RESULT_FILE is set; finishShutdown unlinks it (ownership-guarded)
- cli.ts: `daemon` command + --parent-pid validation (exit 2 on bad value)
- tests: unit (deps-injection), parseArgs cases, real-process integration
  incl. parent-gone reaping
@ranxianglei
ranxianglei force-pushed the 2026-09-04_bili-daemon-subcommand branch from e515720 to 51547cc Compare September 7, 2026 16:27
@ranxianglei

Copy link
Copy Markdown
Owner Author

Rebased onto master 5836f78 (was 237 commits behind, 2 conflicts in src/launcher.ts + src/server.ts, resolved keeping BOTH sides):

Pre-flight on rebased head 51547cc: typecheck ✓ · 1250/1252 tests (2 known env fails in plugin-agent.test.ts, unrelated; +12 daemon tests pass) · build ✓.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-09-04_bili-daemon-subcommand (51547cc)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-523

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

Option B — Download artifact

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

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Rebased head 51547cc re-verified — LGTM stands. Conflict resolutions audited line-by-line, pre-flight re-run independently, full e2e smoke re-done on the rebuilt artifact.

Conflict-resolution audit (the part that can actually break)

One new nit introduced by "keep both sides" (non-blocking): allocateDynamicPort (src/launcher.ts:1429, from this PR) and pickEphemeralPort (src/launcher.ts:1454, from master's #446) are now byte-identical duplicate implementations of the same bind-0 probe. No functional impact, but two exported names doing exactly the same thing will confuse future readers about which is canonical. Suggest collapsing to one (alias or re-point the 3 call sites) — happy to push that as a tiny follow-up commit if you want it in this PR.

Independent pre-flight on 51547cc

  • npm run typecheck ✓ · npm run build
  • npm test: 1251/1252 here — the single failure is the known env-specific resolveClientCommand: codex/claude resolve to themselves (same root cause as last round: real client binaries resolvable on PATH in this sandbox). Your second plugin-agent.test.ts failure did not reproduce in my environment — those tests are hermetic fake-proxy tests, so likely timing-sensitive rather than env-specific. Worth a rerun on your side to confirm it doesn't recur.
  • All 16 daemon-related tests pass (8 unit + e2e-daemon + 7 parseargs).

E2E smoke (rebuilt artifact, real process tree)

  • stdout exactly one line: {"origin":"http://127.0.0.1:36567","port":36567,"pid":…,"logPath":"/tmp/bili-proxy-36567.log"}, exit 0 at 320 ms, post-print latency 8 ms
  • /__bili/healthok:true; host killed → proxy self-exited 1903 ms later; zero tmp/bili-daemon-* leftovers
  • Fail-fast check: bili daemon --host <unresolvable> → exit 1 in 115 ms with clean stderr and empty stdout (machine contract preserved on failure too); the childExit loop composition verified by inspection plus master's fix(launcher): fail fast when the spawned proxy child exits pre-bind #481 unit tests passing in-suite

Verdict unchanged: ready to merge. Only open item is the optional allocateDynamicPort/pickEphemeralPort dedup — your call whether to fold it in or leave for a cleanup pass.

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