[jwrosewell]

Part of 51degrees-cxx#61, which covers the same fault in other languages as well. This pull request does not close that issue.

Merge condition. Do not merge this before the native code the Python engines are built from is 51degrees-cxx 4.5.100 or later. pipeline-python has no native code of its own. The Device Detection engine in device-detection-python builds today from device-detection-cxx at 0a2f0f80, with common-cxx at 61b8da9a, which is older than 4.5.100. On that native code a reload that fails has already put a half built data set in place before refreshData raises, so the next lookup crashes the process whatever this package does, as the runs below show. In 51degrees-cxx 4.5.100 a failed reload keeps the data set in use (51degrees-cxx#60), and that is what makes catching the failure and carrying on the right response. This stays a draft until then, as the nightly pull request workflow in this repository merges a pull request once it is marked ready and has no review requested.

Problem

track_local_data_file_changes is the file system watcher. Every 10 seconds it compares the data file's modified time with the last one it saw, refreshes the engine when the time has moved on, and only then starts the timer for its next check. Nothing catches a refresh that raises, and a corrupt or truncated data file makes refreshData raise. The exception ends the timer thread before the next timer starts, so the watcher stops for good and a later good file is never seen. The service keeps running on whatever data it had. The calls to os.stat at lines 274 and 277 end a check in the same way when the file is missing at that moment, for example part way through being replaced.

The update route catches the same failure (lines 198 to 206) and reports AUTO_UPDATE_REFRESH_FAILED to the on_complete methods.

The watcher is on by default in DataFile and off by default in the Device Detection engine (file_system_watcher=False), so the fault reaches an application that turns the watcher on. The engine's refresh calls refreshData and lets its RuntimeError through.

What was checked and found

A corrupt data file went through the real Device Detection engine from device-detection-python at d14d0d94, on Ubuntu and Windows with Python 3.12. The engine was built twice, once from device-detection-cxx 0a2f0f80 as it is today, and once from 51degrees-cxx 4.5.100 with only the source paths in setup.py and the one include in hash_python_wrap.cxx changed for the new layout. The corrupt file is the Lite data file with one more component in its header than the file holds, which is the broken data the 51degrees-cxx reload tests use. It passes the checks on the version and the collections and fails while the components are read. The engine kept a temporary copy of the data file, so writing over the original does not touch the data in use, and the name of the temporary copy shows which data set is in use. Each case ran in its own process, so that a crash ends only that case.

Native code	How the reload ran	After the corrupt file	Next lookup	Later good file
device-detection-cxx 0a2f0f80	refreshData called directly	Raises	The process ends, with SIGSEGV on Linux and an access violation (0xC0000005) on Windows	Not reached
device-detection-cxx 0a2f0f80	Watcher, main	The refresh raises and the watcher's timer is gone	The process ends in the same way	Not reached
device-detection-cxx 0a2f0f80	Watcher, this change	Logged at error level, AUTO_UPDATE_REFRESH_FAILED reported, the watcher's timer still running	The process ends in the same way	Not reached
51degrees-cxx 4.5.100	refreshData called directly	Raises	Same answers, same temporary copy	Loaded into a new temporary copy
51degrees-cxx 4.5.100	Watcher, main	The refresh raises and the watcher's timer is gone	Same answers, same temporary copy	Never refreshed, so the old temporary copy stays in use
51degrees-cxx 4.5.100	Watcher, this change	Logged at error level, AUTO_UPDATE_REFRESH_FAILED reported, the watcher's timer still running	Same answers, same temporary copy	Picked up and loaded into a new temporary copy

Every row is the same on Linux and Windows. The two rows that call refreshData directly, with no watcher, show that the crash comes from the native code and not from the watcher. This is the output for the last row on Linux.

watcher refresh raised Offset used to retrieve an item from a collection was out of range.
pipeline error log: Refreshing from the changed data file '/tmp/corrupt-dd-wuwq0pfq/master.hash' failed. Offset used to retrieve an item from a collection was out of range.
on_complete: AUTO_UPDATE_REFRESH_FAILED
watcher timers alive: AutoUpdate - FileWatcher
lookup after the corrupt file: ismobile=True platformname='iOS' browsername='Mobile Safari' deviceid='12280-24384-24305-18092' (same answers)
temp copy after the corrupt file: master-3989-AGHIXJILHIIMVNKONJSA
writing the good file over master.hash
watcher refresh returned
temp copy after the good file: master-3989-QFIQKDUZTNPSJBJTVCRT

The rows for main ran before the change was written (Linux, Windows). The rows for this change ran with the change (Linux, Windows). Both came from temporary workflows on branches that were deleted afterwards, and the scripts are in the commits those runs name.

Change

1. track_local_data_file_changes catches a refresh that raises. It logs the failure through the pipeline at error level, as the update route logs its other failures, and reports AUTO_UPDATE_REFRESH_FAILED to the on_complete methods, as the update route does for a failed refresh.
2. The next check is scheduled in a finally block, so it is scheduled whatever the check does.
3. A file that is missing at a check is looked for again at the next check, rather than os.stat ending the watcher. Nothing is logged or reported for it, as no refresh was tried.
4. os.stat is called once for each check rather than twice.
5. The 10 second interval is the class constant FILE_SYSTEM_WATCHER_INTERVAL_SECONDS.

The time of a file that failed is kept as the last time seen, so a broken file is not tried again every 10 seconds. The next refresh waits for the file to change again.

Exit paths of track_local_data_file_changes

1. The file is missing or cannot be read at os.stat. No refresh is tried and the next check is scheduled.
2. The file has not changed, or updating is set. No refresh is tried and the next check is scheduled.
3. The file has changed and the refresh works. The next check is scheduled.
4. The file has changed and the refresh raises. The failure is logged and reported, and the next check is scheduled.
5. Inside 4, the pipeline's log or an on_complete method raises. The next check is still scheduled, because it is in finally, and the exception ends only that timer thread, which Python reports through threading.excepthook.

Tests

fiftyone_pipeline_engines/tests/test_datafile_update_service.py is new, in the same unittest style as test_engines.py.

• testFileSystemWatcherKeepsWatchingAfterFailedRefresh drops an unusable file while the watcher runs. It checks that the failure is logged at error level and reported as AUTO_UPDATE_REFRESH_FAILED, that the data in use is kept and that a next check is scheduled, and then that a later good file is picked up.
• testFileSystemWatcherKeepsWatchingWhileFileIsMissing removes the file and checks that a next check is scheduled, and then that a later good file is picked up with no failure reported.

The engines live in other repositories, so these tests use a refresh made to fail rather than a real engine. The data file in the tests keeps its data when a refresh fails, as an engine on 51degrees-cxx 4.5.100 does, and the real engine is covered by the runs above. The tests take the place of threading.Timer, so that each scheduled check runs in turn with no waiting.

Check	Before the change, at 0ec0f72	With the change, at 44111b6
pylint with the package's .pylintrc, as CI runs it	10.00/10	10.00/10
pytest fiftyone_pipeline_engines/tests	2 failed, 5 passed	7 passed
tox -e py in fiftyone_pipeline_engines, as CI runs it	Not run	7 passed

These are the same on Ubuntu and Windows (Linux, Windows). Before the change the two new tests fail with the exception raised out of the check.

FAILED fiftyone_pipeline_engines/tests/test_datafile_update_service.py::FileSystemWatcherTests::testFileSystemWatcherKeepsWatchingAfterFailedRefresh - Exception: The data file cannot be used.
FAILED fiftyone_pipeline_engines/tests/test_datafile_update_service.py::FileSystemWatcherTests::testFileSystemWatcherKeepsWatchingWhileFileIsMissing - FileNotFoundError: [Errno 2] No such file or directory: '/tmp/tmp8y9lle5o/data.dat'

What remains

1. The merge condition above. The Python engines have to be built from 51degrees-cxx 4.5.100 or later before this merges.
2. The watcher still reports nothing when a refresh works, where the Node watcher reports AUTO_UPDATE_SUCCESS. That is unchanged here.
3. update_data_file never sets data_file.updating to True, and its if data_file.updating: pass at lines 93 and 94 does nothing, so the watcher's check of updating never skips a refresh. Found while reading and not changed here.
4. The full pull request build, macOS, Python 3.13 and 3.14, and the IP Intelligence engine were not run here.

[oleksandrlazarenko-pi]

Closing: this repository has moved into 51degrees-python, and this change is already on its main as dce0fa7, with your authorship kept. It was released in fiftyone_pipeline_* 4.5.41.