chore(rivetkit): migrate file system driver to sqlite#4206
chore(rivetkit): migrate file system driver to sqlite#4206NathanFlurry wants to merge 1 commit intomainfrom
Conversation
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4206
|
🚅 Deployed to the rivet-pr-4206 environment in rivet-frontend
|
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR Review: Migrate file system driver to SQLiteThis is a solid migration from in-memory array-based KV storage to SQLite for the file system driver. The abstraction layer, migration path, and test coverage are all well-considered. A few issues worth addressing before merging. Bug: Resource leak in destroyActor when onStop throwsIn global-state.ts, #closeActorKvDatabase is called inside the try block after actor.actor.onStop('destroy') (lines 696-698), but NOT in the finally block (lines 755-769). If onStop throws, the SQLite connection for that actor is never closed. Compare with sleepActor, where #closeActorKvDatabase is correctly placed in the finally block (line 644). The pattern should be consistent. The finally block in destroyActor should include this.#closeActorKvDatabase(actorId). Indentation inconsistenciesBoth sleepActor and destroyActor have indentation inconsistencies introduced in this PR. In sleepActor, the closing brace of try and the finally keyword are indented one level too deep relative to the try opening (line 641 vs 617). In destroyActor, lines 696-698 are indented one level deeper than the surrounding code. JavaScript does not care about indentation, so the runtime behavior is correct, but this makes the control flow hard to read. Worth fixing to avoid confusion. Statement re-preparation on every callIn sqlite-runtime.ts, createPreparedDatabaseAdapter calls prepare(sql) inside each method invocation (run, get, all). For bun:sqlite, .query() caches prepared statements internally, so this is fine. For node:sqlite and better-sqlite3, .prepare() creates a new statement object each time. Acceptable for a local dev driver, but worth noting as a future optimization if the file-system driver sees more intensive use. loadSqliteRuntime: silent failure when better-sqlite3 loads but provides no constructorIf better-sqlite3 loads without throwing but BetterSqlite3 ends up not being a function (neither the module itself nor .default), the code falls through to the final throw at the bottom. At that point, only the bun and node:sqlite errors are in loadErrors; there is no mention of why better-sqlite3 did not work. Unusual edge case, but could make debugging confusing. Minor: isKvDbPopulated uses COUNT(*)For actors with many KV entries, SELECT COUNT(*) AS count FROM kv scans the entire index. SELECT 1 FROM kv LIMIT 1 would be faster. This is only called during startup migration, so the impact is minimal. Positive observations
|
NathanFlurry
force-pushed
the
02-16-chore_rivetkit_migrate_file_system_driver_to_sqlite
branch
from
04f3cba to
ee7072e
Compare
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4206
NathanFlurry
force-pushed
the
02-16-chore_rivetkit_migrate_file_system_driver_to_sqlite
branch
from
ee7072e to
c7cc1bf
Compare
railway-app
bot
temporarily deployed
to
rivet-frontend / rivet-pr-4206
Merge activity
|
# 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
graphite-app
bot
deleted the
02-16-chore_rivetkit_migrate_file_system_driver_to_sqlite
branch
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: