feat(mtproto): self-heal wedged and silent transport connections - #1830
feat(mtproto): self-heal wedged and silent transport connections#1830pylakey wants to merge 5 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1830 +/- ##
==========================================
+ Coverage 71.44% 71.83% +0.38%
==========================================
Files 509 509
Lines 24137 24289 +152
==========================================
+ Hits 17245 17448 +203
+ Misses 5650 5594 -56
- Partials 1242 1247 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Keep connection recovery inside the library so a stalled or half-dead socket recovers on its own instead of wedging the mtproto connection or surfacing to callers as a terminal failure. - transport: acquire the read/write I/O locks under the caller context and force deadlines from Close, so a blocked Write can no longer trap the ping loop and Close can no longer block behind an in-flight I/O. - exchange: enforce the existing ExchangeTimeout on the raw reads in key-exchange steps 5 and 7, bounding a handshake that otherwise hangs indefinitely when the peer accepts the TCP connection but never answers. - mtproto: close the connection when the context is cancelled mid key exchange; add an idle watchdog that closes a connection whose reads have gone silent past IdleTimeout, independent of the ping round trip. - mtproto: expose PingDelayDisconnect and IdleTimeout as connection options. PingDelayDisconnect defaults to PingInterval + PingTimeout, preserving the value previously hardcoded on the wire; IdleTimeout defaults to the same and is validated against a PingInterval + PingTimeout floor. - rpc: distinguish an engine-closed-after-ack from caller cancellation via the new ErrEngineClosedAfterAck sentinel, and never resend a request that the server already acknowledged. - telegram: classify transport write failures as retryable on a fresh connection and forward the new ping options.
2ea9a92 to
a3e3c1b
Compare
|
Hello! Won't there be endless reconnection in different cases with unstable proxy connections? I use gotd to connect thousands of accounts with different proxies, and it is important to me that when the socket falls off due to a timeout or any other network problems, there are no unnecessary reconnection requests inside the library, because in these cases I rotate the proxy and reconnect at my service level. Have you tested your changes on a large number of accounts with proxies that have a large number of active chats? I have a highly loaded service based on this library and the cost of error is very high, so I'm worried about such a big diff) Did you write using AI or by yourself? Did you do a review using AI? I took the liberty of running your diff through the AI and this is what it gave out: I found a few regression risks in this commit. 1.
|
…ailures codec.Write mixes local validation with I/O: checkOutgoingMessage and checkAlign reject a message before the socket is touched. Wrapping every codec.Write error in ErrWriteFailed therefore marked an unencodable message as "the frame never reached the server, safe to retry on a new connection". That retry fails identically, and the connection itself stays healthy — so neither the ping loop nor the idle watchdog ever fires. A caller parked in Client.invokeConn waited for a reconnect nothing would trigger, and pool.DC spun marking connections dead and redialling instead. Export ErrInvalidMessageLength and ErrPayloadNotAligned from proto/codec and skip the ErrWriteFailed wrap for them, so only a genuine failure of the link is reported as retryable.
… settings disconnect_delay is sent as an int of seconds, so PingInterval + PingTimeout floors to zero when both are sub-second. The validation caught that, but its correction re-applied the same sum, leaving disconnect_delay=0 on the wire — asking the server to drop the connection immediately. Derive the fallback from the first whole second above PingInterval instead. With the defaults (60s + 15s) the sum already clears it, so the wire value is unchanged at 75s.
|
Fair questions, and two of the four findings were real. Answering the process ones first, since they frame the rest. Was this written/reviewed with AI? Yes, AI-assisted, with the design decisions and the final call mine. Your skepticism is reasonable and it paid off here: your run found a genuine bug I had missed. For what it is worth, the process did catch itself once — an earlier draft of the Have you tested this on many accounts with proxies and many active chats? No, and I would rather say so plainly than imply otherwise. What was tested: targeted fault injection against a single live account (packets blackholed both ways, Will this cause endless reconnection, and does it break your proxy detection? This is the part I took most seriously. Separating what the PR changes from what gotd already did: Already true on What the PR changed for you, and what I did about it: exactly one error class was newly fed into that loop — a transport write failure on the first send, the EPIPE/RST your own write syscall catches. On But the RPC error path was never a dependable proxy signal, including before this PR. Dial failures, blackholes caught by the ping loop, and read-side RSTs all produced On that hook this PR gives you strictly more than
Concretely, what I would suggest: rotate on One thing I should flag rather than let you discover: the idle watchdog currently cannot be turned off — a zero or negative On the four findings — two fixed, two not the mechanism they were reported as. Both fixes are pushed. 3. Codec validation errors misclassified — correct, fixed. This is the real bug, and it is worse than the report suggests. 4. Sub-second ping settings → 1 and 2 — the symptoms are real, the mechanism is not, and fixing 3 resolves both. First, the part of your suggestion that is simply right: all three official clients do destroy the connection on a genuine socket write error. Android does it most directly — Three things about how they do it changed my mind about porting it as-is:
Concretely for gotd: a genuine socket write failure already tears the connection down here, just by a different route — the read half breaks at the same instant, Here is each write-failure shape I checked before deciding not to touch
So the only case where a caller could park on a reconnect that never comes was the codec rejection, where the connection is healthy and the retry is futile by construction. That is finding 3, now fixed at the source. With it fixed, the invariant If you disagree with that reasoning I would genuinely like to see the counter-case — a write-failure shape where the socket survives and the read half stays alive. |
Retrying a request whose transport send failed changes what the caller sees: the error stops surfacing, the call parks until the connection is replaced and is then re-sent. A caller that acts on that error itself — rotating a proxy or an endpoint, say — depends on receiving it. Put it behind Options.RetryOnWriteFailed, and DCOptions.RetryOnWriteFailed for pools, disabled by default, so this change does not alter caller-visible behavior. Requests that were sent but not acknowledged (ErrConnDead, ErrEngineClosed) keep being retried unconditionally, as they already were.
af6db37 to
cdf1b83
Compare
|
Closing this in favour of a series of smaller, independently reviewable PRs. On reflection this PR's blast radius is too large to ask anyone to review as a unit: +2840/-105 across 29 files, six independent changes in six packages, adding five exported option fields and six exported error values. Two of those changes rewrite The findings behind it are real, and I am resubmitting them one at a time, smallest and best-evidenced first. #1838 — Planned next, separately: closing the connection when the context is cancelled during Later, and only if the above are welcome: an idle watchdog for connections whose peer goes silent in steady state. That one does add exported API, so it deserves its own discussion rather than riding along. The remaining pieces here I am keeping in our fork rather than proposing. The context-aware transport locks rewrite the least-churned file in the module, and for the failure they address I have the mechanism by inspection but no captured dump — weaker evidence than I would want for a change that size. The rpc acknowledgement semantics encode a product-specific error taxonomy; that is a design preference rather than a bug fix, and it belongs in a discussion rather than a patch. Thanks for the patience, and apologies for the size of this one. |
Motivation
This work started from a specific, recurring failure on busy connections: a
request whose write hits a saturated socket — the peer's receive window has
closed and stays closed — wedges the entire connection. The socket stays
ESTABLISHED and inbound updates may still arrive, but no request completes, and
nothing recovers it short of destroying and recreating the whole client from
the outside.
Two things make that worse than it should be:
place transport-failure detection outside the write path and recover
connections internally, so a stalled or half-dead socket never surfaces
to the application as a terminal state. gotd does the opposite in a few
places (below).
reconnect loop replaces the primary connection (the work that closed
[Upload Error]: [upload part: send upload part 81 RPC: rpcDoRequest: retryUntilAck: engine forcibly closed: context canceled] #1021 / bug: client can't recover from connection loss #1030). That retry is gated on the reconnect actually firing — and in
exactly these failure modes it never does, so the existing recovery machinery
sits idle while requests block forever. (See "Relationship to the existing
reconnect handling" below.)
Concretely, the current transport has these observable problems:
A blocked write wedges the entire connection.
transport.connection.Sendholds
writeMuxfor the wholecodec.Write, and clears its own deadline,only re-arming one if the caller's context happens to carry a deadline.
Background loops (
pingLoop,ackLoop) call with no deadline. When thepeer's receive window closes and stays closed (a saturated connection), the
write blocks forever holding
writeMux. Every other sender — including theping loop, the only stall detector — then blocks on that same mutex. The
errgroupnever returns, so the higher-level reconnect loop never runs andthe connection is never rebuilt. The single tool that could interrupt the
I/O,
Close(), only runs after the group dies — which the wedged writeprevents. The result is a live-looking connection that passes no requests
until the process is restarted.
Key exchange can hang indefinitely. In
exchange/client_flow, steps 5and 7 read directly from the connection without applying
ExchangeTimeout.If the peer completes the TCP handshake but never answers the key exchange
(a silently dropped inbound path), the read blocks with no bound: the
connection sits mid-handshake forever, no timeout ever fires, and because
this is on the connect path the client never reaches a usable state — the
reconnect loop cannot even start its next attempt, because the current one
never returns.
The only stall detector is the ping round-trip. With
interval=60s+timeout=15sa silent-but-established socket is noticed in at best 75s —and, per (1), never, if the ping loop is itself wedged behind the write
lock. Until it is noticed, every request on that connection quietly waits
with no error and no progress. The official clients additionally run a
detector keyed on time since last receive, independent of the write path
(Android's
checkTimeout, TDLib'slast_read_at_/last_pong_at_,Desktop's
_waitForReceivedTimer).Transport write failures leak to the caller raw. A frame that fails
mid-write (e.g.
EPIPEon a connection the peer already dropped) is notclassified as retryable, so the caller receives a broken-pipe error for a
request the server never processed — one that is provably safe to resend on
a fresh connection, yet is surfaced as a hard failure instead.
An acked-but-unanswered request is indistinguishable from user
cancellation. When the engine is force-closed after the server
acknowledged a request but before the result arrives, the caller sees only a
context error — identical to its own cancellation. It therefore cannot tell
"the server may already have run this, do not resend" from "I cancelled
this," so it can neither safely retry the request nor safely treat it as
never-sent.
What this changes
transport — closes (1), enables (4).
Send/Recvacquire theread/write I/O locks under the caller's context (a context-cancellable lock
acquisition) and force a deadline from
Close, so a blockedWritecan nolonger trap the ping loop and
Closecan no longer block behind an in-flightI/O. That breaks the deadlock and lets the
errgroupreturn, which is whatfinally allows the reconnect loop to run. Write failures are annotated with
the new
transport.ErrWriteFailedsentinel.exchange — closes (2). The existing
ExchangeTimeoutis now enforced onthe raw reads in key-exchange steps 5 and 7, so a handshake against a
non-answering peer fails within the timeout and hands control back to the
reconnect loop instead of hanging forever.
mtproto — closes (3), backstops (1). The connection is closed when the
context is cancelled mid key exchange; a new idle watchdog closes a
connection whose reads have gone silent past
IdleTimeout, keyed on timesince last receive and therefore working even when the ping loop itself is
wedged behind the write lock.
IdleTimeoutandPingDelayDisconnectbecomeconfigurable connection options.
rpc — closes (5). An engine-closed-after-ack is reported via the new
rpc.ErrEngineClosedAfterAcksentinel (stillerrors.Is(context.Canceled),so existing checks keep working), letting a caller finally tell that case
apart from its own cancellation. A request the server already acknowledged is
never transparently resent — neither on the
Doclose path nor in theretryUntilAckretry path (newrpc.ErrRetrySendFailed).telegram — closes (4), opt-in. Transport write failures can be
classified as retryable on a fresh connection, so a request that never reached
the server is retried instead of failing. This sits behind
Options.RetryOnWriteFailedand is off by default: a caller that acts onthat error itself — rotating a proxy or an endpoint — needs to keep receiving
it, so the default preserves today's behaviour. The new ping options are
forwarded from
telegram.Options.Relationship to the existing reconnect handling
gotd already carries requests that the server has not processed across a
reconnect:
Client.invokeConnretries a request classified as retryable-on-a-new-connection once the reconnect loop replaces the primary connection (the
work that closed #1021 / #1030). That retry is gated on the reconnect
actually firing — it blocks on the connection being replaced before retrying.
The failure modes above are precisely the ones where the reconnect never fires.
A wedged write, a silent socket, or a hung key exchange keeps
Conn.Runfromever returning, so the reconnect loop never iterates, the connection is never
replaced, and the waiting retries block indefinitely. This PR makes
Conn.Runreturn in those cases (forced deadline on
Close, idle watchdog, bounded keyexchange), so the existing retry path runs as intended — the connection is
rebuilt and the not-yet-processed requests are resent on it.
It also completes that retry classification without crossing its safety
boundary. A request whose frame failed mid-write (
transport.ErrWriteFailed)is now retryable, since the server discards partial frames. But a reconnect
starts a new session with a fresh
msg_id, so an acknowledged request mustnever be transparently resent — a fresh-
msg_idresend would duplicate arequest the server may already have executed. That request stays non-retryable;
ErrEngineClosedAfterAckonly makes the case identifiable instead ofsurfacing as a bare cancellation. (The recent stalled-request tracing added in
mtprototargets this same class of hangs; this PR supplies the fixes.)Public contract / breaking changes
No signatures change; all additions are backward compatible for callers that
do not opt in. The additions and behavioral changes to be aware of:
New exported errors
transport.ErrWriteFailed— a write to the underlying connection failed.Included in the "retryable on a new connection" classification only when
RetryOnWriteFailedis enabled; by default it still surfaces to the caller.Messages the codec rejects locally never carry it, since they would fail the
same way on any connection.
rpc.ErrEngineClosedAfterAck— the engine was force-closed after the serveracknowledged the request but before a result arrived. Deliberately not
retryable (a transparent resend could duplicate a side-effecting RPC). Still
satisfies
errors.Is(err, context.Canceled), so existing cancellation checksare unaffected; the sentinel only adds the ability to distinguish this case.
rpc.ErrRetrySendFailed— a request went unacknowledged forRetryIntervaland the retry send then failed. The original send already reached the wire,
so this is not retryable either.
New connection options (
mtproto.Options, forwarded fromtelegram.Options)PingDelayDisconnect time.Duration— thedisconnect_delayvalue sent to theserver in
ping_delay_disconnect. Previously computed inline asPingInterval + PingTimeout; now configurable, defaulting to the samevalue, so the wire behavior is unchanged unless set. Validated to exceed
PingIntervalafter truncation to whole seconds; an invalid value falls backto the default with a warning.
IdleTimeout time.Duration— maximum time without any received data beforethe idle watchdog closes the connection. Defaults to
max(PingDelayDisconnect, PingInterval + PingTimeout)and is validatedagainst a
PingInterval + PingTimeoutfloor (below the floor a late-but-finepong could trip it).
PingInterval/PingTimeoutare now also settable at thetelegram.Optionslevel.New behaviour option
telegram.Options.RetryOnWriteFailed(mirrored bypool.DCOptions.RetryOnWriteFailed) — retry a request whose transport sendfailed on the next connection instead of returning the error to the caller.
Off by default, so callers that rotate a proxy or an endpoint on that
error keep receiving it.
Behavioral changes
IdleTimeoutderived as above). On ahealthy connection pongs and traffic keep the receive timer fresh, so it only
fires on a genuinely silent socket; it is a backstop for the case where the
ping loop itself cannot run.
mid-write is opt-in (
Options.RetryOnWriteFailed, andpool.DCOptions.RetryOnWriteFailedfor pools); left off, the write errorsurfaces exactly as it does today. Requests that were sent but not
acknowledged (
ErrConnDead,ErrEngineClosed) are retried unconditionally,as they already were.
Testing
transport,exchange,mtproto,rpc,pool,telegram) passes under-race; the full suite passes under-coverpkg=./...with no failures.62.2%, so the patch raises overall coverage. The uncovered remainder is
defensive branches (a
Close()that itself errors, a watchdog tick thatoutruns the first receive) that are not deterministically reachable in a
unit test.
Writesimulated at thetransport layer, not via a live network):
Closeforces the in-flight I/O to unblock (
transport/connection_test.go);ExchangeTimeout(
exchange/client_flow_timeout_test.go);one, with the
PingInterval + PingTimeoutfloor enforced(
mtproto/watchdog_test.go,mtproto/options_ping_test.go);hanging (
mtproto/connect_cancel_test.go);ErrEngineClosedAfterAckwhile still matchingcontext.Canceled; a failedretry send is reported as
ErrRetrySendFailedand not resent(
rpc/engine_test.go);telegram.Options(
telegram/ping_options_test.go).Invariants the patch protects:
Out of scope (deliberately)
mtproto.Conn, at-least-onceresend with a stable
msg_id,msgs_state_req). The official clients dothis; it is a larger, core-touching change and is intentionally left out so
this PR stays a set of self-contained transport fixes.