Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(a3p-integration): replaceFeeDistributor flakiness fix #11006

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

anilhelvaci
Copy link
Collaborator

closes: #10998

Description

In #10998, we see a flake introduced after a3p-integration/p:upgrade-19 tests migrated to a3p-integration/n:upgrade-next. The investigations showed that feeDistributor was collecting more fees than our test is producing.

Passing

2025-02-11T22:51:00.145Z SwingSet: vat: v36: ----- ReserveKit.5  9 received collateral { brand: Object [Alleged: IST brand] {}, value: 500_000n }

Failing

2025-02-11T22:04:40.861Z SwingSet: vat: v36: ----- ReserveKit.5  8 received collateral { brand: Object [Alleged: IST brand] {}, value: 550_000n }

My argument is, priorly we were introducing the new feeDistributor dedicated to testing during our test initiation phase. And after introduction of the feeDistributor, we were waiting for one full collectionInterval to clear any leftover fees that haven't been collected by the old feeDistributor. And only then we start producing new fees. I confirmed that this is not the current behavior after the n:upgrade-next migration as of the commit 271c55f#diff-7dd0ed44de879823d10b252f6483f52174e4fb9240966e7fcd1754298b41b0d7L161

  await evalBundles(coreEvalDir);
  // Wait for a round to give the new feeDistributor time to clear out outstanding fees, if any.
  await sleep(collectionInterval + 5000, { log: console.log, setTimeout });

As of the migration to upgrade-19, we introduce the new feeDistributor in uprage.go which causes fees produced by other tests to get in the way of replaceFeeDistributor.test.js in a flakey way since the collectionInterval is 30 seconds.

Solution

Just switch from AmountMath.isEqual to AmountMath.isGTE when polling for results.

Testing Considerations

Make sure integration tests pass in a consistent way.

Upgrade Considerations

Get rid of the flake so upgrade-19 shipment isn't blocked.

@anilhelvaci anilhelvaci added the force:integration Force integration tests to run on PR label Feb 14, 2025
@anilhelvaci anilhelvaci self-assigned this Feb 14, 2025
@anilhelvaci anilhelvaci requested a review from turadg February 14, 2025 19:26
@anilhelvaci
Copy link
Collaborator Author

CI fails in multichain tests:

I haven't been able to find anything related to feeDistributor in the logs. You mentioned multichain test failures yesterday as well. Are these the same as the ones you saw? @turadg

@anilhelvaci anilhelvaci marked this pull request as ready for review February 14, 2025 19:26
@anilhelvaci anilhelvaci requested a review from a team as a code owner February 14, 2025 19:26
@turadg
Copy link
Member

turadg commented Feb 14, 2025

CI fails in multichain tests:

Independent flake,

@anilhelvaci anilhelvaci added the automerge:rebase Automatically rebase updates, then merge label Feb 19, 2025
@anilhelvaci anilhelvaci force-pushed the anil/10988-flakey-fee-distributor branch from 66a18be to 58de927 Compare February 19, 2025 12:24
Copy link

cloudflare-workers-and-pages bot commented Feb 19, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 58de927
Status: ✅  Deploy successful!
Preview URL: https://18c2298d.agoric-sdk.pages.dev
Branch Preview URL: https://anil-10988-flakey-fee-distri.agoric-sdk.pages.dev

View logs

@mergify mergify bot merged commit f0fb3c8 into master Feb 19, 2025
83 checks passed
@mergify mergify bot deleted the anil/10988-flakey-fee-distributor branch February 19, 2025 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replaceFeeDistributor flake
2 participants