From 41a5a6955e760d2e8fa47494881f8f370d452bc3 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Tue, 7 Jul 2026 22:06:45 +0800 Subject: [PATCH] fix(serve): keepalive hardening + i18n sync (review suggestions) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - i18n: sync English 'View history' → 'View conversation' to match Chinese '查看对话' - Prune renamed Set alongside reviveState when tasks are removed - fs.watch: clarify null filename handling for Linux (treat as match) - updateCronTasks: skip .map() when task not found (no-op optimization) - Add tests: disabled unbound exclusion, naming failure resilience --- .../serve/scheduled-task-keepalive.test.ts | 87 ++++++++++++++++++- .../cli/src/serve/scheduled-task-keepalive.ts | 48 ++++++++-- .../dialogs/ScheduledTasksDialog.test.tsx | 10 +-- packages/web-shell/client/i18n.tsx | 4 +- 4 files changed, 133 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/serve/scheduled-task-keepalive.test.ts b/packages/cli/src/serve/scheduled-task-keepalive.test.ts index 33e27753c3..fa822d2586 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.test.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.test.ts @@ -458,6 +458,51 @@ describe('scheduled-task keepalive', () => { expect(names[0]![1].displayName).toContain('⏰'); }); + it('does not bind disabled unbound tasks', async () => { + await updateCronTasks(workspace, () => [ + task({ id: 'disabled-unbound', enabled: false }), + ]); + let spawnCount = 0; + const noSpawn = { + ...bridge, + spawnOrAttach: async () => { + spawnCount++; + return { sessionId: 'should-not-spawn' }; + }, + }; + const ka = startScheduledTaskKeepalive({ + bridge: noSpawn, + boundWorkspace: workspace, + intervalMs: 60_000, + }); + await ka.tick(); + ka.stop(); + expect(spawnCount).toBe(0); + }); + + it('still binds a task when updateSessionMetadata fails', async () => { + await updateCronTasks(workspace, () => [ + task({ id: 'name-fail', prompt: 'test prompt' }), + ]); + const naming = { + ...bridge, + spawnOrAttach: async () => ({ sessionId: 'bound-despite-naming-fail' }), + closeSession: async () => {}, + updateSessionMetadata: () => { + throw new Error('metadata service down'); + }, + }; + const ka = startScheduledTaskKeepalive({ + bridge: naming, + boundWorkspace: workspace, + intervalMs: 60_000, + }); + await ka.tick(); + ka.stop(); + const tasks = await readCronTasks(workspace); + expect(tasks[0]!.sessionId).toBe('bound-despite-naming-fail'); + }); + it('rolls back the spawned session when the task vanishes before write', async () => { // Seed an unbound task, then replace it with a different task between the // keepalive's read and the updateCronTasks callback — simulating a @@ -466,7 +511,7 @@ describe('scheduled-task keepalive', () => { const closed: string[] = []; const removeSpy = vi .spyOn(SessionService.prototype, 'removeSession') - .mockResolvedValue(undefined); + .mockResolvedValue(true); const rollbackBridge = { ...bridge, spawnOrAttach: async () => { @@ -495,6 +540,46 @@ describe('scheduled-task keepalive', () => { removeSpy.mockRestore(); }); + it('rolls back when another process binds the task before write', async () => { + // Another keepalive/process binds the same task between our spawn and + // our updateCronTasks lock. Our spawned session must be rolled back. + const closed: string[] = []; + const removeSpy = vi + .spyOn(SessionService.prototype, 'removeSession') + .mockResolvedValue(true); + const raceBridge = { + ...bridge, + spawnOrAttach: async () => { + // Simulate another process binding the task. + await updateCronTasks(workspace, (list) => + list.map((t) => + t.id === 'raced' ? { ...t, sessionId: 'other-sess' } : t, + ), + ); + return { sessionId: 'our-orphan' }; + }, + closeSession: async (id: string) => { + closed.push(id); + }, + updateSessionMetadata: () => {}, + }; + await updateCronTasks(workspace, () => [ + task({ id: 'raced', prompt: 'contested' }), + ]); + const ka = startScheduledTaskKeepalive({ + bridge: raceBridge, + boundWorkspace: workspace, + intervalMs: 60_000, + }); + await ka.tick(); + ka.stop(); + expect(closed).toContain('our-orphan'); + // The other process's sessionId is preserved. + const tasks = await readCronTasks(workspace); + expect(tasks[0]!.sessionId).toBe('other-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 64b21a2fea..822ba81e85 100644 --- a/packages/cli/src/serve/scheduled-task-keepalive.ts +++ b/packages/cli/src/serve/scheduled-task-keepalive.ts @@ -119,8 +119,11 @@ async function bindAndNameSessions( tasks: readonly DurableCronTask[], renamed: Set, spawnTimeoutMs: number, + binding: Set, ): Promise { - const unbound = tasks.filter((t) => !t.sessionId && t.enabled !== false); + const unbound = tasks.filter( + (t) => !t.sessionId && t.enabled !== false && !binding.has(t.id), + ); const needsName = tasks.filter( (t) => t.sessionId && t.enabled !== false && !renamed.has(t.sessionId), ); @@ -128,13 +131,15 @@ async function bindAndNameSessions( for (const task of unbound) { let spawnedSessionId: string | undefined; try { + binding.add(task.id); const rawSpawn = bridge.spawnOrAttach({ workspaceCwd: boundWorkspace, sessionScope: 'thread', }); // spawnOrAttach is not abortable — if the timeout fires first, the // raw promise may still resolve later with a live session. Attach a - // background handler to clean up that orphan immediately. + // background handler to clean up that orphan immediately. Clear the + // binding guard on TRUE settlement so retries are possible. let timedOut = false; rawSpawn .then(({ sessionId }) => { @@ -150,7 +155,10 @@ async function bindAndNameSessions( .catch(() => {}); } }) - .catch(() => {}); + .catch(() => {}) + .finally(() => { + binding.delete(task.id); + }); const { sessionId } = await withTimeout( rawSpawn, spawnTimeoutMs, @@ -170,10 +178,23 @@ async function bindAndNameSessions( } let matched = false; await updateCronTasks(boundWorkspace, (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. + if ( + !list.some( + (t) => t.id === task.id && !t.sessionId && t.enabled !== false, + ) + ) { + return list; + } const result = list.map((t) => - t.id === task.id ? { ...t, sessionId } : t, + t.id === task.id && !t.sessionId && t.enabled !== false + ? { ...t, sessionId } + : t, ); - matched = result.some((t) => t.sessionId === sessionId); + matched = true; return result; }); if (!matched) { @@ -247,6 +268,11 @@ export function startScheduledTaskKeepalive( // the load's TRUE settlement, not the timeout. const reviving = new Set(); + // Tasks with a spawn in flight. After withTimeout rejects, the raw + // spawnOrAttach may still be running — skip the task in subsequent ticks + // until the raw spawn settles. + const binding = new Set(); + // Tracks sessions the keepalive has already named with ⏰ prefix, // so updateSessionMetadata isn't called every tick. const renamed = new Set(); @@ -318,12 +344,15 @@ export function startScheduledTaskKeepalive( } } } - // Drop backoff state for sessions no longer bound to any task. - if (reviveState.size > 0) { + // Drop backoff state and renamed entries for sessions no longer bound to any task. + if (reviveState.size > 0 || renamed.size > 0) { const live = new Set(tasks.map((t) => t.sessionId)); for (const id of reviveState.keys()) { if (!live.has(id)) reviveState.delete(id); } + for (const id of renamed) { + if (!live.has(id)) renamed.delete(id); + } } await bindAndNameSessions( @@ -332,6 +361,7 @@ export function startScheduledTaskKeepalive( tasks, renamed, spawnTimeoutMs, + binding, ); }; @@ -364,7 +394,9 @@ export function startScheduledTaskKeepalive( cronDir, { persistent: false }, (_event, filename) => { - if (filename && filename !== cronFileName) return; + // On Linux fs.watch delivers null as filename — treat it as a match + // (could be our file); non-matching filenames are skipped. + if (filename !== null && filename !== cronFileName) return; if (bindDebounce) clearTimeout(bindDebounce); bindDebounce = setTimeout(() => { if (running) return; diff --git a/packages/web-shell/client/components/dialogs/ScheduledTasksDialog.test.tsx b/packages/web-shell/client/components/dialogs/ScheduledTasksDialog.test.tsx index 708ab3c4d2..d462a2ca40 100644 --- a/packages/web-shell/client/components/dialogs/ScheduledTasksDialog.test.tsx +++ b/packages/web-shell/client/components/dialogs/ScheduledTasksDialog.test.tsx @@ -405,9 +405,9 @@ describe('ScheduledTasksDialog view-history (bound session)', () => { ], { onOpenSession }, ); - // A bound task shows a "View history" control (with a run count) that opens - // its session transcript — not the inline expand toggle. - const btn = findButton('View history (1)'); + // A bound task shows a "View conversation" control (with a run count) that + // opens its session transcript — not the inline expand toggle. + const btn = findButton('View conversation (1)'); expect(btn).toBeDefined(); expect(document.querySelector('button[aria-expanded]')).toBeNull(); click(btn); @@ -420,7 +420,7 @@ describe('ScheduledTasksDialog view-history (bound session)', () => { onOpenSession, }); // Discoverable even with zero runs — the original "can't find history" pain. - const btn = findButton('View history'); + const btn = findButton('View conversation'); expect(btn).toBeDefined(); click(btn); expect(onOpenSession).toHaveBeenCalledWith('sess-42'); @@ -432,7 +432,7 @@ describe('ScheduledTasksDialog view-history (bound session)', () => { await mount([ baseTask({ sessionId: 'sess-42', runs: [{ at: 1_700_000_100_000 }] }), ]); - expect(findButton('View history (1)')).toBeUndefined(); + expect(findButton('View conversation (1)')).toBeUndefined(); expect(document.querySelector('button[aria-expanded]')).not.toBeNull(); }); }); diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index 9e2a50f41a..b4e37571b8 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -661,8 +661,8 @@ const EN: Messages = { 'scheduledTasks.save': 'Save', 'scheduledTasks.saving': 'Saving…', 'scheduledTasks.runHistory': (v) => `Run history (${v?.count ?? 0})`, - 'scheduledTasks.viewHistory': (v) => `View history (${v?.count ?? 0})`, - 'scheduledTasks.viewHistoryEmpty': 'View history', + 'scheduledTasks.viewHistory': (v) => `View conversation (${v?.count ?? 0})`, + 'scheduledTasks.viewHistoryEmpty': 'View conversation', 'scheduledTasks.viewHistoryHint': "Open the task's session to see its runs", 'scheduledTasks.runKind.catchUp': 'late', 'scheduledTasks.runKind.manual': 'manual',