paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(workflows): restore the once-per-process seed guard

Browse Source 
`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.
This commit is contained in:
Paul Nothaft
2026-09-02 09:43:10 +02:00
parent 355fe4ff43
commit a7d45ddd0d
2 changed files with 53 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/__tests__/integration/workflowEngine.test.js
+36 -12
View File
@@ -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);
});
});
backend/src/services/_workflowSeedBoot.js
+17 -5
View File
@@ -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,
};