[CI test only] non-blocking event delivery + artificial delay #6793
Conversation
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
This ability was only used during tests; in actual production, there is always a DB. Not having to support the DB-less use is going to make it a lot cleaner to use the DB as a queue (see stacks-network#6543). This commit also makes the method names and order of operations a little less confusing -- we had `send_payload()`, `send_payload_directly()`, `send_payload_given_db_path()` (ok that one was my fault), and `send_payload_with_bytes()`. Now we have this instead: ``` dispatch_to_observer() -> get_payload_bytes() -> save_to_db() -> make_http_request_and_delete_from_db() -> make_http_request() -> delete_from_db() ```
... instead of using unnamed tuples and long parameter lists.
This will allow us to output warnings if the (non-blocking) delivery gets too far behind, because we can tell how long it took between enqueuing the event and actually sending it. This commit adds another migration to said database, so I slightly refactored the migration code.
This commit is the main implementation work for stacks-network#6543. It moves event dispatcher HTTP requests to a separate thread. That way, a slow event observer doesn't block the node from continuing its work. Only if your event observers are so slow that the node is continuously producing events faster than they can be delivered, will it eventually start blocking again, because the queue size for pending requests is bounded (at 1,000 right now, but I picked that number out of a hat, happy to change it if anyone has thoughts). Each new event payload is stored in the event observer DB, and its ID is then sent to the subthread, which will make the request and then delete the DB entry. That way, if a node is shut down while there are pending requests, they're in the DB ready to be retried after restart via `process_pending_payloads()` (which blocks until completion). So that's exactly as before (except that previously there couldn't have been more than one or two pending payloads).
This fixes [this integration test failure](https://github.com/stacks-network/stacks-core/actions/runs/20749024845/job/59577684952?pr=6762), caused by the fact that event delivery wasn't complete by the time the assertions were made.
Doing this work in the RunLoop implementations' startup code is *almost* the same thing, but not quite, since the nakamoto run loop might be started later (after an epoch 3 transition), at which point the event DB may already have new items from the current run of the application, which should *not* be touched by `process_pending_payloads`. This used to not be a problem, but now that that DB is used for the actual queue of the (concurrently running) EventDispatcherWorker, it has become one.
This is like 72437b2, but it works for all the tests instead of only the one. While only that one test very obviously failed, the issue exists for pretty much all of the integration tests, because they rely on the test_observer to capture all relevant data. Things are fast enough, and therefore we've only seen one blatant failure, but 1) it's going to be flaky (I can create a whole lot of test failures by adding a small artificial delay to event delivery), and 2) it might actually be *hiding* test failures (in some cases, like e.g. neon_integrations::deep_contract, we're asserting that certain things are *not* in the data, and if the data is incomplete to begin with, those assertions are moot).
When switching runloops at the epoch 2/3 transition, this ensures that the same event dispatcher worker thread is handling delivery, which in turn ensures that all payloads are delivered in order
This is just to check if CI passes. If it does, we can reasonably assume that d5fa2fc was all that's necessary.
benjamin-stacks
changed the title
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (73.47%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6793 +/- ##
===========================================
- Coverage 78.40% 73.47% -4.94%
===========================================
Files 585 586 +1
Lines 361384 362297 +913
===========================================
- Hits 283360 266197 -17163
- Misses 78024 96100 +18076
... and 259 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Lots of tests failing with state machine update timeouts. It's expected that the tests take longer now, that's not what we're worried about.
these tests fire a *lot* of events
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Not meant to be merged, just testing 3cd2e25 on top of #6762 in CI.