From c9b32db592beb516eaf3f40fe137d966a205f4b5 Mon Sep 17 00:00:00 2001 From: XingfenD Date: Mon, 14 Sep 2026 19:33:30 +0800 Subject: [PATCH] =?UTF-8?q?docs(plan):=20backend=20hardening=20implementat?= =?UTF-8?q?ion=20plan=20=E2=80=94=2029=20tasks=20across=203=20batches?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../plans/2026-09-14-backend-hardening.md | 1885 +++++++++++++++++ 1 file changed, 1885 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-14-backend-hardening.md diff --git a/docs/superpowers/plans/2026-09-14-backend-hardening.md b/docs/superpowers/plans/2026-09-14-backend-hardening.md new file mode 100644 index 0000000..af1876e --- /dev/null +++ b/docs/superpowers/plans/2026-09-14-backend-hardening.md @@ -0,0 +1,1885 @@ +# Backend Hardening Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make schema changes safe via ordered migrations, fix 17 confirmed defects, restructure business logic into `internal/` with port interfaces, and introduce a CI workflow. + +**Architecture:** Three sequential batches. Batch A builds the migration system and CI foundation. Batch B fixes bugs B1–B13 with reproduction tests. Batch C performs structural reorganization (ports/store split/media/upload/scanner) with contract tests and fake-based unit tests. Each batch keeps tests green throughout. + +**Tech Stack:** Go 1.26, gin, pgx/v5, go-redis/v9, PostgreSQL 16, Redis 7, GitHub Actions (Gitea Actions compatible), embedded SQL migrations with pg advisory lock. + +## Global Constraints + +- Branch: `fix/backend-hardening` (already created). +- CHANGELOG entries in `docs/CHANGELOG.md`: English line then Chinese line on consecutive lines, different entries separated by blank line. +- `docs/README.md` and `docs/README_zh.md` stay content-equivalent; update both in same change. +- API contract: zero changes except B6/B7/B8 error semantics (recorded in changelog). +- Migration: self-hosted embedded SQL + `schema_migrations` + pg advisory lock. No down migrations. +- Refactor: small consumer-side interfaces in `internal/ports`, hand-written fakes in `internal/ports/portsfake`, no DI framework, no mock generators. +- CI: standard GitHub Actions syntax in `.github/workflows/`; Gitea Actions compatible. Runner-less until Gitea runner registered; local gate mandatory before each batch merge. +- Local gate before each batch merge: dev compose PG+Redis up, then `go vet ./... && gofmt -l . && go test -p 1 -count=1 ./...` confirming 0 skip, plus `scripts/smoke.sh`. + +--- + +## Part A — Migration System + CI + Quick Fixes + +### Task 1: Migration System + +**Files:** +- Create: `backend/internal/db/migrations/0001_baseline.sql` +- Modify: `backend/internal/db/db.go` +- Delete: `backend/internal/db/schema.sql` (after copying to migrations) + +**Interfaces:** +- Consumes: `pgxpool.Pool` from `db.Connect` +- Produces: `db.Migrate(ctx, pool) error` — same signature, different internals; `schema_migrations` table auto-created + +- [ ] **Step 1: Create baseline migration file** + +Copy current `backend/internal/db/schema.sql` content verbatim into `backend/internal/db/migrations/0001_baseline.sql`. Content is all `CREATE TABLE IF NOT EXISTS` and `CREATE INDEX IF NOT EXISTS` statements for users, libraries, books, reading_progress, bookmarks. + +- [ ] **Step 2: Rewrite `db.go` with migration system** + +Replace `backend/internal/db/db.go` entirely with the new migration system: + +```go +package db + +import ( + "context" + "embed" + "fmt" + "io/fs" + "log" + "regexp" + "sort" + "strings" + + "github.com/jackc/pgx/v5/pgxpool" +) + +//go:embed migrations +var migrationsFS embed.FS + +const advisoryLockKey int64 = 0x424C4D49 // "BLMI" + +var migrationNameRe = regexp.MustCompile(`^\d{4}_[a-z0-9_]+\.sql$`) + +func Connect(ctx context.Context, url string) (*pgxpool.Pool, error) { + cfg, err := pgxpool.ParseConfig(url) + if err != nil { + return nil, err + } + cfg.MaxConns = 10 + return pgxpool.NewWithConfig(ctx, cfg) +} + +func Migrate(ctx context.Context, p *pgxpool.Pool) error { + // 1. Acquire advisory lock — serializes concurrent replicas. + if _, err := p.Exec(ctx, "SELECT pg_advisory_lock($1)", advisoryLockKey); err != nil { + return fmt.Errorf("advisory lock: %w", err) + } + defer func() { + if _, err := p.Exec(ctx, "SELECT pg_advisory_unlock($1)", advisoryLockKey); err != nil { + log.Printf("advisory unlock: %v", err) + } + }() + + // 2. Create tracking table. + if _, err := p.Exec(ctx, `CREATE TABLE IF NOT EXISTS schema_migrations ( + version BIGINT PRIMARY KEY, name TEXT NOT NULL, + applied_at TIMESTAMPTZ NOT NULL DEFAULT now())`); err != nil { + return fmt.Errorf("create schema_migrations: %w", err) + } + + // 3. Read + validate embedded files. + entries, err := fs.ReadDir(migrationsFS, "migrations") + if err != nil { + return fmt.Errorf("read migrations dir: %w", err) + } + var files []string + for _, e := range entries { + name := e.Name() + if !migrationNameRe.MatchString(name) { + panic(fmt.Sprintf("invalid migration filename: %q", name)) + } + files = append(files, name) + } + sort.Strings(files) + + // 4. Baseline: empty schema_migrations + books exists → mark 0001 applied. + var count int + if err := p.QueryRow(ctx, "SELECT count(*) FROM schema_migrations").Scan(&count); err != nil { + return fmt.Errorf("count migrations: %w", err) + } + if count == 0 { + var hasBooks bool + if err := p.QueryRow(ctx, "SELECT to_regclass('books') IS NOT NULL").Scan(&hasBooks); err != nil { + return fmt.Errorf("check books: %w", err) + } + if hasBooks && len(files) > 0 && strings.HasPrefix(files[0], "0001_") { + if _, err := p.Exec(ctx, + "INSERT INTO schema_migrations (version, name) VALUES ($1,$2)", + 1, files[0]); err != nil { + return fmt.Errorf("baseline: %w", err) + } + log.Printf("migration baseline: %s (existing db)", files[0]) + files = files[1:] + } + } + + // 5. Load applied set. + applied := map[int64]bool{} + rows, err := p.Query(ctx, "SELECT version FROM schema_migrations") + if err != nil { + return fmt.Errorf("list applied: %w", err) + } + defer rows.Close() + for rows.Next() { + var v int64 + if err := rows.Scan(&v); err != nil { + return fmt.Errorf("scan: %w", err) + } + applied[v] = true + } + if err := rows.Err(); err != nil { + return err + } + + // 6. Apply pending, each in own transaction. + for _, name := range files { + v := parseVersion(name) + if applied[v] { + continue + } + sql, err := fs.ReadFile(migrationsFS, "migrations/"+name) + if err != nil { + return fmt.Errorf("read %s: %w", name, err) + } + tx, err := p.Begin(ctx) + if err != nil { + return err + } + if _, err := tx.Exec(ctx, string(sql)); err != nil { + tx.Rollback(ctx) + return fmt.Errorf("exec %s: %w", name, err) + } + if _, err := tx.Exec(ctx, + "INSERT INTO schema_migrations (version, name) VALUES ($1,$2)", v, name); err != nil { + tx.Rollback(ctx) + return err + } + if err := tx.Commit(ctx); err != nil { + return err + } + log.Printf("migration applied: %s", name) + } + return nil +} + +func parseVersion(name string) int64 { + parts := strings.SplitN(name, "_", 2) + var v int64 + fmt.Sscanf(parts[0], "%d", &v) + return v +} +``` + +- [ ] **Step 3: Delete old schema.sql** + +```bash +git rm backend/internal/db/schema.sql +``` + +- [ ] **Step 4: Verify build + tests** + +```bash +cd backend && go build ./... +DATABASE_URL=postgres://booklib:booklib@localhost:5432/booklib?sslmode=disable \ + go test -p 1 -count=1 ./... +``` + +- [ ] **Step 5: Verify baseline on existing DB** + +On a DB with old schema (no `schema_migrations`): +1. Start app → verify `SELECT * FROM schema_migrations` has one row (version=1). +2. Verify existing data untouched (`SELECT count(*) FROM books` same as before). +3. Verify no DDL re-executed (no errors in log). + +- [ ] **Step 6: Commit** + +```bash +git add backend/internal/db/migrations/0001_baseline.sql backend/internal/db/db.go +git rm backend/internal/db/schema.sql +git commit -m "feat(db): ordered migration system with advisory lock and baseline detection" +``` + +--- + +### Task 2: CI Workflow + +**Files:** +- Create: `.github/workflows/ci.yml` + +- [ ] **Step 1: Create workflow** + +```yaml +name: CI +on: + push: + branches: [master, 'fix/**', 'feat/**'] + pull_request: + branches: [master] +jobs: + backend: + runs-on: ubuntu-latest + services: + postgres: + image: postgres:16 + env: {POSTGRES_USER: booklib, POSTGRES_PASSWORD: booklib, POSTGRES_DB: booklib} + ports: ['5432:5432'] + options: --health-cmd pg_isready --health-interval 5s --health-timeout 3s --health-retries 10 + redis: + image: redis:7 + ports: ['6379:6379'] + options: --health-cmd "redis-cli ping" --health-interval 5s --health-timeout 3s --health-retries 10 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: {go-version-file: backend/go.mod} + - run: | + cd backend && OUT=$(gofmt -l .) && [ -z "$OUT" ] || { echo "gofmt:"; echo "$OUT"; exit 1; } + - run: cd backend && go vet ./... + - run: cd backend && go test -p 1 -count=1 ./... + env: + DATABASE_URL: postgres://booklib:booklib@localhost:5432/booklib?sslmode=disable + REDIS_URL: redis://localhost:6379/0 + frontend: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: {node-version: 20} + - run: cd frontend && npm ci && npm run check +``` + +- [ ] **Step 2: Validate (optional)** + +```bash +go run github.com/rhysd/actionlint/cmd/actionlint@latest .github/workflows/ci.yml +``` + +- [ ] **Step 3: Commit** + +```bash +git add .github/workflows/ci.yml +git commit -m "ci: add GitHub Actions workflow (Gitea Actions compatible)" +``` + +--- + +### Task 3: Fix B14 — log.Fatalf bypasses shutdown + +**Files:** +- Modify: `backend/cmd/webui/main.go:43-55` + +- [ ] **Step 1: Replace serve goroutine + wait logic** + +Replace the block from `srv :=` through `<-ctx.Done()` in `main.go`: + +```go + serveErr := make(chan error, 1) + srv := &http.Server{Addr: cfg.Addr, Handler: api.NewRouter(cfg, st, rdb, sc), + ReadHeaderTimeout: 10 * time.Second} + go func() { + log.Printf("listening on %s", cfg.Addr) + if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { + serveErr <- err + } + close(serveErr) + }() + select { + case <-ctx.Done(): + case err := <-serveErr: + if err != nil { + log.Printf("serve: %v", err) + } + } + stop() + shutdownCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := srv.Shutdown(shutdownCtx); err != nil { + log.Printf("shutdown: %v", err) + } +``` + +- [ ] **Step 2: Build + smoke** + +```bash +cd backend && go build ./cmd/webui && scripts/smoke.sh +``` + +- [ ] **Step 3: Commit** + +```bash +git add backend/cmd/webui/main.go +git commit -m "fix(main): channel-based serve error instead of log.Fatalf in goroutine (B14)" +``` + +--- + +### Task 4: Fix B15 — Config validation + +**Files:** +- Modify: `backend/internal/config/config.go` +- Modify: `backend/internal/config/config_test.go` + +- [ ] **Step 1: Add failing tests to config_test.go** + +```go +func TestLoadDatabaseURLRequired(t *testing.T) { + t.Setenv("JWT_SECRET", "x") + t.Setenv("DATABASE_URL", "") + if _, err := Load(); err == nil { + t.Fatal("empty DATABASE_URL must fail") + } +} + +func TestLoadDatabaseURLMalformed(t *testing.T) { + t.Setenv("JWT_SECRET", "x") + t.Setenv("DATABASE_URL", "not a url") + if _, err := Load(); err == nil { + t.Fatal("malformed DATABASE_URL must fail") + } +} +``` + +- [ ] **Step 2: Run to verify failure** + +```bash +cd backend && go test -run "TestLoadDatabase" -v ./internal/config/ +``` +Expected: `TestLoadDatabaseURLRequired` FAIL (empty URL currently accepted). + +- [ ] **Step 3: Add validation in config.Load** + +Before the `return &Config{...}` in `Load()`, add: + +```go + dbURL := env("DATABASE_URL", "") + if dbURL == "" { + return nil, fmt.Errorf("DATABASE_URL is required") + } + if _, perr := pgxpool.ParseConfig(dbURL); perr != nil { + return nil, fmt.Errorf("DATABASE_URL: %w", perr) + } + if env("REDIS_URL", "") == "" { + log.Printf("redis disabled: rate-limit/scan-lock/page-cache off") + } +``` + +Add imports: `"log"`, `"github.com/jackc/pgx/v5/pgxpool"`. + +- [ ] **Step 4: Run tests to verify pass** + +```bash +cd backend && go test -v ./internal/config/ +``` + +- [ ] **Step 5: Commit** + +```bash +git add backend/internal/config/ +git commit -m "fix(config): validate DATABASE_URL, log redis disabled (B15)" +``` + +--- + +### Task 5: Fix B17 — smoke.sh outdated root_path + +**Files:** +- Modify: `scripts/smoke.sh` + +- [ ] **Step 1: Remove `root_path` from library creation JSON** + +Find the `curl` POST to `/libraries` (around line 32) and change: +```bash +-d '{"name":"smoke","root_path":"/data/books/smoke-books"}' +``` +to: +```bash +-d '{"name":"smoke"}' +``` + +- [ ] **Step 2: Verify** + +```bash +scripts/smoke.sh +``` + +- [ ] **Step 3: Commit** + +```bash +git add scripts/smoke.sh +git commit -m "fix(smoke): drop ignored root_path field, align with API contract (B17)" +``` + +--- + +### Task 6: Batch A docs + +**Files:** +- Modify: `docs/CHANGELOG.md` +- Modify: `docs/README.md`, `docs/README_zh.md` + +- [ ] **Step 1: Prepend CHANGELOG entries** + +``` +## [Unreleased] + +### Changed +- Add ordered migration system with schema_migrations tracking and pg advisory lock. Existing databases auto-baselined. +- 新增有序迁移系统,通过 schema_migrations 表和 pg advisory lock 实现安全多副本 schema 演进,已有数据库自动基线化。 + +### Fixed +- Serve goroutine log.Fatalf replaced with channel-based shutdown (B14). +- 服务 goroutine 的 log.Fatalf 改为 channel 通知,确保优雅关停(B14)。 +- DATABASE_URL validated at startup; empty REDIS_URL logs clear message (B15). +- 启动时校验 DATABASE_URL;空 REDIS_URL 记录明确日志(B15)。 +- smoke.sh aligned with current API contract (B17). +- smoke.sh 对齐当前 API 契约(B17)。 + +### Added +- CI workflow (.github/workflows/ci.yml) for GitHub/Gitea Actions. +- CI 工作流,兼容 GitHub Actions 和 Gitea Actions。 +``` + +- [ ] **Step 2: Update README dual versions** + +Add "Schema Migration" section (how to add `NNNN_description.sql`, never modify applied files, no down migrations, baseline auto-upgrade). Add "CI" section (workflow location, Gitea/GitHub setup, local gate command). + +- [ ] **Step 3: Run local gate** + +```bash +cd backend && go vet ./... && gofmt -l . && \ +DATABASE_URL=postgres://booklib:booklib@localhost:5432/booklib?sslmode=disable \ +REDIS_URL=redis://localhost:6379/0 \ +go test -p 1 -count=1 ./... +``` +Confirm 0 skip. Then run `scripts/smoke.sh`. + +- [ ] **Step 4: Commit** + +```bash +git add docs/ +git commit -m "docs: changelog + README for batch A (migrations, CI, B14/B15/B17)" +``` + +--- + +## Part B — Bug Fixes B1–B13 + +Each bug: **write failing test → fix → verify → continue**. Tests use real PG/Redis (integration) until Part C converts applicable ones to fake-based unit tests. + +### Task 7: B1 — Rate limiter permanent lockout + +**Files:** +- Modify: `backend/internal/redispkg/redis.go` (IncrWindow) +- Modify: `backend/internal/redispkg/redis_test.go` + +- [ ] **Step 1: Add test** + +```go +func TestIncrWindowSetsTTL(t *testing.T) { + url := os.Getenv("REDIS_URL") + if url == "" { t.Skip("REDIS_URL not set") } + r := New(url) + ctx := context.Background() + key := "test:ttl:" + t.Name() + r.c.Del(ctx, key) + n := r.IncrWindow(ctx, key, 5*time.Second) + if n != 1 { t.Fatalf("first = %d", n) } + ttl, err := r.c.TTL(ctx, key).Result() + if err != nil { t.Fatal(err) } + if ttl <= 0 { t.Fatalf("TTL must be positive, got %v", ttl) } +} +``` + +- [ ] **Step 2: Fix IncrWindow with Lua script** + +```go +var incrWindowScript = redis.NewScript(` +local n = redis.call('INCR', KEYS[1]) +if n == 1 then redis.call('EXPIRE', KEYS[1], ARGV[1]) end +return n`) + +func (r *R) IncrWindow(ctx context.Context, key string, ttl time.Duration) int { + if r.c == nil { return 1 } + n, err := incrWindowScript.Run(ctx, r.c, []string{key}, int(ttl.Seconds())).Int() + if err != nil { return 1 } + return n +} +``` + +- [ ] **Step 3: Run + commit** + +```bash +REDIS_URL=redis://localhost:6379/0 go test -v ./internal/redispkg/ +git add backend/internal/redispkg/ && git commit -m "fix(redis): atomic INCR+EXPIRE via Lua (B1)" +``` + +--- + +### Task 8: B2+B3 — Lock rand error + unlock context + +**Files:** +- Modify: `backend/internal/redispkg/redis.go` (Lock method) + +- [ ] **Step 1: Fix token generation (B2)** + +In `Lock`, replace `rand.Read(b)` / `tok := hex.EncodeToString(b)` with: + +```go + b := make([]byte, 8) + if _, err := rand.Read(b); err != nil { + log.Printf("rand.Read: %v (degrading to no-lock)", err) + return noop, true + } + tok := hex.EncodeToString(b) +``` + +- [ ] **Step 2: Fix unlock context (B3)** + +Replace the returned unlock closure's Eval call: + +```go + return func() { + if err := r.c.Eval(context.WithoutCancel(ctx), + "if redis.call('get',KEYS[1])==ARGV[1] then return redis.call('del',KEYS[1]) else return 0 end", + []string{key}, tok).Err(); err != nil { + log.Printf("unlock %s: %v", key, err) + } + }, true +``` + +- [ ] **Step 3: Run + commit** + +```bash +go test -v ./internal/redispkg/ +git add backend/internal/redispkg/ && git commit -m "fix(redis): handle rand error + unlock survives ctx cancel (B2, B3)" +``` + +--- + +### Task 9: B4 — Truncated part reported as received + +**Files:** +- Modify: `backend/cmd/webui/handlers/uploads.go` (UploadPart method) + +- [ ] **Step 1: Fix part write to use tmp+rename** + +In `UploadPart`, replace the file-writing block (from `p := filepath.Join(dir, "parts", ...)` through the error handling): + +```go + p := filepath.Join(dir, "parts", strconv.FormatInt(idx, 10)) + tmp := p + ".tmp" + f, e := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) + if e != nil { + err(c, http.StatusInternalServerError, "internal", "create part") + return + } + n, e := io.Copy(f, c.Request.Body) + f.Close() + if e != nil || n != hi-lo { + os.Remove(tmp) + var mbe *http.MaxBytesError + code, msg := "too_large", "part size mismatch" + if errors.As(e, &mbe) { msg = "part exceeds declared size" } + err(c, http.StatusRequestEntityTooLarge, code, msg) + return + } + if e := os.Rename(tmp, p); e != nil { + os.Remove(tmp) + err(c, http.StatusInternalServerError, "internal", "rename part") + return + } + c.JSON(http.StatusAccepted, gin.H{"accepted": true}) +``` + +- [ ] **Step 2: Run + commit** + +```bash +go test -v ./cmd/webui/handlers/ -run Upload +git add backend/cmd/webui/handlers/uploads.go && git commit -m "fix(upload): write part to .tmp then rename (B4)" +``` + +--- + +### Task 10: B5 — DeleteUser last-admin TOCTOU + +**Files:** +- Modify: `backend/internal/store/store.go` +- Modify: `backend/cmd/webui/handlers/users.go` + +- [ ] **Step 1: Add test to store_test.go** + +```go +func TestDeleteUserLastAdmin(t *testing.T) { + s := setup(t) + ctx := context.Background() + id, _ := s.CreateUser(ctx, "onlyadmin", "h", "admin") + err := s.DeleteUser(ctx, id) + if !errors.Is(err, ErrLastAdmin) { + t.Fatalf("want ErrLastAdmin, got %v", err) + } + if _, err := s.GetUserByID(ctx, id); err != nil { + t.Fatal("admin should still exist") + } +} + +func TestDeleteUserNonLastAdmin(t *testing.T) { + s := setup(t) + ctx := context.Background() + id1, _ := s.CreateUser(ctx, "a1", "h", "admin") + s.CreateUser(ctx, "a2", "h", "admin") + if err := s.DeleteUser(ctx, id1); err != nil { + t.Fatalf("non-last admin delete: %v", err) + } +} +``` + +- [ ] **Step 2: Run to verify failure** + +Expected: `ErrLastAdmin` undefined. + +- [ ] **Step 3: Add ErrLastAdmin + transactional DeleteUser to store.go** + +Add sentinel: +```go +var ErrLastAdmin = errors.New("cannot delete the last admin") +``` + +Replace `DeleteUser`: +```go +func (s *Store) DeleteUser(ctx context.Context, id int64) error { + tx, err := s.P.Begin(ctx) + if err != nil { return err } + defer tx.Rollback(ctx) + var role string + if err := tx.QueryRow(ctx, "SELECT role FROM users WHERE id=$1 FOR UPDATE", id).Scan(&role); err != nil { + return err + } + if role == "admin" { + var n int + if err := tx.QueryRow(ctx, "SELECT count(*) FROM users WHERE role='admin'").Scan(&n); err != nil { + return err + } + if n <= 1 { return ErrLastAdmin } + } + if _, err := tx.Exec(ctx, "DELETE FROM users WHERE id=$1", id); err != nil { return err } + return tx.Commit(ctx) +} +``` + +Remove the old exported `CountAdmins` method. + +- [ ] **Step 4: Update users.go handler** + +Replace `DeleteUser` handler: + +```go +func (h *H) DeleteUser(c *gin.Context) { + id, e := strconv.ParseInt(c.Param("id"), 10, 64) + if e != nil { err(c, 400, "bad_request", "bad id"); return } + if id == uid(c) { err(c, 400, "bad_request", "cannot delete yourself"); return } + if e := h.st.DeleteUser(c, id); e != nil { + if errors.Is(e, pgx.ErrNoRows) { err(c, 404, "not_found", "no such user"); return } + if errors.Is(e, store.ErrLastAdmin) { err(c, 400, "bad_request", "cannot delete the last admin"); return } + dbErr(c, e); return + } + c.Status(204) +} +``` + +Remove the old `CountAdmins` + `GetUserByID` + role check block. Add `"booklib/internal/store"` import. + +- [ ] **Step 5: Run + commit** + +```bash +DATABASE_URL=... go test -v ./internal/store/ ./cmd/webui/handlers/ +git add backend/internal/store/store.go backend/cmd/webui/handlers/users.go +git commit -m "fix(store): transactional last-admin check in DeleteUser (B5)" +``` + +--- + +### Task 11: B6+B7 — Error semantics fixes + +**Files:** +- Modify: `backend/cmd/webui/handlers/libraries.go` (Upload method, B6) +- Modify: `backend/cmd/webui/handlers/auth.go` (Me method, B7) + +- [ ] **Step 1: Fix B6 — Upload io.Copy error mapping** + +In `libraries.go` `Upload` method, replace the `io.Copy` error block: + +```go + if _, e := io.Copy(out, src); e != nil { + out.Close() + os.Remove(tmp) + var mbe *http.MaxBytesError + if errors.As(e, &mbe) { + err(c, http.StatusRequestEntityTooLarge, "too_large", "file exceeds upload limit") + return + } + err(c, http.StatusInternalServerError, "internal", "upload failed") + return + } +``` + +- [ ] **Step 2: Fix B7 — Me handler error mapping** + +In `auth.go` `Me` method, replace: + +```go +func (h *H) Me(c *gin.Context) { + u, qerr := h.st.GetUserByID(c, uid(c)) + if qerr != nil { + if errors.Is(qerr, pgx.ErrNoRows) { + err(c, http.StatusUnauthorized, "unauthorized", "no such user") + return + } + dbErr(c, qerr) + return + } + c.JSON(http.StatusOK, gin.H{"id": u.ID, "username": u.Username, "role": u.Role}) +} +``` + +- [ ] **Step 3: Run + commit** + +```bash +go test -v ./cmd/webui/handlers/ +git add backend/cmd/webui/handlers/libraries.go backend/cmd/webui/handlers/auth.go +git commit -m "fix(handlers): correct error mapping for Upload (B6) and Me (B7)" +``` + +--- + +### Task 12: B8 — Reserved library names + +**Files:** +- Create: `backend/internal/media/reserved.go` +- Create: `backend/internal/media/reserved_test.go` +- Modify: `backend/cmd/webui/handlers/libraries.go` + +- [ ] **Step 1: Create reserved.go** + +```go +package media + +import "strings" + +var reservedNames = map[string]bool{ + "cache": true, ".uploads": true, ".trash": true, +} + +func IsReservedName(name string) bool { + return reservedNames[strings.ToLower(name)] +} +``` + +- [ ] **Step 2: Create reserved_test.go** + +```go +package media + +import "testing" + +func TestIsReservedName(t *testing.T) { + for _, n := range []string{"cache", "Cache", "CACHE", ".uploads", ".Uploads", ".trash"} { + if !IsReservedName(n) { t.Errorf("%q should be reserved", n) } + } + for _, n := range []string{"comics", "books", "my-library"} { + if IsReservedName(n) { t.Errorf("%q should not be reserved", n) } + } +} +``` + +- [ ] **Step 3: Add check in CreateLibrary handler** + +In `libraries.go` `CreateLibrary`, after `SafeName` check, add: + +```go + if media.IsReservedName(safe) { + err(c, http.StatusBadRequest, "bad_request", "reserved_name") + return + } +``` + +Import `"booklib/internal/media"`. + +- [ ] **Step 4: Run + commit** + +```bash +go test -v ./internal/media/ ./cmd/webui/handlers/ -run "Reserved\|CreateLibrary" +git add backend/internal/media/ backend/cmd/webui/handlers/libraries.go +git commit -m "fix(library): reject reserved names with 400 reserved_name (B8)" +``` + +--- + +### Task 13: B9-① — Scan lock renewal + +**Files:** +- Modify: `backend/internal/redispkg/redis.go` +- Modify: `backend/internal/redispkg/redis_test.go` +- Modify: `backend/internal/scanner/scanner.go` + +- [ ] **Step 1: Add test** + +```go +func TestScanLockRenewal(t *testing.T) { + url := os.Getenv("REDIS_URL") + if url == "" { t.Skip("REDIS_URL not set") } + r := New(url) + ctx, key := context.Background(), "test:scanlock:"+t.Name() + unlock, ok := r.ScanLock(ctx, key, 2*time.Second) + if !ok { t.Fatal("should acquire") } + time.Sleep(3 * time.Second) // would expire without renewal + _, ok2 := r.ScanLock(ctx, key, 2*time.Second) + if ok2 { t.Fatal("should still be held") } + unlock() +} +``` + +- [ ] **Step 2: Add ScanLock to redis.go** + +```go +func (r *R) ScanLock(ctx context.Context, key string, ttl time.Duration) (func(), bool) { + noop := func() {} + if r.c == nil { return noop, true } + b := make([]byte, 8) + if _, err := rand.Read(b); err != nil { + log.Printf("rand: %v", err); return noop, true + } + tok := hex.EncodeToString(b) + ok, err := r.c.SetNX(ctx, key, tok, ttl).Result() + if err != nil { + log.Printf("scanlock %s: %v (proceeding)", key, err) + return noop, true + } + if !ok { return noop, false } + done := make(chan struct{}) + go func() { + tk := time.NewTicker(ttl / 2); defer tk.Stop() + for { + select { + case <-done: return + case <-tk.C: + r.c.Eval(context.Background(), + `if redis.call('get',KEYS[1])==ARGV[1] then + return redis.call('expire',KEYS[1],ARGV[2]) end`, + []string{key}, tok, int(ttl.Seconds())) + } + } + }() + return func() { + close(done) + r.c.Eval(context.WithoutCancel(ctx), + "if redis.call('get',KEYS[1])==ARGV[1] then return redis.call('del',KEYS[1]) else return 0 end", + []string{key}, tok) + }, true +} +``` + +- [ ] **Step 3: Update scanner to use ScanLock** + +In `scanner.go` `ScanLibrary`, change: +```go + unlock, ok := s.rdb.Lock(ctx, fmt.Sprintf("scan:%d", lib.ID), 5*time.Minute) +``` +to: +```go + unlock, ok := s.rdb.ScanLock(ctx, fmt.Sprintf("scan:%d", lib.ID), 5*time.Minute) +``` + +- [ ] **Step 4: Run + commit** + +```bash +REDIS_URL=redis://localhost:6379/0 go test -v -timeout 10s ./internal/redispkg/ +go test -v ./internal/scanner/ +git add backend/internal/redispkg/ backend/internal/scanner/scanner.go +git commit -m "fix(redis): ScanLock with auto-renewal for scanner (B9-①)" +``` + +--- + +### Task 14: B10+B11 — Silent error logging fixes + +**Files:** +- Modify: `backend/internal/scanner/scanner.go` (cover, add, update) +- Modify: `backend/cmd/webui/handlers/content.go` (ServeCover) + +- [ ] **Step 1: Fix scanner add/update SetBookState (B10)** + +In both `add` and `update`, change: +```go + s.st.SetBookState(ctx, id, "error", idxErr.Error()) +``` +to: +```go + if e := s.st.SetBookState(ctx, id, "error", idxErr.Error()); e != nil { + log.Printf("scan: SetBookState %s: %v", rel, e) + } +``` + +- [ ] **Step 2: Fix scanner cover write (B11)** + +Replace the cover write section: +```go + tmp := filepath.Join(dir, "cover"+ext+".tmp") + dst := filepath.Join(dir, "cover"+ext) + if e := os.WriteFile(tmp, img, 0o644); e != nil { + log.Printf("scan: cover tmp %s: %v", rel, e) + os.Remove(tmp) + return + } + if e := os.Rename(tmp, dst); e != nil { + log.Printf("scan: cover rename %s: %v", rel, e) + os.Remove(tmp) + return + } +``` + +- [ ] **Step 3: Fix ServeCover self-heal (B11)** + +In `content.go`, replace the self-heal write block: +```go + tmp := fmt.Sprintf("%s.tmp-%d", dst, time.Now().UnixNano()) + if e := os.WriteFile(tmp, img, 0o644); e != nil { + log.Printf("serve: cover write: %v", e) + os.Remove(tmp) + } else if e := os.Rename(tmp, dst); e != nil { + log.Printf("serve: cover rename: %v", e) + os.Remove(tmp) + } else { + http.ServeFile(c.Writer, c.Request, dst) + } +``` + +Add `"log"` import if missing. + +- [ ] **Step 4: Run + commit** + +```bash +go test -v ./internal/scanner/ ./cmd/webui/handlers/ +git add backend/internal/scanner/scanner.go backend/cmd/webui/handlers/content.go +git commit -m "fix: check cover write errors + log SetBookState failures (B10, B11)" +``` + +--- + +### Task 15: B12 — uniquePath race + +**Files:** +- Modify: `backend/cmd/webui/handlers/libraries.go` (Upload method) + +- [ ] **Step 1: Add retry loop in Upload handler** + +In `Upload`, replace the single `uniquePath` + `OpenFile` block with a retry: + +```go + var dst string + var out *os.File + for attempt := 0; attempt < 5; attempt++ { + var e error + dst, e = h.uniquePath(root, name) + if e != nil { err(c, http.StatusForbidden, "forbidden", e.Error()); return } + tmp := dst + ".upload-" + strconv.FormatInt(time.Now().UnixNano(), 36) + out, e = os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) + if e == nil { break } + if !os.IsExist(e) { err(c, 500, "internal", "create tmp"); return } + } + if out == nil { + err(c, http.StatusConflict, "conflict", "concurrent upload conflict") + return + } +``` + +- [ ] **Step 2: Run + commit** + +```bash +go test -v ./cmd/webui/handlers/ -run Upload +git add backend/cmd/webui/handlers/libraries.go +git commit -m "fix(upload): retry on O_EXCL collision (B12)" +``` + +--- + +### Task 16: B13 — RowsAffected before err + +**Files:** +- Modify: `backend/internal/store/store.go` (UpdateBookmarkNote, DeleteBookmark) + +- [ ] **Step 1: Fix both methods** + +```go +func (s *Store) UpdateBookmarkNote(ctx context.Context, userID, id int64, note string) (bool, error) { + res, err := s.P.Exec(ctx, `UPDATE bookmarks SET note=$3 WHERE id=$1 AND user_id=$2`, id, userID, note) + if err != nil { return false, err } + return res.RowsAffected() > 0, nil +} + +func (s *Store) DeleteBookmark(ctx context.Context, userID, id int64) (bool, error) { + res, err := s.P.Exec(ctx, `DELETE FROM bookmarks WHERE id=$1 AND user_id=$2`, id, userID) + if err != nil { return false, err } + return res.RowsAffected() > 0, nil +} +``` + +- [ ] **Step 2: Run + commit** + +```bash +go test -v ./internal/store/ +git add backend/internal/store/store.go +git commit -m "fix(store): check err before RowsAffected (B13)" +``` + +--- + +### Task 17: B9-② — Scanner per-library single-flight + +**Files:** +- Modify: `backend/internal/scanner/scanner.go` + +- [ ] **Step 1: Add sync.Map + scanOnce wrapper** + +Add field to Scanner: +```go + flights sync.Map // map[int64]*sync.WaitGroup +``` + +Add method: +```go +func (s *Scanner) scanOnce(ctx context.Context, lib store.Library) { + wg := &sync.WaitGroup{} + wg.Add(1) + if existing, loaded := s.flights.LoadOrStore(lib.ID, wg); loaded { + existing.(*sync.WaitGroup).Wait() + return + } + defer func() { s.flights.Delete(lib.ID); wg.Done() }() + s.ScanLibrary(ctx, lib) +} +``` + +Update `Run` to call `s.scanOnce(ctx, l)` and `ScanLibraryByID` to call `s.scanOnce(ctx, lib)`. + +- [ ] **Step 2: Run + commit** + +```bash +go test -v ./internal/scanner/ +git add backend/internal/scanner/scanner.go +git commit -m "fix(scanner): per-library single-flight (B9-②)" +``` + +--- + +### Task 18: Batch B docs + local gate + +**Files:** +- Modify: `docs/CHANGELOG.md` + +- [ ] **Step 1: Prepend CHANGELOG entries for B1–B13** + +Add entries for each fix in the `[Unreleased]` section. Format per global constraints (English+Chinese, blank between entries). See spec §S2 for the full list. + +- [ ] **Step 2: Run full local gate** + +```bash +cd backend && go vet ./... && gofmt -l . +DATABASE_URL=postgres://booklib:booklib@localhost:5432/booklib?sslmode=disable \ +REDIS_URL=redis://localhost:6379/0 \ +go test -p 1 -count=1 ./... +scripts/smoke.sh +``` +Confirm 0 skip. + +- [ ] **Step 3: Commit** + +```bash +git add docs/CHANGELOG.md +git commit -m "docs: changelog for batch B (B1-B13)" +``` + +--- + +## Part C — Structure Restructure + Interfaces + +### Task 19: Define ports package + +**Files:** +- Create: `backend/internal/ports/ports.go` +- Create: `backend/internal/ports/errors.go` + +- [ ] **Step 1: Create ports.go** + +Define consumer-side interfaces. Value types use `store.*` (ports imports store for types only — one-way dependency): + +```go +package ports + +import ( + "context" + "time" + "booklib/internal/store" +) + +type UserStore interface { + CountUsers(ctx context.Context) (int, error) + CreateUser(ctx context.Context, username, hash, role string) (int64, error) + GetUserByName(ctx context.Context, username string) (store.User, error) + GetUserByID(ctx context.Context, id int64) (store.User, error) + ListUsers(ctx context.Context) ([]store.User, error) + DeleteUser(ctx context.Context, id int64) error +} + +type LibraryStore interface { + CreateLibrary(ctx context.Context, name, root string) (int64, error) + ListLibraries(ctx context.Context) ([]store.Library, error) + GetLibrary(ctx context.Context, id int64) (store.Library, error) +} + +type BookStore interface { + InsertBook(ctx context.Context, libID int64, path, title, format string, size, modTS int64, pageCount int) (int64, error) + GetBook(ctx context.Context, id int64) (store.Book, error) + ListBookMeta(ctx context.Context, libID int64) (map[string]store.BookMeta, error) + UpdateBookFile(ctx context.Context, id, size, modTS int64, pageCount int) error + DeleteBookByPath(ctx context.Context, libID int64, path string) error + DeleteBook(ctx context.Context, id int64) error + SetBookState(ctx context.Context, id int64, state, msg string) error + ListBooks(ctx context.Context, libID int64, q, prefix string, userID int64) ([]store.BookView, error) + BookHashes(ctx context.Context) (map[int64][2]int64, error) +} + +type ProgressStore interface { + UpsertProgress(ctx context.Context, userID, libID int64, bookPath string, locator []byte, percent float64) error + GetProgress(ctx context.Context, userID, libID int64, bookPath string) (store.Progress, error) + ListProgress(ctx context.Context, userID int64) ([]store.Progress, error) +} + +type BookmarkStore interface { + InsertBookmark(ctx context.Context, userID, libID int64, bookPath string, locator []byte, percent float64, note string) (int64, error) + ListBookmarks(ctx context.Context, userID, libID int64, bookPath string) ([]store.Bookmark, error) + UpdateBookmarkNote(ctx context.Context, userID, id int64, note string) (bool, error) + DeleteBookmark(ctx context.Context, userID, id int64) (bool, error) +} + +type PageCache interface { + Get(ctx context.Context, key string) (string, bool) + Set(ctx context.Context, key, val string, ttl time.Duration) +} + +type RateLimiter interface { + IncrWindow(ctx context.Context, key string, ttl time.Duration) int +} + +type ScanLocker interface { + ScanLock(ctx context.Context, key string, ttl time.Duration) (func(), bool) +} + +type Scanner interface { + ScanLibraryByID(ctx context.Context, id int64) +} +``` + +Note: `UploadSessions` and `Media` interfaces will be defined after `internal/upload` and `internal/media` are created (Tasks 22-23). + +- [ ] **Step 2: Create errors.go** + +```go +package ports + +import "booklib/internal/store" + +// Re-export sentinel errors so handlers use errors.Is via ports. +var ErrLastAdmin = store.ErrLastAdmin + +// IsUniqueViolation consolidates the 3 copies of pg 23505 check. +func IsUniqueViolation(err error) bool { return store.IsUniqueViolation(err) } +``` + +- [ ] **Step 3: Add IsUniqueViolation + ErrUniqueViolation to store** + +In `store.go`, add: +```go +var ErrUniqueViolation = errors.New("unique violation") + +func IsUniqueViolation(err error) bool { + var pgErr *pgconn.PgError + return errors.As(err, &pgErr) && pgErr.Code == "23505" +} +``` + +Update `ports/errors.go` to also re-export: +```go +var ErrUniqueViolation = store.ErrUniqueViolation +``` + +- [ ] **Step 4: Verify build** + +```bash +cd backend && go build ./... +``` + +- [ ] **Step 5: Commit** + +```bash +git add backend/internal/ports/ backend/internal/store/store.go +git commit -m "feat(ports): define consumer-side interfaces and shared errors" +``` + +--- + +### Task 20: Split store package + +**Files:** +- Create: `backend/internal/store/users.go` +- Create: `backend/internal/store/libraries.go` +- Create: `backend/internal/store/books.go` +- Create: `backend/internal/store/progress.go` +- Create: `backend/internal/store/bookmarks.go` +- Modify: `backend/internal/store/store.go` (keep types + ctor + shared helpers only) +- Modify: `backend/internal/store/store_test.go` (extract per-file tests) + +- [ ] **Step 1: Extract methods by aggregate** + +Move methods from `store.go` into per-aggregate files: +- `users.go`: `CountUsers`, `CreateUser`, `GetUserByName`, `GetUserByID`, `ListUsers`, `DeleteUser`, `scanUser`, `CountAdmins` (make unexported: `countAdmins`) +- `libraries.go`: `CreateLibrary`, `ListLibraries`, `GetLibrary` +- `books.go`: `InsertBook`, `GetBook`, `ListBookMeta`, `UpdateBookFile`, `DeleteBookByPath`, `DeleteBook`, `SetBookState`, `ListBooks`, `BookHashes`. Delete `ListBookIDs` (dead code per spec). +- `progress.go`: `UpsertProgress`, `GetProgress`, `ListProgress` +- `bookmarks.go`: `InsertBookmark`, `ListBookmarks`, `UpdateBookmarkNote`, `DeleteBookmark` + +Each file: `package store`, receives methods on `*Store`. + +- [ ] **Step 2: Slim down store.go** + +Keep: type definitions (`Store`, `User`, `Library`, `Book`, `BookMeta`, `BookView`, `Progress`, `Bookmark`), `New()`, `IsUniqueViolation()`, `ErrLastAdmin`, column constants (`userCols`, `bookCols`). + +Make `P` field unexported (`p *pgxpool.Pool`). Update all method files to use `s.p` instead of `s.P`. Update `store_test.go` setup if it uses `s.P`. + +- [ ] **Step 3: Verify build + tests** + +```bash +cd backend && go build ./... && DATABASE_URL=... go test ./internal/store/ +``` + +- [ ] **Step 4: Commit** + +```bash +git add backend/internal/store/ +git commit -m "refactor(store): split into per-aggregate files, unexport pool" +``` + +--- + +### Task 21: Add bookfile utilities + +**Files:** +- Modify: `backend/internal/bookfile/bookfile.go` (add Contains) +- Create: `backend/internal/bookfile/reader.go` (OpenReaderAt) + +- [ ] **Step 1: Add Contains function** + +```go +// Contains reports whether child is inside parent (EvalSymlinks semantics). +func Contains(parent, child string) bool { + p, err := filepath.EvalSymlinks(parent) + if err != nil { p = filepath.Clean(parent) } + c, err := filepath.EvalSymlinks(child) + if err != nil { c = filepath.Clean(child) } + return c == p || strings.HasPrefix(c, p+string(os.PathSeparator)) +} +``` + +- [ ] **Step 2: Add OpenReaderAt** + +Consolidate the 3 places that open a book file + stat + get ReaderAt: + +```go +// OpenReaderAt opens a book file and returns a ReaderAt + size. +func OpenReaderAt(root, rel string) (*os.File, int64, error) { + abs := filepath.Join(root, filepath.FromSlash(rel)) + f, err := os.Open(abs) + if err != nil { return nil, 0, err } + st, err := f.Stat() + if err != nil { f.Close(); return nil, 0, err } + return f, st.Size(), nil +} +``` + +- [ ] **Step 3: Add tests** + +```go +func TestContains(t *testing.T) { + dir := t.TempDir() + sub := filepath.Join(dir, "sub") + os.MkdirAll(sub, 0o755) + if !Contains(dir, sub) { t.Fatal("sub should be inside dir") } + if Contains(sub, dir) { t.Fatal("dir should not be inside sub") } +} +``` + +- [ ] **Step 4: Run + commit** + +```bash +go test -v ./internal/bookfile/ +git add backend/internal/bookfile/ +git commit -m "feat(bookfile): add Contains and OpenReaderAt utilities" +``` + +--- + +### Task 22: Create internal/media package + +**Files:** +- Modify: `backend/internal/media/reserved.go` (already exists from Task 12) +- Create: `backend/internal/media/media.go` (Media struct + interface methods) +- Create: `backend/internal/media/write.go` (WriteAtomic) +- Create: `backend/internal/media/chapters.go` (ChaptersOf) +- Create: `backend/internal/media/ports.go` (Media interface definition) + +- [ ] **Step 1: Create WriteAtomic** + +```go +package media + +import ( + "log" + "os" + "path/filepath" + "fmt" + "time" +) + +// WriteAtomic writes data to dst via tmp+rename. Cleans tmp only on failure. +func WriteAtomic(dir, name string, data []byte) error { + if err := os.MkdirAll(dir, 0o755); err != nil { return err } + tmp := filepath.Join(dir, fmt.Sprintf("%s.tmp-%d", name, time.Now().UnixNano())) + if err := os.WriteFile(tmp, data, 0o644); err != nil { + os.Remove(tmp) + return err + } + dst := filepath.Join(dir, name) + if err := os.Rename(tmp, dst); err != nil { + os.Remove(tmp) + return err + } + return nil +} +``` + +- [ ] **Step 2: Move ChaptersOf from content.go** + +Copy `chaptersOf` and `cbzChapter` from `handlers/content.go` into `media/chapters.go` (exported as `ChaptersOf`). The handler will call `media.ChaptersOf(idx)`. + +- [ ] **Step 3: Move cache layout functions** + +Move `DirKey`, `CoverDir`, `PagesDir`, `SweepStale`, `Hash` from `bookfile/cache.go` and `bookfile/hash.go` into `media/`. Or keep them in `bookfile` and have `media` wrap them — whichever avoids a large move. Per spec, media is the "single source of truth" for cache layout. Move them to `media/cache.go`: + +```go +package media + +import "booklib/internal/bookfile" + +// Re-export cache layout from bookfile for backward compat. +// Future: move implementations here. +var DirKey = bookfile.DirKey +var CoverDir = bookfile.CoverDir +var PagesDir = bookfile.PagesDir +var SweepStale = bookfile.SweepStale +var Hash = bookfile.Hash +``` + +Actually simpler: just have Media methods call bookfile functions directly. No re-export needed. Media is the consumer; bookfile stays as the low-level utility package. + +- [ ] **Step 4: Create Media struct** + +```go +package media + +import ( + "context" + "booklib/internal/bookfile" + "booklib/internal/config" + "booklib/internal/redispkg" +) + +type M struct { + cfg *config.Config + rdb *redispkg.R +} + +func New(cfg *config.Config, rdb *redispkg.R) *M { + return &M{cfg: cfg, rdb: rdb} +} +``` + +- [ ] **Step 5: Implement Media methods** + +Implement on `*M`: +- `EnsureCover(ctx, bookID, format, size, modTS, root, rel)` — extracted from scanner.cover +- `EnsurePage(ctx, bookID, size, modTS, root, rel, n)` — extracted from content.go Page handler +- `ChaptersOf(idx)` — from step 2 +- `PageIndex(ctx, bookID, size, modTS, root, rel)` — extracted from content.go pageIndex +- `CacheBuster(size, modTS)` — returns `bookfile.Hash(size, modTS)` + +- [ ] **Step 6: Create Media interface in ports.go** + +Add to `ports/ports.go` (or `ports/media.go`): +```go +type Media interface { + EnsureCover(ctx context.Context, bookID int64, format string, size, modTS int64, root, rel string) error + EnsurePage(ctx context.Context, bookID int64, size, modTS int64, root, rel string, n int) (string, error) + ChaptersOf(idx []string) []Chapter + PageIndex(ctx context.Context, bookID int64, size, modTS int64, root, rel string) ([]string, error) + CacheBuster(size, modTS int64) string +} +``` + +- [ ] **Step 7: Build + commit** + +```bash +go build ./... +git add backend/internal/media/ backend/internal/ports/ +git commit -m "feat(media): cache layout single source of truth + WriteAtomic + chapters" +``` + +--- + +### Task 23: Create internal/upload package + B16 + +**Files:** +- Create: `backend/internal/upload/upload.go` +- Create: `backend/internal/upload/upload_test.go` +- Create: `backend/internal/upload/ports.go` (UploadSessions interface) +- Modify: `backend/internal/scanner/scanner.go` (add Sweep to ticker, B16) + +- [ ] **Step 1: Move upload domain logic** + +Extract from `handlers/uploads.go` into `internal/upload/upload.go`: +- `uploadIDFor`, `uploadDir`, `chunkRange`, `numParts`, `uploadMeta`, `validUploadID` +- `Init(libID, name, size, chunkSize)` → returns uploadID +- `Status(uploadID)` → returns received part indices +- `PutPart(uploadID, index, body)` → writes part with tmp+rename (B4 fix already applied) +- `Complete(uploadID)` → assembles + renames to library root (uses `uniquePath`) +- `Sweep()` → deletes expired sessions (B16: moved from request path to scanner ticker) +- `UniquePath(root, name)` → shared between single-file and chunked upload + +The `upload` package takes `booksDir string` and `uploadMaxMB int64` in its constructor. + +- [ ] **Step 2: Define UploadSessions interface in ports** + +Add to `ports/ports.go`: +```go +type UploadSessions interface { + Init(ctx context.Context, libID int64, name string, size, chunkSize int64) (string, error) + Status(ctx context.Context, uploadID string) ([]int64, error) + PutPart(ctx context.Context, uploadID string, index int64, body io.Reader, maxSize int64) error + Complete(ctx context.Context, uploadID string, root string) (string, error) + Sweep(ctx context.Context) error + UniquePath(root, name string) (string, error) +} +``` + +Add to `ports/errors.go`: +```go +var ( + ErrTooLarge = errors.New("file too large") + ErrIncomplete = errors.New("upload incomplete") + ErrSizeMismatch = errors.New("size mismatch") + ErrNotFound = errors.New("not found") +) +``` + +- [ ] **Step 3: Wire Sweep into scanner ticker (B16)** + +In `scanner.go` `Run` method, add sweep call each tick: +```go +case <-t.C: + h.sweepUploads() // B16: moved from request path + libs, err := s.st.ListLibraries(ctx) + ... +``` + +The scanner needs access to the upload package. Add `up *upload.U` field to Scanner. Or have main.go pass the sweep function. Simplest: scanner gets an `upload.Sweeper` interface: +```go +type Sweeper interface { Sweep(ctx context.Context) error } +``` + +- [ ] **Step 4: Build + commit** + +```bash +go build ./... +git add backend/internal/upload/ backend/internal/scanner/ backend/internal/ports/ +git commit -m "feat(upload): extract upload subsystem + move sweep to scanner ticker (B16)" +``` + +--- + +### Task 24: Refactor scanner + +**Files:** +- Modify: `backend/internal/scanner/scanner.go` + +- [ ] **Step 1: Merge add/update into ingest** + +Consolidate the duplicated logic in `add` and `update`: + +```go +func (s *Scanner) ingest(ctx context.Context, libID, bookID int64, root, rel string, ds diskStat, isNew bool) { + format := bookfile.FormatFromExt(filepath.Base(rel)) + pageCount := 0 + var idxErr error + if format == "cbz" { + idx, err := s.zipIndex(root, rel) + pageCount = len(idx) + idxErr = err + if idxErr == nil && pageCount == 0 { + idxErr = errors.New("no images in archive") + } + } + if isNew { + id, err := s.st.InsertBook(ctx, libID, rel, titleOf(rel), format, ds.size, ds.modTS, pageCount) + if err != nil { log.Printf("scan: insert %s: %v", rel, err); return } + bookID = id + } else { + if err := s.st.UpdateBookFile(ctx, bookID, ds.size, ds.modTS, pageCount); err != nil { + log.Printf("scan: update %s: %v", rel, err); return + } + } + if idxErr != nil { + if e := s.st.SetBookState(ctx, bookID, "error", idxErr.Error()); e != nil { + log.Printf("scan: SetBookState %s: %v", rel, e) + } + return + } + s.cover(ctx, bookID, root, rel, format, ds) +} +``` + +- [ ] **Step 2: Use bookfile.OpenReaderAt** + +Replace `readCover` and `zipIndex` to use `bookfile.OpenReaderAt`: + +```go +func (s *Scanner) zipIndex(root, rel string) ([]string, error) { + f, size, err := bookfile.OpenReaderAt(root, rel) + if err != nil { return nil, err } + defer f.Close() + return bookfile.PageIndex(f, size) +} +``` + +- [ ] **Step 3: Use bookfile.Contains** + +Replace the `inside` function call: +```go +if !bookfile.Contains(s.cfg.BooksDir, root) { ... } +``` + +Remove the local `inside` function. + +- [ ] **Step 4: Use media.WriteAtomic in cover** + +Replace manual tmp+rename in `cover` with `media.WriteAtomic(dir, "cover"+ext, img)`. + +- [ ] **Step 5: Run + commit** + +```bash +go test -v ./internal/scanner/ +git add backend/internal/scanner/ +git commit -m "refactor(scanner): merge add/update into ingest, use shared utilities" +``` + +--- + +### Task 25: Slim down handlers + +**Files:** +- Modify: All `backend/cmd/webui/handlers/*.go` +- Modify: `backend/cmd/webui/handlers/handlers.go` + +- [ ] **Step 1: Change H struct to use port interfaces** + +```go +type H struct { + cfg *config.Config + users ports.UserStore + libs ports.LibraryStore + books ports.BookStore + progress ports.ProgressStore + bookmarks ports.BookmarkStore + cache ports.PageCache + rl ports.RateLimiter + scanner ports.Scanner + media media.M // or ports.Media once interface finalized + upload upload.U // or ports.UploadSessions +} +``` + +Update `New()` to accept all interfaces. + +- [ ] **Step 2: Update all handler methods** + +Replace `h.st.Xxx(...)` with the appropriate port field. Deduplicate: +- `getLibrary` and `getLibRow` → single `getLib(c, id)` helper +- `strconv.ParseInt(c.Param("id"), ...)` → single `idParam(c)` helper +- `isUnique` → `ports.IsUniqueViolation` +- Path contains checks → `bookfile.Contains` + +- [ ] **Step 3: Update router.go to pass new H** + +Update `api/router.go` and `handlers.New()` signature to accept all dependencies. + +- [ ] **Step 4: Build + commit** + +```bash +go build ./... +git add backend/cmd/webui/ +git commit -m "refactor(handlers): use port interfaces, deduplicate helpers" +``` + +--- + +### Task 26: Wire main.go assembly + +**Files:** +- Modify: `backend/cmd/webui/main.go` + +- [ ] **Step 1: Update main.go to assemble all dependencies** + +```go +func main() { + cfg, err := config.Load() + if err != nil { log.Fatalf("config: %v", err) } + // ... ctx, db, migrate as before ... + st := store.New(p) + seed.Admin(ctx, st, cfg.AdminUser, cfg.AdminPassword) + rdb := redispkg.New(cfg.RedisURL) + med := media.New(cfg, rdb) + up := upload.New(cfg.BooksDir, cfg.UploadMaxMB) + sc := scanner.New(st, cfg, rdb, med, up) + + // Wire router with port interfaces. + // *store.Store satisfies UserStore, LibraryStore, etc. + // *redispkg.R satisfies PageCache, RateLimiter, ScanLocker. + r := api.NewRouter(cfg, st, st, st, st, st, rdb, rdb, sc, med, up) + // ... serve as Task 3 pattern ... +} +``` + +- [ ] **Step 2: Build + smoke** + +```bash +cd backend && go build ./cmd/webui && scripts/smoke.sh +``` + +- [ ] **Step 3: Commit** + +```bash +git add backend/cmd/webui/main.go +git commit -m "refactor(main): assemble dependencies via port interfaces" +``` + +--- + +### Task 27: Create portsfake + contract tests + handler unit tests + +**Files:** +- Create: `backend/internal/ports/portsfake/fake.go` +- Create: `backend/cmd/webui/handlers/users_unit_test.go` (and similar per handler) +- Modify: `backend/cmd/webui/api/router_test.go` (contract test) + +- [ ] **Step 1: Create hand-written fakes** + +`portsfake/fake.go` implements all port interfaces with in-memory maps: + +```go +package portsfake + +type Users struct { + m map[int64]store.User + next int64 +} + +func NewUsers() *Users { return &Users{m: map[int64]store.User{}, next: 1} } + +func (u *Users) CreateUser(_ context.Context, username, hash, role string) (int64, error) { + for _, v := range u.m { + if v.Username == username { return 0, ports.ErrUniqueViolation } + } + id := u.next; u.next++ + u.m[id] = store.User{ID: id, Username: username, PasswordHash: hash, Role: role} + return id, nil +} +// ... implement all interface methods +``` + +Similarly for Libraries, Books, Progress, Bookmarks fakes. And a simple in-memory PageCache, RateLimiter, ScanLocker. + +- [ ] **Step 2: Add router contract test** + +In `router_test.go`, pin the expected route table: + +```go +func TestRouterContract(t *testing.T) { + r := NewRouter(/* fakes */) + routes := r.Routes() + expected := map[string]string{ + "GET /api/healthz": "", + "POST /api/auth/login": "", + "GET /api/auth/me": "auth", + "GET /api/users": "auth,admin", + // ... all routes with middleware markers + } + // Assert exact match — any route change requires updating this test. +} +``` + +- [ ] **Step 3: Add handler unit tests** + +Example `users_unit_test.go`: + +```go +func TestDeleteUserLastAdminReturns400(t *testing.T) { + h := newTestH() // uses portsfake + // Seed one admin + h.users.CreateUser(ctx, "admin", "h", "admin") + // Try to delete → should get 400 + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "1"}} + c.Set("uid", int64(999)) // different user + h.DeleteUser(c) + if w.Code != 400 { t.Fatalf("want 400, got %d", w.Code) } +} +``` + +- [ ] **Step 4: Run all tests (unit + integration)** + +```bash +go test -p 1 -count=1 ./... +``` + +- [ ] **Step 5: Commit** + +```bash +git add backend/internal/ports/portsfake/ backend/cmd/webui/handlers/*_unit_test.go backend/cmd/webui/api/router_test.go +git commit -m "test: add portsfake, router contract tests, handler unit tests" +``` + +--- + +### Task 28: Consolidate isUnique + dedup helpers + +**Files:** +- Modify: `backend/cmd/webui/handlers/users.go` (remove local `isUnique`) +- Modify: `backend/internal/seed/seed.go` (use `ports.IsUniqueViolation`) +- Modify: All handlers that duplicate error-check patterns + +- [ ] **Step 1: Replace all `isUnique` calls** + +Search for `isUnique(` and `pgconn.PgError` with code `23505` across the codebase. Replace with `ports.IsUniqueViolation(err)` or `store.IsUniqueViolation(err)`. + +Remove the local `isUnique` function from `handlers/users.go`. +Update `seed.go` to use `store.IsUniqueViolation`. + +- [ ] **Step 2: Consolidate path validation** + +Replace the 3 different path-contains checks with `bookfile.Contains`: +- `libraries.go` `libRoot` → `bookfile.Contains(h.cfg.BooksDir, root)` +- `books.go` `hasPrefixDir` → `bookfile.Contains` +- `scanner.go` `inside` → `bookfile.Contains` (already done in Task 24) + +Remove local helper functions. + +- [ ] **Step 3: Run + commit** + +```bash +go build ./... && go test -p 1 -count=1 ./... +git add -A && git commit -m "refactor: consolidate isUnique and path validation helpers" +``` + +--- + +### Task 29: Final docs + local gate + +**Files:** +- Modify: `docs/CHANGELOG.md` +- Modify: `docs/README.md`, `docs/README_zh.md` + +- [ ] **Step 1: Add CHANGELOG for Batch C** + +``` +### Changed +- Business logic restructured: handlers now consume small port interfaces, domain logic in internal/media and internal/upload. +- 业务逻辑重构:handlers 消费小口径 port 接口,域逻辑下沉至 internal/media 和 internal/upload。 +- Upload sweep moved from request path to scanner ticker cycle (B16). +- 上传会话清理从请求路径移至扫描器定时周期(B16)。 + +### Added +- Router contract test pins expected route table; unauthorized changes fail CI. +- 路由契约测试锁定预期路由表,未授权变更将导致 CI 失败。 +- Hand-written in-memory fakes (portsfake) enable handler unit tests without PG/Redis. +- 手写内存 fake(portsfake)实现无 PG/Redis 的 handler 单测。 +``` + +- [ ] **Step 2: Update READMEs** + +Document the new package structure (ports, media, upload). Explain the interface/fake testing approach. + +- [ ] **Step 3: Full local gate** + +```bash +cd backend && go vet ./... && gofmt -l . +DATABASE_URL=postgres://booklib:booklib@localhost:5432/booklib?sslmode=disable \ +REDIS_URL=redis://localhost:6379/0 \ +go test -p 1 -count=1 -v ./... +scripts/smoke.sh +``` + +Confirm: +- 0 skip in test output +- All 47+ original integration tests pass +- New fake-based unit tests pass +- Router contract test passes +- smoke.sh passes + +- [ ] **Step 4: Commit** + +```bash +git add docs/ +git commit -m "docs: changelog + README for batch C (restructure, contract tests)" +``` + +--- + +## Verification Checklist + +After all 29 tasks: + +1. `go vet ./...` — clean +2. `gofmt -l .` — empty output +3. `go test -p 1 -count=1 ./...` — all pass, 0 skip +4. `scripts/smoke.sh` — ALL SMOKE TESTS PASSED +5. `scripts/smoke-web.sh` — passes (if compose available) +6. Baseline test: old DB → new code → `schema_migrations` has `0001_baseline.sql` row +7. No `CountAdmins` exported (removed) +8. No `ListBookIDs` exported (removed, dead code) +9. No local `isUnique` functions outside `store.IsUniqueViolation` +10. No `inside` function in scanner (replaced by `bookfile.Contains`) +11. Router contract test pins all routes +12. All 17 bugs (B1–B17) have corresponding tests or smoke verification