fix: fix panic in migrator.upByOne when already migrated by a newer version

[Azorlogh]

Rolling upgrade timing/restart issues can cause a pod from an older version to start after a newer one has finished its migrations, leading to an out of bounds panic here.

Instead return AlreadyUpToDate when encountering newer versions

[coderabbitai]

Walkthrough

A guard condition in the upByOne function is modified from equality (==) to less-than-or-equal (<=), broadening when migrations are considered complete. The function now returns ErrAlreadyUpToDate when lastVersion meets or exceeds the total migration count.

Changes

Cohort / File(s)	Summary
Migration Guard Logic migrations/migrator.go	Modified guard condition in upByOne from strict equality check (len(m.migrations) == lastVersion) to inclusive comparison (len(m.migrations) <= lastVersion).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A guard was standing at the gate so tall,
Checking strict equality—it caught it all.
Now with <= in place, it sees the way,
"Already up to date!" the function will say.
A single line hops forward, wise and keen! 🎯

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main fix: preventing a panic in migrator.upByOne when migrations have already been applied by a newer version.
Description check	✅ Passed	The description is directly related to the changeset, explaining the root cause (rolling upgrade timing issues), the problem (out of bounds panic), and the solution (return AlreadyUpToDate instead).
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/migrator-upbyone-panic

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 28.71%. Comparing base (026c610) to head (e6bf4c6).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #564      +/-   ##
==========================================
- Coverage   28.75%   28.71%   -0.05%     
==========================================
  Files         175      175              
  Lines        7062     7059       -3     
==========================================
- Hits         2031     2027       -4     
  Misses       4913     4913              
- Partials      118      119       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

🧹 Nitpick comments (1)

migrations/migrator.go (1)

247-247: Consider aligning IsUpToDate with the new guard logic.

For consistency with the fix in upByOne, this condition could be changed to version >= len(m.migrations). Currently, if lastVersion > len(m.migrations) (newer version already migrated), this returns false which is semantically misleading, though safe since upByOne now handles it gracefully.

♻️ Optional: Align semantic consistency

-	return version == len(m.migrations), nil
+	return version >= len(m.migrations), nil

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@migrations/migrator.go` at line 247, The IsUpToDate check currently returns
version == len(m.migrations) which misreports cases where lastVersion >
len(m.migrations); update the condition in IsUpToDate to use version >=
len(m.migrations) so it returns true when the recorded version is at or beyond
the number of migrations (aligning semantics with the guard added to upByOne),
referencing the IsUpToDate function, the version/lastVersion variables and
m.migrations.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@migrations/migrator.go`:
- Line 247: The IsUpToDate check currently returns version == len(m.migrations)
which misreports cases where lastVersion > len(m.migrations); update the
condition in IsUpToDate to use version >= len(m.migrations) so it returns true
when the recorded version is at or beyond the number of migrations (aligning
semantics with the guard added to upByOne), referencing the IsUpToDate function,
the version/lastVersion variables and m.migrations.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6ac6ce1d-c3fc-4d08-a487-4f75e3bb37c9

📥 Commits

Reviewing files that changed from the base of the PR and between 04a6a6d and e6bf4c6.

📒 Files selected for processing (1)

• migrations/migrator.go