refactor(daemon): Synchronize host `evaluate` method #2092

rekmarks · 2024-02-21T06:00:43Z

Progresses: #2086
Fixes: #2021

Synchronizes the host's evaluate() method by delegating all incarnations to the daemon via incarnateEval(). The latter is responsible for incarnating its dependents as necessary, and for the generation of their formula numbers. To facilitate this, the synchronous methods incarnateLookupSync() and incarnateWorkerSync() have been added. These methods synchronously mutate the formula graph, and return promises that resolve when the formulas have been written to disk. The result is that the formula graph is mutated within a single turn of the event loop.

To achieve this, the implementation introduces new constraints on the daemon and its dependents. #2089 introduced the notion of "incarnating" values. By the current definition, incarnating a value consists of the following steps:

Collecting dependencies (async)
1. Generating requisite formula numbers (async)
  - We use the asynchronous signature of crypto.randomBytes to do
    this for performance reasons.
2. Incarnating any dependent values (recursion!)
Updating the in-memory formula graph (sync)
Writing the resulting formula to disk (async)
Reifiying the resulting value (async)

In order to make formula graph mutations mutually exclusive, we introduce a "formula graph mutex" under which step 1 must be performed. This mutex is currently only used by incarnateEval(), and must be expanded to its sibling methods in the future.

incarnateEval() also introduces the notion of "incarnation hooks" to the codebase. Originators of incarnations that are exogenous to the daemon may themselves have asynchronous work perform. For example, petName -> formulaIdentifier mappings are the responsibility of the host and its pet store, and pet names must be associated with their respective formula identifiers the moment that those identifiers are observable to the consumer. To handle sych asynchronous side effects, the implementation introduces a notion of "hooks" to incarnateEval(), with the intention of spreading this to other incarnation methods as necessary. These hooks receive as an argument all formula identifiers created by the incarnation, and are executed under the formula graph mutex. This will surface IO errors to the consumer, and help us uphold the principle of "death before confusion".

Also of note, provideValueForNumberedFormula has been modified such that the formula is written to disk after the controller has been constructed. This is critical in order to synchronize formula graph mutations.

Finally, it appears that the implementation incidentally fixed #2021. We may still wish to adopt the more robust solution proposed in that issue.

packages/daemon/src/daemon.js

rekmarks · 2024-02-22T01:41:40Z

packages/daemon/src/daemon.js

-   * @param {string[]} endowmentFormulaIdentifiers
+   * @param {(string | string[])[]} endowmentFormulaPointers
+   * @param {import('./types.js').EvalFormulaHook[]} hooks
+   * @param {string} [specifiedWorkerFormulaIdentifier]
   * @returns {Promise<{ formulaIdentifier: string, value: unknown }>}
   */
  const incarnateEval = async (


This is the only incarnateXyz() method that is synchronized as of this PR. Just noting that we will probably want to expand the conventions it establishes to its siblings ASAP.

packages/daemon/src/daemon.js

kriskowal

This looks like it closes the particular bug. Does this synchronize all formula writes or is the intention to capture those in follow-ups?

packages/daemon/src/daemon.js

packages/daemon/src/host.js

kriskowal · 2024-02-22T02:10:00Z

packages/daemon/src/host.js

-        }),
+          // TODO:lookup Check if a formula already exists for the path. May have to be
+          // done in the daemon itself.
+          return petNamePath;


This function appears to either return a pet name path or a formula identifier. Is a “pointer” the union of pet name path and identifier? If so, how are they distinguished?

I've renamed the property / array to endowmentFormulaIdsOrPaths. In this particular array, strings are formula identifiers and arrays are pet name paths. They are distinguished using typeof value === 'string'.

Ah, the plural agreement is confusing, but there’s perhaps nothing for it. I would like to consider (in follow-up) whether we can normalize these to formula identifiers up front.

packages/daemon/test/test-endo.js

kriskowal · 2024-02-22T02:22:08Z

packages/daemon/src/daemon.js

