Fix CI failure: DB Reopen race and install test drift

internal/db/db.go

@@ -6,6 +6,7 @@ import (
 	_ "embed"
 	"errors"
 	"fmt"
+	"log"
 	"net/url"
 	"os"
 	"path/filepath"
@@ -48,10 +49,11 @@ END;
 // concurrent HTTP handler goroutines can safely read while
 // Reopen/CloseConnections swap the underlying *sql.DB.
 type DB struct {
-	path   string
-	writer atomic.Pointer[sql.DB]
-	reader atomic.Pointer[sql.DB]
-	mu     sync.Mutex // serializes writes
+	path    string
+	writer  atomic.Pointer[sql.DB]
+	reader  atomic.Pointer[sql.DB]
+	mu      sync.Mutex // serializes writes
+	retired []*sql.DB  // old pools kept open for in-flight reads
 
 	cursorMu     sync.RWMutex
 	cursorSecret []byte
@@ -359,24 +361,40 @@ func (db *DB) init() error {
 	return nil
 }
 
-// Close closes both writer and reader connections.
+// Close closes both writer and reader connections, plus any
+// retired pools left over from previous Reopen calls.
 func (db *DB) Close() error {
-	return errors.Join(
-		db.getWriter().Close(),
-		db.getReader().Close(),
-	)
+	db.mu.Lock()
+	w := db.getWriter()
+	r := db.getReader()
+	retired := db.retired
+	db.retired = nil
+	db.mu.Unlock()
+
+	errs := []error{w.Close(), r.Close()}
+	for _, p := range retired {
+		errs = append(errs, p.Close())
+	}
+	return errors.Join(errs...)
 }
 
 // CloseConnections closes both connections without reopening,
 // releasing file locks so the database file can be renamed.
+// Also drains any retired pools from previous Reopen calls.
 // Callers must call Reopen afterwards to restore service.
 func (db *DB) CloseConnections() error {
 	db.mu.Lock()
 	defer db.mu.Unlock()
-	return errors.Join(
+
+	errs := []error{
 		db.getWriter().Close(),
 		db.getReader().Close(),
-	)
+	}
+	for _, p := range db.retired {
+		errs = append(errs, p.Close())
+	}
+	db.retired = nil
+	return errors.Join(errs...)
 }
 
 // Reopen closes and reopens both connections to the same
@@ -409,10 +427,26 @@ func (db *DB) reopenLocked() error {
 	}
 	reader.SetMaxOpenConns(4)
 
+	// Close pools from any previous reopen. They have been
+	// retired for at least one full Reopen cycle, so all
+	// in-flight queries on them have long since completed.
+	for _, p := range db.retired {
+		if err := p.Close(); err != nil {
+			log.Printf(
+				"warning: closing retired db pool: %v", err,
+			)
+		}
+	}
+	db.retired = db.retired[:0]
+
 	oldWriter := db.writer.Swap(writer)
 	oldReader := db.reader.Swap(reader)
-	_ = oldWriter.Close()
-	_ = oldReader.Close()
+
+	// Retire the just-swapped pools. Concurrent readers that
+	// loaded the old pointer before the swap may still have
+	// in-flight queries; these pools will be closed on the
+	// next Reopen, CloseConnections, or Close call.
+	db.retired = append(db.retired, oldWriter, oldReader)
 	return nil
 }
 

internal/db/db_test.go

@@ -2475,6 +2475,58 @@ func TestConcurrentReadsWhileReopen(t *testing.T) {
 	}
 }
 
+func TestRepeatedReopenBoundsRetiredPools(t *testing.T) {
+	d := testDB(t)
+	insertSession(t, d, "s1", "proj")
+
+	// Reopen many times; retired pools from earlier rounds
+	// should be closed by subsequent reopens, keeping only
+	// the most recent pair alive.
+	for range 20 {
+		if err := d.Reopen(); err != nil {
+			t.Fatalf("Reopen: %v", err)
+		}
+	}
+
+	// After 20 reopens the retired slice should hold at most
+	// the last pair (2 entries), not 40.
+	d.mu.Lock()
+	n := len(d.retired)
+	d.mu.Unlock()
+	if n > 2 {
+		t.Errorf("retired pool count = %d, want <= 2", n)
+	}
+
+	// Data should still be readable.
+	s, err := d.GetSession(context.Background(), "s1")
+	if err != nil {
+		t.Fatalf("GetSession: %v", err)
+	}
+	if s == nil {
+		t.Error("session s1 missing after repeated Reopen")
+	}
+}
+
+func TestCloseAfterCloseConnectionsReopen(t *testing.T) {
+	d := testDB(t)
+	insertSession(t, d, "s1", "proj")
+
+	// CloseConnections + Reopen is the normal resync lifecycle.
+	if err := d.CloseConnections(); err != nil {
+		t.Fatalf("CloseConnections: %v", err)
+	}
+	if err := d.Reopen(); err != nil {
+		t.Fatalf("Reopen: %v", err)
+	}
+
+	// Close should succeed without "database is closed" errors
+	// from double-closing the pools that CloseConnections
+	// already closed.
+	if err := d.Close(); err != nil {
+		t.Fatalf("Close: %v", err)
+	}
+}
+
 func TestCopyInsightsFrom(t *testing.T) {
 	dir := t.TempDir()
 

scripts/install.sh

@@ -199,4 +199,9 @@ main() {
     echo "  agentsview update   # Check for and install updates"
 }
 
-main "$@"
+# Guard: only run main when executed directly, not when sourced.
+# ${BASH_SOURCE[0]-} defaults to empty when piped via stdin
+# (curl ... | bash), which we treat as direct execution.
+if [[ "${BASH_SOURCE[0]-}" == "${0}" || -z "${BASH_SOURCE[0]-}" ]]; then
+    main "$@"
+fi

scripts/install_test.sh

@@ -1,7 +1,16 @@
 #!/bin/bash
-# Tests for install.sh version parsing logic
+# Tests for install.sh version parsing logic.
+# Sources install.sh directly so the test exercises the real
+# get_latest_version function (with curl mocked out).
 set -euo pipefail
 
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+
+# Source install.sh to get access to get_latest_version.
+# The main() call is guarded so nothing runs on source.
+# shellcheck source=install.sh
+source "$SCRIPT_DIR/install.sh"
+
 PASS=0
 FAIL=0
 
@@ -18,42 +27,42 @@ assert_eq() {
     fi
 }
 
-parse_tag_name() {
-    echo "$1" \
-        | grep -o '"tag_name"[[:space:]]*:[[:space:]]*"[^"]*"' \
-        | head -1 \
-        | cut -d'"' -f4
-}
+# Mock curl to return a fixture instead of hitting the network.
+# get_latest_version uses `curl -fsSL "$url"` so we intercept
+# that and emit $MOCK_JSON.
+MOCK_JSON=""
+curl() { printf '%s\n' "$MOCK_JSON"; }
+export -f curl
 
 echo "=== get_latest_version parsing ==="
 
 # Pretty-printed JSON (typical curl response)
-PRETTY='{
+MOCK_JSON='{
   "url": "https://api.github.com/repos/wesm/agentsview/releases/291105519",
   "tag_name": "v0.8.0",
   "name": "v0.8.0"
 }'
-assert_eq "pretty-printed JSON" "v0.8.0" "$(parse_tag_name "$PRETTY")"
+assert_eq "pretty-printed JSON" "v0.8.0" "$(get_latest_version)"
 
 # Minified JSON (the case that caused #61)
-MINIFIED='{"url":"https://api.github.com/repos/wesm/agentsview/releases/291105519","assets_url":"https://api.github.com/repos/wesm/agentsview/releases/291105519/assets","tag_name":"v0.8.0","name":"v0.8.0"}'
-assert_eq "minified JSON" "v0.8.0" "$(parse_tag_name "$MINIFIED")"
+MOCK_JSON='{"url":"https://api.github.com/repos/wesm/agentsview/releases/291105519","assets_url":"https://api.github.com/repos/wesm/agentsview/releases/291105519/assets","tag_name":"v0.8.0","name":"v0.8.0"}'
+assert_eq "minified JSON" "v0.8.0" "$(get_latest_version)"
 
 # tag_name before url field
-REORDERED='{"tag_name":"v1.2.3","url":"https://api.github.com/repos/wesm/agentsview/releases/1"}'
-assert_eq "tag_name before url" "v1.2.3" "$(parse_tag_name "$REORDERED")"
+MOCK_JSON='{"tag_name":"v1.2.3","url":"https://api.github.com/repos/wesm/agentsview/releases/1"}'
+assert_eq "tag_name before url" "v1.2.3" "$(get_latest_version)"
 
 # Extra whitespace around colon
-SPACED='{  "tag_name" :  "v2.0.0"  }'
-assert_eq "extra whitespace" "v2.0.0" "$(parse_tag_name "$SPACED")"
+MOCK_JSON='{  "tag_name" :  "v2.0.0"  }'
+assert_eq "extra whitespace" "v2.0.0" "$(get_latest_version)"
 
 # Pre-release version
-PRERELEASE='{"tag_name":"v0.9.0-rc1","name":"v0.9.0-rc1"}'
-assert_eq "pre-release version" "v0.9.0-rc1" "$(parse_tag_name "$PRERELEASE")"
+MOCK_JSON='{"tag_name":"v0.9.0-rc1","name":"v0.9.0-rc1"}'
+assert_eq "pre-release version" "v0.9.0-rc1" "$(get_latest_version)"
 
 # No tag_name field (API error / rate limit)
-NO_TAG='{"message":"API rate limit exceeded"}'
-assert_eq "missing tag_name returns empty" "" "$(parse_tag_name "$NO_TAG")"
+MOCK_JSON='{"message":"API rate limit exceeded"}'
+assert_eq "missing tag_name returns empty" "" "$(get_latest_version)"
 
 echo
 echo "Results: $PASS passed, $FAIL failed"