diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index 7e6b01e26d..02a9250614 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -943,11 +943,11 @@ describe('AppContainer State Management', () => { describe('Cancel Handler (issue #3204)', () => { // The cancel handler is wired through useGeminiStream's onCancelSubmit - // arg (positional index 14 — see the useGeminiStream call site in + // arg (positional index 15 — see the useGeminiStream call site in // AppContainer.tsx). We capture it via mockImplementation so a future // signature change surfaces as a clear test failure rather than silently // grabbing the wrong callback. - const ON_CANCEL_SUBMIT_ARG_INDEX = 14; + const ON_CANCEL_SUBMIT_ARG_INDEX = 15; type CapturedCancelSubmit = (info?: { pendingItem: HistoryItemWithoutId | null; lastTurnUserItem: { id: number; text: string } | null; diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index a680afdac8..a1cc171940 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -1483,6 +1483,7 @@ export const AppContainer = (props: AppContainerProps) => { historyManager.history, historyManager.addItem, config, + isConfigInitialized, settings, onDebugMessage, handleSlashCommand, diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index c4a4c1e71a..b13b3b24a6 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -332,6 +332,7 @@ describe('useGeminiStream', () => { props.history, props.addItem, props.config, + true, props.loadedSettings, props.onDebugMessage, props.handleSlashCommand, @@ -620,6 +621,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -719,6 +721,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -854,6 +857,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -996,6 +1000,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -1115,6 +1120,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -1239,6 +1245,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -1352,6 +1359,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -1458,6 +1466,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -1557,6 +1566,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -1645,6 +1655,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -1757,6 +1768,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -1861,6 +1873,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -2140,6 +2153,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -2278,6 +2292,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -2425,6 +2440,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -2540,6 +2556,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -2662,6 +2679,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -2863,6 +2881,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -2988,6 +3007,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -3126,6 +3146,7 @@ describe('useGeminiStream', () => { historyWithToolGroup, mockAddItem, config, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -3307,6 +3328,7 @@ describe('useGeminiStream', () => { history, mockAddItem, config, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -3804,6 +3826,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -3853,6 +3876,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -3909,6 +3933,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -3968,6 +3993,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -4049,6 +4075,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -4104,6 +4131,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -4161,6 +4189,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -4234,6 +4263,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -4582,6 +4612,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -4639,6 +4670,7 @@ describe('useGeminiStream', () => { [], mockAddItem, testConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -5132,6 +5164,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -5187,6 +5220,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -5243,6 +5277,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -5355,6 +5390,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -5409,6 +5445,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -5483,6 +5520,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -5580,6 +5618,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -5986,6 +6025,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -6102,6 +6142,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -6175,6 +6216,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -6245,6 +6287,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -6374,6 +6417,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -6429,6 +6473,7 @@ describe('useGeminiStream', () => { [], mockAddItem, mockConfig, + true, mockLoadedSettings, mockOnDebugMessage, mockHandleSlashCommand, @@ -7498,6 +7543,150 @@ describe('useGeminiStream', () => { }); }); }); + + describe('cron scheduler initialization', () => { + // Renders useGeminiStream wired to a provided cron scheduler mock, with a + // controllable isConfigInitialized gate. `config` identity is stable across + // rerenders so the cron effect only re-runs when `initialized` flips. + const renderCronHook = (scheduler: unknown, initialized: boolean) => { + const cronConfig = { + ...mockConfig, + isCronEnabled: vi.fn(() => true), + getCronScheduler: vi.fn(() => scheduler), + } as unknown as Config; + return renderHook( + (props: { initialized: boolean }) => + useGeminiStream( + new MockedGeminiClientClass(cronConfig), + [], + mockAddItem, + cronConfig, + props.initialized, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand as unknown as ( + cmd: PartListUnion, + ) => Promise, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + false, + () => {}, + () => {}, + () => {}, + () => {}, + 80, + 24, + ), + { initialProps: { initialized } }, + ); + }; + + it('defers enableDurable and start until isConfigInitialized is true', async () => { + const callOrder: string[] = []; + const scheduler = { + // A real async gap before recording: a synchronous push would make the + // order assertion pass even if production dropped the `await`. + enableDurable: vi.fn().mockImplementation(async () => { + await new Promise((r) => setTimeout(r, 10)); + callOrder.push('enableDurable'); + }), + start: vi.fn().mockImplementation(() => { + callOrder.push('start'); + }), + stop: vi.fn(), + getExitSummary: vi.fn(() => null), + hasPendingWork: false, + }; + + const { rerender } = renderCronHook(scheduler, false); + + // Before initialization: the scheduler must not be touched. + expect(scheduler.enableDurable).not.toHaveBeenCalled(); + expect(scheduler.start).not.toHaveBeenCalled(); + + rerender({ initialized: true }); + + await waitFor(() => { + expect(scheduler.start).toHaveBeenCalled(); + }); + // enableDurable is awaited before start despite the 10ms gap. + expect(callOrder).toEqual(['enableDurable', 'start']); + }); + + it('does not start scheduler when isConfigInitialized remains false', async () => { + const scheduler = { + enableDurable: vi.fn().mockResolvedValue(undefined), + start: vi.fn(), + stop: vi.fn(), + getExitSummary: vi.fn(() => null), + hasPendingWork: false, + }; + + renderCronHook(scheduler, false); + + // Give effects time to run. + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(scheduler.enableDurable).not.toHaveBeenCalled(); + expect(scheduler.start).not.toHaveBeenCalled(); + }); + + it('does not start scheduler if unmounted during the enableDurable gap', async () => { + // enableDurable stays pending until we resolve it by hand, so we can + // unmount inside the async gap — the exact race the `stopped` flag guards. + let resolveEnable: () => void = () => {}; + const scheduler = { + enableDurable: vi.fn().mockImplementation( + () => + new Promise((resolve) => { + resolveEnable = resolve; + }), + ), + start: vi.fn(), + stop: vi.fn(), + getExitSummary: vi.fn(() => null), + hasPendingWork: false, + }; + + const { unmount } = renderCronHook(scheduler, true); + + await waitFor(() => { + expect(scheduler.enableDurable).toHaveBeenCalled(); + }); + expect(scheduler.start).not.toHaveBeenCalled(); + + // Unmount while enableDurable is still in flight, then let it resolve. + unmount(); + resolveEnable(); + await new Promise((resolve) => setTimeout(resolve, 10)); + + // The stopped guard must suppress the late start(); cleanup ran stop(). + expect(scheduler.start).not.toHaveBeenCalled(); + expect(scheduler.stop).toHaveBeenCalled(); + }); + + it('still starts the scheduler when enableDurable rejects', async () => { + const scheduler = { + enableDurable: vi.fn().mockRejectedValue(new Error('lock contention')), + start: vi.fn(), + stop: vi.fn(), + getExitSummary: vi.fn(() => null), + hasPendingWork: false, + }; + + renderCronHook(scheduler, true); + + // A failed enableDurable must NOT skip start(): session-only cron tasks + // (created via cron_create during this session) still need the scheduler + // running — only durable/persistent tasks are lost. Regression guard for + // the catch falling through instead of returning (#5022 review). + await waitFor(() => { + expect(scheduler.start).toHaveBeenCalled(); + }); + }); + }); }); describe('classifyApiError', () => { diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 1233eabe37..a5bf15f657 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -349,6 +349,7 @@ export const useGeminiStream = ( history: HistoryItem[], addItem: UseHistoryManagerReturn['addItem'], config: Config, + isConfigInitialized: boolean, settings: LoadedSettings, onDebugMessage: (message: string) => void, handleSlashCommand: ( @@ -2924,44 +2925,68 @@ export const useGeminiStream = ( const cronSessionIdRef = useRef(sessionStates.sessionId); cronSessionIdRef.current = sessionStates.sessionId; - // Start the cron scheduler on mount, stop on unmount. + // Start the cron scheduler once config is initialized, stop on unmount. // Cron fires enqueue onto the shared notification queue. + // Gated on isConfigInitialized: without this gate, enableDurable() runs + // before config.initialize() completes, and overdue-task fires delivered + // through the notification drain reach a chat client whose startChat() has + // not yet run — producing "Chat not initialized" on every fresh launch + // that has pending durable work (#5022). This matches the ordering the + // ACP (Session.ts) and headless (nonInteractiveCli.ts) paths already use. useEffect(() => { + if (!isConfigInitialized) return; if (!config.isCronEnabled()) return; const scheduler = config.getCronScheduler(); - // Enable durable (file-backed) cron support (loads tasks from the - // user's per-project runtime dir, acquires the lock). The tasks file - // lives under ~/.qwen, not the working tree, so it's user-owned rather - // than project-controlled — no folder-trust gate needed; the user's - // own loops run regardless of how the folder is trusted. - // Missed one-shots arrive as late fires through the start() callback. - void scheduler.enableDurable(cronSessionIdRef.current).catch((err) => { - debugLogger.warn( - `Durable cron init failed — persistent tasks will not fire in this session: ${err}`, + let stopped = false; + // Await enableDurable before start so overdue fires buffer into + // pendingFires (onFire is still null) and flush through start()'s + // buffer-drain — matching the ACP and headless startup order. + void (async () => { + try { + // Enable durable (file-backed) cron support (loads tasks from the + // user's per-project runtime dir, acquires the lock). The tasks file + // lives under ~/.qwen, not the working tree, so it's user-owned + // rather than project-controlled — no folder-trust gate needed; the + // user's own loops run regardless of how the folder is trusted. + // Missed one-shots arrive as late fires through the start() callback. + await scheduler.enableDurable(cronSessionIdRef.current); + } catch (err) { + // Fall through (no `return`): a failed enableDurable must NOT skip + // start(), or session-only cron tasks (created via cron_create during + // this session) would silently never fire. Only durable/persistent + // tasks are lost when enableDurable fails. Pre-#5022 the unconditional + // start() preserved this; keep that behavior. + debugLogger.warn( + `Durable cron init failed — persistent tasks will not fire in this session: ${err}`, + ); + } + // Unmount may have happened during the await above; the cleanup below + // already ran scheduler.stop(), so do not (re)install onFire. + if (stopped) return; + scheduler.start( + (job: { prompt: string; cronExpr?: string; missed?: boolean }) => { + const label = job.prompt.slice(0, 40); + const source = job.cronExpr === '@wakeup' ? 'Loop' : 'Cron'; + notificationQueueRef.current.push({ + displayText: `${job.missed ? 'Missed' : source}: ${label}`, + modelText: job.prompt, + sendMessageType: SendMessageType.Cron, + }); + setNotificationTrigger((n) => n + 1); + }, ); - }); + })(); - scheduler.start( - (job: { prompt: string; cronExpr?: string; missed?: boolean }) => { - const label = job.prompt.slice(0, 40); - const source = job.cronExpr === '@wakeup' ? 'Loop' : 'Cron'; - notificationQueueRef.current.push({ - displayText: `${job.missed ? 'Missed' : source}: ${label}`, - modelText: job.prompt, - sendMessageType: SendMessageType.Cron, - }); - setNotificationTrigger((n) => n + 1); - }, - ); return () => { + stopped = true; const summary = scheduler.getExitSummary(); scheduler.stop(); if (summary) { process.stderr.write(summary + '\n'); } }; - }, [config]); + }, [config, isConfigInitialized]); // Register background agent notification callback onto the shared queue. useEffect(() => {