Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
327 stars 208 forks source link

retry helper to durably watch idempotent promises #9541

Closed turadg closed 1 month ago

turadg commented 4 months ago

What is the Problem Being Solved?

Vows that wrap promises risk having the promise sever. E.g. a remote call to agoricNames could sever upon upgrade. Fortunately, in that case simply retrying is safe.

Description of the Design

A helper that makes a remote call that will retry upon upgrade failure.

const lookupAsset = retry(privateArgs.agoricNames, 'lookup', 'vbankAsset');

Security Considerations

Scaling Considerations

Test Plan

Upgrade Considerations

turadg commented 4 months ago

https://github.com/Agoric/agoric-sdk/pull/9685 added the API for one but it doesn't support upgrade

mhofman commented 4 months ago

I'm thinking that retriable should support not only disconnection reasons, but also error rejections whose message is "vat terminated". This can currently occur (#9582) when we send a message to a heap or virtual object in the remote vat which gets upgraded. Since one purpose of retriable is to handle operations breaking because of the upgrade of a remote vat that isn't resumable, it's possible that this situation could legitimately occur in the middle of a retriable flow. Unlike upgrade disconnection, we must limit ourselves to a single consecutive retry since we cannot differentiate the case of serial remote upgrades (no incarnation number field).

OTOH, retrySend might not need to support "vat terminated" errors because retrying the send will only help if the error is nested: it's not the target of the retriable send that was abandoned, but an object used by the remote, and retrying the operation will cause the remote to re-obtain a fresh object.

erights commented 4 months ago

Would it be better for these to be rejected with UpgradeDisconnection as well?

mhofman commented 4 months ago

possibly something similar, but we likely need the ability to differentiate between the target being abandoned and the pending operation on the target being disconnected. In #9582 we discuss in the future using a chain of error.cause to maintain more information on disconnection errors including stack traces.

mhofman commented 3 months ago

Recent discussions and audit of use cases highlights the following:

dckc commented 2 months ago

A wish: rather than moving the method name to a string argument:

const lookupAsset = retry(privateArgs.agoricNames, 'lookup', 'vbankAsset');

keep it in the method position, so that we could preserve types:

const lookupAsset = retryE(privateArgs.agoricNames).lookup('vbankAsset');

but that reminds me of heapVowE. Is it straightforward to make a durable form of heapVowE using makeE from @agoric/vows?

mhofman commented 2 months ago

so that we could preserve types

I believe preserving types is an orthogonal concern. There may be a way to maintain typing with the current form.

The suggested retryE is really proxy sugar on top of the underlying retry functionality.

that reminds me of heapVowE. Is it straightforward to make a durable form of heapVowE using makeE from @agoric/vows?

I really do not understand this.

mhofman commented 1 month ago

retryable was done in #9785. #10227 will track a potential retrySend