From 938e5f4ee5f47a89c44f670009fc9e8121887e1d Mon Sep 17 00:00:00 2001
From: "M. J. Fromberger" <fromberger@tailscale.com>
Date: Thu, 1 Aug 2024 07:14:40 -0700
Subject: [PATCH] server: restore separate connections for the local state db
 (#29)

In #25 I switched away from using a transaction for queries on the local state
database, because it did not interact properly with the queryable interface we
added in #24. But because I'd switched away from having a separate read-only
connection for these queries, this had the undesirable side-effect of allowing
write queries on the local state database.

Switch back to using a separate read-only connection for local state.  Morally
this is a partial revert of #24, but done in a slightly different way.
---
 server/tailsql/local.go   | 37 ++++++++++++++++++++++++++++---------
 server/tailsql/options.go |  9 +--------
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/server/tailsql/local.go b/server/tailsql/local.go
index 60eb519..1330ecd 100644
--- a/server/tailsql/local.go
+++ b/server/tailsql/local.go
@@ -8,6 +8,7 @@ import (
 	"database/sql"
 	"errors"
 	"fmt"
+	"strings"
 	"sync"
 	"time"
 
@@ -42,17 +43,35 @@ var schema = &squibble.Schema{
 type localState struct {
 	// Exclusive: Write transaction
 	// Shared: Read transaction
-	txmu sync.RWMutex
-	db   *sql.DB
+	txmu   sync.RWMutex
+	rw, ro *sql.DB
 }
 
-// newLocalState constructs a new LocalState helper using the given database.
-func newLocalState(db *sql.DB) (*localState, error) {
-	if err := schema.Apply(context.Background(), db); err != nil {
-		db.Close()
+// newLocalState constructs a new LocalState helper for the given database URL.
+func newLocalState(url string) (*localState, error) {
+	if !strings.HasPrefix(url, "file:") {
+		url = "file:" + url
+	}
+	urlRO := url + "?mode=ro"
+
+	// Open separate copies of the database for writing query logs vs. serving
+	// queries to the UI.
+	rw, err := openAndPing("sqlite", url)
+	if err != nil {
+		return nil, err
+	}
+	if err := schema.Apply(context.Background(), rw); err != nil {
+		rw.Close()
 		return nil, fmt.Errorf("initializing schema: %w", err)
 	}
-	return &localState{db: db}, nil
+
+	ro, err := openAndPing("sqlite", urlRO)
+	if err != nil {
+		rw.Close()
+		return nil, err
+	}
+
+	return &localState{rw: rw, ro: ro}, nil
 }
 
 // LogQuery adds the specified query to the query log.
@@ -67,7 +86,7 @@ func (s *localState) LogQuery(ctx context.Context, user string, q Query, elapsed
 	}
 	s.txmu.Lock()
 	defer s.txmu.Unlock()
-	tx, err := s.db.BeginTx(ctx, nil)
+	tx, err := s.rw.BeginTx(ctx, nil)
 	if err != nil {
 		return err
 	}
@@ -98,7 +117,7 @@ func (s *localState) LogQuery(ctx context.Context, user string, q Query, elapsed
 func (s *localState) Query(ctx context.Context, query string, params ...any) (RowSet, error) {
 	s.txmu.RLock()
 	defer s.txmu.RUnlock()
-	return s.db.QueryContext(ctx, query, params...)
+	return s.ro.QueryContext(ctx, query, params...)
 }
 
 // Close satisfies part of the Queryable interface.  For this database the
diff --git a/server/tailsql/options.go b/server/tailsql/options.go
index 3a946f6..513e90f 100644
--- a/server/tailsql/options.go
+++ b/server/tailsql/options.go
@@ -205,14 +205,7 @@ func (o Options) localState() (*localState, error) {
 		return nil, nil
 	}
 	url := os.ExpandEnv(o.LocalState)
-	db, err := sql.Open("sqlite", url)
-	if err != nil {
-		return nil, fmt.Errorf("open %q: %w", url, err)
-	} else if err := db.PingContext(context.Background()); err != nil {
-		db.Close()
-		return nil, fmt.Errorf("ping %q: %w", url, err)
-	}
-	return newLocalState(db)
+	return newLocalState(url)
 }
 
 func (o Options) routePrefix() string {