diff --git a/.github/workflows/ci-tests.yml b/.github/workflows/ci-tests.yml index 28b4ce7..4122f9a 100644 --- a/.github/workflows/ci-tests.yml +++ b/.github/workflows/ci-tests.yml @@ -9,14 +9,14 @@ on: - main env: - GO_VERSION: 1.26.4 + GO_VERSION: 1.26.5 GOLANGCI_LINT_VERSION: v2.12.2 jobs: golangci: runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - uses: actions/setup-go@v6 with: go-version: ${{ env.GO_VERSION }} @@ -31,7 +31,7 @@ jobs: name: Unit tests (race) runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - uses: actions/setup-go@v6 with: go-version: ${{ env.GO_VERSION }} @@ -42,7 +42,7 @@ jobs: name: Unit tests (flaky) runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - uses: actions/setup-go@v6 with: go-version: ${{ env.GO_VERSION }} @@ -53,7 +53,7 @@ jobs: name: Vulnerability scan runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - uses: actions/setup-go@v6 with: go-version: ${{ env.GO_VERSION }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1f3bb59..0b0a7e3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -14,7 +14,7 @@ permissions: contents: write env: - GO_VERSION: 1.26.4 + GO_VERSION: 1.26.5 GOLANGCI_LINT_VERSION: v2.12.2 jobs: @@ -24,7 +24,7 @@ jobs: outputs: version: ${{ steps.get.outputs.version }} steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - id: get run: | raw=$(cat VERSION) @@ -35,7 +35,7 @@ jobs: needs: [initialize] runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -87,7 +87,7 @@ jobs: needs: [initialize] runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 - run: bash scripts/check-go-version.sh test-race: @@ -95,7 +95,7 @@ jobs: runs-on: ubuntu-24.04 needs: [version-bump-check, go-version-align] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -112,7 +112,7 @@ jobs: runs-on: ubuntu-24.04 needs: [version-bump-check, go-version-align] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -128,7 +128,7 @@ jobs: runs-on: ubuntu-24.04 needs: [version-bump-check, go-version-align] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -147,7 +147,7 @@ jobs: runs-on: ubuntu-24.04 needs: [version-bump-check, go-version-align] steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -167,7 +167,7 @@ jobs: version: ${{ steps.compute_tag.outputs.version }} is_prerelease: ${{ steps.compute_tag.outputs.is_prerelease }} steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 @@ -377,7 +377,7 @@ jobs: needs: [release] runs-on: ubuntu-24.04 steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@v7 with: fetch-depth: 0 # snapcraft.yaml's `git describe --tags` needs full history diff --git a/CHANGELOG.md b/CHANGELOG.md index c2527e0..7c563ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `wsstat measure --version` and `wsstat stream --version` now print the version instead of failing with an unknown-flag error. + +### Fixed + +- (lib) Mixed subscription + measurement sessions no longer report a zeroed `MessageRTT`: `Subscribe`'s initial payload write is no longer recorded in the RTT write ledger (subscription responses were never recorded as reads, so the unbalanced ledgers discarded all RTT data). A write dropped during connection teardown likewise no longer leaves an orphan write timing. +- (lib) `DialContext` now rejects reuse: dialing a closed instance returns `ErrClosed`, and a second dial on a live instance returns an error instead of silently leaking the previous connection and its pumps. A failed dial leaves the instance reusable. +- (lib) Request/response read timings are now stamped when the frame arrives in the read pump rather than when the consumer drains the channel, so time spent buffered no longer inflates `MessageRTT` (matching how subscription deliveries were already stamped). +- Subcommand help (`wsstat measure -h`, `wsstat stream -h`) now prints to stdout, matching top-level `-h` and GNU convention, so it can be piped to a pager. Parse errors keep printing usage to stderr. +- Misordered invocations now get targeted hints: flags after the URL (`wsstat -v`) explain that flags must come before the URL, and a subcommand after global flags (`wsstat -v stream `) explains that the subcommand must come first. Previously both failed with a bare "expected exactly one URL argument". +- `-b/--buffer` help no longer claims `[default: 0]`; the effective default queue length is 32. + ## [3.1.0] - 2026-06-24 ### Added diff --git a/README.md b/README.md index b726ca2..817f9e7 100644 --- a/README.md +++ b/README.md @@ -14,21 +14,34 @@ This project provides a way to stat a WebSocket connection; measure the latency The CLI client provides a simple and easy to use tool to check the status of a WebSocket endpoint: ```sh -~ wsstat example.org +~ wsstat -v echo.websocket.org -Target: example.org -IP: 1.2.3.4 +Target: echo.websocket.org +IP: 66.241.124.119 +Messages sent: 1 WS version: 13 TLS version: TLS 1.3 DNS Lookup TCP Connection TLS Handshake WS Handshake Message RTT -| 61ms | 22ms | 44ms | 29ms | 27ms | +| 1.541ms | 287.437ms | 321.186ms | 730.487ms | 518.387ms | | | | | | | -| DNS lookup:61ms | | | | -| TCP connected:84ms | | | -| TLS done:128ms | | -| WS done:158ms | -- Total:186ms +| DNS lookup:1.541ms | | | | +| TCP connected:288.979ms | | | +| TLS done:610.165ms | | +| WS done:1340.652ms | +- Total:2366.209ms +``` + +Without `-v`, output is a compact summary: + +```sh +~ wsstat echo.websocket.org + +URL: echo.websocket.org +IP: 66.241.124.119 + +Round-trip time: 492.884ms (1 message) +Total time: 2296.079ms ``` The client replicates what [reorx/httpstat](https://github.com/reorx/httpstat) and [davecheney/httpstat](https://github.com/davecheney/httpstat) does for HTTP, but for WebSocket. It is said that imitation is the sincerest form of flattery, and inspiration has for certain been sourced from these projects. @@ -51,17 +64,15 @@ Install via Go: ```sh # To install the latest version, specify other releases with @ -go install github.com/jkbrsn/wsstat/v3@latest +go install github.com/jkbrsn/wsstat/v3/cmd/wsstat@latest -# To include the version in the binary, run the install from the root of the repo -git clone github.com/jkbrsn/wsstat +# To include the version in the binary, build from a clone of the repo +git clone https://github.com/jkbrsn/wsstat.git cd wsstat -git fetch --all -git checkout origin/main go install -ldflags "-X main.version=$(cat VERSION)" ./cmd/wsstat ``` -Note: installing the package with `@latest` will always install the latest version no matter the other parameters of the command. +Note: a remote `go install ...@` cannot inject the version via ldflags, so a binary installed that way reports its version as `unknown`. Build from a clone (as above) to get the version stamped. The snap is listed here: [snapcraft.io/wsstat](https://snapcraft.io/wsstat) @@ -136,6 +147,9 @@ wsstat --rpc-method eth_blockNumber wss://rpc.example.com/ws # Start a stream wsstat stream --summary-interval 5s wss://stream.example.com/feed +# Record response payloads to a file as NDJSON (in addition to normal output) +wsstat stream --file capture.ndjson wss://stream.example.com/feed + # Attach headers to dial request wsstat -H "Authorization: Bearer TOKEN" -H "Origin: https://foo" wss://api.example.com/ws @@ -191,8 +205,9 @@ wsstat stream --once -t '{"method":"subscribe_ticker"}' wss://example.org/ws Output is split across three orthogonal axes: - `-o, --output text|json|raw` — the whole-stdout contract. `json` emits - newline-delimited envelopes with a stable schema (`-v`/`-vv` never change which - fields appear); `raw` writes payload bytes verbatim with nothing added — no + newline-delimited envelopes with a stable, published schema (see + [docs/schema/](./docs/schema/); `-v`/`-vv` never change which fields appear); + `raw` writes payload bytes verbatim with nothing added — no label, color, or trailing newline, so frames stay binary-safe and stream frames concatenate undelimited (use `-o json` when you need a delimiter). In measure mode `raw` requires `--text` or `--rpc-method`; with `--rpc-method` the response @@ -203,6 +218,11 @@ Output is split across three orthogonal axes: - `--clip` — clips each rendered line to the terminal width with a trailing `...` (text output, TTY only; a no-op when piped or redirected). +Orthogonal to all three axes, `--file ` additionally records each response +payload to `` as NDJSON, one per line. It captures response bodies only +(summaries and other chrome still go to stdout), refuses to overwrite an +existing file, and removes the file again if nothing was recorded. + ```sh # Machine-readable streaming summaries and events wsstat stream -o json --summary-interval 5s wss://example.org/stream diff --git a/cmd/wsstat/config.go b/cmd/wsstat/config.go index 13ba14d..d21e137 100644 --- a/cmd/wsstat/config.go +++ b/cmd/wsstat/config.go @@ -54,7 +54,8 @@ type commonFlags struct { subprotocol string validateUTF8 bool - debug bool + debug bool + version bool } // registerCommon registers the shared flags onto fs, binding them to c. @@ -96,6 +97,7 @@ func registerCommon(fs *flag.FlagSet, c *commonFlags) { "validate UTF-8 on inbound text frames and warn on violations (coder/websocket skips this)") fs.BoolVar(&c.debug, "debug", false, "emit core debug logs to stderr (independent of -v/-vv output verbosity)") + fs.BoolVar(&c.version, "version", false, "print program version and exit") } // textOnlyFlags maps the internal flag names rejected under json/raw output to @@ -130,7 +132,7 @@ func resolveCommon(fs *flag.FlagSet, c *commonFlags, mode app.Mode) ([]app.Optio switch color { case "auto", "always", "never": default: - return nil, nil, errors.New("-color must be auto, always, or never") + return nil, nil, errors.New("--color must be auto, always, or never") } verbosity := 0 @@ -334,7 +336,7 @@ func setFlagNames(fs *flag.FlagSet) map[string]bool { func positionalURL(fs *flag.FlagSet) (*url.URL, error) { rest := fs.Args() if len(rest) != 1 { - return nil, errors.New("expected exactly one URL argument") + return nil, positionalErr(rest) } target, err := parseWSURI(rest[0]) if err != nil { @@ -343,6 +345,27 @@ func positionalURL(fs *flag.FlagSet) (*url.URL, error) { return target, nil } +// positionalErr diagnoses the common orderings behind a wrong positional count: +// a subcommand placed after global flags (dispatch keys on args[0] only), and +// flags placed after the URL (stdlib flag stops parsing at the first positional). +func positionalErr(rest []string) error { + base := errors.New("expected exactly one URL argument") + if len(rest) < 2 { + return base + } + if rest[0] == "measure" || rest[0] == "stream" { + return fmt.Errorf("%w: the subcommand must come first: wsstat %s [options] ", + base, rest[0]) + } + for _, arg := range rest[1:] { + if strings.HasPrefix(arg, "-") { + return fmt.Errorf("%w: flags must come before the URL (found %q after %q)", + base, arg, rest[0]) + } + } + return base +} + // parseWSURI parses rawURI into a URL, defaulting a missing scheme to wss://. Only the // ws and wss schemes are accepted; http/https (and anything else) are rejected so they are // not silently dialed as plaintext by the lenient underlying dialer. diff --git a/cmd/wsstat/config_test.go b/cmd/wsstat/config_test.go index b99524f..0416cdc 100644 --- a/cmd/wsstat/config_test.go +++ b/cmd/wsstat/config_test.go @@ -158,6 +158,15 @@ func TestMeasureFlags(t *testing.T) { _, _, err := buildMeasure([]string{"-v", "stream", "example.com"}) require.Error(t, err) assert.Contains(t, err.Error(), "exactly one URL") + assert.Contains(t, err.Error(), "the subcommand must come first") + }) + + t.Run("flags after the URL get an ordering hint", func(t *testing.T) { + // stdlib flag stops parsing at the first positional, so -v becomes a positional. + _, _, err := buildMeasure([]string{"example.com", "-v"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "exactly one URL") + assert.Contains(t, err.Error(), "flags must come before the URL") }) t.Run("headers accumulate", func(t *testing.T) { diff --git a/cmd/wsstat/main.go b/cmd/wsstat/main.go index 6c2dc96..f56c0a8 100644 --- a/cmd/wsstat/main.go +++ b/cmd/wsstat/main.go @@ -36,6 +36,7 @@ import ( "errors" "flag" "fmt" + "io" "net/url" "os" "os/signal" @@ -57,6 +58,10 @@ const ( // so main should exit without printing anything further. var errUsageShown = errors.New("usage shown") +// errVersionShown signals that --version was handled on a subcommand; main +// exits 0 with no further output. +var errVersionShown = errors.New("version shown") + // responseFilePerm is the mode for the --file response sink (owner read/write, group/other read). const responseFilePerm = 0o644 @@ -74,7 +79,8 @@ func (e *cliError) Unwrap() error { return e.err } // usageErr classifies a build/validation failure as exit 2. The flag-parse sentinels // (already-printed usage, help) pass through unchanged for main's dispatch switch. func usageErr(err error) error { - if errors.Is(err, flag.ErrHelp) || errors.Is(err, errUsageShown) { + if errors.Is(err, flag.ErrHelp) || errors.Is(err, errUsageShown) || + errors.Is(err, errVersionShown) { return err } return &cliError{code: exitUsage, err: err} @@ -127,7 +133,7 @@ func main() { } switch { - case err == nil, errors.Is(err, flag.ErrHelp): + case err == nil, errors.Is(err, flag.ErrHelp), errors.Is(err, errVersionShown): return case errors.Is(err, errUsageShown): os.Exit(exitUsage) @@ -230,10 +236,14 @@ func buildMeasure(args []string) (*app.Client, *url.URL, error) { registerRemoved(fs) count := fs.Int("c", 1, "number of interactions to perform (>= 1)") fs.IntVar(count, "count", 1, "number of interactions to perform (>= 1)") - fs.Usage = func() { printMeasureUsage(fs.Output()) } + fs.Usage = func() {} // parseErr owns usage printing (stdout for -h, stderr otherwise) if err := fs.Parse(args); err != nil { - return nil, nil, parseErr(err) + return nil, nil, parseErr(err, printMeasureUsage) + } + if cf.version { + fmt.Printf("wsstat %s\n", version) + return nil, nil, errVersionShown } if err := removedFlagError(fs); err != nil { return nil, nil, err @@ -269,10 +279,14 @@ func buildStream(args []string) (*app.Client, *url.URL, error) { fs.IntVar(buffer, "buffer", 0, "delivery buffer size (messages)") summary := fs.Duration("summary-interval", 0, "print stat summaries every interval (e.g., 5s, 1m); 0 disables") - fs.Usage = func() { printStreamUsage(fs.Output()) } + fs.Usage = func() {} // parseErr owns usage printing (stdout for -h, stderr otherwise) if err := fs.Parse(args); err != nil { - return nil, nil, parseErr(err) + return nil, nil, parseErr(err, printStreamUsage) + } + if cf.version { + fmt.Printf("wsstat %s\n", version) + return nil, nil, errVersionShown } if err := removedFlagError(fs); err != nil { return nil, nil, err @@ -310,12 +324,16 @@ func buildStream(args []string) (*app.Client, *url.URL, error) { return client, target, nil } -// parseErr maps a FlagSet parse error to the appropriate sentinel: ErrHelp passes -// through (help already printed), anything else became errUsageShown. -func parseErr(err error) error { +// parseErr maps a FlagSet parse error to the appropriate sentinel and prints the +// command usage: to stdout for explicitly requested help (GNU convention, and +// matching top-level -h), to stderr after a genuine parse error (whose message +// the FlagSet already printed there). +func parseErr(err error, usage func(io.Writer)) error { if errors.Is(err, flag.ErrHelp) { + usage(os.Stdout) return flag.ErrHelp } + usage(os.Stderr) return errUsageShown } diff --git a/cmd/wsstat/main_test.go b/cmd/wsstat/main_test.go index aa358c8..9f5bd4e 100644 --- a/cmd/wsstat/main_test.go +++ b/cmd/wsstat/main_test.go @@ -77,6 +77,10 @@ func TestErrorClassification(t *testing.T) { assert.ErrorIs(t, usageErr(errUsageShown), errUsageShown) }) + t.Run("version-shown passes through", func(t *testing.T) { + assert.ErrorIs(t, usageErr(errVersionShown), errVersionShown) + }) + t.Run("validation becomes exit 2", func(t *testing.T) { var ce *cliError require.ErrorAs(t, usageErr(errors.New("count must be greater than 0")), &ce) diff --git a/cmd/wsstat/usage.go b/cmd/wsstat/usage.go index 0c345ea..2355721 100644 --- a/cmd/wsstat/usage.go +++ b/cmd/wsstat/usage.go @@ -94,6 +94,7 @@ func printCommonFlags(w io.Writer) { fmt.Fprintln(w) fmt.Fprintln(w, "Diagnostics:") fmt.Fprintln(w, " --debug emit core debug logs to stderr (independent of -v/-vv)") + fmt.Fprintln(w, " --version print program version and exit") } // printMeasureUsage prints usage for the measure subcommand. @@ -127,7 +128,7 @@ func printStreamUsage(w io.Writer) { fmt.Fprintln(w, "Stream:") fmt.Fprintln(w, " -c, --count number of events to receive [default: 0 = unlimited]") fmt.Fprintln(w, " --once exit after the first event") - fmt.Fprintln(w, " -b, --buffer delivery buffer size in messages [default: 0]") + fmt.Fprintln(w, " -b, --buffer delivery buffer size in messages [default: 32]") fmt.Fprintln(w, " --summary-interval ") fmt.Fprintln(w, " print stat summaries every interval (e.g., 5s, 1m) [default: disabled]") fmt.Fprintln(w) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index f7ca8bc..d093ce4 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -56,4 +56,5 @@ What follows. What becomes easier, what becomes harder, what's now load-bearing. | # | Title | Status | |---|---|---| | 0001 | [`-o raw` is verbatim for text/binary, compact re-encode for JSON-RPC](0001-raw-output-not-verbatim-for-rpc.md) | accepted | -| 0002 | [Measurement API shape: connection methods plus free one-shot functions](0002-measurement-api-shape.md) | proposed | +| 0002 | [Measurement API shape: connection methods plus free one-shot functions](0002-measurement-api-shape.md) | accepted | +| 0003 | [JSON output schema: single monotonic version, float-ms timing precision](0003-json-output-schema-and-timing-precision.md) | accepted | diff --git a/docs/plans/check-mode-plan.md b/docs/plans/check-mode-plan.md new file mode 100644 index 0000000..ae3069d --- /dev/null +++ b/docs/plans/check-mode-plan.md @@ -0,0 +1,321 @@ +# Check Mode: Observational RFC 6455 Conformance Checks + +- **Date**: 2026-07-13 +- **Commit**: 34f69f0 +- **Branch**: various-cleanup +- **Status**: Proposed + +## Problem + +wsstat has two modes: `measure` (latency) and `stream` (subscription events). Neither answers +"does this endpoint behave like a correct WebSocket server?" The ecosystem is bimodal: the +Autobahn Testsuite runs ~500 fuzz cases against implementations you control (Docker, +Python-2-era tooling, minutes per run), while cat-style clients (websocat, wscat) do no +conformance checking at all. No tool offers a fast, polite, point-at-a-URL conformance report. + +Add a third mode, `wsstat check `, that runs a small set of **observational** RFC 6455 +checks (handshake correctness, negotiation, ping/pong, close semantics, fragmentation +tolerance) in a few seconds over a handful of connections, and reports pass/warn/fail in the +existing text/JSON output contracts. + +**Scope boundary**: Tier 1 only. Checks that require sending malformed frames (RSV bits, +reserved opcodes, invalid close codes, invalid UTF-8) are blocked by `coder/websocket` +exposing no raw-frame write path and are explicitly deferred (see Future Work). + +## Constraints + +- `coder/websocket` frame construction is private: the client always emits valid, + masked frames. Everything in Tier 1 must be expressible through its public API + (`Ping`, `Write`, `Writer`, `Close(status, reason)`) plus plain HTTP requests. +- Checks must be polite against production endpoints: every well-formed exchange, no + connection storms. Budget: ≤ 5 connections + 1 plain HTTP request, sequential. +- coder strictly validates *inbound* frames (RSV, opcodes, fragmented control frames, + close payloads) and fails the read with a close-status error. Check mode inherits this + for free: a misbehaving server surfaces as a failed check, not a crash. +- The JSON output contract is schema-pinned: `TestSchemaDocDrift` + (`internal/app/schema_doc_test.go`) asserts `docs/schema/wsstat-output-v1.schema.json` + lists every record type the code emits. A new record type must land in the same change. +- The `internal/app` package must not import `coder/websocket` directly; peer close codes + must be surfaced through the root package. + +## Design + +### 1. Check catalog + +Each check produces `pass`, `warn`, `fail`, or `skip` plus a one-line detail. Grouped: + +**Handshake** (connection 1, reused by Behavior checks) + +| ID | Check | Method | Verdict logic | +|----|-------|--------|---------------| +| `handshake.upgrade` | 101 upgrade completes | `DialContext` succeeds | fail on dial error; detail carries the HTTP status/body wsstat already captures on failed handshakes (`wsstat.go` DialContext, ~:483) | +| `handshake.accept` | `Sec-WebSocket-Accept` valid | Library-enforced: coder's `verifyServerResponse` rejects a bad accept hash during dial | pass when dial succeeds (annotate "validated during handshake") | +| `handshake.headers` | `Upgrade: websocket` / `Connection: Upgrade` tokens present in response | Inspect `Result.ResponseHeaders` (`result.go:37`) | warn on missing/miscased tokens (coder tolerates some) | + +**Negotiation** + +| ID | Check | Method | Verdict logic | +|----|-------|--------|---------------| +| `negotiation.subprotocol-none` | Server must not select a subprotocol when none offered (RFC 6455 §4.2.2) | Connection 1 dials with no `WithSubprotocols`; check `Result.Subprotocol == ""` | fail if non-empty | +| `negotiation.subprotocol-echo` | Server selects one of the offered subprotocols or none | Connection 2 offers `["wsstat-check", ]`; coder fails the dial if the server invents one | pass on empty or offered value; fail if dial rejects an invented protocol | +| `negotiation.deflate` | `permessage-deflate` response params valid (RFC 7692 §7) | Connection 3 dials with `WithCompression(true)`; parse `Result.Compression` (`result.go:35`) | pass if absent (not negotiated → informational detail) or params well-formed; warn on unknown/duplicate params, `server_max_window_bits` out of 8–15 | +| `negotiation.version-reject` | Unsupported `Sec-WebSocket-Version` rejected with the advertised-versions header (RFC 6455 §4.4) | One plain `net/http` request with `Sec-WebSocket-Version: 99` (honoring `-insecure` TLS config and custom headers) | pass on non-101 + `Sec-WebSocket-Version` response header listing 13; warn on non-101 without the header; fail on 101 | + +**Behavior** (connection 1 continued, then connections 4–5) + +| ID | Check | Method | Verdict logic | +|----|-------|--------|---------------| +| `behavior.ping-pong` | Ping answered by pong with matching payload | `WSStat.PingPong()` (`wsstat.go:603`); coder verifies the payload match internally | fail on timeout/error; detail includes RTT | +| `behavior.fragmentation` | Fragmented text message tolerated | Connection 4: new core method sends a 3-fragment text message, then `PingPong()` proves the connection survived | fail if the server kills the connection (close 1002 etc.); echo content is server-dependent and not asserted | +| `behavior.close-echo` | Close handshake completed with a valid echoed status (RFC 6455 §5.5.1, §7.4) | Connection 5: `CloseWith(1000, "")` (`wsstat.go` CloseWith) while a `ReadMessage` drains; extract the peer status from the read error | pass on echoed 1000 (or a valid registered code, warn); warn on no-echo/TCP-drop (wsstat's `gracefulClose` timeout path); skip if connection 5 fails to dial | + +Skipped-by-design (documented in `check` usage text): unsolicited-pong tolerance (coder has no +public pong-send), masking enforcement, UTF-8/RSV/opcode probes (Tier 2), limits/perf. + +### 2. Core package additions (root `wsstat`) + +Two small, generally-useful additions; no new transport. + +```go +// CloseStatus returns the RFC 6455 close status carried by an error from +// ReadMessage/ReadMessageJSON, or -1 if the error is not a close error. +// Wraps coder's websocket.CloseStatus so callers need not import coder. +func CloseStatus(err error) int + +// WriteMessageFragmented sends one text or binary message as len(fragments) +// frames (fin only on the last), synchronously, bypassing the write pump. +// Uses coder's Conn.Writer; one Write+flush per fragment. +func (ws *WSStat) WriteMessageFragmented(messageType int, fragments [][]byte) error +``` + +Notes: +- `classifyReadErr` (`wsstat.go:631`) wraps non-1000/1001 closes as "unexpected close error" + but preserves the chain, so `CloseStatus` works on its output via `errors` unwrapping + (coder's `CloseStatus` already walks the chain). +- Verify during implementation that coder's `msgWriter` emits one frame per `Write` call + (it does for uncompressed messages; with flate, fragment boundaries are not guaranteed — + run the fragmentation check on a connection dialed without compression). +- Verify that after `CloseWith`, the read pump surfaces the peer's close echo as an error on + `readChan` before coder's internal `waitCloseHandshake` swallows it. If it races, add a + `ReceivedCloseStatus() int` accessor populated in `readPump` (`wsstat.go:312`) instead; + decide by test, prefer the error-based route. + +### 3. App layer (`internal/app`) + +New file `internal/app/check.go`: + +```go +type CheckStatus int // CheckPass, CheckWarn, CheckFail, CheckSkip + +type CheckEntry struct { + ID string // e.g. "behavior.ping-pong" + Group string // "handshake" | "negotiation" | "behavior" + Status CheckStatus + Detail string // one line; "" when self-evident + Took time.Duration +} + +type CheckReport struct { + Target *url.URL + Entries []CheckEntry + // Passed/Warned/Failed/Skipped counts derived, not stored +} + +// RunCheck executes the Tier 1 catalog sequentially and always returns a +// report; a dial failure fails the dependent checks and skips the rest of +// that connection's checks rather than aborting. +func (c *Client) RunCheck(ctx context.Context, target *url.URL) (*CheckReport, error) +``` + +- `RunCheck` parallels `MeasureLatency` (`internal/app/client.go:363`): builds `wsstat.New` + instances from `c.wsstatOptions()` (`client.go:304`), one per connection in the catalog, + each closed before the next dials. The version-reject probe uses `net/http` with the + client's TLS config and headers. +- Per-check timeout: the existing `-timeout` (dial) plus a per-check cap derived from it; + a hung server must not stall the run past `~4×timeout`. +- Add `ModeCheck` to the `Mode` enum (`internal/app/format.go:33`) and a `case ModeCheck` + in `Client.Validate()` (`client.go:401`) rejecting measure/stream-only options + (`-c`, `--once`, `-b`, `--summary-interval`, `-t`, `--rpc-method`). +- The error return is for runtime failures only (bad target, ctx canceled). Check verdicts + live in the report. + +### 4. Output + +**Text** — `PrintCheckResults(report *CheckReport) error` in `internal/app/output.go`, +following the printer pattern (`PrintTimingResults`, `output.go:646`): branch on `c.output`, +reuse `colorizeGreen`/`colorizeOrange` (`output.go:83-91`) plus red for fail. Layout: + +``` +Handshake + ✓ 101 Switching Protocols + ✓ Sec-WebSocket-Accept valid (validated during handshake) + ✗ Connection header missing "Upgrade" token +Negotiation + ✓ subprotocol: none offered, none selected + ⚠ permessage-deflate: server ignores client_max_window_bits + ✓ unsupported version rejected (426, advertises 13) +Behavior + ✓ ping → pong (12ms) + ✓ fragmented text accepted + ✓ close 1000 echoed, clean shutdown + +8 passed, 1 warning, 1 failed +``` + +ASCII fallback (`ok`/`warn`/`FAIL`) when color is off, matching the existing color-mode +plumbing (`ColorMode`, `client.go:292`). `-v` adds `Detail` and `Took` per line; `-q` prints +the summary line only. + +**JSON** — add to `internal/app/types.go`, mirroring `subscriptionSummaryJSON` (:78): + +```go +type checkEntryJSON struct { + ID string `json:"id"` + Group string `json:"group"` + Status string `json:"status"` // "pass" | "warn" | "fail" | "skip" + Detail string `json:"detail,omitempty"` + TookMs float64 `json:"took_ms"` +} + +type checkReportJSON struct { + Schema string `json:"schema_version"` + Type string `json:"type"` // "check_report" + URL string `json:"url"` + Checks []checkEntryJSON `json:"checks"` + Passed int `json:"passed"` + Warned int `json:"warned"` + Failed int `json:"failed"` + Skipped int `json:"skipped"` +} +``` + +One `check_report` object via `printJSONLine` (`output.go:99`). Update +`docs/schema/wsstat-output-v1.schema.json` (add the `check_report` record) in the same +change so `TestSchemaDocDrift` passes. `-o raw` is rejected at validation (no raw payload +in this mode). + +### 5. CLI (`cmd/wsstat`) + +- `main.go`: add `case args[0] == "check": err = runCheck(args[1:])` to the dispatch switch + (:117-133); add `buildCheck`/`runCheck` mirroring `buildMeasure` (:232) / `runMeasure` + (:379). `buildCheck` registers only `registerCommon` + `registerRemoved` (no + check-specific flags in v1) and calls `resolveCommon(fs, &cf, app.ModeCheck)`. +- Exit codes (`main.go:50`): add `exitCheckFailed = 3` — `runCheck` returns it when any + check is `fail` (warnings exit 0). Runtime errors keep `exitRuntime = 1`. This makes + `wsstat check` usable as a CI gate. +- `usage.go`: `printCheckUsage`, a `case "check"` in `printHelpFor` (:12), a line in + `printTopUsage` command/description/examples blocks (:32-58), and the new exit code in + the exit-codes block (:45). + +### 6. Name + +`check`, not `validate`: shorter, and `validate` collides with `Client.Validate()`. + +## Affected Files + +| File | Change | +|------|--------| +| `wsstat.go` | `CloseStatus(err)` helper, `WriteMessageFragmented` | +| `internal/app/check.go` | new: check catalog, `CheckReport`, `RunCheck` | +| `internal/app/format.go` | `ModeCheck` | +| `internal/app/client.go` | `Validate()` branch for `ModeCheck` | +| `internal/app/types.go` | `checkReportJSON`, `checkEntryJSON` | +| `internal/app/output.go` | `PrintCheckResults` | +| `cmd/wsstat/main.go` | dispatch case, `buildCheck`, `runCheck`, `exitCheckFailed` | +| `cmd/wsstat/usage.go` | check usage, top usage, exit codes | +| `docs/schema/wsstat-output-v1.schema.json` | `check_report` record | +| `internal/app/check_test.go`, `wsstat_test.go`, `cmd/wsstat/main_test.go` | tests | +| `README.md`, `CHANGELOG.md`, `CLAUDE.md`/`AGENTS.md` | docs (phase 4) | + +## Implementation Phases + +### Phase 1 — Core primitives + +1. `CloseStatus(err error) int` in `wsstat.go` + test: dial the echo server, `CloseWith(1000)`, + assert the drained read error yields 1000. This test also settles the close-echo race + noted in Design §2; fall back to `ReceivedCloseStatus()` if it proves unreliable. +2. `WriteMessageFragmented` + test against the shared echo server (`TestMain` / + `echoServerAddrWs`): send 3 fragments, read one reassembled echo. Verify per-`Write` + framing with a raw-frame-recording test handler if the echo server reassembles silently. + +### Phase 2 — Check runner + +3. `internal/app/check.go`: catalog, runner, per-connection sequencing, version-reject HTTP + probe. Table-driven tests with purpose-built `httptest` handlers per misbehavior: + - well-behaved echo server → all pass + - handler selecting an unoffered subprotocol → `negotiation.subprotocol-echo` fail + - handler returning malformed `Sec-WebSocket-Extensions` → `negotiation.deflate` warn + - handler answering 101 to `Sec-WebSocket-Version: 99` → `negotiation.version-reject` fail + - handler dropping TCP instead of close handshake → `behavior.close-echo` warn + - unreachable port → `handshake.upgrade` fail, dependent checks skip, runner still + returns a full report +4. `ModeCheck` + `Validate()` rejections (flag-combination tests alongside + `client_validation_test.go`). + +### Phase 3 — Output and CLI + +5. `PrintCheckResults` text/JSON + `checkReportJSON`; update the published schema doc; + `TestSchemaDocDrift` green. Output tests mirror `client_output_test.go`. +6. CLI wiring: dispatch, `buildCheck`/`runCheck`, exit code 3, usage text. E2E test in + `cmd/wsstat` against a local echo server asserting exit codes 0 (all pass) and 3 (a fail). +7. `make lint && make test`. + +### Phase 4 — Documentation + +8. `README.md`: extend Modes (~:165) with `check`; add usage examples (`wsstat check + wss://echo.example.com`, `-o json` sample `check_report` line); document exit code 3 in + Exit Codes (~:237); note the mode under the CLI feature list. +9. `CHANGELOG.md`: `## [Unreleased]` → Added: `check` mode entry. +10. `CLAUDE.md` and `AGENTS.md`: update the app-layer description ("orchestrates + measurement/subscription flows") to include check; `docs/schema/README.md` if it + enumerates record types. + +## Edge Cases + +- **Dial failure**: `handshake.*` fail with the captured HTTP status/body; all + connection-dependent checks report `skip`; exit 3. +- **ws:// targets**: all checks apply unchanged (no TLS-specific checks in the catalog). +- **Servers that close idle connections fast**: each connection performs its checks + immediately after dial; no sleeps between checks. +- **Compression interplay**: the fragmentation connection always dials with compression off + (fragment boundaries are undefined under permessage-deflate). +- **Ctrl-C**: ctx cancellation mid-run prints the report for completed checks, remaining as + `skip`, and exits 130 (existing signal path, `main.go:226`). +- **Non-echo servers**: no check asserts echoed payloads; `behavior.fragmentation` proves + liveness via a follow-up ping, not echo content. + +## Acceptance Criteria + +- `wsstat check wss://` prints the grouped report, all pass, exit 0. +- `wsstat check -o json ` emits exactly one `check_report` line conforming to the + updated schema; `TestSchemaDocDrift` passes. +- A server with any `fail` verdict → exit 3; runtime error (DNS failure with unreachable + host) → exit 1; bad flags → exit 2. +- Full run against a live endpoint completes in ≤ ~4× the configured `-timeout`, ≤ 5 + WebSocket connections + 1 HTTP request. +- `make lint && make test` (race, 16×) green; no `coder/websocket` import in `internal/app`. +- README, CHANGELOG, agent docs updated. + +## Future Work — Tier 2 Adversarial Probes (NOT to be implemented now) + +Autobahn-grade conformance requires sending malformed frames and asserting the server +closes with 1002/1007: RSV bits without a negotiated extension, reserved opcodes, +fragmented/oversized control frames, invalid and reserved close codes, unmasked client +frames, invalid UTF-8 (fail-fast) in text frames, interleaved fragmentation violations, +unsolicited pongs. + +This is blocked by `coder/websocket`: frame construction (`writeFrame`, opcodes, RSV, +masking) is unexported and there is no client-side hijack. Tier 2 therefore needs a raw +transport path: manual HTTP/1.1 upgrade + a minimal hand-rolled RFC 6455 frame codec over +the raw `net.Conn`. wsstat already captures that conn post-TLS (`captureNetConn`, +`wsstat.go:201`), so the dial/TLS/timing instrumentation is reusable, but the codec and a +parallel read path are a substantial new layer (~the size of the entire Tier 1 change). + +Design intent when it happens: `wsstat check --probe `, opt-in because firing +malformed frames at production endpoints is borderline hostile; one fresh connection per +probe; probe results appear as a fourth report group (`probe.*`) in the same `check_report` +schema (additive, no schema version bump). Letter grades (SSL-Labs style) stay out of scope +until the check set stabilizes. + +Tier 1's report schema and exit-code contract are designed so Tier 2 slots in without +breaking changes; nothing in this plan should be blocked or complicated to accommodate it. diff --git a/docs/plans/ci-release-hardening-plan.md b/docs/plans/ci-release-hardening-plan.md new file mode 100644 index 0000000..1592e95 --- /dev/null +++ b/docs/plans/ci-release-hardening-plan.md @@ -0,0 +1,218 @@ +# CI & Release Hardening + +**Date:** 2026-07-10 +**Commit:** 68c92e0 (various-cleanup) + +## Problem Statement + +The release workflow holds the repo's most sensitive capabilities — `contents: write` and the snap store credential — while every third-party action it runs is pinned to a mutable major tag. A compromised tag on any of those actions can push tags/releases or exfiltrate `SNAPCRAFT_STORE_CREDENTIALS`. Around that core risk sit four smaller gaps, all of the same "duplicated fact without a drift guard" class this repo otherwise defends against: + +- Four quality-gate jobs are duplicated verbatim between `ci-tests.yml` and `release.yml` (~120 lines of drift surface). +- `scripts/check-go-version.sh` runs only at release time, so Go-version drift merges green on PRs and fails weeks later when a release is cut. +- The VERSION validation regex accepts `-rc.1`/`+build` suffixes its own comment says it rejects, which would mint malformed tags. +- The smoke/soak harness — which `dev/README.md` calls part of a flag's definition of done — never runs in CI; the exit-code contract, JSON error envelope, and SIGINT behavior are unverified on every PR. + +**Constraints:** + +- The release flow's rerun-resilience (tag reconciliation, idempotent re-tagging) must be preserved untouched. +- CI runtime budget: the smoke suite must stay a fast gate (< ~1 min including mock-server startup); soak stays manual/nightly. +- No new external services or secrets. + +## Non-Goals + +- Release artifact strategy changes (multi-platform binaries, checksums) — separate discussion. +- Soak test in CI — too slow for the PR gate; revisit as a nightly if flakes appear. +- OIDC/`id-token` hardening for the snap upload — snapcraft auth is credential-based today. + +## Proposed Solution + +Six independent changes, each committable on its own: + +1. **Pin third-party actions to commit SHAs** in `release.yml`; pin `govulncheck` to a version in both workflows. +2. **Scope `permissions` per job** — workflow-level `contents: read`, `write` only where tagging/publishing happens. +3. **Fix the VERSION regex** to match its comment (bare `X.Y.Z` only). +4. **Run `check-go-version.sh` on PRs** as a step in the existing lint job. +5. **Extract the quality gates** into a reusable `workflow_call` workflow consumed by both `ci-tests.yml` and `release.yml`. +6. **Run the smoke suite in CI without Docker** via a native mode in `dev/run.sh` (the mock server is a self-contained Go binary that generates its own TLS cert). + +## Detailed Design + +### 1) Pin actions and tools + +**Goal:** No mutable tag resolves inside the workflow that holds write permissions and store credentials. + +As of 68c92e0 the third-party pins in `.github/workflows/release.yml` are: + +- `orhun/git-cliff-action@v4` — lines 341, 351 +- `softprops/action-gh-release@v2` — line 361 +- `snapcore/action-build@v1` — line 388 +- `snapcore/action-publish@v1` — line 391 + +Replace each with the full commit SHA of the current release, keeping the tag as a comment for readability: + +```yaml +uses: softprops/action-gh-release@<40-char-sha> # v2.x.y +``` + +First-party `actions/checkout@v6`, `actions/setup-go@v6`, and `golangci/golangci-lint-action@v9` may stay on tags (lower risk, and SHA-pinning them adds update friction for little gain) — but pinning them too is fine if consistency is preferred. + +Also pin the vulnerability scanner in **both** workflows (`release.yml:158`, `ci-tests.yml:60`): + +```yaml +- run: go install golang.org/x/vuln/cmd/govulncheck@v1.1.4 # was @latest +``` + +Optionally add Dependabot config (`.github/dependabot.yml`, `package-ecosystem: github-actions`) so SHA pins get PR-driven updates instead of rotting. + +### 2) Per-job permissions + +**Goal:** Only the jobs that tag/publish hold `contents: write`. + +`release.yml:13-14` currently sets workflow-level `permissions: contents: write`, inherited by every job including lint and tests. Change to: + +```yaml +permissions: + contents: read # workflow-level default + +jobs: + release: # tags via gh api (release.yml:267-326) + gh-release publish + permissions: + contents: write + snap: # needs only the checkout + the store secret + permissions: + contents: read +``` + +The `snap` job (release.yml:375-396) needs no `write` — `SNAPCRAFT_STORE_CREDENTIALS` (line 393) is a secret, not a token permission. Verify with a full dry run (workflow_dispatch with prerelease) that `gh api` tag reconciliation still works under the job-scoped token. + +### 3) VERSION regex + +**Goal:** The regex enforces what the comment at `release.yml:50` states ("we only accept bare X.Y.Z for base version"). + +Line 51, in the `version-bump-check` job: + +```bash +# was: ^[0-9]+(\.[0-9]+){2}([\-+][0-9A-Za-z\.-]+)?$ +[[ "$version" =~ ^[0-9]+(\.[0-9]+){2}$ ]] +``` + +Without this, a suffixed VERSION file (e.g. `3.2.0-rc.1`) passes validation, behaves oddly in the `sort -V` bump comparison, and flows into `compute_tag` as the base version — minting `v3.2.0-rc.1` as a "final" tag or `v3.2.0-rc.1-rc.1` with the prerelease box ticked. RC numbering is the workflow input's job, not the file's. + +### 4) Go-version alignment on PRs + +**Goal:** Go-version drift fails the PR that introduces it, not the release weeks later. + +`scripts/check-go-version.sh` verifies go.mod's `toolchain` line against both workflows' `GO_VERSION` and the snapcraft Go tarball. It is wired only into `release.yml:85-91` (`go-version-align` job). Add one step to the existing `golangci` job in `ci-tests.yml` (lines 16-28): + +```yaml + - name: Go version alignment + run: bash scripts/check-go-version.sh +``` + +Keep the release-side job as belt-and-braces (it costs seconds), or drop it once the reusable workflow (component 5) carries the check. + +### 5) Reusable quality-gate workflow + +**Goal:** One definition of the quality gates, consumed by both CI and release. + +The duplicated pairs as of 68c92e0: + +| ci-tests.yml | release.yml | +|---|---| +| `golangci` (16-28) | `golangci-lint` (127-143) | +| `unit-tests` (30-39) | `test-race` (93-108) | +| `unit-tests-flaky` (41-50) | `test-flaky` (110-125) | +| `govulncheck` (52-61) | `govulncheck` (145-159) | + +Create `.github/workflows/quality-gates.yml`: + +```yaml +name: Quality gates +on: + workflow_call: + inputs: + fetch-depth: + type: number + default: 1 +jobs: + golangci: ... # moved verbatim, plus the check-go-version step + unit-tests: ... + unit-tests-flaky: ... + govulncheck: ... +``` + +- `ci-tests.yml` becomes: trigger block + `uses: ./.github/workflows/quality-gates.yml` + the `summary` job (`needs` the called workflow's job via `needs: [quality]`). +- `release.yml` replaces its four duplicated jobs with the same `uses:` and points the downstream `needs:` at it. +- `env` values (`GO_VERSION`, `GOLANGCI_LINT_VERSION`) move into the reusable workflow; `check-go-version.sh` keeps them honest against go.mod. + +Gotcha: workflow-level `env` is not inherited across `workflow_call` — define the env inside quality-gates.yml, not in the callers. + +### 6) Smoke tests in CI (Docker-free) + +**Goal:** The exit-code contract, JSON envelope, TLS verify path, and validation-rejection behavior run on every PR. + +The mock server (`dev/mock-server/main.go`) is a standalone Go module that generates its own self-signed cert in-process (`selfSignedCert()`, lines 96-130), serves it at `/ca.pem`, and takes `PORT`/`TLS_PORT` env overrides. `smoke-test.sh` already accepts `WS_URL`/`WSS_URL`/`WSSTAT` env overrides (lines 11-13) and bootstraps TLS trust from `/ca.pem` itself. Docker adds nothing but the healthcheck wait. + +Add a `native` mode to `dev/run.sh` (which currently hard-wires `docker compose`): + +```bash +# dev/run.sh native [soak] +start_native() { + (cd dev/mock-server && go build -o "$BIN_DIR/mock-ws" .) + PORT=17080 TLS_PORT=17443 "$BIN_DIR/mock-ws" & + MOCK_PID=$! + trap 'kill "$MOCK_PID" 2>/dev/null' EXIT + for _ in $(seq 1 50); do + curl -fsS http://localhost:17080/healthz >/dev/null 2>&1 && return + sleep 0.1 + done + echo "mock server failed to become healthy" >&2; exit 1 +} +``` + +Then a Makefile target `smoke-native` and a CI job in quality-gates.yml (or ci-tests.yml directly): + +```yaml + smoke: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - uses: actions/setup-go@v6 + - run: make build + - run: make smoke-native +``` + +PTY-dependent checks (`--clip`/`--color` via `dev/pty-run.py`) need python3, present on ubuntu runners; if they prove flaky in CI, gate them behind an env var (`SMOKE_SKIP_PTY=1`) rather than dropping the job. + +## Affected Files + +- `.github/workflows/release.yml` (modify) — SHA pins, per-job permissions, regex, jobs replaced by `workflow_call` +- `.github/workflows/ci-tests.yml` (modify) — govulncheck pin, check-go-version step, jobs replaced by `workflow_call`, smoke job +- `.github/workflows/quality-gates.yml` (new) — reusable gates +- `.github/dependabot.yml` (new, optional) — github-actions ecosystem updates +- `scripts/check-go-version.sh` (unchanged; new call site) +- `dev/run.sh` (modify) — `native` mode +- `Makefile` (modify) — `smoke-native` target +- `CHANGELOG.md` — `(ci)` entries under Unreleased + +## Implementation Phases + +1. **Quick wins, zero behavior risk** — regex fix (3), govulncheck pins, check-go-version on PRs (4). One commit. +2. **Supply chain** — SHA pins + dependabot (1), per-job permissions (2). Verify with a prerelease dry run of the release workflow. +3. **Structure** — reusable quality-gates workflow (5). Verify both callers go green on a PR and a dry-run dispatch. +4. **Smoke in CI** — native mode + CI job (6). Land last; it has the only real flake risk. + +## Acceptance Criteria + +- `grep -E 'uses:.*@(v[0-9]|main|master)' .github/workflows/release.yml` matches only `actions/*` and `golangci/*` (or nothing, if pinning those too). +- No workflow-level `contents: write` remains; `release` is the only job with it. +- A PR that bumps `GO_VERSION` in one place fails CI. +- `echo "3.2.0-rc.1" > VERSION` fails `version-bump-check` on a dry run. +- The four gate jobs exist in exactly one file. +- A PR run shows the smoke job passing in under ~90s. +- A full prerelease dispatch of release.yml succeeds end to end after all changes. + +## Open Questions + +- Keep release re-running the full gates via the reusable workflow (current behavior, belt-and-braces) or switch to asserting the CI run for `$GITHUB_SHA` succeeded? Plan assumes the former. +- Pin first-party `actions/*` to SHAs as well? Cheap with Dependabot in place; default here is tags-only. diff --git a/docs/plans/v4-api-plan.md b/docs/plans/v4-api-plan.md new file mode 100644 index 0000000..cc9eb91 --- /dev/null +++ b/docs/plans/v4-api-plan.md @@ -0,0 +1,210 @@ +# wsstat v4 API Changes + +**Date:** 2026-07-10 +**Commit:** 68c92e0 (various-cleanup) + +## TL;DR + +Several library behaviors are honest only for the wsstat CLI's lockstep dial → write → read → close pattern: writes swallow errors at debug level, the subscription demux hooks are unexported and therefore unusable, "connection went away" surfaces as three different error shapes, and an idle connection self-destructs after the read timeout. v4 fixes the public contract (writes return errors, exported subscription hooks, sentinel-based error surface, idle keepalive) and pays down the internal/app option ceremony. Ships as `github.com/jkbrsn/wsstat/v4` with a migration table in the CHANGELOG in the 3.0.0 style. + +## Problem Statement + +The v3 library works correctly for the CLI but misleads external importers: + +1. **Silent write failures.** `WriteMessage`/`WriteMessageJSON` (wsstat.go:569, 583) return nothing; a JSON marshal failure or a drop-during-close is logged at debug and vanishes. For a measurement library, a failed send is a result the caller needs. +2. **Unreachable subscription demux.** `SubscriptionOptions.decoder`/`matcher` (subscription.go:47, 51) are unexported with no setters; only same-package tests can populate them. External callers can run exactly one subscription — `dispatchIncoming` delivers matcherless frames only when `len(states) == 1` (subscription.go:374, 394) — and registering a second silently routes frames to *neither*. +3. **Inconsistent error surface.** The package launders message types into stable ints so the transport can swap (wsstat.go:57-65), yet: `ReadMessage` passes raw coder close errors through (`classifyReadErr`, wsstat.go:631-641), forcing callers to import coder/websocket to classify them; a read blocked at Close time returns `ws.ctx.Err()` = `context.Canceled` (wsstat.go:678) instead of `ErrClosed`; `Subscribe` returns an ad-hoc `errors.New("websocket connection is not established")` (subscription.go:158-160) instead of the existing `ErrConnectionNotEstablished` sentinel. +4. **Bounded idle lifetime.** With no active subscription, `readFrame` applies `ws.timeout` (default 5s) to every read (wsstat.go:375-390); a deadline hit takes the pump's error path and closes the connection (wsstat.go:334-340). "Dial, wait 6s, ping" fails with default config, undocumented. +5. **internal/app ceremony.** 25 `WithX` options (client.go:129-267) plus 10 getters (client.go:271-301) exist so the CLI can read back values it just set; count/once validation is duplicated between `Validate` (client.go:433-441) and cmd/wsstat (main.go:256, 299-305). `StreamSubscriptionOnce` duplicates `StreamSubscription` via a `c.count` defer-restore hack (subscription.go:229-231) and has already drifted (a blank line escapes the quiet guard, subscription.go:106-110). The output layer hardcodes stdout across ~55 call sites with a split write-error policy (JSON propagates, output.go:99-108; text swallows, e.g. output.go:127). +6. **Result formatting.** `printURLAndIPSection` dereferences `r.URL` without a nil check (result.go:128-142) — garbled `%!v(PANIC=...)` output on a zero-value Result — and all durations truncate to integer milliseconds (result.go:95, 193-204), so a latency tool prints "0 ms" on localhost. + +**Constraints:** + +- Module path bumps to `/v4`; v3 stays tagged and importable. +- The CLI (cmd/wsstat, internal/app) migrates in the same change — it is the reference consumer. +- Behavior the CLI's smoke/soak matrix pins (output contracts, exit codes) must not change; this is a library-surface release. +- Component 6 is API-compatible and may ship in a v3 patch first; it is listed here so it isn't forgotten. + +## Non-Goals + +- Reconnect/redial support — `WSStat` stays single-use (guard added in 68c92e0). +- Synchronous per-write acks (blocking until `conn.Write` returns) — would serialize pipelined writers; the enqueue contract below is deliberate. +- Promoting internal/app to a public package. +- New transports; coder/websocket remains the engine. + +## Detailed Design + +### 1) Writes return errors; wire-time write stamps + +**Goal:** A failed send is observable, and write timings mark wire time, not enqueue time. + +New signatures (wsstat.go): + +```go +// WriteMessage queues a message for delivery. A nil return means the message was +// queued (delivery is asynchronous); ErrClosed means it was dropped because the +// connection is closing and was never sent. +func (ws *WSStat) WriteMessage(messageType int, data []byte) error + +// WriteMessageJSON is WriteMessage for JSON payloads; a marshal failure is +// returned and nothing is queued. +func (ws *WSStat) WriteMessageJSON(v any) error +``` + +Steps: + +1. `enqueueWrite` (wsstat.go:547-563) already returns `bool`; map `false` → `ErrClosed` in both public methods. +2. Move the write-timing stamp from the public methods into `writePump`, around the successful `conn.Write` (wsstat.go:419-425 region): append to `timings.messageWrites` only after `conn.Write` returns nil. Enqueue order equals pump processing order (single channel, single consumer), so ledger pairing is preserved and a full `writeChan` no longer backdates RTT for pipelined callers. +3. A `conn.Write` failure already ends the pump and triggers Close; keep that, but raise the log from debug to warn — it is now the only place a wire-level write failure is recorded. +4. Update `examples/main.go`, internal/app call sites, and godoc ("Sets time: MessageWrites" comments). + +### 2) Exported subscription demux hooks + +**Goal:** Multi-subscription demux is usable, or a second subscription is an explicit error — never silent misrouting. + +Export the two hook types and fields (subscription.go:30, 34, 47, 51): + +```go +// Decoder optionally decodes a frame before matching/delivery. +type Decoder func(messageType int, data []byte) (any, error) + +// Matcher reports whether this subscription claims the frame. +type Matcher func(messageType int, data []byte, decoded any) bool + +type SubscriptionOptions struct { + ID string + MessageType int + Payload []byte + Decoder Decoder // was: decoder subscriptionDecoder + Matcher Matcher // was: matcher subscriptionMatcher + Buffer int +} +``` + +Steps: + +1. Rename the unexported types/fields; `Subscribe` (subscription.go:180-181) and `dispatchIncoming` need only the field renames. +2. Guard the silent-misroute hole: in `Subscribe`, if another matcherless subscription is already active and the new one is also matcherless, return a new sentinel `ErrSubscriptionConflict` ("a second subscription requires a Matcher"). The `deliverAll := len(states) == 1` fallback (subscription.go:374) stays for the single-subscription case. +3. Fix the doc comment advertising "internal matching heuristics (such as explicit IDs)" — describe what the code does. +4. Same-package tests that set `decoder:`/`matcher:` literals update mechanically. + +### 3) Coherent error surface + +**Goal:** Callers classify every failure with `errors.Is` against package sentinels and never import coder/websocket. + +Add to the sentinel block (wsstat.go:69-75): + +```go +// ErrNormalClosure is returned by reads when the peer closed the connection +// cleanly (close status 1000/1001). +var ErrNormalClosure = errors.New("wsstat: connection closed by peer") +``` + +Steps: + +1. `classifyReadErr` (wsstat.go:631-641): wrap normal/going-away closes as `fmt.Errorf("%w: %w", ErrNormalClosure, err)` instead of passing raw coder errors through; keep the "unexpected close error" wrap for other statuses (consider a second sentinel `ErrUnexpectedClosure` so that class is also branchable). +2. `ReadMessage`'s ctx-done branch (wsstat.go:678): return `ErrClosed` instead of `ws.ctx.Err()`. +3. `Subscribe` (subscription.go:158-160): return `ErrConnectionNotEstablished`. +4. Audit remaining `errors.New` returns in the root package for the same treatment. +5. Regression boundary: `TestReadMessageDoesNotMaskCloseError` and `TestCloseHandshakeStatus` in wsstat_test.go assert the current shapes — update them deliberately as part of this component, not incidentally. + +### 4) Idle connections survive + +**Goal:** A quiet connection with no subscription stays open; the read deadline becomes a keepalive tick. + +`readFrame` already distinguishes a deadline hit from a real error via `deadlineHit` (wsstat.go:388). In `readPump`'s error path (wsstat.go:334-340), the subscription case continues the loop; extend the same treatment to the no-subscription case **when no consumer is blocked in ReadMessage** — a deadline hit with a pending read must still surface as a timeout error. + +Sketch: track pending reads with an `atomic.Int64` incremented on entry to `ReadMessage`/`ReadMessageJSON`'s blocking select and decremented on exit: + +```go +if deadlineHit && ws.pendingReads.Load() == 0 { + continue // idle tick: nothing was waiting for this frame +} +``` + +Document on `WithTimeout` (and `DialContext`) that the timeout bounds dial and each in-flight read, not connection lifetime. Add a test mirroring `TestSubscriptionSurvivesIdleBeyondTimeout` for the no-subscription case: dial with a shrunk timeout, sleep past it, `PingPong` must succeed. + +### 5) internal/app: Config struct, single stream path, injected writer + +**Goal:** The app layer stops wearing a public-library costume; one validation site, one output writer, one write-error policy. + +1. **Config struct.** Replace `NewClient(opts ...Option)` (client.go:114-126), the 25 options, and the getters with: + + ```go + type Config struct { + Count int; Headers []string; Resolves []string + RPCMethod, RPCVersion, Text string + Output Output; ResponseFile string; Body Body; Clip bool + ShowSecrets bool; ColorMode string; Quiet bool; Verbosity int + Mode Mode; Once bool; Buffer int; SummaryInterval time.Duration + Insecure bool; Timeout, CloseGrace time.Duration; ReadLimit int64 + Subprotocols []string; ValidateUTF8, Debug bool + DebugW io.Writer; Out io.Writer // nil → os.Stderr / os.Stdout + } + func NewClient(cfg Config) (*Client, error) // absorbs Validate() + ``` + + cmd/wsstat fills the struct directly from flags; the duplicated count/once checks (main.go:256, 299-305 vs client.go:433-441) collapse into `NewClient`. The few getters the CLI genuinely needs (`Output()`, `ResponseFilePath()`, `Once()` at main.go:360, 388, 426, 433) either stay as the only survivors or the CLI reads its own flag values. + +2. **Delete `StreamSubscriptionOnce`** (subscription.go:239-263). `NewClient` sets `Count = 1` when `Once` is set; `runStream` calls `StreamSubscription` unconditionally. Gate the banner on `!c.once` if the once path's missing banner is intentional. This removes the defer-restore mutation (subscription.go:229-231) and fixes the `-q` blank-line drift (subscription.go:106-110) in one move. + +3. **Inject `Out io.Writer`.** Route all ~55 direct `fmt.Print*`/`os.Stdout` sites in output.go through `c.out` (mirroring `debugW`, client.go:107). Retire `captureStdoutFrom` (testing_helpers_test.go:22); output tests assert against a `bytes.Buffer` and can run `t.Parallel`. + +4. **One write-error policy: propagate.** Text-path prints adopt the JSON path's behavior (output.go:99-108): return write errors so EPIPE exits non-zero under every output mode. Also fix `runSubscriptionLoop`'s closed-channel busy-spin (subscription.go:88-90): set the drained channel variable to nil on `!ok` instead of `continue`. + +### 6) Result formatting (v3-patch eligible) + +**Goal:** The formatter never garbles and sub-millisecond latencies are visible. + +- Nil-check `r.URL` in `printURLAndIPSection` (result.go:128-142); print `` or skip the section. +- Replace integer-ms truncation (result.go:95, 193-204) with the CLI's precision approach (up to 3 decimals; internal/app/formatting.go already does this) so `%+v` on a Result matches the tool's own standard. + +## Affected Files + +- `wsstat.go` — write signatures, sentinels, classifyReadErr, ReadMessage ctx branch, readPump idle tick, writePump stamping +- `subscription.go` (root) — exported Decoder/Matcher, Subscribe sentinel + conflict guard +- `result.go` — nil guard, duration precision +- `measure.go`, `examples/main.go` — adapt to write errors +- `wsstat_test.go`, `measure_test.go` — new tests + deliberate updates to close-error assertions +- `internal/app/client.go` — Config struct, NewClient, getter removal +- `internal/app/subscription.go` — StreamSubscriptionOnce deletion, loop fix +- `internal/app/output.go`, `formatting.go` — injected writer, unified error policy +- `internal/app/*_test.go` — buffer-based output assertions, helper retirement +- `cmd/wsstat/main.go`, `config.go` — Config construction, validation consolidation +- `go.mod` — module path `/v4` +- `CHANGELOG.md`, `README.md`, `docs/architecture/overview.md` — v4 entry with migration table, import-path updates + +## Implementation Phases + +Each phase leaves the tree green; 1–2 are mechanical, 3–4 carry the behavior risk, 5 is the big internal diff. + +1. **Component 6** on v3 (patch release) — no API change. +2. **Module bump** to `/v4` + component 3 (error surface) — small diff, touches the most tests. +3. **Component 1** (write errors + pump stamping) — update all call sites in one commit. +4. **Components 2 and 4** (demux export, idle tick) — independent; either order. +5. **Component 5** (internal/app rework) — largest diff, zero external surface. +6. **Docs pass** — CHANGELOG migration table (v3 → v4: signature changes, new sentinels, idle-behavior change), README library section, ADR if the error-surface decision warrants one. + +## Edge Cases & Safety + +- **Pump-stamped writes vs ExtractResult:** the stamp now lands after `conn.Write`, so a snapshot taken between enqueue and write sees fewer writes — acceptable; the ledger only ever contains messages that reached the wire. +- **Idle tick vs pending read:** without the `pendingReads` guard, a caller blocked in `ReadMessage` on a dead-silent server would wait forever instead of timing out. The guard preserves the timeout contract for active readers. +- **`ErrNormalClosure` wrapping:** wrap (`%w: %w`), don't replace — `websocket.CloseStatus` must still work on the chain for anyone who does drop to coder types. +- **Subscription conflict guard:** check-and-register must be atomic under `subscriptionMu` to avoid two racing matcherless Subscribes both passing the check. +- **Race exposure:** every component touches pump/consumer seams; the 16x shuffled `-race` CI job is the backstop — run `make test RACE=1` locally per phase. + +## Acceptance Criteria + +- `grep -rn 'coder/websocket' examples/ internal/app/` shows no imports needed for error classification (message-type ints and sentinels suffice). +- A dropped or failed write is observable by the caller (error return) — no debug-only failure paths remain on the write surface. +- Two subscriptions with matchers both receive their frames (new test); two matcherless subscriptions fail fast with `ErrSubscriptionConflict`. +- Dial + idle sleep past timeout + `PingPong` succeeds; a blocked `ReadMessage` against a silent server still times out. +- `internal/app` has no functional options and no stdout-swap test helper; output tests run under `t.Parallel`. +- `fmt.Sprintf("%+v", &wsstat.Result{})` contains no `PANIC` marker; sub-ms RTTs render with decimals. +- CHANGELOG carries a v3→v4 migration table; smoke + soak matrices pass unchanged. + +## Open Questions + +- Fold clean-close into `ErrClosed` instead of a separate `ErrNormalClosure`? Separate is proposed: "we closed" vs "peer closed" is a distinction measurement callers act on. +- Should `WriteMessage` optionally block until wire-write (a `WriteMessageSync` or option) for callers who want hard delivery confirmation? Deferred unless a concrete consumer asks. +- Keep any getters on `app.Client`, or have cmd/wsstat track the three values it needs from its own flags? Cosmetic; decide at implementation. diff --git a/go.mod b/go.mod index df63834..01e7232 100644 --- a/go.mod +++ b/go.mod @@ -2,20 +2,20 @@ module github.com/jkbrsn/wsstat/v3 go 1.26 -toolchain go1.26.4 +toolchain go1.26.5 require ( github.com/coder/websocket v1.8.15 github.com/mattn/go-isatty v0.0.22 github.com/rs/zerolog v1.35.1 github.com/stretchr/testify v1.11.1 - golang.org/x/term v0.44.0 + golang.org/x/term v0.45.0 ) require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/mattn/go-colorable v0.1.15 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - golang.org/x/sys v0.46.0 // indirect + golang.org/x/sys v0.47.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 0e51291..570049e 100644 --- a/go.sum +++ b/go.sum @@ -12,10 +12,10 @@ github.com/rs/zerolog v1.35.1 h1:m7xQeoiLIiV0BCEY4Hs+j2NG4Gp2o2KPKmhnnLiazKI= github.com/rs/zerolog v1.35.1/go.mod h1:EjML9kdfa/RMA7h/6z6pYmq1ykOuA8/mjWaEvGI+jcw= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= -golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= -golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= -golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc= -golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= +golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/internal/app/testing_helpers.go b/internal/app/testing_helpers_test.go similarity index 100% rename from internal/app/testing_helpers.go rename to internal/app/testing_helpers_test.go diff --git a/subscription.go b/subscription.go index 90c1f55..3af5bc4 100644 --- a/subscription.go +++ b/subscription.go @@ -205,9 +205,12 @@ func (ws *WSStat) Subscribe(ctx context.Context, opts SubscriptionOptions) (*Sub sub := ws.registerSubscription(state) go ws.watchSubscription(state) - // Send initial subscription request if payload is provided. + // Send the initial subscription request if a payload is provided. The write is + // deliberately untimed: subscription responses are consumed by the dispatch path + // and never recorded as reads, so a timed write here would unbalance the + // write/read ledgers and zero out MessageRTT for mixed sessions. if len(opts.Payload) > 0 { - ws.WriteMessage(opts.MessageType, opts.Payload) + ws.enqueueWrite(opts.MessageType, opts.Payload) } return sub, nil @@ -349,7 +352,12 @@ func (ws *WSStat) dispatchIncoming(read *wsRead) bool { return false } - receivedAt := time.Now() + // Use the read pump's arrival stamp so subscription and request/response + // paths report the same moment for the same frame. + receivedAt := read.at + if receivedAt.IsZero() { + receivedAt = time.Now() + } if read.err != nil { for _, state := range states { diff --git a/wsstat.go b/wsstat.go index 3ff7c1a..6441b22 100644 --- a/wsstat.go +++ b/wsstat.go @@ -123,6 +123,7 @@ type WSStat struct { cancel context.CancelFunc closeOnce sync.Once closed atomic.Bool // set once Close begins; distinguishes closed from never-dialed + dialed atomic.Bool // set once DialContext starts the pumps; the instance is single-use wgPumps sync.WaitGroup // instance configuration @@ -206,6 +207,7 @@ type wsRead struct { data []byte err error messageType int + at time.Time // frame arrival, stamped in the read pump } // wsWrite holds the data to be written to the WebSocket connection. @@ -382,7 +384,9 @@ func (ws *WSStat) readFrame(conn *websocket.Conn) (*wsRead, bool) { deadlineHit = readCtx.Err() == context.DeadlineExceeded && ws.ctx.Err() == nil cancel() } - return &wsRead{data: p, err: err, messageType: fromCoderType(coderType)}, deadlineHit + return &wsRead{ + data: p, err: err, messageType: fromCoderType(coderType), at: time.Now(), + }, deadlineHit } // writePump writes messages to the WebSocket connection. @@ -430,6 +434,9 @@ func (ws *WSStat) writePump() { // DialContext establishes a new WebSocket connection bound to ctx. Canceling ctx (or calling // Close) tears down the connection and unblocks in-flight reads and writes. If required, specify // custom headers to merge with the default headers. +// +// A WSStat instance is single-use: after a successful dial (or a Close), DialContext returns +// an error; create a new instance to reconnect. A failed dial leaves the instance reusable. // Sets times: dialStart, wsHandshakeDone func (ws *WSStat) DialContext( ctx context.Context, targetURL *url.URL, customHeaders http.Header, @@ -439,6 +446,15 @@ func (ws *WSStat) DialContext( if ctx == nil { return errors.New("nil context") } + // Close is permanent (closeOnce is consumed), so a redial would start pumps that + // nothing can ever cancel; a second dial on a live instance would orphan the first + // connection and its pumps. Guard both. + if ws.closed.Load() { + return ErrClosed + } + if !ws.dialed.CompareAndSwap(false, true) { + return errors.New("wsstat: instance already dialed; create a new WSStat to reconnect") + } // Install the connection context from the caller, replacing the placeholder created in New so // the pumps and read/write paths honor caller cancellation and deadlines. ws.cancel() @@ -462,6 +478,8 @@ func (ws *WSStat) DialContext( CompressionMode: compression, }) if err != nil { + // The pumps never started; allow the caller to retry on the same instance. + ws.dialed.Store(false) if resp != nil { body, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrBodyBytes)) defer func() { @@ -523,58 +541,59 @@ func applyDefaultHeaders(headers http.Header) http.Header { return dst } -// WriteMessage sends a message through the WebSocket connection. -// Sets time: MessageWrites -func (ws *WSStat) WriteMessage(messageType int, data []byte) { +// enqueueWrite queues a frame for the write pump without recording a write timing. +// It reports whether the frame was queued; false means the connection is closing +// and the frame was dropped. +func (ws *WSStat) enqueueWrite(messageType int, data []byte) bool { // Check if connection is closing before attempting to write select { case <-ws.ctx.Done(): ws.log.Debug().Msg("Dropping write message, connection closing") - return + return false default: } - ws.timings.mu.Lock() - ws.timings.messageWrites = append(ws.timings.messageWrites, time.Now()) - ws.timings.mu.Unlock() - select { case ws.writeChan <- &wsWrite{data: data, messageType: messageType}: - // Message sent successfully + return true case <-ws.ctx.Done(): // Connection is closing, drop the message ws.log.Debug().Msg("Dropping write message, connection closing") + return false } } -// WriteMessageJSON sends a message through the WebSocket connection. -// Sets time: MessageWrites -func (ws *WSStat) WriteMessageJSON(v any) { - // Check if connection is closing before attempting to write - select { - case <-ws.ctx.Done(): - ws.log.Debug().Msg("Dropping JSON write message, connection closing") +// WriteMessage sends a message through the WebSocket connection. +// Sets time: MessageWrites. A message dropped because the connection is closing +// records no timing, keeping the write/read ledgers pairable. +func (ws *WSStat) WriteMessage(messageType int, data []byte) { + t := time.Now() + if !ws.enqueueWrite(messageType, data) { return - default: } + ws.timings.mu.Lock() + ws.timings.messageWrites = append(ws.timings.messageWrites, t) + ws.timings.mu.Unlock() +} +// WriteMessageJSON sends a message through the WebSocket connection. +// Sets time: MessageWrites. A message dropped because the connection is closing +// (or one that fails to marshal) records no timing, keeping the write/read +// ledgers pairable. +func (ws *WSStat) WriteMessageJSON(v any) { b, err := json.Marshal(v) if err != nil { ws.log.Debug().Err(err).Msg("Failed to encode JSON") return } + t := time.Now() + if !ws.enqueueWrite(TextMessage, b) { + return + } ws.timings.mu.Lock() - ws.timings.messageWrites = append(ws.timings.messageWrites, time.Now()) + ws.timings.messageWrites = append(ws.timings.messageWrites, t) ws.timings.mu.Unlock() - - select { - case ws.writeChan <- &wsWrite{data: b, messageType: TextMessage}: - // Message sent successfully - case <-ws.ctx.Done(): - // Connection is closing, drop the message - ws.log.Debug().Msg("Dropping JSON write message, connection closing") - } } // PingPong sends a ping through the WebSocket connection and blocks until the matching pong @@ -620,6 +639,8 @@ func classifyReadErr(err error) error { } // handleRead processes a value received from readChan, recording read timing on success. +// The timing is the frame's arrival in the read pump, not the moment the consumer drained +// the channel, so time spent buffered does not inflate MessageRTT. func (ws *WSStat) handleRead(msg *wsRead) (int, []byte, error) { if msg == nil { return 0, nil, ErrClosed @@ -628,7 +649,7 @@ func (ws *WSStat) handleRead(msg *wsRead) (int, []byte, error) { return msg.messageType, nil, classifyReadErr(msg.err) } ws.timings.mu.Lock() - ws.timings.messageReads = append(ws.timings.messageReads, time.Now()) + ws.timings.messageReads = append(ws.timings.messageReads, msg.at) ws.timings.mu.Unlock() return msg.messageType, msg.data, nil } diff --git a/wsstat_test.go b/wsstat_test.go index 260d026..2005526 100644 --- a/wsstat_test.go +++ b/wsstat_test.go @@ -329,6 +329,64 @@ func TestSubscribeReceivesMessage(t *testing.T) { assert.GreaterOrEqual(t, stats.LastEvent, stats.FirstEvent) } +func TestSubscribePayloadDoesNotSkewMessageRTT(t *testing.T) { + // Regression: Subscribe's initial payload used to be recorded in the RTT write + // ledger while subscription responses never touch the read ledger, so a session + // mixing a subscription with request/response measurement ended with more writes + // than reads and calculateResultLocked silently zeroed MessageRTT. + ws := New() + defer ws.Close() + require.NoError(t, ws.DialContext(context.Background(), echoServerAddrWs, http.Header{})) + + sub, err := ws.Subscribe(context.Background(), SubscriptionOptions{ + MessageType: TextMessage, + Payload: []byte("hello-sub"), + }) + require.NoError(t, err) + + select { + case msg := <-sub.Updates(): + assert.Equal(t, "hello-sub", string(msg.Data)) + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for subscription message") + } + + require.NoError(t, ws.PingPong()) + + result := ws.ExtractResult() + assert.Greater(t, result.MessageRTT, time.Duration(0), + "subscription payload write must not unbalance the RTT ledger") +} + +func TestDialContextSingleUse(t *testing.T) { + t.Run("dial after close is rejected", func(t *testing.T) { + ws := New() + require.NoError(t, ws.DialContext(context.Background(), echoServerAddrWs, http.Header{})) + ws.Close() + // Close is permanent: a redial would start pumps nothing can ever cancel. + err := ws.DialContext(context.Background(), echoServerAddrWs, http.Header{}) + assert.ErrorIs(t, err, ErrClosed) + }) + + t.Run("second dial on a live instance is rejected", func(t *testing.T) { + ws := New() + defer ws.Close() + require.NoError(t, ws.DialContext(context.Background(), echoServerAddrWs, http.Header{})) + err := ws.DialContext(context.Background(), echoServerAddrWs, http.Header{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "already dialed") + }) + + t.Run("failed dial leaves the instance reusable", func(t *testing.T) { + ws := New() + defer ws.Close() + badTarget, err := url.Parse("ws://127.0.0.1:1") + require.NoError(t, err) + require.Error(t, ws.DialContext(context.Background(), badTarget, http.Header{})) + assert.NoError(t, ws.DialContext(context.Background(), echoServerAddrWs, http.Header{})) + }) +} + func TestSubscriptionSurvivesIdleBeyondTimeout(t *testing.T) { // Regression: the per-read dial/read timeout must not tear down a long-lived // subscription that is merely idle. A 5s (here 100ms) silence used to finalize