DiamondLightSource / DiamondLightSource/smartem-devtools

EPU Parser: potential race condition

Aperta
#89 0 commenti 0 reazioni 1 assegnatario Rivendicata da @vredchenko Vedi su GitHub
Lingua principale
TypeScript
Stelle
0
Fork
0
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

## Status

The startup race **window** is confirmed to exist in current `main`. Actual data loss is
**plausible but unproven** — it has never been reproduced. The original description of this
issue overstated both the certainty and the size of the failure, which is likely why attempts
to reproduce it have come up empty.

This issue is now scoped as: **build a test that can catch it, then fix what the test proves.**

## Corrected mechanics

`smartem-agent watch` parses the existing tree to completion, and only then starts the observer
(`src/smartem_agent/__main__.py`):

```python
logging.info("Parsing existing directory contents...") # ~273
watcher.datastore = EpuParser.parse_epu_output_dir(watcher.datastore)
logging.info("..done! Now listening for new filesystem events")

observer = Observer()
observer.schedule(watcher, str(path), recursive=True) # ~278
...
observer.start() # ~296
```

Two corrections to the original write-up:

1. **The window is larger than previously stated.** It is not "parse-end to `schedule()`".
`EpuParser.parse_epu_output_dir` opens with
`list(datastore.root_dir.glob("**/*EpuSession.dm"))` — a full recursive walk materialised
at t0 — and the OS-level watch is not established until `observer.start()`. The exposure is
the **entire parse duration**.

2. **The window is far narrower in effect than "any data arriving is dropped".**
`SmartEMWatcherV2.watched_event_types` is `["created", "modified"]` (`fs_watcher.py:39`),
not `["created"]`. Any file still being written, or ever rewritten, when the observer starts
is still picked up via its `modified` event. The only genuinely lost file is one that is
created, finalised, and **never touched again**, entirely inside the window.

There is no mitigation elsewhere: `on_any_event` is the only event method in `fs_watcher.py`,
and there is no rescan, reconciliation, or periodic full-scan anywhere in the watcher. The
orphan manager handles out-of-*order* events, not missing ones.

## Working hypothesis: only reachable on mid-session restart

On a fresh acquisition the initial glob finds almost nothing, so the window is near-zero and
the bug is probably **not reachable at all**. It should only open meaningfully when the agent
is **restarted against a large, in-progress session**, where the parse can run for a long time
while EPU keeps writing.

If correct, this explains the issue's history: anyone reproducing it the obvious way (start the
agent, drop files in) would see nothing and reasonably conclude it was not real.

## Unknown, and not answerable from this repository

Whether EPU ever writes a matched file **write-once-never-touch** is the deciding fact, and it
is ThermoFisher's proprietary behaviour. If every EPU file gets a later `modified` event, this
bug does not exist in practice. That question is what the test needs to settle empirically.

## Plan

1. **Catch it first.** Add a regression test that reproduces the window deterministically
before changing any production code. Shape: pre-populate a large EPU tree, begin
`watch`, write additional files *during* the parse, then assert the resulting datastore is
equivalent to a clean full parse of the final tree. EPUPlayer
(`smartem-devtools/packages/smartem-epuplayer/`) is the natural driver for the concurrent
writes. Home: `tests/smartem_agent/test_fs_watcher.py`.
To make the window wide enough to hit reliably, the parse should be slowed via injection
(a hook or monkeypatched `parse_epu_output_dir`) rather than by building a genuinely huge
fixture.
2. **Fix it.** Only once the test fails for the right reason. Likely direction: start the
observer *before* the parse and buffer events until the parse completes, reconciling
against the parsed datastore (writes are already idempotent by UUID). Alternatives
considered: post-start revalidation scan; atomic transition.
3. **Retain the test.** It stays in the suite as a permanent regression guard.

## Run it on Windows as well as Linux

Production agents run on Windows EPU workstations, and **the two platforms fail differently
here — in both directions**. A green Linux suite is not evidence about production.

- **Linux (inotify):** inotify is not recursive. Watchdog registers a watch per subdirectory by
walking the tree at emitter start, and only adds watches for new subdirectories when it
processes their creation event. EPU creates deep subdirectories constantly
(`GridSquare_*/FoilHoles/...`), so there is a **second, independent race**: files landing in a
brand-new subdirectory before its watch is registered. Linux-only.
- **Windows (`ReadDirectoryChangesW`):** a single handle with `bWatchSubtree=TRUE` covers the
whole subtree, so the per-subdirectory race does not arise. However the kernel notification
buffer can overflow under burst load and drop events wholesale — a documented characteristic
of that API that watchdog surfaces poorly. Windows-only, and closer to real acquisition load.

The test should assert the same invariant on both platforms.

**This is a small CI change, not a new pipeline:**

- `.github/workflows/ci.yml:44` currently reads
`runs-on: ["ubuntu-latest"] # can add windows-latest, macos-latest`.
- `_test.yml` already takes `runs-on` as an input, so the matrix is parameterised.
- `build_win_smartem_agent.yml` already runs on `windows-latest`, so the Windows toolchain is
proven to install.

Note that adding `windows-latest` to the full test matrix will also surface any unrelated
Windows failures in the existing suite; consider a dedicated job for the filesystem tests if
that turns out to be noisy.

## Code references

- `src/smartem_agent/__main__.py` — `watch` command, ~273-296
- `src/smartem_agent/fs_watcher.py:39` — `watched_event_types`
- `src/smartem_agent/fs_watcher.py:433` — `on_any_event` (sole event method)
- `src/smartem_agent/fs_parser.py:963` — `parse_epu_output_dir`

Guida per i contributori

Nessuna guida per i contributori indicizzata per questo repository

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.