-
Notifications
You must be signed in to change notification settings - Fork 206
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
9303 orchestrate upgrade #9719
base: master
Are you sure you want to change the base?
9303 orchestrate upgrade #9719
Conversation
7e2a619
to
4c8a8ee
Compare
4c8a8ee
to
77018cf
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9719 +/- ##
==========================================
- Coverage 56.37% 56.01% -0.37%
==========================================
Files 667 666 -1
Lines 207560 207135 -425
Branches 343 171 -172
==========================================
- Hits 117022 116017 -1005
- Misses 90420 90999 +579
- Partials 118 119 +1 |
refs: #9303 ## Description Tests restarting an orchestration flow while an IBC message is pending response. #9303 will be closed when this and #9719 have landed. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations per se ### Upgrade Considerations helps, no change
77018cf
to
a3b92fa
Compare
Experimenting with these merged: |
// BUG: this never resolves | ||
return new Promise(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment of what exactly will hang in the test? I tracked down that chainHub
uses agoricNames
with retriable
, so I assume something in the test will trigger usage of chainHub and its retriable vow to not be resolved before upgrade, but I lost track of how exactly that happens.
Also we should provide a little more details as to why this is a valid test (and equivalent to the previous one, which had the pending promise explicitly created in the vat being upgraded). Here the buggy agoricNames
is external to the vat being upgraded, which means upgrading the vat will not reject the result promise for the E(agoricNames).lookup()
call. However that call is simply awaited in a retriable
async function, which means there is a local promise created and watched (the result of the async function), which is pending at vat upgrade, and which will be rejected on upgrade.
Interestingly, the lookup result promise will remain pending forever, causing a "leak" of the promise subscription (in the real world until the decider of the promise upgrades) even though the subscriber is actually no longer interested in the promise. This is a liveslots "limitation" that I created an issue about (#10101)
509278f
to
11879f7
Compare
|
||
// UNTIL https://github.com/Agoric/agoric-sdk/issues/9066 | ||
const logNode = E(privateArgs.storageNode).makeChildNode('log'); | ||
/** @type {(msg: string) => Vow<void>} */ | ||
const log = msg => vowTools.watch(E(logNode).setValue(msg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's expected that the logs of all invocations will write to the same node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Until #9066, yes.
refs: #9303
Description
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations