Skip to content

fix(rivetkit): hide pending connections during preflight#4969

Merged
NathanFlurry merged 1 commit intomainfrom
conn-preflight/hide-pending-conns
May 5, 2026
Merged

fix(rivetkit): hide pending connections during preflight#4969
NathanFlurry merged 1 commit intomainfrom
conn-preflight/hide-pending-conns

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@NathanFlurry NathanFlurry mentioned this pull request May 5, 2026
11 tasks
Copy link
Copy Markdown
Member Author

NathanFlurry commented May 5, 2026

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

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 5, 2026

Code Review: PR #4969 — fix(rivetkit): hide pending connections during preflight

Overview

This PR introduces a preflight phase in the connection lifecycle. Connections are now hidden from c.conns while onBeforeConnect and createConnState are running. The connection only becomes visible after both succeed, and before onConnect runs. This prevents partially-constructed connections from being observable by other connections or actor logic.

The mechanism is a new ConnectionPreflight event that carries params and handles onBeforeConnect/createConnState, while ConnectionOpen is stripped of params and now solely drives onConnect.


Correctness

Lifecycle reordering in connection.rs is the right approach:

// Before: insert → prepare → open
// After: preflight → insert → open
self.emit_connection_preflight(&conn, params.clone(), request.clone()).await?;
self.insert_existing(conn.clone());
if let Err(error) = self.emit_connection_open(&conn, request).await { ... }

The removal of the remove_existing call in the prepare_connection error path is clean since the connection is no longer inserted before that point.

unreachable! in rivetkit/src/event.rs:

ActorEvent::ConnectionOpen { .. } => {
    unreachable!("ConnectionOpen is handled by Events")
}

This is safe because start.rs intercepts ConnectionOpen in handle_runtime_event/handle_runtime_event_sync and returns None before it ever reaches the user-facing Event::from() code path. However, the message could be clearer — something like "ConnectionOpen is intercepted by start.rs before reaching user event dispatch" would better explain the invariant to a future reader.

Timeout composition in emit_connection_preflight:

let timeout_duration = config
    .on_before_connect_timeout
    .saturating_add(config.create_conn_state_timeout);

This outer timeout is the sum of both inner timeouts (which are enforced individually inside the NAPI/WASM handlers). This is correct: in the worst case both callbacks consume their full budget.


Potential Issues

  1. Timing-based absence assertion in failed_preflight_never_exposes_connection:

    assert!(
        tokio::time::timeout(Duration::from_millis(20), events_rx.recv())
            .await
            .is_err()
    );

    This 20 ms window checks that ConnectionOpen is never emitted after a failed preflight. The correctness argument is sound (once connect_with_state returns an error, the system should have already decided not to send ConnectionOpen), but a 20 ms wall-clock assertion can be flaky under heavy CI load. Consider structuring the test to assert the count of events received rather than relying on a timeout, or dropping the assertion (the event_task completing normally already validates the happy path).

  2. Missing NAPI test for ConnectionOpen:
    napi_actor_events.rs was updated to use ConnectionPreflight for the existing test, but there is no new test for the ConnectionOpen event (which now drives only onConnect). Adding one would close the coverage gap on the NAPI bridge for this split.

  3. params clone:
    params.clone() and request.clone() are both cloned before preflight, then request is moved into ConnectionOpen. This is required and correct; just noting it for awareness.


Style / Convention

  • Comments in config.ts and doc strings follow the convention of complete sentences — looks good.
  • The unreachable! is an explicit named arm (not _ =>), which satisfies the CLAUDE.md enum-exhaustiveness rule.
  • All three runtimes (Rust, NAPI, WASM) are updated consistently, which is required by the layer-constraint rules.
  • Existing tests in task.rs correctly enumerate both ConnectionPreflight and ConnectionOpen arms explicitly, matching project conventions.

Test Coverage

Layer Coverage
Rust core (connection.rs) Two new targeted tests: visibility during preflight, failure never exposes connection
Rust integration (counter.rs) Updated to handle both events
Rust NAPI (napi_actor_events.rs) Updated for preflight; no new test for ConnectionOpen
TypeScript driver Two new driver-level tests covering onBeforeConnect and createConnState visibility
Documentation Updated in connections.mdx and config.ts JSDoc

Summary

The change is logically sound, well-structured, and correctly threads the lifecycle change through all four runtimes. Two callouts worth addressing before merge:

  • The 20 ms absence assertion is the only minor flakiness risk — consider whether it can be made deterministic.
  • Adding a NAPI test for ConnectionOpen (onConnect path) would complete the test matrix.

@NathanFlurry NathanFlurry marked this pull request as ready for review May 5, 2026 11:31
@NathanFlurry NathanFlurry force-pushed the 05-04-wip_more_fuzz_tests branch from cf92296 to c7dee23 Compare May 5, 2026 11:53
@NathanFlurry NathanFlurry force-pushed the conn-preflight/hide-pending-conns branch from 660b761 to 8cd9dea Compare May 5, 2026 11:53
@NathanFlurry NathanFlurry force-pushed the 05-04-wip_more_fuzz_tests branch from c7dee23 to e590e33 Compare May 5, 2026 12:09
@NathanFlurry NathanFlurry force-pushed the conn-preflight/hide-pending-conns branch from 8cd9dea to 0cdeda4 Compare May 5, 2026 12:09
Base automatically changed from 05-04-wip_more_fuzz_tests to main May 5, 2026 14:58
@NathanFlurry NathanFlurry merged commit 0cdeda4 into main May 5, 2026
6 of 13 checks passed
@NathanFlurry NathanFlurry deleted the conn-preflight/hide-pending-conns branch May 5, 2026 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant