fix(imap): retry connection failures with bounded exponential backoff by rodboev · Pull Request #761 · kenn-io/msgvault

rodboev · 2026-09-03T21:26:19Z

IMAP connection establishment now retries transient TCP failures and empty-greeting transport failures in plaintext, implicit-TLS, or STARTTLS mode on a bounded 5s, 15s, 45s schedule. TLS negotiation remains with crypto/tls, the pinned IMAP client owns greeting and STARTTLS protocol handling, and LOGIN or XOAUTH2 runs once after establishment succeeds.

The retry boundary records inbound plaintext IMAP progress and, for STARTTLS, separates the completed protocol response from the following TLS handshake transport state, so encrypted TLS handshake records cannot be mistaken for a partial greeting. A fresh connection is opened only before any inbound server greeting byte, or after a completed STARTTLS response with transient TLS transport loss, each retry emits a warning, and context cancellation interrupts connection work or backoff. Established-session reconnects and command replay retain their existing behavior, and there are no usage or configuration changes.

Closes #199

roborev-ci · 2026-09-03T21:31:32Z

roborev: Combined Review (`aec406b`)

Verdict: One medium-severity issue remains.

Medium

internal/imap/client.go:266-270, 484-486 — In STARTTLS mode, transient failures during the initial IMAP greeting occur before startTLSCommandSent is set, causing them to be classified as permanent and skipping retries. Classify these failures using the existing greeting retry logic while preserving protocol and truncated-greeting boundaries, and add a dropped STARTTLS greeting test.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 7m10s

roborev-ci · 2026-09-04T00:14:29Z

roborev: Combined Review (`dbeb827`)

Verdict: One medium-severity retry classification issue remains; otherwise TLS and authentication behavior appears sound.

Medium

internal/imap/client.go:368-375, 484-487 — STARTTLS may classify a clean connection close before the greeting as retryable via dialIMAP’s third return value, but connectOnce rechecks only the raw error with isTransientConnectError, which does not recognize that sentinel. Graceful STARTTLS disconnects therefore skip retry. Return and honor the explicit retryable classification from dialIMAP instead.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 8m41s

roborev-ci · 2026-09-04T01:40:43Z

roborev: Combined Review (`cabae0f`)

Verdict: One medium-severity issue identified; otherwise, the changes preserve expected TLS, STARTTLS, and authentication behavior.

Medium

internal/imap/client.go:321-330 — Write-side EPIPE errors are not classified as transient, so dropped STARTTLS connections may fail without retrying. Include broken-pipe and equivalent platform transport errors in transient connection classification.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 9m55s

roborev-ci · 2026-09-04T02:18:10Z

roborev: Combined Review (`c7eb8da`)

Verdict: One medium-severity retry-handling issue remains.

Medium

internal/imap/client.go:423-430 — Transient connection failures during XOAUTH2 authentication or LOGIN return ordinary errors, so the new retry loop does not retry them. Classify only transport errors from authentication as retryable while preserving permanent authentication failures.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 8m34s

roborev-ci · 2026-09-04T04:52:29Z

roborev: Combined Review (`31b4a7e`)

Verdict: One medium-severity retry-classification issue remains.

Medium

internal/imap/client.go:383-387, internal/imap/connerr.go:290 — After a successful implicit TLS handshake, TLS record bytes remain counted in progressConn.bytesSinceRequestWrite. If the server disconnects before sending the IMAP greeting, the retry classifier may treat handshake bytes as a partial greeting and suppress retrying, especially when the handshake ends with a server read. Reset progress tracking at the TLS-to-IMAP boundary or separate TLS accounting from protocol accounting.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 9m4s

…kenn-io#199)

…o#199)

…nn-io#199)

…io#199) Rebuild the connection retry boundary above completed implicit TLS and keep authentication outside the establishment scheduler.

roborev-ci · 2026-09-04T16:48:33Z

roborev: Combined Review (`cd32371`)

Verdict: One medium-severity issue remains in STARTTLS retry handling.

Medium

internal/imap/client.go:387-394: Any STARTTLS protocol bytes mark the connection as progressed, causing transient EOF/reset/timeout failures during the subsequent TLS handshake to be treated as terminal rather than retried. Track the STARTTLS response separately so transport failures during the TLS handshake remain retryable, while protocol and certificate errors remain terminal.

Reviewers: 2 done | Synthesis: codex, 6s | Total: 9m45s

…#199) Track the STARTTLS response and TLS handshake boundary without parsing protocol or TLS payloads, preserving terminal protocol and certificate failures.

roborev-ci · 2026-09-04T17:53:00Z

roborev: Combined Review (`8ce80dd`)

Verdict: One medium-severity regression remains in STARTTLS retry handling.

Medium

internal/imap/client.go:455-462, 498-500 — Transient STARTTLS disconnects before the TLS handshake begins are no longer retried. retryableSTARTTLS only accepts failures after startTLSHandshakeStarted, so a dropped greeting returns immediately instead of using the bounded retry path. Preserve separate pre-greeting transport tracking and retry EOF/reset failures when no greeting bytes were received, while keeping protocol and ambiguous response failures terminal.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 7m17s

roborev-ci · 2026-09-04T18:48:47Z

roborev: Combined Review (`e50064d`)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 8m55s

roborev-ci · 2026-09-04T19:25:26Z

roborev: Combined Review (`67ca1d0`)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 9m11s

roborev-ci · 2026-09-04T19:54:50Z

roborev: Combined Review (`9b114d0`)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 8m17s

roborev-ci · 2026-09-04T20:48:37Z

roborev: Combined Review (`a91c5e3`)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 8m51s

roborev-ci · 2026-09-04T21:22:10Z

roborev: Combined Review (`9e96446`)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 10m15s

wesm · 2026-09-05T00:26:14Z

looking

Issue kenn-io#199 asks the IMAP client to retry connection-level failures (EOF, reset, timeout) and to leave auth and protocol errors alone. The retry path had grown a stricter policy on top of that: no retry once the server had sent any greeting byte, and for STARTTLS only after the TLS ClientHello was on the wire. Enforcing that took a byte-progress wrapper, a hand-written parser for tagged IMAP responses, TLS record sniffing, and a four-state phase machine with a race-repair branch, none of which the issue needs. A server that sends half a greeting and resets is exactly the transient loss users hit, and refusing to retry there only fails faster. The retry decision now reads: a protocol refusal (IMAP status response, TLS alert, rejected certificate) is terminal; otherwise retry when the returned error or the socket itself reported a transport loss. The socket wrapper survives only because the pinned IMAP client reports a clean close before the greeting as a plain string error with no cause attached. The transient errno table was comparing Windows error numbers on every platform, where 64 and 109 mean unrelated Linux errnos. The Windows values now live in a build-tagged file using the syscall constants. Generated with Claude Code Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

roborev-ci · 2026-09-05T02:11:25Z

roborev: Combined Review (`05dc91e`)

No issues found.

Reviewers: 2 done | Synthesis: codex | Total: 15m22s

CI runs a repository check that rejects a test with four or more direct assert or require package calls, asking for a local helper instead. The partial-greeting and BYE-greeting retry tests crossed that line, which failed the lint job on the pull request. Both tests now bind the helpers once at the top, matching the rest of the file. Generated with Claude Code Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

roborev-ci · 2026-09-05T02:31:16Z

roborev: Combined Review (`5112285`)

Verdict: One medium-severity issue found in the STARTTLS retry test.

Medium

internal/imap/retry_test.go:76-95 — retryNoGreetingConn.Write waits for a client command before writing, but the client must receive the server greeting before sending STARTTLS. This can deadlock the first STARTTLS connection. Close the first connection immediately or allow the greeting to be written before closing.

Reviewers: 2 done | Synthesis: codex, 7s | Total: 12m41s

The transient errno table is now split per platform, and on Windows the syscall package's EPIPE is a placeholder value that Winsock never produces, so it is deliberately absent from the Windows table. The classifier test still wrapped EPIPE and failed on the Windows CI job. The test now wraps an entry from the platform's own table, which is what the classifier consults. Generated with Claude Code Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

roborev-ci · 2026-09-05T03:01:31Z

roborev: Combined Review (`d8bc8ba`)

Verdict: One medium-severity test deadlock should be fixed before merging.

Medium

internal/imap/retry_test.go:78 — The STARTTLS test server waits for commandRead before sending the initial greeting, while the IMAP client waits for that greeting before issuing STARTTLS. This causes both STARTTLS subtests to hang. Close or fail the connection immediately from the first server write instead of waiting for client data.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 10m41s

rodboev · 2026-09-05T03:14:31Z

