From 30f5bff43872300c84519d30dab956aa28861501 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Wed, 5 Aug 2026 09:09:44 +0300 Subject: [PATCH] fix(channels/connect): remove 5-min CONNECT_TIMEOUT, poll until Ctrl+C (AIT-334) - pollForNewChannels no longer rejects after 5 minutes: real Meta OAuth (login + 2FA + permission screens) routinely exceeds it, and the timeout reported failure to users whose connect then succeeded (HOOKMYAPP-CLI-15). - onStillWaiting hook fires every 30s so the open-ended wait stays visible; runChannelsConnect prints elapsed minutes + Ctrl+C hint. - Up-front notice when the workspace already has a connected channel of the requested type: the poll only reports NEW connections, so a re-run on an already-connected workspace previously hit a guaranteed timeout with no clue the connect had already succeeded. --- .../__tests__/channels-connect-poll.test.ts | 47 +++++++++++-------- src/commands/channels-connect-poll.ts | 29 +++++++----- src/commands/channels.ts | 39 ++++++++++++--- 3 files changed, 76 insertions(+), 39 deletions(-) diff --git a/src/commands/__tests__/channels-connect-poll.test.ts b/src/commands/__tests__/channels-connect-poll.test.ts index 24c31e2..fe0d0fd 100644 --- a/src/commands/__tests__/channels-connect-poll.test.ts +++ b/src/commands/__tests__/channels-connect-poll.test.ts @@ -102,7 +102,7 @@ describe('pollForNewChannels — D2 acceptance criteria', () => { expect(result.map((c) => c.id)).toEqual(['ch_REAUTH']); }); - it('legacy backend (no updatedAt on DTO): existing channel ignored even if row updated → falls back to id-diff', async () => { + it('legacy backend (no updatedAt on DTO): existing channel ignored even if row updated → falls back to id-diff, keeps waiting', async () => { const reauthedNoTimestamp = { id: 'ch_REAUTH', type: 'whatsapp', workspaceId: 'ws_TEST0001', metaWabaId: '1', metaResourceId: '1', connectionType: 'cloud_api', @@ -115,30 +115,37 @@ describe('pollForNewChannels — D2 acceptance criteria', () => { const snapshot = new Map([ ['ch_REAUTH', undefined], // older snapshot had no updatedAt either ]); + let settled = false; const promise = pollForNewChannels('ws_TEST0001', snapshot); - promise.catch(() => {}); - for (let elapsed = 0; elapsed < 5 * 60 * 1000 + 1000; elapsed += 30_000) { - await vi.advanceTimersByTimeAsync(30_000); - } - await expect(promise).rejects.toThrow(/No channels appeared within 5 minutes/); + void promise.finally(() => { settled = true; }); + await vi.advanceTimersByTimeAsync(60_000); + expect(settled).toBe(false); // existing channel never reported; still polling }, 30_000); - it('rejects with CONNECT_TIMEOUT after 5 minutes of no new channels', async () => { - vi.mocked(apiClient).mockResolvedValue([]); // every poll returns no new channels + it('no hard timeout (AIT-334): resolves with a channel arriving after 6 minutes instead of rejecting at 5', async () => { + vi.mocked(apiClient).mockResolvedValue([]); // nothing new for the first 6 min const promise = pollForNewChannels('ws_TEST0001', new Map()); - // Attach a no-op catch synchronously so an unhandled rejection cannot - // bubble while we're advancing fake timers iteration-by-iteration. The - // real assertion is still done via `await expect(...).rejects.toThrow` - // below. - promise.catch(() => {}); - // 150 iterations × 2000ms of fake-timer advance need a generous real-time - // budget because each iteration drains microtasks (the awaited apiClient - // mock promise) before the next setTimeout resolves. Vitest's default - // 5s test timeout is real-wall-clock and trips before the fake-time - // 5min loop completes. Advance in 30s chunks to amortize microtask drain. - for (let elapsed = 0; elapsed < 5 * 60 * 1000 + 1000; elapsed += 30_000) { + // Advance in 30s chunks to amortize microtask drain (each iteration + // awaits the mocked apiClient promise before the next setTimeout). + for (let elapsed = 0; elapsed < 6 * 60 * 1000; elapsed += 30_000) { await vi.advanceTimersByTimeAsync(30_000); } - await expect(promise).rejects.toThrow(/No channels appeared within 5 minutes/); + vi.mocked(apiClient).mockResolvedValue([wa]); // OAuth finally completed + await vi.advanceTimersByTimeAsync(2000); // poll picks it up + await vi.advanceTimersByTimeAsync(4000); // stability window + const result = await promise; + expect(result.map((c) => c.id)).toEqual(['ch_NEW_WA']); + }, 30_000); + + it('fires onStillWaiting roughly every 30s while nothing has appeared, then stops once a channel shows', async () => { + vi.mocked(apiClient).mockResolvedValue([]); + const hints: number[] = []; + const promise = pollForNewChannels('ws_TEST0001', new Map(), (ms) => hints.push(ms)); + await vi.advanceTimersByTimeAsync(70_000); // ~2 hint intervals + expect(hints.length).toBe(2); + vi.mocked(apiClient).mockResolvedValue([wa]); + await vi.advanceTimersByTimeAsync(2000); + await vi.advanceTimersByTimeAsync(4000); + await promise; }, 30_000); }); diff --git a/src/commands/channels-connect-poll.ts b/src/commands/channels-connect-poll.ts index 95e4486..60a46b1 100644 --- a/src/commands/channels-connect-poll.ts +++ b/src/commands/channels-connect-poll.ts @@ -1,10 +1,9 @@ // src/commands/channels-connect-poll.ts import { apiClient } from '../api/client.js'; import { parseChannelListItem, type Channel } from '../api/channel.js'; -import { CliError } from '../output/error.js'; /** - * D2 polling acceptance criteria: + * D2 polling acceptance criteria (AIT-334 revision — no hard timeout): * 1. Caller snapshots existing {channelId -> updatedAt} BEFORE opening OAuth * (race-safe — if the snapshot ran AFTER open(), a fast backend write * could include the new channel in "existing" and never report it). @@ -17,10 +16,14 @@ import { CliError } from '../output/error.js'; * updates the row but creates no new id). * (b) requires the backend to return `updatedAt` on the list DTO. * Older backends omit it; in that case (b) is effectively disabled - * and we fall back to id-diff-only (legacy 5min-hang behaviour for - * re-auth, which is no worse than before). - * 4. Exit when: (interesting.length > 0 AND no new interesting in last 4s), - * OR 5min hard timeout. + * and we fall back to id-diff-only. + * 4. Exit when: interesting.length > 0 AND no new interesting in last 4s. + * There is NO hard timeout — a real Meta OAuth (login + 2FA + + * permission screens) routinely exceeds 5 minutes, and the old + * 5-min CONNECT_TIMEOUT reported failure to users whose connect + * then succeeded (Sentry HOOKMYAPP-CLI-15). Ctrl+C is the only + * way out; `onStillWaiting` fires every 30s so the wait is + * visibly alive. * 5. Return ALL interesting channels. * * On Ctrl+C: Node's default behavior terminates the process on unhandled @@ -33,20 +36,16 @@ import { CliError } from '../output/error.js'; export async function pollForNewChannels( workspaceId: string, snapshot: ReadonlyMap, + onStillWaiting?: (elapsedMs: number) => void, ): Promise { const POLL_INTERVAL_MS = 2000; const STABILITY_WINDOW_MS = 4000; - const HARD_TIMEOUT_MS = 5 * 60 * 1000; + const STILL_WAITING_INTERVAL_MS = 30_000; const start = Date.now(); let lastChangeAt = 0; + let lastHintAt = start; const seen = new Map(); while (true) { - if (Date.now() - start > HARD_TIMEOUT_MS) { - throw new CliError( - 'No channels appeared within 5 minutes. Did you complete the OAuth flow in your browser?', - 'CONNECT_TIMEOUT', - ); - } await new Promise((r) => setTimeout(r, POLL_INTERVAL_MS)); const dtos = (await apiClient('/meta/channels', { workspaceId })) as unknown[]; for (const dto of dtos) { @@ -67,5 +66,9 @@ export async function pollForNewChannels( if (seen.size > 0 && Date.now() - lastChangeAt >= STABILITY_WINDOW_MS) { return Array.from(seen.values()); } + if (seen.size === 0 && Date.now() - lastHintAt >= STILL_WAITING_INTERVAL_MS) { + lastHintAt = Date.now(); + onStillWaiting?.(Date.now() - start); + } } } diff --git a/src/commands/channels.ts b/src/commands/channels.ts index bcaef33..6551c84 100644 --- a/src/commands/channels.ts +++ b/src/commands/channels.ts @@ -213,11 +213,33 @@ export async function runChannelsConnect( // channel could be included in the snapshot and never reported. // updatedAt lets the poll also detect re-auth of an existing channel // (token rotation bumps the row without creating a new id). - const initialDtos = (await apiClient('/meta/channels', { workspaceId })) as unknown[]; + const initialChannels = ( + (await apiClient('/meta/channels', { workspaceId })) as unknown[] + ).map(parseChannelListItem); const snapshot = new Map( - initialDtos.map(parseChannelListItem).map((c) => [c.id, c.updatedAt]), + initialChannels.map((c) => [c.id, c.updatedAt]), ); + // AIT-334: if a connected channel of this type already exists, say so up + // front — the poll below only reports NEW connections, so a user re-running + // connect on an already-connected workspace would otherwise wait on a + // channel that may never come, with no clue the connect already succeeded. + const alreadyConnected = initialChannels.filter( + (ch) => ch.type === type && ch.metaConnected, + ); + for (const ch of alreadyConnected) { + const label = + ch.type === 'whatsapp' + ? (ch.whatsappDisplayPhoneNumber ?? ch.id) + : ch.type === 'instagram' + ? `@${ch.instagramUsername ?? ch.id}` + : ch.id; + status( + `Note: this workspace already has a connected ${type} channel: ${label}. ` + + `Waiting for a NEW connection — press Ctrl+C if you meant that one.`, + ); + } + // 2. Route to the per-type OAuth start endpoint via the pure helper. const { path, body } = buildConnectStartRequest(type); const { redirectUrl } = (await apiClient(path, { @@ -241,10 +263,15 @@ export async function runChannelsConnect( console.log(redirectUrl + '\n'); try { await open(redirectUrl); } catch { /* no browser — URL already printed */ } } - status('Waiting for channel(s)...'); - - // 4. Poll for new/updated channels per D2 acceptance criteria. - const newChannels = await pollForNewChannels(workspaceId, snapshot); + status('Waiting for channel(s)... (Ctrl+C to cancel)'); + + // 4. Poll for new/updated channels per D2 acceptance criteria. No hard + // timeout (AIT-334) — a periodic hint keeps the wait visibly alive. + const newChannels = await pollForNewChannels(workspaceId, snapshot, (elapsedMs) => + status( + `Still waiting (${Math.round(elapsedMs / 60_000)} min) — finish the sign-in in your browser, or Ctrl+C to cancel.`, + ), + ); // 5. Report all new channels by type (D7 coexistence shape). In JSON mode // these go to stderr (via `status`) so stdout stays the single