Sustained ~110% CPU: infinite retry loop in SynchronizePeerReadStates for a ghost PeerId(0) sync operation (13.1.0) #194

Activity

1. CraftStick commented on Oct 30, 2026

   

   CraftStick

   on Oct 30, 2026

   Contributor

   More actions

   Thanks for the detailed analysis. I checked the code at the revision the macOS repository pins (submodules/telegram-ios at fb0e62ba), and it matches your description. I did not reproduce the loop itself.

   • In ManagedSynchronizePeerReadStates.swift the error handler of an operation (around line 103) logs "operation retry", removes the active operation and calls update() right away. There is no delay or retry limit. update() then starts the same operation again for the same peer, because currentState still contains it.
   • synchronizePeerReadState in SynchronizePeerReadState.swift first builds an Api.InputPeer from the peer in the postbox (inputPeer(postbox:peerId:), line 12). If the peer cannot be resolved, as for PeerId(0), the signal fails at once, so the operation fails without any network request. That would explain why the loop survives a network disconnect and why traffic stays near zero.

   So any operation that can never complete turns into a busy loop on the postbox queue, and a backoff in the manager would limit the damage whatever writes the invalid row.

   This code is in the Telegram-iOS submodule, so it cannot be changed from this repository. I have not investigated what writes the PeerId(0) row, so I cannot say whether dropping it in the manager is enough or whether the source should be fixed.
2. igabdullin commented on Oct 30, 2026

   

   igabdullin

   on Oct 30, 2026

   Last edited by igabdullin

   Author

   More actions

   Follow-up: how the ghost operation gets created, the wider class, and a verified fix.

   (@CraftStick — your analysis matches; inputPeer(postbox:peerId:) at line 12 is indeed the
   instant, network-free failure point.)

   1. Where the row comes from — mechanism, executed against the fb0e62ba sources

   There is exactly one writer of a .Push(thenSync: true) row: synchronizeReadStateTable.set
   from MessageHistoryTable.applyInteractiveMaxReadIndex (~line 745) — the interactive
   "mark read" path. The other .Push writer (Postbox.swift ~2405) hardcodes thenSync: false,
   which the stored byte rules out. Two findings make the creation possible for any peer without
   stored read state:

   • MessageHistoryReadStateTable.applyInteractiveMaxReadIndex (~line 430) returns
     .Push(thenSync: true) even when the peer has no read state at all — there is no
     peer-existence check. The table stores only two bytes (opType, thenSync); state is
     re-derived at read time, which is why the log shows Push(state: nil, thenSync: true).
     One set() with a zero peer index is enough.
   • peerKeepsReadState(nil) == true (NonMemberChannelReadState.swift:21 — "A peer that is not
     stored keeps today's behaviour"), so the guards in _internal_applyMaxReadIndexInteractively
     and synchronizePeerReadState do not stop it.

   Verified with an out-of-tree harness (Postbox builds standalone with swift build):

   applyInteractiveReadMaxIndex(MessageIndex(peerId: PeerId(0), ...))
     → 0:Id(rawValue: 0) -> .Push(state: nil, thenSync: true)   # the exact row from the incident log
   

   2. Why the instant retries imply a stored peer row — and what is still unknown

   loadedPeerWithId of a missing row never emits, so for a missing peer the consumer's operation
   would hang silently. The observed 831 retries/sec mean PeerId(0) resolved to a stored record
   that apiInputPeer rejects. Where that record (and the zero index that read it) came from is
   still unidentified: the write path is silent, and every interactive-read caller I checked passes
   real message indexes (the macOS notification path is excluded — strict userInfo parser built from
   real message.id, and its legacy NSUserNotificationCenter delegate is not in use on modern
   macOS; the associated-peer branch cannot fire for a peer without an existing read state).

   Two directions for settling the root, short of a repro:

   • one postboxLog at the enqueue site when the peer id is zero would name the caller on the
     next recurrence (affected users can already produce a live loop trace via
     tg://settings/enable_log);
   • if field data ever shows zeroed ids arriving in server responses, the parser level
     (rejecting peers/messages with zero ids at ingestion) would be the true root fix.

   3. The class is wider than zero

   Any never-completing operation turns into this busy loop. Other entry points: a stored peer
   without accessHash (apiInputPeer → nil — peers known only indirectly), validation
   non-convergence (localReadMarkIsAheadOfServer / count mismatch), a permanently rejecting
   server. For the record: error → update() with no delay exists in exactly two files across
   TelegramCore — this manager and Network/FetchV2.swift (which sets state = .failed first,
   so it likely does not spin).

   4. Proposed fix — three layers

   Layer	What	Status
   C′	reject zero-peer operations at the table chokepoint (set()): covers every writer, clears legacy rows	verified red/green locally — ghost appears before, nothing after, valid stateless peer unchanged; in PR #2418
   A	consumer cleanup of rows already persisted in users' databases (the chokepoint alone may never rewrite an existing row)	PR #2418
   B	exponential backoff in the manager — the only layer that bounds the class: a stored peer without accessHash produces the identical loop, and C′/A close only the zero-peer instance	concrete diff below

   Deliberately not proposed: peerKeepsReadState(nil) == false (looks intentional; peers being
   fetched would change semantics) and dropping operations for stored peers without accessHash
   (the hash may legitimately arrive later).

   5. Layer B — concrete diff (v2)

   v1 compiled and behavior-checked in the real macOS app by @CraftStick (comment below); v2
   updates it after an edge-case review: the window is now tied to the operation that failed, so
   a changed operation for the same peer (a fresh read of the chat) starts immediately instead of
   waiting out the old window (verified red/green in our harness).

   Exponential per-peer backoff 1s → 32s (cap), reset on completion; peers inside the backoff
   window are skipped in update(). For the incident's loop this is 831 retries/sec → at most one
   per second, and it bounds every other never-completing operation the same way.

   --- a/submodules/TelegramCore/Sources/State/ManagedSynchronizePeerReadStates.swift
   +++ b/submodules/TelegramCore/Sources/State/ManagedSynchronizePeerReadStates.swift
   @@ -31,6 +31,14 @@
        private var activeOperations: [PeerId: Operation] = [:]
        private var pendingOperations: [PeerId: PeerReadStateSynchronizationOperation] = [:]
        
   +    // A permanently failing operation must not be restarted in a tight loop: retry
   +    // attempts back off exponentially per peer and reset once one completes. The
   +    // backoff is tied to the operation that failed, so a *changed* operation for
   +    // the same peer (a fresh read of the chat) still starts immediately.
   +    private var operationRetryCounts: [PeerId: Int] = [:]
   +    private var operationRetryAt: [PeerId: Double] = [:]
   +    private var operationRetryLast: [PeerId: PeerReadStateSynchronizationOperation] = [:]
   +    
        init(queue: Queue, network: Network, postbox: Postbox, stateManager: AccountStateManager) {
            self.queue = queue
            self.network = network
   @@ -58,6 +66,15 @@
            let peerIds = Set(self.currentState.keys).union(Set(self.pendingOperations.keys))
            
            for peerId in peerIds {
   +            if let retryAt = self.operationRetryAt[peerId], CFAbsoluteTimeGetCurrent() < retryAt {
   +                if let lastOperation = self.operationRetryLast[peerId], self.currentState[peerId] != lastOperation {
   +                    self.operationRetryAt.removeValue(forKey: peerId)
   +                    self.operationRetryCounts.removeValue(forKey: peerId)
   +                    self.operationRetryLast.removeValue(forKey: peerId)
   +                } else {
   +                    continue
   +                }
   +            }
                var maybeOperation: PeerReadStateSynchronizationOperation?
                if let operation = self.currentState[peerId] {
                    maybeOperation = operation
   @@ -100,9 +117,22 @@
                            }
                            if let activeOperation = activeOperation {
                                if let current = strongSelf.activeOperations[peerId], current === activeOperation {
   -                                Logger.shared.log("SynchronizePeerReadStates", "\(peerId): operation retry \(operation)")
   +                                let retryCount = (strongSelf.operationRetryCounts[peerId] ?? 0) + 1
   +                                strongSelf.operationRetryCounts[peerId] = retryCount
   +                                strongSelf.operationRetryLast[peerId] = operation
   +                                let delay = min(pow(2.0, Double(retryCount - 1)), 32.0)
   +                                strongSelf.operationRetryAt[peerId] = CFAbsoluteTimeGetCurrent() + delay
   +                                Logger.shared.log("SynchronizePeerReadStates", "\(peerId): operation retry \(operation), in \(delay)s")
                                    strongSelf.activeOperations.removeValue(forKey: peerId)
   -                                strongSelf.update()
   +                                strongSelf.queue.after(delay, { [weak strongSelf] in
   +                                    guard let strongSelf = strongSelf else {
   +                                        return
   +                                    }
   +                                    if let retryAt = strongSelf.operationRetryAt[peerId], CFAbsoluteTimeGetCurrent() + 0.01 >= retryAt {
   +                                        strongSelf.operationRetryAt.removeValue(forKey: peerId)
   +                                    }
   +                                    strongSelf.update()
   +                                })
                                }
                            }
                        }, completed: { [weak self, weak activeOperation] in
   @@ -113,6 +143,9 @@
                                if let current = strongSelf.activeOperations[peerId], current === activeOperation {
                                    Logger.shared.log("SynchronizePeerReadStates", "\(peerId): operation completed \(operation)")
                                    strongSelf.activeOperations.removeValue(forKey: peerId)
   +                                strongSelf.operationRetryCounts.removeValue(forKey: peerId)
   +                                strongSelf.operationRetryAt.removeValue(forKey: peerId)
   +                                strongSelf.operationRetryLast.removeValue(forKey: peerId)
                                    strongSelf.update()
                                }
                            }

   PR #2418 now carries layers C′ + A + regression tests (their PostboxTests Bazel target globs
   Tests/**/*.swift, so the tests run in CI as-is).
3. igabdullin commented on Oct 30, 2026

   

   igabdullin

   on Oct 30, 2026

   Author

   More actions

   Addendum: layer B verified in execution, class 1 reproduced with TelegramCore's own code,
   and no id validation at peer ingestion
   (follow-up to the layer table and the diff in my previous comment).

   1. A stored peer without accessHash — executed, not just read. After getting TelegramCore
   to build standalone from this revision (SwiftPM; TelegramApi via its own generate-swiftpm.sh,
   FlatSerialization generated with the pinned flatc 24.12.23), the class-1 chain runs end-to-end
   on synthetic data — no account, no network:

   store TelegramUser(id: 1234567, accessHash: nil)   // a "min" peer, e.g. a forward author
   loadedPeerWithId(1234567)      -> resolved (the stored row)
   apiInputPeer(stored peer)      -> nil              // ApiUtils.apiInputPeer, unmodified
   interactive read for the peer  -> Push(state: nil, thenSync: true) queued
   

   The operation is queued and can never be sent to the server — with the manager's no-backoff
   restart this is the incident's loop for a valid peer id. C′ deliberately does not cover this
   case — it only matches the zero peer id; a valid peer without an accessHash is exactly what B
   bounds.

   2. Layer B verification. A verbatim port of the manager's restart logic (Logger → a counter,
   the operation signal replaced by an instantly-failing one modelling the apiInputPeer == nil
   case), same sandbox:

   without backoff:  404,639 attempts in 3.0s  = 134,879 errors/sec
   with the diff:         3 attempts in 5.0s   (the 1s -> 2s -> 4s schedule)
   

   Two takeaways: the patch throttles exactly as designed; and the pure restart machinery is
   capable of ~135K retries/sec — the incident's observed 831/sec was limited only by the
   per-attempt transaction cost, so a future never-completing operation that fails before doing
   any postbox work would spin even harder. This is why B seems worth merging alongside C′+A
   rather than treating it as optional polish.

   3. Peer ingestion does not validate ids. Reading the merge path: updatePeers
   (UpdatePeers.swift ~142) and the public updatePeersCustom store whatever peers they are
   given — there is no check that a peer id is not the zero sentinel. So a zero-id peer can be
   stored, which is consistent with the incident's stored-peer deduction (an instantly retrying
   loop is only possible for a stored row). If field data ever shows zeroed ids arriving in
   server responses, an ingestion-level guard would be the true root fix — noting it here as a
   direction, not proposing a diff: it sits on a hot path (every server peer update) and belongs
   to the maintainers' design space.

   (The verification harnesses live outside the repo; happy to share them. Compilation of the B
   diff itself still needs the CI/macOS toolchain.)
