chore(rivetkit): add connection for raw http #3425

NathanFlurry · 2025-11-09T23:02:27Z

No description provided.

vercel · 2025-11-09T23:02:33Z

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

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Nov 13, 2025 2:48am

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Nov 13, 2025 2:48am
rivet-inspector	Ignored	Preview	Nov 13, 2025 2:48am
rivet-site	Ignored	Preview	Nov 13, 2025 2:48am

NathanFlurry · 2025-11-09T23:02:49Z

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

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

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.

claude · 2025-11-09T23:04:18Z

PR Review: Add connection for raw HTTP

Summary

This PR adds a new createRawHttpSocket() connection driver and handleRawHttpHandler() function to properly track HTTP request lifecycle through the actor's connection management system. Overall, the implementation is well-structured and follows existing patterns in the codebase.

Code Quality & Best Practices

✅ Good Practices:

Follows existing driver patterns (consistent with createHttpSocket() and createRawWebSocketSocket())
Clear documentation explaining the purpose and difference from standard HTTP driver
Proper cleanup of connections in finally block
Consistent code style and formatting

⚠️ Issues & Recommendations:

1. Connection Lifecycle Inconsistency (rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:548-572)

Issue: The signature of getConnectionReadyState is inconsistent with the interface definition.

In createRawHttpSocket():

getConnectionReadyState: (): DriverReadyState | undefined => {
    // HTTP connections are always considered open until the request completes
    return DriverReadyState.OPEN;
},

But the ConnDriver interface expects:

getConnectionReadyState(
    actor: AnyActorInstance,
    conn: AnyConn,
): DriverReadyState | undefined;

Fix: Update the signature to match the interface:

getConnectionReadyState: (_actor, _conn): DriverReadyState | undefined => {
    return DriverReadyState.OPEN;
},

This matches the pattern used in createHttpSocket() at rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/http.ts:10.

2. Disconnect Signature Inconsistency (rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/raw-http.ts:16-18)

Issue: The disconnect method signature doesn't match the ConnDriver interface.

Current implementation:

disconnect: async () => {
    // Noop
},

Expected signature from interface:

disconnect(
    actor: AnyActorInstance,
    conn: AnyConn,
    reason?: string,
): Promise<void>;

Fix: Update to match interface:

disconnect: async (_actor, _conn, _reason?) => {
    // Noop
},

3. Missing Error Handling Context (rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:560-571)

Issue: If actor.createConn() throws an error, the connection cleanup logic in the finally block won't execute properly since createdConn will be undefined.

Current code:

try {
    const conn = await actor.createConn(createRawHttpSocket(), {}, req);
    createdConn = conn;
    return await actor.handleRawRequest(req, {});
} finally {
    if (createdConn) {
        actor.connDisconnected(createdConn, true);
    }
}

Recommendation: This is actually correct as-is - if createConn throws, we don't want to clean up a connection that was never created. However, consider adding error logging for clarity:

try {
    const conn = await actor.createConn(createRawHttpSocket(), {}, req);
    createdConn = conn;
    return await actor.handleRawRequest(req, {});
} catch (error) {
    // Connection creation or request handling failed
    // Only clean up if connection was successfully created
    throw error;
} finally {
    if (createdConn) {
        actor.connDisconnected(createdConn, true);
    }
}

Performance Considerations

✅ Good:

No performance concerns identified
Proper cleanup ensures connections don't leak
Consistent with existing HTTP driver patterns

Security Concerns

No major security issues identified. The implementation properly:

Handles request cleanup
Doesn't expose sensitive data
Follows existing security patterns

Test Coverage

⚠️ Missing:

No tests included for the new handleRawHttpHandler() function
No tests verifying connection lifecycle for raw HTTP requests
No tests verifying cleanup happens in error cases

Recommendation: Add tests to verify:

Connection is created when handling raw HTTP requests
Connection is properly cleaned up after request completes
Connection cleanup happens even when handleRawRequest throws
The connection driver state is correct throughout the lifecycle

Additional Notes

Router path handling (rivetkit-typescript/packages/rivetkit/src/actor/router.ts:178):
The path prefix replacement logic is clean:

const originalPath = url.pathname.replace(/^\/request/, "") || "/";

This is simpler than the previous implementation and correctly handles the path stripping.

Summary

Required Changes:

Fix getConnectionReadyState signature to match interface
Fix disconnect signature to match interface

Recommended Changes:

Add test coverage for the new functionality
Consider adding error logging for better debugging

Overall, this is a solid implementation that follows existing patterns well. The required fixes are minor TypeScript signature issues that should be straightforward to address.

pkg-pr-new · 2025-11-09T23:05:03Z

More templates

@rivetkit/actor

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

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3425

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3425

commit: d1c3287

claude · 2025-11-12T17:16:30Z

PR Review - Raw HTTP Connection Driver

I've reviewed PR #3425 which adds connection tracking for raw HTTP requests. Overall, this is a solid addition that brings consistency to how RivetKit handles different connection types. Here's my detailed feedback:

✅ Strengths

Consistent Pattern: The new createRawHttpSocket() driver follows the same pattern as existing drivers (createHttpSocket(), createRawWebSocketSocket()), which promotes code maintainability.
Proper Connection Lifecycle: The handleRawHttpHandler() correctly:
- Creates a connection before handling the request
- Tracks the connection during the request lifecycle
- Properly cleans up in the finally block, even if errors occur
Code Simplification: The refactoring in router.ts removes unnecessary inline logic and delegates to the centralized handler function, making the router cleaner.
Proper Error Handling: The finally block ensures connection cleanup happens regardless of whether the request succeeds or fails.

🔍 Observations & Suggestions

1. Connection State Handling

The new driver doesn't accept parameters like the WebSocket version does (requestId, requestIdBuf, hibernatable). This is consistent with createHttpSocket(), but worth noting:

// New driver
export function createRawHttpSocket(): ConnDriver {
    return {
        requestId: crypto.randomUUID(),
        requestIdBuf: undefined,
        hibernatable: false,
        // ...
    };
}

Question: Are raw HTTP connections intentionally non-hibernatable, or might there be future use cases for hibernatable HTTP connections? If hibernation is never needed for HTTP, consider adding a comment explaining why.

2. Connection Parameters in `handleRawHttpHandler`

In handleRawHttpHandler(), the connection is created with empty params:

const conn = await actor.createConn(createRawHttpSocket(), {}, req);

This differs from the WebSocket handler which uses getRequestConnParams(). Is this intentional?

Recommendation: If connection params should be extracted from the request headers (like WebSockets do), consider:

const connParams = getRequestConnParams(new HonoRequest(req));
const conn = await actor.createConn(createRawHttpSocket(), connParams ?? {}, req);

3. Consistent Cleanup Pattern

The cleanup logic is correct but slightly differs from the WebSocket handler pattern:

HTTP (this PR):

finally {
    if (createdConn) {
        actor.connDisconnected(createdConn, true);
    }
}

WebSocket (existing):

onClose: (evt: any, ws: any) => {
    if (createdConn) {
        const wasClean = evt?.wasClean || evt?.code === 1000;
        actor.connDisconnected(createdConn, wasClean);
    }
}

HTTP connections are always marked as clean (true), which makes sense for synchronous HTTP requests. This is correct but might benefit from a comment explaining why HTTP connections are always considered clean disconnects.

4. Path Handling in Router

The URL path transformation looks correct:

const originalPath = url.pathname.replace(/^\/request/, "") || "/";

However, there's a TODO comment about this not being clean:

// TODO: This is not a clean way of doing this since /http/ might exist mid-path

The comment references /http/ but the code now uses /request. Should this TODO be updated or removed?

5. Missing `getConnectionReadyState` Implementation

The driver returns a fixed state:

getConnectionReadyState: (): DriverReadyState | undefined => {
    return DriverReadyState.OPEN;
}

This matches createHttpSocket() which also has a TODO about correctness. For HTTP requests, this makes sense since the connection exists only during the request lifecycle. Consider if this needs any adjustment.

🧪 Testing Recommendations

This PR would benefit from tests covering:

Connection lifecycle: Verify connections are created and cleaned up properly
Error handling: Ensure cleanup happens when handleRawRequest throws
Path handling: Test that paths are correctly stripped and passed through
Connection tracking: Verify the connection appears in the actor's connection list during the request

Example test pattern (following driver-memory.test.ts structure):

// Test that raw HTTP requests create and cleanup connections properly
test('raw HTTP connection lifecycle', async () => {
    // Create actor with onRequest handler
    // Make HTTP request
    // Verify connection was created and then cleaned up
});

🎯 Minor Code Quality Notes

Comment in raw-http.ts: The JSDoc comment is clear and explains the driver's purpose well. ✓
Import organization: New imports are properly added at the top. ✓
Variable naming: createdConn clearly indicates its purpose. ✓

📋 Summary

This is a well-implemented feature that follows existing patterns and improves consistency in the codebase. The main items to address are:

Clarify if connection params should be extracted from request headers
Update or remove the outdated TODO comment about path handling
Add tests to verify the connection lifecycle works correctly
Consider adding explanatory comments about hibernation and clean disconnect behavior

The code is functionally correct and ready to merge after addressing these clarifications. Great work maintaining consistency with existing patterns!

Verdict: ✅ Approve with minor suggestions

claude · 2025-11-13T01:48:06Z

PR Review: Add Raw HTTP Connection Driver

Overall Assessment: This PR adds connection lifecycle management for raw HTTP requests. The approach is sound but needs interface compliance fixes and better documentation.

Critical Issues:

Connection Lifecycle Pattern Inconsistency
The new handleRawHttpHandler creates connections, but the old code called actor.handleRawRequest directly. Why is connection tracking needed now? How does this relate to the existing #activeHonoHttpRequests counter (actor/instance/mod.ts:107,436-449)?

Recommendation: Document why connection tracking is necessary for raw HTTP requests.

Inconsistent Driver Implementation
The getConnectionReadyState in raw-http.ts (line 20-23) is missing required parameters. The ConnDriver interface (conn/driver.ts:41-44) expects actor and conn parameters.

Recommendation: Add _actor and _conn parameters to match the interface.

Missing Disconnect Parameters
The disconnect function (line 16-18) is missing required parameters from the ConnDriver interface.

Recommendation: Add _actor, _conn, and _reason parameters.

Design Questions:

Connection Cleanup Timing
The finally block cleans up immediately after handleRawRequest returns. This might be problematic for streaming responses, large downloads, or SSE.

Recommendation: Add tests for streaming responses or document why immediate cleanup is safe.

Double Connection Tracking
Two mechanisms exist: Connection Manager and #activeHonoHttpRequests counter. How do they relate?

Recommendation: Clarify when each tracking mechanism should be used.

Good Points:

Comprehensive test coverage
Consistent pattern with WebSocket handler
Clean error handling with try-finally

Must Fix:

Fix getConnectionReadyState signature
Fix disconnect signature

Should Address:

Document why connection tracking is needed
Clarify relationship with #activeHonoHttpRequests
Add streaming response tests or documentation

Performance Note: Connection object overhead per request could impact high-throughput endpoints. Consider benchmarking.

claude · 2025-11-13T02:56:00Z

Code Review: PR #3425 - Add Connection for Raw HTTP

Overview

This PR adds a new createRawHttpSocket driver for managing raw HTTP connection lifecycle tracking, and creates a dedicated handleRawHttpHandler function to properly track HTTP requests through the actor system.

✅ Positive Observations

Good Pattern Consistency: The new raw-http.ts driver follows the same structure as the existing http.ts and raw-websocket.ts drivers, maintaining codebase consistency.
Proper Connection Lifecycle: The PR correctly implements connection creation and cleanup with createConn and connDisconnected, which is essential for proper resource management.
Documentation: The JSDoc comment in raw-http.ts clearly explains the purpose and how it differs from the standard HTTP driver.
Clean Refactoring: Moving the HTTP handling logic from inline in the router to a dedicated handleRawHttpHandler function improves code organization and follows the pattern established by handleRawWebSocketHandler.

🔍 Issues & Concerns

1. Missing Connection Cleanup on Error Path ⚠️ CRITICAL

Location: rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:548-572

