Skip to content

test: tie loopback server lifetime to client work - #259

Merged
smiggleworth merged 1 commit into
mainfrom
fix/windows-loopback-test-lifetime
Aug 25, 2026
Merged

test: tie loopback server lifetime to client work#259
smiggleworth merged 1 commit into
mainfrom
fix/windows-loopback-test-lifetime

Conversation

@smiggleworth

Copy link
Copy Markdown
Contributor

Summary

  • keep executor loopback listeners alive until their client operation completes
  • stop shared scripted provider servers through an explicit caller signal instead of a startup wall-clock deadline
  • retain the bounded post-response observation window used to prove mutating requests are not retried

Why are you making this contribution?

The downstream Windows Platform workflow failed twice while finishing the documentation release work. The first failure lost the scripted provider listener before the S3 error-shape request; after a failed-job rerun passed, the next merged main run lost two executor listeners before their clients were scheduled. In each failure, reqwest reported a loopback connection error instead of receiving the scripted HTTP response.

These test servers started a fixed five- or ten-second lifetime before the parallel Windows test runner necessarily scheduled the client. Server lifetime should follow the client operation, not wall-clock scheduling latency.

Linked issues

No issue: follow-up to repeatable downstream Windows Platform failures discovered while finishing PR #257 and PR #258.

Acceptance audit

  • Criterion: Loopback fixtures remain available until the corresponding client operation completes.
    Evidence: cargo test --workspace --all-features passes the complete library, integration, and doctest suite after the change. Before the change, Platform run 32852891765 lost the shared scripted listener and Platform run 32854634319 lost two executor listeners under Windows scheduling load.
    Production entry point: Test-only loopback fixtures in storage::cloud::executor::tests and storage::providers::test_support; shipping storage behavior is unchanged.
    Resolution: Replaces arbitrary pre-request deadlines with explicit completion signals from the caller.
  • Criterion: Tests that prohibit mutation retries still observe and count any extra request.
    Evidence: should_not_retry_conditional_mutation_given_ambiguous_transient_response, should_not_retry_unconditional_mutation_given_ambiguous_transient_response, and should_not_retry_generation_not_match_mutation_given_transient_response pass with their exact request_count == 1 assertions.
    Production entry point: The tests exercise CloudExecutor::execute_request through real loopback HTTP requests.
    Resolution: Preserves a bounded 250 ms idle observation after the client completes, while removing only the unsafe pre-request expiry.
  • Criterion: Scripted provider servers remain bounded when a caller finishes early or unwinds.
    Evidence: The shared server's finish and Drop paths signal cancellation before joining; all provider and hybrid all-feature tests pass in the full workspace gate.
    Production entry point: Test-only provider fixture used by AWS, Azure, GCS, resolver, and hybrid classification tests.
    Resolution: Makes shutdown caller-driven and keeps the nonblocking accept loop bounded to its owning test.

Risk and compatibility

Low risk and test-only. No public API, storage behavior, persistence format, dependency, or runtime timeout changes. The main risk is a fixture thread waiting indefinitely; explicit completion signaling plus the existing nonblocking poll prevents that, including during unwind.

Verification

  • cargo test --lib storage::cloud::executor::tests::should_not_retry_conditional_mutation_given_ambiguous_transient_response -- --exact --nocapture — passed
  • cargo test --lib storage::cloud::executor::tests::should_not_retry_generation_not_match_mutation_given_transient_response -- --exact --nocapture — passed
  • cargo test --lib storage::hybrid::backend::tests::should_classify_real_s3_error_shapes_given_wal_catalog_operations --all-features -- --exact --nocapture — passed
  • cargo test --workspace --all-features — passed; complete library, integration, and doctest suite
  • cargo clippy --workspace --all-targets --all-features -- -D warnings -D clippy::pedantic — passed
  • cargo fmt --check — passed
  • cntryl-tools validate-tests — passed; 2,440/2,440 compliant
  • git diff --check — passed

Tool assistance disclosure

  • AI or another generative tool materially assisted this contribution.

Codex correlated the repeated hosted Windows failures, replaced the test-server lifetime policy, reviewed the full diff, and ran the verification above.

Contributor responsibility

  • I understand the complete change and can explain or revise it.
  • I reviewed the complete diff.
  • I reported validation accurately and did not claim checks I did not run.
  • I disclosed material generated assistance.
  • I have the right to submit this work under the repository's license.

Review checklist

  • Behavioral tests use the real production entry point, not only private helpers.
  • No enum, failpoint, production implementation, provider contract, or differential implementation changes.
  • Shared test infrastructure uses explicit bounded cleanup and does not add global locks.

@smiggleworth
smiggleworth marked this pull request as ready for review August 25, 2026 13:59
Copilot AI lite review requested due to automatic review settings August 25, 2026 13:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@smiggleworth
smiggleworth merged commit bb1bc55 into main Aug 25, 2026
7 of 10 checks passed
@smiggleworth
smiggleworth deleted the fix/windows-loopback-test-lifetime branch August 25, 2026 14:19
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.

2 participants