Graylog2 / Graylog2/graylog2-server
Unify system-scheduler jobs onto the JobResourceHandler REST path to prevent verb drift
- Dominant language
- Java
- Stars
- 8.1k
- Forks
- 1.1k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 217
Description
## Summary
Follow-up to #27080 (fixed by #27087). System jobs are wired into the REST layer per verb and per surface, so nothing keeps the verb set consistent: #24497 wired `list`/`get` to `SystemJobManager` but not `cancel`, and #24763 then shipped the first cancelable system job, leaving a dead Cancel button. #27087's own first draft then repeated the drift, cancelling handler-managed archive jobs without archive permissions. This issue removes the structural cause.
Depends on #27087 (merge first, then rebase on it). Refactor, not a fix. Do not backport.
## Background
`SystemJobSummary` (System / Overview) is fed by three back-ends: legacy in-memory jobs (`LegacySystemJobManager`), scheduler jobs via pluggable `JobResourceHandler`s (`JobResourceHandlerService`), and system-scheduler jobs (`SystemJobManager`). `SystemJobResource` (per node) and `ClusterSystemJobResource` (cluster-wide) merge them by hand, per verb:
| | list | get | cancel | acknowledge |
|---|---|---|---|---|
| `SystemJobResource` | legacy + `SystemJobManager` | legacy + `SystemJobManager` | legacy + `SystemJobManager` (after #27087) | handler-service only |
| `ClusterSystemJobResource` | fan-out + handler-service | fan-out + handler-service | fan-out + handler-service | (n/a) |
Nothing keeps this matrix in lockstep, and `acknowledge` is already missing a `SystemJobManager` path everywhere.
## Proposed work
**1. Route system-scheduler jobs through the `JobResourceHandler` path.** Register one handler keyed on `SystemJobDefinitionConfig.TYPE_NAME` that delegates to `SystemJobManager` + `DBSystemJobTriggerService`. All system jobs share that `jobDefinitionType`, so one registration covers every current and future system job. Then all four verbs dispatch through `JobResourceHandlerService`, and a new verb is wired in one place.
System triggers are node-independent, so expose them cluster-wide only: `ClusterSystemJobResource` drops its `ObjectId.isValid` special-casing, and `SystemJobResource` shrinks to legacy jobs. The handler must preserve today's `SYSTEMJOBS_READ`/`SYSTEMJOBS_DELETE` checks per job type and the RUNNING-only filter. Compatibility: a direct per-node `GET /system/jobs` would stop listing system-scheduler jobs. The web UI uses the cluster endpoint, but confirm this is acceptable for API consumers.
**2. Harden the handler registry.** `JobResourceHandlerService` uses its registration map inconsistently, which is what let #27087's first draft bypass archive permissions:
- Only `handlesJobType()` and `resourceHandler(trigger)` consult the keys. `listJobs`/`getJob`/`cancelJob`/`acknowledgeJob` broadcast to every handler via `allResourceHandlers()`, so registration does not scope what a handler may act on and each handler re-derives its own scope. Dispatch should resolve the handler by trigger type, making handler-internal filtering defence in depth rather than the real boundary. The dedup-by-class-name workaround falls out once dispatch is keyed.
- A registration mistake fails open: rename a job type without updating the module binding and `handlesJobType` silently returns false, so core cancels the job under `systemjobs:delete` alone. Add a boot-time check that registered keys resolve to known job types (two tiers: handlers also bind against user-scheduler definition types). An unclaimed type should fail closed rather than default to core-only checks.
- `SystemJobManager.toSystemJobInfo` and `JobResourceHandlerService.jobSummaryFromTrigger` build different payloads for the same trigger (name field, and `status`). Unifying on one path should unify the payload.
**3. Add a parity test.** Encode "listable + cancelable ⇒ cancellable" through the handler seam: submit a cancelable system job, assert `JobResourceHandlerService.cancelJob` reaches it and sets the trigger's cancel flag. Cheap once (1) lands. There is no registry mapping a system-job type to its `isCancelable`, so this asserts through the seam rather than reflecting over registered types.
**4. Stop masking node errors in the cluster resource.** `ClusterSystemJobResource.get` and `cancelJob` discard every non-2xx node response and then throw not-found, so a user who can see a cancelable job but lacks `systemjobs:delete` gets a live Cancel button and a misleading "job not found" instead of the `403` the node returned. Rethrow the first error response when no node succeeded and no handler claims the job.
## Non-goals
- `LegacySystemJobManager` is being phased out; legacy jobs stay hand-wired until it is removed.
- Cancel *semantics* in the shared `DBJobTriggerService` (queued trigger, cancel lost across a retry) are tracked in #27084.
## Checklist
- [ ] This issue fix need to be backported.
- [ ] Does this issue have **security** implications?
Contributor guide
Research direction
Start with SystemJobResource, ClusterSystemJobResource, JobResourceHandlerService, SystemJobManager, and DBSystemJobTriggerService after merging #27087. Trace the current list, get, cancel, and acknowledge paths and their permission checks. Done means system-scheduler jobs use one handler path, registry mistakes fail closed, payloads have parity, the parity test passes, and cluster errors preserve meaningful node responses.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- api, authorization, backend, testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100