feat(scheduled-tasks): run each task in its own dedicated, named session - #6389
Conversation
Scheduled tasks created through the Web Shell management page were never firing in the daemon-only case: the durable-cron tick runs inside an active agent session, and the Web Shell creates a session only lazily on the first prompt, so a task created on the management page (with no chat open) had nothing ticking it. This binds every management-page task to a dedicated session, minted at create time and named "⏰ <task>". The task fires ONLY inside that session — its transcript is the task's run history — instead of via the shared per-project durable owner. A daemon-side keepalive heartbeats those sessions so the idle reaper doesn't stop them, and a boot-time rehydration reloads them after a restart. Archiving, deleting, or unarchiving the session disables, removes, or re-enables the bound task (covered on both the REST and ACP surfaces). Also adds task editing, a live next-run countdown, run history, a one-per-row card layout, and a "run now" that executes in the task's bound session and updates the last-run time. All resident-session management is opt-in and enabled only by the real daemon (runQwenServe), so createServeApp embeds/tests are unaffected.
|
Thanks for the PR, @wenshao — re-run after the latest round of fixes ( Template: complete ✓ — all required sections present (What, Why, Reviewer Test Plan with How to verify + Evidence + Tested on, Risk & Scope, Linked Issues, 中文说明). Problem: Real and well-documented. A scheduled task created on the Web Shell management page never fires when no chat session is open — the durable cron ticks require an active session, and the Web Shell creates sessions lazily. Screenshots in the PR body show the "Never run" state. No linked issue, but the before/after is directly observable. Direction: Clearly aligned. The CHANGELOG shows sustained investment in cron/scheduled tasks and Web Shell. Making management-page tasks reliable is the natural next step — this is the third iteration in the area. Size: Core paths ( Approach: The resident-session-per-task design is the right fit for the existing architecture. The scope is large (~4,600 additions across 32 files) but well-decomposed: session binding + keepalive + rehydration + lifecycle coupling in the backend, task editing + countdown + run history + card layout in the frontend. The Moving on to code review and test verification. 🔍 中文说明感谢贡献，@wenshao——这是在最新修复（ 模板：完整 ✓ 问题：真实存在且有文档记录。管理页创建的定时任务在没有打开对话时永远不触发。PR 正文中的截图展示了"尚未运行"的状态。 方向：与产品方向一致。CHANGELOG 中持续有 cron/定时任务和 Web Shell 的迭代。 规模：核心路径 314 行生产代码 + 407 行测试。低于 500 行关注阈值。 方案：常驻 session 设计与现有架构匹配。~4,600 行新增跨 32 个文件，分解良好。最新提交修复了水合死锁、"通过对话创建"和后续 review 意见。 进入代码审查和测试验证 🔍 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Two required sections from the PR template are missing: Risk & Scope and Linked Issues. Please add them so we can proceed with the full review. See my comment above for the Stage 1 analysis.
|
Added the missing Risk & Scope and Linked Issues sections to the description — the template should be complete now. |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review Summary
Overall a solid, well-designed PR with comprehensive test coverage. The session-per-task architecture cleanly solves the "management-page tasks never fire" problem, and the opt-in manageScheduledTaskSessions flag properly isolates tests/embeds.
Minor observations (non-blocking)
1. updateSessionMetadata not awaited (packages/cli/src/serve/routes/scheduled-tasks.ts)
The call inside the create route is wrapped in a sync try/catch, but if the real bridge implementation is async (returns a Promise), a rejection would escape as an unhandled promise rejection rather than being caught. Since the interface declares the return as unknown, consider either adding await or constraining the interface to void to make the sync contract explicit.
2. recordHeartbeat sync/async ambiguity (packages/cli/src/serve/scheduled-task-keepalive.ts)
Same pattern — KeepaliveBridge.recordHeartbeat returns unknown and is called without await inside a try/catch. If the real bridge ever returns a rejected promise, it will slip through. Worth clarifying the contract (either void for sync, or Promise<unknown> + await).
Both are best-effort paths and unlikely to cause real issues in practice, but tightening the type contract would prevent surprises if the bridge implementation evolves.
Design highlights
- Rollback of the minted session on create failure prevents resident session leaks
- Single
appendCronRunshared by all persist sites ensures consistent ring-buffer cap parseCronToBuilderround-trip guarantee (tested) means editing never silently rewrites a schedule- Session lifecycle coupling via archive/delete choke points covers both REST and ACP surfaces
LGTM — approving.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary
9 review agents + 3 reverse-audit rounds | tsc + eslint clean | 797 unit tests pass
3 Critical · 10 Suggestions · 2 Needs Human Review
Critical
- Missing test:
deleteDaemonSessions→removeTasksForSessions— the archive/unarchive paths have integration tests for their scheduled-task coupling, butdeleteDaemonSessionshas no test verifyingremoveTasksForSessionsis called. A refactor could silently drop the coupling. - Missing test:
unarchiveDaemonSessions→enableTasksForSessions— same pattern: the archive tests verifydisableTasksForSessions, but no unarchive test verifiesenableTasksForSessionsre-enables a bound task. - Empty
catch {}blocks in keepalive heartbeat — bothreadCronTasksandrecordHeartbeathave empty catch blocks. If heartbeat consistently fails (bridge corruption, permission change), there is zero diagnostic output.
Key Suggestions
- DELETE route does 3 separate file reads (TOCTOU + inefficiency) — use a single
updateCronTaskscall handleRunNowfiresrunScheduledTaskas fire-and-forget with noreload(), unlikehandleToggle/handleDeletestopScheduledTaskKeepaliveis stored onapp.localsbut never invoked during shutdownhandleMissedparameter name is misleading — it only gates unbound task missed-fire handlingREVERSE_DEFAULTSduplicatesDEFAULT_BUILDER— export one, derive the other- Sequential session rehydration — parallelize with
Promise.allSettled DaemonScheduledTaskRuntype missingsessionIdfieldscheduledTaskSessionNametruncation is untestedisValidTaskacceptssessionId: ""— scheduler silently loses the bindingPOST /rununalignedlastFiredAt— other writers minute-align
Needs Human Review
- PATCH route doesn't sync session display name on task rename
unarchiveDaemonSessionsdoesn't reload the session after enabling — task won't fire until manual open or restart
See inline comments for details. — qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Downgraded from Request changes to Comment: self-PR.
| * A task bound to a *different* session is never fired here. | ||
| */ | ||
| #shouldFireDurable(job: CronJob): boolean { | ||
| if (job.boundSessionId !== undefined) { |
There was a problem hiding this comment.
[Critical] Matching boundSessionId bypasses the per-project durable lock entirely. If two qwen serve daemons for the same workspace both rehydrate/load the same bound session, both schedulers pass this check and processJob dispatches before updateCronTasks serializes lastFiredAt. That can duplicate external side effects and append conflicting run records. Please add a cross-process claim/lease for bound task fires before dispatch, or make bound session ownership exclusive across daemons.
— GPT-5 via Qwen Code /review
There was a problem hiding this comment.
Confirmed real and reachable — thank you. The durable persist is fire-then-stamp with an unconditional overwrite (no compare-and-set on the on-disk lastFiredAt), and nothing forces a bound session to be live in exactly one scheduler: two qwen serve daemons for one workspace both rehydrate it, or a user opening the task's session interactively while the daemon keeps it resident, gives two schedulers that both pass #shouldFireDurable and both dispatch.
A correct fix needs a claim-then-fire on the durable file — atomically advance lastFiredAt to the slot before onFire, and skip dispatch if another process already claimed it. That reorders the tick's fire/persist path (today onFire runs before the batched updateCronTasks), which is high blast radius on the core scheduler (147 tests, jitter-window + catch-up interactions). Rather than fold a risky core rewrite into this PR, I'm tracking it as a focused follow-up and leaving this thread open so it isn't lost. Scope note: this is a pre-existing property of multi-scheduler durable cron that the per-session firing makes newly relevant for bound tasks; single-daemon, single-instance-per-session (the normal case) is unaffected.
Review fixes for QwenLM#6389: - Distinguish archive-disabled from user-disabled tasks: disableTasksForSessions now marks disabledByArchive; enableTasksForSessions only re-enables tasks carrying that flag, so a task the user deliberately disabled stays disabled across an archive/unarchive cycle. [Critical] - Rehydrate task sessions concurrently with a per-session 30s timeout so one hung loadSession can't stall the boot sweep or leave healthy tasks dormant. [Critical] - Await runScheduledTask + reload before executing the prompt in handleRunNow, so a record failure surfaces and the card's "last run" reflects the trigger. [Critical] - Log keepalive/rehydrate read + heartbeat failures at debug instead of swallowing them silently, so a persistently-failing keepalive is diagnosable. [Critical] - Add integration tests: deleteDaemonSessions -> removeTasksForSessions and unarchiveDaemonSessions -> enableTasksForSessions (guard the coupling). [Critical] - DELETE route: single atomic updateCronTasks that captures the bound session and removes the task in one cycle, closing the read-then-remove TOCTOU. - Stop the keepalive timer during shutdown (matters for embedders that don't process.exit) so it can't fire against a disposed bridge. - Deduplicate DEFAULT_BUILDER: export it once from scheduledTasksSchedule and drop the dialog's copy so the create form and cron-reversal can't drift. - Reject empty-string sessionId in isValidTask: a bound task with "" would silently run unbound under the scheduler's truthy guard.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-review after fix commit (1cd4395)
The fix commit properly addresses all Critical/Suggestion items from qqqys and the CI bot:
✅ disabledByArchive flag distinguishes archive-disabled from user-disabled tasks
✅ Concurrent rehydration with per-session 30s timeout
✅ handleRunNow now awaits record + reload before executing
✅ Debug logging for keepalive/rehydrate failures
✅ Integration tests for delete → removeTasksForSessions and unarchive → enableTasksForSessions
✅ Atomic DELETE (single updateCronTasks, no TOCTOU)
✅ Keepalive stopped during shutdown
✅ DEFAULT_BUILDER single source of truth
✅ Empty-string sessionId rejected in isValidTask
✅ POST /run raw timestamp justified (intentionally not minute-aligned)
Remaining issue: spawnOrAttach without sessionScope: 'thread' (wenshao's comment — confirmed valid)
I verified the bridge implementation. The daemon creates the bridge WITHOUT an explicit sessionScope in server.ts:372, defaulting to 'single' (bridgeOptions.ts → opts.sessionScope ?? 'single'). The ScheduledTasksSessionBridge interface narrows the call to { workspaceCwd: string } with no way to pass sessionScope.
Under 'single' scope:
- First task created → fresh session, set as
defaultEntry - Second task created → attaches to the first task's session (not a new one)
- If a user chat is already open → task binds to the user's chat session
This breaks the PR's core guarantee that "each task gets its own dedicated session." The fix is to widen ScheduledTasksSessionBridge.spawnOrAttach to accept sessionScope and pass 'thread' at the call site.
The other wenshao items (unarchive not reloading session, dual-daemon race, catch-up on PATCH) are documented limitations / edge-cases appropriate for a follow-up.
Verdict: The sessionScope gap is a blocking issue that undermines the feature's session-isolation contract. Everything else looks solid.
Second review round (QwenLM#6389): - Force `sessionScope: 'thread'` when minting a task's session. The daemon's default scope is 'single', which attaches to (reuses) the shared workspace session — so a second task, or a task alongside an open chat, would bind to the same session, rename it, land runs in the wrong transcript, and close it on delete. Thread scope guarantees each task an isolated session. [Critical] - Re-seat a recurring task's schedule anchor to now when a PATCH changes its cron (or flips one-shot→recurring), not just on re-enable. A bound task's catch-up runs on every file-watch reload, so a bare cron edit to an expression with an already-past slot would fire immediately on save. [Critical] - Revive a non-resident bound session from the keepalive when its heartbeat fails (reaper let it go while disabled/archived, now re-enabled). Covers the unarchive and PATCH false→true paths uniformly and retries each interval, so a re-enabled task actually resumes instead of showing a live countdown that never fires. Best-effort, timeout-bounded, non-blocking. [Critical] - Report `nextRunAt` using the scheduler's jittered fire time (`nextDurableFireMs`) instead of the bare cron boundary, so the UI countdown lines up with the real fire (the tick offsets each fire by up to the jitter window) rather than expiring early and advancing prematurely. All four are mutation-verified. The cross-daemon double-fire on bound tasks (same session live in two schedulers) is a separate, architecturally-invasive fix (claim-then-fire on the durable file) tracked as a follow-up.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
[Critical] withTimeout in rehydrateScheduledTaskSessions has zero test coverage (scheduled-task-keepalive.ts:131-134). The per-session 30s timeout is the sole mechanism preventing one hung loadSession from stalling the entire boot rehydration sweep. No test passes a custom loadTimeoutMs, seeds a loadSession that never resolves, or verifies that timed-out sessions appear in failed while siblings still load. If the timeout logic has a bug, the entire Promise.all hangs indefinitely and no bound task re-arms after a restart.
Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Review follow-up — statusAddressed all review comments across two rounds. 18 of 19 threads resolved; one is left open as a tracked follow-up (below). Round 1 (
Round 2 (
Every round-2 fix is mutation-verified (the new test fails when the fix is reverted). Affected core/cli/web-shell suites are green; typecheck + lint + prettier clean. Left open (tracked follow-up): cross-daemon / cross-scheduler double-fire of a bound task. It's real and reachable, but a correct fix needs a claim-then-fire reorder of the core tick (durable persist is currently fire-then-stamp) — too high-blast-radius to fold in here. Details in the open thread. 中文说明两轮评审意见已全部处理。19 条中 18 条已 resolve;剩 1 条作为待办跟进保留(见下)。 第一轮(
第二轮(
第二轮每处都做了变异验证(还原修复后对应测试即失败)。相关 core/cli/web-shell 测试全绿;typecheck + lint + prettier 均通过。 保留待跟进:绑定任务的跨守护进程/跨调度器重复触发。问题真实且可复现,但正确修复需要把核心 tick 改为"先认领再触发"(当前是先触发后落盘),改动面过大,不宜并入本 PR。细节见未关闭的那条线程。 |
|
@qwen-code /triage |
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
scheduled-tasks.ts:467 + scheduled-task-session-lifecycle.ts:92 |
PATCH enabled: false on an archive-disabled task preserves disabledByArchive: true. On unarchive, enableTasksForSessions re-enables it — overriding the user's explicit disable. |
Clear the flag when the user explicitly disables: if (patch.enabled === false && current.disabledByArchive) delete next.disabledByArchive; |
scheduled-tasks.ts:~607 + cronScheduler.ts:~704 |
POST /run on a bound one-shot updates lastFiredAt but doesn't delete the task. File-watch reload triggers catch-up (one-shot anchor = createdAt, ignoring lastFiredAt) → fires again and deletes the task, destroying the manual run record. |
Guard /run against one-shots, or delete the one-shot after recording the manual run. |
scheduled-tasks.ts:~505 |
PATCH recurring: false (recurring→one-shot) doesn't re-seat the anchor. The one-shot's createdAt is stale → catch-up fires it immediately and deletes it. |
Reset lastFiredAt (or createdAt) on recurring→one-shot transition. |
scheduled-task-keepalive.ts (revive path) |
Keepalive revive failures logged only via createDebugLogger('SCHED_KEEPALIVE').debug(...), gated behind QWEN_DEBUG_LOG_FILE. In production, a persistently failing revive is invisible — task stays enabled: true with a live countdown but never fires. |
Escalate to process.stderr.write after N consecutive failures for the same session. |
scheduled-task-keepalive.ts:117 |
setInterval tick has no in-flight guard. If tick() exceeds intervalMs (sequential 30s revives), overlapping ticks read the same file and attempt redundant work. |
Add if (ticking) return; guard. |
scheduled-task-keepalive.ts:94 |
bridge.recordHeartbeat(sessionId) is not awaited. The try/catch only catches sync throws. A future async bridge would silently bypass the catch. |
await the call, or tighten the interface to : void. |
scheduled-task-keepalive.ts:197 |
rehydrateScheduledTaskSessions loads all sessions via unbounded Promise.all. With 50 bound tasks, 20+ child processes spawn concurrently at boot. |
Add a simple concurrency limiter (batches of 5). |
scheduled-task-keepalive.ts:38,138 |
KeepaliveBridge and RehydrateBridge duplicate the identical loadSession signature. KeepaliveBridge is a strict superset. |
interface KeepaliveBridge extends RehydrateBridge. |
App.tsx:2470-2540 |
runTaskManually latch + pendingBoundRunRef useEffect — ~70 lines of async state-machine logic (supersede, timeout, cancellation) with zero test coverage. |
Add a test suite exercising supersede, session-load failure, and cancellation paths. |
cronTasksFile.ts:374 + scheduled-tasks.ts:332 |
isValidTask validation for disabledByArchive/sessionId and rollbackSession on write failure — load-bearing invariants with no direct tests. |
Add direct unit tests for each validation branch and the rollback path. |
— glm-5.2 via Qwen Code /review
Create names a task's session after the task (`⏰ <name>`), but a later PATCH that renamed the task (or edited the prompt of an unnamed task) left the session's display name stale. The PATCH route now re-applies `updateSessionMetadata` with the task's effective label whenever that label actually changes — a bare cron/enabled edit does not touch the session. Best-effort: a metadata failure doesn't fail the committed schedule change. Mutation-verified.
|
@doudouOUC thanks for the careful re-review — that review was against Blocking issue — resolved in Of the three items you flagged as follow-ups, two are also fixed in
Only the dual-daemon / same-session double-fire remains open, tracked as a follow-up (a claim-then-fire reorder of the core tick — too high-blast-radius to fold in here). Also added 中文那条 re-review 是针对 它列为"可后续处理"的三项里,有两项也已在 |
Code ReviewIndependent proposal (before reading the diff): The root cause — Web Shell sessions are lazily created but the cron scheduler needs an active session to tick — has two natural fixes: (a) a daemon-level "task runner" that creates ephemeral sessions on demand, or (b) a persistent resident session per management-page task. Option (a) is simpler but doesn't give each task its own transcript or lifecycle coupling. Option (b) is more tightly coupled to the existing architecture but gives richer semantics. Comparison with the PR's approach: The PR chose option (b) and executed it well. Key design decisions are sound:
No critical blockers found. The latest commits (
One known issue left open: cross-daemon double-fire for bound tasks. The author correctly identified that a proper fix requires a claim-then-fire reorder of the core tick — too high-blast-radius for this PR. Flagged as a tracked follow-up. Test ResultsAll affected test suites pass (run locally from PR branch
Typecheck: clean on core and cli (rebuilt core, then Real-Scenario TestingThis PR is a Web Shell daemon feature (scheduled tasks in No tmux before/after capture is applicable for this feature. 中文说明代码审查独立方案：根因是 Web Shell session 懒创建但 cron 调度器需要活跃 session。两种自然修复：(a) daemon 级任务执行器按需创建临时 session，(b) 每个管理页任务一个常驻 session。方案 (a) 更简单但没有独立 transcript 和生命周期耦合。方案 (b) 更紧密但语义更丰富。 与 PR 方案的比较：PR 选择方案 (b) 并执行良好。 未发现关键 blocker。 最新提交修复了水合死锁、"通过对话创建"和后续 review 意见。跨守护进程重复触发已标记为后续跟进。 测试结果本地运行 PR 分支 真实场景测试此 PR 是 Web Shell daemon 功能，主要行为需要完整的 daemon + web UI 栈，无法通过 CLI 复现。930 个自动化测试（包括每个耦合点的集成测试）是此处合适的验证层。此功能不适用 tmux before/after 捕获。 — Qwen Code · qwen3.7-max |
|
Stepping back and looking at this holistically: this is the third triage pass on a PR that has been through multiple review rounds, and each iteration has made it better. The motivation is genuine — management-page tasks never firing is a broken user experience that's directly observable. The approach (resident sessions per task) is the right fit: it leverages the session model rather than fighting it, and the lifecycle coupling is intuitive and well-covered. The implementation quality is high. 930 tests pass — up from 839 in the previous triage — including targeted integration tests for every coupling point. The latest commits addressed rehydrate deadlock, the "create via chat" edge case, and the follow-up review suggestions (onError try/catch, empty sessionId guard). The one open issue — cross-daemon double-fire — is explicitly flagged with a clear explanation of why the fix is too high-blast-radius for this PR. The scope is large (~4,600 additions across 32 files), but every change is motivated by the stated goal. The backend fix and the UI polish are both directly needed — splitting them would mean shipping a fix with no way to verify it, or a UI with no reliable backend. The author has been responsive across multiple review cycles, addressing 18 of 19 review threads and deferring the one that's too risky. The merge conflict was resolved cleanly. If I had to maintain this code, the test coverage and the comments that explain why would make it straightforward. LGTM. ✅ 中文说明整体来看：这是第三次审查，经过多轮 review 后每次迭代都在改善。 动机真实——管理页任务永远不触发是破损的用户体验。方案（每个任务常驻 session）与现有架构匹配。生命周期耦合直观且覆盖全面。 实现质量高。 930 个测试通过（较上次 839 个增加），包括每个耦合点的集成测试。最新提交修复了水合死锁、"通过对话创建"边缘场景和后续 review 建议。跨守护进程重复触发已明确标记为后续跟进。 范围较大但每项变更都服务于既定目标。作者在多轮 review 中积极响应。合并冲突干净解决。 LGTM. ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…essions # Conflicts: # packages/cli/src/serve/server.ts
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Review Summary
Well-architected PR — clean layering across core/CLI/web-shell, solid opt-in design for resident session management, and thorough test coverage (153 core + 46 CLI tests pass).
Main finding: DaemonScheduledTaskRun is missing the sessionId field that the server type (CronTaskRun) includes and sends on the wire — silent type drift. See inline comment.
Two other minor observations inline (app.locals cast, fire-and-forget rehydration). Neither is blocking.
Overall the architecture is sound, backward compatibility is clean, and the session lifecycle coupling (archive→disable, unarchive→enable, delete→remove) is well-thought-out.
…rver wiring Review follow-up (QwenLM#6389, qqqys): - [Medium] `DaemonScheduledTaskRun` now mirrors the daemon's `CronTaskRun` `sessionId?: string`, so run-attribution the wire already sends isn't silently dropped by the client type (not surfaced in the UI yet; passthrough cast means no mapping change needed). - [Nit] Comment the `app.locals.stopScheduledTaskKeepalive` set site, noting it follows the same convention as `fsFactory`/`boundWorkspace`/`acpHandle` and is read by the run-qwen-serve shutdown path (kept the convention rather than diverge to a one-off return value / declaration merge). - [Nit] Comment the outer `.catch(() => {})` on rehydrate as intentional defense-in-depth (the function already handles read + per-session failures).
…on enqueue Two [Critical] review items (QwenLM#6389, gpt-5-codex): - PATCH re-enable coupling: reject `enabled: true` on a task disabled BY archiving its session (`disabledByArchive`) with 409 `task_session_archived`. Re-enabling it here would show an enabled task with a countdown while its bound session stays archived and can never fire — the caller must unarchive the session (which clears the marker and reloads it). A user-disabled task (no marker) and non-enable edits are unaffected. - Manual "run now" ordering: record the run only AFTER the prompt is enqueued, not before. `runTaskManually` now returns a promise that resolves on enqueue and rejects if the bound session can't be opened (archived/deleted), is superseded, or times out; the dialog awaits it before writing /scheduled-tasks/:id/run, so a failed session switch no longer leaves a phantom run in history. Runs are serialized (one pending at a time, button disabled) so two quick clicks can't drop a prompt on the single bound-run latch. Added coverage for failed session load and double-click; all new tests mutation-verified.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Five items from GPT-5 /review (QwenLM#6389): - [Critical] Bind tasks to sessions only when resident management is on: createServeApp now passes the bridge to the scheduled-task routes only when `manageScheduledTaskSessions` is set. Embedders that leave it off get UNBOUND tasks (shared-owner firing) instead of bound tasks nothing keeps resident or reloads (which would silently go dormant). - [Critical] Keep the keepalive/revive loop running whenever task sessions are managed, not only when a reaper is active — archiving closes a task session, so a re-enabled one still needs reviving with the reaper disabled. Size the interval under the reaper window (≤ half of it) so a small idle timeout can't let a session be reaped before its first heartbeat. - [Critical] Record a manual run only after the prompt is admitted: the bound run latch now resolves only if `sendPrompt` admitted the prompt and rejects on cancellation (e.g. onSubmitBefore) / failure, so a cancelled Run now no longer advances lastFiredAt or appends history. - [Critical] Clamp the dialog's reload timer to the 32-bit setTimeout ceiling (~24.8 days) so a months-away schedule can't overflow and spin a reload loop. - [Suggestion] Pre-check the task cap before spawning a session, so an over-cap create never mints an orphan task session it must roll back. New tests (route unbound-when-no-bridge, cap-no-spawn, computeKeepaliveIntervalMs bounds, far-future timer clamp) mutation-verified; full server suite green.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Review — scheduled-task-per-session
Solid, well-tested PR. 6 inline findings below, most-severe first: two are concrete bugs (setTimeout overflow → reload loop; run-now hang), one is a firing-model race the PR newly exposes, and the rest are lifecycle-coupling gaps.
+1 (no inline anchor — untouched file): The standalone ACP agent's deleteSession in packages/cli/src/acp-integration/acpAgent.ts:7211 calls sessionService.removeSession(sessionId) directly, bypassing removeTasksForSessions. The PR couples task teardown only through the shared deleteDaemonSessions (REST /sessions/delete + daemon-ACP). This in-child qwen --experimental-acp surface (external editors like Zed on the same project) isn't covered, so deleting a bound task's session there orphans the task on disk. Worth routing through the shared path or documenting the gap.
中文
整体扎实、测试充分。下面 6 条 inline 按严重度排序:两条是确定 bug(setTimeout 溢出导致刷新死循环;立即运行卡死),一条是本 PR 新暴露的触发模型竞态,其余是生命周期耦合的缺口。
额外 1 条(未改动文件,无法 inline): 独立 ACP agent 的 deleteSession(packages/cli/src/acp-integration/acpAgent.ts:7211)直接调 sessionService.removeSession,绕过 removeTasksForSessions。PR 只把任务清理接到共享的 deleteDaemonSessions(REST + daemon-ACP),未覆盖 in-child 的 qwen --experimental-acp(外部编辑器如 Zed),在此删除绑定任务的 session 会使任务在磁盘上成为孤儿。建议接入共享路径或记录缺口。
…name The bridge's title guard (hasControlCharacter) only rejects C0/DEL, so Unicode bidi override/embedding/isolate controls (U+202A–202E, U+2066–2069) slip past it and can visually reorder a scheduled-task session name in the session list — a Trojan-Source-style attack (CVE-2021-42574). Strip them alongside the existing terminal-control-sequence pass, matching core's stripDisplayControlChars canonical set. Adds a test built from code points so the test file itself carries no reordering controls.
…ual-run recording, shared helpers, tests Addresses the review findings on the per-task-session work: - keepalive rehydrate no longer awaits a non-abortable loadSession after its timeout. A genuinely hung load would pin its worker and, with enough hangs, wedge the whole boot sweep (Promise.all never settles) so later task sessions never rehydrated. The worker now records the timeout as failed and pulls the next queued session; the background load is left to settle. Rewrote the test that pinned the old "hold the slot" behavior into a no-wedge regression guard. - web-shell manual run drops its pre-admission timeout. sendPrompt isn't abortable, so rejecting on the timer while the send was still in flight let a LATE admission execute an UNRECORDED run the user could retry into a duplicate. The run is now tied to admission (accepted prompts are always recorded); the "session never becomes active" phase stays bounded by the switch timeout in runTaskManually. - extract collectBoundSessionIds() shared by the heartbeat + rehydrate passes (was duplicated) and isBoundTask() in the lifecycle module (was the lone `sessionId !== undefined` check vs. the strict one used everywhere else). - spell the nextDurableFireMs cache-key separator as `\x00` rather than a literal NUL byte, so cronScheduler.ts no longer reads as binary to ripgrep. - add App.test coverage for the manual-run orchestration (admission-resolve, cancel/error reject, immediate fire, supersede, switch timeout) and a keepalive test that a disabled task gets no heartbeat and no revive.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Qwen Code Review Summary
The automated review for this PR failed to complete due to an API throttling error:
[API Error: {"code":"Throttling.BurstRate","message":"Request rate increased too quickly. To ensure system stability, please adjust your client logic to scale requests more smoothly over time.","request_id":"1437b1f8-21ca-97f7-975c-5dfb800eb58d"}]
No review findings or inline comments were produced in this run. Please retry the review later.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Qwen Code Review Summary
The automated review for this PR failed to complete due to an API throttling error:
[API Error: {"code":"Throttling.BurstRate","message":"Request rate increased too quickly. To ensure system stability, please adjust your client logic to scale requests more smoothly over time.","request_id":"1437b1f8-21ca-97f7-975c-5dfb800eb58d"}]
No review findings or inline comments were produced in this run. Please retry the review later.
| \nTest comment from review bot |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Qwen Code Review Summary
The automated review for this PR failed to complete due to an API throttling error:
[API Error: {"code":"Throttling.BurstRate","message":"Request rate increased too quickly. To ensure system stability, please adjust your client logic to scale requests more smoothly over time.","request_id":"1437b1f8-21ca-97f7-975c-5dfb800eb58d"}]
No review findings or inline comments were produced in this run. Please retry the review later.
|
@qwen-code /resolve |
Resolve conflict in App.test.tsx by combining both sides: - Keep new SessionOverviewPanel and SplitView mocks from main - Keep the capturing ScheduledTasksDialog mock from the PR branch (needed for scheduled-task-sessions onRunPrompt tests)
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings at this commit. Downgraded from Approve to Comment: self-PR; CI still running.
Incremental review (36de896 → 8cbe20a): 8 files, +326/-89. Build passes, all tests pass. DRY extraction (collectBoundSessionIds, isBoundTask), bidi Trojan-Source defense, cache-key collision fix, rehydration deadlock fix, and pre-admission timeout removal all verified correct. 4 low-confidence suggestions for human review (DRY consolidation, test gaps).
— qwen3.7-max via Qwen Code /review
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #6389Conflicted file
What conflictedThe conflict was in the component mock setup section of the test file. Both branches modified the area after the
How it was resolvedCombined both sides:
The resolved order is: Commit
|
…tasks The scheduled-tasks "Create via chat" button switched to the chat view but stayed on the CURRENT session, piling the task-creation conversation onto whatever the user was already doing. It now starts a new session first (createNewSession) and jumps to it before priming the composer, so task creation gets its own chat. Covered by a new App.test case asserting clearSession() is called.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
- keepalive rehydrate: guard the onError callback with try/catch. If it threw (e.g. stderr EPIPE during log rotation) the rejection escaped loadOne, failed its worker, and short-circuited Promise.all — stranding every other queued session. - cronScheduler catch-up: use the strict `typeof sessionId === 'string' && length > 0` bound-check instead of `!== undefined`, matching every other "is bound?" site. - server rehydration: log the outer defense-in-depth catch instead of swallowing it, so an unexpected throw isn't a silent "tasks never fire". - session-name sanitizer: also strip the standalone Bidi_Control marks U+061C / U+200E / U+200F, not just the override/isolate ranges. - scheduled-tasks dialog: when a consumed one-shot then fails to deliver, show a specific "deleted but never ran — recreate it" error instead of the generic "run failed" that hid the deletion. Kept the deliberate consume-first ordering.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
… start a new session onCreateViaChat's deferred composer-priming ran unconditionally: if createNewSession() failed, the task-starter text was dropped into the CURRENT session (only onSessionIdChange was gated on success). Gate all post-create side effects on `created`, matching handleMissingSessionNewSession. Adds an App.test failure-path case (new session fails → composer not primed).
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No new review findings at this commit beyond the existing open inline thread on onCreateViaChat.
Reviewed current head 50efce4. Verification passed locally:
npm ci(ran the build/bundle path during install)npm run typecheckpackages/core:npx vitest run src/services/cronScheduler.test.ts src/services/cronTasksFile.test.tspackages/cli:npx vitest run src/serve/routes/scheduled-tasks.test.ts src/serve/scheduled-task-keepalive.test.ts src/serve/scheduled-task-session-lifecycle.test.ts src/serve/server/session-archive.test.ts src/serve/server.test.tspackages/web-shell:npx vitest run client/components/dialogs/ScheduledTasksDialog.test.tsx client/components/dialogs/scheduledTasksSchedule.test.ts client/App.test.tsx
Downgraded from Approve to Comment: self-PR; CI failing: review-pr.
GPT-5 via Qwen Code /review
|
@qwen-code /triage |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
No new review findings at current head 04ebdc224.
I focused on the latest onCreateViaChat follow-up and the scheduled-task session paths. The latest fix correctly skips composer priming when createNewSession() returns false, and the new regression tests cover both success and failure paths.
Local verification:
packages/core:cronScheduler/cronTasksFilerelated tests passed, 156 tests total.packages/web-shell:ScheduledTasksDialog, schedule helper, andApprelated tests passed, 83 tests total.packages/cli: scheduled-task routes, keepalive, session lifecycle, and session archive suites passed;server.test.tshas one local-only/capabilitiesexpectation affected by this machine’s voice-transcription settings, not by this scheduled-task diff.
Existing open cross-daemon double-fire follow-up remains as already discussed.
Reviewed with GPT-5 via Qwen Code.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Final Review — LGTM
全面审查了 22 个 commit (4613+/120-, 32 files) 的最终状态。所有 Critical/High review items 已修复确认。
核心架构验证 ✅
| 关键点 | 状态 |
|---|---|
sessionScope: 'thread' 隔离任务 session |
cd6b6b1 修复，route 传递确认 |
#shouldFireDurable 路由 bound/unbound |
逻辑正确，bound task 只在自己 session 触发 |
disabledByArchive guard (PATCH 409) |
防止误 re-enable archived task |
firePersistPending ref-counted |
解决并发 persist 提前 clear 问题 |
lastFiredAt non-regression (>=) |
tick + catch-up 双路径一致 |
| Rehydration bounded concurrency (4 workers, 30s timeout) | 正确实现 worker pool |
onError try/catch in loadOne |
防止 error sink throw 短路 sweep |
Outer .catch() logs to stderr |
不再 silent swallow |
handleRunNow server-authoritative re-check |
refresh → bail if gone/disabled → use fresh data |
| One-shot consume-first semantics | 防双火，trade-off 明确文档化 |
onCreateViaChat gated on created |
04ebdc2 修复，失败时不污染当前 session |
| Bidi char strip 完整集 | 50efce4 扩展到 full Bidi_Control |
测试覆盖
126 个新 test cases，覆盖：create/edit/delete/run 路径、lifecycle coupling、keepalive tick/rehydrate、catch-up non-regression、rollback、disabled/archive 交互、UI 组件行为。
已确认的 follow-up 项（不阻塞合并）
- Cross-daemon double-fire (需要 claim-then-fire on durable file)
- Task session 意外获得 project lock 时 unbound tasks 触发位置
tick()dead-session 无 backoff 重试
CI: Ubuntu tests pass (26m42s).
5 participants
What this PR does
Scheduled tasks created through the Web Shell management page now run in a dedicated session per task. Each management-page task mints its own session at create time (named "⏰ <task>"), fires only inside that session — so the session's transcript is the task's run history — and the session's lifecycle drives the task: archiving, deleting, or unarchiving the session disables, removes, or re-enables the bound task (covered on both the REST and ACP surfaces, since the Web Shell archives/deletes over ACP). A daemon-side keepalive heartbeats these sessions so the idle reaper doesn't stop them, and a boot-time rehydration reloads them after a restart. All resident-session management is opt-in and enabled only by the real
runQwenServedaemon, so directcreateServeAppembeds/tests are unaffected.The management page also gains task editing (the cron reverses back onto the structured pickers, falling back to a raw-cron field for anything it can't represent), a live next-run countdown, run history, a one-per-row card layout, and a "run now" that executes in the task's bound session and updates the last-run time.
Why it's needed
A task created on the "Scheduled Tasks" page never fired in the daemon-only case: durable cron ticks inside an active agent session, but the Web Shell creates a session only lazily on the first prompt, so a task created on the management page with no chat open had nothing ticking it — it sat at "Never run" forever. Binding each task to its own resident session makes it run reliably while the daemon is up (and again after a restart), keeps every run isolated in that task's transcript, and lets the session double as the task's run history.
Reviewer Test Plan
How to verify
Build the Web Shell and run the daemon:
npm run build --workspace @qwen-code/web-shellthenqwen serve --web, open the Web Shell, and go to "Scheduled Tasks".Automated coverage: unit + integration across
core(per-session firing, run records),cli(route, keepalive, rehydration, session-archive coupling), andweb-shell(dialog, schedule helpers, App). Also verified end-to-end against a realqwen servedaemon: create → the task fires in its own bound session with the run recorded under that session id → archive disables it → unarchive re-enables → delete removes it.Evidence (Before & After)
Before: a task created on the management page with no chat open stayed at "尚未运行 / Never run" — nothing was ticking it.
After — the scheduled-tasks page: one task per row, human-readable schedule, a live next-run countdown (⏳), the last-run time, "view history" into the task's session, per-task run / edit / delete, and a disabled task rendered dimmed.
Editing a task — the stored cron (
*/10 * * * *) is reversed back onto the structured pickers ("every N minutes" / 10), never silently rewritten:Tested on
Environment (optional)
Local:
qwen serve --webfrom source (tsx), a real daemon E2E driver over curl, and a real Chrome screenshot pass. Unit/integration via vitest.Risk & Scope
runQwenServedaemon.sessionId, richerruns) are optional and backward-compatible; tool-created (cron_create) and legacy tasks keep the shared per-project durable-owner firing model. No migration needed.Linked Issues
N/A — no tracking issue; this addresses the "task created on the management page never runs" behavior directly.
中文说明
做了什么：通过 Web Shell 管理页创建的定时任务,现在每个任务归属一个专属 session(建任务时铸出,命名
⏰ 任务名),只在自己的 session 里按点触发——该 session 的对话记录就是任务的运行历史;归档 / 删除 / 恢复该 session 会联动停用 / 删除 / 重新启用任务(REST 和 ACP 两条路都覆盖,因为 Web Shell 走 ACP)。daemon 侧用 heartbeat 保活这些 session 躲过 idle 回收,并在重启时自动重拉。整套常驻管理是 opt-in,只有真实runQwenServe才开,不影响createServeApp的嵌入/测试。管理页另加了:编辑(cron 反解回结构化选择器)、下次运行倒计时、运行记录、一行一个任务的布局,以及"立即运行"(在绑定 session 里跑并更新上次运行时间)。为什么:管理页建的任务在"没开对话"时一直不触发——durable cron 的 tick 跑在活跃会话里,而 Web Shell 的会话是懒创建的,于是管理页建完任务却永远"尚未运行"。给每个任务绑一个常驻 session 让它可靠运行,每次运行隔离在该任务的 transcript 里,会话本身即历史。