From ad56a84eff8b5954134a80ddf75b874c0f2a7886 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Sat, 20 Jun 2026 01:00:40 +0800 Subject: [PATCH] fix(cli): gate cron scheduler startup on config initialization (#5022) (#5230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TUI startup path called enableDurable() fire-and-forget and then start() synchronously. Because start() installs onFire before enableDurable() finishes loading tasks from disk, overdue durable fires were delivered instantly into the notification queue and reached a chat client whose startChat() had not completed — producing "Chat not initialized" on fresh launches with pending durable work. Gate the cron effect on isConfigInitialized and await enableDurable() before start(), matching the ordering the ACP (Session.ts) and headless (nonInteractiveCli.ts) paths already use. A `stopped` flag prevents a stale start() if the component unmounts during the async gap. On enableDurable() failure the catch falls through (does not return) so start() still runs: a failed durable init must not silently disable session-only cron tasks (created via cron_create during the session) — only durable/persistent tasks are lost. Tests: gate ordering (with a real async gap so the assertion has teeth), gate-stays-closed, unmount-during-gap, and enableDurable-rejection- still-starts. The last two lock in the unmount guard and the fall-through fix (both verified by mutation). Co-authored-by: Shaojin Wen --- packages/cli/src/ui/AppContainer.test.tsx | 4 +- packages/cli/src/ui/AppContainer.tsx | 1 + .../cli/src/ui/hooks/useGeminiStream.test.tsx | 189 ++++++++++++++++++ packages/cli/src/ui/hooks/useGeminiStream.ts | 73 ++++--- 4 files changed, 241 insertions(+), 26 deletions(-) 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(() => {