fix: Stop deleting estimation files on transient errors

[thomhurst]

Summary

Fixes #1543

• Remove destructive File.Delete call that was triggered by any exception
• Validate file name format before parsing (check for -Sub- presence)
• Only catch IOException for transient errors (file locked, etc.)
• Skip gracefully on any error instead of deleting potentially valid files

Problem

The previous implementation would:

1. Delete files for ANY exception (including transient ones like file locks)
2. Silently destroy timing data without logging
3. Use Split()[1] which throws if pattern not found

Solution

• Check for -Sub- pattern before parsing using IndexOf
• Only catch IOException for transient I/O errors
• Return null and skip instead of deleting
• Let GetEstimatedTimeAsync handle format errors (it already returns a default value)

Files will only be updated by SaveSubModuleTimeAsync with correct data, never deleted on read errors.

Test plan

• Build succeeds
• Verify timing estimation still works correctly
• Verify files are preserved on transient errors

🤖 Generated with Claude Code

[thomhurst]

Summary

Improves error handling in FileSystemModuleEstimatedTimeProvider by replacing Split with IndexOf, adding explicit pattern validation, and removing automatic file deletion on errors.

Critical Issues

⚠️ Incomplete exception handling (src/ModularPipelines/Engine/FileSystemModuleEstimatedTimeProvider.cs:47-50)

The catch block changed from catch-all to catching only IOException, but other exceptions can still be thrown:

• UnauthorizedAccessException - when lacking permissions to read the file
• SecurityException - when lacking required permissions
• PathTooLongException - rare but possible
• Any exceptions from GetEstimatedTimeAsync() beyond IOException (though FormatException is already handled internally)

Recommendation: Either revert to catch (Exception) or expand to catch the specific exceptions that matter:

catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or SecurityException)
{
    // Transient or permission error - skip gracefully without deleting
    return null;
}

The pattern validation improvement (lines 43-50) is excellent and prevents the original IndexOutOfRangeException.

Suggestions

💡 File cleanup behavior changed: The old code deleted files on any error, the new code doesn't delete them at all. This means corrupted/malformed files will persist and be processed every time. This might be intentional (safer), but worth confirming this is the desired behavior. If a file truly cannot be parsed, it will be retried indefinitely.

Verdict

⚠️ REQUEST CHANGES - The narrowed exception handling could allow unhandled exceptions to propagate where they previously didn't, potentially breaking the parallel processing pipeline.

[thomhurst]

Summary

This PR fixes a critical bug where timing estimation files were being deleted on any exception, including transient errors like file locks.

Critical Issues

None found ✅

The fix is well-reasoned:

• Replaces unsafe Split()[1] with safe IndexOf + range slicing
• Removes destructive File.Delete call that could destroy valid data
• Narrows exception handling to only I/O-related errors (IOException, UnauthorizedAccessException, SecurityException)
• Files with invalid naming patterns are now skipped gracefully instead of triggering exceptions

The exception handling is appropriate because:

• GetEstimatedTimeAsync already handles FormatException internally (lines 76-80), so it won't propagate
• I/O exceptions are transient and shouldn't result in file deletion
• Invalid file name patterns return null before any I/O occurs

Suggestions

None - the implementation is solid and addresses the issue described in #1543.

Verdict

✅ APPROVE - No critical issues. This is a good bug fix that prevents data loss.

[Copilot]

Pull request overview

This PR fixes a critical issue where timing estimation files were being deleted on any exception, including transient errors. The fix prevents data loss by validating file name format before parsing and removing the destructive File.Delete call.

Key Changes:

• Replace Split()[1] with safe IndexOf pattern matching to avoid exceptions on invalid file names
• Remove destructive File.Delete call that destroyed timing data
• Catch only IOException instead of all exceptions to handle transient I/O errors

[Copilot]

Catching only IOException is too narrow and may cause unhandled exceptions. The GetEstimatedTimeAsync method on line 53 can throw FormatException (from TimeSpan.Parse on line 83), and File.ReadAllTextAsync can throw other exceptions beyond IOException such as UnauthorizedAccessException, NotSupportedException, ArgumentException, etc. Since the SafeModuleEstimatedTimeProvider wrapper already catches all exceptions and logs them, it's safer to catch Exception here to prevent any parsing or I/O error from propagating.

[Copilot]

The new error handling behavior lacks test coverage. Consider adding unit tests to verify that files are preserved when IOException occurs, when the file name format is invalid (no "-Sub-" pattern), and when GetEstimatedTimeAsync throws FormatException. This ensures the fix works as intended and prevents regression.