Notification delivery: filter ownership, single-use binding, config-path honesty - #263
Merged
Conversation
added 3 commits
July 6, 2026 17:20
HttpSseFilterChainFactory retained a shared_ptr to every filter it ever built in a filters_ vector, kept for the factory's whole lifetime. Since an HTTP+SSE client sends each request on a one-shot POST connection, this leaked ~3 filter objects (and their buffers) per request until server shutdown, and deferred the combined filter's destructor — which deregisters the SSE stream from the registry — to that same late moment. The vector was write-only: every filter in it was also installed into the connection's FilterManager via addReadFilter/addWriteFilter, which is the per-connection owner. Dropping filters_ makes filter lifetime equal connection lifetime and runs the SSE-stream deregistration in the filter destructor at connection close, where it belongs (the registry removeConnection sweep becomes a pure backstop). Test: opening an SSE stream then destroying only the connection (factory kept alive) drops the registry entry to zero — proof the filter destructed with its connection rather than lingering in the factory.
current_transport_session_id_ was an ambient dispatch-context member that was overwritten on each announcement but never cleared after use. The transport filter announces it right before onRequest/onNotification, but a producer that does not announce — stdio, or any non-SSE filter chain — would resolve the session while a previous message's id still sat in the member, attributing the message to another client's session. getOrCreateCurrentSession now swaps the id out into a local and clears the member as it consumes it, so the binding applies only to the one message whose dispatch it preceded; a stale binding is unrepresentable rather than depending on every producer to announce an empty id. Dispatch is synchronous per message on the dispatcher thread, so consume-and-clear is race-free. Per-message consume correctness is already covered by the multi-client delivery tests (two clients subscribing on separate connections with distinct ids each land in their own session). The remaining guarantee — a non-announcing producer not inheriting an id — is only reachable with two transports in one server instance, which is not currently constructible.
The config-driven filter-chain path builds a listener from named filters and never constructs the HTTP+SSE transport that owns the server->client push channel, so http_sse_factory_ stays null there. Server-initiated notifications (resource updates, etc.) were then dropped with no signal: sendNotificationToSession returned 'SSE stream gone', which reads like a transient client disconnect rather than 'this listener has no push channel at all'. - performListen now logs a warning when it takes the config-driven branch, at the point where the capability is actually decided, so an operator relying on notifications is not left debugging silent non-delivery. - sendNotificationToSession distinguishes a missing channel (no registry) from a closed stream (registry present, session absent), so the returned error names the real cause. This is honest failure reporting, not new delivery capability — building SSE push on the config-driven chain is separate, larger work. Existing delivery tests confirm the built-in HTTP+SSE path is unaffected.
gophergogo
force-pushed
the
followup-envoy-alignment
branch
from
July 7, 2026 00:31
ec0dac8 to
95f17ef
Compare
caleb2h
approved these changes
Jul 7, 2026
caleb2h
pushed a commit
that referenced
this pull request
Jul 7, 2026
HttpSseFilterChainFactory retained a shared_ptr to every filter it ever built in a filters_ vector, kept for the factory's whole lifetime. Since an HTTP+SSE client sends each request on a one-shot POST connection, this leaked ~3 filter objects (and their buffers) per request until server shutdown, and deferred the combined filter's destructor — which deregisters the SSE stream from the registry — to that same late moment. The vector was write-only: every filter in it was also installed into the connection's FilterManager via addReadFilter/addWriteFilter, which is the per-connection owner. Dropping filters_ makes filter lifetime equal connection lifetime and runs the SSE-stream deregistration in the filter destructor at connection close, where it belongs (the registry removeConnection sweep becomes a pure backstop). Test: opening an SSE stream then destroying only the connection (factory kept alive) drops the registry entry to zero — proof the filter destructed with its connection rather than lingering in the factory.
caleb2h
pushed a commit
that referenced
this pull request
Jul 7, 2026
current_transport_session_id_ was an ambient dispatch-context member that was overwritten on each announcement but never cleared after use. The transport filter announces it right before onRequest/onNotification, but a producer that does not announce — stdio, or any non-SSE filter chain — would resolve the session while a previous message's id still sat in the member, attributing the message to another client's session. getOrCreateCurrentSession now swaps the id out into a local and clears the member as it consumes it, so the binding applies only to the one message whose dispatch it preceded; a stale binding is unrepresentable rather than depending on every producer to announce an empty id. Dispatch is synchronous per message on the dispatcher thread, so consume-and-clear is race-free. Per-message consume correctness is already covered by the multi-client delivery tests (two clients subscribing on separate connections with distinct ids each land in their own session). The remaining guarantee — a non-announcing producer not inheriting an id — is only reachable with two transports in one server instance, which is not currently constructible.
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.
Follow-up to #262, addressing the three architectural items called out there as "not addressed here (larger scope)". Each was checked against the network-filter subsystem's reference design before implementing.
1. Own filters per-connection via FilterManager, not the factory
HttpSseFilterChainFactoryretained ashared_ptrto every filter it built in afilters_vector for the factory's whole lifetime. Because an HTTP+SSE client sends each request on a one-shot POST connection, this leaked ~3 filter objects (plus buffers) per request until shutdown, and deferred the combined filter's destructor — which deregisters the SSE stream — to that same late moment.The vector was write-only: every filter was also installed into the connection's
FilterManagerviaaddReadFilter/addWriteFilter, which is the per-connection owner (the connection owns itsFilterManagerby value; factory callbacks install into the manager and retain nothing). Droppingfilters_makes filter lifetime equal connection lifetime; the SSE-stream deregistration now runs in the filter destructor at connection close, and the registryremoveConnectionsweep becomes a pure backstop.Test: open an SSE stream, then destroy only the connection (factory kept alive) → registry entry drops to zero, proving the filter destructed with its connection rather than lingering in the factory.
2. Make the transport session binding strictly single-use
current_transport_session_id_was an ambient dispatch-context member overwritten on each announcement but never cleared after use — the anti-pattern of reading per-request identity from a connection-scoped global. A producer that never announces (stdio, any non-SSE chain) could resolve a message against a previous message's id.getOrCreateCurrentSessionnow swaps the id into a local and clears the member as it consumes it, so a stale binding is unrepresentable. Per-message consume correctness is already covered by the multi-client delivery tests; the remaining guarantee needs two transports in one instance, which isn't currently constructible.3. Surface that config-driven listeners have no server-push channel
The config-driven filter-chain path never builds the SSE transport, so
http_sse_factory_stays null and server-initiated notifications were dropped with a misleading "SSE stream gone".performListennow warns at the point the capability is decided, andsendNotificationToSessiondistinguishes "no channel configured" from "stream closed". Honest failure reporting, not new capability — SSE push on the config-driven chain is separate, larger work.Tests
Full server/client/filter/integration suite passes; new filter-lifetime test added.