[jwrosewell]

Problem

Two defects in pipeline/fiftyone_pipeline_engines/src/fiftyone_pipeline_engines/datafile_update_service.py on rc (df0949d7).

1. Two updates of one data file could run at the same time. update_data_file said "exit if already updating" and then ran if data_file.updating: pass (lines 122 to 125). The flag was only ever set to False there, so a manual update, a scheduled one, an update_from_memory call and a file system watcher refresh could all write and refresh the same data file at once, and the watcher, which leaves a changed file alone while updating is true (line 603), never saw an update running.
2. Timers stacked. check_next_update made a new threading.Timer and replaced data_file.update_timer without cancelling the one still pending (lines 524 to 528). A manual update made while a timer was pending left two chains of checks, and unregister_data_file cancelled only the newest.

What the specification says

data-updates.md, "Reporting each check", lines 345 to 348, "A check for an update MUST be reported to onUpdateCallbacks once it ends, including the checks made by update polling, by the file watcher, by the update on start-up and by a programmatic call." Lines 356 to 359 give the In progress status "for a file watcher event that found nothing left to do, being one that waited while another event for the same file was applied". The steps for a new file, lines 316 and 317, are "pause file watcher, if any" and "pause update polling, if any" before the file is loaded, and line 38 says the Engine's refresh "MUST be thread safe".

The earlier version of this draft refused a second update while one ran and asked James whether to keep that. The specification settles it. A refused programmatic call ends without a report, which the first sentence above does not allow, and the specification's own picture of two events for one file is that the second waits for the first. Neither .NET's CheckForUpdate (DataUpdateService.cs line 798 at 8b3220b3) nor Java's checkForUpdate (DataUpdateServiceDefault.java line 219 at f212dfc9) looks for an update already running. 51degrees-node reports a second check as In progress and makes no request (dataFileUpdateService.js lines 367 to 371 at 4d5832f0). So here a second update waits for the first to finish and then makes its own check, which is reported like any other.

What changed

• DataFileUpdateService keeps a re-entrant lock for each data file (_update_lock). update_data_file holds it for the whole update, sets updating while the update runs and puts the earlier value back at the end, also when the update raises. The body moved to _update_data_file unchanged, apart from the two places that cleared updating before the update complete callbacks, which the wrapper now does. The lock is re-entrant so that an update complete callback that starts an update of the same data file does not wait for itself.
• update_from_memory holds the same lock, so it waits for an update of the same data file that is running, and an update waits for it. Its body moved to _update_from_memory, which puts back the earlier updating value where it cleared it.
• The file system watcher takes the same lock for its refresh, without waiting, and sets updating while it refreshes (track_local_data_file_changes lines 635 to 654). An update asked for during a watcher refresh waits for it, as the steps above pause polling while a file is loaded. A changed file the watcher finds while an update holds the lock is reported as In progress and looked at again at the next check, as before.
• check_next_update cancels the pending check under the service's lock before it schedules the next one, so a data file has one pending check and polling carries on after a manual update. Unregistering then cancels every check. .NET keeps one timer per data file, creating one only when the data file has none (DataUpdateService.cs lines 243 to 265 at 8b3220b3) and clearing it with ClearTimer (lines 1232 to 1245) before it registers again after an update (line 871). Java cancels the pending check before it schedules the next (DataUpdateServiceDefault.java lines 286 to 291 at f212dfc9). So this part matches both.

How it was verified

The tests in pipeline/fiftyone_pipeline_engines/tests/test_datafile_update_guard.py use fake timers and a request or refresh that waits until the test lets it go, so no thread is left behind. Four are new for the waiting, being test_a_second_update_waits_for_the_first_then_makes_its_own_check (the second makes no request while the first runs, then makes its own, and both are reported), test_updating_is_set_while_an_update_runs, test_an_update_from_memory_waits_for_a_running_update and test_an_update_waits_for_a_file_system_watcher_refresh. A test gives an update that should be waiting half a second to show that it is (STILL_WAITING_SECONDS), which cannot fail a correct implementation, because that update cannot end before the test lets the first one go.

Every run below is a temporary branch holding a workflow that installs the pipeline packages from the checkout into a virtual environment of the runner's own Python (3.12.3 on Ubuntu, 3.12.10 on Windows) and runs the listed tests with pytest, on ubuntu-latest and windows-latest.

• Red for the waiting and the timers, rc at df0949d with the earlier head's test file, run 43477704600, failed on both with 6 failed, 2 passed. The six are the first three tests above and test_a_manual_update_leaves_one_pending_check, test_polling_carries_on_after_a_manual_update and test_unregistering_cancels_every_pending_check.
• Red for the watcher, the earlier head 7709feb with this head's test file, run 43520180554, failed on both with 1 failed, 8 passed, the failure being test_an_update_waits_for_a_file_system_watcher_refresh.
• Green, this head, run 43520205364, passed on both, 9 passed, then the whole fiftyone_pipeline_engines suite (74 passed) and the whole fiftyone_pipeline_core suite (481 passed).
• Full build, run 43520230500, started by tools/pr-matrix.py on tmp/pr-matrix-87w, finished with success. Configure printed "Left out 5 macOS entries", and the 11 Setting 'PullRequestSha' lines (Configure and the ten Build and Test jobs, Python 3.10 to 3.14 on Ubuntu and Windows) all give eb9200a. macOS was left out of this draft's full build under the 9 November decision. The rc head is built with macOS before it merges into main.
• Quick checks passed on this head, being run 43520159309 (environment variable names), run 43520159966 (only rc into main) and run 43520159432 (UTM link lint).

Head eb9200a8 is the earlier head 7709feb, then the watcher change and its test (2687dcb), then rc at 32ffdcf1 merged in, which merged cleanly. #69, which added update_from_memory, merged into rc on 12 November, which is why this draft takes the lock there too.

What remains

• update_from_memory reports its result to the update complete callbacks, and the same section of the specification, lines 363 to 366, says an update from memory reports nothing and gives its result to the caller. That is unchanged here, and #149 tracks it.

The last release into main was #114, merged at 06:22 UTC on 13 November.