-
Notifications
You must be signed in to change notification settings - Fork 5
fix: resolve codegraph CLI hangs in git worktrees (#839) #863
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0594945
fix: resolve codegraph CLI hangs in git worktrees (WAL lock contention)
carlos-alm c2b0ac3
fix: apply same worktree-aware DB path to example hooks
carlos-alm 61fe866
fix: address Greptile review — better busy logging and test coverage
carlos-alm 3b9909f
Merge main into fix/839-wal-worktree-contention
carlos-alm 239696c
Merge branch 'main' into fix/839-wal-worktree-contention
carlos-alm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| /** | ||
| * Tests that openRepo re-throws SQLITE_BUSY errors from the native engine | ||
| * instead of silently falling back to better-sqlite3 (which could hang). | ||
| */ | ||
| import fs from 'node:fs'; | ||
| import os from 'node:os'; | ||
| import path from 'node:path'; | ||
| import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| // Mock native module to simulate SQLITE_BUSY | ||
| vi.mock('../../src/infrastructure/native.js', () => ({ | ||
| isNativeAvailable: () => true, | ||
| getNative: () => ({ | ||
| NativeDatabase: { | ||
| openReadonly: () => { | ||
| throw new Error('Failed to open DB readonly: SQLITE_BUSY'); | ||
| }, | ||
| }, | ||
| }), | ||
| })); | ||
|
|
||
| import { closeDb, initSchema, openDb, openRepo } from '../../src/db/index.js'; | ||
|
|
||
| let tmpDir: string; | ||
| let dbPath: string; | ||
|
|
||
| beforeAll(() => { | ||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-busy-')); | ||
| dbPath = path.join(tmpDir, 'graph.db'); | ||
| const db = openDb(dbPath); | ||
| initSchema(db); | ||
| closeDb(db); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| if (tmpDir) fs.rmSync(tmpDir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| describe('openRepo locking error handling', () => { | ||
| it('re-throws SQLITE_BUSY as DbError instead of falling back', () => { | ||
| expect(() => openRepo(dbPath)).toThrow(/Database is busy/); | ||
| }); | ||
|
|
||
| it('thrown error is a DbError with code DB_ERROR', () => { | ||
| try { | ||
| openRepo(dbPath); | ||
| expect.unreachable('should have thrown'); | ||
| } catch (err) { | ||
| expect(err.name).toBe('DbError'); | ||
| expect(err.code).toBe('DB_ERROR'); | ||
| expect(err.message).toContain('SQLITE_BUSY'); | ||
| } | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openRepolocking-error re-throwThe new test validates
busy_timeouton readonly connections (theopenReadonlyOrFailfix), but there is no test for the second fix: the regex guard that re-throws locking errors inopenRepoinstead of silently falling back to better-sqlite3.Consider adding a test that mocks
openRepoNative(orisNativeAvailable) to throw an error withSQLITE_BUSYin the message and verifies thatopenRepore-throws aDbErrorrather than silently degrading:Without this, a regression that removes the regex guard would not be caught by the automated test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 61fe866 — added
tests/unit/openRepo-busy.test.tswhich mocks the native engine to throw SQLITE_BUSY and verifies thatopenRepore-throws it as aDbError(with codeDB_ERRORand message containingSQLITE_BUSY) instead of silently falling back. Two test cases cover both the error message check and the DbError type/code assertions.