From 0c0f22c9bbdd4ce02c1be0ed182c2eedd3880f7b Mon Sep 17 00:00:00 2001 From: Jack Danger Date: Mon, 28 Sep 2026 22:10:02 -0700 Subject: [PATCH 1/2] Test `UniqueOpts.isEmpty` agrees with its `dbunique` counterpart The public UniqueOpts.isEmpty() does not consider ExcludeKind, against the warning in its own doc comment, while dbunique.UniqueOpts.IsEmpty() in internal/dbunique does. A UniqueOpts with only ExcludeKind set is therefore treated as unset by insertParamsFromConfigArgsAndOptions, and uniqueness silently never applies. - TestUniqueOpts_isEmpty now asserts both isEmpty implementations agree for every field permutation. - TestUniqueOpts_validateExcludeKind asserts an ExcludeKind-only config is rejected (the resulting key would be constant across the whole table). - Two subtests in TestInsertParamsFromJobArgsAndOptions cover rejection via the real insert-params path and key equality across kinds with ByArgs+ExcludeKind. These tests fail until the isEmpty()/validate() fix lands. --- client_test.go | 34 ++++++++++++++++++++++++++++++++++ insert_opts_test.go | 16 ++++++++++++++++ job_test.go | 31 +++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/client_test.go b/client_test.go index 4a9ae1612..d05dda828 100644 --- a/client_test.go +++ b/client_test.go @@ -9963,6 +9963,40 @@ func TestInsertParamsFromJobArgsAndOptions(t *testing.T) { require.EqualError(t, err, "UniqueOpts.ByPeriod should not be less than 1 second") require.Nil(t, insertParams) }) + + t.Run("UniqueOptsExcludeKindOnlyValidated", func(t *testing.T) { + t.Parallel() + + insertParams, err := insertParamsFromConfigArgsAndOptions( + archetype, + config, + noOpArgs{}, + &InsertOpts{UniqueOpts: UniqueOpts{ExcludeKind: true}}, + ) + require.EqualError(t, err, "UniqueOpts.ExcludeKind requires ByArgs, ByQueue, or ByPeriod") + require.Nil(t, insertParams) + }) + + t.Run("UniqueOptsExcludeKindRemovesKindFromKey", func(t *testing.T) { + t.Parallel() + + argsKindA := JobArgsStaticKind{kind: "kind_a"} + argsKindB := JobArgsStaticKind{kind: "kind_b"} + + // With ExcludeKind, two different kinds with identical encoded args share a key. + paramsA, err := insertParamsFromConfigArgsAndOptions(archetype, config, argsKindA, &InsertOpts{UniqueOpts: UniqueOpts{ByArgs: true, ExcludeKind: true}}) + require.NoError(t, err) + paramsB, err := insertParamsFromConfigArgsAndOptions(archetype, config, argsKindB, &InsertOpts{UniqueOpts: UniqueOpts{ByArgs: true, ExcludeKind: true}}) + require.NoError(t, err) + require.Equal(t, paramsA.UniqueKey, paramsB.UniqueKey, "unique keys should be identical across kinds with ExcludeKind") + + paramsAWithKind, err := insertParamsFromConfigArgsAndOptions(archetype, config, argsKindA, &InsertOpts{UniqueOpts: UniqueOpts{ByArgs: true}}) + require.NoError(t, err) + paramsBWithKind, err := insertParamsFromConfigArgsAndOptions(archetype, config, argsKindB, &InsertOpts{UniqueOpts: UniqueOpts{ByArgs: true}}) + require.NoError(t, err) + require.NotEqual(t, paramsAWithKind.UniqueKey, paramsBWithKind.UniqueKey) + require.NotEqual(t, paramsA.UniqueKey, paramsAWithKind.UniqueKey) + }) } func TestID(t *testing.T) { diff --git a/insert_opts_test.go b/insert_opts_test.go index d15a2223c..10da19bc0 100644 --- a/insert_opts_test.go +++ b/insert_opts_test.go @@ -76,3 +76,19 @@ func TestUniqueOpts_validate(t *testing.T) { require.NoError(t, (&UniqueOpts{ByState: rivertype.JobStates()}).validate()) } + +func TestUniqueOpts_validateExcludeKind(t *testing.T) { + t.Parallel() + + require.EqualError(t, (&UniqueOpts{ExcludeKind: true}).validate(), + "UniqueOpts.ExcludeKind requires ByArgs, ByQueue, or ByPeriod") + + require.EqualError(t, (&UniqueOpts{ByState: rivertype.UniqueOptsByStateDefault(), ExcludeKind: true}).validate(), + "UniqueOpts.ExcludeKind requires ByArgs, ByQueue, or ByPeriod") + + require.NoError(t, (&UniqueOpts{ByArgs: true, ExcludeKind: true}).validate()) + require.NoError(t, (&UniqueOpts{ByQueue: true, ExcludeKind: true}).validate()) + require.NoError(t, (&UniqueOpts{ByPeriod: 10 * time.Second, ExcludeKind: true}).validate()) + require.NoError(t, (&UniqueOpts{ByArgs: true}).validate()) + require.NoError(t, (&UniqueOpts{}).validate()) +} diff --git a/job_test.go b/job_test.go index 2d9ecb656..e6314513a 100644 --- a/job_test.go +++ b/job_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/riverqueue/river/internal/dbunique" "github.com/riverqueue/river/rivertype" ) @@ -17,4 +18,34 @@ func TestUniqueOpts_isEmpty(t *testing.T) { require.False(t, (&UniqueOpts{ByPeriod: 1 * time.Nanosecond}).isEmpty()) require.False(t, (&UniqueOpts{ByQueue: true}).isEmpty()) require.False(t, (&UniqueOpts{ByState: []rivertype.JobState{rivertype.JobStateAvailable}}).isEmpty()) + + require.False(t, (&UniqueOpts{ExcludeKind: true}).isEmpty()) + + states := []rivertype.JobState{ + rivertype.JobStateAvailable, + rivertype.JobStatePending, + rivertype.JobStateRunning, + rivertype.JobStateScheduled, + } + for _, byArgs := range []bool{false, true} { + for _, byQueue := range []bool{false, true} { + for _, byPeriod := range []time.Duration{0, 10 * time.Second} { + for _, byState := range [][]rivertype.JobState{nil, states} { + for _, excludeKind := range []bool{false, true} { + opts := UniqueOpts{ + ByArgs: byArgs, + ByPeriod: byPeriod, + ByQueue: byQueue, + ByState: byState, + ExcludeKind: excludeKind, + } + internalOpts := (*dbunique.UniqueOpts)(&opts) + + require.Equal(t, internalOpts.IsEmpty(), opts.isEmpty(), + "isEmpty and internal IsEmpty should agree for opts %+v", opts) + } + } + } + } + } } From 8e3ef794b3d3a279cb58745ac60588c62832be4c Mon Sep 17 00:00:00 2001 From: Jack Danger Date: Mon, 28 Sep 2026 22:10:02 -0700 Subject: [PATCH 2/2] Fix `UniqueOpts.isEmpty` ignoring `ExcludeKind` and validate combination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public UniqueOpts.isEmpty() never considered ExcludeKind — against the explicit warning in its own doc comment — even though d977e10 added the field alongside the same option inside internal/dbunique, whose IsEmpty() does consider it. Callers passing only UniqueOpts{ExcludeKind: true} therefore had no unique key computed at all: jobs were inserted with no dedup, silently, while the option's doc promised uniqueness "across all jobs regardless of kind". - isEmpty() now counts ExcludeKind, matching dbunique.UniqueOpts.IsEmpty(). - validate() rejects the ExcludeKind-only combination: the unique key is built from kind/args/queue/period, so kind excluded and nothing else included leaves no key to dedupe by. The combination is safe to re-evaluate if a justified use shows up, per review. - The UniqueOpts.ExcludeKind doc comment states the combination requirement. Signed-off-by: Jack Danger --- CHANGELOG.md | 3 +++ insert_opts.go | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e0952855..ad19c85bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - If you're on Postgres, you can ignore it with no adverse affects. - If you're on SQLite, it rebuilds `river_job` to add an `AUTOINCREMENT` keyword to the primary key, preventing a possible edge case where generated job IDs could be reused after deletion. It's not necessary to run the migration for River to work, but it's a good idea to get it in when convenient. [PR #1390](https://github.com/riverqueue/river/pull/1390). +⚠️ **Breaking behavior change:** `UniqueOpts{ExcludeKind: true}` with no other unique fields set was previously a silent no-op; it's now rejected at insert time with an explicit error. [PR #1404](https://github.com/riverqueue/river/pull/1404). + ### Added - Added `Config.FetchOnlyKnownKinds` to restrict job fetching to registered worker kinds, including aliases. Clients with different workers can share a queue while leaving unknown jobs available without consuming attempts. Disabled by default; leader election and stuck-job rescue behavior are unchanged. [PR #1396](https://github.com/riverqueue/river/pull/1396). @@ -21,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - `UniqueOpts.ByPeriod` now derives a job's period from its effective scheduled time (`InsertOpts.ScheduledAt` when set, otherwise the insertion time), so scheduled jobs are deduplicated against other jobs scheduled in the same period rather than against jobs inserted in the same period. Periods are also now always measured in UTC, so processes and `ScheduledAt` values in different time zones produce the same unique key for the same period. Unique keys for scheduled `ByPeriod` jobs, and for any `ByPeriod` job inserted from a process whose local time zone isn't UTC, differ from those produced by previous versions. During a rolling upgrade, old and new clients may therefore each insert one job for such a period; jobs that aren't scheduled and are inserted from UTC processes are unaffected. [PR #1377](https://github.com/riverqueue/river/pull/1377). +- `UniqueOpts{ExcludeKind: true}` alone is now rejected at insert time instead of being silently ignored. `UniqueOpts.isEmpty()` now considers `ExcludeKind`, in line with the equivalent handling in internal/dbunique. [PR #1404](https://github.com/riverqueue/river/pull/1404). ### Fixed diff --git a/insert_opts.go b/insert_opts.go index d726fae40..0380862eb 100644 --- a/insert_opts.go +++ b/insert_opts.go @@ -219,6 +219,10 @@ type UniqueOpts struct { // ExcludeKind indicates that the job kind should not be included in the // uniqueness check. This is useful when you want to enforce uniqueness // across all jobs regardless of kind. + // + // Must be combined with at least one of ByArgs, ByQueue, or ByPeriod; + // otherwise the key would be constant across the entire table and the + // combination is rejected by validate. ExcludeKind bool } @@ -232,7 +236,8 @@ func (o *UniqueOpts) isEmpty() bool { return !o.ByArgs && o.ByPeriod == time.Duration(0) && !o.ByQueue && - o.ByState == nil + o.ByState == nil && + !o.ExcludeKind } var jobStateAll = rivertype.JobStates() //nolint:gochecknoglobals @@ -262,6 +267,10 @@ func (o *UniqueOpts) validate() error { return errors.New("UniqueOpts.ByPeriod should not be less than 1 second") } + if o.ExcludeKind && !o.ByArgs && !o.ByQueue && o.ByPeriod == 0 { + return errors.New("UniqueOpts.ExcludeKind requires ByArgs, ByQueue, or ByPeriod") + } + // Job states are typed, but since the underlying type is a string, users // can put anything they want in there. for _, state := range o.ByState {