amadan

carlos / platform Public

carlos db: close ATTACH, clean up on a closed pipe, never print an empty error

Fixes from the whole-branch review.

query: mode=ro covers only main, and query_only is a pragma the
statement list can switch off. ATTACH opens with the connection's
read-write flags, so 'PRAGMA query_only=0; ATTACH <main> AS rw; UPDATE
rw...' wrote the copy with no error, and ATTACH or VACUUM INTO created
files anywhere the user could write. The query now runs on one
connection with SQLITE_LIMIT_ATTACHED at 0, which refuses all of them
and creates no file.

query | head: Go kills the process on a write to a closed stdout unless
SIGPIPE is caught, skipping the deferred removal of the private
directory and leaving a plaintext copy behind. pull and query now turn
Interrupt, SIGTERM, SIGHUP and SIGPIPE into context cancellation, and
the row loop stops on the first write error.

A refusal with no {"error"} body (a proxy page during a console roll)
printed an empty message; it now names the status.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 6786a58833cc651d673161b4f2cdf27a7321d1ae parent 9fe9ec2
2 files changed, +140 −7
  • db.go +44 −7
  • db_test.go +96 −0

CI passed — run details

diff --git a/db.go b/db.go
index f3f3f11b6..48cb325ba 100644
--- a/db.go
+++ b/db.go
@@ -29,8 +29,12 @@ import (
"os/signal"
"sort"
"strings"
+ "syscall"
"time"
+ "modernc.org/sqlite"
+ sqlitelib "modernc.org/sqlite/lib"
+
"github.com/carlosframework/platform/internal/dbdoor"
)
@@ -158,7 +162,7 @@ func dbRestoreError(host string, err error) error {
case errors.As(err, &door) && door.Status == 404:
return fmt.Errorf("no instance %s that you can read", host)
case errors.As(err, &door):
- return errors.New(door.Message)
+ return errors.New(door.Error())
case errors.Is(err, dbdoor.ErrNothingReplicated):
return fmt.Errorf("the platform has no copy of %s's database yet. Nothing has been replicated. "+
"If the app replicates its own database (replicate: \"app\"), the platform never will", host)
@@ -195,7 +199,7 @@ func runDBPull(args []string) {
os.Exit(2)
}
t := target()
- ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
+ ctx, stop := dbSignalContext()
defer stop()
if err := dbPull(ctx, t, *out); err != nil {
stop()
@@ -268,7 +272,7 @@ func runDBQuery(args []string) {
os.Exit(2)
}
t := target()
- ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
+ ctx, stop := dbSignalContext()
defer stop()
if err := dbQuery(ctx, t, *sqlText); err != nil {
stop()
@@ -301,8 +305,21 @@ func runQueryContext(ctx context.Context, w io.Writer, path, sqlText string) err
return err
}
defer db.Close()
- db.SetMaxOpenConns(1)
- rows, err := db.QueryContext(ctx, sqlText)
+ conn, err := db.Conn(ctx)
+ if err != nil {
+ return err
+ }
+ defer conn.Close()
+ // mode=ro covers only main, and query_only is a pragma the statement
+ // list itself can switch off. ATTACH opens with the connection's
+ // read-write flags, so it is the way around both: a second name for the
+ // copy that writes silently, or a new file anywhere the user can write.
+ // VACUUM INTO attaches internally. No attached databases at all closes
+ // every one of those.
+ if _, err := sqlite.Limit(conn, sqlitelib.SQLITE_LIMIT_ATTACHED, 0); err != nil {
+ return err
+ }
+ rows, err := conn.QueryContext(ctx, sqlText)
if err != nil {
return err
}
@@ -312,7 +329,9 @@ func runQueryContext(ctx context.Context, w io.Writer, path, sqlText string) err
return err
}
if len(cols) > 0 {
- fmt.Fprintln(w, strings.Join(cols, "\t"))
+ if _, err := fmt.Fprintln(w, strings.Join(cols, "\t")); err != nil {
+ return err
+ }
}
vals := make([]any, len(cols))
ptrs := make([]any, len(cols))
@@ -327,7 +346,11 @@ func runQueryContext(ctx context.Context, w io.Writer, path, sqlText string) err
for i, v := range vals {
line[i] = dbQueryCell(v)
}
- fmt.Fprintln(w, strings.Join(line, "\t"))
+ // A reader that went away (EPIPE, once SIGPIPE is caught) ends the
+ // query here rather than after every remaining row.
+ if _, err := fmt.Fprintln(w, strings.Join(line, "\t")); err != nil {
+ return err
+ }
}
return rows.Err()
}
@@ -349,3 +372,17 @@ func dbQueryCell(v any) string {
}
return strings.NewReplacer("\\", `\\`, "\t", `\t`, "\n", `\n`, "\r", `\r`).Replace(s)
}
+
+// dbStopSignals end a db verb early. Each one cancels the context instead of
+// killing the process, so the deferred removal of the private directory
+// always runs: Ctrl-C, a closed terminal (SIGHUP), a supervisor's SIGTERM,
+// and SIGPIPE. The last is the one that matters most: `carlos db query ...
+// | head` closes stdout early, and with SIGPIPE left to the runtime Go
+// kills the process on the next write, which would leave a plaintext copy
+// of production in the temp directory. Caught, the write returns EPIPE
+// instead and the query loop stops on it.
+var dbStopSignals = []os.Signal{os.Interrupt, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGPIPE}
+
+func dbSignalContext() (context.Context, context.CancelFunc) {
+ return signal.NotifyContext(context.Background(), dbStopSignals...)
+}
diff --git a/db_test.go b/db_test.go
index 4d76cb8ed..7dbf11c13 100644
--- a/db_test.go
+++ b/db_test.go
@@ -9,10 +9,13 @@ import (
"context"
"crypto/sha256"
"database/sql"
+ "errors"
"net/http"
"net/http/httptest"
"os"
+ "os/exec"
"path/filepath"
+ "runtime"
"strings"
"testing"
@@ -252,6 +255,9 @@ func TestDBRestoreErrorsNameTheirCause(t *testing.T) {
}{
{&dbdoor.DoorError{Status: 409, Message: "a.go.tito.io keeps its data in region uk."}, "region uk"},
{&dbdoor.DoorError{Status: 404}, "no instance"},
+ // A proxy's HTML page during a console roll carries no sentence;
+ // the status still has to reach the person.
+ {&dbdoor.DoorError{Status: 502}, "502"},
{dbdoor.ErrNothingReplicated, `replicate: "app"`},
{dbdoor.ErrCheckFailed, "integrity check"},
{os.ErrNotExist, "changed while it was being read"},
@@ -262,3 +268,93 @@ func TestDBRestoreErrorsNameTheirCause(t *testing.T) {
}
}
}
+
+// mode=ro covers only main, and query_only is a pragma a statement list can
+// switch off. ATTACH opens with the connection's read-write flags, so it is
+// the door around both: it could write the copy silently through a second
+// name, or create and fill a file anywhere the user can write. VACUUM INTO
+// attaches internally. All of them must fail, and leave no file behind.
+func TestQueryCannotAttachOrWriteFilesElsewhere(t *testing.T) {
+ path := fixtureDB(t)
+ elsewhere := t.TempDir()
+ stray := filepath.Join(elsewhere, "stray.db")
+ for _, stmt := range []string{
+ "PRAGMA query_only=0; ATTACH (SELECT file FROM pragma_database_list WHERE name='main') AS rw; UPDATE rw.users SET name='x'",
+ "ATTACH '" + stray + "' AS e",
+ "PRAGMA query_only=0; ATTACH '" + stray + "' AS e; CREATE TABLE e.t (x); INSERT INTO e.t VALUES (1)",
+ "VACUUM INTO '" + stray + "'",
+ "PRAGMA query_only=0; VACUUM INTO '" + stray + "'",
+ } {
+ if err := runQuery(&bytes.Buffer{}, path, stmt); err == nil {
+ t.Errorf("%s: succeeded, want a refusal", stmt)
+ }
+ if _, err := os.Stat(stray); !os.IsNotExist(err) {
+ t.Fatalf("%s: left a file at %s", stmt, stray)
+ }
+ }
+ var out bytes.Buffer
+ if err := runQuery(&out, path, "SELECT name FROM users ORDER BY id"); err != nil || out.String() != "name\nada\ngrace\n" {
+ t.Fatalf("rows after = %v %q, want them unchanged", err, out.String())
+ }
+}
+
+// errWriter fails every write, the way a closed pipe does.
+type errWriter struct{ n int }
+
+func (w *errWriter) Write([]byte) (int, error) { w.n++; return 0, errors.New("broken pipe") }
+
+// A reader that went away is an error the query loop stops on, not one it
+// prints a thousand more rows into.
+func TestQueryStopsWhenTheReaderGoesAway(t *testing.T) {
+ w := &errWriter{}
+ if err := runQuery(w, fixtureDB(t), "SELECT * FROM users"); err == nil {
+ t.Fatal("a failing writer was not reported")
+ }
+ if w.n != 1 {
+ t.Fatalf("kept writing after the first failure: %d writes", w.n)
+ }
+}
+
+// `carlos db query ... | head` closes stdout early. Without SIGPIPE caught,
+// Go kills the process on the next write and every deferred cleanup is
+// skipped, leaving a plaintext copy of production in the temp directory.
+// The helper below runs dbSignalContext's handling in a child whose stdout
+// is a pipe nobody reads; it must get EPIPE back, clean up, and exit 0.
+func TestQueryCleansUpWhenStdoutIsClosed(t *testing.T) {
+ if os.Getenv("CARLOS_DB_PIPE_HELPER") != "" {
+ ctx, stop := dbSignalContext()
+ defer stop()
+ dir := os.Getenv("CARLOS_DB_PIPE_HELPER")
+ for i := 0; i < 1<<20 && ctx.Err() == nil; i++ {
+ if _, err := os.Stdout.Write([]byte("row\n")); err != nil {
+ break
+ }
+ }
+ os.RemoveAll(dir)
+ os.Exit(0)
+ }
+ if runtime.GOOS == "windows" {
+ t.Skip("no SIGPIPE on windows")
+ }
+ dir := t.TempDir()
+ private := filepath.Join(dir, "carlos-db-x")
+ if err := os.Mkdir(private, 0o700); err != nil {
+ t.Fatal(err)
+ }
+ r, w, err := os.Pipe()
+ if err != nil {
+ t.Fatal(err)
+ }
+ r.Close() // the reader has gone, like head after its lines
+ cmd := exec.Command(os.Args[0], "-test.run=^TestQueryCleansUpWhenStdoutIsClosed$")
+ cmd.Env = append(os.Environ(), "CARLOS_DB_PIPE_HELPER="+private)
+ cmd.Stdout = w
+ err = cmd.Run()
+ w.Close()
+ if err != nil {
+ t.Fatalf("helper died (%v): the write killed it before cleanup could run", err)
+ }
+ if _, err := os.Stat(private); !os.IsNotExist(err) {
+ t.Fatalf("private directory survived a closed stdout (%v)", err)
+ }
+}