Skip to content

[FIX]: Guard against empty output file path in ServerComparer#14

Open
x15sr71 wants to merge 1 commit into
CCExtractor:masterfrom
x15sr71:fix/server-comparer-empty-path-guard
Open

[FIX]: Guard against empty output file path in ServerComparer#14
x15sr71 wants to merge 1 commit into
CCExtractor:masterfrom
x15sr71:fix/server-comparer-empty-path-guard

Conversation

@x15sr71

@x15sr71 x15sr71 commented Jun 7, 2026

Copy link
Copy Markdown

Description

When CCExtractor produces no output file, ServerComparer.CompareAndAddToResult() calls File.Open("", FileMode.Open), which throws ArgumentException. The caller in Tester.cs swallows it and unconditionally calls
SendExitCodeAndRuntime() — so the test is marked finished with no TestResultFile row written and no comparison data sent to the platform.

Execution chain

Processor.cs      → rd.ResultFiles.Add(key, "")          // file missing
ServerComparer.cs → File.Open("", FileMode.Open)          // throws ArgumentException
Tester.cs         → catch (Exception e) { Logger.Error } // swallowed
Tester.cs         → SendExitCodeAndRuntime(...)           // runs unconditionally
Platform          → receives finish, no comparison row    // status sees zero failures

Findings

Command run on the sample-platform server:

grep -n "Path is empty" /repository/LogFiles/9298.txt

Logs[ERROR] Path is empty immediately followed by [INFO] Finished entry X
in 10 production log files (4778, 4782, 4790, 4792, 4794, 4798, 4804, 4828, 5124, 9298). Example from log 9298 (25 occurrences, wc -l verified):

68:[ERROR] Path is empty
69:[INFO] Finished entry 4 with exit code: 0
95:[ERROR] Path is empty
96:[INFO] Finished entry 17 with exit code: 10

Log 9299 (same test, Windows): 0 occurrences — Windows missing rows have a separate unrelated cause not addressed here.

DBtest_result row exists (finish received) but no test_result_file row (comparison never ran), exit_code=0, expected_rc=0, excluding ignored/stdout tests (139, 238, 239):

SELECT t.test_type, COUNT(*) AS confirmed_silent_failures
FROM test_result tr
JOIN test t ON t.id = tr.test_id
LEFT JOIN test_result_file trf
    ON trf.test_id = tr.test_id AND trf.regression_test_id = tr.regression_test_id
WHERE trf.test_id IS NULL AND tr.exit_code = 0 AND tr.expected_rc = 0
  AND tr.regression_test_id NOT IN (139, 238, 239)
GROUP BY t.test_type;

Counts tests where finish was received but no comparison row was written — includes but is not limited to instances caused by this bug.

commit 11,931
pr 37,155
Total 49,086

11 affected regression test IDs (7, 16, 138, 140, 143, 144, 145, 148, 150, 154, 155) all have thousands of historical TestResultFile rows, confirming this is a runtime failure not a data model gap.

Fix

Add a guard inside the !Hasher.filesAreEqual branch that checks string.IsNullOrEmpty(data.ProducedFile) || !File.Exists(data.ProducedFile) and returns early, preventing File.Open("") from being called.

Impact

Scenario Before After
Output file exists Normal No change
Output file missing ArgumentException swallowed Early return
TestResultFile row Not created Not created
[ERROR] in log Yes (Tester.cs catch) No (exception not thrown)

Risks

  • No behavior change when output files are produced correctly.
  • Does not create a failure-marker TestResultFile row — that requires
    server-side changes and is out of scope.
  • This alone does not fix false-positive status reporting — a separate PR on
    sample-platform addresses the counting logic.

Deployment

Independent of sample-platform. Can deploy in parallel with #1118.

// caller's catch block.
if (string.IsNullOrEmpty(data.ProducedFile) || !File.Exists(data.ProducedFile))
{
return;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you return without posting a status, how is the platform supposed to know something went wrong?

@x15sr71 x15sr71 Jun 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The finish POST (exit code + runtime) goes through the same as before this PR, SendExitCodeAndRuntime runs after the comparison loop, outside the try/catch, so neither the old swallowed exception nor the new early return affects it. The TestResult row is created either way; only the equality/upload POST is skipped, so no TestResultFile row is written for that output.

The platform reads that absence as a failure. In get_test_results() (mod_test/controllers.py), when a regression test has no TestResultFile rows it checks RegressionTestOutput directly and sets test_error = True if a non-ignored output was expected (the else branch). That runs on every status surface that calls it, PR status (via comment_pr), PR comment, badge, detail page, except commit-test status, which still uses the old count-based query in progress_type_request where zero rows reads as zero failures → SUCCESS. That's the actual bug; #1119 fixes it by moving that path onto get_test_results().

So this PR is the narrower part: it stops the silent ArgumentException and makes the missing-output path explicit, it doesn't change what's sent to the platform, since finish already fired and the comparison row was already absent before this change. One flag, the guard returns silently, so unlike the old swallowed exception (which hit processor.Logger.Error) there's no tester-side trace now. ServerComparer has no logger field; happy to add a Console.Error.WriteLine in the guard, or wire an ILogger into ServerComparer like Tester and Processor already use - whichever you prefer.

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.

2 participants