feat(notifier): subscribeEach/subscribeLatest iterators retry when broken by vat upgrade

[gibson042]

Fixes #5185

Description

"latest" iterators look for rejections that indicate failure due to vat upgrade, and upon encountering them retry the getUpdateSince call—if the source is durable, the retry will succeed and provide the next iteration result. If the source is not durable, the retry will go splat and the error associated with that will be propagated in place of the vatUpgraded rejection (which is perhaps unfortunate, but seems to be the only way of guaranteeing that a real fail result with unfortunate timing is not replaced by a vatUpgraded rejection).

"each" iterators behave similarly, but additionally fail if multiple values are published before they can successfully reconnect because they cannot tolerate gaps in the sequence.

Security Considerations

A malicious source can fake vatUpgraded rejections, which the "latest" iterators will dutifully accept and respond to by retrying as long as incarnationNumber indicates forward progress. Something like this is necessary to handle the edge case where a second upgrade occurs in between the consumer receiving a rejection and the producer receiving the retry, but we could put in a retry count limit if this is perceived as an actual risk.

Scaling Considerations

I don't think this affects scaling.

Documentation Considerations

These relatively fine implementation details are covered by JSDoc comments.

Testing Considerations

The rapid-upgrade edge case is not covered by testing, but I consider that acceptable. If reviewers disagree, I think I can add a new file with manually-constructed mock producers.

[mhofman]

subscribeEach iterators continue to fail in that scenario, because they cannot guarantee absence of gaps.

That makes sense. But I'm somewhat worried about what that means for these consumers, especially since it seems they may be using subscibeEach through layers of utils without knowing. That said I don't see the alternative.

seems to be the only way of guaranteeing that a real fail result with unfortunate timing is not replaced by a vatUpgraded rejection).

Could we annotate the rejection with the upgraded rejection?

[gibson042]

Updated to opportunistically reconnect subscribeEach iterators when possible per #5185 (comment) .

[gibson042]

This PR introduces a locally-reproducible failure for the "Watch interest accrue" test in test-stakeFactory.js that might be real—d.waitDays(90) is followed by d.checkRUNDebt(195n), which compares E(vault).getCurrentDebt() against 195e6 and comes up just outside the acceptable tolerance of 0.2e6:

    name: AssertionError
    assertion: deepEqual
    values:
      'Difference (- actual, + expected):': |2-
          {
            brand: Object @Alleged: IST brand {},
        -   value: 194796320n,
        +   value: 195000000n,
          }
    at: >-
      approxEqual
      (packages/inter-protocol/test/stakeFactory/test-stakeFactory.js:507:7)

      Object.checkRUNDebt
      (packages/inter-protocol/test/stakeFactory/test-stakeFactory.js:742:7)

      async packages/inter-protocol/test/stakeFactory/test-stakeFactory.js:991:3

I'm not sure how E(vault).getCurrentDebt() depends upon subscribeEach or subscribeLatest, but I can confirm that the reported value in that test is affected by the count of awaits they entail (e.g., removing the await null in reconnectAsNeeded would reduce that 194.796320e6 to 194.730620e6). The situation reads to me like sensitivity to artificial testing details and can be "fixed" by increasing the value of epsilon from micro.unit / 5n to e.g. micro.unit / 4n, but I'd like confirmation from @dckc and/or @turadg that such a change would maintain the intent of the stakeFactory tests (which were added in entirety by #4741), and also if possible a suggested explanation of what epsilon values are appropriate in case further changes are warranted in the future.

[turadg]

@gibson042 thanks for the detailed question. @dckc and I conferred and agree with your conclusion. For appropriate epsilon value, go with whatever works because StakeFactory is not scheduled for any release.

[michaelfig]

Looking good! Just a few comments that shouldn't block approval.

[michaelfig]

We do have @endo/patterns now.

[gibson042]

My first attempt didn't quite work, so I'm deferring this until we've got a little more time to spend on it.

[michaelfig]

I'm uncertain about why publishCountP is sunk, but tailP (originally pubList at this point) is not. Can you elaborate (either by replying here, or in a code comment) as to why these two calls are necessary?

I suspect that there may be a clearer way to avoid introducing any unhandled rejections with minimal sinks. I just don't see it right now.

[gibson042]

Any rejection of tailP would be handled in reconnectAsNeeded.

[michaelfig]

This is really clear!