fix: Thread index database migrations are now atomic #2280
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
Files not reviewed (1)
- packages/cli/package.json: Language not supported
Comments suppressed due to low confidence (2)
packages/cli/src/rpc/navie/services/threadIndexService.ts:127
- Consider wrapping the lock release call in a try-catch block to handle potential errors during the lock release process.
if (lockRelease) { await lockRelease(); }
packages/cli/src/rpc/navie/services/threadIndexService.ts:109
- Confirm that setting the journal mode outside of a transaction aligns with the intended behavior, especially under concurrent startup scenarios.
this.db.exec('PRAGMA journal_mode = WAL');
dustinbyrne
force-pushed
the
fix/thread-index-migration-lock
branch
from
9c5ef44 to
0130ef8
Compare
A single process will now migrate the database when needed. Migrations will no longer be run every time the RPC service starts.
dustinbyrne
force-pushed
the
fix/thread-index-migration-lock
branch
from
0130ef8 to
5754e31
Compare
dividedmind
approved these changes
Apr 21, 2025
| CREATE INDEX IF NOT EXISTS idx_thread_id ON project_directories (thread_id); | ||
| PRAGMA user_version = ${SCHEMA_VERSION}; |
Collaborator
There was a problem hiding this comment.
Neat! I didn't know about this feature.
appland-release
added a commit
that referenced
this pull request
Apr 22, 2025
# [@appland/appmap-v3.188.5](https://github.com/getappmap/appmap-js/compare/@appland/appmap-v3.188.4...@appland/appmap-v3.188.5) (2025-04-22) ### Bug Fixes * Thread index database migrations are now atomic ([#2280](#2280)) ([0e114d9](0e114d9))
Contributor
|
🎉 This PR is included in version @appland/appmap-v3.188.5 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We're still seeing some cases where the database is locked upon startup. This change aims to resolve this entirely by enforcing the process hold a lock file in order to migrate and setting a busy timeout of 3s on all db connections.
This change adds proper database migration handling to the thread index service. Previously, idempotent migrations were run every time the RPC service started, which could lead to race conditions when multiple instances tried to migrate simultaneously.
Now:
This change improves reliability and prevents potential database corruption when multiple AppMap instances are running.
Dependencies:
proper-lockfilefor file locking