Conversation
wait_process_paused had a single hardcoded 5 second budget. That is right for pause_process, which sends SIGSTOP itself, but the callers that arm a self-stopping debug point (DEBUG PAUSE-AFTER-FORK, DEBUG PAUSE-BEFORE-PSYNC) wait for the server to reach that point, and for pause-after-fork the budget also has to cover the primary deciding on a full sync and completing the fork. Under valgrind, or whenever the fork is delayed, that overruns 5 seconds and the test fails with "process didn't stop" on a healthy server. Default the budget to 30 seconds, 100 seconds under valgrind, and have pause_process keep the short 5 second budget so signal-driven stops still fail fast. Signed-off-by: Madelyn Olson <matolson@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wait_process_pausedused one hardcoded 5 second budget for two different kinds of wait. That is correct forpause_process, which sendsSIGSTOPitself, but the callers that arm a self-stopping debug point wait for the server to get there on its own, and forDEBUG PAUSE-AFTER-FORKthat budget also has to cover the primary noticing the reconnecting replica, deciding on a full sync and completing thefork(). When that sequence takes longer than 5 seconds, under valgrind or whenever the fork start is delayed, the test throwsprocess didn't stopagainst a perfectly healthy server. This defaults the budget to 30 seconds (100 under valgrind) and haspause_processpass the short 5 second budget explicitly, so signal-driven stops still fail fast.Fixes #45.
Details
Problem
tests/support/util.tcl:741before this change:50 * 100ms = 5 seconds for every caller. Two shapes of caller exist:
pause_process(tests/support/util.tcl:750) runskill -SIGSTOPfirst, so the stop is near-immediate and 5 seconds is generous.Self-stop waiters.
tests/integration/dual-channel-replication.tcl:832armsdebug pause-after-fork 1at line 825 and then waits for the primary toraise(SIGSTOP)on itself. The hook runs after the fork,src/replication.c:1054:debugPauseProcessisraise(SIGSTOP),src/debug.c:2613. So the 5 seconds has to cover everything up to and including thefork().tests/integration/dual-channel-replication.tcl:11(wait_and_resume_process) andtests/integration/replication-busy-psync.tcl:29(pause-before-psync) are the same shape.Reproduction
The wait is not slow locally, which is why only CI saw this. Instrumented on unpatched
agents/unstable, it completes in 48-53ms over 5 loops, a 100x margin. Valgrind eats that margin: #4562's message records the enclosing test at 27-31s under valgrind in CI.To force it locally, delay the fork instead of slowing the process. Adding one line before the second arming at
tests/integration/dual-channel-replication.tcl:825:The sync is diskless with an EOF-capable replica, so
syncCommandtakes the delayed branch and logsDelay next BGSAVE for diskless SYNC(src/replication.c:1294) instead of forking.replicationCrononly starts the fork oncemax_idle >= server.repl_diskless_sync_delay(src/replication.c:5585), which putsdebugPauseProcess()about 10 seconds after the wait begins, against a 5 second budget.5/5 fail before the change:
Same frame shape as the reported Daily failure, no
testframe, because the wait sits atstart_serverbody level rather than inside thetestblock. Line 834 rather than 832 is the two injected repro lines.5/5 pass after the change, with the measured wait now showing what the old budget was missing:
Decisions
Why not gate the longer budget on valgrind only. Upstream #4562 (
c27cc74bc) fixes the same helper withif {$::valgrind} {set retries 1000} else {set retries 50}. That leaves 5 seconds for every non-valgrind run, and 5 seconds is not derived from anything: the operation being waited on is a full-sync fork, whose latency depends on dataset size, sync delay and machine load, none of which is valgrind-specific. The reproduction above is a non-valgrind failure of exactly this shape. The test framework has no sanitizer flag (only$::valgrindattests/test_helper.tcl:54), so sanitizer builds get the non-valgrind budget too.Why the default and not per call site. Three self-stop call sites exist (
tests/integration/dual-channel-replication.tcl:11and:832,tests/integration/replication-busy-psync.tcl:29) against one signal-driven caller. Putting the long budget in the default and the short one inpause_processtouches one proc instead of three files, and any new self-stop waiter gets the right budget by default.Fail-fast cost. A genuinely wedged self-stop now takes 30 seconds to report instead of 5.
pause_process, where a 5 second stall really is a bug, keeps the old budget.Testing
The reproduction above is the load-bearing part: the injected
repl-diskless-sync-delay 10fails 5/5 without this patch and passes 5/5 with it.This was generated by AI but verified, with love, by a human.