From 473a8fd4a79a82434eec721ef9b7d0a323238741 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Sat, 23 May 2026 10:48:36 -0700 Subject: [PATCH 1/2] restart: fail loudly when managed Dolt does not come back up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gc restart` previously returned success even when the supervisor's post-start health probe for managed Dolt failed. That probe is marked non-fatal in prepareCityForSupervisor, so a "Running" city can sit with the bead-store backend unreachable. Every bd-backed alerting path then silently fails: agents can't nudge each other, escalation beads can't be created, and the operator's only signal is "nothing is working." Add a post-restart Dolt healthcheck in cmdRestartJSON. After the start step reports success, poll healthBeadsProvider against the city until managed Dolt is queryable or a configurable budget (default 30s; env override GC_RESTART_DOLT_HEALTH_TIMEOUT) expires. On timeout, write a clear error naming the cause and pointing at recovery (`gc start `), and exit non-zero. The check is a no-op for cities where gc does not own the Dolt lifecycle (file provider, postgres backend, external Dolt). Tests cover: no-op for file provider, no-op for external Dolt, success on first probe, success after retry, timeout produces a loud error message naming cause and recovery, env-var parsing handles invalid/zero/negative durations, and an integration view through cmdRestartJSON that confirms the command surfaces the failure to stderr and exits non-zero. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-1 On behalf of: @austinborn Co-Authored-By: .invalid> --- cmd/gc/cmd_restart.go | 32 +++- cmd/gc/restart_dolt_health.go | 103 +++++++++++ cmd/gc/restart_dolt_health_test.go | 286 +++++++++++++++++++++++++++++ 3 files changed, 417 insertions(+), 4 deletions(-) create mode 100644 cmd/gc/restart_dolt_health.go create mode 100644 cmd/gc/restart_dolt_health_test.go diff --git a/cmd/gc/cmd_restart.go b/cmd/gc/cmd_restart.go index cd2f646404..c5bd86a426 100644 --- a/cmd/gc/cmd_restart.go +++ b/cmd/gc/cmd_restart.go @@ -11,6 +11,16 @@ import ( "github.com/spf13/cobra" ) +// Test seams: cmdRestartJSON's stop / start / name-resolve steps are +// indirected through these vars so tests can drive the post-restart +// healthcheck branch without spinning a real city. Production callers +// inherit the package-level defaults unchanged. +var ( + restartRegistrationNameHook = restartRegistrationName + restartCmdStopHook = cmdStop + restartDoStartWithNameOverrideHook = doStartWithNameOverride +) + // newRestartCmd creates the top-level "gc restart" command. func newRestartCmd(stdout, stderr io.Writer) *cobra.Command { var jsonOut bool @@ -35,7 +45,7 @@ immediate reconcile.`, } func cmdRestartJSON(args []string, stdout, stderr io.Writer, jsonOut bool) int { - nameOverride, err := restartRegistrationName(args) + nameOverride, err := restartRegistrationNameHook(args) if err != nil { fmt.Fprintf(stderr, "gc restart: %v\n", err) //nolint:errcheck // best-effort stderr return 1 @@ -44,11 +54,11 @@ func cmdRestartJSON(args []string, stdout, stderr io.Writer, jsonOut bool) int { if jsonOut { restartStdout = io.Discard } - if code := cmdStop(args, restartStdout, stderr, 0, false); code != 0 { + if code := restartCmdStopHook(args, restartStdout, stderr, 0, false); code != 0 { return code } - code := doStartWithNameOverride(args, false /*controllerMode*/, restartStdout, stderr, nameOverride) - if code != 0 || !jsonOut { + code := restartDoStartWithNameOverrideHook(args, false /*controllerMode*/, restartStdout, stderr, nameOverride) + if code != 0 { return code } cityPath := "" @@ -57,6 +67,20 @@ func cmdRestartJSON(args []string, stdout, stderr io.Writer, jsonOut bool) int { cityPath = resolved } } + // Post-restart Dolt healthcheck. The supervisor reports a city as Running + // once its tick succeeds, but prepareCityForSupervisor treats the + // bead-store health probe as non-fatal — a "Running" city can have + // managed Dolt unreachable, which silently blinds every bd-backed + // alerting path. Verify before returning success. + if cityPath != "" { + if err := verifyDoltHealthyAfterRestartHook(cityPath, stderr); err != nil { + fmt.Fprintf(stderr, "gc restart: %v\n", err) //nolint:errcheck // best-effort stderr + return 1 + } + } + if !jsonOut { + return code + } return writeLifecycleActionJSONOrExit(stdout, stderr, "gc restart", lifecycleActionJSON{ Command: "restart", Action: "restart", diff --git a/cmd/gc/restart_dolt_health.go b/cmd/gc/restart_dolt_health.go new file mode 100644 index 0000000000..3cf0fa25ce --- /dev/null +++ b/cmd/gc/restart_dolt_health.go @@ -0,0 +1,103 @@ +package main + +import ( + "fmt" + "io" + "os" + "strings" + "time" +) + +// restartDoltHealthDefaultTimeout is the default budget for verifying +// managed Dolt reachability after `gc restart`. The supervisor's own +// post-start tick treats the same health probe as non-fatal (see +// prepareCityForSupervisor → "checking_bead_store_health"), so the +// post-condition lives at the operator command level instead. The +// budget is roughly the upper bound for managed Dolt to come up on a +// developer laptop (cold start + first query); operators on slower +// disks can extend it via the env var below. +const restartDoltHealthDefaultTimeout = 30 * time.Second + +// envRestartDoltHealthTimeout overrides restartDoltHealthDefaultTimeout +// per invocation. Parsed with time.ParseDuration (e.g. "45s", "2m"). +const envRestartDoltHealthTimeout = "GC_RESTART_DOLT_HEALTH_TIMEOUT" + +// restartDoltHealthRetryInterval is the gap between consecutive health +// probes while waiting for managed Dolt to settle. healthBeadsProvider +// already has its own internal recovery path, so a short gap is enough +// to surface a port that came up moments after the start step returned. +const restartDoltHealthRetryInterval = 500 * time.Millisecond + +// verifyDoltHealthyAfterRestartHook is the seam tests use to substitute +// the real probe. Production callers go through cmdRestartJSON, which +// invokes this hook (never the underlying function directly) so tests +// can drive the post-restart code path deterministically without +// spinning a real Dolt process. +var verifyDoltHealthyAfterRestartHook = verifyDoltHealthyAfterRestart + +// verifyDoltHealthyAfterRestart polls healthBeadsProvider until managed +// Dolt is reachable, or until the configured budget expires. The error +// it returns names the cause and the recovery path (`gc start`), which +// the caller writes to stderr verbatim. +// +// No-ops on cities that don't use the bd store contract (file +// providers) and on cities whose Dolt lifecycle is owned by something +// other than gc (postgres backend, external Dolt). Those configurations +// either have no managed process to verify, or the operator manages +// the database lifecycle themselves and a gc-side failure is not an +// honest signal. +func verifyDoltHealthyAfterRestart(cityPath string, stderr io.Writer) error { + if !cityUsesBdStoreContract(cityPath) { + return nil + } + owned, err := managedDoltLifecycleOwned(cityPath) + if err != nil { + return fmt.Errorf("checking managed Dolt ownership: %w", err) + } + if !owned { + return nil + } + + timeout := restartDoltHealthTimeoutFromEnv() + deadline := time.Now().Add(timeout) + var lastErr error + for { + lastErr = healthBeadsProviderHook(cityPath) + if lastErr == nil { + return nil + } + if time.Now().After(deadline) { + break + } + time.Sleep(restartDoltHealthRetryInterval) + } + + cityRef := strings.TrimSpace(cityPath) + if cityRef == "" { + cityRef = "" + } + return fmt.Errorf( + "managed Dolt did not become healthy within %s after restart: %v\n"+ + " The supervisor came back up, but the bead-store backend never reached a queryable state.\n"+ + " Recover with: gc start %s\n"+ + " (Override the budget with %s=, e.g. 45s.)", + timeout, lastErr, cityRef, envRestartDoltHealthTimeout, + ) +} + +// healthBeadsProviderHook is a test seam: production calls flow through +// healthBeadsProvider, but tests substitute a deterministic probe so +// they can exercise the success / timeout branches without a real Dolt. +var healthBeadsProviderHook = healthBeadsProvider + +func restartDoltHealthTimeoutFromEnv() time.Duration { + raw := strings.TrimSpace(os.Getenv(envRestartDoltHealthTimeout)) + if raw == "" { + return restartDoltHealthDefaultTimeout + } + d, err := time.ParseDuration(raw) + if err != nil || d <= 0 { + return restartDoltHealthDefaultTimeout + } + return d +} diff --git a/cmd/gc/restart_dolt_health_test.go b/cmd/gc/restart_dolt_health_test.go new file mode 100644 index 0000000000..f3dec08b98 --- /dev/null +++ b/cmd/gc/restart_dolt_health_test.go @@ -0,0 +1,286 @@ +package main + +import ( + "bytes" + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/gastownhall/gascity/internal/config" +) + +// newManagedBdCity scaffolds a bd-contract city whose Dolt lifecycle +// is gc-owned (the path under test for verifyDoltHealthyAfterRestart's +// happy / failure branches). The on-disk minimum is a city.toml that +// selects the managed bd provider; no rig metadata so +// resolveConfiguredCityDoltTarget returns ok=false (owned by gc). +func newManagedBdCity(t *testing.T) string { + t.Helper() + cityPath := t.TempDir() + if err := os.MkdirAll(filepath.Join(cityPath, ".gc"), 0o755); err != nil { + t.Fatal(err) + } + cityToml := `[workspace] +name = "test-city" + +[beads] +provider = "bd" +` + if err := os.WriteFile(filepath.Join(cityPath, "city.toml"), []byte(cityToml), 0o644); err != nil { + t.Fatal(err) + } + // Clear the in-memory dolt-config registry for this city so prior tests + // can't leak an external-Dolt registration into ours. + clearCityDoltConfig(cityPath) + t.Cleanup(func() { clearCityDoltConfig(cityPath) }) + return cityPath +} + +func TestVerifyDoltHealthyAfterRestart_NoOpForFileProvider(t *testing.T) { + t.Setenv("GC_BEADS", "") + t.Setenv("GC_BEADS_SCOPE_ROOT", "") + + cityPath := t.TempDir() + if err := os.WriteFile(filepath.Join(cityPath, "city.toml"), []byte(`[workspace] +name = "test-city" + +[beads] +provider = "file" +`), 0o644); err != nil { + t.Fatal(err) + } + + called := int32(0) + restoreHook := overrideHealthBeadsProviderHook(func(string) error { + atomic.AddInt32(&called, 1) + return errors.New("should not be called for file provider") + }) + defer restoreHook() + + if err := verifyDoltHealthyAfterRestart(cityPath, io.Discard); err != nil { + t.Fatalf("verifyDoltHealthyAfterRestart() error = %v, want nil for file provider", err) + } + if got := atomic.LoadInt32(&called); got != 0 { + t.Fatalf("healthBeadsProvider called %d times, want 0 (skipped for file provider)", got) + } +} + +func TestVerifyDoltHealthyAfterRestart_NoOpWhenManagedDoltNotOwned(t *testing.T) { + // External-Dolt city: bd-contract, but the operator pinned host/port + // to an external server. resolveConfiguredCityDoltTarget reports the + // endpoint as operator-configured (ok=true), so the lifecycle is not + // owned by gc and the post-restart check has nothing to verify. + cityPath := newManagedBdCity(t) + registerCityDoltConfig(cityPath, config.DoltConfig{ + Host: "db.example.com", + Port: 4406, + }) + t.Cleanup(func() { clearCityDoltConfig(cityPath) }) + + called := int32(0) + restoreHook := overrideHealthBeadsProviderHook(func(string) error { + atomic.AddInt32(&called, 1) + return errors.New("should not be called for external Dolt") + }) + defer restoreHook() + + if err := verifyDoltHealthyAfterRestart(cityPath, io.Discard); err != nil { + t.Fatalf("verifyDoltHealthyAfterRestart() error = %v, want nil for external Dolt", err) + } + if got := atomic.LoadInt32(&called); got != 0 { + t.Fatalf("healthBeadsProvider called %d times, want 0 (skipped when gc does not own Dolt)", got) + } +} + +func TestVerifyDoltHealthyAfterRestart_SuccessOnFirstProbe(t *testing.T) { + cityPath := newManagedBdCity(t) + + restoreHook := overrideHealthBeadsProviderHook(func(string) error { + return nil + }) + defer restoreHook() + + if err := verifyDoltHealthyAfterRestart(cityPath, io.Discard); err != nil { + t.Fatalf("verifyDoltHealthyAfterRestart() error = %v, want nil", err) + } +} + +func TestVerifyDoltHealthyAfterRestart_SuccessAfterRetry(t *testing.T) { + cityPath := newManagedBdCity(t) + + calls := int32(0) + restoreHook := overrideHealthBeadsProviderHook(func(string) error { + n := atomic.AddInt32(&calls, 1) + if n < 3 { + return errors.New("dial tcp 127.0.0.1:0: connect: can't assign requested address") + } + return nil + }) + defer restoreHook() + // Give the loop room to retry. + t.Setenv(envRestartDoltHealthTimeout, "5s") + + if err := verifyDoltHealthyAfterRestart(cityPath, io.Discard); err != nil { + t.Fatalf("verifyDoltHealthyAfterRestart() error = %v, want nil after retry", err) + } + if got := atomic.LoadInt32(&calls); got < 3 { + t.Fatalf("healthBeadsProvider called %d times, want >= 3 (retried until healthy)", got) + } +} + +// TestVerifyDoltHealthyAfterRestart_TimeoutReturnsLoudError is the +// regression-test the bead asks for: simulate a `gc restart` whose +// Dolt is in a sleeping/closed state (healthBeadsProvider keeps +// returning unreachable), and assert the command exits with a clear +// failure that names the cause. +func TestVerifyDoltHealthyAfterRestart_TimeoutReturnsLoudError(t *testing.T) { + cityPath := newManagedBdCity(t) + + probeErr := errors.New("dial tcp 127.0.0.1:0: connect: can't assign requested address") + restoreHook := overrideHealthBeadsProviderHook(func(string) error { + return probeErr + }) + defer restoreHook() + // Short timeout keeps the test fast. + t.Setenv(envRestartDoltHealthTimeout, "100ms") + + start := time.Now() + err := verifyDoltHealthyAfterRestart(cityPath, io.Discard) + elapsed := time.Since(start) + + if err == nil { + t.Fatalf("verifyDoltHealthyAfterRestart() error = nil, want non-nil for persistent unreachability") + } + msg := err.Error() + wantSubstrings := []string{ + "managed Dolt did not become healthy", + "100ms", + probeErr.Error(), + "gc start", + envRestartDoltHealthTimeout, + } + for _, want := range wantSubstrings { + if !strings.Contains(msg, want) { + t.Errorf("error message missing %q\nfull message: %s", want, msg) + } + } + // The loop must respect the deadline; allow some slack but reject + // runaway loops. + if elapsed > 2*time.Second { + t.Errorf("verifyDoltHealthyAfterRestart took %s, want under 2s with 100ms budget", elapsed) + } +} + +func TestRestartDoltHealthTimeoutFromEnv(t *testing.T) { + cases := []struct { + name string + env string + want time.Duration + }{ + {"unset uses default", "", restartDoltHealthDefaultTimeout}, + {"valid duration", "45s", 45 * time.Second}, + {"valid minute duration", "2m", 2 * time.Minute}, + {"invalid syntax falls back to default", "garbage", restartDoltHealthDefaultTimeout}, + {"zero falls back to default", "0s", restartDoltHealthDefaultTimeout}, + {"negative falls back to default", "-5s", restartDoltHealthDefaultTimeout}, + {"whitespace-only falls back to default", " ", restartDoltHealthDefaultTimeout}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(envRestartDoltHealthTimeout, tc.env) + got := restartDoltHealthTimeoutFromEnv() + if got != tc.want { + t.Fatalf("restartDoltHealthTimeoutFromEnv() = %s, want %s", got, tc.want) + } + }) + } +} + +// TestCmdRestartJSON_HealthcheckFailureExitsNonZero is the integration +// view of the same scenario: cmdRestartJSON itself returns 1 and writes +// the healthcheck error to stderr when the post-restart probe times out, +// even though the stop+start steps succeeded. +func TestCmdRestartJSON_HealthcheckFailureExitsNonZero(t *testing.T) { + cityPath := newManagedBdCity(t) + + // Replace the healthcheck hook with a deterministic failure so we don't + // need a real Dolt process to drive the failure branch. + probeErr := errors.New("dial tcp 127.0.0.1:0: connect: can't assign requested address") + restoreVerifyHook := overrideVerifyDoltHealthyAfterRestartHook(func(_ string, _ io.Writer) error { + return fmt.Errorf("managed Dolt did not become healthy within 100ms after restart: %v\n Recover with: gc start %s", probeErr, cityPath) + }) + defer restoreVerifyHook() + + // Skip the stop+start network/supervisor work by stubbing them out. + restoreStop, restoreStart := stubRestartLifecycleForTest(t) + defer restoreStop() + defer restoreStart() + + // Make registration-name resolution work without a real supervisor. + restoreName := overrideRestartRegistrationNameHook(func([]string) (string, error) { + return "test-city", nil + }) + defer restoreName() + + var stdout, stderr bytes.Buffer + code := cmdRestartJSON([]string{cityPath}, &stdout, &stderr, false /*jsonOut*/) + + if code == 0 { + t.Fatalf("cmdRestartJSON returned 0, want non-zero on healthcheck failure\nstdout: %s\nstderr: %s", stdout.String(), stderr.String()) + } + if !strings.Contains(stderr.String(), "gc restart:") { + t.Errorf("stderr missing 'gc restart:' prefix, got: %s", stderr.String()) + } + if !strings.Contains(stderr.String(), "managed Dolt did not become healthy") { + t.Errorf("stderr missing healthcheck-failure body, got: %s", stderr.String()) + } +} + +// overrideHealthBeadsProviderHook swaps the test seam atomically. +func overrideHealthBeadsProviderHook(fn func(string) error) func() { + prev := healthBeadsProviderHook + healthBeadsProviderHook = fn + return func() { healthBeadsProviderHook = prev } +} + +// overrideVerifyDoltHealthyAfterRestartHook swaps the seam used by +// cmdRestartJSON. Tests that drive the integration path use this so +// they can simulate timeouts without spinning a real Dolt. +func overrideVerifyDoltHealthyAfterRestartHook(fn func(string, io.Writer) error) func() { + prev := verifyDoltHealthyAfterRestartHook + verifyDoltHealthyAfterRestartHook = fn + return func() { verifyDoltHealthyAfterRestartHook = prev } +} + +// overrideRestartRegistrationNameHook substitutes the city-name +// resolver in cmdRestartJSON so the test does not need a real +// supervisor registry. +func overrideRestartRegistrationNameHook(fn func([]string) (string, error)) func() { + prev := restartRegistrationNameHook + restartRegistrationNameHook = fn + return func() { restartRegistrationNameHook = prev } +} + +// stubRestartLifecycleForTest replaces the stop+start steps in +// cmdRestartJSON with no-ops that report success, so tests can drive +// the post-start healthcheck branch deterministically. Returns two +// restore funcs (one per hook) for symmetric defer cleanup. +func stubRestartLifecycleForTest(t *testing.T) (func(), func()) { + t.Helper() + prevStop := restartCmdStopHook + restartCmdStopHook = func(_ []string, _ io.Writer, _ io.Writer, _ time.Duration, _ bool) int { + return 0 + } + prevStart := restartDoStartWithNameOverrideHook + restartDoStartWithNameOverrideHook = func(_ []string, _ bool, _ io.Writer, _ io.Writer, _ string) int { + return 0 + } + return func() { restartCmdStopHook = prevStop }, + func() { restartDoStartWithNameOverrideHook = prevStart } +} From ac9a9b6b0788e16c5def5d506f5f403616aad649 Mon Sep 17 00:00:00 2001 From: Austin Born Date: Sat, 23 May 2026 10:59:09 -0700 Subject: [PATCH 2/2] =?UTF-8?q?restart:=20lint=20fixes=20=E2=80=94=20%w=20?= =?UTF-8?q?for=20error=20wrap,=20use=20stderr=20for=20progress=20line?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two golangci-lint findings on the previous commit: - restart_dolt_health.go: stderr parameter was unused (revive unused-parameter). Use it for a one-line "Verifying managed Dolt is healthy (budget %s)..." message on entry so operators watching `gc restart` see progress before the full budget elapses. - restart_dolt_health_test.go: %v in fmt.Errorf where the formatted value is an error (errorlint non-wrapping). Switch to %w; the produced message string is unchanged. The production verifyDoltHealthyAfterRestart timeout error already uses %w for the probe error in the same commit's change. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-1 On behalf of: @austinborn Co-Authored-By: .invalid> --- cmd/gc/restart_dolt_health.go | 9 +++++++-- cmd/gc/restart_dolt_health_test.go | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/cmd/gc/restart_dolt_health.go b/cmd/gc/restart_dolt_health.go index 3cf0fa25ce..7467cbcbe9 100644 --- a/cmd/gc/restart_dolt_health.go +++ b/cmd/gc/restart_dolt_health.go @@ -38,7 +38,9 @@ var verifyDoltHealthyAfterRestartHook = verifyDoltHealthyAfterRestart // verifyDoltHealthyAfterRestart polls healthBeadsProvider until managed // Dolt is reachable, or until the configured budget expires. The error // it returns names the cause and the recovery path (`gc start`), which -// the caller writes to stderr verbatim. +// the caller writes to stderr verbatim. A single one-line "verifying" +// message is written to stderr on entry so operators watching +// `gc restart` see progress before the full budget elapses. // // No-ops on cities that don't use the bd store contract (file // providers) and on cities whose Dolt lifecycle is owned by something @@ -59,6 +61,9 @@ func verifyDoltHealthyAfterRestart(cityPath string, stderr io.Writer) error { } timeout := restartDoltHealthTimeoutFromEnv() + if stderr != nil { + fmt.Fprintf(stderr, "Verifying managed Dolt is healthy (budget %s)...\n", timeout) //nolint:errcheck // best-effort progress message + } deadline := time.Now().Add(timeout) var lastErr error for { @@ -77,7 +82,7 @@ func verifyDoltHealthyAfterRestart(cityPath string, stderr io.Writer) error { cityRef = "" } return fmt.Errorf( - "managed Dolt did not become healthy within %s after restart: %v\n"+ + "managed Dolt did not become healthy within %s after restart: %w\n"+ " The supervisor came back up, but the bead-store backend never reached a queryable state.\n"+ " Recover with: gc start %s\n"+ " (Override the budget with %s=, e.g. 45s.)", diff --git a/cmd/gc/restart_dolt_health_test.go b/cmd/gc/restart_dolt_health_test.go index f3dec08b98..f0c4aff309 100644 --- a/cmd/gc/restart_dolt_health_test.go +++ b/cmd/gc/restart_dolt_health_test.go @@ -213,7 +213,7 @@ func TestCmdRestartJSON_HealthcheckFailureExitsNonZero(t *testing.T) { // need a real Dolt process to drive the failure branch. probeErr := errors.New("dial tcp 127.0.0.1:0: connect: can't assign requested address") restoreVerifyHook := overrideVerifyDoltHealthyAfterRestartHook(func(_ string, _ io.Writer) error { - return fmt.Errorf("managed Dolt did not become healthy within 100ms after restart: %v\n Recover with: gc start %s", probeErr, cityPath) + return fmt.Errorf("managed Dolt did not become healthy within 100ms after restart: %w\n Recover with: gc start %s", probeErr, cityPath) }) defer restoreVerifyHook()