[jwrosewell]

For #70

Problem

registerDataFile schedules the first update check and only then stores the future that schedule returns, so a check with a short delay can start in between. If the check reads the future in that gap, updatedFileAvailable finds none and clears nothing. Registration then stores the future, and the check's own re-registration at line 245 finds it and schedules no next check. A check that finds no update goes the same way through the reschedule at lines 227 to 240. Either way the stored future belongs to a check that has already run, and no further check ever runs.

DataUpdateService_UpdateFromFile_FileUpdated failed this way in the full build of #68 on MacOS_X64_Java_11 (run 42455459082, attempt 1, job 128653447065).

Proof that this order loses the next check

Two new tests in DataUpdateServiceTests force the losing order on every run, with no reliance on timing:

• DataUpdateService_UpdateFromFile_FileUpdated_CheckBeforeFutureStored, for a check that applies an update
• DataUpdateService_UpdateFromFile_FileNotUpdated_CheckBeforeFutureStored, for a check that finds none, which is also the path the retry after a 429 response takes

The mocked schedule starts the check at once and then holds registration until the check has read the future or is blocked on a lock that registration holds (configureScheduleRace and waitForCheck). A data file subclass, RaceDataFile, holds the check after its first read until registration has stored the future, and records each read and write. Every wait is bounded at 10 seconds, so a design that deadlocks fails rather than hangs.

Against the unchanged service on rc, both tests fail for all four parameter sets on Ubuntu and on macOS x64, and the other 92 tests pass (run 42468489378, Ubuntu job, macOS x64 job). The events are the three steps the issue describes, in that order.

Tests run: 100, Failures: 8, Errors: 0, Skipped: 0, Time elapsed: 8.866 s <<< FAILURE! - in fiftyone.pipeline.engines.services.DataUpdateServiceTests
DataUpdateService_UpdateFromFile_FileUpdated_CheckBeforeFutureStored[0: Test with autoUpdateEnabled=true, setEngineNull=false ]  <<< FAILURE!
java.lang.AssertionError: Schedules. Events: [registration scheduled the first check, check read no future, registration stored the first future, check read the first future] expected:<2> but was:<1>
DataUpdateService_UpdateFromFile_FileNotUpdated_CheckBeforeFutureStored[0: Test with autoUpdateEnabled=true, setEngineNull=false ]  <<< FAILURE!
java.lang.AssertionError: Schedules. Events: [registration scheduled the first check, check read no future, registration stored the first future, check read the first future] expected:<2> but was:<1>

The existing tests before the fix

DataUpdateServiceTests as it is on rc, run 50 times on macos-15-intel with three busy loops per core, failed in 2 of the 50 runs (macOS x64 job). Both failures were in DataUpdateService_UpdateFromFile_FileUpdated, with the same TooFewActualInvocations on futureFactory.schedule as the full build of #68.

run 15 FAILED: Tests run: 92, Failures: 1, Errors: 0, Skipped: 4
DataUpdateService_UpdateFromFile_FileUpdated[0: Test with autoUpdateEnabled=true, setEngineNull=false ]  <<< FAILURE!
org.mockito.exceptions.verification.TooFewActualInvocations:
run 38 FAILED: Tests run: 92, Failures: 1, Errors: 0, Skipped: 4
DataUpdateService_UpdateFromFile_FileUpdated[2: Test with autoUpdateEnabled=true, setEngineNull=true ]  <<< FAILURE!
org.mockito.exceptions.verification.TooFewActualInvocations:
FAILED RUNS: 2 of 50

What changed

DataUpdateServiceDefault gains one lock, futureLock, held for every read and write the service makes of a data file's future:

• registerDataFile holds it from the check that there is no future, through scheduling, to storing the future
• updatedFileAvailable holds it to cancel and clear the future, and releases it before refreshing the engine
• the reschedule after a check that applies no update, at lines 237 to 249, holds it to cancel and replace the future
• unregisterDataFile holds it to cancel and clear the future

A check that starts at once now waits at its first read of the future until registration has stored it. Nothing else changes. The same futures are scheduled with the same delays and cancelled in the same places, and the events fire in the same order.

The lock is the service's own rather than the data file's update lock, because the file system watcher holds the update lock in dataFileUpdated for the whole engine refresh, and for its loop that waits for the new file to open, which has no limit. Taking the update lock for the future would make unregistering, and so closing an engine, wait for a refresh.

Deadlocks checked

