From c16b4ea8d2e9b503a30fb7ce9f22eb483c85d9f5 Mon Sep 17 00:00:00 2001 From: yiliang114 <1204183885@qq.com> Date: Mon, 17 Aug 2026 14:20:15 +0000 Subject: [PATCH 01/14] feat(scheduled-tasks): allow creating a task with an existing session POST /scheduled-tasks and the workspace-qualified endpoint now accept an optional `sessionId`. When provided, the task binds to that existing session instead of minting a dedicated one. The session is validated up front: it must be live in the target workspace, idle, not archived, and not already bound to another scheduled task (checked both in a best-effort pre-read and authoritatively under the cron write lock). A failed create never tears down a caller-provided session (only route-minted sessions roll back); after a successful create the session follows the regular scheduled-task session lifecycle. Omitting `sessionId` keeps the dedicated-session behavior unchanged. Closes #8906 --- .../src/serve/routes/scheduled-tasks.test.ts | 220 ++++++++++++++ .../cli/src/serve/routes/scheduled-tasks.ts | 284 ++++++++++++++---- packages/webui/src/daemon/workspace/types.ts | 6 + 3 files changed, 458 insertions(+), 52 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index 05bf623ff12..2fab1b44836 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -16,7 +16,9 @@ import { Storage, getCronFilePath, readCronTasks, + updateCronTasks, } from '@qwen-code/qwen-code-core'; +import { SessionNotFoundError } from '@qwen-code/acp-bridge/bridgeErrors'; import { registerScheduledTasksRoutes, registerWorkspaceQualifiedScheduledTasksRoutes, @@ -49,6 +51,22 @@ interface StubBridge { sessionId: string, metadata: { displayName?: string }, ): unknown; + getSessionSummary(sessionId: string): { + sessionId: string; + workspaceCwd: string; + hasActivePrompt: boolean; + isArchived?: boolean; + }; + /** The sessions the stub reports as live (for getSessionSummary). */ + liveSessions: Map< + string, + { + sessionId: string; + workspaceCwd: string; + hasActivePrompt: boolean; + isArchived?: boolean; + } + >; spawned: string[]; spawnScopes: Array<'single' | 'thread' | undefined>; spawnSources: Array<{ sourceType?: string; sourceId?: string }>; @@ -66,6 +84,7 @@ function makeStubBridge(): StubBridge { closed: [], named: [], failNext: false, + liveSessions: new Map(), async spawnOrAttach(req) { if (bridge.failNext) { bridge.failNext = false; @@ -88,6 +107,11 @@ function makeStubBridge(): StubBridge { bridge.named.push({ sessionId, ...metadata }); return metadata; }, + getSessionSummary(sessionId) { + const summary = bridge.liveSessions.get(sessionId); + if (!summary) throw new SessionNotFoundError(sessionId); + return summary; + }, }; return bridge; } @@ -598,6 +622,175 @@ describe('scheduled-tasks routes', () => { expect(h.bridge.spawned).toEqual([]); // nothing was spawned }); + // ── Caller-provided sessionId (reuse an existing session) ──────────── + + it('reuses a caller-provided session instead of minting one', async () => { + h.bridge.liveSessions.set('caller-sess', { + sessionId: 'caller-sess', + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + const res = await create({ + name: 'Digest', + cron: '0 9 * * *', + prompt: 'p', + sessionId: 'caller-sess', + }); + expect(res.status).toBe(201); + expect(res.body.sessionId).toBe('caller-sess'); + // No dedicated session was minted for the task. + expect(h.bridge.spawned).toEqual([]); + // The reused session is named after the task like a minted one. + expect(h.bridge.named).toEqual([ + { sessionId: 'caller-sess', displayName: '⏰ Digest' }, + ]); + const tasks = await readCronTasks(h.workspace); + expect(tasks).toHaveLength(1); + expect(tasks[0]?.sessionId).toBe('caller-sess'); + }); + + it('rejects an unknown sessionId with 404 and creates nothing', async () => { + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: 'missing-sess', + }); + expect(res.status).toBe(404); + expect(res.body.code).toBe('session_not_found'); + expect(h.bridge.spawned).toEqual([]); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('rejects a session that belongs to a different workspace', async () => { + h.bridge.liveSessions.set('other-sess', { + sessionId: 'other-sess', + workspaceCwd: path.join(h.scratch, 'some-other-workspace'), + hasActivePrompt: false, + }); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: 'other-sess', + }); + expect(res.status).toBe(400); + expect(res.body.code).toBe('session_workspace_mismatch'); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('rejects a busy session with 409 session_busy', async () => { + h.bridge.liveSessions.set('busy-sess', { + sessionId: 'busy-sess', + workspaceCwd: h.workspace, + hasActivePrompt: true, + }); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: 'busy-sess', + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_busy'); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('rejects an archived session with 409 session_archived', async () => { + h.bridge.liveSessions.set('arch-sess', { + sessionId: 'arch-sess', + workspaceCwd: h.workspace, + hasActivePrompt: false, + isArchived: true, + }); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: 'arch-sess', + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_archived'); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('rejects a session already bound to another task', async () => { + h.bridge.liveSessions.set('caller-sess', { + sessionId: 'caller-sess', + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + await updateCronTasks(h.workspace, (tasks) => [ + ...tasks, + { + id: 'existing-task', + cron: '0 9 * * *', + prompt: 'existing', + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: 1_700_000_000_000, + enabled: true, + sessionId: 'caller-sess', + }, + ]); + const res = await create({ + cron: '0 10 * * *', + prompt: 'p', + sessionId: 'caller-sess', + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_already_bound'); + expect(await readCronTasks(h.workspace)).toHaveLength(1); // unchanged + }); + + it('rejects an invalid sessionId field with 400 invalid_session_id', async () => { + for (const bad of [123, true, '', ' ']) { + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: bad, + }); + expect(res.status).toBe(400); + expect(res.body.code).toBe('invalid_session_id'); + } + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('rejects sessionId when session management is unavailable (no bridge)', async () => { + const app = express(); + app.use(express.json()); + registerScheduledTasksRoutes(app, { + boundWorkspace: h.workspace, + mutate: () => (_req, _res, next) => next(), + safeBody, + // no bridge — unbound creates stay available, binding fails closed + }); + const res = await request(app) + .post('/scheduled-tasks') + .send({ cron: '0 9 * * *', prompt: 'p', sessionId: 'caller-sess' }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_binding_unavailable'); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('leaves the caller-provided session open when the commit fails', async () => { + h.bridge.liveSessions.set('caller-sess', { + sessionId: 'caller-sess', + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + // Corrupt the tasks file: the pre-check read fails (skipped), the + // authoritative write throws → 500. The caller's session must survive. + const file = getCronFilePath(h.workspace); + await fsp.mkdir(path.dirname(file), { recursive: true }); + await fsp.writeFile(file, 'CORRUPT {{{', 'utf8'); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: 'caller-sess', + }); + expect(res.status).toBe(500); + expect(res.body.code).toBe('scheduled_tasks_write_failed'); + expect(h.bridge.closed).toEqual([]); // caller session left open + expect(h.cleanupSession).not.toHaveBeenCalled(); + }); + it('mints the task session with thread scope (never reuses the shared session)', async () => { // The daemon default scope is 'single' (attach to the shared workspace // session). A task MUST get its own isolated session, so the route forces @@ -1997,6 +2190,33 @@ describe('workspace-qualified scheduled-tasks routes', () => { expect(primaryList.body.tasks).toHaveLength(0); }); + it('reuses a caller-provided session on the qualified surface and rejects cross-workspace ones', async () => { + h.secondary.bridge.liveSessions.set('sec-sess', { + sessionId: 'sec-sess', + workspaceCwd: h.secondary.workspaceCwd, + hasActivePrompt: false, + }); + const res = await request(h.app) + .post(qualified(h.secondary.workspaceId)) + .send({ cron: '0 9 * * *', prompt: 'p', sessionId: 'sec-sess' }); + expect(res.status).toBe(201); + expect(res.body.sessionId).toBe('sec-sess'); + expect(h.secondary.bridge.spawned).toEqual([]); + + // A session living in the PRIMARY workspace can't be bound through the + // secondary workspace's endpoint. + h.secondary.bridge.liveSessions.set('primary-sess', { + sessionId: 'primary-sess', + workspaceCwd: h.primary.workspaceCwd, + hasActivePrompt: false, + }); + const bad = await request(h.app) + .post(qualified(h.secondary.workspaceId)) + .send({ cron: '0 9 * * *', prompt: 'q', sessionId: 'primary-sess' }); + expect(bad.status).toBe(400); + expect(bad.body.code).toBe('session_workspace_mismatch'); + }); + it('writes to the targeted workspace’s own cron file on disk', async () => { await request(h.app) .post(qualified(h.secondary.workspaceId)) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index 2e98a558891..0c65ef8ad69 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -49,6 +49,8 @@ import { type DurableCronTask, type CronTaskRun, } from '@qwen-code/qwen-code-core'; +import { SessionNotFoundError } from '@qwen-code/acp-bridge/bridgeErrors'; +import { canonicalizeWorkspace } from '@qwen-code/acp-bridge/workspacePaths'; import { writeStderrLine } from '../../utils/stdioHelpers.js'; import { isChannelDeliveryError } from '../../runtime/channel-delivery-ipc.js'; import { @@ -96,6 +98,15 @@ export interface ScheduledTasksSessionBridge { sessionId: string, metadata: { displayName?: string }, ): unknown; + /** Live summary for one session by id. Throws `SessionNotFoundError` when + * no live session with that id exists on this daemon. Used to validate a + * caller-provided session (workspace, idle, archived) before binding it to + * a new task. */ + getSessionSummary(sessionId: string): { + workspaceCwd: string; + hasActivePrompt: boolean; + isArchived?: boolean; + }; } // Cap for the derived session display name — a session label, not the full @@ -519,6 +530,14 @@ function registerScheduledTaskCrudRoutes( }); return; } + const sessionIdResult = parseSessionIdField(body['sessionId']); + if (sessionIdResult.error) { + res + .status(400) + .json({ error: sessionIdResult.error, code: 'invalid_session_id' }); + return; + } + const providedSessionId = sessionIdResult.value; let delivery: PublicChannelDelivery | undefined; if (body['delivery'] !== undefined) { try { @@ -538,59 +557,145 @@ function registerScheduledTaskCrudRoutes( const enabled = body['enabled'] !== false; const taskId = generateCronTaskId(); - // Mint the task's dedicated session up front. The task is BOUND to it and - // fires only inside it — its transcript becomes the task's run history, and - // archiving/deleting the session stops the task. Done before the write so a - // task never lands on disk without its session; if the bridge is absent - // (minimal embedding) the task is created unbound (shared-owner firing). + // Bind the task's session up front. The task is BOUND to it and fires + // only inside it — its transcript becomes the task's run history, and + // archiving/deleting the session stops the task. Done before the write + // so a task never lands on disk without its session; if the bridge is + // absent (minimal embedding) the task is created unbound (shared-owner + // firing). // - // `sessionScope: 'thread'` is REQUIRED: the daemon's default scope is - // 'single', which would attach to (and reuse) the shared workspace session - // instead of minting a fresh one. Two tasks — or a task and an open chat — - // would then bind to the same session: the task renames it, scheduled runs - // land in the wrong transcript, and deleting one task closes the shared - // session. Forcing 'thread' guarantees each task gets an isolated session. + // Two binding modes: + // - no `sessionId` in the body: mint a DEDICATED session (the original + // behavior), torn back down if the create can't be committed; + // - `sessionId` provided: REUSE that existing session after validating + // it (live in this workspace, idle, not archived, not already bound + // to another task). It pre-existed the task, so a failed create must + // leave it open; after a successful create it follows the regular + // scheduled-task session lifecycle. + // + // `sessionScope: 'thread'` is REQUIRED for the mint path: the daemon's + // default scope is 'single', which would attach to (and reuse) the + // shared workspace session instead of minting a fresh one. Two tasks — + // or a task and an open chat — would then bind to the same session: the + // task renames it, scheduled runs land in the wrong transcript, and + // deleting one task closes the shared session. Forcing 'thread' + // guarantees each minted task session is isolated. let boundSessionId: string | undefined; + // True only when THIS route minted the bound session (and must tear it + // back down if the create fails). False for a caller-provided session. + let sessionMintedHere = false; + if (providedSessionId !== undefined && !bridge) { + // Fail closed: silently creating an UNBOUND task would give the caller + // a materially different task from the one it asked for. + res.status(409).json({ + error: + 'Session management is not available for this workspace; omit `sessionId` to create an unbound task', + code: 'session_binding_unavailable', + }); + return; + } if (bridge) { - // Pre-check the cap BEFORE spawning: an over-cap create must not spawn a - // session it will immediately tear down, because closeSession removes the - // live bridge entry but can leave the just-spawned+named session listed as - // an orphan with no owning task. Best-effort — the write-lock cap check - // below stays authoritative for the concurrent-create race. + if (providedSessionId !== undefined) { + // Validate the caller's session BEFORE any write. + let summary: { + workspaceCwd: string; + hasActivePrompt: boolean; + isArchived?: boolean; + }; + try { + summary = await runWithScheduledTaskTarget(target, () => + bridge.getSessionSummary(providedSessionId), + ); + } catch (err) { + if (err instanceof SessionNotFoundError) { + res.status(404).json({ + error: `Session '${providedSessionId}' was not found`, + code: 'session_not_found', + }); + return; + } + writeStderrLine( + `qwen serve: POST ${base} failed to look up session '${providedSessionId}': ${err instanceof Error ? err.message : String(err)}`, + ); + res.status(500).json({ + error: 'Failed to look up the requested session', + code: 'scheduled_tasks_session_failed', + }); + return; + } + let sameWorkspace = false; + try { + sameWorkspace = + canonicalizeWorkspace(summary.workspaceCwd) === + canonicalizeWorkspace(workspaceCwd); + } catch { + sameWorkspace = false; + } + if (!sameWorkspace) { + res.status(400).json({ + error: + "The requested session belongs to a different workspace; use that workspace's scheduled-task endpoint", + code: 'session_workspace_mismatch', + }); + return; + } + if (summary.isArchived === true) { + res.status(409).json({ + error: + 'The requested session is archived; unarchive it before binding it to a task', + code: 'session_archived', + }); + return; + } + if (summary.hasActivePrompt) { + res.status(409).json({ + error: + 'The requested session is busy; wait for its active prompt to finish before binding it to a task', + code: 'session_busy', + }); + return; + } + } + // Pre-check the cap (and duplicate binding, for a caller-provided + // session) BEFORE spawning: an over-cap create must not spawn a + // session it will immediately tear down, because closeSession removes + // the live bridge entry but can leave the just-spawned+named session + // listed as an orphan with no owning task. Best-effort — the + // write-lock checks below stay authoritative for the concurrent race. try { - if ( - ( - await runWithScheduledTaskTarget(target, () => - readCronTasks(workspaceCwd), - ) - ).length >= MAX_SCHEDULED_TASKS - ) { + const existingTasks = await runWithScheduledTaskTarget(target, () => + readCronTasks(workspaceCwd), + ); + if (existingTasks.length >= MAX_SCHEDULED_TASKS) { res.status(409).json({ error: `Maximum number of scheduled tasks (${MAX_SCHEDULED_TASKS}) reached`, code: 'max_tasks_reached', }); return; } + if ( + providedSessionId !== undefined && + existingTasks.some((t) => t.sessionId === providedSessionId) + ) { + res.status(409).json({ + error: + 'The requested session is already bound to another scheduled task', + code: 'session_already_bound', + }); + return; + } } catch { // Read failure → skip the pre-check; the write below is authoritative. } if (!requireOpenGeneration(target, res)) return; - try { - const session = await runWithScheduledTaskTarget(target, () => - bridge.spawnOrAttach({ - workspaceCwd, - sessionScope: 'thread', - sourceType: 'scheduled_task', - sourceId: taskId, - }), - ); - boundSessionId = session.sessionId; - if (!requireOpenGeneration(target, res)) { - await teardownBoundSession(target, boundSessionId); - return; - } - // Name the session after the task so it's recognizable in the session - // list. Best-effort — a nameless session still fires correctly. + if (providedSessionId !== undefined) { + // Reuse the caller's session — no spawn (sessionMintedHere stays + // false, so a failed create leaves it open). + boundSessionId = providedSessionId; + // Name it after the task like a minted session — the scheduled-task + // session lifecycle (including the keepalive's ⏰ naming) applies + // from here on. Best-effort — a rename failure must not fail the + // create. try { await runWithScheduledTaskTarget(target, async () => bridge.updateSessionMetadata(boundSessionId!, { @@ -602,17 +707,47 @@ function registerScheduledTaskCrudRoutes( } catch { // metadata update is non-critical } - } catch (err) { - if (sendActivityGateError(res, err)) return; - if (sendGenerationClosedError(res, err)) return; - writeStderrLine( - `qwen serve: POST ${base} failed to create the task's session: ${err instanceof Error ? err.message : String(err)}`, - ); - res.status(500).json({ - error: "Failed to create the task's session", - code: 'scheduled_tasks_session_failed', - }); - return; + } else { + try { + const session = await runWithScheduledTaskTarget(target, () => + bridge.spawnOrAttach({ + workspaceCwd, + sessionScope: 'thread', + sourceType: 'scheduled_task', + sourceId: taskId, + }), + ); + boundSessionId = session.sessionId; + sessionMintedHere = true; + if (!requireOpenGeneration(target, res)) { + await teardownBoundSession(target, boundSessionId); + return; + } + // Name the session after the task so it's recognizable in the session + // list. Best-effort — a nameless session still fires correctly. + try { + await runWithScheduledTaskTarget(target, async () => + bridge.updateSessionMetadata(boundSessionId!, { + displayName: scheduledTaskSessionName( + nameResult.value ?? prompt, + ), + }), + ); + } catch { + // metadata update is non-critical + } + } catch (err) { + if (sendActivityGateError(res, err)) return; + if (sendGenerationClosedError(res, err)) return; + writeStderrLine( + `qwen serve: POST ${base} failed to create the task's session: ${err instanceof Error ? err.message : String(err)}`, + ); + res.status(500).json({ + error: "Failed to create the task's session", + code: 'scheduled_tasks_session_failed', + }); + return; + } } } @@ -637,14 +772,17 @@ function registerScheduledTaskCrudRoutes( // deletes the persisted transcript/title record — both are needed, or a // rejected create (the loser of a concurrent create at the cap boundary, // which passes the pre-check but loses the authoritative write) would leave - // a named "⏰ …" session in the list with no owning task. + // a named "⏰ …" session in the list with no owning task. A caller-provided + // session is NEVER torn down here — it pre-existed the task and must stay + // open when the create fails. const rollbackSession = async () => { - if (boundSessionId !== undefined) { + if (boundSessionId !== undefined && sessionMintedHere) { await teardownBoundSession(target, boundSessionId); } }; let overCap = false; + let alreadyBound = false; let rollbackBefore: DurableCronTask[] | undefined; let rollbackAfter: DurableCronTask[] | undefined; try { @@ -659,6 +797,16 @@ function registerScheduledTaskCrudRoutes( overCap = true; return tasks; } + // Same-lock duplicate-binding check for a caller-provided + // session: the pre-check read above is best-effort, and a + // concurrent create may have bound the same session since. + if ( + providedSessionId !== undefined && + tasks.some((t) => t.sessionId === providedSessionId) + ) { + alreadyBound = true; + return tasks; + } rollbackBefore = tasks; rollbackAfter = [...tasks, task]; return rollbackAfter; @@ -702,6 +850,15 @@ function registerScheduledTaskCrudRoutes( }); return; } + if (alreadyBound) { + await rollbackSession(); + res.status(409).json({ + error: + 'The requested session is already bound to another scheduled task', + code: 'session_already_bound', + }); + return; + } if (task.delivery && task.sessionId) { channelDeliveryAuthorizations?.registerScheduledTask(workspaceCwd, { sessionId: task.sessionId, @@ -1432,3 +1589,26 @@ function parseNameField(raw: unknown): { value?: string; error?: string } { } return { value: trimmed }; } + +/** + * Parses the optional `sessionId` field on POST (reuse an existing session + * instead of minting a dedicated one). Accepts: + * - absent / null → `{ value: undefined }` (mint a dedicated session) + * - a non-empty string → `{ value: trimmed }` + * - anything else (including empty/whitespace-only strings — a session id + * can't be "cleared", so unlike `name` they're an error) → `{ error }` + */ +function parseSessionIdField(raw: unknown): { + value?: string; + error?: string; +} { + if (raw === undefined || raw === null) return { value: undefined }; + if (typeof raw !== 'string') { + return { error: '`sessionId` must be a string' }; + } + const trimmed = raw.trim(); + if (trimmed.length === 0) { + return { error: '`sessionId` must be a non-empty string' }; + } + return { value: trimmed }; +} diff --git a/packages/webui/src/daemon/workspace/types.ts b/packages/webui/src/daemon/workspace/types.ts index d487de9bb74..35d0736880e 100644 --- a/packages/webui/src/daemon/workspace/types.ts +++ b/packages/webui/src/daemon/workspace/types.ts @@ -272,6 +272,12 @@ export interface DaemonCreateScheduledTaskRequest { recurring?: boolean; /** Defaults to true. */ enabled?: boolean; + /** Reuse an existing live session instead of minting a dedicated one. The + * session must be live in this workspace, idle, not archived, and not + * already bound to another scheduled task; after a successful create it + * follows the regular scheduled-task session lifecycle. Omit (or null) to + * keep the dedicated-session behavior. */ + sessionId?: string | null; } /** Partial update. `name: null` (or '') clears the name. Omitted fields are From 4d0ea79279f4f1c04f4b66f37bc8cf30a43e90e8 Mon Sep 17 00:00:00 2001 From: yiliang114 <1204183885@qq.com> Date: Mon, 17 Aug 2026 22:57:41 +0000 Subject: [PATCH 02/14] fix(serve): harden scheduled-task session binding per bot review round 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four fixes inside the route, each pinned by a test: - Move the caller-session ⏰ rename to after the cron write commits, so a failed create (over-cap/duplicate 409, write 500, generation rollback) never leaves the caller's pre-existing session permanently renamed with no owning task (nothing restores the prior display name). - On SessionNotFoundError, consult SessionService.getSessionLocation so an archived session — removed from the live map by archiving — still gets the documented 409 session_archived instead of a bare 404. - canonicalizeWorkspace re-throws non-ENOENT filesystem errors (EACCES/EIO/ ELOOP/ESTALE); surface those as a retryable 500 scheduled_tasks_session_failed with a stderr log instead of a misleading 400 session_workspace_mismatch. - Parse sessionId with parseCallerSuppliedSessionId, the parser every other caller-supplied-session-id surface uses: UUID grammar, case-normalized, length-bounded (no unbounded echo in error bodies/stderr), and duplicate-binding equality per session rather than per spelling. New tests: disk-backed archived fallback (runtime harness), ELOOP 500, generic lookup-failure 500 with no side effects, over-cap rejection on the reuse path, concurrent-create single-bind invariant (updateCronTasks serializes writers; deleting the under-write-lock check flips the second response to 201), null→mint, and padded/mixed-case normalization. Stub session ids migrate to valid UUIDs to match the shared grammar. --- .../src/serve/routes/scheduled-tasks.test.ts | 258 +++++++++++++++--- .../cli/src/serve/routes/scheduled-tasks.ts | 99 +++++-- 2 files changed, 298 insertions(+), 59 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index eaa95866a12..2a3d2a7688a 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -37,6 +37,18 @@ function safeBody(req: Request): Record { : {}; } +// Caller-supplied session ids must match the UUID grammar enforced by +// parseCallerSuppliedSessionId (shared with every other caller-id surface), +// so the stub's live sessions are keyed by valid UUIDs rather than freeform +// labels. +const CALLER_SESSION_ID = '10000000-0000-4000-8000-000000000001'; +const MISSING_SESSION_ID = '10000000-0000-4000-8000-000000000002'; +const OTHER_SESSION_ID = '10000000-0000-4000-8000-000000000003'; +const BUSY_SESSION_ID = '10000000-0000-4000-8000-000000000004'; +const ARCHIVED_SESSION_ID = '10000000-0000-4000-8000-000000000005'; +const SECONDARY_SESSION_ID = '10000000-0000-4000-8000-000000000006'; +const PRIMARY_SESSION_ID = '10000000-0000-4000-8000-000000000007'; + /** Stub session bridge: mints sequential fake session ids and records spawns / * closes so tests can assert binding and rollback without a real child. */ interface StubBridge { @@ -627,8 +639,8 @@ describe('scheduled-tasks routes', () => { // ── Caller-provided sessionId (reuse an existing session) ──────────── it('reuses a caller-provided session instead of minting one', async () => { - h.bridge.liveSessions.set('caller-sess', { - sessionId: 'caller-sess', + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, workspaceCwd: h.workspace, hasActivePrompt: false, }); @@ -636,26 +648,26 @@ describe('scheduled-tasks routes', () => { name: 'Digest', cron: '0 9 * * *', prompt: 'p', - sessionId: 'caller-sess', + sessionId: CALLER_SESSION_ID, }); expect(res.status).toBe(201); - expect(res.body.sessionId).toBe('caller-sess'); + expect(res.body.sessionId).toBe(CALLER_SESSION_ID); // No dedicated session was minted for the task. expect(h.bridge.spawned).toEqual([]); // The reused session is named after the task like a minted one. expect(h.bridge.named).toEqual([ - { sessionId: 'caller-sess', displayName: '⏰ Digest' }, + { sessionId: CALLER_SESSION_ID, displayName: '⏰ Digest' }, ]); const tasks = await readCronTasks(h.workspace); expect(tasks).toHaveLength(1); - expect(tasks[0]?.sessionId).toBe('caller-sess'); + expect(tasks[0]?.sessionId).toBe(CALLER_SESSION_ID); }); it('rejects an unknown sessionId with 404 and creates nothing', async () => { const res = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: 'missing-sess', + sessionId: MISSING_SESSION_ID, }); expect(res.status).toBe(404); expect(res.body.code).toBe('session_not_found'); @@ -664,15 +676,15 @@ describe('scheduled-tasks routes', () => { }); it('rejects a session that belongs to a different workspace', async () => { - h.bridge.liveSessions.set('other-sess', { - sessionId: 'other-sess', + h.bridge.liveSessions.set(OTHER_SESSION_ID, { + sessionId: OTHER_SESSION_ID, workspaceCwd: path.join(h.scratch, 'some-other-workspace'), hasActivePrompt: false, }); const res = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: 'other-sess', + sessionId: OTHER_SESSION_ID, }); expect(res.status).toBe(400); expect(res.body.code).toBe('session_workspace_mismatch'); @@ -680,15 +692,15 @@ describe('scheduled-tasks routes', () => { }); it('rejects a busy session with 409 session_busy', async () => { - h.bridge.liveSessions.set('busy-sess', { - sessionId: 'busy-sess', + h.bridge.liveSessions.set(BUSY_SESSION_ID, { + sessionId: BUSY_SESSION_ID, workspaceCwd: h.workspace, hasActivePrompt: true, }); const res = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: 'busy-sess', + sessionId: BUSY_SESSION_ID, }); expect(res.status).toBe(409); expect(res.body.code).toBe('session_busy'); @@ -696,8 +708,8 @@ describe('scheduled-tasks routes', () => { }); it('rejects an archived session with 409 session_archived', async () => { - h.bridge.liveSessions.set('arch-sess', { - sessionId: 'arch-sess', + h.bridge.liveSessions.set(ARCHIVED_SESSION_ID, { + sessionId: ARCHIVED_SESSION_ID, workspaceCwd: h.workspace, hasActivePrompt: false, isArchived: true, @@ -705,7 +717,38 @@ describe('scheduled-tasks routes', () => { const res = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: 'arch-sess', + sessionId: ARCHIVED_SESSION_ID, + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_archived'); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('rejects an archived session the bridge only reports as not-found', async () => { + // Production archiving removes the session from the live map first + // (toSessionSummary never populates isArchived), so the bridge throws + // SessionNotFoundError; the route must still surface the documented 409 + // by consulting the persisted location on disk. The runtime-enabled + // harness gives the target a runtimeBaseDir, which the lookup needs to + // find the workspace's persisted sessions. + await teardown(h); + h = await makeHarness(true); + const archivedFile = path.join( + new Storage(h.workspace, h.scratch).getProjectDir(), + 'chats', + 'archive', + `${ARCHIVED_SESSION_ID}.jsonl`, + ); + await fsp.mkdir(path.dirname(archivedFile), { recursive: true }); + await fsp.writeFile( + archivedFile, + `${JSON.stringify({ cwd: h.workspace })}\n`, + 'utf8', + ); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: ARCHIVED_SESSION_ID, }); expect(res.status).toBe(409); expect(res.body.code).toBe('session_archived'); @@ -713,8 +756,8 @@ describe('scheduled-tasks routes', () => { }); it('rejects a session already bound to another task', async () => { - h.bridge.liveSessions.set('caller-sess', { - sessionId: 'caller-sess', + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, workspaceCwd: h.workspace, hasActivePrompt: false, }); @@ -728,21 +771,56 @@ describe('scheduled-tasks routes', () => { createdAt: 1_700_000_000_000, lastFiredAt: 1_700_000_000_000, enabled: true, - sessionId: 'caller-sess', + sessionId: CALLER_SESSION_ID, }, ]); const res = await create({ cron: '0 10 * * *', prompt: 'p', - sessionId: 'caller-sess', + sessionId: CALLER_SESSION_ID, }); expect(res.status).toBe(409); expect(res.body.code).toBe('session_already_bound'); expect(await readCronTasks(h.workspace)).toHaveLength(1); // unchanged }); + it('binds a session at most once across concurrent creates', async () => { + // Both pre-checks can read an empty cron file before either write lands; + // the under-write-lock duplicate check is the only guard for that race + // (updateCronTasks serializes the writers, so the second mutate re-reads + // the first create's committed task). Deleting the in-lock check turns + // the second response into a 201. + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + const [a, b] = await Promise.all([ + create({ cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID }), + create({ cron: '0 10 * * *', prompt: 'q', sessionId: CALLER_SESSION_ID }), + ]); + const statuses = [a.status, b.status].sort(); + expect(statuses).toEqual([201, 409]); + const rejected = a.status === 409 ? a : b; + expect(rejected.body.code).toBe('session_already_bound'); + // Exactly one task on disk, bound exactly once. + const tasks = await readCronTasks(h.workspace); + expect(tasks).toHaveLength(1); + expect(tasks[0]?.sessionId).toBe(CALLER_SESSION_ID); + expect(h.bridge.closed).toEqual([]); + }); + it('rejects an invalid sessionId field with 400 invalid_session_id', async () => { - for (const bad of [123, true, '', ' ']) { + for (const bad of [ + 123, + true, + '', + ' ', + 'not-a-uuid', + // UUID grammar only — the agent-suffix internal form is not a valid + // CALLER-supplied id on any surface. + '10000000-0000-4000-8000-000000000001-agent-x', + ]) { const res = await create({ cron: '0 9 * * *', prompt: 'p', @@ -754,6 +832,41 @@ describe('scheduled-tasks routes', () => { expect(await readCronTasks(h.workspace)).toEqual([]); }); + it('mints a dedicated session when sessionId is null', async () => { + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: null, + }); + expect(res.status).toBe(201); + expect(h.bridge.spawned).toHaveLength(1); + expect(res.body.sessionId).toBe(h.bridge.spawned[0]); + }); + + it('normalizes a padded or mixed-case caller sessionId', async () => { + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + const padded = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: ` ${CALLER_SESSION_ID} `, + }); + expect(padded.status).toBe(201); + expect(padded.body.sessionId).toBe(CALLER_SESSION_ID); + const upper = await create({ + cron: '0 10 * * *', + prompt: 'q', + sessionId: OTHER_SESSION_ID.toUpperCase(), + }); + // A different (uppercase-spelled) session id that is NOT live → 404, and + // the lookup happened with the normalized lowercase spelling. + expect(upper.status).toBe(404); + expect(upper.body.code).toBe('session_not_found'); + }); + it('rejects sessionId when session management is unavailable (no bridge)', async () => { const app = express(); app.use(express.json()); @@ -765,15 +878,15 @@ describe('scheduled-tasks routes', () => { }); const res = await request(app) .post('/scheduled-tasks') - .send({ cron: '0 9 * * *', prompt: 'p', sessionId: 'caller-sess' }); + .send({ cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID }); expect(res.status).toBe(409); expect(res.body.code).toBe('session_binding_unavailable'); expect(await readCronTasks(h.workspace)).toEqual([]); }); it('leaves the caller-provided session open when the commit fails', async () => { - h.bridge.liveSessions.set('caller-sess', { - sessionId: 'caller-sess', + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, workspaceCwd: h.workspace, hasActivePrompt: false, }); @@ -785,12 +898,89 @@ describe('scheduled-tasks routes', () => { const res = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: 'caller-sess', + sessionId: CALLER_SESSION_ID, }); expect(res.status).toBe(500); expect(res.body.code).toBe('scheduled_tasks_write_failed'); expect(h.bridge.closed).toEqual([]); // caller session left open expect(h.cleanupSession).not.toHaveBeenCalled(); + // The ⏰ rename happens only after the write commits — a failed create + // must not leave the caller's session renamed with no owning task. + expect(h.bridge.named).toEqual([]); + }); + + it('rejects over-cap creates with a caller-provided sessionId too', async () => { + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + await updateCronTasks(h.workspace, (tasks) => [ + ...tasks, + ...Array.from({ length: 50 }, (_, i) => ({ + id: `cap-task-${i}`, + cron: '0 9 * * *', + prompt: `existing ${i}`, + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: 1_700_000_000_000, + enabled: true, + sessionId: `cap-sess-${i}`, + })), + ]); + const res = await create({ + cron: '0 10 * * *', + prompt: 'p', + sessionId: CALLER_SESSION_ID, + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('max_tasks_reached'); + expect(await readCronTasks(h.workspace)).toHaveLength(50); // unchanged + // The caller's session is untouched by the rejected create. + expect(h.bridge.closed).toEqual([]); + expect(h.bridge.named).toEqual([]); + }); + + it('returns 500 scheduled_tasks_session_failed on a generic lookup failure', async () => { + h.bridge.getSessionSummary = () => { + throw new Error('boom'); + }; + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: CALLER_SESSION_ID, + }); + expect(res.status).toBe(500); + expect(res.body.code).toBe('scheduled_tasks_session_failed'); + // No side effects: nothing written, no session spawned or touched. + expect(await readCronTasks(h.workspace)).toEqual([]); + expect(h.bridge.spawned).toEqual([]); + expect(h.bridge.closed).toEqual([]); + expect(h.bridge.named).toEqual([]); + }); + + it('returns 500 (not workspace mismatch) when path canonicalization hits a real I/O error', async () => { + // A two-link symlink cycle makes realpathSync.native throw ELOOP — a + // transient-I/O shaped failure canonicalizeWorkspace deliberately + // re-throws. It must surface as a retryable 500, not a misleading 400 + // mismatch. + const loopDir = path.join(h.scratch, 'loop'); + await fsp.mkdir(loopDir, { recursive: true }); + await fsp.symlink(path.join(loopDir, 'b'), path.join(loopDir, 'a')); + await fsp.symlink(path.join(loopDir, 'a'), path.join(loopDir, 'b')); + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, + workspaceCwd: path.join(loopDir, 'a'), + hasActivePrompt: false, + }); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: CALLER_SESSION_ID, + }); + expect(res.status).toBe(500); + expect(res.body.code).toBe('scheduled_tasks_session_failed'); + expect(await readCronTasks(h.workspace)).toEqual([]); }); it('mints the task session with thread scope (never reuses the shared session)', async () => { @@ -2216,28 +2406,32 @@ describe('workspace-qualified scheduled-tasks routes', () => { }); it('reuses a caller-provided session on the qualified surface and rejects cross-workspace ones', async () => { - h.secondary.bridge.liveSessions.set('sec-sess', { - sessionId: 'sec-sess', + h.secondary.bridge.liveSessions.set(SECONDARY_SESSION_ID, { + sessionId: SECONDARY_SESSION_ID, workspaceCwd: h.secondary.workspaceCwd, hasActivePrompt: false, }); const res = await request(h.app) .post(qualified(h.secondary.workspaceId)) - .send({ cron: '0 9 * * *', prompt: 'p', sessionId: 'sec-sess' }); + .send({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: SECONDARY_SESSION_ID, + }); expect(res.status).toBe(201); - expect(res.body.sessionId).toBe('sec-sess'); + expect(res.body.sessionId).toBe(SECONDARY_SESSION_ID); expect(h.secondary.bridge.spawned).toEqual([]); // A session living in the PRIMARY workspace can't be bound through the // secondary workspace's endpoint. - h.secondary.bridge.liveSessions.set('primary-sess', { - sessionId: 'primary-sess', + h.secondary.bridge.liveSessions.set(PRIMARY_SESSION_ID, { + sessionId: PRIMARY_SESSION_ID, workspaceCwd: h.primary.workspaceCwd, hasActivePrompt: false, }); const bad = await request(h.app) .post(qualified(h.secondary.workspaceId)) - .send({ cron: '0 9 * * *', prompt: 'q', sessionId: 'primary-sess' }); + .send({ cron: '0 9 * * *', prompt: 'q', sessionId: PRIMARY_SESSION_ID }); expect(bad.status).toBe(400); expect(bad.body.code).toBe('session_workspace_mismatch'); }); diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index 3b36676f561..733ea90142e 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -48,9 +48,11 @@ import { type CronTaskDelivery, type DurableCronTask, type CronTaskRun, + type SessionLocation, } from '@qwen-code/qwen-code-core'; import { SessionNotFoundError } from '@qwen-code/acp-bridge/bridgeErrors'; import { canonicalizeWorkspace } from '@qwen-code/acp-bridge/workspacePaths'; +import { parseCallerSuppliedSessionId } from '../../config/session-id.js'; import { writeStderrLine } from '../../utils/stdioHelpers.js'; import { isChannelDeliveryError } from '../../runtime/channel-delivery-ipc.js'; import { @@ -614,6 +616,28 @@ function registerScheduledTaskCrudRoutes( ); } catch (err) { if (err instanceof SessionNotFoundError) { + // Archiving removes a session from the live map first, so an + // archived id surfaces here instead of in `isArchived`. Consult + // the persisted location so the documented 409 still reaches + // the caller; anything not on disk either is genuinely gone. + let location: SessionLocation; + try { + location = await runWithScheduledTaskTarget(target, () => + new SessionService(workspaceCwd, { + runtimeBaseDir: target.runtimeBaseDir, + }).getSessionLocation(providedSessionId), + ); + } catch { + location = undefined; + } + if (location === 'archived') { + res.status(409).json({ + error: + 'The requested session is archived; unarchive it before binding it to a task', + code: 'session_archived', + }); + return; + } res.status(404).json({ error: `Session '${providedSessionId}' was not found`, code: 'session_not_found', @@ -634,8 +658,19 @@ function registerScheduledTaskCrudRoutes( sameWorkspace = canonicalizeWorkspace(summary.workspaceCwd) === canonicalizeWorkspace(workspaceCwd); - } catch { - sameWorkspace = false; + } catch (err) { + // canonicalizeWorkspace swallows ENOENT itself; anything thrown + // here is a real filesystem failure (EACCES/EIO/ELOOP/ESTALE). + // Surface it as a retryable 500 instead of a misleading + // workspace-mismatch 400. + writeStderrLine( + `qwen serve: POST ${base} failed to resolve workspace paths for session '${providedSessionId}': ${err instanceof Error ? err.message : String(err)}`, + ); + res.status(500).json({ + error: 'Failed to look up the requested session', + code: 'scheduled_tasks_session_failed', + }); + return; } if (!sameWorkspace) { res.status(400).json({ @@ -696,23 +731,13 @@ function registerScheduledTaskCrudRoutes( if (!requireOpenGeneration(target, res)) return; if (providedSessionId !== undefined) { // Reuse the caller's session — no spawn (sessionMintedHere stays - // false, so a failed create leaves it open). + // false, so a failed create leaves it open). The ⏰ rename happens + // only AFTER the cron write commits (below): a create that fails + // after renaming would leave the caller's pre-existing session + // permanently named "⏰ …" with no owning task, because + // rollbackSession never touches caller sessions and nothing else + // restores the prior display name. boundSessionId = providedSessionId; - // Name it after the task like a minted session — the scheduled-task - // session lifecycle (including the keepalive's ⏰ naming) applies - // from here on. Best-effort — a rename failure must not fail the - // create. - try { - await runWithScheduledTaskTarget(target, async () => - bridge.updateSessionMetadata(boundSessionId!, { - displayName: scheduledTaskSessionName( - nameResult.value ?? prompt, - ), - }), - ); - } catch { - // metadata update is non-critical - } } else { try { const session = await runWithScheduledTaskTarget(target, () => @@ -865,6 +890,22 @@ function registerScheduledTaskCrudRoutes( }); return; } + if (providedSessionId !== undefined && bridge) { + // Name the reused session after the task — like a minted one, but + // strictly after the cron write commits, so no failure path leaves + // the caller's pre-existing session renamed with no owning task. + // Best-effort — a rename failure must not fail the committed create; + // the keepalive names bound sessions anyway. + try { + await runWithScheduledTaskTarget(target, async () => + bridge.updateSessionMetadata(boundSessionId!, { + displayName: scheduledTaskSessionName(nameResult.value ?? prompt), + }), + ); + } catch { + // metadata update is non-critical + } + } if (task.delivery && task.sessionId) { channelDeliveryAuthorizations?.registerScheduledTask(workspaceCwd, { sessionId: task.sessionId, @@ -1600,7 +1641,12 @@ function parseNameField(raw: unknown): { value?: string; error?: string } { * Parses the optional `sessionId` field on POST (reuse an existing session * instead of minting a dedicated one). Accepts: * - absent / null → `{ value: undefined }` (mint a dedicated session) - * - a non-empty string → `{ value: trimmed }` + * - a valid caller-supplied session id → `{ value }`, canonicalized through + * the same parser every other caller-supplied-session-id surface uses + * (`parseCallerSuppliedSessionId`: UUID grammar, case-normalized, + * length-bounded by the grammar — no unbounded echo in error bodies or + * stderr, and duplicate-binding equality holds per session, not per + * spelling) * - anything else (including empty/whitespace-only strings — a session id * can't be "cleared", so unlike `name` they're an error) → `{ error }` */ @@ -1608,13 +1654,12 @@ function parseSessionIdField(raw: unknown): { value?: string; error?: string; } { - if (raw === undefined || raw === null) return { value: undefined }; - if (typeof raw !== 'string') { - return { error: '`sessionId` must be a string' }; - } - const trimmed = raw.trim(); - if (trimmed.length === 0) { - return { error: '`sessionId` must be a non-empty string' }; + const parsed = parseCallerSuppliedSessionId( + typeof raw === 'string' ? raw.trim() : raw, + ); + if (parsed.kind === 'absent') return { value: undefined }; + if (parsed.kind === 'invalid') { + return { error: '`sessionId` must be a valid session id' }; } - return { value: trimmed }; + return { value: parsed.sessionId }; } From 23cbd6262cf8ffa25cd7592c6e5d2541a0bcac9b Mon Sep 17 00:00:00 2001 From: yiliang114 <1204183885@qq.com> Date: Tue, 18 Aug 2026 01:57:43 +0000 Subject: [PATCH 03/14] fix(serve): classify persisted-but-not-live sessions in task binding probe The scheduled-task binding disk probe only special-cased 'archived'; 'active' and 'conflict' locations fell through to a 404 that misreported existing resumable sessions as nonexistent (routine after daemon restarts, when only task-bound sessions are rehydrated). Answer 409 session_not_live / session_conflict for on-disk states and reserve 404 for genuinely absent ids; add the findSessionIdIgnoringCase fallback for legacy uppercase-spelled session files (mirrors session-id-admission). Also drop the dead isArchived switch the bridge never populates, dedup the repeated rename / lookup-failure bodies behind shared closures, align the invalid_session_id message with the sibling caller-id surfaces, and pin the new behavior plus the post-commit rename-failure invariant with tests. --- .../src/serve/routes/scheduled-tasks.test.ts | 164 ++++++++++++++---- .../cli/src/serve/routes/scheduled-tasks.ts | 152 ++++++++++------ 2 files changed, 226 insertions(+), 90 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index 2a3d2a7688a..36c057b5ab1 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -48,6 +48,10 @@ const BUSY_SESSION_ID = '10000000-0000-4000-8000-000000000004'; const ARCHIVED_SESSION_ID = '10000000-0000-4000-8000-000000000005'; const SECONDARY_SESSION_ID = '10000000-0000-4000-8000-000000000006'; const PRIMARY_SESSION_ID = '10000000-0000-4000-8000-000000000007'; +// Contains cased hex letters so uppercase/lowercase spellings actually differ +// (digit-only fixtures are byte-identical across case changes and can't pin +// case normalization). +const CASED_SESSION_ID = 'abcdef00-0000-4000-8000-000000000003'; /** Stub session bridge: mints sequential fake session ids and records spawns / * closes so tests can assert binding and rollback without a real child. */ @@ -67,7 +71,6 @@ interface StubBridge { sessionId: string; workspaceCwd: string; hasActivePrompt: boolean; - isArchived?: boolean; }; /** The sessions the stub reports as live (for getSessionSummary). */ liveSessions: Map< @@ -76,7 +79,6 @@ interface StubBridge { sessionId: string; workspaceCwd: string; hasActivePrompt: boolean; - isArchived?: boolean; } >; markSessionCatalogChanged: ReturnType; @@ -199,6 +201,28 @@ async function teardown(h: Harness): Promise { await fsp.rm(h.scratch, { recursive: true, force: true }); } +/** Writes a minimal persisted session file for `sessionId` in the given + * archive state so the route's disk-location probe can classify it. Used by + * the not-found fallback tests, where the (stub) bridge reports no live + * session and only the on-disk state decides the response. */ +async function writePersistedSession( + h: Harness, + sessionId: string, + state: 'active' | 'archived', +): Promise { + const dir = path.join( + new Storage(h.workspace, h.scratch).getProjectDir(), + 'chats', + ...(state === 'archived' ? ['archive'] : []), + ); + await fsp.mkdir(dir, { recursive: true }); + await fsp.writeFile( + path.join(dir, `${sessionId}.jsonl`), + `${JSON.stringify({ cwd: h.workspace })}\n`, + 'utf8', + ); +} + function closeGenerationDuringCronCommit(): WorkspaceRuntime['generationGuard'] { let open = true; let checks = 0; @@ -707,13 +731,15 @@ describe('scheduled-tasks routes', () => { expect(await readCronTasks(h.workspace)).toEqual([]); }); - it('rejects an archived session with 409 session_archived', async () => { - h.bridge.liveSessions.set(ARCHIVED_SESSION_ID, { - sessionId: ARCHIVED_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - isArchived: true, - }); + it('rejects an archived session the bridge only reports as not-found', async () => { + // Production archiving removes the session from the live map first, so + // the bridge throws SessionNotFoundError; the route must still surface + // the documented 409 by consulting the persisted location on disk. The + // runtime-enabled harness gives the target a runtimeBaseDir, which the + // lookup needs to find the workspace's persisted sessions. + await teardown(h); + h = await makeHarness(true); + await writePersistedSession(h, ARCHIVED_SESSION_ID, 'archived'); const res = await create({ cron: '0 9 * * *', prompt: 'p', @@ -724,31 +750,56 @@ describe('scheduled-tasks routes', () => { expect(await readCronTasks(h.workspace)).toEqual([]); }); - it('rejects an archived session the bridge only reports as not-found', async () => { - // Production archiving removes the session from the live map first - // (toSessionSummary never populates isArchived), so the bridge throws - // SessionNotFoundError; the route must still surface the documented 409 - // by consulting the persisted location on disk. The runtime-enabled - // harness gives the target a runtimeBaseDir, which the lookup needs to - // find the workspace's persisted sessions. + it('rejects a persisted-but-not-live session with 409 session_not_live', async () => { + // After a daemon restart only task-bound sessions are rehydrated, so a + // plainly persisted session is 'active' on disk but not live in the + // bridge. The probe must answer 409 session_not_live — NOT 404 — so + // clients branching on `session_not_found` are not told an existing, + // resumable session is gone. await teardown(h); h = await makeHarness(true); - const archivedFile = path.join( - new Storage(h.workspace, h.scratch).getProjectDir(), - 'chats', - 'archive', - `${ARCHIVED_SESSION_ID}.jsonl`, - ); - await fsp.mkdir(path.dirname(archivedFile), { recursive: true }); - await fsp.writeFile( - archivedFile, - `${JSON.stringify({ cwd: h.workspace })}\n`, - 'utf8', - ); + await writePersistedSession(h, SECONDARY_SESSION_ID, 'active'); const res = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: ARCHIVED_SESSION_ID, + sessionId: SECONDARY_SESSION_ID, + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_not_live'); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('rejects a conflicted persisted session with 409 session_conflict', async () => { + // A session file present in BOTH states classifies as 'conflict'; the + // probe maps it to the sibling `session_conflict` code instead of 404. + await teardown(h); + h = await makeHarness(true); + await writePersistedSession(h, SECONDARY_SESSION_ID, 'active'); + await writePersistedSession(h, SECONDARY_SESSION_ID, 'archived'); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: SECONDARY_SESSION_ID, + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_conflict'); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + + it('classifies a legacy uppercase-spelled session via the case-insensitive fallback', async () => { + // Legacy CLI sessions may be persisted with `uuidgen`'s uppercase + // spelling while caller ids are canonicalized to lowercase. On a + // case-sensitive filesystem the exact-spelling probe misses the file; + // the findSessionIdIgnoringCase fallback (mirroring + // session-id-admission) must still classify it as archived rather than + // answering a false session_not_found. + await teardown(h); + h = await makeHarness(true); + await writePersistedSession(h, CASED_SESSION_ID.toUpperCase(), 'archived'); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: CASED_SESSION_ID, }); expect(res.status).toBe(409); expect(res.body.code).toBe('session_archived'); @@ -856,15 +907,34 @@ describe('scheduled-tasks routes', () => { }); expect(padded.status).toBe(201); expect(padded.body.sessionId).toBe(CALLER_SESSION_ID); + + // Positive case-normalization discriminator: a live session keyed by the + // lowercase spelling must be found — and bound with the normalized + // lowercase id — when the caller posts an uppercase spelling. A fixture + // with cased hex letters is required: digit-only ids are byte-identical + // across case changes, so they cannot catch a dropped lowercase step. + h.bridge.liveSessions.set(CASED_SESSION_ID, { + sessionId: CASED_SESSION_ID, + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); const upper = await create({ cron: '0 10 * * *', prompt: 'q', - sessionId: OTHER_SESSION_ID.toUpperCase(), + sessionId: CASED_SESSION_ID.toUpperCase(), }); - // A different (uppercase-spelled) session id that is NOT live → 404, and - // the lookup happened with the normalized lowercase spelling. - expect(upper.status).toBe(404); - expect(upper.body.code).toBe('session_not_found'); + expect(upper.status).toBe(201); + expect(upper.body.sessionId).toBe(CASED_SESSION_ID); + + // A genuinely absent id still 404s (looked up by its normalized + // lowercase spelling). + const missing = await create({ + cron: '0 11 * * *', + prompt: 'r', + sessionId: MISSING_SESSION_ID, + }); + expect(missing.status).toBe(404); + expect(missing.body.code).toBe('session_not_found'); }); it('rejects sessionId when session management is unavailable (no bridge)', async () => { @@ -909,6 +979,32 @@ describe('scheduled-tasks routes', () => { expect(h.bridge.named).toEqual([]); }); + it('keeps a committed create when the post-commit rename fails', async () => { + // Pins the documented invariant on the reuse path's post-commit rename: + // a transient updateSessionMetadata failure (catalog rebuild, bridge + // mid-restart) right after the cron write commits must NOT turn the + // successful 201 into a 500 — the task exists on disk, and a retry would + // then fail with session_already_bound for a task that was created. + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + h.bridge.updateSessionMetadata = vi.fn(async () => { + throw new Error('metadata backend unavailable'); + }); + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: CALLER_SESSION_ID, + }); + expect(res.status).toBe(201); + const tasks = await readCronTasks(h.workspace); + expect(tasks).toHaveLength(1); + expect(tasks[0]?.sessionId).toBe(CALLER_SESSION_ID); + expect(h.bridge.named).toEqual([]); // rename swallowed, not rethrown + }); + it('rejects over-cap creates with a caller-provided sessionId too', async () => { h.bridge.liveSessions.set(CALLER_SESSION_ID, { sessionId: CALLER_SESSION_ID, diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index 733ea90142e..e93b770ccee 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -107,12 +107,13 @@ export interface ScheduledTasksSessionBridge { ): unknown; /** Live summary for one session by id. Throws `SessionNotFoundError` when * no live session with that id exists on this daemon. Used to validate a - * caller-provided session (workspace, idle, archived) before binding it to - * a new task. */ + * caller-provided session (workspace, idle) before binding it to a new + * task. Archiving removes a session from the live map, so archived (and + * otherwise persisted-but-not-live) ids surface as `SessionNotFoundError` + * and are classified by the route's on-disk location probe instead. */ getSessionSummary(sessionId: string): { workspaceCwd: string; hasActivePrompt: boolean; - isArchived?: boolean; }; } @@ -592,6 +593,24 @@ function registerScheduledTaskCrudRoutes( // True only when THIS route minted the bound session (and must tear it // back down if the create fails). False for a caller-provided session. let sessionMintedHere = false; + // Best-effort ⏰ rename shared by both binding modes — the mint path + // calls it before the cron write, the reuse path strictly after commit + // (that timing difference is the intentional part and stays at the + // call sites). One copy so the naming payload can't drift between + // minted and reused task sessions. + const nameBoundSession = async () => { + if (!bridge) return; + try { + await runWithScheduledTaskTarget(target, async () => + bridge.updateSessionMetadata(boundSessionId!, { + displayName: scheduledTaskSessionName(nameResult.value ?? prompt), + }), + ); + } catch { + // metadata update is non-critical — a rename failure must not fail + // the create; the keepalive names bound sessions anyway. + } + }; if (providedSessionId !== undefined && !bridge) { // Fail closed: silently creating an UNBOUND task would give the caller // a materially different task from the one it asked for. @@ -605,10 +624,20 @@ function registerScheduledTaskCrudRoutes( if (bridge) { if (providedSessionId !== undefined) { // Validate the caller's session BEFORE any write. + // Two lookup failures share one response shape; keep the body in + // one place so the copies can't drift. + const sendSessionLookupFailed = (detail: string, err: unknown) => { + writeStderrLine( + `qwen serve: POST ${base} ${detail} '${providedSessionId}': ${err instanceof Error ? err.message : String(err)}`, + ); + res.status(500).json({ + error: 'Failed to look up the requested session', + code: 'scheduled_tasks_session_failed', + }); + }; let summary: { workspaceCwd: string; hasActivePrompt: boolean; - isArchived?: boolean; }; try { summary = await runWithScheduledTaskTarget(target, () => @@ -616,17 +645,38 @@ function registerScheduledTaskCrudRoutes( ); } catch (err) { if (err instanceof SessionNotFoundError) { - // Archiving removes a session from the live map first, so an - // archived id surfaces here instead of in `isArchived`. Consult - // the persisted location so the documented 409 still reaches - // the caller; anything not on disk either is genuinely gone. + // Archiving removes a session from the live map first, and + // persisted-but-not-live sessions (the routine state after a + // daemon restart or idle reaping) are absent from it too. + // Probe the persisted location so every on-disk state reaches + // the caller as a machine-actionable classification; only an + // id with nothing on disk is genuinely gone (404). + const sessionService = new SessionService(workspaceCwd, { + runtimeBaseDir: target.runtimeBaseDir, + }); let location: SessionLocation; try { location = await runWithScheduledTaskTarget(target, () => - new SessionService(workspaceCwd, { - runtimeBaseDir: target.runtimeBaseDir, - }).getSessionLocation(providedSessionId), + sessionService.getSessionLocation(providedSessionId), ); + if (location === undefined) { + // Legacy CLI sessions may be persisted with an uppercase + // UUID spelling while caller ids are canonicalized to + // lowercase; mirror session-id-admission's fallback so + // those still resolve on case-sensitive filesystems. + const legacyId = await runWithScheduledTaskTarget( + target, + () => + sessionService.findSessionIdIgnoringCase( + providedSessionId, + ), + ); + if (legacyId !== undefined) { + location = await runWithScheduledTaskTarget(target, () => + sessionService.getSessionLocation(legacyId), + ); + } + } } catch { location = undefined; } @@ -638,19 +688,29 @@ function registerScheduledTaskCrudRoutes( }); return; } + if (location === 'active' || location === 'conflict') { + // The session exists on disk but is not live on this daemon + // (only task-bound sessions are rehydrated at startup). + // Reserve 404 for ids that are genuinely gone so clients + // branching on `session_not_found` are not told an existing + // resumable session does not exist. + res.status(409).json({ + error: + 'The requested session is not live on this daemon; load it before binding it to a task', + code: + location === 'conflict' + ? 'session_conflict' + : 'session_not_live', + }); + return; + } res.status(404).json({ error: `Session '${providedSessionId}' was not found`, code: 'session_not_found', }); return; } - writeStderrLine( - `qwen serve: POST ${base} failed to look up session '${providedSessionId}': ${err instanceof Error ? err.message : String(err)}`, - ); - res.status(500).json({ - error: 'Failed to look up the requested session', - code: 'scheduled_tasks_session_failed', - }); + sendSessionLookupFailed('failed to look up session', err); return; } let sameWorkspace = false; @@ -663,16 +723,18 @@ function registerScheduledTaskCrudRoutes( // here is a real filesystem failure (EACCES/EIO/ELOOP/ESTALE). // Surface it as a retryable 500 instead of a misleading // workspace-mismatch 400. - writeStderrLine( - `qwen serve: POST ${base} failed to resolve workspace paths for session '${providedSessionId}': ${err instanceof Error ? err.message : String(err)}`, + sendSessionLookupFailed( + 'failed to resolve workspace paths for session', + err, ); - res.status(500).json({ - error: 'Failed to look up the requested session', - code: 'scheduled_tasks_session_failed', - }); return; } if (!sameWorkspace) { + // Unreachable under production daemon wiring — one bridge serves + // exactly one workspace runtime, so a cross-workspace id throws + // SessionNotFoundError and is answered above before this runs. + // Kept as a defense for the structural bridge interface (a + // multi-workspace embedder can serve foreign sessions here). res.status(400).json({ error: "The requested session belongs to a different workspace; use that workspace's scheduled-task endpoint", @@ -680,14 +742,6 @@ function registerScheduledTaskCrudRoutes( }); return; } - if (summary.isArchived === true) { - res.status(409).json({ - error: - 'The requested session is archived; unarchive it before binding it to a task', - code: 'session_archived', - }); - return; - } if (summary.hasActivePrompt) { res.status(409).json({ error: @@ -756,17 +810,7 @@ function registerScheduledTaskCrudRoutes( } // Name the session after the task so it's recognizable in the session // list. Best-effort — a nameless session still fires correctly. - try { - await runWithScheduledTaskTarget(target, async () => - bridge.updateSessionMetadata(boundSessionId!, { - displayName: scheduledTaskSessionName( - nameResult.value ?? prompt, - ), - }), - ); - } catch { - // metadata update is non-critical - } + await nameBoundSession(); } catch (err) { if (sendActivityGateError(res, err)) return; if (sendGenerationClosedError(res, err)) return; @@ -892,19 +936,9 @@ function registerScheduledTaskCrudRoutes( } if (providedSessionId !== undefined && bridge) { // Name the reused session after the task — like a minted one, but - // strictly after the cron write commits, so no failure path leaves + // strictly AFTER the cron write commits, so no failure path leaves // the caller's pre-existing session renamed with no owning task. - // Best-effort — a rename failure must not fail the committed create; - // the keepalive names bound sessions anyway. - try { - await runWithScheduledTaskTarget(target, async () => - bridge.updateSessionMetadata(boundSessionId!, { - displayName: scheduledTaskSessionName(nameResult.value ?? prompt), - }), - ); - } catch { - // metadata update is non-critical - } + await nameBoundSession(); } if (task.delivery && task.sessionId) { channelDeliveryAuthorizations?.registerScheduledTask(workspaceCwd, { @@ -1659,7 +1693,13 @@ function parseSessionIdField(raw: unknown): { ); if (parsed.kind === 'absent') return { value: undefined }; if (parsed.kind === 'invalid') { - return { error: '`sessionId` must be a valid session id' }; + // Same actionable grammar hint as the sibling caller-id surfaces + // (POST /session and ACP session/new), so a malformed id gets one + // consistent, machine-translatable answer everywhere. + return { + error: + '`sessionId` must be an RFC UUID v1-v5 (e.g. "550e8400-e29b-41d4-a716-446655440000")', + }; } return { value: parsed.sessionId }; } From ca6a91d5ed5d4a1086c4162175f0ecc881592de5 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 18 Aug 2026 13:11:39 +0800 Subject: [PATCH 04/14] fix(serve): gate scheduled-task delete teardown on session ownership MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Persist whether a task's bound session was minted by the task (sessionOwnedByTask on DurableCronTask) and only close it on DELETE when the task owns it — a caller-provided session pre-existed the task and must survive its deletion. Tasks written before the marker keep today's teardown (their bound sessions were always task-minted), and the keepalive stamps ownership when it binds a freshly minted session. Also stop mapping real filesystem failures in the persisted-session probe to 404 session_not_found: the probe helpers rethrow non-ENOENT errors (EACCES/EIO/ESTALE), which now surface as a retryable 500 scheduled_tasks_session_failed with a stderr log, matching the sibling canonicalizeWorkspace catch in the same block. Keepalive naming now uses the same payload as the route (task.name ?? task.prompt), so the post-restart sweep no longer clobbers the route's ⏰ name on bound sessions (matters now that caller-provided sessions are named by the route too). --- .../src/serve/routes/scheduled-tasks.test.ts | 74 +++++++++++++++++++ .../cli/src/serve/routes/scheduled-tasks.ts | 40 ++++++++-- .../serve/scheduled-task-keepalive.test.ts | 33 +++++++++ .../cli/src/serve/scheduled-task-keepalive.ts | 31 +++++--- packages/core/src/services/cronTasksFile.ts | 11 +++ 5 files changed, 171 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index 36c057b5ab1..8127fc9907b 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -1274,6 +1274,80 @@ describe('scheduled-tasks routes', () => { expect(h.bridge.closed).toEqual([created.body.sessionId]); }); + it('keeps a caller-provided session alive when its task is deleted', async () => { + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + const created = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: CALLER_SESSION_ID, + }); + expect(created.status).toBe(201); + // The create persisted that the session is NOT owned by the task. + const persisted = await readCronTasks(h.workspace); + expect(persisted[0]?.sessionOwnedByTask).toBe(false); + + const del = await request(h.app).delete( + `/scheduled-tasks/${created.body.id}`, + ); + expect(del.status).toBe(200); + expect(await readCronTasks(h.workspace)).toEqual([]); + // The caller's pre-existing session must survive the task's deletion — + // it is the user's live working session, not the task's to tear down. + expect(h.bridge.closed).toEqual([]); + }); + + it('still closes the session of a legacy bound task without the ownership marker', async () => { + // Tasks written before ownership was persisted carry no marker; every + // session bindable back then was task-minted, so delete keeps tearing + // those sessions down (backward-compatible default). + await updateCronTasks(h.workspace, (tasks) => [ + ...tasks, + { + id: 'legacytask', + cron: '0 9 * * *', + prompt: 'p', + recurring: true, + createdAt: Date.now(), + lastFiredAt: null, + sessionId: 'sess-legacy', + }, + ]); + + const del = await request(h.app).delete('/scheduled-tasks/legacytask'); + expect(del.status).toBe(200); + expect(h.bridge.closed).toEqual(['sess-legacy']); + }); + + it('returns 500 (not 404) when the persisted-session probe hits a filesystem failure', async () => { + // The probe helpers rethrow non-ENOENT errors (EACCES/EIO/ESTALE). Such a + // failure is transient I/O, not "genuinely gone" — it must surface as a + // retryable 500, not a definitive 404 session_not_found. + const eacces = Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }); + const probe = vi + .spyOn(SessionService.prototype, 'getSessionLocation') + .mockRejectedValue(eacces); + try { + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: MISSING_SESSION_ID, // not live → falls through to the probe + }); + expect(res.status).toBe(500); + expect(res.body.code).toBe('scheduled_tasks_session_failed'); + expect(await readCronTasks(h.workspace)).toEqual([]); + expect(h.bridge.spawned).toEqual([]); + expect(h.bridge.closed).toEqual([]); + } finally { + probe.mockRestore(); + } + }); + it('preserves a missing DELETE response when no mutation committed', async () => { await teardown(h); let checks = 0; diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index e93b770ccee..431fff593e7 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -677,8 +677,17 @@ function registerScheduledTaskCrudRoutes( ); } } - } catch { - location = undefined; + } catch (err) { + // Both probe helpers swallow ENOENT themselves and rethrow + // every other filesystem error (EACCES/EIO/ESTALE/…), so a + // throw here is a real failure, not "genuinely gone" — answer + // a retryable 500 instead of misreporting an existing session + // as 404 session_not_found (and log it, unlike a true miss). + sendSessionLookupFailed( + 'failed to probe persisted session state', + err, + ); + return; } if (location === 'archived') { res.status(409).json({ @@ -838,7 +847,15 @@ function registerScheduledTaskCrudRoutes( lastFiredAt: now - (now % 60_000), enabled, ...(delivery !== undefined ? { delivery } : {}), - ...(boundSessionId !== undefined ? { sessionId: boundSessionId } : {}), + ...(boundSessionId !== undefined + ? { + sessionId: boundSessionId, + // Persist WHO owns the bound session: DELETE may only tear down + // sessions the task itself minted — a caller-provided session + // pre-existed the task and must survive its deletion. + sessionOwnedByTask: sessionMintedHere, + } + : {}), ...(nameResult.value !== undefined ? { name: nameResult.value } : {}), }; @@ -1267,9 +1284,12 @@ function registerScheduledTaskCrudRoutes( } // Single atomic read-modify-write: capture the task's bound session AND // remove it in one cycle, closing the TOCTOU window a separate - // read-then-remove would open (and cutting three file reads to one). The - // dedicated session exists only to run this task, so it's torn down after. + // read-then-remove would open (and cutting three file reads to one). A + // session the task itself minted exists only to run it, so it's torn + // down after; a caller-provided session pre-existed the task and stays + // open (the persisted sessionOwnedByTask marker tells the two apart). let boundSessionId: string | undefined; + let sessionOwnedByTask = true; let removed = false; let rollbackBefore: DurableCronTask[] | undefined; let rollbackAfter: DurableCronTask[] | undefined; @@ -1283,6 +1303,9 @@ function registerScheduledTaskCrudRoutes( const match = tasks[idx]!.sessionId; if (typeof match === 'string' && match.length > 0) { boundSessionId = match; + // Absent marker = written before ownership was persisted; every + // session bindable then was task-minted, so keep tearing down. + sessionOwnedByTask = tasks[idx]!.sessionOwnedByTask !== false; } removed = true; rollbackBefore = tasks; @@ -1324,8 +1347,11 @@ function registerScheduledTaskCrudRoutes( .json({ error: 'Task not found', code: 'task_not_found' }); return; } - // Stop the now-orphaned session (keeps its transcript on disk as history). - if (boundSessionId && bridge) { + // Stop a task-minted session (keeps its transcript on disk as history). + // A caller-provided session is NEVER closed here — it pre-existed the + // task, may be the user's live working session, and must survive the + // task's deletion (same invariant the create path's rollback honors). + if (boundSessionId && sessionOwnedByTask && bridge) { try { await runWithScheduledTaskTarget(target, () => bridge.closeSession(boundSessionId!), diff --git a/packages/cli/src/serve/scheduled-task-keepalive.test.ts b/packages/cli/src/serve/scheduled-task-keepalive.test.ts index dfacf6055a8..5df1f1005aa 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.test.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.test.ts @@ -696,6 +696,39 @@ describe('scheduled-task keepalive', () => { expect(names[0]![1].displayName).toContain('⏰'); const tasks = await readCronTasks(workspace); expect(tasks[0]!.sessionId).toBe('new-sess-1'); + // The keepalive minted this session, so the task records ownership — + // the DELETE route's gate relies on it. + expect(tasks[0]!.sessionOwnedByTask).toBe(true); + }); + + it('names bound sessions from the task name when one is set', async () => { + // The scheduled-tasks route names a bound session `⏰ `; + // the keepalive must use the same payload or it clobbers the route's + // name (visible on caller-provided sessions, which the route also names). + await updateCronTasks(workspace, () => [ + task({ + id: 'named-bound', + sessionId: 'existing-sess', + prompt: 'summarize the day', + name: 'Digest', + }), + ]); + const names: Array<[string, { displayName?: string }]> = []; + const naming = { + ...bridge, + updateSessionMetadata: (id: string, m: { displayName?: string }) => { + names.push([id, m]); + }, + }; + const ka = startScheduledTaskKeepalive({ + bridge: naming, + boundWorkspace: workspace, + intervalMs: 60_000, + }); + await ka.tick(); + ka.stop(); + expect(names).toHaveLength(1); + expect(names[0]![1].displayName).toBe('⏰ Digest'); }); it('renames a bound session without ⏰ prefix exactly once', async () => { diff --git a/packages/cli/src/serve/scheduled-task-keepalive.ts b/packages/cli/src/serve/scheduled-task-keepalive.ts index 10238fa87c1..15125e15c27 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.ts @@ -112,15 +112,18 @@ const KEEPALIVE_SPAWN_TIMEOUT_MS = 30_000; const MAX_REVIVE_BACKOFF_MS = 30 * 60_000; /** - * Bind unbound durable tasks to dedicated sessions, and rename bound - * sessions that don't yet have the ⏰ prefix. The cron_create tool leaves - * durable tasks unbound so they stay pickable by any lock owner (CLI/ACP - * /headless). In daemon mode this keepalive mints a dedicated session per - * task and names it — binding is a daemon-only concern. + * Bind unbound durable tasks to dedicated sessions, and (re)name bound + * sessions. The cron_create tool leaves durable tasks unbound so they stay + * pickable by any lock owner (CLI/ACP/headless). In daemon mode this + * keepalive mints a dedicated session per task and names it — binding is a + * daemon-only concern. * - * For unbound tasks: mints a dedicated session, names it `⏰ prompt`, - * writes sessionId to disk. - * For bound tasks without ⏰ name: renames the session to `⏰ prompt`. + * For unbound tasks: mints a dedicated session, names it `⏰ `, writes sessionId to disk. + * For bound tasks: renames the session to `⏰ ` — the SAME + * payload the scheduled-tasks route names with, so the route's naming and + * this sweep agree instead of clobbering each other with different names + * (matters for caller-provided sessions, which the route also names). * * A Set tracks renamed sessions so we don't call updateSessionMetadata * every tick. Best-effort — failures are logged and retried next tick. @@ -190,7 +193,7 @@ async function bindAndNameSessions( spawnedSessionId = sessionId; try { bridge.updateSessionMetadata(sessionId, { - displayName: scheduledTaskSessionName(task.prompt), + displayName: scheduledTaskSessionName(task.name ?? task.prompt), }); renamed.add(sessionId); } catch { @@ -211,7 +214,13 @@ async function bindAndNameSessions( } const result = list.map((t) => t.id === task.id && !t.sessionId && t.enabled !== false - ? { ...t, sessionId } + ? { + ...t, + sessionId, + // The keepalive minted this session, so deleting the task + // later may tear it down (see the DELETE route's gate). + sessionOwnedByTask: true, + } : t, ); matched = true; @@ -239,7 +248,7 @@ async function bindAndNameSessions( const sessionId = task.sessionId!; try { bridge.updateSessionMetadata(sessionId, { - displayName: scheduledTaskSessionName(task.prompt), + displayName: scheduledTaskSessionName(task.name ?? task.prompt), }); renamed.add(sessionId); } catch (err) { diff --git a/packages/core/src/services/cronTasksFile.ts b/packages/core/src/services/cronTasksFile.ts index ab7bc0b4b8b..34abd3f1b61 100644 --- a/packages/core/src/services/cronTasksFile.ts +++ b/packages/core/src/services/cronTasksFile.ts @@ -109,6 +109,15 @@ export interface DurableCronTask { * (`cron_create`) and legacy tasks, which keep the shared-owner firing model. */ sessionId?: string; + /** + * Whether the bound session was minted BY the task (`true`) or provided by + * the caller (`false`). Gates delete-time teardown: deleting a task closes a + * session it minted, but must never tear down a caller-provided session — + * that one pre-existed the task and survives it. Absent on tasks written + * before this field existed; every session bindable before then was + * task-minted, so absent is treated as owned (teardown preserved). + */ + sessionOwnedByTask?: boolean; delivery?: CronTaskDelivery; /** * Bounded, newest-last history of recent fires (capped at MAX_TASK_RUNS). @@ -486,6 +495,8 @@ function isValidTask(value: unknown): value is DurableCronTask { // would treat it as unbound, so a "bound" task would silently run unbound. (obj['sessionId'] === undefined || (typeof obj['sessionId'] === 'string' && obj['sessionId'].length > 0)) && + (obj['sessionOwnedByTask'] === undefined || + typeof obj['sessionOwnedByTask'] === 'boolean') && (obj['delivery'] === undefined || isValidDelivery(obj['delivery'])) && (obj['runs'] === undefined || isValidRuns(obj['runs'])) ); From bf8b4183d96e138fc81cdcde3b0eb42aaeaf4eaf Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Tue, 18 Aug 2026 20:45:37 +0800 Subject: [PATCH 05/14] fix(serve): close session-binding races in scheduled-task create/reuse R4-1: re-validate a caller-provided session under the cron write lock; archive/delete tears the session out of the live map before its cron hook runs, so a session that left the map between validation and commit is now rejected with 409 session_not_live instead of binding a 201-returned task to an archived/deleted session. R4-2: the in-lock duplicate-binding check now covers just-minted sessions too (boundSessionId, not only providedSessionId) and runs before the cap check; the alreadyBound branch no longer rolls the session back, since a committed owner task means a concurrent reuse-create won the race and owns the session. R4-3 (narrowed, not closed): DELETE re-reads the cron file right before closeSession and skips teardown when a surviving task references the session; the residual re-read-to-close window needs session-scoped serialization shared with the bind path (follow-up). R4-4: keepalive bind writes also bail when any committed task already references the just-minted session, mirroring the route's in-lock check. R4-5/R4-6: add the missing discriminating tests (mint-site naming, sessionOwnedByTask validation); both mutation-verified. --- .../src/serve/routes/scheduled-tasks.test.ts | 120 ++++++++++++++++++ .../cli/src/serve/routes/scheduled-tasks.ts | 97 +++++++++++--- .../serve/scheduled-task-keepalive.test.ts | 61 ++++++++- .../cli/src/serve/scheduled-task-keepalive.ts | 12 +- .../core/src/services/cronTasksFile.test.ts | 21 +++ 5 files changed, 293 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index 8127fc9907b..448996b8094 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -861,6 +861,83 @@ describe('scheduled-tasks routes', () => { expect(h.bridge.closed).toEqual([]); }); + it('rejects a reuse create whose session is archived between validation and commit', async () => { + // The session passes the pre-lock validation (live, idle) but a + // concurrent archive/delete removes it from the live map before the cron + // write commits. The archive hook (disableTasksForSessions) only sees + // tasks already on disk, so it no-ops for this task — the under-write-lock + // re-validation is the only guard. Deleting it turns the 409 into a 201 + // bound to an archived session. + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + let summaryCalls = 0; + const originalGetSessionSummary = h.bridge.getSessionSummary.bind(h.bridge); + h.bridge.getSessionSummary = (sessionId: string) => { + summaryCalls += 1; + if (summaryCalls > 1) { + // Simulate the archive/delete landing after the first (pre-lock) + // validation: archiving removes a session from the live map first. + throw new SessionNotFoundError(sessionId); + } + return originalGetSessionSummary(sessionId); + }; + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: CALLER_SESSION_ID, + }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_not_live'); + expect(await readCronTasks(h.workspace)).toEqual([]); + // Caller-provided session — never torn down by this route. + expect(h.bridge.closed).toEqual([]); + expect(h.bridge.named).toEqual([]); + }); + + it('does not double-bind a just-minted session a concurrent reuse-create committed', async () => { + // Mint-vs-reuse race: a mint registers its session in the live map + // (doSpawn) BEFORE its cron write commits, so a concurrent reuse-create + // for that session passes every validation and commits first. The in-lock + // duplicate check must cover minted sessions too — deleting the check + // turns this create into a second 201 bound to the same session — and the + // rejected create must NOT tear down the session the winner now owns. + h.bridge.spawnOrAttach = async () => { + const sessionId = 'sess-contested-mint'; + h.bridge.spawned.push(sessionId); + // Simulate the concurrent reuse-create committing while this mint's + // write is still pending. + await updateCronTasks(h.workspace, (tasks) => [ + ...tasks, + { + id: 'reuse-task', + cron: '0 10 * * *', + prompt: 'q', + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: 1_700_000_000_000, + enabled: true, + sessionId, + sessionOwnedByTask: false, + }, + ]); + return { sessionId }; + }; + const res = await create({ cron: '0 9 * * *', prompt: 'p' }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_already_bound'); + // Exactly one task on disk — the reuse winner — still bound. + const tasks = await readCronTasks(h.workspace); + expect(tasks).toHaveLength(1); + expect(tasks[0]?.id).toBe('reuse-task'); + expect(tasks[0]?.sessionId).toBe('sess-contested-mint'); + // The loser must not kill the session the winner committed to. + expect(h.bridge.closed).toEqual([]); + expect(h.cleanupSession).not.toHaveBeenCalled(); + }); + it('rejects an invalid sessionId field with 400 invalid_session_id', async () => { for (const bad of [ 123, @@ -1322,6 +1399,49 @@ describe('scheduled-tasks routes', () => { expect(h.bridge.closed).toEqual(['sess-legacy']); }); + it("does not close a deleted task's session when another committed task references it", async () => { + // DELETE captures the bound session under the lock, but a concurrent + // reuse-create can commit a binding to it right after the removal lands + // (its in-lock duplicate check legitimately passes once the old task is + // gone). The pre-close re-read must notice the surviving reference and + // skip the teardown — closing on the stale capture would kill the + // surviving task's live session. Deleting the recheck puts 'sess-shared' + // back in `closed`. + await updateCronTasks(h.workspace, (tasks) => [ + ...tasks, + { + id: 'deleted-task', + cron: '0 9 * * *', + prompt: 'p', + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: 1_700_000_000_000, + enabled: true, + sessionId: 'sess-shared', + sessionOwnedByTask: true, + }, + { + // The race winner: committed while DELETE's close was still pending. + id: 'surviving-task', + cron: '0 10 * * *', + prompt: 'q', + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: 1_700_000_000_000, + enabled: true, + sessionId: 'sess-shared', + sessionOwnedByTask: false, + }, + ]); + const del = await request(h.app).delete('/scheduled-tasks/deleted-task'); + expect(del.status).toBe(200); + expect(del.body).toEqual({ deleted: true, id: 'deleted-task' }); + expect(h.bridge.closed).toEqual([]); // surviving task keeps its session + const tasks = await readCronTasks(h.workspace); + expect(tasks).toHaveLength(1); + expect(tasks[0]?.id).toBe('surviving-task'); + }); + it('returns 500 (not 404) when the persisted-session probe hits a filesystem failure', async () => { // The probe helpers rethrow non-ENOENT errors (EACCES/EIO/ESTALE). Such a // failure is transient I/O, not "genuinely gone" — it must surface as a diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index 431fff593e7..980045cd5b7 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -875,6 +875,7 @@ function registerScheduledTaskCrudRoutes( let overCap = false; let alreadyBound = false; + let sessionGoneUnderLock = false; let rollbackBefore: DurableCronTask[] | undefined; let rollbackAfter: DurableCronTask[] | undefined; try { @@ -882,6 +883,44 @@ function registerScheduledTaskCrudRoutes( updateCronTasks( workspaceCwd, (tasks) => { + // Same-lock duplicate-binding check in BOTH binding modes: the + // pre-check read above is best-effort, and a concurrent create + // may have bound the same session since. For a caller-provided + // session that's another reuse-create; for a just-minted one + // it's a reuse-create that committed while this request's mint + // was still in flight (the mint registers the session in the + // live map before THIS write commits, so the reuse path's + // validation can pass against it). Runs before the cap check so + // an over-cap loser never tears down a session another + // committed task already references. + if ( + boundSessionId !== undefined && + tasks.some((t) => t.sessionId === boundSessionId) + ) { + alreadyBound = true; + return tasks; + } + // Re-validate a caller-provided session UNDER the write lock: + // archiving/deleting tears the session out of the live map + // BEFORE its cron hook (disable/removeTasksForSessions) runs, + // and that hook only sees tasks already on disk — so a session + // that left the live map between the pre-lock validation and + // this cycle is being archived/deleted and its hook skipped + // this (not yet written) task. Committing anyway would bind a + // 201-returned task to an archived or gone session. Cron write + // cycles are serialized, so a hook that runs after THIS cycle + // sees the new task and disables/removes it correctly. + if (providedSessionId !== undefined && bridge) { + try { + bridge.getSessionSummary(providedSessionId); + } catch (err) { + if (err instanceof SessionNotFoundError) { + sessionGoneUnderLock = true; + return tasks; // no write + } + throw err; + } + } // Cap check under the write lock so two concurrent creates can't both // slip past a stale count. Returning the input unchanged is a no-op // (no write), which the flag below turns into a 409. @@ -889,16 +928,6 @@ function registerScheduledTaskCrudRoutes( overCap = true; return tasks; } - // Same-lock duplicate-binding check for a caller-provided - // session: the pre-check read above is best-effort, and a - // concurrent create may have bound the same session since. - if ( - providedSessionId !== undefined && - tasks.some((t) => t.sessionId === providedSessionId) - ) { - alreadyBound = true; - return tasks; - } rollbackBefore = tasks; rollbackAfter = [...tasks, task]; return rollbackAfter; @@ -942,8 +971,23 @@ function registerScheduledTaskCrudRoutes( }); return; } + if (sessionGoneUnderLock) { + // Reuse mode only — a caller-provided session is never torn down + // here, so there is nothing to roll back. Retryable: the session's + // archive/delete completed between validation and commit. + res.status(409).json({ + error: + 'The requested session was archived or deleted while the task was being created; retry with a live session', + code: 'session_not_live', + }); + return; + } if (alreadyBound) { - await rollbackSession(); + // NO rollbackSession here: the in-lock check fires only when a + // COMMITTED task already references the bound session. For a + // just-minted session that means a concurrent reuse-create won the + // race and owns it — tearing it down would kill that task's session. + // (For a caller-provided session rollbackSession is a no-op anyway.) res.status(409).json({ error: 'The requested session is already bound to another scheduled task', @@ -1352,12 +1396,35 @@ function registerScheduledTaskCrudRoutes( // task, may be the user's live working session, and must survive the // task's deletion (same invariant the create path's rollback honors). if (boundSessionId && sessionOwnedByTask && bridge) { + // Re-read just before teardown: between the removal commit above and + // this close, a concurrent reuse-create can bind THIS session (its + // in-lock duplicate check legitimately passes once the old task is + // gone) — from that task's perspective the session IS caller-provided + // and must survive. Closing on the stale capture would tear down the + // surviving task's live session mid-use. Best-effort: a rebind that + // commits between this re-read and the close still slips through; + // fully closing that window needs session-scoped serialization shared + // with the bind path (tracked as follow-up). A read failure falls + // back to the pre-recheck behavior (close). + let claimedBySurvivingTask = false; try { - await runWithScheduledTaskTarget(target, () => - bridge.closeSession(boundSessionId!), + const currentTasks = await runWithScheduledTaskTarget(target, () => + readCronTasks(workspaceCwd), ); - } catch (error) { - if (sendActivityGateError(res, error)) return; + claimedBySurvivingTask = currentTasks.some( + (t) => t.sessionId === boundSessionId, + ); + } catch { + // Read failure → keep the historical behavior (close the session). + } + if (!claimedBySurvivingTask) { + try { + await runWithScheduledTaskTarget(target, () => + bridge.closeSession(boundSessionId!), + ); + } catch (error) { + if (sendActivityGateError(res, error)) return; + } } } if (boundSessionId) { diff --git a/packages/cli/src/serve/scheduled-task-keepalive.test.ts b/packages/cli/src/serve/scheduled-task-keepalive.test.ts index 5df1f1005aa..33bdfac4f04 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.test.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.test.ts @@ -661,8 +661,12 @@ describe('scheduled-task keepalive', () => { }); it('binds an unbound task to a dedicated session and writes sessionId to disk', async () => { + // Named fixture + exact-name assertion discriminate the MINT-site naming + // payload (`task.name ?? task.prompt`): mutating it to `task.prompt` + // yields '⏰ check build' and fails this test. The already-bound rename + // branch is covered separately below. await updateCronTasks(workspace, () => [ - task({ id: 'unbound-1', prompt: 'check build' }), + task({ id: 'unbound-1', prompt: 'check build', name: 'Digest' }), ]); const spawns: unknown[] = []; const names: Array<[string, { displayName?: string }]> = []; @@ -693,7 +697,7 @@ describe('scheduled-task keepalive', () => { }); expect(names).toHaveLength(1); expect(names[0]![0]).toBe('new-sess-1'); - expect(names[0]![1].displayName).toContain('⏰'); + expect(names[0]![1].displayName).toBe('⏰ Digest'); const tasks = await readCronTasks(workspace); expect(tasks[0]!.sessionId).toBe('new-sess-1'); // The keepalive minted this session, so the task records ownership — @@ -883,6 +887,59 @@ describe('scheduled-task keepalive', () => { removeSpy.mockRestore(); }); + it('rolls back when the just-minted session is already committed to another task', async () => { + // The scheduled-tasks reuse path can bind a session as soon as the spawn + // registers it in the live map — BEFORE this bind write commits. The + // in-lock check must notice the committed reference and leave the task + // unbound (the orphan spawn rolls back), not double-bind the session. + const closed: string[] = []; + const removeSpy = vi + .spyOn(SessionService.prototype, 'removeSession') + .mockResolvedValue(true); + const raceBridge = { + ...bridge, + spawnOrAttach: async () => { + // Simulate a concurrent caller-provided binding committing while our + // spawn is in flight. + await updateCronTasks(workspace, (list) => [ + ...list, + task({ + id: 'caller-task', + sessionId: 'contested-sess', + sessionOwnedByTask: false, + }), + ]); + return { sessionId: 'contested-sess' }; + }, + closeSession: async (id: string) => { + closed.push(id); + }, + markSessionCatalogChanged: vi.fn(), + updateSessionMetadata: () => {}, + }; + await updateCronTasks(workspace, () => [ + task({ id: 'tool-task', prompt: 'contested' }), + ]); + const ka = startScheduledTaskKeepalive({ + bridge: raceBridge, + boundWorkspace: workspace, + intervalMs: 60_000, + }); + await ka.tick(); + ka.stop(); + // The orphaned mint is rolled back... + expect(closed).toContain('contested-sess'); + expect(raceBridge.markSessionCatalogChanged).toHaveBeenCalledTimes(1); + // ...and the session stays bound to exactly ONE task: the caller's. + const tasks = await readCronTasks(workspace); + expect(tasks).toHaveLength(2); + const toolTask = tasks.find((t) => t.id === 'tool-task'); + const callerTask = tasks.find((t) => t.id === 'caller-task'); + expect(toolTask?.sessionId).toBeUndefined(); // still unbound + expect(callerTask?.sessionId).toBe('contested-sess'); + removeSpy.mockRestore(); + }); + it('a hung spawnOrAttach does not stall subsequent ticks', async () => { // spawnOrAttach is not abortable — if it hangs, the keepalive must time // out and move on so later ticks can still heartbeat/revive other diff --git a/packages/cli/src/serve/scheduled-task-keepalive.ts b/packages/cli/src/serve/scheduled-task-keepalive.ts index 15125e15c27..7383ebbb051 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.ts @@ -205,10 +205,20 @@ async function bindAndNameSessions( // read and this write-lock acquisition — only attach when the task is // still unbound and enabled. Otherwise return unchanged so the // orphan spawn is rolled back below. + // + // Also bail when ANY committed task already references the + // just-minted session: the scheduled-tasks reuse path can bind a + // session the moment the spawn above registers it in the live map, + // BEFORE this write commits — without this check the session would be + // bound to two tasks (same transcript, conflicting ⏰ renames), and a + // later delete of THIS task would close the session out from under + // the surviving one. The orphan rollback below then tears the + // unclaimed session back down. if ( !list.some( (t) => t.id === task.id && !t.sessionId && t.enabled !== false, - ) + ) || + list.some((t) => t.sessionId === sessionId) ) { return list; } diff --git a/packages/core/src/services/cronTasksFile.test.ts b/packages/core/src/services/cronTasksFile.test.ts index ee6b21db258..575052591b7 100644 --- a/packages/core/src/services/cronTasksFile.test.ts +++ b/packages/core/src/services/cronTasksFile.test.ts @@ -215,6 +215,27 @@ describe('cronTasksFile', () => { await expect(readCronTasks(tmpDir)).rejects.toThrow(/Invalid task entry/); }); + it('rejects a task whose sessionOwnedByTask is not a boolean', async () => { + // The marker decides whether DELETE tears the bound session down, so a + // hand-edited/corrupted file carrying garbage here must fail fast like + // every sibling optional field — not load silently. Deleting the + // validation branch keeps this test red. + await seedTasksFile( + tmpDir, + JSON.stringify([ + { ...makeTask(), sessionId: 'sess-1', sessionOwnedByTask: 'yes' }, + ]), + ); + await expect(readCronTasks(tmpDir)).rejects.toThrow(/Invalid task entry/); + }); + + it('round-trips the optional sessionOwnedByTask field', async () => { + const task = makeTask({ sessionId: 'sess-1', sessionOwnedByTask: true }); + await writeCronTasks(tmpDir, [task]); + const result = await readCronTasks(tmpDir); + expect(result).toEqual([task]); + }); + it('round-trips the optional runs history', async () => { const task = makeTask({ lastFiredAt: 1718000300000, From 9c4d7567fad0dda746f1b1ff81cd298d775f5e49 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Tue, 18 Aug 2026 23:55:42 +0800 Subject: [PATCH 06/14] fix(serve): keepalive must not tear down a session a committed task owns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-14 review caught a regression in the duplicate-reference bail: it routed the "a committed task already references the just-minted session" case into the orphan rollback. In production wiring cleanupSession is deleteDaemonSessionIfOrphan, whose requireZeroAttaches passes for a just-minted session, and whose persisted removal cascades removeTasksForSessions — so the rollback killed the race-winning task's live session AND deleted its committed task from the cron file. The two no-write bail reasons are now distinguishable: the committed-reference check runs first and, when it fires, keepalive logs and continues without cleanup — the session is left to its owner (mirroring the route's symmetric alreadyBound branch, which performs no rollback for exactly this reason) and this task stays unbound on disk for the next tick to retry with a fresh session. The original bail (task no longer bindable) still rolls the orphan back, unchanged. Also pin three load-bearing behaviors that had no coverage: the duplicate-check-before-cap-check ordering at the cap boundary (session_already_bound, never max_tasks_reached with rollback), the DELETE pre-close re-read failure fallback (still closes the owned session), and the under-lock re-validation generic-error branch (500, never coerced to session_not_live). All three mutation-verified. --- .../src/serve/routes/scheduled-tasks.test.ts | 135 ++++++++++++++++++ .../serve/scheduled-task-keepalive.test.ts | 23 ++- .../cli/src/serve/scheduled-task-keepalive.ts | 48 +++++-- 3 files changed, 189 insertions(+), 17 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index 448996b8094..d476cda1881 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -11,6 +11,7 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import request from 'supertest'; +import * as core from '@qwen-code/qwen-code-core'; import { SessionService, Storage, @@ -897,6 +898,40 @@ describe('scheduled-tasks routes', () => { expect(h.bridge.named).toEqual([]); }); + it('returns 500 (not session_not_live) when the under-lock re-validation hits a generic error', async () => { + // Pins the under-write-lock rethrow contract: ONLY SessionNotFoundError + // maps to the 409 session_not_live branch; any other getSessionSummary + // failure is transient I/O and must abort the write as a retryable 500. + // Coercing generic errors into sessionGoneUnderLock would turn this into + // a 409 with the wrong code. + h.bridge.liveSessions.set(CALLER_SESSION_ID, { + sessionId: CALLER_SESSION_ID, + workspaceCwd: h.workspace, + hasActivePrompt: false, + }); + let summaryCalls = 0; + const originalGetSessionSummary = h.bridge.getSessionSummary.bind(h.bridge); + h.bridge.getSessionSummary = (sessionId: string) => { + summaryCalls += 1; + if (summaryCalls > 1) { + // The pre-lock validation passes; the under-lock re-validation hits + // a generic (non-SessionNotFoundError) failure. + throw new Error('summary backend unavailable'); + } + return originalGetSessionSummary(sessionId); + }; + const res = await create({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: CALLER_SESSION_ID, + }); + expect(res.status).toBe(500); + expect(res.body.code).toBe('scheduled_tasks_write_failed'); + expect(await readCronTasks(h.workspace)).toEqual([]); + // Caller-provided session — never torn down by this route. + expect(h.bridge.closed).toEqual([]); + }); + it('does not double-bind a just-minted session a concurrent reuse-create committed', async () => { // Mint-vs-reuse race: a mint registers its session in the live map // (doSpawn) BEFORE its cron write commits, so a concurrent reuse-create @@ -938,6 +973,65 @@ describe('scheduled-tasks routes', () => { expect(h.cleanupSession).not.toHaveBeenCalled(); }); + it('answers session_already_bound (not max_tasks_reached) when both fire at the cap boundary', async () => { + // Pins the load-bearing ordering of the under-lock checks: the + // duplicate-binding check runs BEFORE the cap check. At the cap boundary + // both conditions are observable at once — a mint create whose + // just-minted session a concurrent reuse-create committed (as the 50th + // task) while its own write was still pending lands exactly at the cap. + // The overCap branch calls rollbackSession() while alreadyBound + // deliberately does not, so swapping the two checks would tear down the + // session the committed winner owns. Swapping them turns the response + // into max_tasks_reached and puts the contested session in `closed`. + await updateCronTasks(h.workspace, (tasks) => [ + ...tasks, + ...Array.from({ length: 49 }, (_, i) => ({ + id: `cap-task-${i}`, + cron: '0 9 * * *', + prompt: `existing ${i}`, + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: 1_700_000_000_000, + enabled: true, + sessionId: `cap-sess-${i}`, + })), + ]); + h.bridge.spawnOrAttach = async () => { + const sessionId = 'sess-cap-contested'; + h.bridge.spawned.push(sessionId); + // Simulate the concurrent reuse-create committing the 50th task — + // referencing the just-minted session — while this create's write is + // still pending. Under the lock, BOTH the duplicate reference and the + // cap are now observable. + await updateCronTasks(h.workspace, (tasks) => [ + ...tasks, + { + id: 'reuse-task', + cron: '0 10 * * *', + prompt: 'q', + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: 1_700_000_000_000, + enabled: true, + sessionId, + sessionOwnedByTask: false, + }, + ]); + return { sessionId }; + }; + const res = await create({ cron: '0 9 * * *', prompt: 'p' }); + expect(res.status).toBe(409); + expect(res.body.code).toBe('session_already_bound'); + // The loser must not roll back the session the winner committed to. + expect(h.bridge.closed).toEqual([]); + expect(h.cleanupSession).not.toHaveBeenCalled(); + const tasks = await readCronTasks(h.workspace); + expect(tasks).toHaveLength(50); // at cap, unchanged + expect(tasks.find((t) => t.id === 'reuse-task')?.sessionId).toBe( + 'sess-cap-contested', + ); + }); + it('rejects an invalid sessionId field with 400 invalid_session_id', async () => { for (const bad of [ 123, @@ -1442,6 +1536,47 @@ describe('scheduled-tasks routes', () => { expect(tasks[0]?.id).toBe('surviving-task'); }); + it('still closes a task-minted session when the DELETE pre-close re-read fails', async () => { + // Pins the documented fallback for the pre-close re-read: when the + // post-commit re-read itself fails (transient I/O), DELETE keeps the + // historical behavior and closes the task-minted session. Letting the + // read failure escape turns the 200 into a 500; changing the fallback + // to skip the close empties `closed`. + await updateCronTasks(h.workspace, (tasks) => [ + ...tasks, + { + id: 'owned-task', + cron: '0 9 * * *', + prompt: 'p', + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: 1_700_000_000_000, + enabled: true, + sessionId: 'sess-owned', + sessionOwnedByTask: true, + }, + ]); + // Reject ONLY the post-commit re-read: the removal above commits through + // updateCronTasks's module-local read, which the barrel spy does not + // intercept. + const readSpy = vi.spyOn(core, 'readCronTasks').mockRejectedValue( + Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }), + ); + const del = await (async () => { + try { + return await request(h.app).delete('/scheduled-tasks/owned-task'); + } finally { + readSpy.mockRestore(); + } + })(); + expect(del.status).toBe(200); + expect(del.body).toEqual({ deleted: true, id: 'owned-task' }); + expect(h.bridge.closed).toEqual(['sess-owned']); + expect(await readCronTasks(h.workspace)).toEqual([]); + }); + it('returns 500 (not 404) when the persisted-session probe hits a filesystem failure', async () => { // The probe helpers rethrow non-ENOENT errors (EACCES/EIO/ESTALE). Such a // failure is transient I/O, not "genuinely gone" — it must surface as a diff --git a/packages/cli/src/serve/scheduled-task-keepalive.test.ts b/packages/cli/src/serve/scheduled-task-keepalive.test.ts index 33bdfac4f04..68befc1ca5e 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.test.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.test.ts @@ -887,11 +887,19 @@ describe('scheduled-task keepalive', () => { removeSpy.mockRestore(); }); - it('rolls back when the just-minted session is already committed to another task', async () => { + it('leaves a just-minted session alone when another committed task already references it', async () => { // The scheduled-tasks reuse path can bind a session as soon as the spawn // registers it in the live map — BEFORE this bind write commits. The // in-lock check must notice the committed reference and leave the task - // unbound (the orphan spawn rolls back), not double-bind the session. + // unbound, not double-bind the session — but it must NOT roll the + // session back either: the committed task owns it now. In production + // wiring cleanupSession is deleteDaemonSessionIfOrphan, whose + // requireZeroAttaches passes for a just-minted session and whose + // persisted removal cascades removeTasksForSessions — a rollback here + // would kill the winner's live session AND delete the winner's task + // from the cron file. (Mirrors the route's alreadyBound branch, which + // deliberately performs no rollbackSession.) Reverting the fix puts + // 'contested-sess' back in `closed` and calls removeSession for it. const closed: string[] = []; const removeSpy = vi .spyOn(SessionService.prototype, 'removeSession') @@ -927,10 +935,13 @@ describe('scheduled-task keepalive', () => { }); await ka.tick(); ka.stop(); - // The orphaned mint is rolled back... - expect(closed).toContain('contested-sess'); - expect(raceBridge.markSessionCatalogChanged).toHaveBeenCalledTimes(1); - // ...and the session stays bound to exactly ONE task: the caller's. + // No rollback: the winner's session survives untouched... + expect(closed).toEqual([]); + expect(removeSpy).not.toHaveBeenCalled(); + expect(raceBridge.markSessionCatalogChanged).not.toHaveBeenCalled(); + // ...and the session stays bound to exactly ONE task: the caller's, + // while THIS task remains unbound for a later tick to retry with a + // fresh session. const tasks = await readCronTasks(workspace); expect(tasks).toHaveLength(2); const toolTask = tasks.find((t) => t.id === 'tool-task'); diff --git a/packages/cli/src/serve/scheduled-task-keepalive.ts b/packages/cli/src/serve/scheduled-task-keepalive.ts index 7383ebbb051..47e2ddb6cd2 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.ts @@ -200,25 +200,32 @@ async function bindAndNameSessions( // naming is non-critical — the session still fires correctly } let matched = false; + // The two no-write reasons must stay distinguishable: when a COMMITTED + // task already references the just-minted session the session is NOT an + // orphan and must not be rolled back (see below); when the task itself + // is no longer bindable the session IS orphaned and rolls back. + let sessionClaimedByCommittedTask = false; await updateCronTasks(boundWorkspace, (list) => { + // Bail when ANY committed task already references the just-minted + // session: the scheduled-tasks reuse path can bind a session the + // moment the spawn above registers it in the live map, BEFORE this + // write commits — without this check the session would be bound to + // two tasks (same transcript, conflicting ⏰ renames), and a later + // delete of THIS task would close the session out from under the + // surviving one. Checked FIRST: a session a committed task references + // must never be torn down, whatever this task's own state. + if (list.some((t) => t.sessionId === sessionId)) { + sessionClaimedByCommittedTask = true; + return list; + } // Another process may have bound or disabled this task between our // read and this write-lock acquisition — only attach when the task is // still unbound and enabled. Otherwise return unchanged so the // orphan spawn is rolled back below. - // - // Also bail when ANY committed task already references the - // just-minted session: the scheduled-tasks reuse path can bind a - // session the moment the spawn above registers it in the live map, - // BEFORE this write commits — without this check the session would be - // bound to two tasks (same transcript, conflicting ⏰ renames), and a - // later delete of THIS task would close the session out from under - // the surviving one. The orphan rollback below then tears the - // unclaimed session back down. if ( !list.some( (t) => t.id === task.id && !t.sessionId && t.enabled !== false, - ) || - list.some((t) => t.sessionId === sessionId) + ) ) { return list; } @@ -237,6 +244,25 @@ async function bindAndNameSessions( return result; }); if (!matched) { + if (sessionClaimedByCommittedTask) { + // A concurrent create committed a reference to the just-minted + // session before this write ran — it owns the session now (the + // route's symmetric `alreadyBound` branch performs NO rollback for + // the same reason). Rolling back here would kill the winner's live + // session: in production wiring cleanupSession is + // deleteDaemonSessionIfOrphan, whose requireZeroAttaches passes + // for a just-minted session, and whose persisted removal cascades + // removeTasksForSessions — deleting the winner's committed task. + // Leave the session to its owner; THIS task stays unbound on + // disk and a later tick retries it with a fresh session. + log.debug( + 'keepalive: session', + sessionId, + 'already committed to another task — leaving it to its owner', + task.id, + ); + continue; + } // Task was deleted between read and write — roll back the orphan. throw new Error(`task ${task.id} no longer on disk`); } From e7608062fbba348c7db5322067974ba6aaa9562e Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Wed, 19 Aug 2026 13:11:15 +0800 Subject: [PATCH 07/14] fix(serve): serialize scheduled-task session teardown with reuse-create binding (#9415) --- .../cli/src/serve/routes/scheduled-tasks.ts | 81 ++++++++++++------- packages/cli/src/serve/server.ts | 2 + 2 files changed, 55 insertions(+), 28 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index 980045cd5b7..fee2dcd3e68 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -60,6 +60,7 @@ import { type PublicChannelDelivery, } from '../../runtime/channel-delivery.js'; import type { ChannelDeliveryAuthorizationStore } from '../channel-delivery-authorization.js'; +import type { SessionArchiveCoordinator } from '../server/session-archive.js'; import type { WorkspaceRegistry, WorkspaceRuntime, @@ -231,6 +232,12 @@ interface RegisterScheduledTaskCrudRoutesDeps { mutate: (opts?: { strict?: boolean }) => RequestHandler; safeBody: (req: Request) => Record; channelDeliveryAuthorizations?: ChannelDeliveryAuthorizationStore; + /** + * Session-scoped serialization shared with the bind path: DELETE teardown + * and reuse-create binding acquire the same per-session lease so a close + * cannot land between a surviving task's validation and its commit (#9415). + */ + sessionArchiveCoordinator?: SessionArchiveCoordinator; } interface RegisterScheduledTasksRoutesDeps { @@ -249,6 +256,7 @@ interface RegisterScheduledTasksRoutesDeps { runtime: WorkspaceRuntime, sessionId: string, ) => Promise; + sessionArchiveCoordinator?: SessionArchiveCoordinator; } interface RegisterWorkspaceQualifiedScheduledTasksRoutesDeps { @@ -270,6 +278,7 @@ interface RegisterWorkspaceQualifiedScheduledTasksRoutesDeps { sessionId: string, ) => Promise; conversationRuntimeActivity?: ConversationRuntimeActivityGate; + sessionArchiveCoordinator?: SessionArchiveCoordinator; } async function runWithScheduledTaskTarget( @@ -410,6 +419,7 @@ function registerScheduledTaskCrudRoutes( mutate, safeBody, channelDeliveryAuthorizations, + sessionArchiveCoordinator, } = deps; const base = `${prefix}/scheduled-tasks`; @@ -878,8 +888,11 @@ function registerScheduledTaskCrudRoutes( let sessionGoneUnderLock = false; let rollbackBefore: DurableCronTask[] | undefined; let rollbackAfter: DurableCronTask[] | undefined; - try { - await runWithScheduledTaskTarget(target, () => + // Serialize a reuse-create's commit with DELETE teardown on the reused + // session's key (#9415): while this shared lease is held, a concurrent + // DELETE cannot hold the exclusive lease and close the session under us. + const commitTask = () => + runWithScheduledTaskTarget(target, () => updateCronTasks( workspaceCwd, (tasks) => { @@ -935,6 +948,13 @@ function registerScheduledTaskCrudRoutes( { assertCanCommit: target.assertGenerationOpen }, ), ); + try { + await (providedSessionId !== undefined && sessionArchiveCoordinator + ? sessionArchiveCoordinator.runSharedMany( + [providedSessionId], + commitTask, + ) + : commitTask()); } catch (err) { await rollbackSession(); if (sendActivityGateError(res, err)) return; @@ -1396,35 +1416,38 @@ function registerScheduledTaskCrudRoutes( // task, may be the user's live working session, and must survive the // task's deletion (same invariant the create path's rollback honors). if (boundSessionId && sessionOwnedByTask && bridge) { - // Re-read just before teardown: between the removal commit above and - // this close, a concurrent reuse-create can bind THIS session (its - // in-lock duplicate check legitimately passes once the old task is - // gone) — from that task's perspective the session IS caller-provided - // and must survive. Closing on the stale capture would tear down the - // surviving task's live session mid-use. Best-effort: a rebind that - // commits between this re-read and the close still slips through; - // fully closing that window needs session-scoped serialization shared - // with the bind path (tracked as follow-up). A read failure falls - // back to the pre-recheck behavior (close). - let claimedBySurvivingTask = false; - try { - const currentTasks = await runWithScheduledTaskTarget(target, () => - readCronTasks(workspaceCwd), - ); - claimedBySurvivingTask = currentTasks.some( - (t) => t.sessionId === boundSessionId, - ); - } catch { - // Read failure → keep the historical behavior (close the session). - } - if (!claimedBySurvivingTask) { + // Serialize with the reuse-create bind path on the session's own key: + // the re-read below narrows the rebind window, and the exclusive lease + // closes it — a reuse-create holds the shared lease while validating + // and committing, so this teardown cannot land in between (#9415). + const teardownSession = async (): Promise => { + let claimedBySurvivingTask = false; try { - await runWithScheduledTaskTarget(target, () => - bridge.closeSession(boundSessionId!), + const currentTasks = await runWithScheduledTaskTarget(target, () => + readCronTasks(workspaceCwd), + ); + claimedBySurvivingTask = currentTasks.some( + (t) => t.sessionId === boundSessionId, + ); + } catch { + // Read failure → keep the historical behavior (close the session). + } + if (claimedBySurvivingTask) return; + return runWithScheduledTaskTarget(target, () => + bridge.closeSession(boundSessionId!), + ); + }; + try { + if (sessionArchiveCoordinator) { + await sessionArchiveCoordinator.runExclusiveMany( + [boundSessionId], + teardownSession, ); - } catch (error) { - if (sendActivityGateError(res, error)) return; + } else { + await teardownSession(); } + } catch (error) { + if (sendActivityGateError(res, error)) return; } } if (boundSessionId) { @@ -1640,6 +1663,7 @@ export function registerScheduledTasksRoutes( mutate, safeBody, channelDeliveryAuthorizations, + sessionArchiveCoordinator: deps.sessionArchiveCoordinator, }); } @@ -1718,6 +1742,7 @@ export function registerWorkspaceQualifiedScheduledTasksRoutes( mutate, safeBody, channelDeliveryAuthorizations, + sessionArchiveCoordinator: deps.sessionArchiveCoordinator, }); } diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index a462864133b..0e949b2299d 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2614,6 +2614,7 @@ export function createServeApp( : undefined, cleanupSession, channelDeliveryAuthorizations: deps.channelDeliveryAuthorizations, + sessionArchiveCoordinator: archiveCoordinator, }); // Workspace-wide active-goal listing (the Web Shell "Goals" page). Read-only @@ -2638,6 +2639,7 @@ export function createServeApp( channelDeliveryAuthorizations: deps.channelDeliveryAuthorizations, cleanupSession, conversationRuntimeActivity, + sessionArchiveCoordinator: archiveCoordinator, }); // Read-only token-usage dashboard (Daemon Status "统计" tab). Aggregate local From d8b128bb06d43df79cefa395513c553a484ce42d Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Wed, 19 Aug 2026 13:24:16 +0800 Subject: [PATCH 08/14] fix(serve): extend scheduled-task session teardown serialization to rollback and keepalive sites (#9415 R6) --- .../cli/src/serve/routes/scheduled-tasks.ts | 43 +++++++++++++------ .../cli/src/serve/scheduled-task-keepalive.ts | 22 +++++++++- packages/cli/src/serve/server.ts | 1 + 3 files changed, 52 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index fee2dcd3e68..2601d7a6c81 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -200,17 +200,28 @@ async function rollbackCronMutation( async function teardownBoundSession( target: ScheduledTaskTarget, sessionId: string, + coordinator?: SessionArchiveCoordinator, ): Promise { - if (target.cleanupSession) { - await target.cleanupSession(sessionId).catch(() => {}); - } else if (target.bridge) { - await target.bridge.closeSession(sessionId).catch(() => {}); - const removed = await new SessionService(target.workspaceCwd, { - runtimeBaseDir: target.runtimeBaseDir, - }) - .removeSession(sessionId) - .catch(() => false); - if (removed) target.bridge.markSessionCatalogChanged?.(); + // Serialize with the reuse-create bind path on the session's key (#9415): + // a teardown from a stale snapshot must not land while a concurrent + // reuse-create holds the shared lease and is about to commit a reference. + const teardown = async (): Promise => { + if (target.cleanupSession) { + await target.cleanupSession(sessionId).catch(() => {}); + } else if (target.bridge) { + await target.bridge.closeSession(sessionId).catch(() => {}); + const removed = await new SessionService(target.workspaceCwd, { + runtimeBaseDir: target.runtimeBaseDir, + }) + .removeSession(sessionId) + .catch(() => false); + if (removed) target.bridge.markSessionCatalogChanged?.(); + } + }; + if (coordinator) { + await coordinator.runExclusiveMany([sessionId], teardown); + } else { + await teardown(); } } @@ -824,7 +835,11 @@ function registerScheduledTaskCrudRoutes( boundSessionId = session.sessionId; sessionMintedHere = true; if (!requireOpenGeneration(target, res)) { - await teardownBoundSession(target, boundSessionId); + await teardownBoundSession( + target, + boundSessionId, + sessionArchiveCoordinator, + ); return; } // Name the session after the task so it's recognizable in the session @@ -879,7 +894,11 @@ function registerScheduledTaskCrudRoutes( // open when the create fails. const rollbackSession = async () => { if (boundSessionId !== undefined && sessionMintedHere) { - await teardownBoundSession(target, boundSessionId); + await teardownBoundSession( + target, + boundSessionId, + sessionArchiveCoordinator, + ); } }; diff --git a/packages/cli/src/serve/scheduled-task-keepalive.ts b/packages/cli/src/serve/scheduled-task-keepalive.ts index 47e2ddb6cd2..bed92fe7712 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.ts @@ -42,6 +42,7 @@ import { } from '@qwen-code/qwen-code-core'; import { MAX_SESSION_RESTORE_TIMEOUT_MS } from '@qwen-code/acp-bridge/sessionRestoreTimeout'; import { scheduledTaskSessionName } from './routes/scheduled-tasks.js'; +import type { SessionArchiveCoordinator } from './server/session-archive.js'; const log = createDebugLogger('SCHED_KEEPALIVE'); @@ -136,7 +137,17 @@ async function bindAndNameSessions( spawnTimeoutMs: number, binding: Set, cleanupSession: (sessionId: string) => Promise, + sessionArchiveCoordinator?: SessionArchiveCoordinator, ): Promise { + // Serialize teardown with the reuse-create bind path (#9415): a stale + // teardown must not land while a concurrent reuse-create holds the shared + // lease and is about to commit a reference to the session. + const teardownSession = (sessionId: string): Promise => + sessionArchiveCoordinator + ? sessionArchiveCoordinator.runExclusiveMany([sessionId], () => + cleanupSession(sessionId), + ) + : cleanupSession(sessionId); const unbound = tasks.filter( (t) => !t.sessionId && @@ -175,7 +186,7 @@ async function bindAndNameSessions( task.id, sessionId, ); - await cleanupSession(sessionId).catch(() => {}); + await teardownSession(sessionId).catch(() => {}); } }) .catch(() => {}) @@ -275,7 +286,7 @@ async function bindAndNameSessions( } catch (err) { log.debug('keepalive: failed to bind task', task.id, err); if (spawnedSessionId !== undefined) { - await cleanupSession(spawnedSessionId).catch(() => {}); + await teardownSession(spawnedSessionId).catch(() => {}); } } } @@ -312,6 +323,12 @@ export interface StartScheduledTaskKeepaliveOptions { /** Per-task spawn timeout; defaults to KEEPALIVE_SPAWN_TIMEOUT_MS. */ spawnTimeoutMs?: number; onTasksRead?: (tasks: readonly DurableCronTask[]) => void; + /** + * Session-scoped serialization shared with the scheduled-tasks bind path + * (#9415): keepalive teardown acquires the exclusive lease so a concurrent + * reuse-create holding the shared lease is never torn down under it. + */ + sessionArchiveCoordinator?: SessionArchiveCoordinator; } export function startScheduledTaskKeepalive( @@ -449,6 +466,7 @@ export function startScheduledTaskKeepalive( spawnTimeoutMs, binding, cleanupSession, + opts.sessionArchiveCoordinator, ); }; const tick = (): Promise => diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 0e949b2299d..77bd9eb87eb 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2732,6 +2732,7 @@ export function createServeApp( cleanupSession: (sessionId) => cleanupSession(runtime, sessionId), onTasksRead: (tasks) => registerScheduledTaskAuthorizations(runtime.workspaceCwd, tasks), + sessionArchiveCoordinator: archiveCoordinator, }); rehydrateWorkspace(runtime); keepaliveStops.set(runtime.workspaceCwd, keepalive.stop); From 8b9116f17cfe117477203b8a27616687d869934a Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 19 Aug 2026 22:37:40 +0800 Subject: [PATCH 09/14] fix(scheduled-tasks): narrow existing session reuse --- .../src/serve/routes/scheduled-tasks.test.ts | 843 ++++-------------- .../cli/src/serve/routes/scheduled-tasks.ts | 545 +++-------- .../serve/scheduled-task-keepalive.test.ts | 110 +-- .../cli/src/serve/scheduled-task-keepalive.ts | 96 +- packages/cli/src/serve/server.ts | 4 +- .../core/src/services/cronTasksFile.test.ts | 13 +- packages/core/src/services/cronTasksFile.ts | 19 +- packages/webui/src/daemon/workspace/types.ts | 6 +- 8 files changed, 335 insertions(+), 1301 deletions(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index d476cda1881..3e3a115884a 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -11,7 +11,6 @@ import * as os from 'node:os'; import * as path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import request from 'supertest'; -import * as core from '@qwen-code/qwen-code-core'; import { SessionService, Storage, @@ -38,21 +37,11 @@ function safeBody(req: Request): Record { : {}; } -// Caller-supplied session ids must match the UUID grammar enforced by -// parseCallerSuppliedSessionId (shared with every other caller-id surface), -// so the stub's live sessions are keyed by valid UUIDs rather than freeform -// labels. const CALLER_SESSION_ID = '10000000-0000-4000-8000-000000000001'; const MISSING_SESSION_ID = '10000000-0000-4000-8000-000000000002'; const OTHER_SESSION_ID = '10000000-0000-4000-8000-000000000003'; const BUSY_SESSION_ID = '10000000-0000-4000-8000-000000000004'; -const ARCHIVED_SESSION_ID = '10000000-0000-4000-8000-000000000005'; -const SECONDARY_SESSION_ID = '10000000-0000-4000-8000-000000000006'; -const PRIMARY_SESSION_ID = '10000000-0000-4000-8000-000000000007'; -// Contains cased hex letters so uppercase/lowercase spellings actually differ -// (digit-only fixtures are byte-identical across case changes and can't pin -// case normalization). -const CASED_SESSION_ID = 'abcdef00-0000-4000-8000-000000000003'; +const SECONDARY_SESSION_ID = '10000000-0000-4000-8000-000000000005'; /** Stub session bridge: mints sequential fake session ids and records spawns / * closes so tests can assert binding and rollback without a real child. */ @@ -72,14 +61,15 @@ interface StubBridge { sessionId: string; workspaceCwd: string; hasActivePrompt: boolean; + sourceType?: string; }; - /** The sessions the stub reports as live (for getSessionSummary). */ liveSessions: Map< string, { sessionId: string; workspaceCwd: string; hasActivePrompt: boolean; + sourceType?: string; } >; markSessionCatalogChanged: ReturnType; @@ -114,10 +104,17 @@ function makeStubBridge(): StubBridge { ...(req.sourceType !== undefined ? { sourceType: req.sourceType } : {}), ...(req.sourceId !== undefined ? { sourceId: req.sourceId } : {}), }); + bridge.liveSessions.set(sessionId, { + sessionId, + workspaceCwd: req.workspaceCwd, + hasActivePrompt: false, + ...(req.sourceType !== undefined ? { sourceType: req.sourceType } : {}), + }); return { sessionId }; }, async closeSession(sessionId: string) { bridge.closed.push(sessionId); + bridge.liveSessions.delete(sessionId); return undefined; }, updateSessionMetadata(sessionId, metadata) { @@ -133,6 +130,20 @@ function makeStubBridge(): StubBridge { return bridge; } +function addLiveSession( + bridge: StubBridge, + sessionId: string, + workspaceCwd: string, + options: { busy?: boolean; sourceType?: string } = {}, +): void { + bridge.liveSessions.set(sessionId, { + sessionId, + workspaceCwd, + hasActivePrompt: options.busy === true, + ...(options.sourceType ? { sourceType: options.sourceType } : {}), + }); +} + interface Harness { app: express.Application; scratch: string; @@ -202,28 +213,6 @@ async function teardown(h: Harness): Promise { await fsp.rm(h.scratch, { recursive: true, force: true }); } -/** Writes a minimal persisted session file for `sessionId` in the given - * archive state so the route's disk-location probe can classify it. Used by - * the not-found fallback tests, where the (stub) bridge reports no live - * session and only the on-disk state decides the response. */ -async function writePersistedSession( - h: Harness, - sessionId: string, - state: 'active' | 'archived', -): Promise { - const dir = path.join( - new Storage(h.workspace, h.scratch).getProjectDir(), - 'chats', - ...(state === 'archived' ? ['archive'] : []), - ); - await fsp.mkdir(dir, { recursive: true }); - await fsp.writeFile( - path.join(dir, `${sessionId}.jsonl`), - `${JSON.stringify({ cwd: h.workspace })}\n`, - 'utf8', - ); -} - function closeGenerationDuringCronCommit(): WorkspaceRuntime['generationGuard'] { let open = true; let checks = 0; @@ -641,7 +630,7 @@ describe('scheduled-tasks routes', () => { expect(liveBridge.spawned).toEqual([]); }); - it('creates an UNBOUND task (no session) when no bridge is provided', async () => { + it('creates an unbound task without a bridge but rejects requested binding', async () => { // Mirrors createServeApp passing no bridge when resident task-session // management is off: binding a task to a session nothing keeps resident / // reloads would leave it dormant, so those callers get unbound tasks. @@ -659,160 +648,90 @@ describe('scheduled-tasks routes', () => { expect(res.status).toBe(201); expect(res.body.sessionId).toBeNull(); // unbound — fires via shared owner expect(h.bridge.spawned).toEqual([]); // nothing was spawned - }); - - // ── Caller-provided sessionId (reuse an existing session) ──────────── - it('reuses a caller-provided session instead of minting one', async () => { - h.bridge.liveSessions.set(CALLER_SESSION_ID, { + const rejected = await request(app).post('/scheduled-tasks').send({ + cron: '0 10 * * *', + prompt: 'p', sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, }); + expect(rejected.status).toBe(409); + expect(rejected.body.code).toBe('session_binding_unavailable'); + }); + + it('reuses a caller-owned session without minting or renaming it', async () => { + addLiveSession(h.bridge, CALLER_SESSION_ID, h.workspace); + const res = await create({ name: 'Digest', cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID, }); + expect(res.status).toBe(201); expect(res.body.sessionId).toBe(CALLER_SESSION_ID); - // No dedicated session was minted for the task. expect(h.bridge.spawned).toEqual([]); - // The reused session is named after the task like a minted one. - expect(h.bridge.named).toEqual([ - { sessionId: CALLER_SESSION_ID, displayName: '⏰ Digest' }, + expect(h.bridge.named).toEqual([]); + expect(await readCronTasks(h.workspace)).toEqual([ + expect.objectContaining({ + sessionId: CALLER_SESSION_ID, + sessionOwnedByTask: false, + }), ]); - const tasks = await readCronTasks(h.workspace); - expect(tasks).toHaveLength(1); - expect(tasks[0]?.sessionId).toBe(CALLER_SESSION_ID); - }); - it('rejects an unknown sessionId with 404 and creates nothing', async () => { - const res = await create({ - cron: '0 9 * * *', - prompt: 'p', - sessionId: MISSING_SESSION_ID, - }); - expect(res.status).toBe(404); - expect(res.body.code).toBe('session_not_found'); - expect(h.bridge.spawned).toEqual([]); - expect(await readCronTasks(h.workspace)).toEqual([]); + await request(h.app) + .patch(`/scheduled-tasks/${res.body.id}`) + .send({ name: 'Renamed task' }) + .expect(200); + expect(h.bridge.named).toEqual([]); }); - it('rejects a session that belongs to a different workspace', async () => { - h.bridge.liveSessions.set(OTHER_SESSION_ID, { - sessionId: OTHER_SESSION_ID, - workspaceCwd: path.join(h.scratch, 'some-other-workspace'), - hasActivePrompt: false, - }); - const res = await create({ + it('rejects invalid, missing, and busy caller sessions', async () => { + const invalid = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: OTHER_SESSION_ID, + sessionId: 'not-a-uuid', }); - expect(res.status).toBe(400); - expect(res.body.code).toBe('session_workspace_mismatch'); - expect(await readCronTasks(h.workspace)).toEqual([]); - }); + expect(invalid.status).toBe(400); + expect(invalid.body.code).toBe('invalid_session_id'); - it('rejects a busy session with 409 session_busy', async () => { - h.bridge.liveSessions.set(BUSY_SESSION_ID, { - sessionId: BUSY_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: true, - }); - const res = await create({ + const missing = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: BUSY_SESSION_ID, + sessionId: MISSING_SESSION_ID, }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_busy'); - expect(await readCronTasks(h.workspace)).toEqual([]); - }); + expect(missing.status).toBe(404); + expect(missing.body.code).toBe('session_not_found'); - it('rejects an archived session the bridge only reports as not-found', async () => { - // Production archiving removes the session from the live map first, so - // the bridge throws SessionNotFoundError; the route must still surface - // the documented 409 by consulting the persisted location on disk. The - // runtime-enabled harness gives the target a runtimeBaseDir, which the - // lookup needs to find the workspace's persisted sessions. - await teardown(h); - h = await makeHarness(true); - await writePersistedSession(h, ARCHIVED_SESSION_ID, 'archived'); - const res = await create({ + addLiveSession(h.bridge, BUSY_SESSION_ID, h.workspace, { busy: true }); + const busy = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: ARCHIVED_SESSION_ID, + sessionId: BUSY_SESSION_ID, }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_archived'); + expect(busy.status).toBe(409); + expect(busy.body.code).toBe('session_busy'); expect(await readCronTasks(h.workspace)).toEqual([]); }); - it('rejects a persisted-but-not-live session with 409 session_not_live', async () => { - // After a daemon restart only task-bound sessions are rehydrated, so a - // plainly persisted session is 'active' on disk but not live in the - // bridge. The probe must answer 409 session_not_live — NOT 404 — so - // clients branching on `session_not_found` are not told an existing, - // resumable session is gone. - await teardown(h); - h = await makeHarness(true); - await writePersistedSession(h, SECONDARY_SESSION_ID, 'active'); - const res = await create({ - cron: '0 9 * * *', - prompt: 'p', - sessionId: SECONDARY_SESSION_ID, + it('rejects sessions reserved for scheduled tasks', async () => { + addLiveSession(h.bridge, OTHER_SESSION_ID, h.workspace, { + sourceType: 'scheduled_task', }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_not_live'); - expect(await readCronTasks(h.workspace)).toEqual([]); - }); - it('rejects a conflicted persisted session with 409 session_conflict', async () => { - // A session file present in BOTH states classifies as 'conflict'; the - // probe maps it to the sibling `session_conflict` code instead of 404. - await teardown(h); - h = await makeHarness(true); - await writePersistedSession(h, SECONDARY_SESSION_ID, 'active'); - await writePersistedSession(h, SECONDARY_SESSION_ID, 'archived'); const res = await create({ cron: '0 9 * * *', prompt: 'p', - sessionId: SECONDARY_SESSION_ID, + sessionId: OTHER_SESSION_ID, }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_conflict'); - expect(await readCronTasks(h.workspace)).toEqual([]); - }); - it('classifies a legacy uppercase-spelled session via the case-insensitive fallback', async () => { - // Legacy CLI sessions may be persisted with `uuidgen`'s uppercase - // spelling while caller ids are canonicalized to lowercase. On a - // case-sensitive filesystem the exact-spelling probe misses the file; - // the findSessionIdIgnoringCase fallback (mirroring - // session-id-admission) must still classify it as archived rather than - // answering a false session_not_found. - await teardown(h); - h = await makeHarness(true); - await writePersistedSession(h, CASED_SESSION_ID.toUpperCase(), 'archived'); - const res = await create({ - cron: '0 9 * * *', - prompt: 'p', - sessionId: CASED_SESSION_ID, - }); expect(res.status).toBe(409); - expect(res.body.code).toBe('session_archived'); + expect(res.body.code).toBe('session_already_bound'); expect(await readCronTasks(h.workspace)).toEqual([]); }); it('rejects a session already bound to another task', async () => { - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); + addLiveSession(h.bridge, CALLER_SESSION_ID, h.workspace); await updateCronTasks(h.workspace, (tasks) => [ ...tasks, { @@ -822,429 +741,95 @@ describe('scheduled-tasks routes', () => { recurring: true, createdAt: 1_700_000_000_000, lastFiredAt: 1_700_000_000_000, - enabled: true, sessionId: CALLER_SESSION_ID, }, ]); - const res = await create({ - cron: '0 10 * * *', - prompt: 'p', - sessionId: CALLER_SESSION_ID, - }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_already_bound'); - expect(await readCronTasks(h.workspace)).toHaveLength(1); // unchanged - }); - - it('binds a session at most once across concurrent creates', async () => { - // Both pre-checks can read an empty cron file before either write lands; - // the under-write-lock duplicate check is the only guard for that race - // (updateCronTasks serializes the writers, so the second mutate re-reads - // the first create's committed task). Deleting the in-lock check turns - // the second response into a 201. - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); - const [a, b] = await Promise.all([ - create({ cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID }), - create({ cron: '0 10 * * *', prompt: 'q', sessionId: CALLER_SESSION_ID }), - ]); - const statuses = [a.status, b.status].sort(); - expect(statuses).toEqual([201, 409]); - const rejected = a.status === 409 ? a : b; - expect(rejected.body.code).toBe('session_already_bound'); - // Exactly one task on disk, bound exactly once. - const tasks = await readCronTasks(h.workspace); - expect(tasks).toHaveLength(1); - expect(tasks[0]?.sessionId).toBe(CALLER_SESSION_ID); - expect(h.bridge.closed).toEqual([]); - }); - - it('rejects a reuse create whose session is archived between validation and commit', async () => { - // The session passes the pre-lock validation (live, idle) but a - // concurrent archive/delete removes it from the live map before the cron - // write commits. The archive hook (disableTasksForSessions) only sees - // tasks already on disk, so it no-ops for this task — the under-write-lock - // re-validation is the only guard. Deleting it turns the 409 into a 201 - // bound to an archived session. - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); - let summaryCalls = 0; - const originalGetSessionSummary = h.bridge.getSessionSummary.bind(h.bridge); - h.bridge.getSessionSummary = (sessionId: string) => { - summaryCalls += 1; - if (summaryCalls > 1) { - // Simulate the archive/delete landing after the first (pre-lock) - // validation: archiving removes a session from the live map first. - throw new SessionNotFoundError(sessionId); - } - return originalGetSessionSummary(sessionId); - }; - const res = await create({ - cron: '0 9 * * *', - prompt: 'p', - sessionId: CALLER_SESSION_ID, - }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_not_live'); - expect(await readCronTasks(h.workspace)).toEqual([]); - // Caller-provided session — never torn down by this route. - expect(h.bridge.closed).toEqual([]); - expect(h.bridge.named).toEqual([]); - }); - it('returns 500 (not session_not_live) when the under-lock re-validation hits a generic error', async () => { - // Pins the under-write-lock rethrow contract: ONLY SessionNotFoundError - // maps to the 409 session_not_live branch; any other getSessionSummary - // failure is transient I/O and must abort the write as a retryable 500. - // Coercing generic errors into sessionGoneUnderLock would turn this into - // a 409 with the wrong code. - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); - let summaryCalls = 0; - const originalGetSessionSummary = h.bridge.getSessionSummary.bind(h.bridge); - h.bridge.getSessionSummary = (sessionId: string) => { - summaryCalls += 1; - if (summaryCalls > 1) { - // The pre-lock validation passes; the under-lock re-validation hits - // a generic (non-SessionNotFoundError) failure. - throw new Error('summary backend unavailable'); - } - return originalGetSessionSummary(sessionId); - }; const res = await create({ - cron: '0 9 * * *', + cron: '0 10 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID, }); - expect(res.status).toBe(500); - expect(res.body.code).toBe('scheduled_tasks_write_failed'); - expect(await readCronTasks(h.workspace)).toEqual([]); - // Caller-provided session — never torn down by this route. - expect(h.bridge.closed).toEqual([]); - }); - it('does not double-bind a just-minted session a concurrent reuse-create committed', async () => { - // Mint-vs-reuse race: a mint registers its session in the live map - // (doSpawn) BEFORE its cron write commits, so a concurrent reuse-create - // for that session passes every validation and commits first. The in-lock - // duplicate check must cover minted sessions too — deleting the check - // turns this create into a second 201 bound to the same session — and the - // rejected create must NOT tear down the session the winner now owns. - h.bridge.spawnOrAttach = async () => { - const sessionId = 'sess-contested-mint'; - h.bridge.spawned.push(sessionId); - // Simulate the concurrent reuse-create committing while this mint's - // write is still pending. - await updateCronTasks(h.workspace, (tasks) => [ - ...tasks, - { - id: 'reuse-task', - cron: '0 10 * * *', - prompt: 'q', - recurring: true, - createdAt: 1_700_000_000_000, - lastFiredAt: 1_700_000_000_000, - enabled: true, - sessionId, - sessionOwnedByTask: false, - }, - ]); - return { sessionId }; - }; - const res = await create({ cron: '0 9 * * *', prompt: 'p' }); expect(res.status).toBe(409); expect(res.body.code).toBe('session_already_bound'); - // Exactly one task on disk — the reuse winner — still bound. - const tasks = await readCronTasks(h.workspace); - expect(tasks).toHaveLength(1); - expect(tasks[0]?.id).toBe('reuse-task'); - expect(tasks[0]?.sessionId).toBe('sess-contested-mint'); - // The loser must not kill the session the winner committed to. - expect(h.bridge.closed).toEqual([]); - expect(h.cleanupSession).not.toHaveBeenCalled(); + expect(await readCronTasks(h.workspace)).toHaveLength(1); }); - it('answers session_already_bound (not max_tasks_reached) when both fire at the cap boundary', async () => { - // Pins the load-bearing ordering of the under-lock checks: the - // duplicate-binding check runs BEFORE the cap check. At the cap boundary - // both conditions are observable at once — a mint create whose - // just-minted session a concurrent reuse-create committed (as the 50th - // task) while its own write was still pending lands exactly at the cap. - // The overCap branch calls rollbackSession() while alreadyBound - // deliberately does not, so swapping the two checks would tear down the - // session the committed winner owns. Swapping them turns the response - // into max_tasks_reached and puts the contested session in `closed`. - await updateCronTasks(h.workspace, (tasks) => [ - ...tasks, - ...Array.from({ length: 49 }, (_, i) => ({ - id: `cap-task-${i}`, - cron: '0 9 * * *', - prompt: `existing ${i}`, - recurring: true, - createdAt: 1_700_000_000_000, - lastFiredAt: 1_700_000_000_000, - enabled: true, - sessionId: `cap-sess-${i}`, - })), - ]); - h.bridge.spawnOrAttach = async () => { - const sessionId = 'sess-cap-contested'; - h.bridge.spawned.push(sessionId); - // Simulate the concurrent reuse-create committing the 50th task — - // referencing the just-minted session — while this create's write is - // still pending. Under the lock, BOTH the duplicate reference and the - // cap are now observable. - await updateCronTasks(h.workspace, (tasks) => [ - ...tasks, - { - id: 'reuse-task', - cron: '0 10 * * *', - prompt: 'q', - recurring: true, - createdAt: 1_700_000_000_000, - lastFiredAt: 1_700_000_000_000, - enabled: true, - sessionId, - sessionOwnedByTask: false, - }, - ]); - return { sessionId }; - }; - const res = await create({ cron: '0 9 * * *', prompt: 'p' }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_already_bound'); - // The loser must not roll back the session the winner committed to. - expect(h.bridge.closed).toEqual([]); - expect(h.cleanupSession).not.toHaveBeenCalled(); - const tasks = await readCronTasks(h.workspace); - expect(tasks).toHaveLength(50); // at cap, unchanged - expect(tasks.find((t) => t.id === 'reuse-task')?.sessionId).toBe( - 'sess-cap-contested', - ); - }); + it('binds a caller session at most once across concurrent creates', async () => { + addLiveSession(h.bridge, CALLER_SESSION_ID, h.workspace); - it('rejects an invalid sessionId field with 400 invalid_session_id', async () => { - for (const bad of [ - 123, - true, - '', - ' ', - 'not-a-uuid', - // UUID grammar only — the agent-suffix internal form is not a valid - // CALLER-supplied id on any surface. - '10000000-0000-4000-8000-000000000001-agent-x', - ]) { - const res = await create({ + const responses = await Promise.all([ + create({ cron: '0 9 * * *', prompt: 'p', - sessionId: bad, - }); - expect(res.status).toBe(400); - expect(res.body.code).toBe('invalid_session_id'); - } - expect(await readCronTasks(h.workspace)).toEqual([]); - }); - - it('mints a dedicated session when sessionId is null', async () => { - const res = await create({ - cron: '0 9 * * *', - prompt: 'p', - sessionId: null, - }); - expect(res.status).toBe(201); - expect(h.bridge.spawned).toHaveLength(1); - expect(res.body.sessionId).toBe(h.bridge.spawned[0]); - }); - - it('normalizes a padded or mixed-case caller sessionId', async () => { - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); - const padded = await create({ - cron: '0 9 * * *', - prompt: 'p', - sessionId: ` ${CALLER_SESSION_ID} `, - }); - expect(padded.status).toBe(201); - expect(padded.body.sessionId).toBe(CALLER_SESSION_ID); - - // Positive case-normalization discriminator: a live session keyed by the - // lowercase spelling must be found — and bound with the normalized - // lowercase id — when the caller posts an uppercase spelling. A fixture - // with cased hex letters is required: digit-only ids are byte-identical - // across case changes, so they cannot catch a dropped lowercase step. - h.bridge.liveSessions.set(CASED_SESSION_ID, { - sessionId: CASED_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); - const upper = await create({ - cron: '0 10 * * *', - prompt: 'q', - sessionId: CASED_SESSION_ID.toUpperCase(), - }); - expect(upper.status).toBe(201); - expect(upper.body.sessionId).toBe(CASED_SESSION_ID); - - // A genuinely absent id still 404s (looked up by its normalized - // lowercase spelling). - const missing = await create({ - cron: '0 11 * * *', - prompt: 'r', - sessionId: MISSING_SESSION_ID, - }); - expect(missing.status).toBe(404); - expect(missing.body.code).toBe('session_not_found'); - }); + sessionId: CALLER_SESSION_ID, + }), + create({ + cron: '0 10 * * *', + prompt: 'q', + sessionId: CALLER_SESSION_ID, + }), + ]); - it('rejects sessionId when session management is unavailable (no bridge)', async () => { - const app = express(); - app.use(express.json()); - registerScheduledTasksRoutes(app, { - boundWorkspace: h.workspace, - mutate: () => (_req, _res, next) => next(), - safeBody, - // no bridge — unbound creates stay available, binding fails closed - }); - const res = await request(app) - .post('/scheduled-tasks') - .send({ cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('session_binding_unavailable'); - expect(await readCronTasks(h.workspace)).toEqual([]); + expect(responses.map((res) => res.status).sort()).toEqual([201, 409]); + expect(responses.find((res) => res.status === 409)?.body.code).toBe( + 'session_already_bound', + ); + expect(await readCronTasks(h.workspace)).toHaveLength(1); }); - it('leaves the caller-provided session open when the commit fails', async () => { - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); - // Corrupt the tasks file: the pre-check read fails (skipped), the - // authoritative write throws → 500. The caller's session must survive. + it('leaves the caller session open when the task write fails', async () => { + addLiveSession(h.bridge, CALLER_SESSION_ID, h.workspace); const file = getCronFilePath(h.workspace); await fsp.mkdir(path.dirname(file), { recursive: true }); await fsp.writeFile(file, 'CORRUPT {{{', 'utf8'); + const res = await create({ cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID, }); + expect(res.status).toBe(500); expect(res.body.code).toBe('scheduled_tasks_write_failed'); - expect(h.bridge.closed).toEqual([]); // caller session left open + expect(h.bridge.closed).toEqual([]); expect(h.cleanupSession).not.toHaveBeenCalled(); - // The ⏰ rename happens only after the write commits — a failed create - // must not leave the caller's session renamed with no owning task. - expect(h.bridge.named).toEqual([]); }); - it('keeps a committed create when the post-commit rename fails', async () => { - // Pins the documented invariant on the reuse path's post-commit rename: - // a transient updateSessionMetadata failure (catalog rebuild, bridge - // mid-restart) right after the cron write commits must NOT turn the - // successful 201 into a 500 — the task exists on disk, and a retry would - // then fail with session_already_bound for a task that was created. - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); - h.bridge.updateSessionMetadata = vi.fn(async () => { - throw new Error('metadata backend unavailable'); - }); + it('fails cleanly when the session disappears before commit', async () => { + addLiveSession(h.bridge, CALLER_SESSION_ID, h.workspace); + const getSummary = h.bridge.getSessionSummary.bind(h.bridge); + let calls = 0; + h.bridge.getSessionSummary = (sessionId) => { + calls += 1; + if (calls === 2) throw new SessionNotFoundError(sessionId); + return getSummary(sessionId); + }; + const res = await create({ cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID, }); - expect(res.status).toBe(201); - const tasks = await readCronTasks(h.workspace); - expect(tasks).toHaveLength(1); - expect(tasks[0]?.sessionId).toBe(CALLER_SESSION_ID); - expect(h.bridge.named).toEqual([]); // rename swallowed, not rethrown - }); - it('rejects over-cap creates with a caller-provided sessionId too', async () => { - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); - await updateCronTasks(h.workspace, (tasks) => [ - ...tasks, - ...Array.from({ length: 50 }, (_, i) => ({ - id: `cap-task-${i}`, - cron: '0 9 * * *', - prompt: `existing ${i}`, - recurring: true, - createdAt: 1_700_000_000_000, - lastFiredAt: 1_700_000_000_000, - enabled: true, - sessionId: `cap-sess-${i}`, - })), - ]); - const res = await create({ - cron: '0 10 * * *', - prompt: 'p', - sessionId: CALLER_SESSION_ID, - }); - expect(res.status).toBe(409); - expect(res.body.code).toBe('max_tasks_reached'); - expect(await readCronTasks(h.workspace)).toHaveLength(50); // unchanged - // The caller's session is untouched by the rejected create. + expect(res.status).toBe(404); + expect(res.body.code).toBe('session_not_found'); + expect(await readCronTasks(h.workspace)).toEqual([]); expect(h.bridge.closed).toEqual([]); - expect(h.bridge.named).toEqual([]); }); - it('returns 500 scheduled_tasks_session_failed on a generic lookup failure', async () => { + it('returns 500 when session lookup fails unexpectedly', async () => { h.bridge.getSessionSummary = () => { - throw new Error('boom'); + throw new Error('lookup failed'); }; - const res = await create({ - cron: '0 9 * * *', - prompt: 'p', - sessionId: CALLER_SESSION_ID, - }); - expect(res.status).toBe(500); - expect(res.body.code).toBe('scheduled_tasks_session_failed'); - // No side effects: nothing written, no session spawned or touched. - expect(await readCronTasks(h.workspace)).toEqual([]); - expect(h.bridge.spawned).toEqual([]); - expect(h.bridge.closed).toEqual([]); - expect(h.bridge.named).toEqual([]); - }); - it('returns 500 (not workspace mismatch) when path canonicalization hits a real I/O error', async () => { - // A two-link symlink cycle makes realpathSync.native throw ELOOP — a - // transient-I/O shaped failure canonicalizeWorkspace deliberately - // re-throws. It must surface as a retryable 500, not a misleading 400 - // mismatch. - const loopDir = path.join(h.scratch, 'loop'); - await fsp.mkdir(loopDir, { recursive: true }); - await fsp.symlink(path.join(loopDir, 'b'), path.join(loopDir, 'a')); - await fsp.symlink(path.join(loopDir, 'a'), path.join(loopDir, 'b')); - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: path.join(loopDir, 'a'), - hasActivePrompt: false, - }); const res = await create({ cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID, }); + expect(res.status).toBe(500); expect(res.body.code).toBe('scheduled_tasks_session_failed'); expect(await readCronTasks(h.workspace)).toEqual([]); @@ -1445,162 +1030,24 @@ describe('scheduled-tasks routes', () => { expect(h.bridge.closed).toEqual([created.body.sessionId]); }); - it('keeps a caller-provided session alive when its task is deleted', async () => { - h.bridge.liveSessions.set(CALLER_SESSION_ID, { - sessionId: CALLER_SESSION_ID, - workspaceCwd: h.workspace, - hasActivePrompt: false, - }); + it('keeps a caller-owned session open when its task is deleted', async () => { + addLiveSession(h.bridge, CALLER_SESSION_ID, h.workspace); const created = await create({ cron: '0 9 * * *', prompt: 'p', sessionId: CALLER_SESSION_ID, }); - expect(created.status).toBe(201); - // The create persisted that the session is NOT owned by the task. - const persisted = await readCronTasks(h.workspace); - expect(persisted[0]?.sessionOwnedByTask).toBe(false); - const del = await request(h.app).delete( + const deleted = await request(h.app).delete( `/scheduled-tasks/${created.body.id}`, ); - expect(del.status).toBe(200); + + expect(deleted.status).toBe(200); expect(await readCronTasks(h.workspace)).toEqual([]); - // The caller's pre-existing session must survive the task's deletion — - // it is the user's live working session, not the task's to tear down. expect(h.bridge.closed).toEqual([]); - }); - - it('still closes the session of a legacy bound task without the ownership marker', async () => { - // Tasks written before ownership was persisted carry no marker; every - // session bindable back then was task-minted, so delete keeps tearing - // those sessions down (backward-compatible default). - await updateCronTasks(h.workspace, (tasks) => [ - ...tasks, - { - id: 'legacytask', - cron: '0 9 * * *', - prompt: 'p', - recurring: true, - createdAt: Date.now(), - lastFiredAt: null, - sessionId: 'sess-legacy', - }, - ]); - - const del = await request(h.app).delete('/scheduled-tasks/legacytask'); - expect(del.status).toBe(200); - expect(h.bridge.closed).toEqual(['sess-legacy']); - }); - - it("does not close a deleted task's session when another committed task references it", async () => { - // DELETE captures the bound session under the lock, but a concurrent - // reuse-create can commit a binding to it right after the removal lands - // (its in-lock duplicate check legitimately passes once the old task is - // gone). The pre-close re-read must notice the surviving reference and - // skip the teardown — closing on the stale capture would kill the - // surviving task's live session. Deleting the recheck puts 'sess-shared' - // back in `closed`. - await updateCronTasks(h.workspace, (tasks) => [ - ...tasks, - { - id: 'deleted-task', - cron: '0 9 * * *', - prompt: 'p', - recurring: true, - createdAt: 1_700_000_000_000, - lastFiredAt: 1_700_000_000_000, - enabled: true, - sessionId: 'sess-shared', - sessionOwnedByTask: true, - }, - { - // The race winner: committed while DELETE's close was still pending. - id: 'surviving-task', - cron: '0 10 * * *', - prompt: 'q', - recurring: true, - createdAt: 1_700_000_000_000, - lastFiredAt: 1_700_000_000_000, - enabled: true, - sessionId: 'sess-shared', - sessionOwnedByTask: false, - }, - ]); - const del = await request(h.app).delete('/scheduled-tasks/deleted-task'); - expect(del.status).toBe(200); - expect(del.body).toEqual({ deleted: true, id: 'deleted-task' }); - expect(h.bridge.closed).toEqual([]); // surviving task keeps its session - const tasks = await readCronTasks(h.workspace); - expect(tasks).toHaveLength(1); - expect(tasks[0]?.id).toBe('surviving-task'); - }); - - it('still closes a task-minted session when the DELETE pre-close re-read fails', async () => { - // Pins the documented fallback for the pre-close re-read: when the - // post-commit re-read itself fails (transient I/O), DELETE keeps the - // historical behavior and closes the task-minted session. Letting the - // read failure escape turns the 200 into a 500; changing the fallback - // to skip the close empties `closed`. - await updateCronTasks(h.workspace, (tasks) => [ - ...tasks, - { - id: 'owned-task', - cron: '0 9 * * *', - prompt: 'p', - recurring: true, - createdAt: 1_700_000_000_000, - lastFiredAt: 1_700_000_000_000, - enabled: true, - sessionId: 'sess-owned', - sessionOwnedByTask: true, - }, - ]); - // Reject ONLY the post-commit re-read: the removal above commits through - // updateCronTasks's module-local read, which the barrel spy does not - // intercept. - const readSpy = vi.spyOn(core, 'readCronTasks').mockRejectedValue( - Object.assign(new Error('EACCES: permission denied'), { - code: 'EACCES', - }), + expect(h.bridge.getSessionSummary(CALLER_SESSION_ID).sessionId).toBe( + CALLER_SESSION_ID, ); - const del = await (async () => { - try { - return await request(h.app).delete('/scheduled-tasks/owned-task'); - } finally { - readSpy.mockRestore(); - } - })(); - expect(del.status).toBe(200); - expect(del.body).toEqual({ deleted: true, id: 'owned-task' }); - expect(h.bridge.closed).toEqual(['sess-owned']); - expect(await readCronTasks(h.workspace)).toEqual([]); - }); - - it('returns 500 (not 404) when the persisted-session probe hits a filesystem failure', async () => { - // The probe helpers rethrow non-ENOENT errors (EACCES/EIO/ESTALE). Such a - // failure is transient I/O, not "genuinely gone" — it must surface as a - // retryable 500, not a definitive 404 session_not_found. - const eacces = Object.assign(new Error('EACCES: permission denied'), { - code: 'EACCES', - }); - const probe = vi - .spyOn(SessionService.prototype, 'getSessionLocation') - .mockRejectedValue(eacces); - try { - const res = await create({ - cron: '0 9 * * *', - prompt: 'p', - sessionId: MISSING_SESSION_ID, // not live → falls through to the probe - }); - expect(res.status).toBe(500); - expect(res.body.code).toBe('scheduled_tasks_session_failed'); - expect(await readCronTasks(h.workspace)).toEqual([]); - expect(h.bridge.spawned).toEqual([]); - expect(h.bridge.closed).toEqual([]); - } finally { - probe.mockRestore(); - } }); it('preserves a missing DELETE response when no mutation committed', async () => { @@ -2744,6 +2191,22 @@ function makeStubRegistry(runtimes: QualifiedRuntime[]): WorkspaceRegistry { const found = runtimes.find((runtime) => runtime.workspaceCwd === cwd); return found ? asRuntime(found) : undefined; }, + resolveLiveSessionOwner: (sessionId: string) => { + const matches = runtimes.filter((runtime) => { + try { + runtime.bridge.getSessionSummary(sessionId); + return true; + } catch (error) { + if (error instanceof SessionNotFoundError) return false; + throw error; + } + }); + if (matches.length === 0) return { kind: 'not_found' }; + if (matches.length === 1) { + return { kind: 'found', runtime: asRuntime(matches[0]!) }; + } + return { kind: 'ambiguous', runtimes: matches.map(asRuntime) }; + }, } as unknown as WorkspaceRegistry; } @@ -2771,6 +2234,7 @@ async function makeQualifiedHarness(): Promise { const untrusted = await mkRuntime('untrusted', false); const runtimes = [primary, secondary, untrusted]; const activity = new ConversationRuntimeActivityGate(); + const workspaceRegistry = makeStubRegistry(runtimes); const app = express(); app.use(express.json()); @@ -2783,9 +2247,10 @@ async function makeQualifiedHarness(): Promise { safeBody, bridge: primary.bridge, getRuntime: () => primary as unknown as WorkspaceRuntime, + workspaceRegistry, }); registerWorkspaceQualifiedScheduledTasksRoutes(app, { - workspaceRegistry: makeStubRegistry(runtimes), + workspaceRegistry, mutate: () => (_req, _res, next) => next(), safeBody, manageScheduledTaskSessions: true, @@ -2830,12 +2295,14 @@ describe('workspace-qualified scheduled-tasks routes', () => { expect(primaryList.body.tasks).toHaveLength(0); }); - it('reuses a caller-provided session on the qualified surface and rejects cross-workspace ones', async () => { - h.secondary.bridge.liveSessions.set(SECONDARY_SESSION_ID, { - sessionId: SECONDARY_SESSION_ID, - workspaceCwd: h.secondary.workspaceCwd, - hasActivePrompt: false, - }); + it('reuses a live-conversation session on the qualified endpoint', async () => { + h.secondary.provenance = 'live-conversation'; + addLiveSession( + h.secondary.bridge, + SECONDARY_SESSION_ID, + h.secondary.workspaceCwd, + ); + const res = await request(h.app) .post(qualified(h.secondary.workspaceId)) .send({ @@ -2843,22 +2310,28 @@ describe('workspace-qualified scheduled-tasks routes', () => { prompt: 'p', sessionId: SECONDARY_SESSION_ID, }); + expect(res.status).toBe(201); expect(res.body.sessionId).toBe(SECONDARY_SESSION_ID); expect(h.secondary.bridge.spawned).toEqual([]); + }); - // A session living in the PRIMARY workspace can't be bound through the - // secondary workspace's endpoint. - h.secondary.bridge.liveSessions.set(PRIMARY_SESSION_ID, { - sessionId: PRIMARY_SESSION_ID, - workspaceCwd: h.primary.workspaceCwd, - hasActivePrompt: false, + it('rejects a foreign session on the primary endpoint', async () => { + addLiveSession( + h.secondary.bridge, + SECONDARY_SESSION_ID, + h.secondary.workspaceCwd, + ); + + const res = await request(h.app).post('/scheduled-tasks').send({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: SECONDARY_SESSION_ID, }); - const bad = await request(h.app) - .post(qualified(h.secondary.workspaceId)) - .send({ cron: '0 9 * * *', prompt: 'q', sessionId: PRIMARY_SESSION_ID }); - expect(bad.status).toBe(400); - expect(bad.body.code).toBe('session_workspace_mismatch'); + + expect(res.status).toBe(400); + expect(res.body.code).toBe('session_workspace_mismatch'); + expect(h.primary.bridge.spawned).toEqual([]); }); it('writes to the targeted workspace’s own cron file on disk', async () => { diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index 2601d7a6c81..2fa10e5f865 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -48,10 +48,8 @@ import { type CronTaskDelivery, type DurableCronTask, type CronTaskRun, - type SessionLocation, } from '@qwen-code/qwen-code-core'; import { SessionNotFoundError } from '@qwen-code/acp-bridge/bridgeErrors'; -import { canonicalizeWorkspace } from '@qwen-code/acp-bridge/workspacePaths'; import { parseCallerSuppliedSessionId } from '../../config/session-id.js'; import { writeStderrLine } from '../../utils/stdioHelpers.js'; import { isChannelDeliveryError } from '../../runtime/channel-delivery-ipc.js'; @@ -60,7 +58,6 @@ import { type PublicChannelDelivery, } from '../../runtime/channel-delivery.js'; import type { ChannelDeliveryAuthorizationStore } from '../channel-delivery-authorization.js'; -import type { SessionArchiveCoordinator } from '../server/session-archive.js'; import type { WorkspaceRegistry, WorkspaceRuntime, @@ -70,6 +67,7 @@ import { resolveWorkspaceRuntimeWithLiveCompatibilityFromParam, sendConversationRuntimeUnavailable, sendGenerationClosedError, + sendWorkspaceRuntimeUnavailable, } from '../workspace-route-runtime.js'; import type { ConversationRuntimeActivityGate } from '../conversations/conversation-runtime-activity.js'; @@ -83,9 +81,8 @@ const MAX_NAME_LENGTH = 200; const MAX_CRON_LENGTH = 200; /** - * The slice of the session bridge this route needs: mint a task's dedicated - * session, and tear it back down if the create fails after minting. Narrowed - * to a structural type so tests can stub it without the full bridge. + * The slice of the session bridge this route needs. Narrowed to a structural + * type so tests can stub it without the full bridge. */ export interface ScheduledTasksSessionBridge { spawnOrAttach(req: { @@ -106,15 +103,10 @@ export interface ScheduledTasksSessionBridge { sessionId: string, metadata: { displayName?: string }, ): unknown; - /** Live summary for one session by id. Throws `SessionNotFoundError` when - * no live session with that id exists on this daemon. Used to validate a - * caller-provided session (workspace, idle) before binding it to a new - * task. Archiving removes a session from the live map, so archived (and - * otherwise persisted-but-not-live) ids surface as `SessionNotFoundError` - * and are classified by the route's on-disk location probe instead. */ getSessionSummary(sessionId: string): { workspaceCwd: string; hasActivePrompt: boolean; + sourceType?: string; }; } @@ -164,6 +156,7 @@ interface ScheduledTaskTarget { cleanupSession?: (sessionId: string) => Promise; assertGenerationOpen?: () => void; activity?: ConversationRuntimeActivityGate; + resolveLiveSessionOwner?: WorkspaceRegistry['resolveLiveSessionOwner']; } function requireOpenGeneration( @@ -200,28 +193,17 @@ async function rollbackCronMutation( async function teardownBoundSession( target: ScheduledTaskTarget, sessionId: string, - coordinator?: SessionArchiveCoordinator, ): Promise { - // Serialize with the reuse-create bind path on the session's key (#9415): - // a teardown from a stale snapshot must not land while a concurrent - // reuse-create holds the shared lease and is about to commit a reference. - const teardown = async (): Promise => { - if (target.cleanupSession) { - await target.cleanupSession(sessionId).catch(() => {}); - } else if (target.bridge) { - await target.bridge.closeSession(sessionId).catch(() => {}); - const removed = await new SessionService(target.workspaceCwd, { - runtimeBaseDir: target.runtimeBaseDir, - }) - .removeSession(sessionId) - .catch(() => false); - if (removed) target.bridge.markSessionCatalogChanged?.(); - } - }; - if (coordinator) { - await coordinator.runExclusiveMany([sessionId], teardown); - } else { - await teardown(); + if (target.cleanupSession) { + await target.cleanupSession(sessionId).catch(() => {}); + } else if (target.bridge) { + await target.bridge.closeSession(sessionId).catch(() => {}); + const removed = await new SessionService(target.workspaceCwd, { + runtimeBaseDir: target.runtimeBaseDir, + }) + .removeSession(sessionId) + .catch(() => false); + if (removed) target.bridge.markSessionCatalogChanged?.(); } } @@ -243,12 +225,6 @@ interface RegisterScheduledTaskCrudRoutesDeps { mutate: (opts?: { strict?: boolean }) => RequestHandler; safeBody: (req: Request) => Record; channelDeliveryAuthorizations?: ChannelDeliveryAuthorizationStore; - /** - * Session-scoped serialization shared with the bind path: DELETE teardown - * and reuse-create binding acquire the same per-session lease so a close - * cannot land between a surviving task's validation and its commit (#9415). - */ - sessionArchiveCoordinator?: SessionArchiveCoordinator; } interface RegisterScheduledTasksRoutesDeps { @@ -256,9 +232,8 @@ interface RegisterScheduledTasksRoutesDeps { mutate: (opts?: { strict?: boolean }) => RequestHandler; safeBody: (req: Request) => Record; /** - * Session bridge used to mint a dedicated session per task. When absent - * (e.g. a minimal embedding), tasks are created without a bound session and - * fall back to the shared per-project durable-owner firing model. + * Session bridge used to mint or validate a task session. When absent, + * creates without `sessionId` remain unbound. */ bridge?: ScheduledTasksSessionBridge; channelDeliveryAuthorizations?: ChannelDeliveryAuthorizationStore; @@ -267,7 +242,7 @@ interface RegisterScheduledTasksRoutesDeps { runtime: WorkspaceRuntime, sessionId: string, ) => Promise; - sessionArchiveCoordinator?: SessionArchiveCoordinator; + workspaceRegistry?: WorkspaceRegistry; } interface RegisterWorkspaceQualifiedScheduledTasksRoutesDeps { @@ -289,7 +264,6 @@ interface RegisterWorkspaceQualifiedScheduledTasksRoutesDeps { sessionId: string, ) => Promise; conversationRuntimeActivity?: ConversationRuntimeActivityGate; - sessionArchiveCoordinator?: SessionArchiveCoordinator; } async function runWithScheduledTaskTarget( @@ -430,7 +404,6 @@ function registerScheduledTaskCrudRoutes( mutate, safeBody, channelDeliveryAuthorizations, - sessionArchiveCoordinator, } = deps; const base = `${prefix}/scheduled-tasks`; @@ -560,14 +533,19 @@ function registerScheduledTaskCrudRoutes( }); return; } - const sessionIdResult = parseSessionIdField(body['sessionId']); - if (sessionIdResult.error) { - res - .status(400) - .json({ error: sessionIdResult.error, code: 'invalid_session_id' }); + const parsedSessionId = parseCallerSuppliedSessionId(body['sessionId']); + if (parsedSessionId.kind === 'invalid') { + res.status(400).json({ + error: + '`sessionId` must be an RFC UUID v1-v5 (e.g. "550e8400-e29b-41d4-a716-446655440000")', + code: 'invalid_session_id', + }); return; } - const providedSessionId = sessionIdResult.value; + const providedSessionId = + parsedSessionId.kind === 'valid' + ? parsedSessionId.sessionId + : undefined; let delivery: PublicChannelDelivery | undefined; if (body['delivery'] !== undefined) { try { @@ -587,240 +565,102 @@ function registerScheduledTaskCrudRoutes( const enabled = body['enabled'] !== false; const taskId = generateCronTaskId(); - // Bind the task's session up front. The task is BOUND to it and fires - // only inside it — its transcript becomes the task's run history, and - // archiving/deleting the session stops the task. Done before the write - // so a task never lands on disk without its session; if the bridge is - // absent (minimal embedding) the task is created unbound (shared-owner - // firing). - // - // Two binding modes: - // - no `sessionId` in the body: mint a DEDICATED session (the original - // behavior), torn back down if the create can't be committed; - // - `sessionId` provided: REUSE that existing session after validating - // it (live in this workspace, idle, not archived, not already bound - // to another task). It pre-existed the task, so a failed create must - // leave it open; after a successful create it follows the regular - // scheduled-task session lifecycle. - // - // `sessionScope: 'thread'` is REQUIRED for the mint path: the daemon's - // default scope is 'single', which would attach to (and reuse) the - // shared workspace session instead of minting a fresh one. Two tasks — - // or a task and an open chat — would then bind to the same session: the - // task renames it, scheduled runs land in the wrong transcript, and - // deleting one task closes the shared session. Forcing 'thread' - // guarantees each minted task session is isolated. let boundSessionId: string | undefined; - // True only when THIS route minted the bound session (and must tear it - // back down if the create fails). False for a caller-provided session. let sessionMintedHere = false; - // Best-effort ⏰ rename shared by both binding modes — the mint path - // calls it before the cron write, the reuse path strictly after commit - // (that timing difference is the intentional part and stays at the - // call sites). One copy so the naming payload can't drift between - // minted and reused task sessions. - const nameBoundSession = async () => { - if (!bridge) return; - try { - await runWithScheduledTaskTarget(target, async () => - bridge.updateSessionMetadata(boundSessionId!, { - displayName: scheduledTaskSessionName(nameResult.value ?? prompt), - }), - ); - } catch { - // metadata update is non-critical — a rename failure must not fail - // the create; the keepalive names bound sessions anyway. - } - }; if (providedSessionId !== undefined && !bridge) { - // Fail closed: silently creating an UNBOUND task would give the caller - // a materially different task from the one it asked for. res.status(409).json({ - error: - 'Session management is not available for this workspace; omit `sessionId` to create an unbound task', + error: 'Session management is not available for this workspace', code: 'session_binding_unavailable', }); return; } if (bridge) { if (providedSessionId !== undefined) { - // Validate the caller's session BEFORE any write. - // Two lookup failures share one response shape; keep the body in - // one place so the copies can't drift. - const sendSessionLookupFailed = (detail: string, err: unknown) => { - writeStderrLine( - `qwen serve: POST ${base} ${detail} '${providedSessionId}': ${err instanceof Error ? err.message : String(err)}`, - ); - res.status(500).json({ - error: 'Failed to look up the requested session', - code: 'scheduled_tasks_session_failed', - }); - }; - let summary: { - workspaceCwd: string; - hasActivePrompt: boolean; - }; try { - summary = await runWithScheduledTaskTarget(target, () => - bridge.getSessionSummary(providedSessionId), - ); + const owner = target.resolveLiveSessionOwner?.(providedSessionId); + if (owner?.kind === 'unavailable') { + sendWorkspaceRuntimeUnavailable(res); + return; + } + if (owner?.kind === 'ambiguous') { + res.status(500).json({ + error: `Session owner is ambiguous for "${providedSessionId}"`, + code: 'ambiguous_session_owner', + }); + return; + } + if ( + owner?.kind === 'found' && + owner.runtime.workspaceCwd !== workspaceCwd + ) { + res.status(400).json({ + error: + "The requested session belongs to a different workspace; use that workspace's scheduled-task endpoint", + code: 'session_workspace_mismatch', + }); + return; + } + const summary = bridge.getSessionSummary(providedSessionId); + if (summary.workspaceCwd !== workspaceCwd) { + res.status(400).json({ + error: + "The requested session belongs to a different workspace; use that workspace's scheduled-task endpoint", + code: 'session_workspace_mismatch', + }); + return; + } + if (summary.hasActivePrompt) { + res.status(409).json({ + error: + 'The requested session is busy; wait for its active prompt to finish before binding it to a task', + code: 'session_busy', + }); + return; + } + if (summary.sourceType === 'scheduled_task') { + res.status(409).json({ + error: + 'The requested session is already reserved for a scheduled task', + code: 'session_already_bound', + }); + return; + } } catch (err) { if (err instanceof SessionNotFoundError) { - // Archiving removes a session from the live map first, and - // persisted-but-not-live sessions (the routine state after a - // daemon restart or idle reaping) are absent from it too. - // Probe the persisted location so every on-disk state reaches - // the caller as a machine-actionable classification; only an - // id with nothing on disk is genuinely gone (404). - const sessionService = new SessionService(workspaceCwd, { - runtimeBaseDir: target.runtimeBaseDir, - }); - let location: SessionLocation; - try { - location = await runWithScheduledTaskTarget(target, () => - sessionService.getSessionLocation(providedSessionId), - ); - if (location === undefined) { - // Legacy CLI sessions may be persisted with an uppercase - // UUID spelling while caller ids are canonicalized to - // lowercase; mirror session-id-admission's fallback so - // those still resolve on case-sensitive filesystems. - const legacyId = await runWithScheduledTaskTarget( - target, - () => - sessionService.findSessionIdIgnoringCase( - providedSessionId, - ), - ); - if (legacyId !== undefined) { - location = await runWithScheduledTaskTarget(target, () => - sessionService.getSessionLocation(legacyId), - ); - } - } - } catch (err) { - // Both probe helpers swallow ENOENT themselves and rethrow - // every other filesystem error (EACCES/EIO/ESTALE/…), so a - // throw here is a real failure, not "genuinely gone" — answer - // a retryable 500 instead of misreporting an existing session - // as 404 session_not_found (and log it, unlike a true miss). - sendSessionLookupFailed( - 'failed to probe persisted session state', - err, - ); - return; - } - if (location === 'archived') { - res.status(409).json({ - error: - 'The requested session is archived; unarchive it before binding it to a task', - code: 'session_archived', - }); - return; - } - if (location === 'active' || location === 'conflict') { - // The session exists on disk but is not live on this daemon - // (only task-bound sessions are rehydrated at startup). - // Reserve 404 for ids that are genuinely gone so clients - // branching on `session_not_found` are not told an existing - // resumable session does not exist. - res.status(409).json({ - error: - 'The requested session is not live on this daemon; load it before binding it to a task', - code: - location === 'conflict' - ? 'session_conflict' - : 'session_not_live', - }); - return; - } res.status(404).json({ error: `Session '${providedSessionId}' was not found`, code: 'session_not_found', }); return; } - sendSessionLookupFailed('failed to look up session', err); - return; - } - let sameWorkspace = false; - try { - sameWorkspace = - canonicalizeWorkspace(summary.workspaceCwd) === - canonicalizeWorkspace(workspaceCwd); - } catch (err) { - // canonicalizeWorkspace swallows ENOENT itself; anything thrown - // here is a real filesystem failure (EACCES/EIO/ELOOP/ESTALE). - // Surface it as a retryable 500 instead of a misleading - // workspace-mismatch 400. - sendSessionLookupFailed( - 'failed to resolve workspace paths for session', - err, + writeStderrLine( + `qwen serve: POST ${base} failed to look up session '${providedSessionId}': ${err instanceof Error ? err.message : String(err)}`, ); - return; - } - if (!sameWorkspace) { - // Unreachable under production daemon wiring — one bridge serves - // exactly one workspace runtime, so a cross-workspace id throws - // SessionNotFoundError and is answered above before this runs. - // Kept as a defense for the structural bridge interface (a - // multi-workspace embedder can serve foreign sessions here). - res.status(400).json({ - error: - "The requested session belongs to a different workspace; use that workspace's scheduled-task endpoint", - code: 'session_workspace_mismatch', - }); - return; - } - if (summary.hasActivePrompt) { - res.status(409).json({ - error: - 'The requested session is busy; wait for its active prompt to finish before binding it to a task', - code: 'session_busy', + res.status(500).json({ + error: 'Failed to look up the requested session', + code: 'scheduled_tasks_session_failed', }); return; } } - // Pre-check the cap (and duplicate binding, for a caller-provided - // session) BEFORE spawning: an over-cap create must not spawn a - // session it will immediately tear down, because closeSession removes - // the live bridge entry but can leave the just-spawned+named session - // listed as an orphan with no owning task. Best-effort — the - // write-lock checks below stay authoritative for the concurrent race. + + // Best-effort pre-check; the write-lock checks below are authoritative. try { - const existingTasks = await runWithScheduledTaskTarget(target, () => + const tasks = await runWithScheduledTaskTarget(target, () => readCronTasks(workspaceCwd), ); - if (existingTasks.length >= MAX_SCHEDULED_TASKS) { + if (tasks.length >= MAX_SCHEDULED_TASKS) { res.status(409).json({ error: `Maximum number of scheduled tasks (${MAX_SCHEDULED_TASKS}) reached`, code: 'max_tasks_reached', }); return; } - if ( - providedSessionId !== undefined && - existingTasks.some((t) => t.sessionId === providedSessionId) - ) { - res.status(409).json({ - error: - 'The requested session is already bound to another scheduled task', - code: 'session_already_bound', - }); - return; - } } catch { // Read failure → skip the pre-check; the write below is authoritative. } if (!requireOpenGeneration(target, res)) return; if (providedSessionId !== undefined) { - // Reuse the caller's session — no spawn (sessionMintedHere stays - // false, so a failed create leaves it open). The ⏰ rename happens - // only AFTER the cron write commits (below): a create that fails - // after renaming would leave the caller's pre-existing session - // permanently named "⏰ …" with no owning task, because - // rollbackSession never touches caller sessions and nothing else - // restores the prior display name. boundSessionId = providedSessionId; } else { try { @@ -835,16 +675,20 @@ function registerScheduledTaskCrudRoutes( boundSessionId = session.sessionId; sessionMintedHere = true; if (!requireOpenGeneration(target, res)) { - await teardownBoundSession( - target, - boundSessionId, - sessionArchiveCoordinator, - ); + await teardownBoundSession(target, boundSessionId); return; } - // Name the session after the task so it's recognizable in the session - // list. Best-effort — a nameless session still fires correctly. - await nameBoundSession(); + try { + await runWithScheduledTaskTarget(target, async () => + bridge.updateSessionMetadata(boundSessionId!, { + displayName: scheduledTaskSessionName( + nameResult.value ?? prompt, + ), + }), + ); + } catch { + // metadata update is non-critical + } } catch (err) { if (sendActivityGateError(res, err)) return; if (sendGenerationClosedError(res, err)) return; @@ -875,10 +719,9 @@ function registerScheduledTaskCrudRoutes( ...(boundSessionId !== undefined ? { sessionId: boundSessionId, - // Persist WHO owns the bound session: DELETE may only tear down - // sessions the task itself minted — a caller-provided session - // pre-existed the task and must survive its deletion. - sessionOwnedByTask: sessionMintedHere, + ...(providedSessionId !== undefined + ? { sessionOwnedByTask: false } + : {}), } : {}), ...(nameResult.value !== undefined ? { name: nameResult.value } : {}), @@ -889,66 +732,43 @@ function registerScheduledTaskCrudRoutes( // deletes the persisted transcript/title record — both are needed, or a // rejected create (the loser of a concurrent create at the cap boundary, // which passes the pre-check but loses the authoritative write) would leave - // a named "⏰ …" session in the list with no owning task. A caller-provided - // session is NEVER torn down here — it pre-existed the task and must stay - // open when the create fails. + // a named "⏰ …" session in the list with no owning task. const rollbackSession = async () => { if (boundSessionId !== undefined && sessionMintedHere) { - await teardownBoundSession( - target, - boundSessionId, - sessionArchiveCoordinator, - ); + await teardownBoundSession(target, boundSessionId); } }; let overCap = false; let alreadyBound = false; - let sessionGoneUnderLock = false; + let sessionNoLongerLive = false; let rollbackBefore: DurableCronTask[] | undefined; let rollbackAfter: DurableCronTask[] | undefined; - // Serialize a reuse-create's commit with DELETE teardown on the reused - // session's key (#9415): while this shared lease is held, a concurrent - // DELETE cannot hold the exclusive lease and close the session under us. - const commitTask = () => - runWithScheduledTaskTarget(target, () => + try { + await runWithScheduledTaskTarget(target, () => updateCronTasks( workspaceCwd, (tasks) => { - // Same-lock duplicate-binding check in BOTH binding modes: the - // pre-check read above is best-effort, and a concurrent create - // may have bound the same session since. For a caller-provided - // session that's another reuse-create; for a just-minted one - // it's a reuse-create that committed while this request's mint - // was still in flight (the mint registers the session in the - // live map before THIS write commits, so the reuse path's - // validation can pass against it). Runs before the cap check so - // an over-cap loser never tears down a session another - // committed task already references. if ( - boundSessionId !== undefined && - tasks.some((t) => t.sessionId === boundSessionId) + providedSessionId !== undefined && + tasks.some((task) => task.sessionId === providedSessionId) ) { alreadyBound = true; return tasks; } - // Re-validate a caller-provided session UNDER the write lock: - // archiving/deleting tears the session out of the live map - // BEFORE its cron hook (disable/removeTasksForSessions) runs, - // and that hook only sees tasks already on disk — so a session - // that left the live map between the pre-lock validation and - // this cycle is being archived/deleted and its hook skipped - // this (not yet written) task. Committing anyway would bind a - // 201-returned task to an archived or gone session. Cron write - // cycles are serialized, so a hook that runs after THIS cycle - // sees the new task and disables/removes it correctly. if (providedSessionId !== undefined && bridge) { try { - bridge.getSessionSummary(providedSessionId); + if ( + bridge.getSessionSummary(providedSessionId).sourceType === + 'scheduled_task' + ) { + alreadyBound = true; + return tasks; + } } catch (err) { if (err instanceof SessionNotFoundError) { - sessionGoneUnderLock = true; - return tasks; // no write + sessionNoLongerLive = true; + return tasks; } throw err; } @@ -967,13 +787,6 @@ function registerScheduledTaskCrudRoutes( { assertCanCommit: target.assertGenerationOpen }, ), ); - try { - await (providedSessionId !== undefined && sessionArchiveCoordinator - ? sessionArchiveCoordinator.runSharedMany( - [providedSessionId], - commitTask, - ) - : commitTask()); } catch (err) { await rollbackSession(); if (sendActivityGateError(res, err)) return; @@ -1010,23 +823,7 @@ function registerScheduledTaskCrudRoutes( }); return; } - if (sessionGoneUnderLock) { - // Reuse mode only — a caller-provided session is never torn down - // here, so there is nothing to roll back. Retryable: the session's - // archive/delete completed between validation and commit. - res.status(409).json({ - error: - 'The requested session was archived or deleted while the task was being created; retry with a live session', - code: 'session_not_live', - }); - return; - } if (alreadyBound) { - // NO rollbackSession here: the in-lock check fires only when a - // COMMITTED task already references the bound session. For a - // just-minted session that means a concurrent reuse-create won the - // race and owns it — tearing it down would kill that task's session. - // (For a caller-provided session rollbackSession is a no-op anyway.) res.status(409).json({ error: 'The requested session is already bound to another scheduled task', @@ -1034,11 +831,12 @@ function registerScheduledTaskCrudRoutes( }); return; } - if (providedSessionId !== undefined && bridge) { - // Name the reused session after the task — like a minted one, but - // strictly AFTER the cron write commits, so no failure path leaves - // the caller's pre-existing session renamed with no owning task. - await nameBoundSession(); + if (sessionNoLongerLive) { + res.status(404).json({ + error: `Session '${providedSessionId}' was not found`, + code: 'session_not_found', + }); + return; } if (task.delivery && task.sessionId) { channelDeliveryAuthorizations?.registerScheduledTask(workspaceCwd, { @@ -1321,7 +1119,12 @@ function registerScheduledTaskCrudRoutes( patch.name !== undefined || clearName || (patch.prompt !== undefined && updated.name === undefined); - if (bridge && updated.sessionId && effectiveLabelChanged) { + if ( + bridge && + updated.sessionId && + updated.sessionOwnedByTask !== false && + effectiveLabelChanged + ) { try { bridge.updateSessionMetadata(updated.sessionId, { displayName: scheduledTaskSessionName( @@ -1368,9 +1171,7 @@ function registerScheduledTaskCrudRoutes( // Single atomic read-modify-write: capture the task's bound session AND // remove it in one cycle, closing the TOCTOU window a separate // read-then-remove would open (and cutting three file reads to one). A - // session the task itself minted exists only to run it, so it's torn - // down after; a caller-provided session pre-existed the task and stays - // open (the persisted sessionOwnedByTask marker tells the two apart). + // task-owned session is torn down after; a caller-owned session survives. let boundSessionId: string | undefined; let sessionOwnedByTask = true; let removed = false; @@ -1386,8 +1187,6 @@ function registerScheduledTaskCrudRoutes( const match = tasks[idx]!.sessionId; if (typeof match === 'string' && match.length > 0) { boundSessionId = match; - // Absent marker = written before ownership was persisted; every - // session bindable then was task-minted, so keep tearing down. sessionOwnedByTask = tasks[idx]!.sessionOwnedByTask !== false; } removed = true; @@ -1430,41 +1229,12 @@ function registerScheduledTaskCrudRoutes( .json({ error: 'Task not found', code: 'task_not_found' }); return; } - // Stop a task-minted session (keeps its transcript on disk as history). - // A caller-provided session is NEVER closed here — it pre-existed the - // task, may be the user's live working session, and must survive the - // task's deletion (same invariant the create path's rollback honors). + // Stop the now-orphaned session (keeps its transcript on disk as history). if (boundSessionId && sessionOwnedByTask && bridge) { - // Serialize with the reuse-create bind path on the session's own key: - // the re-read below narrows the rebind window, and the exclusive lease - // closes it — a reuse-create holds the shared lease while validating - // and committing, so this teardown cannot land in between (#9415). - const teardownSession = async (): Promise => { - let claimedBySurvivingTask = false; - try { - const currentTasks = await runWithScheduledTaskTarget(target, () => - readCronTasks(workspaceCwd), - ); - claimedBySurvivingTask = currentTasks.some( - (t) => t.sessionId === boundSessionId, - ); - } catch { - // Read failure → keep the historical behavior (close the session). - } - if (claimedBySurvivingTask) return; - return runWithScheduledTaskTarget(target, () => + try { + await runWithScheduledTaskTarget(target, () => bridge.closeSession(boundSessionId!), ); - }; - try { - if (sessionArchiveCoordinator) { - await sessionArchiveCoordinator.runExclusiveMany( - [boundSessionId], - teardownSession, - ); - } else { - await teardownSession(); - } } catch (error) { if (sendActivityGateError(res, error)) return; } @@ -1677,12 +1447,17 @@ export function registerScheduledTasksRoutes( assertGenerationOpen: () => runtime.generationGuard?.assertOpen(), } : {}), + ...(deps.workspaceRegistry + ? { + resolveLiveSessionOwner: (sessionId: string) => + deps.workspaceRegistry!.resolveLiveSessionOwner(sessionId), + } + : {}), }; }, mutate, safeBody, channelDeliveryAuthorizations, - sessionArchiveCoordinator: deps.sessionArchiveCoordinator, }); } @@ -1726,7 +1501,9 @@ export function registerWorkspaceQualifiedScheduledTasksRoutes( if ( runtime.provenance === 'live-conversation' && req.method === 'POST' && - req.params['id'] === undefined + req.params['id'] === undefined && + parseCallerSuppliedSessionId(safeBody(req)['sessionId']).kind === + 'absent' ) { res.status(400).json({ error: @@ -1756,12 +1533,13 @@ export function registerWorkspaceQualifiedScheduledTasksRoutes( assertGenerationOpen: () => runtime.generationGuard?.assertOpen(), } : {}), + resolveLiveSessionOwner: (sessionId: string) => + workspaceRegistry.resolveLiveSessionOwner(sessionId), }; }, mutate, safeBody, channelDeliveryAuthorizations, - sessionArchiveCoordinator: deps.sessionArchiveCoordinator, }); } @@ -1807,36 +1585,3 @@ function parseNameField(raw: unknown): { value?: string; error?: string } { } return { value: trimmed }; } - -/** - * Parses the optional `sessionId` field on POST (reuse an existing session - * instead of minting a dedicated one). Accepts: - * - absent / null → `{ value: undefined }` (mint a dedicated session) - * - a valid caller-supplied session id → `{ value }`, canonicalized through - * the same parser every other caller-supplied-session-id surface uses - * (`parseCallerSuppliedSessionId`: UUID grammar, case-normalized, - * length-bounded by the grammar — no unbounded echo in error bodies or - * stderr, and duplicate-binding equality holds per session, not per - * spelling) - * - anything else (including empty/whitespace-only strings — a session id - * can't be "cleared", so unlike `name` they're an error) → `{ error }` - */ -function parseSessionIdField(raw: unknown): { - value?: string; - error?: string; -} { - const parsed = parseCallerSuppliedSessionId( - typeof raw === 'string' ? raw.trim() : raw, - ); - if (parsed.kind === 'absent') return { value: undefined }; - if (parsed.kind === 'invalid') { - // Same actionable grammar hint as the sibling caller-id surfaces - // (POST /session and ACP session/new), so a malformed id gets one - // consistent, machine-translatable answer everywhere. - return { - error: - '`sessionId` must be an RFC UUID v1-v5 (e.g. "550e8400-e29b-41d4-a716-446655440000")', - }; - } - return { value: parsed.sessionId }; -} diff --git a/packages/cli/src/serve/scheduled-task-keepalive.test.ts b/packages/cli/src/serve/scheduled-task-keepalive.test.ts index 68befc1ca5e..4f6c10892ca 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.test.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.test.ts @@ -661,12 +661,8 @@ describe('scheduled-task keepalive', () => { }); it('binds an unbound task to a dedicated session and writes sessionId to disk', async () => { - // Named fixture + exact-name assertion discriminate the MINT-site naming - // payload (`task.name ?? task.prompt`): mutating it to `task.prompt` - // yields '⏰ check build' and fails this test. The already-bound rename - // branch is covered separately below. await updateCronTasks(workspace, () => [ - task({ id: 'unbound-1', prompt: 'check build', name: 'Digest' }), + task({ id: 'unbound-1', prompt: 'check build' }), ]); const spawns: unknown[] = []; const names: Array<[string, { displayName?: string }]> = []; @@ -697,24 +693,18 @@ describe('scheduled-task keepalive', () => { }); expect(names).toHaveLength(1); expect(names[0]![0]).toBe('new-sess-1'); - expect(names[0]![1].displayName).toBe('⏰ Digest'); + expect(names[0]![1].displayName).toContain('⏰'); const tasks = await readCronTasks(workspace); expect(tasks[0]!.sessionId).toBe('new-sess-1'); - // The keepalive minted this session, so the task records ownership — - // the DELETE route's gate relies on it. - expect(tasks[0]!.sessionOwnedByTask).toBe(true); }); - it('names bound sessions from the task name when one is set', async () => { - // The scheduled-tasks route names a bound session `⏰ `; - // the keepalive must use the same payload or it clobbers the route's - // name (visible on caller-provided sessions, which the route also names). + it('renames task-owned sessions once without renaming caller-owned ones', async () => { await updateCronTasks(workspace, () => [ + task({ id: 'bound-1', sessionId: 'existing-sess', prompt: 'lint' }), task({ - id: 'named-bound', - sessionId: 'existing-sess', - prompt: 'summarize the day', - name: 'Digest', + id: 'caller-bound', + sessionId: 'caller-sess', + sessionOwnedByTask: false, }), ]); const names: Array<[string, { displayName?: string }]> = []; @@ -730,28 +720,6 @@ describe('scheduled-task keepalive', () => { intervalMs: 60_000, }); await ka.tick(); - ka.stop(); - expect(names).toHaveLength(1); - expect(names[0]![1].displayName).toBe('⏰ Digest'); - }); - - it('renames a bound session without ⏰ prefix exactly once', async () => { - await updateCronTasks(workspace, () => [ - task({ id: 'bound-1', sessionId: 'existing-sess', prompt: 'lint' }), - ]); - const names: Array<[string, { displayName?: string }]> = []; - const naming = { - ...bridge, - updateSessionMetadata: (id: string, m: { displayName?: string }) => { - names.push([id, m]); - }, - }; - const ka = startScheduledTaskKeepalive({ - bridge: naming, - boundWorkspace: workspace, - intervalMs: 60_000, - }); - await ka.tick(); await ka.tick(); ka.stop(); expect(names).toHaveLength(1); @@ -887,70 +855,6 @@ describe('scheduled-task keepalive', () => { removeSpy.mockRestore(); }); - it('leaves a just-minted session alone when another committed task already references it', async () => { - // The scheduled-tasks reuse path can bind a session as soon as the spawn - // registers it in the live map — BEFORE this bind write commits. The - // in-lock check must notice the committed reference and leave the task - // unbound, not double-bind the session — but it must NOT roll the - // session back either: the committed task owns it now. In production - // wiring cleanupSession is deleteDaemonSessionIfOrphan, whose - // requireZeroAttaches passes for a just-minted session and whose - // persisted removal cascades removeTasksForSessions — a rollback here - // would kill the winner's live session AND delete the winner's task - // from the cron file. (Mirrors the route's alreadyBound branch, which - // deliberately performs no rollbackSession.) Reverting the fix puts - // 'contested-sess' back in `closed` and calls removeSession for it. - const closed: string[] = []; - const removeSpy = vi - .spyOn(SessionService.prototype, 'removeSession') - .mockResolvedValue(true); - const raceBridge = { - ...bridge, - spawnOrAttach: async () => { - // Simulate a concurrent caller-provided binding committing while our - // spawn is in flight. - await updateCronTasks(workspace, (list) => [ - ...list, - task({ - id: 'caller-task', - sessionId: 'contested-sess', - sessionOwnedByTask: false, - }), - ]); - return { sessionId: 'contested-sess' }; - }, - closeSession: async (id: string) => { - closed.push(id); - }, - markSessionCatalogChanged: vi.fn(), - updateSessionMetadata: () => {}, - }; - await updateCronTasks(workspace, () => [ - task({ id: 'tool-task', prompt: 'contested' }), - ]); - const ka = startScheduledTaskKeepalive({ - bridge: raceBridge, - boundWorkspace: workspace, - intervalMs: 60_000, - }); - await ka.tick(); - ka.stop(); - // No rollback: the winner's session survives untouched... - expect(closed).toEqual([]); - expect(removeSpy).not.toHaveBeenCalled(); - expect(raceBridge.markSessionCatalogChanged).not.toHaveBeenCalled(); - // ...and the session stays bound to exactly ONE task: the caller's, - // while THIS task remains unbound for a later tick to retry with a - // fresh session. - const tasks = await readCronTasks(workspace); - expect(tasks).toHaveLength(2); - const toolTask = tasks.find((t) => t.id === 'tool-task'); - const callerTask = tasks.find((t) => t.id === 'caller-task'); - expect(toolTask?.sessionId).toBeUndefined(); // still unbound - expect(callerTask?.sessionId).toBe('contested-sess'); - removeSpy.mockRestore(); - }); - it('a hung spawnOrAttach does not stall subsequent ticks', async () => { // spawnOrAttach is not abortable — if it hangs, the keepalive must time // out and move on so later ticks can still heartbeat/revive other diff --git a/packages/cli/src/serve/scheduled-task-keepalive.ts b/packages/cli/src/serve/scheduled-task-keepalive.ts index bed92fe7712..53e888f3af7 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.ts @@ -5,11 +5,11 @@ */ /** - * Keeps scheduled-task-owned sessions resident against the bridge's idle + * Keeps sessions bound to scheduled tasks resident against the bridge's idle * reaper. * * A durable task created through the Web Shell management page is bound to a - * dedicated session and fires ONLY inside it (its transcript is the task's run + * session and fires ONLY inside it (its transcript is the task's run * history). For that to keep happening the session must stay loaded so its * in-child scheduler ticks — but a session with no client / SSE subscriber is * closed by the bridge's idle reaper after the idle timeout, which would @@ -42,7 +42,6 @@ import { } from '@qwen-code/qwen-code-core'; import { MAX_SESSION_RESTORE_TIMEOUT_MS } from '@qwen-code/acp-bridge/sessionRestoreTimeout'; import { scheduledTaskSessionName } from './routes/scheduled-tasks.js'; -import type { SessionArchiveCoordinator } from './server/session-archive.js'; const log = createDebugLogger('SCHED_KEEPALIVE'); @@ -113,18 +112,15 @@ const KEEPALIVE_SPAWN_TIMEOUT_MS = 30_000; const MAX_REVIVE_BACKOFF_MS = 30 * 60_000; /** - * Bind unbound durable tasks to dedicated sessions, and (re)name bound - * sessions. The cron_create tool leaves durable tasks unbound so they stay - * pickable by any lock owner (CLI/ACP/headless). In daemon mode this - * keepalive mints a dedicated session per task and names it — binding is a - * daemon-only concern. + * Bind unbound durable tasks to dedicated sessions, and rename bound + * task-owned sessions that don't yet have the ⏰ prefix. The cron_create tool leaves + * durable tasks unbound so they stay pickable by any lock owner (CLI/ACP + * /headless). In daemon mode this keepalive mints a dedicated session per + * task and names it — binding is a daemon-only concern. * - * For unbound tasks: mints a dedicated session, names it `⏰ `, writes sessionId to disk. - * For bound tasks: renames the session to `⏰ ` — the SAME - * payload the scheduled-tasks route names with, so the route's naming and - * this sweep agree instead of clobbering each other with different names - * (matters for caller-provided sessions, which the route also names). + * For unbound tasks: mints a dedicated session, names it `⏰ prompt`, + * writes sessionId to disk. + * For task-owned bound tasks without ⏰ name: renames the session to `⏰ prompt`. * * A Set tracks renamed sessions so we don't call updateSessionMetadata * every tick. Best-effort — failures are logged and retried next tick. @@ -137,17 +133,7 @@ async function bindAndNameSessions( spawnTimeoutMs: number, binding: Set, cleanupSession: (sessionId: string) => Promise, - sessionArchiveCoordinator?: SessionArchiveCoordinator, ): Promise { - // Serialize teardown with the reuse-create bind path (#9415): a stale - // teardown must not land while a concurrent reuse-create holds the shared - // lease and is about to commit a reference to the session. - const teardownSession = (sessionId: string): Promise => - sessionArchiveCoordinator - ? sessionArchiveCoordinator.runExclusiveMany([sessionId], () => - cleanupSession(sessionId), - ) - : cleanupSession(sessionId); const unbound = tasks.filter( (t) => !t.sessionId && @@ -158,6 +144,7 @@ async function bindAndNameSessions( const needsName = tasks.filter( (t) => t.sessionId && + t.sessionOwnedByTask !== false && t.enabled !== false && !taskHasLegacyCondition(t) && !renamed.has(t.sessionId), @@ -186,7 +173,7 @@ async function bindAndNameSessions( task.id, sessionId, ); - await teardownSession(sessionId).catch(() => {}); + await cleanupSession(sessionId).catch(() => {}); } }) .catch(() => {}) @@ -204,31 +191,14 @@ async function bindAndNameSessions( spawnedSessionId = sessionId; try { bridge.updateSessionMetadata(sessionId, { - displayName: scheduledTaskSessionName(task.name ?? task.prompt), + displayName: scheduledTaskSessionName(task.prompt), }); renamed.add(sessionId); } catch { // naming is non-critical — the session still fires correctly } let matched = false; - // The two no-write reasons must stay distinguishable: when a COMMITTED - // task already references the just-minted session the session is NOT an - // orphan and must not be rolled back (see below); when the task itself - // is no longer bindable the session IS orphaned and rolls back. - let sessionClaimedByCommittedTask = false; await updateCronTasks(boundWorkspace, (list) => { - // Bail when ANY committed task already references the just-minted - // session: the scheduled-tasks reuse path can bind a session the - // moment the spawn above registers it in the live map, BEFORE this - // write commits — without this check the session would be bound to - // two tasks (same transcript, conflicting ⏰ renames), and a later - // delete of THIS task would close the session out from under the - // surviving one. Checked FIRST: a session a committed task references - // must never be torn down, whatever this task's own state. - if (list.some((t) => t.sessionId === sessionId)) { - sessionClaimedByCommittedTask = true; - return list; - } // Another process may have bound or disabled this task between our // read and this write-lock acquisition — only attach when the task is // still unbound and enabled. Otherwise return unchanged so the @@ -242,38 +212,13 @@ async function bindAndNameSessions( } const result = list.map((t) => t.id === task.id && !t.sessionId && t.enabled !== false - ? { - ...t, - sessionId, - // The keepalive minted this session, so deleting the task - // later may tear it down (see the DELETE route's gate). - sessionOwnedByTask: true, - } + ? { ...t, sessionId } : t, ); matched = true; return result; }); if (!matched) { - if (sessionClaimedByCommittedTask) { - // A concurrent create committed a reference to the just-minted - // session before this write ran — it owns the session now (the - // route's symmetric `alreadyBound` branch performs NO rollback for - // the same reason). Rolling back here would kill the winner's live - // session: in production wiring cleanupSession is - // deleteDaemonSessionIfOrphan, whose requireZeroAttaches passes - // for a just-minted session, and whose persisted removal cascades - // removeTasksForSessions — deleting the winner's committed task. - // Leave the session to its owner; THIS task stays unbound on - // disk and a later tick retries it with a fresh session. - log.debug( - 'keepalive: session', - sessionId, - 'already committed to another task — leaving it to its owner', - task.id, - ); - continue; - } // Task was deleted between read and write — roll back the orphan. throw new Error(`task ${task.id} no longer on disk`); } @@ -286,7 +231,7 @@ async function bindAndNameSessions( } catch (err) { log.debug('keepalive: failed to bind task', task.id, err); if (spawnedSessionId !== undefined) { - await teardownSession(spawnedSessionId).catch(() => {}); + await cleanupSession(spawnedSessionId).catch(() => {}); } } } @@ -295,7 +240,7 @@ async function bindAndNameSessions( const sessionId = task.sessionId!; try { bridge.updateSessionMetadata(sessionId, { - displayName: scheduledTaskSessionName(task.name ?? task.prompt), + displayName: scheduledTaskSessionName(task.prompt), }); renamed.add(sessionId); } catch (err) { @@ -323,12 +268,6 @@ export interface StartScheduledTaskKeepaliveOptions { /** Per-task spawn timeout; defaults to KEEPALIVE_SPAWN_TIMEOUT_MS. */ spawnTimeoutMs?: number; onTasksRead?: (tasks: readonly DurableCronTask[]) => void; - /** - * Session-scoped serialization shared with the scheduled-tasks bind path - * (#9415): keepalive teardown acquires the exclusive lease so a concurrent - * reuse-create holding the shared lease is never torn down under it. - */ - sessionArchiveCoordinator?: SessionArchiveCoordinator; } export function startScheduledTaskKeepalive( @@ -466,7 +405,6 @@ export function startScheduledTaskKeepalive( spawnTimeoutMs, binding, cleanupSession, - opts.sessionArchiveCoordinator, ); }; const tick = (): Promise => @@ -561,7 +499,7 @@ export interface RehydrateResult { } /** - * Reloads every scheduled-task-owned session at daemon startup so its in-child + * Reloads every scheduled-task-bound session at daemon startup so its in-child * scheduler re-arms after a restart — nothing rehydrates sessions on boot * otherwise, so a bound task would sit dormant (its bound session dead, and the * lock owner deliberately never fires a bound task) until something loaded it. diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 77bd9eb87eb..daddcabd4c8 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2613,8 +2613,8 @@ export function createServeApp( ? workspaceRegistry.primaryEntry.current?.runtime : undefined, cleanupSession, + workspaceRegistry, channelDeliveryAuthorizations: deps.channelDeliveryAuthorizations, - sessionArchiveCoordinator: archiveCoordinator, }); // Workspace-wide active-goal listing (the Web Shell "Goals" page). Read-only @@ -2639,7 +2639,6 @@ export function createServeApp( channelDeliveryAuthorizations: deps.channelDeliveryAuthorizations, cleanupSession, conversationRuntimeActivity, - sessionArchiveCoordinator: archiveCoordinator, }); // Read-only token-usage dashboard (Daemon Status "统计" tab). Aggregate local @@ -2732,7 +2731,6 @@ export function createServeApp( cleanupSession: (sessionId) => cleanupSession(runtime, sessionId), onTasksRead: (tasks) => registerScheduledTaskAuthorizations(runtime.workspaceCwd, tasks), - sessionArchiveCoordinator: archiveCoordinator, }); rehydrateWorkspace(runtime); keepaliveStops.set(runtime.workspaceCwd, keepalive.stop); diff --git a/packages/core/src/services/cronTasksFile.test.ts b/packages/core/src/services/cronTasksFile.test.ts index 575052591b7..738427e7135 100644 --- a/packages/core/src/services/cronTasksFile.test.ts +++ b/packages/core/src/services/cronTasksFile.test.ts @@ -215,11 +215,7 @@ describe('cronTasksFile', () => { await expect(readCronTasks(tmpDir)).rejects.toThrow(/Invalid task entry/); }); - it('rejects a task whose sessionOwnedByTask is not a boolean', async () => { - // The marker decides whether DELETE tears the bound session down, so a - // hand-edited/corrupted file carrying garbage here must fail fast like - // every sibling optional field — not load silently. Deleting the - // validation branch keeps this test red. + it('rejects a non-boolean session ownership marker', async () => { await seedTasksFile( tmpDir, JSON.stringify([ @@ -229,13 +225,6 @@ describe('cronTasksFile', () => { await expect(readCronTasks(tmpDir)).rejects.toThrow(/Invalid task entry/); }); - it('round-trips the optional sessionOwnedByTask field', async () => { - const task = makeTask({ sessionId: 'sess-1', sessionOwnedByTask: true }); - await writeCronTasks(tmpDir, [task]); - const result = await readCronTasks(tmpDir); - expect(result).toEqual([task]); - }); - it('round-trips the optional runs history', async () => { const task = makeTask({ lastFiredAt: 1718000300000, diff --git a/packages/core/src/services/cronTasksFile.ts b/packages/core/src/services/cronTasksFile.ts index 34abd3f1b61..ae3c0df157f 100644 --- a/packages/core/src/services/cronTasksFile.ts +++ b/packages/core/src/services/cronTasksFile.ts @@ -101,22 +101,13 @@ export interface DurableCronTask { */ disabledByArchive?: boolean; /** - * Id of the dedicated session this task is bound to. A task created through - * the Web Shell management page mints its own session and stores its id here; - * the task then fires ONLY inside that session (not via the shared per-project - * durable owner), so the session's transcript is the task's run history, and - * archiving/deleting that session stops the task. Absent on tool-created - * (`cron_create`) and legacy tasks, which keep the shared-owner firing model. + * Id of the session this task is bound to. The task fires only inside that + * session, so its transcript is the task's run history. Absent on unbound + * tool-created and legacy tasks, which use the shared durable owner. */ sessionId?: string; - /** - * Whether the bound session was minted BY the task (`true`) or provided by - * the caller (`false`). Gates delete-time teardown: deleting a task closes a - * session it minted, but must never tear down a caller-provided session — - * that one pre-existed the task and survives it. Absent on tasks written - * before this field existed; every session bindable before then was - * task-minted, so absent is treated as owned (teardown preserved). - */ + /** False when the caller, rather than the task, owns the bound session. + * Absent means task-owned for backward compatibility. */ sessionOwnedByTask?: boolean; delivery?: CronTaskDelivery; /** diff --git a/packages/webui/src/daemon/workspace/types.ts b/packages/webui/src/daemon/workspace/types.ts index 35d0736880e..5ba79c8f9d6 100644 --- a/packages/webui/src/daemon/workspace/types.ts +++ b/packages/webui/src/daemon/workspace/types.ts @@ -272,11 +272,7 @@ export interface DaemonCreateScheduledTaskRequest { recurring?: boolean; /** Defaults to true. */ enabled?: boolean; - /** Reuse an existing live session instead of minting a dedicated one. The - * session must be live in this workspace, idle, not archived, and not - * already bound to another scheduled task; after a successful create it - * follows the regular scheduled-task session lifecycle. Omit (or null) to - * keep the dedicated-session behavior. */ + /** Reuse an existing live, idle session instead of creating one. */ sessionId?: string | null; } From e2a8ce3484c60e007077acac35547d433b1afad7 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Thu, 20 Aug 2026 03:15:10 +0800 Subject: [PATCH 10/14] fix(scheduled-tasks): restore conversation-bound tasks --- packages/cli/src/serve/run-qwen-serve.test.ts | 71 +++++++++++++++++++ packages/cli/src/serve/run-qwen-serve.ts | 14 ++-- .../cli/src/serve/scheduled-task-keepalive.ts | 4 +- packages/cli/src/serve/server.ts | 19 ++++- 4 files changed, 99 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index d61c0cc9add..f3925fea606 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -74,6 +74,7 @@ import { import { getDeferredRuntimeRequestTiming } from './server/request-helpers.js'; import type { WorkspaceFileSystemFactory } from './fs/workspace-file-system.js'; import { ConversationWorkspace } from './conversations/conversation-workspace.js'; +import * as scheduledTaskKeepalive from './scheduled-task-keepalive.js'; const originalTestRuntimeDir = process.env['QWEN_RUNTIME_DIR']; const isolatedTestRuntimeDir = fs.realpathSync( @@ -489,6 +490,76 @@ function makeRuntimeBridge(): HttpAcpBridge { } as unknown as HttpAcpBridge; } +it('restores the Conversations runtime for a persisted scheduled task', async () => { + const workspace = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qws-live-task-keepalive-')), + ); + const liveConversationWorkspace = new ConversationWorkspace({ + homeDir: workspace, + }); + await liveConversationWorkspace.getRoot(); + await qwenCore.updateCronTasks(liveConversationWorkspace.rootPath, () => [ + { + id: 'live-task', + cron: '0 9 * * *', + prompt: 'p', + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: null, + sessionId: 'live-session', + sessionOwnedByTask: false, + }, + ]); + const startKeepalive = vi + .spyOn(scheduledTaskKeepalive, 'startScheduledTaskKeepalive') + .mockReturnValue({ + stop: vi.fn(), + tick: vi.fn().mockResolvedValue(undefined), + }); + vi.spyOn(acpBridge, 'createAcpSessionBridge').mockImplementation( + () => + ({ + ...makeRuntimeBridge(), + recordHeartbeat: vi.fn(), + resumeSession: vi.fn().mockResolvedValue({}), + setLiveScreenContextCaptureHandler: vi.fn(), + setLiveTaskToolRequestHandler: vi.fn(), + setLiveSpeakToUserHandler: vi.fn(), + }) as ReturnType, + ); + let handle: RunHandle | undefined; + + try { + handle = await runQwenServe( + { + port: 0, + hostname: '127.0.0.1', + mode: 'http-bridge', + workspace, + maxSessions: 1, + serveWebShell: false, + }, + { + bridge: makeRuntimeBridge(), + liveConversationWorkspace, + resolveOnListen: true, + }, + ); + await handle.runtimeReady; + await vi.waitFor(() => { + expect(startKeepalive).toHaveBeenCalledWith( + expect.objectContaining({ + boundWorkspace: liveConversationWorkspace.rootPath, + }), + ); + }); + } finally { + await handle?.close(); + fs.rmSync(workspace, { recursive: true, force: true }); + vi.restoreAllMocks(); + } +}); + function writeWebShellFixture(workspaceDir: string): string { const shellDir = path.join(workspaceDir, 'web-shell'); fs.mkdirSync(path.join(shellDir, 'assets'), { recursive: true }); diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 46754b9d0fa..035f4502403 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -5553,13 +5553,6 @@ async function runQwenServeImpl( } = { current: undefined }; const workspaceRuntimeRemoval = { async runtimeAdded(runtimeAdded: WorkspaceRuntime): Promise { - if (runtimeAdded.provenance === 'live-conversation') return; - channelWebhookEnvByWorkspace.set( - runtimeAdded.workspaceCwd, - workspaceRuntimeEffectiveEnv(runtimeAdded, daemonRuntimeBaseEnv), - ); - channelWebhookConfigVersion += 1; - refreshChannelWebhookConfigs?.(); const app = serveAppForRuntimeLifecycle.current ?? runtimeApp ?? @@ -5568,6 +5561,13 @@ async function runQwenServeImpl( 'startScheduledTaskKeepaliveForWorkspace' ] as ((runtime: WorkspaceRuntime) => void) | undefined; startScheduledTaskKeepaliveForWorkspace?.(runtimeAdded); + if (runtimeAdded.provenance === 'live-conversation') return; + channelWebhookEnvByWorkspace.set( + runtimeAdded.workspaceCwd, + workspaceRuntimeEffectiveEnv(runtimeAdded, daemonRuntimeBaseEnv), + ); + channelWebhookConfigVersion += 1; + refreshChannelWebhookConfigs?.(); if (!channelWorkerManager) return; try { if (runtimeAdded.trusted) { diff --git a/packages/cli/src/serve/scheduled-task-keepalive.ts b/packages/cli/src/serve/scheduled-task-keepalive.ts index 53e888f3af7..ede43298467 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.ts @@ -50,7 +50,9 @@ const log = createDebugLogger('SCHED_KEEPALIVE'); * unbound, or a duplicate of one already collected. The heartbeat pass and the * boot rehydrate share this so the "which sessions to keep resident" filter lives * in exactly one place and can't drift between them. */ -function collectBoundSessionIds(tasks: readonly DurableCronTask[]): string[] { +export function collectBoundSessionIds( + tasks: readonly DurableCronTask[], +): string[] { const seen = new Set(); const ids: string[] = []; for (const task of tasks) { diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index daddcabd4c8..092e4d922bb 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -10,6 +10,7 @@ import * as path from 'node:path'; import type { DaemonStatusProvider } from '@qwen-code/acp-bridge'; import { hashDaemonWorkspace, + readCronTasks, Storage, type DurableCronTask, } from '@qwen-code/qwen-code-core'; @@ -127,6 +128,7 @@ import { registerChannelNotifyRoutes } from './routes/channel-notify.js'; import { registerGoalsRoutes } from './routes/goals.js'; import { registerUsageStatsRoutes } from './routes/usage-stats.js'; import { + collectBoundSessionIds, startScheduledTaskKeepalive, rehydrateScheduledTaskSessions, } from './scheduled-task-keepalive.js'; @@ -1470,6 +1472,22 @@ export function createServeApp( if (liveVoiceEnabled) { serveAppLifecycle.setBootStarter(startConversationRuntimeBoot); } + if (deps.manageScheduledTaskSessions && deps.liveConversationWorkspace) { + void readCronTasks(deps.liveConversationWorkspace.rootPath) + .then((tasks) => { + if (collectBoundSessionIds(tasks).length > 0) { + return startConversationRuntimeBoot(); + } + return undefined; + }) + .catch((error) => { + process.stderr.write( + `qwen serve: failed to restore the Conversations runtime for scheduled tasks: ${ + error instanceof Error ? error.message : String(error) + }\n`, + ); + }); + } const ensureConversationRuntimeWithLifecycle = async () => { await serveAppLifecycle.startBoot(startConversationRuntimeBoot); if (!liveRuntimeBootResult) { @@ -2716,7 +2734,6 @@ export function createServeApp( // own cron file + bridge. const keepaliveStops = new Map void>(); const startKeepaliveForWorkspace = (runtime: WorkspaceRuntime) => { - if (runtime.provenance === 'live-conversation') return; const trusted = runtime.primary ? isPrimaryWorkspaceTrusted() : runtime.trusted; From d561441f92b3941be082a918ce514968848cc66d Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Thu, 20 Aug 2026 07:06:57 +0800 Subject: [PATCH 11/14] fix(scheduled-tasks): restore conversation runtime tasks --- packages/cli/src/serve/core-runtime.ts | 1 + packages/cli/src/serve/run-qwen-serve.test.ts | 53 +++++++++++++------ packages/cli/src/serve/run-qwen-serve.ts | 24 +++++++++ packages/cli/src/serve/server.ts | 8 ++- 4 files changed, 68 insertions(+), 18 deletions(-) diff --git a/packages/cli/src/serve/core-runtime.ts b/packages/cli/src/serve/core-runtime.ts index 9ab9c48fb53..9c131fd60a4 100644 --- a/packages/cli/src/serve/core-runtime.ts +++ b/packages/cli/src/serve/core-runtime.ts @@ -19,6 +19,7 @@ export { hashDaemonWorkspace, initializeDaemonMetrics, initializeTelemetry, + readCronTasks, recordDaemonCancel, recordDaemonChannelLifecycle, recordDaemonPipeMessage, diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index f3925fea606..bff1cad894c 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -491,25 +491,44 @@ function makeRuntimeBridge(): HttpAcpBridge { } it('restores the Conversations runtime for a persisted scheduled task', async () => { - const workspace = fs.realpathSync( + delete process.env['QWEN_RUNTIME_DIR']; + const tempRoot = fs.realpathSync( fs.mkdtempSync(path.join(os.tmpdir(), 'qws-live-task-keepalive-')), ); + const workspace = path.join(tempRoot, 'workspace'); + const physicalHome = path.join(tempRoot, 'home'); + const linkedHome = path.join(tempRoot, 'home-link'); + const runtimeDir = path.join(tempRoot, 'runtime'); + fs.mkdirSync(workspace); + fs.mkdirSync(physicalHome); + fs.symlinkSync( + physicalHome, + linkedHome, + process.platform === 'win32' ? 'junction' : 'dir', + ); const liveConversationWorkspace = new ConversationWorkspace({ - homeDir: workspace, + homeDir: linkedHome, }); - await liveConversationWorkspace.getRoot(); - await qwenCore.updateCronTasks(liveConversationWorkspace.rootPath, () => [ - { - id: 'live-task', - cron: '0 9 * * *', - prompt: 'p', - recurring: true, - createdAt: 1_700_000_000_000, - lastFiredAt: null, - sessionId: 'live-session', - sessionOwnedByTask: false, - }, - ]); + const { canonicalRoot } = await liveConversationWorkspace.getRoot(); + fs.mkdirSync(path.join(canonicalRoot, '.qwen')); + fs.writeFileSync( + path.join(canonicalRoot, '.qwen', 'settings.json'), + JSON.stringify({ advanced: { runtimeOutputDir: runtimeDir } }), + ); + await qwenCore.Storage.runWithResolvedRuntimeBaseDir(runtimeDir, () => + qwenCore.updateCronTasks(canonicalRoot, () => [ + { + id: 'live-task', + cron: '0 9 * * *', + prompt: 'p', + recurring: true, + createdAt: 1_700_000_000_000, + lastFiredAt: null, + sessionId: 'live-session', + sessionOwnedByTask: false, + }, + ]), + ); const startKeepalive = vi .spyOn(scheduledTaskKeepalive, 'startScheduledTaskKeepalive') .mockReturnValue({ @@ -549,13 +568,13 @@ it('restores the Conversations runtime for a persisted scheduled task', async () await vi.waitFor(() => { expect(startKeepalive).toHaveBeenCalledWith( expect.objectContaining({ - boundWorkspace: liveConversationWorkspace.rootPath, + boundWorkspace: canonicalRoot, }), ); }); } finally { await handle?.close(); - fs.rmSync(workspace, { recursive: true, force: true }); + fs.rmSync(tempRoot, { recursive: true, force: true }); vi.restoreAllMocks(); } }); diff --git a/packages/cli/src/serve/run-qwen-serve.ts b/packages/cli/src/serve/run-qwen-serve.ts index 035f4502403..f41e92929a7 100644 --- a/packages/cli/src/serve/run-qwen-serve.ts +++ b/packages/cli/src/serve/run-qwen-serve.ts @@ -4589,6 +4589,29 @@ async function runQwenServeImpl( }; }; + const readLiveConversationScheduledTasks = async () => { + if (!fs.existsSync(liveConversationWorkspace.rootPath)) return []; + const { canonicalRoot } = await liveConversationWorkspace.revalidate(); + let settings: ReturnType | undefined; + try { + settings = settingsRuntime.settings.loadSettings(canonicalRoot, { + skipLoadEnvironment: true, + skipWorkspaceSettings: false, + workspaceTrusted: true, + }); + } catch (err) { + writeStderrLine( + `qwen serve: could not read full settings for Conversations ` + + `(${err instanceof Error ? err.message : String(err)}); falling back to defaults.`, + ); + } + const env = createRuntimeEnvMetadata(canonicalRoot, settings, true); + return core.Storage.runWithResolvedRuntimeBaseDir( + env.sessionRuntimeBaseDir, + () => core.readCronTasks(canonicalRoot), + ); + }; + // Collects stop() callbacks from every per-workspace sub-session launcher // (primary + secondaries). Called during shutdown so no new sub-sessions // are admitted while bridges are being torn down. @@ -5931,6 +5954,7 @@ async function runQwenServeImpl( : {}), managedScratchRoot, liveConversationWorkspace, + readLiveConversationScheduledTasks, workspaceRegistrationStore, workspaceRuntimeRemoval, workspaceTrustHotReloadAvailable, diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 092e4d922bb..92e5218ef4c 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -608,6 +608,9 @@ export interface ServeAppDeps { liveHostInstaller?: LiveHostInstaller; liveSessionCoordinator?: LiveSessionCoordinator; liveConversationWorkspace?: ConversationWorkspace; + readLiveConversationScheduledTasks?: () => Promise< + readonly DurableCronTask[] + >; liveDiscoveryStableBaseDir?: string; conversationRuntimeOwnershipFactory?: ( pid: number, @@ -1473,7 +1476,10 @@ export function createServeApp( serveAppLifecycle.setBootStarter(startConversationRuntimeBoot); } if (deps.manageScheduledTaskSessions && deps.liveConversationWorkspace) { - void readCronTasks(deps.liveConversationWorkspace.rootPath) + const readTasks = + deps.readLiveConversationScheduledTasks ?? + (() => readCronTasks(deps.liveConversationWorkspace!.rootPath)); + void readTasks() .then((tasks) => { if (collectBoundSessionIds(tasks).length > 0) { return startConversationRuntimeBoot(); From 5adfabbde67c23f40fb425762b4c71882cbb3c51 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Thu, 20 Aug 2026 10:17:00 +0800 Subject: [PATCH 12/14] fix(scheduled-tasks): honor the session-management gate on the primary surface --- .../src/serve/routes/scheduled-tasks.test.ts | 46 +++++++++++++++++++ .../cli/src/serve/routes/scheduled-tasks.ts | 7 ++- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index 3e3a115884a..d8ee186c238 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -658,6 +658,52 @@ describe('scheduled-tasks routes', () => { expect(rejected.body.code).toBe('session_binding_unavailable'); }); + it('rejects requested binding when management is off even with an active runtime bridge', async () => { + // Mirrors the production createServeApp wiring exactly: getRuntime is + // always wired to the primary runtime (active, carrying a bridge), while + // deps `bridge` is undefined because manageScheduledTaskSessions is off. + // The runtime bridge must NOT re-enable session binding in that case — + // nothing would keep the bound session resident or rehydrate it after a + // daemon restart, so caller-session requests fail closed with 409. + const runtimeBridge = makeStubBridge(); + const app = express(); + app.use(express.json()); + registerScheduledTasksRoutes(app, { + boundWorkspace: h.workspace, + mutate: () => (_req, _res, next) => next(), + safeBody, + // no deps bridge — resident task-session management is off + getRuntime: () => + ({ + workspaceId: 'primary', + workspaceCwd: h.workspace, + primary: true, + trusted: true, + bridge: runtimeBridge, + }) as unknown as WorkspaceRuntime, + }); + + const unbound = await request(app) + .post('/scheduled-tasks') + .send({ cron: '0 9 * * *', prompt: 'p' }); + expect(unbound.status).toBe(201); + expect(unbound.body.sessionId).toBeNull(); // unbound — fires via shared owner + expect(runtimeBridge.spawned).toEqual([]); // nothing was spawned + + const rejected = await request(app).post('/scheduled-tasks').send({ + cron: '0 10 * * *', + prompt: 'p', + sessionId: CALLER_SESSION_ID, + }); + expect(rejected.status).toBe(409); + expect(rejected.body.code).toBe('session_binding_unavailable'); + expect(runtimeBridge.spawned).toEqual([]); + // The rejected POST persisted nothing — only the unbound task remains. + expect(await readCronTasks(h.workspace)).toEqual([ + expect.objectContaining({ id: unbound.body.id }), + ]); + }); + it('reuses a caller-owned session without minting or renaming it', async () => { addLiveSession(h.bridge, CALLER_SESSION_ID, h.workspace); diff --git a/packages/cli/src/serve/routes/scheduled-tasks.ts b/packages/cli/src/serve/routes/scheduled-tasks.ts index 2fa10e5f865..146843c3799 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.ts @@ -1441,7 +1441,12 @@ export function registerScheduledTasksRoutes( : {}), } : {}), - bridge: runtime?.bridge ?? bridge, + // The runtime bridge only refines an ENABLED deps bridge; it must never + // re-enable binding when deps `bridge` is undefined. server.ts passes + // the bridge only when resident task-session management is on, and a + // bound task must always have something to keep it resident + rehydrate + // it — the same gate the qualified surface enforces below. + bridge: bridge === undefined ? undefined : (runtime?.bridge ?? bridge), ...(runtime?.generationGuard ? { assertGenerationOpen: () => runtime.generationGuard?.assertOpen(), From 3385904dd583ceeef6bc5ccb52859612d619f5c2 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Thu, 20 Aug 2026 11:57:12 +0800 Subject: [PATCH 13/14] test(scheduled-tasks): isolate the Conversations runtime ownership record The boot-restore test passed no liveDiscoveryStableBaseDir, so runQwenServe resolved it to ~/.qwen and built the Conversations-runtime ownership on the machine-global record. A concurrent live owner under the same HOME (another vitest worker, a shared-runner CI job, a developer's qwen serve) failed the boot with 'The Conversations runtime is owned by another daemon.' Point the test at a temp stable base, matching the four daemon boots in run-qwen-serve-live.test.ts. --- packages/cli/src/serve/run-qwen-serve.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/cli/src/serve/run-qwen-serve.test.ts b/packages/cli/src/serve/run-qwen-serve.test.ts index bff1cad894c..c1c64c2d1af 100644 --- a/packages/cli/src/serve/run-qwen-serve.test.ts +++ b/packages/cli/src/serve/run-qwen-serve.test.ts @@ -561,6 +561,10 @@ it('restores the Conversations runtime for a persisted scheduled task', async () { bridge: makeRuntimeBridge(), liveConversationWorkspace, + // Isolate the Conversations-runtime ownership record from the + // machine-global ~/.qwen path: a concurrent live owner there + // (another worker / a developer's qwen serve) would fail this boot. + liveDiscoveryStableBaseDir: path.join(tempRoot, 'stable'), resolveOnListen: true, }, ); From 1bd85e5cee57c43f75eb7849c215a75605c2b610 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Thu, 20 Aug 2026 14:28:07 +0800 Subject: [PATCH 14/14] test(scheduled-tasks): cover the ambiguous session-owner rejection path --- .../src/serve/routes/scheduled-tasks.test.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/cli/src/serve/routes/scheduled-tasks.test.ts b/packages/cli/src/serve/routes/scheduled-tasks.test.ts index d8ee186c238..96539fab08c 100644 --- a/packages/cli/src/serve/routes/scheduled-tasks.test.ts +++ b/packages/cli/src/serve/routes/scheduled-tasks.test.ts @@ -2380,6 +2380,30 @@ describe('workspace-qualified scheduled-tasks routes', () => { expect(h.primary.bridge.spawned).toEqual([]); }); + it('rejects a session claimed by two runtimes as ambiguous', async () => { + addLiveSession( + h.primary.bridge, + SECONDARY_SESSION_ID, + h.primary.workspaceCwd, + ); + addLiveSession( + h.secondary.bridge, + SECONDARY_SESSION_ID, + h.secondary.workspaceCwd, + ); + + const res = await request(h.app).post('/scheduled-tasks').send({ + cron: '0 9 * * *', + prompt: 'p', + sessionId: SECONDARY_SESSION_ID, + }); + + expect(res.status).toBe(500); + expect(res.body.code).toBe('ambiguous_session_owner'); + expect(h.primary.bridge.spawned).toEqual([]); + expect(h.secondary.bridge.spawned).toEqual([]); + }); + it('writes to the targeted workspace’s own cron file on disk', async () => { await request(h.app) .post(qualified(h.secondary.workspaceId))