Skip to content

Conversation

mdzhigarov
Copy link
Contributor

Problem

Fix race condition in PackageInstall reconciler where status was set based on App state before updating the App spec, leading to temporary inconsistencies.

Solution

  • Refactored reconcileAppWithPackage to accept a status updater closure
  • Status is now set based on the App state AFTER any spec updates
  • Eliminates race condition where PackageInstall shows stale status

Changes

  • Modified reconcile() method to create status updater closure
  • Updated reconcileAppWithPackage() to accept and call the closure with correct App instance
  • Added comprehensive tests demonstrating the fix

Testing

  • Added Test_StatusUpdaterClosureUsesUpdatedAppState - tests the race condition scenario
  • Added Test_StatusUpdaterClosureWithNoAppUpdate - tests non-update scenario
  • All existing tests continue to pass

Fixes: (https://vmw-jira.broadcom.net/browse/TNZ-54843)

Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR Marin!
(Took a quick peek) This is the fix that I had in my mind as well, I will go through the tests in detail once the checks are green.
I think @cppforlife had some more thoughts regarding this so let's wait for him to add his comments.

@cppforlife
Copy link
Contributor

cppforlife commented Aug 19, 2025

lgtm from me. @praveenrewar care to take a look?

Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdzhigarov Changes looks good to me apart from the small (optional) nit.
My only concern is that we don't have a proper test for this. It might be tricky to add a test for this but even if we could validate that the change is fixing the issue would be a good to have.

Signed-off-by: Marin Dzhigarov <[email protected]>
@mdzhigarov mdzhigarov force-pushed the fix-packageinstall-status-race-condition branch from ba8ab60 to 4465ced Compare August 20, 2025 06:22
@praveenrewar praveenrewar merged commit 367ae9a into carvel-dev:develop Aug 20, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this to Closed in Carvel Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants