Skip to content

feat(bidengine): support wildcard gpu multi-bids#415

Open
chalabi2 wants to merge 3 commits into
mainfrom
chalabi/provider-multibid
Open

feat(bidengine): support wildcard gpu multi-bids#415
chalabi2 wants to merge 3 commits into
mainfrom
chalabi/provider-multibid

Conversation

@chalabi2

Copy link
Copy Markdown
Contributor

Summary

Adds provider multi-bid support for wildcard GPU model orders.

The bid engine now increments BSeq, reserves per bid, submits per-bid resource offers, and frees losing reservations when one local bid wins.

Tests

  • GOWORK=off go test ./...

Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
@chalabi2 chalabi2 requested a review from a team as a code owner June 24, 2026 17:32
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d23c2dbf-c14e-44c3-bcf3-3d2a084f6e04

📥 Commits

Reviewing files that changed from the base of the PR and between d6af28a and 87041f8.

📒 Files selected for processing (1)
  • Dockerfile.debug
✅ Files skipped from review due to trivial changes (1)
  • Dockerfile.debug

Walkthrough

Cluster reservations now use BidID-based lookup and unreserve paths, and bidengine/order.go now manages sequential bid attempts with bid-aware reservation, pricing, broadcast, and cleanup. Interfaces, mocks, tests, and inventory confirmation logic were updated to match.

Changes

Bid-keyed reservations and multi-attempt bidding

Layer / File(s) Summary
Reservation struct and inventory request/response rekeyed to BidID
cluster/reservation.go, cluster/inventory.go
newReservation accepts BidID; reservation stores bid; OrderID() derives from bid. inventoryRequest gains bid/exactBid fields; inventoryResponse gains removed counter; initial reservation construction uses BidID.
Inventory bid-aware reserve/unreserve and lease matching
cluster/inventory.go, cluster/inventory_test.go
Adds lookupBid, reserveBid, unreserveBid; reserve(order) delegates to reserveBid(bidIDFromOrder(order)); run loop branches on exactBid for lookup and unreserve; reservationCount decrements by response.removed; reservationMatchesLease replaces order-only matching; IP confirmation uses BidID.
Cluster interface ReserveBid/UnreserveBid and service wiring
cluster/service.go, mocks/cluster/Cluster_mock.go, mocks/cluster/Service_mock.go
Cluster interface adds ReserveBid and UnreserveBid. service implementation delegates to inventory. Manifest handling switches inventory lookup from OrderID to BidID. Generated mocks add ReserveBid and UnreserveBid coverage on Cluster and Service.
bidengine multi-attempt types, helpers, and run initialization
bidengine/order.go
Adds bidAttempt, reservationResult, priceResult, bidIDWithSequence, groupHasGPUModelWildcard, findBidAttempt, placedBidCount. Rewrites order.run to initialize multi-attempt state and control reservation sequencing. Existing-bid query fetches all open bids sorted by BSeq.
bidengine event handling, bid lifecycle, and shutdown cleanup
bidengine/order.go
EventLeaseCreated stores wonBid instead of a boolean. EventBidClosed locates and unreserves the matching attempt. Fulfillment creates or updates attempts; pricing and broadcast failures unreserve and remove attempts. Post-loop drains channels, unreserves non-winning attempts, closes placed bids when no winner, and waits for queryBidsCh and shouldBidCh.
Order test scaffold and bid-based reservation assertions
bidengine/order_test.go
orderTestScaffold gains modifyGroup, reserveBid, and pass hooks; makeOrderForTest accepts functional options; mocks wire ReserveBid/UnreserveBid; existing-bid lookup switches to Bids/QueryBidsRequest; bid ID and GPU model helpers are added; reserve and unreserve assertions switch to bid IDs.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 Hop-hop, the bids now line up neat,
BidIDs and attempts keep the rhythm sweet.
Wildcard carrots bring a few more tries,
Then unreserve the rest beneath moonlit skies.
One lease may win, the others drift away—
A tidy bidengine day!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: bidengine support for wildcard GPU multi-bids.
Description check ✅ Passed The description is directly related and summarizes the multi-bid reservation and offer workflow changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chalabi/provider-multibid

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@bidengine/order_test.go`:
- Around line 249-271: The existing-bid mock in the QueryBids test setup is
inconsistent with the BidOpen filter, since it returns a BidLost result even
though the request only asks for open bids. Update the mock around
QueryBidsRequest/QueryBidsResponse and scaffold.marketMocks.On("Bids", ...) so
it returns a matching open bid only, or an empty response for non-open states,
and make the same adjustment in the duplicated test setup referenced by the same
QueryBidsRequest pattern.

In `@bidengine/order.go`:
- Around line 288-289: The early-exit paths in bidengine/order.go are leaving
the transferred runner in storedGroupCh unobserved after checkForExistingBid
moves groupch there. Update the shutdown/query-error/stale-bid exits in the
order flow around storedGroupCh and the main bid runner logic to explicitly
drain or consume storedGroupCh before returning, so the runner is always
observed even when the final drain path is skipped.
- Around line 480-487: The bid-close handling in the EventBidClosed path
currently removes an attempt, but the later ReserveBid completion can still
re-add that bid and resurrect a closed order. Update the attempt lifecycle
around findBidAttempt/removeAttempt and the reservation callback so closed bid
IDs are tracked as cancelled, and when a late clusterch result arrives for one
of those IDs, immediately unreserve and drop it instead of appending it back
with placed: reservationResult.existing. Apply the same cancelled-ID check in
the related reservation-result handling around the later attempt-processing
block.
- Around line 431-435: The lease-lost cleanup in bidengine/order.go is marking
broadcastingBid as placed too early, before the broadcast path has actually
succeeded. Update the handling around broadcastingBid in the affected
cleanup/broadcast flow (including the related bidch drain and the other
lease-lost paths) so attempts[idx].placed is only set after the broadcast
returns success, not while the bid is still in flight. Keep the logic consistent
across the related sections by using the existing findBidAttempt,
broadcastingBid, and attempts state transitions to avoid sending MsgCloseBid for
bids that were never created.

In `@cluster/inventory.go`:
- Around line 795-800: The IP confirmation loop is matching reservations by the
first `OrderID` hit, which can mark the wrong reservation when multiple bids
share an order; update the logic in the
`res.confirmedResult`/`state.reservations` iteration to identify the winning
reservation by `BidID` (or equivalent bid sequence metadata) instead of
`entry.OrderID()`, then set `ipsConfirmed` on that exact reservation and keep
the log aligned with the matched bid.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 40e88500-caa4-447c-9a67-4b47bf28ab40

📥 Commits

Reviewing files that changed from the base of the PR and between 0e0fd8a and cdd01dc.

📒 Files selected for processing (7)
  • bidengine/order.go
  • bidengine/order_test.go
  • cluster/inventory.go
  • cluster/reservation.go
  • cluster/service.go
  • mocks/cluster/Cluster_mock.go
  • mocks/cluster/Service_mock.go

Comment thread bidengine/order_test.go
Comment thread bidengine/order.go
Comment thread bidengine/order.go Outdated
Comment thread bidengine/order.go
Comment thread cluster/inventory.go Outdated
Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
@chalabi2

chalabi2 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

addressed the review comments:

  • removed the redundant embedded Resources selector in wildcard GPU detection
  • drained storedGroupCh on early exits
  • only mark bids placed after broadcast success
  • prevent late reservation results from resurrecting externally closed bids
  • confirm leased IP reservations by exact BidID
  • fixed existing-bid tests to match the open-bid query filter

Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant