Skip to content

test(archera): reuse one server per error-path harness call - #2275

Merged
cristim merged 2 commits into
mainfrom
fix/2259-archera-flaky
Oct 19, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/2259-archera-flaky

Conversation

@cristim

@cristim cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member

Closes #2259

Root cause: test harness, not production

forArcheraErrorPaths (cmd/archera_comparison_key_test.go) called setupArchera once per padding case, once for decode and once per failing-transport kind. Each call started a new httptest server (cmd/archera_comparison_test.go:81-96) whose cleanup ran only when the test ended, so http.DefaultTransport could never reuse a connection. A temporary ConnState counter measured 1,868 servers and 1,584 new connections for go test ./cmd -run Archera. Under load or repetition that piles up loopback TIME_WAIT sockets, which slowed the hostile-decode test past its 1 s wall-clock budget (and, under heavy repetition, exhausted the host's ephemeral ports).

Production is not affected. archera-comparison makes one request per run, insurance.NewClient (cmd/archera_comparison.go:95) builds one httpclient.New(), bodies are closed (insurance/client.go:143,160,226), and keep-alives are on.

Fix

  • forArcheraErrorPaths uses one server per call with a mutex-guarded swappable handler, and stops starting unused servers in the failing-transport loop.
  • setupArchera counts new connections through ConnState (archeraRecorder.newConns).
  • TestArcheraHostileLargeDecodeValueIsBounded no longer uses wall-clock time. It asserts the bound structurally: the pkg replaces a non-identifier value with its length, and the CLI matcher reads only archeraMaxErrorSize + archeraCheckSlack bytes, so a key split past that window is cut and not withheld.

Regression check

TestArcheraErrorPathHarnessReusesConnections sends 41 requests and asserts newConns <= 3. It measures 1; the old harness opened about 41.

Commit (<= 400 additions+deletions)

  1. test(archera): reuse one server per error-path harness call (80)

Local verification (all -count=1, one process at a time)

  • Loopback TIME_WAIT for one -run Archera pass: 10 → 1,730 before the fix, 7 → 297 after.
  • go vet ./..., go test -count=1 ./cmd -run Archera, one go test -count=1 ./... and golangci-lint (0 issues) all pass.
  • Mutation probe: widening the matcher window (cmd/archera_comparison.go:261) to 1<<30 fails the hostile-decode test ("message withheld").

Independent reviews follow as PR comments.

🤖 Generated with Claude Code

forArcheraErrorPaths started an httptest server per pad, each held a fresh
loopback connection until test end: ~1870 servers and ~1580 connections for
-run Archera, ~1700 TIME_WAIT sockets, which starved loaded machines and made
the wall-clock assertion flake. One server with a swappable handler now serves
the whole call (1 connection). Also assert the hostile 1 MB decode value is
bounded structurally (output shape and matcher window) instead of by elapsed
time. Production client creates one client per run and closes bodies.

Closes #2259

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/internal Team-internal only effort/xs Trivial / one-liner type/bug Defect labels Oct 19, 2026
@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 2 billable files and costs up to $0.50.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 25 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 88 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: c872cd0f-cb7e-4fce-8d57-bc430010dccf
📥 Commits

Reviewing files that changed from the base of the PR and between 5a5eca0 and b75eb27.

📒 Files selected for processing: 2
  • cmd/archera_comparison_key_test.go
  • cmd/archera_comparison_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Independent review A (claude-cc-libs-reviewA-2275, Sonnet, cold brief, not the author)

Reviewed SHA: c9be728
Verdict: APPROVE. No actionable findings.

  1. Production is fine:
    • cmd/archera_comparison.go:95 builds one insurance.NewClient per run. With a nil client it creates one httpclient.New() (pkg client.go:~100), and the transport is shared.
    • Bodies are closed: Comparison defers Close (~143), the non-2xx path in getWithHeaders defers Close (~226), and newHTTPError drains up to 64 KiB.
  2. The harness is race-free:
    • go test -race -count=1 ./cmd -run Archera passes.
    • The handler is mutex-guarded and set before each run. The calls are sequential, so a stale handler cannot answer the next case.
    • The failing-transport loop swaps only archeraHTTPClient, and cleanup restores it.
  3. The structural bound still proves the property:
    • Widening the matcher window (archera_comparison.go:261) to 1<<30 fails with "message withheld".
    • Removing the 200-byte display cap (:216) fails the cut-output assertion.
  4. The regression test measures the right thing: a harness that starts one server per case with a shared recorder fails TestArcheraErrorPathHarnessReusesConnections (41 new connections, limit 3). A naive revert with a separate recorder per case also fails, through the hit-count assertion.
  5. Lint: go vet ./... is clean, and golangci-lint reports 0 issues.

TIME_WAIT (all -count=1): -race -run Archera went from 35 to 197, and a clean -run Archera pass from 102 to 383. It stayed far below any risk level.

Nit: the bound newConns <= 3 is loose against the measured value of 1. That margin is fine for an occasional keep-alive drop.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Independent review B (claude-cc-libs-reviewB-2275, Sonnet, cold brief, not the author, independent of review A)

Reviewed SHA: c9be728
Verdict: APPROVE

#2259 criteria:

Criterion Status Evidence
No wall-clock dependence met time and the < time.Second assertion are removed
Still asserts the hostile 1 MB value is bounded met (a) end to end: "(1048576 bytes, not shown)" and no "pppp", as produced by pkg insurance/quote.go:55. (b) archeraSafeError(filler + split key) must equal exactly "p" × archeraMaxErrorSize
Load cannot fail it met deterministic string checks, and the harness no longer starts ~1,800 servers

(b) is the real bound: a matcher that read the whole message would find the key past the window and withhold the message, while one limited to archeraMaxErrorSize + archeraCheckSlack (cmd/archera_comparison.go:261) cuts it. (a) catches the pkg starting to echo large values. The only thing neither catches is extra linear work that leaves the output unchanged, which only a clock could detect, and #2259 asked to drop the clock.

Other server-per-case harnesses (read, not run):

  • archera_comparison_filters_test.go:34,56,78,86,95: about 50 servers per run.
  • archera_pkg_adoption_test.go:42,72: about 25 or more.
  • The rest start one server per test.

That is about 100 per run against ~1,868 before, which is no exhaustion risk.

Local verification: go vet ./... is clean, go test -count=1 ./cmd -run Archera passes (2.5s), and golangci-lint reports 0 issues. TIME_WAIT stayed between 44 and 346.

Low: if a future case never calls serve(), the nil handler panics inside net/http and the client just sees EOF. Optionally fail loudly on a nil handler.

@cristim

cristim commented Oct 19, 2026

Copy link
Copy Markdown
Member Author

Merge gate verdict (claude-cc-libs)

Every gate holds at head b75eb27:

  • Two independent reviews approve the reviewed head c9be728. The branch was then updated from main, and the PR diff is byte-identical to the reviewed diff (compared mechanically).
  • CI is green on b75eb27, which is the reviewed change on top of current main.
  • 0 unresolved threads; MERGEABLE/CLEAN.

CodeRabbit is waived as a merge gate under the 2026-10-19 decision. Squash-merging.

@cristim
cristim merged commit 1ca5439 into main Oct 19, 2026
12 checks passed
@cristim
cristim deleted the fix/2259-archera-flaky branch October 19, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(archera): TestArcheraHostileLargeDecodeValueIsBounded fails under machine load (1s wall-clock bound)

1 participant