fix(cli): gate cron scheduler startup on config initialization (#5022) - #5230
Merged
Conversation
Collaborator
Author
E2E Verification Report — Issue #5022Bug DescriptionRace condition in TUI startup: Unit Test EvidenceuseGeminiStream tests (packages/cli)AppContainer tests (packages/cli)CronScheduler tests (packages/core)Build & Quality ChecksNew Test DetailsTest 1: "defers enableDurable and start until isConfigInitialized is true"
Test 2: "does not start scheduler when isConfigInitialized remains false"
Before/After Behavior
CommitBranch: |
Collaborator
Author
|
Thanks for the autofix PR! Template: heading names deviate from the template ( Direction: This is a straightforward race-condition fix. The TUI startup path was the odd one out — ACP ( Approach: The scope is tight — 4 files, one behavioural change (gate + await), test updates, and 2 new tests that directly verify the fix. The Moving on to code review. 🔍 中文说明感谢自动修复 PR！ 模板： 标题名称与模板不一致（用了 方向： 这是一个明确的竞态条件修复。TUI 启动路径是三个入口中唯一没有 await 方案： 范围紧凑——4 个文件，一个行为变更（gate + await），测试更新和 2 个直接验证修复的新测试。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Author
Code ReviewOne issue found — a subtle behavioural regression in the error path. The new code's // PR's code (useGeminiStream.ts)
try {
await scheduler.enableDurable(cronSessionIdRef.current);
} catch (err) {
debugLogger.warn(...);
return; // ← start() never called — session-only jobs lost
}Both the ACP and headless paths handle this differently — they catch the error, log it, and fall through to // ACP path (Session.ts) — correct behaviour
try {
await scheduler.enableDurable(this.sessionId);
} catch (err) {
debugLogger.warn(...);
// falls through to start()
}
scheduler.start(...);Fix: remove the try {
await scheduler.enableDurable(cronSessionIdRef.current);
} catch (err) {
debugLogger.warn(
`Durable cron init failed — persistent tasks will not fire in this session: ${err}`,
);
}
if (stopped) return;
scheduler.start(...);Everything else in the diff looks correct. The Unit TestsAll pass on the PR branch:
Real-Scenario Testing (tmux)The specific bug (race condition with overdue durable tasks at TUI launch) requires pre-existing durable cron state to reproduce — this CI environment has none. Basic smoke test confirms the CLI starts and responds without errors on the PR branch: Smoke test (PR branch,
|
Collaborator
Author
ReflectionThis is a good fix for a real bug — the TUI startup race that produces "Chat not initialized" when durable cron tasks are overdue. The diagnosis is spot-on: the TUI path was the odd one out, fire-and-forgetting The one thing that needs fixing before merge: the Once that's addressed, this is ready to ship. The test coverage is solid — 2 new tests that directly verify the gate behaviour — and the smoke test shows clean startup. Verdict: Requesting changes for the error-path regression. The core fix is correct and well-scoped. 中文说明总结这是一个好的修复，解决了真实的 bug——TUI 启动时持久定时任务过期导致的 "Chat not initialized" 竞态条件。诊断准确：TUI 路径是三个入口中唯一没有 await 合并前需要修复的一个问题：新的 async IIFE 中的 修复后即可合并。测试覆盖充分——2 个新测试直接验证了门控行为——冒烟测试显示启动正常。 结论： 因错误路径回归请求修改。核心修复正确且范围合理。 — Qwen Code · qwen3.7-max |
Collaborator
Author
|
@qwen-code-ci-bot — one fix needed before merge: in the new async IIFE in See the Stage 2 comment above for the exact code change. |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
Author
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
wenshao
requested changes
Jun 18, 2026
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).
wenshao
force-pushed
the
autofix/issue-5022
branch
from
June 19, 2026 12:03
2c81852 to
c877ae6
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
Author
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR; CI still running. — qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jun 19, 2026
qqqys
approved these changes
Jun 19, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Earlier critical feedback on the enableDurable failure path is addressed in the current head. I rechecked the TUI cron startup ordering and did not find new critical issues in this pass.
wenshao
reviewed
Jun 20, 2026
| }, | ||
| ); | ||
| }); | ||
| })(); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This floating async IIFE has no .catch(), and scheduler.start() runs outside the inner try/catch (which only wraps enableDurable). It's the only unguarded floating promise in this file — the others guard their tail (e.g. void onComplete().catch(...) at line 2146). If start() (or a future change to the onFire callback) ever throws synchronously, it surfaces as an unhandled rejection, which this codebase notes crashes Node ≥22 (cronScheduler.ts:524-536). Not triggerable today (start()/onFire don't throw), so this is defensive consistency rather than a live bug.
Suggested change
| })(); | |
| })().catch((err) => { | |
| debugLogger.warn(`Cron scheduler effect failed: ${err}`); | |
| }); |
中文
这个浮动的 async IIFE 没有 .catch()，而 scheduler.start() 在内层 try/catch（只包住 enableDurable）之外执行。它是本文件中唯一没有尾部兑底的浮动 promise——其它都加了 .catch()（如第 2146 行 void onComplete().catch(...)）。一旦 start()（或将来 onFire 回调被改动）同步抛错，就会变成 unhandled rejection，而本仓库注释明确指出这在 Node ≥22 会让进程崩溃（cronScheduler.ts:524-536）。当前不可触发（start()/onFire 不会抛错），所以这是防御性一致性建议，不是现存 bug。
— claude-opus-4-8 via Claude Code /qreview
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a race condition in the TUI startup path that produced "Chat not initialized" errors when durable cron tasks had pending overdue fires at launch time.
Motivation
The durable cron scheduler supports persistent tasks that survive across sessions. When a user launches the CLI with overdue durable tasks, those tasks should fire as late notifications. However, the TUI startup path had a race: it called
enableDurable()fire-and-forget (without awaiting), then immediately calledscheduler.start()synchronously. Becausestart()installs theonFirecallback beforeenableDurable()finishes loading tasks from disk, overdue fires were delivered instantly viafireOrBuffer()into the notification queue. The queue-drain effect then tried to submit those fires through a chat client whosestartChat()hadn't completed — producing "Chat not initialized".The ACP and headless paths already handled this correctly by awaiting
enableDurable()before callingstart().Changes
useGeminiStream.ts: Added anisConfigInitialized: booleanparameter. Rewrote the cronuseEffectto gate onisConfigInitializedandawait enableDurable()before callingstart(), matching the ordering used by the ACP (Session.ts) and headless (nonInteractiveCli.ts) paths. Astoppedflag prevents a stalestart()if the component unmounts during the async gap. OnenableDurable()failure the catch logs and falls through (does notreturn) sostart()still runs — a failed durable init must not silently disable session-only cron tasks (created viacron_createduring the session); only durable/persistent tasks are lost.AppContainer.tsx: Pass the existingisConfigInitializedstate intouseGeminiStream.AppContainer.test.tsx: BumpedON_CANCEL_SUBMIT_ARG_INDEX14 → 15 (positional shift from the new parameter).useGeminiStream.test.tsx: Updated all 45 existinguseGeminiStream()call sites with the newtrueargument. Added acron scheduler initializationdescribe block with 4 tests:enableDurable/startuntilisConfigInitializedis true, assertingenableDurableis awaited beforestart— with a real async gap in the mock so the order assertion has teeth.isConfigInitializedstays false.enableDurablegap (covers thestoppedguard).enableDurablerejects (covers the catch fall-through).Tests 3 and 4 are mutation-verified: removing the
stoppedguard / re-adding the earlyreturnmakes the respective test fail.Reviewer Test Plan
How to verify the fix:
/loop 1m echo hello).Automated verification:
cd packages/cli && npx vitest run src/ui/hooks/useGeminiStream.test.tsx— 128 tests pass, including 4 new cron initialization tests.cd packages/cli && npx vitest run src/ui/AppContainer.test.tsx— 93 tests pass.npm run build --workspace @qwen-code/qwen-code-core && (cd packages/cli && npx tsc --noEmit)— typecheck clean.npx eslinton the four changed files — clean.Fixes #5022