🐛 fix(agent): clear AgentClient timers when an agent is removed

Addresses code-review findings on feature/v1.5-rc23.

- 🐛 fix: add an idempotent AgentClient.stop() that clears both
  stableConnectionTimer and reconnectTimer; removeAgent now calls it before
  splicing, so an abandoned client no longer keeps an armed timer alive
- ✅ test: cover the stream-error reconnect path with the stability timer
  and the new stop() teardown

Author: s-b-e-n-s-o-n

app/agent/AgentClient.test.ts

@@ -1275,6 +1275,82 @@ describe('AgentClient', () => {
 
       expect(reconnectDelays).toEqual([4_000]);
     });
+
+    test('should escalate backoff when stream error triggers reconnect before SSE_STABLE_CONNECTION_MS (#362 error path)', async () => {
+      // Mirror of the end-path regression test but using stream.emit('error') instead of 'end'.
+      axios.mockImplementation(() => Promise.resolve({ data: new EventEmitter() }));
+
+      const setTimeoutSpy = vi.spyOn(global, 'setTimeout');
+
+      // Cycle 1: startSse → 200 → stream error immediately → scheduleReconnect (delay=1000)
+      client.startSse();
+      await vi.advanceTimersByTimeAsync(0);
+      const r1 = await (axios.mock.results[0].value as Promise<{ data: EventEmitter }>);
+      r1.data.emit('error', new Error('connection reset'));
+      await vi.advanceTimersByTimeAsync(1_000);
+
+      // Cycle 2: startSse → 200 → stream error immediately → scheduleReconnect (delay=2000)
+      await vi.advanceTimersByTimeAsync(0);
+      const r2 = await (axios.mock.results[1].value as Promise<{ data: EventEmitter }>);
+      r2.data.emit('error', new Error('connection reset'));
+      await vi.advanceTimersByTimeAsync(2_000);
+
+      // Cycle 3: startSse → 200 → stream error immediately → scheduleReconnect (delay=4000)
+      await vi.advanceTimersByTimeAsync(0);
+      const r3 = await (axios.mock.results[2].value as Promise<{ data: EventEmitter }>);
+      r3.data.emit('error', new Error('connection reset'));
+      await vi.advanceTimersByTimeAsync(4_000);
+
+      // Collect only reconnect delays (filter out the 30_000 stability-timer calls)
+      const reconnectDelays = setTimeoutSpy.mock.calls
+        .map(([, delay]) => delay)
+        .filter((delay): delay is number => typeof delay === 'number' && delay !== 30_000);
+
+      expect(reconnectDelays).toEqual([1_000, 2_000, 4_000]);
+    });
+  });
+
+  describe('stop', () => {
+    test('should clear an armed stableConnectionTimer', async () => {
+      const stream = new EventEmitter();
+      axios.mockResolvedValue({ data: stream });
+
+      client.startSse();
+      await vi.advanceTimersByTimeAsync(0); // stability timer is now armed
+
+      const clearTimeoutSpy = vi.spyOn(global, 'clearTimeout');
+      const timerBefore = (client as any).stableConnectionTimer;
+      expect(timerBefore).not.toBeNull();
+
+      client.stop();
+
+      expect(clearTimeoutSpy).toHaveBeenCalledWith(timerBefore);
+      expect((client as any).stableConnectionTimer).toBeNull();
+    });
+
+    test('should clear an armed reconnectTimer', () => {
+      const spy = vi.spyOn(client, 'startSse').mockImplementation(() => {});
+      client.scheduleReconnect(5_000); // arms reconnectTimer
+      const timerBefore = (client as any).reconnectTimer;
+      expect(timerBefore).not.toBeNull();
+
+      const clearTimeoutSpy = vi.spyOn(global, 'clearTimeout');
+      client.stop();
+
+      expect(clearTimeoutSpy).toHaveBeenCalledWith(timerBefore);
+      expect((client as any).reconnectTimer).toBeNull();
+      // Confirm the reconnect never fires after stop()
+      vi.advanceTimersByTime(10_000);
+      expect(spy).not.toHaveBeenCalled();
+    });
+
+    test('should be safe to call when both timers are already null', () => {
+      expect((client as any).stableConnectionTimer).toBeNull();
+      expect((client as any).reconnectTimer).toBeNull();
+      expect(() => client.stop()).not.toThrow();
+      expect((client as any).stableConnectionTimer).toBeNull();
+      expect((client as any).reconnectTimer).toBeNull();
+    });
   });
 
   describe('handleEvent', () => {

app/agent/AgentClient.ts

@@ -559,6 +559,14 @@ export class AgentClient {
     }
   }
 
+  stop() {
+    this.clearStableConnectionTimer();
+    if (this.reconnectTimer) {
+      clearTimeout(this.reconnectTimer);
+      this.reconnectTimer = null;
+    }
+  }
+
   private getNextReconnectDelayMs(): number {
     const nextDelay = Math.min(
       INITIAL_SSE_RECONNECT_DELAY_MS * 2 ** this.reconnectAttempts,

app/agent/manager.test.ts

@@ -14,14 +14,14 @@ describe('manager', () => {
   });
 
   test('addAgent should add a client', () => {
-    const client = { name: 'agent1' };
+    const client = { name: 'agent1', stop: vi.fn() };
     manager.addAgent(client);
     expect(manager.getAgents()).toHaveLength(1);
     expect(manager.getAgents()[0]).toBe(client);
   });
 
   test('getAgent should return client by name', () => {
-    const client = { name: 'agent1' };
+    const client = { name: 'agent1', stop: vi.fn() };
     manager.addAgent(client);
     expect(manager.getAgent('agent1')).toBe(client);
   });
@@ -31,8 +31,8 @@ describe('manager', () => {
   });
 
   test('addAgent should support multiple agents', () => {
-    const c1 = { name: 'a1' };
-    const c2 = { name: 'a2' };
+    const c1 = { name: 'a1', stop: vi.fn() };
+    const c2 = { name: 'a2', stop: vi.fn() };
     manager.addAgent(c1);
     manager.addAgent(c2);
     expect(manager.getAgents()).toHaveLength(2);
@@ -41,8 +41,8 @@ describe('manager', () => {
   });
 
   test('removeAgent should remove a client by name', () => {
-    const c1 = { name: 'a1' };
-    const c2 = { name: 'a2' };
+    const c1 = { name: 'a1', stop: vi.fn() };
+    const c2 = { name: 'a2', stop: vi.fn() };
     manager.addAgent(c1);
     manager.addAgent(c2);
 
@@ -55,4 +55,35 @@ describe('manager', () => {
   test('removeAgent should return false when client does not exist', () => {
     expect(manager.removeAgent('missing-agent')).toBe(false);
   });
+
+  test('removeAgent should call stop() on each removed client before splicing', () => {
+    const stop1 = vi.fn();
+    const stop2 = vi.fn();
+    const c1 = { name: 'target', stop: stop1 };
+    const c2 = { name: 'other', stop: stop2 };
+    manager.addAgent(c1);
+    manager.addAgent(c2);
+
+    manager.removeAgent('target');
+
+    expect(stop1).toHaveBeenCalledTimes(1);
+    expect(stop2).not.toHaveBeenCalled();
+    expect(manager.getAgents()).toHaveLength(1);
+    expect(manager.getAgent('target')).toBeUndefined();
+  });
+
+  test('removeAgent should call stop() on all matching clients when duplicates exist', () => {
+    const stop1 = vi.fn();
+    const stop2 = vi.fn();
+    const c1 = { name: 'dup', stop: stop1 };
+    const c2 = { name: 'dup', stop: stop2 };
+    manager.addAgent(c1);
+    manager.addAgent(c2);
+
+    manager.removeAgent('dup');
+
+    expect(stop1).toHaveBeenCalledTimes(1);
+    expect(stop2).toHaveBeenCalledTimes(1);
+    expect(manager.getAgents()).toHaveLength(0);
+  });
 });

app/agent/manager.ts

@@ -36,6 +36,7 @@ export function removeAgent(name: string): boolean {
   const initialCount = clients.length;
   for (let index = clients.length - 1; index >= 0; index -= 1) {
     if (clients[index].name === name) {
+      clients[index].stop();
       clients.splice(index, 1);
     }
   }