Skip to content

Keep raw inbound handshakes alive during idle shutdown - #464

Open
Officeyutong wants to merge 1 commit into
nervosnetwork:masterfrom
Officeyutong:fix-raw-inbound-session-idle-shutdown
Open

Keep raw inbound handshakes alive during idle shutdown#464
Officeyutong wants to merge 1 commit into
nervosnetwork:masterfrom
Officeyutong:fix-raw-inbound-session-idle-shutdown

Conversation

@Officeyutong

Copy link
Copy Markdown
Collaborator

Summary

Fix an idle-shutdown race for inbound raw sessions.

RawSessionInfo::Inbound previously started a handshake without registering pending work in the service state. For services running with no listener, no active sessions, and forever(false), the main loop could treat the service as idle and shut down before the inbound raw-session handshake completed.

Changes

  • Register inbound raw-session handshakes as pending work.
  • Add a tracked_state flag to handshake events so only handshakes that incremented the service state release it on success or failure.
  • Keep normal listener inbound handshakes untracked, preserving their existing state behavior.
  • Add regression coverage for inbound raw sessions registering pending work.

Tests

  • cargo fmt --check
  • cargo test -p tentacle raw_inbound_session_registers_pending_work -- --nocapture
  • git diff --check

Copilot AI 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.

Pull request overview

This PR fixes an idle-shutdown race by ensuring inbound raw session handshakes are counted as “pending work” in the service state, preventing the service loop from shutting down before the handshake finishes.

Changes:

  • Adds a tracked_state flag to handshake success/error session events so only handshakes that actually incremented pending-work will decrement it.
  • Marks raw-session handshakes as tracked and increments pending-work for inbound raw sessions (matching existing outbound behavior).
  • Adds a regression test asserting inbound raw sessions register pending work.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tentacle/src/session.rs Extends handshake events with tracked_state to indicate whether they should release pending-work state.
tentacle/src/service/helper.rs Plumbs tracked_state through HandshakeContext into emitted handshake events; keeps listener inbound handshakes untracked.
tentacle/src/service.rs Increments state for inbound raw sessions; decrements on handshake completion only when tracked_state is true; adds regression test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@chenyukang

Copy link
Copy Markdown

[P2] Process queued raw sessions before the idle-shutdown check

The new regression test calls InnerService::handle_service_task directly, so it bypasses the initial idle-shutdown check in Service::run. A caller can obtain ServiceAsyncControl, enqueue an inbound raw_session before starting Service::run, and then run the service. With the default forever(false), run observes no listeners, no sessions, and State::Running(0), exits immediately, and drops the queued raw session before the new state.increase() is reached.

I added an end-to-end reproducer through the public control API:

Run:

cargo test -p tentacle --no-default-features --features tokio-runtime,tokio-timer --test test_raw_session_idle_shutdown -- --nocapture

On the exact PR head (8228348), it fails immediately with:

service exited before processing the queued raw session: Canceled

As a control, the same test passes if Service::run consumes an already-ready service_task_receiver item before checking for idle shutdown. Please cover this public queued-task path and either drain ready service tasks before the shutdown check or account for queued work when it is enqueued.

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.

3 participants