Skip to content

fix: report files that could not be downloaded when the import finishes - #144

Merged
oleksandr-nc merged 7 commits into
mainfrom
fix/139/report-failed-downloads
Sep 25, 2026
Merged

oleksandr-nc merged 7 commits into
mainfrom
fix/139/report-failed-downloads

Conversation

@oleksandr-nc

@oleksandr-nc oleksandr-nc commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

Closes #139

Final part, following #142 and #143: a file that failed to download even after the fresh-URL retry was dropped silently, and the completion notification counted it as imported — it reported totalSeenNumber, which increments before the download is attempted.

What changes

  • Download failures are counted in a nb_failed_files user setting, accumulated across import batches the same way nb_imported_files already works, and reset at import start and finish. The names of the first ten failed files are kept as well.
  • The finish notification now reports the number of files actually downloaded (the accumulated imported counter, instead of the last batch's seen count) and, when there were failures, appends: "N files could not be downloaded, check the server logs for details." — with the kept file names in the notification body: "Could not download: DSC02561.jpg, DSC02561_thumb.jpg, and 9 more". Users without server-log access see which files are missing; the full list is in the log (redacted since fix: bound stalled file downloads and keep download URLs out of the log #142).
  • Files skipped because they already existed are counted too (one config write per listing page, since skips are frequent on re-imports) and reported: "1993 files were already there." Re-running an import into an already-imported folder used to end with a notification claiming everything was imported again; with the corrected imported-count alone it would have read "0 files were imported", which looks like a failure — now it explains itself.
  • Distinguishing downloaded / existing / failed outcomes properly in getFile() also fixes a pre-existing quirk where a successfully imported empty file was never counted as imported.
  • Notifications queued before an upgrade carry none of the new parameters and render exactly as before.

With this, all four problems from #139 are addressed: the indefinite hang (#142), the token leak into logs (#142), the expired listing URL (#143), and unreported missing files (this PR).

Tests

  • The counters: an import over a mocked four-file drive (one success, one double-failure, one empty file, one already existing) asserts the failed/skipped/imported counters, the kept failed-file names, and the exact download attempt sequence.
  • The finish path: a finishing job asserts the notification carries the accumulated counts and file names, and that all counters are reset afterwards.
  • The notifier: rendered subject and body with zero, one, few and many failures (including the "and N more" overflow), the skipped-files sentence, the re-run case, the pre-upgrade notification without the new parameters, and the unknown app/subject error paths.

composer cs:check, psalm and the suite (22 tests) pass locally on NC 33 / PHP 8.3. All notification variants were additionally rendered live on a real NC 35 instance through the OCS API to verify the actual L10N output.

A file that failed to download was dropped silently: the import carried on,
nothing recorded the loss, and the completion notification reported the number
of files *seen* - so the failed file was even counted as imported. For a data
migration tool, silently missing files is the worst failure mode.

Count download failures in a user setting accumulated across import batches,
the same way the imported-files counter works. When the import finishes, the
notification now reports the number of files actually downloaded and, if any,
the number of files that could not be downloaded. Their names are already in
the server log at warning level.

Notifications queued before an upgrade carry no failure count and keep
rendering as before.

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
…ication

Two notification improvements on top of the failed-files count:

Keep the names of the first ten files that could not be downloaded and list
them in the notification body, so users without access to the server logs
know which files are missing. The full list stays in the log.

Count files that were skipped because they already existed and say so in the
notification. Re-running an import into the same folder used to end with
"0 files were imported", which read as a failure; it now explains itself.

Distinguishing downloaded, existing and failed files properly also fixes a
quirk where an imported empty file was never counted as imported.

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
The notifier throws UnknownNotificationException since #149. It extends
InvalidArgumentException, so these assertions kept passing after the merge,
but they would also pass if the notifier went back to the deprecated
exception.

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
IRootFolder::getUserFolder() declares an IUserFolder return type since
Nextcloud 36, so PHPUnit refuses the plain Folder mock these two tests
returned. IUserFolder does not exist on older versions, which keep the
Folder mock.

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
@oleksandr-nc
oleksandr-nc force-pushed the fix/139/report-failed-downloads branch 2 times, most recently from 7273403 to 96bf4ff Compare September 23, 2026 10:49
The integration test imported a few files and stopped, so it never reached
what this branch changes: nothing exercised a run in which files are already
there, and the skipped and failed counters were not even reported.

The job now imports a second time over the files the first run brought and
expects them to be counted as skipped, with no failed download on a healthy
drive. The drive behind the secrets holds more than 500 MB, so importing it
completely is out of reach for CI; the second run therefore starts while the
first import is still in progress, after clearing the two flags that a user
would see expire an hour later.

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
@oleksandr-nc
oleksandr-nc force-pushed the fix/139/report-failed-downloads branch 2 times, most recently from b7a7926 to cd04943 Compare September 23, 2026 11:43
The failure path of the import could only be checked with unit tests: a real
drive cannot be made to fail a download on demand, cannot produce an empty
file at will and, being one small account, never returns a second listing
page. The notification this branch rewrites was never rendered in a test
that had actually imported anything either.

A stub of the Graph endpoints the import uses closes that gap. It serves a
fixed drive with a file that downloads, an empty one, one that fails with
the URL from the listing and succeeds with a freshly fetched one, one that
fails both times, one that the test puts in the target folder beforehand,
and a subfolder, with the first listing page carrying a next link. The app
is pointed at it through a new app config value that defaults to Microsoft
Graph, so nothing changes outside tests.

The job then checks the files on disk, the requests the app made, the
notification text and that the counters are reset, and it needs no secrets,
so it also runs on pull requests that cannot see them.

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
Importing a second time over the files of the first run reported nothing on
one leg: every counter was zero and the import was over after one cron run.
`importFiles()` asks for the drive quota first and returns straight away when
that request fails, which makes the job take its finish path, reset the
counters and log nothing at all. A large real drive that was just hammered
with downloads is the wrong place to depend on that request succeeding.

The stubbed Graph job covers the same behaviour without the flakiness: it
puts a file in the target folder beforehand and asserts that the app never
asks to download it.

Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
@oleksandr-nc
oleksandr-nc force-pushed the fix/139/report-failed-downloads branch from d29bf32 to 87decb3 Compare September 23, 2026 11:53
@oleksandr-nc
oleksandr-nc merged commit d4d79d5 into main Sep 25, 2026
25 checks passed
@oleksandr-nc
oleksandr-nc deleted the fix/139/report-failed-downloads branch September 25, 2026 07:37
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.

Files import hangs for hours on an expired downloadUrl — no read timeout, and the failed file is then dropped for good

1 participant