mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-05-20 09:24:03 +00:00
fix(serve): address /review feedback from gpt-5.5 + deepseek-v4-pro
Process the 7 inline /review comments on PR #4113: - C1+C3 (SDK): make `DaemonCapabilities.workspaceCwd` and `CreateSessionRequest.workspaceCwd` optional in the SDK types. `workspaceCwd` is an additive field on the v=1 envelope per #3803 §02; the protocol's "bump v only on incompatible changes" stance is honored by leaving the field optional at the type level. `DaemonClient.createOrAttachSession` now omits `cwd` from the body when `workspaceCwd` isn't passed, matching the PR description's "SDK accepts bound path or none". Adds a unit test pinning the empty-body shape. - C2 (docs/users/qwen-serve.md): the `--http-bridge` row described the pre-§02 per-session model; updated to reflect one child per daemon with N sessions multiplexed via ACP `newSession()`. - C4 (server.ts): `WorkspaceMismatchError` was silently 400'ing without a stderr breadcrumb, leaving operators blind to cross-workspace routing drift. Mirrors the SessionLimitExceeded /InvalidPermissionOption observability pattern. - C5 (server.test.ts): the `/capabilities` fallback test compared `res.body.workspaceCwd` against raw `process.cwd()`; on macOS default tmpdir flows (`/var/folders/...` → `/private/var/...`) the canonicalize-once route value diverges. Use `realpathSync.native(process.cwd())` to match the route's canonicalization. - C6 (server.ts): the cwd-not-absolute error said "cwd is required and must be an absolute path" but cwd is now optional under §02. Tightened wording to "must be an absolute path when provided". - C7 (runQwenServe.ts): the `statSync` catch only wrapped ENOENT with a friendly diagnostic; EACCES / EPERM (typical for SIP-protected dirs on macOS or root-owned paths the daemon's UID can't traverse) re-threw as raw `SystemError`. Wrap both codes with a `--workspace`-context message so the boot failure points at the flag the operator set. Docs: quickstart shows the explicit-pass-or-omit options side by side; protocol reference notes `workspaceCwd` is additive to v=1.
This commit is contained in:
Shaojin Wen
parent
5e309a90bb
commit
4d41fcee76
9 changed files with 90 additions and 12 deletions
|
|
@ -36,11 +36,15 @@ const caps = await client.capabilities();
|
|||
console.log('Daemon features:', caps.features);
|
||||
console.log('Daemon workspace:', caps.workspaceCwd); // canonical bound path
|
||||
|
||||
// 2. Spawn-or-attach a session. The `workspaceCwd` MUST match the
|
||||
// daemon's bound workspace (`caps.workspaceCwd`) — pass it
|
||||
// explicitly, OR omit it and the route falls back to the bound
|
||||
// path. A mismatched `workspaceCwd` returns
|
||||
// `400 workspace_mismatch` (see "Workspace mismatch" below).
|
||||
// 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: caps.workspaceCwd,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -115,7 +115,7 @@ 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`.
|
||||
> **`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`
|
||||
|
||||
|
|
|
|||
|
|
@ -115,7 +115,7 @@ The token comparison is constant-time (SHA-256 + `crypto.timingSafeEqual`); 401
|
|||
| `--max-sessions <n>` | `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 <path>` | `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 <n>` | `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. |
|
||||
| `--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
|
||||
|
|
|
|||
|
|
@ -139,6 +139,20 @@ export async function runQwenServe(
|
|||
`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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@
|
|||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import { realpathSync } from 'node:fs';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { describe, it, expect, afterEach, vi } from 'vitest';
|
||||
import request from 'supertest';
|
||||
|
|
@ -217,7 +218,13 @@ describe('createServeApp', () => {
|
|||
.get('/capabilities')
|
||||
.set('Host', `127.0.0.1:${baseOpts.port}`);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.workspaceCwd).toBe(process.cwd());
|
||||
// `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()));
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -184,7 +184,7 @@ export function createServeApp(
|
|||
if (!path.isAbsolute(cwd)) {
|
||||
res
|
||||
.status(400)
|
||||
.json({ error: '`cwd` is required and must be an absolute path' });
|
||||
.json({ error: '`cwd` must be an absolute path when provided' });
|
||||
return;
|
||||
}
|
||||
const modelServiceId =
|
||||
|
|
@ -840,6 +840,19 @@ function sendBridgeError(
|
|||
// 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.
|
||||
writeStderrLine(
|
||||
`qwen serve: workspace_mismatch (POST /session): ` +
|
||||
`daemon bound to "${err.bound}", rejected "${err.requested}"`,
|
||||
);
|
||||
res.status(400).json({
|
||||
error: err.message,
|
||||
code: 'workspace_mismatch',
|
||||
|
|
|
|||
|
|
@ -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,13 +259,18 @@ export class DaemonClient {
|
|||
async createOrAttachSession(
|
||||
req: CreateSessionRequest,
|
||||
): Promise<DaemonSession> {
|
||||
// Per #3803 §02: omitting `cwd` lets the daemon fall back to its
|
||||
// bound workspace. Send `cwd` ONLY when the caller passed
|
||||
// `workspaceCwd` — sending `cwd: undefined` would JSON.stringify
|
||||
// it out anyway, but conditional spread keeps the on-wire body
|
||||
// shape obvious to anyone tracing the request.
|
||||
return await this.fetchWithTimeout(
|
||||
`${this.baseUrl}/session`,
|
||||
{
|
||||
method: 'POST',
|
||||
headers: this.headers({ 'Content-Type': 'application/json' }),
|
||||
body: JSON.stringify({
|
||||
cwd: req.workspaceCwd,
|
||||
...(req.workspaceCwd ? { cwd: req.workspaceCwd } : {}),
|
||||
...(req.modelServiceId ? { modelServiceId: req.modelServiceId } : {}),
|
||||
}),
|
||||
},
|
||||
|
|
|
|||
|
|
@ -34,8 +34,15 @@ export interface DaemonCapabilities {
|
|||
* 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.
|
||||
*/
|
||||
workspaceCwd: string;
|
||||
workspaceCwd?: string;
|
||||
}
|
||||
|
||||
/** Returned from `POST /session`. */
|
||||
|
|
|
|||
|
|
@ -151,6 +151,25 @@ 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 must therefore NOT
|
||||
// send `cwd: undefined` (or any other placeholder) when the
|
||||
// caller omits `workspaceCwd` — otherwise the wire body
|
||||
// carries an explicit empty field that wouldn't ever match
|
||||
// the fallback semantics on a future stricter daemon.
|
||||
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 modelServiceId when supplied', async () => {
|
||||
const { fetch, calls } = recordingFetch(() =>
|
||||
jsonResponse(200, {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue