JobGetAvailable call has no timeout

[bgentry]

Was skimming through this to debug a customer issue. Realized that we explicitly remove all cancellation from the context, which is intentional because it's derived from work context and we don't want to halt an in-progress fetch during shutdown because we may have already locked/updated those jobs and would need to cleanly release them to avoid them needing to be rescued. With the pro pilot, we have a retry loop with individualized per-attempt timeouts applied. However, for the standard pilot, there are no additional timeouts applied at all to this method:

river/producer.go

Lines 735 to 764 in ed11967

	func (p *producer) dispatchWork(workCtx context.Context, count int, fetchResultCh chan<- producerFetchResult) {
	// This intentionally removes any deadlines or cancellation from the parent
	// context because we don't want it to get cancelled if the producer is asked
	// to shut down. In that situation, we want to finish fetching any jobs we are
	// in the midst of fetching, work them, and then stop. Otherwise we'd have a
	// risk of shutting down when we had already fetched jobs in the database,
	// leaving those jobs stranded. We'd then potentially have to release them
	// back to the queue.
	ctx := context.WithoutCancel(workCtx)
	// Maximum size of the `attempted_by` array on each job row. This maximum is
	// rarely hit, but exists to protect against degenerate cases.
	const maxAttemptedBy = 100
	jobs, err := p.pilot.JobGetAvailable(ctx, p.exec, p.state, &riverdriver.JobGetAvailableParams{
	ClientID: p.config.ClientID,
	MaxAttemptedBy: maxAttemptedBy,
	MaxToLock: count,
	Now: p.Time.NowUTCOrNil(),
	Queue: p.config.Queue,
	ProducerID: p.id.Load(),
	Schema: p.config.Schema,
	})
	if err != nil {
	fetchResultCh <- producerFetchResult{err: err}
	return
	}
	fetchResultCh <- producerFetchResult{jobs: jobs}
	}

What's your thought @brandur? Should we apply a fixed timeout to these fetches in the standard pilot so that it doesn't hang indefinitely (freezing the producer and/or blocking shutdown), or something else?

[brandur]

No strong opinion.

As I'm sure I've said a few times before, I don't recall a single time that timeouts have ever saved us from a production issue. We have very, very few high-level timeouts in our code and things are fine — in my experience, an extremely high level fallback timeout, or something at a low level, will always bubble up with an error and prevent infinite blocking. I know you've said you've experienced otherwise, but this is one I'll have to see myself to believe. I also don't love them because they make debugging very difficult. They don't produce distinct error messages, so if you get one you have to hunt down where it came from (i.e. which timeout caused this problem? there's no way to tell) and that tends to be difficult. Wrapping every operation in its own timeout also makes code quite verbose and ugly.

I guess it would make sense here to make the standard pilot consistent with the pro pilot, so if that seems to make things a little better here, seems okay.

[mitar]

I also don't love them because they make debugging very difficult. They don't produce distinct error messages, so if you get one you have to hunt down where it came from (i.e. which timeout caused this problem? there's no way to tell) and that tends to be difficult. Wrapping every operation in its own timeout also makes code quite verbose and ugly.

There is now WithTimeoutCause. I do not like it but maybe it helps you.

[brandur]

Yeah good point. It might not be a bad idea to always try and use the "with cause" variants if only for the more specific error messages.

[brandur]

Actually, I forgot the custom error isn't accessible without calling context.Cause(ctx), so to make it useful there you'd need to find a way of always making sure to invoke that in the event of detecting a context cancellation.