+          await incarnateWorkerSync(workerFormulaNumber)
+        ).formulaIdentifier,
+        endowmentFormulaIdentifiers: await Promise.all(
+          endowmentFormulaPointers.map(async formulaIdOrPath => {


I think formulaIdOrPath is a misnomer here. I recommend just using the name petNamePath and just fall through to petName if it’s a string. It should never be a nonce / formula identifier.

See: /~https://github.com/endojs/endo/pull/2092/files#r1498644209

rekmarks · 2024-02-22T05:10:02Z

@kriskowal

Does this synchronize all formula writes or is the intention to capture those in follow-ups?

The latter.

rekmarks · 2024-02-22T05:16:39Z

ATTN reviewers:

fix(daemon): Correctly use worker formula number in incarnateEval
incarnateEval() was accidentally passing worker formula identifiers instead of formula numbers when it received a specified worker formula identifier. This caused eval formulas to always fail on Windows due to a : in the path.

Surprisingly, fixing this caused the "indirect termination" test to fail. This is likely just surfacing a deeper problem, see #2074.

Makes `makeControllerForFormula` and its dependent functions synchronous. Also performs some unused parameter cleanup.

Synchronizes the host's `evaluate()` method by delegating all incarnations to the daemon via `incarnateEval()`. The latter is responsible for incarnating its dependents as necessary, and for the generation of their formula numbers. To facilitate this, the synchronous methods `incarnateLookupSync()` and `incarnateWorkerSync()` have been added. These methods synchronously mutate the formula graph, and return promises that resolve when the formulas have been written to disk. The result is that the formula graph is mutated within a single turn of the event loop. To achieve this, the implementation introduces new constraints on the daemon and its dependents. #2089 introduced the notion of "incarnating" values. By the current definition, incarnating a value consists of the following steps: 1. Collecting dependencies (async) a. Generating requisite formula numbers (async) - We use the asynchronous signature of `crypto.randomBytes` to do this for performance reasons. b. Incarnating any dependent values (recursion!) 2. Updating the in-memory formula graph (sync) 3. Writing the resulting formula to disk (async) 4. Reifiying the resulting value (async) In order to make formula graph mutations mutually exclusive, we introduce a "formula graph mutex" under which step 1 must be performed. This mutex is currently only used by `incarnateEval()`, and must be expanded to its sibling methods in the future. `incarnateEval()` also introduces the notion of "incarnation hooks" to the codebase. Originators of incarnations that are exogenous to the daemon may themselves have asynchronous work perform. For example, `petName -> formulaIdentifier` mappings are the responsibility of the host and its pet store, and pet names must be associated with their respective formula identifiers the moment that those identifiers are observable to the consumer. To handle sych asynchronous side effects, the implementation introduces a notion of "hooks" to `incarnateEval()`, with the intention of spreading this to other incarnation methods as necessary. These hooks receive as an argument all formula identifiers created by the incarnation, and are executed under the formula graph mutex. This will surface IO errors to the consumer, and help us uphold the principle of "death before confusion". Also of note, `provideValueForNumberedFormula` has been modified such that the formula is written to disk _after_ the controller has been constructed. This is critical in order to synchronize formula graph mutations. Finally, it appears that the implementation incidentally fixed #2021. We may still wish to adopt the more robust solution proposed in that issue.

`incarnateEval()` was accidentally passing worker formula identifiers instead of formula numbers when it received a specified worker formula identifier. This caused `eval` formulas to always fail on Windows due to a `:` in the path. Surprisingly, fixing this caused the "indirect termination" test to fail. This is likely just surfacing a deeper problem, see #2074.

- Rename various entities for clarity / correctness - Use terser implementation of async flow in `provideValueforNumberedFormula` - Add inline documentation Co-authored-by: Kris Kowal <kris@agoric.com>

kriskowal · 2024-02-22T18:20:39Z

packages/daemon/src/host.js

-        }),
+          // TODO:lookup Check if a formula already exists for the path. May have to be
+          // done in the daemon itself.
+          return petNamePath;


Ah, the plural agreement is confusing, but there’s perhaps nothing for it. I would like to consider (in follow-up) whether we can normalize these to formula identifiers up front.

kriskowal · 2024-02-22T18:22:51Z

packages/daemon/src/daemon.js

   */
-  const incarnateLookup = async (hubFormulaIdentifier, petNamePath) => {
-    const formulaNumber = await randomHex512();
+  const incarnateNumberedLookup = (


This will be tricky to replicate if we go on as I suggest to make lookup formulas single-hop.

Synchronizes the host's `makeUnconfined()` per #2086. Refactoring `daemon.js` in support of this goal fixed one bug while revealing another. In particular, #2074 is progressed by enabling indirect cancellation of caplets via their workers. The issue is not resolved since indirect cancellation of caplets via their caplet dependencies still does not work as intended. A new, failing regression test has been added for this specific case. The revealed bug is #2021, which we believed to be fixed by #2092. Rather than fixing the bug, that PR concealed it by always creating a new incarnation of `eval` formula workers, even if they already existed. The regression test for #2021 has been marked as failing, and we will have to find a different solution for it.

Progresses: #2086 Synchronizes the host's `makeUnconfined()` per #2086. Refactoring `daemon.js` in support of this goal fixed one bug while revealing another. In particular, #2074 is progressed by enabling indirect cancellation of caplets via their workers. The issue is not resolved since indirect cancellation of caplets via their caplet dependencies still does not work as intended. A new, failing regression test has been added for this specific case. The revealed bug is #2021, which we believed to be fixed by #2092. Rather than fixing the bug, that PR concealed it by always creating a new incarnation of `eval` formula workers, even if they already existed. The regression test for #2021 has been marked as failing, and we will have to find a different solution for it.

kumavis reviewed Feb 21, 2024

View reviewed changes

packages/daemon/src/daemon.js Outdated Show resolved Hide resolved

rekmarks force-pushed the rekmarks-mutually-exclusive-formula-graph-mutations branch 2 times, most recently from afdfa21 to c4f5572 Compare February 22, 2024 01:38

rekmarks requested a review from kumavis February 22, 2024 01:39

rekmarks marked this pull request as ready for review February 22, 2024 01:39

rekmarks requested a review from kriskowal February 22, 2024 01:39

rekmarks commented Feb 22, 2024

View reviewed changes

packages/daemon/src/daemon.js Outdated Show resolved Hide resolved

rekmarks force-pushed the rekmarks-mutually-exclusive-formula-graph-mutations branch from c4f5572 to b2522bf Compare February 22, 2024 01:45

kriskowal reviewed Feb 22, 2024

View reviewed changes

rekmarks force-pushed the rekmarks-mutually-exclusive-formula-graph-mutations branch from bdaf855 to d9c865c Compare February 22, 2024 05:07

rekmarks requested a review from kriskowal February 22, 2024 05:10

rekmarks and others added 4 commits February 22, 2024 08:18

refactor(daemon): Make makeControllerForFormula synchronous

3b585a2

Makes `makeControllerForFormula` and its dependent functions synchronous. Also performs some unused parameter cleanup.

refactor(daemon): Refine synchronized incarnateEval implementation

fcd9093

- Rename various entities for clarity / correctness - Use terser implementation of async flow in `provideValueforNumberedFormula` - Add inline documentation Co-authored-by: Kris Kowal <kris@agoric.com>

rekmarks force-pushed the rekmarks-mutually-exclusive-formula-graph-mutations branch from d9c865c to fcd9093 Compare February 22, 2024 16:18

kriskowal approved these changes Feb 22, 2024

View reviewed changes

rekmarks merged commit 065f966 into master Feb 22, 2024
14 checks passed

rekmarks deleted the rekmarks-mutually-exclusive-formula-graph-mutations branch February 22, 2024 18:36

rekmarks mentioned this pull request Feb 22, 2024

daemon: Formula graph must not be mutated concurrently #2086

Closed

12 tasks

rekmarks mentioned this pull request Mar 8, 2024

refactor(daemon): Synchronize host makeUnconfined() #2124

Merged

rekmarks mentioned this pull request Mar 9, 2024

daemon: eval-mediated worker names do not resolve #2021

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(daemon): Synchronize host `evaluate` method #2092

refactor(daemon): Synchronize host `evaluate` method #2092

rekmarks commented Feb 21, 2024 •

edited

Loading

rekmarks Feb 22, 2024

kriskowal left a comment

kriskowal Feb 22, 2024

rekmarks Feb 22, 2024 •

edited

Loading

kriskowal Feb 22, 2024

kriskowal Feb 22, 2024

rekmarks Feb 22, 2024

rekmarks commented Feb 22, 2024

rekmarks commented Feb 22, 2024 •

edited

Loading

kriskowal Feb 22, 2024

kriskowal Feb 22, 2024

refactor(daemon): Synchronize host evaluate method #2092

refactor(daemon): Synchronize host evaluate method #2092

Conversation

rekmarks commented Feb 21, 2024 • edited Loading

rekmarks Feb 22, 2024

Choose a reason for hiding this comment

kriskowal left a comment

Choose a reason for hiding this comment

kriskowal Feb 22, 2024

Choose a reason for hiding this comment

rekmarks Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

kriskowal Feb 22, 2024

Choose a reason for hiding this comment

kriskowal Feb 22, 2024

Choose a reason for hiding this comment

rekmarks Feb 22, 2024

Choose a reason for hiding this comment

rekmarks commented Feb 22, 2024

rekmarks commented Feb 22, 2024 • edited Loading

kriskowal Feb 22, 2024

Choose a reason for hiding this comment

kriskowal Feb 22, 2024

Choose a reason for hiding this comment

refactor(daemon): Synchronize host `evaluate` method #2092

refactor(daemon): Synchronize host `evaluate` method #2092

rekmarks commented Feb 21, 2024 •

edited

Loading

rekmarks Feb 22, 2024 •

edited

Loading

rekmarks commented Feb 22, 2024 •

edited

Loading