Skip to content

Commit 9730341

Browse files
committed
Add statement_timeout on fetching available jobs
Follows up #1255 to add a `statement_timeout` in addition to the Go context timeout. `statement_timeout` will give us a better error message, and also minimizes the chances of accidentally locking rows that won't be work if we had an operation that ran long, succeeded, but then was immediately cancelled as Go's context timeout ran out. `statement_timeout` is Postgres only, so the code is a little gnarlier than would be desirable in that we add a `SetLocalStatementTimeout` function to driver `ExecutorTx`, but which is a no-op on some databases like SQLite. We try to clarify in documentation that it needs to be used in addition to context timeout, not instead of it, because it may no-op depending on the database. I also increased the timeout to 30 seconds. This matches our timeouts in the various maintenance modules, and seems a little safer as job locks on tables with huge numbers of dead rows could potentially take over 10 seconds, and maybe some users have this happening. IMO, it's too random still where we put this stuff in, but we'll have to figure that out on follow up changes. e.g. Why do we do a statement timeout for locking jobs but not for maintenance operations? Hard to justify.
1 parent 965dbad commit 9730341

10 files changed

Lines changed: 116 additions & 7 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10-
### Fixed
11-
1210
⚠️ **Breaking API change:** `rivermigrate.Migrator.Validate` and `rivermigrate.Migrator.ValidateTx` now take a `*rivermigrate.ValidateOpts` parameter. Pass `nil` to preserve previous behavior. We normally endeavor not to make any breaking API changes, but this one will keep the API in a much nicer state, and is on an ancillary function that most installations won't be using. [PR #1259](https://github.com/riverqueue/river/pull/1259)
1311

1412
### Changed
@@ -18,7 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1816

1917
### Fixed
2018

