refactor(scanner): Don't create fake scan results when path scan fails

To resolve an issue where scan results from failed scans were reused even
if the problem was resolved in newer scans, change the behavior of the
path scanners so that they don't create empty scan results just to keep
track of issues, and instead collect these kinds of issues to the
`ScannerRun` issues map.

Resolves #10054.

Signed-off-by: Johanna Lamppu <[email protected]>

Author: lamppu

scanner/src/main/kotlin/ScanController.kt

@@ -65,10 +65,10 @@ internal class ScanController(
     private val nestedProvenanceResolutionIssues = mutableMapOf<Identifier, Issue>()
 
     /**
-     * A map of [Identifier]s associated with a list of [Issue]s that occurred during a scan besides the issues
+     * A map of [Identifier]s associated with a set of [Issue]s that occurred during a scan besides the issues
      * created by the scanners themselves as part of the [ScanSummary].
      */
-    private val issues = mutableMapOf<Identifier, MutableList<Issue>>()
+    private val issues = mutableMapOf<Identifier, MutableSet<Issue>>()
 
     /**
      * A map of [KnownProvenance]s to their resolved [NestedProvenance]s.
@@ -124,7 +124,7 @@ internal class ScanController(
     fun getAllFileLists(): Map<KnownProvenance, FileList> = fileLists
 
     fun addIssue(id: Identifier, issue: Issue) {
-        issues.getOrPut(id) { mutableListOf() } += issue
+        issues.getOrPut(id) { mutableSetOf() } += issue
     }
 
     /**
@@ -197,6 +197,11 @@ internal class ScanController(
      */
     fun getNestedProvenanceResolutionIssue(id: Identifier): Issue? = nestedProvenanceResolutionIssues[id]
 
+    /**
+     * Get issues that are not part of the scan summaries.
+     */
+    fun getIssues(): Map<Identifier, Set<Issue>> = issues
+
     /**
      * Get the [NestedProvenance] for the provided [id], or null if no nested provenance for the [id] is available.
      */

scanner/src/main/kotlin/Scanner.kt

@@ -42,7 +42,6 @@ import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.PackageType
 import org.ossreviewtoolkit.model.ProvenanceResolutionResult
 import org.ossreviewtoolkit.model.ScanResult
-import org.ossreviewtoolkit.model.ScanSummary
 import org.ossreviewtoolkit.model.ScannerRun
 import org.ossreviewtoolkit.model.TextLocation
 import org.ossreviewtoolkit.model.VcsInfo
@@ -149,6 +148,7 @@ class Scanner(
             config = scannerConfig,
             provenances = projectResults.provenances + packageResults.provenances,
             scanResults = projectResults.scanResults + packageResults.scanResults,
+            issues = projectResults.issues + packageResults.issues,
             files = projectResults.files + packageResults.files,
             scanners = projectResults.scanners + packageResults.scanners
         )
@@ -234,12 +234,15 @@ class Scanner(
         val scannerIds = scannerWrappers.mapTo(mutableSetOf()) { it.descriptor.id }
         val scanners = packages.associateBy({ it.id }) { scannerIds }
 
+        val issues = controller.getIssues()
+
         return ScannerRun.EMPTY.copy(
             config = scannerConfig,
             provenances = provenances,
             scanResults = scanResults,
             files = files,
-            scanners = scanners
+            scanners = scanners,
+            issues = issues
         )
     }
 
@@ -452,7 +455,7 @@ class Scanner(
 
             logger.info { "Scanning $provenance (${index + 1} of ${provenances.size})..." }
 
-            val scanResults = scanPath(provenance, scannersWithoutResults, context)
+            val scanResults = scanPath(provenance, scannersWithoutResults, context, controller)
 
             scanResults.forEach { (scanner, scanResult) ->
                 val completedPackages = controller.getPackagesCompletedByProvenance(scanner, provenance)
@@ -573,7 +576,8 @@ class Scanner(
     private fun scanPath(
         provenance: KnownProvenance,
         scanners: List<PathScannerWrapper>,
-        context: ScanContext
+        context: ScanContext,
+        controller: ScanController
     ): Map<PathScannerWrapper, ScanResult> {
         val downloadDir = try {
             provenanceDownloader.download(provenance)
@@ -582,58 +586,54 @@ class Scanner(
                 "Downloader", "Could not download provenance $provenance: ${e.collectMessages()}"
             )
 
-            val time = Instant.now()
-            val summary = ScanSummary(
-                startTime = time,
-                endTime = time,
-                issues = listOf(issue)
-            )
-
-            return scanners.associateWith { scanner ->
-                ScanResult(
-                    provenance = provenance,
-                    scanner = scanner.details,
-                    summary = summary
+            controller.getIdsByProvenance().getValue(provenance).forEach { id ->
+                controller.addIssue(
+                    id,
+                    issue
                 )
             }
+
+            return emptyMap()
         }
 
-        val results = scanners.associateWith { scanner ->
+        val results = scanners.mapNotNull { scanner ->
             logger.info { "Scan of $provenance with path scanner '${scanner.descriptor.displayName}' started." }
 
             // Filter the scan context to hide the excludes from scanner with scan matcher.
             val filteredContext = if (scanner.matcher == null) context else context.copy(excludes = null)
 
             val summary = runCatching {
                 scanner.scanPath(downloadDir, filteredContext)
-            }.getOrElse { e ->
+            }.onFailure { e ->
                 val issue = scanner.createAndLogIssue(
                     "Failed to scan $provenance with path scanner '${scanner.descriptor.displayName}': " +
                         e.collectMessages()
                 )
 
-                val time = Instant.now()
-                ScanSummary(
-                    startTime = time,
-                    endTime = time,
-                    issues = listOf(issue)
-                )
-            }
+                controller.getIdsByProvenance().getValue(provenance).forEach { id ->
+                    controller.addIssue(
+                        id,
+                        issue
+                    )
+                }
+            }.getOrNull()
 
             logger.info { "Scan of $provenance with path scanner '${scanner.descriptor.displayName}' finished." }
 
-            val summaryWithMappedLicenses = summary.copy(
-                licenseFindings = summary.licenseFindings.mapTo(mutableSetOf()) {
-                    val licenseString = it.license.toString()
-                    it.copy(
-                        license = licenseString.mapLicense(scannerConfig.detectedLicenseMapping).toSpdx(),
-                        location = it.location.withRelativePath(downloadDir)
-                    )
-                }
-            )
+            summary?.let {
+                val summaryWithMappedLicenses = summary.copy(
+                    licenseFindings = summary.licenseFindings.mapTo(mutableSetOf()) { finding ->
+                        val licenseString = finding.license.toString()
+                        finding.copy(
+                            license = licenseString.mapLicense(scannerConfig.detectedLicenseMapping).toSpdx(),
+                            location = finding.location.withRelativePath(downloadDir)
+                        )
+                    }
+                )
 
-            ScanResult(provenance, scanner.details, summaryWithMappedLicenses)
-        }
+                scanner to ScanResult(provenance, scanner.details, summaryWithMappedLicenses)
+            }
+        }.toMap()
 
         downloadDir.safeDeleteRecursively()
 

scanner/src/test/kotlin/ScannerTest.kt

@@ -23,12 +23,15 @@ import io.kotest.assertions.throwables.shouldThrow
 import io.kotest.core.spec.style.WordSpec
 import io.kotest.matchers.collections.beEmpty
 import io.kotest.matchers.collections.containExactly
+import io.kotest.matchers.collections.shouldHaveSize
 import io.kotest.matchers.maps.beEmpty as beEmptyMap
 import io.kotest.matchers.maps.containExactly
+import io.kotest.matchers.maps.haveSize
 import io.kotest.matchers.nulls.shouldNotBeNull
 import io.kotest.matchers.should
 import io.kotest.matchers.shouldBe
 import io.kotest.matchers.shouldNot
+import io.kotest.matchers.string.shouldContain
 
 import io.mockk.coEvery
 import io.mockk.every
@@ -39,6 +42,7 @@ import io.mockk.verify
 import java.io.File
 import java.io.IOException
 
+import org.ossreviewtoolkit.downloader.DownloadException
 import org.ossreviewtoolkit.model.ArtifactProvenance
 import org.ossreviewtoolkit.model.Hash
 import org.ossreviewtoolkit.model.HashAlgorithm
@@ -351,6 +355,84 @@ class ScannerTest : WordSpec({
                 provenanceDownloader.download(any())
             }
         }
+
+        "return an issue and no scan result if source download fails" {
+            val pkg = Package.new(name = "repository").copy(
+                vcsProcessed = VcsInfo.valid()
+            )
+
+            val scannerWrapper = FakePathScannerWrapper()
+            val provenanceDownloader = spyk(FakeProvenanceDownloader())
+
+            every { provenanceDownloader.download(any()) } throws DownloadException("Test download error")
+
+            val scanner = createScanner(
+                provenanceDownloader = provenanceDownloader,
+                packageScannerWrappers = listOf(scannerWrapper)
+            )
+
+            val run = scanner.scan(setOf(pkg), createContext())
+
+            run.scanResults should beEmpty()
+            run.issues should haveSize(1)
+            run.issues[pkg.id].shouldNotBeNull().shouldHaveSize(1)
+
+            run.issues[pkg.id]?.first().shouldNotBeNull {
+                source shouldBe "Downloader"
+                message shouldContain "Test download error"
+            }
+        }
+
+        "return an issue and no scan result if scanning path fails" {
+            val pkg = Package.new(name = "repository").copy(
+                vcsProcessed = VcsInfo.valid()
+            )
+
+            val scannerWrapper = spyk(FakePathScannerWrapper())
+            val provenanceDownloader = FakeProvenanceDownloader()
+
+            every { scannerWrapper.scanPath(any(), any()) } throws Exception("Test scan failure")
+
+            val scanner = createScanner(
+                provenanceDownloader = provenanceDownloader,
+                packageScannerWrappers = listOf(scannerWrapper)
+            )
+
+            val run = scanner.scan(setOf(pkg), createContext())
+
+            run.scanResults should beEmpty()
+            run.issues should haveSize(1)
+            run.issues[pkg.id].shouldNotBeNull().shouldHaveSize(1)
+
+            run.issues[pkg.id]?.first().shouldNotBeNull {
+                source shouldBe "fake"
+                message shouldContain "Test scan failure"
+            }
+        }
+
+        "not write any scan results to storage if a scan fails" {
+            val pkg = Package.new(name = "repository").copy(
+                vcsProcessed = VcsInfo.valid()
+            )
+
+            val storageWriter = spyk(FakeProvenanceBasedStorageWriter())
+            val scannerWrapper = spyk(FakePathScannerWrapper())
+            val provenanceDownloader = FakeProvenanceDownloader()
+
+            every { scannerWrapper.scanPath(any(), any()) } throws Exception("Test scan failure")
+
+            val scanner = createScanner(
+                provenanceDownloader = provenanceDownloader,
+                packageScannerWrappers = listOf(scannerWrapper),
+                storageWriters = listOf(storageWriter)
+            )
+
+            scanner.scan(setOf(pkg), createContext())
+
+            verify(exactly = 0) {
+                storageWriter.write(any())
+            }
+        }
     }
 
     "scanning with a package based storage reader" should {