diff --git a/cmd/bughunt_p2_test.go b/cmd/bughunt_p2_test.go index e2ab478..cb7be9f 100644 --- a/cmd/bughunt_p2_test.go +++ b/cmd/bughunt_p2_test.go @@ -21,6 +21,7 @@ package cmd import ( "encoding/json" "net/http" + "net/http/httptest" "strings" "testing" "time" @@ -141,6 +142,44 @@ func TestBugHunt_T16_P2_2_ProvisionTimeoutAtLeast60s(t *testing.T) { } } +// TestProvisionTimeout_SurfacesOrphanGuidance is the BUG 2 durability +// regression: a provision that hits the client/context deadline must NOT exit +// with a bare "context deadline exceeded" — the resource may have already +// landed server-side (provisioning is synchronous on the api), so the user +// needs an actionable next step to find + clean up the potential orphan. +// +// We point a short-timeout client at a server that never responds, forcing a +// real client-timeout (*url.Error with Timeout()==true) through +// provisionResource, and assert the named-const guidance is surfaced. +func TestProvisionTimeout_SurfacesOrphanGuidance(t *testing.T) { + block := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + <-block // hang until the client times out + })) + defer srv.Close() + defer close(block) + + prevURL, prevClient := APIBaseURL, HTTPClient + APIBaseURL = srv.URL + HTTPClient = &http.Client{Timeout: 50 * time.Millisecond} + t.Cleanup(func() { APIBaseURL, HTTPClient = prevURL, prevClient }) + + _, err := provisionResource("/db/new", "orphan-on-timeout", "") + if err == nil { + t.Fatal("provision against a hanging server must error (client timeout)") + } + msg := err.Error() + if !strings.Contains(msg, provisionTimeoutGuidance) { + t.Errorf("timeout error must surface orphan guidance %q; got %q", provisionTimeoutGuidance, msg) + } + // The guidance must name the recovery commands so an agent/user can act. + for _, want := range []string{"instant resources", "instant resource delete"} { + if !strings.Contains(msg, want) { + t.Errorf("timeout guidance must mention %q; got %q", want, msg) + } + } +} + // ── T16 P2-3 — `up` env default is "development", not "production". // TestBugHunt_T16_P2_3_UpDefaultsToDevelopmentNotProd asserts an `up` run with diff --git a/cmd/extras.go b/cmd/extras.go index 9683589..7144ffd 100644 --- a/cmd/extras.go +++ b/cmd/extras.go @@ -141,15 +141,23 @@ func runResourceDetail(cmd *cobra.Command, token string) error { return parseAPIError(resp.StatusCode, raw) } - // The API may return the bare resource object OR an envelope: - // {ok:true, resource:{...}}. Accept either shape. + // The API may return the bare resource object OR an envelope. Prod + // GET /api/v1/resources/:token wraps the object under "item" + // ({"item":{...},"ok":true}); older/alternate shapes use "resource". + // Accept "item" first (today's prod), then "resource" (back-compat), + // then fall back to the bare object — so a future un-enveloped response + // still renders. var envelope struct { OK bool `json:"ok"` + Item json.RawMessage `json:"item"` Resource json.RawMessage `json:"resource"` } _ = json.Unmarshal(raw, &envelope) body := raw - if len(envelope.Resource) > 0 { + switch { + case len(envelope.Item) > 0: + body = envelope.Item + case len(envelope.Resource) > 0: body = envelope.Resource } diff --git a/cmd/extras_test.go b/cmd/extras_test.go index c3b3c66..5741056 100644 --- a/cmd/extras_test.go +++ b/cmd/extras_test.go @@ -131,6 +131,58 @@ func TestRunResourceDetail_Success_JSON(t *testing.T) { } } +// TestRunResourceDetail_ItemEnvelopeRenders is the BUG 1 regression: prod +// GET /api/v1/resources/:token wraps the object under "item" +// ({"item":{...},"ok":true}). Before the fix the detail decoder only knew +// the bare-object and "resource"-envelope shapes, so it rendered an all-empty +// object with exit 0 (silent). This asserts the "item" key is unwrapped and +// the fields actually render to stdout — mirroring how discover.go's list +// path keys off "items". +func TestRunResourceDetail_ItemEnvelopeRenders(t *testing.T) { + withCleanState(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"ok":true,"item":{ + "token":"tok-item","id":"id-item","resource_type":"postgres","name":"app-item", + "env":"production","tier":"pro","status":"active", + "connection_url":"postgres://u:p@x/db" + }}`)) + })) + defer srv.Close() + prev := APIBaseURL + APIBaseURL = srv.URL + t.Cleanup(func() { APIBaseURL = prev }) + + // runResourceDetail short-circuits with errAuthRequired unless haveAuth() + // is true — wire an authTransport so the request actually reaches the mock. + prevClient := HTTPClient + HTTPClient = &http.Client{Transport: &authTransport{base: http.DefaultTransport, apiKey: "x"}} + t.Cleanup(func() { HTTPClient = prevClient }) + + stdout, _ := captureStdout(t, func() { + if err := runResourceDetail(nil, "tok-item"); err != nil { + t.Fatalf("runResourceDetail (item envelope): %v", err) + } + }) + + // The fields nested under "item" must render — an all-empty object is the + // pre-fix bug. + for _, want := range []string{ + "tok-item", // TOKEN + "id-item", // ID + "postgres", // TYPE + "app-item", // NAME + "production", // ENV + "pro", // TIER + "active", // STATUS + "postgres://u:p@x/db", // URL + } { + if !strings.Contains(stdout, want) { + t.Errorf("item-envelope detail must render %q; stdout=%q", want, stdout) + } + } +} + func TestRunResourceDelete_EmptyToken(t *testing.T) { err := runResourceDelete(nil, "") if err == nil || !strings.Contains(err.Error(), "token is required") { diff --git a/cmd/monitor.go b/cmd/monitor.go index 3099eb8..9207641 100644 --- a/cmd/monitor.go +++ b/cmd/monitor.go @@ -2,9 +2,12 @@ package cmd import ( "bytes" + "context" "encoding/json" + "errors" "fmt" "io" + "net" "net/http" "os" "regexp" @@ -67,20 +70,22 @@ func validateResourceName(name string) error { // storage / webhook / vector) MUST reject unknown sub-sub-commands with a // non-zero exit. The previous behaviour was: // -// instant db delete → prints help, exits 0 +// instant db delete → prints help, exits 0 // // which silently hid typo bugs in agent scripts and let `... | xargs instant` // pipelines look successful. The pattern below combines: // -// 1. Args: cobra.NoArgs — refuses any positional arg -// 2. RunE: showGroupHelp — when called with zero args, shows -// help and exits 0 (the legacy path) -// 3. cobra's built-in "did you mean?" suggestions surface for typos that -// are within 2 edits of a valid subcommand (cobra default). +// 1. Args: cobra.NoArgs — refuses any positional arg +// 2. RunE: showGroupHelp — when called with zero args, shows +// help and exits 0 (the legacy path) +// 3. cobra's built-in "did you mean?" suggestions surface for typos that +// are within 2 edits of a valid subcommand (cobra default). // // Together, `instant db delete ` now errors with: -// Error: unknown command "delete" for "instant db" -// Run 'instant db --help' for usage. +// +// Error: unknown command "delete" for "instant db" +// Run 'instant db --help' for usage. +// // and exits 1. func showGroupHelp(cmd *cobra.Command, args []string) error { return cmd.Help() @@ -248,6 +253,13 @@ func provisionResource(endpoint, name, env string) (*provisionResponse, error) { resp, err := HTTPClient.Post(url, "application/json", bytes.NewReader(body)) if err != nil { + // A provision that hit the client/context deadline may have ALREADY + // landed server-side (provisioning is synchronous on the api) — exit 1 + // alone hides a possible orphan. Surface actionable durability guidance + // while preserving the underlying cause for %w-aware callers. + if isTimeoutErr(err) { + return nil, fmt.Errorf("%s (%w)", provisionTimeoutGuidance, err) + } return nil, err } defer func() { _ = resp.Body.Close() }() @@ -273,6 +285,27 @@ func provisionResource(endpoint, name, env string) (*provisionResponse, error) { return &result, nil } +// isTimeoutErr reports whether err is a request timeout — either the +// http.Client.Timeout firing (surfaces as a net.Error with Timeout()==true, +// wrapped in a *url.Error) or a context deadline being exceeded. Both mean the +// provision request was abandoned client-side, so the resource may still be +// landing server-side. Kept separate from json_error.go's network classifier +// because that path emits a generic "network_error"; here we want the orphan +// durability hint specifically on the timeout case. +func isTimeoutErr(err error) bool { + if err == nil { + return false + } + if errors.Is(err, context.DeadlineExceeded) { + return true + } + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return true + } + return false +} + // ── status command ──────────────────────────────────────────────────────────── // statusJSON is the --json flag for `instant status`. T16 P3: machine-readable diff --git a/cmd/monitor_test.go b/cmd/monitor_test.go index bee4837..7263dde 100644 --- a/cmd/monitor_test.go +++ b/cmd/monitor_test.go @@ -6,6 +6,10 @@ package cmd import ( "bytes" + "context" + "errors" + "fmt" + "net" "net/http" "net/http/httptest" "strings" @@ -140,3 +144,47 @@ func withTestAPI(t *testing.T, baseURL string) { HTTPClient = prevClient }) } + +// ── isTimeoutErr unit coverage ──────────────────────────────────────────────── + +// fakeNetTimeoutErrMsg is the message carried by fakeNetTimeoutErr; named so +// tests don't scatter string literals. +const fakeNetTimeoutErrMsg = "dial tcp: i/o timeout (synthetic)" + +// fakeNetTimeoutErr is a minimal net.Error with a configurable Timeout(). +// It deliberately does NOT wrap context.DeadlineExceeded, so it exercises the +// errors.As(net.Error) branch of isTimeoutErr rather than the errors.Is one +// (an http.Client.Timeout error matches DeadlineExceeded first, leaving the +// net.Error branch unreachable through provisionResource alone). +type fakeNetTimeoutErr struct{ timeout bool } + +func (e fakeNetTimeoutErr) Error() string { return fakeNetTimeoutErrMsg } +func (e fakeNetTimeoutErr) Timeout() bool { return e.timeout } +func (e fakeNetTimeoutErr) Temporary() bool { return e.timeout } + +// TestIsTimeoutErr pins every branch of isTimeoutErr: the nil guard, the +// context.DeadlineExceeded path, the net.Error-with-Timeout() path (including +// when wrapped, as *url.Error does), and the non-timeout fallthrough. +func TestIsTimeoutErr(t *testing.T) { + // Compile-time proof the fake satisfies net.Error. + var _ net.Error = fakeNetTimeoutErr{} + + cases := []struct { + name string + err error + want bool + }{ + {"nil error is not a timeout", nil, false}, + {"context deadline exceeded", context.DeadlineExceeded, true}, + {"wrapped context deadline exceeded", fmt.Errorf("post: %w", context.DeadlineExceeded), true}, + {"net.Error with Timeout()==true", fakeNetTimeoutErr{timeout: true}, true}, + {"wrapped net.Error with Timeout()==true", fmt.Errorf("post: %w", fakeNetTimeoutErr{timeout: true}), true}, + {"net.Error with Timeout()==false", fakeNetTimeoutErr{timeout: false}, false}, + {"plain non-timeout error", errors.New("connection refused"), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, isTimeoutErr(tc.err)) + }) + } +} diff --git a/cmd/root.go b/cmd/root.go index b2d7ae3..106dae8 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -9,9 +9,9 @@ import ( "strings" "time" - "github.com/spf13/cobra" "github.com/InstaNode-dev/cli/internal/cliconfig" "github.com/InstaNode-dev/cli/internal/secretstore" + "github.com/spf13/cobra" ) var _ = httpListTimeout // documented constant; referenced in tests / future refactor @@ -64,6 +64,15 @@ const httpListTimeout = 10 * time.Second // CLI simplicity; <=0 falls back to the default). const httpProvisionTimeout = 60 * time.Second +// provisionTimeoutGuidance is appended to a provision error when the request +// hit the client/context deadline. Provisioning is synchronous server-side, so +// a timeout does NOT mean the resource was not created — it may have landed and +// become an orphan the user can't see. Surface an actionable next step rather +// than a bare "context deadline exceeded". Named const (not an inline literal) +// so the message is greppable + asserted by the timeout regression test. +const provisionTimeoutGuidance = "Request timed out, but the resource may still be provisioning — " + + "run `instant resources` to check (and `instant resource delete --yes` if it's an orphan)." + // HTTPClient is the shared HTTP client used by all subcommands. // It is configured with the auth transport during init. // @@ -348,4 +357,3 @@ func initConfig() { }, } } -