[WPB-27169] Include collaborators in contact search. by fisx · Pull Request #5515 · wireapp/wire-server

fisx · 2026-09-03T07:08:57Z

This is the 3rd attempt at making collaborators searchable in ES. Please read commit-by-commit. The first commit has already been reviewed, merged to staging, and reverted. It contains of of the feature but is buggy.

The most important change is in b7547d0 (read the commit message).

I have some hope that this will fix the problem, but I think the best way to test it is to merge it, and watch what happens on staging. Any other ideas welcome!

https://wearezeta.atlassian.net/browse/WPB-27169

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

fisx · 2026-09-07T07:07:15Z

        migration to new index from database (force sync):
FAIL
          Exception: IndexMigrationError "ReindexSameOrNewer: failed to sync 1 documents."
          HasCallStack backtrace:
            collectBacktraces, called at libraries/ghc-internal/src/GHC/Internal/Exception.hs:169:13 in ghc-internal:GHC.Internal.Exception
            toExceptionWithBacktrace, called at libraries/ghc-internal/src/GHC/Internal/IO.hs:260:11 in ghc-internal:GHC.Internal.IO
            throwIO, called at ./Control/Concurrent/Async.hs:78:13 in tasty-1.5.4-HtsomygUJly7mfNW2QbQBg:Control.Concurrent.Async
          Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&/migration to new index from database (force sync)/' to rerun this test only.

it's flaky, but looks related?

fisx · 2026-09-07T07:08:44Z

also the "failing tests" in bf093e1 are not failing. interesting.

…rch (second attempt). (#5488) * Include team collaborators in contact search. * Implement TeamCollaboratorsSubsystem interpreter with BrigAPIAccess. Was previously UserSubsystem, but since it TeamCollaboratorsSubsystem is also used outside of Brig, that is not always available. Further changes: - Support BrigAPIAccess locally in Brig. - Change collaborator field type in UserDoc to collapse `Nothing` and `Just []` (remove the Maybe). * Fix brig-index: do not bump index version in failure case. * Add release notes on required postgres setup steps. --------- Co-authored-by: Gautier DI FOLCO <gautier.difolco@wire.com>

(I expected this to break, but it didn't. The search for the issue continues.)

This reverts commit 7aa933b.

The Problem: User accounts can legitimately be in an inconsistent state, eg., because a team is in the process of being deleted: Team member entry is already gone, but brig account entry still exists. This causes `brig-index`'s bulk sync to fail because Galley returns 404 for team-related lookups. (The `try`-wrapped calls produce `Left SomeException`; `mkUserDoc`, `mkDocVersion` handle this by skipping the account, `ReindexSameOrNewer` throws.) Evidence from the logs coinciding with a failing index migration: ``` 2026-09-02 13:53:42.640 error { "error": "RPCException {"remote" = "galley", "path" = "i/teams/645c4b69-46f1-49a8-be57-35374d0aa947/features/searchVisibilityInbound", "headers" = [("Request-Id","brig-index")], "cause" = HttpExceptionRequest Request { host = "galley" port = 8080 secure = False requestHeaders = [("Request-Id","brig-index")] path = "i/teams/645c4b69-46f1-49a8-be57-35374d0aa947/features/searchVisibilityInbound" queryString = "" method = "GET" proxy = Nothing rawBody = False redirectCount = 10 responseTimeout = ResponseTimeoutDefault requestVersion = HTTP/1.1 proxySecureMode = ProxySecureWithConnect } (Response {responseStatus = Status {statusCode = 404, statusMessage = "Not Found"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Wed, 02 Sep 2026 11:53:35 GMT"), ("traceparent","00-f96c377d85487f15da6832eed1144233-1f618513676d5162-01"),("tracestate",""),("Content-Encoding","gzip"),("Content-Type","application/json"),("Vary","Accept-Encoding")], responseBody = (), responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose, responseOriginalRequest = ... responseEarlyHints = []}) "{\"code\":404,\"label\":\"no-team\",\"message\":\"Team not found\"}")}", "userId": "19d790a4-8ac3-4fd9-81a9-ef24af8c8bf9", "request": "brig-index", "msgs": [ "E", "Error ocurred while indexing user" ] } ``` The solution is graceful degradation in Galley lookups (search visibility, roles): when Galley says "team not found" (or any Galley error), treat it as "team no longer exists" and fall back to safe defaults instead of skipping the user entirely.

blackheaven

Full review notes; inline comments attached.

Blockers

testCollaboratorNotFoundOldIndex: the search runs after withOldIndex returns → hits the real cluster brig + new index, where memberB already has collaborating_teams (the reindex trigger is synchronous). And an empty search term matches 0 ES documents anyway, so the assertion passes vacuously. Fix: keep the galley call outside (per your comment), move updateSearchIndex + refreshIndex + search-by-name inside withOldIndex.
Bulk sync degrades on any galley failure (visibility + roles), so a transient outage writes degraded docs and still persists migration v7 (skipped == 0). The collab lookup right next to it fails the page instead. Suggest degrading only on galley 404 no-team; skip the doc on everything else.

Non-blocking

Federated search has its own matchTeamMembersOf (Brig.User.Search.SearchIndex) — not updated, so collaborators stay invisible via federation.
The frozen legacy fixture in TypesSpec was edited; please restore it so the FromJSON default for collaborating_teams stays regression-tested.
GTE switch has a known same-version race; see inline.

Good: fail-closed migration (v7 only persists when skipped == 0), batched per-page collaborator query, graceful old-index behavior (queries no-op on the missing field, dynamic: false drops it). The flaky ReindexSameOrNewer: failed to sync 1 documents in CI is the new strictness surfacing pre-existing dirty data — expect more until the full reindex lands.

Nits

Reindex-on-permission-change is a no-op write (doc carries team ids only).
getTeamCollaborationsForUsersImpl defines its Statement per call; the rest of the module hoists them to top level.
Eval.hs embeds up to 1000 error strings in one IndexMigrationError; count + first N is enough.
Changelog: searchSameTeamOnly deployments now also see personal-user collaborators (TeamOnly branch) — worth a line.
Team deletion: confirm the cascade removes collaborator rows via internalRemoveTeamCollaborator (now a reindex trigger), or accept stale collaborating_teams until full reindex.

blackheaven · 2026-09-10T10:01:19Z

+    refreshIndex brig
+
+  -- refreshIndex brig
+  res <- searchResults <$> executeSearch brig (userId ownerA) ""


This runs after withOldIndex returns → real cluster brig and the NEW index, where memberB already has collaborating_teams from the synchronous reindex above. Two problems: wrong index, and empty term — defaultUserQuery's multi-match with "" has zero clauses and matches 0 docs, so notElem [] passes vacuously.

Keep the galley call outside (per your comment), but move updateSearchIndex, refreshIndex, and the search inside withOldIndex, searching by memberB's name. The old mapping has "dynamic": false, so the unknown field is dropped — exactly the degradation this test should pin.

blackheaven · 2026-09-10T10:01:19Z

+  -- Do NOT add memberB as collaborator
+  -- Search should not find memberB regardless of index version
+  refreshIndex brig
+  res <- searchResults <$> executeSearch brig (userId ownerA) ""


Empty term → 0 ES hits → vacuous pass on both index variants. Replace with assertCan'tFind brig (userId ownerA) (userQualifiedId memberB) (fromName (userDisplayName memberB)).

blackheaven · 2026-09-10T10:01:19Z

+      let vis :: IndexUser -> SearchVisibilityInbound
          vis indexUser =
-            fromMaybe (Right defaultSearchVisibilityInbound) $ flip Map.lookup visMap =<< indexUser.teamId
+            fromMaybe SearchableByOwnTeam $ hush =<< flip Map.lookup visMap =<< indexUser.teamId


Degrades on ANY exception, not just the no-team 404 this series targets. A transient galley outage during a full sync now overwrites good docs with default visibility and leaves skipped == 0, so migrateData persists v7 as complete — degraded docs don't self-heal until the next version bump. The collab lookup below takes the opposite policy (any failure skips the whole page).

Suggest: degrade only on RpcExceptionWai _ err | err.label == "no-team"; anything else → Left, so the doc is skipped and counted. The pre-PR binary-split retry isn't needed for that — a failing team just skips its own users.

blackheaven · 2026-09-10T10:01:19Z

-      roles :: Map UserId (Either SomeException (WithWritetime Role)) <-
+            eithMembers <- try @SomeException $ interpreter $ (.members) <$> selectTeamMemberInfos tid uids
+            pure case eithMembers of
+              Left _ -> Map.empty


Same policy issue: a 500/timeout from galley silently drops roles for the whole team and still counts those users as synced. Restrict the graceful path to the no-team label; on anything else return per-user Lefts so skipped > 0 blocks version persistence.

blackheaven · 2026-09-10T10:01:19Z

+          -- GTE, not GT: the version comes from the user row alone, but the document also
+          -- holds data that changes without touching that row (collaborations), and under
+          -- GT those updates would be dropped as version conflicts.  Older writes still lose.
+          version = ES.ExternalGTE . ES.ExternalDocVersion . docVersion $ indexUserToVersion mRole indexUser


Race to write down: docVersion covers only the user row + role, so a profile-update sync and a collaboration-change sync can carry the same version, and under GTE the collab-less doc can land last — dropping a just-added collaboration until the next profile change or full reindex. Close it by folding a collaborations component into indexUserToVersion (like role writetime) or re-triggering from a retrying job; at minimum document the window in this comment.

blackheaven · 2026-09-10T10:01:19Z

-- Dont touch this. This represents serialized legacy data.
 userDoc1ByteString :: LByteString
-userDoc1ByteString = "{\"email\":\"phoompy@example.com\",\"account_status\":\"active\",\"handle\":\"phoompy\",\"managed_by\":\"scim\",\"role\":\"admin\",\"accent_id\":32,\"name\":\"Carl Phoomp\",\"created_at\":\"2020-08-29T21:50:00.000Z\",\"team\":\"17c59b18-57d6-11ea-9220-8bbf5eee961a\",\"id\":\"0a96b396-57d6-11ea-a04b-7b93d1a5c19c\",\"normalized\":\"carl phoomp\",\"saml_idp\":\"https://issuer.net/214234\"}"
+userDoc1ByteString = "{\"collaborating_teams\":[\"17c59b18-57d6-11ea-9220-8bbf5eee961a\"],\"email\":\"phoompy@example.com\",\"account_status\":\"active\",\"handle\":\"phoompy\",\"managed_by\":\"scim\",\"role\":\"admin\",\"accent_id\":32,\"name\":\"Carl Phoomp\",\"created_at\":\"2020-08-29T21:50:00.000Z\",\"team\":\"17c59b18-57d6-11ea-9220-8bbf5eee961a\",\"id\":\"0a96b396-57d6-11ea-a04b-7b93d1a5c19c\",\"normalized\":\"carl phoomp\",\"saml_idp\":\"https://issuer.net/214234\"}"


This fixture was frozen as legacy data ("Dont touch this") — editing it means the FromJSON default for collaborating_teams (.!= []) is no longer regression-tested. Restore the original JSON (git history has it) and set userDoc1.udCollaboratingTeams = [] instead; the roundtrip property covers the new field via Arbitrary anyway.

blackheaven · 2026-09-10T10:01:19Z

+          [ -- Match users who are members of the team
+            ES.TermQuery (ES.Term "team" $ idToText team) Nothing,
+            -- Match users who are collaborators in the team
+            ES.TermQuery (ES.Term "collaborating_teams" $ idToText team) Nothing


There's a second copy of this function: Brig.User.Search.SearchIndex.matchTeamMembersOf (backs Brig.API.Federation). It's not updated, so collaborators stay invisible via federated search. Update it identically or state the exclusion explicitly — there's already a FUTUREWORK at SearchIndex.hs:205 about unifying the two copies.

blackheaven · 2026-09-10T10:01:19Z

  generateTeamEvents (tUnqualified zUser) team [EdCollaboratorAdd user (Set.toList perms)]

+  -- Reindex the collaborator with their new collaboration team
+  BrigAPIAccess.updateSearchIndex user


Synchronous RPC to brig after the store write and generateTeamEvents: if brig/ES is down, galley 5xxs a mutation that already committed, and a client retry hits collaborator-exists. Consider best-effort reindex (log + BackgroundJobPublisher retry) so index freshness degrades instead of the API. Applies to update/remove below too; same acknowledged debt in background-worker's Wire/Effects.hs:351.

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 3, 2026

fisx mentioned this pull request Sep 7, 2026

[WPB-27169] Do not count apps and collaborators as members in get-team-size. #5445

Open

5 tasks

fisx and others added 8 commits September 10, 2026 10:11

Changelog.

e613d52

Integration test: run new brig on old index.

b142005

(I expected this to break, but it didn't. The search for the issue continues.)

[throw-away debug code] dump ES state.

a0cb8af

Revert "[throw-away debug code] dump ES state."

4b0349a

This reverts commit 7aa933b.

Make (forceR,r}efreshIndex calls to brig fail more helpfully.

6aa2f7d

[drive-by] rm constraint declaration duplication.

9fd650a

fisx force-pushed the WPB-27169-include-collaborators-in-contact-search branch from 6a17417 to b7547d0 Compare September 10, 2026 08:16

Remove stale changelog entry.

c38bf5f

fisx marked this pull request as ready for review September 10, 2026 08:27

fisx requested review from a team as code owners September 10, 2026 08:27

fisx added the ready-for-review looking actively for reviewer label Sep 10, 2026

blackheaven reviewed Sep 10, 2026

View reviewed changes

Navigation Menu

Uh oh!

[WPB-27169] Include collaborators in contact search. - #5515

[WPB-27169] Include collaborators in contact search.#5515
fisx wants to merge 9 commits into
developfrom
WPB-27169-include-collaborators-in-contact-search

fisx commented Sep 3, 2026 •

edited

Loading

Uh oh!

fisx commented Sep 7, 2026

Uh oh!

fisx commented Sep 7, 2026

Uh oh!

blackheaven left a comment

Uh oh!

blackheaven Sep 10, 2026

Uh oh!

blackheaven Sep 10, 2026

Uh oh!

blackheaven Sep 10, 2026

Uh oh!

blackheaven Sep 10, 2026

Uh oh!

blackheaven Sep 10, 2026

Uh oh!

blackheaven Sep 10, 2026

Uh oh!

blackheaven Sep 10, 2026

Uh oh!

blackheaven Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

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

Uh oh!

Checklist

Uh oh!

fisx commented Sep 7, 2026

Uh oh!

fisx commented Sep 7, 2026

Uh oh!

blackheaven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fisx commented Sep 3, 2026 •

edited

Loading