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

cli,daemon: endo cancel does not propagate properly under some circumstances #2074

Open
rekmarks opened this issue Feb 14, 2024 · 2 comments
Assignees
Labels
bug Something isn't working daemon Issues pertaining the the pet dæmon 🐈‍⬛

Comments

@rekmarks
Copy link
Contributor

rekmarks commented Feb 14, 2024

Describe the bug

As of the current endo feature branch (#2073), endo cancel does not propagate as expected under some circumstances. A common factor seems to be that neither direct nor indirect cancellation works for values running "in" guests. See reproduction steps below.

We may not want to bother solving this until we are threading cancellation through messages and, perhaps, until messages are persisted across restarts.

Steps to reproduce & expected behavior

This transcript uses the diff format. The removed (- / red) lines are the expected behavior, and the added (+ / green) lines are the observed behavior.

endo purge -f

endo make counter.js --name counter
Object [Alleged: Counter] {}

endo mkguest doubler-agent
Object [Alleged: EndoGuest] {}

endo make doubler.js -n doubler -p doubler-agent
Object [Alleged: Doubler] {}

endo resolve 0 counter

endo eval 'E(doubler).incr()' doubler
2

endo eval 'E(doubler).incr()' doubler
4

endo eval 'E(doubler).incr()' doubler
6

// This cancels the counter, which should also cancel the doubler
endo cancel counter

endo eval 'E(doubler).incr()' doubler
- 2
+ 8

endo eval 'E(counter).incr()' counter
- 2
+ 1

endo eval 'E(counter).incr()' counter
- 3
+ 2

endo eval 'E(doubler).incr()' doubler
- 8
+ 10

// This should, naturally, cause the doubler to be cancelled
endo cancel doubler

endo eval 'E(doubler).incr()' doubler
- 10
+ 12

endo cancel doubler-agent

// Here the doubler is finally cancelled.
// The return value is correct given the incorrect state of the counter
endo eval 'E(doubler).incr()' doubler
- 12
+ 6
@rekmarks rekmarks added the bug Something isn't working label Feb 14, 2024
@rekmarks rekmarks self-assigned this Feb 14, 2024
@rekmarks rekmarks changed the title endo cancel does not propagate properly under some circumstances cli / daemon: endo cancel does not propagate properly under some circumstances Feb 14, 2024
@rekmarks
Copy link
Contributor Author

rekmarks commented Feb 16, 2024

This issue previously referenced a second case, which "may or may not be distinct". As it turns out, it is both distinct and known, and is now tracked in #2021.

rekmarks added a commit that referenced this issue Feb 22, 2024
`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.
rekmarks added a commit that referenced this issue Feb 22, 2024
`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.
@rekmarks rekmarks changed the title cli / daemon: endo cancel does not propagate properly under some circumstances cli,daemon: endo cancel does not propagate properly under some circumstances Feb 22, 2024
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 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
@rekmarks
Copy link
Contributor Author

rekmarks commented Apr 23, 2024

We observe datalocks about half the time during crossed hellos in #2217, likely on the side that has to drop their existing connection and therefore cancel all dependent formulas. We are likely in one of two situations:

  1. This is a blocker for solving crossed hellos.
  2. There is some other cancellation issue confounding #2217's solution to crossed hellos.

@grypez grypez mentioned this issue Jul 18, 2024
9 tasks
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

No branches or pull requests

2 participants