4. CraftStick commented on Oct 30, 2026

   

   CraftStick

   on Oct 30, 2026

   Contributor

   More actions

   Layer B: checked in the macOS app

   I applied the diff from your comment (layer B, backoff 1 s → 32 s) to ManagedSynchronizePeerReadStates.swift at the pinned submodule revision (fb0e62ba) and built the full macOS app (Xcode 27, arm64, Debug). It compiles without errors or warnings.

   To see the behaviour I forced every synchronization operation to fail asynchronously (a temporary hook, not part of the diff), the way a failing postbox transaction does, with the same hook in both builds:

   build	retries	time
   without layer B	347,959	19.8 s (about 17,500/s)
   with layer B	12	223 s

   With layer B the retries come at +0, 1, 3, 7, 15, 31, 63, 95 s and so on, so the delay doubles up to the 32 s cap.

   Normal use is unaffected: with the hook off, opening a chat with unread messages cleared its unread counter. During that, the app logged two real retry errors for one channel, about 1.4 s apart, and then the operation completed.

   Not checked: this is a simulated failure, not the real PeerId(0) row, so it shows that the backoff bounds a never-completing operation but not where the row comes from.
5. igabdullin commented on Oct 30, 2026

   

   igabdullin

   on Oct 30, 2026

   Author

   More actions

   Thanks for running layer B in the real app — that closes the one gap our port couldn't:
   compilation in the actual build and normal-use behavior on top of it.

   The three retry rates line up on one curve — 831/s in the incident (each attempt pays
   postbox transactions), ~17.5K/s with your hook, 134.9K/s with a pure-signal stub in our
   port. Same unrestricted loop, differing only in per-attempt cost — which is also why the
   incident's rate shouldn't be read as the loop's ceiling.

   Your normal-use observation (two retries ~1.4s apart, then the operation completed) is
   exactly the evidence that the 1s seed delay doesn't disturb normal synchronization — worth
   having on the record, since "won't backoff delay legit syncs?" is the natural review
   question and now it has a data answer.

   One update before this goes further: reviewing the edge cases, we found and fixed a flaw in
   the version you tested — the backoff window was keyed by peer only, so a changed
   operation for the same chat (a fresh read) could wait out the old operation's window, up
   to the 32s cap. The diff in the comment above is updated to v2: it records which operation
   failed and resets the window when the queued operation differs, so a fresh read starts
   immediately. Neither of your scenarios is affected — your forced-failure hook kept the
   operation constant (no resets), and normal use completes anyway; the changed-operation
   case is verified red/green in our harness (old version waits, updated picks up at once).

   Practical question: would you fold layer B into PR #2418 as a fourth commit, or keep it as
   a separate PR? It's now verified end-to-end by two independent harnesses, so whichever
   vehicle the maintainers prefer, the diff is ready.
6. CraftStick commented on Oct 30, 2026

   

   CraftStick

   on Oct 30, 2026

   Contributor

   More actions

   A writer of the empty-peer row in the macOS client

   Following your analysis (the row is created by an interactive read with a zero peer index), I looked for callers in this repository and found one.

   ChatController's updateChatLocationThread resets maxVisibleIncomingMessageIndex to MessageIndex.absoluteLowerBound() (a MessageIndex of PeerId(0)) and clears the remembered indexes. The readHistory signal fires on that value, chatHistoryKeepsReadState lets members of channels, forums and monoforums through, and the index reaches applyMaxReadIndexInteractively. The callers of updateChatLocationThread are the topic separators in a forum shown as a message feed, a new topic and the thread switches in a monoforum.

   I reproduced the creation of the row with a temporary hook that repeats this sequence on an open chat. The application log then shows, without any other change:

   0:Id(rawValue: 0): take new operation Push(state: nil, thenSync: true)
   0:Id(rawValue: 0): begin operation Push(state: nil, thenSync: true)
   0:Id(rawValue: 0): do nothing, no change in Push(state: nil, thenSync: true)
   

   It matches the row from your incident log. In my database there is no stored record for PeerId(0), so the operation just stays active and does not spin, which is what you predicted ("for a missing peer the consumer's operation would hang silently"). I did not reproduce the 100 % CPU loop, and I did not try the real topic click or monoforum switch.

   With a guard against the empty peer in readHistory the same hook creates no such row. The change is in #206. It removes this one writer on the macOS side; layers C′, A and B from your comments are still needed for rows already stored and for the other never-completing operations.