Explicit async-flow wake #10208

mhofman · 2024-10-03T18:12:53Z

closes: #9377
closes: #9383
refs: #9753

best reviewed commit-by-commit

Description

This PR adds a lot of helpers to more ergonomically write async flow / liveslots upgrade tests, uses these to add a bunch of test cases around async-flow wake, then changes the behavior of async-flow to require an explicit wakeAll call once all flows are redefined. It updates the withOrchestration helper to call wakeAll.

Security Considerations

To avoid the risk of stuck flows if wakeAll is not called, a "finalize gate" is introduced that would cause the upgrade to fail if the function is not called.

Scaling Considerations

None

Documentation Considerations

The change in behavior is a breaking change, and marked as such, however no deployed code directly uses async-flow.

Testing Considerations

Lots of testing infra updates.
Full unit testing of the wake logic, including the new explicit wakeAll and its gate.

TODO: I am looking at further streamlining the test helpers to make it more obvious the tests are all serial and carry state between each test definition.

Upgrade Considerations

Deployable as new NPM packages. I have to check whether the breaking change commit would also cause the orchestration package to get a semver major change, as that may be unnecessary.
TODO: potentially split that commit

cloudflare-workers-and-pages · 2024-10-03T18:15:36Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`332eb05`
Status:	✅ Deploy successful!
Preview URL:	https://4bd08da4.agoric-sdk.pages.dev
Branch Preview URL:	https://mhofman-9377-wake-all.agoric-sdk.pages.dev

View logs

…rrors

Implicitly prepare the vowTools

Rewrite wake test to use helper

michaelfig

It's a really good improvement! I'd be happy to see this land given your attention to my change suggestions.

michaelfig · 2024-10-04T00:47:57Z

packages/async-flow/src/async-flow.js

-  // Cannot call this until everything is prepared, so postpone to a later
-  // turn. (Ideally, we'd postpone to a later crank because prepares are
-  // allowed anytime in the first crank. But there's currently no pleasant
-  // way to postpone to a later crank.)
-  // See https://github.com/Agoric/agoric-sdk/issues/9377
-  const allWokenP = E.when(null, () => adminAsyncFlow.wakeAll());
-


Good to see this code go away. wakeUpgradeGate has the right semantics and is much more understandable.

michaelfig · 2024-10-04T00:49:36Z

packages/async-flow/test/_utils.js

 * @import {Zone} from '@agoric/base-zone';
 * @import {PreparationOptions} from '../src/types.js';
 */

+/** @typedef {(e: any, t: ExecutionContext) => void} TestAsyncFlowPanicHandler */


michaelfig · 2024-10-04T00:52:40Z

packages/async-flow/test/async-flow-crank.test.js

packages/async-flow/test/async-flow-wake.test.js

michaelfig · 2024-10-04T01:14:43Z

packages/async-flow/test/async-flow-wake.test.js

+// new incarnation, but otherwise should wait until a wake watcher is triggered.
+
+import { Fail } from '@endo/errors';
+import { testAsyncLife } from './prepare-test-env-ava.js';


I suggest there could be an initial testAsyncLife, which returns a new testAsyncLife with its reincarnation state curried in:

Suggested change

import { testAsyncLife } from './prepare-test-env-ava.js';

import { testAsyncLifeFromScratch } from './prepare-test-env-ava.js';

michaelfig · 2024-10-04T01:15:35Z

packages/async-flow/test/async-flow-wake.test.js

