feat(scheduler): shared background-job runner (PMS-135) #69

Merged

nrupard merged 2 commits from feat/pms-135-scheduler into main

2026-06-04 22:03:27 +02:00

nrupard commented

2026-06-04 21:48:59 +02:00

Owner

Summary

New src/scheduler/ module providing a Job trait and Scheduler struct (register + start) so background jobs no longer copy-paste the tokio::time::interval + MissedTickBehavior::Skip + tracing::warn boilerplate per worker.
One tokio task per registered job (so a slow tick on one job does not delay another). Per-tick tracing::info_span!("scheduler_tick", job = ...) composes with PMS-126 service-method instrumentation. Tick errors log at warn and the loop continues; backoff stays the job's responsibility.
tests/scheduler.rs smoke covers two cases: happy-path job ticks at the configured interval; a job that returns Err every other tick does not kill the loop.
Interval-only by design (cron / wall-clock alignment deferred until first consumer needs it).

Follow-up (not in this PR)

Migrate DispatcherWorker (src/modules/notifications/worker.rs) to implement Job and register via the scheduler. Backoff ladder stays inside the worker.
Migrate RmmSyncWorker (src/modules/rmm/worker.rs) to implement Job and register via the scheduler.

Test plan

Local cargo fmt --all --check, cargo clippy --all-targets -- -D warnings, cargo test --test scheduler (2/2 pass) via the shared rust-builder image.
CI: same.

Closes PMS-135. Last subtask of PMS-121 (cross-cutting server infrastructure).

## Summary - New `src/scheduler/` module providing a `Job` trait and `Scheduler` struct (`register` + `start`) so background jobs no longer copy-paste the `tokio::time::interval` + `MissedTickBehavior::Skip` + `tracing::warn` boilerplate per worker. - One tokio task per registered job (so a slow tick on one job does not delay another). Per-tick `tracing::info_span!("scheduler_tick", job = ...)` composes with PMS-126 service-method instrumentation. Tick errors log at `warn` and the loop continues; backoff stays the job's responsibility. - `tests/scheduler.rs` smoke covers two cases: happy-path job ticks at the configured interval; a job that returns `Err` every other tick does not kill the loop. - Interval-only by design (cron / wall-clock alignment deferred until first consumer needs it). ## Follow-up (not in this PR) - Migrate `DispatcherWorker` (`src/modules/notifications/worker.rs`) to implement `Job` and register via the scheduler. Backoff ladder stays inside the worker. - Migrate `RmmSyncWorker` (`src/modules/rmm/worker.rs`) to implement `Job` and register via the scheduler. ## Test plan - [x] Local `cargo fmt --all --check`, `cargo clippy --all-targets -- -D warnings`, `cargo test --test scheduler` (2/2 pass) via the shared rust-builder image. - [ ] CI: same. Closes PMS-135. Last subtask of PMS-121 (cross-cutting server infrastructure).

nrupard added 1 commit

2026-06-04 21:48:59 +02:00

feat(scheduler): shared background-job runner (PMS-135)

Check / fmt + clippy + compile + tests (pull_request) Successful in 1m34s

Details

Build OCI container / Build and push mokosh-api image (push) Successful in 3m3s

Details

887dbd71de

Replaces the ad-hoc-spawn-per-worker pattern (notifications dispatcher + RMM sync worker, each independently `tokio::spawn`'d from `main.rs`) with a single `Scheduler` registry. Future jobs from PMS-58 (calendar reminders), PMS-64 (contract renewals/expiry), PMS-100 (RMM sync future ticks), and PMS-106 (SLA breach checks) all hang off the same abstraction instead of re-implementing the `tokio::time::interval` + `MissedTickBehavior::Skip` + `tracing::warn!` boilerplate.

Shape:

```rust
let mut sched = Scheduler::new();
sched.register(notifications_worker, Duration::from_secs(5));
sched.register(rmm_worker, Duration::from_secs(60));
sched.start();
```

Each registered `Job` runs on its own tokio task so a slow tick on one job does not delay any other. Per-tick `tracing::info_span!("scheduler_tick", job = job.name())` composes with the PMS-126 service-method instrumentation. Tick errors log at `warn` and the loop continues; richer retry semantics (e.g. the notifications dispatcher's 1m/5m/30m/2h/6h backoff ladder) stay inside the `Job::run` impl, not the scheduler, so migration of existing workers is byte-for-byte semantic.

This PR only ships the abstraction + `tests/scheduler.rs` smoke (one happy-path job, one job that returns Err every other tick to prove the loop survives). The existing `DispatcherWorker::run_forever` and `RmmSyncWorker::run_forever` entry points are untouched; cut-over to the new API ships in two follow-up PRs (one per worker) so each diff stays small and the existing backoff / dedupe behavior is reviewed against the new shape one worker at a time.

#PMS-135

nrupard referenced this pull request from a commit

2026-06-04 21:56:55 +02:00

fix(scheduler): address code-review findings on shared scheduler (PMS-135)

nrupard added 1 commit

2026-06-04 21:56:55 +02:00

fix(scheduler): address code-review findings on shared scheduler (PMS-135)

Check / fmt + clippy + compile + tests (pull_request) Successful in 3m27s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

Build OCI container / Build and push mokosh-api image (push) Successful in 7m14s

Details

3895e1732f

Seven concerns from the cavecrew review of #69:

- **HIGH: span across .await.** `let _enter = span.enter()` is a sync guard tied to the OS thread; holding it across `job.run().await` lets the task drift onto another thread mid-await and the log lines drift off the wrong context. Switched to `job.run().instrument(span).await` (textbook tracing-in-async fix).
- **HIGH: duplicate Job::name collisions.** Two jobs registered with the same name produced identical span fields, breaking per-job filtering. `register()` now panics on duplicate; `start()` re-verifies with a HashSet as defense in depth.
- **MEDIUM: missing `#[must_use]` on Scheduler.** Building a scheduler and forgetting `.start()` was silent; annotated `Scheduler` so the compiler warns.
- **MEDIUM: wall-clock test flake.** Both smoke tests previously slept on the real clock and asserted on tick counts; on a loaded CI runner the conservative `>= 3` threshold could still drop. Switched to `tokio::time::pause()` + a virtual-time advance helper so the tests are deterministic. Required adding `test-util` to the workspace's `tokio` dev-dependency features; documented in the Cargo.toml comment.
- **MEDIUM: `#[tokio::test]` flavor.** Initially tried `flavor = "multi_thread"` for fair scheduling, but `tokio::time::pause()` panics on multi-thread; reverted to the default `current_thread` runtime (single-runtime is fine here because every wait is a `tokio::time::sleep` that yields).
- **MEDIUM: JoinHandle discarded.** `start()` now returns `Vec<JoinHandle<()>>`; the caller may drop the vec for fire-and-forget daemon semantics or keep it for graceful-shutdown coordination.
- **MEDIUM: undocumented immediate first tick.** Added a paragraph at the module docs noting `tokio::time::interval` fires immediately, so jobs that need a warmup delay must sleep inside `run` on first invocation.

New third test (`register_rejects_duplicate_job_names`) pins the duplicate-name panic semantics. All three tests pass under virtual time on the shared rust-builder image.

#PMS-135

nrupard merged commit a305bcab2e into main

2026-06-04 22:03:27 +02:00

nrupard deleted branch feat/pms-135-scheduler

2026-06-04 22:03:28 +02:00

nrupard referenced this pull request from a commit

2026-06-04 22:03:29 +02:00

Merge pull request 'feat(scheduler): shared background-job runner (PMS-135)' (#69) from feat/pms-135-scheduler into main