diff --git a/docs/developers/examples/daemon-client-quickstart.md b/docs/developers/examples/daemon-client-quickstart.md index a72069aca47..16d1dd864ce 100644 --- a/docs/developers/examples/daemon-client-quickstart.md +++ b/docs/developers/examples/daemon-client-quickstart.md @@ -9,9 +9,11 @@ In one terminal: ```bash cd your-project/ qwen serve --port 4170 -# → qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge) +# → qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge, workspace=/path/to/your-project) ``` +Per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02 each daemon binds to one workspace at boot (the current `cwd`, or override with `--workspace /path/to/dir`). The daemon's bound path is advertised on `/capabilities.workspaceCwd` so clients can pre-flight check + omit `cwd` from `POST /session`. + In another: ```bash @@ -28,13 +30,23 @@ const client = new DaemonClient({ // token: process.env.QWEN_SERVER_TOKEN, // required for non-loopback binds }); -// 1. Confirm we can reach the daemon and gate UI on its features. +// 1. Confirm we can reach the daemon, gate UI on its features, and +// read back the daemon's bound workspace (#3803 §02). const caps = await client.capabilities(); console.log('Daemon features:', caps.features); - -// 2. Spawn-or-attach a session for the current workspace. +console.log('Daemon workspace:', caps.workspaceCwd); // canonical bound path + +// 2. Spawn-or-attach a session. Two equally-valid shapes: +// (a) pass `workspaceCwd: caps.workspaceCwd` to be explicit, or +// (b) omit `workspaceCwd` entirely — the SDK then sends no `cwd` +// field and the daemon route falls back to its bound +// workspace. The (b) shape is concise but assumes you trust +// `caps.workspaceCwd` to be whatever you intended. +// A non-empty `workspaceCwd` that doesn't canonicalize to the +// daemon's bound path yields `400 workspace_mismatch` (see +// "Workspace mismatch" below). const session = await client.createOrAttachSession({ - workspaceCwd: process.cwd(), + workspaceCwd: caps.workspaceCwd, }); console.log(`session=${session.sessionId} attached=${session.attached}`); @@ -139,9 +151,12 @@ case 'permission_request': { ## Shared-session collaboration -Two clients pointed at the same daemon and `cwd` end up on the same session: +Two clients pointed at the **same daemon** end up on the same session. Per #3803 §02 each daemon is bound to ONE workspace at boot, so the daemon launched as `qwen serve --workspace /work/repo` (or `cd /work/repo && qwen serve`) is what both clients connect to: ```ts +// Daemon was launched as `qwen serve --workspace /work/repo` so +// `caps.workspaceCwd === '/work/repo'` for both clients. + // Client A (e.g. an IDE plugin) const a = await clientA.createOrAttachSession({ workspaceCwd: '/work/repo' }); console.log(a.attached); // false — A spawned the agent @@ -154,6 +169,35 @@ console.log(a.sessionId === b.sessionId); // true Both clients see the same `session_update` / `permission_request` stream. Either can send a prompt; they FIFO-queue per the agent's "one active prompt per session" guarantee. +## Workspace mismatch + +If `workspaceCwd` doesn't match the daemon's bound workspace, `createOrAttachSession` rejects with `DaemonHttpError` carrying status `400` and a structured body: + +```ts +import { DaemonHttpError } from '@qwen-code/sdk'; + +try { + await client.createOrAttachSession({ workspaceCwd: '/some/other/project' }); +} catch (err) { + if (err instanceof DaemonHttpError && err.status === 400) { + const body = err.body as { + code?: string; + boundWorkspace?: string; + requestedWorkspace?: string; + }; + if (body.code === 'workspace_mismatch') { + console.error( + `This daemon is bound to ${body.boundWorkspace}, ` + + `not ${body.requestedWorkspace}. Start a separate daemon ` + + `for that workspace, or route to the right one.`, + ); + } + } +} +``` + +Multi-workspace deployments run one daemon per workspace on separate ports — there's no intra-daemon routing under §02. An orchestrator (or the user's launcher) picks the right daemon based on the project the client wants to talk to. + ## Authentication When the daemon was started with a token (any non-loopback bind requires one): diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 4b73183e169..74224ab1cfc 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -42,6 +42,19 @@ with status `400`. with status `404`. +`WorkspaceMismatchError` for a `POST /session` whose `cwd` doesn't canonicalize to the daemon's bound workspace (#3803 §02 — 1 daemon = 1 workspace) returns `400` with: + +```json +{ + "error": "Workspace mismatch: daemon is bound to \"…\" but request asked for \"…\". …", + "code": "workspace_mismatch", + "boundWorkspace": "/path/the/daemon/binds", + "requestedWorkspace": "/path/in/the/request" +} +``` + +Use this to detect mismatch pre-flight: read `workspaceCwd` off `/capabilities` and omit `cwd` from `POST /session` (it falls back to the bound workspace), or route the request to a daemon bound to `requestedWorkspace`. + `POST /session` past the daemon's `--max-sessions` cap returns `503` with a `Retry-After: 5` header and: ```json @@ -93,7 +106,8 @@ Pass `?deep=1` (also accepts `?deep=true` or bare `?deep`) for a probe that expo "v": 1, "mode": "http-bridge", "features": ["health", "capabilities", "..."], - "modelServices": [] + "modelServices": [], + "workspaceCwd": "/canonical/path/to/workspace" } ``` @@ -101,6 +115,8 @@ Stable contract: when `v` increments the frame layout has changed in a backwards > **`modelServices` is always `[]` in Stage 1.** The agent uses its single default model service and doesn't enumerate it over the wire. Stage 2 will populate this from registered model adapters so SDK clients can build service-pickers; until then, do NOT rely on this field being non-empty. +> **`workspaceCwd`** is the canonical absolute path this daemon binds to (#3803 §02 — 1 daemon = 1 workspace). Use it to (a) detect mismatch before posting `/session` and (b) omit `cwd` on `POST /session` (the route falls back to this path). Multi-workspace deployments expose multiple daemons on different ports, each with its own `workspaceCwd`. Additive to v=1: pre-§02 v=1 daemons omit the field — clients that target older builds should null-check before consuming it. + ### `POST /session` Spawn a new agent or attach to an existing one (under `sessionScope: 'single'`, the default). @@ -116,7 +132,7 @@ Request: | Field | Required | Notes | | ---------------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `cwd` | yes | Absolute path. Relative paths return `400`. Workspace paths are canonicalized via `realpathSync.native` (with a resolve-only fallback for non-existent paths) so case-insensitive filesystems don't fork sessions per spelling. | +| `cwd` | no | Absolute path matching the daemon's bound workspace. If omitted, the route falls back to `boundWorkspace` (read it off `/capabilities.workspaceCwd`). A mismatched non-empty `cwd` returns `400 workspace_mismatch` (#3803 §02 — 1 daemon = 1 workspace). Workspace paths are canonicalized via `realpathSync.native` (with a resolve-only fallback for non-existent paths) so case-insensitive filesystems don't reject sessions per spelling. | | `modelServiceId` | no | Selects which configured _model service_ the agent will route through (the back-end provider — Alibaba ModelStudio, OpenRouter, etc). If omitted the agent uses its default. If the workspace already has a session, this calls `setSessionModel` on the existing one and broadcasts `model_switched`. Distinct from `modelId` on `POST /session/:id/model`, which selects the model **within** an already-bound service. The `modelServices` array on `/capabilities` is reserved for advertising configured services; in Stage 1 it is always `[]` (the agent's default service is used and not enumerated over HTTP). | Response: diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 66c1b8acc21..1c7ef153218 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -8,9 +8,10 @@ Run Qwen Code as a local HTTP daemon so multiple clients (IDE plugins, web UIs, ## What it gives you -- **One agent process, many clients** — under the default `sessionScope: 'single'`, every client connecting to the same workspace shares one ACP session. Live cross-client collaboration on the same conversation, the same file diffs, the same permission prompts. +- **One agent process, many clients** — under the default `sessionScope: 'single'`, every client connecting to the daemon shares one ACP session. Live cross-client collaboration on the same conversation, the same file diffs, the same permission prompts. - **Reconnect-safe streaming** — SSE with `Last-Event-ID` reconnect lets a client drop and pick up exactly where it left off (within the ring's replay window). - **First-responder permissions** — when the agent asks for permission to run a tool, every connected client sees the request; whichever client answers first wins. +- **One daemon, one workspace** — each `qwen serve` process binds to exactly one workspace at boot (per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02). Multi-workspace deployments run one daemon per workspace on separate ports (or behind an orchestrator). ## Quickstart @@ -19,11 +20,11 @@ Run Qwen Code as a local HTTP daemon so multiple clients (IDE plugins, web UIs, ```bash cd your-project/ qwen serve -# → qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge) +# → qwen serve listening on http://127.0.0.1:4170 (mode=http-bridge, workspace=/path/to/your-project) # → qwen serve: bearer auth disabled (loopback default). Set QWEN_SERVER_TOKEN to enable. ``` -The default bind is `127.0.0.1:4170`. Bearer auth is **off** on loopback so local development "just works". +The default bind is `127.0.0.1:4170`. Bearer auth is **off** on loopback so local development "just works". The daemon binds to the current working directory; use `--workspace /path/to/dir` to override. ### 2. Sanity-check it @@ -32,19 +33,23 @@ curl http://127.0.0.1:4170/health # → {"status":"ok"} curl http://127.0.0.1:4170/capabilities -# → {"v":1,"mode":"http-bridge","features":["health","capabilities","session_create",...]} +# → {"v":1,"mode":"http-bridge","features":["health","capabilities","session_create",...],"workspaceCwd":"/path/to/your-project"} ``` +The `workspaceCwd` field surfaces the bound workspace so clients can pre-flight check + omit `cwd` on `POST /session`. + ### 3. Open a session ```bash curl -X POST http://127.0.0.1:4170/session \ -H 'Content-Type: application/json' \ - -d '{"cwd":"'"$PWD"'"}' + -d '{}' # → {"sessionId":"","workspaceCwd":"…","attached":false} ``` -A second client posting to `/session` with the same `cwd` gets `"attached": true` — they're now sharing the agent. +`cwd` may be omitted — the route falls back to the daemon's bound workspace. Posting a `cwd` that doesn't match the bound workspace returns `400 workspace_mismatch` (the daemon is bound to exactly one workspace; start a separate daemon for a different one). + +A second client posting to `/session` (any matching `cwd` or none) gets `"attached": true` — they're now sharing the agent. ### 4. Subscribe to the event stream (in another terminal first) @@ -94,7 +99,7 @@ Clients then send `Authorization: Bearer $QWEN_SERVER_TOKEN` on every request. ` ```bash curl -H "Authorization: Bearer $QWEN_SERVER_TOKEN" http://your-host:4170/capabilities -# → {"v":1,"mode":"http-bridge","features":[...],"modelServices":[]} +# → {"v":1,"mode":"http-bridge","features":[...],"modelServices":[],"workspaceCwd":"/path/to/your-project"} # Wrong token → 401 ``` @@ -102,14 +107,15 @@ The token comparison is constant-time (SHA-256 + `crypto.timingSafeEqual`); 401 ## CLI flags -| Flag | Default | Purpose | -| ----------------------- | ----------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `--port ` | `4170` | TCP port. `0` = OS-assigned ephemeral port. | -| `--hostname ` | `127.0.0.1` | Bind interface. Anything beyond loopback requires a token. | -| `--token ` | — | Bearer token. Falls back to `QWEN_SERVER_TOKEN` env var (with leading/trailing whitespace stripped — handy for `$(cat token.txt)`). | -| `--max-sessions ` | `20` | Cap on concurrent live sessions. New `POST /session` requests that would spawn a fresh child return `503` (with `Retry-After: 5`) when the cap is hit; attaches to existing sessions are NOT counted. Set to `0` to disable. Sized for single-user / small-team usage; raise it if your deployment has the RAM/FD headroom (~30–50 MB per session). | -| `--max-connections ` | `256` | Listener-level TCP connection cap (`server.maxConnections`). Bounds raw socket count irrespective of session count — slow / phantom SSE clients get rejected at accept time once full. Raise alongside `--max-sessions` if your deployment expects many SSE subscribers per session. | -| `--http-bridge` | `true` | Stage 1 mode: per-session `qwen --acp` child process. Stage 2 native in-process becomes available later. | +| Flag | Default | Purpose | +| ----------------------- | --------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `--port ` | `4170` | TCP port. `0` = OS-assigned ephemeral port. | +| `--hostname ` | `127.0.0.1` | Bind interface. Anything beyond loopback requires a token. | +| `--token ` | — | Bearer token. Falls back to `QWEN_SERVER_TOKEN` env var (with leading/trailing whitespace stripped — handy for `$(cat token.txt)`). | +| `--max-sessions ` | `20` | Cap on concurrent live sessions. New `POST /session` requests that would spawn a fresh child return `503` (with `Retry-After: 5`) when the cap is hit; attaches to existing sessions are NOT counted. Set to `0` to disable. Sized for single-user / small-team usage; raise it if your deployment has the RAM/FD headroom (~30–50 MB per session). | +| `--workspace ` | `process.cwd()` | Absolute workspace path this daemon binds to (per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02 — 1 daemon = 1 workspace). `POST /session` requests with a mismatched `cwd` return `400 workspace_mismatch`. For multi-workspace deployments, run one `qwen serve` per workspace on separate ports. | +| `--max-connections ` | `256` | Listener-level TCP connection cap (`server.maxConnections`). Bounds raw socket count irrespective of session count — slow / phantom SSE clients get rejected at accept time once full. Raise alongside `--max-sessions` if your deployment expects many SSE subscribers per session. | +| `--http-bridge` | `true` | Stage 1 mode: one `qwen --acp` child per daemon (bound to one workspace at boot, per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02); N sessions multiplex onto that child via ACP `newSession()`. Stage 2 native in-process becomes available later. | > **Sizing the load knobs.** `--max-sessions` is the **new-child** cap. > Three other layers also limit load — when sizing for a high-concurrency @@ -157,13 +163,15 @@ The token comparison is constant-time (SHA-256 + `crypto.timingSafeEqual`); 401 > swallow RSTs may want to lower `server.keepAliveTimeout` via a > reverse proxy or accept periodic daemon restarts. -## Multi-session & remote deployment +## Multi-session & multi-workspace deployment + +Per [#3803](https://github.com/QwenLM/qwen-code/issues/3803) §02, each `qwen serve` process binds to **one workspace** at boot. Within that workspace it multiplexes N sessions onto a single `qwen --acp` child via the agent's native session map — sessions share the child's process / OAuth state / file-read cache / hierarchy-memory parse. -A single `qwen serve` process can manage sessions for any workspace path passed via `cwd` on `POST /session` — under the default `sessionScope: 'single'` it keeps one ACP session per canonicalized workspace, sharing it across every client that posts the same `cwd`. So one daemon will happily host sessions for many workspaces at once. +To host **multiple workspaces** (one user, several repos; or several users on the same host), run **multiple daemon processes** — one per workspace, each on its own port, supervised by systemd / docker-compose / k8s / a `qwen-coordinator` reference orchestrator. The trade-off is intentional: one workspace per child means `loadSettings(cwd)` / OAuth / MCP server scope stay aligned with the bound directory and don't drift across requests. > **Subscribe BEFORE posting `modelServiceId` on attach.** When a client `POST /session` with a `modelServiceId` and the workspace already has a session running a different model, the daemon issues an internal `setSessionModel` call — failures are NOT propagated as an HTTP error (the session stays operational on its current model). The visible failure signal is a `model_switch_failed` event on the session's SSE stream. If you call `POST /session` and only THEN open `GET /session/:id/events`, you'll miss the failure event and silently keep talking to the wrong model. Open the SSE stream first, or pass `Last-Event-ID: 0` on subscribe to replay the ring's oldest available event. -To handle multiple **users** (each with their own quota, audit log, sandbox) or to scale beyond one process's reach (cold-start budget, FD count, RSS), you spawn multiple daemon instances behind an external orchestrator. That orchestrator (multi-tenancy / OIDC / Quota / Audit / k8s) is **out of scope** for the qwen-code project — see issue [#3803](https://github.com/QwenLM/qwen-code/issues/3803) "External Reference Architecture" for the design pointers. +To handle multiple **users** (each with their own quota, audit log, sandbox) or to scale beyond one process's reach (cold-start budget, FD count, RSS), spawn one daemon per workspace per user behind an external orchestrator. That orchestrator (multi-tenancy / OIDC / Quota / Audit / k8s) is **out of scope** for the qwen-code project — see issue [#3803](https://github.com/QwenLM/qwen-code/issues/3803) "External Reference Architecture" for the design pointers. ## Durability model @@ -255,7 +263,7 @@ Concrete cost at N=5 sessions on the same workspace: | Auto-memory learned facts | shared | one knowledge base per child | | Cold start | first only | <200 ms after first session | -The bridge keeps **one channel per workspace** (cross-workspace sharing is intentionally not done — different workspaces have different settings/auth scope, and `acpAgent.ts:601` reloads settings per newSession `cwd`, which would interfere). The channel stays alive while at least one session is live; the last `killSession` (or a channel-level crash) kills the child. +The bridge keeps **one channel per daemon** (one daemon per workspace, per §02). The channel stays alive while at least one session is live; the last `killSession` (or a channel-level crash) kills the child. **MCP server children** are still per-session today — each session's config can specify different servers, so they're independently spawned. Stage 1.5 follow-up: refcount MCP server children by `(workspace, config-hash)` so identical configs share. Not in scope for this PR. diff --git a/integration-tests/cli/qwen-serve-routes.test.ts b/integration-tests/cli/qwen-serve-routes.test.ts index 295cdb1c7bd..bef03a6e6c0 100644 --- a/integration-tests/cli/qwen-serve-routes.test.ts +++ b/integration-tests/cli/qwen-serve-routes.test.ts @@ -20,6 +20,7 @@ * in `qwen-serve-streaming.test.ts` and skip when no auth is set. */ import { spawn, type ChildProcess } from 'node:child_process'; +import { realpathSync } from 'node:fs'; import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; @@ -60,6 +61,14 @@ beforeAll(async () => { TOKEN, '--hostname', '127.0.0.1', + // Per #3803 §02 (1 daemon = 1 workspace), pin the bound + // workspace so test assertions that POST `workspaceCwd: + // REPO_ROOT` succeed regardless of where the test runner + // happens to be cwd'd. Without this the daemon would inherit + // the test runner's cwd, which is brittle across CI / local + // / IDE-launcher environments. + '--workspace', + REPO_ROOT, ], { stdio: ['ignore', 'pipe', 'pipe'] }, ); @@ -221,20 +230,82 @@ describe('qwen serve — POST /session validation + concurrent coalescing', () = // Tearing the session down on model-switch failure would force // the caller into a 500 with no way to recover. The // `model_switch_failed` SSE event is the visible failure signal. - const cwd = '/tmp'; + // + // Use REPO_ROOT (the daemon's bound workspace) — under #3803 §02 + // any other cwd would return 400 workspace_mismatch before the + // session is even spawned. + const cwd = REPO_ROOT; const session = await client.createOrAttachSession({ workspaceCwd: cwd, modelServiceId: 'definitely-not-a-real-model', }); expect(session.sessionId).toBeTypeOf('string'); - expect(session.attached).toBe(false); + // `attached` may be true or false depending on whether earlier + // tests in this file already created a REPO_ROOT session. The + // shape of the response is what matters here (sessionId present, + // listWorkspaceSessions sees it). + expect(typeof session.attached).toBe('boolean'); const sessions = await client.listWorkspaceSessions(cwd); - expect(sessions).toHaveLength(1); - expect(sessions[0]?.sessionId).toBe(session.sessionId); + expect(sessions.some((s) => s.sessionId === session.sessionId)).toBe(true); // No teardown — Stage 1 has no DELETE /session route, and the - // session persists in `byId` until daemon shutdown. The other - // tests in this file use unique workspace cwds so the surviving - // session here doesn't interfere. + // session persists in `byId` until daemon shutdown. + }); + + it('rejects cross-workspace cwd with 400 workspace_mismatch (#3803 §02)', async () => { + // The daemon is bound to REPO_ROOT (via `--workspace` in beforeAll). + // A POST /session with `cwd: '/tmp'` (or any other absolute path + // that doesn't canonicalize to REPO_ROOT) must reject with 400 + // `workspace_mismatch`, carrying both paths in the body so an + // orchestrator-aware client can spawn / route to the right + // daemon. + const res = await fetch(`${base}/session`, { + method: 'POST', + headers: { + Authorization: `Bearer ${TOKEN}`, + 'content-type': 'application/json', + }, + body: JSON.stringify({ cwd: '/tmp' }), + }); + expect(res.status).toBe(400); + const body = (await res.json()) as { + code?: string; + boundWorkspace?: string; + requestedWorkspace?: string; + }; + expect(body.code).toBe('workspace_mismatch'); + expect(body.boundWorkspace).toBe(REPO_ROOT); + // The bridge canonicalizes the requested cwd via `realpathSync.native` + // so the response carries the on-disk canonical form, NOT the literal + // we POSTed. On macOS `/tmp` is a symlink to `/private/tmp`, so the + // hardcoded `/tmp` literal would diverge there. Resolve the same way + // the bridge does to keep the assertion portable. + expect(body.requestedWorkspace).toBe(realpathSync.native('/tmp')); + }); + + it('omits cwd → falls back to bound workspace (#3803 §02)', async () => { + // The route accepts an empty body and falls back to the daemon's + // bound workspace. Asserting this end-to-end through a real + // daemon process verifies the runQwenServe → createServeApp → + // bridge plumbing for the fallback path. + const res = await fetch(`${base}/session`, { + method: 'POST', + headers: { + Authorization: `Bearer ${TOKEN}`, + 'content-type': 'application/json', + }, + body: JSON.stringify({}), + }); + expect(res.status).toBe(200); + const session = (await res.json()) as { + sessionId?: string; + workspaceCwd?: string; + }; + expect(session.workspaceCwd).toBe(REPO_ROOT); + }); + + it('GET /capabilities surfaces workspaceCwd (#3803 §02)', async () => { + const caps = await client.capabilities(); + expect(caps.workspaceCwd).toBe(REPO_ROOT); }); }); diff --git a/integration-tests/cli/qwen-serve-streaming.test.ts b/integration-tests/cli/qwen-serve-streaming.test.ts index 08369bc2309..afc25f6d9dc 100644 --- a/integration-tests/cli/qwen-serve-streaming.test.ts +++ b/integration-tests/cli/qwen-serve-streaming.test.ts @@ -71,6 +71,17 @@ beforeAll(async () => { TOKEN, '--hostname', '127.0.0.1', + // Per #3803 §02 (1 daemon = 1 workspace), pin the bound + // workspace so every `createOrAttachSession({ workspaceCwd: + // REPO_ROOT })` below matches. Without this the daemon inherits + // the test runner's cwd (CI / IDE-launcher / direct vitest + // invocations all differ) and every session create returns + // 400 workspace_mismatch — the SSE / permission / Last-Event-ID + // tests below would all silently 404 once `SKIP_LLM_TESTS` is + // unset. Same fix the sibling routes test received earlier in + // this PR — missed in this file in the original §02 pass. + '--workspace', + REPO_ROOT, ], { stdio: ['ignore', 'pipe', 'pipe'] }, ); diff --git a/integration-tests/tsconfig.json b/integration-tests/tsconfig.json index 0cd24f82f59..8e94434c0c3 100644 --- a/integration-tests/tsconfig.json +++ b/integration-tests/tsconfig.json @@ -5,7 +5,8 @@ "allowJs": true, "baseUrl": ".", "paths": { - "@qwen-code/sdk": ["../packages/sdk-typescript/dist/index.d.ts"] + "//": "Resolve types from SDK source rather than dist so `tsc -p` here does not require a fresh `npm run build` of the SDK package before checking integration tests. The runtime vitest alias (`integration-tests/vitest.config.ts`) still points at the built `dist/index.mjs` to exercise the published-bundle shape; this paths entry only affects type resolution.", + "@qwen-code/sdk": ["../packages/sdk-typescript/src/index.ts"] } }, "include": ["**/*.ts"], diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 6240f970907..20262145cce 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -30,6 +30,7 @@ interface ServeArgs { token?: string; 'max-sessions': number; 'max-connections': number; + workspace?: string; // Read from the kebab-case key only — the camelCase mirror that yargs // synthesizes is convenient for handlers but type-confusing here. The // handler reads `argv['http-bridge']` directly. @@ -66,6 +67,15 @@ export const serveCommand: CommandModule = { 'Cap on concurrent live sessions. New spawn requests beyond this return 503; ' + 'attach to existing sessions still works. Set to 0 to disable.', }) + .option('workspace', { + type: 'string', + description: + 'Absolute workspace path this daemon binds to. ' + + 'POST /session requests with a mismatched cwd return 400 workspace_mismatch. ' + + 'Defaults to process.cwd() when omitted. ' + + 'For multi-workspace deployments, run one `qwen serve` per workspace ' + + 'on separate ports (or behind an external orchestrator).', + }) .option('max-connections', { type: 'number', default: 256, @@ -78,10 +88,10 @@ export const serveCommand: CommandModule = { type: 'boolean', default: true, description: - 'Stage 1 mode: one `qwen --acp` child per workspace behind the HTTP routes, ' + - "with multiple sessions multiplexed onto each child via the agent's native " + - '`newSession()`. Stage 2 native in-process mode is not yet implemented; ' + - 'this flag will become opt-in then.', + 'Stage 1 mode: one `qwen --acp` child per daemon (the daemon binds to ' + + 'one workspace at boot, multiplexing N sessions onto that child via ' + + "the agent's native `newSession()`). Stage 2 native in-process mode " + + 'is not yet implemented; this flag will become opt-in then.', }) as unknown as Argv, handler: async (argv) => { if (!argv['http-bridge']) { @@ -112,6 +122,7 @@ export const serveCommand: CommandModule = { mode: 'http-bridge', maxSessions: argv['max-sessions'], maxConnections: argv['max-connections'], + workspace: argv.workspace, }); } catch (err) { writeStderrLine( diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index 51ba8b0fe35..6036137567e 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -35,9 +35,13 @@ import type { import { createHttpAcpBridge, InvalidPermissionOptionError, + MAX_WORKSPACE_PATH_LENGTH, SessionNotFoundError, + WorkspaceMismatchError, type AcpChannel, + type BridgeOptions, type ChannelFactory, + type HttpAcpBridge, } from './httpAcpBridge.js'; import type { BridgeEvent } from './eventBus.js'; @@ -51,6 +55,17 @@ const WS_A = path.resolve(path.sep, 'work', 'a'); const WS_B = path.resolve(path.sep, 'work', 'b'); const SESS_A = `sess:${WS_A}`; +/** + * Convenience wrapper: `createHttpAcpBridge` now requires `boundWorkspace` + * (per #3803 §02 — 1 daemon = 1 workspace). Tests that only ever talk to + * `WS_A` would otherwise repeat `boundWorkspace: WS_A` everywhere; this + * helper defaults it. Tests that need a different bind path (e.g. the + * mismatch test) pass `boundWorkspace` explicitly. + */ +function makeBridge(opts: Partial = {}): HttpAcpBridge { + return createHttpAcpBridge({ boundWorkspace: WS_A, ...opts }); +} + interface FakeAgentOpts { /** What the fake agent returns from `newSession`. */ sessionIdPrefix?: string; @@ -66,6 +81,16 @@ interface FakeAgentOpts { p: PromptRequest, self: FakeAgent, ) => Promise | PromptResponse; + /** + * Custom `newSession` handler. Default returns a synthesized id (see + * `newSession` below). Used by tests that need to exercise the + * doSpawn newSession-failure path (e.g. throwing to cover the + * `isDying`-mark-then-kill cleanup). + */ + newSessionImpl?: ( + p: NewSessionRequest, + self: FakeAgent, + ) => Promise | NewSessionResponse; } class FakeAgent implements Agent { @@ -89,6 +114,9 @@ class FakeAgent implements Agent { async newSession(p: NewSessionRequest): Promise { this.newSessionCalls.push(p); + if (this.opts.newSessionImpl) { + return this.opts.newSessionImpl(p, this); + } const prefix = this.opts.sessionIdPrefix ?? 'sess'; // Stage 1.5 multi-session: one FakeAgent can host multiple // sessions (same as the real ACP agent), so each newSession call @@ -211,7 +239,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); expect(session.sessionId).toBe(SESS_A); @@ -232,7 +260,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const first = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const second = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); @@ -246,27 +274,74 @@ describe('createHttpAcpBridge', () => { await bridge.shutdown(); }); - it('does NOT reuse across workspaces', async () => { + it('rejects cross-workspace requests with WorkspaceMismatchError (#3803 §02)', async () => { + // Per #3803 §02 (1 daemon = 1 workspace), `spawnOrAttach` calls + // whose canonical `workspaceCwd` doesn't match `boundWorkspace` + // throw `WorkspaceMismatchError`. The server route translates + // this to a 400 with `code: 'workspace_mismatch'` so clients can + // route to (or spawn) a daemon for the other workspace. const handles: ChannelHandle[] = []; const factory: ChannelFactory = async () => { const h = makeChannel(); handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const a = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - const b = await bridge.spawnOrAttach({ workspaceCwd: WS_B }); - - expect(a.sessionId).not.toBe(b.sessionId); - expect(a.attached).toBe(false); - expect(b.attached).toBe(false); - expect(handles).toHaveLength(2); - expect(bridge.sessionCount).toBe(2); + expect(a.sessionId).toBe(SESS_A); + + // Cross-workspace POST throws before touching the channel. + // Single `.catch` capture — assert instance + carried fields off + // the same caught value rather than firing the rejection twice. + const err = await bridge + .spawnOrAttach({ workspaceCwd: WS_B }) + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(WorkspaceMismatchError); + expect((err as WorkspaceMismatchError).bound).toBe(WS_A); + expect((err as WorkspaceMismatchError).requested).toBe(WS_B); + + // Only the original WS_A spawn succeeded — no channel spawned for WS_B. + expect(handles).toHaveLength(1); + expect(bridge.sessionCount).toBe(1); await bridge.shutdown(); }); + it('WorkspaceMismatchError truncates oversized `requested` to MAX_WORKSPACE_PATH_LENGTH (defense-in-depth)', () => { + // The route-level cap in `server.ts` rejects oversized `cwd` + // bodies before reaching the bridge, but `WorkspaceMismatchError` + // can be constructed directly by other callers (tests, embeds, + // future entry points) or by passing pre-validated paths that + // somehow grew. The constructor interpolates `requested` into + // `.message` twice + downstream code echoes it on stderr + + // `res.json` — without truncation a 10 MB string amplifies + // ~6× per request. The truncation here is the cross-caller + // belt-and-suspenders defense. + const oversized = '/' + 'a'.repeat(MAX_WORKSPACE_PATH_LENGTH * 2); + const err = new WorkspaceMismatchError('/work/bound', oversized); + expect(err.requested.length).toBeLessThanOrEqual( + MAX_WORKSPACE_PATH_LENGTH + 32, // truncation marker overhead + ); + expect(err.requested.endsWith('…[truncated]')).toBe(true); + // `.message` interpolates `requested` twice; both go through the + // truncated form, so the message is bounded too. + expect(err.message.length).toBeLessThan( + MAX_WORKSPACE_PATH_LENGTH * 2 + 1024, + ); + // Bound is operator-controlled — not truncated. + expect(err.bound).toBe('/work/bound'); + }); + + it('WorkspaceMismatchError passes through `requested` shorter than MAX_WORKSPACE_PATH_LENGTH untouched', () => { + // Common case: legitimate `requested` paths (PATH_MAX is 4096 on + // Linux, 1024 on macOS) should not be modified. + const normal = '/work/different'; + const err = new WorkspaceMismatchError('/work/bound', normal); + expect(err.requested).toBe(normal); + expect(err.requested.endsWith('…[truncated]')).toBe(false); + }); + it('creates fresh session per call under sessionScope:thread (Stage 1.5 multi-session: shares channel)', async () => { const handles: ChannelHandle[] = []; const factory: ChannelFactory = async () => { @@ -274,7 +349,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ sessionScope: 'thread', channelFactory: factory, }); @@ -296,7 +371,7 @@ describe('createHttpAcpBridge', () => { }); it('rejects relative workspace paths', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -313,7 +388,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const a = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const aNoisy = await bridge.spawnOrAttach({ workspaceCwd: '/work/./a' }); @@ -334,7 +409,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); // ACP SDK rewrites unhandled exceptions to a JSON-RPC Internal error // object (code -32603); the original message text is intentionally not @@ -355,7 +430,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, initializeTimeoutMs: 50, }); @@ -367,59 +442,64 @@ describe('createHttpAcpBridge', () => { expect(bridge.sessionCount).toBe(0); }); - it('shutdown kills every live channel', async () => { + it('shutdown kills the live channel and its multiplexed sessions', async () => { + // Stage 1.5 multi-session under single-workspace mode (#3803 §02): + // a daemon hosts one channel with N sessions multiplexed on it. + // Shutdown kills that one channel and tears down every multiplexed + // session. const handles: ChannelHandle[] = []; const factory: ChannelFactory = async () => { - const h = makeChannel(); + const h = makeChannel({ sessionIdPrefix: 's' }); handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ + sessionScope: 'thread', + channelFactory: factory, + }); await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await bridge.spawnOrAttach({ workspaceCwd: WS_B }); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); expect(bridge.sessionCount).toBe(2); + expect(handles).toHaveLength(1); // one channel multiplexing two sessions await bridge.shutdown(); - expect(handles.every((h) => h.killed)).toBe(true); + expect(handles[0]?.killed).toBe(true); expect(bridge.sessionCount).toBe(0); }); - it('killAllSync force-kills channels even after shutdown cleared byWorkspaceChannel (BkUyD)', async () => { - // tanzhenxin BkUyD regression: shutdown clears - // `byWorkspaceChannel` BEFORE awaiting per-child SIGTERM grace. - // If the operator double-Ctrl+C's during that window, - // killAllSync MUST still see the in-flight-being-killed - // channels. Pre-fix: killAllSync iterated `byWorkspaceChannel` - // and silently no-op'd; children orphaned. Fix: separate - // `liveChannels` set, only emptied on channel.exited. - const killSyncInvoked: number[] = []; - let nextChannelTag = 0; + it('killAllSync force-kills the live channel mid-shutdown (BkUyD)', async () => { + // tanzhenxin BkUyD regression: pre-fix, `shutdown()` cleared the + // live-channel reference BEFORE awaiting the child's SIGTERM + // grace. A mid-drain double-Ctrl+C invoked `killAllSync`, found + // nothing to force-kill, and `process.exit(1)` orphaned the + // child. Under #3803 §02 the bridge has at most one channel, but + // the invariant is the same: `channelInfo` MUST stay set until + // `channel.exited` fires (OS-level reap), not be eagerly cleared + // by `shutdown()`. + const killSyncInvoked: string[] = []; const factory: ChannelFactory = async () => { - const tag = nextChannelTag++; - const h = makeChannel({ sessionIdPrefix: `s${tag}` }); + const h = makeChannel({ sessionIdPrefix: 's' }); const realKillSync = h.channel.killSync; - // Spy on killSync calls so we can assert the force-kill path - // actually fired for every live channel. h.channel = { ...h.channel, kill: () => // Never resolve — simulates a stuck SIGTERM grace window. new Promise(() => {}), killSync: () => { - killSyncInvoked.push(tag); + killSyncInvoked.push('called'); realKillSync(); }, }; return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await bridge.spawnOrAttach({ workspaceCwd: WS_B }); // Kick off shutdown — its `channel.kill()` will hang on the - // never-resolving Promise above, so `byWorkspaceChannel` clears - // but the awaits never finish. This is the mid-drain state. + // never-resolving Promise above, so the entry maps clear but + // the channel-kill await never finishes. This is the mid-drain + // state. const shutdownPromise = bridge.shutdown(); // Yield twice so shutdown's sync prefix runs (clear maps, // publish session_died, start awaits). @@ -429,9 +509,10 @@ describe('createHttpAcpBridge', () => { // Operator double-Ctrl+C arrives now. bridge.killAllSync(); - // Both channels' killSync was invoked. Pre-fix this would have - // been an empty array. - expect(killSyncInvoked).toHaveLength(2); + // The channel's killSync fired. Pre-fix this would have been an + // empty array because `channelInfo` was cleared in shutdown's + // sync prefix. + expect(killSyncInvoked).toHaveLength(1); // Cleanup: the never-resolving kill keeps shutdownPromise // pending forever. Don't await it (would hang the test). The @@ -439,6 +520,326 @@ describe('createHttpAcpBridge', () => { void shutdownPromise; }); + it('killAllSync force-kills the channel during the initialize handshake (tanzhenxin cold-spawn-window)', async () => { + // tanzhenxin cold-spawn-window finding: the agent child exists + // from the moment `channelFactory(boundWorkspace)` returns, but + // pre-fix `aliveChannels.add(info)` ran only AFTER the + // `initialize` handshake completed (up to `initTimeoutMs`, + // default 10s). A double-Ctrl+C in that handshake window played + // out as: first SIGINT entered `shutdown()` and awaited the + // in-flight spawn; second SIGINT called `killAllSync()` against + // an empty `aliveChannels` (the channel hadn't been added yet) + // and `process.exit(1)` orphaned the child. The fix moves the + // add + the `channel.exited` handler registration BEFORE the + // `initialize` await; this test pins that the channel is + // reachable via `killAllSync` during the handshake. + const killSyncCalls: string[] = []; + const factory: ChannelFactory = async () => { + // Bespoke agent whose `initialize` never resolves — that's the + // handshake-hanging window the finding is about. A real agent + // can spend up to `initTimeoutMs` ms here before the bridge's + // `withTimeout` aborts it. + const ab = new TransformStream(); + const ba = new TransformStream(); + const clientStream = ndJsonStream(ab.writable, ba.readable); + const agentStream = ndJsonStream(ba.writable, ab.readable); + let resolveExited: + | (( + info?: + | { + exitCode: number | null; + signalCode: NodeJS.Signals | null; + } + | undefined, + ) => void) + | undefined; + const exited = new Promise< + | { exitCode: number | null; signalCode: NodeJS.Signals | null } + | undefined + >((r) => { + resolveExited = r; + }); + const stuckAgent: Agent = { + async initialize() { + // Hang forever — the bridge's `withTimeout` would normally + // bound this, but the test asserts behavior DURING the + // handshake, so we let it sit until killAllSync resolves + // `exited` and tears the channel down externally. + return new Promise(() => {}); + }, + async newSession() { + throw new Error('newSession should not be reached'); + }, + async loadSession() { + throw new Error('loadSession should not be reached'); + }, + async authenticate() { + throw new Error('authenticate should not be reached'); + }, + async prompt() { + throw new Error('prompt should not be reached'); + }, + async cancel() { + /* no-op */ + }, + async setSessionMode() { + throw new Error('setSessionMode should not be reached'); + }, + async setSessionConfigOption() { + throw new Error('setSessionConfigOption should not be reached'); + }, + }; + new AgentSideConnection(() => stuckAgent, agentStream); + return { + stream: clientStream, + exited, + kill: async () => { + resolveExited!(undefined); + }, + killSync: () => { + killSyncCalls.push('called'); + resolveExited!(undefined); + }, + }; + }; + const bridge = makeBridge({ + channelFactory: factory, + // Bump initializeTimeoutMs so it doesn't race with the + // killAllSync we fire below. We're NOT testing the timeout + // path — we're testing the cold-spawn window before it. + initializeTimeoutMs: 30_000, + }); + + // Kick off a spawn — `initialize` hangs forever in this fake, + // so the spawn promise never resolves naturally. Don't await + // (would block the test); `.catch` keeps the rejection from + // being unhandled when killAllSync eventually tears things down. + const spawnPromise = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch(() => undefined); + + // Yield enough microtasks for `channelFactory` to return AND the + // bridge's `info` creation + `aliveChannels.add(info)` + the + // `channel.exited` handler registration to all run BEFORE the + // bridge enters `await initialize`. Pre-fix the alive-set add + // sat AFTER initialize, so any number of yields here would still + // find an empty set when killAllSync fires below. + for (let i = 0; i < 5; i++) { + await new Promise((r) => setImmediate(r)); + } + + // Operator double-Ctrl+C arrives during the handshake window. + bridge.killAllSync(); + + // Post-fix expectation: channel was added to `aliveChannels` + // BEFORE the `initialize` await, so killAllSync iterates a set + // containing it and fires killSync. Pre-fix this array would + // have been empty — and `process.exit(1)` after this would have + // orphaned the agent child. + expect(killSyncCalls).toEqual(['called']); + + // Cleanup: spawnPromise resolves on its own once killSync's + // `resolveExited` propagates through the bridge's + // `channel.exited` handler and the IIFE's catch reaps the half- + // initialized channel. + void spawnPromise; + }); + + it('killSession marks the channel dying so concurrent spawnOrAttach gets a fresh channel', async () => { + // After the last session is killed, `channel.kill()` runs through + // its SIGTERM grace window before SIGKILL — up to 10s in the real + // factory. During that window a concurrent `spawnOrAttach` MUST + // get a FRESH channel, never the dying one. Pre-fix: `channelInfo` + // stayed set with no `isDying` flag, so `ensureChannel` returned + // the dying channel and `newSession()` either succeeded onto a + // transport about to close (landing a sessionId that 404s on the + // next request when `channel.exited` fires) or hung until the + // newSession timeout. Fix: `killSession` sets `isDying = true` + // synchronously before `await ci.channel.kill()`; `ensureChannel` + // skips dying channels and spawns a fresh one. + const handles: ChannelHandle[] = []; + const factory: ChannelFactory = async () => { + const h = makeChannel({ sessionIdPrefix: `s${handles.length}` }); + // Make kill() hang forever so the SIGTERM grace window stays + // open for the test (simulates a slow-to-exit child). + h.channel = { ...h.channel, kill: () => new Promise(() => {}) }; + handles.push(h); + return h.channel; + }; + const bridge = makeBridge({ channelFactory: factory }); + const first = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(handles).toHaveLength(1); + + // Kick off killSession (the only session leaving triggers the + // channel teardown). The kill() Promise never resolves, so the + // method's await hangs — we fire-and-forget. + const killPromise = bridge.killSession(first.sessionId); + // Yield once so killSession's sync prefix runs (it marks + // `isDying = true` synchronously before `await ci.channel.kill()`). + await new Promise((r) => setImmediate(r)); + + // A new spawn MUST get a FRESH channel, not reuse the dying one. + const second = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(handles).toHaveLength(2); + expect(second.sessionId).not.toBe(first.sessionId); + // The second session is on the fresh channel (handles[1]), not + // multiplexed onto the dying one (handles[0]). + expect(handles[1]?.agent.newSessionCalls).toHaveLength(1); + + // Cleanup: both channels' kill() never resolves (factory above + // overrides it). Don't await killSession or shutdown — same + // pattern as the BkUyD test above. The test runner GCs the + // dangling promises when this `it` returns. + void killPromise; + }); + + it('doSpawn newSession-failure marks the empty channel dying so the next spawn gets a fresh one', async () => { + // Parallel to "killSession marks the channel dying" above, but + // covers the OTHER `isDying = true` site: `doSpawn`'s + // `connection.newSession()` rejection path. When the channel's + // first/only `newSession` fails (auth, bad config, agent crash + // during init), the bridge marks the empty channel dying and + // kicks off `channel.kill()`. The kill awaits a SIGTERM grace, + // and during that window the next `spawnOrAttach` retry MUST + // get a FRESH channel — not reuse the one whose newSession just + // failed (which would re-issue newSession to a transport about + // to close, almost certainly hanging or failing identically). + // Pre-fix the equivalent code eagerly cleared `channelInfo` so + // the BkUyD invariant was violated; the round-2 fix uses + // `isDying` + `aliveChannels` instead. + let factoryCount = 0; + const killSyncCalls: string[] = []; + const factory: ChannelFactory = async () => { + const tag = `c${factoryCount++}`; + // First channel's newSession rejects; subsequent channels succeed. + const firstChannel = factoryCount === 1; + const h = makeChannel({ + sessionIdPrefix: tag, + newSessionImpl: firstChannel + ? () => { + throw new Error('agent refused newSession (test)'); + } + : undefined, + }); + const realKillSync = h.channel.killSync; + h.channel = { + ...h.channel, + // Hang kill() so the SIGTERM grace stays open for the + // duration of the test. We don't await spawnOrAttach's + // rejection (which would block on the kill) — instead we + // catch it via .catch() and yield enough cycles for the + // sync prefix (`isDying = true`) to settle. + kill: () => new Promise(() => {}), + killSync: () => { + killSyncCalls.push(tag); + realKillSync(); + }, + }; + return h.channel; + }; + // Thread scope so calls don't coalesce via `inFlightSpawns` — + // the second spawn must not wait on the first one's hanging + // doSpawn. Without thread scope the single-scope coalescing + // would make `spawnOrAttach` call 2 await call 1's in-flight + // promise (still pending on the never-resolving kill). + const bridge = makeBridge({ + channelFactory: factory, + sessionScope: 'thread', + }); + + // First spawn: newSession on c0 fails. `doSpawn`'s catch runs + // `ci.isDying = true` synchronously, then `await ci.channel.kill()` + // (hangs in this test). The original error never propagates + // because the kill never resolves — so we DON'T await the + // rejection. Capture it for cleanup. + let firstErr: unknown; + const firstAttempt = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((err) => { + firstErr = err; + }); + + // Yield enough times for `ensureChannel`'s spawn to complete, + // newSession to reject, and doSpawn's catch sync prefix + // (`ci.isDying = true`) to run before the kill-await hangs. + for (let i = 0; i < 5; i++) { + await new Promise((r) => setImmediate(r)); + } + expect(factoryCount).toBe(1); + + // Second attempt: `ensureChannel` finds c0 with `isDying: true`, + // skips it, spawns a fresh c1. Pre-fix the equivalent code + // (eagerly clearing `channelInfo`) made this work via a + // different mechanism that violated BkUyD; the current fix uses + // `isDying` + `aliveChannels` for both correctness AND BkUyD. + const second = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(factoryCount).toBe(2); + expect(second.attached).toBe(false); + + // Both channels live in `aliveChannels` (c0 is dying but its + // `channel.exited` hasn't fired; c1 is freshly attached). + // `killAllSync` MUST find both. + bridge.killAllSync(); + expect(killSyncCalls.sort()).toEqual(['c0', 'c1']); + + // Cleanup: firstAttempt is pending forever (kill never resolves). + // Touch firstErr to satisfy linters about the variable. + void firstAttempt; + void firstErr; + }); + + it('killAllSync force-kills BOTH the dying channel AND the fresh attach-target (BkUyD overwrite race)', async () => { + // The killSession → spawnOrAttach race opens a window where two + // channels are simultaneously "alive" from the daemon's + // perspective: the dying one (sessionIds.size === 0, in + // SIGTERM grace) and the fresh one (just spawned to serve the new + // request). Pre-fix `killAllSync()` iterated only `channelInfo` + // (the fresh one), missing the dying channel and orphaning its + // child when `process.exit(1)` fired before its SIGTERM + // escalation timer. Fix: separate `aliveChannels: Set` + // that `killAllSync` iterates, only cleared by each channel's + // `channel.exited` (the OS-reap signal). + const killSyncCalls: string[] = []; + let factoryCount = 0; + const factory: ChannelFactory = async () => { + const tag = `c${factoryCount++}`; + const h = makeChannel({ sessionIdPrefix: tag }); + const realKillSync = h.channel.killSync; + h.channel = { + ...h.channel, + // kill() hangs forever so the dying channel stays in + // SIGTERM grace for the duration of the test. + kill: () => new Promise(() => {}), + killSync: () => { + killSyncCalls.push(tag); + realKillSync(); + }, + }; + return h.channel; + }; + const bridge = makeBridge({ channelFactory: factory }); + const first = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + // Trigger the overwrite race: kill the only session → channel + // marked dying, kill awaits a never-resolving Promise; then + // spawn a new session → fresh channel, `channelInfo` reassigned. + const killPromise = bridge.killSession(first.sessionId); + await new Promise((r) => setImmediate(r)); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + // Both channels are alive from the OS's perspective. A + // double-Ctrl+C arrives. + bridge.killAllSync(); + + // BOTH channels received killSync. Pre-fix only `c1` (the fresh + // one in `channelInfo`) would have fired — `c0` was dying in + // unreachable state and would have orphaned its child. + expect(killSyncCalls.sort()).toEqual(['c0', 'c1']); + + // Cleanup: dangling never-resolving promises GC'd by the runner. + void killPromise; + }); + describe('sendPrompt', () => { it('forwards a prompt and returns the agent response', async () => { const handles: ChannelHandle[] = []; @@ -449,7 +850,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const result = await bridge.sendPrompt(session.sessionId, { @@ -469,7 +870,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); await bridge.sendPrompt(session.sessionId, { @@ -506,7 +907,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const p1 = bridge.sendPrompt(session.sessionId, { @@ -551,7 +952,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const failed = await bridge @@ -575,7 +976,7 @@ describe('createHttpAcpBridge', () => { }); it('throws SessionNotFoundError for unknown session ids', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -597,7 +998,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); await bridge.cancelSession(session.sessionId); @@ -612,7 +1013,7 @@ describe('createHttpAcpBridge', () => { }); it('throws SessionNotFoundError for unknown session ids', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -660,7 +1061,7 @@ describe('createHttpAcpBridge', () => { }, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); return { bridge, session, conn: capturedConn!, handles }; } @@ -848,7 +1249,7 @@ describe('createHttpAcpBridge', () => { }); it('respondToPermission returns false for unknown requestId', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => makeChannel().channel, }); const accepted = bridge.respondToPermission('does-not-exist', { @@ -977,7 +1378,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); // Kick off sendPrompt — agent will issue a permission request @@ -1055,7 +1456,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); return { bridge, setModelCalls }; } @@ -1160,7 +1561,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); expect(bridge.sessionCount).toBe(1); @@ -1172,7 +1573,7 @@ describe('createHttpAcpBridge', () => { }); // Simulate a child crash (channel.exited resolves but we never called - // kill() — entry is still in byId/byWorkspace at the moment of crash). + // kill() — entry is still in byId / defaultEntry at the moment of crash). handles[0]?.crash(); // Drain the bus — first frame is `session_died`. @@ -1204,7 +1605,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); // No subscribers; planned shutdown removes the entry first, THEN @@ -1252,7 +1653,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A, modelServiceId: 'first', @@ -1332,7 +1733,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); // First call spawns the session AND applies model "A". await bridge.spawnOrAttach({ workspaceCwd: WS_A, @@ -1406,7 +1807,7 @@ describe('createHttpAcpBridge', () => { it('applies modelServiceId on attach via unstable_setSessionModel', async () => { const { factory, setModelCalls } = setupRecording(); - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); // First call spawns; second call attaches with a DIFFERENT model. const first = await bridge.spawnOrAttach({ @@ -1431,7 +1832,7 @@ describe('createHttpAcpBridge', () => { it('attach without modelServiceId does NOT issue setSessionModel', async () => { const { factory, setModelCalls } = setupRecording(); - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); await bridge.spawnOrAttach({ workspaceCwd: WS_A, @@ -1503,7 +1904,7 @@ describe('createHttpAcpBridge', () => { killSync: () => resolveExited!(), }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const promptResult = bridge.sendPrompt(session.sessionId, { @@ -1522,17 +1923,17 @@ describe('createHttpAcpBridge', () => { describe('opts validation', () => { it('rejects an invalid sessionScope', () => { expect(() => - createHttpAcpBridge({ + makeBridge({ sessionScope: 'bogus' as unknown as 'single', }), ).toThrow(/Invalid sessionScope/); }); it('rejects a non-positive initializeTimeoutMs', () => { - expect(() => createHttpAcpBridge({ initializeTimeoutMs: 0 })).toThrow( + expect(() => makeBridge({ initializeTimeoutMs: 0 })).toThrow( /initializeTimeoutMs/, ); - expect(() => createHttpAcpBridge({ initializeTimeoutMs: -1 })).toThrow( + expect(() => makeBridge({ initializeTimeoutMs: -1 })).toThrow( /initializeTimeoutMs/, ); }); @@ -1541,17 +1942,13 @@ describe('createHttpAcpBridge', () => { // A typo / parse error in CLI / config that yields NaN must // NOT silently disable the daemon's resource cap. We fail // boot loud instead of serving unbounded. - expect(() => createHttpAcpBridge({ maxSessions: NaN })).toThrow( + expect(() => makeBridge({ maxSessions: NaN })).toThrow( /maxSessions: NaN/, ); - expect(() => createHttpAcpBridge({ maxSessions: -5 })).toThrow( - /maxSessions: -5/, - ); + expect(() => makeBridge({ maxSessions: -5 })).toThrow(/maxSessions: -5/); // Explicit zero or Infinity remain valid "unlimited" sentinels. - expect(() => createHttpAcpBridge({ maxSessions: 0 })).not.toThrow(); - expect(() => - createHttpAcpBridge({ maxSessions: Infinity }), - ).not.toThrow(); + expect(() => makeBridge({ maxSessions: 0 })).not.toThrow(); + expect(() => makeBridge({ maxSessions: Infinity })).not.toThrow(); }); }); @@ -1565,7 +1962,7 @@ describe('createHttpAcpBridge', () => { await new Promise((r) => setTimeout(r, 10)); return makeChannel().channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const [a, b] = await Promise.all([ bridge.spawnOrAttach({ workspaceCwd: WS_A }), @@ -1595,7 +1992,7 @@ describe('createHttpAcpBridge', () => { } return makeChannel().channel; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); await expect( bridge.spawnOrAttach({ workspaceCwd: WS_A }), @@ -1636,7 +2033,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); return { bridge, session, conn: capturedConn! }; } @@ -1955,7 +2352,7 @@ describe('createHttpAcpBridge', () => { }); describe('listWorkspaceSessions', () => { - it('returns sessions matching the canonical workspace cwd', async () => { + it('returns sessions matching the bound workspace cwd', async () => { let n = 0; const factory: ChannelFactory = async () => { // Distinct sessionIdPrefix per spawn so two thread-scope sessions @@ -1964,22 +2361,24 @@ describe('createHttpAcpBridge', () => { const h = makeChannel({ sessionIdPrefix: `s${n++}` }); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ sessionScope: 'thread', channelFactory: factory, }); const a1 = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const a2 = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await bridge.spawnOrAttach({ workspaceCwd: WS_B }); const aList = bridge.listWorkspaceSessions(WS_A); expect(aList).toHaveLength(2); expect(aList.map((s) => s.sessionId).sort()).toEqual( [a1.sessionId, a2.sessionId].sort(), ); + // Querying a different workspace returns an empty list (the + // bridge only hosts `boundWorkspace` per #3803 §02; a UI asking + // for sessions in some other path is correct to see "none"). const bList = bridge.listWorkspaceSessions(WS_B); - expect(bList).toHaveLength(1); + expect(bList).toEqual([]); const idleList = bridge.listWorkspaceSessions('/work/c'); expect(idleList).toEqual([]); @@ -1988,7 +2387,7 @@ describe('createHttpAcpBridge', () => { it('canonicalizes the lookup path', async () => { const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const list = bridge.listWorkspaceSessions('/work/./a'); @@ -1999,7 +2398,7 @@ describe('createHttpAcpBridge', () => { }); it('returns empty for relative paths instead of throwing', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -2046,7 +2445,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); return { bridge, session, setModelCalls }; } @@ -2085,7 +2484,7 @@ describe('createHttpAcpBridge', () => { }); it('throws SessionNotFoundError for unknown session ids', async () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -2101,7 +2500,7 @@ describe('createHttpAcpBridge', () => { describe('subscribeEvents', () => { it('throws SessionNotFoundError for unknown session ids', () => { - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: async () => { throw new Error('factory should not be called'); }, @@ -2132,7 +2531,7 @@ describe('createHttpAcpBridge', () => { killSync: () => {}, }; }; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const abort = new AbortController(); @@ -2163,7 +2562,7 @@ describe('createHttpAcpBridge', () => { it('shutdown closes live event subscriptions', async () => { const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ channelFactory: factory }); + const bridge = makeBridge({ channelFactory: factory }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); const abort = new AbortController(); @@ -2195,8 +2594,10 @@ describe('createHttpAcpBridge', () => { describe('maxSessions cap (chiga0 Rec 3)', () => { it('refuses NEW spawns past the cap with SessionLimitExceededError', async () => { - const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + let n = 0; + const factory: ChannelFactory = async () => + makeChannel({ sessionIdPrefix: `s${n++}` }).channel; + const bridge = makeBridge({ channelFactory: factory, maxSessions: 2, // `thread` so each call is a fresh session, not an attach. @@ -2205,7 +2606,7 @@ describe('createHttpAcpBridge', () => { // First two spawns succeed. await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await bridge.spawnOrAttach({ workspaceCwd: WS_B }); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); expect(bridge.sessionCount).toBe(2); // Third hits the cap. @@ -2223,7 +2624,7 @@ describe('createHttpAcpBridge', () => { it('attach to an existing session under single scope is NOT counted toward the cap', async () => { const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, maxSessions: 1, sessionScope: 'single', @@ -2240,12 +2641,11 @@ describe('createHttpAcpBridge', () => { expect(b.sessionId).toBe(a.sessionId); expect(bridge.sessionCount).toBe(1); - // But a NEW workspace (would need a fresh spawn) is rejected. + // A cross-workspace request rejects with WorkspaceMismatchError + // (#3803 §02) — the bridge is bound to one workspace. await expect( bridge.spawnOrAttach({ workspaceCwd: WS_B }), - ).rejects.toMatchObject({ - name: 'SessionLimitExceededError', - }); + ).rejects.toBeInstanceOf(WorkspaceMismatchError); await bridge.shutdown(); }); @@ -2256,7 +2656,7 @@ describe('createHttpAcpBridge', () => { // for the same workspace and gets attached:true. Without the // race guard, A's reaper would tear down B's session. const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2288,7 +2688,7 @@ describe('createHttpAcpBridge', () => { }); return makeChannel().channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: slowFactory, sessionScope: 'single', }); @@ -2321,7 +2721,7 @@ describe('createHttpAcpBridge', () => { // when the spawn owner ALSO indicated they want it (via the // killSession-bail tombstone). const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2345,7 +2745,7 @@ describe('createHttpAcpBridge', () => { // set during the spawn-owner bail, B's detach now completes // the deferred reap. const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2369,7 +2769,7 @@ describe('createHttpAcpBridge', () => { // Counterpart: when client C is actively subscribed, detach // from a transient B must NOT reap C's session. const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2400,7 +2800,7 @@ describe('createHttpAcpBridge', () => { // negative path so a future change can't accidentally make // it always-skip. const factory: ChannelFactory = async () => makeChannel().channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'single', }); @@ -2420,7 +2820,7 @@ describe('createHttpAcpBridge', () => { let n = 0; const factory: ChannelFactory = async () => makeChannel({ sessionIdPrefix: `s${n++}` }).channel; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, maxSessions: 0, sessionScope: 'thread', @@ -2449,7 +2849,7 @@ describe('createHttpAcpBridge', () => { factoryCalls++; return makeChannel({ sessionIdPrefix: `s${factoryCalls}` }).channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, maxSessions: 0, sessionScope: 'thread', @@ -2478,7 +2878,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'thread', }); @@ -2512,7 +2912,7 @@ describe('createHttpAcpBridge', () => { handles.push(h); return h.channel; }; - const bridge = createHttpAcpBridge({ + const bridge = makeBridge({ channelFactory: factory, sessionScope: 'thread', }); diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 10b0a773731..af751d3a3f5 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -40,12 +40,18 @@ import type { /** * Stage 1 HTTP→ACP bridge. * - * Per design §08 (Roadmap, Stage 1) and the issue body's Caveat: - * - One `qwen --acp` child PER WORKSPACE; multiple sessions on the same - * workspace multiplex onto that child via `connection.newSession()` - * (the agent's native `sessions: Map` — see - * `acp-integration/acpAgent.ts:194`). Sessions share the child's - * process / OAuth state / `FileReadCache` / hierarchy-memory parse. + * Per #3803 §02 (architectural revision) and design §08 (Roadmap, Stage 1): + * - **1 daemon = 1 workspace**: every bridge instance is bound to a + * single canonical workspace path at construction + * (`BridgeOptions.boundWorkspace`). All `spawnOrAttach` calls must + * target that workspace; cross-workspace requests throw + * `WorkspaceMismatchError`. Multi-workspace deployments use multiple + * daemon processes (one per workspace, supervised externally). + * - One `qwen --acp` child total; multiple sessions multiplex onto it + * via `connection.newSession()` (the agent's native + * `sessions: Map` — see `acp-integration/acpAgent.ts:194`). + * Sessions share the child's process / OAuth state / `FileReadCache` / + * hierarchy-memory parse. * - HTTP request bodies are forwarded as ACP NDJSON over the child's stdin. * - Child stdout NDJSON notifications publish onto each session's * `EventBus`; HTTP SSE subscribers (`GET /session/:id/events`) drain @@ -54,10 +60,6 @@ import type { * bridge (FIFO; honors ACP's "one active prompt per session" invariant). * Different sessions on the same channel can prompt concurrently — * the ACP layer demultiplexes by sessionId. - * - Cross-workspace channel sharing is intentionally NOT done. Different - * workspaces have different `loadSettings(cwd)` state; one child would - * step on the previous workspace's settings. One channel per workspace - * is the safe scope. * * Stage 2 replaces the spawn step with an in-process call into core's * ACP-equivalent API. The `HttpAcpBridge` interface stays the same so HTTP @@ -221,8 +223,8 @@ export interface HttpAcpBridge { */ export class SessionNotFoundError extends Error { readonly sessionId: string; - constructor(sessionId: string) { - super(`No session with id "${sessionId}"`); + constructor(sessionId: string, extra?: string) { + super(`No session with id "${sessionId}"` + (extra ? `. ${extra}` : '')); this.name = 'SessionNotFoundError'; this.sessionId = sessionId; } @@ -244,6 +246,59 @@ export class SessionLimitExceededError extends Error { } } +/** + * Thrown by `spawnOrAttach` when the requested `workspaceCwd` doesn't + * canonicalize to the daemon's bound workspace. Per #3803 §02 every + * bridge instance is bound to exactly one workspace; cross-workspace + * requests are rejected at the daemon boundary. The server route + * translates this to a 400 response with `code: 'workspace_mismatch'` + * and both paths in the body so clients can fall through to spawning + * their own daemon / routing to a different one via an orchestrator. + */ +export class WorkspaceMismatchError extends Error { + readonly bound: string; + readonly requested: string; + constructor(bound: string, requested: string) { + // Truncate `requested` to PATH_MAX so a malicious or buggy client + // can't amplify a multi-MB `cwd` body through this error. The + // constructor interpolates `requested` into `.message` TWICE, the + // route's `sendBridgeError` echoes it in stderr (now JSON.stringify + // -wrapped per the log-injection fix), and `res.json` echoes it in + // the 400 body — without truncation a ~10 MB cwd (right under the + // `express.json({limit: '10mb'})` cap) becomes ~20 MB message + + // ~10 MB stderr + ~30 MB JSON response per request, × + // `maxConnections` (default 256). The route also caps `cwd.length` + // at this same limit upstream (POST /session); this is + // defense-in-depth for non-HTTP callers (tests, embeds, future + // entry points that throw the error directly). + const safeRequested = + requested.length > MAX_WORKSPACE_PATH_LENGTH + ? `${requested.slice(0, MAX_WORKSPACE_PATH_LENGTH)}…[truncated]` + : requested; + super( + `Workspace mismatch: daemon is bound to "${bound}" but ` + + `request asked for "${safeRequested}". Each \`qwen serve\` ` + + `daemon binds to exactly one workspace; start a separate ` + + `daemon for "${safeRequested}" (or route the request to one ` + + `via an orchestrator).`, + ); + this.name = 'WorkspaceMismatchError'; + this.bound = bound; + this.requested = safeRequested; + } +} + +/** + * PATH_MAX on Linux is 4096; macOS / BSD is 1024. We use the Linux + * value as a generous ceiling — anything bigger is either a + * malformed client request (memory amplification attack against the + * 400 / stderr / error-message echo paths) or a synthetic test + * input. The route's POST /session pre-check rejects bodies past + * this; `WorkspaceMismatchError` truncates for any caller that + * skips the pre-check. + */ +export const MAX_WORKSPACE_PATH_LENGTH = 4096; + /** * One ACP NDJSON channel to a single agent. Tests inject a fake by replacing * the channel factory; production uses `defaultSpawnChannelFactory`. @@ -265,8 +320,8 @@ export interface AcpChannel { * Resolves when the channel has terminated for any reason — planned * (`kill()` called) OR unexpected (child process crashed, stream closed). * The bridge subscribes to this so a SessionEntry whose underlying - * channel dies between requests is removed from `byWorkspace`/`byId` - * instead of lingering as a stuck session. + * channel dies between requests is removed from `byId` / + * `defaultEntry` instead of lingering as a stuck session. * * Resolves to `{ exitCode, signalCode }` when the spawn factory can * capture them (the standard `child.on('exit', code, signal)` path), @@ -348,40 +403,93 @@ export interface BridgeOptions { * cap. */ maxPendingPermissionsPerSession?: number; + /** + * Absolute, **already-canonical** path this daemon is bound to (per + * #3803 §02: 1 daemon = 1 workspace). `spawnOrAttach` calls whose + * `workspaceCwd` doesn't canonicalize to this same value throw + * `WorkspaceMismatchError` (route → 400 with code `workspace_mismatch`). + * + * **Caller contract**: pass the result of + * `canonicalizeWorkspace(path)`. `runQwenServe` does this at boot + * and threads the same canonical value into the bridge AND + * `createServeApp` (via `deps.boundWorkspace`) so all three — + * `/capabilities.workspaceCwd`, the `POST /session` cwd fallback, + * and this bridge's mismatch check — share one canonical form. The + * constructor only checks `path.isAbsolute`; it does NOT + * re-canonicalize (a redundant `realpathSync.native` could + * theoretically diverge from the runQwenServe canonicalize on + * NFS-transient / mid-rename filesystems, landing the bridge with + * one canonical form while `/capabilities` advertises another). + * Direct embeds / tests calling `createHttpAcpBridge` themselves + * MUST canonicalize before passing. + */ + boundWorkspace: string; } /** - * One `qwen --acp` child + the ACP connection on top of it, shared by - * all SessionEntries whose workspace maps to this channel. Stage 1.5 - * multi-session work (per LaZzyMan / tanzhenxin reviews) leverages - * the agent's native `sessions: Map` (see - * `acp-integration/acpAgent.ts:194`) so multiple `newSession()` calls - * on one channel get separate session ids while sharing the child's - * process / OAuth / file-cache / hierarchy-memory parse. + * The single `qwen --acp` child + the ACP connection on top of it, + * shared by every SessionEntry in this daemon. Per #3803 §02 the + * bridge is bound to one workspace at construction, so there is at + * most one channel alive at any moment. Multiple sessions multiplex + * onto it via the agent's native `sessions: Map` + * (see `acp-integration/acpAgent.ts:194`), each `newSession()` call + * returning a distinct id while sharing the child's process / OAuth / + * file-cache / hierarchy-memory parse. * - * Lifetime: created on first `spawnOrAttach` for a workspace, kept - * alive while `sessionIds.size > 0`, and killed by `killSession` when - * the last entry leaves OR by `channel.exited` when the child dies. - * Cross-workspace channel sharing is intentionally NOT done in this - * bridge — `acpAgent.ts:601 (this.settings = loadSettings(cwd))` - * replaces the cached settings on each newSession call, so different - * workspaces in one child would step on each other's settings. One - * channel per workspace is the safe scope for Stage 1.5. + * Lifetime: created on first `spawnOrAttach`, kept alive while + * `sessionIds.size > 0`, and killed by `killSession` when the last + * entry leaves OR by `channel.exited` when the child dies. */ interface ChannelInfo { channel: AcpChannel; connection: ClientSideConnection; /** Shared BridgeClient — its methods route ACP params by sessionId. */ client: BridgeClient; - workspaceCwd: string; + // Note: pre-§02 a `workspaceCwd: string` field lived here so the + // `byWorkspaceChannel.get(entry.workspaceCwd)` lookup could route + // multi-workspace requests. Under "1 daemon = 1 workspace" the + // module-scope `boundWorkspace` is the single source of truth and + // every channel inherits it. Per-channel storage would suggest + // variance the model doesn't allow; dropping it makes the + // single-workspace invariant visible at the type level. /** * Live session ids multiplexed on this channel. Updated when * `doSpawn` registers a new session and when `killSession` / - * `channel.exited` removes one. When the set drops to empty AND no - * session is mid-attach, the channel is killed and removed from - * `byWorkspaceChannel`. + * `channel.exited` removes one. When the set drops to empty under + * `killSession`, the channel is marked `isDying = true` and its + * `channel.kill()` is awaited; `channelInfo` itself is left + * pointing at the dying channel until `channel.exited` fires (see + * BkUyD invariant on `isDying` below). */ sessionIds: Set; + /** + * MUST be set to `true` synchronously by any teardown path BEFORE + * awaiting `channel.kill()`. `ensureChannel` treats a dying channel + * as absent and spawns a fresh one — without this flag a concurrent + * `spawnOrAttach` arriving during the SIGTERM grace window (up to + * 10s) would attach to a transport about to close, landing the + * caller with a sessionId that 404s on every follow-up request. + * + * **Set-sites (5)** — any new teardown path MUST call into one of + * these or replicate the pattern: + * + * 1. `ensureChannel`: `initialize`-failure catch. + * 2. `ensureChannel`: late-shutdown re-check (shuttingDown flipped + * during handshake). + * 3. `doSpawn`: newSession-failure on an empty channel + * (sessionIds.size === 0). + * 4. `killSession`: last session leaving (sessionIds.size === 0 + * after the delete). + * 5. `shutdown`: bulk-mark every entry in `aliveChannels`. + * + * **BkUyD invariant (why we don't clear `channelInfo` here)**: + * `killAllSync` must still find the channel during the SIGTERM + * grace window to fire SIGKILL on `process.exit(1)`. `aliveChannels` + * holds the dying entry until `channel.exited` fires (OS-level + * reap); `isDying` is the "available-for-new-spawns" half of the + * two-bit (alive, dying) state. + */ + isDying: boolean; } interface SessionEntry { @@ -879,7 +987,7 @@ const DEFAULT_PERMISSION_TIMEOUT_MS = 5 * 60 * 1000; // `BridgeOptions.maxPendingPermissionsPerSession`. const DEFAULT_MAX_PENDING_PER_SESSION = 64; -export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { +export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { const sessionScope = opts.sessionScope ?? 'single'; // `undefined` → default 20 (intentionally tight per #3803 N≈50 cliff). // `0` → explicitly unlimited (operator opt-out). @@ -936,38 +1044,73 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { maxPendingRaw > 0 && Number.isFinite(maxPendingRaw) ? maxPendingRaw : Infinity; - - // Single-scope reuse keyed by canonical workspace path. Tracks the - // SessionEntry that a same-workspace attach should re-use. With - // Stage 1.5 multi-session per channel, this points at the FIRST - // session created for the workspace under `single` scope; under - // `thread` scope additional sessions on the same workspace don't - // overwrite this entry. - const byWorkspace = new Map(); - // Stage 1.5 multi-session: one channel per workspace, N sessions - // multiplex on it via `connection.newSession({cwd, mcpServers})`. - // `byWorkspaceChannel.get(workspaceKey)` returns the shared channel - // for spawn-vs-reuse decisions in `doSpawn`. Channel is kept alive - // while `sessionIds.size > 0`; the last `killSession` (or the - // `channel.exited` cleanup) drops the entry from this map. - const byWorkspaceChannel = new Map(); - // tanzhenxin BkUyD: source of truth for "channels with potentially- - // alive child processes" — independent of `byWorkspaceChannel`, - // which `shutdown()` clears BEFORE awaiting per-child SIGTERM- - // grace kills. `killAllSync()` (the double-Ctrl+C force-exit - // path) iterates THIS set so a mid-shutdown second signal still - // sees the children that haven't yet finished their SIGTERM grace. - // Only removed when `channel.exited` fires (the OS-level "really - // dead" signal). The earlier design iterated `byWorkspaceChannel` - // and silently no-op'd during the shutdown await window. - const liveChannels = new Set(); - // Coalesces concurrent channel-spawn requests for the same workspace - // (regardless of sessionScope). Without this, two parallel callers - // would both `channelFactory(workspaceKey)` and one of the - // spawned children would never make it into `byWorkspaceChannel`, - // becoming a permanent orphan. Cleared in the `finally` of the - // creator regardless of outcome. - const inFlightChannelSpawns = new Map>(); + // #3803 §02: the bound path is the canonical form `spawnOrAttach` + // compares incoming `workspaceCwd` against. The caller MUST pass an + // already-canonical value (via `canonicalizeWorkspace`). `runQwenServe` + // does this at boot and threads the same value into both + // `createHttpAcpBridge` and `createServeApp` (via + // `deps.boundWorkspace`); direct embeds / tests that construct the + // bridge themselves must call `canonicalizeWorkspace` first. + // + // Pre-fix the bridge re-canonicalized defensively here. The fix + // (deepseek-v4-pro review) drops the redundant `realpathSync.native`: + // (a) on case-insensitive / symlinked filesystems two independent + // `realpathSync.native` calls could theoretically disagree if the FS + // mutates between them (NFS transient, operator rename), landing + // the bridge with one canonical form while `runQwenServe` advertises + // another and `/capabilities` clients see `workspace_mismatch` on + // every POST; (b) it's a syscall removed from the boot path. The + // `path.isAbsolute` guard stays — it's a structural input check, not + // a syscall. + if (!path.isAbsolute(opts.boundWorkspace)) { + throw new TypeError( + `Invalid boundWorkspace: "${opts.boundWorkspace}". Must be an ` + + `absolute path.`, + ); + } + const boundWorkspace = opts.boundWorkspace; + + // #3803 §02 single-workspace model: the bridge hosts AT MOST one + // ATTACH-AVAILABLE channel and one default attach-target entry. + // Multi-session multiplexing happens through `channelInfo.sessionIds`; + // the `defaultEntry` slot is the FIRST session created (the one a + // same-workspace attach under `single` scope reuses). Thread-scope + // sessions add to `byId` but don't displace `defaultEntry`. + let defaultEntry: SessionEntry | undefined; + // `channelInfo` is the SINGLE attach-available channel. Cleared + // ONLY by the `channel.exited` handler (see below) when the OS + // reaps the underlying child process. Teardown initiators + // (`killSession` last-session-leaving, `doSpawn`-newSession-failure + // on an empty channel, `ensureChannel` init-failure / + // late-shutdown, `shutdown`) set `isDying = true` but LEAVE + // `channelInfo` pointing at the dying channel until OS reap — that + // asymmetry IS the BkUyD invariant. It lets `killAllSync` reach a + // mid-SIGTERM-grace channel through `aliveChannels` while a + // concurrent `spawnOrAttach` can already start spawning a fresh + // replacement (which overwrites `channelInfo` when its + // handshake completes). Race-aware code paths (`ensureChannel`, + // `killAllSync`) gate on `isDying` rather than presence; see + // `ChannelInfo.isDying` for the per-set-site rationale. + let channelInfo: ChannelInfo | undefined; + // tanzhenxin BkUyD: superset of `channelInfo` covering channels + // that are dying but not yet OS-reaped. `killSession` / + // `doSpawn`-newSession-failure / `shutdown` mark a channel as + // `isDying` and start its async kill; meanwhile a concurrent + // `spawnOrAttach` can spawn a FRESH channel and reassign + // `channelInfo`. Without this set, the dying channel becomes + // unreachable — a double-Ctrl+C arriving mid-grace would call + // `killAllSync()`, find only the fresh channel in `channelInfo`, + // force-kill it, and `process.exit(1)` would orphan the dying one + // whose SIGTERM hadn't yet completed. The set is the OS-level + // "still alive" source of truth: entries are added when a channel + // is created and removed when its `channel.exited` resolves. + // `killAllSync` iterates THIS set to fire SIGKILL on every alive + // child regardless of whether it's still the attach target. + const aliveChannels = new Set(); + // Coalesces a concurrent second `ensureChannel()` call onto the + // first one's spawn so we never create two children for the same + // daemon. Cleared in the `finally` of the creator. + let inFlightChannelSpawn: Promise | undefined; const byId = new Map(); // Daemon-wide pending permission table; requestIds are UUIDs so collisions // across sessions are infeasible in practice. @@ -981,11 +1124,13 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // (b) `server.close` rejecting new connections, during which a // late-arriving `POST /session` slips a fresh child past cleanup. let shuttingDown = false; - // Coalesces concurrent `spawnOrAttach` calls for the same workspace under - // single-scope. Without this, two parallel callers would both pass the - // `byWorkspace.get` check, both spawn, and one entry would be orphaned - // (in `byId` but not in `byWorkspace`) — violating the - // "at most one session per workspace" invariant. + // Coalesces concurrent `spawnOrAttach` calls under single-scope and + // tracks in-progress thread-scope spawns for shutdown to await. + // Single-scope uses the workspaceKey as the dedup key (at most one + // entry; concurrent callers pass the `defaultEntry` check together + // and coalesce here). Thread-scope uses `workspaceKey#uuid` so + // simultaneous calls don't collide while still being awaitable from + // `shutdown()`. const inFlightSpawns = new Map>(); const registerPending = (p: PendingPermission) => { @@ -1033,28 +1178,27 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { }; /** - * Get-or-create the shared `qwen --acp` channel for a workspace. - * Stage 1.5 multi-session: one channel hosts N sessions via - * `connection.newSession()`. Concurrent callers coalesce through - * `inFlightChannelSpawns` so we never spawn two children for one - * workspace. The returned `ChannelInfo` is shared — caller adds - * their session id to `sessionIds` and uses `info.connection.newSession()`. + * Get-or-create the daemon's single `qwen --acp` channel (#3803 §02). + * N sessions multiplex onto it via `connection.newSession()`. + * Concurrent callers coalesce through `inFlightChannelSpawn` so we + * never spawn two children. The returned `ChannelInfo` is shared — + * the caller adds their session id to `sessionIds` and uses + * `info.connection.newSession()`. * * Wires up the one-and-only `channel.exited` cleanup on first - * creation so the late-arriving event tears down ALL sessions on - * the channel (vs. the previous 1-session-per-channel design where - * each entry registered its own listener). + * creation so the late-arriving event tears down ALL multiplexed + * sessions. */ - async function getOrCreateChannel( - workspaceKey: string, - ): Promise { - const existing = byWorkspaceChannel.get(workspaceKey); - if (existing) return existing; - const inFlight = inFlightChannelSpawns.get(workspaceKey); - if (inFlight) return await inFlight; + async function ensureChannel(): Promise { + // Skip a channel that's marked dying — its underlying transport is + // mid-SIGTERM-or-already-dead and `connection.newSession()` on it + // would either hang or land the caller with a sessionId that + // immediately 404s on every follow-up. + if (channelInfo && !channelInfo.isDying) return channelInfo; + if (inFlightChannelSpawn) return await inFlightChannelSpawn; const promise = (async () => { - const channel = await channelFactory(workspaceKey); + const channel = await channelFactory(boundWorkspace); const client = new BridgeClient( // BfFut: ACP today carries a sessionId on every per-session // notification / request, so the no-sessionId branch is @@ -1064,12 +1208,11 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // instead of throwing. Surface that ambiguity loudly. (sessionId) => { if (sessionId) return byId.get(sessionId); - const info = byWorkspaceChannel.get(workspaceKey); - if (info && info.sessionIds.size > 1) { + if (channelInfo && channelInfo.sessionIds.size > 1) { throw new Error( 'BridgeClient: ACP call without sessionId on a ' + 'multi-session channel cannot be routed — workspace=' + - workspaceKey, + boundWorkspace, ); } return undefined; @@ -1084,57 +1227,93 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { ); const connection = new ClientSideConnection(() => client, channel.stream); - try { - await withTimeout( - connection.initialize({ - protocolVersion: PROTOCOL_VERSION, - clientCapabilities: { - fs: { readTextFile: true, writeTextFile: true }, - }, - clientInfo: { name: 'qwen-serve-bridge', version: '0' }, - }), - initTimeoutMs, - 'initialize', - ); - } catch (err) { - await channel.kill().catch(() => {}); - throw err; - } - - // Late-shutdown re-check: if shutdown flipped during `initialize`, - // tear this channel down rather than leak past `process.exit(0)`. - if (shuttingDown) { - await channel.kill().catch(() => {}); - throw new Error('HttpAcpBridge is shutting down'); - } - + // Add to `aliveChannels` + register the `channel.exited` handler + // BEFORE the `initialize` handshake (tanzhenxin cold-spawn-window + // finding): the agent child exists from the moment + // `channelFactory(boundWorkspace)` returns, so a `killAllSync()` + // during the handshake window (up to `initTimeoutMs`, default + // 10s) must find it to avoid orphaning on `process.exit(1)`. + // Init-failure / child-crash / late-shutdown all converge on + // the same cleanup path via the handler below. + // `channelInfo` (the attach target) is assigned only AFTER + // initialize succeeds so callers don't attach to a still- + // handshaking channel. const info: ChannelInfo = { channel, connection, client, - workspaceCwd: workspaceKey, sessionIds: new Set(), + isDying: false, }; - byWorkspaceChannel.set(workspaceKey, info); - liveChannels.add(info); + aliveChannels.add(info); + // Belt-and-suspenders leak detection. The set is intentionally + // multi-entry to cover the `killSession`-then-`spawnOrAttach` + // overlap window (size 2 is legitimate: one dying + one fresh + // attach-target). Anything higher implies a `channel.exited` + // handler never fired for some prior channel — a real leak we'd + // otherwise notice only as gradually-growing RSS over hours. + // The warning surfaces it the moment it happens. Threshold is + // 2 because that's the design ceiling; bumping it requires + // updating both this guard and the comments around + // `aliveChannels` declaration. + if (aliveChannels.size > 2) { + writeStderrLine( + `qwen serve: WARNING aliveChannels.size=${aliveChannels.size} ` + + `(expected 1, max 2 during killSession-then-spawnOrAttach ` + + `overlap) — possible channel leak; check that prior channels' ` + + `channel.exited fired and the handler ran cleanup.`, + ); + } // One-time channel.exited cleanup. The child dying takes ALL // multiplexed sessions with it — iterate `sessionIds` (snapshot // first to be safe against concurrent killSession during // iteration), publish `session_died` on each session's bus, - // remove from byId / byWorkspace / pending tables. + // remove from byId / defaultEntry / pending tables. + // + // Registered BEFORE the `initialize` await (tanzhenxin + // cold-spawn-window fix above) so init-failure / child-crash / + // late-shutdown all converge here. During handshake + // `sessionIds` is empty — the loop below no-ops, the stderr + // line still fires to tell operators "agent process gone + // during init", and `aliveChannels.delete(info)` clears the + // entry through the normal exit path. + // + // tanzhenxin BkUyD: drop from `aliveChannels` ONLY when the OS + // process is actually gone. Async kill paths (`killSession` + // reap, `shutdown()` await, `doSpawn`'s newSession-failure + // tear-down) mark `isDying = true` but leave the entry in + // `aliveChannels` until this handler fires, so the double-Ctrl+C + // `killAllSync` force-kill path still has a reference to fire + // SIGKILL against during the SIGTERM grace window — even if a + // concurrent `spawnOrAttach` has already reassigned + // `channelInfo` to a fresh channel. void channel.exited.then((exitInfo) => { - // tanzhenxin BkUyD: drop from `liveChannels` ONLY when the - // OS process is actually gone. Async kill paths - // (`killSession` reap, `shutdown()` await) remove from - // `byWorkspaceChannel` early but the child's SIGTERM grace - // can still be in-flight; the force-kill path needs the - // entry until `channel.exited` fires here. - liveChannels.delete(info); - const stillOurs = byWorkspaceChannel.get(workspaceKey) === info; - if (stillOurs) byWorkspaceChannel.delete(workspaceKey); + aliveChannels.delete(info); + if (channelInfo === info) channelInfo = undefined; const sessions = Array.from(info.sessionIds); info.sessionIds.clear(); + // Operator breadcrumb for UNEXPECTED channel exits. Without + // this an agent crash (OOM / segfault) is invisible from the + // daemon log: each affected SSE subscriber sees a + // `session_died` frame and disconnects, the daemon's + // child-stderr forwarder emits whatever the child wrote before + // dying (often nothing on a SIGKILL / segfault), and operators + // can't tell from `qwen serve`'s own output that the agent + // process is gone. + // + // Suppressed during `shuttingDown` because the operator + // already saw "received SIGINT, draining..." from + // `runQwenServe`'s signal handler. The standalone + // killSession case (last session leaves, channel torn down + // but daemon stays up) still logs — there's no upstream + // context line in that flow, and the message confirms the + // cleanup actually ran. + if (!shuttingDown) { + writeStderrLine( + `qwen serve: channel exited (code=${exitInfo?.exitCode ?? 'none'}, signal=${exitInfo?.signalCode ?? 'none'}, ${sessions.length} session(s) torn down)`, + ); + } for (const sid of sessions) { const sessEntry = byId.get(sid); if (!sessEntry) continue; @@ -1154,33 +1333,79 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { /* bus already closed */ } byId.delete(sid); - if (byWorkspace.get(sessEntry.workspaceCwd) === sessEntry) { - byWorkspace.delete(sessEntry.workspaceCwd); - } + if (defaultEntry === sessEntry) defaultEntry = undefined; sessEntry.events.close(); } }); + // Initialize handshake. The channel is already in + // `aliveChannels` and the `channel.exited` handler above is + // registered, so failure paths (init throw, timeout, late + // shutdown) only need to mark dying + kill — the handler does + // the alive-set cleanup when the OS reaps the child. + try { + await withTimeout( + connection.initialize({ + protocolVersion: PROTOCOL_VERSION, + clientCapabilities: { + fs: { readTextFile: true, writeTextFile: true }, + }, + clientInfo: { name: 'qwen-serve-bridge', version: '0' }, + }), + initTimeoutMs, + 'initialize', + ); + } catch (err) { + // Mark the half-initialized channel as dying/unavailable, then + // kill it. Coalesced callers (`inFlightChannelSpawn` branch in + // `ensureChannel`) observe the same rejection on this promise + // and propagate it to their callers; the `inFlightSpawns` + // tracker is cleared in `spawnOrAttach`'s finally so a follow- + // up call retries cleanly. The `channel.exited` handler + // registered earlier removes `info` from `aliveChannels` once + // the OS reaps the child. `isDying` here is the cross-path + // invariant marker (matches `killSession` / `doSpawn`- + // newSession-failure / `shutdown`): "any channel in + // `aliveChannels` with `isDying === true` is mid-teardown." + info.isDying = true; + await channel.kill().catch(() => {}); + throw err; + } + + // Late-shutdown re-check: if shutdown flipped during the + // handshake, tear this channel down rather than leak past + // `process.exit(0)`. Same cleanup pattern as the init-failure + // path: mark dying + kill, let the exited handler reap. + if (shuttingDown) { + info.isDying = true; + await channel.kill().catch(() => {}); + throw new Error('HttpAcpBridge is shutting down'); + } + + // Handshake succeeded — now publish the channel as the + // attach-available slot. `channelInfo` is assigned LAST so + // `ensureChannel`'s fast-path (`if (channelInfo && !.isDying)`) + // never returns a still-handshaking channel to a concurrent + // caller. + channelInfo = info; return info; })(); - inFlightChannelSpawns.set(workspaceKey, promise); + inFlightChannelSpawn = promise; try { return await promise; } finally { - inFlightChannelSpawns.delete(workspaceKey); + inFlightChannelSpawn = undefined; } } - async function doSpawn( - workspaceKey: string, - modelServiceId?: string, - ): Promise { - // Stage 1.5 multi-session: get-or-create the channel for this - // workspace, then call `connection.newSession()` on it. Sessions - // share the child's process / OAuth / file-cache / hierarchy- - // memory parse via the agent's `sessions: Map` - // (see `acp-integration/acpAgent.ts:194`). + async function doSpawn(modelServiceId?: string): Promise { + // #3803 §02: get-or-create the daemon's single channel, then call + // `connection.newSession()` on it. Sessions share the child's + // process / OAuth / file-cache / hierarchy-memory parse via the + // agent's `sessions: Map` (see + // `acp-integration/acpAgent.ts:194`). + // // newSession on an established channel can fail (auth, config, // etc.) without the channel dying. We DON'T kill the channel on // newSession failure when OTHER sessions are still using it — @@ -1188,17 +1413,17 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // // BkwQA: when the failed newSession was the channel's ONLY // attempt (sessionIds.size === 0), the empty channel must NOT - // linger — it would sit in `byWorkspaceChannel` invisible to + // linger — it would stay set as `channelInfo` invisible to // `sessionCount` / `maxSessions` (both backed by `byId`), and // repeated failing creates would still find this channel via - // `getOrCreateChannel`, never spawning a fresh one. Tear down - // the empty channel so the next attempt gets a clean spawn. - const channelInfo = await getOrCreateChannel(workspaceKey); + // `ensureChannel`, never spawning a fresh one. Tear down the + // empty channel so the next attempt gets a clean spawn. + const ci = await ensureChannel(); let newSessionResp: { sessionId: string }; try { newSessionResp = await withTimeout( - channelInfo.connection.newSession({ - cwd: workspaceKey, + ci.connection.newSession({ + cwd: boundWorkspace, mcpServers: [], }), initTimeoutMs, @@ -1208,11 +1433,15 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // Only reap when this newSession was the channel's first/only // attempt — a populated channel keeps running for its other // live sessions. - if (channelInfo.sessionIds.size === 0) { - if (byWorkspaceChannel.get(workspaceKey) === channelInfo) { - byWorkspaceChannel.delete(workspaceKey); - } - await channelInfo.channel.kill().catch(() => { + if (ci.sessionIds.size === 0) { + // Mark dying SYNCHRONOUSLY so a concurrent `spawnOrAttach` + // calling `ensureChannel()` between this point and the + // `channel.exited` cleanup spawns a fresh channel instead of + // attaching to the one we're about to tear down. `channelInfo` + // stays set until OS reap so `killAllSync` mid-SIGTERM still + // finds a target (tanzhenxin BkUyD invariant). + ci.isDying = true; + await ci.channel.kill().catch(() => { /* best-effort — channel.exited handler still runs */ }); } @@ -1228,9 +1457,9 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { const entry: SessionEntry = { sessionId: newSessionResp.sessionId, - workspaceCwd: workspaceKey, - channel: channelInfo.channel, - connection: channelInfo.connection, + workspaceCwd: boundWorkspace, + channel: ci.channel, + connection: ci.connection, events: new EventBus(), promptQueue: Promise.resolve(), modelChangeQueue: Promise.resolve(), @@ -1238,14 +1467,12 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { attachCount: 0, spawnOwnerWantedKill: false, }; - channelInfo.sessionIds.add(entry.sessionId); + ci.sessionIds.add(entry.sessionId); byId.set(entry.sessionId, entry); - // `byWorkspace` is the single-scope attach lookup — only the - // FIRST session for a workspace wins this slot. Subsequent - // thread-scope sessions don't overwrite it. - if (!byWorkspace.has(workspaceKey)) { - byWorkspace.set(workspaceKey, entry); - } + // `defaultEntry` is the single-scope attach target — only the + // FIRST session wins this slot. Subsequent thread-scope sessions + // don't overwrite it. + if (!defaultEntry) defaultEntry = entry; // ACP `newSession` doesn't take a model id; honor the caller's // `modelServiceId` via `unstable_setSessionModel`. See @@ -1434,10 +1661,28 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { `workspaceCwd must be an absolute path; got "${req.workspaceCwd}"`, ); } - const workspaceKey = canonicalizeWorkspace(req.workspaceCwd); + // Fast-path the common §02 case: clients pre-flight `caps.workspaceCwd` + // and post back the exact same string, so the equality check + // saves a `realpathSync.native` syscall per spawnOrAttach. The + // omit-cwd path in `server.ts` also synthesizes `cwd = + // boundWorkspace` before calling here, so it hits this branch + // too. Falls through to the full canonicalize when the client + // sent a non-canonical alias (`/work/./bound`, mixed casing on + // case-insensitive FS, a symlinked aliased path, …) — that + // still needs the realpath to compare correctly. + const workspaceKey = + req.workspaceCwd === boundWorkspace + ? boundWorkspace + : canonicalizeWorkspace(req.workspaceCwd); + // #3803 §02: reject cross-workspace requests at the daemon + // boundary. The route layer catches `WorkspaceMismatchError` + // and translates to 400 with `code: 'workspace_mismatch'`. + if (workspaceKey !== boundWorkspace) { + throw new WorkspaceMismatchError(boundWorkspace, workspaceKey); + } if (sessionScope === 'single') { - const existing = byWorkspace.get(workspaceKey); + const existing = defaultEntry; if (existing) { // BRSCi: bump attach counter BEFORE any await so the // spawn-owner's disconnect reaper (server.ts: @@ -1511,12 +1756,14 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // continuation running (e.g. channel.exited firing during // a crash spawn, or a direct bridge.killSession call from // outside the route handler). In those cases byId.get() - // returned undefined; we'd otherwise return - // `{ attached: true, sessionId: }` and every - // subsequent prompt/cancel call would 404. Fail loud - // instead so the caller can retry into a fresh spawn. + // returned undefined. Fail loud with a descriptive error + // so the caller can distinguish "immediate agent death" + // from a stale sessionId and retry into a fresh spawn. if (!attachedEntry) { - throw new SessionNotFoundError(session.sessionId); + throw new SessionNotFoundError( + session.sessionId, + 'the agent child likely crashed during initialization — retry to spawn a new session', + ); } if (req.modelServiceId) { // Same swallow as above — we picked up an in-flight @@ -1540,7 +1787,7 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { throw new SessionLimitExceededError(maxSessions); } - const promise = doSpawn(workspaceKey, req.modelServiceId); + const promise = doSpawn(req.modelServiceId); // Track in-flight spawns regardless of scope. Under `single` // this also serves the coalescing path above (a parallel // `spawnOrAttach` finds the entry and waits for the same @@ -1728,7 +1975,16 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { listWorkspaceSessions(workspaceCwd) { if (!path.isAbsolute(workspaceCwd)) return []; - const key = canonicalizeWorkspace(workspaceCwd); + // fast-path: under §02 single-workspace, string equality + // with boundWorkspace avoids a realpathSync syscall on + // every poll. If the literal doesn't match, canonicalize + // to handle symlink aliases; if that still doesn't match, + // this daemon doesn't own the workspace. + const key = + workspaceCwd === boundWorkspace + ? boundWorkspace + : canonicalizeWorkspace(workspaceCwd); + if (key !== boundWorkspace) return []; const out: BridgeSessionSummary[] = []; for (const entry of byId.values()) { if (entry.workspaceCwd === key) { @@ -1850,18 +2106,16 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { entry.spawnOwnerWantedKill = true; return; } - // Remove from the maps eagerly so concurrent `spawnOrAttach` + // Remove from the state eagerly so concurrent `spawnOrAttach` // can't reattach to a session we're tearing down. - if (byWorkspace.get(entry.workspaceCwd) === entry) { - byWorkspace.delete(entry.workspaceCwd); - } + if (defaultEntry === entry) defaultEntry = undefined; byId.delete(sessionId); - // Stage 1.5 multi-session: detach from the channel. The channel - // dies only when its LAST session leaves — other sessions on - // the same channel keep running. - const channelInfo = byWorkspaceChannel.get(entry.workspaceCwd); - if (channelInfo && channelInfo.channel === entry.channel) { - channelInfo.sessionIds.delete(sessionId); + // Detach from the channel. The channel dies only when its LAST + // session leaves — other sessions on the same channel keep + // running. + const ci = channelInfo; + if (ci && ci.channel === entry.channel) { + ci.sessionIds.delete(sessionId); } // Resolve any still-pending permission as cancelled (matches the // shutdown path) so callers awaiting requestPermission unwind. @@ -1889,10 +2143,20 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // 20) so memory is capped. FIXME(stage-1.5): if ACP grows a // `closeSession` notification, send it here so the agent can // drop the entry from its map immediately rather than at - // channel exit. - if (channelInfo && channelInfo.sessionIds.size === 0) { - byWorkspaceChannel.delete(entry.workspaceCwd); - await channelInfo.channel.kill().catch(() => { + // channel exit. (`channelInfo` itself is cleared by the + // `channel.exited` handler once the OS reaps the child — + // tanzhenxin BkUyD invariant.) + if (ci && ci.sessionIds.size === 0) { + // Mark dying SYNCHRONOUSLY before the await so a concurrent + // `spawnOrAttach` arriving during the SIGTERM grace window + // doesn't attach to a transport we're tearing down — without + // this it would land the caller with a sessionId that 404s on + // every follow-up once `channel.exited` fires (the equivalent + // of the pre-PR eager `byWorkspaceChannel.delete()` from the + // Stage 1 routing era). `channelInfo` stays set until OS reap + // so `killAllSync` still finds a target (BkUyD). + ci.isDying = true; + await ci.channel.kill().catch(() => { // Best-effort kill — channel may already be dead. }); } @@ -1933,22 +2197,22 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { }, killAllSync() { - // Bd1y6: synchronous best-effort SIGKILL on every live channel. - // Set `shuttingDown` so any racing async path fails fast. - // tanzhenxin BkUyD fix: iterate `liveChannels` (the OS-level - // source of truth) NOT `byWorkspaceChannel`. The latter is - // cleared by `shutdown()` BEFORE awaiting per-child SIGTERM - // grace; if the operator double-Ctrl+C's during that window, - // iterating `byWorkspaceChannel` would find nothing and - // `process.exit(1)` would orphan children still inside their - // SIGTERM grace. `liveChannels` only loses an entry when - // `channel.exited` fires (OS exit), so the force-kill path - // catches every still-alive child regardless of where the - // graceful drain is. + // Bd1y6: synchronous best-effort SIGKILL on EVERY alive channel + // (typically 1, but during a `killSession`-then-`spawnOrAttach` + // overlap there can be 2 — the dying one in `aliveChannels` + // plus a fresh attach-target in `channelInfo`). Set + // `shuttingDown` so any racing async path fails fast. + // + // tanzhenxin BkUyD: iterate `aliveChannels` (the OS-level "still + // alive" source of truth) — `channelInfo` only points at the + // CURRENT attach target, missing any dying channel whose + // `channel.exited` hasn't fired yet. Without this, a fresh + // spawn overwriting `channelInfo` during the prior channel's + // SIGTERM grace would leave the dying child without SIGKILL + // escalation when `process.exit(1)` fires. shuttingDown = true; - const channels = Array.from(liveChannels); - byWorkspaceChannel.clear(); - byWorkspace.clear(); + const channels = Array.from(aliveChannels); + defaultEntry = undefined; byId.clear(); for (const info of channels) { try { @@ -1966,11 +2230,20 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { // spawning a child this teardown won't see. shuttingDown = true; const entries = Array.from(byId.values()); - // Snapshot channels too — Stage 1.5 multi-session means N - // sessions may share one channel; we tear down channels - // (which transitively takes all their sessions), not entries - // one-by-one. - const channelInfos = Array.from(byWorkspaceChannel.values()); + // Snapshot every alive channel (typically 1; up to 2 during a + // `killSession`-then-`spawnOrAttach` overlap) — entries are + // intentionally NOT removed from `aliveChannels` here; their + // `channel.exited` handlers clear them once the OS has reaped + // each child. That preserves the BkUyD invariant: a + // double-Ctrl+C arriving mid-SIGTERM-grace can still find every + // alive channel via `killAllSync`. Marking each `isDying` makes + // them invisible to any racing `ensureChannel` call — but + // `shuttingDown` already blocks new `spawnOrAttach` upstream, + // so this is mostly belt-and-suspenders (a direct internal + // `ensureChannel` past the gate would still see the dying + // state and not attach). + const channels = Array.from(aliveChannels); + for (const ci of channels) ci.isDying = true; // Resolve every still-pending permission as cancelled before clearing // the maps so callers awaiting `requestPermission` unwind cleanly. for (const e of entries) { @@ -1979,9 +2252,8 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { resolvePending(id, { outcome: { outcome: 'cancelled' } }); } } - byWorkspace.clear(); + defaultEntry = undefined; byId.clear(); - byWorkspaceChannel.clear(); pendingPermissions.clear(); // Publish a terminal `session_died` BEFORE closing each bus so SSE // subscribers can distinguish "daemon shut down" from a transient @@ -2001,16 +2273,15 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { } e.events.close(); } - // Wait for in-flight channel spawns + session spawns. The - // snapshots above only see what's already registered; a doSpawn - // past `newSession()` but pre-`byId.set` is missed, as is a - // `getOrCreateChannel` past `channelFactory()` but pre- - // `byWorkspaceChannel.set`. The late-shutdown re-checks at - // doSpawn/getOrCreateChannel catch both — but without these - // awaits, `bridge.shutdown()` would resolve before they - // finish, and the orphan stderr error from a half-built - // child would fire AFTER the daemon claimed graceful - // shutdown (log-confusing). + // Wait for in-flight channel + session spawns. The snapshot + // above only sees what's already registered; a doSpawn past + // `newSession()` but pre-`byId.set` is missed, as is an + // `ensureChannel` past `channelFactory()` but pre-`channelInfo + // = info`. The late-shutdown re-checks at doSpawn/ensureChannel + // catch both — but without these awaits, `bridge.shutdown()` + // would resolve before they finish, and the orphan stderr + // error from a half-built child would fire AFTER the daemon + // claimed graceful shutdown (log-confusing). const inFlightSessionAwaits = Array.from(inFlightSpawns.values()).map( (p): Promise => p.then( @@ -2018,22 +2289,16 @@ export function createHttpAcpBridge(opts: BridgeOptions = {}): HttpAcpBridge { () => undefined, ), ); - const inFlightChannelAwaits = Array.from( - inFlightChannelSpawns.values(), - ).map( - (p): Promise => - p.then( + const inFlightChannelAwait: Promise = inFlightChannelSpawn + ? inFlightChannelSpawn.then( () => undefined, () => undefined, - ), - ); + ) + : Promise.resolve(); await Promise.all([ - // Kill each unique channel once. With multi-session per - // channel, the same channel object can be referenced by - // multiple entries; `channelInfos` is the deduplicated set. - ...channelInfos.map((ci) => ci.channel.kill().catch(() => {})), + ...channels.map((ci) => ci.channel.kill().catch(() => {})), ...inFlightSessionAwaits, - ...inFlightChannelAwaits, + inFlightChannelAwait, ]); }, }; @@ -2090,12 +2355,14 @@ function sliceLineRange( } /** - * Canonicalize a workspace path so two callers referring to the same - * directory get the same `byWorkspace` key. `path.resolve` alone collapses - * `..` and `.` segments and absolutizes, but on case-insensitive filesystems - * (macOS APFS, Windows NTFS) `/Work/A` and `/work/a` are the same directory - * yet `resolve` returns them verbatim — two `byWorkspace` entries form for - * one physical workspace and `sessionScope: 'single'` silently degrades to + * Canonicalize a workspace path so the boot-time bound path and every + * request's `workspaceCwd` collapse to the same key. `path.resolve` + * alone normalizes `..` and `.` segments and absolutizes, but on + * case-insensitive filesystems (macOS APFS, Windows NTFS) `/Work/A` + * and `/work/a` are the same directory yet `resolve` returns them + * verbatim — without normalization the `boundWorkspace` check would + * reject every request that spelled the path with different casing + * and `sessionScope: 'single'` re-attach would silently degrade to * "one per spelling". * * `realpathSync.native` (when the path exists) walks symlinks and returns @@ -2108,17 +2375,17 @@ function sliceLineRange( * * NOTE: This is a **cross-module contract** (BX9_q) — `config.ts`, * `settings.ts`, `sandbox.ts`, and this file all need to canonicalize - * the same way for `sessionScope: 'single'` re-attach to work - * correctly across paths. The contract: use `realpathSync.native` on - * the resolved absolute path; fall back to `path.resolve` only when - * the path doesn't exist yet. If a future change breaks this - * alignment (e.g. one module starts lowercasing on Windows but this - * one doesn't), `byWorkspace.get()` lookup misses for the same - * physical directory → duplicate sessions silently spawn, and - * `sessionScope: 'single'` degrades to "one per spelling" with no - * error. There's no test that pins the alignment; the integration - * suite would catch a divergence only if it tested the specific - * casing / symlink path the affected module changed. + * the same way for the bound-workspace check + `sessionScope: + * 'single'` re-attach to work correctly across paths. The contract: + * use `realpathSync.native` on the resolved absolute path; fall back + * to `path.resolve` only when the path doesn't exist yet. If a future + * change breaks this alignment (e.g. one module starts lowercasing on + * Windows but this one doesn't), the canonicalized request path + * won't match the canonicalized bound path → every request returns + * `workspace_mismatch` even though the human-readable paths look + * equivalent. There's no test that pins the alignment; the + * integration suite would catch a divergence only if it tested the + * specific casing / symlink path the affected module changed. * * Stage 2 in-process (#3803 §10) collapses the bridge into core, * removing the bridge-side path resolution entirely. Stage 1.5 @@ -2129,7 +2396,7 @@ function sliceLineRange( * call sites to converge. Until then, *any* change to how those * modules resolve workspace paths needs a matching change here. */ -function canonicalizeWorkspace(p: string): string { +export function canonicalizeWorkspace(p: string): string { const resolved = path.resolve(p); try { // FIXME(stage-2): switch to `fs.promises.realpath` once the @@ -2141,8 +2408,19 @@ function canonicalizeWorkspace(p: string): string { // entire bridge-side path resolution anyway, but if Stage 2 // ever lands without that change, switch to the async version. return realpathSync.native(resolved); - } catch { - return resolved; + } catch (err) { + // Only fall back to path.resolve for ENOENT (path doesn't exist + // yet). Other filesystem errors (EACCES, EIO, ELOOP) should + // propagate — swallowing them would hide transient I/O failures + // behind misleading workspace_mismatch rejections. + if ( + err && + typeof err === 'object' && + (err as { code?: unknown }).code === 'ENOENT' + ) { + return resolved; + } + throw err; } } diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index 13bdb7841fc..80c2d2e6a23 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -4,9 +4,15 @@ * SPDX-License-Identifier: Apache-2.0 */ +import * as fs from 'node:fs'; import { type Server } from 'node:http'; +import * as path from 'node:path'; import { writeStderrLine, writeStdoutLine } from '../utils/stdioHelpers.js'; -import { createHttpAcpBridge, type HttpAcpBridge } from './httpAcpBridge.js'; +import { + canonicalizeWorkspace, + createHttpAcpBridge, + type HttpAcpBridge, +} from './httpAcpBridge.js'; import { isLoopbackBind } from './loopbackBinds.js'; import { createServeApp } from './server.js'; import type { ServeOptions } from './types.js'; @@ -100,10 +106,82 @@ export async function runQwenServe( ); } + // Resolve the bound workspace per #3803 §02 (1 daemon = 1 workspace). + // Explicit `--workspace` wins; otherwise default to process.cwd(). + // `POST /session` with a mismatched `cwd` is rejected by the bridge + // with `WorkspaceMismatchError`. Multi-workspace deployments use + // multiple daemon processes, not intra-daemon routing. + // + // Boot-loud validation: absolute path, exists, is a directory. + // Without the stat() check, `canonicalizeWorkspace`'s ENOENT fallback + // to `path.resolve` would let the daemon boot pointed at a + // non-existent directory; every `POST /session` would then spawn a + // `qwen --acp` child with that cwd and the agent would fail with an + // opaque ENOENT — operator pain we can avoid by failing at boot. + const rawWorkspace = opts.workspace ?? process.cwd(); + if (!path.isAbsolute(rawWorkspace)) { + throw new Error( + `Invalid --workspace "${rawWorkspace}": must be an absolute path.`, + ); + } + try { + const stats = fs.statSync(rawWorkspace); + if (!stats.isDirectory()) { + throw new Error( + `Invalid --workspace "${rawWorkspace}": exists but is not a directory.`, + ); + } + } catch (err) { + if (err && typeof err === 'object' && 'code' in err) { + const code = (err as { code?: unknown }).code; + if (code === 'ENOENT') { + throw new Error( + `Invalid --workspace "${rawWorkspace}": directory does not exist.`, + ); + } + // EACCES / EPERM: the path exists but the current user can't + // stat it (typical for SIP-protected paths on macOS, root-owned + // dirs the daemon's user can't traverse, etc.). The raw Node + // SystemError has the path AND the syscall but no operator- + // facing breadcrumb that this came from `--workspace`. Wrap + // both codes so the boot failure points at the flag the + // operator actually set. + if (code === 'EACCES' || code === 'EPERM') { + throw new Error( + `Invalid --workspace "${rawWorkspace}": permission denied ` + + `(${String(code)}). The path exists but cannot be stat'd ` + + `by the current user.`, + ); + } + } + throw err; + } + // Canonicalize ONCE here so `/capabilities` and the POST /session + // fallback (both via server.ts) AND the bridge agree on the same + // path. Without this, server.ts and the bridge each compute + // `boundWorkspace` independently; on symlinks or case-insensitive + // filesystems the bridge's `realpathSync.native` form diverges from + // server.ts's raw `opts.workspace` and clients see one path on + // `/capabilities` but another on `POST /session` responses. + const boundWorkspace = canonicalizeWorkspace(rawWorkspace); const bridge = - deps.bridge ?? createHttpAcpBridge({ maxSessions: opts.maxSessions }); + deps.bridge ?? + createHttpAcpBridge({ + maxSessions: opts.maxSessions, + boundWorkspace, + }); let actualPort = opts.port; - const app = createServeApp(opts, () => actualPort, { bridge }); + // Pass the already-canonical `boundWorkspace` into `createServeApp` + // via `deps.boundWorkspace`. That field is the pre-canonicalized + // fast-path: createServeApp skips its own `canonicalizeWorkspace` + // call (which would issue a redundant `realpathSync.native` + // syscall — idempotent but unnecessary I/O at boot). Direct + // callers of createServeApp (tests / embeds) omit it and the + // server canonicalizes itself. + const app = createServeApp(opts, () => actualPort, { + bridge, + boundWorkspace, + }); // Node's `app.listen()` wants the unbracketed IPv6 literal (`::1`) but // operators conventionally type `[::1]` (or copy/paste from URLs that @@ -188,7 +266,25 @@ export async function runQwenServe( const addr = server.address(); actualPort = typeof addr === 'object' && addr ? addr.port : opts.port; const url = `http://${formatHostForUrl(opts.hostname)}:${actualPort}`; - writeStdoutLine(`qwen serve listening on ${url} (mode=${opts.mode})`); + writeStdoutLine( + `qwen serve listening on ${url} (mode=${opts.mode}, ` + + `workspace=${boundWorkspace})`, + ); + // Operator log on stderr too (systemd/docker/k8s default + // captures only stderr for service diagnostics, and the + // workspace= breadcrumb is the single piece of information + // operators need most when triaging §02 migration issues — + // "did the daemon bind to the right workspace?"). The stdout + // line above stays put so integration tests + scripts that + // parse stdout for the listening URL keep working; + // `JSON.stringify(boundWorkspace)` quotes the value + // symmetrically with the workspace_mismatch log (defends + // against control-char log injection if `boundWorkspace` + // somehow contained one — operator-controlled today, but + // cheap defense-in-depth). + writeStderrLine( + `qwen serve: bound to workspace ${JSON.stringify(boundWorkspace)}`, + ); if (!token) { writeStderrLine( `qwen serve: bearer auth disabled (loopback default). Set ${QWEN_SERVER_TOKEN_ENV} to enable.`, diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index c9ec984b3d8..1c916d271b9 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -4,6 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { realpathSync } from 'node:fs'; +import * as path from 'node:path'; +import { fileURLToPath } from 'node:url'; import { describe, it, expect, afterEach, vi } from 'vitest'; import request from 'supertest'; import { createServeApp } from './server.js'; @@ -20,6 +23,7 @@ import { InvalidPermissionOptionError, SessionLimitExceededError, SessionNotFoundError, + WorkspaceMismatchError, type BridgeSession, type BridgeSessionSummary, type BridgeSpawnRequest, @@ -38,6 +42,16 @@ const baseOpts: ServeOptions = { mode: 'http-bridge', }; +// Workspace fixtures must round-trip through `path.resolve` so the +// expected values match the canonicalized form the route produces on +// every platform. On Windows `path.resolve('/work/bound')` returns +// `D:\work\bound` (drive-relative absolute), so hardcoding `/work/bound` +// as a literal makes the test fail on Windows CI even though the code +// is correct. Mirror the pattern used by httpAcpBridge.test.ts (WS_A / +// WS_B). +const WS_BOUND = path.resolve(path.sep, 'work', 'bound'); +const WS_DIFFERENT = path.resolve(path.sep, 'work', 'different'); + interface FakeBridgeOpts { spawnImpl?: (req: BridgeSpawnRequest) => Promise; promptImpl?: ( @@ -199,6 +213,30 @@ describe('createServeApp', () => { expect(res.body.features).toEqual([...STAGE1_FEATURES]); expect(res.body.modelServices).toEqual([]); }); + + it('reports the bound workspace (#3803 §02)', async () => { + const app = createServeApp({ ...baseOpts, workspace: WS_BOUND }); + const res = await request(app) + .get('/capabilities') + .set('Host', `127.0.0.1:${baseOpts.port}`); + expect(res.status).toBe(200); + expect(res.body.workspaceCwd).toBe(WS_BOUND); + }); + + it('falls back to process.cwd() when --workspace is omitted', async () => { + const app = createServeApp(baseOpts); + const res = await request(app) + .get('/capabilities') + .set('Host', `127.0.0.1:${baseOpts.port}`); + expect(res.status).toBe(200); + // `createServeApp` runs `canonicalizeWorkspace` on + // `process.cwd()`, which collapses symlinks via + // `realpathSync.native`. On macOS the default tmpdir is + // `/var/folders/...` whose canonical form is + // `/private/var/folders/...`; a raw `process.cwd()` assertion + // would diverge there. Use the same realpath the route does. + expect(res.body.workspaceCwd).toBe(realpathSync.native(process.cwd())); + }); }); describe('host allowlist (loopback bind)', () => { @@ -277,20 +315,31 @@ describe('createServeApp', () => { }); describe('POST /session', () => { - it('400 when cwd is missing', async () => { + it('200 when cwd is omitted (falls back to bound workspace, #3803 §02)', async () => { + // 1 daemon = 1 workspace: the daemon binds to + // `opts.workspace ?? process.cwd()` at boot, so clients may + // omit `cwd` and the route falls back to the bound path. const bridge = fakeBridge(); - const app = createServeApp(baseOpts, undefined, { bridge }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); const res = await request(app) .post('/session') .set('Host', `127.0.0.1:${baseOpts.port}`) .send({}); - expect(res.status).toBe(400); - expect(bridge.calls).toHaveLength(0); + expect(res.status).toBe(200); + expect(bridge.calls[0]?.workspaceCwd).toBe(WS_BOUND); }); it('400 when cwd is relative', async () => { const bridge = fakeBridge(); - const app = createServeApp(baseOpts, undefined, { bridge }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); const res = await request(app) .post('/session') .set('Host', `127.0.0.1:${baseOpts.port}`) @@ -299,6 +348,115 @@ describe('createServeApp', () => { expect(bridge.calls).toHaveLength(0); }); + it('400 when cwd is present but not a string (#3803 §02 — distinguishes omitted vs malformed)', async () => { + // Three non-string shapes a buggy client / orchestrator could + // serialize for the `cwd` field: `null`, a number, an object. + // Pre-fix the route treated all three the same as "omitted" and + // fell back to `boundWorkspace`, silently masking client bugs. + // Now the route distinguishes "absent" (legitimate §02 fallback) + // from "present but malformed" (client-side bug → 400 + actionable + // error message). Empty string still falls through to the + // `path.isAbsolute` check (and 400s there with the + // "absolute path when provided" message). + const malformed: unknown[] = [null, 123, { foo: 'bar' }, []]; + for (const cwd of malformed) { + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); + const res = await request(app) + .post('/session') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/must be a string absolute path/); + // Bridge must NOT be touched — silent fallback regressions + // would otherwise let the malformed input hit `spawnOrAttach`. + expect(bridge.calls).toHaveLength(0); + } + }); + + it('400 when cwd is the empty string', async () => { + // Empty string is technically a string so the type-check above + // lets it through; `path.isAbsolute('')` is false so the + // "must be an absolute path when provided" branch catches it. + // Important: the `'cwd' in body` presence test means an empty + // string is NOT treated as omitted (which would fall back to + // boundWorkspace) — empty-string is the strongest "client + // explicitly passed nothing useful" signal we have. + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); + const res = await request(app) + .post('/session') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd: '' }); + expect(res.status).toBe(400); + expect(bridge.calls).toHaveLength(0); + }); + + it('400 when cwd exceeds MAX_WORKSPACE_PATH_LENGTH (memory amplification guard)', async () => { + // Real filesystem paths fit well under PATH_MAX (4096 on Linux). + // A multi-MB `cwd` is either a malformed client or a memory- + // amplification attempt — `WorkspaceMismatchError` interpolates + // `requested` into `.message` twice, `sendBridgeError` writes it + // to stderr, and `res.json` echoes it again, so a ~10 MB body + // (right under express.json's 10 MB cap) would amplify to + // ~60 MB/request × maxConnections. The route caps the input + // before any of those echoes. + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); + // Build an absolute path of MAX+1 chars. `path.isAbsolute` + // sees the leading `/` and the length cap fires before the + // isAbsolute branch — verifying both invariants in one go. + const longCwd = `/${'a'.repeat(4096)}`; + const res = await request(app) + .post('/session') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd: longCwd }); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/exceeds the 4096-character limit/); + // Bridge must NOT be touched — silent fallback or pass-through + // would defeat the cap. + expect(bridge.calls).toHaveLength(0); + }); + + it('400 workspace_mismatch when bridge rejects cross-workspace cwd (#3803 §02)', async () => { + // Single-workspace mode: bridge throws WorkspaceMismatchError + // when the route forwards a non-bound cwd. Route translates + // to 400 with code `workspace_mismatch` + both paths in the + // body so orchestrator-aware clients can route correctly. + const bridge = fakeBridge({ + spawnImpl: async (req) => { + throw new WorkspaceMismatchError(WS_BOUND, req.workspaceCwd); + }, + }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge }, + ); + const res = await request(app) + .post('/session') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({ cwd: WS_DIFFERENT }); + expect(res.status).toBe(400); + expect(res.body).toMatchObject({ + code: 'workspace_mismatch', + boundWorkspace: WS_BOUND, + requestedWorkspace: WS_DIFFERENT, + }); + }); + it('200 with the BridgeSession shape on success', async () => { const bridge = fakeBridge(); const app = createServeApp(baseOpts, undefined, { bridge }); @@ -503,31 +661,66 @@ describe('createServeApp', () => { describe('GET /workspace/:id/sessions', () => { it('returns the list returned by the bridge', async () => { + // #3803 §02 (commit 0c6e963cd): the route now rejects + // cross-workspace queries with 400 workspace_mismatch (so + // orchestrators don't mistake "no sessions here" for + // "workspace is idle"). Bind the daemon to the same workspace + // we'll query so the happy path runs. const bridge = fakeBridge({ listImpl: () => [ - { sessionId: 's-1', workspaceCwd: '/work/a' }, - { sessionId: 's-2', workspaceCwd: '/work/a' }, + { sessionId: 's-1', workspaceCwd: WS_BOUND }, + { sessionId: 's-2', workspaceCwd: WS_BOUND }, ], }); - const app = createServeApp(baseOpts, undefined, { bridge }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge, boundWorkspace: WS_BOUND }, + ); const res = await request(app) - .get(`/workspace/${encodeURIComponent('/work/a')}/sessions`) + .get(`/workspace/${encodeURIComponent(WS_BOUND)}/sessions`) .set('Host', `127.0.0.1:${baseOpts.port}`); expect(res.status).toBe(200); expect(res.body.sessions).toHaveLength(2); - expect(bridge.listCalls).toEqual(['/work/a']); + expect(bridge.listCalls).toEqual([WS_BOUND]); }); it('returns an empty array when no sessions exist for the workspace', async () => { const bridge = fakeBridge(); - const app = createServeApp(baseOpts, undefined, { bridge }); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge, boundWorkspace: WS_BOUND }, + ); const res = await request(app) - .get(`/workspace/${encodeURIComponent('/work/idle')}/sessions`) + .get(`/workspace/${encodeURIComponent(WS_BOUND)}/sessions`) .set('Host', `127.0.0.1:${baseOpts.port}`); expect(res.status).toBe(200); expect(res.body).toEqual({ sessions: [] }); }); + it('400 workspace_mismatch when querying a cross-workspace path (#3803 §02)', async () => { + // Pin the §02 cross-workspace rejection: querying any path + // that doesn't canonicalize to the bound workspace gets a 400 + // with `code: 'workspace_mismatch'` and both paths in the + // body — so an orchestrator-aware client can route to / spawn + // the right daemon. The bridge MUST NOT be touched (a silent + // fallback would defeat the whole purpose of §02). + const bridge = fakeBridge(); + const app = createServeApp( + { ...baseOpts, workspace: WS_BOUND }, + undefined, + { bridge, boundWorkspace: WS_BOUND }, + ); + const res = await request(app) + .get(`/workspace/${encodeURIComponent(WS_DIFFERENT)}/sessions`) + .set('Host', `127.0.0.1:${baseOpts.port}`); + expect(res.status).toBe(400); + expect(res.body.code).toBe('workspace_mismatch'); + expect(res.body.boundWorkspace).toBe(WS_BOUND); + expect(bridge.listCalls).toHaveLength(0); + }); + it('400 when :id does not decode to an absolute path', async () => { const bridge = fakeBridge(); const app = createServeApp(baseOpts, undefined, { bridge }); @@ -1100,6 +1293,83 @@ describe('runQwenServe', () => { ).rejects.toThrow(/Invalid --hostname/); }); + it('--workspace flows end-to-end and surfaces on /capabilities (#3803 §02)', async () => { + // Use process.cwd() so the boot-time existence check passes — any + // real absolute directory works. The bridge canonicalizes this + // once at boot; `/capabilities.workspaceCwd` returns the canonical + // form, NOT the raw input. Tests inject a fake bridge here so we + // verify the route layer's canonicalization (not the bridge's), + // making this a true E2E that doesn't require a real `qwen --acp` + // child. + const bridge = fakeBridge(); + handle = await runQwenServe( + { + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + workspace: process.cwd(), + }, + { bridge }, + ); + const port = (handle.server.address() as { port: number }).port; + const caps = await ( + await fetch(`http://127.0.0.1:${port}/capabilities`) + ).json(); + // Canonical form per `canonicalizeWorkspace` — realpath of cwd + // (handles symlinks like `/var` → `/private/var` on macOS). + const expected = await import('node:fs').then((m) => + m.realpathSync.native(process.cwd()), + ); + expect(caps.workspaceCwd).toBe(expected); + }); + + it('rejects --workspace pointing at a non-existent directory (BkUyD followup — boot-loud over opaque ENOENT)', async () => { + // Without the boot-time stat check, `canonicalizeWorkspace`'s + // ENOENT fallback to `path.resolve` would let the daemon boot + // pointed at a non-existent directory; every `POST /session` + // would then spawn a `qwen --acp` child with that cwd and the + // agent would fail with an opaque ENOENT. + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + workspace: `/tmp/qwen-serve-no-such-path-${Date.now()}-${Math.random().toString(36).slice(2)}`, + }), + ).rejects.toThrow(/directory does not exist/); + }); + + it('rejects --workspace pointing at a regular file', async () => { + // Pointing the daemon at a file (vs. a directory) is operator error + // — the agent would fail at child-spawn time with ENOTDIR. Catch + // it at boot for a clearer error message. + // + // `fileURLToPath` (not `new URL(...).pathname`) — on Windows the + // latter returns `/C:/path/...` with a leading slash, which + // `statSync` resolves as path-from-current-drive-root and the + // test would then see ENOENT instead of the expected + // "not a directory" branch. + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + workspace: fileURLToPath(import.meta.url), + }), + ).rejects.toThrow(/exists but is not a directory/); + }); + + it('rejects relative --workspace at boot', async () => { + await expect( + runQwenServe({ + hostname: '127.0.0.1', + port: 0, + mode: 'http-bridge', + workspace: 'relative/path', + }), + ).rejects.toThrow(/must be an absolute path/); + }); + it('drains the bridge before closing the listener', async () => { const bridge = fakeBridge(); handle = await runQwenServe( diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 456b2ffe8aa..39cb996bde3 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -11,10 +11,13 @@ import { writeStderrLine } from '../utils/stdioHelpers.js'; import { bearerAuth, denyBrowserOriginCors, hostAllowlist } from './auth.js'; import { isLoopbackBind } from './loopbackBinds.js'; import { + canonicalizeWorkspace, createHttpAcpBridge, InvalidPermissionOptionError, + MAX_WORKSPACE_PATH_LENGTH, SessionLimitExceededError, SessionNotFoundError, + WorkspaceMismatchError, type HttpAcpBridge, } from './httpAcpBridge.js'; import { SubscriberLimitExceededError, type BridgeEvent } from './eventBus.js'; @@ -28,6 +31,19 @@ import { export interface ServeAppDeps { /** Bridge instance; tests inject a fake. Defaults to a fresh real one. */ bridge?: HttpAcpBridge; + /** + * Pre-canonicalized workspace path. When supplied, `createServeApp` + * skips its own `canonicalizeWorkspace` call (which would issue a + * redundant `realpathSync.native` syscall — idempotent, but a hot + * boot-time stat we can avoid). `runQwenServe` passes this after + * its own boot-time canonicalize so the value used by + * `/capabilities`, the `POST /session` cwd fallback, and the + * bridge are all the SAME canonical form. Callers that haven't + * canonicalized yet (tests, direct embeds) omit this and + * `createServeApp` falls back to canonicalizing `opts.workspace ?? + * process.cwd()` itself. + */ + boundWorkspace?: string; } /** @@ -49,6 +65,22 @@ export interface ServeAppDeps { * - `POST /session/:id/model` * - `GET /session/:id/events` (SSE) * - `POST /permission/:requestId` + * + * **Workspace validation contract.** `createServeApp` itself does NOT + * verify that `opts.workspace` exists or is a directory — it + * canonicalizes via `canonicalizeWorkspace`, which falls back to + * `path.resolve` on ENOENT so the app boots even against a missing + * path. `runQwenServe` is the production entry point and DOES + * perform the `fs.statSync` + `isDirectory()` boot-loud check before + * calling this function. Tests inject synthetic paths (`/work/bound` + * etc.) on purpose: they want to exercise the route layer's + * canonicalization and `workspace_mismatch` translation without + * needing a real directory on disk. If a future entry point binds + * `createServeApp` directly to user input, it MUST replicate the + * `runQwenServe` validation (or call into a shared helper if one is + * extracted) — otherwise a non-existent `--workspace` would boot + * a "healthy"-looking daemon whose every spawn fails with cryptic + * child-process ENOENT. */ export function createServeApp( opts: ServeOptions, @@ -61,8 +93,34 @@ export function createServeApp( // cap they configured via `ServeOptions`. Previously the default // bridge silently fell back to `DEFAULT_MAX_SESSIONS` (20) and // only the `runQwenServe` path piped the option through. + // + // Workspace binding mirrors `runQwenServe`: per #3803 §02 the + // daemon is bound to exactly one workspace (`opts.workspace` or + // `process.cwd()`). `POST /session` with a mismatched cwd is + // rejected with 400 `workspace_mismatch`. + // + // The value advertised on `/capabilities`, used for the `POST + // /session` cwd fallback, AND passed into the bridge must be the + // SAME canonical form — otherwise the bridge's + // `realpathSync.native` would diverge from what `/capabilities` + // shows on symlinks / case-insensitive filesystems, and clients + // echoing the advertised path back would see a response whose + // `workspaceCwd` differs from what they sent. + // + // `deps.boundWorkspace` is the pre-canonicalized fast-path — + // `runQwenServe` passes it after its own boot-time + // `canonicalizeWorkspace`, so we skip the redundant + // `realpathSync.native` here. When omitted (tests, direct embeds) + // we canonicalize ourselves. + const boundWorkspace = + deps.boundWorkspace ?? + canonicalizeWorkspace(opts.workspace ?? process.cwd()); const bridge = - deps.bridge ?? createHttpAcpBridge({ maxSessions: opts.maxSessions }); + deps.bridge ?? + createHttpAcpBridge({ + maxSessions: opts.maxSessions, + boundWorkspace, + }); // Order matters: rejection guards (CORS / Host allowlist / bearer auth) // run BEFORE the JSON body parser. Otherwise an unauthenticated POST @@ -142,17 +200,64 @@ export function createServeApp( mode: opts.mode, features: [...STAGE1_FEATURES], modelServices: [], + // #3803 §02: surface the bound workspace so clients can detect + // mismatch pre-flight and omit `cwd` on `POST /session`. + workspaceCwd: boundWorkspace, }; res.status(200).json(envelope); }); app.post('/session', async (req, res) => { const body = safeBody(req); - const cwd = typeof body['cwd'] === 'string' ? (body['cwd'] as string) : ''; - if (!cwd || !path.isAbsolute(cwd)) { + // #3803 §02: 1 daemon = 1 workspace. Three input shapes: + // - `cwd` ABSENT from body → fall back to the daemon's bound + // workspace (the §02 documented shape — clients pre-flight + // `caps.workspaceCwd` and may then omit `cwd`). + // - `cwd` PRESENT but not a string → 400 malformed. A + // client/orchestrator serialization bug (`cwd: null`, + // `cwd: 123`, `cwd: {}`) must not silently bind a session + // to the daemon's workspace; surface the bug instead. + // - `cwd` PRESENT as a string → fall through to the + // `path.isAbsolute` check (empty string and relative both + // fail there with "must be an absolute path when provided"). + // + // `safeBody` returns an `Object.create(null)` map, so + // `'cwd' in body` reflects exactly "did the client send the + // key?" without prototype-chain confusion. The presence-check + // is safe as long as `PROTOTYPE_POLLUTION_KEYS` doesn't grow to + // include `cwd` — see the cross-reference in the const's JSDoc + // for what to do if that invariant ever has to break. + const hasCwd = 'cwd' in body; + if (hasCwd && typeof body['cwd'] !== 'string') { res .status(400) - .json({ error: '`cwd` is required and must be an absolute path' }); + .json({ error: '`cwd` must be a string absolute path when provided' }); + return; + } + // Length cap BEFORE assignment so a multi-MB `cwd` body can't + // amplify through downstream interpolations + // (`WorkspaceMismatchError`'s `.message` echoes `requested` twice; + // `sendBridgeError` writes it to stderr; `res.json` echoes it + // again). On the loopback-default-no-token deployment shape this + // is pre-auth, so a 10 MB cwd body — right under + // `express.json({limit: '10mb'})` — would otherwise cost + // ~60 MB per request × `maxConnections` (default 256). The + // `MAX_WORKSPACE_PATH_LENGTH` constant matches Linux's PATH_MAX + // (4096); legitimate filesystem paths fit well under it. The + // `WorkspaceMismatchError` constructor also truncates as a + // belt-and-suspenders defense for non-route callers (tests, + // embeds, future entry points that throw the error directly). + if (hasCwd && (body['cwd'] as string).length > MAX_WORKSPACE_PATH_LENGTH) { + res.status(400).json({ + error: `\`cwd\` exceeds the ${MAX_WORKSPACE_PATH_LENGTH}-character limit`, + }); + return; + } + const cwd = hasCwd ? (body['cwd'] as string) : boundWorkspace; + if (!path.isAbsolute(cwd)) { + res + .status(400) + .json({ error: '`cwd` must be an absolute path when provided' }); return; } const modelServiceId = @@ -166,7 +271,7 @@ export function createServeApp( }); // Client may have disconnected during the 1–3s spawn window. If // so, the response can't be delivered. The session is otherwise - // orphaned (in `byId` / `byWorkspace` with no client knowing the + // orphaned (in `byId` / `defaultEntry` with no client knowing the // id), and under churn this leaks one child per aborted request. // // Detect "can we still write the response?" via `res.writable`, @@ -349,6 +454,18 @@ export function createServeApp( .json({ error: '`:id` must decode to an absolute workspace path' }); return; } + // #3803 §02: reject cross-workspace queries so orchestrators + // don't mistake "no sessions here" for "workspace is idle". + const key = canonicalizeWorkspace(workspaceCwd); + if (key !== boundWorkspace) { + res.status(400).json({ + error: `Workspace mismatch: daemon is bound to "${boundWorkspace}"`, + code: 'workspace_mismatch', + boundWorkspace, + requestedWorkspace: key, + }); + return; + } const sessions = bridge.listWorkspaceSessions(workspaceCwd); res.status(200).json({ sessions }); }); @@ -680,19 +797,40 @@ export function createServeApp( return app; } +/** + * Keys stripped by `safeBody` to defend against prototype-pollution + * — see BZ9uv/va/vs/wD/Bd1zz. Routes downstream of `safeBody` spread + * the filtered result into objects passed to the bridge / ACP SDK; + * without this scrub a client could set + * `{"__proto__": {"polluted": true}}` and pollute + * `Object.prototype` via downstream spreads. + * + * **Cross-reference for route maintainers:** the POST `/session` + * route distinguishes "absent" from "present" via `'cwd' in body` + * against `safeBody`'s output. The semantics rely on this set NOT + * overlapping with user-payload keys. If you ever add a key here + * that a route's presence-check cares about (highly unlikely — this + * set is the JS prototype-attack triple, plus a route would have + * to deliberately name a property after one of these), the + * presence-check needs to move to the pre-`safeBody` `req.body` + * (with its own pollution guard) or `safeBody` needs to return a + * separate "raw-keys" set alongside the filtered object. + */ +const PROTOTYPE_POLLUTION_KEYS: ReadonlySet = new Set([ + '__proto__', + 'constructor', + 'prototype', +]); + /** * Coerce `req.body` into a safe `Record` for route * handlers. Replaces the 5-site copy-pasted preamble * `typeof req.body === 'object' && req.body !== null ? ... : {}` * (Bd10m). * - * Also strips prototype-pollution keys (`__proto__`, `constructor`, - * `prototype`) before returning — see BZ9uv/va/vs/wD/Bd1zz. Routes - * downstream of this helper spread the result into objects passed to - * the bridge / ACP SDK; without this scrub, a client could set - * `{"__proto__": {"polluted": true}}` and pollute `Object.prototype`. - * Uses an `Object.create(null)` target so the returned object itself - * has no prototype either, blocking second-order spread-into-default- + * Strips the `PROTOTYPE_POLLUTION_KEYS` set before returning. Uses an + * `Object.create(null)` target so the returned object itself has no + * prototype either, blocking second-order spread-into-default- * prototype attacks. */ function safeBody(req: import('express').Request): Record { @@ -702,9 +840,7 @@ function safeBody(req: import('express').Request): Record { } const out = Object.create(null) as Record; for (const [key, value] of Object.entries(raw as Record)) { - if (key === '__proto__' || key === 'constructor' || key === 'prototype') { - continue; - } + if (PROTOTYPE_POLLUTION_KEYS.has(key)) continue; out[key] = value; } return out; @@ -801,6 +937,48 @@ function sendBridgeError( res.status(404).json({ error: err.message, sessionId: err.sessionId }); return; } + if (err instanceof WorkspaceMismatchError) { + // #3803 §02 single-workspace mode: the daemon binds to one + // workspace at boot; cross-workspace POSTs are rejected here. + // 400 (not 404 — the daemon is "fine", the client just picked + // the wrong daemon for their workspace). Body includes both + // paths so orchestrator-aware clients can route to the right + // daemon / spawn a new one. + // + // Operator log line: unlike SessionNotFoundError (per-session + // 404 with rich URL context), workspace_mismatch indicates an + // orchestration / deployment drift (operator booted with the + // wrong workspace, or client is routing to the wrong daemon). + // Without a breadcrumb the daemon's log looks healthy while + // every client request silently 400s. Limited to authenticated + // requests by the upstream bearer-token gate, so probing-DoS + // log noise stays bounded. + // SECURITY: `err.requested` is derived from the request body + // (`req.workspaceCwd` → `canonicalizeWorkspace` → here). `path.resolve` + // + `realpathSync.native` both preserve control characters inside + // path segments — they only normalize separators / `..` / `.` and + // walk symlinks. A body like `{"cwd": "/legit/path\nqwen serve: + // FAKE LOG LINE"}` would otherwise emit two valid-looking daemon + // log lines, weaponizing line-based log shippers (Splunk / Loki / + // journald → SIEM). `JSON.stringify` escapes control chars and + // wraps in quotes so any injection attempt surfaces as + // visible-as-quoted-noise rather than forged-line. `err.bound` is + // safe (canonicalized at boot from operator-controlled + // `--workspace` / `process.cwd()`) but quoted symmetrically for + // readability. + writeStderrLine( + `qwen serve: workspace_mismatch (POST /session): ` + + `daemon bound to ${JSON.stringify(err.bound)}, ` + + `rejected ${JSON.stringify(err.requested)}`, + ); + res.status(400).json({ + error: err.message, + code: 'workspace_mismatch', + boundWorkspace: err.bound, + requestedWorkspace: err.requested, + }); + return; + } if (err instanceof SessionLimitExceededError) { // 503 Service Unavailable + `Retry-After` is the canonical // "we'd serve you, but we're full right now" shape. The hint diff --git a/packages/cli/src/serve/types.ts b/packages/cli/src/serve/types.ts index 11a4b8e9c76..19ccdf4c3e9 100644 --- a/packages/cli/src/serve/types.ts +++ b/packages/cli/src/serve/types.ts @@ -7,14 +7,15 @@ /** * Stage 1 daemon mode shape. * - * `http-bridge` (Stage 1): one `qwen --acp` child PER WORKSPACE, with - * multiple sessions multiplexed onto that child via the agent's native - * `connection.newSession()` (see `acp-integration/acpAgent.ts:194`). - * Sessions on the same workspace share the child's process / OAuth / - * file-cache / hierarchy-memory parse. The daemon pipes ACP NDJSON over - * HTTP/SSE. Same-session multi-client requests serialize through the - * bridge's per-session FIFO; cross-session requests on the same channel - * can run concurrently (the ACP layer demultiplexes by sessionId). + * `http-bridge` (Stage 1): per #3803 §02, one `qwen --acp` child per + * daemon (the daemon binds to ONE workspace at boot). Multiple + * sessions multiplex onto that child via the agent's native + * `connection.newSession()` (see `acp-integration/acpAgent.ts:194`), + * sharing the child's process / OAuth / file-cache / hierarchy-memory + * parse. The daemon pipes ACP NDJSON over HTTP/SSE. Same-session + * multi-client requests serialize through the bridge's per-session + * FIFO; cross-session requests on the same channel can run + * concurrently (the ACP layer demultiplexes by sessionId). * `native` (Stage 2+): in-process multi-session, AsyncLocalStorage; not yet * implemented. */ @@ -56,6 +57,26 @@ export interface ServeOptions { * (default cap 64) plus short-lived REST calls. */ maxConnections?: number; + /** + * Absolute workspace path this daemon binds to. Per #3803 §02 the + * daemon is **1 daemon = 1 workspace × N sessions**: one bound + * workspace at boot, sessions multiplexed on the single + * `qwen --acp` child via `connection.newSession()`. + * + * `POST /session` calls whose `cwd` doesn't canonicalize to this + * path are rejected with `400 workspace_mismatch`. Clients may + * also omit `cwd` — the route falls back to this bound path. + * + * Multi-workspace deployments use **multiple daemon processes** + * (one per workspace, each on its own port), supervised by + * systemd / docker-compose / k8s / `qwen-coordinator` reference + * orchestrator. There is no intra-daemon multi-workspace mode + * (the previous Stage 1 `byWorkspaceChannel` routing layer was + * removed in the §02 design revision). + * + * Defaults to `process.cwd()` when omitted. + */ + workspace?: string; } /** @@ -77,6 +98,23 @@ export interface CapabilitiesEnvelope { * this field being non-empty. */ modelServices: string[]; + /** + * Absolute workspace path this daemon is bound to (per #3803 §02: + * `1 daemon = 1 workspace`). Clients use this to: + * - Detect mismatch before posting `/session` (vs. waiting for + * 400 workspace_mismatch from the bridge). + * - Omit `cwd` on `POST /session` — the route falls back to this + * path when the body has no `cwd` field. + * + * Optional at the type level (matches the SDK's `DaemonCapabilities` + * type) because the field is an additive extension of the v=1 + * envelope introduced by #3803 §02. Daemons predating §02 still + * announce `v: 1` and omit this field; the protocol's "bump v only + * on incompatible frame changes" stance (see `qwen-serve-protocol.md` + * "Additive to v=1" note) makes additive optionality the correct + * shape. The post-§02 server code here always populates it. + */ + workspaceCwd?: string; } export const CAPABILITIES_SCHEMA_VERSION = 1 as const; diff --git a/packages/sdk-typescript/src/daemon/DaemonClient.ts b/packages/sdk-typescript/src/daemon/DaemonClient.ts index 3983c63af9e..c96ef245642 100644 --- a/packages/sdk-typescript/src/daemon/DaemonClient.ts +++ b/packages/sdk-typescript/src/daemon/DaemonClient.ts @@ -88,7 +88,16 @@ export class DaemonHttpError extends Error { } export interface CreateSessionRequest { - workspaceCwd: string; + /** + * Workspace path the daemon must be bound to (per #3803 §02). When + * omitted, the SDK sends no `cwd` field and the daemon route falls + * back to its boot-time `boundWorkspace`. Pass `caps.workspaceCwd` + * to be explicit, or omit it for the daemon-knows-best path. A + * non-empty `workspaceCwd` that doesn't canonicalize to the + * daemon's bound path yields a `400 workspace_mismatch` + * `DaemonHttpError`. + */ + workspaceCwd?: string; modelServiceId?: string; } @@ -250,6 +259,18 @@ export class DaemonClient { async createOrAttachSession( req: CreateSessionRequest, ): Promise { + // Per #3803 §02: omitting `cwd` lets the daemon fall back to its + // bound workspace. JSON.stringify strips `undefined` values, so + // `cwd: undefined` becomes "no `cwd` key" on the wire — and the + // server then takes the documented fallback path. + // + // Send EVERY defined `workspaceCwd` value through as-is, including + // the empty string. A truthy guard would silently swallow + // `workspaceCwd: ""` (a likely client-side bug) and let the server + // fall back instead of returning a clear 400 for the malformed + // input. The SDK should be a transparent layer here: passing the + // caller's value verbatim lets the server's validation surface + // bugs that would otherwise hide as "wrong workspace bound". return await this.fetchWithTimeout( `${this.baseUrl}/session`, { diff --git a/packages/sdk-typescript/src/daemon/index.ts b/packages/sdk-typescript/src/daemon/index.ts index c463fed9280..68dab9fd332 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -13,6 +13,10 @@ export { type SubscribeOptions, } from './DaemonClient.js'; export { parseSseStream, SseFramingError } from './sse.js'; +export { + DaemonCapabilityMissingError, + requireWorkspaceCwd, +} from './types.js'; export type { DaemonCapabilities, DaemonEvent, diff --git a/packages/sdk-typescript/src/daemon/types.ts b/packages/sdk-typescript/src/daemon/types.ts index 29d49c0da82..7114b0f6670 100644 --- a/packages/sdk-typescript/src/daemon/types.ts +++ b/packages/sdk-typescript/src/daemon/types.ts @@ -25,6 +25,78 @@ export interface DaemonCapabilities { */ features: string[]; modelServices: string[]; + /** + * Absolute canonical workspace path this daemon is bound to + * (per #3803 §02: 1 daemon = 1 workspace). Clients use this to + * (a) detect mismatch before posting `/session` (vs. waiting for + * a 400 `workspace_mismatch` response), and (b) omit `cwd` on + * `POST /session` — the route falls back to this path when the + * body has no `cwd` field. Multi-workspace deployments expose + * multiple daemons on different ports, each advertising its own + * `workspaceCwd`. + * + * Optional at the type level because the field is an additive + * extension to v=1 envelopes (added by #3803 §02). Daemons + * predating §02 still announce `v: 1` but omit this field; the + * protocol's "bump v only on incompatible frame changes" stance + * (see `qwen-serve-protocol.md`) makes additive optionality the + * correct shape. All post-§02 daemons populate it. + * + * **SDK consumers**: if you need the value as a non-undefined + * `string` (e.g. to call `.startsWith()` or pass into a function + * typed `string`), use the `requireWorkspaceCwd` helper from this + * module — it throws `DaemonCapabilityMissingError` with an + * actionable "this daemon predates §02" message instead of + * letting the call site hit a cryptic + * "Cannot read properties of undefined". + */ + workspaceCwd?: string; +} + +/** + * Thrown by `requireWorkspaceCwd` (and any future + * `requireCapability` helpers) when the daemon's + * `/capabilities` envelope is missing a field the caller needs. + * Carries the field name so handlers can branch on it. + */ +export class DaemonCapabilityMissingError extends Error { + readonly capability: string; + constructor(capability: string, hint: string) { + super( + `DaemonCapabilities.${capability} is missing — ${hint}. The daemon ` + + `you are connected to likely predates the feature that added ` + + `this field; upgrade the daemon or fall back to a different ` + + `code path that doesn't require it.`, + ); + this.name = 'DaemonCapabilityMissingError'; + this.capability = capability; + } +} + +/** + * Assert that `caps.workspaceCwd` is populated (i.e. the daemon was + * built post-§02) and return it as a non-undefined `string`. Throws + * `DaemonCapabilityMissingError` otherwise so the call site gets an + * actionable error rather than a downstream + * `Cannot read properties of undefined`. + * + * Use this when you need the value as a guaranteed `string` — + * e.g. to render in UI, log, compare with `.startsWith()`, or pass + * into a function typed `string`. If your code is fine with the + * value being absent (e.g. you fall back to `POST /session` without + * `workspaceCwd` and let the daemon choose), just read + * `caps.workspaceCwd` directly. + */ +export function requireWorkspaceCwd(caps: DaemonCapabilities): string { + if (typeof caps.workspaceCwd !== 'string' || caps.workspaceCwd.length === 0) { + throw new DaemonCapabilityMissingError( + 'workspaceCwd', + caps.workspaceCwd === '' + ? 'daemon returned an empty workspaceCwd (post-§02 daemon with a bug)' + : 'daemon predates #3803 §02 (1 daemon = 1 workspace); upgrade it', + ); + } + return caps.workspaceCwd; } /** Returned from `POST /session`. */ diff --git a/packages/sdk-typescript/src/index.ts b/packages/sdk-typescript/src/index.ts index 64d3214f108..2dc5a2e8fea 100644 --- a/packages/sdk-typescript/src/index.ts +++ b/packages/sdk-typescript/src/index.ts @@ -5,9 +5,11 @@ export { SdkLogger } from './utils/logger.js'; // Daemon HTTP client (talks to `qwen serve`; see GitHub issue #3803) export { + DaemonCapabilityMissingError, DaemonClient, DaemonHttpError, parseSseStream, + requireWorkspaceCwd, SseFramingError, type CreateSessionRequest, type DaemonCapabilities, diff --git a/packages/sdk-typescript/test/unit/DaemonClient.test.ts b/packages/sdk-typescript/test/unit/DaemonClient.test.ts index 27c9aadb14f..e39c93d1ba3 100644 --- a/packages/sdk-typescript/test/unit/DaemonClient.test.ts +++ b/packages/sdk-typescript/test/unit/DaemonClient.test.ts @@ -11,6 +11,11 @@ import { abortTimeout, composeAbortSignals, } from '../../src/daemon/DaemonClient.js'; +import { + DaemonCapabilityMissingError, + requireWorkspaceCwd, +} from '../../src/daemon/types.js'; +import type { DaemonCapabilities } from '../../src/daemon/types.js'; function jsonResponse(status: number, body: unknown): Response { return new Response(JSON.stringify(body), { @@ -96,11 +101,15 @@ describe('DaemonClient', () => { mode: 'http-bridge' as const, features: ['health', 'capabilities'], modelServices: [], + workspaceCwd: '/work/bound', }; const { fetch } = recordingFetch(() => jsonResponse(200, envelope)); const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); const caps = await client.capabilities(); expect(caps).toEqual(envelope); + // #3803 §02: clients use `workspaceCwd` to pre-flight check + + // omit `cwd` from `POST /session` (route falls back). + expect(caps.workspaceCwd).toBe('/work/bound'); }); }); @@ -147,6 +156,40 @@ describe('DaemonClient', () => { expect(JSON.parse(calls[0]!.body!)).toEqual({ cwd: '/work/a' }); }); + it('omits cwd when workspaceCwd is not provided (#3803 §02)', async () => { + // Per #3803 §02 the daemon route falls back to its bound + // workspace when `cwd` is absent. The SDK relies on + // JSON.stringify stripping `undefined` values, so an + // omitted `workspaceCwd` ends up as "no `cwd` key" on the + // wire — exactly the fallback shape the server expects. + const { fetch, calls } = recordingFetch(() => + jsonResponse(200, { + sessionId: 's-1', + workspaceCwd: '/work/bound', + attached: false, + }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + await client.createOrAttachSession({}); + expect(JSON.parse(calls[0]!.body!)).toEqual({}); + }); + + it('forwards empty-string workspaceCwd verbatim so the server can 400 it', async () => { + // `workspaceCwd: ""` is a likely client-side bug shape. A + // truthy-guard SDK would silently drop the field and let the + // server's fallback bind the session — masking the bug. We + // forward it verbatim so the server's + // `cwd must be an absolute path when provided` 400 surfaces. + const { fetch, calls } = recordingFetch(() => + jsonResponse(400, { error: 'bad cwd' }), + ); + const client = new DaemonClient({ baseUrl: 'http://daemon', fetch }); + await expect( + client.createOrAttachSession({ workspaceCwd: '' }), + ).rejects.toMatchObject({ status: 400 }); + expect(JSON.parse(calls[0]!.body!)).toEqual({ cwd: '' }); + }); + it('forwards modelServiceId when supplied', async () => { const { fetch, calls } = recordingFetch(() => jsonResponse(200, { @@ -635,4 +678,57 @@ describe('DaemonClient', () => { expect(elapsed).toBeLessThan(2000); }); }); + + describe('requireWorkspaceCwd', () => { + // Helper: build a `DaemonCapabilities`-shaped envelope without + // having to spell out the unrelated fields on every call. + const caps = (overrides: Partial): DaemonCapabilities => + ({ + v: 1, + mode: 'http-bridge', + features: [], + modelServices: [], + ...overrides, + }) as DaemonCapabilities; + + it('returns the workspaceCwd when populated', () => { + expect(requireWorkspaceCwd(caps({ workspaceCwd: '/work/bound' }))).toBe( + '/work/bound', + ); + }); + + it('throws DaemonCapabilityMissingError when the field is undefined (pre-§02 daemon)', () => { + // Pre-§02 daemons emit v=1 envelopes without `workspaceCwd`. + // The helper exists so SDK consumers get an actionable error + // instead of a downstream `Cannot read properties of undefined`. + expect(() => requireWorkspaceCwd(caps({}))).toThrow( + DaemonCapabilityMissingError, + ); + const err = (() => { + try { + requireWorkspaceCwd(caps({})); + return null; + } catch (e) { + return e; + } + })(); + expect(err).toBeInstanceOf(DaemonCapabilityMissingError); + expect((err as DaemonCapabilityMissingError).capability).toBe( + 'workspaceCwd', + ); + expect((err as DaemonCapabilityMissingError).message).toMatch( + /predates the feature|workspaceCwd/, + ); + }); + + it('treats empty-string as missing (defensive)', () => { + // A daemon that erroneously sends `workspaceCwd: ""` would + // otherwise satisfy `typeof === 'string'` while still being + // useless to consumers. Treat it like a missing field so the + // call site lands in the same error branch. + expect(() => requireWorkspaceCwd(caps({ workspaceCwd: '' }))).toThrow( + DaemonCapabilityMissingError, + ); + }); + }); });