Fixes #39211 - Defer capsule distribution refreshes after all syncs#11700
Fixes #39211 - Defer capsule distribution refreshes after all syncs#11700pablomh wants to merge 1 commit into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
RefreshDistribution#invoke_external_taskyou removed the localsmart_proxy/repolookups but still referencesmart_proxyand don't use the newrepohelper, which will raise at runtime; either restore thesmart_proxylookup and use therepohelper, or add asmart_proxyhelper method consistent withrepo. - The
distribution_uniqueness_conflict?check relies on substring matching'base_path'and'unique'in the error message, which is brittle; consider checking more structured attributes onPulp3Error(or at least tightening the pattern to the exact Pulp uniqueness payload you expect) to avoid misclassifying unrelated errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RefreshDistribution#invoke_external_task` you removed the local `smart_proxy`/`repo` lookups but still reference `smart_proxy` and don't use the new `repo` helper, which will raise at runtime; either restore the `smart_proxy` lookup and use the `repo` helper, or add a `smart_proxy` helper method consistent with `repo`.
- The `distribution_uniqueness_conflict?` check relies on substring matching `'base_path'` and `'unique'` in the error message, which is brittle; consider checking more structured attributes on `Pulp3Error` (or at least tightening the pattern to the exact Pulp uniqueness payload you expect) to avoid misclassifying unrelated errors.
## Individual Comments
### Comment 1
<location path="test/actions/katello/capsule_content_test.rb" line_range="67" />
<code_context>
- assert_equal capsule_content.smart_proxy.id, input[:smart_proxy_id]
- assert_equal repo.id, input[:repository_id]
- end
+ assert_tree_planned_steps(tree, Actions::Pulp3::CapsuleContent::RefreshAllDistributions)
end
</code_context>
<issue_to_address>
**suggestion (testing):** Sync planning tests only assert that `RefreshAllDistributions` exists, not that it is wired with the correct repositories and proxy.
Previously, this test verified that `RefreshDistribution` was planned with the expected `smart_proxy_id` and `repository_id` per repo. After switching to `RefreshAllDistributions`, it now only checks that the step exists. Since `RefreshAllDistributions` takes both a proxy and a list of repositories, the test should also assert that it receives the expected repos (and ideally the proxy) rather than, for example, an empty or incorrect list. For example, you could use `assert_tree_planned_with` (or a similar helper) to inspect the action inputs, or add a dedicated test in `SyncTest` that validates the inputs to `RefreshAllDistributions`, including the multi-repo case, to preserve the guarantees we had before.
Suggested implementation:
```ruby
assert_tree_planned_with(tree, Actions::Pulp3::CapsuleContent::RefreshAllDistributions) do |input|
# Ensure the correct capsule/smart proxy is used
assert_equal capsule_content.smart_proxy.id, input[:smart_proxy_id]
# Ensure the expected repositories are passed to the action
# If the test sets up multiple repos, replace `[repo]` with the correct
# collection (e.g. `repos`).
expected_repo_ids = [repo].flatten.map(&:id).sort
actual_repo_ids = Array(input[:repository_ids] || input[:repository_id]).flatten.map(&:id).sort rescue Array(input[:repository_ids] || input[:repository_id]).sort
assert_equal expected_repo_ids, actual_repo_ids
end
end
```
1. Adjust the `expected_repo_ids` construction to match the actual test setup:
- If the test uses a single repo variable (`repo`), leave `[repo].flatten.map(&:id)` as-is.
- If the test uses multiple repos (e.g. `repos`), change `expected_repo_ids = [repo].flatten.map(&:id).sort` to `expected_repo_ids = repos.map(&:id).sort`.
2. Confirm the input key used by `RefreshAllDistributions`:
- If the action passes repository IDs under a different key (e.g. `:repository_ids`, `:repository_uuids`, or `:repository_href_list`), replace `:repository_ids`/`:repository_id` with the correct key(s) and adjust how IDs are extracted.
3. If `assert_tree_planned_with` does not yield the raw input hash but a wrapper object, you may need to access the inputs via `input.input` or similar, depending on the existing helper implementation.
4. Optionally, to explicitly cover the multi-repo case, add a separate `it` block in this file that sets up multiple repos on the capsule, runs the planning step, and uses the same `assert_tree_planned_with` pattern to assert that the full set of repo IDs is passed to `RefreshAllDistributions`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert_equal capsule_content.smart_proxy.id, input[:smart_proxy_id] | ||
| assert_equal repo.id, input[:repository_id] | ||
| end | ||
| assert_tree_planned_steps(tree, Actions::Pulp3::CapsuleContent::RefreshAllDistributions) |
There was a problem hiding this comment.
suggestion (testing): Sync planning tests only assert that RefreshAllDistributions exists, not that it is wired with the correct repositories and proxy.
Previously, this test verified that RefreshDistribution was planned with the expected smart_proxy_id and repository_id per repo. After switching to RefreshAllDistributions, it now only checks that the step exists. Since RefreshAllDistributions takes both a proxy and a list of repositories, the test should also assert that it receives the expected repos (and ideally the proxy) rather than, for example, an empty or incorrect list. For example, you could use assert_tree_planned_with (or a similar helper) to inspect the action inputs, or add a dedicated test in SyncTest that validates the inputs to RefreshAllDistributions, including the multi-repo case, to preserve the guarantees we had before.
Suggested implementation:
assert_tree_planned_with(tree, Actions::Pulp3::CapsuleContent::RefreshAllDistributions) do |input|
# Ensure the correct capsule/smart proxy is used
assert_equal capsule_content.smart_proxy.id, input[:smart_proxy_id]
# Ensure the expected repositories are passed to the action
# If the test sets up multiple repos, replace `[repo]` with the correct
# collection (e.g. `repos`).
expected_repo_ids = [repo].flatten.map(&:id).sort
actual_repo_ids = Array(input[:repository_ids] || input[:repository_id]).flatten.map(&:id).sort rescue Array(input[:repository_ids] || input[:repository_id]).sort
assert_equal expected_repo_ids, actual_repo_ids
end
end- Adjust the
expected_repo_idsconstruction to match the actual test setup:- If the test uses a single repo variable (
repo), leave[repo].flatten.map(&:id)as-is. - If the test uses multiple repos (e.g.
repos), changeexpected_repo_ids = [repo].flatten.map(&:id).sorttoexpected_repo_ids = repos.map(&:id).sort.
- If the test uses a single repo variable (
- Confirm the input key used by
RefreshAllDistributions:- If the action passes repository IDs under a different key (e.g.
:repository_ids,:repository_uuids, or:repository_href_list), replace:repository_ids/:repository_idwith the correct key(s) and adjust how IDs are extracted.
- If the action passes repository IDs under a different key (e.g.
- If
assert_tree_planned_withdoes not yield the raw input hash but a wrapper object, you may need to access the inputs viainput.inputor similar, depending on the existing helper implementation. - Optionally, to explicitly cover the multi-repo case, add a separate
itblock in this file that sets up multiple repos on the capsule, runs the planning step, and uses the sameassert_tree_planned_withpattern to assert that the full set of repo IDs is passed toRefreshAllDistributions.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSyncCapsule now filters computed repos for Pulp 3 support and, if any, plans a new concurrent action to refresh distributions for those repos. Pulp3 metadata generation stops auto-planning distribution refreshes when repository publication is skipped. A new RefreshAllDistributions action was added, and RefreshDistribution gained retry-on-uniqueness-conflict logic and simplified inputs. Changes
Sequence Diagram(s)sequenceDiagram
participant SyncCapsule as SyncCapsule
participant RefreshAll as RefreshAllDistributions
participant RefreshDist as RefreshDistribution
participant SmartProxy as SmartProxy/Backend
SyncCapsule->>SyncCapsule: compute repos
SyncCapsule->>SyncCapsule: filter pulp3_repos (smart_proxy.pulp3_support?)
alt pulp3_repos not empty
SyncCapsule->>RefreshAll: plan(smart_proxy, pulp3_repos)
RefreshAll->>RefreshDist: plan each (repository, smart_proxy) concurrently
RefreshDist->>SmartProxy: invoke_external_task -> backend.refresh_distributions
SmartProxy-->>RefreshDist: task result or Pulp3Error
alt uniqueness/overlap error
RefreshDist->>RefreshDist: rescue_external_task -> retry invoke_external_task
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/actions/pulp3/capsule_content/refresh_distribution.rb`:
- Around line 35-39: Update the rescue predicate in refresh_distribution.rb to
also treat the "Overlaps with existing distribution" overlap error as the same
recoverable create-race as the unique-base_path case: modify the condition in
the rescue/if that currently only matches "This field must be unique" to accept
either "base_path":["This field must be unique."] or "base_path":["Overlaps with
existing distribution"] (keep behavior consistent with
Katello::Pulp3::Repository#refresh_distributions), and add a unit test
exercising the async failure path that returns the overlap payload so the
overlap variant is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a02f4517-e32e-448c-9641-1d54c9372247
📥 Commits
Reviewing files that changed from the base of the PR and between cc28088 and 959f3931df7409e7d07c0e69f6a4d057957772ee.
📒 Files selected for processing (6)
app/lib/actions/katello/capsule_content/sync_capsule.rbapp/lib/actions/pulp3/capsule_content/generate_metadata.rbapp/lib/actions/pulp3/capsule_content/refresh_all_distributions.rbapp/lib/actions/pulp3/capsule_content/refresh_distribution.rbtest/actions/katello/capsule_content_test.rbtest/actions/pulp3/capsule_content/refresh_all_distributions_test.rb
|
Thanks for the review! Addressing the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/actions/pulp3/capsule_content/refresh_distribution.rb (1)
35-40: Consider sharing this conflict matcher with the service layer.This string-based predicate now needs to stay aligned with the other
refresh_distributionsconflict handling path. Extracting one shared helper/constant would reduce the chance of the two matchers drifting again when Pulp changes its error text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/actions/pulp3/capsule_content/refresh_distribution.rb` around lines 35 - 40, Extract the string-matching logic out of distribution_uniqueness_conflict? into a shared helper (either a constant regex or a predicate method) that both this method and the service layer's refresh_distributions path can call; for example, create a Pulp3ErrorMatchers::DISTRIBUTION_UNIQUENESS_PATTERN or Pulp3ErrorMatchers.distribution_uniqueness_conflict?(error) and replace the inline message checks in distribution_uniqueness_conflict? and in the refresh_distributions conflict handling with calls to that shared symbol so the matching logic is maintained in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/actions/pulp3/capsule_content/refresh_distribution.rb`:
- Around line 35-40: Extract the string-matching logic out of
distribution_uniqueness_conflict? into a shared helper (either a constant regex
or a predicate method) that both this method and the service layer's
refresh_distributions path can call; for example, create a
Pulp3ErrorMatchers::DISTRIBUTION_UNIQUENESS_PATTERN or
Pulp3ErrorMatchers.distribution_uniqueness_conflict?(error) and replace the
inline message checks in distribution_uniqueness_conflict? and in the
refresh_distributions conflict handling with calls to that shared symbol so the
matching logic is maintained in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 558436cb-132e-4614-a317-a5d139f859c2
📥 Commits
Reviewing files that changed from the base of the PR and between 959f3931df7409e7d07c0e69f6a4d057957772ee and 70a5ffd46a704a7745fdf38a59d84e5acbba170b.
📒 Files selected for processing (3)
app/lib/actions/pulp3/capsule_content/refresh_distribution.rbtest/actions/katello/capsule_content_test.rbtest/actions/pulp3/capsule_content/refresh_all_distributions_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- test/actions/katello/capsule_content_test.rb
- test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
70a5ffd to
006dd1c
Compare
pablomh
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/actions/katello/capsule_content_test.rb (1)
67-71: Assert the new planning boundary, not just action presence.These checks still pass if
GenerateMetadatakeeps planningRefreshDistributioninline, because they only prove both actions exist somewhere in the tree. Please tighten this to show theRefreshDistributionsteps are children ofRefreshAllDistributions, or explicitly refute them underGenerateMetadata.Also applies to: 108-112, 126-130, 140-143, 171-175, 250-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/actions/katello/capsule_content_test.rb` around lines 67 - 71, The tests currently only assert that RefreshAllDistributions and RefreshDistribution exist somewhere in the plan; update them to assert the planning boundary: locate the Actions::Pulp3::CapsuleContent::RefreshAllDistributions node (using assert_tree_planned_with / assert_tree_planned_steps) and then assert that its child steps include Actions::Pulp3::CapsuleContent::RefreshDistribution with the expected inputs (smart_proxy_id and repository_id); alternatively, add an explicit negative assertion on Actions::Pulp3::CapsuleContent::GenerateMetadata to ensure it does NOT plan RefreshDistribution inline. Use the existing helpers (assert_tree_planned_with, assert_tree_planned_steps) around the RefreshAllDistributions and GenerateMetadata nodes to enforce parent/child relationships rather than just presence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/actions/pulp3/capsule_content/refresh_distribution.rb`:
- Around line 18-23: The rescue_external_task path currently re-invokes
invoke_external_task unbounded when distribution_uniqueness_conflict?(error)
hits; persist a one-time retry flag on the instance (e.g. `@refreshed_once` or set
a field like retried_refresh_distribution) before assigning self.external_task =
invoke_external_task and on subsequent calls check that flag and call
super(error) instead; update rescue_external_task to set the flag when
scheduling the replacement refresh and to fall back to super on the second
conflict to avoid infinite replay.
---
Nitpick comments:
In `@test/actions/katello/capsule_content_test.rb`:
- Around line 67-71: The tests currently only assert that
RefreshAllDistributions and RefreshDistribution exist somewhere in the plan;
update them to assert the planning boundary: locate the
Actions::Pulp3::CapsuleContent::RefreshAllDistributions node (using
assert_tree_planned_with / assert_tree_planned_steps) and then assert that its
child steps include Actions::Pulp3::CapsuleContent::RefreshDistribution with the
expected inputs (smart_proxy_id and repository_id); alternatively, add an
explicit negative assertion on Actions::Pulp3::CapsuleContent::GenerateMetadata
to ensure it does NOT plan RefreshDistribution inline. Use the existing helpers
(assert_tree_planned_with, assert_tree_planned_steps) around the
RefreshAllDistributions and GenerateMetadata nodes to enforce parent/child
relationships rather than just presence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d48d6c9d-b496-4bbe-aacd-3e58f9e06c52
📥 Commits
Reviewing files that changed from the base of the PR and between 70a5ffd46a704a7745fdf38a59d84e5acbba170b and 006dd1c91b8a141676fc20809658657a94a7a124.
📒 Files selected for processing (6)
app/lib/actions/katello/capsule_content/sync_capsule.rbapp/lib/actions/pulp3/capsule_content/generate_metadata.rbapp/lib/actions/pulp3/capsule_content/refresh_all_distributions.rbapp/lib/actions/pulp3/capsule_content/refresh_distribution.rbtest/actions/katello/capsule_content_test.rbtest/actions/pulp3/capsule_content/refresh_all_distributions_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- app/lib/actions/pulp3/capsule_content/refresh_all_distributions.rb
- app/lib/actions/katello/capsule_content/sync_capsule.rb
- app/lib/actions/pulp3/capsule_content/generate_metadata.rb
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
006dd1c to
3b31409
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb (1)
20-31: Assert the exact refresh fan-out here.This only proves each expected repo appears at least once. It would still pass if
RefreshAllDistributionsduplicated a repo or scheduled extras, so please add an exact count/set assertion for the plannedRefreshDistributionchildren.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb` around lines 20 - 31, The test currently only verifies each expected repository appears at least once; update it to assert exact fan-out by checking the planned RefreshDistribution children count equals repos.size and that the set of (repository_id, smart_proxy_id) pairs exactly matches repos.map { |r| [r.id, proxy.id] }. Use the existing plan_action_tree(::Actions::Pulp3::CapsuleContent::RefreshAllDistributions, proxy, repos) result and inspect its children for instances of ::Actions::Pulp3::CapsuleContent::RefreshDistribution, replacing the loose assert_tree_planned_with loop with assertions that (1) children.select { |c| c.action == ::Actions::Pulp3::CapsuleContent::RefreshDistribution }.size == repos.size and (2) the set of child inputs.map { |i| [i[:repository_id], i[:smart_proxy_id]] } equals the expected set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb`:
- Around line 20-31: The test currently only verifies each expected repository
appears at least once; update it to assert exact fan-out by checking the planned
RefreshDistribution children count equals repos.size and that the set of
(repository_id, smart_proxy_id) pairs exactly matches repos.map { |r| [r.id,
proxy.id] }. Use the existing
plan_action_tree(::Actions::Pulp3::CapsuleContent::RefreshAllDistributions,
proxy, repos) result and inspect its children for instances of
::Actions::Pulp3::CapsuleContent::RefreshDistribution, replacing the loose
assert_tree_planned_with loop with assertions that (1) children.select { |c|
c.action == ::Actions::Pulp3::CapsuleContent::RefreshDistribution }.size ==
repos.size and (2) the set of child inputs.map { |i| [i[:repository_id],
i[:smart_proxy_id]] } equals the expected set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8cb0d181-65fa-47c5-9fdd-c83f4351ca0b
📥 Commits
Reviewing files that changed from the base of the PR and between 006dd1c91b8a141676fc20809658657a94a7a124 and 3b31409c0ef3e61a70c58034189e88b385dfd591.
📒 Files selected for processing (6)
app/lib/actions/katello/capsule_content/sync_capsule.rbapp/lib/actions/pulp3/capsule_content/generate_metadata.rbapp/lib/actions/pulp3/capsule_content/refresh_all_distributions.rbapp/lib/actions/pulp3/capsule_content/refresh_distribution.rbtest/actions/katello/capsule_content_test.rbtest/actions/pulp3/capsule_content/refresh_all_distributions_test.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- app/lib/actions/pulp3/capsule_content/generate_metadata.rb
- app/lib/actions/pulp3/capsule_content/refresh_all_distributions.rb
- test/actions/katello/capsule_content_test.rb
- app/lib/actions/katello/capsule_content/sync_capsule.rb
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
3b31409 to
558587b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb (2)
111-117: Assert error propagation for non-Pulp3Errorexceptions.Line 116 should also assert that the
RuntimeErroris re-raised (or failed) by parent handling; otherwise this test won’t catch future regressions that accidentally swallow non-Pulp errors.Proposed test hardening
it 'does not attempt recovery for non-Pulp3Errors' do action = create_action(::Actions::Pulp3::CapsuleContent::RefreshDistribution) action.stubs(:input).returns('repository_id' => repo.id, 'smart_proxy_id' => proxy.id) action.expects(:invoke_external_task).never - action.rescue_external_task(RuntimeError.new("connection refused")) + assert_raises(RuntimeError) do + action.rescue_external_task(RuntimeError.new("connection refused")) + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb` around lines 111 - 117, The test currently stubs action.rescue_external_task(RuntimeError.new(...)) but does not assert the RuntimeError is re-raised; update the test for Actions::Pulp3::CapsuleContent::RefreshDistribution (the action variable) to wrap the rescue_external_task call in an assertion that the RuntimeError is raised (e.g., use assert_raises or equivalent RSpec expectation) so the test verifies non-Pulp3Error exceptions are propagated rather than swallowed.
57-63: StrengthenGenerateMetadatabranch coverage.Line 58 currently validates only “no inline
RefreshDistribution”. Please add an explicit case for a repository type wherepulp3_skip_publicationisfalseto ensure the active planning branch is still exercised while keeping this assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb` around lines 57 - 63, Add a new test case that uses a repository fixture where pulp3_skip_publication is false (or mutate the repo fixture in the test) and call plan_action_tree(::Actions::Pulp3::CapsuleContent::GenerateMetadata, repo, proxy); keep the existing assertion refute_tree_planned(tree, ::Actions::Pulp3::CapsuleContent::RefreshDistribution) and also assert that the active planning branch is exercised by asserting the plan includes ::Actions::Pulp3::CapsuleContent::RefreshAllDistributions (or that GenerateMetadata triggers planning of RefreshAllDistributions) so the GenerateMetadata branch for non-skipped publication is covered while still ensuring no inline RefreshDistribution is planned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/actions/pulp3/capsule_content/refresh_all_distributions_test.rb`:
- Around line 111-117: The test currently stubs
action.rescue_external_task(RuntimeError.new(...)) but does not assert the
RuntimeError is re-raised; update the test for
Actions::Pulp3::CapsuleContent::RefreshDistribution (the action variable) to
wrap the rescue_external_task call in an assertion that the RuntimeError is
raised (e.g., use assert_raises or equivalent RSpec expectation) so the test
verifies non-Pulp3Error exceptions are propagated rather than swallowed.
- Around line 57-63: Add a new test case that uses a repository fixture where
pulp3_skip_publication is false (or mutate the repo fixture in the test) and
call plan_action_tree(::Actions::Pulp3::CapsuleContent::GenerateMetadata, repo,
proxy); keep the existing assertion refute_tree_planned(tree,
::Actions::Pulp3::CapsuleContent::RefreshDistribution) and also assert that the
active planning branch is exercised by asserting the plan includes
::Actions::Pulp3::CapsuleContent::RefreshAllDistributions (or that
GenerateMetadata triggers planning of RefreshAllDistributions) so the
GenerateMetadata branch for non-skipped publication is covered while still
ensuring no inline RefreshDistribution is planned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d69122aa-7e4c-4bd1-852c-49763d12f043
📥 Commits
Reviewing files that changed from the base of the PR and between 3b31409c0ef3e61a70c58034189e88b385dfd591 and 558587b54e258ae20fb52de855e196edda4525b3.
📒 Files selected for processing (6)
app/lib/actions/katello/capsule_content/sync_capsule.rbapp/lib/actions/pulp3/capsule_content/generate_metadata.rbapp/lib/actions/pulp3/capsule_content/refresh_all_distributions.rbapp/lib/actions/pulp3/capsule_content/refresh_distribution.rbtest/actions/katello/capsule_content_test.rbtest/actions/pulp3/capsule_content/refresh_all_distributions_test.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- app/lib/actions/katello/capsule_content/sync_capsule.rb
- app/lib/actions/pulp3/capsule_content/refresh_all_distributions.rb
- app/lib/actions/pulp3/capsule_content/refresh_distribution.rb
- test/actions/katello/capsule_content_test.rb
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
558587b to
e02806d
Compare
|
Regarding nitpick 1 ( For nitpick 2: added a second |
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
e02806d to
d257ab3
Compare
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
abcc5d7 to
5bfe7b8
Compare
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
2 times, most recently
from
58ca8ec to
3451575
Compare
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
ae574af to
3919d3c
Compare
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
3 times, most recently
from
1fc68e9 to
7ed0b03
Compare
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
7ed0b03 to
abd97a0
Compare
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
abd97a0 to
a1caee3
Compare
pablomh
changed the title
| CREATE_RACE_PATTERN = / | ||
| ["']base_path["'].*? | ||
| ( | ||
| must\ be\ unique | code=['"]unique | | ||
| Overlaps\ with\ existing\ distribution | ||
| ) | ||
| /mx |
There was a problem hiding this comment.
Do you mind adding a comment on what this expression is doing?
There was a problem hiding this comment.
Added a comment — does this wording work?
# Matches Pulp3 errors indicating a distribution base_path conflict
# from a concurrent create: either a uniqueness violation or an overlap.
| module Pulp3 | ||
| module CapsuleContent | ||
| class RefreshDistribution < Pulp3::AbstractAsyncTask | ||
| include Helpers::Presenter |
There was a problem hiding this comment.
Why was this Presenter removed?
There was a problem hiding this comment.
It was unused — RefreshDistribution never defined a presenter method, so the include was a no-op. The sibling Repository::RefreshDistribution still includes it because it uses a custom presenter.
| def sync_url_params(_sync_options) | ||
| params = {remote: repo.remote_href, mirror: repo.root.mirroring_policy == Katello::RootRepository::MIRRORING_POLICY_CONTENT} | ||
| params[:skip_types] = skip_types if (skip_types && repo.root.mirroring_policy != Katello::RootRepository::MIRRORING_POLICY_COMPLETE) | ||
| params[:skip_types] = skip_types if skip_types && repo.root.mirroring_policy != Katello::RootRepository::MIRRORING_POLICY_COMPLETE |
There was a problem hiding this comment.
Is changing this intentional? I get its a style change but doesn't seem related to this issue so just want to make sure you meant to change this.
There was a problem hiding this comment.
Drive-by RuboCop style fix (redundant parentheses). Happy to revert if you'd prefer to keep it out of this PR.
There was a problem hiding this comment.
I don't mind if it stays in this PR, just wanted to check.
Refresh capsule distributions after sync batches complete and recover once from uniqueness races, while keeping the test coverage focused on the deferred refresh flow. Co-authored-by: Cursor <cursoragent@cursor.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pablomh
force-pushed
the
fixes/capsule-refresh-distribution-race
branch
from
a1caee3 to
08910c6
Compare
aidenfine
left a comment
aidenfine
left a comment
There was a problem hiding this comment.
Just tested this manually and things seem to work fine. @ianballou will also be taking a look. LGTM
2 participants
Problem
When
SyncCapsuleruns multiple repos concurrently, each repo'sRefreshDistributionwas planned inline insideGenerateMetadata. Two repos sharing the same Pulp3 distribution (samebase_path/name) could race to create it. The loser fails with a uniquenessPulp3Errorthat surfaces only during async task polling - too late for a service-level rescue to catch.Observed under concurrent capsule sync stress tests across multiple content types (RPM, container). Tracked in SAT-31994.
Solution
Two changes working together:
1. Deferred distribution refresh — post-sync planning step
Move distribution refresh out of per-repo
GenerateMetadatainto a post-sync planning step inSyncCapsule. After all sync batches complete,RefreshDistributionis planned for each repo, batched and run concurrently (concurrenceblock).This also gives consumers a more consistent view of the capsule: content becomes accessible for all repos together rather than repo-by-repo as each finishes syncing. This mirrors the approach Pulp upstream adopted for its own replication task in pulpcore 3.107.0 (issue #7333).
2.
rescue_external_taskinRefreshDistributionIf two
RefreshDistributionactions still race (e.g. two repos sharing the same distribution path), the loser recovers: re-invokesrefresh_distributions, which finds the existing distribution and issues apartial_updateinstead of a create. Dynflow re-polls the new task automatically viasuspend_and_pingafter the rescue returns without raising (confirmed against Dynflow 2.0.0 source).Test plan
RefreshDistributionat the sync levelGenerateMetadataTest: verifiesRefreshDistributionis no longer planned inlineRefreshDistributionTest:rescue_external_taskrecovers on uniqueness conflict, re-raises unrelated errorsGenerated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests