Repository navigation
fix(mux): end a timed-out request on its backend before answering it - #1924
Open
FlorentinDUBOIS wants to merge 2 commits into
Open
FlorentinDUBOIS wants to merge 2 commits into
FlorentinDUBOIS wants to merge 2 commits into
Conversation
A request answered 504 by a timeout could leave its backend connection able to answer another request. An HTTP/2 frontend outlives the 504, so the client's next request on the connection could read the late response of the request that timed out. Backend timeout, the default timer order: the backend-token branch of `Mux::timeout_inner` installed the 504 and only then called `end_stream`. `ConnectionH1::end_stream` pools the connection when the response is terminated, read the 504 as the backend's own whole response, and pooled a connection whose request the backend had not answered. The session's next request for that backend was written behind it and read its answer. Frontend timeout, when `back_timeout` is longer than `front_timeout`: the frontend-token branch answered a `Linked` stream (504, or a cut response) without ending it on the backend, and its second pass only ends streams still `Linked`. An H1 backend kept naming the stream slot and an H2 backend kept its wire mapping. Once the slot was recycled and reused, the backend's late answer was delivered on the new stream; once `Context::shrink_trailing_recycle` had dropped the slot, a late read or close indexed past `Context::streams` and panicked. Both branches now end the stream on the backend while `Stream::back` still holds what the backend sent, then install the answer. The H1 connection is closed, an H2 backend stream is reset, and a backend that answered whole is still pooled. The backend branch keeps a single `end_stream` site, ahead of its arms, and asserts that a backend still owing an answer is never `KeepAlive`. Tests, each seen red with its hunk reverted: seven `Mux`-level tests over loopback sockets and real H2 frames, and two e2e tests with real timers in `h2_timeout_backend_detach_tests.rs`. LIFECYCLE.md invariant 22 follows the new position of `end_stream`. Signed-off-by: Florentin Dubois <florentin.dubois@clever.cloud>
main gained #1922 and an actions bump. Git merged every file on its own; CHANGELOG.md is rearranged by hand so the two Security entries under Unreleased share one heading, main's first. Signed-off-by: Florentin Dubois <florentin.dubois@clever.cloud>
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.
What was wrong
A request answered
504by a timeout could leave its backend connection able to answer another request. An HTTP/2 frontend outlives the504, so the client's next request on the same connection could receive the late response of the request that timed out. Two branches ofMux::timeout_inner(lib/src/protocol/mux/mod.rs) led there.Backend timeout, the default timer order (
back_timeout30 s <front_timeout60 s). The backend-token branch installed the504and only then calledend_stream.ConnectionH1::end_streampools the connection when the response is terminated: it read the504as the backend's own whole response and moved the connection toBackendStatus::KeepAlivewith its request still unanswered. The session's next request for that backend was written behind it and read its answer. Only an HTTP/1.1 backend that had not started answering is affected; a cut response leavesbackin error, and an HTTP/2 backend stream is reset.Frontend timeout, when
back_timeoutis longer thanfront_timeout. The frontend-token branch answered aLinkedstream (504, or a cut response) without ending it on the backend, and its second pass only ends streams stillLinked. An H1 backend keptConnectionH1::stream = Some(gid)and an H2 backend kept its wire mapping. Once the slot was recycled and reused, the backend's late answer was delivered on the new stream. OnceContext::shrink_trailing_recyclehad dropped the slot, a late read or close on it indexed pastContext::streamsand panicked.Impact
Muxlevel only; no end-to-end test drives it.504(Connection: close) and is not affected with the default answers. A custom504template withoutConnection: closewas not tested.The fix
Both branches end the stream on the backend while
Stream::backstill holds what the backend sent, then install the answer. The H1 connection is closed instead of pooled or left attached, an H2 backend stream is reset, and a backend that answered whole is still pooled.end_streambeforeset_default_answerand beforeforcefully_terminate_answer.end_streamsite, moved ahead of the arms. The deferral to a slow client (response_waits_on_client) is decided first and still skips it. Adebug_assert!states the invariant: a backend whose response is neither terminated nor in error is neverKeepAlive.Tests
Each seen red with its hunk reverted, green with it, and red again with debug assertions compiled out.
lib/src/protocol/mux/mod.rs, over loopback sockets and real H2 frames:a_frontend_timeout_answering_504_detaches_the_backend_it_was_waiting_ona_frontend_timeout_cutting_a_response_detaches_the_backend_still_sending_ita_late_backend_answer_never_reaches_the_request_that_reused_the_slota_late_h2_backend_answer_never_reaches_the_request_that_reused_the_slota_late_backend_answer_never_indexes_a_slot_the_session_droppeda_backend_timeout_answering_504_closes_the_backend_that_still_owes_an_answera_backend_timeout_after_a_whole_response_keeps_the_backend_pooled(the pair)e2e/src/tests/h2_timeout_backend_detach_tests.rs, real worker, TLS H2 client, scripted H1 backend:test_h2_late_backend_answer_after_frontend_timeout_stays_off_the_next_request(front_timeout2 s,back_timeout20 s)test_h2_late_backend_answer_after_backend_timeout_stays_off_the_next_request(back_timeout2 s,front_timeout60 s)Before the fix, both e2e tests print the first request's answer on the second stream:
After:
Commands run
On this commit:
cargo +nightly fmt --all -- --check→ exit 0cargo clippy --all-targets --locked -- -D warnings→ exit 0cargo test -p sozu-lib --locked→1722 passed; 0 failedcargo test -p sozu-e2e --locked -- h2_timeout_backend_detach→2 passed; 0 failedpython3 .github/scripts/check_doc_citations.py --base 072b610e7→ exit 0On the same tree before the
CHANGELOG.mdentry was added:cargo test -p sozu-e2e --locked -- --skip tests::fuzz_tests::→691 passed; 0 failedDocs
lib/src/protocol/mux/LIFECYCLE.mdinvariant 22 and the comments that saidend_streamruns "at the bottom of the loop" follow its new position.CHANGELOG.md: aSecurityentry underUnreleased.Relation to #1922
Independent of it: the same defects reproduced on its tip. #1922 merged first, and
cd0f78e3dmergesmain(68f34840f) into this branch. Git merged every file on its own;CHANGELOG.mdis rearranged by hand so the twoSecurityentries underUnreleasedshare one heading. On the merged tree:cargo +nightly fmt --all -- --check→ exit 0cargo clippy --all-targets --locked -- -D warnings→ exit 0cargo test -p sozu-lib --locked→1729 passed; 0 failedcargo test -p sozu-e2e --locked -- h2_timeout_backend_detach h2_stream_slot→4 passed; 0 failedpython3 .github/scripts/check_doc_citations.py --base 68f34840f→ exit 0The full e2e suite was not re-run locally on the merged tree. It passed (
693 passed) on the #1922 tip with this change applied, which differs from the merged tree byCHANGELOG.mdand two workflow files.