Make jobs cancelled due to a soft stop immediately available

[brandur]

While working on #1289, I realized that jobs which are "soft stopped"
via context cancellation are still prone to the same side effects as if
they errored in any other way:

• Their number of attempts is incremented.
• They may be discarded if reaching max attempts.
• They'll have to wait to be retried according to retry policy.

This doesn't really seem right because these jobs didn't actually
misbehave in any way, but were rather just slow-to-run jobs that
couldn't finish cleanly inside the default stop allowance while a client
was restarting or being deployed.

The proper behavior should probably be more like a snooze. i.e. The soft
timeout cancellation doesn't count and the jobs get a chance to be
retried immediately. Here, make that change.

The proper event to emit from a client under this condition is a little
tricky because none of the existing EventKindJobCompleted,
EventKindJobFailed, or EventKindJobSnoozed are a good fit for it. We
instead introduce a new value for EventKindJobInterrupted which
describes this precise case and nothing else. Because existing event
stream integrations are required to react based on the existing set of
enum values, it's safe to add a new one for which isn't necessary to
react to (i.e. if a client does nothing about EventKindJobInterrupted
when it receives it, that's perfectly fine).

[brandur]

@bgentry Do you agree with the rationale for this one?

[brandur]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3c090646e1

ℹ️ 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".

[chatgpt-codex-connector]

Keep soft-stop records out of retry backoff

For jobs that are soft-stopped and then later fail for a real reason, this path resets attempt but still appends errData to errors. The default retry policy uses len(job.Errors)+1 rather than attempt, so a deployment stop still changes future retry behavior: the first genuine failure after one soft stop is treated like the second error and backs off for 16s instead of 1s, with repeated stops inflating it further. This means soft-stop cancellations still count for retry scheduling even though the attempt count is restored.

Useful? React with 👍 / 👎.

[brandur]

Fixed! Changed this to be more like snooze and not persist the cancellation error at all.

[bgentry]

I think the general direction makes some sense and it's something I've also pondered in the past, but there are some tradeoffs we should discuss before adopting it.

First, this path still appears to publish a job_failed event because the job is completed back into available state with Snoozed=false. That feels inconsistent with the new model where this cancellation is intentionally not counted as an error, leaves errors unchanged, and restores attempt. Users consuming events for alerting or metrics may see a “failed” event for a job that no longer has failure state/history to explain it.

Second, skipping ErrorHandler removes a user-visible observation/control point. That may be the right outcome if River-owned stop cancellation is not considered a job error, but some users may currently rely on the handler to record shutdown interruptions, cleanup failures, or partial-work cases where the worker returned ctx.Err() because it did not reach a safe checkpoint.

Third, this bypasses retry policy and max-attempt accounting entirely. That is the desired property for normal deploy/restart interruption, but it also means repeated stops can keep a context-sensitive long-running job alive forever with no backoff and no eventual discard. That's intentional I think, but it’s a real semantic tradeoff we should decide on.

[brandur]

@bgentry I started making some changes for this to potentially bypass event subscription, but thought about it a while, and had trouble coming up with a really good argument for doing so. Maybe it's a bit inconsistent with the failure not counting against retries, but maybe it isn't? Even if making the claim that it is, is there a good reason that it matters?

I wonder if we should have it still fire ErrorHandler and consider that good enough.

Third, this bypasses retry policy and max-attempt accounting entirely. That is the desired property for normal deploy/restart interruption, but it also means repeated stops can keep a context-sensitive long-running job alive forever with no backoff and no eventual discard. That's intentional I think, but it’s a real semantic tradeoff we should decide on.

The only way for a job to stay alive forever this way is if the restarts were constant and went on forever. This might be possible if you had restart on a cron job or something like that, but that feels like pretty clear user error.

The alternative seems worse to me — you could have a job that goes through a brief but extremely tumultuous period in production (i.e. a whole bunch of restarts happen in a short period due to a bug or something), and then you'd lose them because they hit their max retries.

What do you think?

[bgentry]

I think I'm mainly hung up on "job failed" not being quite the right label given it didn't actually fail and is merely being rescheduled due to an unrelated shutdown. Maybe it warrants a new enum value specific to this scenario?

Other than that I think I'm good with this because it seems preferable to the status quo.

[brandur]

@bgentry Okay I like the enum addition idea. I also played around with this a bit with Codex, and it came up with the same concept independently here.

Latest push now includes a new EventKindJobInterrupted value which is emitted under this specific case where a job is parked during a restart. This should make for good forward compatibility on event subscribers, while also giving me a way to monitor for this condition if they want to.

At Codex's suggestion, also changed the Snoozed boolean in places like the JobCompleter over to a Reason enum so we don't end up with a series of boolean flags. This method seems a little cleaner and more sustainable.

Do you want to take another look and see what you think?

[brandur]

Thx!

[mitar]

I was trying to rebase #1219 on top of this change and I am finding some behavior differences which makes it harder to rebase it and maybe I should not push "fixes" (or what I believe are fixes) into #1219. So I am making a comment here.

I asked Claude to write me reproduction comparing 0.43.0 version behavior (238776f commit) and current master (with this PR):

Case 1 — JobCompleteTx, then the worker returns an error

func (w *Worker) Work(ctx context.Context, job *river.Job[Args]) error {
      tx, _ := w.dbPool.Begin(ctx)
      defer tx.Rollback(ctx)

      river.JobCompleteTx[*riverpgxv5.Driver](ctx, tx, job) // row -> completed
      tx.Commit(ctx)

      return errors.New("boom") // executor reports an error, but its UPDATE is an IfRunning no-op
}

Row ends completed. In 0.43.0 this posts job_completed event to client.Subscribe. In master this posts job_failed. Deterministic, 5/5 both sides.

Cases 2 and 3 both come from the same place: when a job is cancelled remotely while it is still running, client.JobCancel cannot finalize it directly, so it only writes cancel_attempted_at into the job's metadata. The state change happens later, in JobSetStateIfRunningMany, which rewrites whatever state the executor asked for. So the row lands in cancelled while the executor's own reason still describes what it asked for.

Case 2 — job cancelled remotely while running, then the worker returns an error

// Elsewhere, while the job is running. Only writes cancel_attempted_at into metadata.
client.JobCancel(ctx, jobID)

func (w *Worker) Work(ctx context.Context, job *river.Job[Args]) error {
      return errors.New("boom") // executor asks for retryable, SQL rewrites it to cancelled
}

This needs the next retry to land beyond SchedulerInterval, so the executor asks for retryable (JobSetStateErrorRetryable). I forced that with a Config.RetryPolicy returning now + 1h. With a short retry the executor asks for available instead, which the IN ('retryable','scheduled') condition does not match, so the row stays available and both versions agree on job_failed.

Row ends cancelled. In 0.43.0 this posts job_cancelled event to client.Subscribe. In master this posts job_failed. Deterministic, 5/5 both sides.

Case 3 — same remote cancel, but the worker snoozes

client.JobCancel(ctx, jobID)

func (w *Worker) Work(ctx context.Context, job *river.Job[Args]) error {
      return river.JobSnooze(time.Hour) // executor asks for scheduled, same SQL branch rewrites it to cancelled
}

Row ends cancelled. In 0.43.0 this posts job_snoozed, and in master it also posts job_snoozed, so this PR does not change this case. I am including it because it is the same underlying disagreement between the row and the event: 0.43.0 keyed the snooze event off an unconditional params.Snoozed bool, which is just as blind to the rewritten state as reason is. A subscriber is told the job was snoozed when it was actually cancelled.

Master is 5/5 job_snoozed here. 0.43.0 measured 4/5 job_snoozed and 1/5 job_cancelled, because of the race noted below.

Note on reproducing cases 2 and 3

client.JobCancel on a running job also cancels that job's context, so the context-cancellation path can win the race and produce job_cancelled on its own regardless of which branch above applies. That is where the 1/5 in case 3 comes from, so these two need a few runs to read reliably.

Question

So what about this? This looks like fundamental question which I attempted to answer in #1219, too. What to do if you call JobCompleteTx but then return an error. What should be the result? I think still completed. (And I modeled the same in #1219 for JobCancelTx and JobFailTx.)

This investigation opened another conflict between outside/remote client.JobCancel and what work returns. Should remote one win? Or what work returns? I think for consistency, it should be remote one. But that changes existing behavior of a released version (case 3).

[brandur]

Thanks @mitar. Yeah that definitely seems like a place that we should tighten up. Opened #1350 to resolve this.