Skip to content

client: classify connection failures and retry transient ones - #136

Merged
Fizzadar merged 5 commits into
mainfrom
nick/connection-state-lifecycle
Oct 29, 2026
Merged

Fizzadar merged 5 commits into
mainfrom
nick/connection-state-lifecycle

Conversation

@Fizzadar

Copy link
Copy Markdown
Member

Boot and RTM connection failures now share one classifier: revoked or invalid tokens send BAD_CREDENTIALS and log the user out, while deactivated accounts, disabled workspaces and removed users send BAD_CREDENTIALS with a restart action. Network, 5xx and rate-limit failures at boot send TRANSIENT_DISCONNECT and retry inside the connector with capped, jittered backoff, which Disconnect cancels at any point, so they no longer end in UNKNOWN_ERROR. When the RTM websocket fails to connect, the bridge now checks the token with auth.test, so a token revoked during a reconnect is reported instead of retrying forever.

🤖 Generated with Claude Code

Fizzadar and others added 2 commits October 28, 2026 16:12
Revoked or invalid tokens now ask for a relogin, deactivated accounts,
disabled workspaces and removed users report BAD_CREDENTIALS with a
restart action, and network, 5xx and rate limit errors keep retrying
with jittered backoff instead of ending in UNKNOWN_ERROR.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Fizzadar
Fizzadar marked this pull request as ready for review October 28, 2026 17:08
@indent

indent Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Issues

All clear! No issues remaining. 🎉

2 issues already resolved
  • The new CompareAndSwap only guards the end of connect(). With DMOnly set, connect() calls fetchConversationsForSync, which dereferences s.Client; a concurrent Disconnect sets s.Client to nil, and when connect() runs from the retryConnect goroutine the nil dereference crashes the bridge. (fixed by commit 362c572)
    Found by Indent Review Agent
  • The new ConnectionErrorEvent handler calls s.Client.AuthTestContext without a nil check. A concurrent Disconnect sets s.Client to nil, causing a recovered panic, and the auth check also runs after the context is already cancelled. (fixed by commit c226618)
    Found by Indent Review Agent

CI Checks

All required CI checks passed on 362c572.

Review agents

Select any unchecked box below to run or rerun that agent.

Passed (1)
  • Indent Review Agent · No remaining issues found
Full results

Indent Review Agent

  • Summary: No remaining issues found
  • Last ran on commit: 362c5721
  • Latest result
    {
      "summary": "No remaining issues found",
      "findings": []
    }

Comment thread pkg/connector/client.go Resolved
Comment thread pkg/connector/handleslack.go Resolved
connectAfterBoot cleared stopConnection before connect() did its
network and database work, so a Disconnect in that window cancelled
nothing, set RTM to nil and connect() then started goroutines on it.
connect() now swaps its cancel in only if the boot guard is still
current, and starts nothing otherwise.
connect() now runs from the retry goroutine, so a Disconnect during the
DM prefetch would crash the bridge instead of being caught by a caller.
@Fizzadar
Fizzadar merged commit 55ba54a into main Oct 29, 2026
12 checks passed
@Fizzadar
Fizzadar deleted the nick/connection-state-lifecycle branch October 29, 2026 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant