From 4a7f9789f5efa51d56eb905c70b796baf5090bd3 Mon Sep 17 00:00:00 2001 From: winsznx Date: Wed, 12 Aug 2026 09:02:30 +0100 Subject: [PATCH 1/2] fix(execute): reconcile writes that report completed without a transaction hash A successful POST /api/execute/contract-call can return 202 with status "completed" and no transactionHash. The hash only appears on the status endpoint. Both transfer and contract-call treated any "completed" as terminal, so --wait printed a success and returned while the caller still had no transaction to verify, which is the one outcome that flag exists to prevent. --wait now reconciles against the status endpoint when a completed write carries no hash, and returns immediately when it does, so the common case costs no extra request. Also honours X-Poll-Interval-Hint. fetchExecStatus previously discarded the response and both poll loops ran on a hardcoded two second ticker, so the documented pacing header had no effect. The hint is now used, a hint of 0 is treated as terminal, and an absent or unparseable hint falls back to the previous two second default. Polling is still bounded by the caller's --timeout. unconfirmed is unchanged: it is absent from execTerminalStatuses and so remains non-terminal, which is already correct. Tests: six cases for the reconciliation predicate, six for hint parsing, and three behavioural tests driving the commands against an httptest server. All three behavioural tests were confirmed to fail against the previous behaviour, one of them catching the old 2s timer at 1.99s. --- cmd/execute/contract_call.go | 2 +- cmd/execute/pollhint_internal_test.go | 68 ++++++++++++++ cmd/execute/reconcile_test.go | 126 ++++++++++++++++++++++++++ cmd/execute/status.go | 60 ++++++++---- cmd/execute/transfer.go | 75 ++++++++------- 5 files changed, 281 insertions(+), 50 deletions(-) create mode 100644 cmd/execute/pollhint_internal_test.go create mode 100644 cmd/execute/reconcile_test.go diff --git a/cmd/execute/contract_call.go b/cmd/execute/contract_call.go index 6ea1f54..48821b2 100644 --- a/cmd/execute/contract_call.go +++ b/cmd/execute/contract_call.go @@ -124,7 +124,7 @@ func NewContractCallCmd(f *cmdutil.Factory) *cobra.Command { }) } - if execTerminalStatuses[writeResp.Status] { + if execTerminalStatuses[writeResp.Status] && !writeNeedsReconciliation(writeResp.Status, writeResp.TransactionHash) { return printContractCallWriteResult(p, &writeResp) } diff --git a/cmd/execute/pollhint_internal_test.go b/cmd/execute/pollhint_internal_test.go new file mode 100644 index 0000000..2b07c2f --- /dev/null +++ b/cmd/execute/pollhint_internal_test.go @@ -0,0 +1,68 @@ +package execute + +import ( + "net/http" + "testing" + "time" +) + +// A successful /api/execute/contract-call broadcast can return 202 with status "completed" and +// no transactionHash; the hash only appears on the status endpoint. Treating that as final means +// --wait returns success while the caller still has no transaction to verify. +func TestWriteNeedsReconciliation(t *testing.T) { + empty := "" + hash := "0xabc" + + cases := []struct { + name string + status string + tx *string + want bool + }{ + {"completed without a hash must be reconciled", "completed", nil, true}, + {"completed with an empty hash must be reconciled", "completed", &empty, true}, + {"completed with a hash is final", "completed", &hash, false}, + {"failed is final regardless of the hash", "failed", nil, false}, + {"running is not terminal anyway", "running", nil, false}, + {"unconfirmed is not terminal anyway", "unconfirmed", nil, false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := writeNeedsReconciliation(tc.status, tc.tx); got != tc.want { + t.Errorf("writeNeedsReconciliation(%q, %v) = %v, want %v", tc.status, tc.tx, got, tc.want) + } + }) + } +} + +func TestNextPollDelay(t *testing.T) { + cases := []struct { + name string + header string + setHeader bool + wantDelay time.Duration + wantTerminal bool + }{ + {"no header falls back to the default", "", false, defaultPollInterval, false}, + {"a hint is honoured", "5", true, 5 * time.Second, false}, + {"a hint of zero means terminal", "0", true, 0, true}, + {"surrounding whitespace is tolerated", " 3 ", true, 3 * time.Second, false}, + {"an unparseable hint falls back rather than failing", "soon", true, defaultPollInterval, false}, + {"a negative hint falls back", "-1", true, defaultPollInterval, false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resp := &http.Response{Header: http.Header{}} + if tc.setHeader { + resp.Header.Set("X-Poll-Interval-Hint", tc.header) + } + delay, terminal := nextPollDelay(resp) + if delay != tc.wantDelay || terminal != tc.wantTerminal { + t.Errorf("nextPollDelay(%q) = (%v, %v), want (%v, %v)", + tc.header, delay, terminal, tc.wantDelay, tc.wantTerminal) + } + }) + } +} diff --git a/cmd/execute/reconcile_test.go b/cmd/execute/reconcile_test.go new file mode 100644 index 0000000..8fe8478 --- /dev/null +++ b/cmd/execute/reconcile_test.go @@ -0,0 +1,126 @@ +package execute_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/keeperhub/cli/cmd/execute" + "github.com/keeperhub/cli/pkg/iostreams" +) + +// The regression this addresses. A write reports completed with no transaction hash, which the +// live API does for /api/execute/contract-call, so --wait must reconcile against the status +// endpoint instead of returning a success the caller cannot verify. +func TestTransferCmd_WaitReconcilesCompletedWithoutHash(t *testing.T) { + statusCalls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.HasSuffix(r.URL.Path, "/status") { + statusCalls++ + w.Header().Set("X-Poll-Interval-Hint", "0") + _, _ = w.Write([]byte(`{"executionId":"exec-1","status":"completed","transactionHash":"0xdeadbeef"}`)) + return + } + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-1","status":"completed"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + cmd := execute.NewTransferCmd(newTransferFactory(ios, srv)) + cmd.SetArgs([]string{"--chain", "84532", "--to", "0xabc", "--amount", "0.1", "--wait"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if statusCalls == 0 { + t.Fatal("expected the status endpoint to be polled so the caller ends up with a transaction hash") + } + if out := buf.String(); !strings.Contains(out, "0xdeadbeef") { + t.Errorf("expected the reconciled transaction hash in the output, got: %q", out) + } +} + +func TestContractCallCmd_WaitReconcilesCompletedWithoutHash(t *testing.T) { + statusCalls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.HasSuffix(r.URL.Path, "/status") { + statusCalls++ + w.Header().Set("X-Poll-Interval-Hint", "0") + _, _ = w.Write([]byte(`{"executionId":"exec-cc","status":"completed","transactionHash":"0xc0ffee"}`)) + return + } + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-cc","status":"completed"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + cmd := execute.NewContractCallCmd(newContractCallFactory(ios, srv)) + cmd.SetArgs([]string{ + "--chain", "84532", + "--contract", "0x2A6FC8182Bf9928Ef7517dA980dC79e8107c555A", + "--method", "ping", + "--wait", + }) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if statusCalls == 0 { + t.Fatal("expected the status endpoint to be polled for a completed write with no hash") + } + if out := buf.String(); !strings.Contains(out, "0xc0ffee") { + t.Errorf("expected the reconciled transaction hash in the output, got: %q", out) + } +} + +// The server's pacing is honoured rather than a fixed client-side timer. A large hint on the +// first poll would previously have been ignored, and the client would have polled on its own +// two second cadence regardless of what the server asked for. +func TestPollHonoursServerInterval(t *testing.T) { + var firstPollAt, secondPollAt time.Time + polls := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if !strings.HasSuffix(r.URL.Path, "/status") { + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-slow","status":"running"}`)) + return + } + polls++ + switch polls { + case 1: + firstPollAt = time.Now() + w.Header().Set("X-Poll-Interval-Hint", "1") + _, _ = w.Write([]byte(`{"executionId":"exec-slow","status":"running"}`)) + default: + secondPollAt = time.Now() + w.Header().Set("X-Poll-Interval-Hint", "0") + _, _ = w.Write([]byte(`{"executionId":"exec-slow","status":"completed","transactionHash":"0xok"}`)) + } + })) + defer srv.Close() + + ios, _, _, _ := iostreams.Test() + cmd := execute.NewTransferCmd(newTransferFactory(ios, srv)) + cmd.SetArgs([]string{"--chain", "84532", "--to", "0xabc", "--amount", "0.1", "--wait"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if polls < 2 { + t.Fatalf("expected at least two polls, got %d", polls) + } + gap := secondPollAt.Sub(firstPollAt) + if gap < 900*time.Millisecond { + t.Errorf("second poll came after %v, expected roughly the 1s the server asked for", gap) + } + if gap > 1900*time.Millisecond { + t.Errorf("second poll came after %v, which suggests the old fixed 2s timer rather than the hint", gap) + } +} diff --git a/cmd/execute/status.go b/cmd/execute/status.go index b2d5a99..3d31053 100644 --- a/cmd/execute/status.go +++ b/cmd/execute/status.go @@ -2,6 +2,9 @@ package execute import ( "fmt" + "net/http" + "strconv" + "strings" "time" "github.com/jedib0t/go-pretty/v6/table" @@ -11,6 +14,30 @@ import ( "github.com/spf13/cobra" ) +// defaultPollInterval is used when the server sends no polling hint. +const defaultPollInterval = 2 * time.Second + +// nextPollDelay reads the X-Poll-Interval-Hint response header, which the Direct Execution API +// documents as the number of seconds to wait before polling again. Honouring it lets the server +// pace clients instead of every client polling on its own fixed timer. +// +// A hint of 0 means the execution has reached a terminal state, reported here as (0, true) so a +// caller stops rather than sleeping for zero and spinning. +func nextPollDelay(resp *http.Response) (time.Duration, bool) { + raw := strings.TrimSpace(resp.Header.Get("X-Poll-Interval-Hint")) + if raw == "" { + return defaultPollInterval, false + } + secs, err := strconv.Atoi(raw) + if err != nil || secs < 0 { + return defaultPollInterval, false + } + if secs == 0 { + return 0, true + } + return time.Duration(secs) * time.Second, false +} + // ExecStatusResponse represents the execution status API response. // Shared by transfer, contract-call and status commands. type ExecStatusResponse struct { @@ -58,7 +85,7 @@ See also: kh r st, kh ex transfer, kh ex cc`, p := output.NewPrinter(f.IOStreams, cmd) if !watch { - sr, fetchErr := fetchExecStatus(client, host, executionID) + sr, _, _, fetchErr := fetchExecStatus(client, host, executionID) if fetchErr != nil { return fetchErr } @@ -114,29 +141,26 @@ func renderExecStatus(p *output.Printer, f *cmdutil.Factory, sr *ExecStatusRespo func watchExecStatus(f *cmdutil.Factory, client *khhttp.Client, host, executionID string, p *output.Printer) error { isTTY := f.IOStreams.IsTerminal() - ticker := time.NewTicker(2 * time.Second) - defer ticker.Stop() for { - select { - case <-ticker.C: - sr, err := fetchExecStatus(client, host, executionID) - if err != nil { - return err - } + sr, delay, serverSaysTerminal, err := fetchExecStatus(client, host, executionID) + if err != nil { + return err + } + + if isTTY && !p.IsJSON() { + fmt.Fprintf(f.IOStreams.Out, "\r%s %s", executionID, sr.Status) + } + if execTerminalStatuses[sr.Status] || serverSaysTerminal { if isTTY && !p.IsJSON() { - fmt.Fprintf(f.IOStreams.Out, "\r%s %s", executionID, sr.Status) + fmt.Fprintln(f.IOStreams.Out) } + return renderExecStatus(p, f, sr) + } - if execTerminalStatuses[sr.Status] { - if isTTY && !p.IsJSON() { - fmt.Fprintln(f.IOStreams.Out) - } - return renderExecStatus(p, f, sr) - } - default: - time.Sleep(50 * time.Millisecond) + if delay > 0 { + time.Sleep(delay) } } } diff --git a/cmd/execute/transfer.go b/cmd/execute/transfer.go index 04f7589..d9aaf8e 100644 --- a/cmd/execute/transfer.go +++ b/cmd/execute/transfer.go @@ -109,7 +109,7 @@ func NewTransferCmd(f *cmdutil.Factory) *cobra.Command { }) } - if execTerminalStatuses[execResp.Status] { + if execTerminalStatuses[execResp.Status] && !writeNeedsReconciliation(execResp.Status, execResp.TransactionHash) { return printTransferResult(p, &execResp) } @@ -145,62 +145,75 @@ func printTransferResult(p *output.Printer, execResp *transferResponse) error { func pollExecStatus(f *cmdutil.Factory, client *khhttp.Client, host, executionID string, timeout time.Duration, p *output.Printer) error { deadline := time.Now().Add(timeout) - ticker := time.NewTicker(2 * time.Second) - defer ticker.Stop() for { - select { - case <-ticker.C: - statusResp, err := fetchExecStatus(client, host, executionID) - if err != nil { - return err - } + statusResp, delay, serverSaysTerminal, err := fetchExecStatus(client, host, executionID) + if err != nil { + return err + } - if execTerminalStatuses[statusResp.Status] { - if statusResp.Status == "failed" { - msg := fmt.Sprintf("execution %s failed", executionID) - if statusResp.Error != nil { - msg = *statusResp.Error - } - return fmt.Errorf("%s", msg) + if execTerminalStatuses[statusResp.Status] || serverSaysTerminal { + if statusResp.Status == "failed" { + msg := fmt.Sprintf("execution %s failed", executionID) + if statusResp.Error != nil { + msg = *statusResp.Error } - return printExecStatusResult(p, statusResp) + return fmt.Errorf("%s", msg) } + return printExecStatusResult(p, statusResp) + } - if time.Now().After(deadline) { - return fmt.Errorf("timeout after %s: execution %s still %s", timeout, executionID, statusResp.Status) - } - default: - if time.Now().After(deadline) { - return fmt.Errorf("timeout after %s: execution %s timed out", timeout, executionID) - } - time.Sleep(50 * time.Millisecond) + if time.Now().After(deadline) { + return fmt.Errorf("timeout after %s: execution %s still %s", timeout, executionID, statusResp.Status) + } + + // Wait as long as the server asked, but never past the caller's deadline. + if remaining := time.Until(deadline); delay > remaining { + delay = remaining + } + if delay > 0 { + time.Sleep(delay) } } } -func fetchExecStatus(client *khhttp.Client, host, executionID string) (*ExecStatusResponse, error) { +// fetchExecStatus returns the execution status, the delay the server asked us to wait before +// polling again, and whether the server declared the execution terminal via a hint of 0. +func fetchExecStatus(client *khhttp.Client, host, executionID string) (*ExecStatusResponse, time.Duration, bool, error) { url := khhttp.BuildBaseURL(host) + "/api/execute/" + executionID + "/status" req, err := client.NewRequest(http.MethodGet, url, nil) if err != nil { - return nil, err + return nil, 0, false, err } resp, err := client.Do(req) if err != nil { - return nil, err + return nil, 0, false, err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, khhttp.NewAPIError(resp) + return nil, 0, false, khhttp.NewAPIError(resp) } + delay, serverSaysTerminal := nextPollDelay(resp) + var sr ExecStatusResponse if err := json.NewDecoder(resp.Body).Decode(&sr); err != nil { - return nil, fmt.Errorf("decoding status response: %w", err) + return nil, 0, false, fmt.Errorf("decoding status response: %w", err) } - return &sr, nil + return &sr, delay, serverSaysTerminal, nil +} + +// writeNeedsReconciliation reports whether a direct-write response that claims a terminal status +// is missing the evidence the caller actually needs. +// +// A successful /api/execute/contract-call broadcast can return 202 with status "completed" and no +// transactionHash; the hash only appears on the status endpoint. Treating that response as final +// means --wait returns "completed" while the caller still has no transaction to verify, which is +// the one outcome the flag exists to prevent. +func writeNeedsReconciliation(status string, txHash *string) bool { + return status == "completed" && (txHash == nil || *txHash == "") } func printExecStatusResult(p *output.Printer, sr *ExecStatusResponse) error { From ec3de643724730996719b55abab0ede6d7b7dded Mon Sep 17 00:00:00 2001 From: Jacob Sussmilch Date: Tue, 18 Aug 2026 09:13:03 +1000 Subject: [PATCH 2/2] fix(execute): report completions with no transaction, clamp poll hint A completed execution can legitimately carry no transaction hash. Any action that submits nothing onchain - a read-only contract call, a non-transaction plugin step - finishes exactly that way, so the three paths that consume a status response now report the condition in their output and leave the exit code alone. Failing on it is opt-in behaviour and belongs behind a flag. Clamp X-Poll-Interval-Hint to 30 seconds. watchExecStatus has no deadline, so an oversized or hostile hint parked the loop for as long as the server asked. A hint of zero still means terminal and a negative or unparseable hint still falls back to the default, so neither can spin the loop. Treat unconfirmed as terminal. The server hands it back once a transaction was broadcast but no receipt was observed, and nothing moves it until the reconciler runs on its own schedule, so --wait and --watch previously polled on until the caller's timeout. They now stop there and exit zero, because a non-zero exit invites a retry, and retrying a broadcast can double-spend. --- cmd/execute/contract_call.go | 2 +- cmd/execute/pollhint_internal_test.go | 40 +++++--- cmd/execute/reconcile_test.go | 130 ++++++++++++++++++++++++++ cmd/execute/status.go | 15 ++- cmd/execute/transfer.go | 38 ++++++-- 5 files changed, 200 insertions(+), 25 deletions(-) diff --git a/cmd/execute/contract_call.go b/cmd/execute/contract_call.go index 48821b2..12ed071 100644 --- a/cmd/execute/contract_call.go +++ b/cmd/execute/contract_call.go @@ -124,7 +124,7 @@ func NewContractCallCmd(f *cmdutil.Factory) *cobra.Command { }) } - if execTerminalStatuses[writeResp.Status] && !writeNeedsReconciliation(writeResp.Status, writeResp.TransactionHash) { + if execTerminalStatuses[writeResp.Status] && !completedWithoutTransaction(writeResp.Status, writeResp.TransactionHash) { return printContractCallWriteResult(p, &writeResp) } diff --git a/cmd/execute/pollhint_internal_test.go b/cmd/execute/pollhint_internal_test.go index 2b07c2f..67ecdaf 100644 --- a/cmd/execute/pollhint_internal_test.go +++ b/cmd/execute/pollhint_internal_test.go @@ -7,9 +7,10 @@ import ( ) // A successful /api/execute/contract-call broadcast can return 202 with status "completed" and -// no transactionHash; the hash only appears on the status endpoint. Treating that as final means -// --wait returns success while the caller still has no transaction to verify. -func TestWriteNeedsReconciliation(t *testing.T) { +// no transactionHash; the hash only appears on the status endpoint, so a direct-write response in +// this shape is worth one reconciling fetch. On a status response the same shape is legitimate, +// because an action that submits nothing onchain completes this way, so it is only reported. +func TestCompletedWithoutTransaction(t *testing.T) { empty := "" hash := "0xabc" @@ -19,23 +20,38 @@ func TestWriteNeedsReconciliation(t *testing.T) { tx *string want bool }{ - {"completed without a hash must be reconciled", "completed", nil, true}, - {"completed with an empty hash must be reconciled", "completed", &empty, true}, - {"completed with a hash is final", "completed", &hash, false}, - {"failed is final regardless of the hash", "failed", nil, false}, - {"running is not terminal anyway", "running", nil, false}, - {"unconfirmed is not terminal anyway", "unconfirmed", nil, false}, + {"completed without a hash", "completed", nil, true}, + {"completed with an empty hash", "completed", &empty, true}, + {"completed with a hash", "completed", &hash, false}, + {"failed carries no such expectation", "failed", nil, false}, + {"running is not completed", "running", nil, false}, + {"unconfirmed already implies a broadcast", "unconfirmed", nil, false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := writeNeedsReconciliation(tc.status, tc.tx); got != tc.want { - t.Errorf("writeNeedsReconciliation(%q, %v) = %v, want %v", tc.status, tc.tx, got, tc.want) + if got := completedWithoutTransaction(tc.status, tc.tx); got != tc.want { + t.Errorf("completedWithoutTransaction(%q, %v) = %v, want %v", tc.status, tc.tx, got, tc.want) } }) } } +// unconfirmed is terminal for a client: nothing moves it until the reconciler runs, so a poll loop +// that treats it as pending just burns requests until the caller's timeout. +func TestExecTerminalStatuses(t *testing.T) { + for _, status := range []string{"completed", "failed", "unconfirmed"} { + if !execTerminalStatuses[status] { + t.Errorf("expected %q to be terminal", status) + } + } + for _, status := range []string{"pending", "running"} { + if execTerminalStatuses[status] { + t.Errorf("expected %q not to be terminal", status) + } + } +} + func TestNextPollDelay(t *testing.T) { cases := []struct { name string @@ -50,6 +66,8 @@ func TestNextPollDelay(t *testing.T) { {"surrounding whitespace is tolerated", " 3 ", true, 3 * time.Second, false}, {"an unparseable hint falls back rather than failing", "soon", true, defaultPollInterval, false}, {"a negative hint falls back", "-1", true, defaultPollInterval, false}, + {"a hint at the ceiling is honoured", "30", true, maxPollIntervalSecs * time.Second, false}, + {"an oversized hint is clamped to the ceiling", "3600", true, maxPollIntervalSecs * time.Second, false}, } for _, tc := range cases { diff --git a/cmd/execute/reconcile_test.go b/cmd/execute/reconcile_test.go index 8fe8478..d8cc671 100644 --- a/cmd/execute/reconcile_test.go +++ b/cmd/execute/reconcile_test.go @@ -79,6 +79,136 @@ func TestContractCallCmd_WaitReconcilesCompletedWithoutHash(t *testing.T) { } } +// completed with no transaction is legitimate: a read-only call or a non-transaction step +// completes without submitting anything. The three paths that consume a status response all +// report the condition and all exit zero. Erroring on it is opt-in behaviour, not the default. +func TestTransferCmd_WaitReportsCompletedWithoutTransaction(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.HasSuffix(r.URL.Path, "/status") { + w.Header().Set("X-Poll-Interval-Hint", "0") + _, _ = w.Write([]byte(`{"executionId":"exec-noop","status":"completed"}`)) + return + } + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-noop","status":"completed"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + cmd := execute.NewTransferCmd(newTransferFactory(ios, srv)) + cmd.SetArgs([]string{"--chain", "84532", "--to", "0xabc", "--amount", "0.1", "--wait"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("expected a completed execution with no transaction to succeed, got: %v", err) + } + if out := buf.String(); !strings.Contains(out, "none submitted") { + t.Errorf("expected the output to report that nothing was submitted, got: %q", out) + } +} + +func TestExecStatusCmd_ReportsCompletedWithoutTransaction(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"executionId":"exec-noop","status":"completed","type":"contract-call"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + cmd := execute.NewStatusCmd(newStatusFactory(ios, srv)) + cmd.SetArgs([]string{"exec-noop"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("expected a completed execution with no transaction to succeed, got: %v", err) + } + if out := buf.String(); !strings.Contains(out, "none submitted") { + t.Errorf("expected the output to report that nothing was submitted, got: %q", out) + } +} + +func TestExecStatusCmd_WatchReportsCompletedWithoutTransaction(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Poll-Interval-Hint", "0") + _, _ = w.Write([]byte(`{"executionId":"exec-noop","status":"completed"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + cmd := execute.NewStatusCmd(newStatusFactory(ios, srv)) + cmd.SetArgs([]string{"exec-noop", "--watch"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("expected a completed execution with no transaction to succeed, got: %v", err) + } + if out := buf.String(); !strings.Contains(out, "none submitted") { + t.Errorf("expected the output to report that nothing was submitted, got: %q", out) + } +} + +// unconfirmed is terminal. Nothing moves it until the reconciler runs on its own schedule, so a +// poll loop that keeps going only burns requests until the caller's timeout. It exits zero, since +// a non-zero exit invites a retry that can re-broadcast a transaction already onchain. +func TestTransferCmd_WaitStopsOnUnconfirmed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.HasSuffix(r.URL.Path, "/status") { + w.Header().Set("X-Poll-Interval-Hint", "1") + _, _ = w.Write([]byte(`{"executionId":"exec-unc","status":"unconfirmed","transactionHash":"0xunc"}`)) + return + } + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"executionId":"exec-unc","status":"pending"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + cmd := execute.NewTransferCmd(newTransferFactory(ios, srv)) + cmd.SetArgs([]string{"--chain", "84532", "--to", "0xabc", "--amount", "0.1", "--wait", "--timeout", "3s"}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("expected --wait to exit zero on unconfirmed, got: %v", err) + } + out := buf.String() + if !strings.Contains(out, "unconfirmed") { + t.Errorf("expected the unconfirmed status in the output, got: %q", out) + } + if !strings.Contains(out, "0xunc") { + t.Errorf("expected the transaction hash in the output, got: %q", out) + } +} + +// If unconfirmed were treated as pending, --watch has no deadline to fall back on and this would +// poll until the test binary is killed. +func TestExecStatusCmd_WatchStopsOnUnconfirmed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Poll-Interval-Hint", "1") + _, _ = w.Write([]byte(`{"executionId":"exec-unc","status":"unconfirmed","transactionHash":"0xunc"}`)) + })) + defer srv.Close() + + ios, buf, _, _ := iostreams.Test() + cmd := execute.NewStatusCmd(newStatusFactory(ios, srv)) + cmd.SetArgs([]string{"exec-unc", "--watch"}) + + done := make(chan error, 1) + go func() { done <- cmd.Execute() }() + + select { + case err := <-done: + if err != nil { + t.Fatalf("expected --watch to exit zero on unconfirmed, got: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("--watch kept polling an unconfirmed execution instead of stopping") + } + + if out := buf.String(); !strings.Contains(out, "0xunc") { + t.Errorf("expected the transaction hash in the output, got: %q", out) + } +} + // The server's pacing is honoured rather than a fixed client-side timer. A large hint on the // first poll would previously have been ignored, and the client would have polled on its own // two second cadence regardless of what the server asked for. diff --git a/cmd/execute/status.go b/cmd/execute/status.go index 3d31053..41d94e6 100644 --- a/cmd/execute/status.go +++ b/cmd/execute/status.go @@ -17,12 +17,18 @@ import ( // defaultPollInterval is used when the server sends no polling hint. const defaultPollInterval = 2 * time.Second +// maxPollIntervalSecs caps the server's hint. watchExecStatus has no deadline, so an oversized or +// hostile hint would otherwise park the loop for as long as the server asked. +const maxPollIntervalSecs = 30 + // nextPollDelay reads the X-Poll-Interval-Hint response header, which the Direct Execution API // documents as the number of seconds to wait before polling again. Honouring it lets the server -// pace clients instead of every client polling on its own fixed timer. +// pace clients instead of every client polling on its own fixed timer. The hint is clamped to +// maxPollIntervalSecs so it can slow a caller down but not stall it. // // A hint of 0 means the execution has reached a terminal state, reported here as (0, true) so a -// caller stops rather than sleeping for zero and spinning. +// caller stops rather than sleeping for zero and spinning. A negative or unparseable hint falls +// back to the default rather than yielding a zero delay the loop would spin on. func nextPollDelay(resp *http.Response) (time.Duration, bool) { raw := strings.TrimSpace(resp.Header.Get("X-Poll-Interval-Hint")) if raw == "" { @@ -35,7 +41,7 @@ func nextPollDelay(resp *http.Response) (time.Duration, bool) { if secs == 0 { return 0, true } - return time.Duration(secs) * time.Second, false + return time.Duration(min(secs, maxPollIntervalSecs)) * time.Second, false } // ExecStatusResponse represents the execution status API response. @@ -114,6 +120,9 @@ func renderExecStatus(p *output.Printer, f *cmdutil.Factory, sr *ExecStatusRespo if sr.TransactionLink != nil && *sr.TransactionLink != "" { tw.AppendRow(table.Row{"TX Link", *sr.TransactionLink}) } + if completedWithoutTransaction(sr.Status, sr.TransactionHash) { + tw.AppendRow(table.Row{"Transaction", noTransactionNote}) + } if sr.CreatedAt != "" { tw.AppendRow(table.Row{"Created", sr.CreatedAt}) } diff --git a/cmd/execute/transfer.go b/cmd/execute/transfer.go index d9aaf8e..caeec25 100644 --- a/cmd/execute/transfer.go +++ b/cmd/execute/transfer.go @@ -27,11 +27,21 @@ type transferResponse struct { TransactionHash *string `json:"transactionHash,omitempty"` } +// execTerminalStatuses are the statuses a poll loop stops on. +// +// unconfirmed is terminal for a client: the server hands it back once a transaction was broadcast +// but no receipt was observed, and nothing moves it until the reconciler runs on its own schedule, +// so polling past it only burns requests. --wait and --watch stop there and exit zero, because a +// non-zero exit invites a retry, and retrying a broadcast can double-spend. var execTerminalStatuses = map[string]bool{ - "completed": true, - "failed": true, + "completed": true, + "failed": true, + "unconfirmed": true, } +// noTransactionNote labels a completed execution that put nothing onchain. +const noTransactionNote = "none submitted" + func NewTransferCmd(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "transfer", @@ -109,7 +119,7 @@ func NewTransferCmd(f *cmdutil.Factory) *cobra.Command { }) } - if execTerminalStatuses[execResp.Status] && !writeNeedsReconciliation(execResp.Status, execResp.TransactionHash) { + if execTerminalStatuses[execResp.Status] && !completedWithoutTransaction(execResp.Status, execResp.TransactionHash) { return printTransferResult(p, &execResp) } @@ -205,14 +215,19 @@ func fetchExecStatus(client *khhttp.Client, host, executionID string) (*ExecStat return &sr, delay, serverSaysTerminal, nil } -// writeNeedsReconciliation reports whether a direct-write response that claims a terminal status -// is missing the evidence the caller actually needs. +// completedWithoutTransaction reports an execution that reached "completed" carrying no +// transaction hash. // -// A successful /api/execute/contract-call broadcast can return 202 with status "completed" and no -// transactionHash; the hash only appears on the status endpoint. Treating that response as final -// means --wait returns "completed" while the caller still has no transaction to verify, which is -// the one outcome the flag exists to prevent. -func writeNeedsReconciliation(status string, txHash *string) bool { +// On a direct-write response it means one status fetch is worth making: a successful +// /api/execute/contract-call broadcast can return 202 with status "completed" and no +// transactionHash, because the hash only appears on the status endpoint, so --wait would +// otherwise report a completion the caller cannot tie to a transaction. +// +// On a status response it is not an error. Any action that submits nothing onchain - a read-only +// contract call, a non-transaction plugin step - completes exactly this way by design, so the +// condition is reported in the output and the exit code is left alone. Failing on it is opt-in +// behaviour and belongs behind an explicit flag. +func completedWithoutTransaction(status string, txHash *string) bool { return status == "completed" && (txHash == nil || *txHash == "") } @@ -226,6 +241,9 @@ func printExecStatusResult(p *output.Printer, sr *ExecStatusResponse) error { if sr.TransactionLink != nil && *sr.TransactionLink != "" { tw.AppendRow(table.Row{"TX Link", *sr.TransactionLink}) } + if completedWithoutTransaction(sr.Status, sr.TransactionHash) { + tw.AppendRow(table.Row{"Transaction", noTransactionNote}) + } tw.Render() }) }