test: increase spawn test child timeout to 30 seconds #266

Sjors commented at 7:46 AM on April 1, 2026: member

Bump timeout from 1 second to 30 to prevent spurious timeouts.

Fixes https://github.com/bitcoin/bitcoin/issues/34975

DrahtBot commented at 7:47 AM on April 1, 2026: none

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

“Give the child some time to exit. If it does not, terminate it and reap it to avoid leaving a zombie behind.” -> “Give the child some time to exit. If it does not exit, terminate it and reap it to avoid leaving a zombie behind.” [The added sentence is understandable, but “If it does not, …” can momentarily read as the child not doing something unspecified; adding “exit” removes any ambiguity.]

in test/mp/test/spawn_tests.cpp:99 in ad6171d936

  93 | @@ -94,9 +94,9 @@ KJ_TEST("SpawnProcess does not run callback in child")
  94 |      ::close(fd);
  95 |  
  96 |      int status{0};
  97 | -    // Give the child up to 1 second to exit. If it does not, terminate it and
  98 | +    // Give the child up to 30 seconds to exit. If it does not, terminate it and
  99 |      // reap it to avoid leaving a zombie behind.
 100 | -    const bool exited{WaitPidWithTimeout(pid, std::chrono::milliseconds{1000}, status)};
 101 | +    const bool exited{WaitPidWithTimeout(pid, std::chrono::milliseconds{30000}, status)};

maflcko commented at 8:00 AM on April 1, 2026:

    // Give the child some time to exit. If it does not, terminate it and
// reap it to avoid leaving a zombie behind.
    const bool exited{WaitPidWithTimeout(pid, std::chrono::seconds{30}, status)};

nit: The chrono header can do the needed multiplication for you, no need to do it manually in the source code

Sjors commented at 8:08 AM on April 1, 2026:

Nice, I'll let CI finish running and then push this.

Sjors force-pushed on Apr 1, 2026

Sjors commented at 8:38 AM on April 1, 2026: member

No timeouts on the 3 jobs x 2 pushes worth of Bitcoin Core CI runs here. However, I don't remember seeing those here before anyway.

I also opened a subtree update PR https://github.com/bitcoin/bitcoin/pull/34977 so that should give us another run.

ryanofsky approved

ryanofsky commented at 2:36 PM on April 2, 2026: collaborator

Code review ACK dba3485c044a72ed20adf50b33a05d53b177dc50. Thanks for the fix!

One suggestion I'd make would be to introduce a global constant constexpr auto FAILURE_TIMEOUT = 30s; and just pass that constant to WaitPidWithTimeout.

I don't think it is a good practice for tests to choose idiosyncratic, individual timeouts except in special cases. There should be some shared idea of how long tests are allowed to wait without making progress before they are considered failed.

Sjors force-pushed on Apr 3, 2026

test: increase spawn test child timeout to 30 seconds

Example CI failure:

  3/157 Test   #3: mptest ...............................***Failed    1.06 sec
[ TEST ] spawn_tests.cpp:44: SpawnProcess does not run callback in child
./ipc/libmultiprocess/test/mp/test/spawn_tests.cpp:108: failed: expected exited; Timeout waiting for child process to exit
stack: bf2ed1 cc4091 cc4545 d817f9 d82ba3 bf220b ee33f4b9 ee33f555
./ipc/libmultiprocess/test/mp/test/spawn_tests.cpp:109: failed: expected WIFEXITED(status) && WEXITSTATUS(status) == 0
stack: bf2e03 cc4091 cc4545 d817f9 d82ba3 bf220b ee33f4b9 ee33f555
[ FAIL ] spawn_tests.cpp:44: SpawnProcess does not run callback in child (1051610 μs)
[ TEST ] test.cpp:124: Call FooInterface methods
[ PASS ] test.cpp:124: Call FooInterface methods (3744 μs)
[ TEST ] test.cpp:216: Call IPC method after client connection is closed
[ PASS ] test.cpp:216: Call IPC method after client connection is closed (363 μs)
[ TEST ] test.cpp:233: Calling IPC method after server connection is closed
[ PASS ] test.cpp:233: Calling IPC method after server connection is closed (346 μs)
[ TEST ] test.cpp:250: Calling IPC method and disconnecting during the call
[ PASS ] test.cpp:250: Calling IPC method and disconnecting during the call (354 μs)
[ TEST ] test.cpp:270: Calling IPC method, disconnecting and blocking during the call
[ PASS ] test.cpp:270: Calling IPC method, disconnecting and blocking during the call (554 μs)
[ TEST ] test.cpp:319: Make simultaneous IPC calls on single remote thread
[ PASS ] test.cpp:319: Make simultaneous IPC calls on single remote thread (741 μs)
6 test(s) passed
1 test(s) failed

Fixes bitcoin/bitcoin#34975

cc0b23fc32

Sjors force-pushed on Apr 3, 2026

Sjors commented at 8:06 PM on April 3, 2026: member

CI passed. Doing another push.

ryanofsky approved

ryanofsky commented at 9:06 PM on April 3, 2026: collaborator

Code review ACK cc0b23fc323c7854c2b42100a4bd3cbba5798f8d, just adding FAILURE_TIMEOUT constant since last review (thanks!)

ryanofsky merged this on Apr 3, 2026

ryanofsky closed this on Apr 3, 2026

Sjors deleted the branch on Apr 7, 2026