fix(serve): keepalive hardening + i18n sync (review suggestions)

- 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
This commit is contained in:
Shaojin Wen 2026-07-07 22:06:45 +08:00
parent 237811c1e0
commit 41a5a6955e
4 changed files with 133 additions and 16 deletions

View file

@ -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

View file

@ -119,8 +119,11 @@ async function bindAndNameSessions(
tasks: readonly DurableCronTask[],
renamed: Set<string>,
spawnTimeoutMs: number,
binding: Set<string>,
): Promise<void> {
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<string>();
// 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<string>();
// Tracks sessions the keepalive has already named with ⏰ prefix,
// so updateSessionMetadata isn't called every tick.
const renamed = new Set<string>();
@ -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;

View file

@ -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();
});
});

View file

@ -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',