• futureLock is always the innermost lock. Only getFuture, setFuture, cancel, futureFactory.schedule, getInterval and debug logging run while it is held, and none of them takes configLock or an update lock or calls back into the service.
• The watcher path takes the data file's update lock and then futureLock, through updatedFileAvailable. unregisterDataFile takes configLock and then futureLock. registerDataFile releases futureLock before it takes configLock. No path takes any two of these locks in the opposite order.
• checkForUpdate releases futureLock before it calls registerDataFile, the engine's refreshData or the update complete callbacks.
• Update on startup runs checkForUpdate synchronously from registerDataFile while holding no lock, and the registerDataFile call that the check ends with takes futureLock for itself.
• cancel(true) on the check's own future interrupts the check's thread without waiting, as it did before.
• With FutureFactoryDefault the scheduled checks and the watcher's poll share one thread, so they never contend for the lock.
• A custom FutureFactory whose schedule waited for the scheduled task to finish would now deadlock, because that task needs futureLock. FutureFactoryDefault returns at once, and it is the only implementation in this repository.

I read every exit path of the four changed functions. Each lock is a synchronized block, so it is released on every return and exception. A RejectedExecutionException from schedule after the factory is closed is still caught and logged in registerDataFile, and still propagates from the reschedule, as before.

After the fix

All of these are in run 42468849600, at ce1a533. The head commit, 9424d53, changes only the wording of three new comments.

DataUpdateServiceTests 50 times on macos-15-intel with three busy loops per core (job).

run 1 passed: Tests run: 100, Failures: 0, Errors: 0, Skipped: 4, Time elapsed: 9.55 s - in fiftyone.pipeline.engines.services.DataUpdateServiceTests
run 2 passed: Tests run: 100, Failures: 0, Errors: 0, Skipped: 4, Time elapsed: 8.002 s - in fiftyone.pipeline.engines.services.DataUpdateServiceTests
...
FAILED RUNS: 0 of 50

The whole pipeline.engines module and the modules that use the data update service, built with the native libraries and tested with the Enterprise data files on Java 11 (Ubuntu, Windows, macOS x64). Each cell is tests run, failed, skipped.

Module	Ubuntu	Windows	macOS x64
pipeline.engines	137, 0, 0	137, 0, 3	137, 0, 4
pipeline.engines.fiftyone	73, 0, 1	73, 0, 1	73, 0, 1
device-detection.hash.engine.on-premise	33, 0, 3	33, 0, 3	33, 0, 3
device-detection	38, 0, 0	38, 0, 0	38, 0, 0
ip-intelligence.engine.on-premise	22, 0, 2	22, 0, 2	22, 0, 2
ip-intelligence	15, 0, 5	15, 0, 5	15, 0, 5

DataUpdateServiceTests ran 100 tests with no failures on each of the three.

Full pull request build

common-ci's nightly-pull-request.yml, run as a dry run on this pull request at 9424d53, passed in all 15 configurations from ci/options.json, being Ubuntu, Ubuntu ARM, Windows, macOS and macOS x64 on Java 8 to 21 (run 42473320389). DataUpdateServiceTests ran 100 tests with no failures in each, for example on MacOS_X64_Java_11 (job).

Tests run: 100, Failures: 0, Errors: 0, Skipped: 4, Time elapsed: 6.189 s - in fiftyone.pipeline.engines.services.DataUpdateServiceTests

What remains

• The production delays are minutes. The first check waits the polling interval, 30 minutes plus up to 10 minutes at random by default, a retry after a failure waits the same, and a check after an update waits until the data file's next update time. Reading the startup and retry paths, a check could only start before its future was stored with a polling interval of 0 and no randomisation, or with a next update time a few milliseconds away.
• AspectEngineDataFileDefault.close() still cancels the future directly, without the lock, after unregisterDataFile. A check that is already running when its data file is closed can still schedule another check afterwards, as before. Neither is changed here.
• Separately, when the next update time is in the future, registerDataFile passes milliseconds to applyIntervalRandomisation, which adds a random number of seconds as if they were milliseconds. The random spread there is at most 600 milliseconds by default rather than 10 minutes. Not changed here.
• The .NET data update service has the same order. RegisterDataFile creates the timer, which starts counting at once (TimerFactory), and only then stores it in dataFile.Timer. The callback clears the timer through ClearTimer and re-registers at lines 859 to 866, or reschedules through dataFile.Timer.Change at lines 893 to 900, with no lock around any of it. That has not been reproduced in .NET and is not changed here.