From be917c3d30756e370d27620c12b9775c4451398a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 17:23:52 -0700 Subject: [PATCH 01/11] test: give each conmon its own pidfile $CONMON_PID_FILE is a single path per test, but a test may start more than one conmon (one for the container and one for an --exec), in which case the second one overwrites the first one's pidfile, and there is no way to tell what the first conmon's pid was. Have start_conmon_with_default_args use a pidfile of its own for every conmon it starts, and expose the pid of the one just started as $CONMON_PID, so that tests do not have to deal with the path at all. $CONMON_PID_FILE stays for the tests that run conmon directly. Signed-off-by: Kir Kolyshkin --- test/test_helper.bash | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/test_helper.bash b/test/test_helper.bash index ca6195e2..982a58af 100644 --- a/test/test_helper.bash +++ b/test/test_helper.bash @@ -311,6 +311,8 @@ setup_test_env() { CTR_ID=$(generate_ctr_id) export LOG_PATH="$TEST_TMPDIR/container.log" export PID_FILE="$TEST_TMPDIR/pidfile" + # For tests that run conmon directly; conmons started by + # start_conmon_with_default_args each get their own pidfile. export CONMON_PID_FILE="$TEST_TMPDIR/conmon-pidfile" export BUNDLE_PATH="$TEST_TMPDIR" export ROOTFS="$TEST_TMPDIR/rootfs" @@ -406,6 +408,10 @@ wait_for_runtime_status() { # Additional conmon arguments can be passed to this function. start_conmon_with_default_args() { local extra_args=("$@") + # A test may start more than one conmon (an --exec one, say), so give + # each one its own pidfile rather than having them clobber a shared one. + local pidfile="$TEST_TMPDIR/conmon-pidfile.$((++CONMON_STARTED))" + run timeout 10s "$CONMON_BINARY" \ --cid "$CTR_ID" \ --cuuid "$CTR_ID" \ @@ -415,12 +421,16 @@ start_conmon_with_default_args() { --log-level trace \ --container-pidfile "$PID_FILE" \ --syslog \ - --conmon-pidfile "$CONMON_PID_FILE" "${extra_args[@]}" + --conmon-pidfile "$pidfile" "${extra_args[@]}" if [ "$status" -ne 0 ]; then return fi + # The pid of the conmon just started. A test starting more than one has + # to save this before starting the next. + CONMON_PID=$(cat "$pidfile") + # Do not try to start the container if it has already been started. This # happens when `start_conmon_with_default_args` has already been called # and this second call uses an option like --exec, which connects to an @@ -439,7 +449,7 @@ start_conmon_with_default_args() { wait_for_runtime_status "$CTR_ID" created # Check that conmon pidfile was created - [ -f "$CONMON_PID_FILE" ] + [ -f "$pidfile" ] # Start the container and wait until it really starts. run_runtime start "$CTR_ID" From 2239f7545c440ad4cde6c3c93845756c610e7f3d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 19 Aug 2026 00:12:21 -0700 Subject: [PATCH 02/11] test: give each conmon its own container pidfile Every conmon a test starts was told to write the container's pid to the same $PID_FILE, so the runtimes of a container and of an exec into it were handed the same --pid-file. runc writes that file by creating "." with O_EXCL and renaming it afterwards, which makes a shared path a race waiting to happen -- and it happens: : calling runtime args: /usr/bin/runc create --pid-file /tmp/conmon-test-oXpcPe/pidfile : calling runtime args: /usr/bin/runc exec --pid-file /tmp/conmon-test-oXpcPe/pidfile --process ... --detach : runtime stderr: level=error msg="exec failed: open /tmp/conmon-test-oXpcPe/.pidfile: file exists" : Failed to read pidfile: Failed to open file ".../pidfile": No such file or directory The exec then never runs, the container waits for it forever, and the test fails thirty seconds later saying the container did not stop -- a symptom bearing no resemblance to the cause. This accounted for a dozen failures across the parallel runs used to chase it, in every job that hit it. The window is between runc saving the container state, which is what "runc state" starts reporting as "created", and runc writing the pid file a moment later: the test sees "created" and starts the exec while the create's temporary file is still there. Give each conmon its own, exposed as $CONTAINER_PIDFILE, the way $CONMON_PIDFILE already is. Both tests that read the pid want the last conmon started -- the only one in 04-runtime, the exec one in 08-exec -- which is exactly what they now get, rather than relying on the exec's runtime having clobbered the container's file. $PID_FILE stays for the tests that run conmon directly, where there is only ever one conmon and nothing to collide with. Signed-off-by: Kir Kolyshkin Co-Authored-By: Claude Opus 5 --- test/04-runtime.bats | 2 +- test/08-exec.bats | 2 +- test/test_helper.bash | 12 +++++++++--- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/test/04-runtime.bats b/test/04-runtime.bats index 9e03a5cb..682e1aae 100644 --- a/test/04-runtime.bats +++ b/test/04-runtime.bats @@ -88,7 +88,7 @@ teardown() { # Check that the pid is sent to the sync pipe. assert_file_exists $TEST_TMPDIR/syncpipe-output run cat $TEST_TMPDIR/syncpipe-output - CONTAINER_PID=$(cat "$PID_FILE") + CONTAINER_PID=$(cat "$CONTAINER_PIDFILE") assert_json "${output}" =~ "\"pid\": $CONTAINER_PID" } diff --git a/test/08-exec.bats b/test/08-exec.bats index 6d0057fe..7d5e5f4d 100644 --- a/test/08-exec.bats +++ b/test/08-exec.bats @@ -177,7 +177,7 @@ teardown() { # the second one is the exit code. assert_file_exists $TEST_TMPDIR/syncpipe-output run cat $TEST_TMPDIR/syncpipe-output - CONTAINER_PID=$(cat "$PID_FILE") + CONTAINER_PID=$(cat "$CONTAINER_PIDFILE") assert_json "${output}" =~ "\"data\": $CONTAINER_PID" assert_json "${output}" =~ '"data": 0' } diff --git a/test/test_helper.bash b/test/test_helper.bash index 982a58af..7c7627cc 100644 --- a/test/test_helper.bash +++ b/test/test_helper.bash @@ -310,6 +310,8 @@ setup_test_env() { export CTR_ID CTR_ID=$(generate_ctr_id) export LOG_PATH="$TEST_TMPDIR/container.log" + # For tests that run conmon directly; conmons started by + # start_conmon_with_default_args each get their own, in $CONTAINER_PIDFILE. export PID_FILE="$TEST_TMPDIR/pidfile" # For tests that run conmon directly; conmons started by # start_conmon_with_default_args each get their own pidfile. @@ -409,8 +411,12 @@ wait_for_runtime_status() { start_conmon_with_default_args() { local extra_args=("$@") # A test may start more than one conmon (an --exec one, say), so give - # each one its own pidfile rather than having them clobber a shared one. - local pidfile="$TEST_TMPDIR/conmon-pidfile.$((++CONMON_STARTED))" + # each one its own pidfiles rather than having them clobber shared ones. + # $CONTAINER_PIDFILE is left set for the caller: the tests that read the + # pid want the one from the conmon started last. + ((++CONMON_STARTED)) + local pidfile="$TEST_TMPDIR/conmon-pidfile.$CONMON_STARTED" + CONTAINER_PIDFILE="$TEST_TMPDIR/pidfile.$CONMON_STARTED" run timeout 10s "$CONMON_BINARY" \ --cid "$CTR_ID" \ @@ -419,7 +425,7 @@ start_conmon_with_default_args() { --bundle "$BUNDLE_PATH" \ --socket-dir-path "$SOCKET_PATH" \ --log-level trace \ - --container-pidfile "$PID_FILE" \ + --container-pidfile "$CONTAINER_PIDFILE" \ --syslog \ --conmon-pidfile "$pidfile" "${extra_args[@]}" From abd20f5baafe3c5c7833b63d4d80f876cc82488d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 23:20:04 -0700 Subject: [PATCH 03/11] test: fail loudly when conmon fails to start start_conmon_with_default_args swallowed a failing conmon: run timeout 10s "$CONMON_BINARY" ... "${extra_args[@]}" if [ "$status" -ne 0 ]; then return fi so a test whose conmon died, or was killed by that 10 second timeout, carried on regardless and failed thirty seconds later in wait_for_runtime_status, reporting that the container never reached the expected state -- while the actual error, sitting right there in $output, was thrown away. Debugging that from a CI log is guesswork. Die instead, with the status and the output. The behaviour was load bearing for eight tests which use the helper to check that conmon rejects what it should reject, so give them a helper that says as much: run_conmon_expecting_failure, which runs conmon and leaves $status and $output alone. Six of those eight only ever checked the message conmon printed, never its exit code -- a gap that predates this: a conmon that regressed to printing the error and exiting 0 would have passed them. Rather than add the check to each, the helper asserts it, which is what its name promises anyway, and leaves the caller to check the particular complaint. While at it, factor the conmon invocation the two share into _run_conmon, rather than having the argument list twice. Signed-off-by: Kir Kolyshkin Co-Authored-By: Claude Opus 5 --- test/02-ctr-logs.bats | 12 +++++------ test/08-exec.bats | 6 ++---- test/test_helper.bash | 46 +++++++++++++++++++++++++++++-------------- 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/test/02-ctr-logs.bats b/test/02-ctr-logs.bats index 4a3a7615..9843752c 100644 --- a/test/02-ctr-logs.bats +++ b/test/02-ctr-logs.bats @@ -78,7 +78,7 @@ run_conmon_with_log_opts() { } @test "ctr logs: journald with --log-label, no '=' in label" { - start_conmon_with_default_args \ + run_conmon_expecting_failure \ --log-path "journald:" \ --log-label "CONMON_TEST_LABEL1" @@ -86,7 +86,7 @@ run_conmon_with_log_opts() { } @test "ctr logs: journald with --log-label, multiple '=' in label" { - start_conmon_with_default_args \ + run_conmon_expecting_failure \ --log-path "journald:" \ --log-label "CONMON_TEST_LABEL1=FOO=$CTR_ID" @@ -94,7 +94,7 @@ run_conmon_with_log_opts() { } @test "ctr logs: journald with --log-label, no label name" { - start_conmon_with_default_args \ + run_conmon_expecting_failure \ --log-path "journald:" \ --log-label "=$CTR_ID" @@ -102,7 +102,7 @@ run_conmon_with_log_opts() { } @test "ctr logs: journald with --log-label, invalid character" { - start_conmon_with_default_args \ + run_conmon_expecting_failure \ --log-path "journald:" \ --log-label "MY%LABEL=$CTR_ID" @@ -110,7 +110,7 @@ run_conmon_with_log_opts() { } @test "ctr logs: k8s-file with --log-label" { - start_conmon_with_default_args \ + run_conmon_expecting_failure \ --log-path "k8s-file:$LOG_PATH" \ --log-label "CONMON_TEST_LABEL1=$CTR_ID" @@ -134,7 +134,7 @@ run_conmon_with_log_opts() { } @test "ctr logs: k8s-file with --log-tag" { - start_conmon_with_default_args \ + run_conmon_expecting_failure \ --log-path "k8s-file:$LOG_PATH" \ --log-tag "CONMON_TEST_LABEL1" diff --git a/test/08-exec.bats b/test/08-exec.bats index 7d5e5f4d..76a18725 100644 --- a/test/08-exec.bats +++ b/test/08-exec.bats @@ -39,14 +39,13 @@ teardown() { start_conmon_with_default_args --log-path "k8s-file:$LOG_PATH" wait_for_runtime_status "$CTR_ID" running - start_conmon_with_default_args \ + run_conmon_expecting_failure \ --log-path "k8s-file:$LOG_PATH.exec" \ --sync \ --exec \ --exec-process-spec "${BUNDLE_PATH}/process.json" \ --exec-attach - assert_failure assert "${output}" =~ "Attach can only be specified for a non-legacy exec session" } @@ -54,7 +53,7 @@ teardown() { start_conmon_with_default_args --log-path "k8s-file:$LOG_PATH" wait_for_runtime_status "$CTR_ID" running - start_conmon_with_default_args \ + run_conmon_expecting_failure \ --log-path "k8s-file:$LOG_PATH.exec" \ --api-version 1 \ --sync \ @@ -62,7 +61,6 @@ teardown() { --exec-process-spec "${BUNDLE_PATH}/process.json" \ --exec-attach - assert_failure assert "${output}" =~ "--attach specified but _OCI_ATTACHPIPE was not" } diff --git a/test/test_helper.bash b/test/test_helper.bash index 7c7627cc..1262ef5d 100644 --- a/test/test_helper.bash +++ b/test/test_helper.bash @@ -310,11 +310,11 @@ setup_test_env() { export CTR_ID CTR_ID=$(generate_ctr_id) export LOG_PATH="$TEST_TMPDIR/container.log" - # For tests that run conmon directly; conmons started by - # start_conmon_with_default_args each get their own, in $CONTAINER_PIDFILE. + # For tests that run conmon directly; conmons started by _run_conmon each + # get their own, in $CONTAINER_PIDFILE. export PID_FILE="$TEST_TMPDIR/pidfile" - # For tests that run conmon directly; conmons started by - # start_conmon_with_default_args each get their own pidfile. + # For tests that run conmon directly; conmons started by _run_conmon each + # get their own, in $CONMON_PIDFILE. export CONMON_PID_FILE="$TEST_TMPDIR/conmon-pidfile" export BUNDLE_PATH="$TEST_TMPDIR" export ROOTFS="$TEST_TMPDIR/rootfs" @@ -406,16 +406,15 @@ wait_for_runtime_status() { die "timed out waiting for '$expected_status' from $cid" } -# Helper function to start conmon with default arguments. -# Additional conmon arguments can be passed to this function. -start_conmon_with_default_args() { - local extra_args=("$@") - # A test may start more than one conmon (an --exec one, say), so give - # each one its own pidfiles rather than having them clobber shared ones. - # $CONTAINER_PIDFILE is left set for the caller: the tests that read the - # pid want the one from the conmon started last. +# _run_conmon runs conmon with the default arguments plus the ones given, +# leaving the result in $status and $output as `run` does. $CONMON_PIDFILE and +# $CONTAINER_PIDFILE are set to the pidfiles this conmon was told to write. +# +# A test may start more than one conmon (an --exec one, say), so each gets +# pidfiles of its own rather than having them clobber shared ones. +_run_conmon() { ((++CONMON_STARTED)) - local pidfile="$TEST_TMPDIR/conmon-pidfile.$CONMON_STARTED" + CONMON_PIDFILE="$TEST_TMPDIR/conmon-pidfile.$CONMON_STARTED" CONTAINER_PIDFILE="$TEST_TMPDIR/pidfile.$CONMON_STARTED" run timeout 10s "$CONMON_BINARY" \ @@ -427,10 +426,27 @@ start_conmon_with_default_args() { --log-level trace \ --container-pidfile "$CONTAINER_PIDFILE" \ --syslog \ - --conmon-pidfile "$pidfile" "${extra_args[@]}" + --conmon-pidfile "$CONMON_PIDFILE" "$@" +} + +# Helper function to run conmon with default arguments where conmon is +# expected to fail. That it did is asserted here, so the caller is left to +# check $output for the particular complaint it is after. +run_conmon_expecting_failure() { + _run_conmon "$@" + assert_failure +} + +# Helper function to start conmon with default arguments. +# Additional conmon arguments can be passed to this function. +start_conmon_with_default_args() { + local pidfile + + _run_conmon "$@" + pidfile=$CONMON_PIDFILE if [ "$status" -ne 0 ]; then - return + die "conmon failed with status $status: $output" fi # The pid of the conmon just started. A test starting more than one has From 93ad2c75c4ba0ef6f727aa716247ec11bf76fdbe Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 17:24:05 -0700 Subject: [PATCH 04/11] test: fix flaky "container finishes immediately without --stdin" The test waits for the container to reach the "stopped" state and then reads the container log, expecting the container output to be there. This is racy: the container being stopped does not mean conmon is done writing the log, and the test can (and in CI does) read an empty log: not ok 92 attach: container finishes immediately without --stdin # (from function `bail-now' in file conmon/test/test_helper.bash, line 519, # from function `assert' in file conmon/test/test_helper.bash, line 635, # in test file conmon/test/07-attach.bats, line 37) # `assert "${output}" =~ "Container stopped!" "'Container stopped!' found in the log"' failed #| FAIL: 'Container stopped!' found in the log #| expected: =~ Container stopped\! #| actual: '' conmon having exited does mean the log is complete, so add a helper waiting for that, and use it in the affected test. Signed-off-by: Kir Kolyshkin --- test/07-attach.bats | 3 +++ test/test_helper.bash | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/test/07-attach.bats b/test/07-attach.bats index 9370a6f4..94b7fa46 100644 --- a/test/07-attach.bats +++ b/test/07-attach.bats @@ -30,6 +30,9 @@ teardown() { # Pipe is closed without --stdin, so `/cat` does not hang indefinitely, but finishes. start_conmon_with_default_args --log-path "k8s-file:$LOG_PATH" wait_for_runtime_status "$CTR_ID" stopped + # The container being stopped does not mean conmon is done writing its + # output to the log; wait for conmon to exit before reading the log. + wait_for_conmon_exit "$CONMON_PID" # Check that log file was created assert_file_exists "$LOG_PATH" diff --git a/test/test_helper.bash b/test/test_helper.bash index 1262ef5d..64e22831 100644 --- a/test/test_helper.bash +++ b/test/test_helper.bash @@ -406,6 +406,23 @@ wait_for_runtime_status() { die "timed out waiting for '$expected_status' from $cid" } +# Helper function to wait until the conmon process $pid has exited. +# +# The container reaching the "stopped" state does not mean its output has made +# it to the log yet; conmon having exited does. +wait_for_conmon_exit() { + local pid=$1 + local how_long=${2:-10} + + local t1=$((SECONDS + how_long)) + while [ "$SECONDS" -lt "$t1" ]; do + kill -0 "$pid" 2>/dev/null || return 0 + sleep 0.1 + done + + die "timed out waiting for conmon (pid $pid) to exit" +} + # _run_conmon runs conmon with the default arguments plus the ones given, # leaving the result in $status and $output as `run` does. $CONMON_PIDFILE and # $CONTAINER_PIDFILE are set to the pidfiles this conmon was told to write. From dac7fc9aa9f766d112bc6e2bc7bf3129ccb305c6 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 17:24:28 -0700 Subject: [PATCH 05/11] test: wait for conmon in the remaining log checks The same pattern as fixed by the previous commit -- wait for the container to be stopped, then read a conmon log -- is used in a few more places. These have not been seen to fail, but the race is the same, so convert them, too. Where a test starts more than one conmon, $CONMON_PID is saved right after the start, and only the conmon(s) owning the log(s) actually read are waited for. Note 10-ctrl.bats "rotate logs with --log-rotate": its second conmon is started through run_conmon_with_default_args, which waits for that one, while the rotated logs it goes on to read belong to the first. The most common instance of the pattern is run_conmon_with_default_args itself, so the barrier goes right into it, covering all of its callers. This one has been seen to fail: not ok 45 ctr logs: k8s partial message # `assert "${output}" =~ "stdout P"' failed #| expected: =~ stdout P #| actual: '' in the coverage job, where conmon is built with --coverage and is thus slow enough to lose the race more often. Signed-off-by: Kir Kolyshkin Co-Authored-By: Claude Opus 5 --- test/08-exec.bats | 6 ++++++ test/10-ctrl.bats | 8 ++++++++ test/test_helper.bash | 2 ++ 3 files changed, 16 insertions(+) diff --git a/test/08-exec.bats b/test/08-exec.bats index 76a18725..cfdc7c2e 100644 --- a/test/08-exec.bats +++ b/test/08-exec.bats @@ -16,13 +16,18 @@ teardown() { @test "exec: simple --exec --exec-process-spec" { start_conmon_with_default_args --log-path "k8s-file:$LOG_PATH" wait_for_runtime_status "$CTR_ID" running + local main_conmon_pid=$CONMON_PID start_conmon_with_default_args \ --log-path "k8s-file:$LOG_PATH.exec" \ --exec \ --exec-process-spec "${BUNDLE_PATH}/process.json" + local exec_conmon_pid=$CONMON_PID wait_for_runtime_status "$CTR_ID" stopped + # Both logs are read below, so wait for both conmons to write them out. + wait_for_conmon_exit "$main_conmon_pid" + wait_for_conmon_exit "$exec_conmon_pid" # Check that the main process noticed the /tmp/test.txt. assert_file_exists "$LOG_PATH" @@ -133,6 +138,7 @@ teardown() { # The exec should start now. wait_for_runtime_status "$CTR_ID" stopped + wait_for_conmon_exit "$CONMON_PID" assert_file_exists "$LOG_PATH.exec" run cat "$LOG_PATH.exec" assert "${output}" =~ "Hello from exec!" diff --git a/test/10-ctrl.bats b/test/10-ctrl.bats index 6844cf44..c1ac3648 100644 --- a/test/10-ctrl.bats +++ b/test/10-ctrl.bats @@ -18,6 +18,7 @@ test_ctl_command() { local command="$1" start_conmon_with_default_args --log-path "k8s-file:$LOG_PATH" -t wait_for_runtime_status "$CTR_ID" running + local main_conmon_pid=$CONMON_PID echo "$command" > ${CTL_PATH} @@ -27,6 +28,8 @@ test_ctl_command() { --exec-process-spec "${BUNDLE_PATH}/process.json" wait_for_runtime_status "$CTR_ID" stopped + # Callers read $LOG_PATH, written by the container's conmon. + wait_for_conmon_exit "$main_conmon_pid" } # Helper function to send the resize command. Fails if the resize command @@ -87,6 +90,7 @@ test_resize_command_ok() { @test "ctrl: rotate logs" { start_conmon_with_default_args --log-path "k8s-file:$LOG_PATH" -t wait_for_runtime_status "$CTR_ID" running + local main_conmon_pid=$CONMON_PID # Remove the log. rm -f $LOG_PATH @@ -99,6 +103,7 @@ test_resize_command_ok() { --exec-process-spec "${BUNDLE_PATH}/process.json" wait_for_runtime_status "$CTR_ID" stopped + wait_for_conmon_exit "$main_conmon_pid" # Check that the log exists now. assert_file_exists "$LOG_PATH" @@ -166,6 +171,7 @@ test_resize_command_ok() { -t \ --log-rotate wait_for_runtime_status "$CTR_ID" running + local main_conmon_pid=$CONMON_PID # The control message should rotate the log echo "2 1 1" > ${CTL_PATH} @@ -174,6 +180,8 @@ test_resize_command_ok() { --log-path "k8s-file:$LOG_PATH.exec" \ --exec \ --exec-process-spec "${BUNDLE_PATH}/process.json" + # $LOG_PATH and $LOG_PATH.1, read below, are the main conmon's. + wait_for_conmon_exit "$main_conmon_pid" assert_file_exists "$LOG_PATH.exec" run cat "$LOG_PATH.exec" diff --git a/test/test_helper.bash b/test/test_helper.bash index 64e22831..789177b8 100644 --- a/test/test_helper.bash +++ b/test/test_helper.bash @@ -502,6 +502,8 @@ start_conmon_with_default_args() { run_conmon_with_default_args() { start_conmon_with_default_args "$@" wait_for_runtime_status "$CTR_ID" stopped + # Every caller reads a log written by this conmon afterwards. + wait_for_conmon_exit "$CONMON_PID" } # Generic helper function to create pipe and read from it. From bd382e0827cb3f9e99fc005ef388acc58c011c83 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 17:24:28 -0700 Subject: [PATCH 06/11] test: fix bogus wait in 04-runtime The intent was to wait for conmon to exit, but the argument is the name of the pidfile rather than the pid it contains, so bash rightfully complains wait: `/tmp/.../conmon-pidfile': not a pid or valid job spec which the test then discards along with the rest of the errors. In other words, this waits for nothing at all. Use wait_for_conmon_exit instead. The sleep above it is left as is: it is what gives the sync pipe reader time to write out what conmon sent it. Signed-off-by: Kir Kolyshkin --- test/04-runtime.bats | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/04-runtime.bats b/test/04-runtime.bats index 682e1aae..2745c3bd 100644 --- a/test/04-runtime.bats +++ b/test/04-runtime.bats @@ -116,7 +116,7 @@ teardown() { assert_file_exists $CONMON_PID_FILE CONMON_PID=$(cat "$CONMON_PID_FILE") - wait $CONMON_PID_FILE 2>/dev/null || true + wait_for_conmon_exit "$CONMON_PID" # Check that the error is sent to the sync pipe. assert_file_exists $TEST_TMPDIR/syncpipe-output From 12f0be0b685ff5747840a9d4dd1b3c2a4cb3b252 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 20:42:29 -0700 Subject: [PATCH 07/11] test: bump the wait_for_runtime_status timeout Five seconds for a container to change state is not much when the CI runner is loaded, and it has been observed to be too little: not ok 51 runtime: container execution with multiple log drivers not ok 101 ctrl: resize the terminal, negative width and height #| FAIL: timed out waiting for 'stopped' from conmon-test-... The helper polls, so a longer limit costs nothing when things are well -- it returns as soon as the state is reached -- and only comes into play where the alternative is a spurious failure. Make it 30 seconds. Signed-off-by: Kir Kolyshkin Co-Authored-By: Claude Opus 5 --- test/test_helper.bash | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/test_helper.bash b/test/test_helper.bash index 789177b8..b008fbbe 100644 --- a/test/test_helper.bash +++ b/test/test_helper.bash @@ -391,7 +391,10 @@ assert_stderr_contains() { wait_for_runtime_status() { local cid=$1 local expected_status=$2 - local how_long=5 + # Generous on purpose: this polls, so on a healthy machine it returns on + # the first iteration, and the only thing a low limit buys is flakes on a + # loaded CI runner. + local how_long=30 t1=$(expr $SECONDS + $how_long) while [ $SECONDS -lt $t1 ]; do From 15cf328d72fb0eaa5e465ab891ad2d20c1ea8da8 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 20:42:18 -0700 Subject: [PATCH 08/11] test: time out the image pull setup_suite pulls the image the test rootfs is made of. It is the only network access in the whole suite, podman has no timeout of its own, and a hang there stops the suite before a single test runs -- with no output at all, since bats shows setup_suite's output only once it completes. The test step has been seen hanging for 35 minutes with exactly that signature. Bound the pull, and say in the failure message that a timeout is a possibility. Signed-off-by: Kir Kolyshkin Co-Authored-By: Claude Opus 5 --- test/setup_suite.bash | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/setup_suite.bash b/test/setup_suite.bash index a4d94769..242e1453 100644 --- a/test/setup_suite.bash +++ b/test/setup_suite.bash @@ -34,8 +34,11 @@ setup_suite() { # reason for the failure is the whole point. # NB: no --policy here, it is not supported by podman < 5.0 (as found # on e.g. Ubuntu 24.04), and plain "podman pull" pulls anyway. - if ! podman pull "$UBI10_MICRO_IMAGE"; then - suite_fail "failed to pull $UBI10_MICRO_IMAGE" + # The pull is the one thing here that talks to the network, and podman + # has no timeout of its own, so a stalled registry hangs the whole suite + # before a single test runs. Five minutes is plenty for a ~15 MB image. + if ! timeout 300 podman pull "$UBI10_MICRO_IMAGE"; then + suite_fail "failed to pull $UBI10_MICRO_IMAGE (timed out?)" return 1 fi From 7c960401210da0694b6722913dcb94ebf8c1657f Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 21:14:07 -0700 Subject: [PATCH 09/11] test: bound the podman calls in the exec exit status test "integration: exec exit codes work correctly" runs six podman commands, of which exactly one -- the initial probe -- has a timeout. The rest can hang forever, and one of them did: ok 67 exec requires proper arguments <17 minutes of nothing> The job has exceeded the maximum execution time of 20m0s Test 68 is this one. Nothing is printed while it hangs, because bats shows a test's output only once the test ends, so the suite just goes quiet -- and before the job timeout added earlier in this series, quiet for up to six hours. The container creation is the likeliest culprit: container_id=$(podman --conmon $conmon_path run -dt ... sleep 30) a command substitution waits for stdout to be closed, and conmon is started by podman with that very stdout -- this test runs the freshly built conmon on purpose, so it is exactly the thing that may hold it open. Wrap every podman invocation here in a timeout. A hang now fails the test in a minute, with the rest of the suite still running. Signed-off-by: Kir Kolyshkin Co-Authored-By: Claude Opus 5 --- test/06-exec-exit-status.bats | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/test/06-exec-exit-status.bats b/test/06-exec-exit-status.bats index d250e374..6cb48763 100755 --- a/test/06-exec-exit-status.bats +++ b/test/06-exec-exit-status.bats @@ -50,6 +50,11 @@ teardown() { fi # Check if we can create a simple container for testing. + # + # NB: every podman invocation in this test is wrapped in a timeout. None + # of them has any business taking long, and an unbounded one hangs the + # whole suite -- bats runs tests serially, and a command substitution + # waits for stdout to be closed, which a misbehaving conmon may never do. run timeout 10 podman --conmon $conmon_path run --rm "$UBI10_MICRO_IMAGE" true if [ "$status" -ne 0 ]; then die "cannot create test containers with podman: $output" @@ -59,37 +64,37 @@ teardown() { # Create a test container local container_id - container_id=$(podman --conmon $conmon_path run -dt "$UBI10_MICRO_IMAGE" sleep 30) + container_id=$(timeout 60 podman --conmon $conmon_path run -dt "$UBI10_MICRO_IMAGE" sleep 30) if [ -z "$container_id" ]; then die "failed to create test container" fi # Test 1: Success case - if ! podman --conmon $conmon_path exec "$container_id" true; then - podman --conmon $conmon_path rm -f "$container_id" >/dev/null 2>&1 + if ! timeout 60 podman --conmon $conmon_path exec "$container_id" true; then + timeout 60 podman --conmon $conmon_path rm -f "$container_id" >/dev/null 2>&1 echo "FAIL: true command should succeed" return 1 fi # Test 2: Failure case - this would fail with the regression - if podman --conmon $conmon_path exec "$container_id" false; then - podman --conmon $conmon_path rm -f "$container_id" >/dev/null 2>&1 + if timeout 60 podman --conmon $conmon_path exec "$container_id" false; then + timeout 60 podman --conmon $conmon_path rm -f "$container_id" >/dev/null 2>&1 echo "FAIL: false command should fail (regression detected!)" echo "This indicates the fc0a342 regression where all exec commands return 0" return 1 fi # Test 3: Custom exit code - this would return 0 with the regression - if podman --conmon $conmon_path exec "$container_id" sh -c 'exit 42'; then - podman --conmon $conmon_path rm -f "$container_id" >/dev/null 2>&1 + if timeout 60 podman --conmon $conmon_path exec "$container_id" sh -c 'exit 42'; then + timeout 60 podman --conmon $conmon_path rm -f "$container_id" >/dev/null 2>&1 echo "FAIL: 'exit 42' should fail with code 42 (regression detected!)" echo "This indicates the fc0a342 regression where all exec commands return 0" return 1 fi # Clean up - podman --conmon $conmon_path rm -f "$container_id" >/dev/null 2>&1 + timeout 60 podman --conmon $conmon_path rm -f "$container_id" >/dev/null 2>&1 echo "Integration test passed: exec exit codes work correctly" } \ No newline at end of file From 5cd3a5b08df5a8e4d1fb4125bd40b45c9088fd3d Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 20:41:14 -0700 Subject: [PATCH 10/11] ci: set job timeouts The GitHub Actions default job timeout is 6 hours, so a job that hangs squats a runner for the rest of the day instead of failing. This is not hypothetical: today both the setup step (apt, five git clones, wget) and the test step (which starts with a podman pull) have been seen hanging on what looks like stalled network I/O, and one such job was still sitting there 2.5 hours later. Give the jobs timeouts a few times their normal runtime, so a hang fails in minutes rather than hours: the conmon job normally takes about 4 minutes and the cri-o one about 40. Note static.yml already sets one (360, the default), left alone here. Signed-off-by: Kir Kolyshkin Co-Authored-By: Claude Opus 5 --- .github/workflows/coverage.yml | 1 + .github/workflows/integration.yml | 2 ++ .github/workflows/validate.yml | 1 + 3 files changed, 4 insertions(+) diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 543a2121..393c1f4c 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -12,6 +12,7 @@ jobs: coverage: runs-on: ubuntu-latest + timeout-minutes: 20 steps: - uses: actions/checkout@v7 - name: Install dependencies diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 2ecde8b2..7e180d1e 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -12,6 +12,7 @@ jobs: conmon: runs-on: ubuntu-latest + timeout-minutes: 20 steps: - uses: actions/checkout@v7 - run: sudo hack/github-actions-setup @@ -22,6 +23,7 @@ jobs: cri-o: runs-on: ubuntu-latest + timeout-minutes: 90 strategy: matrix: go-version: [stable, oldstable] diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index a89e0120..31ca5740 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -13,6 +13,7 @@ jobs: lint: runs-on: ubuntu-latest + timeout-minutes: 10 steps: - uses: actions/checkout@v7 - name: Check C code formatting From 1286f9a6c72c80310f9b9ea5a7f4476a251598a7 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 18 Aug 2026 20:42:07 -0700 Subject: [PATCH 11/11] ci: time out and retry network access in the setup script This script clones five repositories, downloads a tarball and a repo key, and runs apt, and not one of those has a usable timeout by default: - git has none at all; - curl has no total timeout and no retries, only a 300 second connect timeout, so a connection that stalls mid-transfer hangs forever; - wget does have a 900 second read timeout, but also 20 tries by default, which adds up to hours; - apt retries nothing by default. This is not theoretical: this step has been seen hanging for 49 minutes, and was still going when the job was cancelled. Give each of them a bound. The git ones get GIT_HTTP_LOW_SPEED_LIMIT and GIT_HTTP_LOW_SPEED_TIME, which abort a connection that transfers less than 1 KiB/s for a minute, and cover all five clones at once. apt needs more than its own options. Acquire::*::Timeout applies to individual requests of the acquisition method, while the frontend waits for that method indefinitely, which is exactly what happened: 04:56:18 + sudo apt -o Acquire::Retries=3 ... update 04:57:27 Get:5 https://archive.ubuntu.com/ubuntu noble-security InRelease [126 kB] <19 minutes of nothing> 05:16:30 The job has exceeded the maximum execution time of 20m0s Five of ten parallel jobs died that way in one experiment, all of them after apt gave up on the azure.archive.ubuntu.com mirror (a screenful of "Ign:") and fell back to archive.ubuntu.com. So bound the whole command and retry it. Also stop waiting on the dpkg lock, which a timed out run may well have left behind, and switch to apt-get, whose CLI is the one meant for scripts. The two apt-get invocations get rather different limits. "update" is network and nothing else, so three minutes is generous. "install" unpacks and configures a few dozen packages, and one run was killed at the three minute mark just as it was processing triggers, leaving dpkg with a half-applied transaction that no amount of retrying could fix: Processing triggers for install-info (7.1-3build2) ... apt-get install: failed or timed out, attempt 1 of 3 E: dpkg was interrupted, you must manually run 'sudo dpkg --configure -a' to correct the problem. So give it ten minutes, and run dpkg --configure -a before each retry, so that a half-applied transaction is recovered rather than repeated. Signed-off-by: Kir Kolyshkin Co-Authored-By: Claude Opus 5 --- hack/github-actions-setup | 56 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/hack/github-actions-setup b/hack/github-actions-setup index 84d6e8b8..c77da829 100755 --- a/hack/github-actions-setup +++ b/hack/github-actions-setup @@ -15,6 +15,11 @@ declare -A VERSIONS=( main() { set -x + # None of the git clones below has a timeout of its own, and a stalled + # one hangs this script until the job times out. Make git give up on a + # connection that transfers less than 1 KiB/s for a minute. + export GIT_HTTP_LOW_SPEED_LIMIT=1024 GIT_HTTP_LOW_SPEED_TIME=60 + prepare_system install_packages @@ -61,15 +66,56 @@ remove_runtimes() { sudo rm -f /usr/{local/,}{s,}bin/{runc,crun} } +APT_OPTS=( + -o Acquire::Retries=3 + -o Acquire::http::Timeout=60 + -o Acquire::https::Timeout=60 + # Fail rather than wait for a lock a timed out run may have left behind. + -o DPkg::Lock::Timeout=60 +) + +# apt_get runs apt-get with the options above, under a timeout, retrying a few +# times. +# +# The Acquire::*::Timeout settings apply to individual requests of the +# acquisition method, and are not enough on their own: a run has been seen +# printing a few "Get:" lines and then sitting there for 19 minutes, until the +# job timed out. Bounding the whole command is the only thing that reliably +# helps, and since a stalled mirror is a transient thing, retrying it is +# usually all it takes. +apt_get() { + local try timeout=180 + + # "install" is not just a download: it unpacks and configures a few dozen + # packages, and needs a good deal more time than a metadata refresh. Being + # killed in the middle of that leaves dpkg with a half-applied + # transaction, which retrying does not fix by itself -- hence the + # --configure below. + [ "$1" = "install" ] && timeout=600 + + for try in 1 2 3; do + if sudo timeout "$timeout" apt-get "${APT_OPTS[@]}" "$@"; then + return 0 + fi + echo "apt-get $1: failed or timed out, attempt $try of 3" >&2 + sudo dpkg --configure -a || true + sleep 10 + done + + return 1 +} + install_packages() { . /etc/os-release CRIU_REPO="https://download.opensuse.org/repositories/devel:/tools:/criu/xUbuntu_$VERSION_ID" - curl -fSsL $CRIU_REPO/Release.key | sudo gpg --dearmor -o /etc/apt/trusted.gpg.d/criu.gpg + # curl has no total timeout by default, and no retries at all. + curl -fSsL --retry 5 --retry-delay 3 --max-time 120 "$CRIU_REPO"/Release.key | + sudo gpg --dearmor -o /etc/apt/trusted.gpg.d/criu.gpg echo "deb $CRIU_REPO/ /" | sudo tee /etc/apt/sources.list.d/criu.list - sudo apt update - sudo apt install -y \ + apt_get update + apt_get install -y \ autoconf \ automake \ conntrack \ @@ -134,7 +180,9 @@ install_cni_plugins() { TARBALL=cni-plugins-linux-amd64-${VERSIONS["cni-plugins"]}.tgz CNI_DIR=/opt/cni/bin sudo mkdir -p "$CNI_DIR" - wget -O "$TARBALL" $URL/"${VERSIONS["cni-plugins"]}"/"$TARBALL" + # wget defaults to 20 tries and a 900 second read timeout; that is a lot + # of patience for a job that should take seconds. + wget --timeout=60 --tries=3 -O "$TARBALL" $URL/"${VERSIONS["cni-plugins"]}"/"$TARBALL" sudo tar xf "$TARBALL" -C "$CNI_DIR" rm "$TARBALL" ls -lah "$CNI_DIR"