Skip to content

Make registerSSE idempotent and stop re-processing from duplicating SSE state - #191

Open
milanobrtlik wants to merge 3 commits into
bigskysoftware:mainfrom
milanobrtlik:fix/sse-eventsource-leak-on-reprocess
Open

Make registerSSE idempotent and stop re-processing from duplicating SSE state#191
milanobrtlik wants to merge 3 commits into
bigskysoftware:mainfrom
milanobrtlik:fix/sse-eventsource-leak-on-reprocess

Conversation

@milanobrtlik

@milanobrtlik milanobrtlik commented Jul 31, 2026

Copy link
Copy Markdown

Description

Two bugs, and the same non-idempotent registerSSE under both. The second one was found from
the outside while this PR sat open (comment by @jeffothy,
hit in production). It is worth reading first, because it needs no morphing at all — it is
reachable by anyone whose connection drops once.

A reconnect stacks listeners on nested streams

onopen rebinds descendants after a retry:

const childrenToFix = elt.querySelectorAll("[sse-swap], [data-sse-swap], [hx-trigger], [data-hx-trigger]")
for (let i = 0; i < childrenToFix.length; i++) {
  registerSSE(childrenToFix[i])
}

querySelectorAll takes the whole subtree, but registerSSE resolves each match against its
own
closest source. For a plain subtree that is the same stream that just reconnected, and the
rebinding is correct: the descendants move to the new EventSource, and whatever they had
registered on the old one is inert, because a closed EventSource never fires. That is why this
has been fine in the common case.

It stops being fine as soon as a descendant sits under a nested sse-connect. Then it
resolves to the inner stream — which never dropped, is still OPEN, and is still holding the
listener it registered at startup. The outer reconnect adds a second one beside it. Every
subsequent message renders twice, then three times, once per outer reconnect. @jeffothy's
report: an app shell holding a user-scoped stream, a chat panel nested inside holding its own,
a phone that sleeps and wakes.

Nothing about this involves attribute changes or morphing. It is on main today.

Re-processing an element duplicates its state

Processing an element that carries sse-connect a second time duplicated everything the
extension had set up for it: it opened a second EventSource and orphaned the first, and it
registered the element's sse-swap listeners a second time. Full analysis and a reproduction of
the connection leak are in #190.

Here the root cause is one level deeper. The extension keeps its state in the element's internal
data, and htmx clears that data in deInitNode() right before it re-fires
htmx:afterProcessNode — so the extension has no reference left to either the connection or the
listeners it created earlier. This is also why the obvious guard, checking
api.getInternalData(elt).sseEventSource before creating a new source, cannot work: it never
sees anything.

htmx:afterProcessNode re-fires whenever an element's attribute hash changes, which is what a
morph swap does — idiomorph preserves the node and mutates its attributes, then htmx
re-processes it. That combination has been supported since #165.

The fix

Both kinds of state move into WeakMaps that survive the internal data reset.

Listeners. registerSSE drops the listeners the element registered earlier before
registering it again, which makes it idempotent. That is the whole of the nested-reconnect fix,
and it covers both branches of registerSSE (sse-swap and hx-trigger="sse:*"). It also
stops the dead EventSource from retaining listeners that close over the element.

Connections. ensureEventSource now behaves the way ensureEventSourceOnElement's doc
comment already claims it does ("If a usable EventSource already exists, then it is returned"):

  • already connected to the same sse-connect (and same sse-close), not CLOSED — keep the
    stream, restore the reference in internal data, return. No reconnect, no dropped events.
  • sse-connect changed — close the previous stream, open the new one, rebind descendants.
  • sse-connect removed — close the previous stream. This leaked unrecoverably before, since
    internal data had already been wiped by then.
  • previous stream already CLOSED (the normal retry path) — unchanged behaviour, and no event
    is fired, so the existing backoff logic and its tests are untouched.

One more thing falls out: re-processing an element while a reconnect was pending used to produce
two connections, and the pending retry now finds the fresh connection and returns.

