From 472f70855801120b323a3234a12f0d103de62faa Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Sat, 7 Nov 2026 18:19:29 +0800 Subject: [PATCH] fix(workflow): a refused store write leaves no ghost workflow (#2598) create() pushed the new graph and its creation line into state.workflows and state.logs before write(), and write() refuses before its first byte when the workflow store's temp leaf is not a plain file of its own, so the caller got a refusal while the graph sat in memory for the next successful write to persist. create() now remembers the workflow count, the log line and seq and undoes them in the catch. rename() had the same write-after-mutate shape with no catch at all: it now returns the refusal and puts the old name back. The new test drives create() against a store whose temp leaf is a symlink, so tempWritable() refuses, and asserts state.workflows and state.logs are unchanged; a second case checks rename keeps the name it could not save. Fixes #2598 Docs-not-needed: rollback inside the existing save path; the documented behaviour is unchanged --- changelog.d/2598.fixed.md | 1 + plugins/workflow/src/service.js | 41 ++++++++-- .../test/refused-write-rollback.test.js | 75 +++++++++++++++++++ 3 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 changelog.d/2598.fixed.md create mode 100644 plugins/workflow/test/refused-write-rollback.test.js diff --git a/changelog.d/2598.fixed.md b/changelog.d/2598.fixed.md new file mode 100644 index 000000000..9e7a8fd7a --- /dev/null +++ b/changelog.d/2598.fixed.md @@ -0,0 +1 @@ +A workflow save that the store refused no longer leaves a half-created workflow in memory: the refusal is returned and the graph, its creation line and the sequence number are dropped, so the next successful save cannot persist a workflow the caller was told was not created; a refused rename keeps the name it could not save ([#2598](https://github.com/ThinkFlowLab/FlyBot/issues/2598)). diff --git a/plugins/workflow/src/service.js b/plugins/workflow/src/service.js index 4c5fa3ad8..b347bf594 100644 --- a/plugins/workflow/src/service.js +++ b/plugins/workflow/src/service.js @@ -1270,12 +1270,27 @@ export function createWorkflowService({ if (!item) return { ok: false, error: 'No such workflow' } const name = cleanLine(body.name, 80) if (!name) return { ok: false, error: 'A workflow needs a name' } - item.name = name - if (body.description !== undefined) { - item.description = cleanLine(body.description, 400) + // #2598: the new name is memory until `write()` has landed it, so a + // refused write puts the old one back instead of listing a name the + // store never took. + const before = { name: item.name, description: item.description, hadDescription: Object.hasOwn(item, 'description') } + let written = false + try { + item.name = name + if (body.description !== undefined) { + item.description = cleanLine(body.description, 400) + } + write() + written = true + return present() + } catch (error) { + if (!written) { + item.name = before.name + if (before.hadDescription) item.description = before.description + else delete item.description + } + return refuse(error) } - write() - return present() }, delete(body = {}) { if (readOnly()) return { ok: false, code: 'read-only', error: 'The host is read-only' } @@ -1324,6 +1339,14 @@ export function createWorkflowService({ }, create(body = {}) { if (readOnly()) return { ok: false, code: 'read-only', error: 'The host is read-only' } + // #2598: `write()` refuses before its first byte when the temp leaf is not + // writable, but the caller is told nothing was created. The push and its + // creation line below are memory, so remember what they change and undo + // them when the write refuses: the next successful write must not persist + // a graph the caller never got. + const before = { workflows: state.workflows.length, seq, stateSeq: state.seq } + let created = null + let written = false try { if (body.template) { const template = templateById(body.template) @@ -1360,6 +1383,7 @@ export function createWorkflowService({ ...(Array.isArray(body.edges) ? { edges: body.edges } : { layers: body.layers }), }) if (!item.name) throw new Error('A workflow needs a name') + created = item // #1544: the Bot that wrote the graph caps the connectors a run may reach from it. const authoredBy = cleanLine(body.callerBotId ?? body.authoredBy, 80) || undefined if (authoredBy) item.authoredBy = authoredBy @@ -1378,8 +1402,15 @@ export function createWorkflowService({ text: `Workflow「${item.name}」已创建 · 成员 ${memberNames}`, }], now()) write() + written = true return present() } catch (error) { + if (!written && created) { + state.workflows.length = before.workflows + delete state.logs[created.id] + seq = before.seq + state.seq = before.stateSeq + } return refuse(error) } }, diff --git a/plugins/workflow/test/refused-write-rollback.test.js b/plugins/workflow/test/refused-write-rollback.test.js new file mode 100644 index 000000000..ed5be855a --- /dev/null +++ b/plugins/workflow/test/refused-write-rollback.test.js @@ -0,0 +1,75 @@ +// #2598: a store write that refuses before its first byte must leave memory as +// it was. `write()` refuses when the temp leaf is a link (temp-name sweep), and +// `create()` had already pushed the graph and its creation line by then, so the +// caller was told nothing was created while the next successful write persisted +// it. `rename()` writes before it keeps the new name for the same reason. +import assert from 'node:assert/strict' +import { mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { test } from 'node:test' +import { createWorkflowService } from '../src/service.js' +import { DELIVERY_ID } from '../src/template.js' + +const GRAPH = { name: 'Audit graph', nodes: [{ id: 'plan', name: 'Planner' }], edges: [] } + +// The store lands through `/workflows.json..tmp`, so a symlink there +// refuses the write before any byte reaches the store (tempWritable). +function linkedStore() { + const dir = mkdtempSync(join(tmpdir(), 'flybot-wf-refused-')) + const svc = createWorkflowService({ dir, persist: true, now: () => 5, bots: () => [] }) + const outside = join(dir, 'outside-workflows.json') + writeFileSync(outside, 'OUTSIDE\n') + const tmp = join(dir, `workflows.json.${process.pid}.tmp`) + symlinkSync(outside, tmp) + return { dir, svc, outside, tmp } +} + +test('a refused create leaves no graph or creation line behind', () => { + const { dir, svc, outside, tmp } = linkedStore() + try { + const before = svc.state() + const refused = svc.create(GRAPH) + assert.equal(refused.ok, false, 'the write is refused') + assert.match(refused.error, /not a plain file/) + assert.equal(readFileSync(outside, 'utf8'), 'OUTSIDE\n', 'the file behind the temp link is not written through') + + assert.deepEqual(svc.state().workflows.map(item => item.name), before.workflows.map(item => item.name), 'the refused create left no graph behind') + + // The ghost used to land on disk with the next successful write. + rmSync(tmp) + const second = svc.create({ name: 'Second', nodes: [{ id: 'plan', name: 'Planner' }], edges: [] }) + assert.equal(second.ok, true) + const stored = JSON.parse(readFileSync(join(dir, 'workflows.json'), 'utf8')) + assert.deepEqual( + stored.workflows.map(item => item.name), + [...before.workflows.map(item => item.name), 'Second'], + 'the next write persists only the graph that was saved', + ) + const created = new Set(stored.workflows.map(item => item.id)) + assert.deepEqual( + Object.keys(stored.logs).filter(id => !created.has(id)), + [], + 'a log line belongs to a workflow that is in the store', + ) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +test('a refused rename keeps the name it could not save', () => { + const { dir, svc } = linkedStore() + try { + const before = svc.state().workflows.find(item => item.id === DELIVERY_ID).name + const refused = svc.rename({ workflowId: DELIVERY_ID, name: 'Renamed' }) + assert.equal(refused.ok, false, 'the write is refused') + assert.match(refused.error, /not a plain file/) + assert.equal( + svc.state().workflows.find(item => item.id === DELIVERY_ID).name, + before, + 'the refused rename left the workflow name as it was', + ) + } finally { + rmSync(dir, { recursive: true, force: true }) + } +})