fix: catch OSError when exiftool binary is missing (#1960)

[doitgo]

Fixes #1960
When exiftool_path points to a binary that does not exist, the subprocess.run calls raised FileNotFoundError, which propagated unhandled through exiftool_metadata() and crashed the whole conversion.

Add OSError to the except clauses at both subprocess.run sites and wrap the error in a RuntimeError that includes the path, giving users a clear, actionable error message.

Add a regression test for the missing-binary case and a sanity check that the exiftool_path=None early return still works.

[noezhiya-dot]

Clean fix for #1960. Adding OSError to the except clauses is the right call — FileNotFoundError and PermissionError are both subclasses of OSError, so this covers all the cases where the exiftool binary can't be found or executed. The error message now includes the actual path, which is much more actionable for users debugging their setup.

The regression tests are well-structured: one for the nonexistent binary case and one sanity check for the None early return. Good coverage.

One minor note: the second subprocess.run site (the actual exiftool invocation around line 48) already had a broad except, so adding OSError there is defensive but reasonable. The test only covers the version-check path — consider adding a test for the run path too, but not blocking.

LGTM.

[noezhiya-dot]

Clean fix. Adding OSError to the exception tuple is the right approach — FileNotFoundError is a subclass of OSError, so this catches the case where the exiftool binary doesn't exist.

The error message improvement is helpful too; including the path makes debugging much easier.

Tests are thorough:

• Verifies RuntimeError is raised (not raw FileNotFoundError) with a matching message
• The sanity check for exiftool_path=None returning {} is a nice touch

One minor thought: the function signature says exiftool_path: str | None, but the OSError path would only fire when a non-None path is given. That's fine — just noting the early return for None is already handled above. Good to merge.

[noezhiya-dot]

Nice cleanup. Looks good.

[noezhiya-dot]

LGTM — Clean, minimal fix.

The change correctly catches OSError (parent of FileNotFoundError) in both the version-check and metadata-extraction call sites. The improved error message that includes the path is a nice touch for debugging.

The regression tests are well-structured:

• test_exiftool_metadata_with_nonexistent_binary verifies the wrapping behavior
• test_exiftool_metadata_with_no_path ensures the None early-return path is unaffected

One minor note: the error message format changed from "Failed to verify ExifTool version." to "Failed to invoke exiftool at {path}: {e}". This is strictly better, but if anyone was matching on the old message string, they'd need to update. Not a concern for this repo though.

Approve.

[noezhiya-dot]

Code Review: fix: catch OSError when exiftool binary is missing (#1960)

Clean, focused bug fix.

Strengths

• Correctly catches OSError (parent of FileNotFoundError) in both the version check and the metadata extraction paths.
• Error message now includes the exiftool path, which is much more debuggable than a raw FileNotFoundError.
• Includes regression tests for both the nonexistent binary case and the None path sanity check.
• Follows the existing error wrapping pattern (RuntimeError with from e).

Notes

• The fix is minimal and exactly targets the reported issue. No concerns.

Approved.

[doitgo]

Hi @afourney,

I'd like to draw your attention to PR #2082, which fixes #1960.

Issue: when the configured exiftool path points to a binary that doesn't exist, the current code raises an unhandled FileNotFoundError that crashes the entire conversion.

Fix (+27 / -2 across 2 files):

• src/markitdown/converters/_exiftool.py: catch OSError (parent of FileNotFoundError / PermissionError) at both subprocess.run sites and re-raise as RuntimeError with the offending path included for easier debugging.
• tests/test_exiftool_converter.py: add a regression test for the missing-binary case and a sanity check that the exiftool_path=None early-return path still returns {}.

Local tests pass. Could you or another maintainer take a look when you have a moment? Happy to adjust the implementation, the error message wording, or the test coverage based on your feedback.

Thanks!