feat: retry-with-backoff for the parallel publish path#642
Open
ianpittwood wants to merge 16 commits into
Open
feat: retry-with-backoff for the parallel publish path#642ianpittwood wants to merge 16 commits into
ianpittwood wants to merge 16 commits into
Conversation
…sh ordered Rewrites ImageToolsPlugin.publish() to fan Stage 1 (wait + index-create + soci-convert) out across targets via the parallel ShellJob executor, while keeping Stage 2 (index-copy, the real registry push) and Stage 3 (verify) as sequential push_sort_key-ordered loops -- one target's Stage 1 failure no longer blocks any other target's publish. Also fixes test/cli/test_ci.py's merge.feature fixture (patch_image_target_merge_method), whose MockOrasIndexCreateWorkflow.run() didn't accept the runner kwarg that publish() now always passes through _run_publish_stage1.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rrupt handling Adds three test cases that were in the task brief but missing from the implementation: - test_job_value_is_callable_return: Verifies job callable return values are captured - test_on_result_called_once_per_job_on_main_thread: Confirms on_result callback runs once per job on main thread - test_sigint_terminates_in_flight_children: Verifies interrupt safety for run_jobs() matches run() Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The Test job runs pytest against imagetools code that shells out to soci, but never installed it (unlike bakery-build-native.yml), so SOCI-dependent tests failed with BakeryToolNotFoundError. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 8, 2026
Contributor
Author
|
Tracked by #648. |
publish() had two related problems with how it resolved the soci binary: - find_soci_bin was only imported at module load time, so unittest.mock.patch() on the source module never took effect at call time inside publish() (unlike find_oras_bin, which is imported locally for exactly this reason). CI worked around the resulting test failures by installing a real soci binary (68fa1db) instead of fixing the mock. - soci_bin was resolved unconditionally, so any bakery ci publish/merge call became a hard failure without soci installed, even for configs where no target has SOCI enabled. This broke the "Merging multiplatform builds" functional test, which exercises ORAS-only merging with no SOCI configuration anywhere in its fixture. Import find_soci_bin locally to match find_oras_bin, and only treat a missing binary as fatal when at least one target in the run actually has SOCI enabled. Also memoize find_soci_bin with functools.cache, mirroring #552's convention for per-invocation idempotent lookups, so a run with multiple SOCI-enabled targets resolves the binary once instead of once per target.
68fa1db installed a real soci binary in the Test job to work around BakeryToolNotFoundError failures, but that papered over two actual bugs in how publish() resolved the soci binary rather than fixing them: - find_soci_bin was only imported at module load time, so unittest.mock.patch() on the source module never took effect at call time, causing mocked unit tests to fall through to the real function instead of the mock. - soci_bin was resolved unconditionally, even when no target in the run had SOCI enabled, making the binary a hard requirement for every bakery ci publish/merge call regardless of whether it was ever used. Both are fixed in 4f25a18, so the test suite no longer needs a real soci binary. Confirmed by running the full suite with no soci binary on PATH: no soci-related failures remain.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add retry_policy field to OrasIndexVerifyWorkflow and wrap the manifest
fetch in retry_on_transient, matching the pattern already used by
OrasIndexCreateWorkflow and OrasIndexCopyWorkflow. Runs on the main
thread, so no runner/interruptible-sleep plumbing is needed.
Also update test_run_failure_reports_partial's second failure to a
non-transient stderr message ("unauthorized: authentication required")
since "not found" is now a retryable transient pattern.
bschwedler
approved these changes
Jul 9, 2026
3feb2da to
5672335
Compare
1a65e9a to
8eec3fd
Compare
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.
Stacked on #641. Part of splitting #599 (parallelize + retry + unify oras/soci into
imagetools) into smaller, independently-reviewable pieces.What & why
Retry-with-backoff for transient registry errors (
RetryPolicy/retry_on_transientinposit_bakery/retry.py) already covered the riskiest phases of the parallel publish path introduced in #641 — index-create and the SOCI pull step, both carried forward from a pre-existing GHCR-flakiness fix. Two gaps remained:OrasIndexVerifyWorkflowdid a bareoras manifest fetch— a transient read-after-write blip failed the whole publish with zero backoff, even though every phase before it was protected.retry_on_transientslept via plaintime.sleep. A Stage-1 job (running on aParallelShellExecutorworker thread) blocked mid-backoff wouldn't notice Ctrl-C — the executor's interrupt handling returns immediately on the main thread, butThreadPoolExecutorregisters an atexit hook that joins every thread it ever spawned, so the process wouldn't actually exit until that sleep (and any remaining retry attempts) finished, up to ~62s per stuck job with default settings.Changes
retry_on_transient()gains an injectablesleepcallable (defaults totime.sleep, looked up fresh per call so existingpatch("posit_bakery.retry.time.sleep")tests keep working unmodified).CommandRunner.sleep()(new, in theparallelmodule): sleeps in short slices, checking the bound executor's shutdown flag between slices, raising a newExecutorInterruptedas soon as shutdown is observed — so a mid-backoff job unwinds within about one slice interval of Ctrl-C instead of blocking process exit.OrasIndexCreateWorkflowandSociConvertWorkflow's pull-step retries (the only retries reachable from a Stage-1ShellJobworker thread) now passsleep=runner.sleepwhen a runner is available.OrasIndexVerifyWorkflowgains aretry_policyfield and now retries transient errors on the final tag-resolution check, mirroring the existing Create/Copy pattern. Norunner/interrupt plumbing there — it runs on the main thread, not inside the executor, so there's no process-exit-blocking risk to fix.Explicitly out of scope (see design doc): unifying
OrasWaitForSourcesWorkflow's poll loop ontoRetryPolicy(different, already-correct mechanism), adding jitter, changingRetryPolicydefaults.Verification
just test— 1947 passed;ruff check+ruff format --checkclean.CommandRunner.sleep→ExecutorInterruptedpropagation through_run_job) is correct end to end and all stated non-goals were honored.OrasWaitForSourcesWorkflow's own poll sleep is the one Stage-1 wait left non-interruptible (~5s Ctrl-C latency bound vs. the ~62s this PR fixes for retry backoff) — same one-line fix pattern would close it if desired.🤖 Generated with Claude Code