Handle library scan failure gracefully

[mikiher]

This is in partial response to #3406

Library scan is a complex process and can fail in unexpected ways.
This tries wrap it up with a try-catch block, do the sensible things on error, save the scan log, and not crash the server.

There are a couple of questions I still have regarding the code:

• In the following code:

      if (canceled && !libraryScan.totalResults) {
        task.setFinished('Scan canceled')
        TaskManager.taskFinished(task)
  
        const emitData = libraryScan.getScanEmitData
        emitData.results = null
        return
      }

  what is the last part trying to achieve? it seems it has no effect. Also, the premature return prevents some cleanups from happening.
• In the following code:

    if (libraryScan.totalResults) {
      libraryScan.saveLog()
    }

  Why do we check library.Scan.totalResults? don't we want to save the scan log anyway?

[advplyr]

Way back when scans showed a persistent toast and when the scan emit data results were null that was when it would show "canceled" in the toast. It looks like that last part was accidently leftover when I refactored. It doesn't do anything.

If the user were to start a scan and cancel it quickly I don't think we want to update the lastScan, lastScanVersion and lastScanMetadataPrecedence. I don't think it would matter much if we did though.

I'm not sure why I setup the scan log to only save when it has updates. I think we can save it regardless and we can also save it if the scan was canceled.

[mikiher]

Thanks for the clarifications, I'll make the necessary changes.

…

On Thu, Sep 12, 2024, 19:33 advplyr ***@***.***> wrote: Way back when scans showed a persistent toast and when the scan emit data results were null that was when it would show "canceled" in the toast. It looks like that last part was accidently leftover when I refactored. It doesn't do anything. If the user were to start a scan and cancel it quickly I don't think we want to update the lastScan, lastScanVersion and lastScanMetadataPrecedence. I don't think it would matter much if we did though. I'm not sure why I setup the scan log to only save when it has updates. I think we can save it regardless and we can also save it if the scan was canceled. — Reply to this email directly, view it on GitHub <#3410 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMDFVRKQ65U2GMMEFRH5Q3ZWG66FAVCNFSM6AAAAABODT47ZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBWG42TSNZSHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mikiher]

If the user were to start a scan and cancel it quickly I don't think we want to update the lastScan, lastScanVersion and lastScanMetadataPrecedence. I don't think it would matter much if we did though.

Updating lastScan and lastScanVersion probably won't matter much, but if we update lastScanMetadataPrecedence and this scan had forceRescan=true (which happens when metadata precedence changes), then the next scan will have forceRescan=false (assuming data precedence hasn't changed again) - is that the intended behavior? I'm not sure. What do you think?

It seems to me that for the sake of simplicity, we should only update all those fields after a complete full scan was run - that's what I implemented.

This is ready for review now.

[advplyr]

Yeah you're right. We don't want to update lastScanMetadataPrecedence on a cancel because when the metadata precedence changes we want the next scan to do a full re-scan.

Thanks!