VowTools retryable #9785

mhofman · 2024-07-25T22:39:05Z

Description

Adds a retryable helper which retries an idempotent function on upgrade disconnection reasons.

Update the upgrade disconnection isRetryableReason predicate to accept vat terminated errors created by liveslots when trying to message an ephemeral object disconnected by an upgrade (until #9582 is addressed)

Remove the stub retriable helper, and use the retryable implementation.

Security Considerations

To avoid infinite loops, the vat terminated detection logic does not allow retrying such consecutive reasons.

Scaling Considerations

None

Documentation Considerations

JSDoc for retryable

Testing Considerations

Full unit test and liveslots upgrade test coverage

Upgrade Considerations

The current retriable implementation was just a stub that was advertised as not actually working with upgrades. This introduces the actual implementation, which can be picked up by contract authors by simply updating to the next NPM release containing this change. Going forward, defined retryable flows will need to be redefined on upgrade like any other durable exo defined by a contract.

codecov · 2024-07-25T22:41:16Z

Codecov Report

Attention: Patch coverage is 56.27530% with 108 lines in your changes missing coverage. Please review.

Project coverage is 56.39%. Comparing base (173e1d8) to head (a36498a).
Report is 35 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9785      +/-   ##
==========================================
+ Coverage   56.37%   56.39%   +0.02%     
==========================================
  Files         667      668       +1     
  Lines      207526   207871     +345     
  Branches      339      368      +29     
==========================================
+ Hits       116986   117227     +241     
- Misses      90422    90526     +104     
  Partials      118      118

Files	Coverage Δ
packages/internal/src/upgrade-api.js	`100.00% <100.00%> (ø)`
packages/orchestration/src/exos/chain-hub.js	`98.53% <100.00%> (-0.01%)`	⬇️
packages/orchestration/src/utils/start-helper.js	`96.17% <100.00%> (+0.02%)`	⬆️
packages/vow/src/tools.js	`100.00% <100.00%> (+3.89%)`	⬆️
packages/vow/src/retriable.js	`50.45% <50.45%> (ø)`

... and 5 files with indirect coverage changes

erights · 2024-07-26T00:10:02Z

packages/internal/src/upgrade-api.js

 harden(isUpgradeDisconnection);
+
+/**
+ * Returns whether a reason is a 'vat terminated' error generated when an object


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.

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

Please add a comment here with that link.

erights · 2024-07-26T00:12:46Z

packages/internal/test/upgrade-api.test.js

+  const error = harden(Error('vat terminated'));
+  const remoteError = fromCapData(toCapData(error));
+
+  t.true(isAbandonedError(remoteError));


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

packages/orchestration/src/exos/chain-hub.js

packages/orchestration/src/utils/start-helper.js

packages/orchestration/test/examples/snapshots/sendAnywhere.test.ts.md

erights · 2024-07-26T00:38:49Z

packages/vow/vat.js

 import { makeHeapZone } from '@agoric/base-zone/heap.js';
 import { makeE, prepareVowTools as rawPrepareVowTools } from './src/index.js';

 /** @type {import('./src/types.js').IsRetryableReason} */
 const isRetryableReason = (reason, priorRetryValue) => {
  if (
    isUpgradeDisconnection(reason) &&
-    (!priorRetryValue ||
+    (!isUpgradeDisconnection(priorRetryValue) ||


What is the significance of this change? (including lines 24-28 below)

When shortening a vow, or checking whether to retry a retriable flow, we will now consider that we're not eligible for a retry when:

The current and previous reasons were both "upgrade disconnection" records, but only if different incarnation

The current and previous reasons were both "vat terminated" errors

In particular the following situations are still eligible for retries:

Going from a "vat terminated" error to an "upgrade disconnection" record, or vice versa

Going from an "upgrade disconnection" record to another, if the incarnation number changes

Before this change, only "upgrade disconnection" records were recorded as "previous retry reason" so we need to narrow down the check here.

As mentioned IMO we ultimately want to implement a .cause resolution and only root checks in disconnection records, as described in #9582

cloudflare-workers-and-pages · 2024-09-23T04:38:00Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`0237350`
Status:	✅ Deploy successful!
Preview URL:	https://8f3d1453.agoric-sdk.pages.dev
Branch Preview URL:	https://mhofman-9541-retriable.agoric-sdk.pages.dev

View logs

refs: #9541 ## Description Extracted the chain hub changes from #9785 Changed `makeChainHub` to accept a zone instead of it implicitly creating a heap zone. A durable zone is required for `retriable`. ### Security Considerations None ### Scaling Considerations This store the map in durable storage instead of keeping it in the heap, which uses more persistent storage. ### Documentation Considerations Not sure if any docs need to be updated besides the JS doc for `makeChainHub` ### Testing Considerations Updated tests and examples ### Upgrade Considerations This moves into durable storage some info that was heap only before, effectively commiting ourselves to the shape of the data in these maps.

refs: #9303 ## Description This adds a test for upgrading send-anywhere. It doesn't yet pass without these in-flight PRs, - #9736 - #9785 We've agreed to land this without those to reduce work-in-flight. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations The failing test has a link to the issue to make it pass. ### Upgrade Considerations none

michaelfig

I like this implementation; it is clear and direct. I didn't come across any issues that I think would prevent merging it.

LGTM!

turadg · 2024-10-01T21:47:32Z

packages/vow/src/types.ts

@@ -100,6 +100,27 @@ export type AsPromiseFunction<
  watcherArgs?: C | undefined,
 ) => Promise<TResult1 | TResult2>;

+export interface RetryableTool {
+  /**


thanks for the jsdoc!

turadg · 2024-10-01T21:54:11Z

packages/vow/test/retryable-restart.test.js

+
+      zone.exo('DurableVowTestWatcher', undefined, {
+        onFulfilled(value) {
+          t.is(value, 42, 'vow resolved with value 42');


consider losing the t.plan to make the logic clearer. E.g. in the test closer define a promise kit and resolve it with the when(result). Then at the end of the test have await t.is(await resultP, 24), something like that.

Not a biggy but would make the logic easier to read and modify

A promise kit wouldn't help because it doesn't guarantee the other handlers didn't trigger (as promise resolve logic ignores any subsequent resolution). Of course we could set a counter to check how many times the handlers got triggered, but at that point, isn't that what plan does?

because it doesn't guarantee the other handlers didn't trigger

i was thinking

Suggested change

t.is(value, 42, 'vow resolved with value 42');

if (value === 42) {

p.resolve('success');

}

But you're right that something else could resolve it with success even without success, so I suppose that t.plan() is safer

turadg · 2024-10-01T21:55:29Z

packages/vow/test/retryable-restart.test.js

+  t.false(zone.isStorable(nonStorableArg), 'arg is actually non storable');
+
+  let resultV;
+  t.notThrows(() => {


thanks for these checks that help describe the expected behavior

mhofman requested a review from erights July 25, 2024 22:41

erights reviewed Jul 26, 2024

View reviewed changes

packages/orchestration/src/utils/start-helper.js Outdated Show resolved Hide resolved

erights reviewed Jul 26, 2024

View reviewed changes

packages/orchestration/test/examples/snapshots/sendAnywhere.test.ts.md Outdated Show resolved Hide resolved

erights reviewed Jul 26, 2024

View reviewed changes

turadg mentioned this pull request Jul 29, 2024

9303 test upgrading send-anywhere #9719

Merged

mhofman force-pushed the mhofman/9541-retriable branch 2 times, most recently from 20417e3 to ea795cd Compare September 23, 2024 05:01

mhofman mentioned this pull request Sep 23, 2024

fix(orchestration): chainHub accepts zone #10129

Merged

mhofman force-pushed the mhofman/9541-retriable branch 2 times, most recently from 515a4e3 to 09980df Compare October 1, 2024 08:23

mhofman requested review from michaelfig and erights October 1, 2024 08:33

mhofman changed the title ~~VowTools retriable~~ VowTools retryable Oct 1, 2024

mhofman marked this pull request as ready for review October 1, 2024 18:03

mhofman requested a review from a team as a code owner October 1, 2024 18:03

michaelfig approved these changes Oct 1, 2024

View reviewed changes

mhofman force-pushed the mhofman/9541-retriable branch 2 times, most recently from 34980e5 to f38e095 Compare October 1, 2024 21:22

mhofman added the automerge:rebase Automatically rebase updates, then merge label Oct 1, 2024

mhofman added 6 commits October 1, 2024 21:38

feat(vow): abandoned errors are retriable

1ac054f

type(liveslots-tools): missing types for strict test env helpers

f82a96b

chore(vow): retriable -> retryable

1b1772b

chore(orchestration): retriable -> retryable

cf36da4

feat(vow): retryable tools

5303913

test(vow): add retryable tests

0237350

mhofman force-pushed the mhofman/9541-retriable branch from f38e095 to 0237350 Compare October 1, 2024 21:38

turadg reviewed Oct 1, 2024

View reviewed changes

mergify bot merged commit 69f8e4b into master Oct 1, 2024
79 of 80 checks passed

mergify bot deleted the mhofman/9541-retriable branch October 1, 2024 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VowTools retryable #9785

VowTools retryable #9785

mhofman commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

erights Jul 26, 2024

erights Jul 26, 2024

erights Jul 26, 2024

erights Jul 26, 2024 •

edited

Loading

mhofman Jul 26, 2024

cloudflare-workers-and-pages bot commented Sep 23, 2024 •

edited

Loading

michaelfig left a comment

turadg Oct 1, 2024

turadg Oct 1, 2024

mhofman Oct 1, 2024

turadg Oct 1, 2024 •

edited

Loading

turadg Oct 1, 2024

-          t.is(value, 42, 'vow resolved with value 42');
+          if (value === 42) {
+            p.resolve('success');
+          }

VowTools retryable #9785

VowTools retryable #9785

Conversation

mhofman commented Jul 25, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

codecov bot commented Jul 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Sep 23, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

michaelfig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

erights Jul 26, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Sep 23, 2024 •

edited

Loading

turadg Oct 1, 2024 •

edited

Loading