Stabilize the rate_limit SNI expiry AuTest - #13721
Conversation
Open the FIFO before forking clients so cleanup cannot race their startup. Wait for limiter events instead of fixed sleeps so the expiry path is exercised reliably on loaded runners. Fixes: apache#13714
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The changes address the test race and add reliable synchronization and cleanup.
Review effort: Lite
Findings: None
What changed in this PR
Stabilizes the rate-limit SNI expiry AuTest by removing FIFO startup races and synchronizing client actions with limiter events.
Changes:
- Preopens and inherits the FIFO before launching clients.
- Replaces fixed sleeps with event-driven waits.
- Adds bounded cleanup and stronger expiry/accounting assertions.
| File | Description |
|---|---|
tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry.test.py |
Adds event and accounting assertions and passes the traffic log path. |
tests/gold_tests/pluginTest/rate_limit/rate_limit_sni_expiry_client.sh |
Implements synchronized client orchestration, FIFO handling, and bounded cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Describe the three connections directly so the test's purpose is clear without knowing the helper's variable names or the plugin's internals.
Make the FIFO's role clear without requiring readers to trace its file descriptor through the client commands.
bneradt
left a comment
There was a problem hiding this comment.
Reviewed a1b4028. No actionable correctness findings.
This is the expiry-test counterpart to #13681, which stabilized rate_limit_sni_queue; it is not a duplicate. Both tests were introduced in #13406. Our #13540 changes addressed the separate rate_limit_sni test, bounding its queued-client wait while retaining a positive expiration assertion.
The lessons from those changes are applied well here:
- As in #13681, open the FIFO before spawning clients and pass the descriptor directly. Keeping stdin idle and terminating the actual openssl PID avoids depending on the different OpenSSL/LibreSSL EOF behavior. The EXIT trap uses the same bounded TERM/KILL/wait cleanup as normal teardown.
- Wait for the plugin's events instead of guessed sleeps. Requiring both queueing and expiry, and excluding resume/rejection, prevents a healthy-looking run from silently exercising the wrong path. This preserves the positive expiry check emphasized in #13540.
- Keep release-event synchronization separate from counter-value assertions.
Limiter::free()logs after dropping its lock, so the lesson from #13681 is not to demand a zero value as a general synchronization condition; the wrap exclusion is the useful accounting backstop. - Preserve the distinction between entering expiry and proving close-hook accounting. #13406 fixed close-hook dispatch when closure occurs, but that does not itself force a parked connection to observe the client's FIN. The updated documentation correctly states that this test cannot detect missing expiry detachment. As a follow-up, dedicated coverage that forces ATS-side closure should demonstrate failure with detachment removed before claiming regression protection for that bug, following the mutation check used in #13681. This existing coverage gap need not block the test-stability fix.
The existing test class is appropriate for this custom TLS-client choreography; I would not force an ATSReplayTest conversion as part of this fix.
Validation: Python parsing and Bash syntax passed. A temporary mock openssl client exercised the unchanged shell helper's successful three-client sequence, missing-expiry timeout, and signal-triggered cleanup of a TERM-immune client; all returned the expected status and left no mock clients alive. These checks validate shell control flow only. I did not run the real ATS/TLS AuTest locally because this checkout has no configured build. All 14 reported CI checks currently pass, including the four AuTest shards.
rate_limit_sni_expiryuses a FIFO to keep its first TLS client’s standard input open. The test can delete this FIFO before the client opens it, preventing that connection from starting. Open the FIFO before launching clients and pass the already-open descriptor to each client. Wait for the plugin's log messages instead of fixed sleeps to confirm that the first connection is accepted and the second connection queues and expires. Clean up client processes with bounded waits on both success and failure.After the queued connection expires, close the first connection and check that the plugin accepts another connection without crashing ATS. Keep the checks for incorrect connection counts and clarify that the expired connection's close hook is outside this test's coverage on the current TLS core.
Fixes #13714.