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

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

Open
rekmarks opened this issue Jan 30, 2024 · 3 comments · Fixed by #2092
Open

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

rekmarks opened this issue Jan 30, 2024 · 3 comments · Fixed by #2092
Assignees
Labels
bug Something isn't working daemon Issues pertaining the the pet dæmon 🐈‍⬛

Comments

@rekmarks
Copy link
Contributor

Describe the bug

When pet-naming a worker that is returned from an eval formula, you cannot later use that name to specify a --worker to the daemon. Doing so will result in the following error: Cannot deliver "evaluate" to target; typeof target is "undefined".

Steps to reproduce

Using the INFO feature of #1916:

> endo make counter.js --name counter
  Object [Alleged: Counter] {}
> endo eval 'E(INFO).lookup("counter")' INFO --name inspector
  Object [Alleged: Inspector (make-bundle ... )]
> endo eval 'E(inspector).lookup("worker")' inspector --name cworker
  Object [Alleged: EndoWorker] {}
> endo eval 'E(counter).incr()' counter --worker cworker
  CapTP cli exception: (RemoteTypeError(error:captp:Endo#20001)#1)
  RemoteTypeError(error:captp:Endo#20001)#1: Cannot deliver "evaluate" to target; typeof target is "undefined"

Expected behavior

As above except:

...
> endo eval 'E(counter).incr()' counter --worker cworker
  1
@rekmarks rekmarks added the bug Something isn't working label Jan 30, 2024
@kriskowal
Copy link
Member

Design: we should go back to keeping an internal a WeakMap from worker handles to their corresponding bootstrap objects and stop using the internal facet trick. The WeakMap will work for workers even if laundered through an eval formula.

@rekmarks rekmarks changed the title @endo/daemon: eval-mediated worker names do not resolve daemon: eval-mediated worker names do not resolve Feb 17, 2024
@rekmarks
Copy link
Contributor Author

I believe this is another instance of the same problem (originally reported in #2074):

Steps to reproduce

> endo make counter.js --name counter

> endo eval 'E(counter).incr()' counter
1

> endo eval 'E(counter).incr()' counter
2

> endo eval 'foo' foo:INFO.counter.worker -n main-worker
Object [Alleged: EndoWorker] {}

// This should cancel the worker, but actually cancels a lookup formula instead.
// The bug is that this should, but does not, propagate to the worker (and then the counter).
> endo cancel main-worker

> endo eval 'E(counter).incr()' counter
3

Expected Behavior

> endo make counter.js --name counter

> endo eval 'E(counter).incr()' counter
1

> endo eval 'E(counter).incr()' counter
2

> endo eval 'foo' foo:INFO.counter.worker -n main-worker
Object [Alleged: EndoWorker] {}

> endo cancel main-worker

> endo eval 'E(counter).incr()' counter
1

rekmarks added a commit that referenced this issue Feb 22, 2024
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.
rekmarks added a commit that referenced this issue Feb 22, 2024
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.
rekmarks added a commit that referenced this issue Feb 22, 2024
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.
rekmarks added a commit that referenced this issue Feb 22, 2024
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.
rekmarks added a commit that referenced this issue Feb 22, 2024
Fixes: #2021
Progresses: #2086
Ref: #2098

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.
rekmarks added a commit that referenced this issue Mar 8, 2024
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.
rekmarks added a commit that referenced this issue Mar 8, 2024
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.
rekmarks added a commit that referenced this issue Mar 8, 2024
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.
rekmarks added a commit that referenced this issue Mar 8, 2024
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.
rekmarks added a commit that referenced this issue Mar 9, 2024
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.
@rekmarks rekmarks self-assigned this Mar 9, 2024
@rekmarks rekmarks reopened this Mar 9, 2024
@rekmarks
Copy link
Contributor Author

rekmarks commented Mar 9, 2024

We believed that this was fixed by #2092. Rather than fixing the bug, however, that PR concealed it by always creating a new incarnation of eval formula workers, even if they already existed. It's back to the drawing board with this one.

rekmarks added a commit that referenced this issue Mar 9, 2024
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.
@kriskowal kriskowal added the daemon Issues pertaining the the pet dæmon 🐈‍⬛ label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working daemon Issues pertaining the the pet dæmon 🐈‍⬛
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants