When there are multiple message-consumer processes running, the Lock mechanism can be used to ensure a single process takes responsibility of the scheduling.
However when the responsible process releases the lock and a new process acquires it, the state of the checkpoint is reset on purpose.

I'm not sure what the reason behind this is. Maybe I'm missing something and this PR isn't valid. But it seems strange that a Checkpoint stores it's progress which is then lost when that process stops.

Perhaps the 'reset' was more intended like a `reload', since the cached value may have changed by another process? Then this PR would fix it to be an actual reload.

Also I think the change in logic requires this to go to6.4 branch?

Jeroeny requested a review fromkbond as acode owner

October 13, 2023 09:48

carsonbot added Status: Needs Review Feature Scheduler labels

Oct 13, 2023

carsonbot added this to the6.4 milestone

Oct 13, 2023

Jeroeny changed the title~~[Scheduler] Continue with checkpoint time on lock~~[Scheduler] Continue with storedCheckpoint::$time on lock

Oct 13, 2023

Jeroeny force-pushed thefixschedulereset branch from601f9f6 tob649406Compare

October 13, 2023 09:52

Jeroeny commented

Oct 13, 2023

View reviewed changes

src/Symfony/Component/Scheduler/Generator/Checkpoint.phpShow resolvedHide resolved

fabpot added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" label

Oct 20, 2023

nicolas-grekas removed the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" label

Oct 20, 2023

Jeroeny mentioned this pull request

Oct 28, 2023

[Scheduler] Save checkpoint in a finally block#52307

Merged

Copy link

Member

nicolas-grekas commentedNov 15, 2023•
edited by OskarStark
Loading

I'm trying to understand what you mean here, I'm not deep enough into the topic 😬

Does your description also apply when there is no cache defined?

How about this patch instead?

--- a/src/Symfony/Component/Scheduler/Generator/Checkpoint.php+++ b/src/Symfony/Component/Scheduler/Generator/Checkpoint.php@@ -37,14 +37,12 @@ final class Checkpoint implements CheckpointInterface             return false;         }-        if ($this->reset) {-            $this->reset = false;-            $this->save($now, -1);-        }-         if ($this->cache) {             [$this->time, $this->index, $this->from] = $this->cache->get($this->name, fn () => [$now, -1, $now]) + [2 => $now];             $this->save($this->time, $this->index);+        } elseif ($this->reset) {+            $this->reset = false;+            $this->save($now, -1);         }

nicolas-grekas reviewed

Nov 15, 2023

View reviewed changes

src/Symfony/Component/Scheduler/CHANGELOG.md OutdatedShow resolvedHide resolved

nicolas-grekas added Bug and removed Feature labels

Nov 15, 2023

nicolas-grekas modified the milestones:6.4,7.1

Nov 15, 2023

yceruto modified the milestones:6.4,7.1

Nov 15, 2023

Jeroeny force-pushed thefixschedulereset branch fromb649406 tofe6e951Compare

November 16, 2023 12:38

Copy link

ContributorAuthor

Jeroeny commentedNov 16, 2023

Does you description also apply when there is no cache defined?

That's an interesting case. I think that may indeed be the thought behind this reset.
Because if you have two processes running without shared cache, apparently you do not want them to continue from exactly the last saved time. So maybe to prevent re-processing a certain time frame in a second worker when it has acquired a lock after another worker, it gets reset.

But, if they do have a shared cache, I think worker two can continue where worker one left off. As I'd say that is the purpose of the cache.

How about this patch instead?

With the above reasoning, that patch does indeed work. I've added a test to specifically test a second checkpoint acquiring the lock after the first and test that it continues from the saved time.