21-
- Add a 10-second timeout around `StandardPilot.JobGetAvailable` so a stalled standard-pilot fetch no longer hangs a producer indefinitely. [PR #1255](https://github.com/riverqueue/river/pull/1255)
19+
- Add a 30-second timeout around `StandardPilot.JobGetAvailable` so a stalled standard-pilot fetch no longer hangs a producer indefinitely. [PR #1255](https://github.com/riverqueue/river/pull/1255) [PR #1263](https://github.com/riverqueue/river/pull/1263)
2220
- Fixed `rivertest.Worker.Work` and `WorkJob` to honor a configured custom `Config.Schema` when transitioning a job to its running state. Previously, the running-state update ran unqualified and could fail on a connection whose `search_path` didn't include the configured schema. [PR #1262](https://github.com/riverqueue/river/pull/1262)
2321

2422
## [0.38.0] - 2026-05-22

riverdriver/river_driver_interface.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,14 @@ type ExecutorTx interface {
295295
//
296296
// API is not stable. DO NOT USE.
297297
Rollback(ctx context.Context) error
298+
299+
// SetLocalStatementTimeout sets a statement timeout local to the current
300+
// transaction if supported by the underlying database. Some databases don't
301+
// support this behavior, so this should be used in addition to context
302+
// timeouts, not instead of them.
303+
//
304+
// API is not stable. DO NOT USE.
305+
SetLocalStatementTimeout(ctx context.Context, timeout time.Duration) error
298306
}
299307

300308
type GetListenenerParams struct {

riverdriver/river_driver_interface_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,17 @@ func TestJobSetStateCancelled(t *testing.T) {
4949
})
5050
}
5151

52+
func TestPostgresStatementTimeoutValue(t *testing.T) {
53+
t.Parallel()
54+
55+
require.Equal(t, "0ms", PostgresStatementTimeoutValue(0))
56+
require.Equal(t, "1ms", PostgresStatementTimeoutValue(time.Nanosecond))
57+
require.Equal(t, "1ms", PostgresStatementTimeoutValue(999*time.Microsecond))
58+
require.Equal(t, "1ms", PostgresStatementTimeoutValue(time.Millisecond))
59+
require.Equal(t, "2ms", PostgresStatementTimeoutValue(time.Millisecond+time.Nanosecond))
60+
require.Equal(t, "1234ms", PostgresStatementTimeoutValue(1234*time.Millisecond))
61+
}
62+
5263
func TestJobSetStateCompleted(t *testing.T) {
5364
t.Parallel()
5465

riverdriver/riverdatabasesql/river_database_sql_driver.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1050,6 +1050,10 @@ func (t *ExecutorTx) Rollback(ctx context.Context) error {
10501050
return t.tx.Rollback()
10511051
}
10521052

1053+
func (t *ExecutorTx) SetLocalStatementTimeout(ctx context.Context, timeout time.Duration) error {
1054+
return t.Exec(ctx, "SELECT set_config('statement_timeout', $1, true)", riverdriver.PostgresStatementTimeoutValue(timeout))
1055+
}
1056+
10531057
type ExecutorSubTx struct {
10541058
Executor
10551059

@@ -1103,6 +1107,10 @@ func (t *ExecutorSubTx) Rollback(ctx context.Context) error {
11031107
return nil
11041108
}
11051109

1110+
func (t *ExecutorSubTx) SetLocalStatementTimeout(ctx context.Context, timeout time.Duration) error {
1111+
return t.Exec(ctx, "SELECT set_config('statement_timeout', $1, true)", riverdriver.PostgresStatementTimeoutValue(timeout))
1112+
}
1113+
11061114
func interpretError(err error) error {
11071115
if errors.Is(err, sql.ErrNoRows) {
11081116
return rivertype.ErrNotFound

riverdriver/riverdrivertest/executor_tx.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package riverdrivertest
33
import (
44
"context"
55
"testing"
6+
"time"
67

78
"github.com/stretchr/testify/require"
89

@@ -161,6 +162,27 @@ func exerciseExecutorTx[TTx any](ctx context.Context, t *testing.T,
161162
})
162163
})
163164

165+
t.Run("SetLocalStatementTimeout", func(t *testing.T) {
166+
t.Parallel()
167+
168+
exec, driver := executorWithTx(ctx, t)
169+
170+
tx, err := exec.Begin(ctx)
171+
require.NoError(t, err)
172+
t.Cleanup(func() { _ = tx.Rollback(ctx) })
173+
174+
require.NoError(t, tx.SetLocalStatementTimeout(ctx, 999*time.Microsecond))
175+
require.NoError(t, tx.SetLocalStatementTimeout(ctx, 1234*time.Millisecond))
176+
177+
if driver.DatabaseName() == databaseNameSQLite {
178+
return
179+
}
180+
181+
var timeoutMilliseconds int64
182+
require.NoError(t, tx.QueryRow(ctx, "SELECT setting::bigint FROM pg_settings WHERE name = 'statement_timeout'").Scan(&timeoutMilliseconds))
183+
require.Equal(t, int64(1234), timeoutMilliseconds)
184+
})
185+
164186
t.Run("PGAdvisoryXactLock", func(t *testing.T) {
165187
t.Parallel()
166188

riverdriver/riverpgxv5/river_pgx_v5_driver.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,6 +1024,10 @@ func (t *ExecutorTx) Rollback(ctx context.Context) error {
10241024
return t.tx.Rollback(ctx)
10251025
}
10261026

1027+
func (t *ExecutorTx) SetLocalStatementTimeout(ctx context.Context, timeout time.Duration) error {
1028+
return t.Exec(ctx, "SELECT set_config('statement_timeout', $1, true)", riverdriver.PostgresStatementTimeoutValue(timeout))
1029+
}
1030+
10271031
type Listener struct {
10281032
afterConnectExec string // should only ever be used in testing
10291033
conn *pgx.Conn

riverdriver/riversqlite/river_sqlite_driver.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1500,6 +1500,10 @@ func (t *ExecutorTx) Rollback(ctx context.Context) error {
15001500
return t.tx.Rollback()
15011501
}
15021502

1503+
func (t *ExecutorTx) SetLocalStatementTimeout(ctx context.Context, timeout time.Duration) error {
1504+
return nil
1505+
}
1506+
15031507
type ExecutorSubTx struct {
15041508
Executor
15051509

@@ -1560,6 +1564,10 @@ func (t *ExecutorSubTx) Rollback(ctx context.Context) error {
15601564
return nil
15611565
}
15621566

1567+
func (t *ExecutorSubTx) SetLocalStatementTimeout(ctx context.Context, timeout time.Duration) error {
1568+
return nil
1569+
}
1570+
15631571
func interpretError(err error) error {
15641572
if errors.Is(err, sql.ErrNoRows) {
15651573
return rivertype.ErrNotFound

riverdriver/statement_timeout.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
package riverdriver
2+
3+
import (
4+
"strconv"
5+
"time"
6+
)
7+
8+
// PostgresStatementTimeoutValue formats a duration for Postgres'
9+
// statement_timeout setting.
10+
//
11+
// Postgres accepts statement_timeout values as whole milliseconds. Round
12+
// positive sub-millisecond values up so they don't truncate to 0ms, which would
13+
// disable the timeout.
14+
func PostgresStatementTimeoutValue(timeout time.Duration) string {
15+
milliseconds := timeout / time.Millisecond
16+
if timeout > 0 && timeout%time.Millisecond != 0 {
17+
milliseconds++
18+
}
19+
20+
return strconv.FormatInt(int64(milliseconds), 10) + "ms"
21+
}

rivershared/riverpilot/standard_pilot.go

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,16 @@ package riverpilot
22

33
import (
44
"context"
5+
"fmt"
56
"sync/atomic"
67
"time"
78

89
"github.com/riverqueue/river/riverdriver"
910
"github.com/riverqueue/river/rivershared/baseservice"
11+
"github.com/riverqueue/river/rivershared/util/dbutil"
1012
"github.com/riverqueue/river/rivertype"
1113
)
1214

13-
const standardPilotJobGetAvailableTimeoutDefault = 10 * time.Second
14-
1515
type StandardPilot struct {
1616
seq atomic.Int64
1717
}
@@ -23,10 +23,24 @@ func (p *StandardPilot) JobGetAvailable(ctx context.Context, exec riverdriver.Ex
2323
return nil, nil
2424
}
2525

26-
ctx, cancel := context.WithTimeoutCause(ctx, standardPilotJobGetAvailableTimeoutDefault, context.DeadlineExceeded)
26+
// Set an outer context timeout on locking jobs, and where possible (i.e. in
27+
// Postgres, but not SQLite), set an inner `statement_timeout` inside a
28+
// transaction so the configuration isn't durable. The error from the
29+
// Postgres version will be better, so try to have that trigger first. It
30+
// also minimizes the chances of a successful operation that locks jobs but
31+
// then accidentally errors because it's run time was so close to the Go
32+
// timeout.
33+
const timeout = 30 * time.Second
34+
35+
ctx, cancel := context.WithTimeoutCause(ctx, timeout, context.DeadlineExceeded)
2736
defer cancel()
2837

29-
return exec.JobGetAvailable(ctx, params)
38+
return dbutil.WithTxV(ctx, exec, func(ctx context.Context, execTx riverdriver.ExecutorTx) ([]*rivertype.JobRow, error) {
39+
if err := execTx.SetLocalStatementTimeout(ctx, timeout-1*time.Second); err != nil {
40+
return nil, fmt.Errorf("error setting statement timeout: %w", err)
41+
}
42+
return execTx.JobGetAvailable(ctx, params)
43+
})
3044
}
3145

3246
func (p *StandardPilot) JobCancel(ctx context.Context, exec riverdriver.Executor, params *riverdriver.JobCancelParams) (*rivertype.JobRow, error) {

rivershared/riverpilot/standard_pilot_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"errors"
66
"testing"
7+
"time"
78

89
"github.com/stretchr/testify/require"
910

@@ -17,10 +18,24 @@ type standardPilotExecutorMock struct {
1718
jobGetAvailableFunc func(ctx context.Context, params *riverdriver.JobGetAvailableParams) ([]*rivertype.JobRow, error)
1819
}
1920

21+
func (m *standardPilotExecutorMock) Begin(ctx context.Context) (riverdriver.ExecutorTx, error) {
22+
return &standardPilotExecutorTxMock{standardPilotExecutorMock: m}, nil
23+
}
24+
2025
func (m *standardPilotExecutorMock) JobGetAvailable(ctx context.Context, params *riverdriver.JobGetAvailableParams) ([]*rivertype.JobRow, error) {
2126
return m.jobGetAvailableFunc(ctx, params)
2227
}
2328

29+
type standardPilotExecutorTxMock struct {
30+
*standardPilotExecutorMock
31+
}
32+
33+
func (m *standardPilotExecutorTxMock) Commit(ctx context.Context) error { return nil }
34+
func (m *standardPilotExecutorTxMock) Rollback(ctx context.Context) error { return nil }
35+
func (m *standardPilotExecutorTxMock) SetLocalStatementTimeout(ctx context.Context, timeout time.Duration) error {
36+
return nil
37+
}
38+
2439
func TestStandardPilot_JobGetAvailable(t *testing.T) {
2540
t.Parallel()
2641

0 commit comments

Comments
 (0)