Skip to content

fix(cluster): honor fetch cancellation during TCP/TLS connection setup #901

Description

@xe-nvdk

Problem

Found during the deep review of #899, which fixes #796's unbounded acknowledgement/body reads. Cancellation during connection establishment remains unhandled; this predates #899 and is a separate, bounded follow-up.

FetchClient.Fetch checks ctx.Err() and limits the dial timeout to the context's remaining deadline, but calls security.Dial without passing the context. It installs the context.AfterFunc connection-close hook only after dialing succeeds:

security.Dial uses net.DialTimeout for TCP and tls.DialWithDialer for TLS. Neither receives the fetch context. In particular, tls.DialWithDialer uses a background context internally, so cancelling the caller's context cannot interrupt a stalled TLS handshake.

Impact

A cancelled fetch can retain a pull worker until connection establishment times out. Since Puller.Stop cancels its context and joins its workers, this can also delay shutdown. The production dial timeout is ten seconds, shortened by an earlier fetch deadline: this is not the indefinite body-read stall fixed in #899.

Reproduction

Reproduced under the race detector against PR #899:

  1. Start a plain TCP listener that accepts the connection and reads the client's TLS ClientHello, but never sends a TLS response.
  2. Configure FetchClient with TLS and a two-second dial timeout. Start Fetch in a goroutine using an otherwise valid file entry.
  3. Wait until the peer has received ClientHello, then cancel the fetch context.
  4. Assert that Fetch promptly returns an error matching context.Canceled.

The fetch remained blocked beyond the 500 ms observation window in all six repeated cases: three with cancellation only, and three with cancellation before a three-second context deadline. Closing the peer manually released the call with a connection-reset error rather than context.Canceled.

Suggested fix

Make connection establishment context-aware while retaining the configured dial timeout and existing TLS configuration/server-name behavior. Use net.Dialer.DialContext for plain TCP and tls.Dialer.DialContext for TLS, either locally in the fetch client or through a scoped security.DialContext helper. Preserve the existing post-connect cancellation hook for request, acknowledgement and body I/O.

Acceptance criteria

  • A stalled TLS handshake stops promptly when the fetch context is cancelled, with or without a deadline, and returns an error matching context.Canceled.
  • TCP dialing observes cancellation; a pre-cancelled context does not start a connection.
  • The earlier of the context deadline and configured dial timeout still bounds connection establishment.
  • Successful TLS fetches, certificate verification, hostname handling, body cancellation and partial-file resume remain covered and passing.
  • Tests synchronize on the connection/handshake phase and use bounded cleanup so failures do not hang the suite.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions