diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 05c48a22ab..928e552fbd 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -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) diff --git a/apps/backend/internal/db/migratelog.go b/apps/backend/internal/db/migratelog.go index d293b1e64a..679425e7e3 100644 --- a/apps/backend/internal/db/migratelog.go +++ b/apps/backend/internal/db/migratelog.go @@ -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") -} diff --git a/apps/backend/internal/db/migration_errors.go b/apps/backend/internal/db/migration_errors.go new file mode 100644 index 0000000000..eb36f20972 --- /dev/null +++ b/apps/backend/internal/db/migration_errors.go @@ -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) +} diff --git a/apps/backend/internal/db/migration_errors_test.go b/apps/backend/internal/db/migration_errors_test.go new file mode 100644 index 0000000000..bb1e1f5552 --- /dev/null +++ b/apps/backend/internal/db/migration_errors_test.go @@ -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) + } + }) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/postgres_schema_test.go b/apps/backend/internal/task/repository/sqlite/postgres_schema_test.go index 7bb08a5d3d..38c67f8835 100644 --- a/apps/backend/internal/task/repository/sqlite/postgres_schema_test.go +++ b/apps/backend/internal/task/repository/sqlite/postgres_schema_test.go @@ -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) } } diff --git a/apps/backend/internal/task/repository/sqlite/schema_replay_test.go b/apps/backend/internal/task/repository/sqlite/schema_replay_test.go new file mode 100644 index 0000000000..adabb69d10 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/schema_replay_test.go @@ -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) + } +} diff --git a/apps/backend/internal/worktree/store.go b/apps/backend/internal/worktree/store.go index 756431867a..4f076216ce 100644 --- a/apps/backend/internal/worktree/store.go +++ b/apps/backend/internal/worktree/store.go @@ -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: " 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 == "" { diff --git a/apps/backend/internal/worktree/store_postgres_test.go b/apps/backend/internal/worktree/store_postgres_test.go new file mode 100644 index 0000000000..fea1139df7 --- /dev/null +++ b/apps/backend/internal/worktree/store_postgres_test.go @@ -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) + } +} diff --git a/apps/backend/internal/worktree/store_test.go b/apps/backend/internal/worktree/store_test.go index 75ffd0c004..7139f7319e 100644 --- a/apps/backend/internal/worktree/store_test.go +++ b/apps/backend/internal/worktree/store_test.go @@ -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() diff --git a/docs/decisions/0027-replayable-schema-migrations.md b/docs/decisions/0027-replayable-schema-migrations.md new file mode 100644 index 0000000000..bbe1cf2db3 --- /dev/null +++ b/docs/decisions/0027-replayable-schema-migrations.md @@ -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. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index b334f1667b..ac9f3978a7 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -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 |