[nexus] fix busy loop due to race condition in SP ereport ingestion#10746
Open
hawkw wants to merge 5 commits into
Open
[nexus] fix busy loop due to race condition in SP ereport ingestion#10746hawkw wants to merge 5 commits into
hawkw wants to merge 5 commits into
Conversation
95f21d9 to
7ee866c
Compare
There was a problem hiding this comment.
Pull request overview
Fixes a race in SP ereport ingestion where “latest” progress could regress due to wall-clock ordering, causing sp_ereport_ingester to busy-loop. The fix makes ingestion progress track the latest ENA for the restart ID that was actually inserted, and adds a regression test reproducing the stale-restart scenario from #10738.
Changes:
- Update
DataStore::ereports_insertto return the latest ENA for the insertedrestart_id(rather than selecting “latest” by wall-clock across restarts). - Add a regression test that constructs the stale-restart insertion ordering and asserts the ingestion loop terminates.
- Refactor test helper to return fetched ereports for reuse in new assertions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| nexus/src/app/background/tasks/ereport_ingester.rs | Adds regression test for the stale restart-ID busy-loop scenario and refactors ereport-fetch helper. |
| nexus/db-queries/src/db/datastore/ereport.rs | Changes post-insert “latest” computation to be restart-scoped (latest ENA for the inserted restart). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #10738 describes a bug @smklein noticed in the
sp_ereport_ingesterbackground task. Currently theDataStore::ereports_insertmethod returns theEreportId(ENA +restart ID tuple) for the "latest" ereport from the reporter that
produced the inserted batch of ereports, which is determined based on
the wall clock time at which the ereports were received. The returned
EreportIdis then used as the starting ENA and expected restart ID fora subsequent request to the SP. Because the latest ereport ID is
determined using wall-clock time, it is possible for a situation to
occur where another Nexus is holding in memory a batch of ereports
received from a previous restart of that SP, is delayed in inserting
them, and then inserts those ereports at a later wall clock time than
when a batch of ereports from the actual current restart is inserted. If
this occurs, the previous restart will appear to be "latest", rather
than the current one, which can cause the Nexus which just inserted a
batch of ereports from the current restart to busy-loop when it
requests ereports with the old restart ID, and receives the same batch
of ereports it already inserted on its previous iteration. Since no new
ereports are inserted when the same batch is received again, nothing
updates the "latest" ereport from that SP, and the unfortunate Nexus
keeps spinning until the SP resets again and unsticks it. This is quite
unfortunate, although this case should be unlikely in practice.
In commit 945a5ad, I've added a test
that reproduces this bug artificially, and makes a call to
DataStore::ereports_insertloop forever. This test fails onmain.Commit 7ee866c fixes the bug by
changing the
ereports_insertmethod to instead return anEreportIdwith the latest ENA from the restart ID of the ereports that were
inserted. That way, subsequent ingestion loop iterations no longer rely
on wall-clock time to determine what restart ID to send. If the SP has
restarted, Nexus will still ask it for the restart ID it returned in
response to a previous request, but this just causes Nexus to update the
restart ID when it inserts any ereports received from the new restart.
Wall clock time is still used to determine the initial "latest"
ereport ID to request at the top of the loop, but that's fine now, since
if it's not the current restart, we will update our understanding of the
current restart as soon as we've received a response from the SP.
Essentially, this approach changes the ingestion loop from treating the
database as the source of truth for what the current restart ID is, to
using the SP itself as the source of truth...which is how this
protocol was always designed to work from the beginning, and how I
described it working in RFD 520. I just messed it up while actually
implementing it.
Fixes #10738.