fix(limits): Bound idle probing and preserve partial results

CHANGELOG.md

@@ -5,7 +5,24 @@ All notable changes to this project are documented here. The format follows
 
 ## [Unreleased]
 
+### Added
+- `Options.IdleTimeout` and `nq --idle-timeout` bound the entire idle probing
+  phase (default 10 s). A stalled idle response now times out, preserves any
+  completed samples, warns, and allows the selected load phases to run (#39).
+  This flat cap also applies to healthy slow paths and existing callers with
+  large `IdleProbes` values; raise `IdleTimeout` to collect more samples and
+  reach higher percentile thresholds.
+
 ### Fixed
+- Idle-timeout warnings retain the preceding probe error and are omitted
+  when all requested idle samples have already completed (#39).
+- Caller cancellation retains completed idle samples in partial results;
+  caller deadlines during load report `cancelled` rather than the phase's
+  `duration_cap`. Discovery, idle, and load phase budgets are documented
+  together with their teardown and caller-code limitations (#39).
+- Cancellation closes an upload's response body and joins its cleanup, so
+  an HTTP/2 upload peer that responds early and then stops reading cannot
+  leave the client waiting on flow control after its deadline (#39).
 - Connections opened through a caller-supplied `DialTLSContext` (or `DialTLS`)
   were not tracked, so an HTTP/2 connection still winding down when `Run`
   returned outlived it — for ever, on a transport without `IdleConnTimeout`.

README.md

@@ -64,6 +64,7 @@ res, err := netquality.Run(ctx, netquality.Cloudflare, netquality.Options{
     MaxDuration: 10 * time.Second, // per direction; the budget
     MaxBytes:    100 << 20,        // optional: only on metered links
     MaxFlows:    8,
+    IdleTimeout: 5 * time.Second, // budget for all idle probes together
 })
 if err != nil { /* discovery failed, or ctx cancelled (res is then partial) */ }
 fmt.Println(res.Download.RPM, res.Download.ThroughputBPS, res.Download.Truncated)
@@ -102,6 +103,7 @@ nq --well-known nq.example.com:8443 [--insecure]
 nq --config-url https://host/path/config
 nq --download-only | --upload-only
 nq --max-duration 8s --max-flows 8          # time is the budget
+nq --idle-timeout 5s                       # cap the whole idle measurement phase
 nq --max-bytes 100MB                         # metered link: add a byte cap
 nq --json                            # Result as one JSON document on stdout
 nq --events                          # JSON-lines progress on stderr
@@ -215,20 +217,40 @@ compromise) and transparent TCP-level proxies that pass TLS through untouched
 
 | Limit | Default | Effect |
 |---|---|---|
+| `ConfigTimeout` | 10 s | bounds discovery; failure returns no result |
+| `IdleTimeout` | 10 s | bounds all idle probes together; keeps completed samples, warns, then proceeds to load |
 | `MaxDuration` | 12 s per direction | the budget: phase ends; if not yet stable → `truncated`, `reason=duration_cap`. Cost ≤ rate × 12 s |
 | `MaxBytes` | **none** (opt-in) | set on metered links; phase ends → `reason=bytes_cap` |
 | `MaxFlows` | 16 | never more concurrent load connections |
 | `ctx` cancellation | – | all flows stop within ~200 ms; partial result, `cancelled=true` |
 
-There are no retries, no background goroutines after `Run` returns, and no
-telemetry.
+The combined phase budget is `ConfigTimeout + IdleTimeout + N × MaxDuration`,
+where N is the selected direction count; omit `IdleTimeout` when idle is
+skipped. Defaults total 44 s for both directions, 32 s for one, or 34 s for
+both with idle skipped. An earlier caller deadline stops the run and retains
+completed idle samples and other partial results. Zero or negative duration
+options select defaults, so they cannot disable the bounds.
+`IdleTimeout` does not grow with `IdleProbes`. Larger sample sets and healthy
+slow paths may need a larger timeout to complete all probes and obtain the
+requested percentiles. An idle-timeout warning means the measurement budget
+ran out; it does not by itself diagnose a faulty connection.
+
+Return time also includes local orchestration and prompt teardown. Supplied
+transports, dialers, body closers, event sinks, and log handlers must honor
+cancellation where applicable and return promptly; the library cannot
+forcibly stop caller code. Runner goroutines join and owned sockets close
+before return. A custom TLS dialer still executing at teardown has its
+connection closed when it hands it over. There are no retries or telemetry.
+
+The client's `--idle-timeout` measures the whole idle probing phase; the
+server's flag of the same name limits quiet connections between requests.
 
 ## Deviations from the draft
 
 | Item | Draft | Here | Why |
 |---|---|---|---|
 | Interval (ID) | 5 s | **1 s** | 4 intervals must complete before stability can be declared; with the 12 s per-direction budget a 5 s interval could never stabilise. Earlier drafts and shipping tools use 1 s. Configurable via `Stability.Interval`. |
-| Time budget | "implementations may" limit | mandatory `MaxDuration`; `MaxBytes` opt-in | Runs on other people's machines and networks; a time bound makes cost proportional to the link instead of unbounded. |
+| Time budget | "implementations may" limit | mandatory discovery, idle, and per-direction time caps; `MaxBytes` opt-in | Runs on other people's machines and networks; a time bound makes cost proportional to the link instead of unbounded. |
 | Byte cap default | (handoff spec: 250 MB) | none | A fixed byte cap starves fast links of the intervals a confident RPM needs (≈ 8 × rate); the caller knows which networks are metered, the library cannot. |
 | Flow error | abort the test | abort the **phase**, report `reason=flow_error`, keep other results | Partial data with a flag beats none. |
 | Self probes on HTTP/1.1 | use TCP RTT estimate | omitted; RPM from foreign probes only, warning recorded | TCP_INFO is not portable in pure Go. |
@@ -242,7 +264,7 @@ telemetry.
 | Config field names | `*_download_url`, `upload_url` | also accepts Apple/Cloudflare `*_https_*` names, preferring them | Interop with deployed servers. |
 | Cloudflare target | `mach` hardcodes `h3.speed.cloudflare.com` URLs | uses `aim.cloudflare.com/responsiveness/api/v1/config`, which returns the same URLs | Keeps discovery uniform. |
 
-Other constants: `IdleProbes=5` (enough for a median, cheap), `ConfigTimeout=10s`,
+Other constants: `IdleProbes=5` (enough for a median, cheap), `ConfigTimeout=10s`, `IdleTimeout=10s`,
 in-flight probe cap 64 (bounds goroutines on high-RTT links), TLS handshake
 normalised to 1 RTT for TLS 1.3 and 2 for TLS 1.2.
 

clock_test.go

@@ -1,6 +1,9 @@
 package netquality
 
-import "time"
+import (
+	"context"
+	"time"
+)
 
 // fakeClock drives the interval ticker manually; Now, Mono and After use real
 // time so probes and network I/O keep flowing. coarse makes it claim the
@@ -17,7 +20,10 @@ func (f *fakeClock) Now() time.Time                         { return time.Now()
 func (f *fakeClock) Mono() instant                          { return monoNow() }
 func (f *fakeClock) HighResolution() bool                   { return !f.coarse }
 func (f *fakeClock) After(d time.Duration) <-chan time.Time { return time.After(d) }
-func (f *fakeClock) NewTicker(time.Duration) ticker         { return f }
-func (f *fakeClock) C() <-chan time.Time                    { return f.ch }
-func (f *fakeClock) Stop()                                  {}
-func (f *fakeClock) tick()                                  { f.ch <- time.Now() }
+func (f *fakeClock) WithTimeout(ctx context.Context, d time.Duration) (context.Context, context.CancelFunc) {
+	return context.WithTimeout(ctx, d)
+}
+func (f *fakeClock) NewTicker(time.Duration) ticker { return f }
+func (f *fakeClock) C() <-chan time.Time            { return f.ch }
+func (f *fakeClock) Stop()                          {}
+func (f *fakeClock) tick()                          { f.ch <- time.Now() }

cmd/nq/idle_timeout_test.go

@@ -0,0 +1,103 @@
+package main
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	"github.com/korya/netquality"
+	"github.com/korya/netquality/server"
+)
+
+func idleStallServer(t *testing.T, h2, body bool) string {
+	t.Helper()
+	var loading atomic.Bool
+	var small atomic.Int64
+	h := server.Handler(server.Options{MaxClientBytes: -1})
+	srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == server.LargePath {
+			loading.Store(true)
+		}
+		if r.URL.Path == server.SmallPath && !loading.Load() && small.Add(1) > 1 {
+			if body {
+				w.Header().Set("Content-Length", "1")
+				w.WriteHeader(http.StatusOK)
+				w.(http.Flusher).Flush()
+			}
+			<-r.Context().Done()
+			return
+		}
+		h.ServeHTTP(w, r)
+	}))
+	srv.EnableHTTP2 = h2
+	srv.StartTLS()
+	t.Cleanup(srv.Close)
+	return srv.URL + server.ConfigPath
+}
+
+func checkIdleTimeoutOutput(t *testing.T, out []byte) {
+	t.Helper()
+	var res netquality.Result
+	if err := json.Unmarshal(out, &res); err != nil {
+		t.Fatalf("invalid JSON: %v\n%s", err, out)
+	}
+	if res.Idle == nil || res.Idle.Samples != 1 || res.Cancelled || res.Download == nil || res.Upload != nil {
+		t.Fatalf("idle samples and selected load must survive expiry: %+v", res)
+	}
+	var warnings int
+	for _, w := range res.Warnings {
+		if strings.Contains(w, "idle timeout (500ms; 1/1000 probes completed)") {
+			warnings++
+		}
+	}
+	if warnings != 1 {
+		t.Errorf("timeout warning missing or repeated: %v", res.Warnings)
+	}
+}
+
+func TestIdleTimeoutOutput(t *testing.T) {
+	for _, h2 := range []bool{false, true} {
+		for _, body := range []bool{false, true} {
+			t.Run(fmt.Sprintf("h2=%v/body=%v", h2, body), func(t *testing.T) {
+				var out, errb bytes.Buffer
+				args := base(idleStallServer(t, h2, body), "--json", "--download-only", "--max-bytes", "1MB", "--idle-probes", "1000", "--idle-timeout", "500ms")
+				if code := run(args, &out, &errb); code != exitOK {
+					t.Fatalf("idle expiry must not fail the run: exit=%d stderr=%s", code, errb.String())
+				}
+				if errb.Len() != 0 {
+					t.Errorf("JSON run must keep stderr quiet: %s", errb.String())
+				}
+				checkIdleTimeoutOutput(t, out.Bytes())
+			})
+		}
+	}
+}
+
+func TestIdleTimeoutBinary(t *testing.T) {
+	bin := filepath.Join(t.TempDir(), "nq.exe")
+	buildCtx, stopBuild := context.WithTimeout(context.Background(), time.Minute)
+	defer stopBuild()
+	if out, err := exec.CommandContext(buildCtx, "go", "build", "-o", bin, ".").CombinedOutput(); err != nil {
+		t.Fatalf("build: %v\n%s", err, out)
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+	args := base(idleStallServer(t, true, true), "--json", "--download-only", "--max-bytes", "1MB", "--idle-probes", "1000", "--idle-timeout", "500ms")
+	cmd := exec.CommandContext(ctx, bin, args...)
+	var errb bytes.Buffer
+	cmd.Stderr = &errb
+	out, err := cmd.Output()
+	if err != nil {
+		t.Fatalf("binary did not finish successfully: %v; watchdog=%v stderr=%s", err, ctx.Err(), errb.String())
+	}
+	checkIdleTimeoutOutput(t, out)
+}

cmd/nq/main.go

@@ -48,6 +48,7 @@ func run(args []string, stdout, stderr io.Writer) int {
 		maxBytes     = fs.String("max-bytes", "", "per-direction byte cap for metered links (e.g. 100MB, 1GB); omitted = no cap, time bounds the run")
 		maxFlows     = fs.Int("max-flows", netquality.DefaultMaxFlows, "maximum concurrent load connections")
 		idleProbes   = fs.Int("idle-probes", netquality.DefaultIdleProbes, "number of idle latency probes")
+		idleTimeout  = fs.Duration("idle-timeout", netquality.DefaultIdleTimeout, "time cap for all idle probes; increase for more probes or slow paths")
 		interval     = fs.Duration("interval", 0, "stability interval (default 1s; draft says 5s)")
 		insecure     = fs.Bool("insecure", false, "skip TLS certificate verification (self-hosted dev servers)")
 		authToken    = fs.String("auth-token", os.Getenv("NQ_AUTH_TOKEN"), "bearer token for a protected server (env NQ_AUTH_TOKEN)")
@@ -103,6 +104,7 @@ func run(args []string, stdout, stderr io.Writer) int {
 		MaxBytes:    mb,
 		MaxFlows:    *maxFlows,
 		IdleProbes:  *idleProbes,
+		IdleTimeout: *idleTimeout,
 	}
 	opts.Stability.Interval = *interval
 	if *authToken != "" {

docs/architecture.md

@@ -47,8 +47,8 @@ real protocol in-process over TLS + HTTP/2 without a network.
 | One `*http.Transport` per load flow | Each flow is its own TCP/TLS connection (draft requirement); self probes multiplex onto it via HTTP/2. A user-supplied non-`*http.Transport` cannot be cloned, so flows may share connections and the library warns. |
 | Fresh connection per foreign/idle probe | `DisableKeepAlives` on a dedicated transport; `httptrace` supplies DNS/connect/TLS/TTFB stages. |
 | The dial wrappers see every connection | `DialContext` implements `test_endpoint` and records remote/local IPs. A user `DialTLSContext`/`DialTLS` bypasses it for https, so it is wrapped too: its connections are tracked for teardown and recorded, but not redirected to `test_endpoint` (DISC-9). A custom `RoundTripper` is opaque (documented limitation). |
-| The engine is pure | `internal/engine` sees only `Observation`s (elapsed, bytes, flows, probe samples) and returns `Decision`s; it holds no clock, goroutine, or socket, so the same code runs against real transports and, in tests, against recorded series or a simulator. Public latency/confidence types are aliases of engine types. The interval loop in `run.go` is driven by an injectable clock. |
-| Cancellation via context only | Flows read bodies until the context ends; the upload body reader stops on context; no goroutine outlives `Run` (INV-4). |
+| The engine is pure | `internal/engine` sees only `Observation`s (elapsed, bytes, flows, probe samples) and returns `Decision`s; it holds no clock, goroutine, or socket, so the same code runs against real transports and, in tests, against recorded series or a simulator. Public latency/confidence types are aliases of engine types. The interval loop and discovery/idle/load timeout creation in `run.go` go through the injectable clock; production deadlines use `context.WithTimeout`. |
+| Cancellation via context only | Flows read bodies until the context ends; the upload body reader stops on context. Runner goroutines join and owned sockets close before return. Caller code must return promptly; a custom TLS dialer still executing at teardown has its connection closed on handover (INV-4, LIM-9). |
 | Byte accounting is client-side | Upload bytes are counted when handed to the transport, so a few MB of HTTP/2 flow-control window may be in flight beyond `MaxBytes`. |
 | Interval default 1 s, not the draft's 5 s | A 12 s budget cannot fit four 5 s intervals; see README "Deviations" (INV-6). |
 

docs/product-specs/cli.md

@@ -13,8 +13,10 @@ An unknown target name is a usage error.
 
 ### CLI-2: Limits and parameters
 `--max-duration`, `--max-bytes` (accepts `250MB`, `1GB`, `100MiB`, plain
-bytes), `--max-flows`, `--idle-probes`, `--interval` map onto the library
+bytes), `--max-flows`, `--idle-probes`, `--idle-timeout`, `--interval` map onto the library
 options; invalid sizes are usage errors.
+`--idle-timeout` (default 10 s) caps the whole idle measurement phase, not
+each probe; a non-positive value selects the default (LIM-8).
 
 ### CLI-3: Direction flags
 `--download-only` and `--upload-only` select one phase; passing both is a

docs/product-specs/invariants.md

@@ -22,9 +22,13 @@ A result never implies "not measured" with a zero. Absent phases are absent
 levels accompany converged values.
 
 ### INV-4: No work outlives `Run`
-When `Run` returns, all goroutines, connections, and timers it created are
-gone. Cancellation propagates to every flow and probe. This covers the
-client's own sockets. Abandoning a load flow closes its socket with data still
+When `Run` returns, its flow/probe goroutines have joined, its context timers
+are canceled, and its owned connections are closed. Cancellation propagates
+to every flow and probe. Caller-supplied code must cooperate and return
+promptly (LIM-9). A custom TLS dialer may still be inside a transport-owned
+dial goroutine with a socket it has not returned; if it returns after
+teardown, that connection is closed on handover. This covers the
+client's owned sockets. Abandoning a load flow closes its socket with data still
 unread, which TCP turns into an abortive close, so how long the *server's*
 socket survives is the server's business, not something the client can
 promise.

docs/product-specs/latency.md

@@ -12,6 +12,9 @@ Before any load, `IdleProbes` (default 5) sequential GETs of the small
 resource run, each on a brand-new connection. A negative `IdleProbes` skips
 the phase and `Result.Idle` is absent. Failed probes are dropped; if none
 succeed the phase yields no result and a warning.
+The whole phase is bounded by `IdleTimeout` (LIM-8). Completed samples are
+reported even when the phase times out or the caller cancels before all
+requested probes complete; interrupted probes never become latency samples.
 
 ### LAT-2: Per-stage timings
 Every fresh-connection sample records DNS, TCP connect, TLS handshake,
@@ -43,8 +46,12 @@ defined as the mean absolute deviation from the mean. Percentiles use the
 nearest-rank method and appear only when the sample count makes them distinct
 from the maximum: `p80` from 5 samples, `p90` from 10, `p95` from 20, `p99`
 from 100. A percentile field never holds a lower percentile than its name;
-an absent field means too few samples. With the default 5 idle probes the
-idle set reports `p80`; loaded sets usually report all four.
+an absent field means too few samples. When all 5 default idle probes
+complete, the idle set reports `p80`; loaded sets usually report all four.
+For idle percentiles, requesting more probes does not increase the phase
+budget. Increase `IdleTimeout` as needed along with `IdleProbes`, especially
+on slow paths: only samples completed within that budget count towards
+these thresholds, even when every response would eventually succeed.
 
 ### LAT-8: Combined loaded latency
 `loaded.combined` merges foreign and self samples using each probe's