Release sse request when stream setup throws - #3984
Closed
1cg wants to merge 1 commit into
Closed
Conversation
1cg
force-pushed
the
fix/sse-release-on-error
branch
from
August 28, 2026 01:35
7e01cee to
60da0b8
Compare
Collaborator
|
fine to make these changes. they will not trigger in a real situation but might be fine as defense in depth and to stop future work when the potential issue is discovered again later |
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.
Follow-up to #3980.
Problem
#3980 changed the extension to hand core a manually resolved promise instead of the
handleSSEResponsepromise:release()is only reachable from thefinallyinhandleSSEResponse, and that function does not open itstryuntil line 181. Anything that throws between function entry and there skips thefinally, and thecatchdoes not release.Core awaits
extensionPromiseatsrc/htmx.js:618, inside afinally, with__hideIndicatorsright after at line 625. So the await never returns. The indicator spins forever, the element stays inhtmx-request, and no error surfaces to the user.The window is real.
getConfig(element)runs beforereleaseis even defined, so it hangs every mode,immediateincluded.api.htmxProp(element).sse = connectionis also set before thetry, and #3980 dropped thecleanup(element)that used to sit in thecatch, so a throw there leaves the connection registered and blocks reconnects on that element.Before #3980 this was safe.
extensionPromisewas thehandleSSEResponsepromise itself, so a rejection settled it and core continued.Fix
Release and clean up in the
catch:release()is already idempotent, so the normal path is unaffected.Testing
Added
releases the request when setup throws. It forces a throw in the setup window and asserts both symptoms: the indicator hides, and_htmx.ssedoes not stay registered.Verified the test fails without the fix:
Full suite: 1732 passed, 0 failed, 100 percent coverage.
Draft, since #3980 only just landed and you may prefer to fold this in there.