fix: make schema migrations replayable (#1489)

This commit is contained in:
Carlos Florêncio
2026-06-25 10:28:42 +01:00
committed by GitHub
parent 6683a7005d
commit a28c4226de
11 changed files with 369 additions and 29 deletions
+1
View File
@@ -206,6 +206,7 @@ Every long-running goroutine must have a single owner with explicit start and st
- Boot aborts if the backup fails — the pool is closed and `Provide` returns an error.
- After all repos complete `initSchema`, `cmd/kandev/storage.go:recordSchemaVersion` writes the current binary version into `kandev_meta` (non-fatal; a failure just means the next boot will take a fresh snapshot).
- Migration logging: `db.MigrateLogger.Apply(name, stmt)` — success logs Info, "already exists" / "duplicate column name" is silently swallowed, anything else logs Warn but never returns an error (preserving the existing swallow-error contract).
- Schema replay handling: use `internal/db` helpers such as `IsDuplicateColumnError` / `IsAlreadyExistsError` instead of local error-string matching. When adding or changing startup schema code, include fresh-DB plus same-DB replay tests for SQLite; add the same env-gated Postgres replay coverage when the path supports Postgres. See `docs/decisions/0027-replayable-schema-migrations.md`.
## Schema & migrations (SQLite repository)
+1 -12
View File
@@ -1,8 +1,6 @@
package db
import (
"strings"
"github.com/jmoiron/sqlx"
"go.uber.org/zap"
@@ -34,7 +32,7 @@ func NewMigrateLogger(db *sqlx.DB, log *logger.Logger) *MigrateLogger {
// `_, _ = db.Exec(...)` pattern, with observability added.
func (m *MigrateLogger) Apply(name, stmt string) {
if _, err := m.db.Exec(stmt); err != nil {
if isAlreadyExists(err) {
if IsAlreadyExistsError(err) {
return
}
if m.log != nil {
@@ -47,12 +45,3 @@ func (m *MigrateLogger) Apply(name, stmt string) {
m.log.Info("migration applied", zap.String("name", name))
}
}
// isAlreadyExists returns true when err describes a condition that means
// the migration has already been applied. SQLite uses these error strings
// for idempotent schema operations.
func isAlreadyExists(err error) bool {
s := err.Error()
return strings.Contains(s, "duplicate column name") ||
strings.Contains(s, "already exists")
}
@@ -0,0 +1,56 @@
package db
import (
"errors"
"strings"
"github.com/jackc/pgx/v5/pgconn"
)
const (
postgresDuplicateColumn = "42701"
postgresDuplicateTable = "42P07"
postgresDuplicateObject = "42710"
sqliteAlreadyExistsText = " already exists"
)
// IsDuplicateColumnError reports whether err means an ADD COLUMN migration has
// already been applied.
func IsDuplicateColumnError(err error) bool {
if err == nil {
return false
}
var pgErr *pgconn.PgError
if errors.As(err, &pgErr) {
return pgErr.Code == postgresDuplicateColumn
}
return strings.Contains(err.Error(), "duplicate column name")
}
// IsAlreadyExistsError reports whether err means a schema object already
// exists and the migration can be treated as an idempotent replay.
func IsAlreadyExistsError(err error) bool {
if err == nil {
return false
}
var pgErr *pgconn.PgError
if errors.As(err, &pgErr) {
switch pgErr.Code {
case postgresDuplicateColumn, postgresDuplicateTable, postgresDuplicateObject:
return true
default:
return false
}
}
s := err.Error()
return strings.Contains(s, "duplicate column name") ||
isSQLiteDuplicateObjectMessage(s)
}
func isSQLiteDuplicateObjectMessage(s string) bool {
return strings.HasPrefix(s, "table ") && strings.Contains(s, sqliteAlreadyExistsText) ||
strings.HasPrefix(s, "index ") && strings.Contains(s, sqliteAlreadyExistsText) ||
strings.HasPrefix(s, "trigger ") && strings.Contains(s, sqliteAlreadyExistsText) ||
strings.HasPrefix(s, "view ") && strings.Contains(s, sqliteAlreadyExistsText)
}
@@ -0,0 +1,145 @@
package db
import (
"errors"
"fmt"
"testing"
"github.com/jackc/pgx/v5/pgconn"
)
func TestIsDuplicateColumnError(t *testing.T) {
tests := []struct {
name string
err error
want bool
}{
{
name: "sqlite duplicate column",
err: errors.New("duplicate column name: branch_slug"),
want: true,
},
{
name: "postgres duplicate column",
err: &pgconn.PgError{
Code: postgresDuplicateColumn,
Message: `column "branch_slug" of relation "task_session_worktrees" already exists`,
},
want: true,
},
{
name: "wrapped postgres duplicate column",
err: fmt.Errorf("add column: %w", &pgconn.PgError{
Code: postgresDuplicateColumn,
Message: `column "branch_slug" of relation "task_session_worktrees" already exists`,
}),
want: true,
},
{
name: "postgres duplicate table is not a duplicate column",
err: &pgconn.PgError{
Code: postgresDuplicateTable,
Message: `relation "task_session_worktrees" already exists`,
},
want: false,
},
{
name: "unrelated",
err: errors.New("no such table: task_session_worktrees"),
want: false,
},
{
name: "nil",
err: nil,
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsDuplicateColumnError(tt.err); got != tt.want {
t.Fatalf("IsDuplicateColumnError() = %v, want %v", got, tt.want)
}
})
}
}
func TestIsAlreadyExistsError(t *testing.T) {
tests := []struct {
name string
err error
want bool
}{
{
name: "sqlite duplicate column",
err: errors.New("duplicate column name: branch_slug"),
want: true,
},
{
name: "sqlite already exists",
err: errors.New("index idx_task_session_worktrees_status already exists"),
want: true,
},
{
name: "sqlite table already exists",
err: errors.New("table task_session_worktrees already exists"),
want: true,
},
{
name: "sqlite unrelated already exists text",
err: errors.New("migration failed because a dependency already exists in an invalid state"),
want: false,
},
{
name: "postgres duplicate column",
err: &pgconn.PgError{Code: postgresDuplicateColumn},
want: true,
},
{
name: "wrapped postgres duplicate column",
err: fmt.Errorf("add column: %w", &pgconn.PgError{Code: postgresDuplicateColumn}),
want: true,
},
{
name: "postgres duplicate table",
err: &pgconn.PgError{Code: postgresDuplicateTable},
want: true,
},
{
name: "postgres duplicate object",
err: &pgconn.PgError{Code: postgresDuplicateObject},
want: true,
},
{
name: "postgres undefined column",
err: &pgconn.PgError{Code: "42703"},
want: false,
},
{
name: "postgres non-duplicate code ignores broad message text",
err: &pgconn.PgError{
Code: "42703",
Message: `relation "task_session_worktrees" already exists`,
},
want: false,
},
{
name: "unrelated",
err: errors.New("no such table: task_session_worktrees"),
want: false,
},
{
name: "nil",
err: nil,
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := IsAlreadyExistsError(tt.err); got != tt.want {
t.Fatalf("IsAlreadyExistsError() = %v, want %v", got, tt.want)
}
})
}
}
@@ -7,11 +7,14 @@ import (
"github.com/kandev/kandev/internal/testutil"
)
func TestPostgresFreshSchemaInitializes(t *testing.T) {
func TestPostgresSchemaReinitializes(t *testing.T) {
db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t))
if _, err := NewWithDB(db, db, nil); err != nil {
t.Fatalf("init fresh postgres schema: %v", err)
t.Fatalf("first postgres schema init: %v", err)
}
if _, err := NewWithDB(db, db, nil); err != nil {
t.Fatalf("second postgres schema init: %v", err)
}
}
@@ -0,0 +1,27 @@
package sqlite
import (
"path/filepath"
"testing"
"github.com/jmoiron/sqlx"
dbutil "github.com/kandev/kandev/internal/db"
)
func TestSQLiteSchemaReinitializes(t *testing.T) {
dbPath := filepath.Join(t.TempDir(), "schema-replay.db")
dbConn, err := dbutil.OpenSQLite(dbPath)
if err != nil {
t.Fatalf("open sqlite: %v", err)
}
sqlxDB := sqlx.NewDb(dbConn, "sqlite3")
t.Cleanup(func() { _ = sqlxDB.Close() })
if _, err := NewWithDB(sqlxDB, sqlxDB, nil); err != nil {
t.Fatalf("first schema init: %v", err)
}
if _, err := NewWithDB(sqlxDB, sqlxDB, nil); err != nil {
t.Fatalf("second schema init: %v", err)
}
}
+5 -15
View File
@@ -4,11 +4,12 @@ import (
"context"
"database/sql"
"fmt"
"strings"
"time"
"github.com/google/uuid"
"github.com/jmoiron/sqlx"
"github.com/kandev/kandev/internal/db"
)
// SQLiteStore implements Store interface using SQLite.
@@ -64,26 +65,15 @@ func (s *SQLiteStore) initSchema() error {
// Idempotent ALTER for upgrades. Pre-existing rows get branch_slug='' which
// matches the legacy single-branch identity exactly.
if _, err := s.db.Exec(`ALTER TABLE task_session_worktrees ADD COLUMN branch_slug TEXT NOT NULL DEFAULT ''`); err != nil {
// SQLite returns "duplicate column name" when the column already exists;
// treat that as success so the migration is replay-safe.
if !isDuplicateColumnError(err) {
// SQLite and Postgres report duplicate columns differently; treat both
// as success so the migration is replay-safe.
if !db.IsDuplicateColumnError(err) {
return err
}
}
return nil
}
func isDuplicateColumnError(err error) bool {
if err == nil {
return false
}
// SQLite emits "duplicate column name: <col>" for ALTER TABLE ADD COLUMN
// against an existing column. The broader "already exists" substring also
// matches unrelated DDL errors (e.g. "table X already exists"), which
// would silently swallow real schema failures — keep the match narrow.
return strings.Contains(err.Error(), "duplicate column name")
}
// CreateWorktree persists a new worktree record.
func (s *SQLiteStore) CreateWorktree(ctx context.Context, wt *Worktree) error {
if wt.ID == "" {
@@ -0,0 +1,25 @@
package worktree
import (
"testing"
tasksqlite "github.com/kandev/kandev/internal/task/repository/sqlite"
"github.com/kandev/kandev/internal/testutil"
)
func TestSQLiteStore_ReinitializesSchemaOnPostgres(t *testing.T) {
db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t))
if _, err := tasksqlite.NewWithDB(db, db, nil); err != nil {
t.Fatalf("first task schema init: %v", err)
}
if _, err := NewSQLiteStore(db, db); err != nil {
t.Fatalf("first worktree schema init: %v", err)
}
if _, err := tasksqlite.NewWithDB(db, db, nil); err != nil {
t.Fatalf("second task schema init: %v", err)
}
if _, err := NewSQLiteStore(db, db); err != nil {
t.Fatalf("second worktree schema init: %v", err)
}
}
@@ -2,12 +2,16 @@ package worktree
import (
"context"
"path/filepath"
"sort"
"testing"
"time"
"github.com/jmoiron/sqlx"
_ "github.com/mattn/go-sqlite3"
dbutil "github.com/kandev/kandev/internal/db"
tasksqlite "github.com/kandev/kandev/internal/task/repository/sqlite"
)
// newTestStore opens an in-memory SQLite DB and constructs a *SQLiteStore on it.
@@ -27,6 +31,28 @@ func newTestStore(t *testing.T) *SQLiteStore {
return store
}
func TestSQLiteStore_ReinitializesSchema(t *testing.T) {
dbConn, err := dbutil.OpenSQLite(filepath.Join(t.TempDir(), "worktree-replay.db"))
if err != nil {
t.Fatalf("open sqlite: %v", err)
}
db := sqlx.NewDb(dbConn, "sqlite3")
t.Cleanup(func() { _ = db.Close() })
if _, err := tasksqlite.NewWithDB(db, db, nil); err != nil {
t.Fatalf("first task schema init: %v", err)
}
if _, err := NewSQLiteStore(db, db); err != nil {
t.Fatalf("first worktree schema init: %v", err)
}
if _, err := tasksqlite.NewWithDB(db, db, nil); err != nil {
t.Fatalf("second task schema init: %v", err)
}
if _, err := NewSQLiteStore(db, db); err != nil {
t.Fatalf("second worktree schema init: %v", err)
}
}
func TestSQLiteStore_ListActiveWorktreePaths(t *testing.T) {
store := newTestStore(t)
ctx := context.Background()
@@ -0,0 +1,77 @@
# 0027: Replayable schema migrations across SQLite and Postgres
**Status:** accepted
**Date:** 2026-06-24
**Area:** backend
## Context
Kandev runs schema initialization during backend startup. That path must be safe
on a fresh database and safe when replayed against an existing database.
Issue #1353 exposed a gap in that contract for Postgres deployments: the
worktree store added `task_session_worktrees.branch_slug` with an idempotent
SQLite-only duplicate-column check. SQLite reports duplicate `ADD COLUMN`
attempts as an error string containing `duplicate column name`; Postgres reports
the same condition as SQLSTATE `42701` (`duplicate_column`). The first boot
succeeded, but a redeploy replayed startup schema initialization and aborted on
the duplicate Postgres column.
SQLite passing is not enough for startup schema code that also runs on Postgres.
## Decision
Schema replay error classification lives in `internal/db`, not in individual
repository or store packages.
- Use `db.IsDuplicateColumnError(err)` for `ALTER TABLE ... ADD COLUMN` replay
handling. It classifies SQLite duplicate-column strings and Postgres SQLSTATE
`42701`.
- Use `db.IsAlreadyExistsError(err)` for broader migration logging where an
existing table, index, column, or object means the statement has already been
applied. It classifies the SQLite strings used by the legacy migration path and
the relevant Postgres duplicate-object SQLSTATEs.
- Do not add new local `strings.Contains(err.Error(), ...)` migration classifiers
in schema-owning packages. If a new dialect-specific replay case is needed,
add it to `internal/db` with tests.
Every startup schema owner touched by a schema change must have replay coverage
in the same PR:
1. Initialize schema on a fresh SQLite database.
2. Initialize the same schema on the same SQLite database again.
3. If the schema owner supports Postgres, run the same fresh-plus-replay test
against Postgres using `internal/testutil.OpenIsolatedPostgres`.
4. When another repository owns prerequisite tables, the test must use the real
startup order instead of hand-creating a partial table.
## Consequences
**Easier:**
- Postgres and SQLite replay behavior is tested at the schema-owner boundary
instead of inferred from one driver.
- Future migration code has one helper package to extend when dialects differ.
- Startup redeploy failures from already-applied schema changes are caught by
focused package tests.
**Harder:**
- Schema tests need an extra same-connection replay step.
- Postgres replay coverage depends on `KANDEV_TEST_POSTGRES_DSN` being available
in the environment running the integration tests.
- Existing startup schema owners that are not touched by this issue may still
lack replay tests. Backfill them in focused follow-up work instead of
broadening unrelated bug fixes.
- The current imperative migration style remains; this ADR does not introduce a
numbered migration framework.
## Alternatives Considered
- **Keep local classifiers in each package.** Rejected because it caused this
bug: the worktree store knew about SQLite but not Postgres.
- **Only add a Postgres case to the worktree store.** Rejected as too narrow;
the same replay pattern exists in several startup schema paths.
- **Move immediately to a migration framework.** Deferred. A migration framework
could improve ordering and history, but it is a larger refactor than needed to
prevent this class of replay bug.
+1
View File
@@ -32,3 +32,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or
| 0024 | [Go-fronted Vite dev mode](0024-go-fronted-vite-dev-mode.md) | accepted | backend, frontend, cli | 2026-06-18 |
| 0025 | [Runtime cleanup uses `executors_running`](0025-runtime-cleanup-uses-executors-running.md) | accepted | backend | 2026-06-22 |
| 0026 | [Tauri desktop shell over native runtime](0026-tauri-desktop-shell.md) | accepted | frontend, backend, cli, infra | 2026-06-23 |
| 0027 | [Replayable schema migrations across SQLite and Postgres](0027-replayable-schema-migrations.md) | accepted | backend | 2026-06-24 |