qt: answer a rate-limited frame instead of dropping it - #261
Merged
Conversation
QtWebSocketServerConfig::messagesPerSecond dropped an over-budget frame silently -- not replied to, not queued, connection left open. From the caller's side that was an `execute` whose Completion simply never resolved, unless Bridge::setExecuteDeadline was armed, which is off by default. The drop itself is right: refusing is what a rate limiter does, and closing the connection would turn a transient burst into an eviction. What was wrong is that the frame went unanswered. A reply costs nothing at the protocol level and is the difference between a caller's Completion failing and it hanging forever. An over-budget frame is now answered with `err "rate limited"`, addressed via peekCallId's bounded prefix scan -- the same reply-without-full-decode pattern the maxMessageBytes branch a few lines above already used, now hoisted so both branches share it. The frame still never reaches RemoteServer, and the connection still stays open. Addressing matters as much as replying: a zeroed callId would be worse than no reply at all, since callId == 0 is the client's synchronous-reply discriminator and would resume some unrelated parked register/deregister while the execute that triggered it still hung. The test asserts every refusal carries the callId of the frame it refuses. The existing throttle test asserted the *absence* of replies (replies < 20), which this change necessarily invalidates. Rewritten to distinguish outcomes rather than count frames: ok replies and rate-limited replies are counted separately, the throttle is still shown to throttle (ok < 20), and nothing is left unanswered (ok + limited == 20) -- the assertion that fails if the silent drop ever comes back. Verified by mutation: restoring the drop fails it with `0 >= 1`. Docs updated where they described the hang as expected: the header, the backend.md config and design-decision tables, and the completion.md passage listing a rate-limited frame among the requests that "genuinely disappear" -- it no longer belongs there, though a deadline is still worth arming for a dropped connection or a hung server, which no reply can cover. The test_client_execute_deadline.cpp header comment said the same and is corrected. qt suite 535 assertions / 72 cases; framework 20658 / 1170; clang-tidy-diff and -Wdocumentation clean. Closes #225
The original run was cancelled by the supersede-obsolete-runs concurrency rule (#257) shortly after it merged, and re-running the cancelled workflows produced attempts that were themselves cancelled within minutes. An empty commit gives the PR a fresh head so its checks run from a clean slate. No content change.
examples/TESTING.md and examples/polls/README.md each list the client-side execute deadline as a framework gap and cite a rate-limited frame being 'dropped silently' as one of the two things that make a Completion hang. That half is no longer true once the transport answers the frame. Narrowed to the clause this change invalidates. Both lists are stale in a second way that is not mine to fix here -- they name the execute deadline and Bridge::pendingCalls() as missing framework capabilities, and both shipped (bridge.hpp:1099 and bridge.hpp:1166). Filed separately rather than folded in.
CI caught what I missed: examples/polls/tests/test_shared_instance_lifecycle.cpp
has an end-to-end case built on the silent drop. It bursts 20 SubmitVotes past a
messagesPerSecond=5 bucket and asserted `clientTimeouts >= 1` -- that at least
one call was recovered by setExecuteDeadline, which was the only thing that
could settle a frame nobody answered. Now the transport answers it, the deadline
never fires, and the assertion fails.
I had claimed no ladder test configured the limiter. That was wrong, and wrong
for an avoidable reason: I grepped examples/ for messagesPerSecond and read the
first five hits, which were all documentation. The sixth was this test.
Rewritten for the contract the change establishes rather than deleted, since
what it proves is still worth proving -- the rung DoD asks for this harness run
with messagesPerSecond ON:
- it now asserts at least one call settled with the transport's own
`err "rate limited"`, and that successes < calls sent, keeping the
"more calls than replies" proof the brief asks for;
- and it asserts clientTimeouts == 0. The deadline is still armed, and is
deliberately no longer what saves these calls: a timeout here would now mean
a call really did go unanswered, which is the regression this guards.
The file's header bullet and the two long setup comments describing "dropped
silently, no reply of any kind" are corrected with it.
polls ladder suite: 68/68.
This was referenced Aug 24, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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.
Closes #225.
The defect
QtWebSocketServerConfig::messagesPerSeconddropped an over-budget framesilently — not replied to, not queued, connection left open. From the
caller's side that was an
executewhoseCompletionsimply never resolved,unless
Bridge::setExecuteDeadlinewas armed (off by default).The drop itself is right: refusing is what a rate limiter does, and closing the
connection would turn a transient burst into an eviction. What was wrong is that
the frame went unanswered. A reply costs nothing at the protocol level and is
the difference between a caller's
Completionfailing and it hanging forever.The fix
An over-budget frame is answered with
err "rate limited", addressed viapeekCallId's bounded prefix scan — the same reply-without-full-decode patternthe
maxMessageBytesbranch a few lines above already used, now hoisted so bothbranches share it. The frame still never reaches
RemoteServer, and theconnection still stays open.
Addressing matters as much as replying. A zeroed
callIdwould be worse thanno reply at all:
callId == 0is the client's synchronous-reply discriminator, soit would resume some unrelated parked
register/deregisterwhile theexecutethat triggered it still hung. The test asserts every refusal carries the
callIdof the frame it refuses.
The existing test had to change, and got stronger
The throttle test asserted the absence of replies (
replies < 20) — which thischange necessarily invalidates. I rewrote it to distinguish outcomes rather than
count frames:
okreplies andrate limitedreplies counted separatelyok < 20)ok + limited == 20) — the assertion that failsif the silent drop ever comes back
callIdis one of the sent idsConnectedStateafterwardsMutation-verified: restoring the silent drop fails it with
0 >= 1.Docs
Four places described the hang as expected behaviour and are corrected: the
header,
backend.md's config and design-decision tables, and thecompletion.mdpassage listing a rate-limited frame among the requests that "genuinely
disappear" — it no longer belongs there. A deadline is still worth arming for a
dropped connection or a hung server, which no reply can cover, and the text now
says exactly that.
test_client_execute_deadline.cpp's header comment made thesame claim and is fixed.
Verification
morph_qt_testsmorph_tests[limits]casesclang-tidy-diff(clang 22, as CI pins)-Wdocumentation -Werrorscripts/check_spec_citations.shThe last two were run locally because #258 showed CI's
clang-tidy-diffand theWASM job's
-Weverythingcatch things an ordinary local build does not.