[iFixRobots]

A new login backfills every channel at once, and each one fetches its threads inline, so hundreds of conversations.replies calls hit Slack together. They all get rate limited, burn through the 5 retries at the same time, and the thread is dropped without its replies.

Fetching thread replies one at a time per login lets the existing Retry-After handling actually work. I considered moving thread backfill out of the batch in bridgev2 (mautrix/go#665), but that's a lot more machinery for the same result.

Tag @indent to continue the conversation here.

[indent]

Issues

1 potential issue found:

• The per-login threadFetchLock is held across slackgo's Retry-After sleeps, so one rate-limited call can block every portal's event loop for minutes during a new-login backfill. Waiters also ignore ctx cancellation because sync.Mutex.Lock isn't ctx-aware.
  _{Found by Indent Review Agent}

↑ Select any checkbox above to have Indent auto-fix the issue

CI Checks

All required CI checks passed on 26b554d.

Review agents

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

Found issues (1)

• Indent Review Agent · Thread lock can stall portal loops for minutes

Full results

Indent Review Agent

• Summary: Thread lock can stall portal loops for minutes
• Last ran on commit: 26b554df
• Latest result

  {
    "summary": "Thread lock can stall portal loops for minutes",
    "findings": [
      {
        "fix": "Replace the mutex with a ctx-aware semaphore, e.g. `threadFetchSem chan struct{}` (buffer 1, or a small N), acquired with `select { case s.threadFetchSem <- struct{}{}: case <-ctx.Done(): return nil, ctx.Err() }` and released with defer. To keep a slow retry chain from blocking unrelated portals, consider pacing requests with a rate limiter (e.g. golang.org/x/time/rate `Wait(ctx)` tuned to the conversations.replies tier) instead of holding exclusive access across Retry-After sleeps.",
        "line": 87,
        "path": "pkg/connector/backfill.go",
        "issue": "bridgev2 runs thread backfill inside forward backfill. That happens in the portal event loop: `createMatrixRoomInLoop` (with `roomCreateLock` held) and `handleChatResync` both call `doForwardBackfill`, which reaches `fetchThreadBackfill` via `sendBatch`/`sendBackfill`. On a new login with hundreds of channels, each portal's event loop now blocks on the per-login `threadFetchLock` until every thread queued ahead of it, in any channel, has been fetched. The holder keeps the lock through slackgo's retry loop (`OptionRetry(5)`), which sleeps for each 429's Retry-After (60s default when the header is missing). So a single call can hold the lock for minutes, and a channel late in the queue can't process live messages, edits or reactions for the whole backfill. Before this change a portal only waited on its own fetches. `sync.Mutex.Lock` also doesn't watch ctx, so a waiter whose context was cancelled (portal deleted, the CreateMatrixRoom caller giving up, shutdown) keeps waiting until it gets the lock, then makes a request anyway. The only `GetConversationReplies` call is this one, so nothing bypasses the lock, and it covers the whole call (FetchMessages fetches one page per call, so there's no pagination). The serialization does let Retry-After work as described.",
        "title": "Global thread lock stalls every portal's event loop, and waiters ignore ctx",
        "severity": "bug"
      }
    ]
  }

[indent]

🟡 Bug · Global thread lock stalls every portal's event loop, and waiters ignore ctx

bridgev2 runs thread backfill inside forward backfill, in the portal event loop (createMatrixRoomInLoop with roomCreateLock held, and handleChatResync). With this per-login lock, every portal's loop blocks until all threads queued ahead of it, in any channel, are fetched. The holder keeps the lock through slackgo's retry loop (OptionRetry(5)), which sleeps for each 429's Retry-After (60s default if missing), so one call can hold it for minutes and late channels can't process live messages, edits or reactions. sync.Mutex.Lock also ignores ctx, so a waiter whose context was cancelled (portal deleted, shutdown) keeps waiting and then makes a request anyway. Use a ctx-aware semaphore or a rate limiter (Wait(ctx)) instead.

_{Found by Indent Review Agent}