+  return { resolver, waiter };
+};
+
+testAsyncLife(


Suggested change

testAsyncLife(

const testAsyncLifeAfterSetup = testAsyncLifeFromScratch(

michaelfig · 2024-10-04T01:16:59Z

packages/async-flow/test/async-flow-wake.test.js

+  },
+);
+
+testAsyncLife(


Suggested change

testAsyncLife(

const testAsyncLifeAfterSleeping = testAsyncLifeAfterSetup(

michaelfig · 2024-10-04T01:19:00Z

packages/async-flow/test/async-flow.test.js

Nice mechanical simplification.

packages/async-flow/src/async-flow.js

kriskowal · 2024-10-04T19:57:18Z

packages/async-flow/src/async-flow.js

+ * upgrade. In the first incarnation, the state is assumed to be born already
+ * finalized, and there is no failure if the "finalize" is not explicitly
+ * performed. In future incarnation the upgrade will fail if the "finalize" step
+ * is not explicitly performed.


please name the entity that is responsible for performing the finalization

I'm not sure how to shoe horn that in this comment. While defined within async-flow.js, this is written as a generic utility.

How about something along the lines:

Helper to enforce that a post-redefinition "finalize" step has been performed on upgrade. In some cases, it may be necessary to execute some operations on upgraded durable objects, which can only happen after the objects' kinds and their dependencies have all been redefined. A "finalize upgrade gate" helps enforce that this finalize step is performed by the contract on upgrade. It returns a function which should be called by the finalize step to indicate the operations were performed. In the first incarnation, it assumes that no finalizer step is necessary, and there is no failure if the function is not called. However, in a future incarnation, the upgrade will fail if the function is not called.

mhofman · 2024-10-04T21:36:04Z

I'm gonna need to force push to split one of the commits in 2, sorry for the inconvenience.

Done: 198ad9e..332eb05

Co-authored-by: Michael FIG <mfig@agoric.com>

Refs: Agoric/agoric-sdk#10208 ## Description the `ava` test functions can be nested more than one level deep. This change recurses test functions with known names until none is found. ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations Relaxes the type of acceptable test functions as it only really cares about it being a test function, regardless of whatever chain expandos it may have. ### Testing Considerations Manually verified that a `test.serial.only` does show errors details as expected. Not sure if we have any automated coverage for this. ### Compatibility Considerations None ### Upgrade Considerations This is used by tests only, and can be consumed as a non breaking change simply by using the new NPM package including this fix.

mhofman added 11 commits October 3, 2024 19:16

fix(liveslots-tools): startLife enforces build single crank settlement

8658167

feat(liveslots-tools): startLife option to handle kind reconnection e…

debf2a5

…rrors

chore(async-flow): rename misidentified no-this test

440f8d8

chore(async-flow): rename wake test

b09b87c

test(async-flow): refactor async flow test tools

5209a8f

Implicitly prepare the vowTools

test(async-flow): pass test context to panic handler

fa1b76b

test(async-flow): test helpers for async flow

afca2f9

Rewrite wake test to use helper

test(async-flow): pass test context to kind reconnection error handler

603c10f

test(async-flow): support explicit reset between tests

2352c25

test(async-flow): proper wake tests

8ae8d08

feat(async-flow): check wake is called on upgrades

d739b34

mhofman force-pushed the mhofman/9377-wake-all branch from 8beaf10 to ee710ca Compare October 3, 2024 19:17

mhofman requested review from kriskowal and michaelfig October 3, 2024 22:50

mhofman marked this pull request as ready for review October 3, 2024 22:50

mhofman requested a review from a team as a code owner October 3, 2024 22:50

michaelfig approved these changes Oct 4, 2024

View reviewed changes

mhofman force-pushed the mhofman/9377-wake-all branch from 8e80417 to ee710ca Compare October 4, 2024 14:53

kriskowal approved these changes Oct 4, 2024

View reviewed changes

mhofman force-pushed the mhofman/9377-wake-all branch from e21b11e to 198ad9e Compare October 4, 2024 21:33

mhofman and others added 4 commits October 4, 2024 21:41

feat(orchestration): explicitly wake flows after definition

87b619d

feat(async-flow)!: require explicit wakeAll after flows redefined

ba9a000

fixup! feat(async-flow)!: require explicit wakeAll after flows redefined

9948261

fixup! test(async-flow): proper wake tests

332eb05

Co-authored-by: Michael FIG <mfig@agoric.com>

mhofman force-pushed the mhofman/9377-wake-all branch from 198ad9e to 87b619d Compare October 4, 2024 21:46

mhofman mentioned this pull request Oct 17, 2024

fix(ses-ava): wrap test deeply endojs/endo#2589

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicit async-flow wake #10208

Explicit async-flow wake #10208

mhofman commented Oct 3, 2024 •

edited by kriskowal

Loading

cloudflare-workers-and-pages bot commented Oct 3, 2024 •

edited

Loading

michaelfig left a comment

michaelfig Oct 4, 2024

michaelfig Oct 4, 2024

michaelfig Oct 4, 2024

michaelfig Oct 4, 2024

michaelfig Oct 4, 2024

michaelfig Oct 4, 2024

michaelfig Oct 4, 2024

kriskowal Oct 4, 2024

mhofman Oct 4, 2024 •

edited

Loading

mhofman commented Oct 4, 2024 •

edited

Loading

	import { testAsyncLife } from './prepare-test-env-ava.js';
	import { testAsyncLifeFromScratch } from './prepare-test-env-ava.js';

	testAsyncLife(
	const testAsyncLifeAfterSetup = testAsyncLifeFromScratch(

	testAsyncLife(
	const testAsyncLifeAfterSleeping = testAsyncLifeAfterSetup(

Explicit async-flow wake #10208

Are you sure you want to change the base?

Explicit async-flow wake #10208

Conversation

mhofman commented Oct 3, 2024 • edited by kriskowal Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Oct 3, 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

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

Choose a reason for hiding this comment

mhofman Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

mhofman commented Oct 4, 2024 • edited Loading

mhofman commented Oct 3, 2024 •

edited by kriskowal

Loading

cloudflare-workers-and-pages bot commented Oct 3, 2024 •

edited

Loading

mhofman Oct 4, 2024 •

edited

Loading

mhofman commented Oct 4, 2024 •

edited

Loading