fix(search): stop the chat index corrupting itself, and cut the service-side floods - #29467
Open
chrisnojima wants to merge 7 commits into
Open
fix(search): stop the chat index corrupting itself, and cut the service-side floods#29467chrisnojima wants to merge 7 commits into
chrisnojima wants to merge 7 commits into
Conversation
reindexConv paged over a conv's raw min..max message range rather than over the ids actually missing, so every pass re-fetched everything already indexed - 57,648 already-seen messages re-added in a single pass - while never reaching the top of a large conv, because each pass restarts at the lowest missing id and stops once the inbox-wide budget is spent. And a conv can never reach numMissing 0 while ids the server will never return (deleted, or never delivered) keep counting as missing, so SelectiveSync re-queued it every interval forever: ~1800 remote getMessages every 5 minutes on an idle desktop client, with numMissing byte-identical across 7 consecutive cycles. Pages now walk the missing ids and MarkSeen accounts for the ids the fetch will never return. A conv that still cannot converge is backed off with a bounded exponential skip rather than suppressed - "numMissing did not move" is strong evidence, not proof, and a budgeted pass can cover only unfetchable ids while real work remains further up the range. The clamp is on the shift exponent, not the result: streak is unbounded and a large enough shift overflows int negative and then to 0, which reads as "no skips" and turns the backoff off for good. Underneath that sat a worse class of bug: several paths published "these messages are indexed" without the tokens that make them findable ever reaching disk. Writes evicted from the cache before a flush, a flush that failed partway and dropped what it had not written, Remove writing metadata straight to disk ahead of its pending tokens, Add marking a message before indexing it, deletes racing the flush's write-outside-the-lock window, and a storage op whose callback reported success when the op had failed or never run. Each leaves a conv reading as fully indexed with messages no search will return, and nothing revisits a conv that reads as complete. Deletes are now queued in the pending set as nil entries and applied in order, failed writes are requeued, and the storage callback carries an error so the caller stops rather than marking. Because such a conv cannot repair itself, the index has to be dropped once on upgrade. indexMetadataVersion 3 -> 4 does that. Bumping indexVersion instead would have taken the token and alias entries with it and left inbox search returning nothing until the rebuild finished; the damage is confined to the metadata, so every conv re-indexes onto the entries already on disk and search keeps working throughout. Two things survive that a full rebuild would have cleared: alias refcounts, inflated by the re-add, and token entries naming since-deleted messages, which are filtered when a search resolves them and so cost a lookup rather than a wrong result. maxDirtyEntries was 1000, below the ~2600 entries one 300-message page produces, so it fired on every batch: 65 flushes in 2.9s with a 0.04s median gap, re-encrypting hot token entries whole on every pass. It is a memory bound, not a correctness one, so it belongs above one batch. At 20000: 7 flushes over a comparable window, and time spent flushing fell from ~69% of wall clock to 40%. Every one of these is covered by a test that fails against the pre-fix code.
The annotated team is the mostly off-chain bundle behind the team page, and it was rebuilt on every read: 469 team loads at 292ms each, 186 seconds, in a single run. It is now memoized behind an AnnotatedTeamCacher. Invalidation hangs off the notification that already exists: NotifyRouter drops the entry on any teamChangedByID, local or server-pushed, so the next UI read rebuilds from the server. The interface lives in libkb so the router can reach it; the implementation stays in teams.
…ating it Several UI surfaces routinely identify the same user at the same moment - the profile screen, the tracker popup, the profile card - and each sent its own request to the third-party service. Those services rate limit, and a throttled fetch surfaces to the user as a broken proof. checkStatusShared collapses a check with an identical one already in flight. It shares work, never results: a check is eligible only if it began at or after the caller's requestedAt, so the caller gets a live check that ran during its own request, and nothing is remembered past it. This neither lengthens a cache lifetime nor lets a stale answer stand in for a new one. finish is deferred, so a panic in the checker still closes the flight - without that the entry sits in the LRU with its channel open and every session sharing that key blocks until its own context is canceled.
Three places did per-item work inside a loop over a whole set. userEmojis read the gregor state once per emoji to decide whether animations were disabled; the state is now read once for the set and passed in, and the second attachment-URL lookup is skipped when it would return the same URL as the first. The search indexer fetched superseded messages one at a time - 14,621 single-message GetMessages calls in one run - where a batch of edits and attachment uploads can be fetched in one call. The per-message path stays as the fallback, since a failed batch says nothing about which id was at fault. Conversation participants are served from a five minute cache rather than refreshed per request, and uidmap returns the full name package it found on disk instead of dropping the part the caller asked for.
The test queued an op, closed stopCh and called storageLoop. Both of that select's cases are ready at that point and Go chooses between them at random, so half the time the loop ran the op instead of draining it and the callback correctly reported success - which the assertion read as a failure. It passed only because the earlier version ignored the error. The drain is now its own method and the test calls it directly, so the path under test is the one that runs. 8 consecutive runs and 3 under -race are clean.
SetImplicitTeamConflictInfoCacher, SetImplicitTeamCacher and SetKVRevisionCache mutated GlobalContext fields while holding only cacheMu.RLock(), which races with concurrent getters. Also document why the annotated team cache ages entries from the load start rather than the load finish.
Comment on lines
+184
to
+188
| if tmp, found := pc.flights.Get(key); found { | ||
| if f, ok := tmp.(*proofCheckFlight); ok && !f.startedAt.Before(requestedAt) { | ||
| return nil, f | ||
| } | ||
| } |
Comment on lines
+68
to
+72
| _, theirs := pc.CheckFlightBegin("k", t0, t0) | ||
| if theirs == nil { | ||
| t.Fatal("expected to find the flight") | ||
| } | ||
| if _, _, usable := theirs.wait(context.Background()); usable { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #29466 — review that one first; this diff is
go/only and is empty of JS. The two are independent (noprotocol/change, so no generated-type coupling), but the base carries a defensive UI fix that the index rebuild here is what actually triggers, so it should land first.Service-side half of the RPC-flood work. Most of it is the chat search index, which turned out to be corrupting itself rather than merely being noisy.
The search index
Chasing a
GetMessagesflood — 14,621 single-message calls in one run — led into the indexer, and the flood was a symptom.reindexConvpaged over a conv's raw min..max message range rather than over the ids actually missing, so every pass re-fetched everything already indexed (measured at 57,648 already-seen messages re-added in a single pass) while never reaching the top of a large conv, because each pass restarts at the lowest missing id and stops once the inbox-wide budget is spent. And a conv can never reachnumMissing0 while ids the server will never return (deleted, or never delivered) keep counting as missing, soSelectiveSyncre-queued it every interval forever: ~1800 remotegetMessagesevery 5 minutes on an idle desktop client, withnumMissingbyte-identical across 7 consecutive cycles.Fixing that exposed a worse class of bug underneath. Several paths could publish "these messages are indexed" without the tokens that make them findable ever reaching disk — writes evicted from the cache before a flush, a flush that failed partway,
Removewriting metadata ahead of its pending tokens,Addmarking a message before indexing it, deletes racing the flush's write-outside-the-lock window, and a storage op whose callback reported success when the op had failed or never run at all. Each leaves a conv reading as fully indexed with messages no search will ever return, and nothing revisits a conv that reads as complete. Every one is covered by a test that fails against the pre-fix code.Because such a conv cannot repair itself, the index has to be dropped once on upgrade.
indexMetadataVersiongoes 3 → 4 to do that. BumpingindexVersioninstead — the obvious move — would have taken the token and alias entries with it and left inbox search returning nothing until the rebuild finished, which on a large inbox is hours of a working feature simply being gone. The damage is confined to the metadata, so only the metadata is dropped: every conv re-indexes onto the token entries already on disk and search keeps working throughout.Two things survive that a full rebuild would have cleared: alias refcounts, already inflated by the old re-indexing loop and inflated further by the re-add, and token entries naming since-deleted messages, which are filtered when a search resolves them and so cost a lookup rather than a wrong result.
Also here: a bounded backoff for convs that genuinely cannot converge (rather than suppressing them outright, since "numMissing did not move" is strong evidence but not proof), and
maxDirtyEntriesraised 1000 → 20000 after it turned out to sit below the ~2600 entries one 300-message page produces and had become "flush after every batch".Everything else
getAnnotatedTeammemoized — the mostly off-chain bundle behind the team page was rebuilt on every read: 469 team loads at 292ms each, 186 seconds in one run. Invalidation hangs off theteamChangedByIDnotification that already exists.requestedAt.Verification
go build ./...,go vet, andchat/searchunder-race(8 consecutive runs clean). TheteamsTestAudit*/TestBoxAudit*failures reproduce identically onmaster— they need a live server.Validated live against a service built from this checkout, driven by the desktop e2e suite (324k log lines):
mkbot#went 56,661 → 29,961 missing in one pass, ~26.7k messages indexed. Zerostore.Addfailures, zero dropped ops, zero stall backoffs.Indexing: 10%. Under anindexVersionbump that would have been nothing but conversation-name matches.refreshParticipantsRemote— 190 in a window whose baseline was 1498–2568.Five warnings in 324k lines, all benign (one startup firehose, four app-disconnect EOFs). No errors, no panics.