fix(exchange): apply ExchangeTimeout to key exchange steps 5 and 7

[pylakey]

What happens

A peer that completes the TCP handshake, answers key exchange steps 1-4, and then stops responding parks the client forever. No timeout fires, and cancelling the context has no effect.

Production goroutine dump (v0.159.0, PFS enabled), two goroutines parked 27 and 21 minutes:

internal/poll.runtime_pollWait
  net.(*conn).Read
    mtproxy/obfuscated2.(*Obfuscated2).Read
      proto/codec.readAbridged
        transport.(*connection).Recv
          exchange.ClientExchange.Run

There is no unencryptedWriter frame between ClientExchange.Run and connection.Recv, which localizes both to a direct conn.Recv call — steps 5 or 7 — and rules out step 2, which carries the ExchangeTimeout bound. One goroutine was the primary connection (telegram.Client.Run's errgroup), the other a sub-DC pool connection (pool → tdsync.Supervisor). Both block their owners indefinitely: telegram.Client.Run never returns, and pool.DC.Close is cancel() + grp.Wait() with no timeout, so the deferred sub-connection teardown cannot finish either. The connection is never recreated.

The caller cancelled its run context every 55 seconds — thousands of times — with no effect, because cancellation cannot interrupt a parked read on a deadline-less socket.

Why

Exchanger.WithTimeout documents itself as setting the deadline of "every exchange request" (exchange/flow.go:62), and mtproto.Options.ExchangeTimeout as the timeout of "every key exchange request" (mtproto/options.go:50-51). That bound is applied by unencryptedWriter.tryRead (exchange/proto.go:44).

The server flow uses it for all three of its reads (server_flow.go:130, :166, :267). The client flow uses it for every write and for the step 2 read (client_flow.go:34), then reads steps 5 and 7 through a bare conn.Recv (client_flow.go:146, :241). Those are the only two non-test conn.Recv calls in the module outside tryRead itself and the steady-state read loop in mtproto/read.go.

Below the exchange, transport.(*connection).Recv derives its socket deadline solely from ctx.Deadline() (transport/connection.go:63) and clears any previous one at :60. With no deadline it sets none, and codec.Read parks.

Nothing else can rescue the socket during this window: handleClose — the mechanism that closes the connection on context cancellation — is started at mtproto/conn.go:227, after connect() returns at :221. So during the key exchange there is neither a deadline nor a closer.

Likely cause of the original slip: readUnencrypted decodes into a bin.Decoder, while steps 5 and 7 need the raw buffer — so conn.Recv was reached for when tryRead, which reads raw and applies the timeout, was the right one of the two.

Why PFS makes it fatal

mtproto.Conn.connect chooses its timeout policy by mode:

• non-PFS: connectCtx carries DialTimeout across dial and exchange (connect.go:17-22), so the bare reads inherit a deadline by accident and cannot park forever.
• PFS: DialTimeout is scoped to the dial alone and connectPFS(ctx) receives the caller's raw context (connect.go:26-34, :47-48).

Two conditions compose; remove either and the wedge disappears. The PFS reasoning is sound — temporary plus permanent key generation legitimately takes longer than a dial — so this PR does not touch it. It makes the documented per-read bound actually apply, after which both modes are bounded by the same mechanism and the PFS policy is harmless.

EnablePFS is opt-in and off by default, which is why this has gone unnoticed.

The change

Two lines: c.conn.Recv → c.tryRead at both sites, using the package's existing unexported helper.

• No exported API change (verified: go doc -all ./exchange is identical before and after, 36 declarations).
• No new mechanism, option, goroutine or error value.
• Cannot break an errors.Is check: in the affected configuration no error was previously observable — the call hung.
• Non-PFS users are unaffected in practice; their reads already carried DialTimeout.
• Nothing is closed, only a deadline is set. If it fires, connect's existing multierr.AppendInto(&rErr, conn.Close()) discards the socket, so there is no risk of reusing a connection with a half-consumed frame.

Test

exchange/client_flow_timeout_test.go drives the real server flow over transport.Intermediate.Pipe() with a wrapper whose Send succeeds normally and whose Recv goes silent after N replies — reproducing a peer that accepts our writes and then stops answering. Allowing 1 reply pins the step 5 read, allowing 2 pins step 7, so a regression at either site fails its own case.

On main both subtests hang to their 30s guard; with this change both pass in about a second. The two commits are ordered so this is visible in the PR's own history: the test lands first and fails, the fix follows.

The caller context is deliberately context.Background() — the point is that a context with no deadline of its own must still be bounded by the configured ExchangeTimeout.

Note on the error shape: the test's fake conn returns ctx.Err(), so the assertion is context.DeadlineExceeded. Over a real socket the deadline surfaces as a *net.OpError wrapping os.ErrDeadlineExceeded instead. Either way telegram.Client.isPermanentError does not match it, so the reconnect loop retries — the desired behaviour.

Not in this PR

Deliberately excluded, each a separate concern: a cancellation watcher for the connect phase, an idle watchdog for connections whose peer goes silent in steady state, context-aware transport locks, and rpc acknowledgement semantics.

This supersedes #1830, which bundled all of the above and which I am closing. Its blast radius — +2840/-105 across 29 files, six independent changes, rewriting transport/connection.go and mtproto/conn.go — made it unreviewable as a unit, with no partial-accept path. That was my mistake in packaging. The exchange timeout is the only part required to stop the permanent wedge, so it goes first and alone.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.46%. Comparing base (089a561) to head (0fccd5c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1838      +/-   ##
==========================================
+ Coverage   71.35%   71.46%   +0.10%     
==========================================
  Files         509      509              
  Lines       24137    24137              
==========================================
+ Hits        17224    17249      +25     
+ Misses       5658     5649       -9     
+ Partials     1255     1239      -16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.