From e1644ba2f63318f23fb3d888f9141e5ed2aa8fb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=89=E8=90=81?= Date: Wed, 26 Aug 2026 10:58:14 +0800 Subject: [PATCH 1/7] feat(serve): allow relocating session attachment storage via env var Adds QWEN_SERVE_SESSION_ATTACHMENTS_ROOT, which stores session attachments under //attachments instead of the runtime temp dir so operators can pin them to a dedicated volume. Reads and removes that miss the configured root fall back to the default dir so pre-switch attachments stay readable and removable; archive cleanup removes both roots. New uploads never shadow a legacy fallback name, and both roots are removed via the same tombstone dance so a deletion racing a session restore cannot sweep up a successor directory. --- docs/developers/daemon/17-configuration.md | 29 +- docs/users/qwen-serve.md | 15 + packages/acp-bridge/src/bridge.test.ts | 80 ++++ packages/acp-bridge/src/bridge.ts | 8 +- packages/acp-bridge/src/bridgeOptions.ts | 8 + .../acp-bridge/src/sessionAttachments.test.ts | 369 +++++++++++++++++- packages/acp-bridge/src/sessionAttachments.ts | 216 +++++++--- .../cli/src/serve/process-env-guard.test.ts | 8 + packages/cli/src/serve/run-qwen-serve.ts | 39 +- packages/cli/src/serve/server.ts | 11 +- .../serve/session-attachments-root.test.ts | 116 ++++++ .../cli/src/serve/session-attachments-root.ts | 58 +++ 12 files changed, 856 insertions(+), 101 deletions(-) create mode 100644 packages/cli/src/serve/session-attachments-root.test.ts create mode 100644 packages/cli/src/serve/session-attachments-root.ts diff --git a/docs/developers/daemon/17-configuration.md b/docs/developers/daemon/17-configuration.md index 2b59b892522..88b4d347dd6 100644 --- a/docs/developers/daemon/17-configuration.md +++ b/docs/developers/daemon/17-configuration.md @@ -56,20 +56,21 @@ This page collects every setting that affects the `qwen serve` daemon and its ad ### Read by `runQwenServe` / Express middleware -| Env | Effect | -| ----------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `QWEN_SERVER_TOKEN` | Bearer token; trimmed at boot. | -| `QWEN_SERVE_DEBUG` | `1` / `true` / `on` / `yes` (case-insensitive) enables verbose stderr logs. See [`19-observability.md`](./19-observability.md). | -| `QWEN_SERVE_NO_MCP_POOL` | `1` disables the workspace MCP transport pool and falls back to per-session `McpClientManager`; capabilities stop advertising `mcp_workspace_pool` / `mcp_pool_restart`. | -| `QWEN_SERVE_PROMPT_DEADLINE_MS` | Env fallback for `--prompt-deadline-ms`. | -| `QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS` | Env fallback for `--writer-idle-timeout-ms`. | -| `QWEN_SERVE_RATE_LIMIT` | `1` / `true` enables per-tier HTTP rate limiting; CLI `--rate-limit` / `--no-rate-limit` wins. | -| `QWEN_SERVE_RATE_LIMIT_PROMPT` | Env fallback for `--rate-limit-prompt`. | -| `QWEN_SERVE_RATE_LIMIT_MUTATION` | Env fallback for `--rate-limit-mutation`. | -| `QWEN_SERVE_RATE_LIMIT_READ` | Env fallback for `--rate-limit-read`. | -| `QWEN_SERVE_RATE_LIMIT_WINDOW_MS` | Env fallback for `--rate-limit-window-ms`. | -| `QWEN_SERVE_NEW_FILE_MODE` | New-file mode policy for daemon text writes: `owner` (default — NEW files are created `0600`, umask-independent) or `system` (NEW files follow `0o666 & ~umask`). Case-insensitive; the literal `0600` is accepted as an alias for `owner` (no other octal modes are supported), and unrecognized values warn on stderr and keep the `0600` default. Existing files always preserve their mode. See [`qwen-serve.md` — New-file mode for agent text writes](../../users/qwen-serve.md#new-file-mode-for-agent-text-writes). | -| `QWEN_CODE_MEMORY_PROJECT_SCOPE` | `workspace` keys project memory by the exact workspace dir; `git-root` selects the legacy shared scope. When unset, the daemon injects `workspace`; unrecognized values warn once and retain the legacy `git-root` behavior. Propagates via the runtime base env, not `childEnvOverrides`; `--memory-project-scope` wins. Each workspace remember/forget/dream lane caps pending tasks at `MAX_PENDING = 16`; N workspaces allow up to 16·N queued tasks with no daemon-wide cap. | +| Env | Effect | +| ------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `QWEN_SERVER_TOKEN` | Bearer token; trimmed at boot. | +| `QWEN_SERVE_DEBUG` | `1` / `true` / `on` / `yes` (case-insensitive) enables verbose stderr logs. See [`19-observability.md`](./19-observability.md). | +| `QWEN_SERVE_NO_MCP_POOL` | `1` disables the workspace MCP transport pool and falls back to per-session `McpClientManager`; capabilities stop advertising `mcp_workspace_pool` / `mcp_pool_restart`. | +| `QWEN_SERVE_PROMPT_DEADLINE_MS` | Env fallback for `--prompt-deadline-ms`. | +| `QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS` | Env fallback for `--writer-idle-timeout-ms`. | +| `QWEN_SERVE_RATE_LIMIT` | `1` / `true` enables per-tier HTTP rate limiting; CLI `--rate-limit` / `--no-rate-limit` wins. | +| `QWEN_SERVE_RATE_LIMIT_PROMPT` | Env fallback for `--rate-limit-prompt`. | +| `QWEN_SERVE_RATE_LIMIT_MUTATION` | Env fallback for `--rate-limit-mutation`. | +| `QWEN_SERVE_RATE_LIMIT_READ` | Env fallback for `--rate-limit-read`. | +| `QWEN_SERVE_RATE_LIMIT_WINDOW_MS` | Env fallback for `--rate-limit-window-ms`. | +| `QWEN_SERVE_NEW_FILE_MODE` | New-file mode policy for daemon text writes: `owner` (default — NEW files are created `0600`, umask-independent) or `system` (NEW files follow `0o666 & ~umask`). Case-insensitive; the literal `0600` is accepted as an alias for `owner` (no other octal modes are supported), and unrecognized values warn on stderr and keep the `0600` default. Existing files always preserve their mode. See [`qwen-serve.md` — New-file mode for agent text writes](../../users/qwen-serve.md#new-file-mode-for-agent-text-writes). | +| `QWEN_SERVE_SESSION_ATTACHMENTS_ROOT` | Overrides where session attachments are stored; reads/removes that miss the configured root fall back to the default runtime temp dir so pre-switch attachments stay readable. Accepts an absolute path, a path relative to the daemon cwd, or `~` / `~/…`. See [`qwen-serve.md` — Session attachment storage](../../users/qwen-serve.md#session-attachment-storage). | +| `QWEN_CODE_MEMORY_PROJECT_SCOPE` | `workspace` keys project memory by the exact workspace dir; `git-root` selects the legacy shared scope. When unset, the daemon injects `workspace`; unrecognized values warn once and retain the legacy `git-root` behavior. Propagates via the runtime base env, not `childEnvOverrides`; `--memory-project-scope` wins. Each workspace remember/forget/dream lane caps pending tasks at `MAX_PENDING = 16`; N workspaces allow up to 16·N queued tasks with no daemon-wide cap. | Blank `QWEN_CODE_MEMORY_PROJECT_SCOPE` values are treated as unset and therefore default to `workspace`; unrecognized non-empty values still warn once and retain the legacy `git-root` behavior. diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index d12a855eea4..56139711cb5 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -705,6 +705,21 @@ Scope and limits: - Binary uploads (`POST /file/upload`) always create at `0600` regardless of this setting. - The daemon reads the variable at workspace-filesystem construction; restart the daemon after changing it. +### Session attachment storage + +Session attachments (files and images uploaded by Web Shell through `POST /session/:id/attachments`) are stored by default under the workspace's runtime temp dir: `/tmp//attachments`, keyed per session as `session-`. Operators who want attachments to persist outside the runtime temp dir (e.g. on a dedicated volume) can override the root: + +| Env var | Values | Default | What it does | +| ------------------------------------- | ------ | ------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `QWEN_SERVE_SESSION_ATTACHMENTS_ROOT` | path | unset | Stores session attachments under this directory instead of the default runtime temp dir. Accepts an absolute path, a path relative to the daemon's cwd, or `~` / `~/…` expanded against the home directory. | + +Scope and limits: + +- **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable and removable. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. +- **Per-session layout.** Files live under `//attachments/session-/` in both locations, where `` is the same workspace hash used by the default runtime temp dir; the fallback lookup uses the same session layout in the default dir. Two workspaces pointing at the same configured root stay isolated from each other. +- **Archive cleanup.** When a session is archived, its attachment directory is removed from both the configured root and the default fallback dir. +- The daemon reads the variable at startup; restart the daemon after changing it. The directory must be writable by the daemon process. + ## Multi-session & multi-workspace deployment Pass `--workspace` more than once to register several non-overlapping workspaces in one `qwen serve` process. The first path is primary. Each registered workspace owns an isolated runtime boundary, while the daemon-wide listener, authentication policy, and total-session limit are shared. Production attempts to preheat the primary ACP child for compatibility and retries on first use after failure; trusted secondaries start their own child on demand, and untrusted secondaries do not start ACP. Requests may select a registered workspace by canonical `cwd`; requests that omit `cwd` use the primary workspace. Use one daemon per user or security principal; workspace trust is an execution gate, not an ACL. diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 94314c21e9b..e7171a59fd1 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -13710,6 +13710,86 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('reads attachments from the fallback root when the primary misses', async () => { + const mainRoot = await fsp.mkdtemp( + path.join(os.tmpdir(), 'qwen-bridge-main-'), + ); + const fallbackRoot = await fsp.mkdtemp( + path.join(os.tmpdir(), 'qwen-bridge-fallback-'), + ); + const bridge = makeBridge({ + sessionAttachmentsRoot: mainRoot, + sessionAttachmentsFallbackRoot: fallbackRoot, + channelFactory: async () => makeChannel({}).channel, + }); + try { + const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const sessionDir = `session-${encodeURIComponent(session.sessionId)}`; + await fsp.mkdir(path.join(fallbackRoot, sessionDir), { + recursive: true, + }); + await fsp.writeFile( + path.join(fallbackRoot, sessionDir, 'notes.txt'), + 'legacy attachment', + ); + + const read = await bridge.readSessionAttachment( + session.sessionId, + 'notes.txt', + { clientId: session.clientId }, + ); + expect(read?.data.toString()).toBe('legacy attachment'); + expect(read?.mimeType).toBe('text/plain'); + } finally { + await bridge.shutdown(); + await fsp.rm(mainRoot, { recursive: true, force: true }); + await fsp.rm(fallbackRoot, { recursive: true, force: true }); + } + }); + + it('deleteSessionAttachments clears both roots for a non-live session', async () => { + const mainRoot = await fsp.mkdtemp( + path.join(os.tmpdir(), 'qwen-bridge-main-'), + ); + const fallbackRoot = await fsp.mkdtemp( + path.join(os.tmpdir(), 'qwen-bridge-fallback-'), + ); + const bridge = makeBridge({ + sessionAttachmentsRoot: mainRoot, + sessionAttachmentsFallbackRoot: fallbackRoot, + channelFactory: async () => makeChannel({}).channel, + }); + try { + const sessionId = 'sess:unknown'; + const sessionDir = `session-${encodeURIComponent(sessionId)}`; + await fsp.mkdir(path.join(mainRoot, sessionDir), { recursive: true }); + await fsp.writeFile( + path.join(mainRoot, sessionDir, 'current.txt'), + 'current', + ); + await fsp.mkdir(path.join(fallbackRoot, sessionDir), { + recursive: true, + }); + await fsp.writeFile( + path.join(fallbackRoot, sessionDir, 'notes.txt'), + 'legacy', + ); + + await bridge.deleteSessionAttachments(sessionId); + + await expect( + fsp.readdir(path.join(mainRoot, sessionDir)), + ).rejects.toMatchObject({ code: 'ENOENT' }); + await expect( + fsp.readdir(path.join(fallbackRoot, sessionDir)), + ).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + await bridge.shutdown(); + await fsp.rm(mainRoot, { recursive: true, force: true }); + await fsp.rm(fallbackRoot, { recursive: true, force: true }); + } + }); + it('resolves text and binary file attachment references for ACP', async () => { const prompts: PromptRequest[] = []; const bridge = makeBridge({ diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 496977e4b8d..478c5e613fc 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -6035,6 +6035,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { attachments: new SessionAttachmentStore( opts.sessionAttachmentsRoot, sessionId, + opts.sessionAttachmentsFallbackRoot, ), recordingDegraded: false, closing: false, @@ -9556,6 +9557,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const branchAttachments = new SessionAttachmentStore( opts.sessionAttachmentsRoot, result.newSessionId, + opts.sessionAttachmentsFallbackRoot, ); try { await branchAttachments.copyFrom(entry.attachments); @@ -11220,7 +11222,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { async deleteSessionAttachments(sessionId, options) { const store = byId.get(sessionId)?.attachments ?? - new SessionAttachmentStore(opts.sessionAttachmentsRoot, sessionId); + new SessionAttachmentStore( + opts.sessionAttachmentsRoot, + sessionId, + opts.sessionAttachmentsFallbackRoot, + ); await store.delete(options); }, diff --git a/packages/acp-bridge/src/bridgeOptions.ts b/packages/acp-bridge/src/bridgeOptions.ts index 216592c7eeb..9ebb3cd7f5d 100644 --- a/packages/acp-bridge/src/bridgeOptions.ts +++ b/packages/acp-bridge/src/bridgeOptions.ts @@ -220,6 +220,14 @@ export interface BridgeOptions { * root. Direct embedded callers may omit it for process-local storage. */ sessionAttachmentsRoot?: string; + /** + * Fallback root for reading session attachments stored before + * `sessionAttachmentsRoot` was reconfigured (e.g. the previous default + * directory). Writes always go to `sessionAttachmentsRoot`; reads and + * removes that miss there consult this root so existing attachments + * survive a root switch. + */ + sessionAttachmentsFallbackRoot?: string; /** * `single` shares one session per workspace across HTTP * clients (live-collaboration default); `thread` gives each `spawnOrAttach` diff --git a/packages/acp-bridge/src/sessionAttachments.test.ts b/packages/acp-bridge/src/sessionAttachments.test.ts index 5fec06a73dc..30da05f60a0 100644 --- a/packages/acp-bridge/src/sessionAttachments.test.ts +++ b/packages/acp-bridge/src/sessionAttachments.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { promises as fs } from 'node:fs'; +import { promises as fs, statSync } from 'node:fs'; import { tmpdir } from 'node:os'; import * as path from 'node:path'; import type { ContentBlock } from '@agentclientprotocol/sdk'; @@ -16,6 +16,18 @@ import { withAttachmentDegradationMarker, } from './sessionAttachments.js'; +// `node:fs` is a sealed ESM namespace: vi.spyOn cannot redefine `statSync` +// (which the store imports by name). Mock the module and delegate to the real +// implementation by default; only the stat-fault tests override it. Everything +// else (`promises`, the remaining sync exports) stays real via importOriginal. +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + statSync: vi.fn(actual.statSync), + }; +}); + describe('SessionAttachmentStore', () => { it('does not append the attachment degradation marker twice', () => { const once = withAttachmentDegradationMarker([ @@ -1093,4 +1105,359 @@ describe('SessionAttachmentStore', () => { await store.close(); } }); + + describe('fallback root', () => { + const sessionId = 's-1'; + const sessionDir = `session-${encodeURIComponent(sessionId)}`; + + async function createRoots(): Promise<{ + main: string; + fallback: string; + }> { + const main = await fs.mkdtemp( + path.join(tmpdir(), 'qwen-attachments-main-'), + ); + const fallback = await fs.mkdtemp( + path.join(tmpdir(), 'qwen-attachments-fallback-'), + ); + return { main, fallback }; + } + + async function writeIn( + root: string, + name: string, + data: string, + ): Promise { + const directory = path.join(root, sessionDir); + await fs.mkdir(directory, { recursive: true }); + await fs.writeFile(path.join(directory, name), data); + } + + it('reads from the fallback root when the primary misses', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + await writeIn(fallback, 'notes.txt', 'from fallback'); + + expect(await store.read('notes.txt')).toEqual({ + data: Buffer.from('from fallback'), + mimeType: 'text/plain', + }); + expect(() => + store.assertReference({ + type: 'resource', + attachmentId: 'notes.txt', + mimeType: 'text/plain', + size: 13, + }), + ).not.toThrow(); + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('prefers the primary root over the fallback', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + const reference = await store.putAttachment( + new TextEncoder().encode('primary'), + 'text/plain', + 'notes.txt', + ); + await writeIn(fallback, 'notes.txt', 'stale fallback'); + + expect(await store.read(reference.attachmentId)).toEqual({ + data: Buffer.from('primary'), + mimeType: 'text/plain', + }); + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('does not shadow a fallback name with a new upload', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + await writeIn(fallback, 'notes.txt', 'stale fallback'); + + const reference = await store.putAttachment( + new TextEncoder().encode('fresh upload'), + 'text/plain', + 'notes.txt', + ); + + expect(reference.attachmentId).not.toBe('notes.txt'); + expect(await store.read(reference.attachmentId)).toEqual({ + data: Buffer.from('fresh upload'), + mimeType: 'text/plain', + }); + // The pre-switch attachment is still reachable under its own ID. + expect(await store.read('notes.txt')).toEqual({ + data: Buffer.from('stale fallback'), + mimeType: 'text/plain', + }); + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('surfaces a fallback stat error instead of shadowing the name', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + const stat = vi.mocked(statSync).mockImplementationOnce(() => { + throw Object.assign(new Error('permission denied'), { code: 'EACCES' }); + }); + try { + await writeIn(fallback, 'notes.txt', 'stale fallback'); + + // A temporarily-unreadable fallback must not be treated as "name + // free": failing the upload is safer than shadowing the old file. + await expect( + store.putAttachment( + new TextEncoder().encode('fresh upload'), + 'text/plain', + 'notes.txt', + ), + ).rejects.toMatchObject({ code: 'EACCES' }); + } finally { + stat.mockRestore(); + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('still degrades reference validation when the fallback stat fails', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + const stat = vi.mocked(statSync).mockImplementationOnce(() => { + throw Object.assign(new Error('permission denied'), { code: 'EACCES' }); + }); + try { + await writeIn(fallback, 'notes.txt', 'stale fallback'); + + // Reference validation must degrade to session_attachment_gone, not + // surface the raw stat error and abort the prompt. + expect(() => + store.assertReference({ + type: 'resource', + attachmentId: 'notes.txt', + mimeType: 'text/plain', + size: 13, + }), + ).toThrowError( + expect.objectContaining({ code: 'session_attachment_gone' }), + ); + } finally { + stat.mockRestore(); + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('returns undefined when neither root holds the attachment', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + expect(await store.read('missing.txt')).toBeUndefined(); + expect(await store.remove('missing.txt')).toBe(false); + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('removes a fallback attachment when the primary misses', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + await writeIn(fallback, 'notes.txt', 'from fallback'); + + expect(await store.remove('notes.txt')).toBe(true); + expect(await fs.readdir(path.join(fallback, sessionDir))).toEqual([]); + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('removes both copies when both roots hold the same name', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + await writeIn(main, 'notes.txt', 'from primary'); + await writeIn(fallback, 'notes.txt', 'stale fallback copy'); + + expect(await store.remove('notes.txt')).toBe(true); + expect(await store.read('notes.txt')).toBeUndefined(); + expect(await fs.readdir(path.join(main, sessionDir))).toEqual([]); + expect(await fs.readdir(path.join(fallback, sessionDir))).toEqual([]); + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('keeps the primary readable when the fallback unlink fails', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + const unlink = vi + .spyOn(fs, 'unlink') + .mockRejectedValueOnce( + Object.assign(new Error('read-only volume'), { code: 'EROFS' }), + ); + try { + await writeIn(main, 'notes.txt', 'from primary'); + await writeIn(fallback, 'notes.txt', 'stale fallback copy'); + + // The fallback copy cannot be removed; remove() must fail cleanly and + // leave the authoritative primary copy readable instead of deleting it + // and resurrecting stale fallback bytes on the next read. + await expect(store.remove('notes.txt')).rejects.toThrow( + 'read-only volume', + ); + expect(await store.read('notes.txt')).toEqual({ + data: Buffer.from('from primary'), + mimeType: 'text/plain', + }); + } finally { + unlink.mockRestore(); + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('delete clears both the primary and fallback directories', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + await store.putAttachment( + new TextEncoder().encode('primary'), + 'text/plain', + 'notes.txt', + ); + await writeIn(fallback, 'old.txt', 'from fallback'); + + await store.delete(); + + await expect( + fs.readdir(path.join(main, sessionDir)), + ).rejects.toMatchObject({ code: 'ENOENT' }); + await expect( + fs.readdir(path.join(fallback, sessionDir)), + ).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('delete clears the fallback directory when only it holds data', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + await writeIn(fallback, 'old.txt', 'from fallback'); + + await store.delete(); + + await expect( + fs.readdir(path.join(main, sessionDir)), + ).rejects.toMatchObject({ code: 'ENOENT' }); + await expect( + fs.readdir(path.join(fallback, sessionDir)), + ).rejects.toMatchObject({ code: 'ENOENT' }); + }); + + it('delete tombstones both roots so a recreated session dir survives', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + await writeIn(main, 'old.txt', 'from primary'); + await writeIn(fallback, 'old.txt', 'from fallback'); + // For every tombstone rename (primary and fallback), a successor + // re-creates the session directory at its original path — the tombstone + // removal must not sweep that fresh directory up. + const realRename = fs.rename.bind(fs); + const rename = vi + .spyOn(fs, 'rename') + .mockImplementation(async (from, to) => { + await realRename(from, to); + await fs.mkdir(String(from), { recursive: true }); + await fs.writeFile( + path.join(String(from), 'successor.txt'), + 'new owner', + ); + }); + try { + await store.delete(); + expect( + await fs.readFile( + path.join(main, sessionDir, 'successor.txt'), + 'utf8', + ), + ).toBe('new owner'); + expect( + await fs.readFile( + path.join(fallback, sessionDir, 'successor.txt'), + 'utf8', + ), + ).toBe('new owner'); + } finally { + rename.mockRestore(); + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('copyFrom merges fallback-held attachments with primary winning on conflicts', async () => { + const { main, fallback } = await createRoots(); + const source = new SessionAttachmentStore(main, sessionId, fallback); + const targetRoot = await fs.mkdtemp( + path.join(tmpdir(), 'qwen-attachments-target-'), + ); + const target = new SessionAttachmentStore(targetRoot, sessionId); + try { + await source.putAttachment( + new TextEncoder().encode('primary file'), + 'text/plain', + 'primary.txt', + ); + await source.putAttachment( + new TextEncoder().encode('primary version'), + 'text/plain', + 'conflict.txt', + ); + await writeIn(fallback, 'legacy.txt', 'legacy'); + await writeIn(fallback, 'conflict.txt', 'fallback version'); + + await target.copyFrom(source); + + expect(await target.read('primary.txt')).toEqual({ + data: Buffer.from('primary file'), + mimeType: 'text/plain', + }); + expect(await target.read('legacy.txt')).toEqual({ + data: Buffer.from('legacy'), + mimeType: 'text/plain', + }); + expect(await target.read('conflict.txt')).toEqual({ + data: Buffer.from('primary version'), + mimeType: 'text/plain', + }); + } finally { + await source.close(); + await target.delete(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + await fs.rm(targetRoot, { recursive: true, force: true }); + } + }); + }); }); diff --git a/packages/acp-bridge/src/sessionAttachments.ts b/packages/acp-bridge/src/sessionAttachments.ts index 4ff8500ed69..15ec8bdde7b 100644 --- a/packages/acp-bridge/src/sessionAttachments.ts +++ b/packages/acp-bridge/src/sessionAttachments.ts @@ -168,6 +168,29 @@ function isTextAttachment(data: Buffer, mimeType: string): boolean { return Buffer.from(data.toString('utf8'), 'utf8').equals(data); } +function statSize(filePath: string): number | undefined { + try { + return statSync(filePath).size; + } catch { + // Any stat failure means "not verifiably present" — reference validation + // must degrade to session_attachment_gone rather than abort the prompt. + return undefined; + } +} + +// Strict occupancy probe for upload dedup: unlike `statSize`, a non-ENOENT +// failure (EACCES/EIO) surfaces so a temporarily-unreadable fallback root is +// not mistaken for a free name (which would let a new upload shadow an +// existing attachment). +function statSizeStrict(filePath: string): number | undefined { + try { + return statSync(filePath).size; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; + throw error; + } +} + // Append the unavailable marker to the last text block (or as a new text // block) so a partially degraded prompt keeps its surviving blocks instead of // collapsing into one wholesale placeholder. @@ -197,6 +220,7 @@ export function withAttachmentDegradationMarker< export class SessionAttachmentStore { private directoryPromise?: Promise; private readonly persistentDirectory?: string; + private readonly persistentFallbackDirectory?: string; private activeDirectory?: string; private pendingItems = 0; private readonly pendingNames = new Map(); @@ -209,12 +233,19 @@ export class SessionAttachmentStore { constructor( private readonly directoryRoot?: string, sessionId?: string, + fallbackDirectoryRoot?: string, ) { if (!directoryRoot || !sessionId) return; this.persistentDirectory = path.join( directoryRoot, `session-${encodeURIComponent(sessionId)}`, ); + if (fallbackDirectoryRoot) { + this.persistentFallbackDirectory = path.join( + fallbackDirectoryRoot, + `session-${encodeURIComponent(sessionId)}`, + ); + } } async putAttachment( @@ -261,6 +292,18 @@ export class SessionAttachmentStore { if (safeAttachmentName(candidateName) !== candidateName) { throw new TypeError('Session attachment name is invalid'); } + // A legacy fallback copy owns this name; a new upload must not shadow + // it (reads resolve the primary first, so reusing the ID would make an + // old reference surface the new bytes). Treat it as occupied. + if ( + this.persistentFallbackDirectory && + statSizeStrict( + path.join(this.persistentFallbackDirectory, candidateName), + ) !== undefined + ) { + suffix += 1; + continue; + } if (pendingName !== candidateName) { if (pendingName) this.releasePendingName(pendingName); this.reservePendingName(candidateName); @@ -432,16 +475,23 @@ export class SessionAttachmentStore { ): Promise<{ data: Buffer; mimeType: string } | undefined> { const name = safeAttachmentName(attachmentId); if (!name || name !== attachmentId) return undefined; - const filePath = path.join(await this.directory(), name); + const primary = await this.tryRead(await this.directory(), name); + if (primary) return primary; + return await this.tryRead(this.persistentFallbackDirectory, name); + } + + private async tryRead( + directory: string | undefined, + name: string, + ): Promise<{ data: Buffer; mimeType: string } | undefined> { + if (!directory) return undefined; try { return { - data: await fs.readFile(filePath), + data: await fs.readFile(path.join(directory, name)), mimeType: mimeTypeForName(name), }; } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - return undefined; - } + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; throw error; } } @@ -472,45 +522,56 @@ export class SessionAttachmentStore { ); } if (this.closed) throw new Error('Session attachment store is closed'); - const sourceDirectory = - source.persistentDirectory ?? source.activeDirectory; - if (!sourceDirectory) return; - let entries; - try { - entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return; - throw error; - } + const sourceDirectories = [ + source.persistentDirectory ?? source.activeDirectory, + source.persistentFallbackDirectory, + ].filter((directory): directory is string => Boolean(directory)); + if (sourceDirectories.length === 0) return; const targetDirectory = await this.directory(); - await Promise.all( - entries - .filter( - (entry) => entry.isFile() && !source.pendingNames.has(entry.name), - ) - .map(async (entry) => { - const sourcePath = path.join(sourceDirectory, entry.name); - try { - await fs.copyFile( - sourcePath, - path.join(targetDirectory, entry.name), - ); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - try { - await fs.stat(sourcePath); - } catch (sourceError) { - if ( - (sourceError as NodeJS.ErrnoException).code === 'ENOENT' - ) { - return; + // Primary first so a name held by both roots resolves to the primary; + // fallback entries copied later with the same name are skipped. + const copiedNames = new Set(); + for (const sourceDirectory of sourceDirectories) { + let entries; + try { + entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; + throw error; + } + await Promise.all( + entries + .filter( + (entry) => + entry.isFile() && + !source.pendingNames.has(entry.name) && + !copiedNames.has(entry.name), + ) + .map(async (entry) => { + const sourcePath = path.join(sourceDirectory, entry.name); + try { + await fs.copyFile( + sourcePath, + path.join(targetDirectory, entry.name), + ); + copiedNames.add(entry.name); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + try { + await fs.stat(sourcePath); + } catch (sourceError) { + if ( + (sourceError as NodeJS.ErrnoException).code === 'ENOENT' + ) { + return; + } } } + throw error; } - throw error; - } - }), - ); + }), + ); + } } finally { source.copying = false; this.copying = false; @@ -529,15 +590,27 @@ export class SessionAttachmentStore { ) { return false; } - const directory = await this.directory(); - const filePath = path.join(directory, name); + // Unlink the fallback first: if a legacy copy fails to unlink (e.g. the + // old default dir sits on a read-only volume), the authoritative primary + // copy is still intact and remove() can fail cleanly without leaving a + // deleted attachment readable through the fallback. + const fallbackHit = + (await this.tryUnlink(this.persistentFallbackDirectory, name)) === true; + const primaryHit = + (await this.tryUnlink(await this.directory(), name)) === true; + return primaryHit || fallbackHit; + } + + private async tryUnlink( + directory: string | undefined, + name: string, + ): Promise { + if (!directory) return undefined; try { - await fs.unlink(filePath); + await fs.unlink(path.join(directory, name)); return true; } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - return false; - } + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; throw error; } } @@ -571,31 +644,52 @@ export class SessionAttachmentStore { this.persistentDirectory ?? (await this.directoryPromise?.catch(() => undefined)); if (directory) { - options.assertCanCommit?.(); - const tombstone = path.join( - path.dirname(directory), - `.${path.basename(directory)}.deleting-${randomUUID()}`, + await this.removeDirectoryWithTombstone( + directory, + options.assertCanCommit, + ); + } + if (this.persistentFallbackDirectory) { + await this.removeDirectoryWithTombstone( + this.persistentFallbackDirectory, + options.assertCanCommit, ); - try { - await fs.rename(directory, tombstone); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return; - throw error; - } - await fs.rm(tombstone, { recursive: true, force: true }); } } + /** + * Atomically detach `directory` by renaming it to a tombstone, then remove + * the tombstone. A concurrent writer that recreates the directory after the + * rename lands a fresh directory at the original path, which the tombstone + * removal never touches — so a deletion racing a session restore cannot + * sweep up a successor directory. ENOENT (nothing to delete) is a no-op. + */ + private async removeDirectoryWithTombstone( + directory: string, + assertCanCommit?: () => void, + ): Promise { + assertCanCommit?.(); + const tombstone = path.join( + path.dirname(directory), + `.${path.basename(directory)}.deleting-${randomUUID()}`, + ); + try { + await fs.rename(directory, tombstone); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + await fs.rm(tombstone, { recursive: true, force: true }); + } + private assertStored(reference: SessionAttachmentReference): void { const id = reference.attachmentId; const name = safeAttachmentName(id); let size: number | undefined; const directory = this.persistentDirectory ?? this.activeDirectory; if (name && name === id && directory) { - try { - size = statSync(path.join(directory, name)).size; - } catch { - size = undefined; + size = statSize(path.join(directory, name)); + if (size === undefined && this.persistentFallbackDirectory) { + size = statSize(path.join(this.persistentFallbackDirectory, name)); } } const storedMimeType = name ? mimeTypeForName(name) : undefined; diff --git a/packages/cli/src/serve/process-env-guard.test.ts b/packages/cli/src/serve/process-env-guard.test.ts index e3e960e8553..d0209820da3 100644 --- a/packages/cli/src/serve/process-env-guard.test.ts +++ b/packages/cli/src/serve/process-env-guard.test.ts @@ -171,6 +171,14 @@ const allowedProcessEnvAccesses = normalizeAllowances([ accesses: { 'computed:QWEN_SERVER_TOKEN_ENV': 1 }, }, ], + [ + 'packages/cli/src/serve/session-attachments-root.ts', + { + reason: + 'The session-attachment storage root is a process-scoped daemon setting read once at bridge construction.', + accesses: { 'computed:SESSION_ATTACHMENTS_ROOT_ENV': 1 }, + }, + ], [ 'packages/cli/src/serve/sandbox.ts', { diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index a68cabcf1e2..da64e059ce9 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -83,6 +83,7 @@ import { RUNTIME_STARTUP_CANCELLED_MESSAGE } from './runtime-startup-errors.js'; import { resolveWebShellDir } from './web-shell-resolver.js'; import { resolveServeToken } from './serve-token.js'; import { acpChildExtraArgs } from './acp-child-extra-args.js'; +import { sessionAttachmentsRoots } from './session-attachments-root.js'; import { allowOriginCors, bearerAuth, @@ -3691,14 +3692,6 @@ async function runQwenServeImpl( runtimeBootSettings, runtimeEnvSnapshot.effectiveEnv, ); - const sessionAttachmentsRoot = ( - workspace: string, - runtimeBaseDir: string, - ): string => - path.join( - new core.Storage(workspace, runtimeBaseDir).getProjectTempDir(), - 'attachments', - ); const runtimeEffectiveEnv: NodeJS.ProcessEnv = { ...runtimeEnvSnapshot.effectiveEnv, QWEN_RUNTIME_DIR: primarySessionRuntimeBaseDir, @@ -4294,13 +4287,15 @@ async function runQwenServeImpl( runtimeBootSettings?.merged.serve ?? {}, ), }); + const attachmentsRoots = sessionAttachmentsRoots( + boundWorkspace, + primarySessionRuntimeBaseDir, + ); const bridge = deps.bridge ?? runtime.createAcpSessionBridge({ - sessionAttachmentsRoot: sessionAttachmentsRoot( - boundWorkspace, - primarySessionRuntimeBaseDir, - ), + sessionAttachmentsRoot: attachmentsRoots.root, + sessionAttachmentsFallbackRoot: attachmentsRoots.fallback, // Reverse tool channel: let `BridgeClient.extMethod` reach the WS // connection that hosts a named client MCP server (#5626). clientMcpSender: clientMcpSenderRegistry.lookup, @@ -4743,11 +4738,13 @@ async function runQwenServeImpl( secondarySettings?.merged.serve ?? {}, ), }); + const secondaryAttachmentsRoots = sessionAttachmentsRoots( + workspaceInput.cwd, + secondaryEnv.sessionRuntimeBaseDir, + ); const secondaryBridge = runtime.createAcpSessionBridge({ - sessionAttachmentsRoot: sessionAttachmentsRoot( - workspaceInput.cwd, - secondaryEnv.sessionRuntimeBaseDir, - ), + sessionAttachmentsRoot: secondaryAttachmentsRoots.root, + sessionAttachmentsFallbackRoot: secondaryAttachmentsRoots.fallback, clientMcpSender: secondaryClientMcpSenderRegistry.lookup, onCreateSubSession: secondarySubSessionLauncher.launch, onChannelDelivery: createBoundChannelDeliveryHandler( @@ -5315,11 +5312,13 @@ async function runQwenServeImpl( }); let wsBridge: ReturnType; try { + const wsAttachmentsRoots = sessionAttachmentsRoots( + cwd, + wsEnv.sessionRuntimeBaseDir, + ); wsBridge = runtime.createAcpSessionBridge({ - sessionAttachmentsRoot: sessionAttachmentsRoot( - cwd, - wsEnv.sessionRuntimeBaseDir, - ), + sessionAttachmentsRoot: wsAttachmentsRoots.root, + sessionAttachmentsFallbackRoot: wsAttachmentsRoots.fallback, clientMcpSender: wsClientMcpRegistry.lookup, onCreateSubSession: wsSubSessionLauncher.launch, onChannelDelivery: createBoundChannelDeliveryHandler( diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 1bc1a092652..30491fed3b5 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -123,6 +123,7 @@ import { import { registerPermissionRoutes } from './routes/permission.js'; import { registerSessionRoutes } from './routes/session.js'; import { createRequestedSessionIdAdmission } from './session-id-admission.js'; +import { sessionAttachmentsRoots } from './session-attachments-root.js'; import { registerScheduledTasksRoutes, registerWorkspaceQualifiedScheduledTasksRoutes, @@ -1026,14 +1027,16 @@ export function createServeApp( ? createWorkspaceSessionOwnerIndex() : undefined; const acpChildArgs = acpChildExtraArgs(opts); + const attachmentsRoots = sessionAttachmentsRoots( + boundWorkspace, + Storage.getRuntimeBaseDir(), + ); const bridge = injectedWorkspaceRegistry?.primary.bridge ?? deps.bridge ?? createAcpSessionBridge({ - sessionAttachmentsRoot: path.join( - new Storage(boundWorkspace).getProjectTempDir(), - 'attachments', - ), + sessionAttachmentsRoot: attachmentsRoots.root, + sessionAttachmentsFallbackRoot: attachmentsRoots.fallback, maxSessions: opts.maxSessions, ...(totalSessionAdmission ? { freshSessionAdmission: totalSessionAdmission.admit } diff --git a/packages/cli/src/serve/session-attachments-root.test.ts b/packages/cli/src/serve/session-attachments-root.test.ts new file mode 100644 index 00000000000..94d25f39c85 --- /dev/null +++ b/packages/cli/src/serve/session-attachments-root.test.ts @@ -0,0 +1,116 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { tmpdir, homedir } from 'node:os'; +import * as path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { + SESSION_ATTACHMENTS_ROOT_ENV, + defaultSessionAttachmentsRoot, + resolveConfiguredSessionAttachmentsRoot, + sessionAttachmentsRoots, +} from './session-attachments-root.js'; + +const originalEnvValue = process.env[SESSION_ATTACHMENTS_ROOT_ENV]; + +beforeEach(() => { + delete process.env[SESSION_ATTACHMENTS_ROOT_ENV]; +}); + +afterEach(() => { + if (originalEnvValue === undefined) { + delete process.env[SESSION_ATTACHMENTS_ROOT_ENV]; + } else { + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = originalEnvValue; + } +}); + +describe('session attachment root resolution', () => { + const workspace = path.join(tmpdir(), 'qwen-attachments-workspace'); + const runtimeBaseDir = path.join(tmpdir(), 'qwen-runtime'); + const configuredRoot = path.join(tmpdir(), 'qwen-configured-attachments'); + const defaultRoot = defaultSessionAttachmentsRoot(workspace, runtimeBaseDir); + const projectHash = path.basename(path.dirname(defaultRoot)); + + it('defaults to the runtime temp dir under the workspace hash when unset', () => { + expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ + root: defaultRoot, + }); + }); + + it('treats an empty env value as unset', () => { + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = ''; + expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ + root: defaultRoot, + }); + }); + + it('uses an absolute configured root and falls back to the default', () => { + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = configuredRoot; + expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ + root: path.join(configuredRoot, projectHash, 'attachments'), + fallback: defaultRoot, + }); + }); + + it('expands a leading tilde to the home directory', () => { + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = '~/attachments'; + expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ + root: path.join(homedir(), 'attachments', projectHash, 'attachments'), + fallback: defaultRoot, + }); + }); + + it('resolves a bare tilde to the home directory itself', () => { + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = '~'; + expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ + root: path.join(homedir(), projectHash, 'attachments'), + fallback: defaultRoot, + }); + }); + + it('resolves relative paths against the process cwd', () => { + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = 'relative/attachments'; + expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ + root: path.resolve( + process.cwd(), + 'relative/attachments', + projectHash, + 'attachments', + ), + fallback: defaultRoot, + }); + }); + + it('keeps the workspace hash segment separate per workspace', () => { + const otherWorkspace = path.join( + tmpdir(), + 'qwen-attachments-other-workspace', + ); + const otherHash = path.basename( + path.dirname( + defaultSessionAttachmentsRoot(otherWorkspace, runtimeBaseDir), + ), + ); + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = configuredRoot; + expect(sessionAttachmentsRoots(otherWorkspace, runtimeBaseDir).root).toBe( + path.join(configuredRoot, otherHash, 'attachments'), + ); + expect( + sessionAttachmentsRoots(otherWorkspace, runtimeBaseDir).root, + ).not.toBe(sessionAttachmentsRoots(workspace, runtimeBaseDir).root); + }); + + it('expands a bare tilde in the standalone resolver', () => { + expect(resolveConfiguredSessionAttachmentsRoot('~')).toBe(homedir()); + }); + + it('keeps an absolute path unchanged in the standalone resolver', () => { + expect(resolveConfiguredSessionAttachmentsRoot(configuredRoot)).toBe( + configuredRoot, + ); + }); +}); diff --git a/packages/cli/src/serve/session-attachments-root.ts b/packages/cli/src/serve/session-attachments-root.ts new file mode 100644 index 00000000000..82cd5c3901e --- /dev/null +++ b/packages/cli/src/serve/session-attachments-root.ts @@ -0,0 +1,58 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { homedir } from 'node:os'; +import * as path from 'node:path'; +import { Storage } from '@qwen-code/qwen-code-core'; + +export const SESSION_ATTACHMENTS_ROOT_ENV = + 'QWEN_SERVE_SESSION_ATTACHMENTS_ROOT'; + +export function defaultSessionAttachmentsRoot( + workspace: string, + runtimeBaseDir: string, +): string { + return path.join( + new Storage(workspace, runtimeBaseDir).getProjectTempDir(), + 'attachments', + ); +} + +export function resolveConfiguredSessionAttachmentsRoot( + configured: string, +): string { + const expanded = + configured === '~' + ? homedir() + : configured.startsWith('~/') || configured.startsWith('~\\') + ? path.join( + homedir(), + ...configured + .slice(2) + .split(/[/\\]+/) + .filter(Boolean), + ) + : configured; + return path.resolve(process.cwd(), expanded); +} + +export function sessionAttachmentsRoots( + workspace: string, + runtimeBaseDir: string, +): { root: string; fallback?: string } { + const defaultRoot = defaultSessionAttachmentsRoot(workspace, runtimeBaseDir); + const configured = process.env[SESSION_ATTACHMENTS_ROOT_ENV]; + if (!configured) return { root: defaultRoot }; + const projectHash = path.basename(path.dirname(defaultRoot)); + return { + root: path.join( + resolveConfiguredSessionAttachmentsRoot(configured), + projectHash, + 'attachments', + ), + fallback: defaultRoot, + }; +} From da506f7ad9f6337e1588179aae1eb2cbd46e080a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=89=E8=90=81?= Date: Wed, 26 Aug 2026 11:30:41 +0800 Subject: [PATCH 2/7] fix(serve): keep attachment root resolver off fast path --- packages/cli/src/serve/run-qwen-serve.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index da64e059ce9..73cdc792d91 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -83,7 +83,6 @@ import { RUNTIME_STARTUP_CANCELLED_MESSAGE } from './runtime-startup-errors.js'; import { resolveWebShellDir } from './web-shell-resolver.js'; import { resolveServeToken } from './serve-token.js'; import { acpChildExtraArgs } from './acp-child-extra-args.js'; -import { sessionAttachmentsRoots } from './session-attachments-root.js'; import { allowOriginCors, bearerAuth, @@ -4277,6 +4276,9 @@ async function runQwenServeImpl( const { createSubSessionLauncher } = await import( './create-sub-session.js' ); + const { sessionAttachmentsRoots } = await import( + './session-attachments-root.js' + ); // Late-binds the bridge (constructed just below) via `() => bridgeRef`. Only // wired on the daemon-created bridge — an injected `deps.bridge` (embed/test) // brings its own options. From 97e64bc83e3711f998f9f89e4c0b92e91860b2af Mon Sep 17 00:00:00 2001 From: Qwen Code Autofix Date: Wed, 26 Aug 2026 08:14:49 +0000 Subject: [PATCH 3/7] fix(serve): harden session attachment fallback against degraded roots read() and remove() no longer force-create the configured root before consulting the fallback, so a degraded configured volume serves and removes pre-switch attachments from the healthy default dir instead of failing; delete() removes the fallback root first, mirroring remove(), so a failure on the legacy root keeps the primary copy intact; QWEN_SERVE_SESSION_ATTACHMENTS_ROOT is trimmed before use. Corrects the docs to say attachment cleanup happens on session delete, not archive. --- docs/users/qwen-serve.md | 2 +- .../acp-bridge/src/sessionAttachments.test.ts | 187 +++++++++++++++++- packages/acp-bridge/src/sessionAttachments.ts | 34 +++- .../serve/session-attachments-root.test.ts | 23 +++ .../cli/src/serve/session-attachments-root.ts | 2 +- 5 files changed, 229 insertions(+), 19 deletions(-) diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 56139711cb5..7f1732cc64d 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -717,7 +717,7 @@ Scope and limits: - **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable and removable. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. - **Per-session layout.** Files live under `//attachments/session-/` in both locations, where `` is the same workspace hash used by the default runtime temp dir; the fallback lookup uses the same session layout in the default dir. Two workspaces pointing at the same configured root stay isolated from each other. -- **Archive cleanup.** When a session is archived, its attachment directory is removed from both the configured root and the default fallback dir. +- **Delete cleanup.** When a session is deleted, its attachment directory is removed from both the configured root and the default fallback dir. Archiving a session keeps its attachments so they survive unarchive. - The daemon reads the variable at startup; restart the daemon after changing it. The directory must be writable by the daemon process. ## Multi-session & multi-workspace deployment diff --git a/packages/acp-bridge/src/sessionAttachments.test.ts b/packages/acp-bridge/src/sessionAttachments.test.ts index 30da05f60a0..80c89f9f89d 100644 --- a/packages/acp-bridge/src/sessionAttachments.test.ts +++ b/packages/acp-bridge/src/sessionAttachments.test.ts @@ -1158,6 +1158,70 @@ describe('SessionAttachmentStore', () => { } }); + it('reads the fallback when the primary root cannot be created', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + await writeIn(fallback, 'notes.txt', 'legacy bytes'); + // A degraded configured volume must not fail a read that the healthy + // fallback can serve; the read path must not force-create the + // primary directory. + const mkdir = vi.spyOn(fs, 'mkdir').mockRejectedValueOnce( + Object.assign(new Error('permission denied'), { + code: 'EACCES', + }), + ); + try { + expect(await store.read('notes.txt')).toEqual({ + data: Buffer.from('legacy bytes'), + mimeType: 'text/plain', + }); + expect(mkdir).not.toHaveBeenCalled(); + } finally { + mkdir.mockRestore(); + } + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('reads the fallback when an established primary root degrades', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + await store.putAttachment( + new TextEncoder().encode('current'), + 'text/plain', + 'current.txt', + ); + await writeIn(fallback, 'notes.txt', 'legacy bytes'); + // A non-ENOENT primary read failure (the volume degraded after boot) + // must degrade to the fallback instead of rejecting. + const readFile = vi + .spyOn(fs, 'readFile') + .mockRejectedValueOnce( + Object.assign(new Error('volume degraded'), { code: 'EIO' }), + ); + try { + expect(await store.read('notes.txt')).toEqual({ + data: Buffer.from('legacy bytes'), + mimeType: 'text/plain', + }); + expect(readFile.mock.calls[0]?.[0]).toBe( + path.join(main, sessionDir, 'notes.txt'), + ); + } finally { + readFile.mockRestore(); + } + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + it('prefers the primary root over the fallback', async () => { const { main, fallback } = await createRoots(); const store = new SessionAttachmentStore(main, sessionId, fallback); @@ -1173,6 +1237,27 @@ describe('SessionAttachmentStore', () => { data: Buffer.from('primary'), mimeType: 'text/plain', }); + + // With both roots holding the name, the authoritative primary + // reference must validate while the stale fallback size must not. + expect(() => + store.assertReference({ + type: 'resource', + attachmentId: reference.attachmentId, + mimeType: 'text/plain', + size: 7, + }), + ).not.toThrow(); + expect(() => + store.assertReference({ + type: 'resource', + attachmentId: reference.attachmentId, + mimeType: 'text/plain', + size: 14, + }), + ).toThrowError( + expect.objectContaining({ code: 'session_attachment_gone' }), + ); } finally { await store.close(); await fs.rm(main, { recursive: true, force: true }); @@ -1238,24 +1323,41 @@ describe('SessionAttachmentStore', () => { it('still degrades reference validation when the fallback stat fails', async () => { const { main, fallback } = await createRoots(); const store = new SessionAttachmentStore(main, sessionId, fallback); - const stat = vi.mocked(statSync).mockImplementationOnce(() => { - throw Object.assign(new Error('permission denied'), { code: 'EACCES' }); - }); + // Arm the faults per call order: ENOENT for the primary stat, EACCES + // for the fallback stat the test name targets. + const stat = vi + .mocked(statSync) + .mockImplementationOnce(() => { + throw Object.assign(new Error('missing'), { code: 'ENOENT' }); + }) + .mockImplementationOnce(() => { + throw Object.assign(new Error('permission denied'), { + code: 'EACCES', + }); + }); try { await writeIn(fallback, 'notes.txt', 'stale fallback'); // Reference validation must degrade to session_attachment_gone, not - // surface the raw stat error and abort the prompt. + // surface the raw stat error and abort the prompt. The reference size + // matches the fallback file, so the throw can only come from the + // degradation path. expect(() => store.assertReference({ type: 'resource', attachmentId: 'notes.txt', mimeType: 'text/plain', - size: 13, + size: 14, }), ).toThrowError( expect.objectContaining({ code: 'session_attachment_gone' }), ); + expect(stat.mock.calls[0]?.[0]).toBe( + path.join(main, sessionDir, 'notes.txt'), + ); + expect(stat.mock.calls[1]?.[0]).toBe( + path.join(fallback, sessionDir, 'notes.txt'), + ); } finally { stat.mockRestore(); await store.close(); @@ -1292,6 +1394,34 @@ describe('SessionAttachmentStore', () => { } }); + it('removes a fallback attachment when the primary root cannot be created', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + try { + await writeIn(fallback, 'notes.txt', 'legacy bytes'); + // The removal must not force-create the primary directory: a + // degraded configured volume must not fail a deletion whose only + // copy lives in the healthy fallback. + const mkdir = vi.spyOn(fs, 'mkdir').mockRejectedValueOnce( + Object.assign(new Error('permission denied'), { + code: 'EACCES', + }), + ); + try { + expect(await store.remove('notes.txt')).toBe(true); + expect(mkdir).not.toHaveBeenCalled(); + expect(await fs.readdir(path.join(fallback, sessionDir))).toEqual([]); + expect(await fs.readdir(main)).toEqual([]); + } finally { + mkdir.mockRestore(); + } + } finally { + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + it('removes both copies when both roots hold the same name', async () => { const { main, fallback } = await createRoots(); const store = new SessionAttachmentStore(main, sessionId, fallback); @@ -1313,18 +1443,26 @@ describe('SessionAttachmentStore', () => { it('keeps the primary readable when the fallback unlink fails', async () => { const { main, fallback } = await createRoots(); const store = new SessionAttachmentStore(main, sessionId, fallback); + const realUnlink = fs.unlink.bind(fs); const unlink = vi .spyOn(fs, 'unlink') - .mockRejectedValueOnce( - Object.assign(new Error('read-only volume'), { code: 'EROFS' }), - ); + .mockImplementation(async (filePath) => { + if (String(filePath).startsWith(path.join(fallback, sessionDir))) { + throw Object.assign(new Error('read-only volume'), { + code: 'EROFS', + }); + } + return realUnlink(filePath); + }); try { await writeIn(main, 'notes.txt', 'from primary'); await writeIn(fallback, 'notes.txt', 'stale fallback copy'); // The fallback copy cannot be removed; remove() must fail cleanly and // leave the authoritative primary copy readable instead of deleting it - // and resurrecting stale fallback bytes on the next read. + // and resurrecting stale fallback bytes on the next read. Rejecting + // only fallback-targeted unlinks also pins the fallback-first order: + // a primary-first remove() would really delete the primary copy. await expect(store.remove('notes.txt')).rejects.toThrow( 'read-only volume', ); @@ -1375,6 +1513,37 @@ describe('SessionAttachmentStore', () => { ).rejects.toMatchObject({ code: 'ENOENT' }); }); + it('keeps the primary directory intact when the fallback removal fails', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + await writeIn(main, 'current.txt', 'primary data'); + await writeIn(fallback, 'legacy.txt', 'legacy data'); + const realRename = fs.rename.bind(fs); + const rename = vi + .spyOn(fs, 'rename') + .mockImplementation(async (from, to) => { + if (String(from).startsWith(fallback)) { + throw Object.assign(new Error('read-only volume'), { + code: 'EROFS', + }); + } + return realRename(from, to); + }); + try { + // delete() removes the fallback root first so a failure there keeps + // the authoritative primary copy intact. + await expect(store.delete()).rejects.toMatchObject({ code: 'EROFS' }); + expect( + await fs.readFile(path.join(main, sessionDir, 'current.txt'), 'utf8'), + ).toBe('primary data'); + } finally { + rename.mockRestore(); + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + it('delete tombstones both roots so a recreated session dir survives', async () => { const { main, fallback } = await createRoots(); const store = new SessionAttachmentStore(main, sessionId, fallback); diff --git a/packages/acp-bridge/src/sessionAttachments.ts b/packages/acp-bridge/src/sessionAttachments.ts index 15ec8bdde7b..0fd3f91d20d 100644 --- a/packages/acp-bridge/src/sessionAttachments.ts +++ b/packages/acp-bridge/src/sessionAttachments.ts @@ -475,7 +475,15 @@ export class SessionAttachmentStore { ): Promise<{ data: Buffer; mimeType: string } | undefined> { const name = safeAttachmentName(attachmentId); if (!name || name !== attachmentId) return undefined; - const primary = await this.tryRead(await this.directory(), name); + let primary: { data: Buffer; mimeType: string } | undefined; + try { + primary = await this.tryRead(await this.peekDirectory(), name); + } catch (error) { + if (!this.persistentFallbackDirectory) throw error; + // A degraded primary root must not hide healthy fallback bytes: any + // primary lookup failure degrades to the fallback read. + primary = undefined; + } if (primary) return primary; return await this.tryRead(this.persistentFallbackDirectory, name); } @@ -597,7 +605,7 @@ export class SessionAttachmentStore { const fallbackHit = (await this.tryUnlink(this.persistentFallbackDirectory, name)) === true; const primaryHit = - (await this.tryUnlink(await this.directory(), name)) === true; + (await this.tryUnlink(await this.peekDirectory(), name)) === true; return primaryHit || fallbackHit; } @@ -640,6 +648,14 @@ export class SessionAttachmentStore { this.pendingNames.clear(); this.resolvePendingDrainWaiters(); } + // Fallback first, mirroring remove(): if the legacy root cannot be + // removed, the authoritative primary copy must stay intact. + if (this.persistentFallbackDirectory) { + await this.removeDirectoryWithTombstone( + this.persistentFallbackDirectory, + options.assertCanCommit, + ); + } const directory = this.persistentDirectory ?? (await this.directoryPromise?.catch(() => undefined)); @@ -649,12 +665,6 @@ export class SessionAttachmentStore { options.assertCanCommit, ); } - if (this.persistentFallbackDirectory) { - await this.removeDirectoryWithTombstone( - this.persistentFallbackDirectory, - options.assertCanCommit, - ); - } } /** @@ -765,6 +775,14 @@ export class SessionAttachmentStore { } as ContentBlock; } + // The storage directory without forcing creation: reads and removes must + // degrade to the fallback when the configured root is unavailable, not + // fail on a forced mkdir of a degraded volume. + private async peekDirectory(): Promise { + const established = await this.directoryPromise?.catch(() => undefined); + return established ?? this.persistentDirectory; + } + private async directory(): Promise { if (!this.directoryPromise) { const pending = this.persistentDirectory diff --git a/packages/cli/src/serve/session-attachments-root.test.ts b/packages/cli/src/serve/session-attachments-root.test.ts index 94d25f39c85..e96dde15984 100644 --- a/packages/cli/src/serve/session-attachments-root.test.ts +++ b/packages/cli/src/serve/session-attachments-root.test.ts @@ -41,6 +41,14 @@ describe('session attachment root resolution', () => { }); }); + it('pins the default root to the legacy runtime temp layout', () => { + // The fallback only works while the default root equals the pre-env + // layout; assert it from the raw segments, not via the resolver. + expect(defaultRoot).toBe( + path.join(runtimeBaseDir, 'tmp', projectHash, 'attachments'), + ); + }); + it('treats an empty env value as unset', () => { process.env[SESSION_ATTACHMENTS_ROOT_ENV] = ''; expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ @@ -48,6 +56,21 @@ describe('session attachment root resolution', () => { }); }); + it('treats a whitespace-only env value as unset', () => { + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = ' '; + expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ + root: defaultRoot, + }); + }); + + it('trims surrounding whitespace from a configured root', () => { + process.env[SESSION_ATTACHMENTS_ROOT_ENV] = ` ${configuredRoot} `; + expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ + root: path.join(configuredRoot, projectHash, 'attachments'), + fallback: defaultRoot, + }); + }); + it('uses an absolute configured root and falls back to the default', () => { process.env[SESSION_ATTACHMENTS_ROOT_ENV] = configuredRoot; expect(sessionAttachmentsRoots(workspace, runtimeBaseDir)).toEqual({ diff --git a/packages/cli/src/serve/session-attachments-root.ts b/packages/cli/src/serve/session-attachments-root.ts index 82cd5c3901e..2a4cd9ca01d 100644 --- a/packages/cli/src/serve/session-attachments-root.ts +++ b/packages/cli/src/serve/session-attachments-root.ts @@ -44,7 +44,7 @@ export function sessionAttachmentsRoots( runtimeBaseDir: string, ): { root: string; fallback?: string } { const defaultRoot = defaultSessionAttachmentsRoot(workspace, runtimeBaseDir); - const configured = process.env[SESSION_ATTACHMENTS_ROOT_ENV]; + const configured = process.env[SESSION_ATTACHMENTS_ROOT_ENV]?.trim(); if (!configured) return { root: defaultRoot }; const projectHash = path.basename(path.dirname(defaultRoot)); return { From 3b522e4c3076ed63912911db04cde79596698e9e Mon Sep 17 00:00:00 2001 From: Qwen Code Autofix Date: Wed, 26 Aug 2026 15:05:15 +0000 Subject: [PATCH 4/7] fix(serve): address round-2 review findings on session attachment storage (#10066) Co-authored-by: Qwen-Coder --- docs/users/qwen-serve.md | 2 +- .../acp-bridge/src/sessionAttachments.test.ts | 16 +++++---- packages/acp-bridge/src/sessionAttachments.ts | 22 ++++++++---- .../cli/src/config/shared-env-keys.test.ts | 13 +++++++ packages/cli/src/config/shared-env-keys.ts | 8 +++++ packages/cli/src/serve/fast-path.test.ts | 36 +++++++++++++++++++ .../src/serve/server/session-archive.test.ts | 12 +++++++ .../cli/src/serve/server/session-archive.ts | 14 ++++++-- 8 files changed, 108 insertions(+), 15 deletions(-) diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index fef04616a92..b44ba3a6dc3 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -717,7 +717,7 @@ Session attachments (files and images uploaded by Web Shell through `POST /sessi Scope and limits: -- **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable and removable. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. +- **One-way migration.** When the env is set, new attachments are written only under the configured root. Reads and removes that miss the configured root fall back to the default runtime temp dir, so attachments uploaded **before** the switch remain readable, and removable while the default fallback dir stays writable — a remove whose legacy copy cannot be unlinked (e.g. a read-only fallback volume) surfaces the error rather than reporting success. The reverse direction — removing the env after attachments were written to the configured root — makes those attachments unreachable; keep the variable stable for a given workspace. - **Per-session layout.** Files live under `//attachments/session-/` in both locations, where `` is the same workspace hash used by the default runtime temp dir; the fallback lookup uses the same session layout in the default dir. Two workspaces pointing at the same configured root stay isolated from each other. - **Delete cleanup.** When a session is deleted, its attachment directory is removed from both the configured root and the default fallback dir. Archiving a session keeps its attachments so they survive unarchive. - The daemon reads the variable at startup; restart the daemon after changing it. The directory must be writable by the daemon process. diff --git a/packages/acp-bridge/src/sessionAttachments.test.ts b/packages/acp-bridge/src/sessionAttachments.test.ts index 80c89f9f89d..38a093156c0 100644 --- a/packages/acp-bridge/src/sessionAttachments.test.ts +++ b/packages/acp-bridge/src/sessionAttachments.test.ts @@ -1513,7 +1513,7 @@ describe('SessionAttachmentStore', () => { ).rejects.toMatchObject({ code: 'ENOENT' }); }); - it('keeps the primary directory intact when the fallback removal fails', async () => { + it('still clears the primary directory when the fallback removal fails', async () => { const { main, fallback } = await createRoots(); const store = new SessionAttachmentStore(main, sessionId, fallback); await writeIn(main, 'current.txt', 'primary data'); @@ -1530,12 +1530,16 @@ describe('SessionAttachmentStore', () => { return realRename(from, to); }); try { - // delete() removes the fallback root first so a failure there keeps - // the authoritative primary copy intact. + // Session deletion removes the persisted row first, so a legacy + // fault must not skip the configured root's cleanup: the primary + // directory is removed and the fallback failure still rejects. await expect(store.delete()).rejects.toMatchObject({ code: 'EROFS' }); - expect( - await fs.readFile(path.join(main, sessionDir, 'current.txt'), 'utf8'), - ).toBe('primary data'); + await expect( + fs.readdir(path.join(main, sessionDir)), + ).rejects.toMatchObject({ code: 'ENOENT' }); + expect(await fs.readdir(path.join(fallback, sessionDir))).toEqual([ + 'legacy.txt', + ]); } finally { rename.mockRestore(); await store.close(); diff --git a/packages/acp-bridge/src/sessionAttachments.ts b/packages/acp-bridge/src/sessionAttachments.ts index 0fd3f91d20d..59bf3eeb294 100644 --- a/packages/acp-bridge/src/sessionAttachments.ts +++ b/packages/acp-bridge/src/sessionAttachments.ts @@ -648,13 +648,22 @@ export class SessionAttachmentStore { this.pendingNames.clear(); this.resolvePendingDrainWaiters(); } - // Fallback first, mirroring remove(): if the legacy root cannot be - // removed, the authoritative primary copy must stay intact. + // Fallback removal is best-effort here (unlike remove()): the caller + // removes the persisted session row first (deleteDaemonSessions), so a + // legacy-volume fault must not skip the configured root's cleanup — a + // retry would throw SessionNotFoundError and orphan the configured + // bytes for good. A fallback failure still rejects after the primary + // is removed. + let fallbackError: unknown; if (this.persistentFallbackDirectory) { - await this.removeDirectoryWithTombstone( - this.persistentFallbackDirectory, - options.assertCanCommit, - ); + try { + await this.removeDirectoryWithTombstone( + this.persistentFallbackDirectory, + options.assertCanCommit, + ); + } catch (error) { + fallbackError = error; + } } const directory = this.persistentDirectory ?? @@ -665,6 +674,7 @@ export class SessionAttachmentStore { options.assertCanCommit, ); } + if (fallbackError !== undefined) throw fallbackError; } /** diff --git a/packages/cli/src/config/shared-env-keys.test.ts b/packages/cli/src/config/shared-env-keys.test.ts index 73d1a04ab11..d19bb03302e 100644 --- a/packages/cli/src/config/shared-env-keys.test.ts +++ b/packages/cli/src/config/shared-env-keys.test.ts @@ -97,6 +97,19 @@ describe('PROJECT_ENV_HARDCODED_EXCLUSIONS', () => { ); }); + // QWEN_SERVE_SESSION_ATTACHMENTS_ROOT is the daemon-wide attachment + // storage location. A project `.env` redirecting it would capture uploads + // for every workspace the daemon serves (and serve back tampered bytes on + // reads), so only the daemon's launch env or a home `.env` may set it. + it('excludes QWEN_SERVE_SESSION_ATTACHMENTS_ROOT so a project .env cannot redirect attachment storage', () => { + expect(PROJECT_ENV_HARDCODED_EXCLUSIONS).toContain( + 'QWEN_SERVE_SESSION_ATTACHMENTS_ROOT', + ); + expect( + isHardcodedProjectEnvExclusion('qwen_serve_session_attachments_root'), + ).toBe(true); + }); + // The non-Node TLS trust-anchor vars reach the same MITM outcome as // NODE_EXTRA_CA_CERTS for the curl/git/openssl/python tools a session // shells out to; a project .env must not inject an attacker CA. diff --git a/packages/cli/src/config/shared-env-keys.ts b/packages/cli/src/config/shared-env-keys.ts index a05d8a0a602..42cd763d28b 100644 --- a/packages/cli/src/config/shared-env-keys.ts +++ b/packages/cli/src/config/shared-env-keys.ts @@ -192,6 +192,14 @@ export const PROJECT_ENV_HARDCODED_EXCLUSIONS = [ // (documented as a per-daemon opt-in), so only the daemon's launch // environment or a home `.env` may set it. 'QWEN_SERVE_NEW_FILE_MODE', + // QWEN_SERVE_SESSION_ATTACHMENTS_ROOT decides where the daemon stores + // every workspace's session attachments. A project `.env` redirecting it + // would capture uploads for ALL workspaces the daemon serves — and reads + // consult the configured root first, so attacker-modified bytes would be + // served back — with no recovery after unsetting the variable. The + // location is an operator decision (documented as a per-daemon opt-in), so + // only the daemon's launch environment or a home `.env` may set it. + 'QWEN_SERVE_SESSION_ATTACHMENTS_ROOT', // DEV gates the daemon's inherited-loader-env scrub (run-qwen-serve.ts); // only the dev harness (scripts/dev.js) stamps it into the launch env. A // project file setting it would silently keep loader vars in the base env diff --git a/packages/cli/src/serve/fast-path.test.ts b/packages/cli/src/serve/fast-path.test.ts index 4e9849eef36..cc013ce8dad 100644 --- a/packages/cli/src/serve/fast-path.test.ts +++ b/packages/cli/src/serve/fast-path.test.ts @@ -1932,6 +1932,42 @@ describe('serve fast path environment bootstrap', () => { } }); + // QWEN_SERVE_SESSION_ATTACHMENTS_ROOT is the daemon-wide attachment + // storage location: a start-dir .env fixing it redirects storage for every + // workspace the daemon serves, and reads resolve the configured root first + // — an attacker repo would capture uploads and serve back tampered bytes. + it('never applies QWEN_SERVE_SESSION_ATTACHMENTS_ROOT from a project .env on the fast path', () => { + useTempQwenHome(); + const trackedKeys = ['QWEN_SERVE_SESSION_ATTACHMENTS_ROOT'] as const; + const previous: Record = {}; + for (const key of trackedKeys) { + previous[key] = process.env[key]; + delete process.env[key]; + } + tempWorkspace = realpathSync( + mkdtempSync(join(os.tmpdir(), 'qws-fast-path-attachments-root-')), + ); + writeFileSync( + join(tempWorkspace, '.env'), + ['QWEN_SERVE_SESSION_ATTACHMENTS_ROOT=./exfil', ''].join('\n'), + ); + + try { + loadServeFastPathEnvironment({}, tempWorkspace); + expect( + process.env['QWEN_SERVE_SESSION_ATTACHMENTS_ROOT'], + ).toBeUndefined(); + } finally { + for (const key of trackedKeys) { + if (previous[key] === undefined) { + delete process.env[key]; + } else { + process.env[key] = previous[key]; + } + } + } + }); + // The fast-path settings.env loop rejects hardcoded exclusions through the // case-folded isHardcodedProjectEnvExclusion predicate. Every other // settings.env fixture uses loader/allowlisted keys, so a regression to diff --git a/packages/cli/src/serve/server/session-archive.test.ts b/packages/cli/src/serve/server/session-archive.test.ts index 343e8d26cef..18272132751 100644 --- a/packages/cli/src/serve/server/session-archive.test.ts +++ b/packages/cli/src/serve/server/session-archive.test.ts @@ -1651,6 +1651,7 @@ describe('deleteDaemonSessions', () => { writeSessionFile(workspaceDir, sessionId, 'active'); const service = new SessionService(workspaceDir); const acquire = vi.spyOn(service, 'acquireSessionWriterLease'); + const deleteSessionAttachments = vi.fn().mockResolvedValue(undefined); await expect( deleteDaemonSessionIfOrphan({ @@ -1659,11 +1660,13 @@ describe('deleteDaemonSessions', () => { bridge: { killSession: vi.fn().mockResolvedValue(false), markSessionCatalogChanged: vi.fn(), + deleteSessionAttachments, }, coordinator: new SessionArchiveCoordinator(), }), ).resolves.toBe(false); expect(acquire).not.toHaveBeenCalled(); + expect(deleteSessionAttachments).not.toHaveBeenCalled(); expect(fs.existsSync(sessionPath(workspaceDir, sessionId, 'active'))).toBe( true, ); @@ -1696,6 +1699,7 @@ describe('deleteDaemonSessions', () => { writeSessionFile(workspaceDir, sessionId, 'active'); const service = new SessionService(workspaceDir); const markSessionCatalogChanged = vi.fn(); + const deleteSessionAttachments = vi.fn().mockResolvedValue(undefined); await expect( deleteDaemonSessionIfOrphan({ @@ -1704,6 +1708,7 @@ describe('deleteDaemonSessions', () => { bridge: { killSession: vi.fn().mockResolvedValue(true), markSessionCatalogChanged, + deleteSessionAttachments, }, coordinator: new SessionArchiveCoordinator(), }), @@ -1712,6 +1717,10 @@ describe('deleteDaemonSessions', () => { false, ); expect(markSessionCatalogChanged).toHaveBeenCalledTimes(1); + // The reaped orphan is never looked up again; its attachment bytes must + // go with the persisted row. + expect(deleteSessionAttachments).toHaveBeenCalledTimes(1); + expect(deleteSessionAttachments).toHaveBeenCalledWith(sessionId); }); it('returns true when task maintenance fails after orphan deletion', async () => { @@ -1740,6 +1749,7 @@ describe('deleteDaemonSessions', () => { bridge: { killSession: vi.fn().mockResolvedValue(true), markSessionCatalogChanged, + deleteSessionAttachments: vi.fn().mockResolvedValue(undefined), }, coordinator: new SessionArchiveCoordinator(), }), @@ -1765,6 +1775,7 @@ describe('deleteDaemonSessions', () => { .fn() .mockRejectedValue(new SessionNotFoundError(sessionId)), markSessionCatalogChanged, + deleteSessionAttachments: vi.fn().mockResolvedValue(undefined), }, coordinator: new SessionArchiveCoordinator(), }), @@ -1793,6 +1804,7 @@ describe('deleteDaemonSessions', () => { bridge: { killSession: vi.fn().mockResolvedValue(true), markSessionCatalogChanged: vi.fn(), + deleteSessionAttachments: vi.fn().mockResolvedValue(undefined), }, coordinator: new SessionArchiveCoordinator(), }), diff --git a/packages/cli/src/serve/server/session-archive.ts b/packages/cli/src/serve/server/session-archive.ts index 1040d1d443e..285de55196f 100644 --- a/packages/cli/src/serve/server/session-archive.ts +++ b/packages/cli/src/serve/server/session-archive.ts @@ -622,7 +622,10 @@ export async function deleteDaemonSessions(params: { export async function deleteDaemonSessionIfOrphan(params: { sessionId: string; service: SessionService; - bridge: Pick; + bridge: Pick< + AcpSessionBridge, + 'killSession' | 'markSessionCatalogChanged' | 'deleteSessionAttachments' + >; coordinator: SessionArchiveCoordinator; }): Promise { const { sessionId, service, bridge, coordinator } = params; @@ -640,7 +643,14 @@ export async function deleteDaemonSessionIfOrphan(params: { if (!killed) { return undefined; } - return deletePersistedSessionWithLease(service, sessionId); + const removal = await deletePersistedSessionWithLease(service, sessionId); + if (removal.kind !== 'error') { + // Mirror deleteDaemonSessions: a reaped orphan is never looked up + // again, and close() on a persistent store deletes nothing — without + // this the attachment bytes leak from both storage roots. + await bridge.deleteSessionAttachments(sessionId); + } + return removal; }); if (result === undefined) { return false; From 0f3e4a36775ff70e1650eb9875492a73e2477aee Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 27 Aug 2026 22:18:30 +0000 Subject: [PATCH 5/7] fix(cli): stop restore-probe tests depending on ambient host git state (#10066) --- .../src/commands/review/test-efficacy.test.ts | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index b7f1772b01e..2f4821e2f7c 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { execFileSync, spawnSync } from 'node:child_process'; import { replacementMutantsOf, @@ -511,6 +511,22 @@ describe('probeCleanupFailureDetail', () => { }); describe('restoreProbeTreeTracked, through runOneMutant', () => { + // Ambient host git state must not reach this fixture: the restore's own + // git spawns sanitize their env BY DESIGN — dropping the GIT_CONFIG_* + // redirects and reading whatever $HOME carries — so the refusal these + // tests pin can be decided by the host instead of the code (the incident + // class `isolateHostGitConfig` was written for; every sibling real-git + // suite isolates). + let gitIsolation: ReturnType; + + beforeEach(() => { + gitIsolation = isolateHostGitConfig(); + }); + + afterEach(() => { + gitIsolation.dispose(); + }); + it('refuses to run when the tree carries NO .git at all', () => { // The state the tree can never be put back from, and the cheapest one to // reach: `.git` is an untracked pointer file inside the directory the PR's From 6610d34c941d8f326169d1474c39183fc964f6e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=89=E8=90=81?= Date: Fri, 28 Aug 2026 21:55:59 +0800 Subject: [PATCH 6/7] fix(acp): stabilize attachment fallback handling --- .../acp-bridge/src/sessionAttachments.test.ts | 162 ++++++++++++++++++ packages/acp-bridge/src/sessionAttachments.ts | 90 ++++++---- .../src/commands/review/test-efficacy.test.ts | 18 +- 3 files changed, 216 insertions(+), 54 deletions(-) diff --git a/packages/acp-bridge/src/sessionAttachments.test.ts b/packages/acp-bridge/src/sessionAttachments.test.ts index 6b954296b54..7171c2aa0da 100644 --- a/packages/acp-bridge/src/sessionAttachments.test.ts +++ b/packages/acp-bridge/src/sessionAttachments.test.ts @@ -1882,5 +1882,167 @@ describe('SessionAttachmentStore', () => { await fs.rm(targetRoot, { recursive: true, force: true }); } }); + + it('does not unlink an absent root when the other root holds the attachment', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + const realUnlink = fs.unlink.bind(fs); + const unlink = vi + .spyOn(fs, 'unlink') + .mockImplementation(async (filePath) => { + if (String(filePath).startsWith(fallback)) { + throw Object.assign(new Error('fallback unavailable'), { + code: 'EACCES', + }); + } + return realUnlink(filePath); + }); + try { + await writeIn(main, 'primary.txt', 'primary'); + await expect(store.remove('primary.txt')).resolves.toBe(true); + + unlink.mockImplementation(async (filePath) => { + if (String(filePath).startsWith(main)) { + throw Object.assign(new Error('primary unavailable'), { + code: 'EACCES', + }); + } + return realUnlink(filePath); + }); + await writeIn(fallback, 'legacy.txt', 'legacy'); + await expect(store.remove('legacy.txt')).resolves.toBe(true); + } finally { + unlink.mockRestore(); + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('does not mutate either copy when a root lookup is denied', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + await writeIn(main, 'notes.txt', 'primary'); + await writeIn(fallback, 'notes.txt', 'fallback'); + const realStat = fs.stat.bind(fs); + const stat = vi + .spyOn(fs, 'stat') + .mockImplementation((filePath, options) => { + if (String(filePath).startsWith(fallback)) { + throw Object.assign(new Error('fallback unavailable'), { + code: 'EACCES', + }); + } + return realStat(filePath, options); + }); + try { + await expect(store.remove('notes.txt')).rejects.toThrow( + 'fallback unavailable', + ); + expect( + await fs.readFile(path.join(main, sessionDir, 'notes.txt')), + ).toEqual(Buffer.from('primary')); + expect( + await fs.readFile(path.join(fallback, sessionDir, 'notes.txt')), + ).toEqual(Buffer.from('fallback')); + } finally { + stat.mockRestore(); + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('keeps a concurrent same-name upload while removing a fallback attachment', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + await writeIn(fallback, 'notes.txt', 'legacy'); + const realStat = fs.stat.bind(fs); + let notifyPrimaryStatPaused = () => {}; + const primaryStatPaused = new Promise((resolve) => { + notifyPrimaryStatPaused = resolve; + }); + let resumePrimaryStat = () => {}; + const primaryStatResume = new Promise((resolve) => { + resumePrimaryStat = resolve; + }); + const primaryPath = path.join(main, sessionDir, 'notes.txt'); + const stat = vi.spyOn(fs, 'stat').mockImplementation(async (filePath) => { + if (String(filePath) === primaryPath) { + notifyPrimaryStatPaused(); + await primaryStatResume; + } + return realStat(filePath); + }); + try { + const removing = store.remove('notes.txt'); + await primaryStatPaused; + const uploaded = await store.putAttachment( + new TextEncoder().encode('fresh'), + 'text/plain', + 'notes.txt', + ); + resumePrimaryStat(); + + await expect(removing).resolves.toBe(true); + expect(uploaded.attachmentId).not.toBe('notes.txt'); + expect(await store.read(uploaded.attachmentId)).toEqual({ + data: Buffer.from('fresh'), + mimeType: 'text/plain', + }); + } finally { + stat.mockRestore(); + resumePrimaryStat(); + await store.close(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + } + }); + + it('copies fallback attachments when the primary directory is degraded', async () => { + const { main, fallback } = await createRoots(); + const source = new SessionAttachmentStore(main, sessionId, fallback); + const targetRoot = await fs.mkdtemp( + path.join(tmpdir(), 'qwen-attachments-target-'), + ); + const target = new SessionAttachmentStore(targetRoot, sessionId); + await fs.mkdir(path.join(main, sessionDir), { recursive: true }); + await writeIn(fallback, 'legacy.txt', 'legacy'); + const readdir = vi + .spyOn(fs, 'readdir') + .mockRejectedValueOnce( + Object.assign(new Error('primary unavailable'), { code: 'EIO' }), + ); + try { + await expect(target.copyFrom(source)).resolves.toBeUndefined(); + expect(await target.read('legacy.txt')).toEqual({ + data: Buffer.from('legacy'), + mimeType: 'text/plain', + }); + } finally { + readdir.mockRestore(); + await source.close(); + await target.delete(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + await fs.rm(targetRoot, { recursive: true, force: true }); + } + }); + + it('retries a claimed fallback tombstone', async () => { + const { main, fallback } = await createRoots(); + const store = new SessionAttachmentStore(main, sessionId, fallback); + const tombstone = path.join(fallback, `.${sessionDir}.deleting`); + await fs.mkdir(tombstone, { recursive: true }); + await fs.writeFile(path.join(tombstone, 'legacy.txt'), 'legacy'); + + await store.delete(); + + await expect(fs.stat(tombstone)).rejects.toMatchObject({ + code: 'ENOENT', + }); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + }); }); }); diff --git a/packages/acp-bridge/src/sessionAttachments.ts b/packages/acp-bridge/src/sessionAttachments.ts index 16bb21a4d01..4ec246b0732 100644 --- a/packages/acp-bridge/src/sessionAttachments.ts +++ b/packages/acp-bridge/src/sessionAttachments.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { randomUUID } from 'node:crypto'; import { constants as fsConstants, promises as fs, statSync } from 'node:fs'; import { tmpdir } from 'node:os'; import * as path from 'node:path'; @@ -334,6 +333,7 @@ export class SessionAttachmentStore { private activeDirectory?: string; private pendingItems = 0; private readonly pendingNames = new Map(); + private readonly removingNames = new Set(); private readonly pendingDrainWaiters: Array<() => void> = []; private readonly copyDrainWaiters: Array<() => void> = []; private copying = false; @@ -402,6 +402,10 @@ export class SessionAttachmentStore { if (safeAttachmentName(candidateName) !== candidateName) { throw new TypeError('Session attachment name is invalid'); } + if (this.removingNames.has(candidateName)) { + suffix += 1; + continue; + } // A legacy fallback copy owns this name; a new upload must not shadow // it (reads resolve the primary first, so reusing the ID would make an // old reference surface the new bytes). Treat it as occupied. @@ -649,14 +653,18 @@ export class SessionAttachmentStore { // Primary first so a name held by both roots resolves to the primary; // fallback entries copied later with the same name are skipped. const copiedNames = new Set(); + let sourceDirectoryRead = false; + let sourceDirectoryError: unknown; for (const sourceDirectory of sourceDirectories) { let entries; try { entries = await fs.readdir(sourceDirectory, { withFileTypes: true }); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; - throw error; + sourceDirectoryError ??= error; + continue; } + sourceDirectoryRead = true; await Promise.all( entries .filter( @@ -690,6 +698,9 @@ export class SessionAttachmentStore { }), ); } + if (!sourceDirectoryRead && sourceDirectoryError !== undefined) { + throw sourceDirectoryError; + } } finally { source.copying = false; this.copying = false; @@ -704,19 +715,48 @@ export class SessionAttachmentStore { !name || name !== attachmentId || this.copying || - this.pendingNames.has(name) + this.pendingNames.has(name) || + this.removingNames.has(name) ) { return false; } - // Unlink the fallback first: if a legacy copy fails to unlink (e.g. the - // old default dir sits on a read-only volume), the authoritative primary - // copy is still intact and remove() can fail cleanly without leaving a - // deleted attachment readable through the fallback. - const fallbackHit = - (await this.tryUnlink(this.persistentFallbackDirectory, name)) === true; - const primaryHit = - (await this.tryUnlink(await this.peekDirectory(), name)) === true; - return primaryHit || fallbackHit; + this.removingNames.add(name); + try { + const primaryDirectory = await this.peekDirectory(); + // Probe both roots before mutating either so an unreadable root cannot + // turn one remove request into a partial deletion. + const [fallbackExists, primaryExists] = await Promise.all([ + this.hasAttachment(this.persistentFallbackDirectory, name), + this.hasAttachment(primaryDirectory, name), + ]); + // Unlink the fallback first: if a legacy copy fails to unlink (e.g. the + // old default dir sits on a read-only volume), the authoritative primary + // copy is still intact and remove() can fail cleanly without leaving a + // deleted attachment readable through the fallback. + const fallbackHit = + fallbackExists && + (await this.tryUnlink(this.persistentFallbackDirectory, name)) === true; + const primaryHit = + primaryExists && + (await this.tryUnlink(primaryDirectory, name)) === true; + return primaryHit || fallbackHit; + } finally { + this.removingNames.delete(name); + } + } + + private async hasAttachment( + directory: string | undefined, + name: string, + ): Promise { + if (!directory) return false; + try { + await fs.stat(path.join(directory, name)); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return false; + throw error; + } } private async tryUnlink( @@ -767,7 +807,7 @@ export class SessionAttachmentStore { let fallbackError: unknown; if (this.persistentFallbackDirectory) { try { - await this.removeDirectoryWithTombstone( + await this.removeDirectoryDurably( this.persistentFallbackDirectory, options.assertCanCommit, ); @@ -850,30 +890,6 @@ export class SessionAttachmentStore { } } - /** - * Atomically detach `directory` by renaming it to a tombstone, then remove - * the tombstone. A concurrent writer that recreates the directory after the - * rename lands a fresh directory at the original path, which the tombstone - * removal never touches — so a deletion racing a session restore cannot - * sweep up a successor directory. ENOENT (nothing to delete) is a no-op. - */ - private async removeDirectoryWithTombstone( - directory: string, - assertCanCommit?: () => void, - ): Promise { - assertCanCommit?.(); - const tombstone = path.join( - path.dirname(directory), - `.${path.basename(directory)}.deleting-${randomUUID()}`, - ); - try { - await fs.rename(directory, tombstone); - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; - } - await fs.rm(tombstone, { recursive: true, force: true }); - } - private assertStored(reference: SessionAttachmentReference): void { const id = reference.attachmentId; const name = safeAttachmentName(id); diff --git a/packages/cli/src/commands/review/test-efficacy.test.ts b/packages/cli/src/commands/review/test-efficacy.test.ts index 2f4821e2f7c..b7f1772b01e 100644 --- a/packages/cli/src/commands/review/test-efficacy.test.ts +++ b/packages/cli/src/commands/review/test-efficacy.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect } from 'vitest'; import { execFileSync, spawnSync } from 'node:child_process'; import { replacementMutantsOf, @@ -511,22 +511,6 @@ describe('probeCleanupFailureDetail', () => { }); describe('restoreProbeTreeTracked, through runOneMutant', () => { - // Ambient host git state must not reach this fixture: the restore's own - // git spawns sanitize their env BY DESIGN — dropping the GIT_CONFIG_* - // redirects and reading whatever $HOME carries — so the refusal these - // tests pin can be decided by the host instead of the code (the incident - // class `isolateHostGitConfig` was written for; every sibling real-git - // suite isolates). - let gitIsolation: ReturnType; - - beforeEach(() => { - gitIsolation = isolateHostGitConfig(); - }); - - afterEach(() => { - gitIsolation.dispose(); - }); - it('refuses to run when the tree carries NO .git at all', () => { // The state the tree can never be put back from, and the cheapest one to // reach: `.git` is an untracked pointer file inside the directory the PR's From 6589015d0abb9624bf3f98f73147537fa7b54c83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=92=89=E8=90=81?= Date: Mon, 31 Aug 2026 09:43:47 +0800 Subject: [PATCH 7/7] fix(acp): skip attachments being removed during copy --- .../acp-bridge/src/sessionAttachments.test.ts | 50 +++++++++++++++++++ packages/acp-bridge/src/sessionAttachments.ts | 1 + 2 files changed, 51 insertions(+) diff --git a/packages/acp-bridge/src/sessionAttachments.test.ts b/packages/acp-bridge/src/sessionAttachments.test.ts index 64f1aa61e15..78197bb025c 100644 --- a/packages/acp-bridge/src/sessionAttachments.test.ts +++ b/packages/acp-bridge/src/sessionAttachments.test.ts @@ -2013,6 +2013,56 @@ describe('SessionAttachmentStore', () => { } }); + it('does not copy an attachment being removed', async () => { + const { main, fallback } = await createRoots(); + const source = new SessionAttachmentStore(main, sessionId, fallback); + const targetRoot = await fs.mkdtemp( + path.join(tmpdir(), 'qwen-attachments-target-'), + ); + const target = new SessionAttachmentStore(targetRoot, sessionId); + await source.putAttachment( + new TextEncoder().encode('doomed'), + 'text/plain', + 'notes.txt', + ); + const realUnlink = fs.unlink.bind(fs); + let notifyUnlinkPaused = () => {}; + const unlinkPaused = new Promise((resolve) => { + notifyUnlinkPaused = resolve; + }); + let resumeUnlink = () => {}; + const unlinkResume = new Promise((resolve) => { + resumeUnlink = resolve; + }); + const sourcePath = path.join(main, sessionDir, 'notes.txt'); + const unlink = vi + .spyOn(fs, 'unlink') + .mockImplementation(async (filePath) => { + if (String(filePath) === sourcePath) { + notifyUnlinkPaused(); + await unlinkResume; + } + return realUnlink(filePath); + }); + try { + const removing = source.remove('notes.txt'); + await unlinkPaused; + await target.copyFrom(source); + resumeUnlink(); + + await expect(removing).resolves.toBe(true); + await expect(target.read('notes.txt')).resolves.toBeUndefined(); + } finally { + unlink.mockRestore(); + resumeUnlink(); + await source.close(); + await target.delete(); + await fs.rm(main, { recursive: true, force: true }); + await fs.rm(fallback, { recursive: true, force: true }); + await fs.rm(targetRoot, { recursive: true, force: true }); + } + }); + it('copies fallback attachments when the primary directory is degraded', async () => { const { main, fallback } = await createRoots(); const source = new SessionAttachmentStore(main, sessionId, fallback); diff --git a/packages/acp-bridge/src/sessionAttachments.ts b/packages/acp-bridge/src/sessionAttachments.ts index 4ec246b0732..5880bf5012f 100644 --- a/packages/acp-bridge/src/sessionAttachments.ts +++ b/packages/acp-bridge/src/sessionAttachments.ts @@ -671,6 +671,7 @@ export class SessionAttachmentStore { (entry) => entry.isFile() && !source.pendingNames.has(entry.name) && + !source.removingNames.has(entry.name) && !copiedNames.has(entry.name), ) .map(async (entry) => {