refactor: Use explicit state-based pattern in migration controller #256
Conversation
thomas-waite
changed the title
BlockedUserAction state, detect stale InProgress stateBlockedUserAction state, detect stale InProgress state
thomas-waite
changed the title
BlockedUserAction state, detect stale InProgress stateBlockedUserAction state, detect stale InProgress state
thomas-waite
requested review from
andy-t-wang,
aurel-fr,
lukejmann,
murph,
paolodamico,
sideround and
tomislavhoman
as code owners
thomas-waite
changed the base branch from
main
to
feat/timeout-migration-protection
![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: d1940054fe
ℹ️ 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".
thomas-waite
changed the title
BlockedUserAction state, detect stale InProgress state
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d49bb3ffb6
ℹ️ 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".
| // Fresh InProgress - skip to avoid concurrent execution | ||
| info!( | ||
| "Migration {migration_id} currently in progress, skipping" | ||
| ); | ||
| summary.skipped += 1; |
There was a problem hiding this comment.
Skipping a “fresh” InProgress entry means a crash followed by a quick restart leaves the migration stuck for the entire session: run_migrations only executes on app start (per migration/README.md), so once you skip here there is no later retry when the stale window elapses. This regresses crash recovery—if the app restarts within 5 minutes, the migration will never retry until another app restart after the stale threshold.
Useful? React with 👍 / 👎.
thomas-waite
changed the base branch from
feat/timeout-migration-protection
to
main
thomas-waite
dismissed
paolodamico’s stale review
The base branch was changed.
2 participants
Use explicit state-based pattern in migration controller
Refactors the migration controller to use explicit state-based handling with a match statement, making the logic clearer and simpler.
Changes
MigrationStatus, making each state's behavior clear and self-documentingBlockedUserActionstate: Simplified from 5 states to 4. There's no need for a "blocked on user action" state as all planned migrations are non-interactive.Testing
All 23 tests pass in ~6.3 seconds. New tests verify:
Succeeded,FailedTerminal) are skipped correctlyFailedRetryablestateInProgresswithout timestamp is treated as stale