search

riverqueue / river

Fast and reliable background jobs in Go
https://riverqueue.com
Mozilla Public License 2.0
3.51k stars 94 forks source link

Do not insert a new job if PeriodicJobConstructor returns nil #567

Closed semanser closed 2 months ago

semanser commented 2 months ago

This commit fixes an issue with the PerioridJobConstructor ignoring the return value. According to the docs, we should ignore the job is nil is returned.

Before:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x1089c1d1]

goroutine 470 [running]:
github.com/riverqueue/river.insertParamsFromConfigArgsAndOptions(0xc0007e2270, 0xc0003a3500, {0x0, 0x0}, 0x0)
        /Users/semanser/Programming/river/client.go:1218 +0x531
github.com/riverqueue/river.(*PeriodicJobBundle).toInternal-fm.(*PeriodicJobBundle).toInternal.func1()
        /Users/semanser/Programming/river/periodic_job.go:186 +0x4e
github.com/riverqueue/river/internal/maintenance.(*PeriodicJobEnqueuer).insertParamsFromConstructor(0xc0007e2270, {0x11a6dc58, 0xc000a3a550}, 0xc000392438, {0xc1ab4192dd537df8, 0xbb93aaf1, 0x12a4f360})
        /Users/semanser/Programming/river/internal/maintenance/periodic_job_enqueuer.go:413 +0xb2
github.com/riverqueue/river/internal/maintenance.(*PeriodicJobEnqueuer).Start.func1.2(0xc0007e2270, {0xc18a41c1?, 0x12a4f360?, 0x12a4f360?}, {0x11a6dc58, 0xc000a3a550}, 0xc000a27ee8, 0xc000a27f00)
        /Users/semanser/Programming/river/internal/maintenance/periodic_job_enqueuer.go:307 +0x1eb
github.com/riverqueue/river/internal/maintenance.(*PeriodicJobEnqueuer).Start.func1()
        /Users/semanser/Programming/river/internal/maintenance/periodic_job_enqueuer.go:321 +0x3d5
created by github.com/riverqueue/river/internal/maintenance.(*PeriodicJobEnqueuer).Start in goroutine 120
        /Users/semanser/Programming/river/internal/maintenance/periodic_job_enqueuer.go:212 +0xd6

After:

2024-08-26T16:19:33.510+0200    INFO    PeriodicJobEnqueuer: nil returned from periodic job constructor, skipping
bgentry commented 2 months ago

Odd, I could have sworn we had tests for this case. Maybe it was lost during a refactor somehow 🤔 thank you for submitting, will just need to look to see if I can figure out whether we have some existing broken tests for this or if it was simply never covered.

bgentry commented 2 months ago

@semanser would you mind signing off on the project’s CLA as well? https://github.com/riverqueue/rivercla

brandur commented 2 months ago

@semanser Could you take a look at the lint failures on this one? It seems like you might be able to get away without adding a TestJobArgs for the new test case.

semanser commented 2 months ago

@brandur done!

bgentry commented 2 months ago

@semanser I brought over this PR into #572, added a lint fix for a pre-existing lint issue, and added additional high level coverage at the client level to be confident this doesn't break in the future. Thank you again for the fix! :v: