diff --git a/packages/cli/src/ui/commands/workflowsCommand.test.ts b/packages/cli/src/ui/commands/workflowsCommand.test.ts index bb22bfeeab6..54dab34f26c 100644 --- a/packages/cli/src/ui/commands/workflowsCommand.test.ts +++ b/packages/cli/src/ui/commands/workflowsCommand.test.ts @@ -11,7 +11,13 @@ import path from 'node:path'; import { workflowsCommand } from './workflowsCommand.js'; import { type CommandContext } from './types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; -import type { WorkflowTask, WorkflowSnapshot } from '@qwen-code/qwen-code-core'; +import { + Storage, + writeWorkflowManifest, + type Config, + type WorkflowTask, + type WorkflowSnapshot, +} from '@qwen-code/qwen-code-core'; function entry(overrides: Partial = {}): WorkflowTask { return { @@ -536,7 +542,9 @@ describe('workflowsCommand', () => { // Write snapshot JSON files into a fresh temp dir and return a context // whose `config.storage.getWorkflowRunsDir()` points at it. This drives - // the real `listWorkflowSnapshots` (no module mocking). + // the real `listWorkflowRunRecords` (no module mocking) — note the + // different bad-record semantics: an unparseable file surfaces as a + // visible `interrupted · not recoverable` row rather than being skipped. async function ctxWithSnapshots( snaps: Array>, mode: 'interactive' | 'non_interactive' = 'interactive', @@ -562,6 +570,51 @@ describe('workflowsCommand', () => { } as unknown as Parameters[0]); } + async function ctxWithManifest( + run: Partial, + journal: { + integrity: 'complete' | 'failed'; + error?: string; + } = { integrity: 'complete' }, + ): Promise { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'wf-manifest-cli-')); + tmpDirs.push(root); + const storage = new Storage(root, path.join(root, 'runtime')); + const config = { + storage, + getWorkflowRunRegistry: () => ({ + list: listMock, + get: getMock, + pause: pauseMock, + resume: resumeMock, + }), + }; + await writeWorkflowManifest( + config as unknown as Config, + entry({ + runId: 'wf_deadbeef', + status: 'running', + script: 'return 1;', + ...run, + }), + { + args: null, + journal: { + version: 1, + keyVersion: 'v2', + byteLength: 0, + sha256: + 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855', + ...journal, + }, + }, + ); + return createMockCommandContext({ + services: { config }, + executionMode: 'interactive', + } as unknown as Parameters[0]); + } + afterEach(async () => { await Promise.all( tmpDirs @@ -585,6 +638,23 @@ describe('workflowsCommand', () => { expect(result.content).toContain('oldrun'); }); + it.runIf(process.platform !== 'win32')( + 'does not render a control-character legacy filename', + async () => { + listMock.mockReturnValue([]); + const ctx = await ctxWithSnapshots([ + { runId: 'wf_bad\nCONTROL_SENTINEL' }, + ]); + const result = await workflowsCommand.action!(ctx, ''); + expect(result).toMatchObject({ + type: 'message', + messageType: 'info', + content: 'No workflow runs recorded yet.', + }); + expect(JSON.stringify(result)).not.toContain('CONTROL_SENTINEL'); + }, + ); + it('live registry entry shadows a same-runId snapshot (no duplicate row)', async () => { listMock.mockReturnValue([ entry({ @@ -668,5 +738,138 @@ describe('workflowsCommand', () => { content: 'Unknown live workflow runId: wf_ghost', }); }); + + it('classifies an orphaned active manifest as interrupted and recoverable', async () => { + listMock.mockReturnValue([]); + const ctx = await ctxWithManifest({ + runId: 'wf_deadbeef', + status: 'paused', + }); + + const result = await workflowsCommand.action!(ctx, ''); + if (!result || result.type !== 'message') throw new Error('no result'); + expect(result.content).toContain('Workflow runs (1 total · 0 active)'); + expect(result.content).toContain('wf_deadbeef'); + expect(result.content).toContain('interrupted'); + expect(result.content).toContain('· recoverable'); + }); + + it('shows a durable recovery failure reason in detail', async () => { + getMock.mockReturnValue(undefined); + const ctx = await ctxWithManifest( + { runId: 'wf_badc0de', status: 'running' }, + { integrity: 'failed', error: 'disk write failed' }, + ); + + const result = await workflowsCommand.action!(ctx, 'wf_badc0de'); + if (!result || result.type !== 'message') throw new Error('no result'); + expect(result.content).toContain('status : interrupted'); + expect(result.content).toContain('recovery : not recoverable'); + expect(result.content).toContain('reason : disk write failed'); + }); + + // The reason string is content another process wrote, not a runId, so + // the legacy-filename guard never sees it. Both the listing row (which + // slices to 80 chars, keeping any ESC) and the detail view render it. + it('strips terminal control bytes from a durable recovery reason', async () => { + getMock.mockReturnValue(undefined); + const ctx = await ctxWithManifest( + { runId: 'wf_badc0de', status: 'running' }, + { + integrity: 'failed', + error: '\u001b]0;pwned\u0007disk write failed', + }, + ); + + const listing = await workflowsCommand.action!(ctx, ''); + const detail = await workflowsCommand.action!(ctx, 'wf_badc0de'); + if (!listing || listing.type !== 'message') throw new Error('no result'); + if (!detail || detail.type !== 'message') throw new Error('no result'); + + expect(listing.content).not.toContain('\u001b'); + expect(detail.content).not.toContain('\u001b'); + expect(detail.content).toContain('disk write failed'); + }); + + // Same vector as the recovery-reason case, through the rest of the + // rendered family. `parseManifest` type-checks these as strings only, + // so a crafted manifest in the project's workflows dir carries ESC/OSC + // bytes all the way into both render paths. + it('strips terminal control bytes from every rendered manifest string', async () => { + getMock.mockReturnValue(undefined); + const esc = '\u001b]0;pwned\u0007'; + const ctx = await ctxWithManifest({ + runId: 'wf_badc0de', + status: 'failed', + meta: { + name: `${esc}evil-name`, + description: `${esc}evil-description`, + whenToUse: `${esc}evil-when`, + }, + error: `${esc}boom`, + recentLogs: [`${esc}log line`], + phases: [`${esc}phase one`], + perPhaseTokens: new Map([[`${esc}phase one`, 42]]), + }); + + const listing = await workflowsCommand.action!(ctx, ''); + const detail = await workflowsCommand.action!(ctx, 'wf_badc0de'); + if (!listing || listing.type !== 'message') throw new Error('no result'); + if (!detail || detail.type !== 'message') throw new Error('no result'); + + expect(listing.content).not.toContain('\u001b'); + expect(detail.content).not.toContain('\u001b'); + // Sanitizing must not swallow the content itself. + expect(detail.content).toContain('evil-name'); + expect(detail.content).toContain('evil-description'); + expect(detail.content).toContain('evil-when'); + expect(detail.content).toContain('boom'); + expect(detail.content).toContain('log line'); + expect(detail.content).toContain('phase one'); + // The token chip still resolves, so the map keys went through the + // same sanitizing pass as the phase list. + expect(detail.content).toContain('42t'); + }); + + // A kept LF would let a crafted record forge extra rows and extra + // detail fields, which is why the single-line helper flattens it. + it('flattens newlines in single-line manifest fields', async () => { + getMock.mockReturnValue(undefined); + const ctx = await ctxWithManifest({ + runId: 'wf_badc0de', + status: 'failed', + error: 'boom\n status : completed', + }); + + const detail = await workflowsCommand.action!(ctx, 'wf_badc0de'); + if (!detail || detail.type !== 'message') throw new Error('no result'); + + const lines = detail.content.split('\n'); + // The payload stays inside the error row instead of becoming a row + // of its own, so it cannot impersonate a real status field. + expect(lines.some((l) => l.startsWith(' error : boom'))).toBe( + true, + ); + expect(lines).not.toContain(' status : completed'); + expect(lines).toContain(' status : failed'); + }); + + it('never routes pause or resume to an interrupted persisted run', async () => { + getMock.mockReturnValue(undefined); + const ctx = await ctxWithManifest({ + runId: 'wf_deadbeef', + status: 'paused', + }); + + const result = await workflowsCommand.action!(ctx, 'p wf_deadbeef'); + expect(result).toMatchObject({ + type: 'message', + messageType: 'error', + content: + 'Workflow wf_deadbeef is interrupted and has no live owner; it cannot be paused or resumed.', + }); + expect(pauseMock).not.toHaveBeenCalled(); + expect(resumeMock).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/cli/src/ui/commands/workflowsCommand.ts b/packages/cli/src/ui/commands/workflowsCommand.ts index 335dbf08d4c..20a89e86407 100644 --- a/packages/cli/src/ui/commands/workflowsCommand.ts +++ b/packages/cli/src/ui/commands/workflowsCommand.ts @@ -4,58 +4,109 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { WorkflowTask, WorkflowSnapshot } from '@qwen-code/qwen-code-core'; +import type { + WorkflowRunRecord, + WorkflowStatus, + WorkflowTask, +} from '@qwen-code/qwen-code-core'; import { isActiveWorkflowStatus, isTerminalWorkflowStatus, - listWorkflowSnapshots, + listWorkflowRunRecords, } from '@qwen-code/qwen-code-core'; import type { SlashCommand } from './types.js'; import { CommandKind } from './types.js'; import { t } from '../../i18n/index.js'; import { formatDuration, formatTokenCount } from '../utils/formatters.js'; +import { sanitizeTerminalText } from '../utils/textUtils.js'; + +type WorkflowDisplayEntry = Omit & { + status: WorkflowStatus | 'interrupted'; + canResume?: boolean; + resumeBlockedReason?: string; +}; /** - * P7b: adapt a persisted snapshot to the `WorkflowTask` shape the row / - * detail formatters expect. The dialog-only fields (`abortController`, - * `outputFile`, etc.) are filled with inert values — a snapshot is always - * terminal, so the controls that read those fields are never reached. + * Sanitize a persisted string that is interpolated into a single line. + * + * `sanitizeTerminalText` deliberately keeps TAB and LF, because they + * legitimately structure multi-line output — but every field sanitized + * through this helper renders inside one line, where a surviving LF lets + * a crafted record forge extra listing rows or extra detail fields. Strip + * the control bytes first, then flatten the layout characters. */ -function snapshotToTask(s: WorkflowSnapshot): WorkflowTask { +function sanitizeLine(value: string): string { + return sanitizeTerminalText(value).replace(/[\t\n]+/g, ' '); +} + +function recordToDisplay(s: WorkflowRunRecord): WorkflowDisplayEntry { + // Every string below is untrusted content rather than a runId: a v2 + // `manifest.json` or a legacy `wf_.json` in the project's workflows + // dir is parsed with string type-checks only, so ESC/OSC bytes in any of + // these fields survive into `rowLine` (whose 80-char slices keep ESC) + // and `detailLines` — enough for cursor/title manipulation, OSC 52 + // clipboard writes and output spoofing. This function is the one + // boundary where a record becomes display text, so the whole rendered + // family is sanitized here rather than at each render site. + const phases = (s.phases ?? []).map(sanitizeLine); + const meta = s.meta + ? { + ...s.meta, + name: sanitizeLine(s.meta.name), + description: sanitizeLine(s.meta.description), + ...(s.meta.whenToUse === undefined + ? {} + : { whenToUse: sanitizeLine(s.meta.whenToUse) }), + } + : s.meta; + return { id: s.runId, kind: 'workflow', runId: s.runId, - description: s.meta?.name ?? s.runId, - meta: s.meta, + description: meta?.name ?? s.runId, + meta, status: s.status, currentPhase: null, - phases: s.phases ?? [], + phases, agentsDispatched: s.agentsDispatched ?? 0, agentsCompleted: s.agentsCompleted ?? 0, - recentLogs: s.recentLogs ?? [], + recentLogs: (s.recentLogs ?? []).map(sanitizeLine), tokensSpent: s.tokensSpent ?? 0, tokenBudgetTotal: s.tokenBudgetTotal ?? null, - perPhaseTokens: new Map(s.perPhaseTokens ?? []), + // Keyed by phase name, and `detailLines` looks each phase up by the + // sanitized string — so the keys have to go through the same pass or + // a phase carrying control bytes silently loses its token chip. + perPhaseTokens: new Map( + (s.perPhaseTokens ?? []).map(([phase, tokens]) => [ + phase === null ? null : sanitizeLine(phase), + tokens, + ]), + ), pendingApprovals: [], script: s.script ?? '', scriptPath: s.scriptPath, result: s.result, - error: s.error, + error: s.error === undefined ? undefined : sanitizeLine(s.error), startTime: s.startTime, endTime: s.endTime, outputFile: '', outputOffset: 0, notified: true, abortController: new AbortController(), - } as WorkflowTask; + canResume: s.canResume, + resumeBlockedReason: + s.resumeBlockedReason === undefined + ? undefined + : sanitizeLine(s.resumeBlockedReason), + } as WorkflowDisplayEntry; } /** * Format one workflow run as a one-line summary used by both the * top-level listing and the per-run detail view. */ -function rowLine(entry: WorkflowTask, now: number): string { +function rowLine(entry: WorkflowDisplayEntry, now: number): string { const endTime = entry.endTime ?? now; const runtime = formatDuration(endTime - entry.startTime, { hideTrailingZeros: true, @@ -87,10 +138,16 @@ function rowLine(entry: WorkflowTask, now: number): string { entry.status === 'failed' && entry.error ? ` — ${entry.error.slice(0, 80)}` : ''; - return ` ${entry.runId.padEnd(20)} ${entry.status.padEnd(10)} ${runtime.padStart(8)} ${label}${phase}${counts}${phaseCount}${budgetChip}${errorTail}`; + const recoveryTail = + entry.status === 'interrupted' + ? entry.canResume + ? ' · recoverable' + : ` · not recoverable${entry.resumeBlockedReason ? `: ${entry.resumeBlockedReason.slice(0, 80)}` : ''}` + : ''; + return ` ${entry.runId.padEnd(20)} ${entry.status.padEnd(12)} ${runtime.padStart(8)} ${label}${phase}${counts}${phaseCount}${budgetChip}${recoveryTail}${errorTail}`; } -function detailLines(entry: WorkflowTask, now: number): string[] { +function detailLines(entry: WorkflowDisplayEntry, now: number): string[] { const lines: string[] = []; const endTime = entry.endTime ?? now; const runtime = formatDuration(endTime - entry.startTime, { @@ -108,6 +165,14 @@ function detailLines(entry: WorkflowTask, now: number): string[] { lines.push(` whenToUse : ${entry.meta.whenToUse}`); } lines.push(` status : ${entry.status}`); + if (entry.canResume !== undefined) { + lines.push( + ` recovery : ${entry.canResume ? 'recoverable' : 'not recoverable'}`, + ); + if (!entry.canResume && entry.resumeBlockedReason) { + lines.push(` reason : ${entry.resumeBlockedReason}`); + } + } lines.push(` runtime : ${runtime}`); if (entry.currentPhase) { lines.push(` currentPhase: ${entry.currentPhase}`); @@ -197,23 +262,23 @@ export const workflowsCommand: SlashCommand = { }; } const runId = tokens[1]; - let target = registry.get(runId); - let fromSnapshot = false; + const target = registry.get(runId); if (!target) { - // Fall back to a persisted snapshot — the same source the listing - // and detail view merge in. A terminal run evicted from the - // in-memory registry (10-entry cap) or left behind by an earlier - // CLI process is still known to this command; answering "Unknown - // live workflow runId" for it contradicts the listing. - const snapshot = (await listWorkflowSnapshots(config)).find( - (s) => s.runId === runId, + const record = (await listWorkflowRunRecords(config)).find( + (entry) => entry.runId === runId, ); - if (snapshot) { - target = snapshotToTask(snapshot); - fromSnapshot = true; + if (record) { + const content = + record.status !== 'interrupted' && + isTerminalWorkflowStatus(record.status) + ? `Workflow ${runId} is ${record.status} and cannot be paused or resumed.` + : `Workflow ${runId} is ${record.status} and has no live owner; it cannot be paused or resumed.`; + return { + type: 'message' as const, + messageType: 'error' as const, + content, + }; } - } - if (!target) { return { type: 'message' as const, messageType: 'error' as const, @@ -232,7 +297,7 @@ export const workflowsCommand: SlashCommand = { content: `Workflow ${runId} is ${target.status} and cannot be paused or resumed.`, }; } - if (!fromSnapshot && !target.isBackgrounded) { + if (!target.isBackgrounded) { return { type: 'message' as const, messageType: 'error' as const, @@ -284,15 +349,15 @@ export const workflowsCommand: SlashCommand = { // dump for that run if it exists. Reject early on unknown runId so // the user sees a clear error instead of an empty listing. if (trimmedArgs.length > 0) { - let target = registry.get(trimmedArgs); + let target: WorkflowDisplayEntry | undefined = registry.get(trimmedArgs); if (!target) { // Fall back to a persisted snapshot — the run may predate this CLI // process (the in-memory registry dies with the process, the // snapshot on disk does not). - const snapshot = (await listWorkflowSnapshots(config)).find( - (s) => s.runId === trimmedArgs, + const record = (await listWorkflowRunRecords(config)).find( + (entry) => entry.runId === trimmedArgs, ); - if (snapshot) target = snapshotToTask(snapshot); + if (record) target = recordToDisplay(record); } if (!target) { return { @@ -308,16 +373,17 @@ export const workflowsCommand: SlashCommand = { }; } - // Merge persisted snapshots (runs from earlier CLI processes) into the + // Merge persisted run records (runs from earlier CLI processes) into the // listing. In-memory registry entries win on a runId collision — they - // carry live status, while a snapshot is a frozen terminal projection. - const snapshots = await listWorkflowSnapshots(config); + // carry live status, while a record is a persisted projection: terminal, + // or 'interrupted' for an orphaned active run. + const records = await listWorkflowRunRecords(config); const liveRunIds = new Set(allEntries.map((e) => e.runId)); - const snapshotTasks = snapshots - .filter((s) => !liveRunIds.has(s.runId)) - .map(snapshotToTask); + const persistedEntries = records + .filter((entry) => !liveRunIds.has(entry.runId)) + .map(recordToDisplay); - if (allEntries.length === 0 && snapshotTasks.length === 0) { + if (allEntries.length === 0 && persistedEntries.length === 0) { return { type: 'message' as const, messageType: 'info' as const, @@ -328,14 +394,16 @@ export const workflowsCommand: SlashCommand = { const now = Date.now(); // Order: active first (oldest startTime first inside the bucket so // long-runners stay visible), then terminal by endTime DESC. Mirrors - // the dialog's two-bucket sort. Snapshots are always terminal, so they - // only ever join the second bucket. + // the dialog's two-bucket sort. Persisted records are never active — + // terminal, or 'interrupted', which `isActiveWorkflowStatus` excludes — + // so they only ever join the second bucket, and the `endTime` fallbacks + // below are what keep an 'interrupted' record sortable there. const active = allEntries .filter((e) => isActiveWorkflowStatus(e.status)) .sort((a, b) => a.startTime - b.startTime); const terminal = [ ...allEntries.filter((e) => !isActiveWorkflowStatus(e.status)), - ...snapshotTasks, + ...persistedEntries, ].sort((a, b) => (b.endTime ?? 0) - (a.endTime ?? 0)); const lines: string[] = []; diff --git a/packages/core/src/agents/runtime/workflow-dispatch-scheduler.test.ts b/packages/core/src/agents/runtime/workflow-dispatch-scheduler.test.ts index 28747a0960e..09a4b34b770 100644 --- a/packages/core/src/agents/runtime/workflow-dispatch-scheduler.test.ts +++ b/packages/core/src/agents/runtime/workflow-dispatch-scheduler.test.ts @@ -50,6 +50,50 @@ describe('WorkflowDispatchScheduler', () => { expect(scheduler.resume()).toBe(false); }); + it('publishes paused only after the durable checkpoint barrier settles', async () => { + let releaseCheckpoint: (() => void) | undefined; + const checkpoint = new Promise((resolve) => { + releaseCheckpoint = resolve; + }); + const states: string[] = []; + const beforePaused = vi.fn(() => checkpoint); + const scheduler = new WorkflowDispatchScheduler( + 1, + undefined, + (snapshot) => states.push(snapshot.state), + beforePaused, + ); + + expect(scheduler.pause()).toBe(true); + await vi.waitFor(() => expect(beforePaused).toHaveBeenCalledOnce()); + expect(scheduler.snapshot().state).toBe('pausing'); + expect(states).toEqual(['pausing']); + + releaseCheckpoint?.(); + await vi.waitFor(() => expect(scheduler.snapshot().state).toBe('paused')); + expect(states).toEqual(['pausing', 'paused']); + }); + + it('does not publish paused when the durable checkpoint barrier fails', async () => { + const beforePaused = vi.fn(async () => { + throw new Error('checkpoint write failed'); + }); + const states: string[] = []; + const scheduler = new WorkflowDispatchScheduler( + 1, + undefined, + (snapshot) => states.push(snapshot.state), + beforePaused, + ); + + expect(scheduler.pause()).toBe(true); + await vi.waitFor(() => expect(beforePaused).toHaveBeenCalledOnce()); + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(scheduler.snapshot().state).toBe('pausing'); + expect(states).toEqual(['pausing']); + }); + it('stops dequeuing and holds a completed result gate until resume', async () => { const states: string[] = []; const scheduler = new WorkflowDispatchScheduler(1, undefined, (snapshot) => @@ -198,6 +242,217 @@ describe('WorkflowDispatchScheduler', () => { expect(scheduler.snapshot().state).toBe('paused'); }); + // pause()/resume()/run() all refuse once aborted, but the in-flight + // `.finally` path reached finishPause() without that check. The barrier + // durably writes 'paused' + canResume, and that write can land after the + // cancellation's — leaving a cancelled run that resume() re-executes. + it('does not run the pause barrier when the abort lands before the last job settles', async () => { + const controller = new AbortController(); + const beforePaused = vi.fn(async () => {}); + let finish: (() => void) | undefined; + const scheduler = new WorkflowDispatchScheduler( + 1, + controller.signal, + undefined, + beforePaused, + ); + + const running = scheduler.run( + () => + new Promise((resolve) => { + finish = resolve; + }), + ); + await vi.waitFor(() => expect(finish).toBeDefined()); + expect(scheduler.pause()).toBe(true); + expect(scheduler.snapshot().state).toBe('pausing'); + + controller.abort(); + finish?.(); + await running; + + await vi.waitFor(() => expect(scheduler.snapshot().inFlight).toBe(0)); + expect(beforePaused).not.toHaveBeenCalled(); + expect(scheduler.snapshot().state).toBe('pausing'); + }); + + it('still runs the pause barrier when no abort landed', async () => { + const beforePaused = vi.fn(async () => {}); + let finish: (() => void) | undefined; + const scheduler = new WorkflowDispatchScheduler( + 1, + new AbortController().signal, + undefined, + beforePaused, + ); + + const running = scheduler.run( + () => + new Promise((resolve) => { + finish = resolve; + }), + ); + await vi.waitFor(() => expect(finish).toBeDefined()); + expect(scheduler.pause()).toBe(true); + finish?.(); + await running; + + await vi.waitFor(() => expect(scheduler.snapshot().state).toBe('paused')); + expect(beforePaused).toHaveBeenCalledTimes(1); + }); + + it('hands back an already-resolved barrier on a never-paused scheduler', async () => { + // Pins the `pauseBarrier` field initializer. Without it the barrier is + // undefined until the first finishPause(), so beginSettling() hands every + // caller undefined for the whole pre-pause lifetime and any `.then()` + // chain on the result throws instead of ordering the terminal write. + const scheduler = new WorkflowDispatchScheduler(1); + + const settled = scheduler.beginSettling(); + + expect(settled).toBeInstanceOf(Promise); + let chained = false; + expect(() => { + void settled.then(() => { + chained = true; + }); + }).not.toThrow(); + await expect(settled).resolves.toBeUndefined(); + await vi.waitFor(() => expect(chained).toBe(true)); + }); + + it('refuses pause() once settlement has begun', async () => { + // A settling run awaits an fsync + a terminal manifest write. A pause + // accepted in that window would start a barrier beginSettling() never + // covered, and its 'paused' + canResume write would race the terminal one. + const beforePaused = vi.fn(async () => {}); + const scheduler = new WorkflowDispatchScheduler( + 1, + new AbortController().signal, + undefined, + beforePaused, + ); + + await scheduler.beginSettling(); + + expect(scheduler.pause()).toBe(false); + expect(scheduler.snapshot().state).toBe('running'); + expect(beforePaused).not.toHaveBeenCalled(); + }); + + it('does not start a pause barrier from a dispatch that settles after beginSettling', async () => { + // The other arm: pause() was already accepted, so the barrier is started + // by the in-flight dispatch's `.finally` rather than by pause() itself. + // That path has to honour the latch too. + const beforePaused = vi.fn(async () => {}); + let finish: (() => void) | undefined; + const scheduler = new WorkflowDispatchScheduler( + 1, + new AbortController().signal, + undefined, + beforePaused, + ); + + const running = scheduler.run( + () => + new Promise((resolve) => { + finish = resolve; + }), + ); + await vi.waitFor(() => expect(finish).toBeDefined()); + expect(scheduler.pause()).toBe(true); + expect(scheduler.snapshot().state).toBe('pausing'); + + await scheduler.beginSettling(); + finish?.(); + await running; + + await vi.waitFor(() => expect(scheduler.snapshot().inFlight).toBe(0)); + expect(beforePaused).not.toHaveBeenCalled(); + expect(scheduler.snapshot().state).toBe('pausing'); + }); + + it('refuses resume() once settlement has begun, leaving queued work undispatched', async () => { + // The third entrance to the latch. A run paused before settlement latched + // would otherwise flip back to 'running' inside the terminal write window, + // and pump() would dispatch a queued agent that settlement has already + // forbidden — real token spend and tool side effects, killed moments later + // by the settling caller's abort, plus a `true` ack for a terminal run. + let finish: (() => void) | undefined; + const scheduler = new WorkflowDispatchScheduler( + 1, + new AbortController().signal, + undefined, + async () => {}, + ); + + const running = scheduler.run( + () => + new Promise((resolve) => { + finish = resolve; + }), + ); + await vi.waitFor(() => expect(finish).toBeDefined()); + + // Second job stays queued behind the concurrency limit of 1. + const queued = vi.fn(async () => {}); + void scheduler.run(queued); + + expect(scheduler.pause()).toBe(true); + finish?.(); + await running; + await vi.waitFor(() => expect(scheduler.snapshot().state).toBe('paused')); + expect(scheduler.snapshot().queued).toBe(1); + + await scheduler.beginSettling(); + + expect(scheduler.resume()).toBe(false); + expect(scheduler.snapshot().state).toBe('paused'); + expect(scheduler.snapshot().queued).toBe(1); + expect(queued).not.toHaveBeenCalled(); + }); + + it('does not dispatch queued work from a dispatch that completes while settling', async () => { + // The fourth entrance, and the one that needs no pause at all: settlement + // normally begins from state 'running', so the completing in-flight job + // re-enters pump() from its own `.finally` and dispatches the next queued + // agent while the terminal manifest is still being written. The three + // pause-shaped guards never see this path. + let finish: (() => void) | undefined; + const controller = new AbortController(); + const scheduler = new WorkflowDispatchScheduler(1, controller.signal); + + const running = scheduler.run( + () => + new Promise((resolve) => { + finish = resolve; + }), + ); + await vi.waitFor(() => expect(finish).toBeDefined()); + + // Second job stays queued behind the concurrency limit of 1. + const queued = vi.fn(async () => {}); + const queuedResult = scheduler.run(queued); + + // Settlement latches with the run still 'running' and still in flight — + // exactly what the runner's catch path does before its fsync + terminal + // persist, with `controller.abort()` only reached later in `finally`. + await scheduler.beginSettling(); + expect(scheduler.snapshot().state).toBe('running'); + + finish?.(); + await running; + + await vi.waitFor(() => expect(scheduler.snapshot().inFlight).toBe(0)); + expect(queued).not.toHaveBeenCalled(); + expect(scheduler.snapshot().queued).toBe(1); + + // And the queued job is not stranded: the settling caller's abort rejects + // it, which is the outcome it would have reached anyway had it dispatched. + controller.abort(); + await expect(queuedResult).rejects.toThrow(/aborted/i); + }); + it('refuses pause() after the abort signal fired on a running scheduler', async () => { // Symmetric guard: an aborted running run must not transition into // pausing/paused. diff --git a/packages/core/src/agents/runtime/workflow-dispatch-scheduler.ts b/packages/core/src/agents/runtime/workflow-dispatch-scheduler.ts index 38aa1b17948..fb1d028e3eb 100644 --- a/packages/core/src/agents/runtime/workflow-dispatch-scheduler.ts +++ b/packages/core/src/agents/runtime/workflow-dispatch-scheduler.ts @@ -30,6 +30,9 @@ function abortError(): Error { export class WorkflowDispatchScheduler { private state: WorkflowDispatchState = 'running'; private inFlight = 0; + private pauseBarrierPending = false; + private pauseBarrier: Promise = Promise.resolve(); + private settling = false; private readonly queue: Job[] = []; private readonly gateWaiters: GateWaiter[] = []; private readonly stateListeners = new Set< @@ -40,6 +43,7 @@ export class WorkflowDispatchScheduler { private readonly limit: number, private readonly signal?: AbortSignal, onStateChange?: (snapshot: WorkflowDispatchSnapshot) => void, + private readonly beforePaused?: () => Promise, ) { if (!Number.isInteger(limit) || limit < 1) { throw new Error( @@ -85,14 +89,18 @@ export class WorkflowDispatchScheduler { } pause(): boolean { - if (this.state !== 'running' || this.signal?.aborted) return false; + if (this.state !== 'running' || this.settling || this.signal?.aborted) { + return false; + } this.setState('pausing'); - if (this.inFlight === 0) this.setState('paused'); + if (this.inFlight === 0) this.finishPause(); return true; } resume(): boolean { - if (this.state !== 'paused' || this.signal?.aborted) return false; + if (this.state !== 'paused' || this.settling || this.signal?.aborted) { + return false; + } this.setState('running'); while (this.gateWaiters.length > 0) { this.gateWaiters.shift()!.resolve(); @@ -109,6 +117,44 @@ export class WorkflowDispatchScheduler { }); } + /** + * Closes the pause window for good, then resolves once any in-flight pause + * barrier has settled. The barrier is entered from a dispatch's `.finally`, + * which cannot await it, so it runs detached from every caller: a run that + * completes or is cancelled during the barrier's fsync + manifest write has + * to join it here before writing its own terminal manifest, or the barrier's + * 'paused' + canResume write lands last and the settled run stays advertised + * as resumable. Never rejects — a failed barrier settles the run through its + * own error path. + * + * Joining the barrier is not enough on its own: it only covers the barrier + * that exists at call time. Settlement then awaits an fsync + a manifest + * write, and a `pause()` landing anywhere in that window would start a NEW + * barrier whose 'paused' + canResume write races the terminal one. So the + * latch is set here — before the join — and `pause()`, `finishPause()` and + * `resume()` all refuse once it is set. A settling run is no longer + * pausable; `pause()` reporting false surfaces as "could not be paused + * because its state changed", which is exactly what happened. + * + * `resume()` is the third entrance and refuses for the mirror-image reason: + * a run already paused when settlement latches would otherwise flip back to + * 'running' inside the terminal write window and `pump()` would dispatch a + * queued agent that settlement has forbidden — real token spend and tool + * side effects, immediately aborted by the settling caller's `finally`, and + * a `true` ack handed to a client for a run that is going terminal. + * + * `pump()` is the fourth, and it is the one that needs no pause at all: + * settlement usually begins from state 'running', so a dispatch completing + * inside the terminal write window re-enters `pump()` from its own + * `.finally` and dispatches the next queued agent directly. Guarding only + * the three pause entrances left the plain running case — the common one — + * spending tokens against a run already persisted terminal. + */ + beginSettling(): Promise { + this.settling = true; + return this.pauseBarrier; + } + snapshot(): WorkflowDispatchSnapshot { return { state: this.state, @@ -120,6 +166,11 @@ export class WorkflowDispatchScheduler { private pump(): void { while ( this.state === 'running' && + // Settlement is writing the terminal manifest. Queued jobs stay queued + // and are rejected by `abortPending()` when the settling caller's + // `finally` aborts the run, which is the outcome they would have got + // anyway — minus the dispatch. + !this.settling && this.inFlight < this.limit && this.queue.length > 0 ) { @@ -135,7 +186,7 @@ export class WorkflowDispatchScheduler { .finally(() => { this.inFlight--; if (this.state === 'pausing' && this.inFlight === 0) { - this.setState('paused'); + this.finishPause(); } this.pump(); }); @@ -149,6 +200,48 @@ export class WorkflowDispatchScheduler { for (const listener of [...this.stateListeners]) listener(snapshot); } + private finishPause(): void { + if ( + this.state !== 'pausing' || + this.inFlight !== 0 || + this.pauseBarrierPending || + // The run is settling terminal: `beginSettling()` already joined the + // barrier that existed then, and everything after it is the terminal + // manifest write. Starting a barrier now would durably write 'paused' + + // canResume in that window and race the terminal write. + this.settling || + // pause()/resume()/run() all refuse once the run is aborted; the + // in-flight `.finally` path reaches here without that check. Running + // the barrier anyway would durably write 'paused' + canResume for a + // run the user cancelled, and that write can land after the + // 'cancelled' one — leaving a cancelled run that resume() would + // re-execute. + this.signal?.aborted + ) { + return; + } + if (!this.beforePaused) { + this.setState('paused'); + return; + } + this.pauseBarrierPending = true; + this.pauseBarrier = Promise.resolve() + .then(this.beforePaused) + .then(() => { + this.pauseBarrierPending = false; + if ( + this.state === 'pausing' && + this.inFlight === 0 && + !this.signal?.aborted + ) { + this.setState('paused'); + } + }) + .catch(() => { + this.pauseBarrierPending = false; + }); + } + private abortPending(): void { const error = abortError(); while (this.queue.length > 0) this.queue.shift()!.reject(error); diff --git a/packages/core/src/agents/runtime/workflow-journal.test.ts b/packages/core/src/agents/runtime/workflow-journal.test.ts index e38d1d1c3ed..40912dacfae 100644 --- a/packages/core/src/agents/runtime/workflow-journal.test.ts +++ b/packages/core/src/agents/runtime/workflow-journal.test.ts @@ -4,8 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; -import { promises as fs } from 'node:fs'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { constants as fsConstants, promises as fs } from 'node:fs'; +import { createHash } from 'node:crypto'; import * as os from 'node:os'; import * as path from 'node:path'; import { @@ -14,7 +15,11 @@ import { deriveArgsSeed, buildReplay, WorkflowJournal, + JOURNAL_FORMAT_VERSION, JOURNAL_KEY_VERSION, + MAX_WORKFLOW_JOURNAL_BYTES, + readBounded, + type JournalCheckpoint, type JournalEntry, } from './workflow-journal.js'; @@ -109,6 +114,7 @@ describe('WorkflowJournal', () => { dir = await fs.mkdtemp(path.join(os.tmpdir(), 'wf-journal-')); }); afterEach(async () => { + vi.restoreAllMocks(); await fs.rm(dir, { recursive: true, force: true }); }); @@ -126,12 +132,438 @@ describe('WorkflowJournal', () => { expect(replay.started.get('k1')).toHaveLength(1); }); + it.skipIf(process.platform === 'win32')( + 'opens existing journals nonblocking for reads and appends', + async () => { + const journalPath = path.join(dir, 'nonblocking', 'journal.jsonl'); + const journal = new WorkflowJournal(journalPath); + await journal.append({ type: 'started', key: 'k1', agentId: '1' }); + const realOpen = fs.open.bind(fs); + const flags: number[] = []; + const openSpy = vi + .spyOn(fs, 'open') + .mockImplementation((path, flag, mode) => { + if (typeof flag === 'number') flags.push(flag); + return realOpen(path, flag, mode); + }); + try { + await journal.append({ type: 'started', key: 'k2', agentId: '2' }); + await journal.load(); + } finally { + openSpy.mockRestore(); + } + + expect(flags).toContain( + fsConstants.O_APPEND | + fsConstants.O_WRONLY | + fsConstants.O_NOFOLLOW | + fsConstants.O_NONBLOCK, + ); + expect(flags).toContain( + fsConstants.O_RDONLY | fsConstants.O_NOFOLLOW | fsConstants.O_NONBLOCK, + ); + }, + ); + it('load on a missing file returns empty maps', async () => { const j = new WorkflowJournal(path.join(dir, 'nope.jsonl')); const replay = await j.load(); expect(replay.results.size).toBe(0); expect(replay.started.size).toBe(0); }); + + it('continues from an empty checkpoint when the journal file is absent', async () => { + const journalPath = path.join(dir, 'empty-resume', 'journal.jsonl'); + const checkpoint = await new WorkflowJournal(journalPath).flush(); + const resumed = new WorkflowJournal(journalPath); + + await expect(resumed.load(checkpoint)).resolves.toMatchObject({ + results: expect.any(Map), + started: expect.any(Map), + }); + await expect(resumed.flush()).resolves.toMatchObject({ + byteLength: 0, + integrity: 'complete', + }); + await expect( + resumed.append({ type: 'started', key: 'v2:first', agentId: '1' }), + ).resolves.toBeUndefined(); + expect((await resumed.flush()).byteLength).toBeGreaterThan(0); + }); + + it('orders concurrent appends and flushes exactly the queued prefix', async () => { + const journalPath = path.join(dir, 'ordered', 'journal.jsonl'); + const j = new WorkflowJournal(journalPath); + const entries: JournalEntry[] = [ + { type: 'started', key: 'v2:k1', agentId: '1' }, + { type: 'result', key: 'v2:k1', agentId: '1', result: 'first' }, + { type: 'started', key: 'v2:k2', agentId: '2' }, + ]; + + const writes = entries.map((entry) => j.append(entry)); + const checkpoint = await j.flush(); + await Promise.all(writes); + + const bytes = await fs.readFile(journalPath); + expect(bytes.toString('utf8')).toBe( + entries.map((entry) => `${JSON.stringify(entry)}\n`).join(''), + ); + expect(checkpoint).toEqual({ + version: JOURNAL_FORMAT_VERSION, + keyVersion: JOURNAL_KEY_VERSION, + byteLength: bytes.byteLength, + sha256: createHash('sha256').update(bytes).digest('hex'), + integrity: 'complete', + }); + }); + + it('continues the queue after a write failure and remembers the failure', async () => { + const blockedParent = path.join(dir, 'blocked'); + await fs.writeFile(blockedParent, 'not a directory'); + const journalPath = path.join(blockedParent, 'journal.jsonl'); + const j = new WorkflowJournal(journalPath); + + await expect( + j.append({ type: 'started', key: 'v2:failed', agentId: '1' }), + ).rejects.toBeDefined(); + await fs.rm(blockedParent); + await fs.mkdir(blockedParent); + await expect( + j.append({ type: 'started', key: 'v2:survived', agentId: '2' }), + ).resolves.toBeUndefined(); + + const checkpoint = await j.flush(); + expect(checkpoint.integrity).toBe('failed'); + expect(checkpoint.error).toBeTruthy(); + await expect(j.load(checkpoint)).rejects.toThrow(/integrity/i); + expect(await fs.readFile(journalPath, 'utf8')).toContain('v2:survived'); + }); + + it('refuses journal and parent symlinks without modifying outside files', async () => { + const outside = await fs.mkdtemp(path.join(os.tmpdir(), 'wf-outside-')); + try { + for (const kind of ['journal', 'run-dir', 'root'] as const) { + const caseRoot = path.join(dir, kind); + const outsideRun = path.join(outside, kind, 'wf_dead'); + await fs.mkdir(outsideRun, { recursive: true }); + const canary = path.join(outsideRun, 'journal.jsonl'); + await fs.writeFile(canary, 'CANARY\n'); + let journalPath: string; + if (kind === 'journal') { + const runDir = path.join(caseRoot, 'wf_dead'); + await fs.mkdir(runDir, { recursive: true }); + journalPath = path.join(runDir, 'journal.jsonl'); + await fs.symlink(canary, journalPath); + } else if (kind === 'run-dir') { + await fs.mkdir(caseRoot, { recursive: true }); + await fs.symlink(outsideRun, path.join(caseRoot, 'wf_dead')); + journalPath = path.join(caseRoot, 'wf_dead', 'journal.jsonl'); + } else { + await fs.mkdir(path.dirname(caseRoot), { recursive: true }); + const outsideRoot = path.dirname(outsideRun); + await fs.symlink(outsideRoot, caseRoot); + journalPath = path.join(caseRoot, 'wf_dead', 'journal.jsonl'); + } + const journal = new WorkflowJournal(journalPath); + await expect( + journal.append({ + type: 'started', + key: `v2:${kind}`, + agentId: '1', + }), + ).rejects.toBeDefined(); + expect((await journal.flush()).integrity).toBe('failed'); + await expect(fs.readFile(canary, 'utf8')).resolves.toBe('CANARY\n'); + } + } finally { + await fs.rm(outside, { recursive: true, force: true }); + } + }); + + it('refuses to buffer a journal above the read cap', async () => { + const runDir = path.join(dir, 'oversized', 'wf_dead'); + await fs.mkdir(runDir, { recursive: true }); + const journalPath = path.join(runDir, 'journal.jsonl'); + // Sparse file: the apparent size clears the cap without allocating it. + // A repo-shipped journal can be arbitrarily large, and the resume path + // buffers it before any byteLength/hash check can reject it. + const handle = await fs.open(journalPath, 'w'); + try { + await handle.truncate(MAX_WORKFLOW_JOURNAL_BYTES + 1); + } finally { + await handle.close(); + } + const journal = new WorkflowJournal(journalPath); + + // The manifest's `canResume` load — the reachable-by-listing path. + await expect( + journal.load({ + version: JOURNAL_FORMAT_VERSION, + keyVersion: JOURNAL_KEY_VERSION, + byteLength: 0, + sha256: createHash('sha256').update(Buffer.alloc(0)).digest('hex'), + integrity: 'complete', + }), + ).rejects.toThrow(/too large/i); + const checkpoint = await journal.flush(); + expect(checkpoint.integrity).toBe('failed'); + expect(checkpoint.error).toMatch(/too large/i); + }); + + it('refuses a hardlinked journal without modifying its other name', async () => { + const runDir = path.join(dir, 'hardlink', 'wf_dead'); + const canary = path.join(dir, 'outside-canary.txt'); + const journalPath = path.join(runDir, 'journal.jsonl'); + await fs.mkdir(runDir, { recursive: true }); + await fs.writeFile(canary, 'CANARY\n'); + await fs.link(canary, journalPath); + + const journal = new WorkflowJournal(journalPath); + await expect( + journal.append({ type: 'started', key: 'v2:hardlink', agentId: '1' }), + ).rejects.toThrow(/unsafe/i); + expect((await journal.flush()).integrity).toBe('failed'); + await expect(fs.readFile(canary, 'utf8')).resolves.toBe('CANARY\n'); + }); + + it('validates a committed prefix and ignores a later suffix', async () => { + const journalPath = path.join(dir, 'prefix', 'journal.jsonl'); + const j = new WorkflowJournal(journalPath); + await j.append({ type: 'started', key: 'v2:k1', agentId: '1' }); + await j.append({ + type: 'result', + key: 'v2:k1', + agentId: '1', + result: 'committed', + }); + const checkpoint = await j.flush(); + await j.append({ + type: 'result', + key: 'v2:k1', + agentId: '2', + result: 'suffix', + }); + + const replay = await j.load(checkpoint); + expect(replay.results.get('v2:k1')?.result).toBe('committed'); + expect(replay.started.get('v2:k1')).toHaveLength(1); + }); + + it('discards an uncommitted suffix before publishing the resumed checkpoint', async () => { + const journalPath = path.join(dir, 'resume', 'journal.jsonl'); + const original = new WorkflowJournal(journalPath); + await original.append({ + type: 'started', + key: 'v2:k1', + agentId: '1', + }); + const committed = await original.flush(); + await fs.appendFile(journalPath, '{partial'); + + const resumed = new WorkflowJournal(journalPath); + const replay = await resumed.load(committed); + expect(replay.started.get('v2:k1')).toHaveLength(1); + + const repaired = await resumed.flush(); + expect(repaired.byteLength).toBe(committed.byteLength); + await expect(resumed.load(repaired)).resolves.toMatchObject({ + results: expect.any(Map), + started: expect.any(Map), + }); + + await resumed.append({ + type: 'result', + key: 'v2:k1', + agentId: '1', + result: 'done', + }); + const extended = await resumed.flush(); + const extendedReplay = await resumed.load(extended); + expect(extendedReplay.results.get('v2:k1')?.result).toBe('done'); + expect(extendedReplay.started.get('v2:k1')).toHaveLength(1); + }); + + it('returns failed integrity instead of rejecting when suffix repair is unsafe', async () => { + const journalPath = path.join(dir, 'unsafe-repair', 'journal.jsonl'); + const original = new WorkflowJournal(journalPath); + await original.append({ type: 'started', key: 'v2:k1', agentId: '1' }); + const committed = await original.flush(); + await original.append({ + type: 'result', + key: 'v2:k1', + agentId: '1', + result: 'suffix', + }); + + const resumed = new WorkflowJournal(journalPath); + await resumed.load(committed); + const canary = path.join(dir, 'repair-canary.txt'); + await fs.writeFile(canary, 'CANARY\n'); + await fs.rm(journalPath); + await fs.link(canary, journalPath); + + await expect(resumed.flush()).resolves.toMatchObject({ + integrity: 'failed', + error: expect.stringMatching(/unsafe/i), + }); + await expect(fs.readFile(canary, 'utf8')).resolves.toBe('CANARY\n'); + }); + + it.runIf(process.platform !== 'win32')( + 'does not overwrite a journal inode replaced after its prefix is read', + async () => { + const journalPath = path.join(dir, 'replacement-race', 'journal.jsonl'); + const original = new WorkflowJournal(journalPath); + await original.append({ type: 'started', key: 'v2:k1', agentId: '1' }); + const committed = await original.flush(); + await original.append({ + type: 'result', + key: 'v2:k1', + agentId: '1', + result: 'old-suffix', + }); + + const resumed = new WorkflowJournal(journalPath); + await resumed.load(committed); + const replacement = path.join(path.dirname(journalPath), 'replacement'); + const replacementBytes = Buffer.from('REPLACEMENT\n'); + await fs.writeFile(replacement, replacementBytes); + const realOpen = fs.open.bind(fs); + vi.spyOn(fs, 'open').mockImplementationOnce( + async (...args: Parameters) => { + const handle = await realOpen(...args); + let renamed = false; + return { + stat: (...statArgs: Parameters) => + handle.stat(...statArgs), + // Hooks `read` rather than `readFile`: the committed prefix is + // now read in bounded chunks, but the instant being simulated + // is the same one — the bytes are in hand and the pathname is + // about to stop pointing at the inode they came from. + read: async ( + buffer: Buffer, + offset: number, + length: number, + position: number | null, + ) => { + const result = await handle.read( + buffer, + offset, + length, + position, + ); + if (!renamed) { + renamed = true; + await fs.rename(replacement, journalPath); + } + return result; + }, + close: () => handle.close(), + } as unknown as Awaited>; + }, + ); + + await expect(resumed.flush()).resolves.toMatchObject({ + integrity: 'failed', + error: expect.stringMatching(/changed/i), + }); + await expect(fs.readFile(journalPath)).resolves.toEqual(replacementBytes); + }, + ); + + it('rejects committed bytes that are not valid UTF-8', async () => { + // The SHA-256 authenticates the raw bytes, but `toString('utf8')` + // rewrites invalid sequences to U+FFFD — so a lossy decode could hand + // back JSON that parses cleanly and resumes with data the hash never + // covered. The checkpoint here is built over the corrupt bytes on + // purpose, so the hash matches and the decode is the only thing left + // standing between them and the replay. + const journalPath = path.join(dir, 'bad-utf8', 'journal.jsonl'); + await fs.mkdir(path.dirname(journalPath), { recursive: true }); + const entry = JSON.stringify({ + type: 'result', + key: 'v2:k1', + agentId: '1', + result: 'PLACEHOLDER', + }); + // A lone 0x80 continuation byte, inside the result string. + const bytes = Buffer.concat([ + Buffer.from(entry.replace('PLACEHOLDER', 'x')), + Buffer.from('\n'), + ]); + bytes[bytes.indexOf(0x78)] = 0x80; + await fs.writeFile(journalPath, bytes); + + const journal = new WorkflowJournal(journalPath); + await expect( + journal.load({ + version: JOURNAL_FORMAT_VERSION, + keyVersion: JOURNAL_KEY_VERSION, + byteLength: bytes.byteLength, + sha256: createHash('sha256').update(bytes).digest('hex'), + integrity: 'complete', + }), + ).rejects.toThrow(/UTF-8/i); + }); + + it('fails closed for truncated or hash-mismatched committed bytes', async () => { + const journalPath = path.join(dir, 'corrupt', 'journal.jsonl'); + const j = new WorkflowJournal(journalPath); + await j.append({ type: 'started', key: 'v2:k1', agentId: '1' }); + const checkpoint = await j.flush(); + const bytes = await fs.readFile(journalPath); + + await fs.writeFile(journalPath, bytes.subarray(0, bytes.byteLength - 1)); + await expect(j.load(checkpoint)).rejects.toThrow(/truncated/i); + + const changed = Buffer.from(bytes); + changed[changed.indexOf('1')] = '2'.charCodeAt(0); + await fs.writeFile(journalPath, changed); + await expect(j.load(checkpoint)).rejects.toThrow(/hash/i); + }); + + it('does not call a refused read "truncated", and names the invariant', async () => { + const journalPath = path.join(dir, 'refused', 'journal.jsonl'); + const j = new WorkflowJournal(journalPath); + await j.append({ type: 'started', key: 'v2:k1', agentId: '1' }); + const checkpoint = await j.flush(); + + // Same bytes, same length, same hash — only the file's identity is unsafe, + // so nothing here is truncation. + const canary = path.join(dir, 'refused-canary.jsonl'); + await fs.copyFile(journalPath, canary); + await fs.rm(journalPath); + await fs.link(canary, journalPath); + + const failure = await j.load(checkpoint).then( + () => undefined, + (error: unknown) => error as Error, + ); + expect(failure?.message).toMatch(/recovery failed/i); + expect(failure?.message).not.toMatch(/truncated/i); + expect(failure?.message).toMatch(/hard link count 2, expected 1/); + }); + + it.each([ + ['invalid JSON', '{not-json}\n'], + ['invalid entry', '{"type":"started","key":3,"agentId":"1"}\n'], + ])('rejects %s even when its checkpoint hash matches', async (_, content) => { + const journalPath = path.join(dir, 'invalid', 'journal.jsonl'); + await fs.mkdir(path.dirname(journalPath), { recursive: true }); + const bytes = Buffer.from(content, 'utf8'); + await fs.writeFile(journalPath, bytes); + const checkpoint: JournalCheckpoint = { + version: JOURNAL_FORMAT_VERSION, + keyVersion: JOURNAL_KEY_VERSION, + byteLength: bytes.byteLength, + sha256: createHash('sha256').update(bytes).digest('hex'), + integrity: 'complete', + }; + + await expect( + new WorkflowJournal(journalPath).load(checkpoint), + ).rejects.toThrow(/invalid/i); + }); }); // #7: the resume prefix chain is seeded with the run's args, so a resume with @@ -149,3 +581,99 @@ describe('deriveArgsSeed', () => { expect(k1).not.toBe(k2); // same prompt+opts, different args → different key }); }); + +describe('readBounded', () => { + let dir: string; + beforeEach(async () => { + dir = await fs.mkdtemp(path.join(os.tmpdir(), 'wf-bounded-')); + }); + afterEach(async () => { + await fs.rm(dir, { recursive: true, force: true }); + }); + + async function withFile( + bytes: Buffer, + run: (handle: Awaited>) => Promise, + ): Promise { + const filePath = path.join(dir, 'artifact'); + await fs.writeFile(filePath, bytes); + const handle = await fs.open(filePath, 'r'); + try { + return await run(handle); + } finally { + await handle.close(); + } + } + + it('returns the whole file when it fits', async () => { + const bytes = Buffer.from('hello world'); + const out = await withFile(bytes, (h) => readBounded(h, bytes.byteLength)); + expect(out).toEqual(bytes); + }); + + it('returns an empty buffer for an empty file', async () => { + const out = await withFile(Buffer.alloc(0), (h) => readBounded(h, 16)); + expect(out).toEqual(Buffer.alloc(0)); + }); + + it('accepts a file of exactly the limit and rejects one byte more', async () => { + const bytes = Buffer.alloc(64, 0x61); + expect(await withFile(bytes, (h) => readBounded(h, 64))).toEqual(bytes); + expect(await withFile(bytes, (h) => readBounded(h, 63))).toBeNull(); + }); + + it('bounds on the bytes read, not on a size observed beforehand', async () => { + // The point of the change: `fstat` and the read are two observations of + // an inode its owner can still be growing, so a pre-read size cannot + // bound the buffer. Here the handle reports a size well under the limit + // and then serves far more than it. + const bytes = Buffer.alloc(4096, 0x62); + const out = await withFile(bytes, async (handle) => { + vi.spyOn(handle, 'stat').mockResolvedValue({ + size: 8, + } as unknown as Awaited>); + return readBounded(handle, 1024); + }); + expect(out).toBeNull(); + }); + + it('keeps reading through short reads', async () => { + // `read()` may return short of EOF, so a single call cannot tell a + // complete small file from a partially read large one — the reason + // this is a loop and not one `handle.read()`. + const bytes = Buffer.from('abcdefghij'); + const out = await withFile(bytes, async (handle) => { + const real = handle.read.bind(handle); + vi.spyOn(handle, 'read').mockImplementation((( + buffer: Buffer, + offset: number, + _length: number, + position: number | null, + ) => real(buffer, offset, 1, position)) as typeof handle.read); + return readBounded(handle, 32); + }); + expect(out).toEqual(bytes); + }); + + it('never buffers more than one byte past the limit', async () => { + // Reading `limit + 1` is what makes "too large" detectable; reading + // more would reintroduce the unbounded buffering being fixed. + const bytes = Buffer.alloc(4096, 0x63); + let served = 0; + await withFile(bytes, async (handle) => { + const real = handle.read.bind(handle); + vi.spyOn(handle, 'read').mockImplementation((async ( + buffer: Buffer, + offset: number, + length: number, + position: number | null, + ) => { + const result = await real(buffer, offset, length, position); + served += result.bytesRead; + return result; + }) as typeof handle.read); + return readBounded(handle, 100); + }); + expect(served).toBe(101); + }); +}); diff --git a/packages/core/src/agents/runtime/workflow-journal.ts b/packages/core/src/agents/runtime/workflow-journal.ts index 15a250e4c2c..0e710e0da8a 100644 --- a/packages/core/src/agents/runtime/workflow-journal.ts +++ b/packages/core/src/agents/runtime/workflow-journal.ts @@ -32,8 +32,18 @@ */ import { createHash } from 'node:crypto'; -import { read, writeLine } from '../../utils/jsonl-utils.js'; +import { + constants as fsConstants, + promises as fs, + lstatSync, + realpathSync, + type Stats, +} from 'node:fs'; +import type { FileHandle } from 'node:fs/promises'; +import * as path from 'node:path'; +import { parseLineTolerant } from '../../utils/jsonl-utils.js'; import { createDebugLogger } from '../../utils/debugLogger.js'; +import { atomicWriteFile } from '../../utils/atomicFileWrite.js'; import type { WorkflowAgentOpts } from './workflow-sandbox.js'; const debugLogger = createDebugLogger('WORKFLOW_JOURNAL'); @@ -41,6 +51,35 @@ const debugLogger = createDebugLogger('WORKFLOW_JOURNAL'); /** Journal-format version tag, prefixed onto every key. */ export const JOURNAL_KEY_VERSION = 'v2'; +/** Durable checkpoint schema for committed journal prefixes. */ +export const JOURNAL_FORMAT_VERSION = 1; + +/** + * Upper bound on a `journal.jsonl` read into memory. The journal lives under + * `/workflows`, which ships with the repository, so a clone can + * carry a multi-gigabyte journal alongside a small, structurally valid + * `canResume` manifest — and the listing path journal-loads every such + * manifest. Buffering that aborts the process before any `byteLength`/hash + * check can reject it, so the size is checked first. The bound clears any + * ordinary run — the default dispatch cap is 1000, i.e. two lines each — but + * it is not unreachable: `QWEN_CODE_MAX_WORKFLOW_AGENTS` raises that cap to + * `HARD_MAX_AGENTS_PER_RUN_CEILING` (10,000, so 20,000 lines) and per-line + * result size is unbounded, so a run configured that way can cross it and + * lose resumability with an intact journal on disk. Fixing that properly + * means streaming the checkpoint hash and reading only up to + * `checkpoint.byteLength` instead of buffering the whole file. + */ +export const MAX_WORKFLOW_JOURNAL_BYTES = 128 * 1024 * 1024; + +export interface JournalCheckpoint { + version: typeof JOURNAL_FORMAT_VERSION; + keyVersion: typeof JOURNAL_KEY_VERSION; + byteLength: number; + sha256: string; + integrity: 'complete' | 'failed'; + error?: string; +} + export interface JournalStartedEntry { type: 'started'; key: string; @@ -161,6 +200,391 @@ export function buildReplay(entries: JournalEntry[]): JournalReplay { return { results, started }; } +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +function isJournalEntry(value: unknown): value is JournalEntry { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + return false; + } + const entry = value as Record; + if ( + typeof entry['key'] !== 'string' || + entry['key'].length === 0 || + typeof entry['agentId'] !== 'string' || + entry['agentId'].length === 0 + ) { + return false; + } + if (entry['type'] === 'started') return true; + return entry['type'] === 'result' && Object.hasOwn(entry, 'result'); +} + +function validateCheckpoint(checkpoint: JournalCheckpoint): void { + if ( + checkpoint.version !== JOURNAL_FORMAT_VERSION || + checkpoint.keyVersion !== JOURNAL_KEY_VERSION || + !Number.isSafeInteger(checkpoint.byteLength) || + checkpoint.byteLength < 0 || + !/^[0-9a-f]{64}$/.test(checkpoint.sha256) || + (checkpoint.integrity !== 'complete' && checkpoint.integrity !== 'failed') + ) { + throw new Error('Invalid workflow journal checkpoint'); + } + if (checkpoint.integrity !== 'complete') { + throw new Error( + `Workflow journal checkpoint integrity failed${checkpoint.error ? `: ${checkpoint.error}` : ''}`, + ); + } +} + +const UTF8_STRICT = new TextDecoder('utf-8', { fatal: true }); + +function parseCommittedEntries(bytes: Buffer): JournalEntry[] { + if (bytes.byteLength === 0) return []; + if (bytes[bytes.byteLength - 1] !== 0x0a) { + throw new Error('Invalid workflow journal: committed line is truncated'); + } + // The SHA-256 above authenticates these exact bytes, and + // `Buffer.toString('utf8')` silently rewrites invalid sequences to + // U+FFFD — so a lossy decode can hand back JSON that parses cleanly and + // resumes with data the hash never covered. Decode fatally instead: a + // journal that is not valid UTF-8 is corrupt, not repairable. + let text: string; + try { + text = UTF8_STRICT.decode(bytes); + } catch { + throw new Error('Invalid workflow journal: committed bytes are not UTF-8'); + } + const lines = text.split('\n'); + lines.pop(); + return lines.map((line, index) => { + if (line.length === 0) { + throw new Error(`Invalid workflow journal entry at line ${index + 1}`); + } + let parsed: unknown; + try { + parsed = JSON.parse(line); + } catch { + throw new Error(`Invalid workflow journal JSON at line ${index + 1}`); + } + if (!isJournalEntry(parsed)) { + throw new Error(`Invalid workflow journal entry at line ${index + 1}`); + } + return parsed; + }); +} + +interface JournalParentGuard { + assertUnchanged(): void; +} + +function assertSafeJournalFile(stat: Stats, journalPath: string): void { + if (!stat.isFile() || stat.isSymbolicLink() || stat.nlink !== 1) { + // Name the invariant that failed. This message is the only thing that + // survives into the durable checkpoint `error` and the `/workflows` + // resumeBlockedReason, so a bare "unsafe" leaves an operator (or a CI + // failure) with no way to tell a symlink swap from a stray hard link. + const why = !stat.isFile() + ? `not a regular file (mode 0o${(stat.mode & 0o170000).toString(8)})` + : stat.isSymbolicLink() + ? 'symbolic link' + : `hard link count ${stat.nlink}, expected 1`; + throw new Error(`Unsafe workflow journal path: ${journalPath} (${why})`); + } +} + +function assertSameJournalFile( + expected: Stats, + actual: Stats, + journalPath: string, +): void { + if (expected.dev !== actual.dev || expected.ino !== actual.ino) { + throw new Error(`Workflow journal path changed: ${journalPath}`); + } +} + +async function guardJournalParents( + journalPath: string, + create: boolean, +): Promise { + const runDir = path.dirname(journalPath); + const root = path.dirname(runDir); + if (create) { + await fs.mkdir(root, { recursive: true, mode: 0o700 }); + } + let rootStat; + try { + rootStat = await fs.lstat(root); + } catch (error) { + if (!create && (error as NodeJS.ErrnoException).code === 'ENOENT') { + return undefined; + } + throw error; + } + if (!rootStat.isDirectory() || rootStat.isSymbolicLink()) { + throw new Error(`Unsafe workflow journal root: ${root}`); + } + if (create) { + await fs.mkdir(runDir, { mode: 0o700 }).catch((error: unknown) => { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + }); + } + let runStat; + try { + runStat = await fs.lstat(runDir); + } catch (error) { + if (!create && (error as NodeJS.ErrnoException).code === 'ENOENT') { + return undefined; + } + throw error; + } + if (!runStat.isDirectory() || runStat.isSymbolicLink()) { + throw new Error(`Unsafe workflow journal directory: ${runDir}`); + } + const [realRoot, realRunDir] = await Promise.all([ + fs.realpath(root), + fs.realpath(runDir), + ]); + if (path.dirname(realRunDir) !== realRoot) { + throw new Error(`Workflow journal directory escapes its root: ${runDir}`); + } + const rootIdentity = { dev: rootStat.dev, ino: rootStat.ino, realRoot }; + const runIdentity = { dev: runStat.dev, ino: runStat.ino, realRunDir }; + return { + assertUnchanged(): void { + const currentRoot = lstatSync(root); + const currentRun = lstatSync(runDir); + if ( + currentRoot.isSymbolicLink() || + !currentRoot.isDirectory() || + currentRoot.dev !== rootIdentity.dev || + currentRoot.ino !== rootIdentity.ino || + realpathSync(root) !== rootIdentity.realRoot || + currentRun.isSymbolicLink() || + !currentRun.isDirectory() || + currentRun.dev !== runIdentity.dev || + currentRun.ino !== runIdentity.ino || + realpathSync(runDir) !== runIdentity.realRunDir + ) { + throw new Error(`Workflow journal parent changed: ${runDir}`); + } + }, + }; +} + +async function guardJournalTarget( + journalPath: string, +): Promise { + let expected: Stats | undefined; + try { + expected = await fs.lstat(journalPath); + assertSafeJournalFile(expected, journalPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + return { + assertUnchanged(): void { + let current: Stats | undefined; + try { + current = lstatSync(journalPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + if (!expected && !current) return; + if (!expected || !current) { + throw new Error(`Workflow journal path changed: ${journalPath}`); + } + assertSafeJournalFile(current, journalPath); + assertSameJournalFile(expected, current, journalPath); + }, + }; +} + +async function readJournalBytes(journalPath: string): Promise { + const guard = await guardJournalParents(journalPath, false); + if (!guard) return Buffer.alloc(0); + let before: Stats; + try { + before = await fs.lstat(journalPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + guard.assertUnchanged(); + return Buffer.alloc(0); + } + throw error; + } + assertSafeJournalFile(before, journalPath); + let handle; + try { + handle = await fs.open( + journalPath, + fsConstants.O_RDONLY | + (process.platform === 'win32' + ? 0 + : fsConstants.O_NOFOLLOW | fsConstants.O_NONBLOCK), + ); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + guard.assertUnchanged(); + return Buffer.alloc(0); + } + throw error; + } + try { + guard.assertUnchanged(); + const [opened, current] = await Promise.all([ + handle.stat(), + fs.lstat(journalPath), + ]); + assertSafeJournalFile(opened, journalPath); + assertSafeJournalFile(current, journalPath); + assertSameJournalFile(before, current, journalPath); + assertSameJournalFile(opened, current, journalPath); + // Cheap pre-rejection only. `opened.size` and the bytes a read would + // return are two observations of an inode that stays writable to its + // owner, so a size taken before the read cannot bound the read. + if (opened.size > MAX_WORKFLOW_JOURNAL_BYTES) { + throw new Error( + `Workflow journal is too large: ${journalPath} (${opened.size} bytes, limit ${MAX_WORKFLOW_JOURNAL_BYTES})`, + ); + } + const bytes = await readBounded(handle, MAX_WORKFLOW_JOURNAL_BYTES); + if (bytes === null) { + throw new Error( + `Workflow journal is too large: ${journalPath} (over the ${MAX_WORKFLOW_JOURNAL_BYTES} byte limit)`, + ); + } + return bytes; + } finally { + await handle.close(); + } +} + +/** Chunk size for {@link readBounded}; unrelated to either cap. */ +const BOUNDED_READ_CHUNK_BYTES = 1024 * 1024; + +/** + * Read an already-identity-checked handle to EOF, or give up at `limit`. + * + * Returns null when the file is over the limit, having buffered at most + * one byte past it. The caps in this module are enforced here rather than + * from a preceding `fstat` because the size and the bytes are separate + * observations of an inode another process may still be growing — the + * pre-read check is a cheap rejection, not the bound. + * + * Loops because `read()` is allowed to return short of EOF, so a single + * call cannot tell a complete small file from a partially read large one. + */ +export async function readBounded( + handle: FileHandle, + limit: number, +): Promise { + const chunks: Buffer[] = []; + let total = 0; + // `limit + 1 - total` keeps the last read to the single byte that proves + // the file is over the limit, instead of allocating the whole cap up + // front for what is usually a few hundred bytes. + while (total <= limit) { + const chunk = Buffer.allocUnsafe( + Math.min(BOUNDED_READ_CHUNK_BYTES, limit + 1 - total), + ); + const { bytesRead } = await handle.read(chunk, 0, chunk.length, null); + if (bytesRead === 0) break; + total += bytesRead; + chunks.push( + bytesRead === chunk.length ? chunk : chunk.subarray(0, bytesRead), + ); + } + if (total > limit) return null; + return Buffer.concat(chunks, total); +} + +async function appendJournalLine( + journalPath: string, + entry: JournalEntry, +): Promise { + const guard = await guardJournalParents(journalPath, true); + if (!guard) throw new Error(`Workflow journal directory is unavailable.`); + let before: Stats | undefined; + try { + before = await fs.lstat(journalPath); + assertSafeJournalFile(before, journalPath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + const handle = await fs.open( + journalPath, + fsConstants.O_APPEND | + fsConstants.O_WRONLY | + (before ? 0 : fsConstants.O_CREAT | fsConstants.O_EXCL) | + (process.platform === 'win32' + ? 0 + : fsConstants.O_NOFOLLOW | fsConstants.O_NONBLOCK), + 0o600, + ); + try { + guard.assertUnchanged(); + const [opened, current] = await Promise.all([ + handle.stat(), + fs.lstat(journalPath), + ]); + assertSafeJournalFile(opened, journalPath); + assertSafeJournalFile(current, journalPath); + if (before) assertSameJournalFile(before, current, journalPath); + assertSameJournalFile(opened, current, journalPath); + await handle.chmod(0o600); + await handle.writeFile(Buffer.from(`${JSON.stringify(entry)}\n`, 'utf8')); + await handle.sync(); + } finally { + await handle.close(); + } +} + +async function readCommittedPrefix( + journalPath: string, + checkpoint: JournalCheckpoint, +): Promise<{ bytes: Buffer; committed: Buffer; replay: JournalReplay }> { + validateCheckpoint(checkpoint); + let bytes: Buffer; + try { + bytes = await readJournalBytes(journalPath); + } catch (error) { + if ( + (error as NodeJS.ErrnoException).code === 'ENOENT' && + checkpoint.byteLength === 0 + ) { + bytes = Buffer.alloc(0); + } else { + // Only the byteLength shortfall below is genuine truncation. Safety + // rejections (unsafe path, parent changed) and I/O errors (EACCES) reach + // here too, and this message ends up verbatim in the durable checkpoint + // `error` and in the `/workflows` resumeBlockedReason — calling them + // "truncated" tells the operator they lost data when the read was in + // fact refused. + throw new Error( + `Workflow journal recovery failed: ${errorMessage(error)}`, + ); + } + } + if (bytes.byteLength < checkpoint.byteLength) { + throw new Error( + `Workflow journal is truncated: expected ${checkpoint.byteLength} bytes, got ${bytes.byteLength}`, + ); + } + const committed = bytes.subarray(0, checkpoint.byteLength); + const sha256 = createHash('sha256').update(committed).digest('hex'); + if (sha256 !== checkpoint.sha256) { + throw new Error('Workflow journal checkpoint hash mismatch'); + } + return { + bytes, + committed, + replay: buildReplay(parseCommittedEntries(committed)), + }; +} + /** * Append-only JSONL journal for one workflow run. Reads tolerate a missing * file (fresh run); appends are fire-and-forget at the call site (the @@ -168,12 +592,28 @@ export function buildReplay(entries: JournalEntry[]): JournalReplay { * failure must not fail the dispatch). */ export class WorkflowJournal { + private tail: Promise = Promise.resolve(); + private writeError?: string; + private resumeCheckpoint?: JournalCheckpoint; + constructor(readonly path: string) {} /** Load + parse all entries into replay maps. Empty maps if no file. */ - async load(): Promise { + async load(checkpoint?: JournalCheckpoint): Promise { + if (checkpoint) { + const { replay } = await readCommittedPrefix(this.path, checkpoint); + this.resumeCheckpoint = { ...checkpoint }; + return replay; + } try { - const entries = await read(this.path); + const entries = (await readJournalBytes(this.path)) + .toString('utf8') + .split('\n') + .flatMap((line) => + line.trim().length === 0 + ? [] + : parseLineTolerant(line.trim(), this.path), + ); return buildReplay(entries); } catch (e) { debugLogger.warn(`WorkflowJournal.load failed for ${this.path}: ${e}`); @@ -183,6 +623,70 @@ export class WorkflowJournal { /** Append one entry. Rejects only on I/O error (callers `.catch`). */ append(entry: JournalEntry): Promise { - return writeLine(this.path, entry); + const write = this.tail.then(async () => { + await this.discardUncommittedSuffix(); + await appendJournalLine(this.path, entry); + }); + this.tail = write.catch((error: unknown) => { + this.writeError ??= errorMessage(error); + }); + return write; + } + + /** Flush all preceding appends and describe their durable file prefix. */ + flush(): Promise { + const checkpoint = this.tail.then(async (): Promise => { + let bytes = Buffer.alloc(0); + let flushError: string | undefined; + try { + await this.discardUncommittedSuffix(); + bytes = await readJournalBytes(this.path); + } catch (error) { + flushError = errorMessage(error); + } + const error = this.writeError ?? flushError; + return { + version: JOURNAL_FORMAT_VERSION, + keyVersion: JOURNAL_KEY_VERSION, + byteLength: bytes.byteLength, + sha256: createHash('sha256').update(bytes).digest('hex'), + integrity: error ? ('failed' as const) : ('complete' as const), + ...(error ? { error } : {}), + }; + }); + this.tail = checkpoint.then( + () => undefined, + (error: unknown) => { + this.writeError ??= errorMessage(error); + }, + ); + return checkpoint; + } + + private async discardUncommittedSuffix(): Promise { + const checkpoint = this.resumeCheckpoint; + if (!checkpoint) return; + const targetGuard = await guardJournalTarget(this.path); + const { bytes, committed } = await readCommittedPrefix( + this.path, + checkpoint, + ); + targetGuard.assertUnchanged(); + if (bytes.byteLength > checkpoint.byteLength) { + const guard = await guardJournalParents(this.path, false); + if (!guard) { + throw new Error('Workflow journal directory is unavailable.'); + } + await atomicWriteFile(this.path, committed, { + noFollow: true, + mode: 0o600, + forceMode: true, + assertCanCommit: () => { + guard.assertUnchanged(); + targetGuard.assertUnchanged(); + }, + }); + } + this.resumeCheckpoint = undefined; } } diff --git a/packages/core/src/agents/runtime/workflow-runner-persistence.test.ts b/packages/core/src/agents/runtime/workflow-runner-persistence.test.ts new file mode 100644 index 00000000000..61b23d74cf9 --- /dev/null +++ b/packages/core/src/agents/runtime/workflow-runner-persistence.test.ts @@ -0,0 +1,968 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { promises as fs } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import type { Config } from '../../config/config.js'; +import { Storage } from '../../config/storage.js'; +import { WorkflowRunRegistry } from '../workflow-run-registry.js'; +import * as workflowSnapshot from '../workflow-snapshot.js'; +import { readWorkflowManifest } from '../workflow-snapshot.js'; +import { WorkflowDispatchScheduler } from './workflow-dispatch-scheduler.js'; +import { WorkflowJournal } from './workflow-journal.js'; +import { WorkflowRunner } from './workflow-runner.js'; + +describe('WorkflowRunner persistence', () => { + const tempDirs: string[] = []; + + async function harness(): Promise<{ + config: Config; + registry: WorkflowRunRegistry; + storage: Storage; + }> { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'wf-runner-store-')); + tempDirs.push(root); + const registry = new WorkflowRunRegistry(); + const storage = new Storage(root, path.join(root, 'runtime')); + const config = { + storage, + getCliVersion: () => 'test-version', + getWorkflowRunRegistry: () => registry, + } as unknown as Config; + return { config, registry, storage }; + } + + afterEach(async () => { + vi.restoreAllMocks(); + await Promise.all( + tempDirs + .splice(0) + .map((dir) => fs.rm(dir, { recursive: true, force: true })), + ); + }); + + it('writes an initial manifest before returning and a compatible terminal snapshot', async () => { + const { config, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: { topic: 'durability' }, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + + const running = await readWorkflowManifest(config, handle.runId); + expect(running.status).toBe('running'); + expect(running.args).toEqual({ topic: 'durability' }); + + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + finishDispatch?.('done'); + await expect(handle.completion).resolves.toMatchObject({ ok: true }); + + const completed = await readWorkflowManifest(config, handle.runId); + expect(completed.status).toBe('completed'); + await expect( + fs.readFile(storage.getWorkflowRunSnapshotPath(handle.runId), 'utf8'), + ).resolves.toContain('"status": "completed"'); + }); + + it('keeps the run pausing until its flushed checkpoint is atomically published', async () => { + const { config, registry } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const originalFlush = WorkflowJournal.prototype.flush; + let releaseCheckpoint: (() => void) | undefined; + const checkpointGate = new Promise((resolve) => { + releaseCheckpoint = resolve; + }); + const flushSpy = vi + .spyOn(WorkflowJournal.prototype, 'flush') + .mockImplementationOnce(async function (this: WorkflowJournal) { + const checkpoint = await originalFlush.call(this); + await checkpointGate; + return checkpoint; + }); + + expect(handle.pause()).toBe(true); + finishDispatch?.('done'); + await vi.waitFor(() => expect(flushSpy).toHaveBeenCalledOnce()); + expect(registry.get(handle.runId)?.status).toBe('pausing'); + expect((await readWorkflowManifest(config, handle.runId)).status).toBe( + 'running', + ); + + releaseCheckpoint?.(); + await vi.waitFor(() => + expect(registry.get(handle.runId)?.status).toBe('paused'), + ); + expect((await readWorkflowManifest(config, handle.runId)).status).toBe( + 'paused', + ); + + expect(handle.resume()).toBe(true); + await expect(handle.completion).resolves.toMatchObject({ ok: true }); + }); + + // The pause barrier is entered from a dispatch's `.finally`, so nothing + // awaits it. A cancel landing while the barrier is mid-write used to + // publish the terminal manifest first and have the barrier's 'paused' + + // canResume write land on top — a cancelled run that resume() re-executes. + it('does not let a mid-barrier pause write outlive the terminal manifest', async () => { + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const originalWrite = workflowSnapshot.writeWorkflowManifest; + const statuses: Array = []; + let releasePaused: (() => void) | undefined; + const pausedGate = new Promise((resolve) => { + releasePaused = resolve; + }); + vi.spyOn(workflowSnapshot, 'writeWorkflowManifest').mockImplementation( + async (...args: Parameters) => { + const status = args[2].status; + statuses.push(status); + if (status === 'paused') await pausedGate; + return originalWrite(...args); + }, + ); + + expect(handle.pause()).toBe(true); + finishDispatch?.('done'); + await vi.waitFor(() => expect(statuses).toContain('paused')); + // The barrier has flushed and is inside its 'paused' + canResume write. + registry.cancel(handle.runId, Date.now()); + releasePaused?.(); + + await expect(handle.completion).resolves.toMatchObject({ ok: false }); + expect(registry.get(handle.runId)?.status).toBe('cancelled'); + // Read the file directly: a surviving 'paused' manifest also trips the + // terminal-snapshot conflict in readWorkflowManifest, which would mask + // which of the two writes actually landed last. + const durable = JSON.parse( + await fs.readFile( + storage.getWorkflowRunManifestPath(handle.runId), + 'utf8', + ), + ) as { status: string; canResume: boolean }; + expect(durable).toMatchObject({ status: 'cancelled', canResume: false }); + }); + + // Joining the barrier only covers the one that exists at join time. The + // terminal flush + manifest write that follows is a second window: a pause + // landing inside it used to be admitted (the entry is still 'running' until + // registry.complete), start a barrier nothing joined, and race its 'paused' + + // canResume write against the terminal write — which trips atomicWriteFile's + // inode guard and settles a normally-completed run as failed. + it('refuses a pause that lands while the terminal manifest is being written', async () => { + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const originalWrite = workflowSnapshot.writeWorkflowManifest; + const statuses: Array = []; + let releaseCompleted: (() => void) | undefined; + const completedGate = new Promise((resolve) => { + releaseCompleted = resolve; + }); + vi.spyOn(workflowSnapshot, 'writeWorkflowManifest').mockImplementation( + async (...args: Parameters) => { + const status = args[2].status; + statuses.push(status); + if (status === 'completed') await completedGate; + return originalWrite(...args); + }, + ); + + finishDispatch?.('done'); + await vi.waitFor(() => expect(statuses).toContain('completed')); + // Settlement is parked inside the terminal manifest write. The registry + // entry still reads 'running', so registry.pause's own guard admits this. + expect(registry.get(handle.runId)?.status).toBe('running'); + expect(registry.pause(handle.runId)).toBe(false); + releaseCompleted?.(); + + await expect(handle.completion).resolves.toMatchObject({ ok: true }); + expect(registry.get(handle.runId)?.status).toBe('completed'); + expect(statuses).not.toContain('paused'); + const durable = JSON.parse( + await fs.readFile( + storage.getWorkflowRunManifestPath(handle.runId), + 'utf8', + ), + ) as { status: string; canResume: boolean }; + expect(durable).toMatchObject({ status: 'completed', canResume: false }); + }); + + // The catch path's join is the only ordering guarantee between an + // error/cancel settlement and a barrier that is already mid-write. The + // finally block's re-persist heals the end state, so this pins the ordering + // itself: no 'paused' write may land after a terminal one, or a crash inside + // the window leaves a failed run advertised as resumable. + it('orders an error settlement after a barrier that is already mid-write', async () => { + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + // Two dispatches, because reaching the catch path takes some care: a + // rejecting dispatch is retried and the retry parks behind the pause + // gate, and a paused run holds even a *completed* dispatch's result + // gate, so the script cannot throw on its own either. The cancel below + // releases the first gate, and the second `agent()` then rejects + // immediately against the aborted signal — which is what unwinds the + // script into the catch path. + script: 'await agent("a"); await agent("b");', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const originalWrite = workflowSnapshot.writeWorkflowManifest; + const started: Array = []; + const committed: Array = []; + let releasePaused: (() => void) | undefined; + const pausedGate = new Promise((resolve) => { + releasePaused = resolve; + }); + vi.spyOn(workflowSnapshot, 'writeWorkflowManifest').mockImplementation( + async (...args: Parameters) => { + const status = args[2].status; + started.push(status); + if (status === 'paused') await pausedGate; + const result = await originalWrite(...args); + committed.push(status); + return result; + }, + ); + + expect(handle.pause()).toBe(true); + finishDispatch?.('done'); + // The barrier is entered from the dispatch's `.finally` and is now parked + // inside its 'paused' + canResume write. + await vi.waitFor(() => expect(started).toContain('paused')); + registry.cancel(handle.runId, Date.now()); + // Give the catch path room to reach its terminal write while the barrier + // is still mid-write. With the join it stays parked; without it, the + // 'cancelled' write lands here — inside the barrier's window. + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(committed).not.toContain('cancelled'); + releasePaused?.(); + + await expect(handle.completion).resolves.toMatchObject({ ok: false }); + expect(registry.get(handle.runId)?.status).toBe('cancelled'); + // The invariant: the barrier really ran, and every terminal write landed + // after it. + const lastPaused = committed.lastIndexOf('paused'); + const firstTerminal = committed.findIndex( + (status) => status === 'failed' || status === 'cancelled', + ); + expect(lastPaused).toBeGreaterThanOrEqual(0); + expect(firstTerminal).toBeGreaterThan(lastPaused); + const durable = JSON.parse( + await fs.readFile( + storage.getWorkflowRunManifestPath(handle.runId), + 'utf8', + ), + ) as { status: string; canResume: boolean }; + expect(durable).toMatchObject({ status: 'cancelled', canResume: false }); + }); + + it('fails the run instead of publishing paused when the manifest cannot be replaced', async () => { + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const manifestPath = storage.getWorkflowRunManifestPath(handle.runId); + const blockedManifest = storage.getWorkflowRunManifestPath(handle.runId); + await fs.rm(manifestPath); + await fs.mkdir(blockedManifest); + + expect(handle.pause()).toBe(true); + finishDispatch?.('done'); + await expect(handle.completion).resolves.toMatchObject({ + ok: false, + message: expect.stringMatching(/pause checkpoint/i), + }); + expect(registry.get(handle.runId)?.status).toBe('failed'); + expect(registry.get(handle.runId)?.error).toMatch(/pause checkpoint/i); + expect((await fs.lstat(blockedManifest)).isDirectory()).toBe(true); + }); + + it('degrades a pause to canResume:false instead of killing the run when the checkpoint fails', async () => { + // The contrast to the test above: there the manifest write itself failed, + // so there was no durable record of the pause and killing the run is right. + // Here the write is fine and only the checkpoint came back 'failed' — which + // is what both trigger classes look like, a journal grown past + // MAX_WORKFLOW_JOURNAL_BYTES and the sticky `writeError` latch left by one + // transient append failure, since flush() folds each into integrity + // 'failed' rather than throwing. The terminal paths already accept exactly + // this checkpoint, so asserting completeness only on the pause path turned + // the user's next pause into a hard kill of a healthy run — aborting its + // in-flight and queued agents — and contradicted the cap's own documented + // promise that resumability is lost while the journal stays intact. + const { config, registry } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + // Installed after start(), so the initial checkpoint — the one transition + // that legitimately still refuses — is untouched. + const originalFlush = WorkflowJournal.prototype.flush; + vi.spyOn(WorkflowJournal.prototype, 'flush').mockImplementation( + async function (this: WorkflowJournal) { + const checkpoint = await originalFlush.call(this); + return { + ...checkpoint, + integrity: 'failed' as const, + error: 'Workflow journal is too large', + }; + }, + ); + + expect(handle.pause()).toBe(true); + finishDispatch?.('done'); + + await vi.waitFor(() => + expect(registry.get(handle.runId)?.status).toBe('paused'), + ); + expect(registry.get(handle.runId)?.error).toBeUndefined(); + + const manifest = await readWorkflowManifest(config, handle.runId); + expect(manifest).toMatchObject({ status: 'paused', canResume: false }); + expect(manifest.resumeBlockedReason).toMatch(/too large/i); + expect(manifest.journal.integrity).toBe('failed'); + + // Drain the run so the suite leaves nothing pending. + vi.restoreAllMocks(); + expect(handle.resume()).toBe(true); + await expect(handle.completion).resolves.toMatchObject({ ok: true }); + }); + + it('does not publish success when the terminal manifest cannot be replaced', async () => { + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const manifestPath = storage.getWorkflowRunManifestPath(handle.runId); + const blockedManifest = storage.getWorkflowRunManifestPath(handle.runId); + await fs.rm(manifestPath); + await fs.mkdir(blockedManifest); + + finishDispatch?.('done'); + await expect(handle.completion).resolves.toMatchObject({ + ok: false, + message: expect.stringMatching(/terminal checkpoint/i), + }); + expect(registry.get(handle.runId)?.status).toBe('failed'); + expect(registry.get(handle.runId)?.error).toMatch(/terminal checkpoint/i); + expect((await fs.lstat(blockedManifest)).isDirectory()).toBe(true); + }); + + // The background controller is deliberately unlinked from the caller + // signal, so the pre-start check is the only read of it — and this PR put + // an fsync between that check and dispatch. An Esc landing in that window + // used to start the run anyway and report "started in background". + it('cancels a background start when the abort lands during the initial checkpoint', async () => { + const { config, registry } = await harness(); + const dispatch = vi.fn(async () => 'must not run'); + const caller = new AbortController(); + + const originalFlush = WorkflowJournal.prototype.flush; + vi.spyOn(WorkflowJournal.prototype, 'flush').mockImplementationOnce( + async function (this: WorkflowJournal) { + const checkpoint = await originalFlush.call(this); + caller.abort(); + return checkpoint; + }, + ); + + await expect( + WorkflowRunner.start({ + config, + signal: caller.signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch, + }), + ).rejects.toThrow(/Background workflow start was cancelled/); + + expect(dispatch).not.toHaveBeenCalled(); + // The initial persist already wrote 'running' + resumable; leaving it + // there would advertise a cancelled run as recoverable forever. + const active = registry.list().filter((run) => run.status === 'running'); + expect(active).toEqual([]); + }); + + it('drops a crash suffix before a resumed run publishes a new checkpoint', async () => { + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const script = 'return await agent("work")'; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script, + args: { topic: 'resume' }, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + expect(handle.pause()).toBe(true); + finishDispatch?.('cached'); + await vi.waitFor(() => + expect(registry.get(handle.runId)?.status).toBe('paused'), + ); + + await fs.appendFile( + storage.getWorkflowRunJournalPath(handle.runId), + '{partial', + ); + const resumedRegistry = new WorkflowRunRegistry(); + const resumedConfig = { + storage, + getCliVersion: () => 'test-version', + getWorkflowRunRegistry: () => resumedRegistry, + } as unknown as Config; + const resumedDispatch = vi.fn(async () => 'must not run'); + const resumed = await WorkflowRunner.start({ + config: resumedConfig, + signal: new AbortController().signal, + script, + args: { topic: 'resume' }, + resumeFromRunId: handle.runId, + dispatch: resumedDispatch, + }); + + await expect(resumed.completion).resolves.toMatchObject({ ok: true }); + expect(resumedDispatch).not.toHaveBeenCalled(); + const manifest = await readWorkflowManifest(resumedConfig, handle.runId); + await expect( + new WorkflowJournal(storage.getWorkflowRunJournalPath(handle.runId)).load( + manifest.journal, + ), + ).resolves.toMatchObject({ + results: expect.any(Map), + started: expect.any(Map), + }); + + registry.cancel(handle.runId, Date.now()); + await expect(handle.completion).resolves.toMatchObject({ ok: false }); + + // The stale original runner's teardown must not relabel the resumed + // run's terminal state: its settlement persists lose the generation CAS + // (the resumed run owns the run now), and an evicted writer publishes + // no legacy snapshot. Re-read both artifacts — the corruption this test + // used to ship green was exactly these two files saying 'cancelled'. + const durable = JSON.parse( + await fs.readFile( + storage.getWorkflowRunManifestPath(handle.runId), + 'utf8', + ), + ) as { status: string }; + expect(durable.status).toBe('completed'); + const snapshot = JSON.parse( + await fs.readFile( + storage.getWorkflowRunSnapshotPath(handle.runId), + 'utf8', + ), + ) as { status: string }; + expect(snapshot.status).toBe('completed'); + }); + + it('refuses script or args drift instead of silently starting a fresh resume', async () => { + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const script = 'return await agent("work")'; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script, + args: { topic: 'original' }, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + expect(handle.pause()).toBe(true); + finishDispatch?.('done'); + await vi.waitFor(() => + expect(registry.get(handle.runId)?.status).toBe('paused'), + ); + + const freshRegistry = new WorkflowRunRegistry(); + const freshConfig = { + storage, + getCliVersion: () => 'test-version', + getWorkflowRunRegistry: () => freshRegistry, + } as unknown as Config; + await expect( + WorkflowRunner.start({ + config: freshConfig, + signal: new AbortController().signal, + script: `${script}\n// changed`, + args: { topic: 'original' }, + resumeFromRunId: handle.runId, + dispatch: async () => 'unused', + }), + ).rejects.toThrow(/script.*manifest/i); + await expect( + WorkflowRunner.start({ + config: freshConfig, + signal: new AbortController().signal, + script, + args: { topic: 'changed' }, + resumeFromRunId: handle.runId, + dispatch: async () => 'unused', + }), + ).rejects.toThrow(/args.*manifest/i); + + registry.cancel(handle.runId, Date.now()); + await expect(handle.completion).resolves.toMatchObject({ ok: false }); + }); + + it('persists cancellation after draining the journal', async () => { + const { config, registry } = await harness(); + let rejectDispatch: ((error: Error) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((_resolve, reject) => { + rejectDispatch = reject; + }), + }); + await vi.waitFor(() => expect(rejectDispatch).toBeDefined()); + + registry.cancel(handle.runId, Date.now()); + rejectDispatch?.(new Error('cancelled')); + await expect(handle.completion).resolves.toMatchObject({ ok: false }); + + const manifest = await readWorkflowManifest(config, handle.runId); + expect(manifest.status).toBe('cancelled'); + expect(manifest.journal.integrity).toBe('complete'); + expect(manifest.journal.byteLength).toBeGreaterThan(0); + const replay = await new WorkflowJournal( + config.storage!.getWorkflowRunJournalPath(handle.runId), + ).load(manifest.journal); + expect(replay.started.size).toBe(1); + }); + + it('does not report success when cancellation wins during terminal flush', async () => { + const { config, registry } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const originalFlush = WorkflowJournal.prototype.flush; + let releaseFlush: (() => void) | undefined; + const flushGate = new Promise((resolve) => { + releaseFlush = resolve; + }); + const flushSpy = vi + .spyOn(WorkflowJournal.prototype, 'flush') + .mockImplementationOnce(async function (this: WorkflowJournal) { + const checkpoint = await originalFlush.call(this); + await flushGate; + return checkpoint; + }); + + finishDispatch?.('done'); + await vi.waitFor(() => expect(flushSpy).toHaveBeenCalledOnce()); + registry.cancel(handle.runId, Date.now()); + releaseFlush?.(); + + await expect(handle.completion).resolves.toMatchObject({ + ok: false, + message: 'Workflow run cancelled.', + }); + expect((await readWorkflowManifest(config, handle.runId)).status).toBe( + 'cancelled', + ); + }); + + it('does not report success when cancellation wins during terminal manifest publication', async () => { + const { config, registry } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const originalWrite = workflowSnapshot.writeWorkflowManifest; + let releaseManifest: (() => void) | undefined; + const manifestGate = new Promise((resolve) => { + releaseManifest = resolve; + }); + const manifestSpy = vi + .spyOn(workflowSnapshot, 'writeWorkflowManifest') + .mockImplementationOnce(async (...args) => { + await manifestGate; + return originalWrite(...args); + }); + + finishDispatch?.('done'); + await vi.waitFor(() => expect(manifestSpy).toHaveBeenCalledOnce()); + registry.cancel(handle.runId, Date.now()); + releaseManifest?.(); + + await expect(handle.completion).resolves.toMatchObject({ + ok: false, + message: 'Workflow run cancelled.', + }); + expect((await readWorkflowManifest(config, handle.runId)).status).toBe( + 'cancelled', + ); + }); + + it('settles failed even when every terminal journal flush rejects', async () => { + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + vi.spyOn(WorkflowJournal.prototype, 'flush').mockRejectedValue( + new Error('terminal flush broke'), + ); + + finishDispatch?.('done'); + + await expect(handle.completion).resolves.toMatchObject({ + ok: false, + message: 'terminal flush broke', + }); + expect(registry.get(handle.runId)).toMatchObject({ + status: 'failed', + error: 'terminal flush broke', + }); + await expect( + fs.readFile(storage.getWorkflowRunSnapshotPath(handle.runId), 'utf8'), + ).resolves.toContain('"status": "failed"'); + }); + + it('persists wall-clock timeout as a failed terminal checkpoint', async () => { + vi.useFakeTimers(); + const originalTimeout = process.env['QWEN_CODE_MAX_WORKFLOW_SECONDS']; + process.env['QWEN_CODE_MAX_WORKFLOW_SECONDS'] = '1'; + try { + const { config } = await harness(); + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'await new Promise(() => {})', + args: null, + runInBackground: true, + dispatch: async () => 'unused', + }); + + await vi.advanceTimersByTimeAsync(1_000); + await expect(handle.completion).resolves.toMatchObject({ ok: false }); + const manifest = await readWorkflowManifest(config, handle.runId); + expect(manifest.status).toBe('failed'); + expect(manifest.journal.integrity).toBe('complete'); + } finally { + vi.useRealTimers(); + if (originalTimeout === undefined) { + delete process.env['QWEN_CODE_MAX_WORKFLOW_SECONDS']; + } else { + process.env['QWEN_CODE_MAX_WORKFLOW_SECONDS'] = originalTimeout; + } + } + }); + + // The barrier's catch used to kill the run unconditionally. A settlement + // that has already joined the barrier records the terminal state durably + // on its own; killing anyway flipped a script-completed run to 'failed' + // and dropped its result whenever the barrier's 'paused' write hit a + // transient fs error the very next write survived. + it('does not kill a successful run when the barrier write fails after settlement joined', async () => { + // Foreground run: the caller abort below releases the paused result + // gate WITHOUT touching the registry entry, the script completes on the + // delivered result, and settlement joins the barrier while its 'paused' + // write is still gated in flight. Only then does the write fail — the + // old unconditional kill flipped the successful settlement to 'failed' + // and dropped the result; with settlement already joined, the barrier + // must stand down and let the settlement record the terminal state. + const { config, registry, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const caller = new AbortController(); + const handle = await WorkflowRunner.start({ + config, + signal: caller.signal, + script: 'return await agent("work")', + args: null, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + + const originalWrite = workflowSnapshot.writeWorkflowManifest; + const started: Array = []; + let releasePaused: (() => void) | undefined; + const pausedGate = new Promise((resolve) => { + releasePaused = resolve; + }); + vi.spyOn(workflowSnapshot, 'writeWorkflowManifest').mockImplementation( + async (...args: Parameters) => { + const status = args[2].status; + started.push(status); + if (status === 'paused') { + await pausedGate; + throw new Error('ENOSPC: simulated barrier write failure'); + } + return originalWrite(...args); + }, + ); + const beginSettlingSpy = vi.spyOn( + WorkflowDispatchScheduler.prototype, + 'beginSettling', + ); + + expect(handle.pause()).toBe(true); + finishDispatch?.('done'); + await vi.waitFor(() => expect(started).toContain('paused')); + // The barrier is parked inside its 'paused' write. The abort releases + // the held dispatch result, the script completes, and settlement joins + // the in-flight barrier. + caller.abort(); + await vi.waitFor(() => expect(beginSettlingSpy).toHaveBeenCalled()); + releasePaused?.(); + + await expect(handle.completion).resolves.toMatchObject({ ok: true }); + expect(registry.get(handle.runId)?.status).toBe('completed'); + const durable = JSON.parse( + await fs.readFile( + storage.getWorkflowRunManifestPath(handle.runId), + 'utf8', + ), + ) as { status: string; canResume: boolean }; + expect(durable).toMatchObject({ status: 'completed', canResume: false }); + }); + + // An in-process duplicate resume must be refused before the durable + // manifest is touched: acquiring ownership first committed the generation + // CAS under the refused caller's nonce, evicting the live run's next + // persist on behalf of a writer that never started. + it('refuses an in-process duplicate resume without mutating the durable manifest', async () => { + const { config, storage } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const script = 'return await agent("work")'; + const args = { topic: 'duplicate' }; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script, + args, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + await vi.waitFor(() => expect(finishDispatch).toBeDefined()); + const manifestPath = storage.getWorkflowRunManifestPath(handle.runId); + const before = await fs.readFile(manifestPath, 'utf8'); + + await expect( + WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script, + args, + resumeFromRunId: handle.runId, + dispatch: vi.fn(async () => 'must not run'), + }), + ).rejects.toThrow(/already active/); + + expect(await fs.readFile(manifestPath, 'utf8')).toBe(before); + + // The live run still owns its manifests: its next persist advances. + finishDispatch?.('done'); + await expect(handle.completion).resolves.toMatchObject({ ok: true }); + const after = JSON.parse(await fs.readFile(manifestPath, 'utf8')) as { + status: string; + }; + expect(after.status).toBe('completed'); + }); + + it('stamps generation and owner on a fresh run from its first manifest', async () => { + const { config } = await harness(); + let finishDispatch: ((value: string) => void) | undefined; + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + script: 'return await agent("work")', + args: null, + runInBackground: true, + dispatch: () => + new Promise((resolve) => { + finishDispatch = resolve; + }), + }); + + // The first writer of a run is the prior writer every later resume + // races against, so even a fresh start's initial manifest carries the + // CAS stamp. + const manifest = await readWorkflowManifest(config, handle.runId); + expect(manifest.generation).toBe(1); + expect(manifest.owner).toMatchObject({ pid: process.pid }); + + finishDispatch?.('done'); + await expect(handle.completion).resolves.toMatchObject({ ok: true }); + const completed = await readWorkflowManifest(config, handle.runId); + // Gen 2 is the try-path 'completed' write; gen 3 the finally re-persist. + expect(completed.generation).toBe(3); + expect(completed.owner?.nonce).toBe(manifest.owner?.nonce); + }); + + it('projects the settlement result once and writes the same value everywhere', async () => { + const { config, storage } = await harness(); + const handle = await WorkflowRunner.start({ + config, + signal: new AbortController().signal, + // Built INSIDE the sandbox: agent() returns are revived at the vm + // boundary, but the script's own return is not detached, so a live + // `toJSON()` rides into settlement. The settlement must project it + // ONCE — if either persisted writer re-serialized the live object, + // `calls` would advance past 1 and the artifacts would disagree. + script: + 'let calls = 0; return { toJSON: () => ({ projected: true, calls: ++calls }) };', + args: null, + runInBackground: true, + dispatch: async () => 'unused', + }); + await expect(handle.completion).resolves.toMatchObject({ ok: true }); + + const manifest = JSON.parse( + await fs.readFile( + storage.getWorkflowRunManifestPath(handle.runId), + 'utf8', + ), + ) as { result: unknown }; + expect(manifest.result).toEqual({ projected: true, calls: 1 }); + const snapshot = JSON.parse( + await fs.readFile( + storage.getWorkflowRunSnapshotPath(handle.runId), + 'utf8', + ), + ) as { result: unknown }; + expect(snapshot.result).toEqual({ projected: true, calls: 1 }); + }); +}); diff --git a/packages/core/src/agents/runtime/workflow-runner.test.ts b/packages/core/src/agents/runtime/workflow-runner.test.ts index a306f8ca33b..6c13c7360ac 100644 --- a/packages/core/src/agents/runtime/workflow-runner.test.ts +++ b/packages/core/src/agents/runtime/workflow-runner.test.ts @@ -17,10 +17,14 @@ import { WorkflowRunner } from './workflow-runner.js'; const { createProductionDispatchMock, logWorkflowRunMock, + readWorkflowManifestMock, + writeWorkflowManifestMock, writeWorkflowSnapshotMock, } = vi.hoisted(() => ({ createProductionDispatchMock: vi.fn(), logWorkflowRunMock: vi.fn(), + readWorkflowManifestMock: vi.fn(), + writeWorkflowManifestMock: vi.fn().mockResolvedValue(undefined), writeWorkflowSnapshotMock: vi.fn().mockResolvedValue(undefined), })); @@ -29,7 +33,18 @@ vi.mock('../../telemetry/loggers.js', () => ({ })); vi.mock('../workflow-snapshot.js', () => ({ + readWorkflowManifest: readWorkflowManifestMock, + writeWorkflowManifest: writeWorkflowManifestMock, writeWorkflowSnapshot: writeWorkflowSnapshotMock, + createWorkflowRunOwner: () => ({ + machineId: 'test-host', + pid: 1, + nonce: 'test-nonce', + acquiredAt: 0, + }), + acquireWorkflowRunOwnership: async () => 1, + projectWorkflowResult: (result: unknown) => result, + WorkflowRunOwnershipError: class WorkflowRunOwnershipError extends Error {}, })); vi.mock('./workflow-orchestrator.js', async (importOriginal) => { @@ -79,6 +94,8 @@ describe('WorkflowRunner', () => { beforeEach(() => { createProductionDispatchMock.mockReset(); logWorkflowRunMock.mockClear(); + readWorkflowManifestMock.mockReset(); + writeWorkflowManifestMock.mockClear(); writeWorkflowSnapshotMock.mockClear(); }); @@ -545,20 +562,19 @@ describe('WorkflowRunner', () => { it('rejects a concurrent resume while the original run is active', async () => { const { config, registry } = configWithRegistry(); - const runId = 'wf_1234abcd'; let resolveDispatch: ((value: string) => void) | undefined; const original = await WorkflowRunner.start({ config, signal: new AbortController().signal, script: 'return await agent("original")', args: undefined, - resumeFromRunId: runId, runInBackground: true, dispatch: () => new Promise((resolve) => { resolveDispatch = resolve; }), }); + const runId = original.runId; await vi.waitFor(() => expect(resolveDispatch).toBeDefined()); const replacementCaller = new AbortController(); const replacementDispatch = vi.fn(async () => 'replacement'); diff --git a/packages/core/src/agents/runtime/workflow-runner.ts b/packages/core/src/agents/runtime/workflow-runner.ts index cc51e80fb4c..d9d4b68e992 100644 --- a/packages/core/src/agents/runtime/workflow-runner.ts +++ b/packages/core/src/agents/runtime/workflow-runner.ts @@ -8,16 +8,27 @@ import { randomBytes } from 'node:crypto'; import type { Config } from '../../config/config.js'; import { logWorkflowRun } from '../../telemetry/loggers.js'; import { WorkflowRunEvent } from '../../telemetry/types.js'; +import { createDebugLogger } from '../../utils/debugLogger.js'; import { createAbortController, createChildAbortController, } from '../../utils/abortController.js'; import { + isActiveWorkflowStatus, isTerminalWorkflowStatus, type WorkflowRunRegistry, type WorkflowTask, } from '../workflow-run-registry.js'; -import { writeWorkflowSnapshot } from '../workflow-snapshot.js'; +import { + readWorkflowManifest, + writeWorkflowManifest, + acquireWorkflowRunOwnership, + createWorkflowRunOwner, + projectWorkflowResult, + WorkflowRunOwnershipError, + type WorkflowManifestOwnership, + writeWorkflowSnapshot, +} from '../workflow-snapshot.js'; import { createProductionDispatch, resolveConcurrencyLimit, @@ -29,7 +40,11 @@ import { } from './workflow-orchestrator.js'; import { WorkflowBudgetImpl } from './workflow-budget.js'; import { WorkflowDispatchScheduler } from './workflow-dispatch-scheduler.js'; -import { WorkflowJournal, type JournalReplay } from './workflow-journal.js'; +import { + WorkflowJournal, + type JournalCheckpoint, + type JournalReplay, +} from './workflow-journal.js'; import { resolveSavedWorkflowScript } from './workflow-saved.js'; export interface WorkflowRunnerOptions { @@ -44,6 +59,8 @@ export interface WorkflowRunnerOptions { runInBackground?: boolean; } +const debugLogger = createDebugLogger('WORKFLOW_RUNNER'); + export type WorkflowRunSettlement = | { ok: true; outcome: WorkflowRunOutcome } | { ok: false; message: string; details?: WorkflowExecutionError }; @@ -93,13 +110,70 @@ export class WorkflowRunner { const scriptPath = loaded?.scriptPath ?? options.scriptPath; const runId = options.resumeFromRunId ?? `wf_${randomBytes(8).toString('hex')}`; + const registry = config.getWorkflowRunRegistry?.(); const storage = config.storage; const journal = storage ? new WorkflowJournal(storage.getWorkflowRunJournalPath(runId)) : undefined; - const resumeReplay: JournalReplay | undefined = options.resumeFromRunId - ? await journal?.load() - : undefined; + let resumeReplay: JournalReplay | undefined; + // Every run with durable storage carries an ownership claim. A fresh + // start used to skip it — "no prior writer to race" — but the first + // writer of a run is exactly the prior writer every later resume races + // against, and an unguarded stale fresh-start settlement could relabel + // the resumed run's terminal state. + let ownership: WorkflowManifestOwnership | undefined; + if (options.resumeFromRunId && journal) { + const manifest = await readWorkflowManifest(config, runId); + if (!manifest.canResume) { + throw new Error( + manifest.resumeBlockedReason ?? + `Workflow run ${runId} is not recoverable.`, + ); + } + if (!isActiveWorkflowStatus(manifest.status)) { + throw new Error( + `Workflow run ${runId} is ${manifest.status}; terminal runs must be rerun instead of resumed.`, + ); + } + if (manifest.script !== script) { + throw new Error( + `Workflow run ${runId} script does not match its durable manifest.`, + ); + } + if (serializedArgs(options.args) !== JSON.stringify(manifest.args)) { + throw new Error( + `Workflow run ${runId} args do not match its durable manifest.`, + ); + } + // Reject an in-process duplicate BEFORE the claim: acquiring first + // would commit the generation CAS under this caller's nonce and only + // then have register() refuse the duplicate — evicting the live run's + // next persist on behalf of a writer that never started. + if (registry?.hasActiveRegistration(runId)) { + throw new Error(`Workflow run ${runId} is already active.`); + } + // Claim the run before anything is replayed or dispatched. Two + // processes resuming the same run both read this generation and both + // try to commit the next one; exactly one rename lands, and the loser + // throws here — before a single agent has been dispatched. That + // ordering is the point: it keeps the window in which both processes + // are "resuming" free of model calls and tool side effects. + const owner = createWorkflowRunOwner(); + ownership = { + owner, + expectedGeneration: await acquireWorkflowRunOwnership( + config, + runId, + owner, + ), + }; + resumeReplay = await journal.load(manifest.journal); + } else if (options.resumeFromRunId && !registry?.get(runId)) { + throw new Error('Workflow storage is required to resume a run.'); + } + if (journal && !ownership) { + ownership = { owner: createWorkflowRunOwner(), expectedGeneration: 0 }; + } if (runInBackground && options.signal.aborted) { throw new Error('Background workflow start was cancelled.'); } @@ -107,7 +181,6 @@ export class WorkflowRunner { const controller = runInBackground ? createAbortController() : createChildAbortController(options.signal); - const registry = config.getWorkflowRunRegistry?.(); const dispatch = options.dispatch ?? createProductionDispatch( @@ -145,6 +218,81 @@ export class WorkflowRunner { // UI refresh failures must not affect workflow execution. } }; + + let ownershipLost = false; + const persistManifest = async ( + status: WorkflowTask['status'], + checkpoint: JournalCheckpoint, + source: WorkflowTask | undefined = entry, + ): Promise => { + if (!source) return; + try { + const written = await writeWorkflowManifest(config, source, { + args: options.args, + journal: checkpoint, + status, + ...(ownership ? { ownership } : {}), + }); + // Each accepted write advances the generation this process owns, so + // the next one compare-and-swaps against what it just committed. + if (ownership && written?.generation !== undefined) { + ownership = { ...ownership, expectedGeneration: written.generation }; + } + } catch (error) { + // Another writer advanced the run past this one; everything this + // process still persists for it is stale by construction, so the + // settlement stops publishing durable state for the run. + if (error instanceof WorkflowRunOwnershipError) ownershipLost = true; + throw error; + } + }; + + const persistManifestBestEffort = async ( + status: WorkflowTask['status'], + checkpoint: JournalCheckpoint, + source: WorkflowTask | undefined = entry, + ): Promise => { + try { + await persistManifest(status, checkpoint, source); + } catch (error) { + debugLogger.warn( + `Failed to persist workflow manifest for ${runId}: ${extractErrorMessage(error)}`, + ); + } + }; + + if (entry && journal) { + let initialCheckpoint: JournalCheckpoint; + try { + initialCheckpoint = await journal.flush(); + assertCompleteCheckpoint(initialCheckpoint, 'initial'); + await persistManifest('running', initialCheckpoint); + } catch (error) { + const message = `Failed to persist initial workflow checkpoint: ${extractErrorMessage(error)}`; + registry?.fail(runId, message, Date.now()); + controller.abort(); + throw new Error(message); + } + // A background run's controller is deliberately not linked to the + // caller's signal, so nothing re-reads that signal after the + // pre-start check. The persist above is the only await between the + // two, and an fsync is long enough to swallow an Esc: without this + // the run registers, dispatches agents, and reports "started in + // background" for something the user just cancelled. The manifest + // that persist just wrote says 'running' + resumable, so it has to + // be settled here too or the run reads as recoverable forever. + if (runInBackground && options.signal.aborted) { + const endTime = Date.now(); + registry?.cancel(runId, endTime); + controller.abort(); + await persistManifestBestEffort( + 'cancelled', + initialCheckpoint, + freezeWorkflowTask({ ...entry, status: 'cancelled', endTime }), + ); + throw new Error('Background workflow start was cancelled.'); + } + } const emitter: WorkflowOrchestratorEmitter = { phaseStarted: (title) => { registry?.onPhaseStarted(runId, title); @@ -168,12 +316,71 @@ export class WorkflowRunner { }, }; + let fatalPersistenceError: string | undefined; + let settlingJoined = false; const scheduler = new WorkflowDispatchScheduler( resolveConcurrencyLimit(), controller.signal, ({ state }) => registry?.onDispatchStateChange(runId, state), + journal + ? async () => { + try { + // A failed checkpoint is deliberately NOT asserted here, unlike + // the initial transition. The terminal paths already tolerate + // the identical failed checkpoint — they flush and persist + // whatever comes back — so asserting only here meant a journal + // that had outgrown `MAX_WORKFLOW_JOURNAL_BYTES`, or had ever + // seen one transient append failure (the sticky `writeError` + // latch), turned the user's next pause into a hard kill of a + // healthy run: in-flight and queued agents aborted, the run + // lost to "Failed to persist workflow pause checkpoint". That + // also contradicted the cap's own documented contract, which + // promises resumability is lost while the journal stays intact + // on disk. + // + // `writeWorkflowManifest` turns a non-'complete' checkpoint + // into `canResume: false` plus the journal's own reason, so the + // pause still lands durably and advertises itself honestly. + // The kill below is reserved for the manifest write itself + // failing, which is the case that leaves no durable record of + // the pause at all — and for `flush()` throwing outright, + // where there is no checkpoint to degrade with. + const checkpoint = await journal.flush(); + await persistManifest('paused', checkpoint); + } catch (error) { + // A settlement that already joined this barrier records the + // run's terminal state durably on its own; killing the run + // here would flip a completed settlement to 'failed' and drop + // its result. The kill stays for a run left 'pausing' with no + // settlement in sight — the case where the failed write + // leaves no durable record of the pause at all. + if (!settlingJoined) { + fatalPersistenceError = `Failed to persist workflow pause checkpoint: ${extractErrorMessage(error)}`; + registry?.fail(runId, fatalPersistenceError, Date.now()); + controller.abort(); + } + throw error; + } + } + : undefined, ); + // The pause barrier above runs detached (it is entered from a dispatch's + // `.finally`), and it durably writes 'paused' + canResume. A run that + // completes or is cancelled while that write is in flight would publish + // its terminal manifest first and have the barrier overwrite it — a + // finished run advertised as resumable forever. Latching the scheduler + // out of pausing and then joining the barrier before each terminal write + // orders the two: joining alone would only cover the barrier that exists + // at call time, leaving the fsync + manifest write that follows open to a + // pause that starts a fresh one. + const joinPauseBarrier = (): Promise => { + settlingJoined = true; + return scheduler.beginSettling(); + }; + + let terminalCheckpoint: JournalCheckpoint | undefined; + const handle: WorkflowRunHandle = new WorkflowRunHandle( runId, budget, @@ -217,22 +424,94 @@ export class WorkflowRunner { : (entry.error ?? 'Workflow run failed.'), }; } - registry?.complete(runId, outcome.result, Date.now()); + await joinPauseBarrier(); + terminalCheckpoint = await journal?.flush(); + if (entry && isTerminalWorkflowStatus(entry.status)) { + return { + ok: false, + message: + entry.status === 'cancelled' + ? 'Workflow run cancelled.' + : (entry.error ?? 'Workflow run failed.'), + }; + } + const endTime = Date.now(); + // Project the result ONCE and hand the frozen value to every + // persisted consumer — the manifest write and, via the registry + // entry, the legacy snapshot. Re-projecting the live object at + // each writer would run getters/`toJSON()` once per write, free + // to throw the second time or to persist output the run never + // produced. + const projectedResult = projectWorkflowResult(outcome.result); + if (entry && terminalCheckpoint) { + const completedEntry = freezeWorkflowTask({ + ...entry, + status: 'completed', + endTime, + result: projectedResult, + }); + try { + await persistManifest( + 'completed', + terminalCheckpoint, + completedEntry, + ); + } catch (error) { + fatalPersistenceError = `Failed to persist workflow terminal checkpoint: ${extractErrorMessage(error)}`; + throw new Error(fatalPersistenceError); + } + } + if (entry && isTerminalWorkflowStatus(entry.status)) { + return { + ok: false, + message: + entry.status === 'cancelled' + ? 'Workflow run cancelled.' + : (entry.error ?? 'Workflow run failed.'), + }; + } + registry?.complete(runId, projectedResult, endTime); + // The caller still receives the live result: the tool's display + // path has its own per-field degradation for non-serializable + // values, and the persisted writers above already got the + // projection. return { ok: true, outcome }; } catch (error) { const details = error instanceof WorkflowExecutionError ? error : undefined; - const message = extractErrorMessage(error); + const message = fatalPersistenceError ?? extractErrorMessage(error); if (entry && details?.meta && !entry.meta) entry.meta = details.meta; if (details?.logs) registry?.setRecentLogs(runId, details.logs); - if ( + try { + await joinPauseBarrier(); + terminalCheckpoint = await journal?.flush(); + } catch (flushError) { + debugLogger.warn( + `Failed to flush workflow terminal journal for ${runId}: ${extractErrorMessage(flushError)}`, + ); + } + const cancelled = callerWasAbortedBeforeStart || (!runInBackground && options.signal.aborted) || - entry?.status === 'cancelled' - ) { - registry?.cancel(runId, Date.now()); + entry?.status === 'cancelled'; + const endTime = Date.now(); + if (entry && terminalCheckpoint) { + const failedEntry = freezeWorkflowTask({ + ...entry, + status: cancelled ? 'cancelled' : 'failed', + endTime, + ...(cancelled ? {} : { error: message }), + }); + await persistManifestBestEffort( + failedEntry.status, + terminalCheckpoint, + failedEntry, + ); + } + if (cancelled) { + registry?.cancel(runId, endTime); } else { - registry?.fail(runId, message, Date.now()); + registry?.fail(runId, message, endTime); } return { ok: false, message, details }; } finally { @@ -244,15 +523,47 @@ export class WorkflowRunner { // dispatches keep draining (mutating the live entry) across // the snapshot write's awaits, and a post-await read made // the snapshot and telemetry disagree with each other. + const settledEntry = freezeWorkflowTask(entry); const telemetryEvent = new WorkflowRunEvent({ - status: entry.status, - agents_dispatched: entry.agentsDispatched, - agents_completed: entry.agentsCompleted, - phase_count: entry.phases.length, - tokens_spent: entry.tokensSpent, - duration_ms: (entry.endTime ?? entry.startTime) - entry.startTime, + status: settledEntry.status, + agents_dispatched: settledEntry.agentsDispatched, + agents_completed: settledEntry.agentsCompleted, + phase_count: settledEntry.phases.length, + tokens_spent: settledEntry.tokensSpent, + duration_ms: + (settledEntry.endTime ?? settledEntry.startTime) - + settledEntry.startTime, }); - await writeWorkflowSnapshot(config, entry); + // Reached without a terminal flush on the early-return paths + // (the entry settled mid-script), so these writes need the same + // ordering guarantee the try/catch paths already took. + await joinPauseBarrier(); + let checkpoint = terminalCheckpoint; + if (!checkpoint && journal) { + try { + checkpoint = await journal.flush(); + } catch (flushError) { + debugLogger.warn( + `Failed to flush terminal workflow journal for ${runId}: ${extractErrorMessage(flushError)}`, + ); + } + } + if (checkpoint) { + await persistManifestBestEffort( + settledEntry.status, + checkpoint, + settledEntry, + ); + } + // The snapshot goes last, after the barrier join and the + // terminal manifest — starting it ahead used to let it land + // before the terminal write it disagrees with. A writer that + // lost the ownership CAS publishes nothing: its view of the run + // is stale by construction, and writing anyway would relabel a + // run another writer owns. + if (!ownershipLost) { + await writeWorkflowSnapshot(config, settledEntry); + } try { logWorkflowRun(config, telemetryEvent); } catch { @@ -281,3 +592,35 @@ function extractErrorMessage(error: unknown): string { } return String(error); } + +function serializedArgs(args: unknown): string | undefined { + try { + return JSON.stringify(args ?? null); + } catch { + return undefined; + } +} + +// Only the initial transition asserts. The pause barrier and both terminal +// paths degrade on a failed checkpoint instead (canResume: false plus the +// journal's reason); registration is the one point with no manifest to +// degrade into, so it is the one point that still refuses to start. +function assertCompleteCheckpoint( + checkpoint: JournalCheckpoint, + transition: 'initial', +): void { + if (checkpoint.integrity === 'complete') return; + throw new Error( + `${transition} workflow journal checkpoint failed${checkpoint.error ? `: ${checkpoint.error}` : ''}`, + ); +} + +function freezeWorkflowTask(task: WorkflowTask): WorkflowTask { + return { + ...task, + phases: [...task.phases], + recentLogs: [...task.recentLogs], + perPhaseTokens: new Map(task.perPhaseTokens), + pendingApprovals: [], + }; +} diff --git a/packages/core/src/agents/workflow-run-registry.ts b/packages/core/src/agents/workflow-run-registry.ts index fff995c9531..17cdc7a1293 100644 --- a/packages/core/src/agents/workflow-run-registry.ts +++ b/packages/core/src/agents/workflow-run-registry.ts @@ -385,6 +385,21 @@ export class WorkflowRunRegistry { } } + /** + * True when `register()` would refuse this runId as a duplicate. Lets a + * resume preflight reject an in-process duplicate BEFORE it touches any + * durable state — acquiring the run's ownership first would commit the + * generation CAS under the refused caller's nonce and evict the live + * run's next persist. + */ + hasActiveRegistration(runId: string): boolean { + const existing = this.entries.get(runId); + return ( + (existing !== undefined && isActiveWorkflowStatus(existing.status)) || + this.handles.has(runId) + ); + } + /** * Register a new run. Mutates the registration in place to graduate * it to a `WorkflowTask` (sets `id`, `kind`, derived counters), so @@ -392,11 +407,7 @@ export class WorkflowRunRegistry { * observers see updates without an extra `get()`. */ register(registration: WorkflowTaskRegistration): WorkflowTask { - const existing = this.entries.get(registration.runId); - if ( - (existing && isActiveWorkflowStatus(existing.status)) || - this.handles.has(registration.runId) - ) { + if (this.hasActiveRegistration(registration.runId)) { throw new Error(`Workflow run ${registration.runId} is already active.`); } const entry = registration as WorkflowTask; diff --git a/packages/core/src/agents/workflow-snapshot.test.ts b/packages/core/src/agents/workflow-snapshot.test.ts index 0f1228f49d5..4ddc3d6d854 100644 --- a/packages/core/src/agents/workflow-snapshot.test.ts +++ b/packages/core/src/agents/workflow-snapshot.test.ts @@ -5,18 +5,33 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { promises as fs } from 'node:fs'; +import { constants as fsConstants, promises as fs, type Dirent } from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import type { Config } from '../config/config.js'; import { Storage } from '../config/storage.js'; import { + MAX_WORKFLOW_ARGS_BYTES, + MAX_WORKFLOW_ARTIFACT_BYTES, + WORKFLOW_MANIFEST_SCHEMA_VERSION, + WORKFLOW_RUNTIME_VERSION, + listWorkflowRunRecords, + readWorkflowManifest, + projectWorkflowResult, toSnapshot, + writeWorkflowManifest, + acquireWorkflowRunOwnership, + WorkflowRunOwnershipError, writeWorkflowSnapshot, listWorkflowSnapshots, MAX_RETAINED_SNAPSHOTS, + type WorkflowManifestV2, } from './workflow-snapshot.js'; import type { WorkflowTask } from './workflow-run-registry.js'; +import { + WorkflowJournal, + type JournalCheckpoint, +} from './runtime/workflow-journal.js'; function fakeConfig(projectDir: string): Config { return { storage: new Storage(projectDir) } as unknown as Config; @@ -54,6 +69,33 @@ function task(overrides: Partial = {}): WorkflowTask { }; } +/** + * A dirent as a filesystem without d_type reports it: the name is known, the + * type is not, so every predicate answers false. Node exposes no `isUnknown()` + * — that absence is the whole reason this case needs pinning. + */ +function typeErased(name: string): Dirent { + const no = () => false; + return { + name, + isFile: no, + isDirectory: no, + isSymbolicLink: no, + isBlockDevice: no, + isCharacterDevice: no, + isFIFO: no, + isSocket: no, + } as unknown as Dirent; +} + +const EMPTY_JOURNAL: JournalCheckpoint = { + version: 1, + keyVersion: 'v2', + byteLength: 0, + sha256: 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855', + integrity: 'complete', +}; + describe('toSnapshot', () => { it.each(['running', 'pausing', 'paused'] as const)( 'rejects an active %s workflow', @@ -81,6 +123,64 @@ describe('toSnapshot', () => { expect(s.result).toMatch(/non-JSON-serializable/); }); + it('projects the serialization, not the live result object', () => { + // Validating by serializing and then handing the original object on + // proves nothing about what gets written: a getter runs again for the + // manifest and again for the snapshot, and is free to answer + // differently — or throw — the second time, turning a finished + // workflow into a persistence failure. + let reads = 0; + const result = { + get answer() { + reads += 1; + if (reads > 1) throw new Error('one shot only'); + return 42; + }, + }; + const s = toSnapshot(task({ result })); + expect(s.result).toEqual({ answer: 42 }); + // Whatever consumers do with the snapshot, the getter is done running. + expect(() => JSON.stringify(s)).not.toThrow(); + expect(JSON.parse(JSON.stringify(s)).result).toEqual({ answer: 42 }); + }); + + it('drops a result that serializes to undefined without throwing', () => { + // A function or symbol makes `JSON.stringify` return undefined rather + // than throw, so it never reached the placeholder branch; the + // enclosing stringify would have dropped the key either way. + expect(toSnapshot(task({ result: () => 1 })).result).toBeUndefined(); + }); + + it.each([ + 'x'.repeat(MAX_WORKFLOW_ARTIFACT_BYTES + 1), + { nested: ['x'.repeat(MAX_WORKFLOW_ARTIFACT_BYTES + 1)] }, + ])( + 'omits an oversized result before materializing its JSON projection', + (result) => { + expect(projectWorkflowResult(result)).toMatch(/result omitted/i); + }, + ); + + it('keeps a result whose serialized form exactly reaches the limit', () => { + const result = 'x'.repeat(MAX_WORKFLOW_ARTIFACT_BYTES - 2); + + expect(projectWorkflowResult(result)).toBe(result); + }); + + const jsonWithRawSupport = JSON as typeof JSON & { + rawJSON?: (value: string) => unknown; + }; + it.runIf(typeof jsonWithRawSupport.rawJSON === 'function')( + 'bounds raw JSON without materializing its projection', + () => { + const result = jsonWithRawSupport.rawJSON!( + JSON.stringify('x'.repeat(MAX_WORKFLOW_ARTIFACT_BYTES + 1)), + ); + + expect(projectWorkflowResult(result)).toMatch(/result omitted/i); + }, + ); + it('copies arrays defensively (snapshot is decoupled from the live entry)', () => { const t = task(); const s = toSnapshot(t); @@ -144,19 +244,46 @@ describe('writeWorkflowSnapshot + listWorkflowSnapshots', () => { ]); }); + it('omits an oversized result so the snapshot stays readable', async () => { + const config = fakeConfig(projectDir); + const runId = 'wf_0badc0de'; + await writeWorkflowSnapshot( + config, + task({ + runId, + result: 'x'.repeat(MAX_WORKFLOW_ARTIFACT_BYTES + 1), + }), + ); + + const snapshotPath = config.storage.getWorkflowRunSnapshotPath(runId); + expect((await fs.stat(snapshotPath)).size).toBeLessThanOrEqual( + MAX_WORKFLOW_ARTIFACT_BYTES, + ); + await expect(listWorkflowSnapshots(config)).resolves.toMatchObject([ + { runId, result: expect.stringMatching(/result omitted/i) }, + ]); + }); + it('freezes the snapshot projection before the first fs await', async () => { // R11-27: in-flight dispatches keep mutating the live entry across // the fs yields — a projection captured after the first await would // freeze the snapshot at an fs-timing-dependent point mid-drain // (agents_completed reading higher than the settlement value). const config = fakeConfig(projectDir); - const t = task({ runId: 'wf_freeze', agentsCompleted: 1 }); + const t = task({ + runId: 'wf_freeze', + agentsCompleted: 1, + meta: { name: 'demo', description: 'before' }, + result: { nested: { value: 'before' } }, + }); const realMkdir = fs.mkdir.bind(fs); const mkdirSpy = vi .spyOn(fs, 'mkdir') .mockImplementation(async (...args: Parameters) => { // Simulate an in-flight dispatch draining across the yield. t.agentsCompleted += 1; + t.meta!.description = 'after'; + (t.result as { nested: { value: string } }).nested.value = 'after'; return realMkdir(...args); }); try { @@ -168,6 +295,8 @@ describe('writeWorkflowSnapshot + listWorkflowSnapshots', () => { expect(list).toHaveLength(1); // The settlement value, not the post-await drained value. expect(list[0].agentsCompleted).toBe(1); + expect(list[0].meta?.description).toBe('before'); + expect(list[0].result).toEqual({ nested: { value: 'before' } }); }); it('lists newest-first by startTime', async () => { @@ -264,4 +393,1132 @@ describe('writeWorkflowSnapshot + listWorkflowSnapshots', () => { fs.access(path.join(dir, 'notarun', 'keep.txt')), ).resolves.toBeUndefined(); }); + + // A pruned legacy snapshot authorizes the recursive delete of its + // same-stem run dir only when it validates as that run's terminal + // snapshot. A malformed file (repo-planted or corrupt) must delete only + // itself — never a directory it has not proven it belongs to. + it('deletes only the file when a pruned snapshot does not validate for its same-stem run dir', async () => { + const config = fakeConfig(projectDir); + const dir = config.storage.getWorkflowRunsDir(); + await fs.mkdir(dir, { recursive: true }); + + await fs.mkdir(path.join(dir, 'wf_dead'), { recursive: true }); + await fs.writeFile( + path.join(dir, 'wf_dead', 'manifest.json'), + '{"keep":true}', + 'utf8', + ); + const malformed = path.join(dir, 'wf_dead.json'); + await fs.writeFile(malformed, '{ not json', 'utf8'); + await fs.utimes(malformed, new Date(0), new Date(0)); // oldest → pruned + + for (let i = 0; i < MAX_RETAINED_SNAPSHOTS; i++) { + await writeWorkflowSnapshot( + config, + task({ runId: `wf_${i.toString(16)}`, startTime: 10_000 + i }), + ); + } + // One more write tips the count over the cap and triggers the prune. + await writeWorkflowSnapshot( + config, + task({ runId: 'wf_ff00', startTime: 99_999 }), + ); + + await expect(fs.access(malformed)).rejects.toThrow(); + await expect( + fs.access(path.join(dir, 'wf_dead', 'manifest.json')), + ).resolves.toBeUndefined(); + }); + + it.runIf(process.platform !== 'win32')( + 'does not follow a matching run-directory symlink while pruning', + async () => { + const config = fakeConfig(projectDir); + const dir = config.storage.getWorkflowRunsDir(); + const outside = path.join(projectDir, 'outside-run'); + await fs.mkdir(dir, { recursive: true }); + await fs.mkdir(outside); + await fs.writeFile(path.join(outside, 'CANARY.txt'), 'keep', 'utf8'); + await fs.symlink(outside, path.join(dir, 'wf_dead'), 'dir'); + const oldest = path.join(dir, 'wf_dead.json'); + await fs.writeFile(oldest, '{}', 'utf8'); + await fs.utimes(oldest, new Date(0), new Date(0)); + + for (let i = 0; i < MAX_RETAINED_SNAPSHOTS; i++) { + await writeWorkflowSnapshot( + config, + task({ runId: `wf_${i.toString(16)}`, startTime: 10_000 + i }), + ); + } + + await expect( + fs.readFile(path.join(outside, 'CANARY.txt'), 'utf8'), + ).resolves.toBe('keep'); + }, + ); +}); + +describe('durable workflow manifests', () => { + let projectDir: string; + + beforeEach(async () => { + projectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'wf-manifest-')); + }); + afterEach(async () => { + await fs.rm(projectDir, { recursive: true, force: true }); + }); + + it('writes a current manifest atomically with exact replay inputs', async () => { + const config = fakeConfig(projectDir); + const t = task({ + runId: 'wf_a1b2', + status: 'paused', + script: 'export default async ({ value }) => value;', + scriptPath: '/trusted/project/.qwen/workflows/demo.js', + }); + const manifest = await writeWorkflowManifest(config, t, { + args: { value: 'hello' }, + journal: EMPTY_JOURNAL, + checkpointAt: 1_700_000_004_000, + }); + + expect(manifest).toMatchObject({ + schemaVersion: WORKFLOW_MANIFEST_SCHEMA_VERSION, + runtimeVersion: WORKFLOW_RUNTIME_VERSION, + runId: 'wf_a1b2', + status: 'paused', + script: 'export default async ({ value }) => value;', + scriptPath: '/trusted/project/.qwen/workflows/demo.js', + args: { value: 'hello' }, + checkpointAt: 1_700_000_004_000, + journal: EMPTY_JOURNAL, + canResume: true, + }); + expect(manifest?.scriptHash).toMatch(/^[0-9a-f]{64}$/); + + const manifestPath = config.storage.getWorkflowRunManifestPath('wf_a1b2'); + const stat = await fs.stat(manifestPath); + if (process.platform !== 'win32') { + expect(stat.mode & 0o777).toBe(0o600); + } + expect( + (await fs.readdir(config.storage.getWorkflowRunDir('wf_a1b2'))).filter( + (name) => name.endsWith('.tmp'), + ), + ).toEqual([]); + await expect(readWorkflowManifest(config, 'wf_a1b2')).resolves.toEqual( + manifest, + ); + }); + + it('omits an oversized result so the manifest stays readable', async () => { + const config = fakeConfig(projectDir); + const runId = 'wf_0badc0de'; + + const manifest = await writeWorkflowManifest( + config, + task({ + runId, + status: 'paused', + result: 'x'.repeat(MAX_WORKFLOW_ARTIFACT_BYTES + 1), + }), + { args: null, journal: EMPTY_JOURNAL }, + ); + const manifestPath = config.storage.getWorkflowRunManifestPath(runId); + expect((await fs.stat(manifestPath)).size).toBeLessThanOrEqual( + MAX_WORKFLOW_ARTIFACT_BYTES, + ); + expect(manifest?.result).toMatch(/result omitted/i); + await expect(readWorkflowManifest(config, runId)).resolves.toEqual( + manifest, + ); + }); + + it.skipIf(process.platform === 'win32')( + 'opens a manifest nonblocking before validating the opened file', + async () => { + const config = fakeConfig(projectDir); + await writeWorkflowManifest( + config, + task({ runId: 'wf_a1b3', status: 'paused' }), + { args: null, journal: EMPTY_JOURNAL }, + ); + const realOpen = fs.open.bind(fs); + const flags: number[] = []; + const openSpy = vi + .spyOn(fs, 'open') + .mockImplementation((path, flag, mode) => { + if (typeof flag === 'number') flags.push(flag); + return realOpen(path, flag, mode); + }); + try { + await readWorkflowManifest(config, 'wf_a1b3'); + } finally { + openSpy.mockRestore(); + } + + expect(flags).toContain( + fsConstants.O_RDONLY | fsConstants.O_NOFOLLOW | fsConstants.O_NONBLOCK, + ); + }, + ); + + it('atomically replaces an existing manifest inode', async () => { + const config = fakeConfig(projectDir); + const runId = 'wf_a70c'; + const t = task({ runId, status: 'running' }); + await writeWorkflowManifest(config, t, { + args: null, + journal: EMPTY_JOURNAL, + checkpointAt: 1, + }); + const manifestPath = config.storage.getWorkflowRunManifestPath(runId); + const before = await fs.stat(manifestPath); + + await writeWorkflowManifest(config, t, { + args: null, + journal: EMPTY_JOURNAL, + checkpointAt: 2, + }); + const after = await fs.stat(manifestPath); + + if (process.platform !== 'win32') { + expect(after.ino).not.toBe(before.ino); + } + expect((await readWorkflowManifest(config, runId)).checkpointAt).toBe(2); + }); + + it('freezes the manifest before the first filesystem await', async () => { + const config = fakeConfig(projectDir); + const t = task({ + runId: 'wf_a11ce', + status: 'running', + agentsCompleted: 1, + meta: { name: 'demo', description: 'before' }, + result: { nested: { value: 'before' } }, + }); + const journal = { ...EMPTY_JOURNAL }; + const realMkdir = fs.mkdir.bind(fs); + let mutated = false; + const mkdirSpy = vi + .spyOn(fs, 'mkdir') + .mockImplementation(async (...args: Parameters) => { + if (!mutated) { + mutated = true; + t.agentsCompleted = 2; + t.phases.push('Late'); + t.meta!.description = 'after'; + (t.result as { nested: { value: string } }).nested.value = 'after'; + journal.byteLength = 100; + } + return realMkdir(...args); + }); + try { + await writeWorkflowManifest(config, t, { + args: null, + journal, + status: 'paused', + }); + } finally { + mkdirSpy.mockRestore(); + } + + const manifest = await readWorkflowManifest(config, t.runId); + expect(manifest).toMatchObject({ + status: 'paused', + agentsCompleted: 1, + phases: ['Plan', 'Build'], + meta: { name: 'demo', description: 'before' }, + result: { nested: { value: 'before' } }, + journal: EMPTY_JOURNAL, + }); + }); + + describe('durable run ownership', () => { + const owner = (nonce: string) => ({ + machineId: 'test-host', + pid: 4242, + nonce, + acquiredAt: 1000, + }); + + /** Put a generation-0 manifest on disk for a run to be claimed. */ + const seed = async (config: Config, runId: string) => { + const t = task({ runId, status: 'running' }); + await writeWorkflowManifest(config, t, { + args: null, + journal: { ...EMPTY_JOURNAL }, + status: 'running', + }); + return t; + }; + + it('claims a run by advancing its generation', async () => { + const config = fakeConfig(projectDir); + await seed(config, 'wf_0e1'); + + const generation = await acquireWorkflowRunOwnership( + config, + 'wf_0e1', + owner('nonce-a'), + ); + + expect(generation).toBe(1); + const manifest = await readWorkflowManifest(config, 'wf_0e1'); + expect(manifest.generation).toBe(1); + expect(manifest.owner).toMatchObject({ nonce: 'nonce-a', pid: 4242 }); + }); + + it('takes over from a previous owner without probing whether it is alive', async () => { + // The property that makes this a generation counter and not a lock: a + // process that died holding the run leaves a number behind, so the + // next one reads it and carries on. No stale-lock policy, no liveness + // oracle, nothing for a human to clear. + const config = fakeConfig(projectDir); + await seed(config, 'wf_0e2'); + await acquireWorkflowRunOwnership(config, 'wf_0e2', owner('dead')); + + const generation = await acquireWorkflowRunOwnership( + config, + 'wf_0e2', + owner('nonce-b'), + ); + + expect(generation).toBe(2); + expect( + (await readWorkflowManifest(config, 'wf_0e2')).owner, + ).toMatchObject({ nonce: 'nonce-b' }); + }); + + it('refuses a write whose generation the run has moved past', async () => { + // Deliberately the *same* owner on both sides. A stranger is already + // refused by the nonce check below, so using one here would leave the + // generation comparison untested — the mutation that deletes it stays + // green. What this pins is the in-process case: two durable writes for + // one run overlapping, where the loser must not stamp its older view + // over the newer one. + const config = fakeConfig(projectDir); + const t = await seed(config, 'wf_0e3'); + const mine = owner('nonce-mine'); + const claimed = await acquireWorkflowRunOwnership(config, 'wf_0e3', mine); + await writeWorkflowManifest(config, t, { + args: null, + journal: { ...EMPTY_JOURNAL }, + status: 'pausing', + ownership: { owner: mine, expectedGeneration: claimed }, + }); + + await expect( + writeWorkflowManifest(config, t, { + args: null, + journal: { ...EMPTY_JOURNAL }, + status: 'paused', + ownership: { owner: mine, expectedGeneration: claimed }, + }), + ).rejects.toThrow(WorkflowRunOwnershipError); + + // ...and the newer write is intact, not half-overwritten by the loser. + const manifest = await readWorkflowManifest(config, 'wf_0e3'); + expect(manifest.status).toBe('pausing'); + expect(manifest.generation).toBe(claimed + 1); + }); + + it('refuses a write from a stranger holding the right generation', async () => { + // A takeover that committed and was then rolled back to our number is + // indistinguishable from divergence, so the nonce is checked too + // rather than trusting the counter alone. + const config = fakeConfig(projectDir); + const t = await seed(config, 'wf_0e4'); + await acquireWorkflowRunOwnership(config, 'wf_0e4', owner('nonce-real')); + + await expect( + writeWorkflowManifest(config, t, { + args: null, + journal: { ...EMPTY_JOURNAL }, + status: 'paused', + ownership: { owner: owner('nonce-impostor'), expectedGeneration: 1 }, + }), + ).rejects.toThrow(/owned by another writer/); + }); + + it('advances the generation on each accepted write', async () => { + const config = fakeConfig(projectDir); + const t = await seed(config, 'wf_0e5'); + const mine = owner('nonce-mine'); + let generation = await acquireWorkflowRunOwnership( + config, + 'wf_0e5', + mine, + ); + + for (const status of ['running', 'pausing', 'paused'] as const) { + const written = await writeWorkflowManifest(config, t, { + args: null, + journal: { ...EMPTY_JOURNAL }, + status, + ownership: { owner: mine, expectedGeneration: generation }, + }); + expect(written?.generation).toBe(generation + 1); + generation = written!.generation!; + } + + expect((await readWorkflowManifest(config, 'wf_0e5')).generation).toBe(4); + }); + + it('leaves an unclaimed run unstamped', async () => { + // A write without an ownership claim (direct API use, tests) still + // pays no CAS read and stamps no generation. + const config = fakeConfig(projectDir); + await seed(config, 'wf_0e6'); + const manifest = await readWorkflowManifest(config, 'wf_0e6'); + expect(manifest.generation).toBeUndefined(); + expect(manifest.owner).toBeUndefined(); + }); + + // The CAS must pin the target inode BEFORE reading its content. With + // the read first, a competing commit landing between the read and the + // pin was caught by neither check: the generation check saw pre-commit + // content and the pin captured the competitor's inode, so both writers + // committed. The swap below lands on the 3rd manifest lstat of an + // ownership write, which is inside the window under either ordering — + // the read's post-open identity check with pin-then-read, the pin's + // own lstat with read-then-pin. + const raceCompetingCommit = async ( + drive: (config: Config) => Promise, + seedRunId: string, + ) => { + const config = fakeConfig(projectDir); + await seed(config, seedRunId); + const mine = owner('nonce-mine'); + const claimed = await acquireWorkflowRunOwnership( + config, + seedRunId, + mine, + ); + const manifestPath = config.storage.getWorkflowRunManifestPath(seedRunId); + const onDisk = JSON.parse( + await fs.readFile(manifestPath, 'utf8'), + ) as Record; + const rivalCommit = JSON.stringify( + { + ...onDisk, + generation: claimed + 1, + owner: owner('nonce-rival'), + }, + null, + 2, + ); + + let manifestLstats = 0; + const realLstat = fs.lstat.bind(fs); + const lstatSpy = vi + .spyOn(fs, 'lstat') + .mockImplementation(async (...args: Parameters) => { + if (args[0] === manifestPath) { + manifestLstats += 1; + if (manifestLstats === 3) { + const tmp = `${manifestPath}.rival`; + await fs.writeFile(tmp, rivalCommit); + await fs.rename(tmp, manifestPath); + } + } + return realLstat(...args); + }); + try { + await expect(drive(config)).rejects.toThrow(); + } finally { + lstatSpy.mockRestore(); + } + + // The rival commit survives; the loser never renamed over it. + const durable = JSON.parse(await fs.readFile(manifestPath, 'utf8')) as { + generation: number; + owner: { nonce: string }; + }; + expect(durable.generation).toBe(claimed + 1); + expect(durable.owner.nonce).toBe('nonce-rival'); + }; + + it('refuses a competing commit landing inside the settlement CAS window', async () => { + const t = task({ runId: 'wf_0e7', status: 'running' }); + await raceCompetingCommit( + (config) => + writeWorkflowManifest(config, t, { + args: null, + journal: { ...EMPTY_JOURNAL }, + status: 'paused', + ownership: { + owner: owner('nonce-mine'), + expectedGeneration: 1, + }, + }), + 'wf_0e7', + ); + }); + + it('refuses a competing commit landing inside the acquisition CAS window', async () => { + await raceCompetingCommit( + (config) => + acquireWorkflowRunOwnership(config, 'wf_0e8', owner('nonce-late')), + 'wf_0e8', + ); + }); + }); + + it('persists invalid args as non-resumable without persisting their values', async () => { + const cyclic: Record = {}; + cyclic['self'] = cyclic; + const sparse = Array.from({ length: 2 }) as unknown[]; + sparse[1] = 'value'; + const disguisedSparse = Array.from({ length: 2 }) as unknown[] & { + extra?: string; + }; + disguisedSparse[1] = 'value'; + disguisedSparse.extra = 'PRIVATE_ARRAY_SENTINEL'; + const hiddenProperty: Record = { visible: true }; + Object.defineProperty(hiddenProperty, 'hidden', { + value: 'PRIVATE_HIDDEN_SENTINEL', + enumerable: false, + }); + const deeplyNested: unknown[] = []; + let nestedCursor = deeplyNested; + for (let depth = 0; depth < 12_000; depth++) { + const next: unknown[] = []; + nestedCursor.push(next); + nestedCursor = next; + } + const cases: Array<[string, unknown]> = [ + ['cyclic', cyclic], + ['sparse-array', sparse], + ['sparse-array-with-extra-key', disguisedSparse], + ['non-enumerable-property', hiddenProperty], + ['too-deep', deeplyNested], + ['bigint', { secret: 10n }], + // Non-round-trippable in the same way bigint is, but quieter about it: + // JSON.stringify coerces NaN/Infinity to null instead of throwing, so + // without `cloneJsonArgs`'s Number.isFinite gate these persist as + // `{ secret: null }` with canResume true — and the resume drift check + // cannot tell that apart from a caller who really passed null. + ['nan', { secret: Number.NaN }], + ['infinity', { secret: Number.POSITIVE_INFINITY }], + // Quieter still: -0 is finite and stringifies to "0", so it cleared + // both gates above and persisted as +0 under canResume true. A script + // branching on Object.is(args.secret, -0) takes the other path after + // a resume the manifest promised was faithful. + ['negative-zero', { secret: -0 }], + ['function', { secret: () => 'PRIVATE_FUNCTION_SENTINEL' }], + ['symbol', { secret: Symbol('PRIVATE_SYMBOL_SENTINEL') }], + ['too-large', { secret: 'x'.repeat(MAX_WORKFLOW_ARGS_BYTES + 1) }], + ]; + + for (const [index, [name, args]] of cases.entries()) { + const config = fakeConfig(projectDir); + const runId = `wf_f${index}`; + const manifest = await writeWorkflowManifest( + config, + task({ runId, status: 'running' }), + { args, journal: EMPTY_JOURNAL }, + ); + expect({ name, canResume: manifest?.canResume }).toEqual({ + name, + canResume: false, + }); + expect(manifest?.resumeBlockedReason).toMatch(/args/i); + expect(manifest).not.toHaveProperty('args'); + const serialized = await fs.readFile( + config.storage.getWorkflowRunManifestPath(runId), + 'utf8', + ); + expect({ name, leaked: serialized.includes('PRIVATE_') }).toEqual({ + name, + leaked: false, + }); + } + }); + + it('uses UTF-8 bytes, not character count, for the args limit', async () => { + const config = fakeConfig(projectDir); + const args = { value: '界'.repeat(Math.ceil(MAX_WORKFLOW_ARGS_BYTES / 3)) }; + const manifest = await writeWorkflowManifest( + config, + task({ runId: 'wf_beef', status: 'running' }), + { args, journal: EMPTY_JOURNAL }, + ); + expect(manifest?.canResume).toBe(false); + expect(manifest).not.toHaveProperty('args'); + }); + + it('does not persist pending approval payloads or unrelated runtime state', async () => { + const config = fakeConfig(projectDir); + const t = task({ + runId: 'wf_cafe', + status: 'running', + pendingApprovals: [ + { + approvalId: 'APPROVAL_ID_SENTINEL', + subagentId: 'agent-a', + callId: 'call-1', + name: 'Tool', + description: 'TOOL_PARAMS_SENTINEL', + confirmationDetails: { + type: 'info', + title: 'PROMPT_SENTINEL', + prompt: 'PROMPT_SENTINEL', + }, + at: 1, + }, + ], + }); + await writeWorkflowManifest(config, t, { + args: { public: true }, + journal: EMPTY_JOURNAL, + }); + const serialized = await fs.readFile( + config.storage.getWorkflowRunManifestPath(t.runId), + 'utf8', + ); + expect(serialized).not.toContain('APPROVAL_ID_SENTINEL'); + expect(serialized).not.toContain('TOOL_PARAMS_SENTINEL'); + expect(serialized).not.toContain('PROMPT_SENTINEL'); + }); + + it('fails closed for old, future, corrupt, truncated, and hash-mismatched manifests', async () => { + const config = fakeConfig(projectDir); + const dir = config.storage.getWorkflowRunsDir(); + await fs.mkdir(dir, { recursive: true }); + const cases: Array<[string, string]> = [ + [ + 'wf_01', + JSON.stringify({ + schemaVersion: 1, + runId: 'wf_01', + status: 'paused', + startTime: 10, + }), + ], + [ + 'wf_02', + JSON.stringify({ + schemaVersion: WORKFLOW_MANIFEST_SCHEMA_VERSION + 1, + runId: 'wf_02', + status: 'paused', + startTime: 20, + }), + ], + ['wf_03', '{ not json'], + ['wf_04', '{"schemaVersion":2'], + ]; + for (const [runId, raw] of cases) { + const runDir = config.storage.getWorkflowRunDir(runId); + await fs.mkdir(runDir, { recursive: true }); + await fs.writeFile( + config.storage.getWorkflowRunManifestPath(runId), + raw, + 'utf8', + ); + } + const valid = await writeWorkflowManifest( + config, + task({ runId: 'wf_05', status: 'paused', startTime: 50 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + await fs.writeFile( + config.storage.getWorkflowRunManifestPath('wf_05'), + JSON.stringify({ ...valid, scriptHash: '0'.repeat(64) }), + 'utf8', + ); + await fs.mkdir(config.storage.getWorkflowRunDir('wf_06')); + // Same shape as wf_05, and for the same reason: pin `runtimeVersion` + // independently of `schemaVersion`. The raw cases above only ever vary + // schemaVersion, so the `runtimeVersion` clause in parseManifest — which + // looks redundant next to the schema check — can be dropped during a + // version bump with every test still green. A manifest written by a + // different runtime would then parse, and a resume would replay its + // journal under semantics that no longer match. + const otherRuntime = await writeWorkflowManifest( + config, + task({ runId: 'wf_07', status: 'paused', startTime: 70 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + await fs.writeFile( + config.storage.getWorkflowRunManifestPath('wf_07'), + JSON.stringify({ + ...otherRuntime, + runtimeVersion: WORKFLOW_RUNTIME_VERSION + 1, + }), + 'utf8', + ); + + const records = await listWorkflowRunRecords(config); + expect(records.map((record) => record.runId).sort()).toEqual([ + 'wf_01', + 'wf_02', + 'wf_03', + 'wf_04', + 'wf_05', + 'wf_06', + 'wf_07', + ]); + expect(records.every((record) => record.canResume === false)).toBe(true); + expect(records.every((record) => Boolean(record.resumeBlockedReason))).toBe( + true, + ); + for (const runId of cases + .map(([id]) => id) + .concat('wf_05', 'wf_06', 'wf_07')) { + await expect(readWorkflowManifest(config, runId)).rejects.toThrow(); + } + }); + + it('rejects non-string status and journal integrity instead of coercing them', async () => { + // `String(['paused'])` is 'paused': a whitelist that coerces before it + // compares admits single-element arrays, which then escape as the + // manifest's status and abort the whole `/workflows` listing at the + // first `entry.status.padEnd`. The journal integrity whitelist had the + // identical hole. + const config = fakeConfig(projectDir); + const runId = 'wf_c0c0'; + const original = await writeWorkflowManifest( + config, + task({ runId, status: 'running', startTime: 10 }), + { args: null, journal: { ...EMPTY_JOURNAL }, status: 'running' }, + ); + const manifestPath = config.storage.getWorkflowRunManifestPath(runId); + + await fs.writeFile( + manifestPath, + JSON.stringify({ ...original, status: ['paused'] }, null, 2), + 'utf8', + ); + await expect(readWorkflowManifest(config, runId)).rejects.toThrow( + /invalid status/, + ); + + await fs.writeFile( + manifestPath, + JSON.stringify( + { + ...original, + journal: { ...original!.journal, integrity: ['complete'] }, + }, + null, + 2, + ), + 'utf8', + ); + await expect(readWorkflowManifest(config, runId)).rejects.toThrow( + /invalid journal metadata/, + ); + + // Both shapes degrade to an invalid record instead of crashing the + // listing. + const records = await listWorkflowRunRecords(config); + expect(records).toHaveLength(1); + expect(records[0]).toMatchObject({ + runId, + status: 'interrupted', + canResume: false, + }); + }); + + it('converts structurally invalid v2 and legacy records into safe invalid records', async () => { + const config = fakeConfig(projectDir); + const malformedV2 = await writeWorkflowManifest( + config, + task({ runId: 'wf_bad1', status: 'paused' }), + { args: null, journal: EMPTY_JOURNAL }, + ); + await fs.writeFile( + config.storage.getWorkflowRunManifestPath('wf_bad1'), + JSON.stringify({ ...malformedV2, perPhaseTokens: [1] }), + ); + await fs.mkdir(config.storage.getWorkflowRunsDir(), { recursive: true }); + await fs.writeFile( + config.storage.getWorkflowRunSnapshotPath('wf_bad2'), + '{}', + ); + + const records = await listWorkflowRunRecords(config); + for (const runId of ['wf_bad1', 'wf_bad2']) { + expect(records.find((record) => record.runId === runId)).toMatchObject({ + status: 'interrupted', + canResume: false, + resumeBlockedReason: expect.stringMatching(/invalid/i), + }); + } + await expect(readWorkflowManifest(config, 'wf_bad1')).rejects.toThrow( + /invalid/i, + ); + }); + + it('rejects non-string manifest status and journal integrity values', async () => { + const config = fakeConfig(projectDir); + for (const [runId, mutate] of [ + [ + 'wf_bad5', + (manifest: WorkflowManifestV2) => ({ + ...manifest, + status: ['paused'], + }), + ], + [ + 'wf_bad6', + (manifest: WorkflowManifestV2) => ({ + ...manifest, + canResume: false, + resumeBlockedReason: 'test fixture', + journal: { ...manifest.journal, integrity: ['complete'] }, + }), + ], + ] as const) { + const manifest = await writeWorkflowManifest( + config, + task({ runId, status: 'paused' }), + { args: null, journal: EMPTY_JOURNAL }, + ); + await fs.writeFile( + config.storage.getWorkflowRunManifestPath(runId), + JSON.stringify(mutate(manifest!)), + ); + + await expect(readWorkflowManifest(config, runId)).rejects.toThrow( + /invalid/i, + ); + expect( + (await listWorkflowRunRecords(config)).find( + (record) => record.runId === runId, + ), + ).toMatchObject({ status: 'interrupted', canResume: false }); + } + }); + + it.runIf(process.platform !== 'win32')( + 'ignores unsafe legacy filenames instead of exposing terminal controls', + async () => { + const config = fakeConfig(projectDir); + const dir = config.storage.getWorkflowRunsDir(); + await fs.mkdir(dir, { recursive: true }); + for (const runId of [ + 'wf_bad\nCONTROL_SENTINEL', + 'wf_bad\u001b[31mCONTROL_SENTINEL', + `wf_${'x'.repeat(129)}`, + ]) { + await fs.writeFile( + path.join(dir, `${runId}.json`), + JSON.stringify(task({ runId, status: 'completed' })), + ); + } + + const serialized = JSON.stringify(await listWorkflowRunRecords(config)); + expect(serialized).not.toContain('CONTROL_SENTINEL'); + expect(serialized).not.toContain(`wf_${'x'.repeat(129)}`); + }, + ); + + it('refuses to buffer artifacts above the read cap', async () => { + const config = fakeConfig(projectDir); + const dir = config.storage.getWorkflowRunsDir(); + await fs.mkdir(dir, { recursive: true }); + // Sparse files: the apparent size clears the cap without allocating it. + // A repo-shipped multi-gigabyte artifact must be refused from the stat + // that is already in hand, because an OOM abort is not catchable and + // would take the whole listing down instead of one record. + const oversize = async (filePath: string) => { + await fs.mkdir(path.dirname(filePath), { recursive: true }); + const handle = await fs.open(filePath, 'w'); + try { + await handle.truncate(MAX_WORKFLOW_ARTIFACT_BYTES + 1); + } finally { + await handle.close(); + } + }; + await oversize(path.join(dir, 'wf_a1.json')); + await oversize(config.storage.getWorkflowRunManifestPath('wf_b2')); + + const records = await listWorkflowRunRecords(config); + for (const runId of ['wf_a1', 'wf_b2']) { + expect(records.find((record) => record.runId === runId)).toMatchObject({ + status: 'interrupted', + canResume: false, + resumeBlockedReason: expect.stringMatching(/too large/i), + }); + } + await expect(readWorkflowManifest(config, 'wf_b2')).rejects.toThrow( + /too large/i, + ); + await expect(listWorkflowSnapshots(config)).resolves.toEqual([]); + }); + + it('lists runs whose dirent type readdir declined to report', async () => { + // Filesystems that omit d_type (sshfs/s3fs/rclone without readdirplus, + // some NFSv3 exports) hand back DT_UNKNOWN dirents, which Node models by + // answering false to every type predicate. Classifying by predicate alone + // brands valid legacy snapshots "unsafe" and skips valid run directories + // outright — durable history and resume vanish on exactly the setups where + // a shared workspace makes them matter most. + const config = fakeConfig(projectDir); + await writeWorkflowSnapshot( + config, + task({ runId: 'wf_1eaa', status: 'completed', startTime: 1 }), + ); + await writeWorkflowManifest( + config, + task({ runId: 'wf_2fbb', status: 'paused', startTime: 2 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + + // Baseline on a d_type-reporting filesystem, so the assertion below pins + // equivalence rather than merely "something came back". + const reported = await listWorkflowRunRecords(config); + expect(reported.map((record) => record.runId).sort()).toEqual([ + 'wf_1eaa', + 'wf_2fbb', + ]); + + const realReaddir = fs.readdir.bind(fs) as ( + dir: string, + options?: { withFileTypes?: boolean }, + ) => Promise; + const readdirSpy = vi.spyOn(fs, 'readdir').mockImplementation((async ( + dir: string, + options?: { withFileTypes?: boolean }, + ) => { + const entries = await realReaddir(dir, options); + if (!options?.withFileTypes) return entries; + // Same names, same order, type erased — the one difference a + // d_type-less filesystem makes. + return (entries as Dirent[]).map((entry) => typeErased(entry.name)); + }) as unknown as typeof fs.readdir); + + try { + expect(await listWorkflowRunRecords(config)).toEqual(reported); + } finally { + readdirSpy.mockRestore(); + } + }); + + it('rejects a terminal manifest that claims it can be resumed', async () => { + const config = fakeConfig(projectDir); + const runId = 'wf_bad3'; + const manifest = await writeWorkflowManifest( + config, + task({ runId, status: 'completed' }), + { args: null, journal: EMPTY_JOURNAL }, + ); + const contradictory = { ...manifest, canResume: true }; + delete contradictory.resumeBlockedReason; + await fs.writeFile( + config.storage.getWorkflowRunManifestPath(runId), + JSON.stringify(contradictory), + ); + + await expect(readWorkflowManifest(config, runId)).rejects.toThrow( + /resume metadata/i, + ); + expect( + (await listWorkflowRunRecords(config)).find( + (record) => record.runId === runId, + ), + ).toMatchObject({ status: 'interrupted', canResume: false }); + }); + + it('marks a manifest non-recoverable when its committed journal is truncated', async () => { + const config = fakeConfig(projectDir); + const runId = 'wf_fade'; + const journal = new WorkflowJournal( + config.storage.getWorkflowRunJournalPath(runId), + ); + await journal.append({ type: 'started', key: 'v2:key', agentId: '1' }); + const checkpoint = await journal.flush(); + await writeWorkflowManifest(config, task({ runId, status: 'paused' }), { + args: null, + journal: checkpoint, + }); + await fs.truncate(journal.path, checkpoint.byteLength - 1); + + await expect(readWorkflowManifest(config, runId)).rejects.toThrow( + /truncated/i, + ); + expect( + (await listWorkflowRunRecords(config)).find( + (record) => record.runId === runId, + ), + ).toMatchObject({ + status: 'interrupted', + canResume: false, + resumeBlockedReason: expect.stringMatching(/truncated/i), + }); + }); + + it('treats an orphaned active manifest as interrupted and keeps terminal status', async () => { + const config = fakeConfig(projectDir); + await writeWorkflowManifest( + config, + task({ runId: 'wf_10', status: 'running', startTime: 10 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + await writeWorkflowManifest( + config, + task({ runId: 'wf_20', status: 'completed', startTime: 20 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + const records = await listWorkflowRunRecords(config); + expect(records).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + runId: 'wf_10', + status: 'interrupted', + originalStatus: 'running', + canResume: true, + }), + expect.objectContaining({ + runId: 'wf_20', + status: 'completed', + canResume: false, + resumeBlockedReason: expect.stringMatching(/rerun/i), + }), + ]), + ); + }); + + it('keeps legacy snapshots display-only and rejects only a stale active manifest', async () => { + const config = fakeConfig(projectDir); + await writeWorkflowSnapshot( + config, + task({ runId: 'wf_aa', status: 'completed', startTime: 1 }), + ); + await fs.mkdir(config.storage.getWorkflowRunDir('wf_aa')); + await fs.writeFile( + config.storage.getWorkflowRunJournalPath('wf_aa'), + '{}\n', + 'utf8', + ); + await writeWorkflowSnapshot( + config, + task({ runId: 'wf_bb', status: 'failed', startTime: 2 }), + ); + await writeWorkflowManifest( + config, + task({ runId: 'wf_bb', status: 'paused', startTime: 3 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + await writeWorkflowSnapshot( + config, + task({ runId: 'wf_cc', status: 'failed', startTime: 4 }), + ); + await writeWorkflowManifest( + config, + task({ runId: 'wf_cc', status: 'failed', startTime: 5 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + await writeWorkflowSnapshot( + config, + task({ runId: 'wf_dd', status: 'completed', startTime: 6 }), + ); + await writeWorkflowManifest( + config, + task({ runId: 'wf_dd', status: 'paused', startTime: 7 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + await fs.writeFile( + config.storage.getWorkflowRunManifestPath('wf_dd'), + '{broken', + ); + await writeWorkflowSnapshot( + config, + task({ runId: 'wf_ee', status: 'cancelled', startTime: 8 }), + ); + await writeWorkflowManifest( + config, + task({ runId: 'wf_ee', status: 'completed', startTime: 9 }), + { args: null, journal: EMPTY_JOURNAL }, + ); + + const records = await listWorkflowRunRecords(config); + expect(records).toHaveLength(5); + expect(records.find((record) => record.runId === 'wf_aa')).toMatchObject({ + status: 'completed', + canResume: false, + resumeBlockedReason: expect.stringMatching(/legacy/i), + }); + expect(records.find((record) => record.runId === 'wf_bb')).toMatchObject({ + status: 'failed', + schemaVersion: 1, + canResume: false, + resumeBlockedReason: expect.stringMatching(/legacy/i), + }); + await expect(readWorkflowManifest(config, 'wf_bb')).rejects.toThrow( + /terminal snapshot/i, + ); + expect(records.find((record) => record.runId === 'wf_cc')).toMatchObject({ + status: 'failed', + schemaVersion: WORKFLOW_MANIFEST_SCHEMA_VERSION, + canResume: false, + resumeBlockedReason: expect.stringMatching(/rerun/i), + }); + expect(records.find((record) => record.runId === 'wf_dd')).toMatchObject({ + status: 'interrupted', + schemaVersion: 0, + canResume: false, + }); + expect(records.find((record) => record.runId === 'wf_ee')).toMatchObject({ + status: 'cancelled', + schemaVersion: 1, + canResume: false, + }); + await expect(readWorkflowManifest(config, 'wf_ee')).rejects.toThrow( + /conflicts/i, + ); + }); + + it('rejects traversal ids and never follows run-directory or manifest symlinks', async () => { + const config = fakeConfig(projectDir); + await expect( + writeWorkflowManifest(config, task({ runId: '../escape' }), { + args: null, + journal: EMPTY_JOURNAL, + }), + ).rejects.toThrow(/run id/i); + await writeWorkflowSnapshot( + config, + task({ runId: '../escape', status: 'completed' }), + ); + await expect( + fs.stat( + path.join(config.storage.getWorkflowRunsDir(), '..', 'escape.json'), + ), + ).rejects.toMatchObject({ code: 'ENOENT' }); + + const runsDir = config.storage.getWorkflowRunsDir(); + const outsideDir = path.join(projectDir, 'outside'); + await fs.mkdir(runsDir, { recursive: true }); + await fs.mkdir(outsideDir); + await fs.symlink(outsideDir, path.join(runsDir, 'wf_dead'), 'dir'); + await expect( + writeWorkflowManifest(config, task({ runId: 'wf_dead' }), { + args: null, + journal: EMPTY_JOURNAL, + }), + ).rejects.toThrow(/directory|symlink/i); + await expect(fs.readdir(outsideDir)).resolves.toEqual([]); + + const manifest = await writeWorkflowManifest( + config, + task({ runId: 'wf_feed', status: 'paused' }), + { args: null, journal: EMPTY_JOURNAL }, + ); + const canary = path.join(projectDir, 'CANARY.json'); + await fs.writeFile(canary, JSON.stringify(manifest), 'utf8'); + const manifestPath = config.storage.getWorkflowRunManifestPath('wf_feed'); + await fs.unlink(manifestPath); + await fs.symlink(canary, manifestPath); + await expect(readWorkflowManifest(config, 'wf_feed')).rejects.toThrow( + /symlink/i, + ); + const records = await listWorkflowRunRecords(config); + expect(records.find((record) => record.runId === 'wf_feed')).toMatchObject({ + canResume: false, + }); + }); }); diff --git a/packages/core/src/agents/workflow-snapshot.ts b/packages/core/src/agents/workflow-snapshot.ts index 743c9ab29b5..5ec473c970b 100644 --- a/packages/core/src/agents/workflow-snapshot.ts +++ b/packages/core/src/agents/workflow-snapshot.ts @@ -13,13 +13,34 @@ * for caching): a snapshot is the whole-run summary. */ -import { promises as fs } from 'node:fs'; +import { createHash, randomBytes } from 'node:crypto'; +import { + constants as fsConstants, + type Dirent, + type Stats, + promises as fs, + lstatSync, + realpathSync, +} from 'node:fs'; +import { hostname } from 'node:os'; +import * as path from 'node:path'; import type { Config } from '../config/config.js'; +import type { Storage } from '../config/storage.js'; +import { atomicWriteFile } from '../utils/atomicFileWrite.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import type { WorkflowMeta } from './runtime/workflow-sandbox.js'; import { + JOURNAL_FORMAT_VERSION, + JOURNAL_KEY_VERSION, + WorkflowJournal, + readBounded, + type JournalCheckpoint, +} from './runtime/workflow-journal.js'; +import { + isActiveWorkflowStatus, isTerminalWorkflowStatus, type WorkflowTask, + type WorkflowStatus, type WorkflowTerminalStatus, } from './workflow-run-registry.js'; @@ -28,6 +49,64 @@ const debugLogger = createDebugLogger('WORKFLOW_SNAPSHOT'); /** Cap on snapshots retained on disk; oldest are pruned on write. */ export const MAX_RETAINED_SNAPSHOTS = 30; +export const WORKFLOW_MANIFEST_SCHEMA_VERSION = 2; +export const WORKFLOW_RUNTIME_VERSION = 1; +export const MAX_WORKFLOW_ARGS_BYTES = 64 * 1024; + +/** + * Upper bound on a single manifest/snapshot artifact read into memory. + * `/workflows` ships with the repository, so a clone can carry a + * multi-gigabyte `wf_.json`; buffering that aborts the process (heap OOM + * is not a catchable exception, so the per-entry `catch` that degrades every + * other malformed artifact to an `invalidRecord` never runs). The bound is + * far above any real artifact — these are small JSON documents whose largest + * field is the workflow's own `result`. + */ +export const MAX_WORKFLOW_ARTIFACT_BYTES = 32 * 1024 * 1024; + +const WORKFLOW_RESULT_TOO_LARGE = Symbol('workflowResultTooLarge'); + +function omittedWorkflowResult(): string { + return ( + `(workflow result omitted: artifact exceeded the ` + + `${MAX_WORKFLOW_ARTIFACT_BYTES} byte limit)` + ); +} + +function serializeWorkflowArtifact( + artifact: T, +): string { + let serialized = JSON.stringify(artifact, null, 2); + let byteLength = Buffer.byteLength(serialized, 'utf8'); + if (byteLength <= MAX_WORKFLOW_ARTIFACT_BYTES) return serialized; + + if (artifact.result !== undefined) { + serialized = JSON.stringify( + { + ...artifact, + result: omittedWorkflowResult(), + }, + null, + 2, + ); + byteLength = Buffer.byteLength(serialized, 'utf8'); + if (byteLength <= MAX_WORKFLOW_ARTIFACT_BYTES) return serialized; + } + + throw new Error( + `Workflow artifact is too large to persist (${byteLength} bytes, ` + + `limit ${MAX_WORKFLOW_ARTIFACT_BYTES}).`, + ); +} + +type JsonValue = + | null + | boolean + | number + | string + | JsonValue[] + | { [key: string]: JsonValue }; + /** JSON-serializable projection of a terminal workflow run. */ export interface WorkflowSnapshot { runId: string; @@ -49,15 +128,223 @@ export interface WorkflowSnapshot { error?: string; } +export interface WorkflowManifestV2 extends Omit { + schemaVersion: typeof WORKFLOW_MANIFEST_SCHEMA_VERSION; + runtimeVersion: typeof WORKFLOW_RUNTIME_VERSION; + status: WorkflowStatus; + scriptHash: string; + args?: JsonValue; + checkpointAt: number; + journal: JournalCheckpoint; + canResume: boolean; + resumeBlockedReason?: string; + /** + * Monotonic per-run counter. Every durable manifest write increments it, + * which makes it the compare-and-swap token: a writer commits N+1 only + * while disk still says N. Absent on manifests written before this + * existed, which read as generation 0. + */ + generation?: number; + owner?: WorkflowRunOwner; +} + +export interface WorkflowRunRecord extends Omit { + schemaVersion: number; + runtimeVersion: number; + status: WorkflowStatus | 'interrupted'; + originalStatus?: WorkflowStatus; + scriptHash?: string; + args?: JsonValue; + checkpointAt?: number; + journal?: JournalCheckpoint; + canResume: boolean; + resumeBlockedReason?: string; +} + +/** + * Who is currently allowed to advance a durable run. + * + * This is deliberately *not* a lock. Nothing here has to be released, and a + * process that dies holding it blocks nobody: the next process reads the + * manifest, sees whatever generation is on disk, and takes over from there. + * What the record buys is (a) a name to put in the error when a second + * writer is refused, and (b) a way for a writer to notice it has been + * superseded even across a manifest that was copied or restored, where inode + * identity no longer means anything. + */ +export interface WorkflowRunOwner { + /** Best-effort machine name. Diagnostic only — never a trust decision. */ + machineId: string; + pid: number; + /** Distinguishes two runs that share a machine and a recycled pid. */ + nonce: string; + acquiredAt: number; +} + +/** + * Thrown when a durable write loses the compare-and-swap: something else has + * advanced this run since we last committed. Fails the run closed rather + * than overwriting, because the two writers' journals have already diverged + * and there is no safe merge. + */ +export class WorkflowRunOwnershipError extends Error { + constructor( + readonly runId: string, + message: string, + ) { + super(message); + this.name = 'WorkflowRunOwnershipError'; + } +} + +/** The ownership claim a writer presents with each durable manifest write. */ +export interface WorkflowManifestOwnership { + owner: WorkflowRunOwner; + /** + * The generation this writer believes is on disk. The write commits as + * `expectedGeneration + 1` and is refused if disk says otherwise. + */ + expectedGeneration: number; +} + +export interface WriteWorkflowManifestOptions { + args: unknown; + journal: JournalCheckpoint; + checkpointAt?: number; + status?: WorkflowStatus; + ownership?: WorkflowManifestOwnership; +} + /** Project a (terminal) registry entry into a serializable snapshot. */ export function toSnapshot(task: WorkflowTask): WorkflowSnapshot { if (!isTerminalWorkflowStatus(task.status)) { throw new Error(`Cannot snapshot active workflow ${task.runId}.`); } + return { + ...projectTask(task), + status: task.status, + }; +} + +/** + * A non-JSON-serializable result is replaced with a placeholder string. + * + * Returns the *parsed projection* of the serialization, not the original + * object. Validating by serializing and then handing the live object on + * proves nothing: a getter or a `toJSON()` runs again when the manifest + * and the snapshot are written, and it is free to throw the second time + * (turning a finished workflow into a persistence failure) or to return + * something else (persisting output the run never produced). + * + * Exported so the runner can compute the projection ONCE at settlement and + * hand the same frozen value to every writer — if each writer re-projected + * the live result independently, getters/`toJSON()` would run once per + * write, which is exactly the failure mode above. + */ +export function projectWorkflowResult(result: unknown): unknown { + if (result === undefined) return undefined; + let serialized: string | undefined; + let byteLength = 0; + let isRoot = true; + const populatedParents = new WeakSet(); + const jsonWithRawSupport = JSON as typeof JSON & { + isRawJSON?: (value: unknown) => boolean; + }; + const addBytes = (count: number) => { + byteLength += count; + if (byteLength > MAX_WORKFLOW_ARTIFACT_BYTES) { + throw WORKFLOW_RESULT_TOO_LARGE; + } + }; + const addJsonStringBytes = (value: string) => { + addBytes(2); + for (let index = 0; index < value.length; index++) { + const code = value.charCodeAt(index); + if (code === 0x22 || code === 0x5c) { + addBytes(2); + } else if ( + code === 0x08 || + code === 0x09 || + code === 0x0a || + code === 0x0c || + code === 0x0d + ) { + addBytes(2); + } else if (code < 0x20) { + addBytes(6); + } else if (code < 0x80) { + addBytes(1); + } else if (code < 0x800) { + addBytes(2); + } else if (code >= 0xd800 && code <= 0xdbff) { + const next = value.charCodeAt(index + 1); + if (next >= 0xdc00 && next <= 0xdfff) { + addBytes(4); + index += 1; + } else { + addBytes(6); + } + } else if (code >= 0xdc00 && code <= 0xdfff) { + addBytes(6); + } else { + addBytes(3); + } + } + }; + try { + serialized = JSON.stringify(result, function (key, value: unknown) { + const root = isRoot; + isRoot = false; + const omitted = + value === undefined || + typeof value === 'function' || + typeof value === 'symbol'; + if (omitted && !root && !Array.isArray(this)) return value; + + if (!root) { + if (populatedParents.has(this)) addBytes(1); + else populatedParents.add(this); + if (!Array.isArray(this)) { + addJsonStringBytes(key); + addBytes(1); + } + } + + if (omitted) { + if (!root) addBytes(4); + } else if (typeof value === 'string') { + addJsonStringBytes(value); + } else if (typeof value === 'number') { + addBytes(Number.isFinite(value) ? String(value).length : 4); + } else if (typeof value === 'boolean') { + addBytes(value ? 4 : 5); + } else if (value === null) { + addBytes(4); + } else if (typeof value === 'object') { + if (jsonWithRawSupport.isRawJSON?.(value)) { + addBytes( + Buffer.byteLength((value as { rawJSON: string }).rawJSON, 'utf8'), + ); + } else { + addBytes(2); + } + } + return value; + }); + } catch (error) { + if (error === WORKFLOW_RESULT_TOO_LARGE) return omittedWorkflowResult(); + return `(non-JSON-serializable ${typeof result})`; + } + // A function or a symbol serializes to `undefined` without throwing; + // the enclosing stringify would have dropped the key, so drop it here. + if (serialized === undefined) return undefined; + return JSON.parse(serialized); +} + +function projectTask(task: WorkflowTask): Omit { return { runId: task.runId, meta: task.meta, - status: task.status, script: task.script ?? '', scriptPath: task.scriptPath, phases: [...task.phases], @@ -69,20 +356,558 @@ export function toSnapshot(task: WorkflowTask): WorkflowSnapshot { recentLogs: [...task.recentLogs], startTime: task.startTime, endTime: task.endTime, - result: safeResult(task.result), + result: projectWorkflowResult(task.result), error: task.error, }; } -/** A non-JSON-serializable result is replaced with a placeholder string. */ -function safeResult(result: unknown): unknown { - if (result === undefined) return undefined; +function hashScript(script: string): string { + return createHash('sha256').update(script).digest('hex'); +} + +function cloneJsonArgs(args: unknown): { + value?: JsonValue; + reason?: string; +} { try { - JSON.stringify(result); - return result; + const normalized = args === undefined ? null : args; + const seen = new WeakSet(); + const isJson = (value: unknown): boolean => { + if ( + value === null || + typeof value === 'string' || + typeof value === 'boolean' + ) { + return true; + } + // -0 is finite and stringifies to "0", so it passes as strict JSON + // while silently becoming +0 in the manifest. A run that branches on + // `Object.is(args.value, -0)` would then take a different path after + // resuming from a checkpoint marked resumable. + if (typeof value === 'number') { + return Number.isFinite(value) && !Object.is(value, -0); + } + if (typeof value !== 'object' || seen.has(value)) return false; + const proto = Object.getPrototypeOf(value); + if ( + !Array.isArray(value) && + proto !== Object.prototype && + proto !== null + ) { + return false; + } + const ownKeys = Reflect.ownKeys(value); + if (Array.isArray(value)) { + if ( + ownKeys.length !== value.length + 1 || + !ownKeys.includes('length') + ) { + return false; + } + for (let index = 0; index < value.length; index++) { + if (!Object.hasOwn(value, index)) return false; + } + } else if (ownKeys.some((key) => typeof key !== 'string')) { + return false; + } + seen.add(value); + const descriptors = Object.getOwnPropertyDescriptors(value); + const valid = Object.entries(descriptors).every( + ([key, descriptor]) => + (Array.isArray(value) && key === 'length') || + (descriptor.enumerable && + 'value' in descriptor && + isJson(descriptor.value)), + ); + seen.delete(value); + return valid; + }; + if (!isJson(normalized)) { + return { reason: 'Workflow args are not strictly JSON-serializable.' }; + } + const serialized = JSON.stringify(normalized); + if (Buffer.byteLength(serialized, 'utf8') > MAX_WORKFLOW_ARGS_BYTES) { + return { + reason: `Workflow args exceed ${MAX_WORKFLOW_ARGS_BYTES} UTF-8 bytes.`, + }; + } + return { value: JSON.parse(serialized) as JsonValue }; } catch { - return `(non-JSON-serializable ${typeof result})`; + return { reason: 'Workflow args are not strictly JSON-serializable.' }; + } +} + +function assertRunId(runId: string): void { + if (!/^wf_[0-9a-f]+$/.test(runId)) { + throw new Error(`Invalid workflow run id: ${runId}`); + } +} + +function isLegacyRunId(runId: string): boolean { + return /^wf_[A-Za-z0-9_-]{1,128}$/.test(runId); +} + +/** + * True when `readdir` did not report the entry's type at all (DT_UNKNOWN): + * sshfs/s3fs/rclone without readdirplus, and some NFSv3 exports. Node models + * such a dirent by answering `false` to every predicate and exposes no + * `isUnknown()`, so "unknown" can only be read as the absence of every other + * answer — a device/FIFO/socket is a reported type and is not unknown. + * + * The listing loops use this to distinguish "the filesystem says this is the + * wrong kind of entry" from "the filesystem declined to say". Only the former + * is a real answer; the latter has to be resolved by the lstat that + * `readRegularFileNoFollow` and `resolveSafeRunDir` already perform, which is + * where the type is actually enforced. + */ +function isUnknownDirent(entry: Dirent): boolean { + return ( + !entry.isFile() && + !entry.isDirectory() && + !entry.isSymbolicLink() && + !entry.isBlockDevice() && + !entry.isCharacterDevice() && + !entry.isFIFO() && + !entry.isSocket() + ); +} + +function assertLegacyRunId(runId: string): void { + if (!isLegacyRunId(runId)) { + throw new Error(`Invalid legacy workflow run id: ${runId}`); + } +} + +function assertSafeArtifactFile(stat: Stats, filePath: string): void { + if (stat.isSymbolicLink()) { + throw new Error(`Unsafe workflow artifact symlink: ${filePath}`); + } + if (!stat.isFile() || stat.nlink !== 1) { + throw new Error(`Unsafe workflow artifact path: ${filePath}`); + } +} + +async function readRegularFileNoFollow( + filePath: string, + assertParentUnchanged?: () => void, +): Promise { + const before = await fs.lstat(filePath); + assertSafeArtifactFile(before, filePath); + let handle; + try { + handle = await fs.open( + filePath, + fsConstants.O_RDONLY | + (process.platform === 'win32' + ? 0 + : fsConstants.O_NOFOLLOW | fsConstants.O_NONBLOCK), + ); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ELOOP') { + throw new Error(`Unsafe workflow artifact symlink: ${filePath}`); + } + throw error; + } + try { + assertParentUnchanged?.(); + const [opened, current] = await Promise.all([ + handle.stat(), + fs.lstat(filePath), + ]); + assertSafeArtifactFile(opened, filePath); + assertSafeArtifactFile(current, filePath); + if ( + before.dev !== current.dev || + before.ino !== current.ino || + opened.dev !== current.dev || + opened.ino !== current.ino + ) { + throw new Error(`Workflow artifact path changed: ${filePath}`); + } + // The size is already in hand from the identity stat; check it before + // buffering so an oversized artifact degrades to a skipped/invalid record + // instead of aborting the process on an uncatchable heap OOM. It is only + // a cheap pre-rejection though — the size and the bytes are separate + // observations of an inode its owner may still be growing, so the cap + // that actually holds is the one the read enforces. + if (opened.size > MAX_WORKFLOW_ARTIFACT_BYTES) { + throw new Error( + `Workflow artifact is too large: ${filePath} (${opened.size} bytes, limit ${MAX_WORKFLOW_ARTIFACT_BYTES})`, + ); + } + const bytes = await readBounded(handle, MAX_WORKFLOW_ARTIFACT_BYTES); + if (bytes === null) { + throw new Error( + `Workflow artifact is too large: ${filePath} (over the ${MAX_WORKFLOW_ARTIFACT_BYTES} byte limit)`, + ); + } + return bytes.toString('utf8'); + } finally { + await handle.close(); + } +} + +interface DirectoryGuard { + assertUnchanged(): void; +} + +class TerminalSnapshotConflictError extends Error {} + +async function guardArtifactTarget( + filePath: string, + assertParentUnchanged: () => void, +): Promise { + let expected: Stats | undefined; + try { + expected = await fs.lstat(filePath); + assertSafeArtifactFile(expected, filePath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + return { + assertUnchanged(): void { + assertParentUnchanged(); + let current: Stats | undefined; + try { + current = lstatSync(filePath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + if (!expected && !current) return; + if (!expected || !current) { + throw new Error(`Workflow artifact path changed: ${filePath}`); + } + assertSafeArtifactFile(current, filePath); + if (expected.dev !== current.dev || expected.ino !== current.ino) { + throw new Error(`Workflow artifact path changed: ${filePath}`); + } + }, + }; +} + +async function resolveSafeRunsRoot( + storage: Storage, + create: boolean, +): Promise { + const root = storage.getWorkflowRunsDir(); + if (create) await fs.mkdir(root, { recursive: true, mode: 0o700 }); + const stat = await fs.lstat(root); + if (!stat.isDirectory() || stat.isSymbolicLink()) { + throw new Error(`Unsafe workflow runs directory: ${root}`); + } + const identity = { + dev: stat.dev, + ino: stat.ino, + realPath: await fs.realpath(root), + }; + return { + assertUnchanged(): void { + const current = lstatSync(root); + if ( + !current.isDirectory() || + current.isSymbolicLink() || + current.dev !== identity.dev || + current.ino !== identity.ino || + realpathSync(root) !== identity.realPath + ) { + throw new Error(`Workflow runs directory changed: ${root}`); + } + }, + }; +} + +async function resolveSafeRunDir( + storage: Storage, + runId: string, + create: boolean, +): Promise { + assertRunId(runId); + const root = storage.getWorkflowRunsDir(); + const rootGuard = await resolveSafeRunsRoot(storage, create); + const runDir = storage.getWorkflowRunDir(runId); + if (create) { + await fs.mkdir(runDir, { mode: 0o700 }).catch((error: unknown) => { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error; + }); + } + const runStat = await fs.lstat(runDir); + if (!runStat.isDirectory() || runStat.isSymbolicLink()) { + throw new Error(`Unsafe workflow run directory: ${runDir}`); + } + const [realRoot, realRunDir] = await Promise.all([ + fs.realpath(root), + fs.realpath(runDir), + ]); + if (path.dirname(realRunDir) !== realRoot) { + throw new Error(`Workflow run directory escapes storage root: ${runDir}`); + } + const identity = { + dev: runStat.dev, + ino: runStat.ino, + realPath: realRunDir, + }; + return { + assertUnchanged(): void { + rootGuard.assertUnchanged(); + const current = lstatSync(runDir); + if ( + !current.isDirectory() || + current.isSymbolicLink() || + current.dev !== identity.dev || + current.ino !== identity.ino || + realpathSync(runDir) !== identity.realPath + ) { + throw new Error(`Workflow run directory changed: ${runDir}`); + } + }, + }; +} + +export async function writeWorkflowManifest( + config: Config, + task: WorkflowTask, + options: WriteWorkflowManifestOptions, +): Promise { + const storage = config.storage; + if (!storage) return undefined; + const status = options.status ?? task.status; + const args = cloneJsonArgs(options.args); + const journal = { ...options.journal }; + const journalReason = + journal.integrity === 'complete' + ? undefined + : (journal.error ?? 'Workflow journal persistence failed.'); + const statusReason = isTerminalWorkflowStatus(status) + ? `Workflow run is ${status}; terminal runs must be rerun instead of resumed.` + : undefined; + const resumeBlockedReason = statusReason ?? args.reason ?? journalReason; + // Freeze the task-derived half of the manifest BEFORE the first await. + // The caller hands us the live registry entry, in-flight dispatches keep + // mutating it across every fs await below, and `projectTask` copies the + // entry shallowly — `meta` stays the same object — so only a + // serialize/parse round trip actually detaches it. Everything the CAS adds + // (`generation`, `owner`) comes from disk or from this process, never from + // the entry, so it is safe to merge in afterwards. + const frozenBody = JSON.parse( + JSON.stringify({ + ...projectTask(task), + status, + scriptHash: hashScript(task.script ?? ''), + ...(args.value === undefined ? {} : { args: args.value }), + checkpointAt: options.checkpointAt ?? Date.now(), + journal, + canResume: resumeBlockedReason === undefined, + ...(resumeBlockedReason ? { resumeBlockedReason } : {}), + }), + ) as Omit; + const guard = await resolveSafeRunDir(storage, task.runId, true); + const manifestPath = storage.getWorkflowRunManifestPath(task.runId); + + // The compare-and-swap. Pin the target inode BEFORE reading its content, + // then refuse if disk no longer shows the generation this writer owns. + // Pin-then-read is a real CAS rather than a check-then-hope: the content + // read verifies its own identity against the pin, and a competing writer + // commits by renaming a new inode over this path, so its win invalidates + // the pin and `atomicWriteFile` refuses our rename. Reading first and + // pinning after left a window where a competing commit was caught by + // neither check — the generation check saw pre-commit content, and the + // pin captured the competitor's inode, so both writers committed. The + // residual window is the check/rename syscall pair, which is the same + // narrowing every other guarded write site here accepts — Node exposes no + // renameat2(RENAME_EXCHANGE) to close it. + const targetGuard = await guardArtifactTarget( + manifestPath, + guard.assertUnchanged, + ); + const ownership = options.ownership; + let generation = ownership ? ownership.expectedGeneration + 1 : undefined; + if (ownership) { + const current = await readManifestForOwnership( + manifestPath, + targetGuard.assertUnchanged, + ); + if (current) { + const currentGeneration = current.generation ?? 0; + if (currentGeneration !== ownership.expectedGeneration) { + throw new WorkflowRunOwnershipError( + task.runId, + `Workflow run ${task.runId} advanced to generation ${currentGeneration} ` + + `while this process held generation ${ownership.expectedGeneration}; ` + + `another writer${describeOwner(current.owner)} owns it.`, + ); + } + // Same generation but a different claim means a takeover committed and + // was itself rolled back to our number, which we cannot distinguish + // from divergence. Refuse rather than guess. + if (current.owner && current.owner.nonce !== ownership.owner.nonce) { + throw new WorkflowRunOwnershipError( + task.runId, + `Workflow run ${task.runId} is owned by another writer` + + `${describeOwner(current.owner)}.`, + ); + } + generation = currentGeneration + 1; + } + } + + const manifest: WorkflowManifestV2 = { + schemaVersion: WORKFLOW_MANIFEST_SCHEMA_VERSION, + runtimeVersion: WORKFLOW_RUNTIME_VERSION, + ...frozenBody, + ...(generation === undefined ? {} : { generation }), + ...(ownership ? { owner: ownership.owner } : {}), + }; + const serialized = serializeWorkflowArtifact(manifest); + const frozenManifest = JSON.parse(serialized) as WorkflowManifestV2; + await atomicWriteFile(manifestPath, serialized, { + noFollow: true, + mode: 0o600, + forceMode: true, + assertCanCommit: targetGuard.assertUnchanged, + }); + return frozenManifest; +} + +function describeOwner(owner: WorkflowRunOwner | undefined): string { + return owner ? ` (${owner.machineId} pid ${owner.pid})` : ''; +} + +/** + * Claim the right to advance a durable run, before any of its work starts. + * + * Two processes resuming the same run both read generation N and both try to + * commit N+1; exactly one rename lands, and the loser gets a + * {@link WorkflowRunOwnershipError} *before* it has dispatched anything. That + * ordering is the whole point — it is what keeps the duplicate-execution + * window free of model calls and tool side effects. + * + * Taking over from a previous owner is allowed and needs no liveness probe. + * A process that died holding the run left a generation number behind, not a + * lock, so recovery is "read it and carry on" rather than a stale-lock policy + * that has to guess whether the previous holder is coming back. + */ +export async function acquireWorkflowRunOwnership( + config: Config, + runId: string, + owner: WorkflowRunOwner, +): Promise { + const storage = config.storage; + if (!storage) throw new Error('Workflow storage is unavailable.'); + const guard = await resolveSafeRunDir(storage, runId, false); + const manifestPath = storage.getWorkflowRunManifestPath(runId); + // Pin BEFORE reading, exactly like the settlement CAS in + // `writeWorkflowManifest`: a competing commit landing between the read + // and the pin would otherwise be caught by neither check. + const targetGuard = await guardArtifactTarget( + manifestPath, + guard.assertUnchanged, + ); + const raw = await readRegularFileNoFollow( + manifestPath, + targetGuard.assertUnchanged, + ); + const parsed = JSON.parse(raw) as Record; + // Same read rule as `readManifestForOwnership`: a negative or + // non-integer generation is not a claim, not an error — it reads as the + // generation-0 state every pre-generation manifest is in. + const rawGeneration = parsed['generation']; + const currentGeneration = + Number.isSafeInteger(rawGeneration) && (rawGeneration as number) >= 0 + ? (rawGeneration as number) + : 0; + const generation = currentGeneration + 1; + const serialized = JSON.stringify({ ...parsed, generation, owner }, null, 2); + try { + await atomicWriteFile(manifestPath, serialized, { + noFollow: true, + mode: 0o600, + forceMode: true, + assertCanCommit: targetGuard.assertUnchanged, + }); + } catch (error) { + // The pin failing here means somebody else committed between our read + // and our rename — i.e. we lost the acquisition race. Report it as such + // rather than as a generic write failure, because the caller's correct + // response is to abandon the resume, not to retry. + throw new WorkflowRunOwnershipError( + runId, + `Workflow run ${runId} was claimed by another writer while this ` + + `process was acquiring it: ${ + error instanceof Error ? error.message : String(error) + }`, + ); } + return generation; +} + +/** A fresh ownership claim for this process. */ +export function createWorkflowRunOwner(): WorkflowRunOwner { + return { + machineId: hostname(), + pid: process.pid, + nonce: randomBytes(8).toString('hex'), + acquiredAt: Date.now(), + }; +} + +/** + * Read just the ownership fields off the manifest, tolerating a manifest + * that is absent or unreadable. + * + * Deliberately not `parseManifest`: this runs on the write path, and a + * manifest whose *content* is unusable must not stop a writer that owns the + * run from replacing it. Only the ownership fields are consulted, and only + * when they are well-formed — anything else reads as "no claim on record", + * which is the same state a pre-generation manifest is in. + */ +async function readManifestForOwnership( + manifestPath: string, + assertParentUnchanged: () => void, +): Promise<{ generation?: number; owner?: WorkflowRunOwner } | undefined> { + let raw: string; + try { + raw = await readRegularFileNoFollow(manifestPath, assertParentUnchanged); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; + throw error; + } + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return undefined; + } + // A corrupt or non-object manifest is no claim at all. Collapsing it into + // `{}` used to read as a live generation-0 claim and kill the owning + // writer with a phantom-writer ownership error instead of letting it + // replace the unreadable file. + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { + return undefined; + } + const record = parsed as Record; + const generation = record['generation']; + const owner = record['owner'] as Record | undefined; + return { + ...(Number.isSafeInteger(generation) && (generation as number) >= 0 + ? { generation: generation as number } + : {}), + ...(owner && + typeof owner === 'object' && + typeof owner['machineId'] === 'string' && + typeof owner['nonce'] === 'string' && + Number.isSafeInteger(owner['pid']) + ? { + owner: { + machineId: owner['machineId'] as string, + pid: owner['pid'] as number, + nonce: owner['nonce'] as string, + acquiredAt: Number.isSafeInteger(owner['acquiredAt']) + ? (owner['acquiredAt'] as number) + : 0, + }, + } + : {}), + }; } /** @@ -98,19 +923,27 @@ export async function writeWorkflowSnapshot( const storage = config.storage; if (!storage) return; try { + assertLegacyRunId(task.runId); // Project BEFORE the first await: the caller captures this at // settlement, but in-flight dispatches keep mutating the live // entry across the fs awaits below — a post-await projection // froze the snapshot at an fs-timing-dependent point mid-drain. const snapshot = toSnapshot(task); + const serialized = serializeWorkflowArtifact(snapshot); const dir = storage.getWorkflowRunsDir(); - await fs.mkdir(dir, { recursive: true }); - await fs.writeFile( - storage.getWorkflowRunSnapshotPath(task.runId), - JSON.stringify(snapshot, null, 2), - 'utf8', + const guard = await resolveSafeRunsRoot(storage, true); + const snapshotPath = storage.getWorkflowRunSnapshotPath(task.runId); + const targetGuard = await guardArtifactTarget( + snapshotPath, + guard.assertUnchanged, ); - await pruneSnapshots(dir); + await atomicWriteFile(snapshotPath, serialized, { + noFollow: true, + mode: 0o600, + forceMode: true, + assertCanCommit: targetGuard.assertUnchanged, + }); + await pruneSnapshots(dir, guard.assertUnchanged); } catch (e) { debugLogger.warn(`writeWorkflowSnapshot failed for ${task.runId}: ${e}`); } @@ -127,7 +960,9 @@ export async function listWorkflowSnapshots( if (!storage) return []; const dir = storage.getWorkflowRunsDir(); let files: string[]; + let guard: DirectoryGuard; try { + guard = await resolveSafeRunsRoot(storage, false); files = (await fs.readdir(dir)).filter((f) => f.endsWith('.json')); } catch { return []; @@ -135,7 +970,10 @@ export async function listWorkflowSnapshots( const snapshots: WorkflowSnapshot[] = []; for (const file of files) { try { - const raw = await fs.readFile(`${dir}/${file}`, 'utf8'); + const raw = await readRegularFileNoFollow( + `${dir}/${file}`, + guard.assertUnchanged, + ); snapshots.push(JSON.parse(raw) as WorkflowSnapshot); } catch (e) { debugLogger.warn(`skipping unparseable snapshot ${file}: ${e}`); @@ -145,10 +983,372 @@ export async function listWorkflowSnapshots( return snapshots; } +function isWorkflowMeta(value: unknown): value is WorkflowMeta | null { + if (value === null) return true; + if (!value || typeof value !== 'object' || Array.isArray(value)) return false; + const meta = value as Record; + if ( + typeof meta['name'] !== 'string' || + typeof meta['description'] !== 'string' || + (meta['whenToUse'] !== undefined && typeof meta['whenToUse'] !== 'string') + ) { + return false; + } + return ( + meta['phases'] === undefined || + (Array.isArray(meta['phases']) && + meta['phases'].every((phase) => { + if (!phase || typeof phase !== 'object' || Array.isArray(phase)) { + return false; + } + const record = phase as Record; + return ( + typeof record['title'] === 'string' && + (record['detail'] === undefined || + typeof record['detail'] === 'string') && + (record['model'] === undefined || typeof record['model'] === 'string') + ); + })) + ); +} + +function hasValidPersistedSummary(record: Record): boolean { + const nonNegative = (value: unknown): value is number => + typeof value === 'number' && Number.isFinite(value) && value >= 0; + return ( + isWorkflowMeta(record['meta']) && + typeof record['script'] === 'string' && + (record['scriptPath'] === undefined || + typeof record['scriptPath'] === 'string') && + Array.isArray(record['phases']) && + record['phases'].every((phase) => typeof phase === 'string') && + Number.isSafeInteger(record['agentsDispatched']) && + nonNegative(record['agentsDispatched']) && + Number.isSafeInteger(record['agentsCompleted']) && + nonNegative(record['agentsCompleted']) && + nonNegative(record['tokensSpent']) && + (record['tokenBudgetTotal'] === null || + nonNegative(record['tokenBudgetTotal'])) && + Array.isArray(record['perPhaseTokens']) && + record['perPhaseTokens'].every( + (entry) => + Array.isArray(entry) && + entry.length === 2 && + (entry[0] === null || typeof entry[0] === 'string') && + nonNegative(entry[1]), + ) && + Array.isArray(record['recentLogs']) && + record['recentLogs'].every((line) => typeof line === 'string') && + nonNegative(record['startTime']) && + (record['endTime'] === undefined || nonNegative(record['endTime'])) && + (record['error'] === undefined || typeof record['error'] === 'string') + ); +} + +function parseLegacySnapshot( + value: unknown, + expectedRunId: string, +): WorkflowSnapshot { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + throw new Error('Legacy workflow snapshot must be a JSON object.'); + } + const snapshot = value as Record; + if ( + snapshot['runId'] !== expectedRunId || + !isTerminalWorkflowStatus(snapshot['status'] as WorkflowStatus) || + !hasValidPersistedSummary(snapshot) + ) { + throw new Error('Legacy workflow snapshot has an invalid shape.'); + } + return snapshot as unknown as WorkflowSnapshot; +} + +function parseManifest( + value: unknown, + expectedRunId: string, +): WorkflowManifestV2 { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + throw new Error('Workflow manifest must be a JSON object.'); + } + const manifest = value as Record; + if ( + manifest['schemaVersion'] !== WORKFLOW_MANIFEST_SCHEMA_VERSION || + manifest['runtimeVersion'] !== WORKFLOW_RUNTIME_VERSION + ) { + throw new Error('Unsupported workflow manifest version.'); + } + if (manifest['runId'] !== expectedRunId) { + throw new Error('Workflow manifest run id does not match its directory.'); + } + const status = manifest['status']; + // `typeof` first: `String(['paused'])` coerces a single-element array to + // 'paused', which would pass the whitelist and escape as the manifest's + // status — where `entry.status.padEnd` in the listing then aborts the + // entire `/workflows` render for everyone who clones the repo. + if ( + typeof status !== 'string' || + ![ + 'running', + 'pausing', + 'paused', + 'completed', + 'failed', + 'cancelled', + ].includes(status) + ) { + throw new Error('Workflow manifest has an invalid status.'); + } + if ( + typeof manifest['script'] !== 'string' || + manifest['scriptHash'] !== hashScript(manifest['script']) + ) { + throw new Error('Workflow manifest script hash mismatch.'); + } + const journal = manifest['journal'] as Record | undefined; + if ( + !journal || + journal['version'] !== JOURNAL_FORMAT_VERSION || + journal['keyVersion'] !== JOURNAL_KEY_VERSION || + !Number.isSafeInteger(journal['byteLength']) || + (journal['byteLength'] as number) < 0 || + typeof journal['sha256'] !== 'string' || + !/^[0-9a-f]{64}$/.test(journal['sha256']) || + typeof journal['integrity'] !== 'string' || + !['complete', 'failed'].includes(journal['integrity']) + ) { + throw new Error('Workflow manifest has invalid journal metadata.'); + } + if ( + typeof manifest['canResume'] !== 'boolean' || + typeof manifest['checkpointAt'] !== 'number' || + !Number.isFinite(manifest['checkpointAt']) || + !hasValidPersistedSummary(manifest) + ) { + throw new Error('Workflow manifest has invalid checkpoint metadata.'); + } + if ( + manifest['canResume'] && + (journal['integrity'] !== 'complete' || + !Object.hasOwn(manifest, 'args') || + isTerminalWorkflowStatus(status as WorkflowStatus) || + manifest['resumeBlockedReason'] !== undefined) + ) { + throw new Error('Workflow manifest resume metadata is incomplete.'); + } + if ( + !manifest['canResume'] && + typeof manifest['resumeBlockedReason'] !== 'string' + ) { + throw new Error('Workflow manifest lacks a resume-blocked reason.'); + } + if ( + manifest['resumeBlockedReason'] !== undefined && + typeof manifest['resumeBlockedReason'] !== 'string' + ) { + throw new Error('Workflow manifest has an invalid resume-blocked reason.'); + } + if ( + Object.hasOwn(manifest, 'args') && + cloneJsonArgs(manifest['args']).reason + ) { + throw new Error('Workflow manifest contains invalid args.'); + } + return manifest as unknown as WorkflowManifestV2; +} + +export async function readWorkflowManifest( + config: Config, + runId: string, +): Promise { + const storage = config.storage; + if (!storage) throw new Error('Workflow storage is unavailable.'); + const guard = await resolveSafeRunDir(storage, runId, false); + const manifestPath = storage.getWorkflowRunManifestPath(runId); + const manifest = parseManifest( + JSON.parse( + await readRegularFileNoFollow(manifestPath, guard.assertUnchanged), + ) as unknown, + runId, + ); + const snapshotPath = storage.getWorkflowRunSnapshotPath(runId); + try { + const snapshot = parseLegacySnapshot( + JSON.parse( + await readRegularFileNoFollow(snapshotPath, guard.assertUnchanged), + ) as unknown, + runId, + ); + if ( + isActiveWorkflowStatus(manifest.status) || + snapshot.status !== manifest.status + ) { + throw new TerminalSnapshotConflictError( + `Workflow run ${runId} has a terminal snapshot that conflicts with its manifest.`, + ); + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; + } + if (manifest.canResume) { + await new WorkflowJournal(storage.getWorkflowRunJournalPath(runId)).load( + manifest.journal, + ); + } + return manifest; +} + +function invalidRecord( + runId: string, + startTime: number, + reason: string, +): WorkflowRunRecord { + return { + runId, + meta: null, + status: 'interrupted', + script: '', + phases: [], + agentsDispatched: 0, + agentsCompleted: 0, + tokensSpent: 0, + tokenBudgetTotal: null, + perPhaseTokens: [], + recentLogs: [], + startTime, + schemaVersion: 0, + runtimeVersion: 0, + canResume: false, + resumeBlockedReason: reason, + }; +} + +/** Version-aware, read-only workflow history for `/workflows`. */ +export async function listWorkflowRunRecords( + config: Config, +): Promise { + const storage = config.storage; + if (!storage) return []; + const byRunId = new Map(); + let entries: Dirent[]; + let rootGuard: DirectoryGuard; + try { + rootGuard = await resolveSafeRunsRoot(storage, false); + entries = await fs.readdir(storage.getWorkflowRunsDir(), { + withFileTypes: true, + }); + } catch { + return []; + } + for (const entry of entries) { + if (!entry.name.endsWith('.json')) continue; + const runId = entry.name.slice(0, -'.json'.length); + if (!isLegacyRunId(runId)) continue; + const snapshotPath = path.join(storage.getWorkflowRunsDir(), entry.name); + try { + // An unreported type is not a verdict: `readRegularFileNoFollow` below + // lstats the path and rejects anything that is not a single-linked + // regular file, so deferring to it keeps valid snapshots readable on + // filesystems that omit d_type instead of branding them unsafe. + if (!entry.isFile() && !isUnknownDirent(entry)) { + throw new Error(`Unsafe legacy workflow snapshot: ${snapshotPath}`); + } + const snapshot = parseLegacySnapshot( + JSON.parse( + await readRegularFileNoFollow( + snapshotPath, + rootGuard.assertUnchanged, + ), + ) as unknown, + runId, + ); + byRunId.set(runId, { + ...snapshot, + schemaVersion: 1, + runtimeVersion: 0, + canResume: false, + resumeBlockedReason: 'Legacy workflow snapshots are display-only.', + }); + } catch (error) { + const stat = await fs.lstat(snapshotPath).catch(() => undefined); + byRunId.set( + runId, + invalidRecord( + runId, + stat?.mtimeMs ?? 0, + error instanceof Error ? error.message : String(error), + ), + ); + } + } + for (const entry of entries) { + if (!/^wf_[0-9a-f]+$/.test(entry.name)) continue; + // Symlinks are deliberately let through so `resolveSafeRunDir` rejects + // them into a visible invalid record rather than a silent skip; a + // DT_UNKNOWN dirent gets the same treatment for the opposite reason — + // skipping it here would drop a valid run directory, taking its durable + // history and its resume with it. + if ( + !entry.isDirectory() && + !entry.isSymbolicLink() && + !isUnknownDirent(entry) + ) { + continue; + } + try { + const manifest = await readWorkflowManifest(config, entry.name); + const legacy = byRunId.get(entry.name); + if ( + legacy && + isTerminalWorkflowStatus(legacy.status as WorkflowStatus) && + isActiveWorkflowStatus(manifest.status) + ) { + continue; + } + byRunId.set(entry.name, { + ...manifest, + ...(isActiveWorkflowStatus(manifest.status) + ? { status: 'interrupted', originalStatus: manifest.status } + : { status: manifest.status }), + }); + } catch (error) { + const legacy = byRunId.get(entry.name); + if ( + legacy && + isTerminalWorkflowStatus(legacy.status as WorkflowStatus) && + error instanceof TerminalSnapshotConflictError + ) { + continue; + } + if ( + (error as NodeJS.ErrnoException).code === 'ENOENT' && + byRunId.has(entry.name) + ) { + continue; + } + const stat = await fs + .lstat(storage.getWorkflowRunDir(entry.name)) + .catch(() => undefined); + byRunId.set( + entry.name, + invalidRecord( + entry.name, + stat?.mtimeMs ?? 0, + error instanceof Error ? error.message : String(error), + ), + ); + } + } + return [...byRunId.values()].sort((a, b) => b.startTime - a.startTime); +} + /** Remove the oldest snapshots beyond the retention cap. */ -async function pruneSnapshots(dir: string): Promise { +async function pruneSnapshots( + dir: string, + assertRootUnchanged: () => void, +): Promise { let files: string[]; try { + assertRootUnchanged(); files = (await fs.readdir(dir)).filter((f) => f.endsWith('.json')); } catch { return; @@ -168,7 +1368,7 @@ async function pruneSnapshots(dir: string): Promise { stats.sort((a, b) => a.mtime - b.mtime); const toPrune = stats.slice(0, stats.length - MAX_RETAINED_SNAPSHOTS); await Promise.all( - toPrune.map((s) => { + toPrune.map(async (s) => { // Each run also has a sibling `/journal.jsonl` directory (the // resume journal). Removing only the `.json` snapshot would leave // those journal dirs to grow without bound, so prune both together. @@ -182,23 +1382,42 @@ async function pruneSnapshots(dir: string): Promise { // may drive `fs.rm`. The `.json` unlink stays unconditional — it removes // exactly that one file, never a directory. const isRunDir = /^wf_[0-9a-f]+$/.test(runId); + assertRootUnchanged(); + // The recursive delete must be authorized by the pruned file itself: + // only a well-formed terminal snapshot of the same run proves the + // same-stem directory is that run's residue. A malformed file — or one + // planted under a stem no generated runId can take — deletes only + // itself, never the directory. Validate BEFORE unlinking: the read and + // the delete must see the same file. + let representsRun = false; + if (isRunDir) { + try { + parseLegacySnapshot( + JSON.parse( + await readRegularFileNoFollow( + `${dir}/${s.f}`, + assertRootUnchanged, + ), + ) as unknown, + runId, + ); + representsRun = true; + } catch { + // Not a valid terminal snapshot of this run — the file alone is + // deleted below. + } + } + const unlink = fs + .unlink(`${dir}/${s.f}`) + .catch((e) => debugLogger.warn(`prune unlink failed for ${s.f}: ${e}`)); + if (!representsRun) return unlink; return Promise.all([ + unlink, fs - .unlink(`${dir}/${s.f}`) + .rm(`${dir}/${runId}`, { recursive: true, force: true }) .catch((e) => - debugLogger.warn(`prune unlink failed for ${s.f}: ${e}`), + debugLogger.warn(`prune journal dir failed for ${runId}: ${e}`), ), - ...(isRunDir - ? [ - fs - .rm(`${dir}/${runId}`, { recursive: true, force: true }) - .catch((e) => - debugLogger.warn( - `prune journal dir failed for ${runId}: ${e}`, - ), - ), - ] - : []), ]); }), ); diff --git a/packages/core/src/config/storage.ts b/packages/core/src/config/storage.ts index 08dba1e7916..28a6c6750b2 100644 --- a/packages/core/src/config/storage.ts +++ b/packages/core/src/config/storage.ts @@ -414,11 +414,25 @@ export class Storage { return path.join(this.getWorkflowRunsDir(), `${runId}.json`); } + /** + * Directory containing the durable artifacts for one workflow run. + */ + getWorkflowRunDir(runId: string): string { + return path.join(this.getWorkflowRunsDir(), runId); + } + + /** + * Path to the durable replay manifest for a workflow run. + */ + getWorkflowRunManifestPath(runId: string): string { + return path.join(this.getWorkflowRunDir(runId), 'manifest.json'); + } + /** * Path to the resume journal for an in-progress / resumable workflow run. */ getWorkflowRunJournalPath(runId: string): string { - return path.join(this.getWorkflowRunsDir(), runId, 'journal.jsonl'); + return path.join(this.getWorkflowRunDir(runId), 'journal.jsonl'); } /** diff --git a/packages/core/src/tools/workflow/workflow.test.ts b/packages/core/src/tools/workflow/workflow.test.ts index 9a5923fef9c..eac35a01cd4 100644 --- a/packages/core/src/tools/workflow/workflow.test.ts +++ b/packages/core/src/tools/workflow/workflow.test.ts @@ -13,6 +13,7 @@ import type { Config } from '../../config/config.js'; import { ToolNames, ToolDisplayNames } from '../tool-names.js'; import { WorkflowRunRegistry } from '../../agents/workflow-run-registry.js'; import { WorkflowJournal } from '../../agents/runtime/workflow-journal.js'; +import { writeWorkflowManifest } from '../../agents/workflow-snapshot.js'; import { DEFAULT_MAX_AGENTS_PER_RUN, MAX_WORKFLOW_AGENTS_ENV, @@ -251,11 +252,32 @@ describe('WorkflowTool', () => { it('does not register when cancellation arrives during background preflight', async () => { const registry = new WorkflowRunRegistry(); registry.setCompletionCallback(vi.fn()); + const projectDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'workflow-preflight-test-'), + ); + const storage = new Storage(projectDir, path.join(projectDir, 'runtime')); const config = { - storage: new Storage(path.join(os.tmpdir(), 'workflow-preflight-test')), + storage, isInteractive: () => true, getWorkflowRunRegistry: () => registry, } as unknown as Config; + const runId = 'wf_1234abcd'; + const seed = new WorkflowRunRegistry().register({ + runId, + meta: null, + status: 'running', + startTime: Date.now(), + outputFile: '', + abortController: new AbortController(), + script: 'return 1', + }); + const journal = new WorkflowJournal( + storage.getWorkflowRunJournalPath(runId), + ); + await writeWorkflowManifest(config, seed, { + args: undefined, + journal: await journal.flush(), + }); const caller = new AbortController(); const dispatch = vi.fn(async () => 'unused'); const load = vi @@ -269,7 +291,7 @@ describe('WorkflowTool', () => { const result = await new WorkflowTool(config, { dispatch }) .build({ script: 'return 1', - resumeFromRunId: 'wf_1234abcd', + resumeFromRunId: runId, run_in_background: true, }) .execute(caller.signal); @@ -282,6 +304,7 @@ describe('WorkflowTool', () => { expect(dispatch).not.toHaveBeenCalled(); } finally { load.mockRestore(); + await fs.rm(projectDir, { recursive: true, force: true }); } }); @@ -329,7 +352,7 @@ describe('WorkflowTool', () => { expect(updateOutput).not.toHaveBeenCalled(); }); - it('run_in_background=false preserves the foreground ToolResult byte-for-byte', async () => { + it('run_in_background=false preserves the foreground ToolResult shape', async () => { const run = async (runInBackground: false | undefined) => { const registry = new WorkflowRunRegistry(); const config = { @@ -338,7 +361,6 @@ describe('WorkflowTool', () => { } as unknown as Config; const params = { script: `phase('one'); return { answer: 42 };`, - resumeFromRunId: 'wf_1234abcd', ...(runInBackground === undefined ? {} : { run_in_background: runInBackground }), @@ -348,7 +370,14 @@ describe('WorkflowTool', () => { .execute(new AbortController().signal); }; - await expect(run(false)).resolves.toEqual(await run(undefined)); + const explicit = await run(false); + const omitted = await run(undefined); + expect(explicit.llmContent).toEqual(omitted.llmContent); + const normalizeRunId = (value: unknown) => + String(JSON.stringify(value)).replace(/wf_[0-9a-f]+/g, 'wf_RUN_ID'); + expect(normalizeRunId(explicit.returnDisplay)).toBe( + normalizeRunId(omitted.returnDisplay), + ); }); it('execute() loads a saved-workflow scriptPath and records its provenance', async () => {