fix(blobExec): exclude a blob on a tokenizer error, never keep its source

README.md

@@ -190,9 +190,9 @@ would delete a memo of 10,000 entries or more; `--force-clean` overrides it.
 on a **blob denylist** (`BlobDenylistEntries.scala`). They never reach the tokenizer,
 are dropped from the rewritten trees, and are counted as `blobsDenylisted`.
 
-A blob whose tokenizer times out or reports a parser crash is excluded the same
-way without an entry: it is named on an `EXCLUDED failed blob` line, tried once
-per run, and the run exits 0. An entry still saves the `--blob-timeout` (600 s by
+A blob whose tokenizer times out, reports a parser crash or exits non-zero is
+excluded the same way without an entry: it is named on an `EXCLUDED failed blob`
+line, tried once per run, and the run exits 0. An entry still saves the `--blob-timeout` (600 s by
 default) that a known hang costs on each run.
 
 Example run (cregit run on itself):

blobExec/README.org

@@ -38,7 +38,7 @@ java -jar blobExec-0.1.0-assembly.jar [--abort-on-error] \
 | =<db.sqlite>=      | SQLite mapping file. Created with schema if absent                         |
 | =<command>=        | Absolute path to the per-blob script                                       |
 | =<fileMaskRegex>=  | Regex matched against each blob's filename, e.g. ='\.[ch]$'=               |
-| =--abort-on-error= | Exit 2 on the first non-zero =<command>= exit (default: skip the blob)     |
+| =--abort-on-error= | Exit 2 on the first non-zero =<command>= exit (default: exclude the blob)  |
 
 For each blob whose filename matches =<fileMaskRegex>=, =<command>= is
 invoked with:
@@ -48,8 +48,9 @@ invoked with:
 - env =BFG_FILENAME= = blob's basename in the tree
 - env =BFG_PATH=     = blob's full repo-root relative path (e.g. =src/lib/foo.c=)
 
-The command's stdout replaces the blob's bytes. Non-zero exit or
-identical-to-input output leaves the blob unchanged. Every rewritten commit
+The command's stdout replaces the blob's bytes. Identical-to-input output
+leaves the blob unchanged. A non-zero exit excludes the blob from the tree
+and names it on an =EXCLUDED failed blob= line. Every rewritten commit
 gets =Former-commit-id: <orig-sha>= appended to its message (byte-for-byte
 compatible with BFG's =FormerCommitFooter=, which the cregit =remapCommits=
 step parses from each commit's last line).

blobExec/src/main/scala/cregit/blobexec/BlobExec.scala

@@ -38,6 +38,7 @@ object BlobExec {
       timeoutSeconds: Int = DefaultTimeoutSeconds,
       onTimeout: () => Unit = () => (),
       onParserCrash: () => Unit = () => (),
+      onTokenizerError: () => Unit = () => (),
       workerPool: Option[TokenizerWorkerPool] = None
   ): Outcome = {
     val env = Seq("BFG_BLOB" -> origSha, "BFG_FILENAME" -> filename, "BFG_PATH" -> fullPath)
@@ -77,9 +78,15 @@ object BlobExec {
         onParserCrash()
         Outcome.Skip
 
-      case ChildRunner.Outcome.Exited(status, _, stderr) if status != 0 =>
+      case ChildRunner.Outcome.Exited(status, _, stderr) if status != 0 && abortOnError =>
         logError(command, origSha, fullPath, status, stderr)
-        if (abortOnError) Outcome.Abort(stderr, status) else Outcome.Skip
+        Outcome.Abort(stderr, status)
+
+      case ChildRunner.Outcome.Exited(status, _, stderr) if status != 0 =>
+        System.err.println(tokenizerErrorExcluded(command, origSha, fullPath, status))
+        printStderr(command, origSha, fullPath, stderr)
+        onTokenizerError()
+        Outcome.Skip
 
       case ChildRunner.Outcome.Exited(_, stdout, _) if stdout.isEmpty && bytes.nonEmpty =>
         System.err.println(
@@ -98,6 +105,10 @@ object BlobExec {
     }
   }
 
+  private def tokenizerErrorExcluded(command: String, origSha: String, fullPath: String, status: Int): String =
+    s"Warning: command [$command] exited $status on blob $origSha at path [$fullPath]: blob excluded, " +
+      "so its source never goes into the tokenized tree"
+
   private def logError(
       command: String,
       origSha: String,

blobExec/src/main/scala/cregit/blobexec/Main.scala

@@ -58,7 +58,7 @@ object Main {
   private[blobexec] val Usage =
     raw"""Usage: blobExec [options] <src.git> <dst.git> <db.sqlite> <command> <fileMaskRegex>
       |
-      |  --abort-on-error           exit 2 on the first error from <command> instead of skipping the blob
+      |  --abort-on-error           exit 2 on the first error from <command> instead of excluding the blob
       |  --pipeline                 look-ahead pipelined walker; same output as the serial walker
       |  --pipeline-trees           as --pipeline, and also assemble trees on the worker pool
       |  --shard=K/N                tree-only run of shard K (0-based) of N commit ranges, no commits or
@@ -85,8 +85,8 @@ object Main {
       |  <fileMaskRegex>            regex matched against each blob's filename (e.g. '\.[ch]$$')
       |
       |  Denylisted (${BlobDenylist.EntriesSource}) and oversized blobs, and blobs whose tokenizer
-      |  times out or crashes, are left out of the trees and named on an EXCLUDED line; they do not
-      |  change the exit status.
+      |  times out, crashes or exits non-zero, are left out of the trees and named on an EXCLUDED
+      |  line; they do not change the exit status.
       |
       |  Exit status: 0 clean, 1 usage, 2 aborted on a command error, 3 cache mismatch,
       |  ${Walker.StalledExitStatus} stalled, ${RetokenizeIneffectiveExitStatus} --retokenize would invalidate nothing.
@@ -200,11 +200,12 @@ object Main {
         "not contain. The files are absent from the tokenized repository, not present as raw source, " +
         "so they produce no blame and no dataset row. This is not a failure and does not affect the exit status."
 
-    def failed(timedOut: Long, crashed: Long, blobTimeoutSeconds: Int): String =
-      s"blobExec: ${timedOut + crashed} blob(s) were excluded because their tokenizer failed " +
-        s"($timedOut timed out after ${blobTimeoutSeconds}s, $crashed reported a parser crash). Each " +
-        "one is named on an 'EXCLUDED failed blob' line above; those lines are the record of what " +
-        "this project's dataset does not contain. This does not affect the exit status."
+    def failed(timedOut: Long, crashed: Long, errored: Long, blobTimeoutSeconds: Int): String =
+      s"blobExec: ${timedOut + crashed + errored} blob(s) were excluded because their tokenizer failed " +
+        s"($timedOut timed out after ${blobTimeoutSeconds}s, $crashed reported a parser crash, " +
+        s"$errored exited non-zero). Each one is named on an 'EXCLUDED failed blob' line above; " +
+        "those lines are the record of what this project's dataset does not contain. This does " +
+        "not affect the exit status."
   }
 
   /** `--name=<value>` as a pattern: `case Flag(value) =>`. */
@@ -460,15 +461,18 @@ object Main {
       s"blobsOversized=${stats.blobsOversized} " +
       s"blobsDenylisted=${stats.blobsDenylisted} " +
       s"blobsParserCrashed=${stats.blobsParserCrashed} " +
+      s"blobsTokenizerFailed=${stats.blobsTokenizerFailed} " +
       s"aborted=${stats.aborted}"
 
   private def reportExclusions(stats: WalkStats, blobTimeoutSeconds: Int): Unit = {
     if (stats.blobsDenylisted > 0)
       System.err.println(Messages.denylisted(stats.blobsDenylisted, BlobDenylist.shipped.size))
     if (stats.blobsOversized > 0)
       System.err.println(Messages.oversized(stats.blobsOversized))
-    if (stats.blobsTimedOut + stats.blobsParserCrashed > 0)
-      System.err.println(Messages.failed(stats.blobsTimedOut, stats.blobsParserCrashed, blobTimeoutSeconds))
+    val failed = stats.blobsTimedOut + stats.blobsParserCrashed + stats.blobsTokenizerFailed
+    if (failed > 0)
+      System.err.println(
+        Messages.failed(stats.blobsTimedOut, stats.blobsParserCrashed, stats.blobsTokenizerFailed, blobTimeoutSeconds))
   }
 
   private def openSrc(path: Path): FileRepository = {

blobExec/src/main/scala/cregit/blobexec/Walker.scala

@@ -26,6 +26,8 @@ final case class WalkStats(
     blobsDenylisted: Long,
     /** Tokenizer exited [[BlobExec.ParserCrashExitCode]]. */
     blobsParserCrashed: Long = 0L,
+    /** Tokenizer exited non-zero for any other reason, without `--abort-on-error`. */
+    blobsTokenizerFailed: Long = 0L,
     blobCommandExecutions: Long,
     originalBlobCopyRequests: Long,
     originalBlobCopies: Long,
@@ -69,6 +71,7 @@ final class Walker(
   private val blobsOversized                 = new LongAdder
   private val blobsDenylisted                = new LongAdder
   private val blobsParserCrashed             = new LongAdder
+  private val blobsTokenizerFailed           = new LongAdder
   private val originalBlobCopyRequests       = new LongAdder
   private val originalBlobCopies             = new LongAdder
   private val originalBlobAlreadyPresent     = new LongAdder
@@ -204,6 +207,7 @@ final class Walker(
         blobsOversized         = blobsOversized.sum(),
         blobsDenylisted        = blobsDenylisted.sum(),
         blobsParserCrashed     = blobsParserCrashed.sum(),
+        blobsTokenizerFailed   = blobsTokenizerFailed.sum(),
         blobCommandExecutions       = blobCommandExecutions.sum(),
         originalBlobCopyRequests    = originalBlobCopyRequests.sum(),
         originalBlobCopies          = originalBlobCopies.sum(),
@@ -243,6 +247,7 @@ final class Walker(
       blobsOversized         = blobsOversized.sum(),
       blobsDenylisted        = blobsDenylisted.sum(),
       blobsParserCrashed     = blobsParserCrashed.sum(),
+      blobsTokenizerFailed   = blobsTokenizerFailed.sum(),
       blobCommandExecutions       = blobCommandExecutions.sum(),
       originalBlobCopyRequests    = originalBlobCopyRequests.sum(),
       originalBlobCopies          = originalBlobCopies.sum(),
@@ -778,6 +783,7 @@ final class Walker(
         timeoutSeconds = blobTimeoutSeconds,
         onTimeout      = () => { blobsTimedOut.increment(); failure.set("timeout") },
         onParserCrash  = () => { blobsParserCrashed.increment(); failure.set("parser-crash") },
+        onTokenizerError = () => { blobsTokenizerFailed.increment(); failure.set("tokenizer-error") },
         workerPool     = workerPool
       )
       val res = outcome match {
@@ -926,7 +932,8 @@ final class Walker(
               inserter     = workerInserter,
               timeoutSeconds = blobTimeoutSeconds,
               onTimeout      = () => { blobsTimedOut.increment(); failure.set("timeout") },
-              onParserCrash  = () => { blobsParserCrashed.increment(); failure.set("parser-crash") }
+              onParserCrash  = () => { blobsParserCrashed.increment(); failure.set("parser-crash") },
+              onTokenizerError = () => { blobsTokenizerFailed.increment(); failure.set("tokenizer-error") }
             )
             val failed = Option(failure.get())
             failed.foreach(noteFailed(task, _))
@@ -957,7 +964,7 @@ final class Walker(
         case (task, BlobExec.Outcome.Replace(newId)) =>
           (task.origId.name, task.fullPath) -> newId
         case (task, BlobExec.Outcome.Skip) =>
-          // identical, or non-zero-exit (with abortOnError=false)
+          // identical output: `flatten` dropped every failed blob
           (task.origId.name, task.fullPath) -> task.origId
       }.toMap
       MissResolution(ids, abort = false)

blobExec/src/test/scala/cregit/blobexec/BlobExecSpec.scala

@@ -141,6 +141,20 @@ class BlobExecSpec extends AnyFunSuite with Matchers with BeforeAndAfterAll {
     crashes.get shouldEqual 1
   }
 
+  test("a tokenizer that exits 1 with output is excluded and never kept as source") {
+    val errors  = new java.util.concurrent.atomic.AtomicInteger(0)
+    val others  = new java.util.concurrent.atomic.AtomicInteger(0)
+    val cmd = shellScript("echo 'partial tokens'; exit 1")
+    BlobExec.run("int main(){}".getBytes(UTF_8), sampleSha, "x.c", "src/x.c", cmd,
+                 abortOnError = false, inserter,
+                 onTimeout        = () => { others.incrementAndGet(); () },
+                 onParserCrash    = () => { others.incrementAndGet(); () },
+                 onTokenizerError = () => { errors.incrementAndGet(); () }
+    ) shouldBe BlobExec.Outcome.Skip
+    errors.get shouldEqual 1
+    others.get shouldEqual 0
+  }
+
   test("a parser crash skips one blob even with abortOnError, like a timeout") {
     val cmd = shellScript(s"exit ${BlobExec.ParserCrashExitCode}")
     BlobExec.run("int main(){}".getBytes(UTF_8), sampleSha, "x.c", "src/x.c", cmd,

blobExec/src/test/scala/cregit/blobexec/FailedBlobSpec.scala

@@ -37,6 +37,12 @@ class FailedBlobSpec extends AnyFunSuite with Matchers with BeforeAndAfterAll {
 
   private val hang  = "sleep 300 & sleep 300"
   private val crash = s"cat >/dev/null; exit ${BlobExec.ParserCrashExitCode}"
+  private val error = "cat; exit 1"
+
+  private val failures = Seq[(String, String, WalkStats => Long)](
+    ("timed-out", hang, _.blobsTimedOut),
+    ("crashed", crash, _.blobsParserCrashed),
+    ("errored", error, _.blobsTokenizerFailed))
 
   private def run(src: Repository, dstPath: Path, dbPath: Path, command: String, mode: String): WalkStats = {
     val dst = FileRepositoryBuilder.create(dstPath.toFile).asInstanceOf[FileRepository]
@@ -82,7 +88,7 @@ class FailedBlobSpec extends AnyFunSuite with Matchers with BeforeAndAfterAll {
   }
 
   for (mode <- Seq("serial", "pipeline", "pipeline-trees")) {
-    for ((kind, onB) <- Seq("timed-out" -> hang, "crashed" -> crash))
+    for ((kind, onB, counted) <- failures)
       test(s"[$mode] a $kind blob is excluded and the commit is still folded") {
         val f = fixture(extraCommitInDeep = false)
         val dstPath = f.dir.resolve("dst.git")
@@ -93,7 +99,8 @@ class FailedBlobSpec extends AnyFunSuite with Matchers with BeforeAndAfterAll {
 
         stats.aborted shouldBe false
         stats.commitsProcessed shouldEqual 1
-        (stats.blobsTimedOut + stats.blobsParserCrashed) shouldEqual 1
+        (stats.blobsTimedOut + stats.blobsParserCrashed + stats.blobsTokenizerFailed) shouldEqual 1
+        counted(stats) shouldEqual 1
         Main.exitStatus(stats) shouldEqual 0
         val m = Mapping.open(dbPath, command, """\.c$""")
         try {

blobExec/src/test/scala/cregit/blobexec/TokenizerWorkerPoolSpec.scala

@@ -42,6 +42,7 @@ class TokenizerWorkerPoolSpec extends AnyFunSuite with Matchers with BeforeAndAf
          |    sleep 2 if $name eq 'slow.c';
          |    if    ($name eq 'crash.c')   { reply(33, '') }
          |    elsif ($name eq 'timeout.c') { reply(124, '') }
+         |    elsif ($name eq 'error.c')   { reply(1, $body) }
          |    elsif ($name eq 'budget.c')  { reply(0, $timeout) }
          |    else                         { reply(0, uc $body) }
          |}
@@ -73,22 +74,31 @@ class TokenizerWorkerPoolSpec extends AnyFunSuite with Matchers with BeforeAndAf
     }
   }
 
-  test("a worker's timeout and parser crash take BlobExec's timeout and crash paths") {
+  private def runBlob(pool: TokenizerWorkerPool, filename: String, abortOnError: Boolean = true) = {
     val inserter = new InMemoryRepository(new DfsRepositoryDescription("pool-spec")).newObjectInserter()
+    var called = "none"
+    val outcome = BlobExec.run(
+      "body".getBytes(UTF_8), "a" * 40, filename, s"src/$filename", "tokenBySha.pl",
+      abortOnError = abortOnError, inserter = inserter,
+      onTimeout = () => called = "timeout", onParserCrash = () => called = "parser-crash",
+      onTokenizerError = () => called = "tokenizer-error",
+      workerPool = Some(pool))
+    (outcome, called)
+  }
+
+  test("a worker's timeout and parser crash take BlobExec's timeout and crash paths") {
     withPool(1) { pool =>
-      def run(filename: String): (BlobExec.Outcome, String) = {
-        var called = "none"
-        val outcome = BlobExec.run(
-          "body".getBytes(UTF_8), "a" * 40, filename, s"src/$filename", "tokenBySha.pl",
-          abortOnError = true, inserter = inserter,
-          onTimeout = () => called = "timeout", onParserCrash = () => called = "parser-crash",
-          workerPool = Some(pool))
-        (outcome, called)
-      }
-      run("timeout.c") shouldEqual ((BlobExec.Outcome.Skip, "timeout"))
-      run("crash.c") shouldEqual ((BlobExec.Outcome.Skip, "parser-crash"))
-      run("die.c") shouldEqual ((BlobExec.Outcome.Skip, "timeout"))
-      run("ok.c")._1 shouldBe a[BlobExec.Outcome.Replace]
+      runBlob(pool, "timeout.c") shouldEqual ((BlobExec.Outcome.Skip, "timeout"))
+      runBlob(pool, "crash.c") shouldEqual ((BlobExec.Outcome.Skip, "parser-crash"))
+      runBlob(pool, "die.c") shouldEqual ((BlobExec.Outcome.Skip, "timeout"))
+      runBlob(pool, "ok.c")._1 shouldBe a[BlobExec.Outcome.Replace]
+    }
+  }
+
+  test("a worker's exit 1 with output excludes the blob, or aborts with --abort-on-error") {
+    withPool(1) { pool =>
+      runBlob(pool, "error.c", abortOnError = false) shouldEqual ((BlobExec.Outcome.Skip, "tokenizer-error"))
+      runBlob(pool, "error.c")._1 shouldBe a[BlobExec.Outcome.Abort]
     }
   }
 

generate_dataset/DATASET.md

@@ -302,7 +302,7 @@ produces no blame and no dataset row rather than rows of unparsed text:
 |---|---|---|
 | the **blob denylist**, `BlobDenylistEntries.scala` | `blobsDenylisted`, plus one `EXCLUDED denylisted blob` line per blob naming its sha, path and cited reason | no rows for those blobs. The list is compiled into the jar |
 | **oversized** blobs (at or above JGit's stream-file threshold) | `blobsOversized`, plus one `EXCLUDED oversized blob` line each | no rows for those blobs |
-| a blob the tokenizer **timed out** or **crashed** on | `blobsTimedOut` or `blobsParserCrashed`, plus one `EXCLUDED failed blob` line each with `reason=timeout` or `reason=parser-crash` | no rows for those blobs. The run still exits 0 |
+| a blob the tokenizer **timed out**, **crashed** or **failed** on | `blobsTimedOut`, `blobsParserCrashed` or `blobsTokenizerFailed`, plus one `EXCLUDED failed blob` line each with `reason=timeout`, `reason=parser-crash` or `reason=tokenizer-error` | no rows for those blobs. The run still exits 0 |
 
 The denylist holds 209 blob ids, keyed by content, in three srcML 1.1.0 defects:
 8 Java blobs that do not terminate (upstream

tokenizeByBlobId/WORKER_PROTOCOL.md

@@ -36,6 +36,6 @@ worker -> JVM:  RES <exit> <outLength> <errLength>\n<out><err>
   timeout it kills the group and the helpers, and answers `124`.
 - If no answer comes in `timeoutSeconds + 5` seconds, blobExec kills the
   worker and its children, starts a new worker, and reports a timeout.
-- If the worker stops in a request, blobExec reports a tokenizer error for
-  that blob and starts a new worker.
+- If the worker stops in a request, blobExec reports a timeout for that
+  blob and starts a new worker.
 - At EOF on stdin, the worker exits 0. A malformed request makes it exit 2.