Retry the job image pull so a registry reset cannot kill the whole job

[groeneai]

Changelog category (leave one):

• CI Fix or Improvement (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Retry the praktika job's Docker image pull on transport-class registry errors, so a connection reset during the image transfer no longer kills the whole job before any test runs.

Description

docker run pulls the job image implicitly, inside the same invocation that runs the job, and praktika executes that invocation exactly once (TeePopen has no retry facility). So a registry transfer reset there kills the job with zero tests executed, reported as a bare job-level error:

docker: failed to copy: read tcp <runner>:<port>-><registry>:443: read: connection reset by peer

praktika can retry a registry interaction it owns as a distinct step, and the docker.py image build does. The job image pull was not a step at all, so nothing could carry a retry.

Runner._run now pulls the image explicitly before TeePopen, retried on transport-class errors only. Retrying docker run itself is not an option: {job.command} sits inside that command string, so a retry would re-run a whole test job, and rc=125 is ambiguous anyway (a container command exiting 125 returns a pull failure's rc).

Three properties keep this narrow, each measured on docker 29.5.1:

• The pulled command holds no job command, so nothing a job prints can reach the matcher; and retry_errors matches stderr, while a successful pull writes its progress to stdout (335 bytes stdout, 0 stderr).
• Transport phrases only, so manifest unknown / pull access denied / no matching manifest still fail on the first attempt.
• Skipped when the image is present, since a bare docker run then contacts no registry: the warm case is unchanged.

Fail-open is required rather than defensive: an image built locally by this workflow cannot be pulled at all (docker pull gives pull access denied while docker run on that tag succeeds).

Each attempt is bounded, since job.timeout only starts with TeePopen. The bound is timeout --verbose, not Shell.run(timeout=...), whose SIGTERMed child writes nothing to stderr, so the loop would stop after one attempt (measured 1 vs 3).

Reproduction, validation, and why this is not covered by retry_infra_failures.yml

Reproduced end to end against a local registry:2 behind a proxy resetting the downstream direction mid-transfer: rc=125, same error text, no container residue.

A cut-point sweep showed the same fault produces three different messages depending on where the transfer dies; only connection reset by peer matches all three.

Validated in both directions through praktika's real retry loop: a transport reset is retried and succeeds; a permanent error is attempted exactly once. The tests need no docker daemon, and a mutation matrix covers them: dropping --verbose or its allowlist entry each makes the stall protection dead and reddens its own arm.

retry_infra_failures.yml names "Docker image pull failures" and does decide should_rerun=true here, but it is complementary, and three gaps are measured: it reruns the whole workflow; it only selects attempt == 1 (:37), and the motivating sighting was already on attempt 2, so a reset there is terminal; and it is PR-only (:33), so the master occurrence had no retry path. This fixes the pull where it happens, on master too, on any attempt.

All three occurrences, 2026-07-01 to 2026-08-06 (CIDB, each check_status = error with an empty test_name, i.e. no test row):

when	job	where
2026-07-31	Stateless tests (amd_asan_ubsan, db disk, distributed plan, sequential, 1/3)	PR 109212, ca19728c9e86
2026-07-09..14	Build (amd_release)	master
2026-07-09..14	Build (arm_binary)	PR 107775

The first, verbatim after 17 Pull complete layers:

809bd340b23b: Pull complete
docker: failed to copy: read tcp 172.31.94.218:51334->54.231.230.177:443: read: connection reset by peer

Run 'docker run --help' for more information

Rare, but each costs an entire job and no PR diff can influence it.

[groeneai]

Internal second-model review (click to expand)

Four review rounds, each with an independent cold pass plus a second-model gate against
the frozen PR contract. The final round returned no findings in either channel.

❌ Blockers found and fixed during review

#	Finding	Resolution
1	The regression tests could not pass on the runner that runs them. Three of the seventeen cases compared the pull command against a hardcoded clickhouse/test-base:...**_amd**, but Runner._run derives that suffix from the host architecture, and the only job running this suite (CI Tests) is on arm. Every local run was green because dev boxes are amd.	The shared fixture now pins the architecture. Verified with an aarch64 shim: reverting only those two lines is green on amd and red on arm with exactly those three cases failing, and swapping the two suffixes in the runner turns the suite red on both architectures, so the expectation is pinned to a constant rather than mirroring the code under test.
2	The design's central safety property, that the pull command carries no job command so a retry cannot re-run the job, was stated in the PR body and in the test docstring and pinned by nothing. Appending the job command to the pull left every arm green.	An arm now asserts the pull command ends at the image. Its specificity was measured, not assumed: the pre-existing substring check survives that mutation, so with only the new assertions reverted the suite reads green again.
3	The arm claiming to pin the stderr-only retry matching was a vacuous oracle. Its fixture carried no allowlisted phrase, so it read one attempt under a stderr-only matcher and under one that also read stdout.	A new arm sends the verbatim production error to stdout with an empty stderr and requires one attempt, while its sibling sends the same line to stderr and is retried. Under a matcher patched to read both streams the new arm reddens and the old one still passes.
4	Four of the six allowlist entries were unpinned: dropping any of them left the suite green.	One parametrized case per entry, each verified one-to-one before being written (the substring matrix shows each production-shaped line matches exactly one entry, so dropping one cannot redden another's case).

⚠️ Claims corrected rather than code changed

Claim	Correction
"praktika already retries transient registry errors everywhere else it owns a registry interaction"	A false universal, refuted inside praktika's own docker.py: docker login there (:146) is a distinct, praktika-owned, strict=True registry step with no retry, and DockerImage.pull_image() (docker_image.py:27) is another. Narrowed to what the code supports: the docker.py image build does retry.
"The three sibling docker run invocations"	Two. The post-job chown and "the chown after a root job" are the same line, since its guard is ... and from_root.
"A mutation matrix pins each arm"	Three arms are reddened by no mutant, and all three are controls rather than gaps: one is the retried counterpart of the stdout arm, one is the progress arm that by construction carries no allowlisted phrase, and one extends the permanent-error arm with two phrases no mutant in the matrix touches. Changed to "covers them".
Test/mutant/suite counts in the PR body	Every round that added arms falsified them again. Rephrased so the body states the properties rather than digits that go stale.
"five jobs pull their own image through DockerImage.pull_image()"	Five is the call-site count. The job count is unstable across derivations (23 by config, 22 distinct names across four workflows). Restated with the grep that reproduces it: five call sites across four job scripts.

💡 Reviewed and deliberately not changed

• Add --kill-after to the per-attempt bound. Raised as a blocker: timeout sends only
  SIGTERM, so a stalled pull might not be hard-bounded. Refuted on docker 29.5.1 with a
  positive control. A stalled pull under timeout --verbose 5 returns 124 at 5.01s, and a
  genuinely mid-transfer one under --verbose 6 at 6.01s, while a child that really ignores
  SIGTERM (trap "" TERM; sleep 30) does hold timeout for the full 30.01s. The transfer runs
  in dockerd, a system service rather than a CLI child, so there is no unbounded descendant
  to orphan. The in-tree precedent uses plain timeout, so this form is already stronger.
• The worst-case pull time versus job.timeout. Three bounded attempts plus backoff sit
  outside job.timeout, and the generated workflow grants little slack above 360 minutes. Not
  reachable: over 30 days every docker job with a timeout at or above 345 minutes keeps more
  headroom than the worst case consumes.
• Locale sensitivity of the sending signal TERM to command entry. That message is a
  translatable gettext string, so a translated locale would break the match. coreutils ships no
  translation catalogues on the runner image, and under the en_US.UTF-8 the base image sets,
  only the quote characters change, outside the matched substring. Verified across five locales.
• One mutation is knowingly uncaught. Widening the allowlist with a bare lowercase
  timeout reddens no arm. That is measured behaviour-neutral, not a gap: matching is
  case-sensitive substring matching and no permanent docker error reaching this matcher
  contains it (docker writes Client.Timeout). The guard against an over-broad allowlist is
  the permanent-error arm, which does redden when the list is replaced by ["docker"].

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes. A local registry:2 behind a TCP proxy that resets the downstream direction once a global byte budget is exhausted; a synthetic layer blob pushed through the registry API so the daemon has provably never seen its digest. docker run on that image gives rc=125 with the production error text, 3/3 deterministic per budget. A per-connection cut is not enough: docker resumes the blob with a Range request and eventually completes, so the budget must be global.
b	Root cause explained?	docker run pulls the image implicitly, inside the same invocation that runs the job, and Runner._run executes that invocation exactly once through TeePopen, which has no retry facility (no retries, no retry_errors, no loop). A registry transfer reset therefore makes docker run exit 125 before the container ever starts, so the job dies with zero tests executed. praktika retries transport errors at every other registry interaction it owns as a distinct step; the job image pull was not a distinct step, so it inherited no retry.
c	Fix matches root cause?	Yes: the pull becomes its own retried, bounded step. Not a band-aid, and deliberately not a retry of docker run: {job.command} sits inside that command string, so a retry would re-run a whole test job, and rc=125 cannot discriminate the two cases (a container command exiting 125 returns docker rc=125, measured, identical to a pull failure).
d	New tests added?	A new regression file ci/tests/test_job_image_pull_retry.py. Most of its arms drive praktika's real Shell.run retry loop with fake shell commands, so they exercise the actual matching semantics rather than a model of them; the rest drive Runner._run's docker branch with stubbed collaborators. No docker daemon and no jq required. A mutation matrix covers them, and all but one mutant is caught (the uncaught one is uncaught by design and measured: widening the allowlist with a bare lowercase "timeout" is behaviourally inert, because matching is case-sensitive substring matching and none of the permanent docker errors that can reach this matcher contains that substring, docker's own deadline error being Client.Timeout with a capital T). The arm pinning the stderr-only matching split sends the verbatim production error to stdout and requires a single attempt, so it reddens under a mutant that makes the matcher read both streams, while the sibling arm sending that same line to stderr is retried.
e	Both directions demonstrated?	Yes. A transport reset is retried and succeeds (attempt 2), a permanent error is attempted exactly once, and the stall bound retries 3 times with timeout --verbose versus 1 time without it. Each direction is measured through the real retry loop, not asserted.
f	General across code paths?	grep -rn 'docker run' ci/praktika/ returns three non-comment sites: the job invocation this PR fixes (runner.py:456), the root-job chown at runner.py:521, and Utils.fix_ownership_after_docker (utils.py:805). Both siblings run after the job on an image that is therefore already local, so neither contacts a registry, and both are already on Shell.run if that ever changes. Separately, DockerImage.pull_image() (ci/jobs/scripts/docker_image.py:27) pulls an image outside the runner: grep -rn 'pull_image(' ci/ --include=*.py gives five call sites across four job scripts (ast_fuzzer_job.py, install_check.py, libfuzzer_test_check.py, stress_job.py), which parametrize into many more actual jobs. That is a different surface and out of scope here.
g	Generalizes across inputs?	A cut-point sweep showed the same physical fault produces three different messages depending on where the transfer dies (manifest HEAD, manifest GET, blob transfer). Only connection reset by peer matches all three, so the allowlist is keyed on it. The plan's original leading phrase failed to copy: read tcp matches the docker run wording but not what docker pull emits, so it was dropped rather than shipped as decoration.
h	Backward compatible?	Yes. No settings, no serialization, no ClickHouse code: one Python file under ci/ plus one new test file. The added step is a no-op when the image is present, and a failed pull is non-fatal, so behaviour with an unreachable registry is exactly what it is today.
i	Invariants preserved?	The pull is placed after docker is resolved to name:tag and after the stale-container cleanup, and before HostMetricsCollector().start(), so it is not counted as job CPU/RAM and cannot shift the container name or its cleanup. Fail-open is required, not defensive: an image built locally by this workflow cannot be pulled at all (docker pull gives pull access denied while docker run on that tag succeeds), so strict=True would break every locally-built image. docker pull takes no --name, and a failed pull leaves no container, so no name collision is possible on retry.

One mutation is knowingly uncaught: widening the allowlist with a bare timeout reddens no arm. That is measured behaviour-neutral rather than a gap, because no permanent docker error contains that substring (docker writes Client.Timeout, capital T, and matching is case-sensitive), and an arm for it would pin spelling rather than behaviour. The protection against an over-broad allowlist is the permanent-error arm, which does redden when the list is replaced by ["docker"].

Session id: cron:clickhouse-impl-slot-5:20260805-183400

[groeneai]

cc @maxknv @leshikus, could you review this? docker run pulls the job image implicitly inside the same invocation that runs the job, and TeePopen has no retry facility, so a registry transfer reset killed the whole job with zero tests run; the pull now happens as its own step before TeePopen, guarded by docker image inspect, bounded per attempt and retried on transport-class errors only.

[maxknv]

@groenai

1. add run examples with failures
2. add workflow's warning (see add_workflow_warning) if a pull failure was retried

[groeneai]

Both done in c7c4c55.

1. Run examples. Added to the PR body. All three occurrences between 2026-07-01 and
2026-08-06, each check_status = error with an empty test_name, so no test row at all:

when	job	where
2026-07-31	Stateless tests (amd_asan_ubsan, db disk, distributed plan, sequential, 1/3)	PR 109212, ca19728c9e86
2026-07-09..14	Build (amd_release)	master
2026-07-09..14	Build (arm_binary)	PR 107775

The first, verbatim after 17 Pull complete layers:

809bd340b23b: Pull complete
docker: failed to copy: read tcp 172.31.94.218:51334->54.231.230.177:443: read: connection reset by peer

Run 'docker run --help' for more information

Three different job families, one on master. Worth noting for anyone re-running the census:
test_status must be left unfiltered, because a job-level abort writes no test row, and
length(test_context_raw) < 5000 is what separates these from the Dockers Build /
Docker keeper image failures, whose pull BUILDX_RETRY_ERRORS already retries.

2. add_workflow_warning on a retried pull. Shell.run gained an optional on_retry
callback, invoked only where a retry is actually about to be issued, so a permanent error
never reaches it. The runner passes a hook that reports the matched phrase and the image.

Two things I measured while wiring it, both of which changed the implementation:

• I used env.add_workflow_warning on _run's own _Environment rather than Info().
  Info() reads a second copy from disk, and a later dump of the in-frame object discards
  the message: measured, the fresh-from-disk warning list goes back to empty. runner.py
  already uses env.add_workflow_error at five sites.
• The callback is wrapped. Shell.run's pre-existing except Exception around the loop
  body reports an exception-terminated attempt as exit code 1, so a raising hook would
  replace the real exit code (124 for a stalled pull) with 1 and hide the failure.

It also fires only when another attempt follows, and counts retries rather than attempts:
the last iteration matches the allowlist too, but nothing is retried after it, so without
that guard a terminal failure was reported as "retried (3/3)".

Five test cases cover it: the warning fires on a retry, is absent for a clean pull and for
an image already present, fires once per retry issued and never for a permanent error, and a
raising hook changes neither the exit code nor the retry outcome. Each is pinned by its own
mutant, including one that removes the terminal-attempt guard.