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(vow): report unhandled vow rejections on collection or upgrade #10686

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Dec 12, 2024

closes: #10576

Description

This PR uses the design given in #10576 as a guideline. Some adaptations were made, such as needing to touch the vow states at different places and not sinking the ephemeral promise rejections.

Security Considerations

None expected, as the design does not introduce new facilities than have already been available.

Scaling Considerations

More bookkeeping data structures, but only linearly with the number of pending rejected vows.

Documentation Considerations

n/a

Testing Considerations

Testing unhandled rejections under AVA is impossible without causing AVA to report them as failures. The compromise made in this PR is not to test unhandled rejections by default, instead only if running the tests with yarn test:rejection.

Upgrade Considerations

The test suite contains a simple VatData upgrade test to verify basic unhandled vow rejections.

Copy link

cloudflare-workers-and-pages bot commented Dec 12, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: c20b271
Status: ✅  Deploy successful!
Preview URL: https://0697ea21.agoric-sdk.pages.dev
Branch Preview URL: https://mfig-unhandled-vow-rejection.agoric-sdk.pages.dev

View logs

@michaelfig michaelfig force-pushed the mfig-unhandled-vow-rejection branch 3 times, most recently from 2c1627b to 7b6d43f Compare December 16, 2024 00:34
@michaelfig michaelfig self-assigned this Dec 16, 2024
@michaelfig michaelfig added devex developer experience force:integration Force integration tests to run on PR labels Dec 16, 2024
@michaelfig michaelfig force-pushed the mfig-unhandled-vow-rejection branch from 7b6d43f to 4b66d13 Compare December 16, 2024 03:11
Run `yarn test:rejection` to test unhandled rejection reporting.

CAVEAT: AVA has no way to expect unhandled rejections and so will
fail the tests with this setting.
@michaelfig michaelfig marked this pull request as ready for review December 16, 2024 03:20
@michaelfig michaelfig force-pushed the mfig-unhandled-vow-rejection branch from 4b66d13 to 0e5ec4e Compare December 16, 2024 03:20
@michaelfig michaelfig requested a review from a team as a code owner December 16, 2024 03:20
@michaelfig
Copy link
Member Author

michaelfig commented Dec 16, 2024

@mhofman, I'd be interested in hearing what you think about patching AVA to honour a package.json option and/or command-line flag that exits with a 0 status code even if there were unhandled rejections?

It's not hard to find the relevant code by searching node_modules/ava for unhandledRejections, so I could probably cook up a patch for this once we decide on the appropriate behaviour.

@turadg
Copy link
Member

turadg commented Dec 16, 2024

patching AVA to honour a package.json option and/or command-line flag that exits with a 0 status code even if there were unhandled rejections?

I'll offer an opinion: patches are hard to maintain and should be a last resort. The stock Ava can spawn a worker thread or process as suggested here.

@dckc
Copy link
Member

dckc commented Dec 16, 2024

before closing #10576 , re upgrade considerations: are there any vats deployed with the previous vow package? Do we plan to upgrade them?

Also please consider how to verify this fix in a testnet.

For example, an a3p test could have an onFulfilled handler that throws.

Or... perhaps that's not cost-effective? Perhaps unit testing suffices?

@michaelfig
Copy link
Member Author

I'll offer an opinion: patches are hard to maintain and should be a last resort.

I agree that our AVA patch has become unwieldy. However, the change I decided to make affects three lines.

The stock Ava can spawn a worker thread or process as suggested here.

The costs of working around stock AVA's stubbornness are far greater and involve much more code and conceptual overhead than patching AVA to be more flexible.

@michaelfig
Copy link
Member Author

before closing #10576 , re upgrade considerations: are there any vats deployed with the previous vow package? Do we plan to upgrade them?

Vats using vows are:

ibc
localchain
network
orchestration
smart-wallet
transfer
zoe

I don't expect we'd actually upgrade them until u20, when we're targeting Upgrade All Vats.

Also please consider how to verify this fix in a testnet.
Or... perhaps that's not cost-effective? Perhaps unit testing suffices?

I agree it is worth at least an a3p test, which would serve as a canary if we ever unexpectedly broke the resumability of vows. I'll work on that.

@michaelfig michaelfig marked this pull request as draft December 18, 2024 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devex developer experience force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vows do not report unhandled rejections
3 participants