Thanks for catching the scope creep here. Marked a couple other PRs draft while I make sure they are right-sized to the issue while maintaining correctness, and going deeper into distilling lessons from that process once they get there.

rodboev added 14 commits September 4, 2026 11:50

fix(imap): retry connection failures with bounded exponential backoff (…

07650cd

…kenn-io#199)

fix(imap): avoid backoff for permanent connection failures (kenn-io#199)

2fa5712

fix(imap): reject truncated greetings without retrying (kenn-io#199)

ab83a8a

fix(imap): preserve connection-stage error provenance (kenn-io#199)

082032f

fix(imap): cover STARTTLS transport and greeting cancellation (kenn-i…

7130877

…o#199)

fix(imap): separate protocol bytes from TLS handshake observation (ke…

ad9db6f

…nn-io#199)

fix(imap): preserve STARTTLS protocol rejection boundaries (kenn-io#199)

0795d3a

fix(imap): retry dropped STARTTLS greetings

4d240db

fix(imap): honor STARTTLS retry classification

58498dd

fix(imap): preserve truncated STARTTLS greetings

e6435cd

test(imap): cover STARTTLS and TLS handshakes

5d366a6

fix(imap): classify broken pipe as transient

91275fa

fix(imap): retry observed authentication transport failures

4684e03

fix(imap): retry connection establishment with bounded backoff (kenn-…

cd32371

…io#199) Rebuild the connection retry boundary above completed implicit TLS and keep authentication outside the establishment scheduler.

rodboev marked this pull request as draft September 4, 2026 16:41

rodboev force-pushed the pr/199-imap-retry-backoff branch from 31b4a7e to cd32371 Compare September 4, 2026 16:41

fix(imap): retry transient STARTTLS handshake transport loss (kenn-io…

8ce80dd

…#199) Track the STARTTLS response and TLS handshake boundary without parsing protocol or TLS payloads, preserving terminal protocol and certificate failures.

fix(imap): retry empty STARTTLS greetings (kenn-io#199)

e50064d

rodboev marked this pull request as ready for review September 4, 2026 18:52

fix(imap): stabilize STARTTLS retry coverage

67ca1d0

test(imap): make STARTTLS EOF failure deterministic

9b114d0

rodboev added 4 commits September 4, 2026 16:15

fix(imap): preserve STARTTLS handshake phase across races

4898ee0

fix(imap): require STARTTLS response before overlap retry

3209512

test(imap): preserve partial greeting evidence

2123045

test(imap): keep STARTTLS response failure terminal

a91c5e3

test(imap): close handshake fixture after ClientHello

9e96446

wesm self-assigned this Sep 5, 2026

Uh oh!

Conversation

rodboev commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roborev-ci Bot commented Sep 3, 2026

roborev: Combined Review (aec406b)

Medium

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (dbeb827)

Medium

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (cabae0f)

Medium

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (c7eb8da)

Medium

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (31b4a7e)

Medium

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (cd32371)

Medium

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (8ce80dd)

Medium

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (e50064d)

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (67ca1d0)

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (9b114d0)

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (a91c5e3)

Uh oh!

roborev-ci Bot commented Sep 4, 2026

roborev: Combined Review (9e96446)

Uh oh!

wesm commented Sep 5, 2026

Uh oh!

roborev-ci Bot commented Sep 5, 2026

roborev: Combined Review (05dc91e)

Uh oh!

roborev-ci Bot commented Sep 5, 2026

roborev: Combined Review (5112285)

Medium

Uh oh!

roborev-ci Bot commented Sep 5, 2026

roborev: Combined Review (d8bc8ba)

Medium

Uh oh!

rodboev commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

rodboev commented Sep 3, 2026 •

edited

Loading

roborev: Combined Review (`aec406b`)

roborev: Combined Review (`dbeb827`)

roborev: Combined Review (`cabae0f`)

roborev: Combined Review (`c7eb8da`)

roborev: Combined Review (`31b4a7e`)

roborev: Combined Review (`cd32371`)

roborev: Combined Review (`8ce80dd`)

roborev: Combined Review (`e50064d`)

roborev: Combined Review (`67ca1d0`)

roborev: Combined Review (`9b114d0`)

roborev: Combined Review (`a91c5e3`)

roborev: Combined Review (`9e96446`)

roborev: Combined Review (`05dc91e`)

roborev: Combined Review (`5112285`)

roborev: Combined Review (`d8bc8ba`)