Merge pull request #504 from kaitranntt/fix/314-qwen-auth-hang

fix(cliproxy): prevent OAuth process hang on Qwen device code flow

src/cliproxy/auth/oauth-process.ts

@@ -7,6 +7,7 @@
 
 import { spawn, ChildProcess } from 'child_process';
 import { ok, fail, info, warn } from '../../utils/ui';
+import { killWithEscalation } from '../../utils/process-utils';
 import { tryKiroImport } from './kiro-import';
 import { CLIProxyProvider } from '../types';
 import { AccountInfo } from '../account-manager';
@@ -333,8 +334,8 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
     // H8: Also clear stdinKeepalive interval to prevent memory leak
     const cleanup = () => {
       if (stdinKeepalive) clearInterval(stdinKeepalive);
-      if (authProcess && !authProcess.killed) {
-        authProcess.kill('SIGTERM');
+      if (authProcess && authProcess.exitCode === null) {
+        killWithEscalation(authProcess);
       }
     };
     process.on('SIGINT', cleanup);
@@ -358,9 +359,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
 
     // Listen for external cancel signal
     const handleCancel = (cancelledSessionId: string) => {
-      if (cancelledSessionId === state.sessionId && authProcess && !authProcess.killed) {
+      if (cancelledSessionId === state.sessionId && authProcess && authProcess.exitCode === null) {
         log('Session cancelled externally');
-        authProcess.kill('SIGTERM');
+        killWithEscalation(authProcess);
       }
     };
     authSessionEvents.on('session:cancelled', handleCancel);
@@ -425,7 +426,8 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
     }, 2000);
 
     // Timeout handling
-    const timeoutMs = headless ? 300000 : 120000;
+    // Device code flows need longer timeout to match CLIProxy binary's polling window (60 attempts × 5s = 300s)
+    const timeoutMs = headless || isDeviceCodeFlow ? 300000 : 120000;
     const timeout = setTimeout(() => {
       // H7: Clear stdin keepalive interval
       if (stdinKeepalive) clearInterval(stdinKeepalive);
@@ -435,9 +437,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
       authSessionEvents.removeListener('session:cancelled', handleCancel);
       unregisterAuthSession(state.sessionId);
       cancelProjectSelection(state.sessionId);
-      authProcess.kill();
+      killWithEscalation(authProcess);
       console.log('');
-      console.log(fail(`OAuth timed out after ${headless ? 5 : 2} minutes`));
+      console.log(fail(`OAuth timed out after ${timeoutMs / 60000} minutes`));
       for (const line of getTimeoutTroubleshooting(provider, callbackPort ?? null)) {
         console.log(line);
       }

src/delegation/headless-executor.ts

@@ -7,6 +7,7 @@
 
 import { spawn } from 'child_process';
 import * as path from 'path';
+import { killWithEscalation } from '../utils/process-utils';
 import * as fs from 'fs';
 import { SessionManager } from './session-manager';
 import { SettingsParser } from './settings-parser';
@@ -214,11 +215,8 @@ export class HeadlessExecutor {
 
       // Setup signal handlers for cleanup
       const cleanupHandler = () => {
-        if (!proc.killed) {
-          proc.kill('SIGTERM');
-          setTimeout(() => {
-            if (!proc.killed) proc.kill('SIGKILL');
-          }, 2000);
+        if (proc.exitCode === null) {
+          killWithEscalation(proc, 2000);
         }
       };
       process.once('SIGINT', cleanupHandler);
@@ -326,16 +324,14 @@ export class HeadlessExecutor {
       // Handle timeout
       if (timeout > 0) {
         const timeoutHandle = setTimeout(() => {
-          if (!proc.killed) {
+          if (proc.exitCode === null) {
             timedOut = true;
             if (progressInterval) {
               clearInterval(progressInterval);
               process.stderr.write('\r\x1b[K');
             }
-            proc.kill('SIGTERM');
-            setTimeout(() => {
-              if (!proc.killed) proc.kill('SIGKILL');
-            }, 10000);
+            // Longer grace period for timeout (vs 2s for SIGINT) since delegated sessions may need time to flush output
+            killWithEscalation(proc, 10000);
           }
         }, timeout);
         proc.on('close', () => clearTimeout(timeoutHandle));

src/utils/process-utils.ts

@@ -0,0 +1,21 @@
+/**
+ * Process management utilities
+ */
+
+import { ChildProcess } from 'child_process';
+
+/**
+ * Kill process with SIGTERM, escalating to SIGKILL if it doesn't exit.
+ * Uses exitCode === null (not proc.killed) to check if process is still running,
+ * since proc.killed only indicates a signal was sent, not that the process exited.
+ */
+export function killWithEscalation(proc: ChildProcess, gracePeriodMs = 3000): void {
+  proc.kill('SIGTERM');
+  const timer = setTimeout(() => {
+    if (proc.exitCode === null) {
+      proc.kill('SIGKILL');
+    }
+  }, gracePeriodMs);
+  timer.unref(); // Don't keep event loop alive just for escalation
+  proc.once('exit', () => clearTimeout(timer));
+}

tests/unit/utils/process-utils.test.ts

@@ -0,0 +1,132 @@
+/**
+ * Unit tests for process-utils.ts
+ */
+import { describe, it, expect, beforeEach, afterEach, jest } from 'bun:test';
+import { EventEmitter } from 'events';
+import { killWithEscalation } from '../../../src/utils/process-utils';
+import type { ChildProcess } from 'child_process';
+
+// Mock ChildProcess using EventEmitter
+function createMockProcess(exitCode: number | null = null): ChildProcess {
+  const proc = new EventEmitter() as any;
+  proc.killed = false;
+  proc.exitCode = exitCode;
+  proc.kill = jest.fn((signal?: string) => {
+    if (signal === 'SIGTERM' || signal === 'SIGKILL') {
+      proc.killed = true;
+    }
+    return true;
+  });
+  return proc as ChildProcess;
+}
+
+describe('killWithEscalation', () => {
+  beforeEach(() => {
+    jest.useFakeTimers();
+  });
+
+  afterEach(() => {
+    jest.useRealTimers();
+  });
+
+  it('should send SIGTERM immediately', () => {
+    const proc = createMockProcess();
+    killWithEscalation(proc);
+
+    expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+    expect(proc.kill).toHaveBeenCalledTimes(1);
+  });
+
+  it('should send SIGKILL after grace period if process still running', () => {
+    const proc = createMockProcess(null); // exitCode null = still running
+    killWithEscalation(proc, 3000);
+
+    // SIGTERM sent immediately
+    expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+    expect(proc.kill).toHaveBeenCalledTimes(1);
+
+    // Advance time by grace period
+    jest.advanceTimersByTime(3000);
+
+    // SIGKILL sent after grace period
+    expect(proc.kill).toHaveBeenCalledWith('SIGKILL');
+    expect(proc.kill).toHaveBeenCalledTimes(2);
+  });
+
+  it('should NOT send SIGKILL if process exits before grace period', () => {
+    const proc = createMockProcess(null);
+    killWithEscalation(proc, 3000);
+
+    expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+    expect(proc.kill).toHaveBeenCalledTimes(1);
+
+    // Simulate process exit after 1 second
+    jest.advanceTimersByTime(1000);
+    proc.exitCode = 0; // Process exited
+    proc.emit('exit', 0);
+
+    // Advance remaining time
+    jest.advanceTimersByTime(2000);
+
+    // SIGKILL should NOT have been sent
+    expect(proc.kill).toHaveBeenCalledTimes(1);
+    expect(proc.kill).not.toHaveBeenCalledWith('SIGKILL');
+  });
+
+  it('should use default grace period of 3000ms', () => {
+    const proc = createMockProcess(null);
+    killWithEscalation(proc); // No grace period argument
+
+    expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+
+    // Advance by default 3000ms
+    jest.advanceTimersByTime(3000);
+
+    expect(proc.kill).toHaveBeenCalledWith('SIGKILL');
+  });
+
+  it('should respect custom grace period', () => {
+    const proc = createMockProcess(null);
+    killWithEscalation(proc, 5000); // Custom 5 second grace period
+
+    expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+
+    // Advance by less than grace period
+    jest.advanceTimersByTime(4999);
+    expect(proc.kill).toHaveBeenCalledTimes(1); // Still only SIGTERM
+
+    // Advance to grace period
+    jest.advanceTimersByTime(1);
+    expect(proc.kill).toHaveBeenCalledWith('SIGKILL');
+  });
+
+  it('should clear timer when process exits', () => {
+    const proc = createMockProcess(null);
+    killWithEscalation(proc, 3000);
+
+    // Simulate immediate exit
+    proc.exitCode = 0;
+    proc.emit('exit', 0);
+
+    // Advance way past grace period
+    jest.advanceTimersByTime(10000);
+
+    // Should only have SIGTERM, timer was cleared
+    expect(proc.kill).toHaveBeenCalledTimes(1);
+    expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+  });
+
+  it('should handle process that already exited', () => {
+    const proc = createMockProcess(0); // Already exited
+    killWithEscalation(proc, 3000);
+
+    expect(proc.kill).toHaveBeenCalledWith('SIGTERM');
+
+    // Even though exitCode is not null, timer still fires
+    // (because we check exitCode at timer callback time)
+    jest.advanceTimersByTime(3000);
+
+    // SIGKILL should NOT be sent because exitCode is not null
+    expect(proc.kill).toHaveBeenCalledTimes(1);
+  });
+});