From bc5411c30ce1b7cf5196c88931f55e5d97aad04b Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Tue, 8 Sep 2026 15:42:11 +0800 Subject: [PATCH 1/3] =?UTF-8?q?feat=E2=9C=A8:=20report=20what=20the=20migr?= =?UTF-8?q?ation=20registry=20holds=20without=20a=20database?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Status answers what is registered, what is applied, and what is applied while nothing registers it - and needs a database to do it. A readiness check needs only the first half, and it already holds the databases it is asking about. Without this it would have to call SetDb to reuse Status, writing this package's shared state from a request path, for a question that does not depend on any database at all. --- cmd/migrate/migration/init.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/cmd/migrate/migration/init.go b/cmd/migrate/migration/init.go index 669bec65..309d41c2 100644 --- a/cmd/migrate/migration/init.go +++ b/cmd/migrate/migration/init.go @@ -185,6 +185,28 @@ func (e *Migration) mergedEntries() map[string]versionEntry { return out } +// RegisteredVersions returns every migration version this binary registers, +// sorted, without touching a database. +// +// Status answers a richer question - what is registered, what is applied, and +// what is applied while nothing registers it - and needs a database to do it. +// This is the half that can be asked of the process alone, which is what a +// readiness check needs: the check holds the databases it is asking about, and +// reusing Status would mean calling SetDb from a request handler, writing this +// package's shared state from a request path. +func (e *Migration) RegisteredVersions() []string { + all := e.mergedEntries() + out := make([]string, 0, len(all)) + for k := range all { + out = append(out, k) + } + sort.Strings(out) + return out +} + +// RegisteredVersions reports what the process-wide registry holds. +func RegisteredVersions() []string { return Migrate.RegisteredVersions() } + // StatusEntry is one row of migrate status. type StatusEntry struct { AppCode string From e98b65cf900eb399e88abf28c6f88e0001b12f87 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Tue, 8 Sep 2026 15:42:14 +0800 Subject: [PATCH 2/3] =?UTF-8?q?feat=E2=9C=A8:=20let=20the=20host=20add=20a?= =?UTF-8?q?=20readiness=20check=20this=20package=20cannot=20make?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whether the schema matches what the binary expects is answered by the migration registry, which lives under cmd/. common/ has never imported cmd/, and starting with this would put the shared layer behind the command layer for one check. Register instead, from where both are already in scope. A duplicate name panics rather than appending: two checks under one name make the failing one impossible to identify from the response body, and registering the same one twice is a wiring mistake better heard at start-up than never. The registered check is run through the same guard as the built-in ones, so one that panics fails its check rather than taking down the probe that asked. --- common/health/health.go | 56 ++++++++++++++- common/health/register_test.go | 125 +++++++++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 common/health/register_test.go diff --git a/common/health/health.go b/common/health/health.go index 4d4095eb..b6311120 100644 --- a/common/health/health.go +++ b/common/health/health.go @@ -41,6 +41,7 @@ import ( "encoding/hex" "errors" "fmt" + "sync" "sync/atomic" "time" @@ -68,6 +69,55 @@ type Check struct { Err string `json:"error,omitempty"` } +// extra are checks a host registers that this package cannot make itself. +// +// The direction is why this exists. Whether the schema matches what the binary +// expects is answered by the migration registry, which lives under cmd/ - and +// common/ has never imported cmd/. Rather than start, the host registers the +// check from where both are already in scope. +var ( + extraMu sync.RWMutex + extra []namedCheck +) + +type namedCheck struct { + name string + fn func(context.Context) error +} + +// Register adds a check to what Ready asks. +// +// It panics on a duplicate name rather than replacing or appending: two checks +// under one name make the failing one impossible to identify from the response, +// and registering the same one twice is a wiring mistake worth hearing about at +// start-up rather than never. +func Register(name string, fn func(context.Context) error) { + if name == "" { + panic("health: a registered check needs a name") + } + if fn == nil { + panic("health: check " + name + " is nil") + } + extraMu.Lock() + defer extraMu.Unlock() + for _, c := range extra { + if c.name == name { + panic("health: check " + name + " is already registered") + } + } + extra = append(extra, namedCheck{name: name, fn: fn}) +} + +// registered returns the checks a host has added, copied so that Ready is not +// iterating the slice while another goroutine appends to it. +func registered() []namedCheck { + extraMu.RLock() + defer extraMu.RUnlock() + out := make([]namedCheck, len(extra)) + copy(out, extra) + return out +} + // Ready asks every dependency this process cannot serve a request without. // // The queue is deliberately absent. Nothing on AdapterQueue answers "are you @@ -75,10 +125,14 @@ type Check struct { // a queue that is down degrades logging rather than stopping requests - which // is a reason to alert, not a reason to leave the load balancer pool. func Ready(ctx context.Context) []Check { - return []Check{ + checks := []Check{ safely("database", func() error { return pingDB(ctx) }), safely("cache", probeCache), } + for _, c := range registered() { + checks = append(checks, safely(c.name, func() error { return c.fn(ctx) })) + } + return checks } // safely turns a panic into a failed check. diff --git a/common/health/register_test.go b/common/health/register_test.go new file mode 100644 index 00000000..9edb6e19 --- /dev/null +++ b/common/health/register_test.go @@ -0,0 +1,125 @@ +package health + +import ( + "context" + "errors" + "testing" +) + +// isolate empties the registered checks and puts them back, so one test in +// this package cannot decide what the next one sees. +func isolate(t *testing.T) { + t.Helper() + extraMu.Lock() + previous := extra + extra = nil + extraMu.Unlock() + t.Cleanup(func() { + extraMu.Lock() + extra = previous + extraMu.Unlock() + }) +} + +func findCheck(checks []Check, name string) (Check, bool) { + for _, c := range checks { + if c.Name == name { + return c, true + } + } + return Check{}, false +} + +// A registered check has to reach Ready's answer, or the host has wired +// something that never gets asked. +func TestARegisteredCheckIsAsked(t *testing.T) { + isolate(t) + Register("schema", func(context.Context) error { return errors.New("two behind") }) + + checks := Ready(context.Background()) + c, ok := findCheck(checks, "schema") + if !ok { + t.Fatal("Ready did not ask the registered check") + } + if c.OK { + t.Error("the check returned an error and was still reported OK") + } + if c.Err != "two behind" { + t.Errorf("Err = %q, want the check's own message", c.Err) + } + if Healthy(checks) { + t.Error("Healthy said yes while a registered check was failing") + } +} + +// The context Ready is given has to reach the check: it carries the probe's +// deadline, and a check that ignores it can hold the handler past it. +func TestTheRegisteredCheckIsGivenReadysContext(t *testing.T) { + isolate(t) + type key struct{} + Register("ctx", func(ctx context.Context) error { + if ctx.Value(key{}) != "carried" { + return errors.New("the check was handed a different context") + } + return nil + }) + + checks := Ready(context.WithValue(context.Background(), key{}, "carried")) + c, ok := findCheck(checks, "ctx") + if !ok { + t.Fatal("the registered check was not asked") + } + if !c.OK { + t.Errorf("check failed: %s", c.Err) + } +} + +// A check that panics must not take the process down through the probe, the +// same guarantee the built-in checks have. +func TestARegisteredCheckThatPanicsFailsRatherThanCrashes(t *testing.T) { + isolate(t) + Register("boom", func(context.Context) error { panic("registry unreachable") }) + + checks := Ready(context.Background()) + c, ok := findCheck(checks, "boom") + if !ok { + t.Fatal("the registered check was not asked") + } + if c.OK { + t.Error("a panicking check was reported OK") + } +} + +func TestRegisteringTheSameNameTwicePanics(t *testing.T) { + isolate(t) + Register("dup", func(context.Context) error { return nil }) + + defer func() { + if recover() == nil { + t.Error("registering a duplicate name did not panic; two checks under " + + "one name make the failing one impossible to identify") + } + }() + Register("dup", func(context.Context) error { return nil }) +} + +func TestRegisterRefusesAnEmptyNameOrNilCheck(t *testing.T) { + isolate(t) + for _, tc := range []struct { + name string + fn func(context.Context) error + why string + }{ + {"", func(context.Context) error { return nil }, "empty name"}, + {"nilfn", nil, "nil function"}, + } { + func() { + defer func() { + if recover() == nil { + t.Errorf("%s did not panic", tc.why) + } + }() + Register(tc.name, tc.fn) + }() + } +} From d6e2c02fda169c75237b486705ac0b76cbeed0a4 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Tue, 8 Sep 2026 15:42:14 +0800 Subject: [PATCH 3/3] =?UTF-8?q?fix=F0=9F=90=9B:=20fail=20readiness=20while?= =?UTF-8?q?=20the=20database=20is=20behind=20the=20migrations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The process started, both probes passed, and the first sign that the schema did not match was a login failing with a driver-level encoding error - go-admin#919, where an operator upgraded the binary and restarted the API without running migrate. Nothing between those two events had an opinion about the schema. Readiness is where this belongs. Liveness asks "restart me", and a process whose database is on the wrong schema comes back to the same schema. Readiness asks "send me requests", and the answer is no. A rolling update then stalls at the deploy - new instances never become ready, the old ones keep serving - rather than at somebody's login, and running migrate clears it without a restart because the check is evaluated per request. Any tenant database being behind fails the check, not only the one being served: migrations are applied to every database in one run, so one behind means that run did not finish, and serving the rest would let a half-applied deploy look like a partial success. A missing sys_migration table is nothing applied rather than an error. That is a first deploy, where every migration is pending and the operator can act on being told so. The one test that matters is the one that cannot be written normally. The registry is filled by init() in packages cmd/api does not import, so a test that imported them to look at it would pass whatever the real binary links - and a binary that links none of them gives a check that reports every database current, forever, with every other test here still green. TestTheServingBinaryLinksTheMigrationRegistry asks the build instead, with a negative control so that a query matching everything fails rather than passes. Closes #920. --- cmd/api/schema.go | 117 ++++++++++++++++++++++++++++++++++++++ cmd/api/schema_test.go | 124 +++++++++++++++++++++++++++++++++++++++++ cmd/api/server.go | 5 ++ 3 files changed, 246 insertions(+) create mode 100644 cmd/api/schema.go create mode 100644 cmd/api/schema_test.go diff --git a/cmd/api/schema.go b/cmd/api/schema.go new file mode 100644 index 00000000..492dc96b --- /dev/null +++ b/cmd/api/schema.go @@ -0,0 +1,117 @@ +package api + +import ( + "context" + "fmt" + "sort" + "strings" + + "github.com/go-admin-team/go-admin-core/v2/sdk" + "gorm.io/gorm" + + "go-admin/cmd/migrate/migration" + "go-admin/common/health" + commonmodels "go-admin/common/models" +) + +// schemaCheckName is what a failing schema reports itself as in /ready's body. +const schemaCheckName = "schema" + +// registerSchemaCheck adds the pending-migration check to readiness. +// +// Readiness rather than a refusal to start, and rather than a log line alone. +// The two probes answer different questions: liveness is "restart me", and a +// process whose database is on the wrong schema comes back to the same schema, +// so restarting is not the answer. Readiness is "send me requests", and with a +// schema the binary does not match the answer is no. +// +// Issue #919 is what the absence of this looked like: the process started, +// both probes passed, and the first sign of trouble was a login failing with a +// driver-level encoding error. Refusing to start would have been the wrong fix +// - a process that exits tells an operator less than one that runs and says +// why, and under an orchestrator it crash-loops - while a log line alone is +// not something an orchestrator can act on. +func registerSchemaCheck() { + health.Register(schemaCheckName, schemaCheck) +} + +// schemaCheck fails while any tenant database is behind the migrations this +// binary registers. +// +// Any one of them, rather than only the tenant being served: migrations are +// applied to every database in one run, so one database behind means that run +// did not finish. Serving the rest would let a half-applied deploy look like a +// partial success. +// +// Evaluated per request rather than decided at start-up, so that running +// migrate clears it without a restart. +func schemaCheck(ctx context.Context) error { + registered := migration.RegisteredVersions() + if len(registered) == 0 { + // Nothing registered means nothing can be pending, which is the honest + // answer for a tree with no migrations. It is also what a broken build + // would produce - the registry is filled by init() in packages the + // binary has to link - so cmd/api's dependency test asserts the real + // binary links them. + return nil + } + + behind := make([]string, 0, 2) + for name, db := range sdk.Runtime.GetAllDb() { + applied, err := appliedVersions(ctx, db) + if err != nil { + return fmt.Errorf("reading applied migrations for %q: %w", name, err) + } + if pending := pendingVersions(registered, applied); len(pending) > 0 { + behind = append(behind, fmt.Sprintf("%s is %d behind, first pending %s", + name, len(pending), pending[0])) + } + } + if len(behind) == 0 { + return nil + } + sort.Strings(behind) + return fmt.Errorf("%s; run `go-admin migrate -c ` and see `go-admin migrate status`", + strings.Join(behind, "; ")) +} + +// appliedVersions reads what sys_migration records for one database. +// +// A missing table is not an error: a database that has never been migrated has +// applied nothing, which is exactly what the caller needs to hear, and is the +// state a first deploy is in. +func appliedVersions(ctx context.Context, db *gorm.DB) (map[string]bool, error) { + if db == nil { + return nil, fmt.Errorf("no database") + } + db = db.WithContext(ctx) + if !db.Migrator().HasTable(&commonmodels.Migration{}) { + return map[string]bool{}, nil + } + var rows []commonmodels.Migration + if err := db.Select("version").Find(&rows).Error; err != nil { + return nil, err + } + out := make(map[string]bool, len(rows)) + for _, r := range rows { + out[r.Version] = true + } + return out, nil +} + +// pendingVersions returns the registered versions applied does not contain. +// +// Split out and taking both sides as arguments because the registry is +// process-wide and filled by init() in packages cmd/api does not import: a +// test in this package cannot arrange it, so the arranging part is the part +// that is not tested here. +func pendingVersions(registered []string, applied map[string]bool) []string { + out := make([]string, 0) + for _, v := range registered { + if !applied[v] { + out = append(out, v) + } + } + sort.Strings(out) + return out +} diff --git a/cmd/api/schema_test.go b/cmd/api/schema_test.go new file mode 100644 index 00000000..505e0cf5 --- /dev/null +++ b/cmd/api/schema_test.go @@ -0,0 +1,124 @@ +package api + +import ( + "context" + "os/exec" + "strings" + "testing" + + "github.com/glebarez/sqlite" + "gorm.io/gorm" + + commonmodels "go-admin/common/models" +) + +func TestPendingVersionsReportsOnlyWhatIsNotApplied(t *testing.T) { + registered := []string{"1000_a", "2000_b", "3000_c"} + applied := map[string]bool{"1000_a": true, "3000_c": true} + + got := pendingVersions(registered, applied) + if len(got) != 1 || got[0] != "2000_b" { + t.Errorf("pending = %v, want [2000_b]", got) + } +} + +func TestPendingVersionsIsEmptyWhenTheDatabaseIsCurrent(t *testing.T) { + registered := []string{"1000_a", "2000_b"} + applied := map[string]bool{"1000_a": true, "2000_b": true} + + if got := pendingVersions(registered, applied); len(got) != 0 { + t.Errorf("pending = %v, want none", got) + } +} + +// A row recorded that this binary no longer registers is not pending. It is +// the orphan `migrate status` already reports, and readiness has nothing to +// say about it: the schema is ahead, not behind, and requests will be served +// correctly. +func TestPendingVersionsIgnoresAppliedRowsNothingRegisters(t *testing.T) { + registered := []string{"1000_a"} + applied := map[string]bool{"1000_a": true, "9999_gone": true} + + if got := pendingVersions(registered, applied); len(got) != 0 { + t.Errorf("pending = %v, want none - an orphaned row is not a pending migration", got) + } +} + +func memoryDB(t *testing.T) *gorm.DB { + t.Helper() + db, err := gorm.Open(sqlite.Open("file::memory:?cache=shared"), &gorm.Config{}) + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { db.Migrator().DropTable(&commonmodels.Migration{}) }) + return db +} + +// A first deploy has no sys_migration table. That is "nothing applied", not an +// error: reporting it as one would make the check fail for a reason the +// operator cannot act on, on the one deployment where every migration really +// is pending. +func TestAppliedVersionsTreatsAMissingTableAsNothingApplied(t *testing.T) { + db := memoryDB(t) + db.Migrator().DropTable(&commonmodels.Migration{}) + + got, err := appliedVersions(context.Background(), db) + if err != nil { + t.Fatalf("appliedVersions: %v", err) + } + if len(got) != 0 { + t.Errorf("applied = %v, want empty", got) + } +} + +func TestAppliedVersionsReadsWhatTheTableHolds(t *testing.T) { + db := memoryDB(t) + if err := db.AutoMigrate(&commonmodels.Migration{}); err != nil { + t.Fatalf("automigrate: %v", err) + } + db.Create(&commonmodels.Migration{Version: "1000_a"}) + db.Create(&commonmodels.Migration{Version: "2000_b"}) + + got, err := appliedVersions(context.Background(), db) + if err != nil { + t.Fatalf("appliedVersions: %v", err) + } + if !got["1000_a"] || !got["2000_b"] || len(got) != 2 { + t.Errorf("applied = %v, want the two rows written", got) + } +} + +// The check is only worth anything if the registry it reads is populated in +// the binary that serves requests, and it is filled by init() in packages +// cmd/api does not import - cmd/migrate blank-imports them, and cmd wires both +// subcommands into one binary. +// +// This cannot be asserted from an ordinary test: importing the version package +// to look at the registry would put it in the test binary's dependency graph +// and pass whatever the real binary links. So ask the build instead. +// +// Without this, dropping those blank imports leaves a check that reports +// "nothing pending" for every database forever, and every test above still +// passes. +func TestTheServingBinaryLinksTheMigrationRegistry(t *testing.T) { + out, err := exec.Command("go", "list", "-deps", "go-admin").Output() + if err != nil { + t.Skipf("go list unavailable: %v", err) + } + deps := string(out) + + const versions = "go-admin/cmd/migrate/migration/version" + if !strings.Contains(deps, versions+"\n") { + t.Errorf("the main package does not link %s, so the schema check would "+ + "read an empty registry and report every database as current", versions) + } + + // Negative control: a package the binary genuinely must not link, so that a + // `deps` that somehow contained everything would fail here rather than pass + // the assertion above for the wrong reason. + const notLinked = "go-admin/tools/checksilent" + if strings.Contains(deps, notLinked+"\n") { + t.Errorf("%s is in the binary's dependency closure, so this test cannot "+ + "tell a real link from a query that matches anything", notLinked) + } +} diff --git a/cmd/api/server.go b/cmd/api/server.go index 7bc45c58..f7ebbd4e 100644 --- a/cmd/api/server.go +++ b/cmd/api/server.go @@ -82,6 +82,11 @@ func setup() { // can call the API, which is only true once the socket is accepting. sdk.Runtime.SetPhase(runtime.AfterListen, startCronJobs) + // Registered before the configuration is read, because it registers a + // callback rather than reading anything: the check runs per request and + // asks the databases that exist then. + registerSchemaCheck() + //1. 读取配置 bootstrap.SetupConfig( file.NewSource(file.WithPath(configYml)),