feat: catch all exceptions in auction wakers

[Chris-Hibbert]

closes: #8307

Description

#8296 demonstrated an issue in the Timer wakers used by the Auctioneer. #8301 addressed the particular problem, but not the general case. This handles the general case for all wakers in the Auctioneer.

Security Considerations

This is about robustness rather than security.

Scaling Considerations

No impact.

Documentation Considerations

N/A

Testing Considerations

Existing tests pass. No new tests added, as inducing appropriate errors wasn't cost effective in code that has already been fixed for all the cases we know of.

There is a test that serves as a regression test for the known case. If the fix from #8301 were lost, this new try-catch would prevent the damage caused by the situation in that test.

Upgrade Considerations

This doesn't make it any more or less urgent to get upgrades to the chain.

[turadg]

For solving the "general case" of timer exceptions, this covers Auctioneer but doesn't help other contracts that will run into this.

I think we should consider a helper that makes requires a handler for wake exceptions. E.g.

    uponWakeup(
      timer,
      start,
      time => {
        setTimeMonotonically(time);
        auctionDriver.capturePrices();
        // eslint-disable-next-line no-use-before-define
        return startAuction();
      },
      e => console.error(`🚨 Auction threw ${e}. Caught in SchedulerWaker.`),
    );

If a handler (fourth position) isn't provided there'd be a type error.

[turadg]

this is a new emoji to log. what does it mean differently than 🚨 and ⚠️ ?

Also shouldn't this log as an error?

[Chris-Hibbert]

Those are both warnings that something is wrong and danger may persist. This was an attempt to say that the emergency has been handled.

[turadg]

lol I get it now.

Is it a full recovery? No further investigation needed?

[Chris-Hibbert]

Investigation would be warranted. I'd like to know that something happened and figure out why, but I don't expect the chain to be in imminent danger.

[turadg]

Then it sounds like the same attention needed as

agoric-sdk/packages/inter-protocol/src/vaultFactory/vaultManager.js

Lines 749 to 753 in 3356675

	console.error(
	`⚠️ Excess collateral remaining sent to reserve. Expected ${q(
	plan.collatRemaining,
	)}, sent ${q(collateralInLiqSeat)}`,
	);

I think it needs to a) go to the error logs (so console.error) and b) indicate that investigation is warranted. ⚠️ is what we've been using for that.

I see how 🧯 might be a better pictograph for that because it suggests an exception was handled, but a counter-argument is that every exception that doesn't halt the vat is handled so it's not an informative distinction.

[Chris-Hibbert]

done

[Chris-Hibbert]

I think we should consider a helper that makes requires a handler for wake exceptions. E.g.

If we think that should always be required, then we should lobby for a change to the timerService. Not rescheduling when a waker throws was a design choice. If that's sometimes the right behavior, then each caller of setWakeup or repeatAfter should make a conscious decision about whether to unconditionally catch.

[Chris-Hibbert]

Rebased to master, and added one tiny commit.

[github-actions]

@Chris-Hibbert - This PR appears to be stuck. It's had a merge label for > 24 hours

[turadg]

@Mergifyio requeue

[mergify]

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically