Reconcile active jobs against cancel_attempted_at after notifier reconnects - #1361
Open
chrisgeo wants to merge 3 commits into
Open
Reconcile active jobs against cancel_attempted_at after notifier reconnects#1361chrisgeo wants to merge 3 commits into
chrisgeo wants to merge 3 commits into
Conversation
Contributor
|
@chrisgeo Can you add yourself to the CLA? https://github.com/riverqueue/rivercla Also, why don't you go ahead and rebase. We merged another fix quite recently that affected the changelog. |
chrisgeo
force-pushed
the
jobcancel-reconnect-poll
branch
from
August 23, 2026 23:52
989ee34 to
10c23cf
Compare
Author
|
@brandur Rebased onto current master — the changelog conflict from #1359 is resolved. CLA signed (riverqueue/rivercla#33). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the mechanism described in #1358:
Client.JobCancel()delivers its cancellation signal via PostgresLISTEN/NOTIFY, andNOTIFYis fire-and-forget — if the notifier is disconnected/reconnecting (its exponential backoff loop) at the moment theNOTIFYcommits, the signal is lost for good. The running job keeps executing unaware untilJobRescuer's stuck-job sweep eventually catches it (defaults to a 1h window).As discussed in #1358 (comment from @brandur): each client now polls its own currently-running jobs by primary key for
cancel_attempted_atevery time its notifier (re)establishes healthy listening — both the very first connect and every subsequent reconnect. This is cheap (a plainid = any(...)lookup via the existingJobGetByIDMany, no new query/index), rare (only fires on reconnect), and closes exactly the lost window: a signal lost during a reconnect is caught the moment the reconnect completes.Changes (first commit):
internal/notifier: newNotifier.RegisterListenerReadyFunc— lets a caller register a callback invoked every time the notifier establishes healthy listening on all its subscribed topics (including the initial connect; callers that only care about genuine reconnects can no-op on an empty precondition, which is what the producer does below).producer.go: registers aListenerReadyFunc(only when a notifier is configured) that signals a newreconnectCh, consumed on the producer's single owning goroutine. The handler is a no-op when there are no active jobs (true at startup, so the initial-connect firing is free); otherwise it looks up its currently-active job IDs viaJobGetByIDManyand cancels any withcancel_attempted_atset. A query failure retries with the existing exponential backoff utility rather than silently dropping the reconciliation; a malformed metadata row is logged and skipped without aborting the rest of the batch.A second gap from the same root observation (second commit, "Deliver cancellation locally when the client has no notifier"): a client configured without a notifier at all — including an explicit
Config.PollOnlyon an otherwise listener-capable driver (pgx, SQLite) — had no delivery path whatsoever for cancelling a job running in its own process; the local same-process dispatch that already exists for this case only ever activated for drivers that can't supportLISTEN/NOTIFYat all, missing the more common explicit-PollOnlycase entirely. This commit adds a narrowly-scoped fix via its own dispatch helper rather than changing the existing shared one (which is also used by queue pause/resume/update and has different, already-correct semantics for those). Happy to split this into its own PR if preferred.Testing: Added
TestProducer_JobCancelSurvivesNotifierReconnect, a deterministic repro against a real Postgres notifier: forces one simulated connection loss after a job starts, holds the notifier at the start of its reconnect attempt, cancels the job while disconnected (so theNOTIFYis lost), then releases the reconnect and asserts the job observes the cancellation. Fails onmaster(job runs to completion unaware); passes with this change. Also added direct coverage forRegisterListenerReadyFuncininternal/notifier, and for the second commit: a same-process explicit-PollOnlycancellation test and a regression test guarding against blocking when cancelling a burst of non-running jobs.No steady-state query cost is added to either fix — the reconciliation only runs on reconnect and is a no-op when nothing is running; the local dispatch only fires for jobs actually running in-process.