Skip to content

Remove post-persist ambiguity and have events match emitted state - #1350

Open
brandur wants to merge 1 commit into
masterfrom
brandur-events-persist-state
Open

Remove post-persist ambiguity and have events match emitted state#1350
brandur wants to merge 1 commit into
masterfrom
brandur-events-persist-state

Conversation

@brandur

@brandur brandur commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

This one's aimed at fixing [1] in which for some sequences in workers,
we'd emit a surprising event based on the state that was actually
persisted to the database. This contract was also not stable, and
changed subtly with the introduction of #1219.

From [1], this is best illustrated by a short example where we error
after invoking JobCompleteTx:

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
}

This used to emit a job_completed, but has changed in master to emit
a job_failed.

Here, we try to correct the emitted event and officially standardize it:

Scenario Persisted state Previous event New event
JobCompleteTx commits, then worker returns an error completed job_failed job_completed
Remote cancellation, then worker requests a retry cancelled job_failed job_cancelled
Remote cancellation, then worker snoozes cancelled job_snoozed job_cancelled

Unambiguous persisted states always take precedence, though we retain
reason (added in #1219) to distinguish possible states of available,
which may be (1) an immediate retry after failure, (2) a short snooze,
or (3) an interruption caused by client shutdown.

[1] #1290 (comment)

@brandur
brandur force-pushed the brandur-events-persist-state branch from e4d6dcc to 4c45612 Compare August 13, 2026 01:26
@brandur
brandur marked this pull request as draft August 13, 2026 01:28
@brandur
brandur force-pushed the brandur-events-persist-state branch from 4c45612 to 4750557 Compare August 13, 2026 05:10
attempt = CASE
WHEN river_job.state = 'running'
AND NOT (job_input.state IN ('retryable','scheduled') AND river_job.metadata ? 'cancel_attempted_at')
AND NOT (job_input.state IN ('available','retryable','scheduled') AND river_job.metadata ? 'cancel_attempted_at')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are kind of a related fix that's also adjacent. A job completer may set a job back to available if the scheduled time was very short in the future. Adding available here makes it so that a pending cancel wins if this was about to occur which is the more correct behavior (it was a bug before that this was wasn't considered).

This one's aimed at fixing [1] in which for some sequences in workers,
we'd emit a surprising event based on the state that was actually
persisted to the database. This contract was also not stable, and
changed subtly with the introduction of #1219.

From [1], this is best illustrated by a short example where we error
after invoking `JobCompleteTx`:

    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
    }

This used to emit a `job_completed`, but has changed in `master` to emit
a `job_failed`.

Here, we try to correct the emitted event and officially standardize it:

| Scenario | Persisted state | Previous event | New event |
|---|---:|---:|---:|
| `JobCompleteTx` commits, then worker returns an error | `completed` | `job_failed` | `job_completed` |
| Remote cancellation, then worker requests a retry | `cancelled` | `job_failed` | `job_cancelled` |
| Remote cancellation, then worker snoozes | `cancelled` | `job_snoozed` | `job_cancelled` |

Unambiguous persisted states always take precedence, though we retain
reason (added in #1219) to distinguish possible states of `available`,
which may be (1) an immediate retry after failure, (2) a short snooze,
or (3) an interruption caused by client shutdown.

[1] #1290 (comment)
@brandur
brandur force-pushed the brandur-events-persist-state branch from 4750557 to 11ee66d Compare August 13, 2026 05:14
@brandur
brandur marked this pull request as ready for review August 13, 2026 05:15
@brandur
brandur requested a review from bgentry August 13, 2026 05:15
@brandur

brandur commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@bgentry This does seem like a useful fix for previously somewhat ambiguous behavior. Seem okay?

@mitar

mitar commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

FYI, you are making references to #1219 but I think you want to make references to #1290. #1219 is my PR which has not yet landed (but I would love for it to - and it does push similar question further for two new *Tx functions).

@mitar

mitar commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

I tested this branch in combination and #1219 and it fixes all the issues I encountered while rebasing it. Thanks!

BTW, from code it looks to me like newCompleterJobUpdated panics on JobStateRunning/JobStatePending, is this intentional?

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.

2 participants