Skip to content

test(aws/ladder): distinguish pre-write aborts during loopback fixture cleanup - #500

Merged
cristim merged 1 commit into
mainfrom
test/417-ladder-fixture-cleanup
Oct 17, 2026
Merged

cristim merged 1 commit into
mainfrom
test/417-ladder-fixture-cleanup

Conversation

@cristim

@cristim cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Summary

Test-only. In providers/aws/ladder/ce_loopback_body_test.go, TestCELoopbackPublicBaselineAfterBodyClose's cleanup joined the handler and the wrote channel unconditionally. When the request aborted before any connection was dialed (a dial failure, or the guard rejecting before the request), neither ever completed, so cleanup waited out both 5s bounds and buried the real error under two extra "schedule did not complete" errors.

  • ceSchedule gains dialed (set in DialContext on success) and handlerStarted (set at handler entry).
  • Cleanup joins the handler only if it started and wrote only if a connection was dialed; otherwise it logs "request aborted before any connection was dialed; skipping write and handler joins". The primary error is kept; the success path is unchanged; every remaining wait still goes through the 5s-bounded ceAwait. A handler cannot start without a dialed connection, and srv.Close() still waits for any in-flight handler.

Closes #417

Evidence

  • Probe (scratch copy): an injected dial failure took 10.01s and produced two extra join errors with the old cleanup; with this change it takes 0.00s and reports only the primary error plus the skip line.
  • go test -race -count=20 -run TestCELoopbackPublicBaselineAfterBodyClose: ok.
  • make build, make vet, (cd providers/aws && GOWORK=off go test -race -short ./ladder/...), pinned golangci-lint v2.10.1 (0 issues), gofmt, and the commit/pre-push hooks pass.

Local review: non-author review of the 15-line diff (join conditions, the wrote close path at the request-write trace, and handler shutdown), no findings.

🤖 Generated with Claude Code

…e cleanup

The scheduled CE loopback fixture cleanup always joined the handler and
the real WroteRequest callback. A request that fails before any
connection is dialed (dial error, guard rejection) starts neither, so
the failed test also reported two watchdog errors after about ten
seconds, on top of the real primary error.

Track whether the handler started and whether a connection was dialed.
Cleanup now joins the handler only if it started and the write callback
only if a connection exists, and logs when the request never reached the
wire. The write channel is never closed on the callback's behalf, so a
real write completion and its error still count on the success path.

Closes #417

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

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 1 billable file and costs up to $0.25.

  • 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 17 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 72 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-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 66dbe942-998d-47f2-ad70-e01bbb341758
📥 Commits

Reviewing files that changed from the base of the PR and between b2d8e6b and 64012d6.

📒 Files selected for processing: 1
  • providers/aws/ladder/ce_loopback_body_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member Author

Independent review verdict: CLEAN at 64012d6

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. Coordinator: claude-cc-reviewer.

Blocking findings: none. This is a test-only change to providers/aws/ladder/ce_loopback_body_test.go.

  • The skipped joins are safe:
    • If the handler never started, handlerJoin stays nil and srv.Close() runs. httptest.Server.Close waits for outstanding connections, so the later handlerErr assertion is still sound.
    • The write join is skipped only when nothing was dialed, so no write can have happened and no writeErr is hidden. When a connection was dialed, s.wrote is still awaited.
    • The writeErr, sdkClosed, entered, copyErr, status and baseline assertions are unchanged.
  • test(aws): distinguish pre-write aborts during fixture cleanup #417's body-close/EOF behaviour is untouched: only the abort-path joins changed, and the happy path is identical.
  • Stability:
    • go test -race -count=200 -run TestCELoopbackPublicBaselineAfterBodyClose ./ladder/: 200 passed, 0 failed (~6.5s).
    • go vet ./ladder/... is clean, and go test -race -count=1 ./ladder/... passes 268.

Gate: p3, labels on the PR. Merge once CI is green. (claude-cc-reviewer)

@cristim
cristim merged commit 5910502 into main Oct 17, 2026
15 checks passed
@cristim
cristim deleted the test/417-ladder-fixture-cleanup branch October 17, 2026 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours 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(aws): distinguish pre-write aborts during fixture cleanup

1 participant