Cancel tagging when repo times out - #59
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis change replaces ChangesSide-tag lifecycle and task cancellation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RebuildBatch
participant SideTag
participant Koji
participant cancel_tasks
RebuildBatch->>SideTag: create side tag
SideTag->>Koji: create tag and tag initial builds
Koji-->>SideTag: tagging task IDs
SideTag->>SideTag: wait for tag completion
SideTag->>cancel_tasks: cancel tasks on failure
cancel_tasks->>Koji: cancelTask via multicall
Koji-->>cancel_tasks: results keyed by task ID
SideTag-->>RebuildBatch: SideTag or typed error
RebuildBatch->>SideTag: remove on cleanup
SideTag->>Koji: remove side tag
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
elnbuildsync/kojihelpers/tags.py (1)
74-79: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSubmit the cancellation batch only once.
When
resultscontains multiple failures, this branch passes the complete task index tocancel_tasksfor each failure. The same task IDs are then submitted to Koji repeatedly.Guard the call with a flag or move it after the loop. Use one cancellation attempt per failed side-tag preparation unless repeated cancellation is an intentional retry policy.
Suggested fix
+ cancellation_requested = False for success, value in results: if success: logger.info(f"Build {value} tagged into {side_tag_name}") else: ... - await kojihelpers.builds.cancel_tasks(tagging_tasks_index.keys()) + if not cancellation_requested: + cancellation_requested = True + await kojihelpers.builds.cancel_tasks(tagging_tasks_index.keys())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elnbuildsync/kojihelpers/tags.py` around lines 74 - 79, The cancel_tasks call with tagging_tasks_index.keys() is inside a loop that processes each failure from results, causing the same task IDs to be submitted for cancellation multiple times. Move the await kojihelpers.builds.cancel_tasks(tagging_tasks_index.keys()) call outside and after the loop so the cancellation batch is submitted only once, regardless of how many failures exist in the results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@elnbuildsync/kojihelpers/builds.py`:
- Around line 171-181: The helper _cancel_multiple_tasks_thread must be
synchronous because it is invoked through deferToThread and must execute its
multicall body immediately. Change its declaration from async def to def while
preserving the existing task cancellation and result aggregation logic; do not
add coroutine awaiting to _call_koji_once.
- Around line 154-168: The cancel_tasks function assigns results only inside the
try block when call_koji succeeds, but the except handler logs the exception and
then tries to return the unassigned results variable, causing UnboundLocalError.
Initialize results to an empty dictionary before the try block or add a return
statement in the except handler to return an empty dictionary. This preserves
the best-effort behavior where cancellation failures are logged but the function
still returns a valid result instead of propagating UnboundLocalError.
---
Nitpick comments:
In `@elnbuildsync/kojihelpers/tags.py`:
- Around line 74-79: The cancel_tasks call with tagging_tasks_index.keys() is
inside a loop that processes each failure from results, causing the same task
IDs to be submitted for cancellation multiple times. Move the await
kojihelpers.builds.cancel_tasks(tagging_tasks_index.keys()) call outside and
after the loop so the cancellation batch is submitted only once, regardless of
how many failures exist in the results.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 104d3dc9-c0a8-4e23-b16f-db24923cace1
📒 Files selected for processing (2)
elnbuildsync/kojihelpers/builds.pyelnbuildsync/kojihelpers/tags.py
|
This still needs some work; the "nitpick" comment (copied below so that it isn't lost) that CodeRabbit spotted is actually quite important and I overlooked it. Actionable comments posted: 2 🧹 Nitpick comments (1)
🤖 Prompt for all review comments with AI agents🪄 Autofix (Beta)Fix all unresolved CodeRabbit comments on this PR:
ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
elnbuildsync/rebuildbatch.py (1)
92-136: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winFix
_create_and_populate_side_tagto useSideTag.create()consistently.
kojihelpers.tags.prepare_side_tagno longer exists, so this method will raiseAttributeErrorin both import paths. The method currently returns aSideTagas a string via the staleprepare_side_tagname, butasync_init()later usesself.side_tag.name/self.side_tag.remove()as an object, while the Bodhi update path expectsupdate_tagas a plain string. Either returnSideTagconsistently and passupdate_tag.nameinto_submit_bodhi_update, or update the stale return annotation/string/object split before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elnbuildsync/rebuildbatch.py` around lines 92 - 136, Update _create_and_populate_side_tag to create the tag through the available SideTag.create() API instead of the removed prepare_side_tag symbol, preserving timeout retry handling. Return a SideTag object consistently so async_init() can use its name and remove() methods, and pass side_tag.name to _submit_bodhi_update where a plain tag string is required; update the return annotation and related documentation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@elnbuildsync/kojihelpers/tags.py`:
- Around line 86-93: Update the _prepare() docstring to remove the stale
:params: entries for base_tag and initial_build_ids, and replace them with a
concise description of _prepare()’s actual behavior without documenting
nonexistent arguments.
- Around line 60-69: Update the comment describing tagging_tasks_index in the
relevant tag class to document that it maps task IDs to build IDs, matching the
value returned by tag_builds() and the usage in cancel_tasks(). Leave the
dictionary behavior unchanged.
- Around line 86-140: Update _prepare() to call self.remove() during
tagging-failure cleanup after cancelling the tagging tasks, ensuring the
partially created side tag is removed before raising. Update
wait_for_nvrs_in_tag() to preserve or return the DeferredTimeoutError from
failed deferreds despite DeferredList(consumeErrors=True), so _prepare() can
reach its DeferredTimeoutError handling branch and raise SideTagTimeoutError for
timeouts.
---
Outside diff comments:
In `@elnbuildsync/rebuildbatch.py`:
- Around line 92-136: Update _create_and_populate_side_tag to create the tag
through the available SideTag.create() API instead of the removed
prepare_side_tag symbol, preserving timeout retry handling. Return a SideTag
object consistently so async_init() can use its name and remove() methods, and
pass side_tag.name to _submit_bodhi_update where a plain tag string is required;
update the return annotation and related documentation accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb32b1a4-8005-47dd-98b7-b208686243d4
📒 Files selected for processing (3)
elnbuildsync/kojihelpers/builds.pyelnbuildsync/kojihelpers/tags.pyelnbuildsync/rebuildbatch.py
🚧 Files skipped from review as they are similar to previous changes (1)
- elnbuildsync/kojihelpers/builds.py
69d3772 to
8ce52af
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
elnbuildsync/rebuildbatch.py (1)
94-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the type contract with both call paths.
Line 94 accepts
list[int], but_create_and_submit_bodhi_updatespasseslist[str]whenpromote_builds=True. The non-promotion branch returns the originallist[int], although the return type declareslist[str]. This can cause static type-checking failures and mislead callers.Use
list[int | str]for the input and returned references, or add overloads for the two modes.Proposed type fix
async def _create_and_populate_side_tag( - self, build_ids: list[int], promote_builds: bool = False -) -> tuple[kojihelpers.tags.SideTag, list[str]]: + self, build_ids: list[int | str], promote_builds: bool = False +) -> tuple[kojihelpers.tags.SideTag, list[int | str]]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@elnbuildsync/rebuildbatch.py` around lines 94 - 108, Update the side-tag creation method’s type contract and related annotations to accept and return references as list[int | str], covering both build IDs and NVR strings across the promote_builds paths. Ensure callers such as _create_and_submit_bodhi_updates and the non-promotion branch remain type-consistent without changing runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@elnbuildsync/rebuildbatch.py`:
- Around line 94-108: Update the side-tag creation method’s type contract and
related annotations to accept and return references as list[int | str], covering
both build IDs and NVR strings across the promote_builds paths. Ensure callers
such as _create_and_submit_bodhi_updates and the non-promotion branch remain
type-consistent without changing runtime behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eea99e95-08f3-4a52-b1b5-992c19ceb49c
📒 Files selected for processing (3)
elnbuildsync/kojihelpers/builds.pyelnbuildsync/kojihelpers/tags.pyelnbuildsync/rebuildbatch.py
🚧 Files skipped from review as they are similar to previous changes (2)
- elnbuildsync/kojihelpers/builds.py
- elnbuildsync/kojihelpers/tags.py
When Koji is very busy, sometimes the initial side-tag creation will fail because a repo cannot be regenerated within the timeout. In this situation, we were deleting the side-tag and retrying, but we didn't actually cancel all of the pending tag tasks for that side-tag buildroot. In situations where the Koji queue is extremely backlogged, this would only serve to make that backlog worse. For maintainability, this converts side-tag creation into a new Python class. Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
When Koji is very busy, sometimes the initial side-tag creation will fail because a repo cannot be regenerated within the timeout. In this situation, we were deleting the side-tag and retrying, but we didn't actually cancel all of the pending tag tasks for that side-tag buildroot. In situations where the Koji queue is extremely backlogged, this would only serve to make that backlog worse.
Summary by CodeRabbit
New Features
Bug Fixes