Fix sidecar discovery fallback chain and quit-time worker drain#23
Merged
Conversation
The ROI/mask sidecar fallback chain stopped at the first *existing*
candidate: a processing-only provenance export (".probeflow.json" with
"rois"/"masks": null) raised "contains no ROISet/MaskSet data" — violating
missing_ok, and since every call site (viewer open, CLI replay, feature
bridges) wraps the loader in a broad except, ROIs/masks present in a later
candidate were silently never loaded. The documented contract ("provenance
sidecars still allow replay when canonical sidecars are absent") only held
when the first existing file happened to carry the payload.
Candidate search now skips candidates that parse as valid JSON but hold no
ROI/mask payload and keeps looking; if nothing usable is found, missing_ok
returns (None, canonical_path) as documented. Two behaviours are deliberately
kept strict: corrupt files still raise (silently substituting a stale
provenance fallback for a damaged canonical sidecar would hide data loss),
and an explicit sidecar= argument still errors on a payload-less file.
An empty canonical set ("user deleted all ROIs") still wins over a stale
provenance export.
tests/test_sidecar_discovery.py covers the chain semantics plus a
writer-loader format contract for ProvenanceRecord-embedded rois/masks.
Co-Authored-By: Claude Fable 5 <[email protected]>
…seEvent _drain_worker_pools ran only from ProbeFlowWindow.closeEvent (and the restart path). With quitOnLastWindowClosed, the app can quit from a modeless image viewer closed *after* the main window: the viewer keeps starting global-pool renders meanwhile, and the final close tears the application down with no drain — interpreter teardown then races in-flight workers whose signal objects are parented to the dying QApplication (the sporadic crash-on-quit class the closeEvent drain was originally added for). _install_quit_drain (called from __init__) connects aboutToQuit to the drain; Qt drops the connection automatically if the window is destroyed first, and a double drain (closeEvent + aboutToQuit) is an idempotent bounded wait. TestShutdownDrain pins the drain waiting for in-flight work, the bounded no-raise behaviour on a stuck pool, and the wiring. Co-Authored-By: Claude Fable 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two fixes from the continued adversarial seam review (pass 2, items 3–4).
Sidecar discovery (
io/roi_sidecar.py,io/mask_sidecar.py)The ROI/mask fallback chain (
.rois.json/.masks.json→.probeflow.json→.provenance.json) stopped at the first existing candidate. A processing-only provenance export ("rois": null) raised "contains no ROISet data" — violatingmissing_ok, and since all call sites swallow loader exceptions, ROIs/masks present in a later candidate were silently never loaded. The search now skips valid-JSON candidates without a payload and keeps looking. Kept strict: corrupt files still raise (no silent stale fallback), explicitsidecar=still errors on payload-less files, and an empty canonical set still wins over a stale provenance export.Shutdown drain (
gui/app.py)_drain_worker_poolsonly ran from the main window'scloseEvent. WithquitOnLastWindowClosed, closing a modeless viewer after the main window quits the app with no drain, racing teardown against in-flight global-pool renders (the PR #20 crash-on-quit class)._install_quit_drainnow connectsaboutToQuitto the drain; double drain is idempotent, and Qt drops the connection if the window is destroyed first.Test plan
tests/test_sidecar_discovery.py(12 new tests): chain semantics, missing_ok contract, canonical-wins incl. empty-set case, corrupt-raises, writer↔loader format contract.TestShutdownDrainintests/test_browse_worker_seams.py(3 new tests).🤖 Generated with Claude Code