Skip to content

fix(relay): deflake TestSMTPInboundMetric_RejectedLineTooLong - #799

Merged
jiashuoz merged 1 commit into
mainfrom
fix/flaky-rejected-line-too-long
Aug 2, 2026
Merged

fix(relay): deflake TestSMTPInboundMetric_RejectedLineTooLong#799
jiashuoz merged 1 commit into
mainfrom
fix/flaky-rejected-line-too-long

Conversation

@jiashuoz

@jiashuoz jiashuoz commented Aug 2, 2026

Copy link
Copy Markdown
Member

Problem

TestSMTPInboundMetric_RejectedLineTooLong (added in #773) intermittently reds the Go coverage gate and blocks unrelated PRs:

--- FAIL: TestSMTPInboundMetric_RejectedLineTooLong (0.78s)
    metrics_test.go:472: write body: write tcp 127.0.0.1:34262->127.0.0.1:38673: write: connection reset by peer

Seen on main fddf0edf (job). It is flaky, not broken — the same test on recent main: 25356292 pass, bfeec71e pass, 81e8ce87 pass, fddf0edf fail (~1 in 4 in that sample).

Root cause

The test writes a single 160KiB line to exercise the 128KiB MaxLineLength cap. The server hits the cap partway through and resets the connection. Whether the client finishes its Write before the RST arrives depends on socket buffering and scheduling — make cover runs packages at -p 4 on a loaded runner, which is exactly when it loses. The old comment even conceded the assumption ('small enough that the unread remainder … fits in loopback socket buffers'). When the write lost the race, t.Fatalf("write body: …") fired before the real assertion was ever reached.

Fix

Remove the timing dependency instead of re-tuning the body size (a bigger/smaller buffer just moves the threshold and stays racy):

  • A w.Write error is no longer fatal — a mid-write reset is itself evidence the server aborted the transaction. It's captured instead.
  • The test fails only if both the write and w.Close() succeed — the genuine 'server wrongly accepted a 160KiB line' condition.
  • The 554 / 'too long a line' text assertion applies only when the write completed cleanly; after a mid-write reset, Close may surface a bare connection error rather than the SMTP text.
  • The deterministic, server-side proof remains assertSingleOutcome(t, metrics, "rejected_line_too_long", false) — the actual thing under test, independent of wire timing, and it runs on every path.

Verification

  • go test ./internal/relay -run TestSMTPInboundMetric_RejectedLineTooLong -count=30 — ok (30/30)
  • go test ./internal/relay -run TestSMTPInboundMetric -count=10 -p 4 — ok
  • go test ./internal/relay -count=1 (whole package) — ok
  • Regression check: temporarily raised MaxLineLength to 512KiB in server.go → test fails with DATA with a 160KiB line succeeded; want 554 too-long-line rejection; restored → passes. The flake is fixed without neutering the assertion.
  • gofmt -l clean, go vet clean.

🤖 Generated with Claude Code

The test writes a single 160KiB line to exercise the 128KiB
MaxLineLength cap. The server aborts the connection partway through
the transfer, so whether the client's Write finishes before the RST
arrives depends on socket buffering and scheduling — under CI
contention (make cover runs packages at -p 4) the Write intermittently
observes 'connection reset by peer' and the old t.Fatalf fired before
the real assertion ever ran (~1 in 4 on recent main; e.g. fddf0ed).

A mid-write reset is itself evidence the server aborted, so treat it
as such instead of fatal: fail only if both Write and Close succeed
(the genuine 'server accepted a 160KiB line' condition), assert the
554 too-long-line text only when the write completed cleanly, and rely
on the rejected_line_too_long metric — which is the actual behavior
under test and independent of wire timing — on every path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jiashuoz
jiashuoz merged commit 074c92a into main Aug 2, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant