chore(core): fix conn/disconn tests for sse

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Error			Sep 19, 2025 0:48am

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: SSE Connection/Disconnection Test Fixes

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changes in packages/cloudflare-workers/vitest.config.ts
• Analyze changes in packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts
• Provide comprehensive review feedback on code quality, bugs, performance, security, and test coverage

Summary of Changes

This PR fixes connection/disconnection tests for Server-Sent Events (SSE) by addressing timing issues and test logic problems in Cloudflare Workers environment.

File 1: packages/cloudflare-workers/vitest.config.ts

• Increased test timeout from 15 seconds to 60 seconds to accommodate slower package installation

File 2: packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts

• Fixed test logic in the connection lifecycle test
• Added proper connection tracking assertions
• Improved timing for SSE disconnection detection

Code Quality Assessment

✅ Positive Aspects:

1. Proper timeout handling: The vitest config change appropriately addresses the slower execution environment of Cloudflare Workers
2. Improved test reliability: The test fixes address race conditions specific to SSE implementations
3. Clear comments: Good use of comments explaining SSE-specific timing issues (// SSE takes a long time to disconnect on CF Workers)

⚠️ Issues and Concerns:

1. Critical Bug - Variable Reference Error (packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts:176)

   const disconnects = await newConn.getDisconnectionCount();

   This references newConn before it's declared (line 187). This will cause a ReferenceError at runtime.

2. Test Logic Issue (packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts:192)

   const connectionIds = await conn.getConnectionIds(); // Should be newConn

   Using disposed connection conn instead of active newConn connection.

3. Inconsistent Test Assertions (packages/rivetkit/src/driver-test-suite/tests/actor-conn-state.ts:164-168)

   const disconnects = await conn.getDisconnectionCount();
   expect(disconnects).toBe(0);

   This assumes initial state is 0, but the test is checking disconnection count on a connection that may have previous state.

4. Race Condition Risk: The test still has potential race conditions between connection disposal and disconnection count updates, even with the increased timeout.

Performance Considerations

• Timeout Increase: The 4x timeout increase (15s → 60s) seems excessive. Consider if a more modest increase (e.g., 30s) would suffice.
• Test Efficiency: Multiple vi.waitFor calls with 10-second timeouts may make tests slower than necessary.

Security Concerns

No security issues identified. The changes are limited to test configuration and test logic.

Test Coverage

Improvements Needed:

1. Fix the variable reference errors that will cause test failures
2. Consider testing edge cases for SSE disconnection timing
3. Add assertions to verify the exact sequence of events in connection lifecycle

Recommendations

Critical Fixes Required:

// Fix 1: Move newConn declaration before first usage
const newConn = handle.connect();

// Verify disconnection was tracked
await vi.waitFor(
    async () => {
        const disconnects = await newConn.getDisconnectionCount();
        expect(disconnects).toBe(1);
    },
    { timeout: 10_000, interval: 100 }
);

// Fix 2: Use correct connection reference
await vi.waitFor(async () => {
    const connectionIds = await newConn.getConnectionIds(); // Changed from conn
    expect(connectionIds.length).toBe(2);
});

General Improvements:

1. Consider reducing the vitest timeout to 30s instead of 60s
2. Add more specific error messages to waitFor assertions
3. Consider extracting timeout constants for better maintainability

The core approach of fixing SSE timing issues is sound, but the implementation has critical bugs that need to be addressed before merging.

[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(core): migrate gateway to use websocket protocols #1293
• chore(core): update api schema to match new engine schema #1292
• chore(core): fix conn/disconn tests for sse #1290 👈 (View in Graphite)
• fix(core): fix inline client driver for cloudflare-workers #1289
• chore(core): fix missing transport for inline tests #1287 : 1 other dependent PR (#1250 )
• chore(core): fix test for sending message to specific connection sse race condition #1286
• fix(cloudflare-workers): fix alarms #1285
• chore(core): fix broadcast race condition #1284
• chore: list sideEffects #1283
• chore(core): fix connection tests to support sse #1282
• fix(cloudflare-workers): fix aborts on sse streams on cloudflare workers #1281
• fix(cloudflare-workers): fix aborting sse streams #1280
• chore: remove unused FakeEventSource #1279
• 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.

[pkg-pr-new]

More templates

• example-ai-agent
• example-better-auth-external-db
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• counter
• example-crdt
• example-database
• example-sqlite
• example-elysia
• example-express
• example-game
• example-hono
• example-hono-react
• example-next-js
• example-rate
• example-raw-fetch-handler
• example-raw-websocket-handler-proxy
• example-react
• starter
• example-stream
• example-sync
• example-tenant
• example-trpc

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/actor@1290

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/cloudflare-workers@1290

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/core@1290

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/db@1290

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/framework-base@1290

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/next-js@1290

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/react@1290

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivetkit@1290

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/sql-loader@1290

commit: 9ea5720

[graphite-app]

Merge activity

• Sep 24, 6:06 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Sep 24, 6:07 AM UTC: CI is running for this pull request on a draft pull request (#1294) due to your merge queue CI optimization settings.
• Sep 24, 6:08 AM UTC: Merged by the Graphite merge queue via draft PR: #1294.