Notes on specific lines

  • htmx:beforeCleanupElement and maybeCloseSSESource now share a closeEventSource() helper.
    That is not cosmetic: it has to fall back to the WeakMap, because on the replacement path
    the element's internal data has already been wiped, so a close that reads only internal data
    finds nothing and silently skips. The helper also clears internalData.sseEventSource after
    closing, so hasEventSource() stops reporting an already-closed connection as usable (the
    previous code left it dangling — see the commented-out // source = null).
  • onopen's rebinding condition gained replacedSource ||, so descendants are also rebound when
    the stream was replaced by an sse-connect change rather than by a retry.
  • var closeAttribute moved to the top of ensureEventSource because the reuse check needs it,
    and the duplicated api.getAttributeValue(elt, 'sse-connect') collapsed into one variable so
    the new else branch has something to attach to. Those are the only non-functional edits.

One open question

Closing on sse-connect change or removal fires htmx:sseClose with a new detail.type of
sourceReplaced. The docs list three values today (nodeReplaced, nodeMissing, message)
and live in the htmx repo, so this needs a companion one-paragraph docs PR — happy to open it.

Reusing nodeReplaced instead would avoid the cross-repo change, but it would be inaccurate:
the node is still there, and handlers that treat nodeReplaced as "this element is going away"
would run teardown against a live element. Let me know which you prefer and I will adjust.

Htmx version: 2.0.4
Used extension(s) version(s): htmx-ext-sse 2.2.3

Corresponding issue: #190

Testing

11 new tests in src/sse/test/ext/sse.js. As CONTRIBUTING asks, they were written first and
fail against the current main:

1) reuses the existing EventSource when the element is processed again:
     expected [ Array(2) ] to have a length of 1 but got 2
2) keeps children bound to the reused EventSource:
     expected 'init' to equal 'Event 1'
3) replaces the EventSource when sse-connect changes:
     expected 1 to equal 2            (old connection still OPEN, never closed)
4) does not register duplicate listeners when a child is processed again:
     expected [ [Function listener], …(1) ] to have a length of 1 but got 2
5) closes the EventSource when sse-connect is removed:
     expected 1 to equal 2
6) does not open a duplicate EventSource when processed while a reconnect is pending:
     expected [ … ] to have a length of 2 but got 3
7) opens a new EventSource after the connection was closed
8) does not stack listeners on a nested EventSource when the outer one reconnects:
     expected [ [Function listener], …(1) ] to have a length of 1 but got 2

  31 passing
  8 failing

The last one is the nested-reconnect case: outer sse-connect around an inner sse-connect
holding the sse-swap element, simulateConnectionError() on the outer, then assert the inner
stream is still OPEN, still has exactly one listener for e1, and that one event produces one
swap. It also fails with the removeSSEListeners call taken back out of the rest of this PR, so it
pins that call specifically rather than the change as a whole. The existing nested
fixtures only ever reached the initial subscription, which is why nothing caught this.

One of the other new tests — does not register duplicate listeners when the element is processed again, covering <div sse-connect sse-swap> where one element both owns the connection and is a
swap target — passes against main for the wrong reason: with two connections open, the mock
only ever fires an event on one of them, so the duplicate swap does not show up in the harness.
It fails against the connection fix alone (2 listeners instead of 1), which is what it is
there to pin. The real-browser numbers below show the effect that the mock cannot.

With the whole change applied, all 39 pass (28 existing + 11 new):

  39 passing (32ms)

beforeEach gained an eventSources array so a test can count how many connections were
created. The mock itself is unchanged.

Manual testing: I ran the real extension in headless Chrome against a small local SSE server
that counts open connections and broadcasts exactly one named event on request. The
sse-connect URL is deliberately relative — the test mock stores the URL verbatim, whereas a
real EventSource resolves .url to an absolute URL, so this is the one thing the suite cannot
demonstrate. Loading the page, then mutating an attribute and calling htmx.process():

                                                  main    this PR
connections open after re-process                  2         1
swaps caused by ONE server event, before           1         1
swaps caused by ONE server event, after            2         1

Checklist

  • I have read the contribution guidelines
  • I ran the test suite locally (npm run test) and verified that it succeeded

htmx re-fires htmx:afterProcessNode whenever an element's attributes
change, which is what a morph style swap does. Right before firing it,
htmx wipes the element's internal data, so the extension lost its
reference to the connection it had already opened and opened a second
one. The first stayed open forever, holding a connection slot on the
server and counting against the browser's per domain SSE limit.

Keep the connection in a WeakMap that survives the internal data reset,
so an element that is already connected to the same url keeps its
stream instead of opening another one. When the element asks for a
different connection, or drops sse-connect entirely, close the previous
stream and rebind the descendants that were listening on it.
registerSSE always added a listener and never removed the previous one,
and it could not: htmx wipes the element's internal data before
re-firing htmx:afterProcessNode, so sseEventListener no longer referred
to what had been registered earlier. An element processed twice ended up
listening for the same event twice and swapped every message twice.

