fix(test): clean up downgraded package in downgrade-after-uninstall by ayuskauskas · Pull Request #241 · NVIDIA/nodewright

ayuskauskas · 2026-05-19T20:26:46Z

Summary

chainsaw/downgrade-after-uninstall was failing in CI with context deadline exceeded during chainsaw's automatic CLEANUP (the only failing test in the suite).
Root cause: after feat: add explicit uninstall to support uninstalls that require the config information #200 introduced the delete-time uninstall finalizer, the CR-delete path now waits for uninstall pods to complete on every selected node before releasing. This test left [email protected] freshly installed (uninstall.enabled: true, apply: false) at cleanup time, so the finalizer had real work to do — more than chainsaw's default cleanup window allows.
Fix: add a final uninstall-v1 step that flips uninstall.apply: true on the downgraded v1.2.3 spec and asserts nodeState_…: '{}' before chainsaw's auto-CLEANUP runs. With node state empty, the finalizer has nothing to do at delete time and the CR drops promptly.

This mirrors the approach the other uninstall-pool tests already take (they either own the delete inside the test body or have node state empty by the end of the last asserted step).

Test plan

chainsaw/downgrade-after-uninstall passes in CI (uninstall pool)
Other tests in the uninstall pool still pass
No new operator code changes — this is a test-only fix

…fter-uninstall The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]>

coderabbitai · 2026-05-19T20:28:12Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 04ee6bbf-f06f-403b-ab79-6de21a69edbe

📥 Commits

Reviewing files that changed from the base of the PR and between c004c13 and ee5abb1.

📒 Files selected for processing (3)

k8s-tests/chainsaw/skyhook/downgrade-after-uninstall/README.md
k8s-tests/chainsaw/skyhook/downgrade-after-uninstall/chainsaw-test.yaml
k8s-tests/chainsaw/skyhook/downgrade-after-uninstall/update-trigger-uninstall-v1.yaml

📝 Walkthrough

Walkthrough

This PR extends an existing Kubernetes Chainsaw test scenario for downgrading and then uninstalling a package. It adds a new test step that uninstalls the v1.2.3 package after downgrading, introduces a new Skyhook manifest (update-trigger-uninstall-v1.yaml) that configures the v1 uninstall trigger, and updates the test documentation to describe the complete scenario sequence and reference the new fixture file.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding a cleanup step to uninstall the downgraded package in the downgrade-after-uninstall test.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, explaining the root cause, implementation, and test plan.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/downgrade-after-uninstall-cleanup

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coveralls · 2026-05-19T21:08:31Z

Coverage Report for CI Build 26123248910

Warning

No base build found for commit c004c13 on main.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 81.905%

Details

Patch coverage: No coverable lines changed in this PR.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →

Coverage Stats


Relevant Lines:	9439
Covered Lines:	7731
Line Coverage:	81.9%
Coverage Strength:	9.61 hits per line

💛 - Coveralls

* fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]>

* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>

* fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: ayuskauskas <[email protected]>

* ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>

* feat(agent): port enums and steps for agent go rewrite Signed-off-by: Riley Rice <[email protected]> * chore: Cherry pick (#244) * ci: publish keyless release attestations (#232) (#240) * ci: publish keyless release attestations * ci: drop nvcr release attestations * ci: narrow release attestation identity * ci: gate attestations to release tags * docs: show immutable release subjects * ci: attest helm chart releases --------- Signed-off-by: AnouarMohamed <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> * chore(docs): update docs around release and location of helm chart (#237) (#239) * fix(test): uninstall downgraded package before cleanup in downgrade-after-uninstall (#241) The test left [email protected] freshly installed at chainsaw's automatic CLEANUP, where the new delete-time uninstall finalizer (from #200) must run uninstall pods on every node before releasing the CR. That exceeds chainsaw's default cleanup window, causing "context deadline exceeded". Add a final uninstall-v1 step that flips uninstall.apply=true on the downgraded version and waits for nodeState to empty, so the CR has no installed packages when cleanup deletes it. Signed-off-by: Alex Yuskauskas <[email protected]> * fix: deadlock in webhook controller when upgrading from old versions (#243) * fix: close StageInterrupt trap + harden core e2e pool (#242) * fix(test): harden core e2e pool against finalizer-cleanup races and improve diagnosability Two changes across the six core-pool chainsaw tests: 1. Add `timeouts.cleanup: 120s` to every core-pool test. Chainsaw's default cleanup window (~30s) is too tight for the project's CR finalizer, which must uncordon nodes, remove labels/annotations, and GC package pods before releasing. Same failure mode that hit downgrade-after-uninstall (PR #241). 2. Add `catch:` blocks to `depends-on` and `simple-skyhook`, mirroring the pattern used by the other four core tests. When these tests flake, CI now captures the Node, Skyhook CR, and package Pods for diagnosis instead of failing without artifacts. No assert logic changes — this is a foundation pass to remove a known flake class and make future flakes diagnosable. Signed-off-by: Alex Yuskauskas <[email protected]> * fix(operator): close StageInterrupt trap when configUpdates signal decays A package whose only interrupt is a `configInterrupts` entry (no top-level `interrupt` block) could get stuck at `StageInterrupt/StateSkipped` permanently when `Status.ConfigUpdates` cleared or never persisted (e.g. due to a 409 on the spec patch). The state machine queried the dynamic `HasInterrupt(config)` signal at four points, and once that signal decayed to false, the package was untouchable: `ProgressSkipped` wouldn't promote it, `NextStage` wouldn't advance it past Interrupt, and `GetComplete`/`IsPackageComplete` wouldn't count it complete. Decouple progression-past-Interrupt from the dynamic signal: - `NextStage`: add `StageInterrupt → StagePostInterrupt` to the no-interrupt default map. The with-interrupt full-replacement map is preserved as-is — PR #200 deliberately omits `StageUninstall → StageApply` from it so with-interrupt uninstalls route via `StageUninstallInterrupt`; collapsing the maps would silently re-enable that transition. - `ProgressSkipped`: drop the `HasInterrupt` gate. The only writer of `StateSkipped` at `StageInterrupt` is `ProcessInterrupt`'s budget-contention branch, which already decided the package needed an interrupt to schedule. Stage alone is sufficient. - `GetComplete`, `IsPackageComplete`: treat `StagePostInterrupt` as unconditionally terminal. The only way to reach it is via the interrupt cycle; gating the terminal check on a signal that can decay is redundant with the entry gate and only becomes load-bearing when the trap fires. Reproducer in `skyhook_types_test.go` exercises all three sites. Surfaced by the `chainsaw/config-skyhook` "update while running" step, which patches the spec mid-flight and concurrently triggers the `HandleMigrations` and `processSkyhooksPerNode` 409 conflict paths — those stretch convergence enough that ConfigUpdates can be lost between the package reaching StageInterrupt and ProgressSkipped firing. Signed-off-by: Alex Yuskauskas <[email protected]> * refactor(operator): GetComplete delegates to IsPackageComplete GetComplete and IsPackageComplete encoded the same terminal-state logic twice. Have GetComplete iterate the spec packages and call IsPackageComplete for each, and move the explanatory comment to IsPackageComplete where the predicate lives. Behavior preserved: the prior implementation iterated node state and filtered to spec packages by name+version match; the new one iterates spec packages and looks them up in node state by unique name (name|version). Same set of packages either way. Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: Alex Yuskauskas <[email protected]> --------- Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]> --------- Signed-off-by: Riley Rice <[email protected]> Signed-off-by: AnouarMohamed <[email protected]> Signed-off-by: Alex Yuskauskas <[email protected]> Co-authored-by: Brian Lockwood <[email protected]> Co-authored-by: Anouar Mohamed <[email protected]> Co-authored-by: ayuskauskas <[email protected]>

ayuskauskas requested review from lockwobr, mskalka and rice-riley as code owners May 19, 2026 20:26

lockwobr approved these changes May 19, 2026

View reviewed changes

lockwobr assigned ayuskauskas May 19, 2026

ayuskauskas mentioned this pull request May 19, 2026

fix: close StageInterrupt trap + harden core e2e pool #242

Merged

5 tasks

lockwobr merged commit 2dfec98 into main May 22, 2026
59 of 61 checks passed

lockwobr deleted the fix/downgrade-after-uninstall-cleanup branch May 22, 2026 18:09

coderabbitai Bot mentioned this pull request May 22, 2026

chore: Cherry pick #244

Merged

coderabbitai Bot mentioned this pull request May 22, 2026

chore: Cherry pick again #254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(test): clean up downgraded package in downgrade-after-uninstall#241

fix(test): clean up downgraded package in downgrade-after-uninstall#241
lockwobr merged 1 commit into
mainfrom
fix/downgrade-after-uninstall-cleanup

ayuskauskas commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Walkthrough

Estimated code review effort

Uh oh!

coveralls commented May 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ayuskauskas commented May 19, 2026

Summary

Test plan

Uh oh!

coderabbitai Bot commented May 19, 2026

Walkthrough

Estimated code review effort

Uh oh!

coveralls commented May 19, 2026

Coverage Report for CI Build 26123248910

Coverage: 81.905%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants