From 694749adc0a6da961e927bc995c12a4af1db73df Mon Sep 17 00:00:00 2001 From: Hoang Nguyen Date: Fri, 14 Aug 2026 17:13:17 +0200 Subject: [PATCH] fix(agent-manager): preserve registry entries on EPERM --- ...026-08-13-feature-agent-registry-sqlite.md | 1 + ...026-08-13-feature-agent-registry-sqlite.md | 4 + .../src/__tests__/AgentManager.test.ts | 30 ++++++++ .../src/__tests__/utils/AgentRegistry.test.ts | 74 +++++++++++++++++++ .../agent-manager/src/utils/AgentRegistry.ts | 7 +- 5 files changed, 114 insertions(+), 2 deletions(-) diff --git a/docs/ai/implementation/2026-08-13-feature-agent-registry-sqlite.md b/docs/ai/implementation/2026-08-13-feature-agent-registry-sqlite.md index 3b2f1942..8e686c19 100644 --- a/docs/ai/implementation/2026-08-13-feature-agent-registry-sqlite.md +++ b/docs/ai/implementation/2026-08-13-feature-agent-registry-sqlite.md @@ -56,6 +56,7 @@ description: Technical implementation notes, patterns, and code guidelines - Preserve user-managed names over generated fallback names. - Prefer incoming non-empty metadata over empty metadata. - Keep storage errors explicit. +- Treat only an `ESRCH` result from `process.kill(pid, 0)` as definitive process death. `EPERM` and indeterminate probe failures preserve the row so prune, registration conflict cleanup, and rename conflict handling cannot discard live or unknown agents. ## Integration Points diff --git a/docs/ai/testing/2026-08-13-feature-agent-registry-sqlite.md b/docs/ai/testing/2026-08-13-feature-agent-registry-sqlite.md index 1b82b708..f1845b59 100644 --- a/docs/ai/testing/2026-08-13-feature-agent-registry-sqlite.md +++ b/docs/ai/testing/2026-08-13-feature-agent-registry-sqlite.md @@ -27,12 +27,16 @@ description: Define testing approach, test cases, and quality assurance - [x] `rename()` updates the name and preserves all other fields. - [x] `rename()` reports not-found and live-name conflict errors. - [x] `prune()` removes dead PIDs from SQLite. +- [x] `prune()` preserves custom names and tmux metadata when liveness probing returns `EPERM` or another indeterminate error. +- [x] `ESRCH` remains the definitive stale-process signal for pruning. +- [x] Registration and rename name-conflict checks preserve the existing row when its liveness probe returns `EPERM`. - [x] Two registry instances can register the same PID without duplicate rows or temp-file failures. ### AgentManager - [x] `listAgents()` preserves a user-managed name when adapter detection emits a generated fallback for the same PID. - [x] Repeated `listAgents()` calls do not create duplicate registry entries for the same PID. +- [x] Two `listAgents()` refresh cycles preserve the custom name and `tmuxSession` when process probing returns `EPERM`. ## Integration Tests diff --git a/packages/agent-manager/src/__tests__/AgentManager.test.ts b/packages/agent-manager/src/__tests__/AgentManager.test.ts index b4783ae5..318225de 100644 --- a/packages/agent-manager/src/__tests__/AgentManager.test.ts +++ b/packages/agent-manager/src/__tests__/AgentManager.test.ts @@ -353,6 +353,7 @@ describe('AgentManager', () => { }); afterEach(() => { + vi.restoreAllMocks(); fs.rmSync(tmpDir, { recursive: true, force: true }); }); @@ -429,6 +430,35 @@ describe('AgentManager', () => { expect(registry.list()[0].startedAt).toBe('2026-05-30T00:00:00.000Z'); }); + it('preserves custom name and tmux session across two EPERM refresh cycles', async () => { + registry.register({ + name: 'merry', + type: 'claude', + pid: process.pid, + tmuxSession: 'merry-tmux', + cwd: '/cwd/merry', + startedAt: '2026-05-30T00:00:00.000Z', + sessionId: 'sid-merry', + sessionFilePath: '/path/merry.jsonl', + }); + scopedManager.registerAdapter(new MockAdapter('claude', [ + createMockAgent({ name: `ai-devkit-${process.pid}`, pid: process.pid }), + ])); + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }); + }); + + const firstRefresh = await scopedManager.listAgents(); + const secondRefresh = await scopedManager.listAgents(); + + expect(firstRefresh[0].name).toBe('merry'); + expect(secondRefresh[0].name).toBe('merry'); + expect(registry.lookup('merry')).toMatchObject({ + name: 'merry', + tmuxSession: 'merry-tmux', + }); + }); + it('preserves a user-managed name when a fallback row was written later for the same pid', async () => { registry.register({ name: 'agent-list-debug', diff --git a/packages/agent-manager/src/__tests__/utils/AgentRegistry.test.ts b/packages/agent-manager/src/__tests__/utils/AgentRegistry.test.ts index 867e3951..9f382fa6 100644 --- a/packages/agent-manager/src/__tests__/utils/AgentRegistry.test.ts +++ b/packages/agent-manager/src/__tests__/utils/AgentRegistry.test.ts @@ -29,6 +29,7 @@ describe('AgentRegistry', () => { }); afterEach(() => { + vi.restoreAllMocks(); fs.rmSync(tmpDir, { recursive: true, force: true }); }); @@ -81,6 +82,19 @@ describe('AgentRegistry', () => { expect(registry.lookup(`ai-devkit-${process.pid}`)).toBeNull(); expect(registry.list()).toHaveLength(1); }); + + it('preserves an existing name conflict when its probe fails with EPERM', () => { + registry.register(makeEntry({ name: 'claimed-name', pid: process.pid })); + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }); + }); + + expect(() => registry.register(makeEntry({ + name: 'claimed-name', + pid: process.pid + 1, + }))).toThrow(); + expect(registry.lookup('claimed-name')?.pid).toBe(process.pid); + }); }); describe('registerBatch', () => { @@ -157,6 +171,30 @@ describe('AgentRegistry', () => { it('returns false for a PID that does not exist', () => { expect(registry.isAlive(makeEntry({ pid: 999999 }))).toBe(false); }); + + it('returns true when the process probe is forbidden with EPERM', () => { + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }); + }); + + expect(registry.isAlive(makeEntry())).toBe(true); + }); + + it('returns false when the process probe reports ESRCH', () => { + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('no such process'), { code: 'ESRCH' }); + }); + + expect(registry.isAlive(makeEntry())).toBe(false); + }); + + it('returns true when the process probe fails without a definitive error code', () => { + vi.spyOn(process, 'kill').mockImplementation(() => { + throw new Error('indeterminate probe failure'); + }); + + expect(registry.isAlive(makeEntry())).toBe(true); + }); }); describe('prune', () => { @@ -177,6 +215,31 @@ describe('AgentRegistry', () => { expect(after).toEqual(before); }); + it('preserves entries when liveness probing fails with EPERM', () => { + registry.register(makeEntry({ name: 'custom-name', tmuxSession: 'tmux-custom' })); + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }); + }); + + registry.prune(); + + expect(registry.lookup('custom-name')).toMatchObject({ + name: 'custom-name', + tmuxSession: 'tmux-custom', + }); + }); + + it('removes entries when liveness probing fails with ESRCH', () => { + registry.register(makeEntry({ name: 'dead' })); + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('no such process'), { code: 'ESRCH' }); + }); + + registry.prune(); + + expect(registry.lookup('dead')).toBeNull(); + }); + it('does nothing when file is missing', () => { expect(() => registry.prune()).not.toThrow(); }); @@ -215,6 +278,17 @@ describe('AgentRegistry', () => { expect(() => registry.rename('agent-a', 'agent-b')).toThrow(RenameConflictError); }); + it('throws RenameConflictError when the conflicting entry probe fails with EPERM', () => { + registry.register(makeEntry({ name: 'agent-a', pid: process.pid })); + registry.register(makeEntry({ name: 'agent-b', pid: process.ppid })); + vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('operation not permitted'), { code: 'EPERM' }); + }); + + expect(() => registry.rename('agent-a', 'agent-b')).toThrow(RenameConflictError); + expect(registry.lookup('agent-b')?.pid).toBe(process.ppid); + }); + it('succeeds when new name exists only as a stale (dead) entry', () => { registry.register(makeEntry({ name: 'agent-a', pid: process.pid })); registry.register(makeEntry({ name: 'agent-b', pid: 999999 })); diff --git a/packages/agent-manager/src/utils/AgentRegistry.ts b/packages/agent-manager/src/utils/AgentRegistry.ts index 83d25216..4b866dd0 100644 --- a/packages/agent-manager/src/utils/AgentRegistry.ts +++ b/packages/agent-manager/src/utils/AgentRegistry.ts @@ -138,8 +138,11 @@ export class AgentRegistry { try { process.kill(entry.pid, 0); return true; - } catch { - return false; + } catch (error) { + const code = error && typeof error === 'object' && 'code' in error + ? error.code + : undefined; + return code !== 'ESRCH'; } }