From 083d3b6ebb338d30f597201bbe909ede70a99cf8 Mon Sep 17 00:00:00 2001 From: Ellian Carlos Date: Tue, 6 Oct 2026 00:41:25 +0000 Subject: [PATCH 1/2] fix(blobExec): exclude a blob on a tokenizer error Before this change, a tokenizer that exited non-zero with a status other than 33 or a signal status kept the original blob. Thus the raw source went into the token tree and the blob map, and no EXCLUDED line named the blob. Examples: the Rust tokenizer exits 1 on a file that is not UTF-8, and tokenBySha.pl exits non-zero when it dies. Now, without --abort-on-error, a non-zero exit excludes the blob. The EXCLUDED failed blob line gives reason=tokenizer-error, and the done line counts the blob in blobsTokenizerFailed. With --abort-on-error, the run stops with exit 2, as before. The new reason keeps the parser-crash count for srcML crashes only. No script reads the reason values or the parser-crash count, so no consumer changes. --- README.md | 6 +-- blobExec/README.org | 7 ++-- .../main/scala/cregit/blobexec/BlobExec.scala | 15 +++++++- .../src/main/scala/cregit/blobexec/Main.scala | 24 +++++++----- .../main/scala/cregit/blobexec/Walker.scala | 11 +++++- .../scala/cregit/blobexec/BlobExecSpec.scala | 14 +++++++ .../cregit/blobexec/FailedBlobSpec.scala | 11 +++++- .../blobexec/TokenizerWorkerPoolSpec.scala | 38 ++++++++++++------- 8 files changed, 90 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 22fbbc4f..ea89528a 100644 --- a/README.md +++ b/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): diff --git a/blobExec/README.org b/blobExec/README.org index b8c4aa50..0df77604 100644 --- a/blobExec/README.org +++ b/blobExec/README.org @@ -38,7 +38,7 @@ java -jar blobExec-0.1.0-assembly.jar [--abort-on-error] \ | == | SQLite mapping file. Created with schema if absent | | == | Absolute path to the per-blob script | | == | Regex matched against each blob's filename, e.g. ='\.[ch]$'= | -| =--abort-on-error= | Exit 2 on the first non-zero == exit (default: skip the blob) | +| =--abort-on-error= | Exit 2 on the first non-zero == exit (default: exclude the blob) | For each blob whose filename matches ==, == 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: = appended to its message (byte-for-byte compatible with BFG's =FormerCommitFooter=, which the cregit =remapCommits= step parses from each commit's last line). diff --git a/blobExec/src/main/scala/cregit/blobexec/BlobExec.scala b/blobExec/src/main/scala/cregit/blobexec/BlobExec.scala index dde25027..0864b918 100644 --- a/blobExec/src/main/scala/cregit/blobexec/BlobExec.scala +++ b/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, diff --git a/blobExec/src/main/scala/cregit/blobexec/Main.scala b/blobExec/src/main/scala/cregit/blobexec/Main.scala index 96fe4440..8ca84b7e 100644 --- a/blobExec/src/main/scala/cregit/blobexec/Main.scala +++ b/blobExec/src/main/scala/cregit/blobexec/Main.scala @@ -58,7 +58,7 @@ object Main { private[blobexec] val Usage = raw"""Usage: blobExec [options] | - | --abort-on-error exit 2 on the first error from instead of skipping the blob + | --abort-on-error exit 2 on the first error from 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 { | 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=` as a pattern: `case Flag(value) =>`. */ @@ -460,6 +461,7 @@ 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 = { @@ -467,8 +469,10 @@ object Main { 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 = { diff --git a/blobExec/src/main/scala/cregit/blobexec/Walker.scala b/blobExec/src/main/scala/cregit/blobexec/Walker.scala index 343abd82..5c1ece55 100644 --- a/blobExec/src/main/scala/cregit/blobexec/Walker.scala +++ b/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) diff --git a/blobExec/src/test/scala/cregit/blobexec/BlobExecSpec.scala b/blobExec/src/test/scala/cregit/blobexec/BlobExecSpec.scala index d973c613..80f8392a 100644 --- a/blobExec/src/test/scala/cregit/blobexec/BlobExecSpec.scala +++ b/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, diff --git a/blobExec/src/test/scala/cregit/blobexec/FailedBlobSpec.scala b/blobExec/src/test/scala/cregit/blobexec/FailedBlobSpec.scala index 8674eab5..32083434 100644 --- a/blobExec/src/test/scala/cregit/blobexec/FailedBlobSpec.scala +++ b/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 { diff --git a/blobExec/src/test/scala/cregit/blobexec/TokenizerWorkerPoolSpec.scala b/blobExec/src/test/scala/cregit/blobexec/TokenizerWorkerPoolSpec.scala index e9cfd78b..b92205ac 100644 --- a/blobExec/src/test/scala/cregit/blobexec/TokenizerWorkerPoolSpec.scala +++ b/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] } } From c5c7c8f0782c8b4b8170b0d289a533327e81d2a7 Mon Sep 17 00:00:00 2001 From: Ellian Carlos Date: Tue, 6 Oct 2026 00:43:13 +0000 Subject: [PATCH 2/2] docs: name the tokenizer-error exclusion DATASET.md lists reason=tokenizer-error and blobsTokenizerFailed with the other failed-blob exclusions. WORKER_PROTOCOL.md said that a worker that stops in a request gives a tokenizer error. The code reports a timeout, so the text now says that. --- generate_dataset/DATASET.md | 2 +- tokenizeByBlobId/WORKER_PROTOCOL.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/generate_dataset/DATASET.md b/generate_dataset/DATASET.md index bded0b8f..ca045e7e 100644 --- a/generate_dataset/DATASET.md +++ b/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 diff --git a/tokenizeByBlobId/WORKER_PROTOCOL.md b/tokenizeByBlobId/WORKER_PROTOCOL.md index a427c122..6b92e197 100644 --- a/tokenizeByBlobId/WORKER_PROTOCOL.md +++ b/tokenizeByBlobId/WORKER_PROTOCOL.md @@ -36,6 +36,6 @@ worker -> JVM: RES \n 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.