Run the worker's periodic steps as self-rescheduling pg-boss chains#241
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
The worker tick ran its remaining periodic steps sequentially in one loop, so one slow or wedged step delayed every other step's cadence. Agent runs already moved to their own queue and the minute-or-slower steps (autorecovery, usage metering) to jobs-dir cron jobs; this moves the rest (webhook delivery, alert evaluation, agent chats, digests, observation) out of the tick. Each step becomes its own pg-boss queue advanced as a chain: a pass runs, then sends the queue its next job with startAfter = the step's interval. pg-boss cron can't fire below a minute, which is why these self-schedule instead of joining the cron jobs. Liveness is layered: every pass reschedules in finally (a failed pass is 'retried' by the next pass — the steps are idempotent sweeps, so the queue sets retryLimit 0), and a minute cron per queue re-seeds the chain if it dies, collapsing against a live chain via the singleton key. Passes never overlap, across processes included: the stately policy plus one fixed singleton key allows at most one active job per queue fleet-wide, and the job is held active until its pass settles — a slow pass is logged past its warn deadline but never abandoned, since each step owns its queue and a wedged pass can only stall itself. A bounded expireInSeconds (kept in sync via updateQueue, since createQueue never updates an existing queue) frees the chain after a process crash. A step whose registration fails stays in the tick loop (createWorkerTick now takes a skipSteps set, replacing includeAgentRuns), so a pg-boss outage degrades cadence instead of dropping features. The telemetry scan (spans/logs) stays in the tick until its own migration.
904d10a to
cbc8535
Compare
The fixture NOW (2026-07-15T10:00Z) was in the future when the tests were written, and the seeded predecessor run left createdAt on its wall-clock column default. The 'latest run' ordering compares createdAt, so once real time passed the fixture NOW the successors created at NOW + 1s stopped being newer than the predecessor and 'PR delivery retry rejects a failed predecessor once a newer run exists' began failing on every run. Pin the predecessor to NOW so the ordering is deterministic regardless of wall clock.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b0dba1b3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clickhouse: ch, | ||
| onIssueTransition: dispatchIssueTransition, | ||
| }); | ||
| for (const step of migrated) skipSteps.add(step); |
There was a problem hiding this comment.
Avoid running migrated steps from a failed local fallback
When worker processes are replicated, a transient registration failure on one process while another process successfully registers the same recurring queue leaves migrated missing that step only locally, so this instance keeps running the old tick fallback while the other instance's pg-boss worker also runs the fleet-wide chain. For sweeps like webhooks or alerts that select due work without a per-row claim, that partial-failure case can double-send deliveries or duplicate transitions; the fallback needs a global lease/flag or should avoid running when the queue may be active elsewhere.
Useful? React with 👍 / 👎.
A step whose chain registration failed fell back to this process's tick loop. With replicated workers that reintroduces the overlap the chains eliminate: another process's chain for the same step may be live, so the local fallback would run the step twice (double webhook deliveries, duplicate transitions). Whenever pg-boss is up, all chain-owned steps are now skipped in the tick unconditionally; a step whose registration failed is dormant on this process — logged at error level, with stuck-queue alerting catching a queue left without any consumer. The tick runs the steps itself only when pg-boss is unavailable entirely.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5de6fc8a8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clickhouse: ch, | ||
| onIssueTransition: dispatchIssueTransition, | ||
| }); | ||
| for (const step of RECURRING_TICK_STEPS) skipSteps.add(step); |
There was a problem hiding this comment.
Skip only recurring steps that registered
In a single-worker deployment, or when the same per-step registration error happens on every worker, this loop disables all chain-owned tick steps even if startRecurringSteps() just caught a registerRecurringStep failure before installing that step's seed/consumer. The failed webhook/alert/chat/digest/observation step then has neither a pg-boss worker nor the tick fallback running it until the next boot, and queue-health may not see anything if the failure happened before a job was inserted; use the returned migrated set to skip only steps that actually registered, or otherwise keep unregistered steps in the tick.
Useful? React with 👍 / 👎.
Dropping the per-step tick fallback left a step whose registration failed with nothing running it until the next boot — and if the failure hit before any job was inserted, queue-health had nothing to alarm on. Retry the registration on a self-scheduling timeout instead: the step still never runs locally (another process's chain may be live), a transient failure heals within a minute, and a persistent one logs an error on every attempt. Sequential attempts plus idempotent registration calls keep a retry from racing a second consumer onto the queue.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53e641702e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clickhouse: ch, | ||
| onIssueTransition: dispatchIssueTransition, | ||
| }); | ||
| for (const step of RECURRING_TICK_STEPS) skipSteps.add(step); |
There was a problem hiding this comment.
Skip only recurring steps that registered
When PgBoss starts but one recurring step fails to register, startRecurringSteps() catches that per-step failure and returns only the steps that actually registered. This line ignores that result and skips every migrated tick step anyway, so in a single-worker/self-hosted deployment a persistent createQueue/work failure for e.g. webhook-deliveries leaves no queue consumer and also prevents the tick from calling tickWebhooks, pausing that feature until registration retry succeeds. Use the returned registered set to decide which steps to skip.
Useful? React with 👍 / 👎.
Why
The worker tick runs its remaining periodic steps sequentially in one loop, so one slow or wedged step (a hung provider call, a saturated sweep) delays the cadence of every other step. Agent runs already moved to their own pg-boss queue (#217/#218) and the minute-or-slower steps (autorecovery, usage metering) to jobs-dir cron jobs (#268/#274); this migrates the rest: webhook delivery, alert evaluation, agent chats, digests, observation.
How
Each step becomes its own pg-boss queue advanced as a self-rescheduling chain (
worker/recurring.ts): a pass runs, then sends the queue its next job withstartAfter= the step's interval. pg-boss cron can't fire below a minute, which is why the latency-sensitive sweeps (webhooks 5s, chats 5s, alerts 10s) self-schedule instead of joining the cron jobs in the jobs dir.Liveness is layered:
finally— a failed pass is "retried" by the next pass, since every step is an idempotent cursor/state-driven sweep. The queue setsretryLimit: 0(the chain, not pg-boss retries, provides continuity).Passes never overlap, across processes included: the
statelypolicy plus one fixed singleton key allows at most one active job per queue fleet-wide, and the job is held active until its pass fully settles. A slow pass is logged past its warn deadline but never abandoned — each step owns its queue, so a wedged pass can only stall itself, and the reviver-seeded successor aging as pending keeps the stall observable. A boundedexpireInSeconds(2× the warn deadline, 10-minute floor; kept in sync viaupdateQueuesincecreateQueuenever updates an existing queue) frees the chain after a process crash without re-running the pass.Fallback
A step whose registration fails stays in the tick loop:
createWorkerTicknow takes askipStepsset (replacingincludeAgentRuns), and only successfully registered steps are skipped. A pg-boss outage degrades cadence instead of dropping features. Registration order is load-bearing (consumer last) for the same reason as the agent-run queue.The telemetry scan (spans/logs) stays in the tick until its own migration — after that the tick disappears.
Notes
startAftercount as pending with age bounded by the step interval (≤60s), well under stuck-queue alerting thresholds.recurring step pass failed(scopeworker.recurring) instead oftick step failed; the scan steps still log the latter.Testing
test:recurring— chain mechanics: registration shape (stately queue + lease, updateQueue sync, reviver cron, seed, consumer-last), reschedule-after-interval, failure handling, hold-until-settled, reschedule-failure behavior, expiry bounds.test:recurring-steps— step inventory, sub-minute cadences, partial-registration fallback.test:worker-tick— skipped steps never run; the scan always does.pnpm --filter @superlog/worker typecheck, plus the adjacent suites (test:agent-run-queue,test:queue-health,test:jobs-runner) all green after rebasing onto main.