Skip to content

--timeout is checked between polls, so it overshoots and can succeed after expiring #35

Description

@kshahbw

What's wrong

--timeout reads as a wall-clock bound. It isn't one. internal/cmdutil/poll.go:

for {
    done, result, err := cfg.Check()
    if err != nil  { return nil, err }
    if done        { return result, nil }        // (1) returns before testing the deadline
    if time.Now().After(deadline) { return nil, ErrPollTimeout }
    timer := time.NewTimer(cfg.Interval)          // (2) always the full interval
    select { case <-ctx.Done(): ...; case <-timer.C: }
}

Two consequences:

It can succeed after expiring. At (1), done is returned without checking the deadline. With a 5s interval and --timeout 1: the first Check is pending, the loop sleeps 5s, the second Check succeeds — the command exits 0 at t≈5s, five seconds past the stated timeout, reporting success for a timeout the user asked to enforce.

It overshoots on the failure path too. At (2) the loop sleeps the whole interval rather than min(interval, time-until-deadline), so --timeout 1 with a 5s interval reports exit 5 at t≈5s, not t≈1s. A slow Check can push it further — up to the API client's 30s HTTP timeout on top.

Why it matters

Agents branch on exit 5 and on elapsed time. A --timeout that can be exceeded by 5× and can return success after expiring makes both unreliable. It also makes short timeouts useless for their main purpose: bounding a probe.

Shared by every --wait command — number order, call create, transcription create, and the new tendlc brand create / brand delete / vetting request / vetting import.

Reproduce

band tendlc brand create ... --wait --timeout 1
# exits 0 at ~5s if the second poll succeeds, or 5 at ~5s if it doesn't — never at ~1s

Suggested fix

Both are small and independent:

  1. Test the deadline before honoring done, or — better — derive the loop from a context.WithDeadline so expiry is authoritative regardless of where the loop is.
  2. Sleep min(cfg.Interval, time.Until(deadline)) so the final wait lands on the deadline instead of past it.

Worth deciding explicitly whether "succeeded, but after the deadline" should be exit 0 or exit 5. I'd argue exit 0 with the result is right — the operation genuinely completed and discarding that is worse than being slightly late — but then the docs should say --timeout bounds when polling stops, not total duration. Right now they imply a precision the loop doesn't deliver, which is the part that's unambiguously wrong.

Interim state

AGENTS.md now says --timeout bounds when the CLI stops starting new polls, and that a call can overshoot by up to one interval plus the in-flight request. Accurate, but the underlying behavior is still surprising.

Related: #34 (no signal handling, so PollConfig.Context has no real producer). Both were found by an adversarial review pass on #33.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions