fix: sequential migrations

[jpopesculian]

Make run_up / run_down converge on partial failure

Problem

run_up applied all pending migrations concurrently (futures::future::try_join_all) and wrote the applied-ledger once at the end, only if every migration succeeded. This caused two failure modes:

• Ordering race: exec yields migrations in DAG order (dependencies first), but try_join_all drove them concurrently — a migration reading a collection an earlier one creates could see it missing.
• No convergence on partial failure: if migration k failed, migrations 1..k already ran server-side but were never recorded (insert_many was never reached). On the next run they were still Pending and ran again; any non-idempotent step (e.g. a bare create_collection) then errored forever. Downstream, aqora-platform runs Migrator::up on every boot, so a wedged ledger panicked the server on startup.

Fix

Apply migrations sequentially in the order forward_path/backward_path already yields, and write each migration to the ledger immediately after it succeeds. A mid-run failure now leaves durable partial progress, so the next run resumes at the failed migration instead of replaying — even for non-idempotent migrations. Reuses the existing insert_many/delete_many (one-element Vec); no trait or backend changes.

run_up/run_down keep their signatures (so exec is untouched) and delegate to new private, ledger-generic apply_up/apply_down helpers. run_down gets the symmetric treatment; Direction::Refresh works unchanged.

Tests

Adds a #[cfg(test)] mod tests in migrator.rs with a mock ledger + mock migrations injected through the helpers' generic seam (driven by futures::executor::block_on, no new dependency). Covers: sequential ordered recording, prefix-persisted-and-stop on up failure, same on down failure, and the missing-id error path. The failure-case tests pin the regression — they would fail against the old batched-write code.

Summary by CodeRabbit

• Bug Fixes

  • Migration runs now apply changes one at a time instead of all at once, so progress is saved immediately as each step succeeds.
  • If a migration fails, execution stops right away and previously completed steps remain recorded correctly.
  • Down migrations now fail when an expected migration record is missing, preventing incomplete rollback tracking.

• Tests

  • Expanded coverage for sequential migration behavior and failure handling.

[coderabbitai]

Warning

Review limit reached

@jpopesculian, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 45 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 450d0ef9-f3cc-498d-984e-431b73f388ad

📥 Commits

Reviewing files that changed from the base of the PR and between ba7f790 and 3169072.

📒 Files selected for processing (1)

• vectorctl-migration/src/migrator.rs

📝 Walkthrough

Walkthrough

Migration execution in vectorctl-migration was refactored from concurrent/batched processing (try_join_all with single insert_many/delete_many) to sequential per-migration processing, with the ledger updated immediately after each migration succeeds. New unit tests validate ordering and partial persistence on failure.

Changes

Sequential Migration Ledger Updates

Layer / File(s)	Summary
Sequential up migration vectorctl-migration/src/migrator.rs	run_up now calls a new apply_up helper that runs migrations in order and inserts each ledger record immediately after success, replacing the aggregated try_join_all + single insert_many flow.
Sequential down migration vectorctl-migration/src/migrator.rs	run_down now calls a new apply_down helper that runs migrations in order and deletes each ledger record immediately after success, replacing the batched rollback + single delete_many flow.
Tests for sequential behavior vectorctl-migration/src/migrator.rs	Adds MockLedger, MockMigration, and test_ctx, plus unit tests validating in-order persistence, partial persistence on up/down failure, and missing-ID error handling without deletions.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant apply_up
  participant Migration
  participant Ledger
  Caller->>apply_up: run migrations in order
  loop each migration
    apply_up->>Migration: up()
    Migration-->>apply_up: success or error
    alt success
      apply_up->>Ledger: insert ledger record
    else error
      apply_up-->>Caller: stop, return error
    end
  end

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main change: making migrations run sequentially.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jpop/fix-sequential

---

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.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

vectorctl-migration/src/migrator.rs (1)

366-450: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert migration execution order directly.

These tests currently observe ledger writes, not up/down invocation order. Add a shared call log to MockMigration and assert the exact calls, including that migrations after the failure are never invoked.

Also applies to: 453-475

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@vectorctl-migration/src/migrator.rs` around lines 366 - 450, These tests are
only checking ledger state, not the actual `up`/`down` invocation order. Add a
shared call log to `MockMigration` (or a similar test-local tracker) and record
each `MigrationTrait::up`/`down` call in order, then update
`apply_up_records_sequentially_in_order` and
`apply_up_stops_and_persists_prefix_on_failure` to assert the exact call
sequence and that migrations after the failing one are never invoked. Keep the
changes localized around `MockMigration`, `apply_up`, and the existing test
helpers like `test_ctx`.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@vectorctl-migration/src/migrator.rs`:
- Around line 262-269: Move the success print in the rollback path of the
migrator so it happens only after `ledger.delete_many` completes successfully,
and apply the same ordering to the corresponding apply path that uses
`insert_many`. In `migrator.rs`, keep the message construction in the
rollback/apply logic but defer the `println!` of “Rolled back”/“Applied” until
after the durable ledger write succeeds, so failures in `delete_many` or
`insert_many` do not emit a success status prematurely.

---

Nitpick comments:
In `@vectorctl-migration/src/migrator.rs`:
- Around line 366-450: These tests are only checking ledger state, not the
actual `up`/`down` invocation order. Add a shared call log to `MockMigration`
(or a similar test-local tracker) and record each `MigrationTrait::up`/`down`
call in order, then update `apply_up_records_sequentially_in_order` and
`apply_up_stops_and_persists_prefix_on_failure` to assert the exact call
sequence and that migrations after the failing one are never invoked. Keep the
changes localized around `MockMigration`, `apply_up`, and the existing test
helpers like `test_ctx`.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d75debd1-3574-4a9c-b75a-c63e935297fa

📥 Commits

Reviewing files that changed from the base of the PR and between 6a116a7 and ba7f790.

📒 Files selected for processing (1)

• vectorctl-migration/src/migrator.rs

[cimandef]

@jpopesculian I'll review it when I get home from my lunch

[jpopesculian]

no rush!

[cimandef]

LGTM!