Merge pull request #35 from code-rabi/refactor/auth-polling-deadline-and-error-propagation

refactor(auth): tighten browser auth polling and error propagation (followup to #25)

Author: NitayRabi

src/ib-client.ts

@@ -239,10 +239,12 @@ export class IBClient {
       } else {
         Logger.warn("[REAUTH] Re-authentication request sent but auth status is still false, will retry via interceptor");
         this.isAuthenticated = false;
+        this.stopTickle();
       }
     } catch (error) {
       Logger.warn("[REAUTH] Re-authentication failed, will fall back to interceptor-based auth:", error);
       this.isAuthenticated = false;
+      this.stopTickle();
     }
   }
 

src/tool-handlers.ts

@@ -121,7 +121,17 @@ export class ToolHandlers {
       try {
         await this.context.ibClient.reauthenticate();
       } catch (e) {
-        Logger.warn("[ENSURE-AUTH] Reauthenticate after status check failed, but status is true:", e);
+        Logger.warn("[ENSURE-AUTH] Reauthenticate after status check failed, will re-check auth status:", e);
+      }
+      // Re-check auth status to honor the ensureAuth contract: if reauthenticate left
+      // the session unauthenticated, propagate the error rather than silently proceeding.
+      const finalAuthStatus = await this.context.ibClient.checkAuthenticationStatus();
+      if (!finalAuthStatus) {
+        const port = this.context.gatewayManager
+          ? this.context.gatewayManager.getCurrentPort()
+          : this.context.config.IB_GATEWAY_PORT;
+        const authUrl = `https://${this.context.config.IB_GATEWAY_HOST}:${port}`;
+        throw new Error(`Authentication required. Please use the 'authenticate' tool to complete the authentication process at ${authUrl}.`);
       }
     }
   }
