Skip to content

fix(dav): Prevent race condition in useDAVFiles#2511

Open
DerDreschner wants to merge 1 commit into
nextcloud-libraries:mainfrom
DerDreschner:fix/race-conditions
Open

fix(dav): Prevent race condition in useDAVFiles#2511
DerDreschner wants to merge 1 commit into
nextcloud-libraries:mainfrom
DerDreschner:fix/race-conditions

Conversation

@DerDreschner

Copy link
Copy Markdown

This PR fixes a race condition that occurs under high load in our CI runners using playwright.

Notice:   1 failed
    [chrome] › tests/playwright/e2e/files/live-photos.spec.ts:50:3 › Files: Live photos › 'Show hidden files' is enabled › Copies both files when copying the .mov 
  42 passed (8.0m)

  1) [chrome] › tests/playwright/e2e/files/live-photos.spec.ts:50:3 › Files: Live photos › 'Show hidden files' is enabled › Copies both files when copying the .mov 

    Test timeout of 30000ms exceeded.

    Error: page.waitForResponse: Test ended.

       at ../support/sections/CopyMoveDialogPage.ts:68

      66 |
      67 | 	private async confirm(label: string, method: 'COPY' | 'MOVE'): Promise<void> {
    > 68 | 		const done = this.page.waitForResponse((r) => r.request().method() === method
         | 		                       ^
      69 | 			&& /\/(remote|public)\.php\/dav\/files\//.test(r.url()))
      70 | 		await this.confirmButton(label).click()
      71 | 		await done
        at CopyMoveDialogPage.confirm (/home/runner/actions-runner/_work/server/server/tests/playwright/support/sections/CopyMoveDialogPage.ts:68:26)
        at CopyMoveDialogPage.copyToCurrentFolder (/home/runner/actions-runner/_work/server/server/tests/playwright/support/sections/CopyMoveDialogPage.ts:76:14)
        at /home/runner/actions-runner/_work/server/server/tests/playwright/e2e/files/live-photos.spec.ts:52:25

    Error Context: test-results/files-live-photos-Files-Li-c0514--files-when-copying-the-mov-chrome/error-context.md

Here is a summary of the issue that Claude created:

Root cause (confirmed end-to-end)

A race in @nextcloud/dialogs' useDAVFiles (lib/composables/dav.ts). loadDAVFiles aborts the previous load and starts a new one, but the aborted load's finally block unconditionally ran:

finally {
  abortController = undefined
  isLoading.value = false   // ← clobbers the NEWER load's state
}

So when fast navigation supersedes a load, the aborted one flips isLoading back to false while the newer load is still in flight and folder is still null. In FilePicker.vue, the confirm button is only disabled while isLoading — so in that window it's enabled with currentFolder = null, and confirming yields an empty selection → pickNodes() throws FilePickerClosed("No nodes selected")moveOrCopyAction catches it as "user cancelled" → no COPY is ever sent → the test's waitForResponse hangs to the timeout. On a fast machine the load wins the race (passes); under load (0.2 CPU / slow CI) the abort wins (fails). That's exactly the .mov failure — and why it's intermittent and file-agnostic.

Fix (branch fix/race-conditions in nextcloud-dialogs)

lib/composables/dav.ts — capture this invocation's controller and only clear shared state if it's still the current load:

finally {
  if (abortController === thisAbortController) {
    abortController = undefined
    isLoading.value = false
  }
}

Now a superseded load leaves isLoading true until the real load finishes, so the FilePicker never confirms with a null folder. Added a regression test in dav.spec.ts ("a superseded (aborted) load does not clear the loading state of the newer load").

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

Assisted-by: ClaudeCode:claude-fable-5
Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
@DerDreschner

Copy link
Copy Markdown
Author

@susnux : Please review, I have no rights on the nextcloud-libraries repos.

Comment thread lib/composables/dav.ts
Comment on lines +101 to +104
if (abortController === thisAbortController) {
abortController = undefined
isLoading.value = false
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if this can not just be simplified in general to only this change:

			if (abortController && abortController.signal.aborted) {
				abortController = undefined
				isLoading.value = false
			}

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants