feat: add timeout protections to migration controller #255
Conversation
thomas-waite
requested review from
andy-t-wang,
aurel-fr,
lukejmann,
murph,
paolodamico,
sideround and
tomislavhoman
as code owners
|
@codex review |
![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: 1dd4a02631
ℹ️ 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".
| // Execute with timeout to prevent indefinite hangs | ||
| let execute_result = tokio::time::timeout( | ||
| tokio::time::Duration::from_secs(MIGRATION_TIMEOUT_SECS), | ||
| processor.execute(), | ||
| ) |
There was a problem hiding this comment.
The new timeout wraps processor.execute() but does not guarantee the underlying work stops; tokio::time::timeout only drops the future. If an implementation uses blocking work or spawns background tasks, the migration can keep running after the timeout while the controller immediately continues to the next migration. That breaks the controller’s sequential‑execution guarantee and can lead to concurrent migrations mutating shared state. Consider running execute in a task that can be explicitly aborted/joined on timeout, or otherwise enforce that timed‑out work is fully stopped before proceeding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Documented that migrations should not spawn background tasks
1 participant
Add timeout protections to migration controller
The Migration
controller.rsdoes not currently have timeout protections in case a migration hangs indefinitely. This PR adds them, and enforces a limit of 20seconds per migration.If the migration takes longer than 20seconds, it is failed and marked retryable. Exponential backoff is used to determine the next time it can be retried.