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.
wenshao · Sep 1, 2026 · Aug 26, 2026 · Aug 26, 2026 · Aug 26, 2026 · Aug 26, 2026
commit 97e64bc83e3711f998f9f89e4c0b92e91860b2af
diff --git 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 `<root>/<projectHash>/attachments/session-<sessionId>/` in both locations, where `<projectHash>` 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
@@ -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
@@ -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<string | undefined> {
+    const established = await this.directoryPromise?.catch(() => undefined);
+    return established ?? this.persistentDirectory;
+  }
+
   private async directory(): Promise<string> {
     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
@@ -41,13 +41,36 @@ 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({
       root: defaultRoot,
     });
   });
 
+  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
@@ -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 {