Skip to content

fix(scheduler): correctly identify schedulers with 5+ segment ids (#3499) - #4056

Open
maruthang wants to merge 5 commits into
taskforcesh:masterfrom
maruthang:fix/issue-3499-repeated-job-next-advance
Open

fix(scheduler): correctly identify schedulers with 5+ segment ids (#3499)#4056
maruthang wants to merge 5 commits into
taskforcesh:masterfrom
maruthang:fix/issue-3499-repeated-job-next-advance

Conversation

@maruthang

Copy link
Copy Markdown
Contributor

Why

Closes #3499. The worker disambiguates a job scheduler vs a legacy repeatable job by counting : segments in repeatJobKey — if there are fewer than 5, treat it as a scheduler. Any user-provided jobSchedulerId with 5+ colon segments (e.g. calculate:trending:subjects:tenant:instanceId-XXXXX from the report) was misrouted to the legacy repeat.updateRepeatableJob path, which is a no-op for every-only schedulers. The result: next stayed in the past and iterationCount was stuck at 1, so the job never ran a second time.

How

  • Added isJobScheduler(id) in src/classes/job-scheduler.ts. It probes HEXISTS repeat:<id> ic — the ic (iteration count) field is only written by storeJobScheduler, never by the legacy repeatable flow, so it's a reliable discriminator.
  • In src/classes/worker.ts, kept the cheap <5 segments fast path; for ambiguous 5+ segment shapes, the worker now consults isJobScheduler before falling back to the legacy path.
  • Regression test added in tests/job_scheduler.test.ts asserting that next advances and iterationCount increments after the first job runs for a 5-segment scheduler id. The test fails without the fix (expected 1486439640000 to be greater than 1486439640000) and passes with it.

Additional Notes

  • Backwards-compatible: short-id schedulers and legacy repeatable jobs both keep their existing fast paths.
  • One additional HEXISTS round-trip only when the segment count is ambiguous.
  • Full tests/job_scheduler.test.ts suite: 74 passing / 1 pre-existing failure (should create job schedulers with different cron patterns — also fails on master, unrelated).

…askforcesh#3499)

When a job scheduler id contains 5 or more colon-separated segments
(for example "calculate:trending:subjects:tenant:instanceId-XXXXX"),
the worker misclassified the scheduled job as a legacy repeatable job
and routed it through the legacy `repeat.updateRepeatableJob` path.
That path is a no-op for `every`-only schedulers, so the scheduler's
`next` value stayed in the past and `iterationCount` remained at 1
forever, meaning the job never ran a second time.

Replace the segment-count-only discriminator with a metadata probe:
the new `JobScheduler.isJobScheduler` checks `HEXISTS repeat:<id> ic`,
where the `ic` field is written exclusively by `storeJobScheduler`
and never by the legacy `addRepeatableJob` flow. The worker keeps
the cheap segment-count fast path for ids with fewer than 5 segments
and only consults Redis when the shape is ambiguous, so there is no
extra round trip for the common case.

Add a regression test that creates a scheduler with 5 colon segments,
processes the first job, and asserts that `next` advances and that
`iterationCount` increments past 1.
…peated-job-next-advance

# Conflicts:
#	src/classes/worker.ts
#	tests/job_scheduler.test.ts
@maruthang

Copy link
Copy Markdown
Contributor Author

Rebased on master. While resolving conflicts I found master has since landed an equivalent fix (the isJobScheduler() discriminator from #3828), so this PR now only adds a regression test asserting next advances and iterationCount increments for scheduler ids with 5+ segments (#3499). Happy to retitle it as test-only or close it if you prefer.

@maruthang

Copy link
Copy Markdown
Contributor Author

CI note: the single remaining red job (testing node@lts/*, dragonflydb@latest) fails on job_scheduler.test.ts > when job schedulers are upserted in quick succession > should create only one job scheduler and one delayed job — a test this PR doesn't touch, with 816/817 tests passing. The same test/job signature is currently failing on other unrelated PRs too (e.g. two PRs with completely disjoint diffs), while master's own dragonflydb job passes intermittently — so it looks like an upstream-flaky test on Dragonfly rather than anything introduced here.

@manast
manast requested review from Copilot and roggervalf and removed request for Copilot July 15, 2026 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Repeated Jobs never run more than once

2 participants