Fix progress being overwritten to 0 when reinstantiating Progressable#20
Conversation
When connecting to an existing progress state (e.g., from a different process or after a queue job is retried) using the same `localKey` and `overallUniqueName`, the `Progressable` trait would blindly reset the local progress to 0% in the cache. This has been fixed by loading the existing state from cache instead of immediately resetting it. `setLocalKey` was also fixed to not overwrite existing target data. Co-authored-by: insign <1113045+insign@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d401957012
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isset($overallProgressData[$name])) { | ||
| $overallProgressData[$name] = $overallProgressData[$currentKey]; | ||
| } | ||
| unset($overallProgressData[$currentKey]); |
There was a problem hiding this comment.
When setLocalKey() is called and both the current key and the target key already exist, this branch skips copying data to the target but still deletes the current key. In that case, the instance keeps its old in-memory fields ($progress, metadata, status, etc.) while storage keeps the target key's data, so the object and cache diverge. A later setLocalProgress() call from this instance can then overwrite the target entry with stale values, reintroducing data loss in multi-worker or key-collision scenarios.
Useful? React with 👍 / 👎.
1 participant
This PR fixes a bug where reinstantiating a
Progressableclass with an existingoverallUniqueNameandlocalKeywould inadvertently overwrite its persistent progress state to 0% in the cache.By checking if the key already exists in the cache and hydrating the instance state accordingly before resetting, this fix ensures that background progress reporting correctly resumes across different requests or job retries. It also ensures that
setLocalKeysafely renames keys without blindly overwriting existing progress data for the target key.PR created automatically by Jules for task 1638829071095150976 started by @insign