Recover bids for unresolved auction on restart #3109

Tristan-Wilson · 2025-04-08T13:03:25Z

The Bid Validator sends validated bids to the Autonomous Auctioneer by means of Redis streams, using our pubsub package.

This commit changes the Auctioneer to only mark bid messages as fully consumed after the auction they were part of is resolved, or if the auction resolution window has closed and can no longer be resolved. This means if the Auctioneer is restarted mid-round before auction resolution, then it will fetch any bids from earlier in the round from redis.

The Bid Validator sends validated bids to the Autonomous Auctioneer by means of Redis streams, using our pubsub package. This commit changes the Auctioneer to only mark bid messages as fully consumed after the auction they were part of is resolved, or if the auction resolution window has closed and can no longer be resolved. This means if the Auctioneer is restarted mid-round before auction resolution, then it will fetch any bids from earlier in the round from redis.

rauljordan · 2025-04-08T18:13:24Z

timeboost/auctioneer.go

+	// Once resolveAuction returns, we acknowledge all bids to remove them from redis.
+	// We remove them unconditionally, since resolveAuction retries until the round ends,
+	// and there is no way to use them after the round ends.
+	defer a.acknowledgeAllBids(ctx, upcomingRound)


Is it fine if this function exits early because of an error and we still ack all bids?

Yes it is fine because resolveAuction is only called once per auction and not retried.

rauljordan · 2025-04-08T18:14:02Z

timeboost/auctioneer.go

+		if uint64(bid.Round) <= round {
+			if err := a.consumer.SetResult(ctx, msgID, nil); err != nil {
+				log.Error("Error acknowledging bid after auction resolution", "msgID", msgID, "error", err)
+				continue


if we continue because of an error here, is it ok if we have an unacked bid that is not cleared in the map?

Fixed it in the latest version.

Tristan-Wilson · 2025-04-10T13:54:25Z

I am reworking this because I had misunderstood how our pubsub abstraction worked.

Problem with my first attempt

(For tests the heartbeat is every 3ms, idle timeout is 30ms.)
msg.Ack() as soon as they are consumed -> this turns off the heartbeat
Messages are put into list to be SetResult-ed at end of round

The problem with this is that the messages will be continuously replayed because they will have timed out (no heartbeat), and the bid cache uses whatever message it got last. Even if there wasn't the following issue with reading the pending messages manually, the auction may end in the middle of replaying the messages and this isn't good.

The behavior of the consumer picking 1 out of 50 of the oldest messages randomly will break chronological ordering of bids. Bids need to be re-inserted into the bid cache in order since newer bids from the same account overwrite older bids.

Other problems

The production default autoclaim time is too short for the rebooted consumer case we're trying to handle -> if it's set to 5 minutes then messages will never be able to be reclaimed in a given 1 minute round

ganeshvanahalli · 2025-04-14T11:47:48Z

I am reworking this because I had misunderstood how our pubsub abstraction worked.

Problem with my first attempt

(For tests the heartbeat is every 3ms, idle timeout is 30ms.) msg.Ack() as soon as they are consumed -> this turns off the heartbeat Messages are put into list to be SetResult-ed at end of round

The problem with this is that the messages will be continuously replayed because they will have timed out (no heartbeat), and the bid cache uses whatever message it got last. Even if there wasn't the following issue with reading the pending messages manually, the auction may end in the middle of replaying the messages and this isn't good.

The behavior of the consumer picking 1 out of 50 of the oldest messages randomly will break chronological ordering of bids. Bids need to be re-inserted into the bid cache in order since newer bids from the same account overwrite older bids.

For this, on the consumer side (i.e auctioneer), why dont we just Ack all-but-last bid from an address? Because anyway newer bids should replace the older ones so theres not point in not ack-ing old bids. This way even if the auctioneer (consumer) restarts- it will try to reclaim bids from the PEL that are already from unique addresses!

One way to do this is bidcache will store ack functions for last bids from a controller-address along with the validated bid and upon receiving another bid from the same address you can then call that ack function from the cache before calling bidcache.add(). This way acknowledgeAllBids function will just be iterating over bidcache and calling the ack functions after successful auction resolution or at round end.

validateBidTemporal is a great idea that and also probably highlights a possible bug where old bids could've been processed in the next round- though very very unlikely!

…r-mid-round-restart-bid-recovery

Tristan-Wilson · 2025-04-16T20:29:44Z

For this, on the consumer side (i.e auctioneer), why dont we just Ack all-but-last bid from an address? Because anyway newer bids should replace the older ones so theres not point in not ack-ing old bids. This way even if the auctioneer (consumer) restarts- it will try to reclaim bids from the PEL that are already from unique addresses!

One way to do this is bidcache will store ack functions for last bids from a controller-address along with the validated bid and upon receiving another bid from the same address you can then call that ack function from the cache before calling bidcache.add(). This way acknowledgeAllBids function will just be iterating over bidcache and calling the ack functions after successful auction resolution or at round end.

It's a good idea but so far I'm unconvinced that it's worth the extra complexity, especially since we're limiting bids per account to 5.

ganeshvanahalli

Great test! Requesting minor changes required for production

pubsub/consumer.go

timeboost/auctioneer_test.go

…-recovery

rauljordan · 2025-04-22T15:23:50Z

timeboost/auctioneer.go

@@ -263,16 +275,48 @@ func (a *AuctioneerServer) Start(ctx_in context.Context) {
 				// There's nothing in the queue.
 				return time.Millisecond * 250
 			}
+
+			if err := validateBidTemporal(&a.roundTimingInfo, (uint64)(req.Value.Round)); err != nil {


It would have been nicer to have the body of this CallIteratively invocation be its own function, which we can more easily unit test. It seems there are several assertions we would want to ensure in this logic, for instance, that we do not send duplicate bids to the receiver because it cares about orderings. We can also check that we reconsumed the same bid, etc. This business logic could be tested by passing in a mock redis interface as well

Moved it to its own function consumeNextBid and added unit tests.

rauljordan · 2025-04-22T15:24:57Z

timeboost/auctioneer.go

@@ -426,6 +475,33 @@ func (a *AuctioneerServer) resolveAuction(ctx context.Context) error {
 	return nil
 }

+func (a *AuctioneerServer) acknowledgeAllBids(ctx context.Context, round uint64) {


This method could also be unit tested to verify that we delete from the unacked bids map for bids that are within the expected round

This is now tested in TestConsumeNextBid_MultipleValidBids

rauljordan · 2025-04-22T15:25:18Z

timeboost/bid_validator.go

+// Check time-related constraints for bid.
+// It's useful to split out to be able to re-check just these contraints after
+// time has elapsed.
+func validateBidTemporal(roundTimingInfo *RoundTimingInfo, bidRound uint64) error {


validateBidTimeConstraints feels more apt here

Thanks that is a better name.

rauljordan · 2025-04-22T15:26:20Z

pubsub/consumer.go

@@ -203,7 +215,7 @@ func (c *Consumer[Request, Response]) Consume(ctx context.Context) (*Message[Req
 					}
 				}
 				return
-			case <-time.After(c.cfg.IdletimeToAutoclaim / 10):
+			case <-time.After(c.cfg.IdletimeToAutoclaim / 2):


Why cut from 10 to 2? Also, what is the rationale behind the divisor at all?

With 10 then there would be 10 heartbeats for each autoclaim period which seems like overkill. I changed it to 2 instead of 1 because with 1 if there is any delay of the heartbeat goroutine then it could miss a heartbeat and cause the message to be reclaimed.

…-recovery

ganeshvanahalli

LGTM

…-recovery

rauljordan

Very nice tests! This PR looks great :)

…-recovery

Tristan-Wilson requested review from rauljordan, gligneul and ganeshvanahalli April 8, 2025 13:03

rauljordan reviewed Apr 8, 2025

View reviewed changes

Change recovery scheme

b7342bc

Tristan-Wilson added 4 commits April 15, 2025 19:00

Add (currently failing) test of auctioneer restart

6d6afe7

StopAndWait needs to stop Consumer too

3a30c74

Add G115 linter comment

babc175

Merge remote-tracking branch 'origin/master' into timeboost-auctionee…

de4cd11

…r-mid-round-restart-bid-recovery

Tristan-Wilson marked this pull request as ready for review April 16, 2025 20:24

eljobe requested a review from rauljordan April 17, 2025 05:26

eljobe assigned rauljordan Apr 17, 2025

ganeshvanahalli requested changes Apr 17, 2025

View reviewed changes

pubsub/consumer.go Show resolved Hide resolved

timeboost/auctioneer_test.go Show resolved Hide resolved

Tristan-Wilson and others added 2 commits April 18, 2025 15:12

Merge branch 'master' into timeboost-auctioneer-mid-round-restart-bid…

40240dd

…-recovery

Add more bids to auctioneer recovery test

4ec8387

Tristan-Wilson requested a review from ganeshvanahalli April 18, 2025 19:47

rauljordan reviewed Apr 22, 2025

View reviewed changes

eljobe and others added 3 commits April 22, 2025 21:25

Merge branch 'master' into timeboost-auctioneer-mid-round-restart-bid…

c9fe02e

…-recovery

Merge branch 'master' into timeboost-auctioneer-mid-round-restart-bid…

d0750f9

…-recovery

Move consumeNextBid to own function

eb8b112

Tristan-Wilson requested a review from rauljordan April 25, 2025 01:55

Tests for auctioneer bid comsumption

a616359

Tristan-Wilson force-pushed the timeboost-auctioneer-mid-round-restart-bid-recovery branch from a7a692f to a616359 Compare April 25, 2025 01:56

ganeshvanahalli approved these changes Apr 25, 2025

View reviewed changes

Merge branch 'master' into timeboost-auctioneer-mid-round-restart-bid…

22039cb

…-recovery

rauljordan approved these changes Apr 25, 2025

View reviewed changes

Tristan-Wilson enabled auto-merge April 25, 2025 16:53

Merge branch 'master' into timeboost-auctioneer-mid-round-restart-bid…

24d40cd

…-recovery

tsahee added the design-approved label May 1, 2025

Merge branch 'master' into timeboost-auctioneer-mid-round-restart-bid…

43e1d60

…-recovery

Tristan-Wilson merged commit b51c4a3 into master May 1, 2025
17 checks passed

Tristan-Wilson deleted the timeboost-auctioneer-mid-round-restart-bid-recovery branch May 1, 2025 00:53

Recover bids for unresolved auction on restart #3109

Recover bids for unresolved auction on restart #3109

Uh oh!

Conversation

Tristan-Wilson commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tristan-Wilson commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem with my first attempt

Other problems

Uh oh!

ganeshvanahalli commented Apr 14, 2025

Problem with my first attempt

Uh oh!

Tristan-Wilson commented Apr 16, 2025

Uh oh!

ganeshvanahalli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ganeshvanahalli left a comment

Choose a reason for hiding this comment

Uh oh!

rauljordan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Tristan-Wilson commented Apr 10, 2025 •

edited

Loading