Skip to content

fix(sse): ignore pushes after stream close#1411

Merged
pi0 merged 1 commit into
h3js:mainfrom
ccai40359-wq:test/event-stream-ignore-post-close
Jul 2, 2026
Merged

fix(sse): ignore pushes after stream close#1411
pi0 merged 1 commit into
h3js:mainfrom
ccai40359-wq:test/event-stream-ignore-post-close

Conversation

@ccai40359-wq

@ccai40359-wq ccai40359-wq commented Jun 8, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Treat disposed SSE event streams as closed before writing
  • Add matrix coverage that later pushes/comments are ignored after close

Verification

  • pnpm vitest run test/sse.test.ts --coverage=false
  • pnpm lint (passes with existing typescript(await-thenable) warning in test/bench/bench.ts)
  • git diff --check
  • Added-line security scan: 0 findings
  • Independent review: passed

Closes #1410

Summary by CodeRabbit

  • Bug Fixes

    • Fixed event stream closure handling to consistently ignore push operations after the stream is closed.

  • Tests

    • Added test coverage verifying push operations are properly ignored after stream closure.

@ccai40359-wq
ccai40359-wq requested a review from pi0 as a code owner June 8, 2026 06:40
@coderabbitai

coderabbitai Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

EventStream consolidates "closed" state detection into a private _isClosed getter that treats both writer closure and disposal as effectively closed. All write operations (pushComment, _sendEvent, _sendEvents, flush) and the close() method now use this single guard, ensuring consistent no-op behavior after closure. A new test validates that post-close push calls are ignored.

Changes

Post-Close Stream Behavior

Layer / File(s) Summary
Unified closed-state getter and method guards
src/utils/internal/event-stream.ts		 _isClosed getter centralizes both _writerIsClosed and _disposed as effectively closed; all previous closed checks in pushComment, _sendEvent, _sendEvents, flush, and close() now use this single getter instead of _writerIsClosed alone.
Test validation for post-close idempotency
test/sse.test.ts		 Test removes an inline TODO comment and adds a case verifying that push and pushComment calls after eventStream.close() are silently ignored, with only pre-close events appearing in the response body.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Poem

🐰 A getter so wise centralizes the way,

Both closed and disposed now share the same say,

No more scattered checks through the stream's sunny days,

With tests standing guard on the post-close pathways! 🌿✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The PR meets #1410's request for a small behavior-test PR without snapshot padding, mock-only assertions, threshold changes, or excluded file modifications.
Out of Scope Changes check ✅ Passed All changes are scoped to testing the behavior of ignoring pushes after stream close; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly matches the main behavior change: SSE pushes are ignored after the stream is closed.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

pi0x
pi0x approved these changes Jul 2, 2026
View reviewed changes

@pi0x pi0x left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this review was performed by AI agents (Claude Code) on behalf of the maintainer, in two independent passes.

Pass 1 reviewed the diff for correctness, edge cases, and test coverage. Pass 2 (a fresh agent, adversarial by design) independently re-read the diff, checked for merge conflicts/CI status, and tried to find a reason to block approval before this review was posted. Both passes agree this change is correct, adequately tested, and safe to merge.

Caveat: this repo's real CI workflow (lint + build + vitest) requires maintainer approval to run on fork PRs and has not executed on this PR yet — only automated third-party checks (CodeRabbit, Socket Security) have run. Please trigger/approve the CI workflow before merging.

@pi0 pi0 changed the title fix: ignore SSE pushes after stream close fix(sse): ignore pushes after stream close Jul 2, 2026
pi0
pi0 approved these changes Jul 2, 2026
View reviewed changes

@pi0 pi0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pkg-pr-new

pkg-pr-new Bot commented Jul 2, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/h3@1411

commit: 84a5278

@pi0
pi0 merged commit f8c20a0 into h3js:main Jul 2, 2026
8 checks passed
pi0x pushed a commit to mixelburg/h3 that referenced this pull request Jul 2, 2026
@ccai40359-wq @pi0
fix(sse): ignore pushes after stream close (h3js#1411)
7a051e5
This was referenced Jul 18, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pi0 pi0 pi0 approved these changes
@pi0x pi0x pi0x approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Open to a small behavior-test PR?

3 participants

@ccai40359-wq @pi0 @pi0x