fix(triggers): compute nextFireAt in workflow-trigger-service so schedule triggers fire - #568
Merged
Merged
Conversation
…dule triggers fire Schedule triggers created via workflow-trigger-service (the service behind POST/PATCH /api/jobs/:id/triggers) never fired: createTrigger inserted the row without computing next_fire_at, and the schedule poller (getDueScheduleTriggersAll) only selects rows with next_fire_at <= now, so such triggers were perpetually invisible to it. updateTrigger had the same gap — changing the cron or re-enabling a schedule trigger never rescheduled it. The unified /api/tasks/:id/triggers route was unaffected (it goes through workflowService / taskConfigService, which both computed the value). Fix: - Extract computeNextFire into src/utils/cron.ts and reuse it from workflow-service, task-config-service, and workflow-trigger-service (removing the two duplicated private copies). - workflow-trigger-service.createTrigger stamps nextFireAt for enabled schedule triggers; updateTrigger recomputes it on cron change / re-enable and clears it on disable, mirroring the other two services. Tests: - workflow-trigger.int.test.ts: flipped the test that pinned the bug into a positive one (trigger created via the service becomes due and the worker check fires it), plus update-recompute coverage. - scheduled-trigger.e2e.test.ts: the legacy-route section now asserts nextFireAt is computed; the null-nextFireAt due-query case is preserved via direct SQL since the API can no longer produce that state. - workflow-trigger-service.test.ts: unit coverage for the new create/update nextFireAt behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
Schedule triggers created through
workflow-trigger-servicenever fired.There are two creation paths for
workflow_triggersrows:workflowService.createWorkflowTrigger/taskConfigService.createTaskConfigTrigger— used by the unifiedPOST /api/tasks/:id/triggersroute — computenextFireAtfrom the cron expression at creation.workflow-trigger-service.createTrigger— used by the legacyPOST /api/jobs/:id/triggersroute — inserted the row withoutnextFireAt.The schedule poller (
getDueScheduleTriggersAll) only selects rows withnext_fire_at <= now, so a schedule trigger created via/api/jobs/:id/triggerswas perpetually invisible to the poller and never fired.updateTrigger(behindPATCH /api/jobs/:id/triggers/:triggerId) had the same gap: changing the cron expression or re-enabling a schedule trigger never (re)computednextFireAt, so a trigger disabled-then-re-enabled through that route also went dead.Affected routes:
POST /api/jobs/:id/triggers,PATCH /api/jobs/:id/triggers/:triggerId.Unaffected: unified
POST/PATCH /api/tasks/:id/triggers(standalone + repo-blueprint) and/api/task-configs/:id/triggers.Fix
computeNextFireintoapps/api/src/utils/cron.tsand reuse it fromworkflow-service.ts,task-config-service.ts, andworkflow-trigger-service.ts— the first two previously each had their own private copy, so this de-dupes the cron parsing rather than adding a third copy.workflow-trigger-service.createTriggernow stampsnextFireAtfor enabled schedule triggers.workflow-trigger-service.updateTriggernow recomputesnextFireAton cron change / re-enable and clears it on disable, mirroring the other two services (it now also short-circuits tonullfor a missing id before touching the DB, same asupdateTaskConfigTrigger).Tests
apps/api/src/services/workflow-trigger.int.test.ts— the test that explicitly pinned the buggy behavior ("createTrigger does not compute nextFireAt … never fires") is flipped into a positive one: a schedule trigger created via the service gets a computednextFireAt, becomes due, and the worker check fires it (run created,lastFiredAtstamped,nextFireAtadvanced). Added anupdateTriggertest covering recompute-on-re-enable, recompute-on-cron-change, and clear-on-disable.apps/api/e2e/scheduled-trigger.e2e.test.ts— the legacy-route section pinnednextFireAt = null; it now asserts the legacy route computesnextFireAtlike the unified route. The null-nextFireAtdue-query case is preserved by nulling the column via direct SQL, since the API can no longer produce that state.apps/api/src/services/workflow-trigger-service.test.ts— unit coverage for the new create/updatenextFireAtbehavior; existing update mocks adjusted for the new existence check.Verification
cd apps/api && npx tsc --noEmit— cleannpx vitest run(apps/api unit) — 123 files, 2172 tests passednpx vitest run --config vitest.integration.config.ts src/services/workflow-trigger.int.test.ts— 12/12 passed, twicenpx vitest run --config vitest.e2e.config.ts e2e/scheduled-trigger.e2e.test.ts— 4/4 passed, twicepnpm format:check— clean