Skip to content

fix(rivetkit): remove the connection of an aborted HTTP action - #5836

Merged
eersnington merged 1 commit into
stack/fix-rivetkit-deliver-events-sent-to-hibernated-connections-before-they-reconnect-utyzksuofrom
stack/fix-rivetkit-remove-the-connection-of-an-aborted-http-action-kukspqnv
Oct 5, 2026
Merged

eersnington merged 1 commit into
stack/fix-rivetkit-deliver-events-sent-to-hibernated-connections-before-they-reconnect-utyzksuofrom
stack/fix-rivetkit-remove-the-connection-of-an-aborted-http-action-kukspqnv

Conversation

@eersnington

@eersnington eersnington commented Oct 5, 2026 •

Copy link
Copy Markdown
Member

A caller that left in the middle of a request kept the actor awake forever. The request's connection was never removed, so the actor always had an active connection and never idle-slept.

handle_action_fetch (registry/http.rs)
  connect_conn_with_request
    insert_existing(conn)      connection registered
    await onConnect            caller leaves here: leaked
  await dispatch_action        caller leaves here: leaked
  conn.disconnect()            skipped when the future is dropped

Gateway3 sends ToEnvoyRequestAbort when a client disconnects before the response starts, and the envoy drops the request future at whichever await it is on. Everything after that await is skipped.

  • New DisconnectOnDrop guard in actor/connection.rs. It disconnects the connection when dropped, unless it was disconnected or disarmed first.
  • connect_with_state_and_prepare holds it across the onConnect await, so every new connection (HTTP or WebSocket) is covered while onConnect runs.
  • The HTTP action and queue-send handlers hold it for the request and disconnect through it. It replaces the explicit conn.disconnect.
  • The action itself still runs to completion. An in-flight action already keeps the actor awake until it settles. Only the connection is cleaned up.
  • onDisconnect runs after onConnect, because both go through the actor's event inbox in order.
  • Only Gateway3 forwards client aborts, so the driver cases run with engine.gateway3.

This is part 2 of 2 in a stack:

@claude

claude Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review. The approach is sound. A drop guard that disconnects when the caller's future is cancelled fixes the leaked connection and the never-sleeps bug. The driver tests cover both scenarios.

Suggestions

  1. Missing Rust-level coverage. The only tests are the Gateway3 driver tests (native and bare only). DisconnectOnDrop has no unit test. Add one under rivetkit-core/tests/ checking that dropping the guard removes the connection and that disarm() and disconnect() do not double-disconnect.
  2. Drop spawns without a runtime check (actor/connection.rs). RuntimeSpawner::spawn from Drop may panic during runtime teardown or with no runtime, and a panic in Drop during unwinding aborts. Please confirm it is safe.
  3. The spawned disconnect is detached and untracked. If the actor is shutting down when the guard drops, conn.disconnect can race shutdown. Consider tracking it with the in-flight tasks, or confirm disconnect is safe on a stopped actor.
  4. "The action still runs to completion" needs a check. In http.rs the guard lives in the same future as with_action_dispatch_timeout(...). If that future is dropped, the dispatch is dropped too unless the action is spawned elsewhere. If it is spawned, the connection is disconnected while the action runs. Please confirm the action's own keep-awake accounting blocks sleep until it finishes, and add a comment.
  5. Redundant disarm() calls in connection.rs. Disarm once, right after the emit_connection_open await returns.
  6. Test timing. waitFor(SLEEP_TIMEOUT + 250) is a fixed wait. That is acceptable for sleep tests, since actions must not be polled. A comment on why would match the repo's no-bumped-waits rule.
  7. Fixture nit. releaseConnect silently no-ops if onConnect has not run yet. An explicit error would fail faster on a race.

Security and performance. No concerns. The guard adds negligible overhead on the normal path.

Overall this looks good. The main requests are a Rust unit test and confirmation of items 2 to 4.

@the-company-company the-company-company Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 1 medium-severity finding

Reviewed commit a453705.

Comment thread rivetkit-rust/packages/rivetkit-core/src/registry/http.rs Outdated
@eersnington
eersnington force-pushed the stack/fix-rivetkit-deliver-events-sent-to-hibernated-connections-before-they-reconnect-utyzksuo branch from 061ce65 to 775b0a7 Compare October 5, 2026 09:53
@eersnington
eersnington force-pushed the stack/fix-rivetkit-remove-the-connection-of-an-aborted-http-action-kukspqnv branch from a453705 to dc02d69 Compare October 5, 2026 09:53
@eersnington
eersnington merged commit 02fd3b3 into stack/fix-rivetkit-deliver-events-sent-to-hibernated-connections-before-they-reconnect-utyzksuo Oct 5, 2026
8 of 22 checks passed
@eersnington
eersnington deleted the stack/fix-rivetkit-remove-the-connection-of-an-aborted-http-action-kukspqnv branch October 5, 2026 11:51
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