From 25d74d2a3db4c2b84f626b85578879be09747b2f Mon Sep 17 00:00:00 2001 From: Chris George Date: Sun, 23 Aug 2026 14:32:18 -0700 Subject: [PATCH 1/7] Reconcile active jobs against cancel_attempted_at after notifier reconnects --- CHANGELOG.md | 1 + internal/notifier/notifier.go | 59 ++++++++-- internal/notifier/notifier_test.go | 48 ++++++++ producer.go | 119 ++++++++++++++++++- producer_test.go | 177 +++++++++++++++++++++++++++++ 5 files changed, 393 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a6b73c75..7582f2680 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed `Client.InsertManyFast` panicking after its transaction committed on a client configured with queues when the driver doesn't support a listener, such as a `riverdatabasesql` driver created with `New` instead of `NewWithPgxListener`. It now wakes producers for the inserted jobs' queues right away, like `Client.InsertMany`. [PR #1558](https://github.com/riverqueue/river/pull/1558). - Fixed `Client.QueueUpdate` without metadata crashing the program when the client works that queue and the driver doesn't support a listener. [PR #1558](https://github.com/riverqueue/river/pull/1558). - Fixed the notifier's run loop blocking forever when a topic `LISTEN` or `UNLISTEN` handed to it finished after its caller had stopped waiting (after 5 seconds, with `timed out waiting for interrupt operation`), as can happen on a slow or unresponsive connection. `Client.Stop` then never completed, and neither did a `Client.Start` that failed this way. [PR #1559](https://github.com/riverqueue/river/pull/1559). +- A `JobCancel()` sent while a client's notifier is disconnected/reconnecting is no longer lost. Previously, the cancellation's `NOTIFY` could commit while nothing was `LISTEN`ing (Postgres `NOTIFY` is fire-and-forget), leaving the running job unaware of the cancellation until `JobRescuer`'s much longer stuck-job sweep. Each client now reconciles its currently running jobs against `cancel_attempted_at` by primary key every time its notifier (re)establishes healthy listening, closing the gap with no added steady-state query load. [PR #1361](https://github.com/riverqueue/river/pull/1361), as discussed in [#1358](https://github.com/riverqueue/river/issues/1358). ## [0.50.0] - 2026-10-26 diff --git a/internal/notifier/notifier.go b/internal/notifier/notifier.go index d453eddc3..8fba56743 100644 --- a/internal/notifier/notifier.go +++ b/internal/notifier/notifier.go @@ -88,12 +88,46 @@ type Notifier struct { testSignals notifierTestSignals waitInterruptChan chan func() - mu sync.RWMutex - isConnected bool - isStarted bool - isWaiting bool - subscriptions map[NotificationTopic][]*Subscription - waitCancel context.CancelFunc + mu sync.RWMutex + isConnected bool + isStarted bool + isWaiting bool + listenerReadyFuncs map[*ListenerReadySubscription]ListenerReadyFunc + subscriptions map[NotificationTopic][]*Subscription + waitCancel context.CancelFunc +} + +// ListenerReadyFunc is invoked each time the notifier establishes healthy +// listening on all of its currently subscribed topics. This includes the +// very first connect as well as every subsequent reconnect that follows a +// connection loss, so callers whose work is only meaningful after a genuine +// reconnect (as opposed to the initial connect) should make that check +// themselves. +type ListenerReadyFunc func() + +// ListenerReadySubscription is a handle to a func registered with +// RegisterListenerReadyFunc. Call Unregister to stop receiving invocations. +type ListenerReadySubscription struct { + notifier *Notifier +} + +func (s *ListenerReadySubscription) Unregister() { + s.notifier.mu.Lock() + defer s.notifier.mu.Unlock() + delete(s.notifier.listenerReadyFuncs, s) +} + +// RegisterListenerReadyFunc registers a function to be invoked every time the +// notifier (re)establishes healthy listening on all of its subscribed topics. +// It's used to let callers reconcile any state that could have missed a +// notification while the notifier was disconnected/reconnecting. +func (n *Notifier) RegisterListenerReadyFunc(listenerReadyFunc ListenerReadyFunc) *ListenerReadySubscription { + n.mu.Lock() + defer n.mu.Unlock() + + sub := &ListenerReadySubscription{notifier: n} + n.listenerReadyFuncs[sub] = listenerReadyFunc + return sub } func New(archetype *baseservice.Archetype, listener riverdriver.Listener) *Notifier { @@ -102,7 +136,8 @@ func New(archetype *baseservice.Archetype, listener riverdriver.Listener) *Notif notificationBuf: make(chan *riverdriver.Notification, 1000), waitInterruptChan: make(chan func(), 10), - subscriptions: make(map[NotificationTopic][]*Subscription), + listenerReadyFuncs: make(map[*ListenerReadySubscription]ListenerReadyFunc), + subscriptions: make(map[NotificationTopic][]*Subscription), }) return notifier } @@ -214,6 +249,16 @@ func (n *Notifier) listenAndWait(ctx context.Context) error { n.testSignals.ListeningBegin.Signal(struct{}{}) defer n.testSignals.ListeningEnd.Signal(struct{}{}) + listenerReadyFuncs := func() []ListenerReadyFunc { + n.mu.RLock() + defer n.mu.RUnlock() + + return maputil.Values(n.listenerReadyFuncs) + }() + for _, listenerReadyFunc := range listenerReadyFuncs { + listenerReadyFunc() + } + drainInterrupts := func() { for { select { diff --git a/internal/notifier/notifier_test.go b/internal/notifier/notifier_test.go index 8d5c21cc3..c36016f59 100644 --- a/internal/notifier/notifier_test.go +++ b/internal/notifier/notifier_test.go @@ -623,6 +623,54 @@ func TestNotifier(t *testing.T) { // Subscription should still work. require.Equal(t, TopicAndPayload{testTopic1, "msg1"}, riversharedtest.WaitOrTimeout(t, notifyChan)) }) + + t.Run("RegisterListenerReadyFunc", func(t *testing.T) { + t.Parallel() + + notifier, _ := setup(t, nil) + notifier.testDisableSleep = true + + var errorNum int + + listenerMock := NewListenerMock(notifier.listener) + listenerMock.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { + // Returns an error the first time, but then works after. + errorNum++ + switch errorNum { + case 1: + return nil, errors.New("error during wait") + default: + return listenerMock.Listener.WaitForNotification(ctx) + } + } + notifier.listener = listenerMock + + readyChan := make(chan struct{}, 10) + sub := notifier.RegisterListenerReadyFunc(func() { readyChan <- struct{}{} }) + t.Cleanup(sub.Unregister) + + start(t, notifier) + + // Fires once on the initial connect. + riversharedtest.WaitOrTimeout(t, readyChan) + + // And again once the notifier reconnects after the injected failure. + require.EqualError(t, notifier.testSignals.BackoffError.WaitOrTimeout(), "error during wait") + riversharedtest.WaitOrTimeout(t, readyChan) + + // Unregister stops further invocations. There's no direct signal for + // "would have fired again", so drain what's pending, unregister, and + // confirm a subsequent restart doesn't add anything new. + for len(readyChan) > 0 { + <-readyChan + } + sub.Unregister() + + notifier.Stop() + require.NoError(t, notifier.Start(ctx)) + notifier.testSignals.ListeningBegin.WaitOrTimeout() + require.Empty(t, readyChan) + }) } func TestNotifier_Start(t *testing.T) { diff --git a/producer.go b/producer.go index fd5a4e9f5..814cd2697 100644 --- a/producer.go +++ b/producer.go @@ -240,8 +240,22 @@ type producer struct { // Receives control messages from the notifier goroutine. Written by notifier // goroutine, only read from main goroutine. queueControlCh chan *controlEventPayload - retryPolicy ClientRetryPolicy - testSignals producerTestSignals + + // Receives a signal every time the notifier (re)establishes healthy + // listening. Written by the notifier goroutine (via a registered + // ListenerReadyFunc), only read from main goroutine. Used to reconcile + // activeJobs against cancel_attempted_at in case a cancel NOTIFY was lost + // while the notifier was disconnected/reconnecting. Buffered to 1 and + // written to non-blockingly because reconnects are rare and coalescing + // multiple signals into one reconciliation pass is fine. + reconnectCh chan struct{} + + // Only used by main goroutine. Tracks the current backoff attempt for + // retrying the reconnect reconciliation query after a failure. + reconcileAttempt int + + retryPolicy ClientRetryPolicy + testSignals producerTestSignals } func newProducer(archetype *baseservice.Archetype, exec riverdriver.Executor, pilot riverpilot.Pilot, config *producerConfig) *producer { @@ -268,6 +282,7 @@ func newProducer(archetype *baseservice.Archetype, exec riverdriver.Executor, pi jobTimeout: config.JobTimeout, pilot: pilot, queueControlCh: make(chan *controlEventPayload, 100), + reconnectCh: make(chan struct{}, 1), retryPolicy: config.RetryPolicy, workers: config.Workers, }) @@ -373,8 +388,9 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { p.fetchLimiter = chanutil.NewDebouncedChan(fetchCtx, p.config.FetchCooldown, true) var ( - controlSub *notifier.Subscription - insertSub *notifier.Subscription + controlSub *notifier.Subscription + insertSub *notifier.Subscription + reconnectSub *notifier.ListenerReadySubscription ) if p.config.Notifier != nil { var err error @@ -408,6 +424,21 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { } return err } + + // Reconcile activeJobs against cancel_attempted_at every time the + // notifier (re)establishes healthy listening. This closes the window + // described in https://github.com/riverqueue/river/issues/1358: a + // cancel NOTIFY sent while the notifier was disconnected/reconnecting + // is otherwise lost for good (Postgres NOTIFY is fire-and-forget). + // Firing on the very first connect too is harmless: activeJobs is + // always empty at that point in startup, so the fetchAndRunLoop + // handler below is a no-op query-free check. + reconnectSub = p.config.Notifier.RegisterListenerReadyFunc(func() { + select { + case p.reconnectCh <- struct{}{}: + default: + } + }) } go func() { @@ -427,6 +458,10 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { defer controlSub.Unlisten(fetchCtx) } + if reconnectSub != nil { + defer reconnectSub.Unregister() + } + var subroutineWG sync.WaitGroup subroutineCtx, cancelSubroutines := context.WithCancelCause(context.WithoutCancel(fetchCtx)) @@ -606,6 +641,8 @@ func (p *producer) fetchAndRunLoop(fetchCtx, workCtx context.Context) { } case jobID := <-p.cancelCh: p.maybeCancelJob(workCtx, jobID) + case <-p.reconnectCh: + p.handleNotifierReconnect(workCtx) case <-p.fetchLimiter.C(): p.innerFetchLoop(workCtx, fetchResultCh) // Ensure we can't start another fetch when fetchCtx is done, even if @@ -839,6 +876,80 @@ func (p *producer) maybeCancelJob(ctx context.Context, id int64) bool { return true } +// jobMetadataWithCancelAttemptedAt mirrors the identical unexported struct in +// internal/maintenance/job_rescuer.go, which reads the same field for the +// same reason. +type jobMetadataWithCancelAttemptedAt struct { + CancelAttemptedAt time.Time `json:"cancel_attempted_at"` +} + +// handleNotifierReconnect reconciles activeJobs against cancel_attempted_at +// after the notifier has (re)established healthy listening. It exists to +// close the window described in +// https://github.com/riverqueue/river/issues/1358: a cancel NOTIFY sent while +// the notifier was disconnected/reconnecting is lost for good (Postgres +// NOTIFY is fire-and-forget), so a job cancelled during that window would +// otherwise run to completion unaware, undetected until JobRescuer's much +// longer stuck-job sweep. +// +// Only ever called from the main goroutine, so activeJobs can be read +// without a lock. +func (p *producer) handleNotifierReconnect(ctx context.Context) { + if len(p.activeJobs) == 0 { + p.reconcileAttempt = 0 + return + } + + ids := make([]int64, 0, len(p.activeJobs)) + for id := range p.activeJobs { + ids = append(ids, id) + } + + jobs, err := p.exec.JobGetByIDMany(ctx, &riverdriver.JobGetByIDManyParams{ + ID: ids, + Schema: p.config.Schema, + }) + if err != nil { + p.Logger.ErrorContext(ctx, p.Name+": Error reconciling active jobs against cancel_attempted_at after notifier reconnect; will retry", + slog.String("err", err.Error())) + p.scheduleReconnectRetry(ctx) + return + } + p.reconcileAttempt = 0 + + for _, job := range jobs { + var metadata jobMetadataWithCancelAttemptedAt + if err := json.Unmarshal(job.Metadata, &metadata); err != nil { + p.Logger.ErrorContext(ctx, p.Name+": Error unmarshaling job metadata while reconciling cancellations after notifier reconnect", + slog.Int64("job_id", job.ID), slog.String("err", err.Error())) + continue + } + if !metadata.CancelAttemptedAt.IsZero() { + p.maybeCancelJob(ctx, job.ID) + } + } +} + +// scheduleReconnectRetry re-arms p.reconnectCh after a backoff so a +// reconciliation query that failed (e.g. a transient DB error) doesn't +// silently drop the only signal that a reconnect happened, which would +// recreate the lost-cancel window this mechanism exists to close. +func (p *producer) scheduleReconnectRetry(ctx context.Context) { + if ctx.Err() != nil { + return + } + + sleepDuration := serviceutil.ExponentialBackoff(p.reconcileAttempt, serviceutil.MaxAttemptsBeforeResetDefault) + p.reconcileAttempt++ + + time.AfterFunc(sleepDuration, func() { + select { + case p.reconnectCh <- struct{}{}: + default: + } + }) +} + func (p *producer) metricEmitHooksFromLookup() []rivertype.HookMetricEmit { pluginLookup := p.config.PluginLookupGlobal if pluginLookup == nil { diff --git a/producer_test.go b/producer_test.go index 8d8545b7a..d7a9eca24 100644 --- a/producer_test.go +++ b/producer_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "slices" + "sync" "sync/atomic" "testing" "time" @@ -108,6 +109,182 @@ func (p *undecodableKindPilot) JobGetAvailable( return res, nil } +// producerTestListenerMock wraps a real riverdriver.Listener so tests can +// override individual operations. Unlike notifier.ListenerMock (unexported to +// the internal/notifier package), this lives in package river so it can be +// used to drive a real *notifier.Notifier from producer-level tests. +type producerTestListenerMock struct { + riverdriver.Listener + + connectFunc func(ctx context.Context) error + waitForNotificationFunc func(ctx context.Context) (*riverdriver.Notification, error) +} + +func newProducerTestListenerMock(listener riverdriver.Listener) *producerTestListenerMock { + return &producerTestListenerMock{ + Listener: listener, + connectFunc: listener.Connect, + waitForNotificationFunc: listener.WaitForNotification, + } +} + +func (l *producerTestListenerMock) Connect(ctx context.Context) error { return l.connectFunc(ctx) } + +func (l *producerTestListenerMock) WaitForNotification(ctx context.Context) (*riverdriver.Notification, error) { + return l.waitForNotificationFunc(ctx) +} + +// TestProducer_JobCancelSurvivesNotifierReconnect is a deterministic repro of +// https://github.com/riverqueue/river/issues/1358: a `NOTIFY` sent while the +// notifier is disconnected/reconnecting is lost (Postgres `NOTIFY` is +// fire-and-forget), so a job cancelled during that window previously ran to +// completion unaware. It forces exactly one simulated connection loss between +// the job starting and `JobCancel` being applied, holds the notifier at the +// start of its reconnect attempt until the cancellation has landed, then lets +// it finish reconnecting and asserts the job observes the cancellation. +func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + var ( + archetype = riversharedtest.BaseServiceArchetype(t) + dbPool = riversharedtest.DBPool(ctx, t) + driver = riverpgxv5.New(dbPool) + exec = driver.GetExecutor() + jobUpdates = make(chan []jobcompleter.CompleterJobUpdated, 10) + schema = riverdbtest.TestSchema(ctx, t, driver, nil) + queueName = fmt.Sprintf("test-reconnect-cancel-%05d", randutil.IntBetween(1, 100_000)) + pilot = &riverpilot.StandardPilot{} + ) + + completer := jobcompleter.NewInlineCompleter(archetype, schema, exec, pilot, jobUpdates) + require.NoError(t, completer.Start(ctx)) + t.Cleanup(completer.Stop) + + realListener := driver.GetListener(&riverdriver.GetListenenerParams{Schema: schema}) + mockListener := newProducerTestListenerMock(realListener) + + var ( + forceDisconnect atomic.Bool // set by the test to inject one simulated connection loss + reconnecting atomic.Bool // set once that loss has been injected; cleared once Connect is released + releaseReconnect = make(chan struct{}) + releaseReconnectOnce sync.Once + ) + closeReleaseReconnect := func() { releaseReconnectOnce.Do(func() { close(releaseReconnect) }) } + mockListener.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { + if forceDisconnect.CompareAndSwap(true, false) { + reconnecting.Store(true) + return nil, errors.New("simulated connection loss") + } + return realListener.WaitForNotification(ctx) + } + mockListener.connectFunc = func(ctx context.Context) error { + if reconnecting.Load() { + <-releaseReconnect + reconnecting.Store(false) + } + return realListener.Connect(ctx) + } + + notif := notifier.New(archetype, mockListener) + require.NoError(t, notif.Start(ctx)) + t.Cleanup(notif.Stop) + // Cleanups run LIFO, so registering this after notif.Stop makes it run + // first: an assertion failure anywhere in the test body must not leave + // notif.Stop() deadlocked waiting on a mocked Connect call that's + // holding the reconnect gate open forever. + t.Cleanup(closeReleaseReconnect) + + jobStarted := make(chan int64, 1) + workers := NewWorkers() + + type JobArgs struct { + testutil.JobArgsReflectKind[JobArgs] + } + + AddWorker(workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { + jobStarted <- job.ID + <-ctx.Done() + return ctx.Err() + })) + + prod := newProducer(archetype, exec, pilot, &producerConfig{ + ClientID: testClientID, + Completer: completer, + ErrorHandler: newTestErrorHandler(), + FetchCooldown: FetchCooldownDefault, + FetchPollInterval: 50 * time.Millisecond, + PluginLookupByJob: pluginlookup.NewJobPluginLookup(nil), + PluginLookupGlobal: pluginlookup.NewPluginLookup(nil), + JobTimeout: JobTimeoutDefault, + MaxWorkers: 10, + Notifier: notif, + Queue: queueName, + QueuePollInterval: queuePollIntervalDefault, + QueueReportInterval: queueReportIntervalDefault, + RetryPolicy: &DefaultClientRetryPolicy{}, + SchedulerInterval: riverinternaltest.SchedulerShortInterval, + Schema: schema, + StaleProducerRetentionPeriod: time.Minute, + Workers: workers, + }) + + config := newTestConfig(t, schema) + insertParams, err := insertParamsFromConfigArgsAndOptions(&prod.Archetype, config, &JobArgs{}, &InsertOpts{Queue: queueName}) + require.NoError(t, err) + _, err = exec.JobInsertFastMany(ctx, &riverdriver.JobInsertFastManyParams{ + Jobs: []*riverdriver.JobInsertFastParams{(*riverdriver.JobInsertFastParams)(insertParams)}, + Schema: schema, + }) + require.NoError(t, err) + + workCtx, workCancel := context.WithCancel(ctx) + + require.NoError(t, prod.StartWorkContext(ctx, workCtx)) + // Cleanups run LIFO: cancel workCtx (unblocking the worker) before + // prod.Stop tries to wait for it to exit, so cleanup can't deadlock + // regardless of whether the fix under test actually cancelled the job + // itself. + t.Cleanup(prod.Stop) + t.Cleanup(workCancel) + + jobID := riversharedtest.WaitOrTimeout(t, jobStarted) + + // Arm the injected disconnect, then wake the notifier's currently-blocked + // WaitForNotification call with a harmless notification on an unrelated + // queue so it loops back around and picks up the injected error + // immediately (rather than waiting on the notifier's own ping interval). + forceDisconnect.Store(true) + require.NoError(t, exec.NotifyMany(ctx, &riverdriver.NotifyManyParams{ + Topic: string(notifier.NotificationTopicInsert), + Payload: []string{`{"queue":"` + queueName + `-unrelated-wakeup"}`}, + Schema: schema, + })) + + // Wait until the notifier has actually observed the injected error and is + // blocked trying to reconnect. + require.Eventually(t, reconnecting.Load, 5*time.Second, 5*time.Millisecond, + "notifier never reached its reconnect attempt after the injected connection loss") + + // While the notifier is down, cancel the running job. The NOTIFY this + // sends is lost because nothing is LISTENing right now. + _, err = exec.JobCancel(ctx, &riverdriver.JobCancelParams{ + ID: jobID, + CancelAttemptedAt: time.Now().UTC(), + ControlTopic: string(notifier.NotificationTopicControl), + Schema: schema, + }) + require.NoError(t, err) + + // Now let the notifier finish reconnecting. + closeReleaseReconnect() + + update := riversharedtest.WaitOrTimeout(t, jobUpdates) + require.Equal(t, rivertype.JobStateCancelled, update[0].Job.State, + "job should have observed cancellation after the notifier reconnected and reconciled active jobs against cancel_attempted_at") +} + func TestProducer_MetricEmitHook(t *testing.T) { t.Parallel() From 376c5a379aa2065edd02a474a6a240d0c5d8054f Mon Sep 17 00:00:00 2001 From: Chris George Date: Sun, 23 Aug 2026 14:53:09 -0700 Subject: [PATCH 2/7] Deliver cancellation locally when the client has no notifier --- client.go | 73 +++++++++++++++++++++++++++--- client_test.go | 118 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 5 deletions(-) diff --git a/client.go b/client.go index e0abd8956..3bf0a32d4 100644 --- a/client.go +++ b/client.go @@ -1533,11 +1533,22 @@ func (c *Client[TTx]) JobCancel(ctx context.Context, jobID int64) (*rivertype.Jo return nil, err } - c.notifyProducerWithoutListenerQueueControlEvent(job.Queue, &controlEventPayload{ - Action: controlActionCancel, - JobID: job.ID, - Queue: job.Queue, - }) + // Only a running job has an in-process executor to interrupt (an + // available/scheduled/retryable job was cancelled directly above, and a + // finalized job needed no cancellation at all), so there's nothing for a + // non-running job to dispatch. This also matters for correctness, not + // just efficiency: it keeps a burst of cancellations against jobs that + // were never running (e.g. cancelling many freshly-inserted jobs before + // the client has even been started) from filling the producer's + // control-event channel, which TriggerQueueControlEvent below sends to + // unconditionally and would otherwise block this call indefinitely. + if job.State == rivertype.JobStateRunning { + c.notifyProducerOfCancelWithoutNotifier(job.Queue, &controlEventPayload{ + Action: controlActionCancel, + JobID: job.ID, + Queue: job.Queue, + }) + } return job, nil } @@ -3006,6 +3017,58 @@ func (c *Client[TTx]) notifyProducerWithoutListenerQueueControlEvent(queue strin } } +// Notifies an internal producer of a job cancellation so a job running in +// this same process observes it immediately, for clients that have no +// notifier to deliver it via listen/notify. +// +// This deliberately does NOT share notifyProducerWithoutListenerQueueControlEvent +// above despite the near-identical dispatch: the two helpers guard on +// different conditions for a reason, and unifying them would either reopen +// this gap or introduce new bugs in the other one. +// +// - This helper guards on c.notifier == nil (this client has no notifier, +// for any reason, including an explicit Config.PollOnly on an otherwise +// listener-capable driver). Queue control events guard on +// driver.SupportsListener() instead, because pause/resume/metadata +// changes already have an independent, documented convergence path in +// poll-only mode (producer.pollForSettingChanges, on QueuePollInterval) — +// using this same client.notifier == nil guard there would double-deliver +// metadata changes not deduplicated the way pause/resume are, and would +// panic on QueueUpdate's nil control event when metadata is empty. +// - Job cancellation has no such fallback: nothing else ever re-checks a +// running job's cancel_attempted_at in poll-only mode, so a lost signal +// here is only caught by JobRescuer's much longer stuck-job sweep. +// +// The dispatch below (via TriggerQueueControlEvent) sends unconditionally and +// blocks if the target producer's control-event channel is full — the same +// characteristic notifyProducerWithoutListenerQueueControlEvent above already +// has for genuinely listener-incapable drivers. The caller (JobCancel) is +// responsible for only invoking this for jobs it knows are actually running, +// both because there's nothing to dispatch for a job with no in-process +// executor and to avoid a burst of cancellations against non-running jobs +// (e.g. many freshly-inserted jobs cancelled before the client is started) +// filling that channel and blocking indefinitely. +// +// Should only ever be invoked *outside* a transaction. If invoked within a +// transaction, the producer wouldn't yet be able to access the state that +// triggered the notification because it's not committed yet. +func (c *Client[TTx]) notifyProducerOfCancelWithoutNotifier(queue string, controlEvent *controlEventPayload) { + if c.notifier != nil { + return + } + + c.producersMu.RLock() + defer c.producersMu.RUnlock() + + if len(c.producersByQueueName) < 1 { + return + } + + if producer, ok := c.producersByQueueName[queue]; ok { + producer.TriggerQueueControlEvent(controlEvent) + } +} + func (c *Client[TTx]) queueUpdate(ctx context.Context, executorTx riverdriver.ExecutorTx, name string, params *QueueUpdateParams) (*rivertype.Queue, *controlEventPayload, error) { updateMetadata := len(params.Metadata) > 0 diff --git a/client_test.go b/client_test.go index 8ee93b6b2..dbadae3e7 100644 --- a/client_test.go +++ b/client_test.go @@ -1465,6 +1465,112 @@ func Test_Client_Common(t *testing.T) { require.WithinDuration(t, time.Now(), *event.Job.FinalizedAt, 2*time.Second) }) + // Unlike CancelRunningJobPollOnly above (which uses a fake driver that + // reports SupportsListener() == false to simulate a driver that can't + // listen/notify at all), this uses the real pgx driver with an explicit + // Config.PollOnly, which is the actually-common "I don't want a notifier" + // case. driver.SupportsListener() is true here, so a same-process + // cancellation delivered via JobCancel() has nothing to do with the + // listen/notify path at all: it must go through the client's local, + // no-notifier dispatch instead. + t.Run("CancelRunningJobExplicitPollOnly", func(t *testing.T) { + t.Parallel() + + config, bundle := setupConfig(t) + config.PollOnly = true + + client, err := NewClient(bundle.driver, config) + require.NoError(t, err) + + jobStartedChan := make(chan int64) + + type JobArgs struct { + testutil.JobArgsReflectKind[JobArgs] + } + + AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { + jobStartedChan <- job.ID + <-ctx.Done() + return ctx.Err() + })) + + subscribeChan := subscribe(t, client) + startClient(ctx, t, client) + riversharedtest.WaitOrTimeout(t, client.baseStartStop.Started()) + + insertRes, err := client.Insert(ctx, &JobArgs{}, nil) + require.NoError(t, err) + + startedJobID := riversharedtest.WaitOrTimeout(t, jobStartedChan) + require.Equal(t, insertRes.Job.ID, startedJobID) + + updatedJob, err := client.JobCancel(ctx, insertRes.Job.ID) + require.NoError(t, err) + require.Equal(t, rivertype.JobStateRunning, updatedJob.State) + + event := riversharedtest.WaitOrTimeout(t, subscribeChan) + require.Equal(t, EventKindJobCancelled, event.Kind) + require.Equal(t, rivertype.JobStateCancelled, event.Job.State) + require.WithinDuration(t, time.Now(), *event.Job.FinalizedAt, 2*time.Second) + }) + + // A non-running job (available, scheduled, or retryable) is cancelled + // directly in the database and has no in-process executor to notify, so + // JobCancel must not dispatch a local control event for it. This matters + // beyond efficiency: notifyProducerOfCancelWithoutNotifier's dispatch + // blocks if the target producer's control-event channel (buffered to + // 100) fills up, which an unstarted (or otherwise not-yet-draining) + // producer never does. Cancelling more than that many never-started jobs + // against an explicit-PollOnly client (which has no notifier and so + // would otherwise take the local dispatch path) must not block. + t.Run("CancelManyNonRunningJobsExplicitPollOnlyDoesNotBlock", func(t *testing.T) { + t.Parallel() + + config, bundle := setupConfig(t) + config.PollOnly = true + + client, err := NewClient(bundle.driver, config) + require.NoError(t, err) + + type JobArgs struct { + testutil.JobArgsReflectKind[JobArgs] + } + + AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { + return nil + })) + + const numJobs = 101 // more than the producer's 100-slot control-event buffer + + jobIDs := make([]int64, numJobs) + for i := range jobIDs { + insertRes, err := client.Insert(ctx, &JobArgs{}, nil) + require.NoError(t, err) + jobIDs[i] = insertRes.Job.ID + } + + // Deliberately cancelling before the client is started: nothing is + // running yet, and nothing is consuming the (not-yet-existent) + // producer's control-event channel either. + cancelDone := make(chan error, 1) + go func() { + for _, jobID := range jobIDs { + if _, err := client.JobCancel(ctx, jobID); err != nil { + cancelDone <- err + return + } + } + cancelDone <- nil + }() + + select { + case err := <-cancelDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("cancelling non-running jobs blocked, presumably on the producer's control-event channel") + } + }) + t.Run("CancelScheduledJob", func(t *testing.T) { t.Parallel() @@ -1530,15 +1636,27 @@ func Test_Client_Common(t *testing.T) { testutil.JobArgsReflectKind[JobArgs] } + jobStartedChan := make(chan int64) + continueChan := make(chan struct{}) + AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { + jobStartedChan <- job.ID + <-continueChan return nil })) startClient(ctx, t, client) + t.Cleanup(func() { close(continueChan) }) insertRes, err := client.Insert(ctx, &JobArgs{}, nil) require.NoError(t, err) + // Only a running job dispatches a local control event (see + // notifyProducerOfCancelWithoutNotifier), so make sure the job has + // actually started before cancelling it. + startedJobID := riversharedtest.WaitOrTimeout(t, jobStartedChan) + require.Equal(t, insertRes.Job.ID, startedJobID) + _, err = client.JobCancel(ctx, insertRes.Job.ID) require.NoError(t, err) From 42fa20b0f9537e442eccee8c8fe2c16b2d4d6e84 Mon Sep 17 00:00:00 2001 From: Chris George Date: Tue, 20 Oct 2026 07:35:49 -0700 Subject: [PATCH 3/7] Address cancellation reconciliation review feedback Run reconnect reconciliation asynchronously with a bounded context and reject stale execution results. Keep local cancellation lifecycle-aware without blocking on shutdown locks, and clear old cancellation metadata on retry across drivers. Add deterministic regression coverage and shorten comments and changelog entries. --- client.go | 72 ++--- client_test.go | 249 ++++++++++++++++-- internal/notifier/notifier.go | 58 ++-- internal/notifier/notifier_test.go | 100 +++---- producer.go | 223 ++++++++++------ producer_test.go | 233 +++++++++++++--- .../internal/dbsqlc/river_job.sql.go | 1 + riverdriver/riverdrivertest/job_update.go | 9 +- .../riverpgxv5/internal/dbsqlc/river_job.sql | 1 + .../internal/dbsqlc/river_job.sql.go | 1 + .../riversqlite/internal/dbsqlc/river_job.sql | 1 + .../internal/dbsqlc/river_job.sql.go | 1 + 12 files changed, 673 insertions(+), 276 deletions(-) diff --git a/client.go b/client.go index 3bf0a32d4..9a642b535 100644 --- a/client.go +++ b/client.go @@ -1533,17 +1533,8 @@ func (c *Client[TTx]) JobCancel(ctx context.Context, jobID int64) (*rivertype.Jo return nil, err } - // Only a running job has an in-process executor to interrupt (an - // available/scheduled/retryable job was cancelled directly above, and a - // finalized job needed no cancellation at all), so there's nothing for a - // non-running job to dispatch. This also matters for correctness, not - // just efficiency: it keeps a burst of cancellations against jobs that - // were never running (e.g. cancelling many freshly-inserted jobs before - // the client has even been started) from filling the producer's - // control-event channel, which TriggerQueueControlEvent below sends to - // unconditionally and would otherwise block this call indefinitely. if job.State == rivertype.JobStateRunning { - c.notifyProducerOfCancelWithoutNotifier(job.Queue, &controlEventPayload{ + c.notifyProducerOfCancelWithoutNotifier(ctx, job.Queue, &controlEventPayload{ Action: controlActionCancel, JobID: job.ID, Queue: job.Queue, @@ -3017,55 +3008,36 @@ func (c *Client[TTx]) notifyProducerWithoutListenerQueueControlEvent(queue strin } } -// Notifies an internal producer of a job cancellation so a job running in -// this same process observes it immediately, for clients that have no -// notifier to deliver it via listen/notify. -// -// This deliberately does NOT share notifyProducerWithoutListenerQueueControlEvent -// above despite the near-identical dispatch: the two helpers guard on -// different conditions for a reason, and unifying them would either reopen -// this gap or introduce new bugs in the other one. -// -// - This helper guards on c.notifier == nil (this client has no notifier, -// for any reason, including an explicit Config.PollOnly on an otherwise -// listener-capable driver). Queue control events guard on -// driver.SupportsListener() instead, because pause/resume/metadata -// changes already have an independent, documented convergence path in -// poll-only mode (producer.pollForSettingChanges, on QueuePollInterval) — -// using this same client.notifier == nil guard there would double-deliver -// metadata changes not deduplicated the way pause/resume are, and would -// panic on QueueUpdate's nil control event when metadata is empty. -// - Job cancellation has no such fallback: nothing else ever re-checks a -// running job's cancel_attempted_at in poll-only mode, so a lost signal -// here is only caught by JobRescuer's much longer stuck-job sweep. -// -// The dispatch below (via TriggerQueueControlEvent) sends unconditionally and -// blocks if the target producer's control-event channel is full — the same -// characteristic notifyProducerWithoutListenerQueueControlEvent above already -// has for genuinely listener-incapable drivers. The caller (JobCancel) is -// responsible for only invoking this for jobs it knows are actually running, -// both because there's nothing to dispatch for a job with no in-process -// executor and to avoid a burst of cancellations against non-running jobs -// (e.g. many freshly-inserted jobs cancelled before the client is started) -// filling that channel and blocking indefinitely. -// -// Should only ever be invoked *outside* a transaction. If invoked within a -// transaction, the producer wouldn't yet be able to access the state that -// triggered the notification because it's not committed yet. -func (c *Client[TTx]) notifyProducerOfCancelWithoutNotifier(queue string, controlEvent *controlEventPayload) { +// Deliver cancellations locally when no notifier is configured, including PollOnly. +// Only call after commit so the producer can observe the cancellation metadata. +func (c *Client[TTx]) notifyProducerOfCancelWithoutNotifier(ctx context.Context, queue string, controlEvent *controlEventPayload) { if c.notifier != nil { return } c.producersMu.RLock() - defer c.producersMu.RUnlock() + producer := c.producersByQueueName[queue] + c.producersMu.RUnlock() + if producer == nil { + return + } - if len(c.producersByQueueName) < 1 { + state := producer.cancelState.Load() + if state == nil { return } + select { + case <-state.done: + return + default: + } - if producer, ok := c.producersByQueueName[queue]; ok { - producer.TriggerQueueControlEvent(controlEvent) + // cancelCh is also drained while a database fetch is in progress. + select { + case producer.cancelCh <- controlEvent.JobID: + producer.testSignals.QueueControlEventTriggered.Signal(controlEvent) + case <-ctx.Done(): + case <-state.done: } } diff --git a/client_test.go b/client_test.go index dbadae3e7..1372c60a3 100644 --- a/client_test.go +++ b/client_test.go @@ -1465,22 +1465,17 @@ func Test_Client_Common(t *testing.T) { require.WithinDuration(t, time.Now(), *event.Job.FinalizedAt, 2*time.Second) }) - // Unlike CancelRunningJobPollOnly above (which uses a fake driver that - // reports SupportsListener() == false to simulate a driver that can't - // listen/notify at all), this uses the real pgx driver with an explicit - // Config.PollOnly, which is the actually-common "I don't want a notifier" - // case. driver.SupportsListener() is true here, so a same-process - // cancellation delivered via JobCancel() has nothing to do with the - // listen/notify path at all: it must go through the client's local, - // no-notifier dispatch instead. + // Explicit PollOnly also bypasses notifications on listener-capable drivers. t.Run("CancelRunningJobExplicitPollOnly", func(t *testing.T) { t.Parallel() config, bundle := setupConfig(t) config.PollOnly = true + config.queuePollInterval = time.Hour client, err := NewClient(bundle.driver, config) require.NoError(t, err) + client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.Init(t) jobStartedChan := make(chan int64) @@ -1507,6 +1502,9 @@ func Test_Client_Common(t *testing.T) { updatedJob, err := client.JobCancel(ctx, insertRes.Job.ID) require.NoError(t, err) require.Equal(t, rivertype.JobStateRunning, updatedJob.State) + controlEvent := client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.WaitOrTimeout() + require.NotNil(t, controlEvent) + require.Equal(t, insertRes.Job.ID, controlEvent.JobID) event := riversharedtest.WaitOrTimeout(t, subscribeChan) require.Equal(t, EventKindJobCancelled, event.Kind) @@ -1514,15 +1512,66 @@ func Test_Client_Common(t *testing.T) { require.WithinDuration(t, time.Now(), *event.Job.FinalizedAt, 2*time.Second) }) - // A non-running job (available, scheduled, or retryable) is cancelled - // directly in the database and has no in-process executor to notify, so - // JobCancel must not dispatch a local control event for it. This matters - // beyond efficiency: notifyProducerOfCancelWithoutNotifier's dispatch - // blocks if the target producer's control-event channel (buffered to - // 100) fills up, which an unstarted (or otherwise not-yet-draining) - // producer never does. Cancelling more than that many never-started jobs - // against an explicit-PollOnly client (which has no notifier and so - // would otherwise take the local dispatch path) must not block. + t.Run("CancelRunningJobExplicitPollOnlyDuringGracefulStop", func(t *testing.T) { + t.Parallel() + + config, bundle := setupConfig(t) + config.PollOnly = true + config.queuePollInterval = time.Hour + + client, err := NewClient(bundle.driver, config) + require.NoError(t, err) + client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.Init(t) + + type JobArgs struct { + testutil.JobArgsReflectKind[JobArgs] + } + + var signals clientJobCancelTestSignals + signals.Init(t) + AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { + signals.JobStarted.Signal(job.ID) + <-ctx.Done() + return ctx.Err() + })) + + subscribeChan := subscribe(t, client) + require.NoError(t, client.Start(ctx)) + t.Cleanup(func() { + stopCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + require.NoError(t, client.StopAndCancel(stopCtx)) + }) + riversharedtest.WaitOrTimeout(t, client.baseStartStop.Started()) + + insertRes, err := client.Insert(ctx, &JobArgs{}, nil) + require.NoError(t, err) + require.Equal(t, insertRes.Job.ID, signals.JobStarted.WaitOrTimeout()) + + fetchDone := client.queues.fetchCtx.Done() + var stopped testsignal.TestSignal[error] + stopped.Init(t) + go func() { + stopCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + stopped.Signal(client.Stop(stopCtx)) + }() + riversharedtest.WaitOrTimeout(t, fetchDone) + + cancelCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + _, err = client.JobCancel(cancelCtx, insertRes.Job.ID) + require.NoError(t, err) + controlEvent := client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.WaitOrTimeout() + require.NotNil(t, controlEvent) + require.Equal(t, insertRes.Job.ID, controlEvent.JobID) + + event := riversharedtest.WaitOrTimeout(t, subscribeChan) + require.Equal(t, EventKindJobCancelled, event.Kind) + require.Equal(t, insertRes.Job.ID, event.Job.ID) + require.NoError(t, stopped.WaitOrTimeout()) + }) + t.Run("CancelManyNonRunningJobsExplicitPollOnlyDoesNotBlock", func(t *testing.T) { t.Parallel() @@ -1549,9 +1598,7 @@ func Test_Client_Common(t *testing.T) { jobIDs[i] = insertRes.Job.ID } - // Deliberately cancelling before the client is started: nothing is - // running yet, and nothing is consuming the (not-yet-existent) - // producer's control-event channel either. + // Producers exist before Start, but have no consumer yet. cancelDone := make(chan error, 1) go func() { for _, jobID := range jobIDs { @@ -1571,6 +1618,56 @@ func Test_Client_Common(t *testing.T) { } }) + for _, stopped := range []bool{false, true} { + name := "CancelManyRunningJobsExplicitPollOnlyBeforeStart" + if stopped { + name = "CancelManyRunningJobsExplicitPollOnlyAfterStop" + } + t.Run(name, func(t *testing.T) { + t.Parallel() + + config, bundle := setupConfig(t) + config.PollOnly = true + + client, err := NewClient(bundle.driver, config) + require.NoError(t, err) + + if stopped { + require.NoError(t, client.Start(ctx)) + riversharedtest.WaitOrTimeout(t, client.baseStartStop.Started()) + require.NoError(t, client.Stop(ctx)) + } + + producer := client.producersByQueueName[QueueDefault] + jobIDs := make([]int64, cap(producer.queueControlCh)+1) + for i := range jobIDs { + jobIDs[i] = testfactory.Job(ctx, t, bundle.driver.GetExecutor(), &testfactory.JobOpts{ + Schema: bundle.schema, + State: new(rivertype.JobStateRunning), + }).ID + } + + cancelCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + + var cancelled testsignal.TestSignal[error] + cancelled.Init(t) + go func() { + for _, jobID := range jobIDs { + if _, err := client.JobCancel(cancelCtx, jobID); err != nil { + cancelled.Signal(err) + return + } + } + cancelled.Signal(nil) + }() + + require.NoError(t, cancelled.WaitOrTimeout()) + require.Empty(t, producer.cancelCh) + require.Empty(t, producer.queueControlCh) + }) + } + t.Run("CancelScheduledJob", func(t *testing.T) { t.Parallel() @@ -1651,9 +1748,7 @@ func Test_Client_Common(t *testing.T) { insertRes, err := client.Insert(ctx, &JobArgs{}, nil) require.NoError(t, err) - // Only a running job dispatches a local control event (see - // notifyProducerOfCancelWithoutNotifier), so make sure the job has - // actually started before cancelling it. + // Wait for a running attempt before testing local cancellation delivery. startedJobID := riversharedtest.WaitOrTimeout(t, jobStartedChan) require.Equal(t, insertRes.Job.ID, startedJobID) @@ -2764,6 +2859,114 @@ func Test_Client_LeaderElectionDisabled(t *testing.T) { }) } +func Test_Client_NotifyProducerOfCancelWithoutNotifier(t *testing.T) { + t.Parallel() + + type testBundle struct { + client *Client[pgx.Tx] + producer *producer + stopped func() + } + + setup := func(t *testing.T) *testBundle { + t.Helper() + + targetProducer := &producer{cancelCh: make(chan int64, 1)} + ctx, cancel := context.WithCancel(context.Background()) + ctx, _, started, stopped := targetProducer.StartInit(ctx) + targetProducer.cancelState.Store(&producerCancelState{done: ctx.Done()}) + started() + targetProducer.cancelCh <- 123 + stop := sync.OnceFunc(func() { + cancel() + stopped() + }) + t.Cleanup(stop) + + return &testBundle{ + client: &Client[pgx.Tx]{ + producersByQueueName: map[string]*producer{QueueDefault: targetProducer}, + }, + producer: targetProducer, + stopped: stop, + } + } + + t.Run("FullBufferContextCancelled", func(t *testing.T) { + t.Parallel() + + bundle := setup(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var returned testsignal.TestSignal[struct{}] + returned.Init(t) + go func() { + bundle.client.notifyProducerOfCancelWithoutNotifier(ctx, QueueDefault, &controlEventPayload{}) + returned.Signal(struct{}{}) + }() + + returned.WaitOrTimeout() + require.Len(t, bundle.producer.cancelCh, 1) + }) + + t.Run("FullBufferProducerStopped", func(t *testing.T) { + t.Parallel() + + bundle := setup(t) + var returned testsignal.TestSignal[struct{}] + returned.Init(t) + go func() { + bundle.client.notifyProducerOfCancelWithoutNotifier(context.Background(), QueueDefault, &controlEventPayload{}) + returned.Signal(struct{}{}) + }() + bundle.stopped() + + returned.WaitOrTimeout() + require.Len(t, bundle.producer.cancelCh, 1) + }) + + t.Run("RunningProducer", func(t *testing.T) { + t.Parallel() + + bundle := setup(t) + <-bundle.producer.cancelCh + + bundle.client.notifyProducerOfCancelWithoutNotifier(context.Background(), QueueDefault, &controlEventPayload{ + Action: controlActionCancel, + JobID: 456, + Queue: QueueDefault, + }) + + require.Equal(t, int64(456), <-bundle.producer.cancelCh) + }) + + t.Run("StopInProgress", func(t *testing.T) { + t.Parallel() + + bundle := setup(t) + shouldStop, _, finalizeStop := bundle.producer.StopInit() + require.True(t, shouldStop) + defer func() { + bundle.stopped() + finalizeStop(true) + }() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + var returned testsignal.TestSignal[struct{}] + returned.Init(t) + go func() { + bundle.client.notifyProducerOfCancelWithoutNotifier(ctx, QueueDefault, &controlEventPayload{}) + returned.Signal(struct{}{}) + }() + + returned.WaitOrTimeout() + require.Len(t, bundle.producer.cancelCh, 1) + }) +} + func Test_Client_Stop_Common(t *testing.T) { t.Parallel() diff --git a/internal/notifier/notifier.go b/internal/notifier/notifier.go index 8fba56743..6bc5a7731 100644 --- a/internal/notifier/notifier.go +++ b/internal/notifier/notifier.go @@ -22,6 +22,21 @@ import ( "github.com/riverqueue/river/rivershared/util/timeoututil" ) +// ListenerReadyFunc runs after initial connection and each successful reconnect. +type ListenerReadyFunc func() + +// ListenerReadySubscription unregisters a listener-ready callback. +type ListenerReadySubscription struct { + notifier *Notifier +} + +// Unregister stops future callbacks; an invocation already in progress may finish. +func (s *ListenerReadySubscription) Unregister() { + s.notifier.mu.Lock() + defer s.notifier.mu.Unlock() + delete(s.notifier.listenerReadyFuncs, s) +} + type NotificationTopic string const ( @@ -97,39 +112,6 @@ type Notifier struct { waitCancel context.CancelFunc } -// ListenerReadyFunc is invoked each time the notifier establishes healthy -// listening on all of its currently subscribed topics. This includes the -// very first connect as well as every subsequent reconnect that follows a -// connection loss, so callers whose work is only meaningful after a genuine -// reconnect (as opposed to the initial connect) should make that check -// themselves. -type ListenerReadyFunc func() - -// ListenerReadySubscription is a handle to a func registered with -// RegisterListenerReadyFunc. Call Unregister to stop receiving invocations. -type ListenerReadySubscription struct { - notifier *Notifier -} - -func (s *ListenerReadySubscription) Unregister() { - s.notifier.mu.Lock() - defer s.notifier.mu.Unlock() - delete(s.notifier.listenerReadyFuncs, s) -} - -// RegisterListenerReadyFunc registers a function to be invoked every time the -// notifier (re)establishes healthy listening on all of its subscribed topics. -// It's used to let callers reconcile any state that could have missed a -// notification while the notifier was disconnected/reconnecting. -func (n *Notifier) RegisterListenerReadyFunc(listenerReadyFunc ListenerReadyFunc) *ListenerReadySubscription { - n.mu.Lock() - defer n.mu.Unlock() - - sub := &ListenerReadySubscription{notifier: n} - n.listenerReadyFuncs[sub] = listenerReadyFunc - return sub -} - func New(archetype *baseservice.Archetype, listener riverdriver.Listener) *Notifier { notifier := baseservice.Init(archetype, &Notifier{ listener: listener, @@ -142,6 +124,16 @@ func New(archetype *baseservice.Archetype, listener riverdriver.Listener) *Notif return notifier } +// RegisterListenerReadyFunc registers a callback for initial connection and reconnects. +func (n *Notifier) RegisterListenerReadyFunc(listenerReadyFunc ListenerReadyFunc) *ListenerReadySubscription { + n.mu.Lock() + defer n.mu.Unlock() + + sub := &ListenerReadySubscription{notifier: n} + n.listenerReadyFuncs[sub] = listenerReadyFunc + return sub +} + func (n *Notifier) Start(ctx context.Context) error { ctx, shouldStart, started, stopped := n.StartInit(ctx) if !shouldStart { diff --git a/internal/notifier/notifier_test.go b/internal/notifier/notifier_test.go index c36016f59..4af6927c3 100644 --- a/internal/notifier/notifier_test.go +++ b/internal/notifier/notifier_test.go @@ -19,6 +19,7 @@ import ( "github.com/riverqueue/river/riverdriver/riverpgxv5" "github.com/riverqueue/river/rivershared/riversharedtest" "github.com/riverqueue/river/rivershared/startstoptest" + "github.com/riverqueue/river/rivershared/testsignal" "github.com/riverqueue/river/rivershared/util/maputil" "github.com/riverqueue/river/rivershared/util/randutil" "github.com/riverqueue/river/rivershared/util/serviceutil" @@ -281,6 +282,57 @@ func TestNotifier(t *testing.T) { require.Equal(t, TopicAndPayload{testTopic1, "msg1"}, riversharedtest.WaitOrTimeout(t, notifyChan)) }) + t.Run("RegisterListenerReadyFunc", func(t *testing.T) { + t.Parallel() + + notifier, _ := setup(t, nil) + notifier.testDisableSleep = true + + var errorNum int + + listenerMock := NewListenerMock(notifier.listener) + listenerMock.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { + // Returns an error the first time, but then works after. + errorNum++ + switch errorNum { + case 1: + return nil, errors.New("error during wait") + default: + return listenerMock.Listener.WaitForNotification(ctx) + } + } + notifier.listener = listenerMock + + var ready testsignal.TestSignal[struct{}] + ready.Init(t) + sub := notifier.RegisterListenerReadyFunc(func() { ready.Signal(struct{}{}) }) + t.Cleanup(sub.Unregister) + + start(t, notifier) + + // Fires once on the initial connect. + ready.WaitOrTimeout() + + // And again once the notifier reconnects after the injected failure. + require.EqualError(t, notifier.testSignals.BackoffError.WaitOrTimeout(), "error during wait") + ready.WaitOrTimeout() + + notifier.Stop() + sub.Unregister() + + // WaitForNotification is entered after listener-ready callbacks finish. + var waiting testsignal.TestSignal[struct{}] + waiting.Init(t) + listenerMock.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { + waiting.Signal(struct{}{}) + return listenerMock.Listener.WaitForNotification(ctx) + } + + require.NoError(t, notifier.Start(ctx)) + waiting.WaitOrTimeout() + ready.RequireEmpty() + }) + t.Run("SingleTopicMultipleSubscribers", func(t *testing.T) { t.Parallel() @@ -623,54 +675,6 @@ func TestNotifier(t *testing.T) { // Subscription should still work. require.Equal(t, TopicAndPayload{testTopic1, "msg1"}, riversharedtest.WaitOrTimeout(t, notifyChan)) }) - - t.Run("RegisterListenerReadyFunc", func(t *testing.T) { - t.Parallel() - - notifier, _ := setup(t, nil) - notifier.testDisableSleep = true - - var errorNum int - - listenerMock := NewListenerMock(notifier.listener) - listenerMock.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { - // Returns an error the first time, but then works after. - errorNum++ - switch errorNum { - case 1: - return nil, errors.New("error during wait") - default: - return listenerMock.Listener.WaitForNotification(ctx) - } - } - notifier.listener = listenerMock - - readyChan := make(chan struct{}, 10) - sub := notifier.RegisterListenerReadyFunc(func() { readyChan <- struct{}{} }) - t.Cleanup(sub.Unregister) - - start(t, notifier) - - // Fires once on the initial connect. - riversharedtest.WaitOrTimeout(t, readyChan) - - // And again once the notifier reconnects after the injected failure. - require.EqualError(t, notifier.testSignals.BackoffError.WaitOrTimeout(), "error during wait") - riversharedtest.WaitOrTimeout(t, readyChan) - - // Unregister stops further invocations. There's no direct signal for - // "would have fired again", so drain what's pending, unregister, and - // confirm a subsequent restart doesn't add anything new. - for len(readyChan) > 0 { - <-readyChan - } - sub.Unregister() - - notifier.Stop() - require.NoError(t, notifier.Start(ctx)) - notifier.testSignals.ListeningBegin.WaitOrTimeout() - require.Empty(t, readyChan) - }) } func TestNotifier_Start(t *testing.T) { diff --git a/producer.go b/producer.go index 814cd2697..4b00d16a6 100644 --- a/producer.go +++ b/producer.go @@ -215,9 +215,10 @@ type producer struct { pilot riverpilot.Pilot workers *Workers - // Receives job IDs to cancel. Written by notifier and polling goroutines, - // only read from the main goroutine. - cancelCh chan int64 + // Receives job IDs from notifier, polling, and local cancellation calls. + // Only the main goroutine reads it. + cancelCh chan int64 + cancelState atomic.Pointer[producerCancelState] // Set to true when the producer thinks it should trigger another fetch as // soon as slots are available. This is written and read by the main @@ -241,19 +242,9 @@ type producer struct { // goroutine, only read from main goroutine. queueControlCh chan *controlEventPayload - // Receives a signal every time the notifier (re)establishes healthy - // listening. Written by the notifier goroutine (via a registered - // ListenerReadyFunc), only read from main goroutine. Used to reconcile - // activeJobs against cancel_attempted_at in case a cancel NOTIFY was lost - // while the notifier was disconnected/reconnecting. Buffered to 1 and - // written to non-blockingly because reconnects are rare and coalescing - // multiple signals into one reconciliation pass is fine. + // Coalesces reconnect notifications for reconciliation on the main goroutine. reconnectCh chan struct{} - // Only used by main goroutine. Tracks the current backoff attempt for - // retrying the reconnect reconciliation query after a failure. - reconcileAttempt int - retryPolicy ClientRetryPolicy testSignals producerTestSignals } @@ -425,14 +416,7 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { return err } - // Reconcile activeJobs against cancel_attempted_at every time the - // notifier (re)establishes healthy listening. This closes the window - // described in https://github.com/riverqueue/river/issues/1358: a - // cancel NOTIFY sent while the notifier was disconnected/reconnecting - // is otherwise lost for good (Postgres NOTIFY is fire-and-forget). - // Firing on the very first connect too is harmless: activeJobs is - // always empty at that point in startup, so the fetchAndRunLoop - // handler below is a no-op query-free check. + // Recover cancellations whose NOTIFY was lost while disconnected. reconnectSub = p.config.Notifier.RegisterListenerReadyFunc(func() { select { case p.reconnectCh <- struct{}{}: @@ -441,9 +425,13 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { }) } + subroutineCtx, cancelSubroutines := context.WithCancelCause(context.WithoutCancel(fetchCtx)) + p.cancelState.Store(&producerCancelState{done: subroutineCtx.Done()}) + go func() { started() defer stopped() // this defer should come first so it's last out + defer p.cancelState.Store(nil) p.Logger.DebugContext(fetchCtx, p.Name+": Run loop started", slog.String("queue", p.config.Queue), slog.Bool("paused", p.paused)) defer func() { @@ -463,7 +451,6 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { } var subroutineWG sync.WaitGroup - subroutineCtx, cancelSubroutines := context.WithCancelCause(context.WithoutCancel(fetchCtx)) subroutineWG.Add(1) go p.fetchPollLoop(subroutineCtx, &subroutineWG) @@ -595,6 +582,64 @@ func (p *producer) fetchAndRunLoop(fetchCtx, workCtx context.Context) { p.fetchLimiter.Call() fetchResultCh := make(chan producerFetchResult) + reconcileCtx, cancelReconcile := context.WithCancel(fetchCtx) + var reconcileWG sync.WaitGroup + defer func() { + cancelReconcile() + reconcileWG.Wait() + }() + + reconcileResultCh := make(chan producerReconcileResult) + var ( + reconcileAttempt int + reconcilePending bool + reconciling bool + retryTimer *time.Timer + ) + defer func() { + if retryTimer != nil { + retryTimer.Stop() + } + }() + startReconcile := func() { + if reconciling { + reconcilePending = true + return + } + if retryTimer != nil { + retryTimer.Stop() + retryTimer = nil + } + p.activeJobsMu.Lock() + numActiveJobs := len(p.activeJobs) + p.activeJobsMu.Unlock() + if numActiveJobs == 0 { + reconcileAttempt = 0 + return + } + reconciling = true + reconcileWG.Add(1) + p.handleNotifierReconnect(reconcileCtx, reconcileResultCh, &reconcileWG) + } + handleReconcileResult := func(result producerReconcileResult) { + reconciling = false + if result.err != nil { + p.Logger.ErrorContext(workCtx, p.Name+": Error reconciling cancellations; will retry", slog.String("err", result.err.Error())) + if retryTimer != nil { + retryTimer.Stop() + } + retryTimer = p.scheduleReconnectRetry(reconcileCtx, reconcileAttempt) + reconcileAttempt++ + } else { + reconcileAttempt = 0 + p.applyReconcileResult(workCtx, result) + } + if reconcilePending { + reconcilePending = false + startReconcile() + } + } + for { select { case <-fetchCtx.Done(): @@ -642,9 +687,11 @@ func (p *producer) fetchAndRunLoop(fetchCtx, workCtx context.Context) { case jobID := <-p.cancelCh: p.maybeCancelJob(workCtx, jobID) case <-p.reconnectCh: - p.handleNotifierReconnect(workCtx) + startReconcile() + case result := <-reconcileResultCh: + handleReconcileResult(result) case <-p.fetchLimiter.C(): - p.innerFetchLoop(workCtx, fetchResultCh) + p.innerFetchLoop(workCtx, fetchResultCh, reconcileResultCh, handleReconcileResult, startReconcile) // Ensure we can't start another fetch when fetchCtx is done, even if // the fetchLimiter is also ready to fire: select { @@ -698,7 +745,7 @@ func (p *producer) jitteredFetchPollInterval() time.Duration { return randutil.DurationBetween(p.config.FetchPollInterval, p.config.FetchPollInterval+jitterRange) } -func (p *producer) innerFetchLoop(workCtx context.Context, fetchResultCh chan producerFetchResult) { +func (p *producer) innerFetchLoop(workCtx context.Context, fetchResultCh chan producerFetchResult, reconcileResultCh <-chan producerReconcileResult, handleReconcileResult func(producerReconcileResult), startReconcile func()) { var limit int if p.paused { limit = 0 @@ -734,6 +781,10 @@ func (p *producer) innerFetchLoop(workCtx context.Context, fetchResultCh chan pr } } return + case <-p.reconnectCh: + startReconcile() + case result := <-reconcileResultCh: + handleReconcileResult(result) case result := <-p.jobResultCh: p.removeActiveJob(result) case jobID := <-p.cancelCh: @@ -824,6 +875,30 @@ func (p *producer) addActiveJob(id int64, executor *jobexecutor.JobExecutor) { p.activeJobsMu.Unlock() } +// applyReconcileResult runs on the producer goroutine and ignores replaced executions. +func (p *producer) applyReconcileResult(ctx context.Context, result producerReconcileResult) { + for _, job := range result.jobs { + p.activeJobsMu.Lock() + executor := p.activeJobs[job.ID] + p.activeJobsMu.Unlock() + if executor == nil || executor != result.executors[job.ID] || executor.JobRow.Attempt != job.Attempt { + continue + } + + var metadata struct { + CancelAttemptedAt time.Time `json:"cancel_attempted_at"` + } + if err := json.Unmarshal(job.Metadata, &metadata); err != nil { + p.Logger.ErrorContext(ctx, p.Name+": Error unmarshaling cancellation metadata", + slog.Int64("job_id", job.ID), slog.String("err", err.Error())) + continue + } + if !metadata.CancelAttemptedAt.IsZero() { + p.maybeCancelJob(ctx, job.ID) + } + } +} + func (p *producer) removeActiveJob(job *rivertype.JobRow) { p.activeJobsMu.Lock() executor := p.activeJobs[job.ID] @@ -876,73 +951,41 @@ func (p *producer) maybeCancelJob(ctx context.Context, id int64) bool { return true } -// jobMetadataWithCancelAttemptedAt mirrors the identical unexported struct in -// internal/maintenance/job_rescuer.go, which reads the same field for the -// same reason. -type jobMetadataWithCancelAttemptedAt struct { - CancelAttemptedAt time.Time `json:"cancel_attempted_at"` -} - -// handleNotifierReconnect reconciles activeJobs against cancel_attempted_at -// after the notifier has (re)established healthy listening. It exists to -// close the window described in -// https://github.com/riverqueue/river/issues/1358: a cancel NOTIFY sent while -// the notifier was disconnected/reconnecting is lost for good (Postgres -// NOTIFY is fire-and-forget), so a job cancelled during that window would -// otherwise run to completion unaware, undetected until JobRescuer's much -// longer stuck-job sweep. -// -// Only ever called from the main goroutine, so activeJobs can be read -// without a lock. -func (p *producer) handleNotifierReconnect(ctx context.Context) { - if len(p.activeJobs) == 0 { - p.reconcileAttempt = 0 - return - } - +// handleNotifierReconnect snapshots active executions before querying asynchronously. +func (p *producer) handleNotifierReconnect(ctx context.Context, resultCh chan<- producerReconcileResult, wg *sync.WaitGroup) { + p.activeJobsMu.Lock() + executors := make(map[int64]*jobexecutor.JobExecutor, len(p.activeJobs)) ids := make([]int64, 0, len(p.activeJobs)) - for id := range p.activeJobs { + for id, executor := range p.activeJobs { + executors[id] = executor ids = append(ids, id) } - jobs, err := p.exec.JobGetByIDMany(ctx, &riverdriver.JobGetByIDManyParams{ - ID: ids, - Schema: p.config.Schema, - }) - if err != nil { - p.Logger.ErrorContext(ctx, p.Name+": Error reconciling active jobs against cancel_attempted_at after notifier reconnect; will retry", - slog.String("err", err.Error())) - p.scheduleReconnectRetry(ctx) - return - } - p.reconcileAttempt = 0 + p.activeJobsMu.Unlock() - for _, job := range jobs { - var metadata jobMetadataWithCancelAttemptedAt - if err := json.Unmarshal(job.Metadata, &metadata); err != nil { - p.Logger.ErrorContext(ctx, p.Name+": Error unmarshaling job metadata while reconciling cancellations after notifier reconnect", - slog.Int64("job_id", job.ID), slog.String("err", err.Error())) - continue - } - if !metadata.CancelAttemptedAt.IsZero() { - p.maybeCancelJob(ctx, job.ID) + go func() { + defer wg.Done() + + jobs, err := timeoututil.WithTimeoutV(ctx, 10*time.Second, p.Name+".handleNotifierReconnect", func(ctx context.Context) ([]*rivertype.JobRow, error) { + return p.exec.JobGetByIDMany(ctx, &riverdriver.JobGetByIDManyParams{ + ID: ids, + Schema: p.config.Schema, + }) + }) + select { + case <-ctx.Done(): + case resultCh <- producerReconcileResult{err: err, executors: executors, jobs: jobs}: } - } + }() } -// scheduleReconnectRetry re-arms p.reconnectCh after a backoff so a -// reconciliation query that failed (e.g. a transient DB error) doesn't -// silently drop the only signal that a reconnect happened, which would -// recreate the lost-cancel window this mechanism exists to close. -func (p *producer) scheduleReconnectRetry(ctx context.Context) { - if ctx.Err() != nil { - return - } - - sleepDuration := serviceutil.ExponentialBackoff(p.reconcileAttempt, serviceutil.MaxAttemptsBeforeResetDefault) - p.reconcileAttempt++ - - time.AfterFunc(sleepDuration, func() { +// scheduleReconnectRetry retries a failed lookup without blocking the producer. +func (p *producer) scheduleReconnectRetry(ctx context.Context, attempt int) *time.Timer { + sleepDuration := serviceutil.ExponentialBackoff(attempt, serviceutil.MaxAttemptsBeforeResetDefault) + return time.AfterFunc(sleepDuration, func() { + if ctx.Err() != nil { + return + } select { case p.reconnectCh <- struct{}{}: default: @@ -1344,12 +1387,22 @@ func (p *producer) reportQueueStatusOnce(ctx context.Context) { p.testSignals.ReportedQueueStatus.Signal(struct{}{}) } +type producerCancelState struct { + done <-chan struct{} +} + type producerFetchResult struct { decodeErrors map[int64]error err error jobs []*rivertype.JobRow } +type producerReconcileResult struct { + err error + executors map[int64]*jobexecutor.JobExecutor + jobs []*rivertype.JobRow +} + type errorHandlerAdapter struct { errorHandler ErrorHandler } diff --git a/producer_test.go b/producer_test.go index d7a9eca24..c63e04325 100644 --- a/producer_test.go +++ b/producer_test.go @@ -14,11 +14,13 @@ import ( "github.com/stretchr/testify/require" "github.com/riverqueue/river/internal/jobcompleter" + "github.com/riverqueue/river/internal/jobexecutor" "github.com/riverqueue/river/internal/notifier" "github.com/riverqueue/river/internal/pluginlookup" "github.com/riverqueue/river/internal/rivercommon" "github.com/riverqueue/river/internal/riverinternaltest" "github.com/riverqueue/river/internal/riverinternaltest/sharedtx" + "github.com/riverqueue/river/internal/util/chanutil" "github.com/riverqueue/river/riverdbtest" "github.com/riverqueue/river/riverdriver" "github.com/riverqueue/river/riverdriver/riverpgxv5" @@ -77,6 +79,21 @@ func (p *beforeJobGetAvailablePilot) JobGetAvailable( return p.Pilot.JobGetAvailable(ctx, exec, state, params) } +type producerTestExecutor struct { + riverdriver.Executor + + jobGetAvailableFunc func(context.Context, *riverdriver.JobGetAvailableParams) (*riverdriver.JobGetAvailableResult, error) + jobGetByIDManyFunc func(context.Context, *riverdriver.JobGetByIDManyParams) ([]*rivertype.JobRow, error) +} + +func (e *producerTestExecutor) JobGetAvailable(ctx context.Context, params *riverdriver.JobGetAvailableParams) (*riverdriver.JobGetAvailableResult, error) { + return e.jobGetAvailableFunc(ctx, params) +} + +func (e *producerTestExecutor) JobGetByIDMany(ctx context.Context, params *riverdriver.JobGetByIDManyParams) ([]*rivertype.JobRow, error) { + return e.jobGetByIDManyFunc(ctx, params) +} + // undecodableKindPilot reports locked jobs of one kind as undecodable. Postgres' // column types don't allow a job row that can't be decoded, so this simulates // one. @@ -109,10 +126,7 @@ func (p *undecodableKindPilot) JobGetAvailable( return res, nil } -// producerTestListenerMock wraps a real riverdriver.Listener so tests can -// override individual operations. Unlike notifier.ListenerMock (unexported to -// the internal/notifier package), this lives in package river so it can be -// used to drive a real *notifier.Notifier from producer-level tests. +// producerTestListenerMock overrides operations on a real listener. type producerTestListenerMock struct { riverdriver.Listener @@ -134,14 +148,49 @@ func (l *producerTestListenerMock) WaitForNotification(ctx context.Context) (*ri return l.waitForNotificationFunc(ctx) } -// TestProducer_JobCancelSurvivesNotifierReconnect is a deterministic repro of -// https://github.com/riverqueue/river/issues/1358: a `NOTIFY` sent while the -// notifier is disconnected/reconnecting is lost (Postgres `NOTIFY` is -// fire-and-forget), so a job cancelled during that window previously ran to -// completion unaware. It forces exactly one simulated connection loss between -// the job starting and `JobCancel` being applied, holds the notifier at the -// start of its reconnect attempt until the cancellation has landed, then lets -// it finish reconnecting and asserts the job observes the cancellation. +func TestProducer_ApplyReconcileResult(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + attempt int + metadata string + replaced bool + wantCancel bool + }{ + {name: "Cancelled", attempt: 1, metadata: `{"cancel_attempted_at":"2026-10-20T00:00:00Z"}`, wantCancel: true}, + {name: "DifferentAttempt", attempt: 2, metadata: `{"cancel_attempted_at":"2026-10-20T00:00:00Z"}`}, + {name: "MalformedMetadata", attempt: 1, metadata: `{"cancel_attempted_at":true}`}, + {name: "NoCancellation", attempt: 1, metadata: `{}`}, + {name: "ReplacedExecutor", attempt: 1, metadata: `{"cancel_attempted_at":"2026-10-20T00:00:00Z"}`, replaced: true}, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + archetype := riversharedtest.BaseServiceArchetype(t) + ctx, cancel := context.WithCancelCause(context.Background()) + t.Cleanup(func() { cancel(nil) }) + executor := baseservice.Init(archetype, &jobexecutor.JobExecutor{CancelFunc: cancel, JobRow: &rivertype.JobRow{ID: 1, Attempt: 1}}) + snapshotExecutor := executor + if testCase.replaced { + snapshotExecutor = &jobexecutor.JobExecutor{} + } + prod := baseservice.Init(archetype, &producer{activeJobs: map[int64]*jobexecutor.JobExecutor{1: executor}}) + + prod.applyReconcileResult(context.Background(), producerReconcileResult{ + executors: map[int64]*jobexecutor.JobExecutor{1: snapshotExecutor}, + jobs: []*rivertype.JobRow{{ID: 1, Attempt: testCase.attempt, Metadata: []byte(testCase.metadata)}}, + }) + if testCase.wantCancel { + require.ErrorIs(t, context.Cause(ctx), rivertype.ErrJobCancelledRemotely) + } else { + require.NoError(t, ctx.Err()) + } + }) + } +} + +// A cancel NOTIFY lost during a disconnect must be recovered on reconnect. func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { t.Parallel() @@ -167,10 +216,12 @@ func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { var ( forceDisconnect atomic.Bool // set by the test to inject one simulated connection loss - reconnecting atomic.Bool // set once that loss has been injected; cleared once Connect is released + reconnecting atomic.Bool + reconnectStarted testsignal.TestSignal[struct{}] releaseReconnect = make(chan struct{}) releaseReconnectOnce sync.Once ) + reconnectStarted.Init(t) closeReleaseReconnect := func() { releaseReconnectOnce.Do(func() { close(releaseReconnect) }) } mockListener.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { if forceDisconnect.CompareAndSwap(true, false) { @@ -181,7 +232,12 @@ func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { } mockListener.connectFunc = func(ctx context.Context) error { if reconnecting.Load() { - <-releaseReconnect + reconnectStarted.Signal(struct{}{}) + select { + case <-ctx.Done(): + return ctx.Err() + case <-releaseReconnect: + } reconnecting.Store(false) } return realListener.Connect(ctx) @@ -190,13 +246,11 @@ func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { notif := notifier.New(archetype, mockListener) require.NoError(t, notif.Start(ctx)) t.Cleanup(notif.Stop) - // Cleanups run LIFO, so registering this after notif.Stop makes it run - // first: an assertion failure anywhere in the test body must not leave - // notif.Stop() deadlocked waiting on a mocked Connect call that's - // holding the reconnect gate open forever. + // Release a blocked Connect before stopping the notifier. t.Cleanup(closeReleaseReconnect) - jobStarted := make(chan int64, 1) + var jobStarted testsignal.TestSignal[int64] + jobStarted.Init(t) workers := NewWorkers() type JobArgs struct { @@ -204,7 +258,7 @@ func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { } AddWorker(workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { - jobStarted <- job.ID + jobStarted.Signal(job.ID) <-ctx.Done() return ctx.Err() })) @@ -242,19 +296,13 @@ func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { workCtx, workCancel := context.WithCancel(ctx) require.NoError(t, prod.StartWorkContext(ctx, workCtx)) - // Cleanups run LIFO: cancel workCtx (unblocking the worker) before - // prod.Stop tries to wait for it to exit, so cleanup can't deadlock - // regardless of whether the fix under test actually cancelled the job - // itself. + // Cancel workers before waiting for producer shutdown. t.Cleanup(prod.Stop) t.Cleanup(workCancel) - jobID := riversharedtest.WaitOrTimeout(t, jobStarted) + jobID := jobStarted.WaitOrTimeout() - // Arm the injected disconnect, then wake the notifier's currently-blocked - // WaitForNotification call with a harmless notification on an unrelated - // queue so it loops back around and picks up the injected error - // immediately (rather than waiting on the notifier's own ping interval). + // Wake the listener so it observes the injected disconnect. forceDisconnect.Store(true) require.NoError(t, exec.NotifyMany(ctx, &riverdriver.NotifyManyParams{ Topic: string(notifier.NotificationTopicInsert), @@ -262,13 +310,9 @@ func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { Schema: schema, })) - // Wait until the notifier has actually observed the injected error and is - // blocked trying to reconnect. - require.Eventually(t, reconnecting.Load, 5*time.Second, 5*time.Millisecond, - "notifier never reached its reconnect attempt after the injected connection loss") + reconnectStarted.WaitOrTimeout() - // While the notifier is down, cancel the running job. The NOTIFY this - // sends is lost because nothing is LISTENing right now. + // Cancel while no listener is connected. _, err = exec.JobCancel(ctx, &riverdriver.JobCancelParams{ ID: jobID, CancelAttemptedAt: time.Now().UTC(), @@ -277,7 +321,6 @@ func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { }) require.NoError(t, err) - // Now let the notifier finish reconnecting. closeReleaseReconnect() update := riversharedtest.WaitOrTimeout(t, jobUpdates) @@ -499,6 +542,124 @@ func TestProducer_PollOnly(t *testing.T) { }) } +func TestProducer_ReconcileDoesNotBlock(t *testing.T) { + t.Parallel() + + for _, testCase := range []struct { + name string + blockedFetch bool + cancelFromLookup bool + queryError bool + }{ + {name: "BlockedFetchCancelDelivery", blockedFetch: true}, + {name: "BlockedFetchReconcileResult", blockedFetch: true, cancelFromLookup: true}, + {name: "CancelDelivery"}, + {name: "ReconcileResult", cancelFromLookup: true}, + {name: "RetryAfterError", cancelFromLookup: true, queryError: true}, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + var signals struct { + FetchStarted testsignal.TestSignal[struct{}] + LoopStopped testsignal.TestSignal[struct{}] + QueryStarted testsignal.TestSignal[time.Duration] + } + signals.FetchStarted.Init(t) + signals.LoopStopped.Init(t) + signals.QueryStarted.Init(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + releaseFetch := make(chan struct{}) + var releaseOnce sync.Once + release := func() { releaseOnce.Do(func() { close(releaseFetch) }) } + defer release() + archetype := riversharedtest.BaseServiceArchetype(t) + jobCtx, cancelJob := context.WithCancelCause(ctx) + defer cancelJob(nil) + executor := baseservice.Init(archetype, &jobexecutor.JobExecutor{CancelFunc: cancelJob, JobRow: &rivertype.JobRow{ID: 1, Attempt: 1}}) + var queryAttempts int + exec := &producerTestExecutor{ + jobGetAvailableFunc: func(context.Context, *riverdriver.JobGetAvailableParams) (*riverdriver.JobGetAvailableResult, error) { + signals.FetchStarted.Signal(struct{}{}) + <-releaseFetch + return &riverdriver.JobGetAvailableResult{}, nil + }, + jobGetByIDManyFunc: func(ctx context.Context, _ *riverdriver.JobGetByIDManyParams) ([]*rivertype.JobRow, error) { + deadline, ok := ctx.Deadline() + if ok { + signals.QueryStarted.Signal(time.Until(deadline)) + } else { + signals.QueryStarted.Signal(0) + } + queryAttempts++ + if testCase.queryError && queryAttempts == 1 { + return nil, errors.New("temporary reconciliation failure") + } + if testCase.cancelFromLookup { + return []*rivertype.JobRow{{ID: 1, Attempt: 1, Metadata: []byte(`{"cancel_attempted_at":"2026-10-20T00:00:00Z"}`)}}, nil + } + <-ctx.Done() + return nil, ctx.Err() + }, + } + pilot := &riverpilot.StandardPilot{} + _, state, err := pilot.ProducerInit(ctx, exec, nil) + require.NoError(t, err) + prod := baseservice.Init(archetype, &producer{ + activeJobs: map[int64]*jobexecutor.JobExecutor{1: executor}, + cancelCh: make(chan int64, 1), + config: &producerConfig{MaxWorkers: 2}, + exec: exec, + fetchLimiter: chanutil.NewDebouncedChan(ctx, time.Millisecond, true), + jobResultCh: make(chan *rivertype.JobRow), + paused: !testCase.blockedFetch, + pilot: pilot, + state: state, + queueControlCh: make(chan *controlEventPayload), + reconnectCh: make(chan struct{}, 1), + }) + go func() { + prod.fetchAndRunLoop(ctx, ctx) + signals.LoopStopped.Signal(struct{}{}) + }() + defer func() { + release() + cancel() + signals.LoopStopped.WaitOrTimeout() + }() + + if testCase.blockedFetch { + signals.FetchStarted.WaitOrTimeout() + } + prod.reconnectCh <- struct{}{} + queryTimeout := signals.QueryStarted.WaitOrTimeout() + require.Positive(t, queryTimeout) + require.LessOrEqual(t, queryTimeout, 10*time.Second) + if testCase.queryError { + require.Positive(t, signals.QueryStarted.WaitOrTimeout()) + } + + if !testCase.cancelFromLookup { + prod.cancelCh <- 1 + } + select { + case <-jobCtx.Done(): + require.ErrorIs(t, context.Cause(jobCtx), rivertype.ErrJobCancelledRemotely) + case <-time.After(time.Second): + t.Fatal("blocked reconciliation prevented cancellation delivery") + } + + select { + case prod.jobResultCh <- &rivertype.JobRow{ID: 1}: + case <-time.After(time.Second): + t.Fatal("blocked reconciliation prevented job completion") + } + }) + } +} + func TestProducer_WithNotifier(t *testing.T) { t.Parallel() diff --git a/riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go b/riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go index 4bd6cbfab..53dae3d6f 100644 --- a/riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go +++ b/riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go @@ -1320,6 +1320,7 @@ updated_job AS ( state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + metadata = metadata - 'cancel_attempted_at', scheduled_at = coalesce($2::timestamptz, now()) FROM job_to_update WHERE river_job.id = job_to_update.id diff --git a/riverdriver/riverdrivertest/job_update.go b/riverdriver/riverdrivertest/job_update.go index 150445d63..1346c5008 100644 --- a/riverdriver/riverdrivertest/job_update.go +++ b/riverdriver/riverdrivertest/job_update.go @@ -516,8 +516,10 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT exec, _ := setup(ctx, t) + metadata := []byte(`{"cancel_attempted_at":"2026-01-01T00:00:00Z","custom":"preserved"}`) job := testfactory.Job(ctx, t, exec, &testfactory.JobOpts{ - State: new(rivertype.JobStateRunning), + Metadata: metadata, + State: new(rivertype.JobStateRunning), }) jobAfter, err := exec.JobRetry(ctx, &riverdriver.JobRetryParams{ @@ -525,11 +527,13 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT }) require.NoError(t, err) require.Equal(t, rivertype.JobStateRunning, jobAfter.State) + require.JSONEq(t, string(metadata), string(jobAfter.Metadata)) require.WithinDuration(t, job.ScheduledAt, jobAfter.ScheduledAt, time.Microsecond) jobUpdated, err := exec.JobGetByID(ctx, &riverdriver.JobGetByIDParams{ID: job.ID}) require.NoError(t, err) require.Equal(t, rivertype.JobStateRunning, jobUpdated.State) + require.JSONEq(t, string(metadata), string(jobUpdated.Metadata)) }) for _, state := range []rivertype.JobState{ @@ -561,6 +565,7 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT job := testfactory.Job(ctx, t, exec, &testfactory.JobOpts{ FinalizedAt: finalizedAt, + Metadata: []byte(`{"cancel_attempted_at":"2026-01-01T00:00:00Z","custom":"preserved"}`), ScheduledAt: new(now.Add(1 * time.Hour)), State: &state, }) @@ -571,12 +576,14 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT }) require.NoError(t, err) require.Equal(t, rivertype.JobStateAvailable, jobAfter.State) + require.JSONEq(t, `{"custom":"preserved"}`, string(jobAfter.Metadata)) require.WithinDuration(t, now, jobAfter.ScheduledAt, bundle.driver.TimePrecision()) jobUpdated, err := exec.JobGetByID(ctx, &riverdriver.JobGetByIDParams{ID: job.ID}) require.NoError(t, err) require.Equal(t, rivertype.JobStateAvailable, jobUpdated.State) require.Nil(t, jobUpdated.FinalizedAt) + require.JSONEq(t, `{"custom":"preserved"}`, string(jobUpdated.Metadata)) }) } diff --git a/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql b/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql index d87361114..6b11a3bc5 100644 --- a/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql +++ b/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql @@ -533,6 +533,7 @@ updated_job AS ( state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + metadata = metadata - 'cancel_attempted_at', scheduled_at = coalesce(sqlc.narg('now')::timestamptz, now()) FROM job_to_update WHERE river_job.id = job_to_update.id diff --git a/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql.go b/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql.go index a58f06ff0..57bf72f8b 100644 --- a/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql.go +++ b/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql.go @@ -1284,6 +1284,7 @@ updated_job AS ( state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + metadata = metadata - 'cancel_attempted_at', scheduled_at = coalesce($2::timestamptz, now()) FROM job_to_update WHERE river_job.id = job_to_update.id diff --git a/riverdriver/riversqlite/internal/dbsqlc/river_job.sql b/riverdriver/riversqlite/internal/dbsqlc/river_job.sql index 8ef471786..3380dc0dd 100644 --- a/riverdriver/riversqlite/internal/dbsqlc/river_job.sql +++ b/riverdriver/riversqlite/internal/dbsqlc/river_job.sql @@ -588,6 +588,7 @@ SET state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + metadata = jsonb_remove(metadata, '$.cancel_attempted_at'), scheduled_at = coalesce(cast(sqlc.narg('now') AS text), datetime('now', 'subsec')) WHERE id = @id -- Do not touch running jobs: diff --git a/riverdriver/riversqlite/internal/dbsqlc/river_job.sql.go b/riverdriver/riversqlite/internal/dbsqlc/river_job.sql.go index 8fc19993d..2a7472f1d 100644 --- a/riverdriver/riversqlite/internal/dbsqlc/river_job.sql.go +++ b/riverdriver/riversqlite/internal/dbsqlc/river_job.sql.go @@ -1452,6 +1452,7 @@ SET state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + metadata = jsonb_remove(metadata, '$.cancel_attempted_at'), scheduled_at = coalesce(cast(?1 AS text), datetime('now', 'subsec')) WHERE id = ?2 -- Do not touch running jobs: From f04afe471cabb0630fe2c2108d7ff8a535f0e21f Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 30 Oct 2026 14:54:55 -0500 Subject: [PATCH 4/7] =?UTF-8?q?=F0=9F=A4=96=20[go,=20java,=20js,=20rust]?= =?UTF-8?q?=20Replace=20reconnect=20reconciliation=20with=20a=20cancellati?= =?UTF-8?q?on=20poll=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `JobCancel` delivers a cancellation to a running job with a `NOTIFY`, which is lost if the client's notifier isn't listening when it's sent, like while it's reconnecting. The job then keeps running until the rescuer finds it, an hour later by default. Rather than recover after reconnects, reuse a mechanism River already has. Since Yugabyte support (#1347), producers in poll-only mode run `pollForJobCancellations`, which looks up the producer's own running job IDs by primary key and cancels any with `cancel_attempted_at` set. It runs in its own goroutine, keeps running while jobs drain during shutdown, and hands cancellations to the existing `cancelCh`. Clients with a notifier now run it too, once a minute instead of every two seconds. This matches how River already treats insert notifications: producers poll for jobs every second even while listening. Cancellation was the only notification-driven path without a polling fallback. Notifications still deliver cancellations right away, and the poll only matters when one is lost. It runs on a timer rather than reacting to disconnects, because a producer can't tell that a notification was lost, so a lost cancellation arrives within a minute. It reads through the normal pool, so it works even while the listener is down. It also recovers notifications dropped for reasons other than reconnects, like a full notifier or producer buffer. It costs one primary key lookup a minute per producer with running jobs, and nothing for idle producers. Cancelling a job that's already cancelled does nothing, so finding it again is harmless. This replaces the notifier recovery hooks, execution identity in notification payloads, the reconnect reconciliation and its retries, tracking of claimed jobs whose executors aren't registered yet (the next poll finds them), and keeping the notifier running through shutdown. Local delivery for poll-only clients isn't needed because polling covers those clients on master. A check triggered by reconnects would avoid the steady query, but it needs a notifier hook in every language and misses losses that aren't reconnects. Retrying a job now clears an earlier attempt's `cancel_attempted_at` so that a retried job isn't cancelled again by the poll, by Java's check when a job starts, or by JavaScript's check after a reconnect. Ruby already did this. A poll result could still reach a newer attempt of the same job if it's retried and claimed again by the same producer within one query's round trip. A `NOTIFY` has the same race today. Rust gets the same once-a-minute check. JavaScript already rechecks running jobs after its notification stream reconnects, and Java and Ruby already poll while notifications are on, so Java and JavaScript change only to clear the marker on retry. --- CHANGELOG.md | 2 +- client.go | 53 +-- client_test.go | 321 ---------------- internal/notifier/notifier.go | 51 +-- internal/notifier/notifier_test.go | 52 --- java/CHANGELOG.md | 4 + .../resources/com/riverqueue/postgres.sql | 2 +- .../main/resources/com/riverqueue/sqlite.sql | 2 +- .../test/java/com/riverqueue/RetryTest.java | 19 + js/CHANGELOG.md | 4 + js/driver/pg/src/driver.integration.test.ts | 13 + js/driver/pg/src/sql/jobs.ts | 3 + js/driver/sqlite/src/compatibility.test.ts | 3 +- js/driver/sqlite/src/driver.test.ts | 5 +- js/driver/sqlite/src/driver.ts | 3 + producer.go | 220 ++--------- producer_test.go | 362 +----------------- .../internal/dbsqlc/river_job.sql.go | 2 + riverdriver/riverdrivertest/job_update.go | 33 +- .../riverpgxv5/internal/dbsqlc/river_job.sql | 2 + .../internal/dbsqlc/river_job.sql.go | 2 + .../riversqlite/internal/dbsqlc/river_job.sql | 2 + .../internal/dbsqlc/river_job.sql.go | 2 + rust/CHANGELOG.md | 4 + rust/riverqueue/src/__private.rs | 13 + rust/riverqueue/src/client/builder.rs | 2 + rust/riverqueue/src/client/mod.rs | 8 + rust/riverqueue/src/client/producer.rs | 17 +- rust/riverqueue/src/database/sqlite.rs | 1 + rust/riverqueue/src/storage/postgres.rs | 3 +- rust/riverqueue/tests/client_handles.rs | 24 ++ .../tests/poll_only_cancellation.rs | 127 +++++- 32 files changed, 337 insertions(+), 1024 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7582f2680..d56944ee8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,10 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fixed a running job not being cancelled after `Client.JobCancel` when the cancellation notification was lost, as happens if it's sent while the client's notifier is reconnecting. Clients with a notifier now also check their running jobs for cancellation requests once a minute. `Client.JobRetry` now clears an earlier attempt's cancellation request so that the retried job isn't cancelled again. [Issue #1358](https://github.com/riverqueue/river/issues/1358). - Fixed `Client.InsertManyFast` panicking after its transaction committed on a client configured with queues when the driver doesn't support a listener, such as a `riverdatabasesql` driver created with `New` instead of `NewWithPgxListener`. It now wakes producers for the inserted jobs' queues right away, like `Client.InsertMany`. [PR #1558](https://github.com/riverqueue/river/pull/1558). - Fixed `Client.QueueUpdate` without metadata crashing the program when the client works that queue and the driver doesn't support a listener. [PR #1558](https://github.com/riverqueue/river/pull/1558). - Fixed the notifier's run loop blocking forever when a topic `LISTEN` or `UNLISTEN` handed to it finished after its caller had stopped waiting (after 5 seconds, with `timed out waiting for interrupt operation`), as can happen on a slow or unresponsive connection. `Client.Stop` then never completed, and neither did a `Client.Start` that failed this way. [PR #1559](https://github.com/riverqueue/river/pull/1559). -- A `JobCancel()` sent while a client's notifier is disconnected/reconnecting is no longer lost. Previously, the cancellation's `NOTIFY` could commit while nothing was `LISTEN`ing (Postgres `NOTIFY` is fire-and-forget), leaving the running job unaware of the cancellation until `JobRescuer`'s much longer stuck-job sweep. Each client now reconciles its currently running jobs against `cancel_attempted_at` by primary key every time its notifier (re)establishes healthy listening, closing the gap with no added steady-state query load. [PR #1361](https://github.com/riverqueue/river/pull/1361), as discussed in [#1358](https://github.com/riverqueue/river/issues/1358). ## [0.50.0] - 2026-10-26 diff --git a/client.go b/client.go index 9a642b535..0cf0b4e3c 100644 --- a/client.go +++ b/client.go @@ -1504,7 +1504,9 @@ func (c *Client[TTx]) Driver() riverdriver.Driver[TTx] { // If the job is currently running, it is not immediately cancelled, but is // instead marked for cancellation. The client running the job will also be // notified to cancel the running job's context. When running without a notifier, -// clients poll for cancellation requests every two seconds. Although the job's +// clients poll for cancellation requests every two seconds. Clients with a +// notifier also poll once a minute in case a notification was missed, like +// while the notifier was reconnecting. Although the job's // context will be cancelled, since Go does not provide a mechanism to // interrupt a running goroutine the job will continue running until it returns. // As always, it is important for workers to respect context cancellation and @@ -1533,13 +1535,11 @@ func (c *Client[TTx]) JobCancel(ctx context.Context, jobID int64) (*rivertype.Jo return nil, err } - if job.State == rivertype.JobStateRunning { - c.notifyProducerOfCancelWithoutNotifier(ctx, job.Queue, &controlEventPayload{ - Action: controlActionCancel, - JobID: job.ID, - Queue: job.Queue, - }) - } + c.notifyProducerWithoutListenerQueueControlEvent(job.Queue, &controlEventPayload{ + Action: controlActionCancel, + JobID: job.ID, + Queue: job.Queue, + }) return job, nil } @@ -1563,7 +1563,9 @@ func (c *Client[TTx]) JobCancel(ctx context.Context, jobID int64) (*rivertype.Jo // If the job is currently running, it is not immediately cancelled, but is // instead marked for cancellation. The client running the job will also be // notified to cancel the running job's context. When running without a notifier, -// clients poll for cancellation requests every two seconds. Although the job's +// clients poll for cancellation requests every two seconds. Clients with a +// notifier also poll once a minute in case a notification was missed, like +// while the notifier was reconnecting. Although the job's // context will be cancelled, since Go does not provide a mechanism to // interrupt a running goroutine the job will continue running until it returns. // As always, it is important for workers to respect context cancellation and @@ -3008,39 +3010,6 @@ func (c *Client[TTx]) notifyProducerWithoutListenerQueueControlEvent(queue strin } } -// Deliver cancellations locally when no notifier is configured, including PollOnly. -// Only call after commit so the producer can observe the cancellation metadata. -func (c *Client[TTx]) notifyProducerOfCancelWithoutNotifier(ctx context.Context, queue string, controlEvent *controlEventPayload) { - if c.notifier != nil { - return - } - - c.producersMu.RLock() - producer := c.producersByQueueName[queue] - c.producersMu.RUnlock() - if producer == nil { - return - } - - state := producer.cancelState.Load() - if state == nil { - return - } - select { - case <-state.done: - return - default: - } - - // cancelCh is also drained while a database fetch is in progress. - select { - case producer.cancelCh <- controlEvent.JobID: - producer.testSignals.QueueControlEventTriggered.Signal(controlEvent) - case <-ctx.Done(): - case <-state.done: - } -} - func (c *Client[TTx]) queueUpdate(ctx context.Context, executorTx riverdriver.ExecutorTx, name string, params *QueueUpdateParams) (*rivertype.Queue, *controlEventPayload, error) { updateMetadata := len(params.Metadata) > 0 diff --git a/client_test.go b/client_test.go index 1372c60a3..8ee93b6b2 100644 --- a/client_test.go +++ b/client_test.go @@ -1465,209 +1465,6 @@ func Test_Client_Common(t *testing.T) { require.WithinDuration(t, time.Now(), *event.Job.FinalizedAt, 2*time.Second) }) - // Explicit PollOnly also bypasses notifications on listener-capable drivers. - t.Run("CancelRunningJobExplicitPollOnly", func(t *testing.T) { - t.Parallel() - - config, bundle := setupConfig(t) - config.PollOnly = true - config.queuePollInterval = time.Hour - - client, err := NewClient(bundle.driver, config) - require.NoError(t, err) - client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.Init(t) - - jobStartedChan := make(chan int64) - - type JobArgs struct { - testutil.JobArgsReflectKind[JobArgs] - } - - AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { - jobStartedChan <- job.ID - <-ctx.Done() - return ctx.Err() - })) - - subscribeChan := subscribe(t, client) - startClient(ctx, t, client) - riversharedtest.WaitOrTimeout(t, client.baseStartStop.Started()) - - insertRes, err := client.Insert(ctx, &JobArgs{}, nil) - require.NoError(t, err) - - startedJobID := riversharedtest.WaitOrTimeout(t, jobStartedChan) - require.Equal(t, insertRes.Job.ID, startedJobID) - - updatedJob, err := client.JobCancel(ctx, insertRes.Job.ID) - require.NoError(t, err) - require.Equal(t, rivertype.JobStateRunning, updatedJob.State) - controlEvent := client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.WaitOrTimeout() - require.NotNil(t, controlEvent) - require.Equal(t, insertRes.Job.ID, controlEvent.JobID) - - event := riversharedtest.WaitOrTimeout(t, subscribeChan) - require.Equal(t, EventKindJobCancelled, event.Kind) - require.Equal(t, rivertype.JobStateCancelled, event.Job.State) - require.WithinDuration(t, time.Now(), *event.Job.FinalizedAt, 2*time.Second) - }) - - t.Run("CancelRunningJobExplicitPollOnlyDuringGracefulStop", func(t *testing.T) { - t.Parallel() - - config, bundle := setupConfig(t) - config.PollOnly = true - config.queuePollInterval = time.Hour - - client, err := NewClient(bundle.driver, config) - require.NoError(t, err) - client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.Init(t) - - type JobArgs struct { - testutil.JobArgsReflectKind[JobArgs] - } - - var signals clientJobCancelTestSignals - signals.Init(t) - AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { - signals.JobStarted.Signal(job.ID) - <-ctx.Done() - return ctx.Err() - })) - - subscribeChan := subscribe(t, client) - require.NoError(t, client.Start(ctx)) - t.Cleanup(func() { - stopCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - require.NoError(t, client.StopAndCancel(stopCtx)) - }) - riversharedtest.WaitOrTimeout(t, client.baseStartStop.Started()) - - insertRes, err := client.Insert(ctx, &JobArgs{}, nil) - require.NoError(t, err) - require.Equal(t, insertRes.Job.ID, signals.JobStarted.WaitOrTimeout()) - - fetchDone := client.queues.fetchCtx.Done() - var stopped testsignal.TestSignal[error] - stopped.Init(t) - go func() { - stopCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - stopped.Signal(client.Stop(stopCtx)) - }() - riversharedtest.WaitOrTimeout(t, fetchDone) - - cancelCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - _, err = client.JobCancel(cancelCtx, insertRes.Job.ID) - require.NoError(t, err) - controlEvent := client.producersByQueueName[QueueDefault].testSignals.QueueControlEventTriggered.WaitOrTimeout() - require.NotNil(t, controlEvent) - require.Equal(t, insertRes.Job.ID, controlEvent.JobID) - - event := riversharedtest.WaitOrTimeout(t, subscribeChan) - require.Equal(t, EventKindJobCancelled, event.Kind) - require.Equal(t, insertRes.Job.ID, event.Job.ID) - require.NoError(t, stopped.WaitOrTimeout()) - }) - - t.Run("CancelManyNonRunningJobsExplicitPollOnlyDoesNotBlock", func(t *testing.T) { - t.Parallel() - - config, bundle := setupConfig(t) - config.PollOnly = true - - client, err := NewClient(bundle.driver, config) - require.NoError(t, err) - - type JobArgs struct { - testutil.JobArgsReflectKind[JobArgs] - } - - AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { - return nil - })) - - const numJobs = 101 // more than the producer's 100-slot control-event buffer - - jobIDs := make([]int64, numJobs) - for i := range jobIDs { - insertRes, err := client.Insert(ctx, &JobArgs{}, nil) - require.NoError(t, err) - jobIDs[i] = insertRes.Job.ID - } - - // Producers exist before Start, but have no consumer yet. - cancelDone := make(chan error, 1) - go func() { - for _, jobID := range jobIDs { - if _, err := client.JobCancel(ctx, jobID); err != nil { - cancelDone <- err - return - } - } - cancelDone <- nil - }() - - select { - case err := <-cancelDone: - require.NoError(t, err) - case <-time.After(5 * time.Second): - t.Fatal("cancelling non-running jobs blocked, presumably on the producer's control-event channel") - } - }) - - for _, stopped := range []bool{false, true} { - name := "CancelManyRunningJobsExplicitPollOnlyBeforeStart" - if stopped { - name = "CancelManyRunningJobsExplicitPollOnlyAfterStop" - } - t.Run(name, func(t *testing.T) { - t.Parallel() - - config, bundle := setupConfig(t) - config.PollOnly = true - - client, err := NewClient(bundle.driver, config) - require.NoError(t, err) - - if stopped { - require.NoError(t, client.Start(ctx)) - riversharedtest.WaitOrTimeout(t, client.baseStartStop.Started()) - require.NoError(t, client.Stop(ctx)) - } - - producer := client.producersByQueueName[QueueDefault] - jobIDs := make([]int64, cap(producer.queueControlCh)+1) - for i := range jobIDs { - jobIDs[i] = testfactory.Job(ctx, t, bundle.driver.GetExecutor(), &testfactory.JobOpts{ - Schema: bundle.schema, - State: new(rivertype.JobStateRunning), - }).ID - } - - cancelCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - - var cancelled testsignal.TestSignal[error] - cancelled.Init(t) - go func() { - for _, jobID := range jobIDs { - if _, err := client.JobCancel(cancelCtx, jobID); err != nil { - cancelled.Signal(err) - return - } - } - cancelled.Signal(nil) - }() - - require.NoError(t, cancelled.WaitOrTimeout()) - require.Empty(t, producer.cancelCh) - require.Empty(t, producer.queueControlCh) - }) - } - t.Run("CancelScheduledJob", func(t *testing.T) { t.Parallel() @@ -1733,25 +1530,15 @@ func Test_Client_Common(t *testing.T) { testutil.JobArgsReflectKind[JobArgs] } - jobStartedChan := make(chan int64) - continueChan := make(chan struct{}) - AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { - jobStartedChan <- job.ID - <-continueChan return nil })) startClient(ctx, t, client) - t.Cleanup(func() { close(continueChan) }) insertRes, err := client.Insert(ctx, &JobArgs{}, nil) require.NoError(t, err) - // Wait for a running attempt before testing local cancellation delivery. - startedJobID := riversharedtest.WaitOrTimeout(t, jobStartedChan) - require.Equal(t, insertRes.Job.ID, startedJobID) - _, err = client.JobCancel(ctx, insertRes.Job.ID) require.NoError(t, err) @@ -2859,114 +2646,6 @@ func Test_Client_LeaderElectionDisabled(t *testing.T) { }) } -func Test_Client_NotifyProducerOfCancelWithoutNotifier(t *testing.T) { - t.Parallel() - - type testBundle struct { - client *Client[pgx.Tx] - producer *producer - stopped func() - } - - setup := func(t *testing.T) *testBundle { - t.Helper() - - targetProducer := &producer{cancelCh: make(chan int64, 1)} - ctx, cancel := context.WithCancel(context.Background()) - ctx, _, started, stopped := targetProducer.StartInit(ctx) - targetProducer.cancelState.Store(&producerCancelState{done: ctx.Done()}) - started() - targetProducer.cancelCh <- 123 - stop := sync.OnceFunc(func() { - cancel() - stopped() - }) - t.Cleanup(stop) - - return &testBundle{ - client: &Client[pgx.Tx]{ - producersByQueueName: map[string]*producer{QueueDefault: targetProducer}, - }, - producer: targetProducer, - stopped: stop, - } - } - - t.Run("FullBufferContextCancelled", func(t *testing.T) { - t.Parallel() - - bundle := setup(t) - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - var returned testsignal.TestSignal[struct{}] - returned.Init(t) - go func() { - bundle.client.notifyProducerOfCancelWithoutNotifier(ctx, QueueDefault, &controlEventPayload{}) - returned.Signal(struct{}{}) - }() - - returned.WaitOrTimeout() - require.Len(t, bundle.producer.cancelCh, 1) - }) - - t.Run("FullBufferProducerStopped", func(t *testing.T) { - t.Parallel() - - bundle := setup(t) - var returned testsignal.TestSignal[struct{}] - returned.Init(t) - go func() { - bundle.client.notifyProducerOfCancelWithoutNotifier(context.Background(), QueueDefault, &controlEventPayload{}) - returned.Signal(struct{}{}) - }() - bundle.stopped() - - returned.WaitOrTimeout() - require.Len(t, bundle.producer.cancelCh, 1) - }) - - t.Run("RunningProducer", func(t *testing.T) { - t.Parallel() - - bundle := setup(t) - <-bundle.producer.cancelCh - - bundle.client.notifyProducerOfCancelWithoutNotifier(context.Background(), QueueDefault, &controlEventPayload{ - Action: controlActionCancel, - JobID: 456, - Queue: QueueDefault, - }) - - require.Equal(t, int64(456), <-bundle.producer.cancelCh) - }) - - t.Run("StopInProgress", func(t *testing.T) { - t.Parallel() - - bundle := setup(t) - shouldStop, _, finalizeStop := bundle.producer.StopInit() - require.True(t, shouldStop) - defer func() { - bundle.stopped() - finalizeStop(true) - }() - - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - var returned testsignal.TestSignal[struct{}] - returned.Init(t) - go func() { - bundle.client.notifyProducerOfCancelWithoutNotifier(ctx, QueueDefault, &controlEventPayload{}) - returned.Signal(struct{}{}) - }() - - returned.WaitOrTimeout() - require.Len(t, bundle.producer.cancelCh, 1) - }) -} - func Test_Client_Stop_Common(t *testing.T) { t.Parallel() diff --git a/internal/notifier/notifier.go b/internal/notifier/notifier.go index 6bc5a7731..d453eddc3 100644 --- a/internal/notifier/notifier.go +++ b/internal/notifier/notifier.go @@ -22,21 +22,6 @@ import ( "github.com/riverqueue/river/rivershared/util/timeoututil" ) -// ListenerReadyFunc runs after initial connection and each successful reconnect. -type ListenerReadyFunc func() - -// ListenerReadySubscription unregisters a listener-ready callback. -type ListenerReadySubscription struct { - notifier *Notifier -} - -// Unregister stops future callbacks; an invocation already in progress may finish. -func (s *ListenerReadySubscription) Unregister() { - s.notifier.mu.Lock() - defer s.notifier.mu.Unlock() - delete(s.notifier.listenerReadyFuncs, s) -} - type NotificationTopic string const ( @@ -103,13 +88,12 @@ type Notifier struct { testSignals notifierTestSignals waitInterruptChan chan func() - mu sync.RWMutex - isConnected bool - isStarted bool - isWaiting bool - listenerReadyFuncs map[*ListenerReadySubscription]ListenerReadyFunc - subscriptions map[NotificationTopic][]*Subscription - waitCancel context.CancelFunc + mu sync.RWMutex + isConnected bool + isStarted bool + isWaiting bool + subscriptions map[NotificationTopic][]*Subscription + waitCancel context.CancelFunc } func New(archetype *baseservice.Archetype, listener riverdriver.Listener) *Notifier { @@ -118,22 +102,11 @@ func New(archetype *baseservice.Archetype, listener riverdriver.Listener) *Notif notificationBuf: make(chan *riverdriver.Notification, 1000), waitInterruptChan: make(chan func(), 10), - listenerReadyFuncs: make(map[*ListenerReadySubscription]ListenerReadyFunc), - subscriptions: make(map[NotificationTopic][]*Subscription), + subscriptions: make(map[NotificationTopic][]*Subscription), }) return notifier } -// RegisterListenerReadyFunc registers a callback for initial connection and reconnects. -func (n *Notifier) RegisterListenerReadyFunc(listenerReadyFunc ListenerReadyFunc) *ListenerReadySubscription { - n.mu.Lock() - defer n.mu.Unlock() - - sub := &ListenerReadySubscription{notifier: n} - n.listenerReadyFuncs[sub] = listenerReadyFunc - return sub -} - func (n *Notifier) Start(ctx context.Context) error { ctx, shouldStart, started, stopped := n.StartInit(ctx) if !shouldStart { @@ -241,16 +214,6 @@ func (n *Notifier) listenAndWait(ctx context.Context) error { n.testSignals.ListeningBegin.Signal(struct{}{}) defer n.testSignals.ListeningEnd.Signal(struct{}{}) - listenerReadyFuncs := func() []ListenerReadyFunc { - n.mu.RLock() - defer n.mu.RUnlock() - - return maputil.Values(n.listenerReadyFuncs) - }() - for _, listenerReadyFunc := range listenerReadyFuncs { - listenerReadyFunc() - } - drainInterrupts := func() { for { select { diff --git a/internal/notifier/notifier_test.go b/internal/notifier/notifier_test.go index 4af6927c3..8d5c21cc3 100644 --- a/internal/notifier/notifier_test.go +++ b/internal/notifier/notifier_test.go @@ -19,7 +19,6 @@ import ( "github.com/riverqueue/river/riverdriver/riverpgxv5" "github.com/riverqueue/river/rivershared/riversharedtest" "github.com/riverqueue/river/rivershared/startstoptest" - "github.com/riverqueue/river/rivershared/testsignal" "github.com/riverqueue/river/rivershared/util/maputil" "github.com/riverqueue/river/rivershared/util/randutil" "github.com/riverqueue/river/rivershared/util/serviceutil" @@ -282,57 +281,6 @@ func TestNotifier(t *testing.T) { require.Equal(t, TopicAndPayload{testTopic1, "msg1"}, riversharedtest.WaitOrTimeout(t, notifyChan)) }) - t.Run("RegisterListenerReadyFunc", func(t *testing.T) { - t.Parallel() - - notifier, _ := setup(t, nil) - notifier.testDisableSleep = true - - var errorNum int - - listenerMock := NewListenerMock(notifier.listener) - listenerMock.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { - // Returns an error the first time, but then works after. - errorNum++ - switch errorNum { - case 1: - return nil, errors.New("error during wait") - default: - return listenerMock.Listener.WaitForNotification(ctx) - } - } - notifier.listener = listenerMock - - var ready testsignal.TestSignal[struct{}] - ready.Init(t) - sub := notifier.RegisterListenerReadyFunc(func() { ready.Signal(struct{}{}) }) - t.Cleanup(sub.Unregister) - - start(t, notifier) - - // Fires once on the initial connect. - ready.WaitOrTimeout() - - // And again once the notifier reconnects after the injected failure. - require.EqualError(t, notifier.testSignals.BackoffError.WaitOrTimeout(), "error during wait") - ready.WaitOrTimeout() - - notifier.Stop() - sub.Unregister() - - // WaitForNotification is entered after listener-ready callbacks finish. - var waiting testsignal.TestSignal[struct{}] - waiting.Init(t) - listenerMock.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { - waiting.Signal(struct{}{}) - return listenerMock.Listener.WaitForNotification(ctx) - } - - require.NoError(t, notifier.Start(ctx)) - waiting.WaitOrTimeout() - ready.RequireEmpty() - }) - t.Run("SingleTopicMultipleSubscribers", func(t *testing.T) { t.Parallel() diff --git a/java/CHANGELOG.md b/java/CHANGELOG.md index 4d45e1841..cf175f942 100644 --- a/java/CHANGELOG.md +++ b/java/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fixed a cancelled job that was retried being cancelled again as soon as its new attempt started. Retrying a job now clears its earlier attempt's cancellation request, like River for Go. [Issue #1358](https://github.com/riverqueue/river/issues/1358). + ## [0.2.0] - 2026-10-26 ### Changed diff --git a/java/river/src/main/resources/com/riverqueue/postgres.sql b/java/river/src/main/resources/com/riverqueue/postgres.sql index d5a92e3ca..517578057 100644 --- a/java/river/src/main/resources/com/riverqueue/postgres.sql +++ b/java/river/src/main/resources/com/riverqueue/postgres.sql @@ -56,7 +56,7 @@ DELETE FROM {schema}river_job WHERE id=? AND state != 'running' RETURNING {colum -- name: delete_filtered DELETE FROM {schema}river_job WHERE {where} AND id=? AND state != 'running' RETURNING {columns} -- name: retry -UPDATE {schema}river_job SET state='available', finalized_at=NULL, +UPDATE {schema}river_job SET state='available', finalized_at=NULL, metadata=metadata - 'cancel_attempted_at', max_attempts=CASE WHEN attempt=max_attempts THEN max_attempts+1 ELSE max_attempts END, scheduled_at=? WHERE id=? AND state != 'running' AND NOT (state='available' AND scheduled_at < ?) RETURNING {columns} -- name: reset_jobs diff --git a/java/river/src/main/resources/com/riverqueue/sqlite.sql b/java/river/src/main/resources/com/riverqueue/sqlite.sql index 75e2d53b0..dc140d684 100644 --- a/java/river/src/main/resources/com/riverqueue/sqlite.sql +++ b/java/river/src/main/resources/com/riverqueue/sqlite.sql @@ -55,7 +55,7 @@ DELETE FROM {schema}river_job WHERE id=? AND state != 'running' RETURNING {colum -- name: delete_filtered DELETE FROM {schema}river_job WHERE {where} AND id=? AND state != 'running' RETURNING {columns} -- name: retry -UPDATE {schema}river_job SET state='available', finalized_at=NULL, +UPDATE {schema}river_job SET state='available', finalized_at=NULL, metadata=jsonb_remove(metadata,'$.cancel_attempted_at'), max_attempts=CASE WHEN attempt=max_attempts THEN max_attempts+1 ELSE max_attempts END, scheduled_at=? WHERE id=? AND state != 'running' AND NOT (state='available' AND scheduled_at < ?) RETURNING {columns} -- name: reset_jobs diff --git a/java/river/src/test/java/com/riverqueue/RetryTest.java b/java/river/src/test/java/com/riverqueue/RetryTest.java index 683b2a0eb..fb76a4d2c 100644 --- a/java/river/src/test/java/com/riverqueue/RetryTest.java +++ b/java/river/src/test/java/com/riverqueue/RetryTest.java @@ -9,6 +9,7 @@ import java.time.Instant; import java.time.ZoneOffset; import java.util.List; +import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.TimeUnit; @@ -152,6 +153,24 @@ public List claim( assertTrue(errors.isEmpty(), () -> "Unexpected worker errors: " + errors); } + // Like Go, a retry clears a cancellation requested during an earlier attempt + // so that it can't cancel the retried one. Other metadata is preserved. + @Test + void retryClearsAnEarlierCancellationRequest() { + var client = new Client(databases.open(directory.resolve("river.db"))); + var inserted = + client + .insert( + TYPE, "retry", InsertOptions.builder().metadata(Map.of("preserved", true)).build()) + .job(); + assertTrue(client.cancel(inserted.id()).metadata().has("cancel_attempted_at")); + + var retried = client.retry(inserted.id()); + assertEquals(Job.State.AVAILABLE, retried.state()); + assertFalse(retried.metadata().has("cancel_attempted_at")); + assertTrue(retried.metadata().path("preserved").asBoolean()); + } + @ParameterizedTest @ValueSource(booleans = {false, true}) void rolledBackRetriesPreserveEarlierCommittedNotifications(boolean earlierInsert) { diff --git a/js/CHANGELOG.md b/js/CHANGELOG.md index 1a8f980cb..a41b1fbc2 100644 --- a/js/CHANGELOG.md +++ b/js/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Fixed a cancelled job that was retried being cancelled again when the client checked for cancellation requests, which it does while polling and after its notification stream reconnects. Retrying a job now clears its earlier attempt's cancellation request, like River for Go. [Issue #1358](https://github.com/riverqueue/river/issues/1358). + ## [0.3.0] - 2026-10-26 ### Added diff --git a/js/driver/pg/src/driver.integration.test.ts b/js/driver/pg/src/driver.integration.test.ts index 99588eebe..de12c27e9 100644 --- a/js/driver/pg/src/driver.integration.test.ts +++ b/js/driver/pg/src/driver.integration.test.ts @@ -564,6 +564,19 @@ describe("PgDriver integration", () => { expect(unchanged!.state).toBe("running"); }); + it("clears an earlier attempt's cancellation request on retry", async () => { + const inserted = await driver.jobInsert( + insertParams(`${filePrefix}_retry_cancelled`) + ); + const cancelled = await driver.jobCancel(inserted.job.id); + expect(cancelled!.metadata.cancel_attempted_at).toBeDefined(); + + const retried = await driver.jobRetry(inserted.job.id); + + expect(retried!.state).toBe("available"); + expect(retried!.metadata.cancel_attempted_at).toBeUndefined(); + }); + it("returns the committed row to the loser of a cancel or retry race", async () => { const inserted = await driver.jobInsert( insertParams(`${filePrefix}_race`, { diff --git a/js/driver/pg/src/sql/jobs.ts b/js/driver/pg/src/sql/jobs.ts index 37b91ecb6..928c14fc4 100644 --- a/js/driver/pg/src/sql/jobs.ts +++ b/js/driver/pg/src/sql/jobs.ts @@ -827,6 +827,9 @@ export async function jobRetryWithOptions( ELSE river_job.max_attempts END, finalized_at = NULL, + -- A cancellation requested during an earlier attempt must not cancel + -- the next one. + metadata = river_job.metadata - 'cancel_attempted_at', scheduled_at = coalesce($2::timestamptz, now()) FROM job_to_update WHERE river_job.id = job_to_update.id diff --git a/js/driver/sqlite/src/compatibility.test.ts b/js/driver/sqlite/src/compatibility.test.ts index e73c52129..de5a208a3 100644 --- a/js/driver/sqlite/src/compatibility.test.ts +++ b/js/driver/sqlite/src/compatibility.test.ts @@ -629,11 +629,12 @@ describe("SqliteDriver compatibility with Go's riversqlite", () => { const retried = await insert("golden_retry", "available"); await driver.jobCancelDetailed(retried, { now }); await driver.jobRetryDetailed(retried, { now: now.add({ hours: 1 }) }); + // Like Go, a retry clears the earlier cancellation request. expect(stored("golden_retry")).toEqual({ errors: null, errors_null: 1n, finalized_at: null, - metadata: cancelled, + metadata: { m: "x" }, scheduled_at: "2026-01-02 06:06:07.123", state: "available", }); diff --git a/js/driver/sqlite/src/driver.test.ts b/js/driver/sqlite/src/driver.test.ts index a9a41e008..f817367b4 100644 --- a/js/driver/sqlite/src/driver.test.ts +++ b/js/driver/sqlite/src/driver.test.ts @@ -326,8 +326,11 @@ describe("SqliteDriver", () => { const retried = await driver.jobRetryDetailed(inserted.id, { now }); expect(retried.status).toBe("retried"); - if (retried.status === "retried") + if (retried.status === "retried") { expect(retried.job.state).toBe("available"); + // An earlier attempt's cancellation must not cancel the retried one. + expect(retried.job.metadata.cancel_attempted_at).toBeUndefined(); + } expect((await driver.jobRetryDetailed(inserted.id, { now })).status).toBe( "unchanged" ); diff --git a/js/driver/sqlite/src/driver.ts b/js/driver/sqlite/src/driver.ts index 98a0891c5..bb5fc30cb 100644 --- a/js/driver/sqlite/src/driver.ts +++ b/js/driver/sqlite/src/driver.ts @@ -1446,6 +1446,9 @@ export class SqliteRuntime implements RuntimeDriver< ELSE max_attempts END, finalized_at = NULL, + -- A cancellation requested during an earlier attempt must not + -- cancel the next one. + metadata = jsonb_remove(metadata, '$.cancel_attempted_at'), scheduled_at = ? WHERE id = ? AND state != 'running' diff --git a/producer.go b/producer.go index 4b00d16a6..00aa410f7 100644 --- a/producer.go +++ b/producer.go @@ -38,6 +38,7 @@ import ( const ( jobCancelPollBatchSize = 1000 + jobCancelPollIntervalDefault = time.Minute producerReportIntervalDefault = 30 * time.Second queuePollIntervalDefault = 2 * time.Second queueReportIntervalDefault = 10 * time.Minute @@ -91,6 +92,13 @@ type producerConfig struct { // LISTEN/NOTIFY, but this provides a fallback. FetchPollInterval time.Duration + // JobCancelPollInterval is the amount of time between checks for + // cancellation of running jobs when a notifier is configured. Notifications + // normally deliver cancellations right away, so this is only a fallback for + // ones that were lost, like while the notifier was reconnecting. In + // poll-only mode, QueuePollInterval is used instead. + JobCancelPollInterval time.Duration + PluginLookupByJob *pluginlookup.JobPluginLookup PluginLookupGlobal *pluginlookup.PluginLookup JobStuckHandler JobStuckHandler @@ -112,8 +120,8 @@ type producerConfig struct { QueueEventCallback func(event *Event) // QueuePollInterval is the amount of time between periodic checks for - // queue setting changes and job cancellation requests. This is only used in - // poll-only mode (when no notifier is provided). + // queue setting changes and job cancellation requests in poll-only mode + // (when no notifier is provided). QueuePollInterval time.Duration // QueueReportInterval is the amount of time between periodic reports // of the queue status. @@ -138,6 +146,12 @@ func (c *producerConfig) mustValidate() *producerConfig { if c.FetchPollInterval <= 0 { panic("producerConfig.FetchPollInterval must be greater than zero") } + if c.JobCancelPollInterval == 0 { + c.JobCancelPollInterval = jobCancelPollIntervalDefault + } + if c.JobCancelPollInterval <= 0 { + panic("producerConfig.JobCancelPollInterval must be greater than zero") + } if c.JobStuckCount == nil { c.JobStuckCount = &atomic.Int32{} } @@ -215,10 +229,9 @@ type producer struct { pilot riverpilot.Pilot workers *Workers - // Receives job IDs from notifier, polling, and local cancellation calls. - // Only the main goroutine reads it. - cancelCh chan int64 - cancelState atomic.Pointer[producerCancelState] + // Receives job IDs to cancel. Written by notifier and polling goroutines, + // only read from the main goroutine. + cancelCh chan int64 // Set to true when the producer thinks it should trigger another fetch as // soon as slots are available. This is written and read by the main @@ -241,12 +254,8 @@ type producer struct { // Receives control messages from the notifier goroutine. Written by notifier // goroutine, only read from main goroutine. queueControlCh chan *controlEventPayload - - // Coalesces reconnect notifications for reconciliation on the main goroutine. - reconnectCh chan struct{} - - retryPolicy ClientRetryPolicy - testSignals producerTestSignals + retryPolicy ClientRetryPolicy + testSignals producerTestSignals } func newProducer(archetype *baseservice.Archetype, exec riverdriver.Executor, pilot riverpilot.Pilot, config *producerConfig) *producer { @@ -273,7 +282,6 @@ func newProducer(archetype *baseservice.Archetype, exec riverdriver.Executor, pi jobTimeout: config.JobTimeout, pilot: pilot, queueControlCh: make(chan *controlEventPayload, 100), - reconnectCh: make(chan struct{}, 1), retryPolicy: config.RetryPolicy, workers: config.Workers, }) @@ -379,9 +387,8 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { p.fetchLimiter = chanutil.NewDebouncedChan(fetchCtx, p.config.FetchCooldown, true) var ( - controlSub *notifier.Subscription - insertSub *notifier.Subscription - reconnectSub *notifier.ListenerReadySubscription + controlSub *notifier.Subscription + insertSub *notifier.Subscription ) if p.config.Notifier != nil { var err error @@ -415,23 +422,11 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { } return err } - - // Recover cancellations whose NOTIFY was lost while disconnected. - reconnectSub = p.config.Notifier.RegisterListenerReadyFunc(func() { - select { - case p.reconnectCh <- struct{}{}: - default: - } - }) } - subroutineCtx, cancelSubroutines := context.WithCancelCause(context.WithoutCancel(fetchCtx)) - p.cancelState.Store(&producerCancelState{done: subroutineCtx.Done()}) - go func() { started() defer stopped() // this defer should come first so it's last out - defer p.cancelState.Store(nil) p.Logger.DebugContext(fetchCtx, p.Name+": Run loop started", slog.String("queue", p.config.Queue), slog.Bool("paused", p.paused)) defer func() { @@ -446,11 +441,8 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { defer controlSub.Unlisten(fetchCtx) } - if reconnectSub != nil { - defer reconnectSub.Unregister() - } - var subroutineWG sync.WaitGroup + subroutineCtx, cancelSubroutines := context.WithCancelCause(context.WithoutCancel(fetchCtx)) subroutineWG.Add(1) go p.fetchPollLoop(subroutineCtx, &subroutineWG) @@ -464,14 +456,15 @@ func (p *producer) StartWorkContext(fetchCtx, workCtx context.Context) error { subroutineWG.Add(1) go p.reportProducerStatusLoop(subroutineCtx, &subroutineWG) + // Runs with a notifier too, in case a cancel notification is lost. + subroutineWG.Add(1) + go p.pollForJobCancellations(subroutineCtx, &subroutineWG) + if p.config.Notifier == nil { p.Logger.DebugContext(subroutineCtx, p.Name+": No notifier configured; starting in poll mode", "client_id", p.config.ClientID) subroutineWG.Add(1) go p.pollForSettingChanges(subroutineCtx, &subroutineWG, initiallyPaused, initialMetadata) - - subroutineWG.Add(1) - go p.pollForJobCancellations(subroutineCtx, &subroutineWG) } p.fetchAndRunLoop(fetchCtx, workCtx) @@ -582,64 +575,6 @@ func (p *producer) fetchAndRunLoop(fetchCtx, workCtx context.Context) { p.fetchLimiter.Call() fetchResultCh := make(chan producerFetchResult) - reconcileCtx, cancelReconcile := context.WithCancel(fetchCtx) - var reconcileWG sync.WaitGroup - defer func() { - cancelReconcile() - reconcileWG.Wait() - }() - - reconcileResultCh := make(chan producerReconcileResult) - var ( - reconcileAttempt int - reconcilePending bool - reconciling bool - retryTimer *time.Timer - ) - defer func() { - if retryTimer != nil { - retryTimer.Stop() - } - }() - startReconcile := func() { - if reconciling { - reconcilePending = true - return - } - if retryTimer != nil { - retryTimer.Stop() - retryTimer = nil - } - p.activeJobsMu.Lock() - numActiveJobs := len(p.activeJobs) - p.activeJobsMu.Unlock() - if numActiveJobs == 0 { - reconcileAttempt = 0 - return - } - reconciling = true - reconcileWG.Add(1) - p.handleNotifierReconnect(reconcileCtx, reconcileResultCh, &reconcileWG) - } - handleReconcileResult := func(result producerReconcileResult) { - reconciling = false - if result.err != nil { - p.Logger.ErrorContext(workCtx, p.Name+": Error reconciling cancellations; will retry", slog.String("err", result.err.Error())) - if retryTimer != nil { - retryTimer.Stop() - } - retryTimer = p.scheduleReconnectRetry(reconcileCtx, reconcileAttempt) - reconcileAttempt++ - } else { - reconcileAttempt = 0 - p.applyReconcileResult(workCtx, result) - } - if reconcilePending { - reconcilePending = false - startReconcile() - } - } - for { select { case <-fetchCtx.Done(): @@ -686,12 +621,8 @@ func (p *producer) fetchAndRunLoop(fetchCtx, workCtx context.Context) { } case jobID := <-p.cancelCh: p.maybeCancelJob(workCtx, jobID) - case <-p.reconnectCh: - startReconcile() - case result := <-reconcileResultCh: - handleReconcileResult(result) case <-p.fetchLimiter.C(): - p.innerFetchLoop(workCtx, fetchResultCh, reconcileResultCh, handleReconcileResult, startReconcile) + p.innerFetchLoop(workCtx, fetchResultCh) // Ensure we can't start another fetch when fetchCtx is done, even if // the fetchLimiter is also ready to fire: select { @@ -745,7 +676,7 @@ func (p *producer) jitteredFetchPollInterval() time.Duration { return randutil.DurationBetween(p.config.FetchPollInterval, p.config.FetchPollInterval+jitterRange) } -func (p *producer) innerFetchLoop(workCtx context.Context, fetchResultCh chan producerFetchResult, reconcileResultCh <-chan producerReconcileResult, handleReconcileResult func(producerReconcileResult), startReconcile func()) { +func (p *producer) innerFetchLoop(workCtx context.Context, fetchResultCh chan producerFetchResult) { var limit int if p.paused { limit = 0 @@ -781,10 +712,6 @@ func (p *producer) innerFetchLoop(workCtx context.Context, fetchResultCh chan pr } } return - case <-p.reconnectCh: - startReconcile() - case result := <-reconcileResultCh: - handleReconcileResult(result) case result := <-p.jobResultCh: p.removeActiveJob(result) case jobID := <-p.cancelCh: @@ -875,30 +802,6 @@ func (p *producer) addActiveJob(id int64, executor *jobexecutor.JobExecutor) { p.activeJobsMu.Unlock() } -// applyReconcileResult runs on the producer goroutine and ignores replaced executions. -func (p *producer) applyReconcileResult(ctx context.Context, result producerReconcileResult) { - for _, job := range result.jobs { - p.activeJobsMu.Lock() - executor := p.activeJobs[job.ID] - p.activeJobsMu.Unlock() - if executor == nil || executor != result.executors[job.ID] || executor.JobRow.Attempt != job.Attempt { - continue - } - - var metadata struct { - CancelAttemptedAt time.Time `json:"cancel_attempted_at"` - } - if err := json.Unmarshal(job.Metadata, &metadata); err != nil { - p.Logger.ErrorContext(ctx, p.Name+": Error unmarshaling cancellation metadata", - slog.Int64("job_id", job.ID), slog.String("err", err.Error())) - continue - } - if !metadata.CancelAttemptedAt.IsZero() { - p.maybeCancelJob(ctx, job.ID) - } - } -} - func (p *producer) removeActiveJob(job *rivertype.JobRow) { p.activeJobsMu.Lock() executor := p.activeJobs[job.ID] @@ -951,48 +854,6 @@ func (p *producer) maybeCancelJob(ctx context.Context, id int64) bool { return true } -// handleNotifierReconnect snapshots active executions before querying asynchronously. -func (p *producer) handleNotifierReconnect(ctx context.Context, resultCh chan<- producerReconcileResult, wg *sync.WaitGroup) { - p.activeJobsMu.Lock() - executors := make(map[int64]*jobexecutor.JobExecutor, len(p.activeJobs)) - ids := make([]int64, 0, len(p.activeJobs)) - for id, executor := range p.activeJobs { - executors[id] = executor - ids = append(ids, id) - } - - p.activeJobsMu.Unlock() - - go func() { - defer wg.Done() - - jobs, err := timeoututil.WithTimeoutV(ctx, 10*time.Second, p.Name+".handleNotifierReconnect", func(ctx context.Context) ([]*rivertype.JobRow, error) { - return p.exec.JobGetByIDMany(ctx, &riverdriver.JobGetByIDManyParams{ - ID: ids, - Schema: p.config.Schema, - }) - }) - select { - case <-ctx.Done(): - case resultCh <- producerReconcileResult{err: err, executors: executors, jobs: jobs}: - } - }() -} - -// scheduleReconnectRetry retries a failed lookup without blocking the producer. -func (p *producer) scheduleReconnectRetry(ctx context.Context, attempt int) *time.Timer { - sleepDuration := serviceutil.ExponentialBackoff(attempt, serviceutil.MaxAttemptsBeforeResetDefault) - return time.AfterFunc(sleepDuration, func() { - if ctx.Err() != nil { - return - } - select { - case p.reconnectCh <- struct{}{}: - default: - } - }) -} - func (p *producer) metricEmitHooksFromLookup() []rivertype.HookMetricEmit { pluginLookup := p.config.PluginLookupGlobal if pluginLookup == nil { @@ -1184,10 +1045,19 @@ func (p *producer) handleWorkerDone(job *rivertype.JobRow) { // Cancellation polling is independent of fetching: saturated and paused producers // must still cancel jobs, including while draining during graceful shutdown. +// +// Without a notifier, polling is the only way to learn about cancellations. +// With one, it's a slower fallback because a notification is lost if it's sent +// while the notifier isn't listening, like while it's reconnecting. func (p *producer) pollForJobCancellations(ctx context.Context, wg *sync.WaitGroup) { defer wg.Done() - ticker := time.NewTicker(p.config.QueuePollInterval) + interval := p.config.QueuePollInterval + if p.config.Notifier != nil { + interval = p.config.JobCancelPollInterval + } + + ticker := time.NewTicker(interval) defer ticker.Stop() for { select { @@ -1387,22 +1257,12 @@ func (p *producer) reportQueueStatusOnce(ctx context.Context) { p.testSignals.ReportedQueueStatus.Signal(struct{}{}) } -type producerCancelState struct { - done <-chan struct{} -} - type producerFetchResult struct { decodeErrors map[int64]error err error jobs []*rivertype.JobRow } -type producerReconcileResult struct { - err error - executors map[int64]*jobexecutor.JobExecutor - jobs []*rivertype.JobRow -} - type errorHandlerAdapter struct { errorHandler ErrorHandler } diff --git a/producer_test.go b/producer_test.go index c63e04325..8a6d83eae 100644 --- a/producer_test.go +++ b/producer_test.go @@ -6,7 +6,6 @@ import ( "errors" "fmt" "slices" - "sync" "sync/atomic" "testing" "time" @@ -14,13 +13,11 @@ import ( "github.com/stretchr/testify/require" "github.com/riverqueue/river/internal/jobcompleter" - "github.com/riverqueue/river/internal/jobexecutor" "github.com/riverqueue/river/internal/notifier" "github.com/riverqueue/river/internal/pluginlookup" "github.com/riverqueue/river/internal/rivercommon" "github.com/riverqueue/river/internal/riverinternaltest" "github.com/riverqueue/river/internal/riverinternaltest/sharedtx" - "github.com/riverqueue/river/internal/util/chanutil" "github.com/riverqueue/river/riverdbtest" "github.com/riverqueue/river/riverdriver" "github.com/riverqueue/river/riverdriver/riverpgxv5" @@ -79,21 +76,6 @@ func (p *beforeJobGetAvailablePilot) JobGetAvailable( return p.Pilot.JobGetAvailable(ctx, exec, state, params) } -type producerTestExecutor struct { - riverdriver.Executor - - jobGetAvailableFunc func(context.Context, *riverdriver.JobGetAvailableParams) (*riverdriver.JobGetAvailableResult, error) - jobGetByIDManyFunc func(context.Context, *riverdriver.JobGetByIDManyParams) ([]*rivertype.JobRow, error) -} - -func (e *producerTestExecutor) JobGetAvailable(ctx context.Context, params *riverdriver.JobGetAvailableParams) (*riverdriver.JobGetAvailableResult, error) { - return e.jobGetAvailableFunc(ctx, params) -} - -func (e *producerTestExecutor) JobGetByIDMany(ctx context.Context, params *riverdriver.JobGetByIDManyParams) ([]*rivertype.JobRow, error) { - return e.jobGetByIDManyFunc(ctx, params) -} - // undecodableKindPilot reports locked jobs of one kind as undecodable. Postgres' // column types don't allow a job row that can't be decoded, so this simulates // one. @@ -126,208 +108,6 @@ func (p *undecodableKindPilot) JobGetAvailable( return res, nil } -// producerTestListenerMock overrides operations on a real listener. -type producerTestListenerMock struct { - riverdriver.Listener - - connectFunc func(ctx context.Context) error - waitForNotificationFunc func(ctx context.Context) (*riverdriver.Notification, error) -} - -func newProducerTestListenerMock(listener riverdriver.Listener) *producerTestListenerMock { - return &producerTestListenerMock{ - Listener: listener, - connectFunc: listener.Connect, - waitForNotificationFunc: listener.WaitForNotification, - } -} - -func (l *producerTestListenerMock) Connect(ctx context.Context) error { return l.connectFunc(ctx) } - -func (l *producerTestListenerMock) WaitForNotification(ctx context.Context) (*riverdriver.Notification, error) { - return l.waitForNotificationFunc(ctx) -} - -func TestProducer_ApplyReconcileResult(t *testing.T) { - t.Parallel() - - for _, testCase := range []struct { - name string - attempt int - metadata string - replaced bool - wantCancel bool - }{ - {name: "Cancelled", attempt: 1, metadata: `{"cancel_attempted_at":"2026-10-20T00:00:00Z"}`, wantCancel: true}, - {name: "DifferentAttempt", attempt: 2, metadata: `{"cancel_attempted_at":"2026-10-20T00:00:00Z"}`}, - {name: "MalformedMetadata", attempt: 1, metadata: `{"cancel_attempted_at":true}`}, - {name: "NoCancellation", attempt: 1, metadata: `{}`}, - {name: "ReplacedExecutor", attempt: 1, metadata: `{"cancel_attempted_at":"2026-10-20T00:00:00Z"}`, replaced: true}, - } { - t.Run(testCase.name, func(t *testing.T) { - t.Parallel() - - archetype := riversharedtest.BaseServiceArchetype(t) - ctx, cancel := context.WithCancelCause(context.Background()) - t.Cleanup(func() { cancel(nil) }) - executor := baseservice.Init(archetype, &jobexecutor.JobExecutor{CancelFunc: cancel, JobRow: &rivertype.JobRow{ID: 1, Attempt: 1}}) - snapshotExecutor := executor - if testCase.replaced { - snapshotExecutor = &jobexecutor.JobExecutor{} - } - prod := baseservice.Init(archetype, &producer{activeJobs: map[int64]*jobexecutor.JobExecutor{1: executor}}) - - prod.applyReconcileResult(context.Background(), producerReconcileResult{ - executors: map[int64]*jobexecutor.JobExecutor{1: snapshotExecutor}, - jobs: []*rivertype.JobRow{{ID: 1, Attempt: testCase.attempt, Metadata: []byte(testCase.metadata)}}, - }) - if testCase.wantCancel { - require.ErrorIs(t, context.Cause(ctx), rivertype.ErrJobCancelledRemotely) - } else { - require.NoError(t, ctx.Err()) - } - }) - } -} - -// A cancel NOTIFY lost during a disconnect must be recovered on reconnect. -func TestProducer_JobCancelSurvivesNotifierReconnect(t *testing.T) { - t.Parallel() - - ctx := context.Background() - - var ( - archetype = riversharedtest.BaseServiceArchetype(t) - dbPool = riversharedtest.DBPool(ctx, t) - driver = riverpgxv5.New(dbPool) - exec = driver.GetExecutor() - jobUpdates = make(chan []jobcompleter.CompleterJobUpdated, 10) - schema = riverdbtest.TestSchema(ctx, t, driver, nil) - queueName = fmt.Sprintf("test-reconnect-cancel-%05d", randutil.IntBetween(1, 100_000)) - pilot = &riverpilot.StandardPilot{} - ) - - completer := jobcompleter.NewInlineCompleter(archetype, schema, exec, pilot, jobUpdates) - require.NoError(t, completer.Start(ctx)) - t.Cleanup(completer.Stop) - - realListener := driver.GetListener(&riverdriver.GetListenenerParams{Schema: schema}) - mockListener := newProducerTestListenerMock(realListener) - - var ( - forceDisconnect atomic.Bool // set by the test to inject one simulated connection loss - reconnecting atomic.Bool - reconnectStarted testsignal.TestSignal[struct{}] - releaseReconnect = make(chan struct{}) - releaseReconnectOnce sync.Once - ) - reconnectStarted.Init(t) - closeReleaseReconnect := func() { releaseReconnectOnce.Do(func() { close(releaseReconnect) }) } - mockListener.waitForNotificationFunc = func(ctx context.Context) (*riverdriver.Notification, error) { - if forceDisconnect.CompareAndSwap(true, false) { - reconnecting.Store(true) - return nil, errors.New("simulated connection loss") - } - return realListener.WaitForNotification(ctx) - } - mockListener.connectFunc = func(ctx context.Context) error { - if reconnecting.Load() { - reconnectStarted.Signal(struct{}{}) - select { - case <-ctx.Done(): - return ctx.Err() - case <-releaseReconnect: - } - reconnecting.Store(false) - } - return realListener.Connect(ctx) - } - - notif := notifier.New(archetype, mockListener) - require.NoError(t, notif.Start(ctx)) - t.Cleanup(notif.Stop) - // Release a blocked Connect before stopping the notifier. - t.Cleanup(closeReleaseReconnect) - - var jobStarted testsignal.TestSignal[int64] - jobStarted.Init(t) - workers := NewWorkers() - - type JobArgs struct { - testutil.JobArgsReflectKind[JobArgs] - } - - AddWorker(workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { - jobStarted.Signal(job.ID) - <-ctx.Done() - return ctx.Err() - })) - - prod := newProducer(archetype, exec, pilot, &producerConfig{ - ClientID: testClientID, - Completer: completer, - ErrorHandler: newTestErrorHandler(), - FetchCooldown: FetchCooldownDefault, - FetchPollInterval: 50 * time.Millisecond, - PluginLookupByJob: pluginlookup.NewJobPluginLookup(nil), - PluginLookupGlobal: pluginlookup.NewPluginLookup(nil), - JobTimeout: JobTimeoutDefault, - MaxWorkers: 10, - Notifier: notif, - Queue: queueName, - QueuePollInterval: queuePollIntervalDefault, - QueueReportInterval: queueReportIntervalDefault, - RetryPolicy: &DefaultClientRetryPolicy{}, - SchedulerInterval: riverinternaltest.SchedulerShortInterval, - Schema: schema, - StaleProducerRetentionPeriod: time.Minute, - Workers: workers, - }) - - config := newTestConfig(t, schema) - insertParams, err := insertParamsFromConfigArgsAndOptions(&prod.Archetype, config, &JobArgs{}, &InsertOpts{Queue: queueName}) - require.NoError(t, err) - _, err = exec.JobInsertFastMany(ctx, &riverdriver.JobInsertFastManyParams{ - Jobs: []*riverdriver.JobInsertFastParams{(*riverdriver.JobInsertFastParams)(insertParams)}, - Schema: schema, - }) - require.NoError(t, err) - - workCtx, workCancel := context.WithCancel(ctx) - - require.NoError(t, prod.StartWorkContext(ctx, workCtx)) - // Cancel workers before waiting for producer shutdown. - t.Cleanup(prod.Stop) - t.Cleanup(workCancel) - - jobID := jobStarted.WaitOrTimeout() - - // Wake the listener so it observes the injected disconnect. - forceDisconnect.Store(true) - require.NoError(t, exec.NotifyMany(ctx, &riverdriver.NotifyManyParams{ - Topic: string(notifier.NotificationTopicInsert), - Payload: []string{`{"queue":"` + queueName + `-unrelated-wakeup"}`}, - Schema: schema, - })) - - reconnectStarted.WaitOrTimeout() - - // Cancel while no listener is connected. - _, err = exec.JobCancel(ctx, &riverdriver.JobCancelParams{ - ID: jobID, - CancelAttemptedAt: time.Now().UTC(), - ControlTopic: string(notifier.NotificationTopicControl), - Schema: schema, - }) - require.NoError(t, err) - - closeReleaseReconnect() - - update := riversharedtest.WaitOrTimeout(t, jobUpdates) - require.Equal(t, rivertype.JobStateCancelled, update[0].Job.State, - "job should have observed cancellation after the notifier reconnected and reconciled active jobs against cancel_attempted_at") -} - func TestProducer_MetricEmitHook(t *testing.T) { t.Parallel() @@ -542,124 +322,6 @@ func TestProducer_PollOnly(t *testing.T) { }) } -func TestProducer_ReconcileDoesNotBlock(t *testing.T) { - t.Parallel() - - for _, testCase := range []struct { - name string - blockedFetch bool - cancelFromLookup bool - queryError bool - }{ - {name: "BlockedFetchCancelDelivery", blockedFetch: true}, - {name: "BlockedFetchReconcileResult", blockedFetch: true, cancelFromLookup: true}, - {name: "CancelDelivery"}, - {name: "ReconcileResult", cancelFromLookup: true}, - {name: "RetryAfterError", cancelFromLookup: true, queryError: true}, - } { - t.Run(testCase.name, func(t *testing.T) { - t.Parallel() - - var signals struct { - FetchStarted testsignal.TestSignal[struct{}] - LoopStopped testsignal.TestSignal[struct{}] - QueryStarted testsignal.TestSignal[time.Duration] - } - signals.FetchStarted.Init(t) - signals.LoopStopped.Init(t) - signals.QueryStarted.Init(t) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - releaseFetch := make(chan struct{}) - var releaseOnce sync.Once - release := func() { releaseOnce.Do(func() { close(releaseFetch) }) } - defer release() - archetype := riversharedtest.BaseServiceArchetype(t) - jobCtx, cancelJob := context.WithCancelCause(ctx) - defer cancelJob(nil) - executor := baseservice.Init(archetype, &jobexecutor.JobExecutor{CancelFunc: cancelJob, JobRow: &rivertype.JobRow{ID: 1, Attempt: 1}}) - var queryAttempts int - exec := &producerTestExecutor{ - jobGetAvailableFunc: func(context.Context, *riverdriver.JobGetAvailableParams) (*riverdriver.JobGetAvailableResult, error) { - signals.FetchStarted.Signal(struct{}{}) - <-releaseFetch - return &riverdriver.JobGetAvailableResult{}, nil - }, - jobGetByIDManyFunc: func(ctx context.Context, _ *riverdriver.JobGetByIDManyParams) ([]*rivertype.JobRow, error) { - deadline, ok := ctx.Deadline() - if ok { - signals.QueryStarted.Signal(time.Until(deadline)) - } else { - signals.QueryStarted.Signal(0) - } - queryAttempts++ - if testCase.queryError && queryAttempts == 1 { - return nil, errors.New("temporary reconciliation failure") - } - if testCase.cancelFromLookup { - return []*rivertype.JobRow{{ID: 1, Attempt: 1, Metadata: []byte(`{"cancel_attempted_at":"2026-10-20T00:00:00Z"}`)}}, nil - } - <-ctx.Done() - return nil, ctx.Err() - }, - } - pilot := &riverpilot.StandardPilot{} - _, state, err := pilot.ProducerInit(ctx, exec, nil) - require.NoError(t, err) - prod := baseservice.Init(archetype, &producer{ - activeJobs: map[int64]*jobexecutor.JobExecutor{1: executor}, - cancelCh: make(chan int64, 1), - config: &producerConfig{MaxWorkers: 2}, - exec: exec, - fetchLimiter: chanutil.NewDebouncedChan(ctx, time.Millisecond, true), - jobResultCh: make(chan *rivertype.JobRow), - paused: !testCase.blockedFetch, - pilot: pilot, - state: state, - queueControlCh: make(chan *controlEventPayload), - reconnectCh: make(chan struct{}, 1), - }) - go func() { - prod.fetchAndRunLoop(ctx, ctx) - signals.LoopStopped.Signal(struct{}{}) - }() - defer func() { - release() - cancel() - signals.LoopStopped.WaitOrTimeout() - }() - - if testCase.blockedFetch { - signals.FetchStarted.WaitOrTimeout() - } - prod.reconnectCh <- struct{}{} - queryTimeout := signals.QueryStarted.WaitOrTimeout() - require.Positive(t, queryTimeout) - require.LessOrEqual(t, queryTimeout, 10*time.Second) - if testCase.queryError { - require.Positive(t, signals.QueryStarted.WaitOrTimeout()) - } - - if !testCase.cancelFromLookup { - prod.cancelCh <- 1 - } - select { - case <-jobCtx.Done(): - require.ErrorIs(t, context.Cause(jobCtx), rivertype.ErrJobCancelledRemotely) - case <-time.After(time.Second): - t.Fatal("blocked reconciliation prevented cancellation delivery") - } - - select { - case prod.jobResultCh <- &rivertype.JobRow{ID: 1}: - case <-time.After(time.Second): - t.Fatal("blocked reconciliation prevented job completion") - } - }) - } -} - func TestProducer_WithNotifier(t *testing.T) { t.Parallel() @@ -888,10 +550,8 @@ func testProducer(t *testing.T, makeProducer func(ctx context.Context, t *testin t.Parallel() producer, bundle := setup(t) - if producer.config.Notifier != nil { - t.Skip("requires polling without a notifier") - } producer.config.FetchPollInterval = time.Hour + producer.config.JobCancelPollInterval = 20 * time.Millisecond producer.config.MaxWorkers = 1 producer.config.QueuePollInterval = 20 * time.Millisecond @@ -926,19 +586,25 @@ func testProducer(t *testing.T, makeProducer func(ctx context.Context, t *testin Name: producer.config.Queue, Schema: producer.config.Schema, })) + if producer.config.Notifier != nil { + // Only Client.QueuePause sends a notification. + producer.TriggerQueueControlEvent(&controlEventPayload{Action: controlActionPause, Queue: producer.config.Queue}) + } producer.testSignals.Paused.WaitOrTimeout() case "Stopping": fetchCancel() producer.testSignals.ExecutorShutdownStarted.WaitOrTimeout() } - // Use the executor directly so no local client shortcut can cancel - // the worker. No more jobs can be fetched in any of these states. - _, err := bundle.exec.JobCancel(ctx, &riverdriver.JobCancelParams{ - ID: jobID, - CancelAttemptedAt: time.Now().UTC(), - ControlTopic: string(notifier.NotificationTopicControl), - Schema: producer.config.Schema, + // Request cancellation the way JobCancel does, but without a + // notification, as if it was lost while the notifier was + // reconnecting. Only polling can find it. No more jobs can be + // fetched in any of these states. + _, err := bundle.exec.JobUpdate(ctx, &riverdriver.JobUpdateParams{ + ID: jobID, + Metadata: fmt.Appendf(nil, `{"cancel_attempted_at":%q}`, time.Now().UTC().Format(time.RFC3339Nano)), + MetadataDoMerge: true, + Schema: producer.config.Schema, }) require.NoError(t, err) require.ErrorIs(t, workerErr.WaitOrTimeout(), rivertype.ErrJobCancelledRemotely) diff --git a/riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go b/riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go index 53dae3d6f..0f7f8142f 100644 --- a/riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go +++ b/riverdriver/riverdatabasesql/internal/dbsqlc/river_job.sql.go @@ -1320,6 +1320,8 @@ updated_job AS ( state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + -- A cancellation requested during an earlier attempt must not cancel + -- the next one. metadata = metadata - 'cancel_attempted_at', scheduled_at = coalesce($2::timestamptz, now()) FROM job_to_update diff --git a/riverdriver/riverdrivertest/job_update.go b/riverdriver/riverdrivertest/job_update.go index 1346c5008..e537229a5 100644 --- a/riverdriver/riverdrivertest/job_update.go +++ b/riverdriver/riverdrivertest/job_update.go @@ -516,10 +516,8 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT exec, _ := setup(ctx, t) - metadata := []byte(`{"cancel_attempted_at":"2026-01-01T00:00:00Z","custom":"preserved"}`) job := testfactory.Job(ctx, t, exec, &testfactory.JobOpts{ - Metadata: metadata, - State: new(rivertype.JobStateRunning), + State: new(rivertype.JobStateRunning), }) jobAfter, err := exec.JobRetry(ctx, &riverdriver.JobRetryParams{ @@ -527,13 +525,11 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT }) require.NoError(t, err) require.Equal(t, rivertype.JobStateRunning, jobAfter.State) - require.JSONEq(t, string(metadata), string(jobAfter.Metadata)) require.WithinDuration(t, job.ScheduledAt, jobAfter.ScheduledAt, time.Microsecond) jobUpdated, err := exec.JobGetByID(ctx, &riverdriver.JobGetByIDParams{ID: job.ID}) require.NoError(t, err) require.Equal(t, rivertype.JobStateRunning, jobUpdated.State) - require.JSONEq(t, string(metadata), string(jobUpdated.Metadata)) }) for _, state := range []rivertype.JobState{ @@ -565,7 +561,6 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT job := testfactory.Job(ctx, t, exec, &testfactory.JobOpts{ FinalizedAt: finalizedAt, - Metadata: []byte(`{"cancel_attempted_at":"2026-01-01T00:00:00Z","custom":"preserved"}`), ScheduledAt: new(now.Add(1 * time.Hour)), State: &state, }) @@ -576,14 +571,12 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT }) require.NoError(t, err) require.Equal(t, rivertype.JobStateAvailable, jobAfter.State) - require.JSONEq(t, `{"custom":"preserved"}`, string(jobAfter.Metadata)) require.WithinDuration(t, now, jobAfter.ScheduledAt, bundle.driver.TimePrecision()) jobUpdated, err := exec.JobGetByID(ctx, &riverdriver.JobGetByIDParams{ID: job.ID}) require.NoError(t, err) require.Equal(t, rivertype.JobStateAvailable, jobUpdated.State) require.Nil(t, jobUpdated.FinalizedAt) - require.JSONEq(t, `{"custom":"preserved"}`, string(jobUpdated.Metadata)) }) } @@ -614,6 +607,30 @@ func exerciseJobUpdate[TTx any](ctx context.Context, t *testing.T, executorWithT require.WithinDuration(t, now, jobAfter.ScheduledAt, 5*time.Second) }) + // A cancellation requested during an earlier attempt must not apply to + // the retried one. Other metadata is preserved. + t.Run("ClearsCancelAttemptedAt", func(t *testing.T) { + t.Parallel() + + exec, _ := setup(ctx, t) + + now := time.Now().UTC() + + job := testfactory.Job(ctx, t, exec, &testfactory.JobOpts{ + FinalizedAt: &now, + Metadata: []byte(`{"cancel_attempted_at":"2026-01-01T00:00:00Z","foo":"bar"}`), + State: new(rivertype.JobStateCancelled), + }) + + jobAfter, err := exec.JobRetry(ctx, &riverdriver.JobRetryParams{ + ID: job.ID, + Now: &now, + }) + require.NoError(t, err) + require.Equal(t, rivertype.JobStateAvailable, jobAfter.State) + require.JSONEq(t, `{"foo":"bar"}`, string(jobAfter.Metadata)) + }) + t.Run("DoesNotAlterScheduledAtIfInThePastAndJobAlreadyAvailable", func(t *testing.T) { // We don't want to update ScheduledAt if the job was already available // because doing so can make it lose its place in line. diff --git a/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql b/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql index 6b11a3bc5..cc921c261 100644 --- a/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql +++ b/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql @@ -533,6 +533,8 @@ updated_job AS ( state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + -- A cancellation requested during an earlier attempt must not cancel + -- the next one. metadata = metadata - 'cancel_attempted_at', scheduled_at = coalesce(sqlc.narg('now')::timestamptz, now()) FROM job_to_update diff --git a/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql.go b/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql.go index 57bf72f8b..165b85af0 100644 --- a/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql.go +++ b/riverdriver/riverpgxv5/internal/dbsqlc/river_job.sql.go @@ -1284,6 +1284,8 @@ updated_job AS ( state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + -- A cancellation requested during an earlier attempt must not cancel + -- the next one. metadata = metadata - 'cancel_attempted_at', scheduled_at = coalesce($2::timestamptz, now()) FROM job_to_update diff --git a/riverdriver/riversqlite/internal/dbsqlc/river_job.sql b/riverdriver/riversqlite/internal/dbsqlc/river_job.sql index 3380dc0dd..fd239638e 100644 --- a/riverdriver/riversqlite/internal/dbsqlc/river_job.sql +++ b/riverdriver/riversqlite/internal/dbsqlc/river_job.sql @@ -588,6 +588,8 @@ SET state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + -- A cancellation requested during an earlier attempt must not cancel the + -- next one. metadata = jsonb_remove(metadata, '$.cancel_attempted_at'), scheduled_at = coalesce(cast(sqlc.narg('now') AS text), datetime('now', 'subsec')) WHERE id = @id diff --git a/riverdriver/riversqlite/internal/dbsqlc/river_job.sql.go b/riverdriver/riversqlite/internal/dbsqlc/river_job.sql.go index 2a7472f1d..2e4718390 100644 --- a/riverdriver/riversqlite/internal/dbsqlc/river_job.sql.go +++ b/riverdriver/riversqlite/internal/dbsqlc/river_job.sql.go @@ -1452,6 +1452,8 @@ SET state = 'available', max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, finalized_at = NULL, + -- A cancellation requested during an earlier attempt must not cancel the + -- next one. metadata = jsonb_remove(metadata, '$.cancel_attempted_at'), scheduled_at = coalesce(cast(?1 AS text), datetime('now', 'subsec')) WHERE id = ?2 diff --git a/rust/CHANGELOG.md b/rust/CHANGELOG.md index 45edf8a4d..7215df8c9 100644 --- a/rust/CHANGELOG.md +++ b/rust/CHANGELOG.md @@ -10,6 +10,10 @@ Changes to River for Go are recorded in the [repository changelog](../CHANGELOG. ## [Unreleased] +### Fixed + +- Fixed a running job not being cancelled when its cancellation notification was lost, as happens if it's sent while the client's listener is reconnecting. Clients with notifications now also check their running jobs for cancellation requests once a minute, like River for Go. Retrying a job now clears its earlier attempt's cancellation request so that the retried job isn't cancelled again. [Issue #1358](https://github.com/riverqueue/river/issues/1358). + ## [0.4.0] - 2026-10-26 ### Added diff --git a/rust/riverqueue/src/__private.rs b/rust/riverqueue/src/__private.rs index 5c3f36930..9008d3c1e 100644 --- a/rust/riverqueue/src/__private.rs +++ b/rust/riverqueue/src/__private.rs @@ -109,6 +109,14 @@ pub trait ClientBuilderExt: Sized { /// jobs, should reject that configuration the same way. fn leader_election_disabled(&self) -> bool; + /// Sets how often a client with a notifier checks its running jobs for + /// cancellation requests in case a notification was lost, a minute by + /// default. Checks happen on the two-second poll for queue settings, so + /// shorter intervals check that often. It's a control for tests, not a + /// tuning option. + #[must_use] + fn job_cancel_poll_interval(self, interval: Duration) -> Self; + /// Installs a pilot from a companion crate. #[must_use] fn pilot(self, pilot: P) -> Self; @@ -121,6 +129,11 @@ pub trait ClientBuilderExt: Sized { } impl ClientBuilderExt for crate::ClientBuilder { + fn job_cancel_poll_interval(mut self, interval: Duration) -> Self { + self.job_cancel_poll_interval = interval; + self + } + fn leader_election_disabled(&self) -> bool { self.leader_election_disabled } diff --git a/rust/riverqueue/src/client/builder.rs b/rust/riverqueue/src/client/builder.rs index 0c378c36d..8fe8f3a5b 100644 --- a/rust/riverqueue/src/client/builder.rs +++ b/rust/riverqueue/src/client/builder.rs @@ -350,6 +350,7 @@ pub struct ClientBuilder { pub(super) hooks: Vec>, pub(super) id: String, pub(super) insert_middleware: Vec>, + pub(crate) job_cancel_poll_interval: Duration, pub(super) job_stuck_threshold: Duration, pub(super) job_timeout: Option, pub(crate) leader_election_disabled: bool, @@ -814,6 +815,7 @@ impl ClientBuilder { id: self.id, insert_middleware: self.insert_middleware, insert_notify_limiter: InsertNotifyLimiter::new(self.fetch_cooldown), + job_cancel_poll_interval: self.job_cancel_poll_interval, job_stuck_threshold: self.job_stuck_threshold, job_timeout: self.job_timeout, leader_election_disabled: self.leader_election_disabled, diff --git a/rust/riverqueue/src/client/mod.rs b/rust/riverqueue/src/client/mod.rs index 68b517853..81da170df 100644 --- a/rust/riverqueue/src/client/mod.rs +++ b/rust/riverqueue/src/client/mod.rs @@ -79,6 +79,10 @@ use crate::{ pub(crate) const ATTEMPTED_BY_MAX: i32 = 100; const EVENT_BUFFER_CAPACITY: usize = 10_000; +/// How often a client with a notifier checks its running jobs for +/// cancellation requests whose notification was lost, like River Go's +/// producers. +const JOB_CANCEL_POLL_INTERVAL_DEFAULT: Duration = Duration::from_mins(1); const PENDING_CANCELLATION_LIMIT: usize = 10_000; const PENDING_CANCELLATION_RETENTION: Duration = Duration::from_mins(1); // Large queues otherwise become limited by a single Postgres claim round trip. @@ -122,6 +126,9 @@ pub(crate) struct ClientInner { /// Skips a queue's insert notification sent within the fetch cooldown /// of the previous one. pub(crate) insert_notify_limiter: InsertNotifyLimiter, + /// How often a client with a notifier checks its running jobs for + /// cancellation requests in case a notification was lost. + job_cancel_poll_interval: Duration, job_stuck_threshold: Duration, pub(crate) job_timeout: Option, leader_election_disabled: bool, @@ -310,6 +317,7 @@ impl Client { hooks: Vec::new(), id: default_client_id(), insert_middleware: Vec::new(), + job_cancel_poll_interval: JOB_CANCEL_POLL_INTERVAL_DEFAULT, job_stuck_threshold: JOB_STUCK_THRESHOLD_DEFAULT, job_timeout: Some(JOB_TIMEOUT_DEFAULT), leader_election_disabled: false, diff --git a/rust/riverqueue/src/client/producer.rs b/rust/riverqueue/src/client/producer.rs index fa68924f9..59af2a5bf 100644 --- a/rust/riverqueue/src/client/producer.rs +++ b/rust/riverqueue/src/client/producer.rs @@ -56,8 +56,9 @@ pub(super) async fn run_dynamic_queues( mut changes: watch::Receiver, queues_ready: oneshot::Sender<()>, ) -> Result<(), Error> { - // Without a notifier, running jobs learn of cancellation requests by - // polling until every producer has drained. Whether the client has one is + // Running jobs learn of cancellation requests by polling until every + // producer has drained: regularly without a notifier, and occasionally + // with one in case a notification was lost. Whether the client has one is // known only once a Postgres server's capabilities are detected, so the // poll checks each time. let _cancellation_poll = AbortOnDrop(tokio::spawn(poll_job_cancellations( @@ -144,9 +145,11 @@ fn jittered_fetch_poll_interval(interval: Duration) -> Duration { /// Cancels this client's running attempts whose jobs have a cancellation /// request, checking every [`QUEUE_CONFIG_POLL_INTERVAL`] while the client -/// has no notifier, like River Go's producers without one. That's checked -/// each time, since a Postgres server's lack of `LISTEN`/`NOTIFY` is -/// detected only once the client runs. +/// has no notifier, like River Go's producers without one. With a notifier, +/// it's a fallback for a notification that was lost, like while the notifier +/// was reconnecting, so it checks only every `job_cancel_poll_interval`. +/// Whether the client has a notifier is checked each time, since a Postgres +/// server's lack of `LISTEN`/`NOTIFY` is detected only once the client runs. /// It keeps checking after `stopping` fires, so jobs can still be cancelled /// while producers drain; the caller aborts it once they have. async fn poll_job_cancellations(inner: Arc, stopping: CancellationToken) { @@ -154,11 +157,13 @@ async fn poll_job_cancellations(inner: Arc, stopping: CancellationT poll.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); // The first tick completes at once; running jobs start later. poll.tick().await; + let mut last_checked = tokio::time::Instant::now(); loop { poll.tick().await; - if inner.has_notifier() { + if inner.has_notifier() && last_checked.elapsed() < inner.job_cancel_poll_interval { continue; } + last_checked = tokio::time::Instant::now(); let mut ids = inner .running .lock() diff --git a/rust/riverqueue/src/database/sqlite.rs b/rust/riverqueue/src/database/sqlite.rs index 2152e1341..b9b9d5c51 100644 --- a/rust/riverqueue/src/database/sqlite.rs +++ b/rust/riverqueue/src/database/sqlite.rs @@ -957,6 +957,7 @@ pub(crate) async fn retry( ELSE max_attempts END, finalized_at = NULL, + metadata = jsonb_remove(metadata, '$.cancel_attempted_at'), scheduled_at = ? WHERE id = ? AND state != 'running' diff --git a/rust/riverqueue/src/storage/postgres.rs b/rust/riverqueue/src/storage/postgres.rs index 316eb3587..e5b3bf151 100644 --- a/rust/riverqueue/src/storage/postgres.rs +++ b/rust/riverqueue/src/storage/postgres.rs @@ -239,7 +239,8 @@ impl Backend for PostgresBackend<'_> { "WITH locked AS (SELECT id FROM {table} WHERE id = $1 FOR UPDATE), \ updated AS (UPDATE {table} AS job SET state = 'available', \ max_attempts = CASE WHEN attempt = max_attempts THEN max_attempts + 1 ELSE max_attempts END, \ - finalized_at = NULL, scheduled_at = now() \ + finalized_at = NULL, metadata = metadata - 'cancel_attempted_at', \ + scheduled_at = now() \ FROM locked WHERE job.id = locked.id AND job.state != 'running' \ AND NOT (job.state = 'available' AND job.scheduled_at < now()) RETURNING job.*) \ SELECT {}, false AS unique_skipped_as_duplicate FROM updated AS job \ diff --git a/rust/riverqueue/tests/client_handles.rs b/rust/riverqueue/tests/client_handles.rs index 8f00521a1..c58e35cc5 100644 --- a/rust/riverqueue/tests/client_handles.rs +++ b/rust/riverqueue/tests/client_handles.rs @@ -775,6 +775,30 @@ macro_rules! scenarios { fixture.cleanup().await; } + // Like Go's `JobRetry`, a retry clears the cancellation requested + // during an earlier attempt so that it can't cancel the retried one. + #[tokio::test(flavor = "multi_thread")] + async fn retry_clears_a_cancellation_request() { + let fixture = Fixture::new().await; + let jobs = fixture.client.jobs(); + let metadata: riverqueue::JobMetadata = r#"{"preserved":true}"#.parse().unwrap(); + let id = fixture + .client + .insert(args("cancel then retry")) + .opts(InsertOpts::default().with_metadata(metadata)) + .await + .unwrap() + .id(); + let cancelled = jobs.cancel(id).await.unwrap(); + assert!(cancelled.metadata.contains_key("cancel_attempted_at")); + + let retried = jobs.retry(id).await.unwrap(); + assert_eq!(retried.state, JobState::Available); + assert!(!retried.metadata.contains_key("cancel_attempted_at")); + assert_eq!(retried.metadata.get::("preserved").unwrap(), Some(true)); + fixture.cleanup().await; + } + #[tokio::test(flavor = "multi_thread")] async fn local_queues_change_configuration_without_lock_errors() { let fixture = Fixture::new().await; diff --git a/rust/riverqueue/tests/poll_only_cancellation.rs b/rust/riverqueue/tests/poll_only_cancellation.rs index 76b6928d1..5dc2ebc5c 100644 --- a/rust/riverqueue/tests/poll_only_cancellation.rs +++ b/rust/riverqueue/tests/poll_only_cancellation.rs @@ -1,4 +1,5 @@ -//! Cancellation of running jobs in clients without notifications, on every +//! Cancellation of running jobs found by polling, in clients without +//! notifications and in clients whose notification was lost, on every //! backend. //! //! Postgres tests run in a unique schema and fail rather than skip when @@ -11,8 +12,8 @@ mod support; use std::{convert::Infallible, sync::Arc, time::Duration}; use riverqueue::{ - __private::Database, Client, EventKind, Job, JobArgs, JobState, QueueConfig, WorkContext, - WorkOutcome, Workers, + __private::{ClientBuilderExt, Database}, + Client, EventKind, Job, JobArgs, JobState, QueueConfig, WorkContext, WorkOutcome, Workers, }; use serde::{Deserialize, Serialize}; use tokio::sync::Semaphore; @@ -54,6 +55,97 @@ impl Backend { Self::Sqlite(pool, path) => support::sqlite_cleanup(pool, path).await, } } + + /// Requests a job's cancellation the way `JobCancel` does, but without a + /// notification, as if it was lost. + async fn request_cancellation_without_notification(&self, id: i64) { + let cancel_attempted_at = chrono::Utc::now().to_rfc3339(); + match self { + #[cfg(all(feature = "postgres", river_postgres_tests))] + Self::Postgres(schema) => { + sqlx::query(sqlx::AssertSqlSafe(format!( + "UPDATE {} SET metadata = jsonb_set(metadata, '{{cancel_attempted_at}}', to_jsonb($2::text)) \ + WHERE id = $1", + schema.table("river_job") + ))) + .bind(id) + .bind(cancel_attempted_at) + .execute(&schema.pool) + .await + .unwrap(); + } + #[cfg(feature = "sqlite")] + Self::Sqlite(pool, _) => { + sqlx::query( + "UPDATE river_job SET metadata = jsonb_set(metadata, '$.cancel_attempted_at', ?) \ + WHERE id = ?", + ) + .bind(cancel_attempted_at) + .bind(id) + .execute(pool) + .await + .unwrap(); + } + } + } +} + +/// Workers that block until their attempt is cancelled, adding a permit to +/// `started` when one starts. +fn waiting_workers(started: &Arc) -> Workers { + let mut workers = Workers::new(); + let started = Arc::clone(started); + workers + .add_fn(move |context: WorkContext, _job: Job| { + let started = Arc::clone(&started); + async move { + started.add_permits(1); + context.cancellation_token().cancelled().await; + // Any outcome but completion becomes the cancellation. + Ok::<_, Infallible>(WorkOutcome::Snooze(Duration::from_hours(1))) + } + }) + .unwrap(); + workers +} + +/// A client with notifications still cancels its running job when the +/// cancellation's notification was lost, like if it was sent while the +/// client's notifier was reconnecting, because it also checks for +/// cancellation requests occasionally. +async fn polls_for_lost_cancellation_notification(backend: Backend) { + let started = Arc::new(Semaphore::new(0)); + let client = Client::builder(backend.database()) + .job_cancel_poll_interval(Duration::ZERO) + .queue(riverqueue::QUEUE_DEFAULT, QueueConfig::new(1)) + .without_leader_election() + .workers(waiting_workers(&started)) + .build() + .unwrap(); + let mut events = client.subscribe(&[EventKind::JobCancelled]).unwrap(); + let mut run = client.start().unwrap(); + run.wait_ready().await.unwrap(); + + let id = client.insert(WaitArgs {}).await.unwrap().job.row.id; + tokio::time::timeout(TIMEOUT, started.acquire()) + .await + .expect("the job should start") + .unwrap() + .forget(); + backend.request_cancellation_without_notification(id).await; + + let event = tokio::time::timeout(TIMEOUT, events.recv()) + .await + .expect("the job should be cancelled") + .unwrap(); + let job = &event.as_job().expect("a job event").job; + assert_eq!(job.id, id); + assert_eq!(job.state, JobState::Cancelled, "{job:?}"); + tokio::time::timeout(TIMEOUT, run.stop()) + .await + .expect("the client should stop") + .unwrap(); + backend.cleanup().await; } /// A client without notifications cancels its running job once it polls the @@ -62,24 +154,11 @@ impl Backend { async fn polls_for_remote_cancellation(backend: Backend) { for while_stopping in [false, true] { let started = Arc::new(Semaphore::new(0)); - let mut workers = Workers::new(); - let worker_started = Arc::clone(&started); - workers - .add_fn(move |context: WorkContext, _job: Job| { - let started = Arc::clone(&worker_started); - async move { - started.add_permits(1); - context.cancellation_token().cancelled().await; - // Any outcome but completion becomes the cancellation. - Ok::<_, Infallible>(WorkOutcome::Snooze(Duration::from_hours(1))) - } - }) - .unwrap(); let client = Client::builder(backend.database()) .queue(riverqueue::QUEUE_DEFAULT, QueueConfig::new(1)) .without_leader_election() .without_notifications() - .workers(workers) + .workers(waiting_workers(&started)) .build() .unwrap(); let other = Client::builder(backend.database()).build().unwrap(); @@ -117,6 +196,14 @@ async fn polls_for_remote_cancellation(backend: Backend) { mod postgres { use super::*; + #[tokio::test] + async fn polls_for_lost_cancellation_notification() { + super::polls_for_lost_cancellation_notification(Backend::Postgres( + support::PostgresSchema::new("river_poll_cancel_lost").await, + )) + .await; + } + #[tokio::test] async fn polls_for_remote_cancellation() { super::polls_for_remote_cancellation(Backend::Postgres( @@ -130,6 +217,12 @@ mod postgres { mod sqlite { use super::*; + #[tokio::test] + async fn polls_for_lost_cancellation_notification() { + let (pool, path) = support::sqlite_file_pool(4).await; + super::polls_for_lost_cancellation_notification(Backend::Sqlite(pool, path)).await; + } + #[tokio::test] async fn polls_for_remote_cancellation() { let (pool, path) = support::sqlite_file_pool(4).await; From f5808198e9ec1e9e9b7276a2600cc11a50978da3 Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 30 Oct 2026 14:54:55 -0500 Subject: [PATCH 5/7] =?UTF-8?q?=F0=9F=A4=96=20[go,=20rust]=20Test=20that?= =?UTF-8?q?=20each=20mode=20polls=20for=20cancellations=20on=20its=20own?= =?UTF-8?q?=20interval?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Go's cancellation polling test set both poll intervals to the same value, so it passed even if poll-only mode used the one-minute interval meant for clients with a notifier. Set the interval each mode shouldn't use to an hour so the test fails if it's used. Rust had no test that a client with notifications waits for its interval rather than checking on every two-second tick. Add one, using the default interval, that a cancellation request without a notification isn't acted on right away while a notified one still is. --- producer_test.go | 12 +++- .../tests/poll_only_cancellation.rs | 61 +++++++++++++++++++ 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/producer_test.go b/producer_test.go index 8a6d83eae..640d2dde4 100644 --- a/producer_test.go +++ b/producer_test.go @@ -551,9 +551,17 @@ func testProducer(t *testing.T, makeProducer func(ctx context.Context, t *testin producer, bundle := setup(t) producer.config.FetchPollInterval = time.Hour - producer.config.JobCancelPollInterval = 20 * time.Millisecond producer.config.MaxWorkers = 1 - producer.config.QueuePollInterval = 20 * time.Millisecond + + // Each mode polls on its own interval. The other one is too long + // for the test to pass if it's used instead. + if producer.config.Notifier == nil { + producer.config.JobCancelPollInterval = time.Hour + producer.config.QueuePollInterval = 20 * time.Millisecond + } else { + producer.config.JobCancelPollInterval = 20 * time.Millisecond + producer.config.QueuePollInterval = time.Hour + } var jobStarted testsignal.TestSignal[int64] var workerErr testsignal.TestSignal[error] diff --git a/rust/riverqueue/tests/poll_only_cancellation.rs b/rust/riverqueue/tests/poll_only_cancellation.rs index 5dc2ebc5c..4d999a0c7 100644 --- a/rust/riverqueue/tests/poll_only_cancellation.rs +++ b/rust/riverqueue/tests/poll_only_cancellation.rs @@ -109,6 +109,53 @@ fn waiting_workers(started: &Arc) -> Workers { workers } +/// A client with notifications checks for cancellation requests only once +/// its interval (a minute by default) has passed, not on every two-second +/// tick like a client without them. A notified cancellation still arrives +/// right away. +async fn checks_for_lost_cancellations_only_after_interval(backend: Backend) { + let started = Arc::new(Semaphore::new(0)); + let client = Client::builder(backend.database()) + .queue(riverqueue::QUEUE_DEFAULT, QueueConfig::new(1)) + .without_leader_election() + .workers(waiting_workers(&started)) + .build() + .unwrap(); + let mut events = client.subscribe(&[EventKind::JobCancelled]).unwrap(); + let mut run = client.start().unwrap(); + run.wait_ready().await.unwrap(); + + let id = client.insert(WaitArgs {}).await.unwrap().job.row.id; + tokio::time::timeout(TIMEOUT, started.acquire()) + .await + .expect("the job should start") + .unwrap() + .forget(); + backend.request_cancellation_without_notification(id).await; + // This spans at least one tick, on which a client without notifications + // would have found the request. + assert!( + tokio::time::timeout(Duration::from_secs(3), events.recv()) + .await + .is_err(), + "the job shouldn't be cancelled before the interval passes" + ); + + client.jobs().cancel(id).await.unwrap(); + let event = tokio::time::timeout(TIMEOUT, events.recv()) + .await + .expect("the job should be cancelled") + .unwrap(); + let job = &event.as_job().expect("a job event").job; + assert_eq!(job.id, id); + assert_eq!(job.state, JobState::Cancelled, "{job:?}"); + tokio::time::timeout(TIMEOUT, run.stop()) + .await + .expect("the client should stop") + .unwrap(); + backend.cleanup().await; +} + /// A client with notifications still cancels its running job when the /// cancellation's notification was lost, like if it was sent while the /// client's notifier was reconnecting, because it also checks for @@ -196,6 +243,14 @@ async fn polls_for_remote_cancellation(backend: Backend) { mod postgres { use super::*; + #[tokio::test] + async fn checks_for_lost_cancellations_only_after_interval() { + super::checks_for_lost_cancellations_only_after_interval(Backend::Postgres( + support::PostgresSchema::new("river_poll_cancel_interval").await, + )) + .await; + } + #[tokio::test] async fn polls_for_lost_cancellation_notification() { super::polls_for_lost_cancellation_notification(Backend::Postgres( @@ -217,6 +272,12 @@ mod postgres { mod sqlite { use super::*; + #[tokio::test] + async fn checks_for_lost_cancellations_only_after_interval() { + let (pool, path) = support::sqlite_file_pool(4).await; + super::checks_for_lost_cancellations_only_after_interval(Backend::Sqlite(pool, path)).await; + } + #[tokio::test] async fn polls_for_lost_cancellation_notification() { let (pool, path) = support::sqlite_file_pool(4).await; From 1fa864f6e425b2f8c4e35a52665fce8008eceae1 Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 30 Oct 2026 15:21:14 -0500 Subject: [PATCH 6/7] =?UTF-8?q?=F0=9F=A4=96=20[go,=20rust]=20Cancel=20the?= =?UTF-8?q?=20polled=20attempt=20instead=20of=20looking=20up=20its=20job?= =?UTF-8?q?=20ID=20later?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cancellation poll looked up running jobs by ID, then cancelled whatever attempt was running under each requested ID once the query returned. If a job finished, was retried, and had its new attempt claimed by the same client during that window, the poll could cancel the retry even though `JobRetry` had cleared its cancellation request. Rust could also hold a requested ID as a pending cancellation for up to a minute and apply it to an attempt claimed later. Snapshot each running attempt's executor (Go) or cancellation token (Rust) before the query, and cancel the snapshotted one. Cancelling an attempt that's already finished does nothing to its job. In Go this also means the poll no longer goes through the producer's main goroutine. --- producer.go | 20 ++++++---- producer_test.go | 55 ++++++++++++++++++++++++++ rust/riverqueue/src/client/producer.rs | 31 +++++++-------- 3 files changed, 82 insertions(+), 24 deletions(-) diff --git a/producer.go b/producer.go index 00aa410f7..a660f7a78 100644 --- a/producer.go +++ b/producer.go @@ -213,7 +213,8 @@ type producer struct { startstop.BaseStartStop // Jobs which are currently being worked. The polling goroutine snapshots - // their IDs; only the main goroutine accesses the executors themselves. + // them and cancels the snapshotted executors it finds cancellation + // requests for; otherwise only the main goroutine accesses executors. activeJobsMu sync.Mutex activeJobs map[int64]*jobexecutor.JobExecutor @@ -229,8 +230,8 @@ type producer struct { pilot riverpilot.Pilot workers *Workers - // Receives job IDs to cancel. Written by notifier and polling goroutines, - // only read from the main goroutine. + // Receives job IDs to cancel. Written by the notifier goroutine, only read + // from the main goroutine. cancelCh chan int64 // Set to true when the producer thinks it should trigger another fetch as @@ -1075,9 +1076,14 @@ func (p *producer) pollForJobCancellations(ctx context.Context, wg *sync.WaitGro } func (p *producer) pollForJobCancellationsOnce(ctx context.Context) error { + // Snapshot executors rather than IDs. By the time a query returns, a job + // could have finished and been retried, with a new attempt running under + // the same ID that must not be cancelled. Cancelling an executor that's + // already finished only logs a warning. p.activeJobsMu.Lock() - jobIDs := slices.Collect(maps.Keys(p.activeJobs)) + executors := maps.Clone(p.activeJobs) p.activeJobsMu.Unlock() + jobIDs := slices.Collect(maps.Keys(executors)) // Bound query size (including SQLite's parameter count) and skip database // access entirely when there are no active jobs. @@ -1093,10 +1099,8 @@ func (p *producer) pollForJobCancellationsOnce(ctx context.Context) error { } for _, id := range cancelIDs { - select { - case <-ctx.Done(): - return ctx.Err() - case p.cancelCh <- id: + if executor := executors[id]; executor != nil { + executor.Cancel(ctx) } } } diff --git a/producer_test.go b/producer_test.go index 640d2dde4..368a7080a 100644 --- a/producer_test.go +++ b/producer_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" "github.com/riverqueue/river/internal/jobcompleter" + "github.com/riverqueue/river/internal/jobexecutor" "github.com/riverqueue/river/internal/notifier" "github.com/riverqueue/river/internal/pluginlookup" "github.com/riverqueue/river/internal/rivercommon" @@ -76,6 +77,18 @@ func (p *beforeJobGetAvailablePilot) JobGetAvailable( return p.Pilot.JobGetAvailable(ctx, exec, state, params) } +// jobGetCancelRequestedFuncExecutor replaces JobGetCancelRequested on the +// wrapped executor. +type jobGetCancelRequestedFuncExecutor struct { + riverdriver.Executor + + jobGetCancelRequestedFunc func(params *riverdriver.JobGetCancelRequestedParams) ([]int64, error) +} + +func (e *jobGetCancelRequestedFuncExecutor) JobGetCancelRequested(ctx context.Context, params *riverdriver.JobGetCancelRequestedParams) ([]int64, error) { + return e.jobGetCancelRequestedFunc(params) +} + // undecodableKindPilot reports locked jobs of one kind as undecodable. Postgres' // column types don't allow a job row that can't be decoded, so this simulates // one. @@ -622,6 +635,48 @@ func testProducer(t *testing.T, makeProducer func(ctx context.Context, t *testin } }) + // A cancellation request found by polling cancels the attempt that was + // checked, not a newer one running under the same ID by the time the + // check returns, like one started after the job was retried. + t.Run("CancellationPollingCancelsCheckedAttempt", func(t *testing.T) { + t.Parallel() + + producer, bundle := setup(t) + + var ( + job = &rivertype.JobRow{ID: 123} + checkedCause error + retriedCause error + ) + checkedExecutor := baseservice.Init(bundle.archetype, &jobexecutor.JobExecutor{ + CancelFunc: func(cause error) { checkedCause = cause }, + JobRow: job, + }) + retriedExecutor := baseservice.Init(bundle.archetype, &jobexecutor.JobExecutor{ + CancelFunc: func(cause error) { retriedCause = cause }, + JobRow: job, + }) + producer.addActiveJob(job.ID, checkedExecutor) + + producer.exec = &jobGetCancelRequestedFuncExecutor{ + Executor: producer.exec, + jobGetCancelRequestedFunc: func(params *riverdriver.JobGetCancelRequestedParams) ([]int64, error) { + require.Equal(t, []int64{job.ID}, params.ID) + + // The checked attempt finishes and a retried one starts. + producer.activeJobsMu.Lock() + producer.activeJobs[job.ID] = retriedExecutor + producer.activeJobsMu.Unlock() + + return []int64{job.ID}, nil + }, + } + + require.NoError(t, producer.pollForJobCancellationsOnce(ctx)) + require.ErrorIs(t, checkedCause, rivertype.ErrJobCancelledRemotely) + require.NoError(t, retriedCause) + }) + // A locked job whose row can't be decoded isn't worked. Its attempt fails // with the decode error, and the other jobs locked with it are worked // normally. diff --git a/rust/riverqueue/src/client/producer.rs b/rust/riverqueue/src/client/producer.rs index 59af2a5bf..4125785f1 100644 --- a/rust/riverqueue/src/client/producer.rs +++ b/rust/riverqueue/src/client/producer.rs @@ -16,9 +16,7 @@ use tokio_util::sync::CancellationToken; use tracing::{debug, error, warn}; use crate::__private::DatabaseConnection as PilotDatabaseConnection; -use crate::client::attempts::{ - FetchRegistrationGuard, register_running_attempt, signal_running_attempt, -}; +use crate::client::attempts::{FetchRegistrationGuard, register_running_attempt}; use crate::client::backoff::{RestartBackoff, exponential_backoff}; use crate::client::completer::CompletionUpdate; use crate::client::executor::{AbortOnDrop, execute_job}; @@ -164,31 +162,32 @@ async fn poll_job_cancellations(inner: Arc, stopping: CancellationT continue; } last_checked = tokio::time::Instant::now(); - let mut ids = inner + // Snapshot attempts rather than IDs. By the time a query returns, a + // job could have finished and been retried, with a new attempt + // running under the same ID that must not be cancelled. + let mut attempts = inner .running .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) - .keys() - .copied() + .iter() + .map(|(id, cancellation)| (*id, cancellation.clone())) .collect::>(); - ids.sort_unstable(); - for batch in ids.chunks(JOB_CANCEL_POLL_BATCH_SIZE) { + attempts.sort_unstable_by_key(|(id, _)| *id); + for batch in attempts.chunks(JOB_CANCEL_POLL_BATCH_SIZE) { + let ids = batch.iter().map(|(id, _)| *id).collect::>(); let requested = tokio::time::timeout( JOB_CANCEL_POLL_TIMEOUT, - crate::storage::job_cancel_requested(&inner, batch), + crate::storage::job_cancel_requested(&inner, &ids), ) .await .map_err(|_| Error::runtime_context("job cancellation poll", "timed out")) .and_then(|requested| requested); match requested { Ok(requested) => { - for id in requested { - signal_running_attempt( - &inner.running, - &inner.pending_cancellations, - &inner.fetch_registration_windows, - id, - ); + for (id, cancellation) in batch { + if requested.contains(id) { + cancellation.cancel(); + } } } Err(poll_error) => { From 367c178ef3af9b9c7d9380ed94d1990a68d6a708 Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 30 Oct 2026 15:45:26 -0500 Subject: [PATCH 7/7] =?UTF-8?q?=F0=9F=A4=96=20[go,=20java,=20js,=20rust]?= =?UTF-8?q?=20Link=20changelog=20entries=20to=20the=20pull=20request?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 2 +- java/CHANGELOG.md | 2 +- js/CHANGELOG.md | 2 +- rust/CHANGELOG.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d56944ee8..e5176eb2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Fixed a running job not being cancelled after `Client.JobCancel` when the cancellation notification was lost, as happens if it's sent while the client's notifier is reconnecting. Clients with a notifier now also check their running jobs for cancellation requests once a minute. `Client.JobRetry` now clears an earlier attempt's cancellation request so that the retried job isn't cancelled again. [Issue #1358](https://github.com/riverqueue/river/issues/1358). +- Fixed a running job not being cancelled after `Client.JobCancel` when the cancellation notification was lost, as happens if it's sent while the client's notifier is reconnecting. Clients with a notifier now also check their running jobs for cancellation requests once a minute. `Client.JobRetry` now clears an earlier attempt's cancellation request so that the retried job isn't cancelled again. [PR #1564](https://github.com/riverqueue/river/pull/1564). - Fixed `Client.InsertManyFast` panicking after its transaction committed on a client configured with queues when the driver doesn't support a listener, such as a `riverdatabasesql` driver created with `New` instead of `NewWithPgxListener`. It now wakes producers for the inserted jobs' queues right away, like `Client.InsertMany`. [PR #1558](https://github.com/riverqueue/river/pull/1558). - Fixed `Client.QueueUpdate` without metadata crashing the program when the client works that queue and the driver doesn't support a listener. [PR #1558](https://github.com/riverqueue/river/pull/1558). - Fixed the notifier's run loop blocking forever when a topic `LISTEN` or `UNLISTEN` handed to it finished after its caller had stopped waiting (after 5 seconds, with `timed out waiting for interrupt operation`), as can happen on a slow or unresponsive connection. `Client.Stop` then never completed, and neither did a `Client.Start` that failed this way. [PR #1559](https://github.com/riverqueue/river/pull/1559). diff --git a/java/CHANGELOG.md b/java/CHANGELOG.md index cf175f942..963e4c7c4 100644 --- a/java/CHANGELOG.md +++ b/java/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Fixed a cancelled job that was retried being cancelled again as soon as its new attempt started. Retrying a job now clears its earlier attempt's cancellation request, like River for Go. [Issue #1358](https://github.com/riverqueue/river/issues/1358). +- Fixed a cancelled job that was retried being cancelled again as soon as its new attempt started. Retrying a job now clears its earlier attempt's cancellation request, like River for Go. [PR #1564](https://github.com/riverqueue/river/pull/1564). ## [0.2.0] - 2026-10-26 diff --git a/js/CHANGELOG.md b/js/CHANGELOG.md index a41b1fbc2..124185cc4 100644 --- a/js/CHANGELOG.md +++ b/js/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Fixed a cancelled job that was retried being cancelled again when the client checked for cancellation requests, which it does while polling and after its notification stream reconnects. Retrying a job now clears its earlier attempt's cancellation request, like River for Go. [Issue #1358](https://github.com/riverqueue/river/issues/1358). +- Fixed a cancelled job that was retried being cancelled again when the client checked for cancellation requests, which it does while polling and after its notification stream reconnects. Retrying a job now clears its earlier attempt's cancellation request, like River for Go. [PR #1564](https://github.com/riverqueue/river/pull/1564). ## [0.3.0] - 2026-10-26 diff --git a/rust/CHANGELOG.md b/rust/CHANGELOG.md index 7215df8c9..5aa55487f 100644 --- a/rust/CHANGELOG.md +++ b/rust/CHANGELOG.md @@ -12,7 +12,7 @@ Changes to River for Go are recorded in the [repository changelog](../CHANGELOG. ### Fixed -- Fixed a running job not being cancelled when its cancellation notification was lost, as happens if it's sent while the client's listener is reconnecting. Clients with notifications now also check their running jobs for cancellation requests once a minute, like River for Go. Retrying a job now clears its earlier attempt's cancellation request so that the retried job isn't cancelled again. [Issue #1358](https://github.com/riverqueue/river/issues/1358). +- Fixed a running job not being cancelled when its cancellation notification was lost, as happens if it's sent while the client's listener is reconnecting. Clients with notifications now also check their running jobs for cancellation requests once a minute, like River for Go. Retrying a job now clears its earlier attempt's cancellation request so that the retried job isn't cancelled again. [PR #1564](https://github.com/riverqueue/river/pull/1564). ## [0.4.0] - 2026-10-26