@@ -159,35 +169,40 @@ export class ToolHandlers {
    * After browser opens for OAuth, poll the gateway until authenticated,
    * then trigger reauthenticate to establish the REST API session.
    * This bridges the gap between browser-based OAuth and the REST API auth state.
+   *
+   * Polling is bounded by a deadline (~2 minutes from start) rather than by attempt
+   * count, so the upper bound matches the documented timeout regardless of backoff.
    */
   private startBrowserAuthPolling(authUrl: string, port: number): void {
-    const maxAttempts = 60; // 2 minutes total
+    const pollWindowMs = 120_000; // 2 minutes total
     const initialDelay = 2000; // 2 second initial delay
+    const maxDelay = 10_000;
+    const deadline = Date.now() + pollWindowMs;
     let attempts = 0;
 
     const poll = async () => {
       attempts++;
-      Logger.log(`[BROWSER-AUTH-POLL] Attempt ${attempts}/${maxAttempts}`);
+      Logger.log(`[BROWSER-AUTH-POLL] Polling ${authUrl} (port ${port}) attempt ${attempts} until ${new Date(deadline).toISOString()}`);
 
       try {
         const isAuth = await this.context.ibClient.checkAuthenticationStatus();
-        
+
         if (isAuth) {
-          Logger.log("[BROWSER-AUTH-POLL] Authentication detected, reauthenticating REST session");
+          Logger.log(`[BROWSER-AUTH-POLL] Authentication detected for ${authUrl} (port ${port}), reauthenticating REST session`);
           // Trigger reauthenticate to establish REST API session
           await this.context.ibClient.reauthenticate();
-          Logger.log("[BROWSER-AUTH-POLL] Reauthentication successful, REST session established");
+          Logger.log(`[BROWSER-AUTH-POLL] Reauthentication successful for ${authUrl} (port ${port}), REST session established`);
           return; // Success, stop polling
         }
       } catch (error) {
-        Logger.warn("[BROWSER-AUTH-POLL] Poll attempt failed:", error);
+        Logger.warn(`[BROWSER-AUTH-POLL] Poll attempt ${attempts} failed for ${authUrl} (port ${port}):`, error);
       }
 
-      if (attempts < maxAttempts) {
-        const delay = Math.min(initialDelay + (attempts * 500), 10000);
+      const delay = Math.min(initialDelay + (attempts * 500), maxDelay);
+      if (Date.now() + delay < deadline) {
         setTimeout(poll, delay);
       } else {
-        Logger.warn("[BROWSER-AUTH-POLL] Timed out waiting for browser authentication");
+        Logger.warn(`[BROWSER-AUTH-POLL] Timed out waiting for browser authentication at ${authUrl} (port ${port}) after ${attempts} attempts`);
       }
     };
 

test/ib-client.test.ts

@@ -377,9 +377,9 @@ describe('IBClient', () => {
           data: { authenticated: false },
         }),
       };
-      
+
       vi.mocked(axios.create).mockReturnValueOnce(mockAuthClient as any);
-      
+
       // Should not throw — handled internally
       await expect(client.reauthenticate()).resolves.not.toThrow();
       expect(mockAuthClient.post).toHaveBeenCalledWith('/iserver/reauthenticate');
@@ -390,12 +390,79 @@ describe('IBClient', () => {
         post: vi.fn().mockRejectedValue(new Error('Connection refused')),
         get: vi.fn(),
       };
-      
+
       vi.mocked(axios.create).mockReturnValueOnce(mockAuthClient as any);
-      
+
       // Should not throw — errors are caught internally
       await expect(client.reauthenticate()).resolves.not.toThrow();
     });
+
+    it('should stop tickle when reauth status returns false', async () => {
+      vi.useFakeTimers();
+      try {
+        // Step 1: bring tickle up by simulating a successful auth status check.
+        const checkClient = {
+          get: vi.fn().mockResolvedValue({ data: { authenticated: true } }),
+        };
+        vi.mocked(axios.create).mockReturnValueOnce(checkClient as any);
+        await client.checkAuthenticationStatus();
+
+        // Capture the tickle axios instance the next time axios.create is called
+        // (startTickle uses axios.create per ping).
+        const tickleClient = {
+          post: vi.fn().mockResolvedValue({ data: {} }),
+        };
+        // Step 2: trigger reauth failure (status=false). reauthenticate() itself
+        // calls axios.create once; subsequent tickle pings (which should NOT happen)
+        // would also call axios.create.
+        const reauthClient = {
+          post: vi.fn().mockResolvedValue({ data: {} }),
+          get: vi.fn().mockResolvedValue({ data: { authenticated: false } }),
+        };
+        vi.mocked(axios.create).mockReturnValueOnce(reauthClient as any);
+        // Any further axios.create calls (e.g., from a stray tickle) would hit this
+        // mock and produce visible POST /tickle calls.
+        vi.mocked(axios.create).mockReturnValue(tickleClient as any);
+
+        await client.reauthenticate();
+
+        // Advance well past the tickle interval (30s). If tickle were still running
+        // we would see /tickle POSTs against tickleClient.
+        await vi.advanceTimersByTimeAsync(120_000);
+        expect(tickleClient.post).not.toHaveBeenCalled();
+      } finally {
+        vi.useRealTimers();
+      }
+    });
+
+    it('should stop tickle when reauth throws', async () => {
+      vi.useFakeTimers();
+      try {
+        // Bring tickle up first.
+        const checkClient = {
+          get: vi.fn().mockResolvedValue({ data: { authenticated: true } }),
+        };
+        vi.mocked(axios.create).mockReturnValueOnce(checkClient as any);
+        await client.checkAuthenticationStatus();
+
+        const tickleClient = {
+          post: vi.fn().mockResolvedValue({ data: {} }),
+        };
+        const reauthClient = {
+          post: vi.fn().mockRejectedValue(new Error('Connection refused')),
+          get: vi.fn(),
+        };
+        vi.mocked(axios.create).mockReturnValueOnce(reauthClient as any);
+        vi.mocked(axios.create).mockReturnValue(tickleClient as any);
+
+        await client.reauthenticate();
+
+        await vi.advanceTimersByTimeAsync(120_000);
+        expect(tickleClient.post).not.toHaveBeenCalled();
+      } finally {
+        vi.useRealTimers();
+      }
+    });
   });
 
   describe('Cleanup', () => {

test/tool-handlers.test.ts

@@ -352,25 +352,39 @@ describe('ToolHandlers', () => {
 
     it('should call reauthenticate when auth status flips to true', async () => {
       mockIBClient.checkAuthenticationStatus = vi.fn()
-        .mockResolvedValueOnce(false)  // Continue past early return
-        .mockResolvedValueOnce(true);  // Browser path: now authenticated
+        .mockResolvedValueOnce(false) // Initial check: not yet authenticated (skip early return)
+        .mockResolvedValueOnce(true)  // Browser path: now authenticated
+        .mockResolvedValueOnce(true); // Final re-check after reauth: still authenticated
       mockIBClient.reauthenticate = vi.fn().mockResolvedValue(undefined);
 
       await (handlers as any).ensureAuth();
 
       expect(mockIBClient.reauthenticate).toHaveBeenCalled();
     });
 
-    it('should proceed even if reauthenticate fails', async () => {
+    it('should proceed when reauthenticate fails but session is still authenticated', async () => {
       mockIBClient.checkAuthenticationStatus = vi.fn()
-        .mockResolvedValueOnce(false)
-        .mockResolvedValueOnce(true);
+        .mockResolvedValueOnce(false) // Initial check: skip early return
+        .mockResolvedValueOnce(true)  // Browser path: authenticated
+        .mockResolvedValueOnce(true); // Final re-check still passes
       mockIBClient.reauthenticate = vi.fn().mockRejectedValue(new Error('Reauth failed'));
 
-      // Should not throw — error is caught and logged
+      // Reauth error is swallowed because the final auth status check still succeeds.
       await expect((handlers as any).ensureAuth()).resolves.not.toThrow();
       expect(mockIBClient.reauthenticate).toHaveBeenCalled();
     });
+
+    it('should throw when reauthenticate fails and session is no longer authenticated', async () => {
+      mockIBClient.checkAuthenticationStatus = vi.fn()
+        .mockResolvedValueOnce(false) // Initial check: skip early return
+        .mockResolvedValueOnce(true)  // Browser path: authenticated
+        .mockResolvedValueOnce(false); // Final re-check: session lost
+      mockIBClient.reauthenticate = vi.fn().mockRejectedValue(new Error('Reauth failed'));
+
+      await expect((handlers as any).ensureAuth())
+        .rejects.toThrow('Authentication required');
+      expect(mockIBClient.reauthenticate).toHaveBeenCalled();
+    });
   });
 
   describe('startBrowserAuthPolling', () => {
@@ -389,21 +403,32 @@ describe('ToolHandlers', () => {
       mockIBClient.reauthenticate = vi.fn().mockResolvedValue(undefined);
 
       (handlers as any).startBrowserAuthPolling('https://localhost:5000', 5000);
-      await vi.runAllTimersAsync();
+      await vi.advanceTimersByTimeAsync(120_000);
 
       expect(mockIBClient.checkAuthenticationStatus).toHaveBeenCalledTimes(2);
       expect(mockIBClient.reauthenticate).toHaveBeenCalledTimes(1);
     });
 
-    it('should not call reauthenticate when auth never detected', async () => {
+    it('should stop polling once the deadline passes when auth never detected', async () => {
       mockIBClient.checkAuthenticationStatus = vi.fn().mockResolvedValue(false);
       mockIBClient.reauthenticate = vi.fn();
 
       (handlers as any).startBrowserAuthPolling('https://localhost:5000', 5000);
-      await vi.runAllTimersAsync();
+      // Advance past the 2-minute deadline.
+      await vi.advanceTimersByTimeAsync(120_000);
 
-      expect(mockIBClient.checkAuthenticationStatus).toHaveBeenCalledTimes(60);
+      // Deadline-based loop produces fewer than the legacy 60 attempts since the
+      // backoff caps at 10s. We assert (a) reauthenticate was never called and
+      // (b) polling stayed within the documented 2-minute upper bound.
       expect(mockIBClient.reauthenticate).not.toHaveBeenCalled();
+      const attemptsWithinDeadline = (mockIBClient.checkAuthenticationStatus as any).mock.calls.length;
+      expect(attemptsWithinDeadline).toBeGreaterThan(0);
+      expect(attemptsWithinDeadline).toBeLessThan(60);
+
+      // Advancing further must not produce additional polls.
+      await vi.advanceTimersByTimeAsync(120_000);
+      expect((mockIBClient.checkAuthenticationStatus as any).mock.calls.length)
+        .toBe(attemptsWithinDeadline);
     });
 
     it('should handle checkAuthenticationStatus throwing without stopping', async () => {
@@ -413,7 +438,7 @@ describe('ToolHandlers', () => {
       mockIBClient.reauthenticate = vi.fn().mockResolvedValue(undefined);
 
       (handlers as any).startBrowserAuthPolling('https://localhost:5000', 5000);
-      await vi.runAllTimersAsync();
+      await vi.advanceTimersByTimeAsync(120_000);
 
       expect(mockIBClient.checkAuthenticationStatus).toHaveBeenCalledTimes(2);
       expect(mockIBClient.reauthenticate).toHaveBeenCalledTimes(1);