Skip to content

Conversation

troian
Copy link
Member

@troian troian commented Oct 2, 2025

No description provided.

@troian troian requested a review from boz as a code owner October 2, 2025 14:29
Copy link

coderabbitai bot commented Oct 2, 2025

Walkthrough

The background ordersFetcher task in bidengine/service.go is disabled by commenting out its initialization, with a note to re-enable post SDK53 upgrade. The CI workflow tests.yaml changes runner from ubuntu-latest to core-e2e and adds a preliminary build folder cleanup step.

Changes

Cohort / File(s) Summary of Changes
Bid Engine service control flow
bidengine/service.go
Commented out the ordersFetcher background task startup (previously launched via group.Go), leaving a note to enable after SDK53 upgrade. No other logic altered.
CI workflow adjustments
.github/workflows/tests.yaml
Changed build-bins job runner to core-e2e and added an initial cleanup step (rm -rf on build folders) before existing steps.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant C as Caller
  participant S as Service
  participant OF as OrdersFetcher (bg task)

  Note over S: Previous behavior (pre-change)
  C->>S: Start()
  activate S
  S->>OF: Spawn via group.Go
  activate OF
  OF-->>S: Running in background
  S-->>C: Start returns
  deactivate S
Loading
sequenceDiagram
  autonumber
  participant C as Caller
  participant S as Service
  participant OF as OrdersFetcher (bg task) 

  Note over S: Current behavior (ordersFetcher disabled pending SDK53)
  C->>S: Start()
  activate S
  Note right of S: ordersFetcher init block is commented out
  S-->>C: Start returns (no bg task)
  deactivate S
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I paused my hops, the fetcher sleeps,
A queue of dreams the engine keeps.
CI sweeps the build path clean,
New runner hums, a quiet machine.
When SDK53 drums begin,
I’ll thump the ground—and spin it in. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request lacks any description, providing no context or summary of the changes to the order fetcher in service.go or the CI workflow modifications. Please add a detailed pull request description that outlines the primary changes, such as disabling the order fetcher and updates to the test workflow.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title “chore(bidengine): disable order fetcher” accurately and concisely highlights the primary change in the changeset by indicating that the order fetcher background task is being disabled in the bidengine component.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch disable-order-fetch

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 59065a0 and e7f22dd.

📒 Files selected for processing (2)
  • .github/workflows/tests.yaml (1 hunks)
  • bidengine/service.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • bidengine/service.go
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/tests.yaml

19-19: label "core-e2e" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2022", "windows-2019", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "ubuntu-20.04", "macos-latest", "macos-latest-xl", "macos-latest-xlarge", "macos-latest-large", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xl", "macos-14-xlarge", "macos-14-large", "macos-14", "macos-13-xl", "macos-13-xlarge", "macos-13-large", "macos-13", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file

(runner-label)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: integration-tests / crd-e2e
  • GitHub Check: lint
  • GitHub Check: tests
  • GitHub Check: release-dry-run
🔇 Additional comments (2)
.github/workflows/tests.yaml (2)

19-20: Runner change aligns with existing core-e2e usage.

Thanks for moving build-bins onto the same core-e2e fleet we already depend on for release-dry-run; this keeps build parity tight.


21-24: Workspace cleanup mirrors release workflow hygiene.

Duplicating the proven cleanup used in release-dry-run should prevent stray artifacts from poisoning builds on the self-hosted box. Looks solid.


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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 39b143d and 59065a0.

📒 Files selected for processing (1)
  • bidengine/service.go (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: integration-tests / crd-e2e
  • GitHub Check: lint
  • GitHub Check: build-bins
  • GitHub Check: release-dry-run
  • GitHub Check: tests

Comment on lines +131 to +134
// to enable after SDK53 upgrade
//group.Go(func() error {
// return s.ordersFetcher(ctx, aqc)
//})
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing catch-up path after disabling fetcher

With group.Go commented out, ordersFetcher never runs, so ordersch is never populated. After any restart the service will fail to rehydrate existing open orders and will only see new pubsub events, leaving active orders unmanaged until SDK53 is in place. Please keep a safe catch-up path (feature flag, alternate fetch, or explicit acknowledgment of the operational impact) before merging.

🤖 Prompt for AI Agents
In bidengine/service.go around lines 131-134, commenting out the group.Go call
disables ordersFetcher so ordersch is never populated and the service cannot
rehydrate open orders after restart; restore a safe catch-up path by either (a)
re-enabling ordersFetcher behind a feature flag or config toggle so it can be
turned off for SDK53 rollout, (b) add a one-time startup catch-up routine that
calls the existing fetch logic to populate ordersch before returning to
real-time pubsub processing, or (c) if you must keep it disabled, add explicit
logging and an operational guard (panic or health-check failure) that documents
the impact; implement one of these fixes and ensure it populates ordersch and
handles errors with clear logs.

@troian troian force-pushed the disable-order-fetch branch from 59065a0 to e7f22dd Compare October 2, 2025 14:50
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