The handleRawHttpHandler function has a potential resource leak. If actor.createConn throws an exception, the createdConn variable will remain undefined, but if it succeeds and then actor.handleRawRequest throws, the finally block will still clean up properly. However, there's a subtle issue:

try {
    const conn = await actor.createConn(createRawHttpSocket(), {}, req);
    createdConn = conn;
    return await actor.handleRawRequest(req, {});
} finally {
    if (createdConn) {
        actor.connDisconnected(createdConn, true);
    }
}

Issue: If handleRawRequest throws, the error propagates correctly, but we should verify that connDisconnected doesn't throw. If it does, the original error would be masked.

Recommendation: Consider wrapping the cleanup in a try-catch:

} finally {
    if (createdConn) {
        try {
            actor.connDisconnected(createdConn, true);
        } catch (cleanupError) {
            // Log cleanup error but don't mask the original error
            actor.rLog.error({ msg: "error during connection cleanup", error: cleanupError });
        }
    }
}

2. Inconsistent Driver Implementation

Location: rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/raw-http.ts:15-24

The getConnectionReadyState function signature differs from the interface:

getConnectionReadyState: (): DriverReadyState | undefined => {
    return DriverReadyState.OPEN;
}

But the ConnDriver interface expects:

getConnectionReadyState(
    actor: AnyActorInstance,
    conn: AnyConn,
): DriverReadyState | undefined;

Recommendation: Update to match the interface signature (even if parameters are unused):

getConnectionReadyState: (_actor, _conn): DriverReadyState | undefined => {
    return DriverReadyState.OPEN;
}

This matches the pattern used in http.ts:10.

3. Potential State Management Issue

Location: rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/raw-http.ts:12-13

requestId: crypto.randomUUID(),
requestIdBuf: undefined,

For HTTP connections, requestIdBuf is always undefined. This is consistent with http.ts, but it's worth noting that:

The connection is NOT hibernatable (which is correct for HTTP)
The requestId is randomly generated each time

Question: Should the requestIdBuf be derived from requestId for consistency? Looking at the codebase, it seems HTTP connections intentionally don't support hibernation, so this is fine. But consider adding a comment explaining why requestIdBuf is undefined.

4. Missing Error Context

Location: rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:548-572

The function doesn't add any logging for connection creation/cleanup. Compare this to the WebSocket handler pattern around line 270-296 which includes detailed logging:

if (event.wasClean) {
    actor.rLog.info({
        msg: "websocket closed",
        code: event.code,
        reason: event.reason,
        wasClean: event.wasClean,
    });
}

Recommendation: Add structured logging for debugging:

export async function handleRawHttpHandler(
    req: Request,
    actorDriver: ActorDriver,
    actorId: string,
): Promise<Response> {
    const actor = await actorDriver.loadActor(actorId);
    let createdConn: AnyConn | undefined;

    try {
        actor.rLog.debug({ msg: "creating raw http connection" });
        const conn = await actor.createConn(createRawHttpSocket(), {}, req);
        createdConn = conn;

        return await actor.handleRawRequest(req, {});
    } finally {
        if (createdConn) {
            actor.rLog.debug({ msg: "cleaning up raw http connection", connId: createdConn.id });
            actor.connDisconnected(createdConn, true);
        }
    }
}

🧪 Test Coverage

Missing: There don't appear to be any tests added for the new functionality. The PR should include:

Test that raw HTTP connections are properly created and cleaned up
Test that the connection appears in the actor's connection list during the request
Test that connections are cleaned up even when handleRawRequest throws
Test that the connection lifecycle integrates correctly with the actor system

Recommendation: Look at existing tests in the test files (e.g., driver-engine.test.ts) for patterns on how to test connection lifecycle.

🔒 Security Considerations

✅ No obvious security issues. The connection tracking is internal to the actor system and doesn't expose any new attack surface.

⚡ Performance Considerations

Connection Overhead: Creating a connection object for every HTTP request adds overhead. This is intentional for lifecycle tracking, but worth noting.
UUID Generation: crypto.randomUUID() is called for every HTTP request. This is likely fine for most workloads, but if you're dealing with very high-frequency requests, consider if you need the full UUID entropy or if a simpler ID generation would suffice.

📝 Additional Recommendations

Type Safety: Consider adding a type for the empty object literal {} used as params:

const conn = await actor.createConn(createRawHttpSocket(), {} as Record<never, never>, req);

Consistency with Actions: The handleAction function (line 319-367) also uses createHttpSocket() for HTTP-based actions. Should raw HTTP requests use a different driver than actions? If they should be tracked differently, the current approach is correct. If not, consider documenting why they differ.
Path Handling: The router change at line 178 changes the path stripping from /raw/http to /request. Verify this is intentional and doesn't break existing functionality.

Summary

Verdict: ✅ Approve with minor changes recommended

The PR implements a clean solution for tracking raw HTTP connection lifecycle. The main concerns are:

Missing error handling in cleanup (recommended but not critical)
Minor signature inconsistency in the driver
Missing test coverage (should be addressed before merge)
Could benefit from structured logging

The code follows established patterns well and the architecture decision to track HTTP connections through the actor system makes sense for observability and lifecycle management.

claude · 2025-11-13T02:56:57Z

PR Review: Add Connection for Raw HTTP

Summary

This PR adds connection lifecycle management for raw HTTP requests by creating a createRawHttpSocket driver and handleRawHttpHandler function. This ensures HTTP requests are properly tracked through the actor's connection system, similar to how WebSocket connections are managed.

Code Quality & Best Practices ✅

Positive aspects:

Follows existing patterns well - mirrors the raw-websocket.ts driver structure
Clean separation of concerns - driver creation separate from handler logic
Good code comments explaining purpose and differences from standard HTTP driver
Consistent with TypeScript/RivetKit coding conventions

Issues & Concerns

1. Missing connection cleanup on error path ⚠️

Location: rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:548-572

export async function handleRawHttpHandler(
	req: Request,
	actorDriver: ActorDriver,
	actorId: string,
): Promise<Response> {
	const actor = await actorDriver.loadActor(actorId);

	// Track connection outside of scope for cleanup
	let createdConn: AnyConn | undefined;

	try {
		const conn = await actor.createConn(createRawHttpSocket(), {}, req);

		createdConn = conn;

		return await actor.handleRawRequest(req, {});
	} finally {
		// Clean up the connection after the request completes
		if (createdConn) {
			actor.connDisconnected(createdConn, true);
		}
	}
}

Issue: If actor.createConn() throws an error, createdConn will be undefined, but a connection might have been partially created and tracked in the actor's connection manager. Looking at connection-manager.ts:267-335, the #createNewConn method adds the connection to this.#connections and this.#changedConnections before calling lifecycle hooks. If onConnect throws an error, the connection is added to the maps but never cleaned up.

Recommendation: Either:

Wrap the actor.connDisconnected() call in a try-catch to handle cases where the connection doesn't exist
Check if the connection was successfully added to the actor's connection map before calling connDisconnected

2. Inconsistent parameter signature 🤔

Location: rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/raw-http.ts:10-26

The createRawHttpSocket() function takes no parameters, while the similar createRawWebSocketSocket() in raw-websocket.ts:14-52 takes:

requestId: string
requestIdBuf: ArrayBuffer | undefined
hibernatable: boolean
websocket: UniversalWebSocket
closePromise: Promise<void>

Looking at createHttpSocket() in http.ts, it also takes no parameters and generates a random requestId. However, the inconsistency raises a question: Should raw HTTP connections support hibernation?

Context from the code:

From connection-manager.ts:78-91, the createConn method checks driver.requestIdBuf && driver.hibernatable to determine if a connection can be reconnected after hibernation
Raw HTTP connections currently set hibernatable: false and requestIdBuf: undefined
The handleRawHttpHandler doesn't extract rivetRequestId from the request event like handleRawWebSocketHandler does at line 398

Question: Is this intentional? HTTP requests are typically stateless and short-lived, so not supporting hibernation makes sense. However, it would be good to document this decision explicitly.

3. Missing `terminate` method ℹ️

Location: rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/raw-http.ts:10-26

