feat(jobsriver): bound worker Timeout() below the pinned rescue horizon
📦 What
Bounds the run budget every periodic worker hands River, against River's stuck-job rescue window, and pins that window instead of inheriting it.
River's rescuer cannot distinguish an orphaned job from one still working: it reads only state = 'running' and attempted_at, scanning every 30s. Its only protection is margin between a worker's own deadline and the rescue gate, and nothing checked that margin existed.
The horizon was River's inherited one-hour default, and RegisterWorker never called Timeout(). River validates RescueStuckJobsAfter >= JobTimeout, but only against the client-level JobTimeout, which this client does not set — so a per-kind override was unbounded. reconciliation_orphan_sweep_interval (default 3h) therefore had negative margin: its deadline and the rescue gate cleared at the same instant, and River has no fencing token, so the row could return to retryable while the original goroutine was still unwinding.
🔨 Changes
internal/jobsriver — the horizon and the ceilings. Client.Start pins RescueStuckJobsAfter at four hours rather than inheriting it, so a River upgrade cannot move the value the ceilings are defined against. RegisterWorker probes Timeout() on a zero-valued job and panics on a negative value or one above MaxWorkerTimeout (one hour). A kind whose budget genuinely needs more registers with the new AllowLongTimeout(), raising its ceiling to MaxLongWorkerTimeout (three hours) and no further. Every kind keeps at least an hour of margin. Compile-time assertions pin both margins, so editing any of the three constants fails the build rather than only falsifying prose.
The clamp, and why it is not a config ceiling. Every periodic worker in the tree derives Timeout() from an operator-settable cadence knob, so a ceiling enforced at config load would confiscate the operator's schedule rather than bound the worker's run budget. WorkerTimeout and LongWorkerTimeout apply it where the budget is derived instead: a non-positive duration passes through unchanged, so zero keeps meaning "use River's client default" and a negative still reaches the registration probe rather than being clamped into validity; anything else is bounded by the kind's ceiling.
Six periodic workers derive their budget through them — both reconciliation triggers, both drain triggers, lifecycle.SweepWorker and remote.HealthSweepWorker. Each one's cadence is left alone, and ReconciliationTriggerWorker's staleness threshold keeps reading the unclamped interval, because that use is genuinely the cadence rather than a budget.
remote.HealthSweepWorker needed more than a clamped return. Its probe pacing spread starts across ScheduledInterval, so clamping only the deadline would have paced an eight-hour fan-out against a one-hour budget and cut the tail off — worse than doing nothing. Both the deadline and probeSpacing now read one budget() accessor, and a test pins that the two cannot diverge.
internal/config — five policy ceilings removed, one arithmetic bound added. No cadence knob is bounded above by a policy ceiling any more: not the three under storage_accounting, and not the two on lifecycle.sweep_interval and virtual_repositories.health_check.scheduled_interval that predate this work. Both of the latter documented themselves as restating River's rescue-horizon margin and named no independent bound, which the probe and the clamp now cover. storage_accounting's validation returns to its original eight rejection arms.
The one bound that goes back in is arithmetic rather than policy, and it sits nowhere near a schedule an operator would set: virtual_repositories.health_check.scheduled_interval is rejected past 640511h56m49.213693951s, the widest cadence whose consecutive-failure-counter TTL — four times the interval — still fits a time.Duration. Past that the TTL wraps and managementFailureCounter panics at boot, so the load rejection turns a boot panic into a startup error naming the field. The constructor guard stays as the backstop, with a separate sentinel per arm: the overflow arm's product is positive, so one shared "must be positive" message would send an operator hunting a value no configuration holds.
remote.RetryAfterSeconds — the minted value is capped. A 503 this service mints for an unavailable upstream carries the health-sweep cadence rounded up to whole seconds, and with the policy ceiling gone that cadence is unbounded, so the header could name a wait of hours: a pull abandoned rather than a hint waited out. MaxMintedRetryAfterSeconds caps it at 3600, the bound internal/format/oci/remote_relay.go already applies to a value an upstream chose. This is the branch's one client-facing change.
internal/accounting. Registers the orphan sweep with AllowLongTimeout() — the only kind in the tree using the option, because its Timeout() derives from a three-hour cadence knob that exceeds MaxWorkerTimeout.
Prose. One hour is a policy ceiling on a run budget and the horizon is four hours. Claims of the form "stays under the horizon" survive that widening and were left alone; claims of the form "at or past the horizon", every claim that a cadence knob is rejected at load, and every claim naming a Retry-After value, do not. All three classes were re-derived across internal/, docs/dev/, docs/specs/, config.example.yaml, the config proto, and the run recipe under .claude/skills/run-artifact-registry/.
⚖️ Accepted cost
The rescuer fires at max(RescueStuckJobsAfter, Timeout()), so pinning one hour to four widens the dead-pod window for every short-budget kind. It takes four shapes:
lifecycle:purgesetsPurgeMaxAttemptsto one, so the rescuer discards rather than re-runs. BecausePurgeEnqueueOptionstakes uniqueness over the live states, which includerunning, a pod killed mid-purge holds its scope's dedup lock until that discard, and each tombstone-sweep tick meanwhile re-enqueues and is dropped silently.- Maven's reconciler takes the same uniqueness but keeps River's default ladder, so its lock is held until the rescuer retries rather than discards.
- The three bulk-delete kinds take no uniqueness at all, so for them the same four hours is a retry delay that a manual resubmission bypasses.
- The two counter-drain chunk kinds override no
Timeout()at all. A pod killed mid-chunk strands a claim whose scopes the tick'sSPOPalready removed from the dirty set;runChunk's staleness check re-adds them and emitscounter_drain_chunk_bailed_totalonly once the rescuer returns the row. The counter drift itself is unaffected, because reconciliation recomputes from source rows withinreconciliation_interval.
In each case the wait moves from about an hour to about four. Nothing counts the window: River's rescuer never invokes the installed ErrorHandler, so jobs_processed_total's only emitting site is unreachable on that path and jobs_in_flight's deferred decrement dies with the pod. jobs_enqueued_total does move — Client.result calls ObserveEnqueue before reading Duplicate — but it counts a deduplicated re-enqueue as a successful one and cannot tell them apart. A rescue counter needs an in-process hook River does not expose, so it is follow-up work.
Widening the two config bounds is also safe forward but not backward. Releases before this one reject lifecycle.sweep_interval and virtual_repositories.health_check.scheduled_interval at or above an hour when the configuration is loaded, and config.Load is the first statement of assembleApp, so a rollback taken while either knob carries such a value leaves every pod of the older binary exiting before it binds. The fleet cannot be recovered by rolling the binary back alone; the YAML has to be lowered first, which is the reverse of the order an incident rollback runs in. Adopting a value at or above an hour is therefore a one-way change until this release is fully rolled out, and both config.example.yaml and the configuration reference say so beside each knob.
The Retry-After cap is a change a client can observe. Above an hour of cadence the header now names a wait shorter than the interval it derives from, so a client may retry against a health status the next sweep has not yet moved. That is the intended trade: at an hour the value has already stopped being a hint a client waits out.
📏 Size
65 files, +1376/-988, measured at 399c6468 against the merge base with main. Reviewable LOC is 2364, past the 500 guideline, and worth reading by group rather than as one diff:
| Group | Files | Added | Removed |
|---|---|---|---|
| Tests | 22 | +657 | -239 |
| Go source | 22 | +257 | -586 |
| Docs and recipe | 18 | +363 | -96 |
| Config and proto | 2 | +74 | -52 |
| Generated | 1 | +25 | -15 |
Groups are by path, so the table is re-derivable from --numstat at any head: gen/ is generated; *_test.go is tests; every other *.go is Go source; config.example.yaml and proto/ are config and proto; everything left — docs/, the two new internal/**/*.md sibling pages, and the run recipe under .claude/skills/run-artifact-registry/ — is docs and recipe.
The size is correction, not production change. Go source is net -329: what it adds is three constants, two compile-time assertions, one register option, one registration probe, two clamp helpers, six call sites, the Retry-After cap, and the failure-counter TTL bound with its own sentinel; the rest of that group is deleted config validation and comment text moved to a sibling page or corrected in place. The remaining volume is the prose and the tests this MR's own behaviour change falsified — every claim that a cadence knob is rejected at load, and every claim naming a Retry-After value, is false at this head unless corrected here, which is why guardrail 17 attaches the correction to this change rather than to a follow-up. Each corrected claim is checkable only against the change that falsified it.
🧪 Testing
- Clamp coverage at, below, and above each ceiling, plus the non-positive pass-through that keeps
0meaning "River default" and leaves a negative for the probe. TestTimeoutCeilings_MarginContractpins the three constants' relationships and the horizon's own value, alongside the compile-time assertions.- A registration case whose worker reads
job.Attemptfrom itsTimeout()argument, so the zero-valued-job contract fails if the probe's argument is ever weakened. - A health-sweep test asserting the deadline and the probe pacing derive from one clamped value.
- Rejected-registration ordering: the probe runs before the option maps are written, so a rejected kind cannot leave a raised ceiling behind.
- The failure-counter TTL bound at three points, mirroring the storage-accounting suite that guards the same shape: the bound itself loads, one nanosecond above it is rejected, and
700000his rejected — both rejections naming the field and printing the bound. The constructor table pins which of the two sentinels each rejected cadence takes. driver.sh smokeon this head: 73 passed, 0 failed. Guardrail 21's trigger is the service changing how it boots, which this branch does — config-load rejection arms removed, a registration panic added, and thescheduled_intervalbound added.
The behavior change guardrail 12 asks about is the Retry-After cap rather than the rescue horizon: no scenario references the horizon, but capping a client-facing header does reach the catalogs. e2e.oci.remote.blob-unhealthy-remote is the one scenario stating the header's value, and it now reads "the health-sweep interval capped at one hour". Three more reference Retry-After without one — npm.md:167, npm.md:182 and oci.md:136 — and each asserts the header's presence rather than what it carries, so the cap leaves all three true and none is changed.
Related to #519 (closed)