From 94d5e0b52c05bb1904002add2b0b6d8f27882d39 Mon Sep 17 00:00:00 2001 From: Brandur Date: Wed, 28 May 2025 23:26:09 -0600 Subject: [PATCH] Better error in panic when accessing periodic jobs bundle in client that won't work Related to #937. If an insert-only client is created and the `PeriodicJobs()` bundle is accessed, it'll panic on a nil pointer error. The error is appropriate because if the client will never work jobs, it can never be started, which means it can never win leader election, which means that adding periodic jobs to it will never have any effect, but the bad error message is definitely not ideal. Here, detect this condition and panic with an error that better explains the situation to the caller. Fixes #937. --- CHANGELOG.md | 1 + client.go | 16 ++++++++++++++-- client_test.go | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a93fce8..213f2fd9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Correct handling an explicit schema in the reindexer maintenance service. [PR #916](https://github.com/riverqueue/river/pull/916). - Return specific explanatory error when attempting to use `JobListParams.Metadata` with `JobListTx` on SQLite. [PR #924](https://github.com/riverqueue/river/pull/924). - The reindexer now skips work if artifacts from a failed reindex are present under the assumption that if they are, a new reindex build is likely to fail again. Context cancel timeout is increased from 15 seconds to 1 minute, allowing more time for reindexes to finish. Timeout becomes configurable with `Config.ReindexerTimeout`. [PR #935](https://github.com/riverqueue/river/pull/935). +- Accessing `Client.PeriodicJobs()` on an insert-only client now panics with a more helpful explanatory error message rather than an unhelpful nil pointer panic. [PR #938](https://github.com/riverqueue/river/pull/938). ## [0.22.0] - 2025-05-10 diff --git a/client.go b/client.go index 60dbb9a0..25b4d4f1 100644 --- a/client.go +++ b/client.go @@ -2085,8 +2085,20 @@ func (c *Client[TTx]) JobListTx(ctx context.Context, tx TTx, params *JobListPara } // PeriodicJobs returns the currently configured set of periodic jobs for the -// client, and can be used to add new ones or remove existing ones. -func (c *Client[TTx]) PeriodicJobs() *PeriodicJobBundle { return c.periodicJobs } +// client, and can be used to add new or remove existing ones. +// +// This function should only be invoked on clients capable of running perioidc +// jobs. Running periodic jobs requires that the client be electable as leader +// to run maintenance services, and being electable as leader requires that a +// client be started. To be startable, a client must have Queues and Workers +// configured. Invoking this function will panic if these conditions aren't met. +func (c *Client[TTx]) PeriodicJobs() *PeriodicJobBundle { + if !c.config.willExecuteJobs() { + panic("client Queues and Workers must be configured to modify periodic jobs (otherwise, they'll have no effect because a client not configured to work jobs can't be started)") + } + + return c.periodicJobs +} // Driver exposes the underlying pilot used by the client. // diff --git a/client_test.go b/client_test.go index 9047d9d2..783ed24b 100644 --- a/client_test.go +++ b/client_test.go @@ -4479,6 +4479,20 @@ func Test_Client_Maintenance(t *testing.T) { } }) + t.Run("PeriodicJobsPanicIfClientNotConfiguredToExecuteJobs", func(t *testing.T) { + t.Parallel() + + config := newTestConfig(t, "") + config.Queues = nil + config.Workers = nil + + client := newTestClient(t, nil, config) + + require.PanicsWithValue(t, "client Queues and Workers must be configured to modify periodic jobs (otherwise, they'll have no effect because a client not configured to work jobs can't be started)", func() { + client.PeriodicJobs() + }) + }) + t.Run("PeriodicJobEnqueuerUsesMiddleware", func(t *testing.T) { t.Parallel()