Feature Request: Expose Time Control for Testing

[mastercactapus]

Description:

Our app manages scheduling, and we're migrating some workloads to River, including updating rotations (e.g., advancing to the next person at a set time) which we're looking to leverage the ScheduledAt option. In app, everything is based on the now() value to avoid clock-drift issues. In our behavioral tests, we currently manipulate PostgreSQL's now() function (via proxying) to control time, but we’ve noticed that scheduled jobs don’t run when DB time advances. Looking deeper, it appears River’s scheduling relies on system time instead of database time.

Feature Request:

Would it be possible to expose a way to control time? The river.Config has a time property that takes a baseservice.TimeGenerator—would making it public (e.g., Time) be enough? This would help with:

• Testing: Simulating different time scenarios without modifying system time.
• Production: Keeping ScheduledAt jobs aligned with database time to mitigate clock drift issues.

Potential Approaches:

1. Make Time Public: Allowing a custom time source to be set.
2. Sync to DB Time: Providing an option for River to rely on PostgreSQL's now().

Benefits:

• More reliable scheduling in both tests and production.
• Prevents early job execution due to clock drift.
• Simplifies simulating past and future time scenarios in tests.

I would love to hear your thoughts on this; thanks for considering it!

[brandur]

Yeah, definitely not against this.

My fear at the moment is that it might not work that well without some substantial refactoring. Per your two options above:

1. (Make time public) This might work fairly well, but I think we're still using a mix of the time generator and database now(), so it'd work in some places, but not always reliably.
2. (Sync to DB time) I think we've generally tried to do this wherever it's easily doable, but there are some places where it'd require an extra round trip to the DB, which isn't great. e.g. When generating a unique key using ByPeriod in job uniqueness. Also, stubbing Postgres' now() is pretty freaking hard. You guys have done it, but it's a big lift if you haven't built this out already.

@bgentry Any thoughts here? (1) seems the most doable to me, but I'm a bit afraid we'd find out a whole bunch of new bugs with it as soon as we implement it. Probably we'd need to audit every use of now() in the code base and then have it be overridable with a stubbed time if provided. Might be possible given a few hours of work.

[mastercactapus]

I think 1. would be a good start; it is virtually risk-free since there's no change if left nil, and we could comment/document it as best we can to warn that there is a mix of usage and that it may not always behave as expected. As a first step, maybe recommend it only be used for tests.

In our particular case, that would be fine -- since we'd adjust an "offset" to the actual time.Now() call for testing to match the one we push into Postgres (for ref, that's what we do for now()). I'll experiment with this to validate in the meantime though, there's always the chance that it doesn't solve the problem :).

For 2., syncing to DB time is probably worth a more extensive discussion since clock drift issues can be challenging to debug.

Example scenario: A job is scheduled to run at midnight, and the engineer's expectation is "it will wait until midnight to run," but the leader's clock is a second fast, so the job runs, then said job inserts rows with created_at set before midnight (as it's set to now()). Later they don't see the records for the current day because their timestamps are before midnight. To work around that, you'd need to start scheduling jobs for earliest_time + best_guess_at_possible_drift.

It would be valuable as a first step to scope it to scheduled jobs specifically. However, there may be other quirks that could arise without being fully solved.

[bgentry]

@brandur In the past I think we talked about stashing some test-related attrs under a sub-struct in Config—I think that would help vs including it in the Config struct directly due to it being made obvious it should not be used outside tests. I think that would be appropriate here, maybe a Test field and corresponding struct?

We should probably also ensure that anything we do here is compatible with synctest because I expect that will soon be the standard way to test concurrent code.

[brandur]

Alright, looks like @bgentry took a stab at this in #754.

@brandur In the past I think we talked about stashing some test-related attrs under a sub-struct in Config—I think that would help vs including it in the Config struct directly due to it being made obvious it should not be used outside tests. I think that would be appropriate here, maybe a Test field and corresponding struct?

WFM.