Skip to content

[CP Staging] Revert "[No QA] Fix leader-election deadlock in ActiveClientManager (SequentialQueue unable to flush)"#95718

Merged
grgia merged 1 commit into
mainfrom
revert-95148-eliran/94963-leader-election-deadlock
Jul 9, 2026
Merged

[CP Staging] Revert "[No QA] Fix leader-election deadlock in ActiveClientManager (SequentialQueue unable to flush)"#95718
grgia merged 1 commit into
mainfrom
revert-95148-eliran/94963-leader-election-deadlock

Conversation

@grgia

@grgia grgia commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Reverts #95148

$ #95705

#95148 added: if a tab sees the list without its own GUID, it re-appends itself. Sign-in resets ACTIVE_CLIENTS, so every open tab re-appends simultaneously. Each append puts that tab last in the list, so while the writes race, multiple tabs simultaneously pass the isClientTheLeader() check.

RequestNewValidateCode is one persisted request in the shared queue. Each self-believed leader flushes it before seeing another tab's dequeue, so it's sent once per tab: 7 tabs, 7 API calls, 7 emails. BeginSignIn wasn't duplicated because it bypasses the queue. Any queued write is affected, which is why SignUpUser duplicated too.

@grgia grgia changed the title Revert "[No QA] Fix leader-election deadlock in ActiveClientManager (SequentialQueue unable to flush)" [CP Staging] Revert "[No QA] Fix leader-election deadlock in ActiveClientManager (SequentialQueue unable to flush)" Jul 9, 2026
@grgia
grgia marked this pull request as ready for review July 9, 2026 15:14
@grgia
grgia requested review from a team as code owners July 9, 2026 15:14
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here.

@melvin-bot
melvin-bot Bot requested review from joekaufmanexpensify and removed request for a team July 9, 2026 15:14
@melvin-bot
melvin-bot Bot requested review from pecanoro and removed request for a team July 9, 2026 15:15
@melvin-bot

melvin-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

@pecanoro Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@grgia
grgia requested review from a team and removed request for joekaufmanexpensify and pecanoro July 9, 2026 15:15
@grgia

grgia commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@pecanoro added the team from offending PR, no worries

@melvin-bot
melvin-bot Bot requested review from pecanoro and removed request for a team July 9, 2026 15:16
@melvin-bot

melvin-bot Bot commented Jul 9, 2026

Copy link
Copy Markdown

@pecanoro Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

Files with missing lines Coverage Δ
src/libs/ActiveClientManager/index.ts 0.00% <0.00%> (-76.48%) ⬇️
... and 14 files with indirect coverage changes

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fcd33c590d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +49 to 50
if (removed) {
setActiveClients(activeClients);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Re-register a live client after ACTIVE_CLIENTS is cleared

When a live Onyx reset path that does not preserve ONYXKEYS.ACTIVE_CLIENTS runs (for example clearOnyxAndResetApp()/the 2FA clear path using KEYS_TO_PRESERVE), this callback now accepts the incoming empty or stale list and only writes back when trimming. That leaves every surviving tab without its own GUID in activeClients, so isClientTheLeader() returns false and SequentialQueue.flush() takes the follower early return; queued writes such as the OpenApp issued immediately after the reset are persisted but never sent until a later app-state transition or refresh calls init() again.

Useful? React with 👍 / 👎.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@grgia

grgia commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

tested, confirmed fix

@grgia
grgia merged commit b1e92e9 into main Jul 9, 2026
52 checks passed
@grgia
grgia deleted the revert-95148-eliran/94963-leader-election-deadlock branch July 9, 2026 15:36
@OSBotify

OSBotify commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify pushed a commit that referenced this pull request Jul 9, 2026
…ader-election-deadlock

[CP Staging] Revert "[No QA] Fix leader-election deadlock in ActiveClientManager (SequentialQueue unable to flush)"

(cherry picked from commit b1e92e9)

(cherry-picked to staging by grgia)
@OSBotify OSBotify added the CP Staging marks PRs that have been CP'd to staging label Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🚧 grgia has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

OSBotify commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by https://github.com/grgia in version: 9.4.32-1 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 failure ❌
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 No help-site changes required.

I reviewed the changes in this PR against docs/articles, and no updates are needed. This is a purely internal-infrastructure revert with no user-facing impact:

  • src/libs/ActiveClientManager/index.ts — client-side leader election for the SequentialQueue (which browser tab drains the persisted request queue). Internal plumbing only.
  • contributingGuides/SEQUENTIAL_QUEUE.md — a contributor guide, not a help-site article.
  • tests/unit/ActiveClientManagerTest.ts — a unit test.

None of these touch a documented, user-facing feature, setting, tab, or button. The help site (docs/articles) has no article describing multi-tab leader election or sequential-queue behavior — the only near-match (Partner-Billing-Guide.md) uses "active clients" in a billing-seat sense, which is unrelated. Since no docs changes are needed, I did not open a draft help-site PR.

@grgia, there's no linked help site PR to review here since no documentation changes were required. If you believe this revert does affect a user-facing behavior that's documented, let me know and I'll open one.

@OSBotify

OSBotify commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by https://github.com/grgia in version: 9.4.32-1 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 cancelled 🔪
🍎 iOS 🍎 success ✅

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 No help site changes required.

I reviewed the changes in this PR. This is a revert of an internal leader-election fix in ActiveClientManager / SequentialQueue. The files touched are:

  • contributingGuides/SEQUENTIAL_QUEUE.md — an internal contributing guide (not part of the public help site)
  • src/libs/ActiveClientManager/index.ts — internal source code
  • tests/unit/ActiveClientManagerTest.ts — a unit test (deleted)

These are purely internal plumbing changes (cross-tab leader election / persisted request queue) with no user-facing feature, setting, or UI behavior change. Nothing here corresponds to any article under App/docs/articles, so no draft docs PR was created.

@grgia, if you believe there's a customer-facing behavior change I've missed that should be documented, let me know and I'll draft the help site update.

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/grgia in version: 9.4.32-3 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Cherry-picked to staging by https://github.com/grgia in version: 9.4.33-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 cancelled 🔪

@MelvinBot

Copy link
Copy Markdown
Contributor

🤖 No help site changes required.

I reviewed the changes in this PR against the help site files under docs/articles. No draft docs PR is needed.

Why: This PR reverts an internal, client-side fix — it has no customer-facing behavior to document. The three changed files are all internal:

  • src/libs/ActiveClientManager/index.ts — leader-election logic across browser tabs (ACTIVE_CLIENTS)
  • contributingGuides/SEQUENTIAL_QUEUE.md — an internal contributor guide, not a help site article
  • tests/unit/ActiveClientManagerTest.ts — a unit test

The help site (docs/articles) documents customer-facing product behavior — billing, workspaces, integrations, expense/report flows, etc. Nothing in this revert changes any of that, so there is no article to update.

Since no docs changes are required, no draft help site PR was created — there's nothing to mark Ready for review. cc @grgia

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/lakchote in version: 9.4.33-8 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Bundle Size Analysis (Sentry):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CP Staging marks PRs that have been CP'd to staging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants