Skip to content

Make the already-updating guard real and keep one pending check per data file - #87

Draft
jwrosewell wants to merge 3 commits into
rcfrom
fix/update-service-guard
Draft

jwrosewell wants to merge 3 commits into
rcfrom
fix/update-service-guard

Conversation

@jwrosewell

@jwrosewell jwrosewell commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Two defects in pipeline/fiftyone_pipeline_engines/src/fiftyone_pipeline_engines/datafile_update_service.py on rc. data-updates.md does not say what happens when an update is asked for while another runs, or how many checks may be pending, so this draft follows the code's own stated intent and the other languages, as below.

  1. The already-updating guard did nothing. update_data_file said "exit if already updating" and then ran if data_file.updating: pass. The flag was only ever set to False, so a manual update and a scheduled one could run at the same time.
  2. Timers stacked. check_next_update made a new threading.Timer and replaced data_file.update_timer without cancelling the one still pending. A manual update made while a timer was pending left two chains of checks, and unregister_data_file cancelled only the newest.

What changed

  • update_data_file sets updating under a lock and returns False when an update is already running for the data file, as Node's updateDataFile does (dataFileUpdateService.js lines 165 to 172). The flag is cleared in a finally, so an update that raises does not leave the data file blocked. The body moved to _update_data_file unchanged.
  • check_next_update cancels the pending check, under the same 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 (DataUpdateService.cs lines 236 to 260 and ClearTimer at 1229 to 1245), and Java cancels the pending check before it schedules the next (DataUpdateServiceDefault.java lines 236 to 246), so this part matches both.
  • The file system watcher already skips a refresh while updating is true. That flag is now set during an update, so a file change during an update is picked up at the next watcher check.

For James to decide

Refusing an update while another runs for the same data file. Python's code on rc says it means to ("exit if already updating", and the file system watcher's check of updating), and Node refuses. Neither .NET (CheckForUpdate, DataUpdateService.cs lines 797 to 920) nor Java (checkForUpdate, DataUpdateServiceDefault.java lines 185 to 250) refuses, and both run the two updates. The specification is silent. Dropping the refusal would keep only the single pending check, and would leave updating set by nothing, so its checks would go too.

Overlap

This draft's base is rc and it depends on no other draft. The data update draft #69, which holds the licence key draft #78, changes update_data_file heavily. git merge-tree of this head with #69 and with #78 is clean. #69's new update_from_memory sets updating without this draft's lock, so whichever of #69 and this draft merges second takes the lock in update_from_memory too.

How it was verified

New tests in pipeline/fiftyone_pipeline_engines/tests/test_datafile_update_guard.py use fake timers and a request that waits, so no thread is left behind.

  • Red, the tests on rc: run 42724595757, 4 of 6 fail (one pending check, no overlap, polling carries on, and unregistering).
  • Green, head d2f6eae6: run 42724742569, 6 passed.
  • Green, head 0270803a, which only names the tests' 10 second wait as WAIT_SECONDS: run 42800175939, 6 passed, the update service tests 4 passed, core 354 passed.
  • Full build of head 0270803aa25ef041341421fd4fc87f7f0028c84b: queued, and this line is updated when it finishes.

The last release into main merged on 2026-11-04.

…ata file

An update returns False while another is running for the same data file, with the
flag set under a lock, and the flag is cleared when an update ends, however it
ends. The pending scheduled check is cancelled before the next is scheduled, so
a manual update no longer leaves two chains of checks.
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.

1 participant