The createRawHttpSocket driver doesn't implement the optional terminate() method from the ConnDriver interface (see driver.ts:35). While this is optional, the raw-websocket.ts driver implements it at lines 41-43:

terminate: () => {
	(websocket as any).terminate?.();
},

Recommendation: While HTTP connections don't typically need forceful termination like WebSockets, consider adding a comment explaining why terminate is not implemented, or add a no-op implementation for consistency.

4. Async return without await ⚠️

Location: rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/raw-http.ts:17-19

disconnect: async () => {
	// Noop
},

The disconnect method is marked async but doesn't await anything. While this is technically correct (returns Promise<void>), it's inconsistent with the pattern in http.ts:14-17 which is also a noop but uses the same pattern.

Recommendation: This is a minor style issue. Consider being consistent across drivers - either all use async () => {} or all use () => Promise.resolve() for no-op disconnects.

Performance Considerations ✅

Good:

Connection tracking overhead is minimal - just map operations
No unnecessary async operations
Proper cleanup in finally block ensures resources are released

Note: The connection lifecycle adds a small amount of overhead to each HTTP request (creating/tracking/cleaning connection objects), but this is necessary for consistency with the actor model and enabling features like connection tracking, onConnect/onDisconnect hooks, and potential future hibernation support.

Security Concerns ✅

No security issues identified. The PR doesn't introduce any new security vectors:

Uses existing actor authentication/authorization mechanisms
No user input handling changes
Connection cleanup prevents resource leaks

Test Coverage ✅

Looking at driver-test-suite/tests/raw-http.ts, the existing tests extensively cover raw HTTP functionality:

Basic GET/POST requests ✅
State tracking across requests ✅
Header handling ✅
Error cases (404, void return, missing handler) ✅
Multiple HTTP methods ✅
Binary data ✅
Hono router integration ✅

Recommendation: Consider adding a test that specifically validates connection lifecycle:

Verify onConnect is called when HTTP request starts
Verify onDisconnect is called when HTTP request completes
Verify connection appears in actor.connections during request
Verify connection is removed after request completes

This would ensure the connection tracking added by this PR works correctly.

Additional Notes

Documentation: The inline comments are good, but consider adding a note in the module documentation about when to use raw HTTP vs the standard protocol-based approach.

Consistency with router changes: The refactoring in router.ts:173-201 that extracts the HTTP handler logic to handleRawHttpHandler is a good architectural improvement - it makes the router cleaner and puts connection management logic in the endpoints file where it belongs.

Summary

This is a solid PR that properly extends the connection management system to raw HTTP requests. The main concerns are:

Critical: Error handling in connection cleanup (should be addressed)
Minor: Missing documentation about hibernation support decision
Nice-to-have: Test coverage for connection lifecycle hooks

The code follows existing patterns well and the architectural changes improve code organization. With the error handling fix, this would be ready to merge.

NathanFlurry mentioned this pull request Nov 9, 2025

chore(rivetkit): split ActorInstance logic in to multiple classes #3421

Open

This was referenced Nov 9, 2025

chore(rivetkit): rename onFetch to onRequest #3424

Open

chore(rivetkit): add conn type #3426

Open

vercel bot deployed to Preview – rivetkit-serverless November 9, 2025 23:04 View deployment

This was referenced Nov 10, 2025

feat(site): add typedoc docs #3446

Open

chore(rivetkit): remove deprecated packages #3447

Open

NathanFlurry requested a review from MasterPtato November 11, 2025 00:11

jog1t force-pushed the 11-09-chore_rivetkit_add_connection_for_raw_http branch from 30eb6e5 to ec5e52d Compare November 12, 2025 17:06

vercel bot deployed to Preview – rivetkit-serverless November 12, 2025 17:07 View deployment

jog1t mentioned this pull request Nov 12, 2025

fix(fe): use publishable token for integrating next.js with other services #3451

Draft

jog1t mentioned this pull request Nov 12, 2025

refactor: performance improvements for inspector #3452

Draft

NathanFlurry force-pushed the 11-09-chore_rivetkit_rename_onfetch_to_onrequest_ branch from 5bab9a4 to 5bf1455 Compare November 13, 2025 01:31

