fix(proto): Avoid double-emitting Draining event, causing an active_connections underflow#748
Merged
Merged
Conversation
Member
Author
|
Closes #743 |
active_connections underflow
matheus23
force-pushed
the
matheus23/stateless-reset-and-draining
branch
from
July 13, 2026 20:39
310e980 to
78ccde6
Compare
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5397.4 Mbps | 7768.1 Mbps | -30.5% | 94.5% / 100.0% |
| medium-concurrent | 5378.2 Mbps | 7751.0 Mbps | -30.6% | 94.3% / 99.0% |
| medium-single | 3799.6 Mbps | 4749.8 Mbps | -20.0% | 97.6% / 151.0% |
| small-concurrent | 3719.9 Mbps | 5268.1 Mbps | -29.4% | 92.5% / 103.0% |
| small-single | 3352.3 Mbps | 4889.1 Mbps | -31.4% | 92.7% / 104.0% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3038.7 Mbps | 3987.4 Mbps | -23.8% |
| lan | 782.4 Mbps | 810.3 Mbps | -3.4% |
| lossy | 69.8 Mbps | 55.9 Mbps | +25.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 27.5% slower on average
532663116bb9371290a225eeefa2a797238c24d2 - artifacts
No results available
fcd420c02e03cd0bc620f4345a590f4a8b8ae88a - artifacts
Raw Benchmarks (localhost)
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5342.4 Mbps | 7979.3 Mbps | -33.0% | 93.2% / 99.0% |
| medium-concurrent | 5389.0 Mbps | 7881.6 Mbps | -31.6% | 94.4% / 100.0% |
| medium-single | 3818.5 Mbps | 4749.3 Mbps | -19.6% | 94.0% / 101.0% |
| small-concurrent | 3834.2 Mbps | 5185.7 Mbps | -26.1% | 100.6% / 154.0% |
| small-single | 3475.6 Mbps | 4785.5 Mbps | -27.4% | 89.8% / 97.7% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 2933.3 Mbps | 3920.6 Mbps | -25.2% |
| lan | 782.4 Mbps | 805.8 Mbps | -2.9% |
| lossy | 69.8 Mbps | 69.8 Mbps | ~0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 27.4% slower on average
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/748/docs/noq/ Last updated: 2026-07-16T12:10:47Z |
flub
reviewed
Jul 14, 2026
| }) => { | ||
| let was_draining = self.state.move_to_drained(Some(conn_err)); | ||
| if !was_draining { | ||
| if !was_draining && !was_drained { |
Collaborator
There was a problem hiding this comment.
This fix seems correct as far as I can tell. But oh, all this state-transition code makes me so unhappy. It is so brittle. I wish we would do a more thorough refactor.
move_to_drainedcan panic. I hate panics by now, why not make it a no-op if it was already drained?move_to_drainedknows what the previous state was. Therefore it knows what events need emitting. The fact that we have to collect a bunch of bools and carefully read them in exactly the right way is infuriating. It should just give us the events that need to be emitted, or emit them itself or something.move_to_drainedis called from other places which do similar brittle things. So doing the above or something similar that forces callers to do the right thing is much safer.
This is what comes to mind. But of course I haven't tried any of this.
Member
Author
There was a problem hiding this comment.
Agreed. I think the history would be cleaner if we merged this regression test + fix combination, and I'll follow up with a refactor PR, if that ends up being feasible/useful. How does that sound?
2 tasks
flub
approved these changes
Jul 16, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 16, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 16, 2026
Collaborator
|
github wobbly in the queue |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 16, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 16, 2026
Member
Author
|
merge queue is failing because of cfg_aliases. That crate needs an update: katharostech/cfg_aliases#15 I'll ignore the lint, but follow for updates on that PR so that I can remove the ignored lint once that PR is merged. |
matheus23
enabled auto-merge
July 16, 2026 12:02
matheus23
force-pushed
the
matheus23/stateless-reset-and-draining
branch
from
July 16, 2026 12:08
5326631 to
fcd420c
Compare
ifdario
added a commit
to rayfish/net-tools
that referenced
this pull request
Jul 16, 2026
New nightly cargo lints fire inside the cfg_aliases macro; allow it until katharostech/cfg_aliases#15 lands, same as n0-computer/noq#748.
Stanley00
pushed a commit
to stanley-fork/noq
that referenced
this pull request
Jul 16, 2026
…llers (n0-computer#753) ## Description Follow-up from n0-computer#748 - Refactors how `Draining` and `Drained` endpoint events are emitted: They're now handled within state.rs. - `move_to_draining` and `move_to_drained` are now passed `&mut self.endpoint_events`. This is a pure refactor without behavioral changes. ## Breaking Changes None ## Change checklist - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.
6 tasks
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
2 participants
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.
Description
Drainingevent was emitted twice when a stateless reset token was duplicated and received twiceThis is one of those bugs that triggers an underflow in
active_connectionsin noq.Breaking Changes
None
Notes & open questions
I'm still investigating whether there are further such cases.
Change checklist