Skip to content

Commit d1a55b1

Browse files
authored
Fix periodic enqueue test flakiness (#1174)
* split flaky pilot periodic test The pilot invocation test mixed startup durable-state sync with the case where a durable job is already due at startup. The short-interval job could run again before the assertions observed the pilot upsert batches, which made the expected inserts and upserts race under `-race`. Split that coverage into a startup-state test and a due-job test with longer schedules. The startup test now waits for the initial pilot upsert before shutdown so startup sync is complete before asserting the captured batch. The due-job test keeps a single durable job and waits through startup upsert, insert, and post-run upsert explicitly, which still verifies periodic metadata and `nextRunAt` advancement. This keeps the original pilot coverage while removing the timing coupling that made the combined scenario flaky. Flaky CI example: https://github.com/riverqueue/river/actions/runs/22471420916/job/65089036291 * make PilotNotInvokedWithoutID deterministic `PilotNotInvokedWithoutID` used short timer-driven jobs to prove that periodic work happened before asserting that pilot upsert and keepalive were never called for anonymous jobs. That made the test depend on how many 100 ms runs landed before the assertion rather than on the pilot behavior it was actually checking. Replace that setup with a single anonymous `RunOnStart` job and keep the exact job-count assertion. The test still proves that work was inserted, waits for the keepalive loop to tick, and then stops the service before checking the pilot-call flags. This keeps the coverage focused on "no pilot calls without IDs" while removing the scheduler timing dependency that made the test flaky. Flaky CI example: https://github.com/riverqueue/river/actions/runs/23114627657/job/67137937381
1 parent 7cc266d commit d1a55b1

1 file changed

Lines changed: 83 additions & 30 deletions

File tree

internal/maintenance/periodic_job_enqueuer_test.go

Lines changed: 83 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -822,7 +822,7 @@ func TestPeriodicJobEnqueuer(t *testing.T) {
822822
require.Equal(t, 1*time.Hour, svc.timeUntilNextRun())
823823
})
824824

825-
t.Run("InvokesPilot", func(t *testing.T) {
825+
t.Run("InvokesPilotStartupDurableState", func(t *testing.T) {
826826
t.Parallel()
827827

828828
svc, bundle := setup(t)
@@ -832,16 +832,16 @@ func TestPeriodicJobEnqueuer(t *testing.T) {
832832
bundle.pilotMock.PeriodicJobGetAllMock = func(ctx context.Context, exec riverdriver.Executor, params *riverpilot.PeriodicJobGetAllParams) ([]*riverpilot.PeriodicJob, error) {
833833
require.Equal(t, bundle.schema, params.Schema)
834834
return []*riverpilot.PeriodicJob{
835-
{ID: "periodic_job_500ms", NextRunAt: now.Add(1 * time.Hour)},
836-
{ID: "periodic_job_1500ms", NextRunAt: now.Add(2 * time.Hour)},
837-
{ID: "periodic_job_999ms", NextRunAt: now.Add(3 * time.Hour)},
835+
{ID: "pilot_startup_durable_1h", NextRunAt: now.Add(1 * time.Hour)},
836+
{ID: "pilot_startup_durable_2h", NextRunAt: now.Add(2 * time.Hour)},
837+
{ID: "pilot_unmatched_job", NextRunAt: now.Add(3 * time.Hour)},
838838
}, nil
839839
}
840840

841841
var periodicJobKeepAliveAndReapMockCalled bool
842842
bundle.pilotMock.PeriodicJobKeepAliveAndReapMock = func(ctx context.Context, exec riverdriver.Executor, params *riverpilot.PeriodicJobKeepAliveAndReapParams) ([]*riverpilot.PeriodicJob, error) {
843843
periodicJobKeepAliveAndReapMockCalled = true
844-
require.ElementsMatch(t, []string{"periodic_job_100ms", "periodic_job_500ms", "periodic_job_1500ms"}, params.ID)
844+
require.ElementsMatch(t, []string{"pilot_startup_new_job", "pilot_startup_durable_1h", "pilot_startup_durable_2h"}, params.ID)
845845
require.Equal(t, bundle.schema, params.Schema)
846846
return nil, nil
847847
}
@@ -857,40 +857,87 @@ func TestPeriodicJobEnqueuer(t *testing.T) {
857857
return nil, nil
858858
}
859859

860-
handles, err := svc.AddManySafely([]*PeriodicJob{
861-
{ID: "periodic_job_100ms", ScheduleFunc: periodicIntervalSchedule(100 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_100ms", false)},
862-
{ID: "periodic_job_500ms", ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms", false)},
863-
{ID: "periodic_job_1500ms", ScheduleFunc: periodicIntervalSchedule(1500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_1500ms", false)},
860+
_, err := svc.AddManySafely([]*PeriodicJob{
861+
{ID: "pilot_startup_new_job", ScheduleFunc: periodicIntervalSchedule(10 * time.Second), ConstructorFunc: jobConstructorFunc("pilot_startup_new_job", false)},
862+
{ID: "pilot_startup_durable_1h", ScheduleFunc: periodicIntervalSchedule(10 * time.Second), ConstructorFunc: jobConstructorFunc("pilot_startup_durable_1h", false)},
863+
{ID: "pilot_startup_durable_2h", ScheduleFunc: periodicIntervalSchedule(10 * time.Second), ConstructorFunc: jobConstructorFunc("pilot_startup_durable_2h", false)},
864864
})
865865
require.NoError(t, err)
866866

867867
startService(t, svc)
868868

869-
svc.TestSignals.InsertedJobs.WaitOrTimeout()
869+
requireNJobs(t, bundle, "pilot_startup_new_job", 0)
870+
requireNJobs(t, bundle, "pilot_startup_durable_1h", 0)
871+
requireNJobs(t, bundle, "pilot_startup_durable_2h", 0)
870872

871-
// periodic_job_100ms runs immediately because it didn't have a
872-
// persisted record from PeriodicJobGetAllMock
873-
insertedPeriodicJobs := requireNJobs(t, bundle, "periodic_job_100ms", 1)
874-
requireNJobs(t, bundle, "periodic_job_500ms", 0)
875-
requireNJobs(t, bundle, "periodic_job_1500ms", 0)
873+
svc.TestSignals.PeriodicJobUpserted.WaitOrTimeout()
874+
svc.TestSignals.PeriodicJobKeepAliveAndReap.WaitOrTimeout()
875+
require.True(t, periodicJobKeepAliveAndReapMockCalled)
876876

877-
require.Equal(t, "periodic_job_100ms", gjson.GetBytes(insertedPeriodicJobs[0].Metadata, rivercommon.MetadataKeyPeriodicJobID).Str)
877+
svc.Stop()
878878

879-
// During the first invocation periodic job records for all three jobs
880-
// are inserted (this happens on start up), then after one run we expect
881-
// only an insertion for the job that actually ran.
882879
require.Equal(t, [][]string{
883-
{"periodic_job_100ms", "periodic_job_500ms", "periodic_job_1500ms"},
884-
{"periodic_job_100ms"},
880+
{"pilot_startup_new_job", "pilot_startup_durable_1h", "pilot_startup_durable_2h"},
885881
}, insertedPeriodicJobIDs)
886882

887-
svc.TestSignals.PeriodicJobKeepAliveAndReap.WaitOrTimeout()
888-
require.True(t, periodicJobKeepAliveAndReapMockCalled)
883+
require.WithinDuration(t, now.Add(1*time.Hour), svc.periodicJobs[svc.periodicJobIDs["pilot_startup_durable_1h"]].nextRunAt, time.Microsecond)
884+
require.WithinDuration(t, now.Add(2*time.Hour), svc.periodicJobs[svc.periodicJobIDs["pilot_startup_durable_2h"]].nextRunAt, time.Microsecond)
885+
})
886+
887+
t.Run("InvokesPilotDueDurableJobRunsOnce", func(t *testing.T) {
888+
t.Parallel()
889+
890+
svc, bundle := setup(t)
891+
892+
now := time.Now()
893+
894+
bundle.pilotMock.PeriodicJobGetAllMock = func(ctx context.Context, exec riverdriver.Executor, params *riverpilot.PeriodicJobGetAllParams) ([]*riverpilot.PeriodicJob, error) {
895+
require.Equal(t, bundle.schema, params.Schema)
896+
return []*riverpilot.PeriodicJob{
897+
{ID: "pilot_due_job", NextRunAt: now.Add(-1 * time.Second)},
898+
}, nil
899+
}
900+
901+
bundle.pilotMock.PeriodicJobKeepAliveAndReapMock = func(ctx context.Context, exec riverdriver.Executor, params *riverpilot.PeriodicJobKeepAliveAndReapParams) ([]*riverpilot.PeriodicJob, error) {
902+
return nil, nil
903+
}
904+
905+
var insertedPeriodicJobIDs [][]string
906+
bundle.pilotMock.PeriodicJobUpsertManyMock = func(ctx context.Context, exec riverdriver.Executor, params *riverpilot.PeriodicJobUpsertManyParams) ([]*riverpilot.PeriodicJob, error) {
907+
insertedPeriodicJobIDs = append(insertedPeriodicJobIDs, sliceutil.Map(params.Jobs, func(j *riverpilot.PeriodicJobUpsertParams) string { return j.ID }))
908+
require.Equal(t, bundle.schema, params.Schema)
909+
for _, job := range params.Jobs {
910+
require.NotZero(t, job.NextRunAt)
911+
require.NotZero(t, job.UpdatedAt)
912+
}
913+
return nil, nil
914+
}
915+
916+
_, err := svc.AddManySafely([]*PeriodicJob{
917+
{ID: "pilot_due_job", ScheduleFunc: periodicIntervalSchedule(10 * time.Second), ConstructorFunc: jobConstructorFunc("pilot_due_job", false)},
918+
})
919+
require.NoError(t, err)
920+
921+
startService(t, svc)
922+
923+
// First upsert is startup durable-state sync.
924+
svc.TestSignals.PeriodicJobUpserted.WaitOrTimeout()
925+
svc.TestSignals.InsertedJobs.WaitOrTimeout()
926+
// Second upsert happens after the due job runs.
927+
svc.TestSignals.PeriodicJobUpserted.WaitOrTimeout()
889928

890929
svc.Stop()
891930

892-
require.WithinDuration(t, now.Add(1*time.Hour), svc.periodicJobs[handles[1]].nextRunAt, time.Microsecond)
893-
require.WithinDuration(t, now.Add(2*time.Hour), svc.periodicJobs[handles[2]].nextRunAt, time.Microsecond)
931+
insertedPeriodicJobs := requireNJobs(t, bundle, "pilot_due_job", 1)
932+
require.Equal(t, "pilot_due_job", gjson.GetBytes(insertedPeriodicJobs[0].Metadata, rivercommon.MetadataKeyPeriodicJobID).Str)
933+
934+
// First upsert is startup state sync; second is after running the due job.
935+
require.Equal(t, [][]string{
936+
{"pilot_due_job"},
937+
{"pilot_due_job"},
938+
}, insertedPeriodicJobIDs)
939+
940+
require.WithinDuration(t, now.Add(9*time.Second), svc.periodicJobs[svc.periodicJobIDs["pilot_due_job"]].nextRunAt, time.Microsecond)
894941
})
895942

896943
t.Run("PilotNotInvokedWithoutID", func(t *testing.T) {
@@ -915,19 +962,25 @@ func TestPeriodicJobEnqueuer(t *testing.T) {
915962
}
916963

917964
_, err := svc.AddManySafely([]*PeriodicJob{
918-
{ScheduleFunc: periodicIntervalSchedule(100 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_100ms", false)},
919-
{ScheduleFunc: periodicIntervalSchedule(500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_500ms", false)},
920-
{ScheduleFunc: periodicIntervalSchedule(1500 * time.Millisecond), ConstructorFunc: jobConstructorFunc("periodic_job_1500ms", false)},
965+
{
966+
ConstructorFunc: jobConstructorFunc("periodic_job_no_id_start", false),
967+
RunOnStart: true,
968+
ScheduleFunc: periodicIntervalSchedule(10 * time.Second),
969+
},
921970
})
922971
require.NoError(t, err)
923972

924973
startService(t, svc)
925974

926975
svc.TestSignals.InsertedJobs.WaitOrTimeout()
927-
requireNJobs(t, bundle, "periodic_job_100ms", 1)
928-
require.False(t, periodicJobUpsertManyMockCalled)
976+
977+
requireNJobs(t, bundle, "periodic_job_no_id_start", 1)
929978

930979
svc.TestSignals.PeriodicJobKeepAliveAndReap.WaitOrTimeout()
980+
981+
svc.Stop()
982+
983+
require.False(t, periodicJobUpsertManyMockCalled)
931984
require.False(t, periodicJobKeepAliveAndReapMockCalled)
932985
})
933986

0 commit comments

Comments
 (0)