fix(core): fix disconnecting sse connections

[NathanFlurry]

No description provided.

[claude]

Claude finished @NathanFlurry's task —— View job

---

Code Review: SSE Connection Disconnect Fix

I've completed my review of this PR that fixes disconnecting SSE connections. Here are my findings:

Review Checklist:

• Read and understand the changed files
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Review security concerns
• Assess test coverage
• Provide comprehensive feedback

---

✅ Overall Assessment: Good Fix

This PR properly addresses SSE connection cleanup issues with clean, focused changes.

📋 Key Changes

1. Fixed SSE abort handling (router-endpoints.ts:363-386)
2. Re-enabled SSE transport tests (mod.ts:91)
3. Improved test reliability (actor-conn.ts:87-108)

---

🔍 Detailed Review

packages/core/src/actor/router-endpoints.ts ✅

Strengths:

• Proper event handling: Correctly switched from c.req.raw.signal.addEventListener("abort") to stream.onAbort() - this is the proper way to handle SSE stream disconnections
• Resource cleanup: Properly removes connections from global state and calls actor.__removeConn(conn)
• Error handling: Good error handling with logging and resolver cleanup on both success and error paths
• Clear logging: Updated log messages are more descriptive ("sse stream aborted" vs "sse shutting down")

Code Quality:

• Clean removal of unnecessary HACK comment and workaround code
• Consistent error handling pattern with the rest of the codebase
• Proper use of promise resolvers for async cleanup

packages/core/src/driver-test-suite/mod.ts ✅

Strengths:

• Confidence in fix: Re-enabling SSE tests shows confidence the fix works
• Good test coverage: Ensures both WebSocket and SSE transports are tested

packages/core/src/driver-test-suite/tests/actor-conn.ts ✅

Strengths:

• Better test reliability: Using promise-based event waiting instead of timing-dependent expectations
• Race condition fix: Properly waits for all expected events before asserting, preventing flaky tests
• Clear intent: Test logic is more explicit about what it's waiting for

---

🚀 Performance Considerations

• Improved cleanup: Proper SSE cleanup should prevent resource leaks
• Faster disconnect detection: Using stream.onAbort() should detect disconnections more quickly than polling request signals

🔒 Security Assessment

• No security concerns identified
• Proper cleanup prevents potential resource exhaustion
• No sensitive data exposure risks

🧪 Test Coverage Assessment

• Excellent: SSE transport tests are now re-enabled
• Race condition fix: Test improvements make the suite more reliable
• Comprehensive coverage: Tests cover WebSocket, SSE, and various connection scenarios

---

🎯 Minor Suggestions

1. Consider adding a timeout in router-endpoints.ts:394:

   // Wait until connection aborted
   await abortResolver.promise;

   Consider adding a timeout to prevent indefinite waiting, though this may not be necessary given the SSE stream lifecycle.

2. Potential enhancement: The error handling in the abort callback could potentially differentiate between expected disconnects and actual errors, but the current approach is adequate.

---

✅ Approval Recommendation

This is a solid fix that:

• ✅ Properly addresses the SSE disconnection issue
• ✅ Follows existing code patterns and best practices
• ✅ Includes proper test coverage
• ✅ Has no security or performance concerns
• ✅ Improves system reliability

The changes are focused, well-tested, and use the correct Hono SSE APIs. This should resolve the SSE connection disconnect issues mentioned in the PR title.

---

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore: simplify package directories #1205
• chore(core): migrate actor persist & file system to bare #1204
• chore(core): migrate client protocol to BARE #1203
• chore(core): move actor liveness check to setInterval #1202
• fix(core): fix disconnecting sse connections #1201 👈 (View in Graphite)
• feat(core): add abort controller & implement waitUntil #1200
• feat(core): actor sleeping #1178
• fix(core): revert deleted fixtures #1199
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• merge queue - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

Merge activity

• Sep 11, 10:34 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Sep 11, 10:35 PM UTC: CI is running for this pull request on a draft pull request (#1206) due to your merge queue CI optimization settings.
• Sep 11, 10:35 PM UTC: Merged by the Graphite merge queue via draft PR: #1206.