Skip to content

fix(sqlite-storage): allow invalidate_v1_migration on non-migrating META#4813

Closed
NathanFlurry wants to merge 1 commit intoengine-stabilize/runner-envoy-testsfrom
engine-stabilize/sqlite-invalidate-v1-passthrough
Closed

fix(sqlite-storage): allow invalidate_v1_migration on non-migrating META#4813
NathanFlurry wants to merge 1 commit intoengine-stabilize/runner-envoy-testsfrom
engine-stabilize/sqlite-invalidate-v1-passthrough

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 27, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 27, 2026

PR #4813fix(sqlite-storage): allow invalidate_v1_migration on non-migrating META

What the PR does. reset_v1_migration previously called ensure!(matches!(...MigratingFromV1), ...) for both its callers (prepare_v1_migration and invalidate_v1_migration). Calling invalidate_v1_migration on an actor already in Native or MigratedFromV1 origin would hard-error. The fix replaces the single ensure! with a branch: actors not in MigratingFromV1 state return an early Ok(None) (no-op) when require_stage_in_progress == true (the invalidate_v1_migration path), and still bail! with InvalidV1MigrationState when require_stage_in_progress == false (the prepare_v1_migration path). This is correct because invalidate_v1_migration is called speculatively at migration start to clean up stale partial state, so it must tolerate actors that never needed migration or have already finished it.


Issues found

1. Em dash in comment (style violation)

engine/packages/sqlite-storage/src/open.rs line 102 contains an em dash. CLAUDE.md is explicit: "Do not use em dashes. Use periods to separate sentences instead."

Current:

// no-op — there is nothing stale to clean up. For

Should be:

// no-op. There is nothing stale to clean up. For

2. Missing test for the new behavior path

The PR adds a meaningful behavioral change (invalidate_v1_migration returning Ok(false) on Native/MigratedFromV1 actors) but no test covers it. The existing suite already has prepare_v1_migration_wipes_actor_rows_and_chunk_subkeys and the infrastructure makes adding invalidate_v1_migration_on_already_migrated_actor_returns_false straightforward. Without it, a regression that re-introduces the error would go undetected.

3. PR description checklist left blank (minor, non-blocking)

None of the template checklist items are checked.


No issues found with

  • bail! import: correct, added to the existing use anyhow::{...} line with no glob imports.
  • The else if require_stage_in_progress { return Ok(None) } path (no META at all): unchanged and still correct.
  • Return type threading: invalidate_v1_migration calls .is_some() on reset_v1_migration's Option result, so Ok(None) correctly maps to Ok(false).
  • No use of std::sync::Mutex, println!, eprintln!, or other CLAUDE.md anti-patterns.
  • Conventional commit message format is correct.

Verdict

Logically sound and minimal. Two concrete asks before merge: fix the em dash on line 102, and add a test for the invalidate_v1_migration no-op path on already-migrated actors.

@NathanFlurry NathanFlurry marked this pull request as ready for review April 27, 2026 10:02
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-envoy-tests branch from 7f0614e to 963eec7 Compare April 27, 2026 17:35
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/sqlite-invalidate-v1-passthrough branch 2 times, most recently from 02fd1d0 to 50a6834 Compare April 27, 2026 19:30
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-envoy-tests branch 2 times, most recently from 32bb640 to 2c349ab Compare April 27, 2026 19:40
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/sqlite-invalidate-v1-passthrough branch 2 times, most recently from 097bd69 to e04dbe3 Compare April 27, 2026 20:07
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-envoy-tests branch from 2c349ab to 17ad29d Compare April 27, 2026 20:07
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/sqlite-invalidate-v1-passthrough branch from e04dbe3 to a8b3d58 Compare April 27, 2026 20:30
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-envoy-tests branch 2 times, most recently from c2430c4 to f714007 Compare April 27, 2026 20:48
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/sqlite-invalidate-v1-passthrough branch 2 times, most recently from 5ae9672 to 22a83c9 Compare April 27, 2026 21:29
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-envoy-tests branch from f714007 to 009ddff Compare April 27, 2026 21:29
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-envoy-tests branch from 009ddff to 897c8a1 Compare April 27, 2026 21:38
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/sqlite-invalidate-v1-passthrough branch from 22a83c9 to 43eb873 Compare April 27, 2026 21:38
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/runner-envoy-tests branch from 897c8a1 to 99ad191 Compare April 27, 2026 22:03
@NathanFlurry NathanFlurry force-pushed the engine-stabilize/sqlite-invalidate-v1-passthrough branch from 43eb873 to 8652d5e Compare April 27, 2026 22:03
@NathanFlurry
Copy link
Copy Markdown
Member Author

Landed in main via stack-merge fast-forward push. Commits are in main; closing to match.

@github-actions
Copy link
Copy Markdown
Contributor

Preview packages published to npm

Install with:

npm install rivetkit@pr-4813

All packages published as 0.0.0-pr.4813.1264534 with tag pr-4813.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-1264534
docker pull rivetdev/engine:full-1264534
Individual packages
npm install rivetkit@pr-4813
npm install @rivetkit/react@pr-4813
npm install @rivetkit/rivetkit-napi@pr-4813
npm install @rivetkit/workflow-engine@pr-4813

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant