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

VowTools retryable #9785

Merged
merged 6 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/boot/test/orchestration/contract-upgrade.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ test.after.always(t => t.context.shutdown?.());
* Because the send-anywhere flow requires a lookup(), it waits forever. This
* gives us a point at which we can upgrade the vat with a working agoricNames
* and see that the flow continues from that point. (The lookup call is not made
* directly in a flow, but instead from a host API which uses the retriable
* helper. As such it tests both the idempotent retry mechanism of retriable on
* directly in a flow, but instead from a host API which uses the retryable
* helper. As such it tests both the idempotent retry mechanism of retryable on
* upgrades, and the ability to resume an async-flow for which a host vow
* settles after an upgrade.)
*/
Expand Down
26 changes: 25 additions & 1 deletion packages/internal/src/upgrade-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,29 @@ harden(makeUpgradeDisconnection);
* @returns {reason is UpgradeDisconnection}
*/
export const isUpgradeDisconnection = reason =>
isFrozen(reason) && matches(reason, UpgradeDisconnectionShape);
reason != null && // eslint-disable-line eqeqeq
isFrozen(reason) &&
matches(reason, UpgradeDisconnectionShape);
harden(isUpgradeDisconnection);

/**
* Returns whether a reason is a 'vat terminated' error generated when an object
Copy link
Member

Choose a reason for hiding this comment

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

I understand this as an expedient measure, until we distinguish this rejection condition some other way. Either, like UpgradeDisconnection, use something other than an Error as a rejection to be tested. For an error, the only content that a program should make a semantic decision on is .name. Most emphatically, code should not make a semantic decision on an error .message.

So, an encapsulated temporary expedient measure is fine if clearly marked as such, and with a filed issue to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

( @mhofman let me know the relevant issue is #9582 )

Please add a comment here with that link.

* is abandoned by a vat during an upgrade.
*
* Normally we do not want to rely on the `message` of an error object, but this
* is a pragmatic solution to the current state of vat upgrade errors. In the
* future we'd prefer having an error with a cause referencing a disconnection
* object like for promise rejections. See
* https://github.com/Agoric/agoric-sdk/issues/9582
*
* @param {any} reason
* @returns {reason is Error}
*/
export const isAbandonedError = reason =>
reason != null && // eslint-disable-line eqeqeq
isFrozen(reason) &&
matches(reason, M.error()) &&
// We're not using a constant here since this special value is already
// sprinkled throughout the SDK
reason.message === 'vat terminated';
harden(isAbandonedError);
15 changes: 15 additions & 0 deletions packages/internal/test/upgrade-api.test.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// @ts-check
import test from 'ava';
import { makeMarshal } from '@endo/marshal';

import {
makeUpgradeDisconnection,
isUpgradeDisconnection,
isAbandonedError,
} from '../src/upgrade-api.js';

test('isUpgradeDisconnection must recognize disconnection objects', t => {
Expand All @@ -18,3 +21,15 @@ test('isUpgradeDisconnection must recognize original-format disconnection object
});
t.true(isUpgradeDisconnection(disconnection));
});

test('isAbandonedError recognizes marshalled vat terminated errors', t => {
const { fromCapData, toCapData } = makeMarshal(undefined, undefined, {
serializeBodyFormat: 'smallcaps',
errorIdNum: 70_000,
marshalSaveError: () => {},
});
const error = harden(Error('vat terminated'));
const remoteError = fromCapData(toCapData(error));

t.true(isAbandonedError(remoteError));
Copy link
Member

Choose a reason for hiding this comment

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

Nice. But to reiterate, this probably depends on stability beyond what I expect to propose for ocapn to mandate.

});
6 changes: 3 additions & 3 deletions packages/orchestration/src/exos/chain-hub.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ export const makeChainHub = (zone, agoricNames, vowTools) => {
valueShape: M.string(),
});

const lookupChainInfo = vowTools.retriable(
const lookupChainInfo = vowTools.retryable(
zone,
'lookupChainInfo',
/** @param {string} chainName */
Expand All @@ -227,7 +227,7 @@ export const makeChainHub = (zone, agoricNames, vowTools) => {
},
);

const lookupConnectionInfo = vowTools.retriable(
const lookupConnectionInfo = vowTools.retryable(
zone,
'lookupConnectionInfo',
/**
Expand Down Expand Up @@ -258,7 +258,7 @@ export const makeChainHub = (zone, agoricNames, vowTools) => {
);

/* eslint-disable no-use-before-define -- chainHub defined below */
const lookupChainsAndConnection = vowTools.retriable(
const lookupChainsAndConnection = vowTools.retryable(
zone,
'lookupChainsAndConnection',
/**
Expand Down
2 changes: 1 addition & 1 deletion packages/orchestration/src/utils/zoe-tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export const makeZoeTools = (zcf, { when, allVows, allSettled, asVow }) => {
// const { zcfSeat: tempSeat, userSeat: userSeatP } =
// zcf.makeEmptySeatKit();
// const uSeat = await userSeatP;
// // TODO how do I store in the place for this retriable?
// // TODO how do I store in the place for this retryable?
// atomicTransfer(zcf, srcSeat, tempSeat, amounts);
// tempSeat.exit();
// return uSeat;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ Generated by [AVA](https://avajs.dev).
chainInfos: {},
connectionInfos: {},
denom: {},
lookupChainInfo_kindHandle: 'Alleged: kind',
lookupChainsAndConnection_kindHandle: 'Alleged: kind',
lookupConnectionInfo_kindHandle: 'Alleged: kind',
},
contract: {
'ChainHub Admin_kindHandle': 'Alleged: kind',
Expand Down Expand Up @@ -74,8 +77,11 @@ Generated by [AVA](https://avajs.dev).
},
},
vows: {
AdminRetryableFlow_kindHandle: 'Alleged: kind',
AdminRetryableFlow_singleton: 'Alleged: AdminRetryableFlow',
PromiseWatcher_kindHandle: 'Alleged: kind',
VowInternalsKit_kindHandle: 'Alleged: kind',
WatchUtils_kindHandle: 'Alleged: kind',
retryableFlowForOutcomeVow: {},
},
}
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ Generated by [AVA](https://avajs.dev).
chainName: 'agoric',
},
},
lookupChainInfo_kindHandle: 'Alleged: kind',
lookupChainsAndConnection_kindHandle: 'Alleged: kind',
lookupConnectionInfo_kindHandle: 'Alleged: kind',
},
contract: {
'ChainHub Admin_kindHandle': 'Alleged: kind',
Expand Down Expand Up @@ -208,8 +211,11 @@ Generated by [AVA](https://avajs.dev).
},
},
vows: {
AdminRetryableFlow_kindHandle: 'Alleged: kind',
AdminRetryableFlow_singleton: 'Alleged: AdminRetryableFlow',
PromiseWatcher_kindHandle: 'Alleged: kind',
VowInternalsKit_kindHandle: 'Alleged: kind',
WatchUtils_kindHandle: 'Alleged: kind',
retryableFlowForOutcomeVow: {},
},
}
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ Generated by [AVA](https://avajs.dev).
},
},
denom: {},
lookupChainInfo_kindHandle: 'Alleged: kind',
lookupChainsAndConnection_kindHandle: 'Alleged: kind',
lookupConnectionInfo_kindHandle: 'Alleged: kind',
},
contract: {
orchestration: {
Expand Down Expand Up @@ -156,8 +159,11 @@ Generated by [AVA](https://avajs.dev).
},
},
vows: {
AdminRetryableFlow_kindHandle: 'Alleged: kind',
AdminRetryableFlow_singleton: 'Alleged: AdminRetryableFlow',
PromiseWatcher_kindHandle: 'Alleged: kind',
VowInternalsKit_kindHandle: 'Alleged: kind',
WatchUtils_kindHandle: 'Alleged: kind',
retryableFlowForOutcomeVow: {},
},
}
Binary file not shown.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ export { flushIncarnation };
export { eventLoopIteration as nextCrank };

/**
* @import { PromiseKit } from '@endo/promise-kit'
* @import { Baggage } from '@agoric/swingset-liveslots'
* @import { ReincarnateOptions } from './setup-vat-data.js'
*/

Expand All @@ -37,6 +39,7 @@ export const annihilate = (options = {}) => {
return incarnation;
};

/** @returns {Baggage} */
export const getBaggage = () => {
return incarnation.fakeVomKit.cm.provideBaggage();
};
Expand All @@ -51,7 +54,7 @@ export const nextLife = (fromIncarnation = incarnation) => {
};

/**
* @template {(baggage: import('@agoric/swingset-liveslots').Baggage) => Promise<any> | any} B
* @template {(baggage: Baggage) => Promise<any> | any} B
* @param {B} build
* @param {(tools: Awaited<ReturnType<B>>) => Promise<void> | void} [run]
* @param {object} [options]
Expand All @@ -72,7 +75,7 @@ export const startLife = async (
oldIncarnationNumber,
);
const { fakeVomKit } = nextLife(fromIncarnation);
/** @type {Map<string, import('@endo/promise-kit').PromiseKit<any>>} */
/** @type {Map<string, PromiseKit<any>>} */
const previouslyWatchedPromises = new Map();
let buildTools;
try {
Expand Down
4 changes: 2 additions & 2 deletions packages/vow/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,8 @@ Here is an (oversimplified) algorithm that `watch` and `when` use to obtain a
final result:

```js
// Directly await the non-retriable original specimen.
// This is non-retriable because we don't know how our caller obtained
// Directly await the non-retryable original specimen.
// This is non-retryable because we don't know how our caller obtained
// it in the first place, since it is an application-specific detail
// that may not be side-effect free.
let result = await specimenP;
Expand Down
Loading
Loading