riverqueue / riverqueue/river

Remote JobCancel() can be silently lost while the notifier is reconnecting (no durable-poll fallback)

Open
#1,358 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
5.7k
Forks
179
Avg merge
15h 43m
Merged PRs (30d)
13

Description

Summary

Client.JobCancel() delivers its cancellation signal to a running job's executor exclusively via Postgres LISTEN/NOTIFY. If the notifier happens to be mid-reconnect (its exponential backoff loop, internal/notifier/notifier.go) at the moment the NOTIFY is sent, that specific signal is lost — not delayed — because Postgres NOTIFY is fire-and-forget and only reaches sessions that are actively LISTENing at commit time. The job keeps running unaware it was cancelled, and the only recovery is JobRescuer's stuck-job sweep, which defaults RescueStuckJobsAfter to one hour when unconfigured.

This is a real, reproducible failure mode, not a theoretical one — details and evidence below.

Mechanism

  1. notifier.go's run loop treats any listener error (a transient connection reset, a pooler cycling the backend, a brief network blip, etc.) as cause to disconnect and retry with exponential backoff: 0.5s, 1s, 2s, 4s, 8s, 16s, 32s, 64s (rivershared/util/serviceutil.ExponentialBackoff, MaxAttemptsBeforeResetDefault = 7; the run loop's own attempt counter starts at 0, one step earlier than ExponentialBackoff's doc comment assumes, so the very first retry lands at 0.5s before the schedule settles into the documented 1s→64s cycle on later retries). Each connect/listen attempt also has its own 10s timeout (listenerTimeout).
  2. While the notifier is in that backoff window, it is not LISTENing on river_control (or any topic) at all.
  3. If JobCancel() is called during that window, its NOTIFY commits successfully but has no active listener to reach — it is gone. JobCancel() itself has no visibility into notifier connectivity and returns success regardless (correctly — the durable state write, cancel_attempted_at in the job's metadata, did succeed).
  4. The running job's executor never observes ctx.Done(). It keeps running until it completes normally, or until JobRescuer eventually rescues it — up to an hour later by default, since RescueStuckJobsAfter isn't tied to cancel_attempted_at specifically; it's the same general stuck-job window.

Confirmed by reading the source directly (not inferring from behavior) against v0.44.0 — this is not a stale/fixed-since concern; I checked master too (currently a few commits past v0.44.1, none touching notifier/producer/rescuer).

Evidence

Found while investigating an intermittent CI failure in a compatibility test harness that exercises River under session-mode PgBouncer pooling (session-mode pins one backend connection per client for its whole life, which is the documented-correct pooling mode for River's LISTEN-dependent components per the River PgBouncer docs). The harness's own JobCancel()-driven cancellation test would intermittently hang until its bounded wait (20s) expired, under CPU-constrained CI runners. That investigation turned up two separate, independently-confirmed issues on our side, only one of which is River's: (1) our own connection-pool sizing was too tight under load — our bug, already fixed; (2) the mechanism described in this issue, confirmed present in River's source and unaffected by our pool fix.

For the mechanism in this issue specifically: I'm reporting it because I read and understand the code path, not because I caught the notifier's reconnect/backoff loop firing at the exact instant of a specific test hang (that's a narrow window to catch live, and I don't want to overclaim a live capture I don't have). What I can say with confidence: NOTIFY semantics in Postgres are fire-and-forget by design (not River-specific), the notifier's backoff loop is real and unchanged as of the current master, and the two combine straightforwardly into exactly the failure mode described above with no additional assumptions needed.

Suggested direction

#1135 raised a related situation earlier this year, and the response there (also echoing the original design discussion in #630 back in 2024) was that a durable-poll fallback for all running jobs on all clients is rightly seen as prohibitive query load for what's framed as a rare event. I think that's the right call for that shape of fallback — but there's a narrower version worth considering:

A much cheaper, targeted version of that idea: poll only the (typically tiny) set of jobs that actually have metadata->>'cancel_attempted_at' set — i.e., jobs someone has already explicitly tried to cancel — rather than every running job. That set should be empty almost all the time in a healthy system, making the query cost negligible in the common case while providing deterministic convergence in the rare case a NOTIFY is genuinely lost. This could live client-side (each client polling its own currently-running job IDs against that condition) or as an addition to the existing rescuer's responsibilities with a much shorter, cancel-specific window than the general stuck-job RescueStuckJobsAfter.

Happy to discuss further or take a stab at a PR if there's interest in this direction.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with internal/notifier/notifier.go and the Client.JobCancel() path, then inspect JobRescuer and the cancel_attempted_at metadata described in the issue. Review related discussions in #1135 and #630 before choosing between client-side polling and rescuer changes. Done means a cancellation remains observable when PostgreSQL LISTEN/NOTIFY is unavailable during reconnect, with convergence sooner than the general stuck-job window.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, postgresql
Domain
backend, databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.