diff --git a/CHANGELOG.md b/CHANGELOG.md index d03d8bc..0dfac0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ separately by `model.SchemaVersion` (currently 1.2.0). ## [Unreleased] +### Fixed +- **`pgbot tune --timeout`** (#26, #30, contributed by @YIKUAIBANZI). `tune` ran + under a fixed 30s budget with no flag to raise it, so a slow or remote database + died with `collect: context deadline exceeded`; it now takes the same + `--timeout` (default 30s) as the other collection commands. The shared + `gather` path also forwards that budget to the collector, which previously + kept its own 20s+interval cap regardless — so `--timeout` above ~21s on + `indexes`, `queries`, `tables`, and `vacuum` now actually extends the run. + ## [0.7.2] - 2026-09-01 ### Added diff --git a/cmd/pgbot/gather.go b/cmd/pgbot/gather.go index 8026a15..0ecc69e 100644 --- a/cmd/pgbot/gather.go +++ b/cmd/pgbot/gather.go @@ -13,8 +13,10 @@ import ( // gather runs the full read-only collection and returns a computed Context plus // the target host (for the header). It closes the connection before returning — -// findings and rendering need no live connection. Shared by `ask` and `indexes`; -// `inspect`/`explain` keep their own flow because they also render/exit. +// findings and rendering need no live connection. Shared by every command that +// only needs a Context (`ask`, `indexes`, `queries`, `tables`, `vacuum`, `tune`, +// `report`, `config explain`, the MCP tools); `inspect`/`explain` keep their own +// flow because they also render/exit. func gather(ctx context.Context, connString string, f inspectFlags) (*model.Context, string, error) { target, err := conn.Connect(ctx, connString) if err != nil { @@ -30,7 +32,7 @@ func gather(ctx context.Context, connString string, f inspectFlags) (*model.Cont fmt.Fprintln(os.Stderr, target.Pooler.Note()) } - c, err := collect.Run(ctx, target, collect.Options{Interval: f.interval, ASHHz: f.ashHz, ASHWindow: f.window}) + c, err := collect.Run(ctx, target, gatherOptions(f)) if err != nil { return nil, "", fmt.Errorf("collect: %s", conn.RedactConnString(err.Error())) } @@ -46,3 +48,10 @@ func gather(ctx context.Context, connString string, f inspectFlags) (*model.Cont } return c, host, nil } + +// gatherOptions maps the command flags onto the collector. Deadline must be +// forwarded: when it is zero the collector applies its own 20s+interval cap, which +// silently overrides any larger --timeout the command was given. +func gatherOptions(f inspectFlags) collect.Options { + return collect.Options{Interval: f.interval, ASHHz: f.ashHz, ASHWindow: f.window, Deadline: f.timeout} +} diff --git a/cmd/pgbot/gather_test.go b/cmd/pgbot/gather_test.go new file mode 100644 index 0000000..8ccedc3 --- /dev/null +++ b/cmd/pgbot/gather_test.go @@ -0,0 +1,39 @@ +package main + +import ( + "testing" + "time" + + "github.com/pgrundev/pgbot/internal/collect" +) + +// gatherOptions replaced an inline collect.Options literal. A forwarding helper +// that silently drops a field (interval, ASH rate, window) would still compile +// and still pass a deadline-only check, so pin the whole mapping. +func TestGatherOptionsForwardsEveryCollectionFlag(t *testing.T) { + f := inspectFlags{ + interval: 750 * time.Millisecond, + ashHz: 25, + window: 7 * time.Second, + timeout: 90 * time.Second, + } + want := collect.Options{ + Interval: 750 * time.Millisecond, + ASHHz: 25, + ASHWindow: 7 * time.Second, + Deadline: 90 * time.Second, + } + if got := gatherOptions(f); got != want { + t.Fatalf("gatherOptions(%+v) = %+v; want %+v", f, got, want) + } +} + +// Callers that never set a --timeout (ask, config explain, the MCP tools) pass a +// zero inspectFlags.timeout; that must reach the collector as a zero Deadline so +// its own fallback applies, rather than being replaced by some other default here. +func TestGatherOptionsZeroTimeoutLeavesCollectorFallback(t *testing.T) { + got := gatherOptions(inspectFlags{interval: time.Second}) + if got.Deadline != 0 { + t.Fatalf("gatherOptions with no timeout set Deadline = %s; want 0 (collector default)", got.Deadline) + } +} diff --git a/cmd/pgbot/tune.go b/cmd/pgbot/tune.go index c348dc0..10f40e0 100644 --- a/cmd/pgbot/tune.go +++ b/cmd/pgbot/tune.go @@ -29,6 +29,7 @@ func newTuneCmd() *cobra.Command { fl := cmd.Flags() fl.BoolVar(&f.noColor, "no-color", false, "disable ANSI color") fl.DurationVar(&f.interval, "interval", time.Second, "gap between the two counter samples (min 500ms)") + fl.DurationVar(&f.timeout, "timeout", 30*time.Second, "total wall-clock budget for the run (raise it for slow or remote databases)") return cmd } @@ -40,7 +41,7 @@ func runTune(cmd *cobra.Command, args []string, f inspectFlags) error { f.ashHz = 0 // no wait sampling needed for tuning f.noStore = true - ctx, cancel := context.WithTimeout(cmd.Context(), 30*time.Second) + ctx, cancel := context.WithTimeout(cmd.Context(), f.timeout) defer cancel() c, host, err := gather(ctx, connString, f) diff --git a/cmd/pgbot/tune_test.go b/cmd/pgbot/tune_test.go new file mode 100644 index 0000000..eb77ac2 --- /dev/null +++ b/cmd/pgbot/tune_test.go @@ -0,0 +1,101 @@ +package main + +import ( + "context" + "fmt" + "net" + "strings" + "testing" + "time" + + "github.com/spf13/cobra" +) + +func TestTuneTimeoutBoundsRun(t *testing.T) { + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + done := make(chan struct{}) + go func() { + conn, acceptErr := listener.Accept() + if acceptErr != nil { + return + } + defer conn.Close() + <-done + }() + t.Cleanup(func() { + close(done) + _ = listener.Close() + }) + + dsn := fmt.Sprintf("postgres://pgbot:secret@%s/postgres?sslmode=disable", listener.Addr()) + cmd := newTuneCmd() + cmd.SilenceErrors = true + cmd.SilenceUsage = true + cmd.SetArgs([]string{dsn, "--timeout=50ms"}) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + started := time.Now() + err = cmd.ExecuteContext(ctx) + elapsed := time.Since(started) + + if err == nil || !strings.Contains(err.Error(), context.DeadlineExceeded.Error()) { + t.Fatalf("tune --timeout error = %v; want context deadline exceeded", err) + } + if elapsed > 500*time.Millisecond { + t.Fatalf("tune --timeout=50ms returned after %s; want it to bound the run", elapsed) + } +} + +func TestGatherOptionsForwardsTimeout(t *testing.T) { + want := 2 * time.Minute + got := gatherOptions(inspectFlags{timeout: want}) + + if got.Deadline != want { + t.Fatalf("gather collection deadline = %s; want %s", got.Deadline, want) + } +} + +// Every collection command built on inspectFlags exposes --timeout with the same +// 30s default, so `--timeout 60s` from the docs works uniformly. tune was the +// one that lacked it (#26); keep the set in step. +func TestCollectionCommandsShareTimeoutDefault(t *testing.T) { + cmds := map[string]func() *cobra.Command{ + "tune": newTuneCmd, + "indexes": newIndexesCmd, + "queries": newQueriesCmd, + "tables": newTablesCmd, + "vacuum": newVacuumCmd, + "inspect": newInspectCmd, + } + for name, build := range cmds { + fl := build().Flags().Lookup("timeout") + if fl == nil { + t.Errorf("%s: no --timeout flag", name) + continue + } + if fl.DefValue != "30s" { + t.Errorf("%s: --timeout default = %q; want \"30s\"", name, fl.DefValue) + } + } +} + +func TestTuneTimeoutFlagParsesDurations(t *testing.T) { + cmd := newTuneCmd() + if err := cmd.Flags().Parse([]string{"--timeout=2m30s"}); err != nil { + t.Fatal(err) + } + got, err := cmd.Flags().GetDuration("timeout") + if err != nil { + t.Fatal(err) + } + if want := 150 * time.Second; got != want { + t.Fatalf("--timeout=2m30s parsed as %s; want %s", got, want) + } + if err := newTuneCmd().Flags().Parse([]string{"--timeout=soon"}); err == nil { + t.Fatal("--timeout=soon parsed without error; want a duration syntax error") + } +} diff --git a/internal/collect/collector.go b/internal/collect/collector.go index 3de646a..44ea663 100644 --- a/internal/collect/collector.go +++ b/internal/collect/collector.go @@ -24,7 +24,7 @@ const ( // Options tune a collection run. type Options struct { Interval time.Duration // gap between the two counter samples (default 1s, min 500ms) - Deadline time.Duration // hard cap on total wall time (default 5s + interval) + Deadline time.Duration // hard cap on total wall time (default 20s + interval; the --timeout flag) RawQueryText bool // keep raw pg_stat_activity query text (default: scrub — PII) ASHHz int // wait-event poll rate in Hz (default 10; 0 disables the sampler) ASHWindow time.Duration // active-session sampling window (default 5s)