diff --git a/packages/core/src/agents/team/teamHelpers.test.ts b/packages/core/src/agents/team/teamHelpers.test.ts index 9a0d23e3a28..64ee4680ac9 100644 --- a/packages/core/src/agents/team/teamHelpers.test.ts +++ b/packages/core/src/agents/team/teamHelpers.test.ts @@ -48,28 +48,43 @@ vi.mock('../../config/storage.js', async (importOriginal) => { }; }); +// Mock node:fs/promises to allow per-test override of fs.rm. +// All other functions pass through to the real implementation. +let rmMockOverride: + | ((...args: Parameters) => Promise) + | null = null; // Optional readFile hook for simulating mid-reclaim I/O failures. // While a hook is installed and returns a value, that value is used; // otherwise the real readFile runs. vi.mock('node:fs/promises', async (importOriginal) => { - const actual = await importOriginal(); - type ReadFileHook = (...args: Parameters) => unknown; + const original = await importOriginal(); + type ReadFileHook = (...args: Parameters) => unknown; let readFileHook: ReadFileHook | undefined; return { - ...actual, + ...original, + default: original, __setReadFileHook: (fn: ReadFileHook | undefined) => { readFileHook = fn; }, - readFile: (...args: Parameters) => { + rm: (...args: Parameters) => { + if (rmMockOverride) return rmMockOverride(...args); + return original.rm(...args); + }, + readFile: (...args: Parameters) => { const hooked = readFileHook?.(...args); if (hooked !== undefined) { return hooked; } - return actual.readFile(...args); + return original.readFile(...args); }, }; }); +function setFsRmMock( + fn: ((...args: Parameters) => Promise) | null, +) { + rmMockOverride = fn; +} const { __setReadFileHook } = (await import('node:fs/promises')) as unknown as { __setReadFileHook: (fn?: unknown) => void; }; @@ -387,6 +402,11 @@ describe('file I/O', () => { }); describe('deleteTeamDirs', () => { + afterEach(() => { + setFsRmMock(null); + vi.restoreAllMocks(); + }); + it('deletes team and task directories', async () => { await writeTeamFile('doomed', makeTeamFile()); const tasksDir = getTasksDir('doomed'); @@ -402,6 +422,46 @@ describe('file I/O', () => { it('does not throw for missing directories', async () => { await expect(deleteTeamDirs('nonexistent')).resolves.not.toThrow(); }); + + it('throws AggregateError when both rm calls fail (e.g. EACCES)', async () => { + const eaccesError = Object.assign(new Error('permission denied'), { + code: 'EACCES', + }); + setFsRmMock(() => Promise.reject(eaccesError)); + + const err: unknown = await deleteTeamDirs('any-team').then( + () => undefined, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(AggregateError); + expect((err as AggregateError).errors).toHaveLength(2); + // Member errno/path detail must survive in the wrapper message — + // serializers reading only `.message`/`.stack` never see `.errors`. + expect((err as AggregateError).message).toContain('permission denied'); + }); + + it('throws AggregateError when both rm calls fail (EIO)', async () => { + const eioError = Object.assign(new Error('I/O error'), { code: 'EIO' }); + setFsRmMock(() => Promise.reject(eioError)); + + await expect(deleteTeamDirs('any-team')).rejects.toThrow(AggregateError); + }); + + it('throws the single error when only the second rm call fails', async () => { + const eaccesError = Object.assign(new Error('permission denied'), { + code: 'EACCES', + }); + let callCount = 0; + setFsRmMock(() => { + callCount++; + if (callCount === 1) return Promise.resolve(); + return Promise.reject(eaccesError); + }); + + await expect(deleteTeamDirs('any-team')).rejects.toThrow( + 'permission denied', + ); + }); }); describe('tryReclaimStaleTeam', () => { diff --git a/packages/core/src/agents/team/teamHelpers.ts b/packages/core/src/agents/team/teamHelpers.ts index cf1126bc1b7..fad54805237 100644 --- a/packages/core/src/agents/team/teamHelpers.ts +++ b/packages/core/src/agents/team/teamHelpers.ts @@ -374,16 +374,38 @@ export async function tryReclaimStaleTeam(teamName: string): Promise { /** * Delete an entire team directory and its associated task - * directory. Silently ignores missing directories. + * directory. Missing directories are silently ignored because + * fs.rm is called with { force: true }. + * Throws on real filesystem failures (EACCES, EIO, etc.). + * When both removals fail, throws an AggregateError covering both. */ export async function deleteTeamDirs(teamName: string): Promise { const teamDir = getTeamDir(teamName); const tasksDir = getTasksDir(teamName); - await Promise.allSettled([ + const results = await Promise.allSettled([ fs.rm(teamDir, { recursive: true, force: true }), fs.rm(tasksDir, { recursive: true, force: true }), ]); + + const errors = results + .filter((r): r is PromiseRejectedResult => r.status === 'rejected') + .map((r) => r.reason); + + if (errors.length === 1) { + throw errors[0]; + } + if (errors.length > 1) { + // Fold member messages into the wrapper message: serializers that + // only read `.stack`/`.message` (e.g. debugLogger) would otherwise + // drop the per-directory errno/path detail of `.errors`. + throw new AggregateError( + errors, + `Failed to delete team directories for "${teamName}": ${errors + .map((e) => (e instanceof Error ? e.message : String(e))) + .join('; ')}`, + ); + } } /** diff --git a/packages/core/src/tools/team-delete.test.ts b/packages/core/src/tools/team-delete.test.ts index fa5587b3a2f..9fa86fa9468 100644 --- a/packages/core/src/tools/team-delete.test.ts +++ b/packages/core/src/tools/team-delete.test.ts @@ -9,6 +9,20 @@ import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import { TeamDeleteTool } from './team-delete.js'; +import { deleteTeamDirs } from '../agents/team/teamHelpers.js'; + +// Mock at the tool boundary so cleanup-failure paths can be injected +// without relying on real-fs permissions (which root bypasses). Keep the +// rest of the module real: disposeInboxLocks (mailbox.ts) resolves +// getInboxesDir through it. +vi.mock('../agents/team/teamHelpers.js', async (importOriginal) => { + const original = + await importOriginal(); + return { + ...original, + deleteTeamDirs: vi.fn().mockResolvedValue(undefined), + }; +}); vi.mock('../config/storage.js', () => { let mockDir = '/tmp/test'; @@ -49,6 +63,8 @@ beforeEach(async () => { afterEach(async () => { await fs.rm(tmpDir, { recursive: true, force: true }); + vi.mocked(deleteTeamDirs).mockReset(); + vi.mocked(deleteTeamDirs).mockResolvedValue(undefined); }); describe('TeamDeleteTool', () => { @@ -68,6 +84,54 @@ describe('TeamDeleteTool', () => { expect(config.setTeamContext).toHaveBeenCalledWith(null); }); + it('resets team state and surfaces failure when directory deletion fails', async () => { + // Issue #10210's invariant at the tool boundary: a non-benign + // cleanup failure must NOT be converted into complete success, + // and it must NOT wedge the session ("team active" forever). + const eacces = Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }); + vi.mocked(deleteTeamDirs).mockRejectedValue(eacces); + + const config = makeConfig({ hasManager: true }); + const result = await new TeamDeleteTool(config) + .build({}) + .execute(new AbortController().signal); + + // The state-reset tail still ran... + expect(config.setTeamManager).toHaveBeenCalledWith(null); + expect(config.setTeamContext).toHaveBeenCalledWith(null); + // ...and the result reports failure instead of complete deletion. + expect(result.error).toBeDefined(); + expect(String(result.llmContent)).toContain('cleanup failed'); + expect(String(result.llmContent)).toContain('EACCES: permission denied'); + expect(String(result.llmContent)).not.toContain('deleted.'); + expect(result.error?.message).toContain('cleanup failed'); + expect(result.error?.message).toContain('EACCES: permission denied'); + }); + + it('runs the delayed second sweep even when the first sweep fails', async () => { + // The second sweep exists to catch the straggler-writeMessage + // race; a first-sweep throw must not cancel it. A retry that + // succeeds means the directories are genuinely gone. + vi.mocked(deleteTeamDirs) + .mockRejectedValueOnce( + Object.assign(new Error('ENOTEMPTY: directory not empty'), { + code: 'ENOTEMPTY', + }), + ) + .mockResolvedValueOnce(undefined); + + const config = makeConfig({ hasManager: true }); + const result = await new TeamDeleteTool(config) + .build({}) + .execute(new AbortController().signal); + + expect(deleteTeamDirs).toHaveBeenCalledTimes(2); + expect(result.error).toBeUndefined(); + expect(result.llmContent).toContain('deleted'); + }); + it('returns error when no team is active', async () => { const tool = new TeamDeleteTool(makeConfig()); const invocation = tool.build({}); diff --git a/packages/core/src/tools/team-delete.ts b/packages/core/src/tools/team-delete.ts index c679cf89a62..42f8a9122b2 100644 --- a/packages/core/src/tools/team-delete.ts +++ b/packages/core/src/tools/team-delete.ts @@ -83,9 +83,31 @@ class TeamDeleteInvocation extends BaseToolInvocation< // we delete it — leaving an orphan dir that wedges the team // name on the next `team_create`. Sweep once more after a // short delay to catch the race. - await deleteTeamDirs(teamName); + // + // Filesystem errors (EACCES, EIO, etc.) must NOT prevent the + // state-reset tail below — otherwise the session is left + // permanently in a "team active" state with no recovery. Each + // sweep is wrapped separately so a first-sweep failure cannot + // skip the delayed second sweep (the race-catcher above). A + // failure is surfaced only if the FINAL sweep still fails: if + // the retry succeeds the directories are gone and deletion + // genuinely completed. + let fsCleanupError: unknown; + try { + await deleteTeamDirs(teamName); + } catch (err) { + debug.warn('First cleanup sweep failed; retrying after delay:', err); + } await new Promise((r) => setTimeout(r, 250)); - await deleteTeamDirs(teamName); + try { + await deleteTeamDirs(teamName); + } catch (err) { + fsCleanupError = err; + debug.warn( + 'Filesystem cleanup failed; resetting team state anyway:', + err, + ); + } // Drop this team's in-process inbox locks now that its inboxes are // gone, so the lock map doesn't retain a dead Mutex per inbox for @@ -97,6 +119,20 @@ class TeamDeleteInvocation extends BaseToolInvocation< this.config.setTeamContext(null); unregisterLeader(); + if (fsCleanupError) { + // State was reset so the session is not wedged, but the cleanup + // failure must NOT be converted into a complete-success claim — + // directories may remain on disk (issue #10210). + const detail = + fsCleanupError instanceof Error + ? fsCleanupError.message + : String(fsCleanupError); + const msg = + `Team "${teamName}" was torn down, but filesystem cleanup ` + + `failed: ${detail}. Team directories may remain on disk.`; + return { llmContent: msg, returnDisplay: msg, error: { message: msg } }; + } + const display: TeamResultDisplay = { type: 'team_result', teamName,