From a7d45ddd0d59e5ef08a8de023900a89d58ed5df8 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Tue, 1 Sep 2026 18:51:10 +0200 Subject: [PATCH] fix(workflows): restore the once-per-process seed guard `booted` was assigned but never read, so the guard's early return was missing and the builtin workflow seeder ran on every call. Impact was wasteful, not harmful: seedOneBuiltin is idempotent -- it keys on builtin_key and returns early when adminOwned or storedVersion >= def.version, writing a graph only on a fresh insert or a version bump. So repeat calls cost a lookup per builtin plus a graph rebuild, with no duplicate rows. `booted = true` stays inside the try, so a seed that never got off the ground (workflows table not migrated, DB down) leaves the flag clear and retries. A per-builtin failure is still swallowed by the inner catch and does not block the flag, unchanged. Restoring the guard broke workflowEngine.test.js, which calls the boot seeder seven times in one worker and needs the second call to run in two of them. Followed the existing _backupPathsBoot/_restoreSettingsBoot precedent: exported _resetBootForTests(). Refs testplan REPORT.md B3. --- .../integration/workflowEngine.test.js | 48 ++++++++++++++----- backend/src/services/_workflowSeedBoot.js | 22 +++++++-- 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/backend/__tests__/integration/workflowEngine.test.js b/backend/__tests__/integration/workflowEngine.test.js index 83072f72..e16eaa7c 100644 --- a/backend/__tests__/integration/workflowEngine.test.js +++ b/backend/__tests__/integration/workflowEngine.test.js @@ -34,6 +34,15 @@ async function makeWorkflow({ nodes, edges, trigger = 'test.event', enabled = tr return workflowId; } +// seedBuiltinWorkflowsAtBoot carries a once-per-process guard; these tests +// deliberately re-run it (idempotency, version bumps) inside one worker, so +// clear the flag each time. +async function seedBuiltins(logger) { + const mod = require('../../src/services/_workflowSeedBoot'); + mod._resetBootForTests(); + return mod.seedBuiltinWorkflowsAtBoot(db, logger); +} + beforeAll(async () => { ({ db, cleanup } = await bootCrmDb()); // Engine requires the singleton db — require AFTER bootCrmDb wired the test path. @@ -240,9 +249,9 @@ describe('workflow engine', () => { }); test('seeds the invoice-dunning built-in as the delegation graph (disabled for first beta)', async () => { - const { seedBuiltinWorkflowsAtBoot, DUNNING_KEY } = require('../../src/services/_workflowSeedBoot'); + const { DUNNING_KEY } = require('../../src/services/_workflowSeedBoot'); const noopLogger = { info() {}, warn() {} }; - await seedBuiltinWorkflowsAtBoot(db, noopLogger); + await seedBuiltins(noopLogger); const wf = await db('workflows').where({ builtin_key: DUNNING_KEY }).first(); expect(wf).toBeTruthy(); @@ -256,13 +265,13 @@ describe('workflow engine', () => { expect(nodes.some((n) => JSON.parse(n.config || '{}').action === 'queue_payment_check')).toBe(true); expect(nodes.some((n) => JSON.parse(n.config || '{}').action === 'escalate_to_collections')).toBe(true); - await seedBuiltinWorkflowsAtBoot(db, noopLogger); // idempotent at current seed version + await seedBuiltins(noopLogger); // idempotent at current seed version const all = await db('workflows').where({ builtin_key: DUNNING_KEY }); expect(all.length).toBe(1); }); test('re-seeds a stale built-in on version bump, but never an admin-owned one', async () => { - const { seedBuiltinWorkflowsAtBoot, DUNNING_KEY } = require('../../src/services/_workflowSeedBoot'); + const { DUNNING_KEY } = require('../../src/services/_workflowSeedBoot'); const noopLogger = { info() {}, warn() {} }; // Simulate an older, never-touched seed (v1, with a legacy gate node). @@ -270,7 +279,7 @@ describe('workflow engine', () => { await db('workflows').where({ id: wf.id }).update({ enabled: true, admin_toggled_at: null, trigger_config: JSON.stringify({ seedVersion: 1 }) }); await db('workflow_nodes').insert({ workflow_id: wf.id, version: wf.version, node_key: 'legacyGate', type: 'gate', config: '{}', pos_x: 0, pos_y: 0 }); - await seedBuiltinWorkflowsAtBoot(db, noopLogger); + await seedBuiltins(noopLogger); const reseeded = await db('workflows').where({ id: wf.id }).first(); expect(reseeded.version).toBe(wf.version + 1); // bumped expect(JSON.parse(reseeded.trigger_config).seedVersion).toBe(7); @@ -281,15 +290,14 @@ describe('workflow engine', () => { // Admin-owned (admin_toggled_at set) + stale → must NOT be touched. await db('workflows').where({ id: wf.id }).update({ enabled: true, admin_toggled_at: new Date().toISOString(), trigger_config: JSON.stringify({ seedVersion: 1 }) }); const before = await db('workflows').where({ id: wf.id }).first(); - await seedBuiltinWorkflowsAtBoot(db, noopLogger); + await seedBuiltins(noopLogger); const after = await db('workflows').where({ id: wf.id }).first(); expect(after.version).toBe(before.version); // unchanged expect(!!after.enabled).toBe(true); // admin's choice preserved }); test('seeds the gallery, pre-event + booking built-ins (all disabled for first beta)', async () => { - const { seedBuiltinWorkflowsAtBoot } = require('../../src/services/_workflowSeedBoot'); - await seedBuiltinWorkflowsAtBoot(db, { info() {}, warn() {} }); + await seedBuiltins({ info() {}, warn() {} }); // First beta: cutover flows ship DISABLED (legacy paths run until enabled); // they delegate to the proven send functions once turned on. @@ -484,8 +492,7 @@ describe('workflow engine', () => { }); test('isBuiltinFlowActive reflects the built-in ENABLED state (enabled-based mutex)', async () => { - const { seedBuiltinWorkflowsAtBoot } = require('../../src/services/_workflowSeedBoot'); - await seedBuiltinWorkflowsAtBoot(db, { info() {}, warn() {} }); + await seedBuiltins({ info() {}, warn() {} }); // All built-ins ship disabled → inactive until the admin enables one. expect(await engine.isBuiltinFlowActive('gallery_expiring')).toBe(false); expect(await engine.isBuiltinFlowActive('does_not_exist')).toBe(false); @@ -496,8 +503,7 @@ describe('workflow engine', () => { }); test('legacy event-reminder pass stands down ONLY when the pre_event_email flow is enabled', async () => { - const { seedBuiltinWorkflowsAtBoot } = require('../../src/services/_workflowSeedBoot'); - await seedBuiltinWorkflowsAtBoot(db, { info() {}, warn() {} }); // pre_event_email seeded DISABLED + await seedBuiltins({ info() {}, warn() {} }); // pre_event_email seeded DISABLED // crm_event_reminders_enabled must be on to reach the mutex guard. await db('app_settings') .insert({ setting_key: 'crm_event_reminders_enabled', setting_value: JSON.stringify(true), setting_type: 'boolean' }) @@ -658,4 +664,22 @@ describe('workflow engine', () => { const emailStep = steps.find((s) => s.node_key === 'a'); expect(JSON.parse(emailStep.result).dryRun).toBe(true); // send_email mocked, no real mail }); + + test('the once-per-process guard short-circuits a second boot seed', async () => { + const { seedBuiltinWorkflowsAtBoot, DUNNING_KEY } = require('../../src/services/_workflowSeedBoot'); + await seedBuiltins({ info() {}, warn() {} }); + + // Make the row look stale + never-touched, so an UNGUARDED call would + // re-seed it (that's exactly what the version-bump test above asserts). + const before = await db('workflows').where({ builtin_key: DUNNING_KEY }).first(); + await db('workflows').where({ id: before.id }) + .update({ admin_toggled_at: null, trigger_config: JSON.stringify({ seedVersion: 1 }) }); + + // No _resetBootForTests() — `booted` is still set from seedBuiltins above. + await seedBuiltinWorkflowsAtBoot(db, { info() {}, warn() {} }); + + const after = await db('workflows').where({ id: before.id }).first(); + expect(after.version).toBe(before.version); // no re-seed happened + expect(JSON.parse(after.trigger_config).seedVersion).toBe(1); + }); }); diff --git a/backend/src/services/_workflowSeedBoot.js b/backend/src/services/_workflowSeedBoot.js index 897c5978..2c3624b5 100644 --- a/backend/src/services/_workflowSeedBoot.js +++ b/backend/src/services/_workflowSeedBoot.js @@ -335,10 +335,12 @@ const BUILTINS = [ }, ]; -// NOTE: written at the end of seedBuiltinWorkflowsAtBoot but never read — the -// intended "seed only once per process" guard is missing its `if (booted) return;` -// check. Left in place so the gap stays visible rather than being silently dropped. -// eslint-disable-next-line no-unused-vars -- write-only boot guard, see note above +// Seed-once-per-process guard. Re-seeding is idempotent (seedOneBuiltin +// updates in place, keyed on builtin_key, and skips admin-owned or +// already-current rows), so the cost of a repeat call is a table scan per +// builtin plus a graph rebuild — wasted work, not duplicate rows. Set inside +// the try, so a seed that never got off the ground (workflows table not +// migrated yet, DB down) leaves the flag clear and a later call can retry. let booted = false; function parseSeedConfig(raw) { @@ -420,6 +422,7 @@ async function seedOneBuiltin(db, logger, def) { } async function seedBuiltinWorkflowsAtBoot(db, logger) { + if (booted) return; try { if (!(await db.schema.hasTable('workflows'))) return; for (const def of BUILTINS) { @@ -435,4 +438,13 @@ async function seedBuiltinWorkflowsAtBoot(db, logger) { } } -module.exports = { seedBuiltinWorkflowsAtBoot, buildDunningGraph, DUNNING_KEY, BUILTINS }; +// Test-only: reset the module-level boot flag so jest can re-exercise the +// seeder against a fresh test DB inside a single worker. Matches +// _backupPathsBoot / _restoreSettingsBoot. +function _resetBootForTests() { + booted = false; +} + +module.exports = { + seedBuiltinWorkflowsAtBoot, buildDunningGraph, DUNNING_KEY, BUILTINS, _resetBootForTests, +};