Track the registered listeners in a WeakMap that survives the internal
data reset, and drop them at the start of registerSSE. This also makes
the onopen rebinding safe for descendants that were already registered
against the new connection in the same htmx.process pass.
@netlify

netlify Bot commented Jul 31, 2026

Copy link
Copy Markdown

Deploy Preview for htmx-extensions canceled.

Name Link
🔨 Latest commit 692460b
🔍 Latest deploy log https://app.netlify.com/projects/htmx-extensions/deploys/6a967737a069e6000901cd6f

@jeffothy

Copy link
Copy Markdown

This fixes a second bug too, and nothing here pins it.

onopen re-registers every sse-swap / sse:* descendant on a retry, including ones that own their own sse-connect. registerSSE resolves those against their source, so an outer stream reconnecting stacks a handler on an inner stream that never dropped, and every message renders one more time per outer reconnect. We hit it in production: app shell holds a user-scoped stream, a chat panel nested inside holds its own, phone sleeps and wakes, messages double then triple.

removeSSEListeners at the top of registerSSE fixes it on both branches. But the nested fixtures only cover initial subscription, so a regression would go unnoticed. Outer sse-connect plus inner sse-connect with sse-swap, simulateConnectionError() on the outer, assert the inner's _listeners.e1 is still length 1.

The onopen rebinding walks the whole subtree with querySelectorAll, but
registerSSE resolves every match against its own closest source. A
descendant that sits under a nested sse-connect is therefore rebound
onto the inner stream, which never dropped and is still holding the
listener it registered at startup. Every reconnect of the outer stream
stacked one more, and every message rendered one more time.

Dropping the listeners at the start of registerSSE already covers this,
but the nested fixtures only reach the initial subscription, so nothing
would catch the day that call is removed as redundant.

Reported against bigskysoftware#191 by jeffothy, who hit it in production: an app
shell holding a user scoped stream, a chat panel nested inside holding
its own, and messages doubling every time the device woke up.
milanobrtlik added a commit to milanobrtlik/htmx-extensions that referenced this pull request Sep 1, 2026
The onopen rebinding walks the whole subtree with querySelectorAll, but
registerSSE resolves every match against its own closest source. A
descendant that sits under a nested sse-connect is therefore rebound
onto the inner stream, which never dropped and is still holding the
listener it registered at startup. Every reconnect of the outer stream
stacked one more, and every message rendered one more time.

Dropping the listeners at the start of registerSSE already covers this,
but the nested fixtures only reach the initial subscription, so nothing
would catch the day that call is removed as redundant.

Reported against bigskysoftware#191 by jeffothy, who hit it in production: an app
shell holding a user scoped stream, a chat panel nested inside holding
its own, and messages doubling every time the device woke up.

Claude-Session: https://claude.ai/code/session_01TrmeGu1jE9QSPZF92aGxVP
@milanobrtlik milanobrtlik changed the title Do not duplicate an element's SSE state when it is processed again Make registerSSE idempotent and stop re-processing from duplicating SSE state Sep 1, 2026
@milanobrtlik

Copy link
Copy Markdown
Author

@jeffothy Thanks — you're right, and I verified it rather than taking it on trust.

I wrote the test you described: outer sse-connect around an inner one that holds the
sse-swap element, simulateConnectionError() on the outer, then assert the inner's
_listeners.e1. It fails against main with expected [ … ] to have a length of 1 but got 2,
and it still fails if I take removeSSEListeners back out of this PR while leaving the
connection half in place — so it pins that call specifically rather than the change as a whole.
It's in the branch now.

One detail worth adding to your analysis, because it explains why this survived so long: for a
flat subtree the onopen rebinding is actually correct. The descendants move to the new
EventSource, and whatever they registered on the old one is inert, since a closed
EventSource never fires. It only goes wrong when getClosestMatch resolves to a live
source, which is exactly the nested sse-connect case you hit.

I've also restructured the PR description to lead with this rather than with the morph
re-processing bug, since this one needs no attribute changes at all and is reachable by anyone
whose connection drops once. The production report is a considerably stronger case for the fix
than the edge case I opened it with, so thanks for taking the time to write it up.

@milanobrtlik
milanobrtlik force-pushed the fix/sse-eventsource-leak-on-reprocess branch from a6dcaf4 to 692460b Compare September 1, 2026 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants