mirror of
https://github.com/Kpa-clawbot/meshcore-analyzer.git
synced 2026-08-28 21:48:16 +00:00
Closes the recurring "sync migration on large table" regression class
(#791-style, #1483-style).
## Problem
Pattern that keeps repeating:
1. A perf/feature PR adds `CREATE INDEX` / `ALTER TABLE` / `UPDATE ...
WHERE` in a migration file (typically `cmd/ingestor/db.go`).
2. Local dev DB has ~100 rows. Migration returns in milliseconds. CI is
green.
3. Reviewers approve on plan correctness; nobody knows what the prod
table size is.
4. First prod boot at scale (Cascadia: ~2600 nodes, 80K+ obs; previous
prod: 1.9M+ obs) pins the ingestor at `[migration] Adding index...` for
minutes.
5. Healthcheck times out → container restart → loop. Operator pages.
Hotfix.
Most recent case: `obs_observer_ts_idx_v1` in v3.8.3 — release notes
already document an "expect a longer first boot" warning because we knew
it would hit prod hard.
## What this PR adds
**Async helper (`cmd/ingestor/async_migration.go`):**
- `Store.RunAsyncMigration(ctx, name, fn)` — registers the migration as
`pending_async` in a new `_async_migrations` bookkeeping table, returns
to caller immediately, schedules `fn` in a goroutine on the shared
backfill `WaitGroup`, transitions to `done` (or `failed` with error
captured) on completion.
- `Store.AsyncMigrationStatus(name)` and
`Store.WaitForAsyncMigrations()` for tests/shutdown.
- Idempotent: `done` rows short-circuit; `pending_async`/`failed` rows
are retried on next boot.
**Retroactive #1483 conversion (`cmd/ingestor/db.go`):**
- `obs_observer_ts_idx_v1` (the composite `(observer_idx, timestamp)`
index build on `observations`) is now scheduled via `RunAsyncMigration`
from `OpenStore()` so the ingestor accepts packets immediately while the
index builds in the background.
- Legacy `_migrations` gate is preserved by the async fn → DBs that
already completed the sync build stay no-op.
**Annotation convention (`MIGRATIONS.md`):**
Every new `CREATE INDEX` / `ALTER TABLE` / data-rewrite in a migration
file must do ONE of:
1. Run via `Store.RunAsyncMigration(...)` (preferred for backfills).
2. Carry a `// PREFLIGHT: async=true reason="..."` comment directly
above the migration block.
3. Include a `PREFLIGHT-MIGRATION-SCALE: <30s N=<scale>` line in the PR
body.
**TDD pair:**
- Red commit `2c6744cc` — `TestRunAsyncMigration_PendingThenDone`
against a stub helper. Build passes, assertion fails (`async migration
fn did not start within 2s`).
- Green commit `38354f32` — real helper + retroactive fix + docs. Test
green.
**Fixtures (`cmd/ingestor/testdata/preflight-migrations/`):**
- `bad_sync_migration.go` — known-bad sample with no annotation.
- `good_annotated_migration.go` — known-good sample with annotation.
The preflight gate script can be unit-tested against these.
## Gate location (NOT in this PR)
The actual `check-async-migrations.sh` lives in the OpenClaw skills
directory at `~/.openclaw/skills/pr-preflight/scripts/` (separate from
the repo) and is wired into `run-all.sh`. It greps the diff for
new/modified migration blocks and hard-fails (exit 1) on any sync schema
mutation lacking one of the three opt-outs above. The fixtures in this
PR give maintainers a reproducible target.
## Why annotation-discipline, not size detection
You cannot determine table size from a diff. The gate enforces that
every author who adds a schema migration must consciously decide which
bucket it falls into and write that down. That is the cheapest possible
intervention that breaks the cycle.
## Testing
- `go test ./...` in `cmd/ingestor` — all tests pass including the new
`TestRunAsyncMigration_PendingThenDone`.
- Manual: red commit fails on assertion (not build), green commit passes
— verifiable by `git checkout 2c6744cc --
cmd/ingestor/async_migration.go && go test -run TestRunAsync
./cmd/ingestor` from the green commit.
## Preflight overrides
None — clean run after the convention is applied.
---------
Co-authored-by: clawbot <bot@openclaw.local>
Co-authored-by: clawbot <bot@openclaw>
149 lines
5.8 KiB
Go
149 lines
5.8 KiB
Go
// Async migration helper — runs schema/backfill work that may take minutes on
|
|
// large prod tables WITHOUT blocking ingestor startup.
|
|
//
|
|
// MIGRATION ANNOTATION CONVENTION (read this before touching migrations):
|
|
//
|
|
// Sync schema/data migrations (CREATE INDEX, ALTER TABLE, UPDATE ... WHERE)
|
|
// that run inline during OpenStore() block the ingestor from accepting
|
|
// packets until they finish. On an empty dev DB they return in milliseconds;
|
|
// at prod scale (1.9M+ observations, 80K+ adverts) they can pin the boot
|
|
// for minutes and trigger restart loops. This regression class has bitten us
|
|
// repeatedly (#791 resolved_path backfill, #1483 obs_observer_ts_idx_v1).
|
|
//
|
|
// ANY new CREATE INDEX / ALTER TABLE / data-rewrite migration MUST EITHER:
|
|
// 1. Run via Store.RunAsyncMigration(...) below (preferred for backfills
|
|
// and any work that may touch >1K rows). The migration is recorded as
|
|
// `pending_async` immediately, returns to the caller (boot proceeds),
|
|
// and completes in a goroutine. Status flips to `done` (or `failed`
|
|
// with an error message) when fn returns.
|
|
// 2. Carry the preflight annotation comment immediately above the
|
|
// migration block, e.g.
|
|
// // PREFLIGHT: async=true reason="<one-line justification>"
|
|
// Use this for migrations that are genuinely cheap at any scale
|
|
// (e.g. ALTER TABLE ADD COLUMN, CREATE INDEX on a known-bounded
|
|
// table). The annotation is grepped by
|
|
// ~/.openclaw/skills/pr-preflight/scripts/check-async-migrations.sh
|
|
// — its absence on a touched migration block is a hard-fail gate.
|
|
//
|
|
// See MIGRATIONS.md in the repo root for the full policy and examples.
|
|
|
|
package main
|
|
|
|
import (
|
|
"context"
|
|
"database/sql"
|
|
"fmt"
|
|
"log"
|
|
)
|
|
|
|
// ensureAsyncMigrationsTable creates the bookkeeping table used by
|
|
// RunAsyncMigration / AsyncMigrationStatus. Idempotent.
|
|
func ensureAsyncMigrationsTable(db *sql.DB) error {
|
|
_, err := db.Exec(`
|
|
CREATE TABLE IF NOT EXISTS _async_migrations (
|
|
name TEXT PRIMARY KEY,
|
|
status TEXT NOT NULL, -- pending_async | done | failed
|
|
started_at TEXT NOT NULL DEFAULT (datetime('now')),
|
|
ended_at TEXT,
|
|
error TEXT
|
|
)
|
|
`)
|
|
return err
|
|
}
|
|
|
|
// RunAsyncMigration registers `name` as a pending async migration and
|
|
// schedules `fn` to run in a background goroutine. It returns to the caller
|
|
// immediately so the ingestor can keep booting.
|
|
//
|
|
// Contract (pinned by async_migration_test.go):
|
|
// - status is `pending_async` IMMEDIATELY after this returns.
|
|
// - fn runs in a goroutine; on success status becomes `done`, on error or
|
|
// panic status becomes `failed` and the error is recorded.
|
|
// - Idempotent: if a row with the same name already exists in `done`
|
|
// state, fn is NOT re-run. If in `failed` or `pending_async` state,
|
|
// fn IS re-scheduled (a previous run may have crashed mid-flight).
|
|
// - The caller's WaitGroup tracks the goroutine so tests/shutdown can
|
|
// wait via Store.WaitForAsyncMigrations().
|
|
func (s *Store) RunAsyncMigration(ctx context.Context, name string, fn func(context.Context, *sql.DB) error) error {
|
|
if err := ensureAsyncMigrationsTable(s.db); err != nil {
|
|
return fmt.Errorf("ensure _async_migrations: %w", err)
|
|
}
|
|
|
|
var existing string
|
|
row := s.db.QueryRow(`SELECT status FROM _async_migrations WHERE name = ?`, name)
|
|
switch err := row.Scan(&existing); err {
|
|
case nil:
|
|
if existing == "done" {
|
|
return nil // already complete, nothing to do
|
|
}
|
|
// pending_async or failed → reset and retry.
|
|
if _, err := s.db.Exec(`
|
|
UPDATE _async_migrations
|
|
SET status = 'pending_async', started_at = datetime('now'), ended_at = NULL, error = NULL
|
|
WHERE name = ?`, name); err != nil {
|
|
return fmt.Errorf("reset async migration %q: %w", name, err)
|
|
}
|
|
case sql.ErrNoRows:
|
|
if _, err := s.db.Exec(`
|
|
INSERT INTO _async_migrations (name, status) VALUES (?, 'pending_async')`,
|
|
name); err != nil {
|
|
return fmt.Errorf("register async migration %q: %w", name, err)
|
|
}
|
|
default:
|
|
return fmt.Errorf("lookup async migration %q: %w", name, err)
|
|
}
|
|
|
|
s.backfillWg.Add(1)
|
|
go func() {
|
|
defer s.backfillWg.Done()
|
|
var runErr error
|
|
defer func() {
|
|
if r := recover(); r != nil {
|
|
runErr = fmt.Errorf("panic: %v", r)
|
|
log.Printf("[async-migration] %q panic recovered: %v", name, r)
|
|
}
|
|
if runErr != nil {
|
|
if _, err := s.db.Exec(`
|
|
UPDATE _async_migrations
|
|
SET status = 'failed', ended_at = datetime('now'), error = ?
|
|
WHERE name = ?`, runErr.Error(), name); err != nil {
|
|
log.Printf("[async-migration] failed to record failure for %q: %v", name, err)
|
|
}
|
|
log.Printf("[async-migration] %q FAILED: %v", name, runErr)
|
|
return
|
|
}
|
|
if _, err := s.db.Exec(`
|
|
UPDATE _async_migrations
|
|
SET status = 'done', ended_at = datetime('now'), error = NULL
|
|
WHERE name = ?`, name); err != nil {
|
|
log.Printf("[async-migration] failed to mark %q done: %v", name, err)
|
|
return
|
|
}
|
|
log.Printf("[async-migration] %q done", name)
|
|
}()
|
|
log.Printf("[async-migration] %q starting (boot continues)", name)
|
|
runErr = fn(ctx, s.db)
|
|
}()
|
|
|
|
return nil
|
|
}
|
|
|
|
// AsyncMigrationStatus returns the current status of an async migration
|
|
// (one of "pending_async", "done", "failed") or sql.ErrNoRows if no such
|
|
// migration has been registered.
|
|
func (s *Store) AsyncMigrationStatus(name string) (string, error) {
|
|
if err := ensureAsyncMigrationsTable(s.db); err != nil {
|
|
return "", err
|
|
}
|
|
var status string
|
|
err := s.db.QueryRow(`SELECT status FROM _async_migrations WHERE name = ?`, name).Scan(&status)
|
|
return status, err
|
|
}
|
|
|
|
// WaitForAsyncMigrations blocks until all currently-scheduled async migrations
|
|
// finish. Intended for tests + graceful shutdown; production boot path does NOT
|
|
// call this (that's the whole point).
|
|
func (s *Store) WaitForAsyncMigrations() {
|
|
s.backfillWg.Wait()
|
|
}
|