NathanFlurry force-pushed the 11-09-chore_rivetkit_add_connection_for_raw_http branch from ec5e52d to 2c681e1 Compare November 13, 2025 01:31

vercel bot deployed to Preview – rivetkit-serverless November 13, 2025 01:32 View deployment

chore(rivetkit): add connection for raw http

d1c3287

NathanFlurry force-pushed the 11-09-chore_rivetkit_add_connection_for_raw_http branch from 2c681e1 to d1c3287 Compare November 13, 2025 02:47

NathanFlurry force-pushed the 11-09-chore_rivetkit_rename_onfetch_to_onrequest_ branch from 5bf1455 to fc159fd Compare November 13, 2025 02:47

vercel bot deployed to Preview – rivetkit-serverless November 13, 2025 02:48 View deployment

This was referenced Nov 13, 2025

chore(rivetkit-typescript): remove dependency on node modules #3460

Open

chore(rivetkit): switch dynamic node imports to use require #3461

Open

chore(rivetkit): fix type checks #3462

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore(rivetkit): add connection for raw http #3425

chore(rivetkit): add connection for raw http #3425

Uh oh!

NathanFlurry commented Nov 9, 2025

Uh oh!

vercel bot commented Nov 9, 2025 •

edited

Loading

Uh oh!

NathanFlurry commented Nov 9, 2025 •

edited by jog1t

Loading

Uh oh!

claude bot commented Nov 9, 2025

Uh oh!

pkg-pr-new bot commented Nov 9, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 12, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

claude bot commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chore(rivetkit): add connection for raw http #3425

Are you sure you want to change the base?

chore(rivetkit): add connection for raw http #3425

Uh oh!

Conversation

NathanFlurry commented Nov 9, 2025

Uh oh!

vercel bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NathanFlurry commented Nov 9, 2025 • edited by jog1t Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 9, 2025

PR Review: Add connection for raw HTTP

Summary

Code Quality & Best Practices

1. Connection Lifecycle Inconsistency (rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:548-572)

2. Disconnect Signature Inconsistency (rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/raw-http.ts:16-18)

3. Missing Error Handling Context (rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:560-571)

Performance Considerations

Security Concerns

Test Coverage

Additional Notes

Summary

Uh oh!

pkg-pr-new bot commented Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude bot commented Nov 12, 2025

PR Review - Raw HTTP Connection Driver

✅ Strengths

🔍 Observations & Suggestions

1. Connection State Handling

2. Connection Parameters in handleRawHttpHandler

3. Consistent Cleanup Pattern

4. Path Handling in Router

5. Missing getConnectionReadyState Implementation

🧪 Testing Recommendations

🎯 Minor Code Quality Notes

📋 Summary

Uh oh!

claude bot commented Nov 13, 2025

PR Review: Add Raw HTTP Connection Driver

Uh oh!

claude bot commented Nov 13, 2025

Code Review: PR #3425 - Add Connection for Raw HTTP

Overview

✅ Positive Observations

🔍 Issues & Concerns

1. Missing Connection Cleanup on Error Path ⚠️ CRITICAL

2. Inconsistent Driver Implementation

3. Potential State Management Issue

4. Missing Error Context

🧪 Test Coverage

🔒 Security Considerations

⚡ Performance Considerations

📝 Additional Recommendations

Summary

Uh oh!

claude bot commented Nov 13, 2025

PR Review: Add Connection for Raw HTTP

Summary

Code Quality & Best Practices ✅

Issues & Concerns

1. Missing connection cleanup on error path ⚠️

2. Inconsistent parameter signature 🤔

3. Missing terminate method ℹ️

4. Async return without await ⚠️

Performance Considerations ✅

Security Concerns ✅

Test Coverage ✅

Additional Notes

Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

vercel bot commented Nov 9, 2025 •

edited

Loading

NathanFlurry commented Nov 9, 2025 •

edited by jog1t

Loading

pkg-pr-new bot commented Nov 9, 2025 •

edited

Loading

2. Connection Parameters in `handleRawHttpHandler`

5. Missing `getConnectionReadyState` Implementation

3. Missing `terminate` method ℹ️