You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bryancall (via GitHub)" <gi...@apache.org> on 2026/03/23 17:23:01 UTC
[PR] Fix flaky autests for timeout, sigusr2, and thread_config [trafficserver]
bryancall opened a new pull request, #13012:
URL: https://github.com/apache/trafficserver/pull/13012
## Summary
- Stabilize three flaky AuTests by hardening helper/process behavior under parallel ASAN runs.
- Ignore SIGPIPE and retry `accept()` on EINTR in `ssl-delay-server` to avoid spurious helper exits.
- Remove deadlocking startup sequencing in `sigusr2` and improve `thread_config` process matching for ASAN command lines.
## Test plan
- [x] Run AuTests in ASAN configuration and verify these tests pass reliably.
- [x] Validate no production code paths are changed (test-only PR).
- [ ] Re-run Apache CI to confirm cross-platform stability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky autests for timeout, sigusr2, and thread_config [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall merged PR #13012:
URL: https://github.com/apache/trafficserver/pull/13012
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky autests for timeout, sigusr2, and thread_config [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13012:
URL: https://github.com/apache/trafficserver/pull/13012#discussion_r2976632510
##########
tests/gold_tests/logging/sigusr2.test.py:
##########
@@ -130,9 +130,9 @@ def get_sigusr2_signal_command(self):
# Configure the signaling of SIGUSR2 to traffic_server.
tr1.Processes.Default.Command = diags_test.get_sigusr2_signal_command()
tr1.Processes.Default.Return = 0
-tr1.Processes.Default.Ready = When.FileExists(diags_test.diags_log)
-
-# Configure process order.
+# Configure process order: ts starts first, then rotate moves diags.log,
+# then Default sends SIGUSR2. No Ready condition needed on Default since the
+# StartBefore chain already ensures ts is fully started before rotate runs.
tr1.Processes.Default.StartBefore(rotate_diags_log)
rotate_diags_log.StartBefore(diags_test.ts)
Review Comment:
The comment says the StartBefore chain ensures TS is "fully started", but StartBefore only enforces launch ordering; it doesn’t guarantee `diags.log` exists before `mv` runs. With the Ready condition removed from Default, `rotate_diags_log` can still race TS startup and fail if `diags.log` hasn’t been created yet. Consider moving the file-exists gating to the rotate step (e.g., via a `ready=` condition on the StartBefore edge or by making the rotate command wait/retry) and/or adjust the comment to avoid implying readiness guarantees.
##########
tests/gold_tests/timeout/ssl-delay-server.cc:
##########
@@ -199,8 +204,10 @@ main(int argc, char *argv[])
for (;;) {
sfd = accept(listenfd, (struct sockaddr *)nullptr, nullptr);
if (sfd <= 0) {
Review Comment:
`accept()` returns `-1` on error, but can legally return fd `0` on success (e.g., if stdin is closed). The `if (sfd <= 0)` check will treat a valid connection as failure and exit, potentially reintroducing flakiness. Use `sfd < 0` for the error check, keeping the EINTR retry logic the same.
```suggestion
if (sfd < 0) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky autests for timeout, sigusr2, and thread_config [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13012:
URL: https://github.com/apache/trafficserver/pull/13012#discussion_r2984535919
##########
tests/gold_tests/logging/sigusr2.test.py:
##########
@@ -130,9 +130,9 @@ def get_sigusr2_signal_command(self):
# Configure the signaling of SIGUSR2 to traffic_server.
tr1.Processes.Default.Command = diags_test.get_sigusr2_signal_command()
tr1.Processes.Default.Return = 0
-tr1.Processes.Default.Ready = When.FileExists(diags_test.diags_log)
-
-# Configure process order.
+# Configure process order: ts starts first, then rotate moves diags.log,
+# then Default sends SIGUSR2. No Ready condition needed on Default since the
+# StartBefore chain already ensures ts is fully started before rotate runs.
tr1.Processes.Default.StartBefore(rotate_diags_log)
rotate_diags_log.StartBefore(diags_test.ts)
Review Comment:
All three of these comments - the new source code comment, Copilot's review, and this latter one - are all a nonsensical dialog. They all miss the point to the Default Ready condition.
Let's start with the process chain is the simple sequence of events it describes:
* Start ATS via `rotate_diags_log.StartBefore(diags_test.ts)`. Nothing mysterious here. ATS is started and the diags.log **will** exist and be checked for "trafficserver is fully initialized" in it. That's a part of the ready condition for the ts process in the trafficserver.text.ext extension.
* rotate_diags_log `mv`s the diag.log out of the way. This is simple: the diags.log will now be moved to some other filename, diags.log_old I believe.
* Default then runs, sending a SIGUSR2.
That's the chain of events.
Now, the question for this patch is the Default Ready condition. The ready condition from before this patch was to ensure that the new diags.log is created. Thus, the command would send the SIGUSR2 to ATS as described above, and the Ready condition would wait for ATS to process the signal and recreate the diags.log file. Things should not proceed until that new diags.log file is created, thus the Ready.
Make no mistake: we need to somehow await that new file being created to ensure proper timing alignment.
I'm not entirely sure what problem is being solved here, but I assume that there is a race condition between the Default process sending the SIGUSR2, and the Ready awaiting the file creation? Maybe autest has a problem processing Ready when the process goes away, which it might do before the diags.log file is created?
If that is the case, then let's do this: update the `ts_process_handler.py` script to take a path argument upon which to await the creation of the file before finishing. Maybe loop on the file creation with a .1 second sleep between iterations. Timeout after five seconds and exit with a non-zero if the file never gets created (no infinite loops, please).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky autests for timeout, sigusr2, and thread_config [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13012:
URL: https://github.com/apache/trafficserver/pull/13012#discussion_r2988477106
##########
tests/gold_tests/logging/sigusr2.test.py:
##########
@@ -130,9 +130,9 @@ def get_sigusr2_signal_command(self):
# Configure the signaling of SIGUSR2 to traffic_server.
tr1.Processes.Default.Command = diags_test.get_sigusr2_signal_command()
tr1.Processes.Default.Return = 0
-tr1.Processes.Default.Ready = When.FileExists(diags_test.diags_log)
-
-# Configure process order.
+# Configure process order: ts starts first, then rotate moves diags.log,
+# then Default sends SIGUSR2. No Ready condition needed on Default since the
+# StartBefore chain already ensures ts is fully started before rotate runs.
tr1.Processes.Default.StartBefore(rotate_diags_log)
rotate_diags_log.StartBefore(diags_test.ts)
Review Comment:
Actually, let's do this: I'll work on a separate PR for sigusr2. Knowing what I know now, it would be better to organize this test differently to avoid what I think are some race conditions we may be seeing with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix flaky autests for timeout, sigusr2, and thread_config [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13012:
URL: https://github.com/apache/trafficserver/pull/13012#discussion_r2983176086
##########
tests/gold_tests/timeout/ssl-delay-server.cc:
##########
@@ -199,8 +204,10 @@ main(int argc, char *argv[])
for (;;) {
sfd = accept(listenfd, (struct sockaddr *)nullptr, nullptr);
if (sfd <= 0) {
Review Comment:
Good catch — fixed. Changed to `sfd < 0` since fd 0 is technically valid.
##########
tests/gold_tests/logging/sigusr2.test.py:
##########
@@ -130,9 +130,9 @@ def get_sigusr2_signal_command(self):
# Configure the signaling of SIGUSR2 to traffic_server.
tr1.Processes.Default.Command = diags_test.get_sigusr2_signal_command()
tr1.Processes.Default.Return = 0
-tr1.Processes.Default.Ready = When.FileExists(diags_test.diags_log)
-
-# Configure process order.
+# Configure process order: ts starts first, then rotate moves diags.log,
+# then Default sends SIGUSR2. No Ready condition needed on Default since the
+# StartBefore chain already ensures ts is fully started before rotate runs.
tr1.Processes.Default.StartBefore(rotate_diags_log)
rotate_diags_log.StartBefore(diags_test.ts)
Review Comment:
Not applicable — in the AuTest framework, `StartBefore` waits for the earlier process's Ready condition (port-up for ATS), not just launch ordering. By the time TS is ready, `diags.log` has already been created during startup. There is no race here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org