fix(routing): close original store before replacing with target store (#1215)
* fix(routing): close original store before replacing with target store Fix "database is closed" error during auto-flush when contributor routing redirects issue creation to a different repository (e.g., ~/.beads-planning). When a user is detected as a contributor (based on git remote URL pattern), the default configuration routes new issues to ~/.beads-planning instead of the current repository. This routing caused auto-flush to fail with: Warning: auto-flush failed: failed to get stored JSONL hash: failed to get jsonl_file_hash: sql: database is closed In create.go, when switching to the planning repo: 1. targetStore was opened for the planning database 2. A defer was set to close targetStore at function end 3. store = targetStore replaced the global store pointer 4. Issue was created successfully 5. Command Run ended → defer closed targetStore 6. PersistentPostRun tried to flush using the global store → error! The bug: the defer closed targetStore, but the global `store` variable pointed to the same object. When PersistentPostRun called flushManager.Shutdown(), it tried to use the already-closed store for the flush operation. - Remove the defer that prematurely closed targetStore - Explicitly close the ORIGINAL store before replacing it (it won't be used) - Let PersistentPostRun close whatever store is current at exit time This ensures proper store lifecycle: - Original store: closed when replaced (no longer needed) - Target store: closed by PersistentPostRun after flush completes 1. Have a git repo with a remote URL that doesn't match maintainer patterns (e.g., github-a:org/repo.git instead of git@github.com:org/repo.git) 2. Have ~/.beads-planning directory with beads initialized 3. Run: cd /path/to/repo && bd create "Test issue" 4. Observe: Issue created successfully, but followed by: Warning: auto-flush failed: ... sql: database is closed Added debug prints to SQLiteStorage.Close() and GetJSONLFileHash() which revealed two different databases were involved: - /path/to/repo/.beads/beads.db (closed first, unexpectedly) - ~/.beads-planning/.beads/beads.db (used for flush after being closed) * fix(test): resolve race condition in TestAutoFlushOnExit Not directly related to the previous routing fix, but exposed by CI timing. Root cause: The test had a latent race condition between two channel operations: 1. markDirtyAndScheduleFlush() sends event to markDirtyCh 2. Shutdown() sends request to shutdownCh Without any delay, Go's scheduler might process the shutdown event first. Since isDirty was still false (markDirty event not yet processed), the FlushManager would skip the flush entirely, causing the test to fail with "Expected JSONL file to be created on exit". The race was always present but only manifested in CI environments with different timing characteristics (CPU contention, virtualization, scheduler behavior). Fix: Add a 10ms sleep after markDirtyAndScheduleFlush() to allow the FlushManager's background goroutine to process the event before shutdown. This mimics real-world behavior where there's always some time between CRUD operations and process exit.
This commit is contained in:
committed by
GitHub
parent
228d78c180
commit
4ccdd7a2df
@@ -361,11 +361,14 @@ var createCmd = &cobra.Command{
|
||||
if err != nil {
|
||||
FatalError("failed to open target store: %v", err)
|
||||
}
|
||||
defer func() {
|
||||
if err := targetStore.Close(); err != nil {
|
||||
fmt.Fprintf(os.Stderr, "warning: failed to close target store: %v\n", err)
|
||||
}
|
||||
}()
|
||||
|
||||
// Close the original store before replacing it (it won't be used anymore)
|
||||
// Note: We don't defer-close targetStore here because PersistentPostRun
|
||||
// will close whatever store is assigned to the global `store` variable.
|
||||
// This fixes the "database is closed" error during auto-flush (GH#routing-close-bug).
|
||||
if store != nil {
|
||||
_ = store.Close()
|
||||
}
|
||||
|
||||
// Replace store for remainder of create operation
|
||||
// This also bypasses daemon mode since daemon owns the current repo's store
|
||||
|
||||
@@ -76,6 +76,12 @@ func TestAutoFlushOnExit(t *testing.T) {
|
||||
// Mark dirty (simulating CRUD operation)
|
||||
markDirtyAndScheduleFlush()
|
||||
|
||||
// Allow time for the FlushManager's background goroutine to process the markDirty event.
|
||||
// Without this, there's a race condition: if Shutdown() is processed before the markDirty
|
||||
// event, isDirty remains false and no flush occurs. This mimics real-world behavior where
|
||||
// there's always some time between CRUD operations and process exit.
|
||||
time.Sleep(10 * time.Millisecond)
|
||||
|
||||
// Simulate PersistentPostRun exit behavior - shutdown FlushManager
|
||||
// This performs the final flush before exit
|
||||
if err := flushManager.Shutdown(); err != nil {
|
||||
|
||||
Reference in New Issue
Block a user