[HOLD for payment 2025-01-13] [HOLD for payment 2024-12-30] [$250] Accounting - Accounting tab loads infinitely when the workspace is created offline

[IuliiaHerets]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.57-3
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go offline.
3. Create a new workspace.
4. Go to workspace settings > More features.
5. Enable Accounting.
6. Go to Accounting.
7. Go online.

Expected Result:

Accounting tab will load after returning online.

Actual Result:

Accounting tab loads infinitely when the workspace is created offline.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6655070_1730782888555.20241105_125138.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021853787863203385081
• Upwork Job ID: 1853787863203385081
• Last Price Increase: 2024-11-05

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @slafortune

[melvin-bot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Infinity loading in accounting page if creating the workspace while offline.

What is the root cause of that problem?

The loading will show if the condition below is met.

App/src/pages/workspace/withPolicyConnections.tsx

Lines 58 to 60 in b1bffa6

	if ((isConnectionDataFetchNeeded || isFetchingData || isOnyxDataLoading) && shouldBlockView) {
	return <FullScreenLoadingIndicator />;
	}

The one that we need to focus on is hasConnectionsDataBeenFetched.

App/src/pages/workspace/withPolicyConnections.tsx

Lines 32 to 35 in b1bffa6

	const [hasConnectionsDataBeenFetched, hasConnectionsDataBeenFetchedResult] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED}${props.policy?.id ?? '-1'}`);
	const isOnyxDataLoading = isLoadingOnyxValue(hasConnectionsDataBeenFetchedResult);
	const isConnectionDataFetchNeeded =
	!isOnyxDataLoading && !isOffline && !!props.policy && (!!props.policy.areConnectionsEnabled || !isEmptyObject(props.policy.connections)) && !hasConnectionsDataBeenFetched;

The hasConnectionsDataBeenFetched will become true if the OPEN_POLICY_ACCOUNTING_PAGE read is completed. We request it only while online.

App/src/libs/actions/PolicyConnections.ts

Lines 8 to 33 in b1bffa6

	function openPolicyAccountingPage(policyID: string) {
	const hasConnectionsDataBeenFetchedKey = `${ONYXKEYS.COLLECTION.POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED}${policyID}` as const;
	const successData: OnyxUpdate[] = [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: hasConnectionsDataBeenFetchedKey,
	value: true,
	},
	];
	const failureData: OnyxUpdate[] = [
	{
	onyxMethod: Onyx.METHOD.MERGE,
	key: hasConnectionsDataBeenFetchedKey,
	value: false,
	},
	];
	const parameters: OpenPolicyAccountingPageParams = {
	policyID,
	};
	API.read(READ_COMMANDS.OPEN_POLICY_ACCOUNTING_PAGE, parameters, {
	successData,
	failureData,
	});

When we make a read request, it will wait for all write requests to complete first, by using the isReadyPromise.

App/src/libs/API/index.ts

Lines 170 to 179 in b1bffa6

	* Ensure all write requests on the sequential queue have finished responding before running read requests.
	* Responses from read requests can overwrite the optimistic data inserted by
	* write requests that use the same Onyx keys and haven't responded yet.
	*/
	function waitForWrites<TCommand extends ReadCommand>(command: TCommand) {
	if (PersistedRequests.getLength() > 0) {
	Log.info(`[API] '${command}' is waiting on ${PersistedRequests.getLength()} write commands`);
	}
	return SequentialQueue.waitForIdle();
	}

App/src/libs/Network/SequentialQueue.ts

Lines 270 to 272 in b1bffa6

	function waitForIdle(): Promise<unknown> {
	return isReadyPromise;
	}

However, in our case, the promise is never resolved even after all write requests are completed. That's because, while waiting for the promise to be resolved, the promise is recreated, so the old one is never resolved.

App/src/libs/Network/SequentialQueue.ts

Lines 155 to 158 in b1bffa6

	// Reset the isReadyPromise so that the queue will be flushed as soon as the request is finished
	isReadyPromise = new Promise((resolve) => {
	resolveIsReadyPromise = resolve;
	});

It's recreated every time flush is called. After the flush is called, isSequentialQueueRunning is set to true which prevents any subsequent flush from being processed when there is one already running.

App/src/libs/Network/SequentialQueue.ts

Line 153 in b1bffa6

isSequentialQueueRunning = true;

App/src/libs/Network/SequentialQueue.ts

Lines 136 to 139 in b1bffa6

	if (isSequentialQueueRunning) {
	Log.info('[SequentialQueue] Unable to flush. Queue is already running.');
	return;
	}

But in our case, isSequentialQueueRunning becomes false. That's because when we create the workspace and enable the accounting while offline, the BE will return shouldPauseQueue for EnablePolicyConnections which pause the queue.

App/src/libs/Network/SequentialQueue.ts

Lines 95 to 98 in b1bffa6

	if (response?.shouldPauseQueue) {
	Log.info("[SequentialQueue] Handled 'shouldPauseQueue' in response. Pausing the queue.");
	pause();
	}

When it processes the next request, the promise is resolved because the queue is paused,

App/src/libs/Network/SequentialQueue.ts

Lines 68 to 71 in b1bffa6

	if (isQueuePaused) {
	Log.info('[SequentialQueue] Unable to process. Queue is paused.');
	return Promise.resolve();
	}

which sets isSequentialQueueRunning to false.

App/src/libs/Network/SequentialQueue.ts

Lines 168 to 170 in b1bffa6

	process().finally(() => {
	Log.info('[SequentialQueue] Finished processing queue.');
	isSequentialQueueRunning = false;

When the queue is unpaused, we call flush again.

App/src/libs/Network/SequentialQueue.ts

Lines 188 to 198 in b1bffa6

	function unpause() {
	if (!isQueuePaused) {
	Log.info('[SequentialQueue] Unable to unpause queue. We are already processing.');
	return;
	}
	const numberOfPersistedRequests = PersistedRequests.getAll().length || 0;
	Log.info(`[SequentialQueue] Unpausing the queue and flushing ${numberOfPersistedRequests} requests`);
	isQueuePaused = false;
	flush();
	}

And because isSequentialQueueRunning is false, the isReadyPromise is recreated.

flush -> process (CreateWorkspace) -> process (EnablePolicyConnections) -> pause -> GetMissingOnyxMessages -> unpause -> flush (new isReadyPromise) -> process (ReconnectApp)

What changes do you think we should make in order to solve the problem?

We shouldn't reset the isReadyPromise when unpausing the queue. To do that, we can accept a new param for flush called resume. If it's true, then don't recreate isReadyPromise.

if (!resume) {
    // Reset the isReadyPromise so that the queue will be flushed as soon as the request is finished
    isReadyPromise = new Promise((resolve) => {
        resolveIsReadyPromise = resolve;
    });
}

And pass true when flushing from unpause.

App/src/libs/Network/SequentialQueue.ts

Lines 188 to 198 in b1bffa6

	function unpause() {
	if (!isQueuePaused) {
	Log.info('[SequentialQueue] Unable to unpause queue. We are already processing.');
	return;
	}
	const numberOfPersistedRequests = PersistedRequests.getAll().length || 0;
	Log.info(`[SequentialQueue] Unpausing the queue and flushing ${numberOfPersistedRequests} requests`);
	isQueuePaused = false;
	flush();
	}

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~021853787863203385081

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ntdiary (External)

[ntdiary]

Under review.

still verifying the RCA, will provide an update tomorrow.

[ntdiary]

@bernhardoj's RCA is accurate, with only a slight difference from what I checked. :)
pause is called here (in saveUpdateInformation), and shouldPauseQueue is also set to true here:

App/src/libs/Middleware/SaveResponseInOnyx.ts

Lines 38 to 45 in e453cfd

	// Save the update IDs to Onyx so they can be used to fetch incremental updates if the client gets out of sync from the server
	OnyxUpdates.saveUpdateInformation(responseToApply);
	// Ensure the queue is paused while the client resolves the gap in onyx updates so that updates are guaranteed to happen in a specific order.
	return Promise.resolve({
	...response,
	shouldPauseQueue: true,
	});

---

BTW, just a note, in our current SequentialQueue, we don’t have an exact concept of "current batch"

App/src/libs/Network/SequentialQueue.ts

Lines 251 to 257 in e000d80

	// If the queue is running this request will run once it has finished processing the current batch
	if (isSequentialQueueRunning) {
	isReadyPromise.then(flush);
	return;
	}
	flush();

we just ensure that all requests in persistedRequests are executed in order by calling flush in multiple places.

[ntdiary]

Additionally, @bernhardoj, just a small question: since our code calls unpause in several places, do you think adding this parameter will be safe enough to avoid causing regressions? 😂

[bernhardoj]

I think it should be safe. unpause will only take effect if the queue is currently paused. I think in all cases where pause and unpause happens, we don't want to have a new isReadyPromise because it's still in one "batch".

[ntdiary]

Nice, I think it's fine to move forward with @bernhardoj's proposal.
BTW, just a tiny thought, I'm not entirely sure if resume is clear and accurate enough (my English isn’t good), maybe can use shouldResetPromise(default value is true). Let's see if the internal engineer has any other thoughts. :)

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @srikarparsi, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

@ntdiary, @slafortune, @srikarparsi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[srikarparsi]

Will look at this tomorrow

[melvin-bot]

@ntdiary, @slafortune, @srikarparsi Whoops! This issue is 2 days overdue. Let's get this updated quick!

[srikarparsi]

I agree with the RCA and the proposed solution, lets move forward with @bernhardoj's proposal. I agree that shouldResetPromise is better than resume. One thing I'm not completely sure about is this comment, maybe @danieldoglas does this sound right to you? Saw that you worked on some of the SequentialQueue logic.

[danieldoglas]

thanks for that investigation @bernhardoj !

I agree that it looks a lot like an issue in the frontend, but I think it's an issue in the backend. If you're doing 2 sequential writes and constantly finding a gap between the requests (CreateWorkspace -> EnablePolicyConnections) then that means that the first request is not returning all the data the app needs.

@srikarparsi I recommend you try this flow locally, and find out which onyxUpdates were not returned, and check the backend code to confirm if they're being sent back on the HTTPs request before we start anything in the front end here.

[melvin-bot]

@ntdiary, @slafortune, @srikarparsi Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@ntdiary, @slafortune, @srikarparsi 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[melvin-bot]

@ntdiary @slafortune @srikarparsi this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[ntdiary]

@srikarparsi I recommend you try this flow locally, and find out which onyxUpdates were not returned, and check the backend code to confirm if they're being sent back on the HTTPs request before we start anything in the front end here.

It seems to be waiting for some investigation results from the backend. :)

[bernhardoj]

PR is ready

cc: @ntdiary

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.77-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix accounting tab load indefinitely #54110

If no regressions arise, payment will be issued on 2024-12-30. 🎊

For reference, here are some details about the assignees on this issue:

• @ntdiary requires payment through NewDot Manual Requests
• @bernhardoj requires payment through NewDot Manual Requests

[melvin-bot]

@ntdiary @slafortune @ntdiary The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @ntdiary owed $250 via NewDot
• Reviewer: @bernhardoj owed $250 via NewDot

BugZero Checklist (@slafortune)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1853787863203385081/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[ntdiary]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: https://github.com/Expensify/App/pull/24607/files#r1899540203

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: N/A

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
  No need, since we have added a unit test.

Regression Test Proposal Template

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree 👍 or 👎

[slafortune]

@ntdiary - Role C+ - owed $250 via NewDot
Reviewer: @bernhardoj -Role Contributor - owed $250 via NewDot

[JmillsExpensify]

$250 approved for @ntdiary

[bernhardoj]

Requested in ND.

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.80-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [No QA] Update comment followup from 54110 #54229

If no regressions arise, payment will be issued on 2025-01-13. 🎊

For reference, here are some details about the assignees on this issue:

• @ntdiary requires payment through NewDot Manual Requests
• @bernhardoj requires payment through NewDot Manual Requests

[melvin-bot]

@ntdiary @slafortune @ntdiary The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[JmillsExpensify]

$250 approved for @bernhardoj