diff --git a/AGENTS.md b/AGENTS.md index aa84aa9..87c3c82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -215,15 +215,15 @@ band transcription create --wait # blocks until t All `--wait` commands support `--timeout `. Exit code 5 on timeout. **`--timeout` bounds when polling stops, not wall-clock duration.** The -underlying poll loop (`internal/cmdutil/poll.go`) checks the deadline -*between* poll attempts, not during one, and always sleeps the full -`--interval` rather than whatever time remains before the deadline. So a -call can run past `--timeout` by up to one poll interval plus one in-flight -request — and if that late attempt happens to succeed, the command exits -**0**, not 5, seconds after the timeout you asked for. Concretely: a 5s -poll interval with `--timeout 1` can still succeed at t≈5s. Do not treat -`--timeout` as a precise deadline; treat it as a lower bound on how long the -CLI will keep trying. +underlying poll loop (`internal/cmdutil/poll.go`) starts no new poll after +the deadline, and the sleep before the deadline is capped so the final poll +lands on the deadline rather than a full interval past it. A poll that +starts on time and succeeds returns exit **0** with the result even if it +finishes slightly after the deadline — the operation genuinely completed. +So total overshoot is bounded by one in-flight request (the API client's +HTTP timeout in the worst case), not by the poll interval: a 5s poll +interval with `--timeout 1` resolves at t≈1s plus one request, as success +or as exit 5. ## Output diff --git a/internal/cmdutil/poll.go b/internal/cmdutil/poll.go index 78ba7ef..5d062be 100644 --- a/internal/cmdutil/poll.go +++ b/internal/cmdutil/poll.go @@ -28,6 +28,14 @@ type PollConfig struct { // Poll runs cfg.Check repeatedly at cfg.Interval until it returns done=true or // cfg.Timeout is exceeded. On success it returns the result from Check. // On timeout it returns ErrPollTimeout. +// +// cfg.Timeout bounds when polling stops, not total wall-clock duration: no +// check starts after the deadline, and the sleep before the deadline is +// capped so the final check lands on the deadline rather than a full +// interval past it. A check that starts on time and completes successfully +// returns its result even if it finishes after the deadline — the operation +// genuinely completed, and discarding that would be worse than being +// slightly late. Total overshoot is bounded by one in-flight request. func Poll(cfg PollConfig) (interface{}, error) { ctx := cfg.Context if ctx == nil { @@ -43,9 +51,14 @@ func Poll(cfg PollConfig) (interface{}, error) { if done { return result, nil } - if time.Now().After(deadline) { + remaining := time.Until(deadline) + if remaining <= 0 { return nil, fmt.Errorf("timed out after %s: %w", cfg.Timeout, ErrPollTimeout) } + wait := cfg.Interval + if remaining < wait { + wait = remaining + } // A fresh timer per iteration. Go 1.23+ made timer channels // unbuffered and Stop() cancels any in-flight send, so there is // nothing left to drain after Stop() returns — draining an @@ -53,7 +66,7 @@ func Poll(cfg PollConfig) (interface{}, error) { // Stop() on the cancellation path is a courtesy; an abandoned // timer is garbage collected once unreferenced, so there is no // leak either way. - timer := time.NewTimer(cfg.Interval) + timer := time.NewTimer(wait) select { case <-ctx.Done(): timer.Stop() diff --git a/internal/cmdutil/poll_test.go b/internal/cmdutil/poll_test.go index 1a79f28..e8251a5 100644 --- a/internal/cmdutil/poll_test.go +++ b/internal/cmdutil/poll_test.go @@ -77,6 +77,51 @@ func TestPollRespectsContextCancellation(t *testing.T) { } } +// The final sleep must be capped at the time remaining before the deadline, +// not a full interval — a 250ms interval with a 20ms timeout should report +// the timeout at ~20ms, not ~250ms. +func TestPollTimeoutDoesNotOvershootByFullInterval(t *testing.T) { + start := time.Now() + _, err := Poll(PollConfig{ + Interval: 250 * time.Millisecond, + Timeout: 20 * time.Millisecond, + Check: func() (bool, interface{}, error) { return false, nil, nil }, + }) + elapsed := time.Since(start) + if !errors.Is(err, ErrPollTimeout) { + t.Fatalf("err = %v, want ErrPollTimeout", err) + } + if elapsed >= 150*time.Millisecond { + t.Errorf("elapsed = %v, want ~20ms (final sleep must be capped at the deadline, not the full interval)", elapsed) + } +} + +// A check that starts at the deadline and succeeds still returns the result — +// the operation genuinely completed, and it does so at ~deadline rather than +// a full interval later. +func TestPollFinalCheckAtDeadlineCanSucceed(t *testing.T) { + start := time.Now() + calls := 0 + got, err := Poll(PollConfig{ + Interval: 250 * time.Millisecond, + Timeout: 20 * time.Millisecond, + Check: func() (bool, interface{}, error) { + calls++ + return calls >= 2, "done", nil + }, + }) + elapsed := time.Since(start) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "done" { + t.Fatalf("got %v, want done", got) + } + if elapsed >= 150*time.Millisecond { + t.Errorf("elapsed = %v, want ~20ms (the pre-deadline sleep must be capped so the final check runs at the deadline)", elapsed) + } +} + // A nil Context must behave exactly as before — all existing callers omit it. func TestPollNilContextStillTimesOut(t *testing.T) { _, err := Poll(PollConfig{