From cf5fa0cce2701f6ac6fdc3d91130118fad842efb Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:00:20 +0200 Subject: [PATCH 1/8] fix(h5browser): skip broken HDF5 links instead of failing to open the file A soft link to a missing object or an external link to a missing file (e.g. synchrotron master files without their detector files) made open_file fail with AttributeError: the children were iterated with values(), which yields None for unresolved links, and only some errors were caught. Valid datasets of such files could not be browsed. Children are now iterated by name; broken links are skipped with a diagnostic naming the missing target, like in DataLab desktop (SigimaX). open_file also closes the file and removes its temporary copy when the tree cannot be built, instead of only when h5py cannot open it. Assisted-by: Claude Opus 5.5 --- src/runtime/dlw_h5browser.py | 67 +++++++++++++++++++++++++--------- tests/python/test_h5browser.py | 54 +++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 18 deletions(-) diff --git a/src/runtime/dlw_h5browser.py b/src/runtime/dlw_h5browser.py index 5328899..7628d69 100644 --- a/src/runtime/dlw_h5browser.py +++ b/src/runtime/dlw_h5browser.py @@ -326,6 +326,19 @@ def _array_is_signal(shape: tuple[int, ...]) -> bool: _MAX_H5_DEPTH = 100 +def _broken_link(group: Any, name: str) -> str: + """Describe the broken link *name* of *group* (for diagnostics).""" + try: + link = group.get(name, getlink=True) + except Exception: # noqa: BLE001 + return "object cannot be opened" + if isinstance(link, h5py.ExternalLink): + return f"broken external link to '{link.filename}:{link.path}'" + if isinstance(link, h5py.SoftLink): + return f"broken soft link to '{link.path}'" + return "object cannot be opened" + + def _build_node(file_id: str, h5path: str, dset: Any, _depth: int = 0) -> _NodeInfo: """Recursively build one :class:`_NodeInfo`.""" metadata = _collect_attrs(dset) @@ -355,15 +368,23 @@ def _build_node(file_id: str, h5path: str, dset: Any, _depth: int = 0) -> _NodeI f"{h5path!r}: not descending further" ) return info - for child_dset in dset.values(): + for child_name in dset: + child_path = f"{h5path.rstrip('/')}/{child_name}" + child_dset = dset.get(child_name) + if child_dset is None: + # Broken link: soft link to a missing object, or external link + # to a missing file (e.g. detector data of synchrotron master + # files). The rest of the file stays browsable. + print( + f"[h5browser] skip {child_path!r}: {_broken_link(dset, child_name)}" + ) + continue try: info.children.append( _build_node(file_id, child_dset.name, child_dset, _depth + 1) ) except (UnicodeDecodeError, TypeError, ValueError, KeyError) as exc: - print( - f"[h5browser] skip {child_dset.name!r}: {type(exc).__name__}: {exc}" - ) + print(f"[h5browser] skip {child_path!r}: {type(exc).__name__}: {exc}") return info kind, data = _classify_dataset(dset) @@ -576,14 +597,19 @@ def open_file(filename: str, data: Any) -> dict[str, Any]: try: h5 = h5py.File(path, "r") except Exception as exc: # noqa: BLE001 - try: - os.remove(path) - os.rmdir(tmpdir) - except OSError: - pass + _remove_temp_file(path) raise ValueError(f"Cannot open {base!r} as an HDF5 file: {exc}") from exc file_id = uuid.uuid4().hex[:8] - root_info = _build_node(file_id, "/", h5) + try: + root_info = _build_node(file_id, "/", h5) + except BaseException: + # Never leave the file open nor its temporary copy behind + try: + h5.close() + except Exception: # noqa: BLE001 + pass + _remove_temp_file(path) + raise # Override root display name to file basename (mirrors Qt RootNode). root_info.name = osp.basename(filename) or osp.basename(path) root_info.icon_name = "h5file.svg" @@ -601,6 +627,18 @@ def open_file(filename: str, data: Any) -> dict[str, Any]: return {"file_id": file_id, "filename": filename, "root": _info_to_dict(root_info)} +def _remove_temp_file(path: str) -> None: + """Remove the temporary copy *path* of a browsed file and its directory.""" + try: + os.remove(path) + except OSError: + pass + try: + os.rmdir(osp.dirname(path)) + except OSError: + pass + + def close_file(file_id: str) -> None: """Close *file_id* and free its resources.""" entry = STATE.pop(file_id, None) @@ -610,14 +648,7 @@ def close_file(file_id: str) -> None: entry.h5.close() except Exception: # noqa: BLE001 pass - try: - os.remove(entry.temp_path) - except OSError: - pass - try: - os.rmdir(osp.dirname(entry.temp_path)) - except OSError: - pass + _remove_temp_file(entry.temp_path) def close_all() -> None: diff --git a/tests/python/test_h5browser.py b/tests/python/test_h5browser.py index 08a8854..1de214c 100644 --- a/tests/python/test_h5browser.py +++ b/tests/python/test_h5browser.py @@ -67,6 +67,60 @@ def test_open_invalid_file_raises(): h5b.open_file("bogus.h5", b"definitely not an HDF5 file") +def test_open_file_skips_broken_links(tmp_path, capsys): + """Broken soft or external links (e.g. synchrotron master files without + their detector files) are skipped: the rest of the file stays browsable. + + Regression: ``open_file`` raised ``AttributeError`` on the first broken link. + """ + path = tmp_path / "broken.h5" + with h5py.File(path, "w") as f: + f["ok"] = np.arange(10.0) + f["broken"] = h5py.SoftLink("/missing") + grp = f.create_group("entry") + grp["frames"] = h5py.ExternalLink("missing_detector.h5", "/frames") + grp["signal"] = np.linspace(0.0, 1.0, 8) + result = h5b.open_file("broken.h5", path.read_bytes()) + try: + nodes = h5b.STATE[result["file_id"]].nodes_by_path + assert {"/ok", "/entry", "/entry/signal"} <= set(nodes) + assert not {"/broken", "/entry/frames"} & set(nodes) + imported = h5b.import_nodes(result["file_id"], ["/ok", "/entry/signal"]) + assert len(imported["objects"]) == 2 + finally: + h5b.close_all() + output = capsys.readouterr().out + assert "'/broken': broken soft link to '/missing'" in output + assert "broken external link to 'missing_detector.h5:/frames'" in output + + +def test_open_file_cleans_up_when_tree_fails(h5_bytes, monkeypatch): + """A failure while building the tree closes the file and removes its + temporary copy; the next file opens normally.""" + tempdirs = [] + mkdtemp = h5b.tempfile.mkdtemp + + def spy_mkdtemp(*args, **kwargs): + tempdirs.append(mkdtemp(*args, **kwargs)) + return tempdirs[-1] + + def fail(*_args, **_kwargs): + raise RuntimeError("tree failure") + + monkeypatch.setattr(h5b.tempfile, "mkdtemp", spy_mkdtemp) + with monkeypatch.context() as patch: + patch.setattr(h5b, "_build_node", fail) + with pytest.raises(RuntimeError, match="tree failure"): + h5b.open_file("sample.h5", h5_bytes) + assert len(tempdirs) == 1 and not h5b.osp.exists(tempdirs[0]) + assert not h5b.STATE + result = h5b.open_file("sample.h5", h5_bytes) + try: + assert result["file_id"] in h5b.STATE + finally: + h5b.close_all() + + def test_close_file_clears_state(opened): file_id = opened["file_id"] assert file_id in h5b.STATE From 8acf4f9af26813264c2f2b2e33e77724afb90a52 Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:03:07 +0200 Subject: [PATCH 2/8] fix(h5browser): skip links back to a parent group instead of walking them An HDF5 file is a graph: a group can be linked into itself or into one of its subgroups. The browser walked such links until the depth limit (100), building phantom levels such as /entry/self/self/... and classifying their datasets again at each level; with several such links, the walk grew exponentially and the file could not be opened in practice. _build_node now passes the chain of parent groups and skips, with a diagnostic, any child group identical (same file and object) to one of them, as DataLab desktop (SigimaX) does. Other aliases of a group or dataset are still listed at each of their paths. The depth limit remains as a secondary guard. Assisted-by: Claude Opus 5.5 --- src/runtime/dlw_h5browser.py | 29 ++++++++++++++++++++++--- tests/python/test_h5browser.py | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/runtime/dlw_h5browser.py b/src/runtime/dlw_h5browser.py index 7628d69..0d021c6 100644 --- a/src/runtime/dlw_h5browser.py +++ b/src/runtime/dlw_h5browser.py @@ -339,8 +339,18 @@ def _broken_link(group: Any, name: str) -> str: return "object cannot be opened" -def _build_node(file_id: str, h5path: str, dset: Any, _depth: int = 0) -> _NodeInfo: - """Recursively build one :class:`_NodeInfo`.""" +def _build_node( + file_id: str, + h5path: str, + dset: Any, + _depth: int = 0, + _ancestors: tuple[Any, ...] = (), +) -> _NodeInfo: + """Recursively build one :class:`_NodeInfo`. + + *_ancestors* are the groups containing *dset*: a link back to one of them + (or to *dset*) closes a cycle and is skipped. + """ metadata = _collect_attrs(dset) if isinstance(dset, h5py.Group): @@ -368,6 +378,7 @@ def _build_node(file_id: str, h5path: str, dset: Any, _depth: int = 0) -> _NodeI f"{h5path!r}: not descending further" ) return info + ancestors = (*_ancestors, dset) for child_name in dset: child_path = f"{h5path.rstrip('/')}/{child_name}" child_dset = dset.get(child_name) @@ -379,9 +390,21 @@ def _build_node(file_id: str, h5path: str, dset: Any, _depth: int = 0) -> _NodeI f"[h5browser] skip {child_path!r}: {_broken_link(dset, child_name)}" ) continue + if isinstance(child_dset, h5py.Group): + # Identity comparison (file and object): a group reached again + # through a hard, soft or external link would be walked forever + looped = next((grp for grp in ancestors if grp == child_dset), None) + if looped is not None: + print( + f"[h5browser] skip {child_path!r}: link back to its parent " + f"group {looped.name!r}" + ) + continue try: info.children.append( - _build_node(file_id, child_dset.name, child_dset, _depth + 1) + _build_node( + file_id, child_dset.name, child_dset, _depth + 1, ancestors + ) ) except (UnicodeDecodeError, TypeError, ValueError, KeyError) as exc: print(f"[h5browser] skip {child_path!r}: {type(exc).__name__}: {exc}") diff --git a/tests/python/test_h5browser.py b/tests/python/test_h5browser.py index 1de214c..607a7c3 100644 --- a/tests/python/test_h5browser.py +++ b/tests/python/test_h5browser.py @@ -67,6 +67,45 @@ def test_open_invalid_file_raises(): h5b.open_file("bogus.h5", b"definitely not an HDF5 file") +def test_open_file_skips_link_cycles(tmp_path, capsys): + """Links back to a parent group are skipped, other aliases are kept. + + Regression: a group linked into itself was walked down to the depth limit, + producing 100 phantom levels (``/entry/self/self/...``). + """ + path = tmp_path / "cycles.h5" + with h5py.File(path, "w") as f: + entry = f.create_group("entry") + entry["signal"] = np.arange(10.0) + entry["self"] = entry # hard link to itself + inner = entry.create_group("inner") + inner["up"] = entry # hard link to its parent (two-group cycle) + inner["root"] = h5py.SoftLink("/") # soft link to the root group + f["alias"] = entry["signal"] # dataset alias, not a cycle + f["entry_alias"] = inner # group alias, not a cycle + result = h5b.open_file("cycles.h5", path.read_bytes()) + try: + # The group alias keeps its own path to the parent group of the aliased + # group (which is not one of its own parents) + assert set(h5b.STATE[result["file_id"]].nodes_by_path) == { + "/", + "/entry", + "/entry/signal", + "/entry/inner", + "/alias", + "/entry_alias", + "/entry_alias/up", + "/entry_alias/up/signal", + } + finally: + h5b.close_all() + output = capsys.readouterr().out + assert "'/entry/self': link back to its parent group '/entry'" in output + assert "'/entry/inner/up': link back to its parent group '/entry'" in output + assert "'/entry/inner/root': link back to its parent group '/'" in output + assert "max depth" not in output + + def test_open_file_skips_broken_links(tmp_path, capsys): """Broken soft or external links (e.g. synchrotron master files without their detector files) are skipped: the rest of the file stays browsable. From fc91e54c94e212bc699e75b0f20f06d2968eef1d Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:04:32 +0200 Subject: [PATCH 3/8] perf(h5browser): classify datasets from bounded samples when opening a file Opening an HDF5 file read every dataset entirely to classify it and build its one-line description, including datasets that cannot be imported (e.g. a 3D cube) or that the user never selects: in WebAssembly, these allocations could make a large file unusable although only a small dataset was wanted (the file itself is already copied to memory). Datasets are now classified from their shape, type and a sample of at most 100 leading elements (the same number along each axis); shapes and signal or image detection use the dataset shape. Mixed compound datasets are checked on their first records only, as in DataLab desktop. Full data is still read on preview, array display and import. A test checks that the nodes classified from samples match those classified from full reads. Assisted-by: Claude Opus 5.5 --- src/runtime/dlw_h5browser.py | 81 ++++++++++++++++++++----------- tests/python/test_h5browser.py | 88 ++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 28 deletions(-) diff --git a/src/runtime/dlw_h5browser.py b/src/runtime/dlw_h5browser.py index 0d021c6..168d6db 100644 --- a/src/runtime/dlw_h5browser.py +++ b/src/runtime/dlw_h5browser.py @@ -82,12 +82,15 @@ def _safe_decode_bytes(data: Any, fallback: str = "") -> str: return fallback -def _safe_read(dset: Any) -> Any: - """Read *dset* with encoding fallbacks; return ``None`` on failure.""" +def _safe_read(dset: Any, selection: Any = ()) -> Any: + """Read *selection* of *dset* with encoding fallbacks; ``None`` on failure.""" try: - return dset[()] + return dset[selection] except UnicodeDecodeError: - for strat in (lambda d: d.asstr()[()], lambda d: d.astype("S")[()]): + for strat in ( + lambda d: d.asstr()[selection], + lambda d: d.astype("S")[selection], + ): try: return strat(dset) except Exception: # noqa: BLE001 @@ -97,6 +100,24 @@ def _safe_read(dset: Any) -> Any: return None +#: Maximum number of elements read to classify and describe a dataset while +#: opening a file: the full data is read only on preview or import +_SAMPLE_MAX_ITEMS = 100 + + +def _read_sample(dset: Any) -> Any: + """Read a scalar *dset*, or the leading elements of an array *dset*. + + At most about :data:`_SAMPLE_MAX_ITEMS` elements are read, whatever the size + of the dataset (the same number along each axis). + """ + shape = dset.shape + if not shape: # scalar (``()``) or null dataspace (``None``) + return _safe_read(dset) + per_axis = max(1, int(_SAMPLE_MAX_ITEMS ** (1.0 / len(shape)))) + return _safe_read(dset, tuple(slice(0, min(size, per_axis)) for size in shape)) + + def _is_supported_num_dtype(arr: Any) -> bool: try: return arr.dtype.name.startswith(("int", "uint", "float", "complex")) @@ -241,28 +262,34 @@ class _NodeInfo: kind: str # "group" | "scalar" | "text" | "array" | "compound" | "image" | "signal" -def _classify_dataset(dset: Any) -> tuple[str, np.ndarray | Any]: - """Return ``(kind, data)`` for a non-Group dataset. +def _classify_dataset(dset: Any) -> tuple[str, Any, tuple[int, ...]]: + """Return ``(kind, data, shape)`` for a non-Group dataset. ``kind`` is one of ``"scalar"``, ``"text"``, ``"array"``, ``"compound"`` or ``"unknown"`` (matches Qt's GenericScalarNode / GenericTextNode / GenericArrayNode / GenericCompoundNode taxonomy). + + Only a sample of the dataset is read (see :func:`_read_sample`): ``data`` + is that sample (its numeric part for compound arrays), used to describe the + node, and ``shape`` is the shape of the whole (numeric) dataset. Mixed + compound datasets are checked on their first records only. """ - data = _safe_read(dset) + data = _read_sample(dset) + shape = tuple(dset.shape or ()) if data is None: try: if dset.dtype.names is not None: - return "compound", None + return "compound", None, shape if dset.dtype.kind in ("S", "a", "U"): - return "text", None + return "text", None, shape except Exception: # noqa: BLE001 - return "unknown", None - return "unknown", None + return "unknown", None, shape + return "unknown", None, shape if isinstance(data, np.generic) and _is_supported_num_dtype(data): - return "scalar", data + return "scalar", data, shape if isinstance(data, bytes) or _is_supported_str_dtype(data): - return "text", data + return "text", data, shape if isinstance(data, np.ndarray): if data.dtype.names is not None: @@ -272,21 +299,19 @@ def _classify_dataset(dset: Any) -> tuple[str, np.ndarray | Any]: and _is_supported_num_dtype(numeric) and numeric.ndim in (1, 2) ): - return "array", numeric - return "compound", data + # Several numeric fields are stacked along a last axis + return "array", numeric, shape + numeric.shape[data.ndim :] + return "compound", data, shape if data.dtype.kind in ("S", "a", "U"): - return "text", data + return "text", data, shape if _is_supported_num_dtype(data) and data.ndim in (1, 2): - return "array", data + return "array", data, shape - return "unknown", data + return "unknown", data, shape -def _shape_str(arr: Any) -> str: - try: - return " x ".join(str(s) for s in arr.shape) - except Exception: # noqa: BLE001 - return "" +def _shape_str(shape: tuple[int, ...]) -> str: + return " x ".join(str(s) for s in shape) def _dtype_str(dset: Any, kind: str, data: Any) -> str: @@ -410,7 +435,7 @@ def _build_node( print(f"[h5browser] skip {child_path!r}: {type(exc).__name__}: {exc}") return info - kind, data = _classify_dataset(dset) + kind, data, data_shape = _classify_dataset(dset) name = osp.basename(h5path) shape = "" @@ -420,10 +445,10 @@ def _build_node( if kind == "array": is_array = True - shape = _shape_str(data) - supported = data is not None and data.size > 1 + shape = _shape_str(data_shape) + supported = int(np.prod(data_shape)) > 1 is_supported = bool(supported) - if supported and _array_is_signal(data.shape): + if supported and _array_is_signal(data_shape): icon = "signal.svg" kind = "signal" elif supported: @@ -434,7 +459,7 @@ def _build_node( elif kind == "compound": is_array = True if data is not None: - shape = _shape_str(data) + shape = _shape_str(data_shape) icon = "h5array.svg" elif kind == "scalar": icon = "h5scalar.svg" diff --git a/tests/python/test_h5browser.py b/tests/python/test_h5browser.py index 607a7c3..3ab67ba 100644 --- a/tests/python/test_h5browser.py +++ b/tests/python/test_h5browser.py @@ -160,6 +160,94 @@ def fail(*_args, **_kwargs): h5b.close_all() +@pytest.fixture +def large_h5(tmp_path): + """Return the path of a file with large datasets of various kinds.""" + path = tmp_path / "large.h5" + table = np.zeros(5000, dtype=[("x", "f8"), ("y", "f8")]) + mixed = np.zeros(3000, dtype=[("label", "S8"), ("value", "f8")]) + mixed["label"] = b"abc" + with h5py.File(path, "w") as f: + f["small"] = np.arange(10.0) + f["vector"] = np.arange(1_000_000.0) + f["column"] = np.zeros((50_000, 1)) + f["image"] = np.zeros((300, 400), dtype=np.uint16) + f["cube"] = np.zeros((20, 30, 40)) + f["text"] = np.array([b"line"] * 1000) + f["table"] = table + f["mixed"] = mixed + f["label"] = "hello" + f["value"] = 3.5 + return path + + +def _nodes_by_path(result: dict) -> dict[str, dict]: + """Return the nodes of an ``open_file`` result by path.""" + nodes = {} + stack = [result["root"]] + while stack: + node = stack.pop() + nodes[node["id"]] = node + stack.extend(node["children"]) + return nodes + + +def test_open_file_reads_only_samples(large_h5, monkeypatch): + """Opening a file reads a bounded sample of each dataset, not its data. + + Regression: every dataset was read entirely while building the tree, even + datasets that cannot be imported (e.g. 3D cubes). + """ + reads: dict[str, int] = {} + getitem = h5py.Dataset.__getitem__ + + def spy(self, *args, **kwargs): + result = getitem(self, *args, **kwargs) + reads[self.name] = reads.get(self.name, 0) + int(np.size(result)) + return result + + monkeypatch.setattr(h5py.Dataset, "__getitem__", spy) + result = h5b.open_file("large.h5", large_h5.read_bytes()) + try: + assert reads and max(reads.values()) <= h5b._SAMPLE_MAX_ITEMS + reads.clear() + h5b.import_nodes(result["file_id"], ["/vector"]) + assert reads == {"/vector": 1_000_000} + finally: + h5b.close_all() + + +def test_sampled_classification_matches_full_read(large_h5, monkeypatch): + """The nodes classified from samples are those classified from full reads + (kind, icon, shape, type and support; only the text preview may differ).""" + sampled = h5b.open_file("large.h5", large_h5.read_bytes()) + h5b.close_all() + monkeypatch.setattr(h5b, "_read_sample", h5b._safe_read) + full = h5b.open_file("large.h5", large_h5.read_bytes()) + h5b.close_all() + sampled_nodes, full_nodes = _nodes_by_path(sampled), _nodes_by_path(full) + assert sampled_nodes.keys() == full_nodes.keys() + for path, node in full_nodes.items(): + for key in ("kind", "icon_name", "shape_str", "dtype_str", "is_supported"): + assert sampled_nodes[path][key] == node[key], (path, key) + kinds = {path: node["kind"] for path, node in sampled_nodes.items()} + assert kinds == { + "/": "group", + "/small": "signal", + "/vector": "signal", + "/column": "signal", + "/image": "image", + "/cube": "unknown", + "/text": "text", + "/table": "signal", + "/mixed": "compound", + "/label": "text", + "/value": "scalar", + } + assert sampled_nodes["/table"]["shape_str"] == "5000 x 2" + assert sampled_nodes["/image"]["shape_str"] == "300 x 400" + + def test_close_file_clears_state(opened): file_id = opened["file_id"] assert file_id in h5b.STATE From 40695d19b193baf108564552e233c25c2facca40 Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:08:09 +0200 Subject: [PATCH 4/8] fix(h5browser): keep complex parts and exact integers when importing HDF5 data The HDF5 browser converted every signal that was neither float64 nor complex128 to float64, so that complex64 signals lost their imaginary part (with only a ComplexWarning), and it clipped uint32 images to the int32 range (2 147 483 647), losing larger counts that float64 represents exactly. DataLab desktop fixed both in 1.4, but DataLab-Web keeps its own adapter. Signals are now converted to complex128 when complex, to float64 otherwise. Images follow the rules of Sigima >= 1.4 (smallest image integer type holding the values, else an exact floating-point type; complex64 to complex128), implemented locally because DataLab-Web still uses Sigima 1.3, whose converter rounds large integers to float32. 2D signal tables with complex X values or uncertainties are rejected instead of truncated. The uint32 clipping warning is replaced by a precision warning (inexact_values), shown when integers beyond 2**53 cannot be represented exactly; French translation added. Previews show the real part of complex data, which cannot be sent to the JavaScript preview. Assisted-by: Claude Opus 5.5 --- src/App.tsx | 16 ++-- src/components/H5BrowserDialog.tsx | 8 +- src/locales/fr.json | 1 + src/runtime/bootstrap.py | 5 +- src/runtime/dlw_h5browser.py | 123 +++++++++++++++++++++++------ src/runtime/runtime.ts | 4 +- tests/python/test_h5browser.py | 68 +++++++++++++++- 7 files changed, 185 insertions(+), 40 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index d2cea2c..8e5ba8b 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -3677,11 +3677,12 @@ export default function App() { Array.from(supported), null, ); - if (result.uint32_clipped) { + if (result.inexact_values) { await notify({ kind: "warning", - message: - "Some uint32 image data was clipped to int32 range during import.", + message: t( + "Some integer values could not be represented exactly and were rounded during import.", + ), }); } setWorkspaceVersion((v) => v + 1); @@ -3915,13 +3916,14 @@ export default function App() { ); const handleH5BrowserImport = useCallback( - async (oids: string[], uint32Clipped: boolean) => { + async (oids: string[], inexactValues: boolean) => { setH5BrowserFiles(null); - if (uint32Clipped) { + if (inexactValues) { await notify({ kind: "warning", - message: - "Some uint32 image data was clipped to int32 range during import.", + message: t( + "Some integer values could not be represented exactly and were rounded during import.", + ), }); } await refreshImportedObjects(oids); diff --git a/src/components/H5BrowserDialog.tsx b/src/components/H5BrowserDialog.tsx index 786907d..7429cd0 100644 --- a/src/components/H5BrowserDialog.tsx +++ b/src/components/H5BrowserDialog.tsx @@ -43,7 +43,7 @@ interface Props { initial?: H5BrowserFile[]; /** Called with the list of object ids created in the model when the * user clicks OK. */ - onImport: (oids: string[], uint32Clipped: boolean) => void; + onImport: (oids: string[], inexactValues: boolean) => void; onCancel: () => void; } @@ -362,7 +362,7 @@ export function H5BrowserDialog({ initial, onImport, onCancel }: Props) { setBusy(true); setError(null); const allOids: string[] = []; - let uintClipped = false; + let inexactValues = false; // Flatten (file, node) pairs so the progress bar reflects per-node // granularity. Each node is imported individually to keep the bar // responsive and cancellation reactive between nodes. @@ -381,7 +381,7 @@ export function H5BrowserDialog({ initial, onImport, onCancel }: Props) { setLabel(`${i + 1} / ${tasks.length} — ${nodeId}`); const result = await runtime.importH5BrowserNodes(fileId, [nodeId]); allOids.push(...result.oids); - if (result.uint32_clipped) uintClipped = true; + if (result.inexact_values) inexactValues = true; }, }); if (cancelled && allOids.length === 0) { @@ -394,7 +394,7 @@ export function H5BrowserDialog({ initial, onImport, onCancel }: Props) { await Promise.all( ids.map((fid) => runtime.closeH5Browser(fid).catch(() => {})), ); - onImport(allOids, uintClipped); + onImport(allOids, inexactValues); } catch (err) { setError(err instanceof Error ? err.message : String(err)); setBusy(false); diff --git a/src/locales/fr.json b/src/locales/fr.json index 26ab992..e52fe33 100644 --- a/src/locales/fr.json +++ b/src/locales/fr.json @@ -1196,6 +1196,7 @@ "Sobel filter (vertical)": "Filtre de Sobel (vertical)", "Software": "Logiciel", "Solid": "Continu", + "Some integer values could not be represented exactly and were rounded during import.": "Certaines valeurs entières ne pouvaient pas être représentées exactement et ont été arrondies lors de l'import.", "Some selected objects have regions of interest. Do you want to delete them as well?": "Certains objets sélectionnés ont des régions d'intérêt. Souhaitez-vous les supprimer également ?", "Source": "Source", "Source {index}": "Source {index}", diff --git a/src/runtime/bootstrap.py b/src/runtime/bootstrap.py index 8534a9c..32a1cd5 100644 --- a/src/runtime/bootstrap.py +++ b/src/runtime/bootstrap.py @@ -7474,13 +7474,14 @@ def h5_browser_import( ) -> dict[str, Any]: """Import every node in *node_ids* into the live model. - Returns ``{"oids": [...], "uint32_clipped": bool}``. + Returns ``{"oids": [...], "inexact_values": bool}``, *inexact_values* + being True when integer values could not all be represented exactly. """ result = _h5br.import_nodes(file_id, node_ids, group_id=group_id) oids: list[str] = [] for kind, obj in result["objects"]: oids.append(_MODEL.add_object(kind, obj, group_id=result["group_id"])) - return {"oids": oids, "uint32_clipped": bool(result["uint32_clipped"])} + return {"oids": oids, "inexact_values": bool(result["inexact_values"])} # --------------------------------------------------------------------------- diff --git a/src/runtime/dlw_h5browser.py b/src/runtime/dlw_h5browser.py index 168d6db..237e399 100644 --- a/src/runtime/dlw_h5browser.py +++ b/src/runtime/dlw_h5browser.py @@ -553,37 +553,107 @@ def _data_to_xy(data: np.ndarray): return x, y, dx, dy -def _build_native_signal(node_data: np.ndarray, title: str): - """Convert *node_data* into a :class:`SignalObj` (or ``None``).""" +def _to_signal_values(data: np.ndarray) -> tuple[np.ndarray, bool]: + """Return signal values as float64, or as complex128 for complex data. + + Returns ``(values, inexact)``: *inexact* is True when integer values cannot + all be represented exactly as float64 (beyond 2**53). + """ + data = np.asarray(data) + if np.iscomplexobj(data): + return data.astype(np.complex128, copy=False), False + return data.astype(np.float64, copy=False), _int_exceeds(data, np.float64) + + +def _int_exceeds(data: np.ndarray, dtype: Any) -> bool: + """Return True if integer *data* cannot all be represented exactly as + floating-point *dtype*.""" + if data.dtype.kind not in "iu" or data.size == 0: + return False + magnitude = max(abs(int(data.min())), abs(int(data.max()))) + return magnitude > 2 ** (np.finfo(dtype).nmant + 1) + + +# The conversion of image data follows Sigima >= 1.4 +# (``sigima.io.common.converters.convert_array_to_valid_dtype``), which DataLab +# desktop uses: replace it by Sigima's converter once DataLab-Web requires +# Sigima >= 1.4 (Sigima 1.3 rounds large integers to float32). +_INT_CODES = ("u1", "i1", "u2", "i2", "u4", "i4", "u8", "i8") +_FLOAT_CODES = {"f": ("f2", "f4", "f8"), "c": ("c8", "c16")} + + +def _to_image_data(data: np.ndarray) -> tuple[np.ndarray, bool]: + """Convert *data* to a data type of image objects without losing values. + + Integers use the smallest supported integer type holding all values, or else + the smallest supported floating-point type (at least as large as the input) + representing them exactly; other types use the smallest supported type of + the same kind at least as large (e.g. ``complex64`` to ``complex128``). + + Returns ``(data, inexact)``: *inexact* is True when no supported type + represents all integer values exactly (beyond 2**53). + """ + from sigima.objects import ImageObj + + data = np.asarray(data) + valid = [np.dtype(dtype) for dtype in ImageObj.VALID_DTYPES] + if data.dtype in valid: + return data, False + kind = data.dtype.kind + if kind in "iu": + amin, amax = (int(data.min()), int(data.max())) if data.size else (0, 0) + for code in _INT_CODES: + info = np.iinfo(code) + if np.dtype(code) in valid and info.min <= amin and amax <= info.max: + return data.astype(code), False + floats = [np.dtype(code) for code in _FLOAT_CODES["f"]] + floats = [dtype for dtype in floats if dtype in valid] + for dtype in floats: + if dtype.itemsize >= data.dtype.itemsize and not _int_exceeds(data, dtype): + return data.astype(dtype), False + return data.astype(floats[-1]), True + if kind in _FLOAT_CODES: + same_kind = [np.dtype(code) for code in _FLOAT_CODES[kind]] + same_kind = [dtype for dtype in same_kind if dtype in valid] + for dtype in same_kind: + if dtype.itemsize >= data.dtype.itemsize: + return data.astype(dtype), False + return data.astype(same_kind[-1]), False + return data, False + + +def _build_native_signal(node_data: np.ndarray, title: str) -> tuple[Any, bool]: + """Convert *node_data* into a :class:`SignalObj`. + + Returns ``(signal_obj, inexact)``, see :func:`_to_signal_values`. + + Raises: + ValueError: if a 2D table has complex X values or uncertainties + """ from sigima.objects import create_signal obj = create_signal(title) - data = node_data - if data.dtype not in (float, np.complex128): - data = np.asarray(data, dtype=float) + data, inexact = _to_signal_values(node_data) if data.ndim == 1: obj.set_xydata(np.arange(data.size), data) else: x, y, dx, dy = _data_to_xy(data) + if any(np.iscomplexobj(array) for array in (x, dx, dy)): + raise ValueError("complex X values or uncertainties are not supported") obj.set_xydata(x, y, dx, dy) - return obj + return obj, inexact def _build_native_image(node_data: np.ndarray, title: str) -> tuple[Any, bool]: """Convert *node_data* into a :class:`ImageObj`. - Returns ``(image_obj, uint32_clipped)``. + Returns ``(image_obj, inexact)``, see :func:`_to_image_data`. """ from sigima.objects import create_image - uint32_clipped = False - data = node_data - if data.dtype == np.uint32: - uint32_clipped = bool(data.max() > np.iinfo(np.int32).max) - data = np.asarray(data.clip(0, np.iinfo(np.int32).max), dtype=np.int32) obj = create_image(title) - obj.data = data - return obj, uint32_clipped + obj.data, inexact = _to_image_data(node_data) + return obj, inexact # --------------------------------------------------------------------------- @@ -748,20 +818,22 @@ def preview(file_id: str, node_id: str) -> dict[str, Any]: return {"kind": "unsupported"} raw = numeric try: + # Complex values cannot be sent to the JavaScript preview: like PlotPy + # curves and images, the preview shows their real part if info.kind == "signal": - obj = _build_native_signal(raw, info.name) + obj, _inexact = _build_native_signal(raw, info.name) return { "kind": "signal", "title": obj.title, "x": obj.x.tolist(), - "y": obj.y.tolist(), + "y": np.real(obj.y).tolist(), } # image - img, _u32 = _build_native_image(raw, info.name) + img, _inexact = _build_native_image(raw, info.name) return { "kind": "image", "title": img.title, - "data": img.data.tolist(), + "data": np.real(img.data).tolist(), "width": int(img.data.shape[1]), "height": int(img.data.shape[0]), } @@ -793,7 +865,9 @@ def array_data(file_id: str, node_id: str) -> dict[str, Any]: def import_nodes(file_id: str, node_ids: Any, group_id: str | None = None): """Import every node id in *node_ids* into the live model. - Returns a dict ``{"oids": [...], "uint32_clipped": bool}``. Caller + Returns a dict ``{"objects": [...], "inexact_values": bool, "group_id": ...}``, + *inexact_values* being True when integer values could not all be + represented exactly (beyond 2**53). Caller (``bootstrap.py``) is responsible for actually attaching the objects to the live :data:`_MODEL`; this function returns the constructed native objects, paired with their target panel kind, so we do not @@ -806,7 +880,7 @@ def import_nodes(file_id: str, node_ids: Any, group_id: str | None = None): raise KeyError(f"Unknown HDF5 browser file: {file_id!r}") entry = STATE[file_id] built: list[tuple[str, Any]] = [] - uint32_clipped = False + inexact_values = False for nid in node_ids: if nid not in entry.nodes_by_path: raise KeyError(f"Unknown HDF5 node: {nid!r}") @@ -824,16 +898,15 @@ def import_nodes(file_id: str, node_ids: Any, group_id: str | None = None): raw = numeric try: if info.kind == "signal": - obj = _build_native_signal(raw, info.name) + obj, inexact = _build_native_signal(raw, info.name) kind = "signal" else: - obj, clipped = _build_native_image(raw, info.name) - if clipped: - uint32_clipped = True + obj, inexact = _build_native_image(raw, info.name) kind = "image" except Exception as exc: # noqa: BLE001 print(f"[h5browser] failed to build object for {nid!r}: {exc}") continue + inexact_values = inexact_values or inexact # Attach origin metadata (mirrors Qt BaseNode.__process_metadata). try: obj.set_metadata_option("HDF5Path", entry.filename) @@ -842,4 +915,4 @@ def import_nodes(file_id: str, node_ids: Any, group_id: str | None = None): except Exception: # noqa: BLE001 pass built.append((kind, obj)) - return {"objects": built, "uint32_clipped": uint32_clipped, "group_id": group_id} + return {"objects": built, "inexact_values": inexact_values, "group_id": group_id} diff --git a/src/runtime/runtime.ts b/src/runtime/runtime.ts index 92a323f..843f25c 100644 --- a/src/runtime/runtime.ts +++ b/src/runtime/runtime.ts @@ -1299,7 +1299,9 @@ export interface H5BrowserArray { /** Result of importing selected HDF5 nodes into the live model. */ export interface H5BrowserImportResult { oids: string[]; - uint32_clipped: boolean; + /** True when integer values could not all be represented exactly + * (beyond 2**53). */ + inexact_values: boolean; } /** diff --git a/tests/python/test_h5browser.py b/tests/python/test_h5browser.py index 3ab67ba..a79cefb 100644 --- a/tests/python/test_h5browser.py +++ b/tests/python/test_h5browser.py @@ -352,7 +352,73 @@ def test_import_nodes_builds_objects(opened): result = h5b.import_nodes(file_id, ["/sig", "/img"]) kinds = sorted(kind for kind, _obj in result["objects"]) assert kinds == ["image", "signal"] - assert result["uint32_clipped"] is False + assert result["inexact_values"] is False + + +@pytest.fixture +def typed_h5(tmp_path): + """Open a file with datasets of various numeric types (closed afterwards).""" + path = tmp_path / "types.h5" + with h5py.File(path, "w") as f: + f["c64_signal"] = np.array([1 + 2j, 3 + 4j, 5 - 6j], dtype=np.complex64) + f["c64_image"] = np.full((4, 4), 1 - 2j, dtype=np.complex64) + f["u32_image"] = np.array( + [[1, 2**32 - 1, 5], [2**31, 7, 8], [9, 10, 11]], dtype=np.uint32 + ) + f["u16_image"] = np.arange(65527, 65536, dtype=np.uint16).reshape(3, 3) + f["i64_image"] = np.array([[0, 2**60 + 1, 2], [3, 4, 5], [6, 7, 8]], np.int64) + f["i64_signal"] = np.array([0, 2**60 + 1, 3], dtype=np.int64) + f["f32_signal"] = np.array([0.5, 1.5, 2.5], dtype=np.float32) + result = h5b.open_file("types.h5", path.read_bytes()) + try: + yield result["file_id"] + finally: + h5b.close_all() + + +def _import_one(file_id: str, node_id: str): + """Import one node and return ``(object, inexact_values)``.""" + result = h5b.import_nodes(file_id, [node_id]) + ((_kind, obj),) = result["objects"] + return obj, result["inexact_values"] + + +def test_import_complex64_keeps_imaginary_part(typed_h5): + """Regression: complex64 signals lost their imaginary part.""" + signal, inexact = _import_one(typed_h5, "/c64_signal") + assert signal.y.dtype == np.complex128 and not inexact + np.testing.assert_array_equal(signal.y, [1 + 2j, 3 + 4j, 5 - 6j]) + image, inexact = _import_one(typed_h5, "/c64_image") + assert image.data.dtype == np.complex128 and not inexact + assert image.data[0, 0] == 1 - 2j + + +def test_import_integers_without_clipping(typed_h5): + """Regression: uint32 images were clipped to the int32 range.""" + image, inexact = _import_one(typed_h5, "/u32_image") + assert image.data.dtype == np.float64 and not inexact + assert image.data[0, 1] == 2**32 - 1 and image.data[1, 0] == 2**31 + image, inexact = _import_one(typed_h5, "/u16_image") + assert image.data.dtype == np.uint16 and not inexact + signal, inexact = _import_one(typed_h5, "/f32_signal") + assert signal.y.dtype == np.float64 and not inexact + + +@pytest.mark.parametrize("node_id", ["/i64_image", "/i64_signal"]) +def test_import_flags_inexact_integers(typed_h5, node_id): + """Integers beyond 2**53 are converted to float64 and reported.""" + obj, inexact = _import_one(typed_h5, node_id) + assert inexact + values = obj.data if node_id.endswith("image") else obj.y + assert values.dtype == np.float64 + + +def test_preview_complex_and_large_integers(typed_h5): + """Previews show the real part of complex data and exact integers.""" + prev = h5b.preview(typed_h5, "/c64_signal") + assert prev["kind"] == "signal" and prev["y"] == [1.0, 3.0, 5.0] + prev = h5b.preview(typed_h5, "/u32_image") + assert prev["kind"] == "image" and prev["data"][0][1] == 2**32 - 1 def test_import_nodes_unknown_id_raises(opened): From 76efa72a2895bc18017213adad8989f923980f3d Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:11:23 +0200 Subject: [PATCH 5/8] fix(storage): keep the data of an object whose OPFS write fails In on-disk storage mode, spilling an object serialized its array and replaced it with a placeholder in Python before the bytes were written to OPFS. When the write failed (quota exceeded, write error...), nothing attached the bytes back: the only copy of a new or changed object was lost, Python considered it spilled while the JavaScript registry did not. The rewrite of a paged-in object changed by a call had the same flaw. The bytes are now attached back when they cannot be persisted, before the error is propagated: the object stays resident with its latest values and both registries agree it is not spilled. attach_object_array gets an on_disk flag so that such an object is serialized again (never just released) when it is next returned to disk, and a partially written copy is deleted. The asynchronous OPFS store also aborts a failed write instead of committing it over the previous copy. Assisted-by: Claude Opus 5.5 --- src/runtime/bootstrap.py | 10 +- src/runtime/runtime.ts | 35 ++++- src/storage/opfsObjectStore.ts | 7 +- tests/python/test_disk_spill_lifecycle.py | 22 +++ tests/ts/runtime/diskSpillRollback.test.ts | 131 ++++++++++++++++ tests/ts/storage/opfsObjectStore.test.ts | 164 +++++++++++++++++++++ 6 files changed, 362 insertions(+), 7 deletions(-) create mode 100644 tests/ts/runtime/diskSpillRollback.test.ts create mode 100644 tests/ts/storage/opfsObjectStore.test.ts diff --git a/src/runtime/bootstrap.py b/src/runtime/bootstrap.py index 32a1cd5..9bd6364 100644 --- a/src/runtime/bootstrap.py +++ b/src/runtime/bootstrap.py @@ -3143,13 +3143,16 @@ def detach_object_array(oid: str) -> bytes: return payload -def attach_object_array(oid: str, payload: bytes) -> bool: +def attach_object_array(oid: str, payload: bytes, on_disk: bool = True) -> bool: """Restore *oid*'s heavy array from ``.npy`` *payload* (page-in). Args: oid: object identifier in :data:`_MODEL`. payload: the ``.npy`` bytes previously returned by :func:`detach_object_array` (read back from OPFS by JS). + on_disk: whether the on-disk copy of the object is *payload* (page-in). + False when *payload* could not be persisted: the object then has no + current on-disk copy, and is serialized again before being released. Returns: ``True`` when the array was restored, ``False`` when *oid* was not @@ -3174,7 +3177,10 @@ def attach_object_array(oid: str, payload: bytes) -> bool: arr = np.load(io.BytesIO(data), allow_pickle=False) setattr(entry.obj, _SPILLED[oid]["attr"], arr) _SPILLED.pop(oid, None) - _PAGED_IN_REVISIONS[oid] = _DATA_REVISIONS.get(oid, 0) + if on_disk: + _PAGED_IN_REVISIONS[oid] = _DATA_REVISIONS.get(oid, 0) + else: + _PAGED_IN_REVISIONS.pop(oid, None) return True diff --git a/src/runtime/runtime.ts b/src/runtime/runtime.ts index 843f25c..d0503be 100644 --- a/src/runtime/runtime.ts +++ b/src/runtime/runtime.ts @@ -2324,18 +2324,45 @@ await micropip.install(${JSON.stringify(bootRequirements)}) return pagedIn; } - /** Spill object *oid* to disk, serialising it afresh (direct, unqueued). */ + /** Spill object *oid* to disk, serialising it afresh (direct, unqueued). + * + * The Python side drops the array when it serialises it: if the bytes + * cannot be persisted (quota exceeded, write error...), they are attached + * back before the error is propagated, so that the object stays resident + * with its data and both registries agree it is not spilled. */ private async spillDirect(oid: string): Promise { if (this.spilledOids.has(oid)) return false; const bytes = (await this.invokePy("detach_object_array", { oid, })) as Uint8Array | null; if (!bytes || bytes.byteLength === 0) return false; - await this.requireStore().put(oid, bytes); + await this.persistOrRestore(oid, bytes); this.spilledOids.add(oid); return true; } + /** Persist the *bytes* just detached from object *oid*, or attach them + * back to the object if they cannot be persisted (and rethrow). A copy + * left partially written on disk is deleted (best effort): the object is + * no longer spilled, so it would never be read anyway. */ + private async persistOrRestore( + oid: string, + bytes: Uint8Array, + ): Promise { + const store = this.requireStore(); + try { + await store.put(oid, bytes); + } catch (err) { + await this.invokePy("attach_object_array", { + oid, + payload: bytes, + on_disk: false, + }); + await store.delete(oid).catch(() => undefined); + throw err; + } + } + /** Return a paged-in object to disk once the call that needed it is over * (direct, unqueued). Its on-disk copy is rewritten only when the call * changed its data (always when *rewrite* is set), and deleted when the @@ -2354,7 +2381,9 @@ await micropip.install(${JSON.stringify(bootRequirements)}) return; } if (settled.state === "rewritten") { - await this.requireStore().put(oid, settled.payload as Uint8Array); + // The changed data is attached back if it cannot replace the on-disk + // copy: the object then stays resident (see ``persistOrRestore``) + await this.persistOrRestore(oid, settled.payload as Uint8Array); } this.spilledOids.add(oid); } diff --git a/src/storage/opfsObjectStore.ts b/src/storage/opfsObjectStore.ts index 01f4322..fed95ab 100644 --- a/src/storage/opfsObjectStore.ts +++ b/src/storage/opfsObjectStore.ts @@ -116,9 +116,12 @@ export class OpfsObjectStore implements ObjectByteStore { ? bytes.buffer : bytes.slice().buffer) as ArrayBuffer, ); - } finally { - await writable.close(); + } catch (err) { + // Discard the partial write: closing would commit it over the file + await writable.abort().catch(() => undefined); + throw err; } + await writable.close(); this.sizes.set(oid, bytes.byteLength); } diff --git a/tests/python/test_disk_spill_lifecycle.py b/tests/python/test_disk_spill_lifecycle.py index 1ebfa3a..83b4bd5 100644 --- a/tests/python/test_disk_spill_lifecycle.py +++ b/tests/python/test_disk_spill_lifecycle.py @@ -49,6 +49,28 @@ def test_settle_releases_data_left_unchanged(fresh_bootstrap): assert bs._MODEL.get(oid).xydata.shape == (2, 200) +def test_unpersisted_payload_is_attached_back(fresh_bootstrap): + """A payload that could not be persisted is attached back as the only copy. + + The runtime attaches back the bytes of a failed OPFS write: the object must + be resident with its data, not spilled, and never released afterwards + without being serialized again (its on-disk copy is not current). + """ + bs = fresh_bootstrap + oid = _add_signal(bs) + expected = bs._MODEL.get(oid).xydata.copy() + payload = _spill(bs, oid) + assert bs.is_object_spilled(oid) + + assert bs.attach_object_array(oid, payload, on_disk=False) + + assert not bs.is_object_spilled(oid) + np.testing.assert_array_equal(bs._MODEL.get(oid).xydata, expected) + settled = bs.settle_object_array(oid) + assert settled["state"] == "rewritten" + np.testing.assert_array_equal(_decode(settled["payload"]), expected) + + def test_settle_rewrites_data_changed_by_the_call(fresh_bootstrap): bs = fresh_bootstrap oid = _add_signal(bs) diff --git a/tests/ts/runtime/diskSpillRollback.test.ts b/tests/ts/runtime/diskSpillRollback.test.ts new file mode 100644 index 0000000..8531e13 --- /dev/null +++ b/tests/ts/runtime/diskSpillRollback.test.ts @@ -0,0 +1,131 @@ +import { describe, expect, it, vi } from "vitest"; + +import { DataLabRuntime } from "../../../src/runtime/runtime"; + +/** Private disk-mode members exercised by these tests. */ +type RuntimeInternals = { + py: { + globals: { + get: (name: string) => { + callKwargs: (kwargs: Record) => unknown; + destroy: () => void; + }; + }; + }; + storageMode: "ram" | "disk"; + opfsStore: { + put: (oid: string, bytes: Uint8Array) => Promise; + delete: (oid: string) => Promise; + }; + spilledOids: Set; + spillDirect(oid: string): Promise; + settleDirect(oid: string, rewrite: boolean): Promise; +}; + +const PAYLOAD = new Uint8Array([1, 2, 3, 4]); + +/** + * Disk-mode runtime whose fake Python side mirrors the spill bookkeeping of + * ``bootstrap.py`` (``detach_object_array`` drops the array and marks the + * object spilled, ``attach_object_array`` restores it), with an OPFS store + * whose ``put`` behaves as *put*. + */ +function makeRuntime(put: (oid: string, bytes: Uint8Array) => Promise) { + const runtime = Object.create(DataLabRuntime.prototype) as RuntimeInternals; + const calls: Array<{ name: string; kwargs: Record }> = []; + const py = { spilled: new Set() }; + const handlers: Record) => unknown> = + { + detach_object_array: ({ oid }) => { + py.spilled.add(String(oid)); + return PAYLOAD; + }, + attach_object_array: ({ oid }) => { + py.spilled.delete(String(oid)); + return true; + }, + settle_object_array: ({ oid }) => { + py.spilled.add(String(oid)); + return { state: "rewritten", payload: PAYLOAD }; + }, + }; + runtime.py = { + globals: { + get: (name) => ({ + callKwargs: (kwargs) => { + calls.push({ name, kwargs }); + return handlers[name]?.(kwargs) ?? null; + }, + destroy: () => {}, + }), + }, + }; + runtime.storageMode = "disk"; + const store = { + put: vi.fn(put), + delete: vi.fn<(oid: string) => Promise>(async () => {}), + }; + runtime.opfsStore = store; + runtime.spilledOids = new Set(); + const attachCalls = () => + calls.filter((c) => c.name === "attach_object_array").map((c) => c.kwargs); + return { runtime, store, py, attachCalls }; +} + +const quotaExceeded = async () => { + throw new DOMException("quota exceeded", "QuotaExceededError"); +}; + +describe("disk spill — a failed OPFS write never loses the data", () => { + it("spills an object once its bytes are persisted", async () => { + const { runtime, store, py, attachCalls } = makeRuntime(async () => {}); + + await expect(runtime.spillDirect("A")).resolves.toBe(true); + + expect(store.put).toHaveBeenCalledWith("A", PAYLOAD); + expect(runtime.spilledOids).toEqual(new Set(["A"])); + expect(py.spilled).toEqual(new Set(["A"])); + expect(attachCalls()).toEqual([]); + }); + + it("attaches the bytes back when the first spill cannot be written", async () => { + const { runtime, store, py, attachCalls } = makeRuntime(quotaExceeded); + + await expect(runtime.spillDirect("A")).rejects.toThrow("quota exceeded"); + + // Same bytes, flagged as not persisted: the object is resident again + expect(attachCalls()).toEqual([ + { oid: "A", payload: PAYLOAD, on_disk: false }, + ]); + expect(py.spilled).toEqual(new Set()); + expect(runtime.spilledOids).toEqual(new Set()); + // A partially written copy is removed + expect(store.delete).toHaveBeenCalledWith("A"); + }); + + it("attaches the changed data back when its rewrite cannot be written", async () => { + const { runtime, py, attachCalls } = makeRuntime(quotaExceeded); + + await expect(runtime.settleDirect("A", false)).rejects.toThrow( + "quota exceeded", + ); + + expect(attachCalls()).toEqual([ + { oid: "A", payload: PAYLOAD, on_disk: false }, + ]); + expect(py.spilled).toEqual(new Set()); + expect(runtime.spilledOids).toEqual(new Set()); + }); + + it("keeps the data resident when a forced rewrite cannot be written", async () => { + const { runtime, py, attachCalls } = makeRuntime(quotaExceeded); + + await expect(runtime.settleDirect("A", true)).rejects.toThrow( + "quota exceeded", + ); + + expect(attachCalls()).toHaveLength(1); + expect(py.spilled).toEqual(new Set()); + expect(runtime.spilledOids).toEqual(new Set()); + }); +}); diff --git a/tests/ts/storage/opfsObjectStore.test.ts b/tests/ts/storage/opfsObjectStore.test.ts new file mode 100644 index 0000000..fac8685 --- /dev/null +++ b/tests/ts/storage/opfsObjectStore.test.ts @@ -0,0 +1,164 @@ +/** + * Unit tests for {@link OpfsObjectStore} — the asynchronous OPFS backend used + * by the on-disk runtime mode on the main thread. + * + * A real OPFS is unavailable under jsdom, so we inject a fake OPFS tree whose + * file handles mimic ``createWritable``: writes go to a swap buffer that + * ``close`` commits and ``abort`` discards. + */ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { OpfsObjectStore } from "../../../src/storage/opfsObjectStore"; + +class FakeFile { + bytes = new Uint8Array(0); +} + +/** Fake ``FileSystemWritableFileStream`` writing to a swap buffer. */ +class FakeWritable { + private swap: Uint8Array | null = null; + closed = false; + aborted = false; + constructor( + private readonly file: FakeFile, + private readonly failWrite: () => boolean, + ) {} + async write(data: ArrayBuffer): Promise { + // A failing write leaves a partial swap file behind + this.swap = new Uint8Array(data).slice(0, 1); + if (this.failWrite()) { + throw new DOMException("quota exceeded", "QuotaExceededError"); + } + this.swap = new Uint8Array(data).slice(); + } + async close(): Promise { + this.closed = true; + if (this.swap) this.file.bytes = this.swap; + } + async abort(): Promise { + this.aborted = true; + this.swap = null; + } +} + +class FakeFileHandle { + constructor( + private readonly file: FakeFile, + private readonly owner: FakeDirHandle, + ) {} + async createWritable(): Promise { + const writable = new FakeWritable(this.file, () => this.owner.failWrites); + this.owner.writables.push(writable); + return writable; + } + async getFile(): Promise { + return new Blob([this.file.bytes.slice()]); + } +} + +class FakeDirHandle { + files = new Map(); + subdirs = new Map(); + writables: FakeWritable[] = []; + failWrites = false; + async getDirectoryHandle( + name: string, + opts?: { create?: boolean }, + ): Promise { + let d = this.subdirs.get(name); + if (!d) { + if (!opts?.create) throw new Error("NotFound"); + d = new FakeDirHandle(); + this.subdirs.set(name, d); + } + return d; + } + async getFileHandle( + name: string, + opts?: { create?: boolean }, + ): Promise { + let f = this.files.get(name); + if (!f) { + if (!opts?.create) throw new Error("NotFound"); + f = new FakeFile(); + this.files.set(name, f); + } + return new FakeFileHandle(f, this); + } + async removeEntry(name: string): Promise { + if (!this.files.delete(name) && !this.subdirs.delete(name)) { + throw new Error("NotFound"); + } + } +} + +const ORIGINAL = { + storage: (navigator as { storage?: unknown }).storage, + secure: globalThis.isSecureContext, +}; + +let opfsRoot: FakeDirHandle; + +/** Return the directory holding the files of *store* in the fake tree. */ +function storeDir(): FakeDirHandle { + const dirs: FakeDirHandle[] = []; + const walk = (dir: FakeDirHandle) => { + if (dir.files.size) dirs.push(dir); + dir.subdirs.forEach(walk); + }; + walk(opfsRoot); + expect(dirs).toHaveLength(1); + return dirs[0]; +} + +beforeEach(() => { + opfsRoot = new FakeDirHandle(); + Object.defineProperty(navigator, "storage", { + configurable: true, + value: { getDirectory: async () => opfsRoot }, + }); + Object.defineProperty(globalThis, "isSecureContext", { + configurable: true, + value: true, + }); +}); + +afterEach(() => { + Object.defineProperty(navigator, "storage", { + configurable: true, + value: ORIGINAL.storage, + }); + Object.defineProperty(globalThis, "isSecureContext", { + configurable: true, + value: ORIGINAL.secure, + }); +}); + +describe("OpfsObjectStore", () => { + it("round-trips a payload byte-for-byte", async () => { + const store = new OpfsObjectStore(); + await store.init(); + await store.put("obj-1", new Uint8Array([1, 2, 3, 250])); + expect(Array.from(await store.get("obj-1"))).toEqual([1, 2, 3, 250]); + expect(store.count()).toBe(1); + }); + + it("discards a failed write instead of committing it", async () => { + const store = new OpfsObjectStore(); + await store.init(); + await store.put("obj-1", new Uint8Array([1, 2, 3])); + const dir = storeDir(); + dir.failWrites = true; + + await expect( + store.put("obj-1", new Uint8Array([7, 8, 9, 10])), + ).rejects.toThrow("quota exceeded"); + + const failed = dir.writables[dir.writables.length - 1]; + expect(failed.aborted).toBe(true); + expect(failed.closed).toBe(false); + // The previous copy is intact + expect(Array.from(await store.get("obj-1"))).toEqual([1, 2, 3]); + expect(store.totalBytes()).toBe(3); + }); +}); From 5f47bd51f90d45529c71eb88e2fb44d71fbd1a13 Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:14:41 +0200 Subject: [PATCH 6/8] fix(storage): give each session its own OPFS directory All on-disk object stores of an origin used the same OPFS directory (dlw-object-store), and clearing a store removed that whole directory. With two tabs in on-disk storage mode, switching one back to RAM or replacing its workspace deleted the arrays spilled by the other tab, whose objects then could no longer be paged in. Each store now keeps its files in its own session directory under dlw-object-sessions, owned through an exclusive Web Lock held for the lifetime of the page or worker; clear() only resets that directory. When a store is opened, the directories of ended sessions (lock free: closed tab, crash) are removed; the lock of a new session is acquired before its directory is created, so that it is never taken for an abandoned one. Without the Web Locks API, other directories are left in place. The shared directory of earlier versions is not touched, since an earlier version still open in another tab may use it. Assisted-by: Claude Opus 5.5 --- src/storage/opfsObjectStore.ts | 37 +++-- src/storage/opfsSession.ts | 143 +++++++++++++++++ src/storage/opfsSyncObjectStore.ts | 30 ++-- src/storage/pluginWheelStore.ts | 5 +- tests/ts/storage/opfsSession.test.ts | 230 +++++++++++++++++++++++++++ 5 files changed, 412 insertions(+), 33 deletions(-) create mode 100644 src/storage/opfsSession.ts create mode 100644 tests/ts/storage/opfsSession.test.ts diff --git a/src/storage/opfsObjectStore.ts b/src/storage/opfsObjectStore.ts index fed95ab..c3ccbff 100644 --- a/src/storage/opfsObjectStore.ts +++ b/src/storage/opfsObjectStore.ts @@ -19,14 +19,20 @@ * bounded writes/deletes and per-file locking — unlike a single * workspace file that would serialise every access behind one lock. * + * Each store keeps its files in the directory of its own session (see + * :mod:`opfsSession`), so that the stores of several tabs never share or + * clear each other's files. + * * The store is deliberately dependency-free and UI-agnostic: it deals in * opaque ``Uint8Array`` payloads keyed by object id. The runtime decides * *what* those bytes are (``.npy`` encodings produced by * ``bootstrap.detach_object_array``). */ - -/** Directory name under the OPFS root holding the spilled object files. */ -export const STORE_DIR = "dlw-object-store"; +import { + openSessionDirectory, + resetSessionDirectory, + type SessionDirectory, +} from "./opfsSession"; /** Per-object filename for object id *oid*. */ export function fileName(oid: string): string { @@ -61,11 +67,11 @@ export interface ObjectByteStore { * A directory-of-objects byte store on top of OPFS. * * All methods are asynchronous. Instances are cheap; call {@link init} - * once before use (idempotent). Not safe for concurrent use across - * threads on the same directory (one owner per OPFS subtree). + * once before use (idempotent). Each instance owns its session directory + * (one owner per OPFS subtree). */ export class OpfsObjectStore implements ObjectByteStore { - private root: FileSystemDirectoryHandle | null = null; + private session: SessionDirectory | null = null; private dir: FileSystemDirectoryHandle | null = null; /** Tracks the on-disk byte size of each stored object (for totals). */ private readonly sizes = new Map(); @@ -82,14 +88,16 @@ export class OpfsObjectStore implements ObjectByteStore { ); } - /** Open (creating if needed) the dedicated store directory. Idempotent. */ + /** Create the session directory of the store. Idempotent. */ async init(): Promise { if (this.initialised) return; if (!OpfsObjectStore.isSupported()) { throw new Error("OPFS is not available in this browser/context"); } - this.root = await navigator.storage.getDirectory(); - this.dir = await this.root.getDirectoryHandle(STORE_DIR, { create: true }); + this.session = await openSessionDirectory( + await navigator.storage.getDirectory(), + ); + this.dir = this.session.dir; this.initialised = true; } @@ -156,16 +164,11 @@ export class OpfsObjectStore implements ObjectByteStore { this.sizes.delete(oid); } - /** Remove every stored payload and reset the directory. */ + /** Remove every payload of this store (and only those). */ async clear(): Promise { - if (!this.root) return; - try { - await this.root.removeEntry(STORE_DIR, { recursive: true }); - } catch { - // Directory may not exist yet. - } + if (!this.session) return; this.sizes.clear(); - this.dir = await this.root.getDirectoryHandle(STORE_DIR, { create: true }); + this.dir = await resetSessionDirectory(this.session); } /** Total on-disk byte size of all payloads written through this instance. */ diff --git a/src/storage/opfsSession.ts b/src/storage/opfsSession.ts new file mode 100644 index 0000000..c6cbe7c --- /dev/null +++ b/src/storage/opfsSession.ts @@ -0,0 +1,143 @@ +/** + * Per-session directories of the on-disk object stores. + * + * Several DataLab-Web sessions (tabs, windows) of the same origin share one + * OPFS. Each object store therefore keeps its files in its own session + * directory, owned through an exclusive Web Lock held for the lifetime of the + * page (or worker): clearing a store only removes its own directory, and the + * directories of sessions that ended without cleaning up (closed tab, crash) + * are removed by the next session that opens a store, once their lock is + * free. Without the Web Locks API, abandoned directories are left in place: + * nothing tells them apart from those of running sessions. + */ + +/** Parent of the session directories, under the OPFS root. Distinct from the + * shared directory of earlier versions (``dlw-object-store``), which an + * earlier version still open in another tab may clear at any time. */ +export const SESSIONS_DIR = "dlw-object-sessions"; + +/** Prefix of the Web Lock owning a session directory. */ +export const SESSION_LOCK_PREFIX = "dlw-object-session:"; + +/** Subset of the Web Locks API used here. */ +interface LockManagerLike { + request( + name: string, + options: { mode?: "exclusive" | "shared"; ifAvailable?: boolean }, + callback: (lock: unknown) => unknown, + ): Promise; +} + +/** Directory handle able to list its entries. */ +type ListableDirectory = FileSystemDirectoryHandle & { + values(): AsyncIterable; +}; + +/** Session directory of an object store. */ +export interface SessionDirectory { + /** Parent of all session directories. */ + parent: FileSystemDirectoryHandle; + /** Name of the session directory in *parent*. */ + name: string; + /** The session directory. */ + dir: FileSystemDirectoryHandle; +} + +function lockManager(): LockManagerLike | null { + const locks = ( + globalThis.navigator as { locks?: Partial } | undefined + )?.locks; + return locks && typeof locks.request === "function" + ? (locks as LockManagerLike) + : null; +} + +function newSessionName(): string { + const cryptoApi = globalThis.crypto as Crypto | undefined; + if (typeof cryptoApi?.randomUUID === "function") + return cryptoApi.randomUUID(); + return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`; +} + +/** Acquire the exclusive lock *name* and hold it until the page or worker + * ends (the browser then releases it). Resolves once the lock is held. */ +function holdLock(locks: LockManagerLike, name: string): Promise { + return new Promise((acquired, failed) => { + locks + .request(name, { mode: "exclusive" }, () => { + acquired(); + return new Promise(() => {}); + }) + .catch(failed); + }); +} + +/** Remove the session directories of *parent* whose lock is free, i.e. whose + * session has ended (all but *own*). Failures are ignored: they only delay + * the reclaim of disk space. */ +async function removeAbandonedSessions( + parent: FileSystemDirectoryHandle, + own: string, + locks: LockManagerLike, +): Promise { + const names: string[] = []; + try { + for await (const entry of (parent as ListableDirectory).values()) { + if (entry.kind === "directory" && entry.name !== own) { + names.push(entry.name); + } + } + } catch { + return; + } + for (const name of names) { + try { + await locks.request( + SESSION_LOCK_PREFIX + name, + { mode: "exclusive", ifAvailable: true }, + async (lock) => { + if (lock === null) return; // The session is running + await parent.removeEntry(name, { recursive: true }); + }, + ); + } catch { + // Ignored (see above) + } + } +} + +/** + * Create the directory of a new session under *root*, and remove those of + * the sessions that have ended. + * + * The lock of the new session is acquired before its directory is created, + * so that another session never takes it for an abandoned one. + */ +export async function openSessionDirectory( + root: FileSystemDirectoryHandle, +): Promise { + const parent = await root.getDirectoryHandle(SESSIONS_DIR, { create: true }); + const name = newSessionName(); + const locks = lockManager(); + if (locks) { + await holdLock(locks, SESSION_LOCK_PREFIX + name); + await removeAbandonedSessions(parent, name, locks); + } + const dir = await parent.getDirectoryHandle(name, { create: true }); + return { parent, name, dir }; +} + +/** Empty the directory of *session* and return its new handle. */ +export async function resetSessionDirectory( + session: SessionDirectory, +): Promise { + try { + await session.parent.removeEntry(session.name, { recursive: true }); + } catch { + // Directory may not exist anymore. + } + session.dir = await session.parent.getDirectoryHandle(session.name, { + create: true, + }); + return session.dir; +} diff --git a/src/storage/opfsSyncObjectStore.ts b/src/storage/opfsSyncObjectStore.ts index a94bcbd..603469a 100644 --- a/src/storage/opfsSyncObjectStore.ts +++ b/src/storage/opfsSyncObjectStore.ts @@ -15,10 +15,15 @@ * spill/page-in machinery in ``runtime.ts`` is backend-agnostic. * * Layout is identical to the async store (a *directory of objects*: one - * file per object id under {@link STORE_DIR}), so the two are interchangeable - * and even read each other's files. + * file per object id in the directory of the store's session, see + * :mod:`opfsSession`), so the two are interchangeable. */ -import { STORE_DIR, fileName, type ObjectByteStore } from "./opfsObjectStore"; +import { fileName, type ObjectByteStore } from "./opfsObjectStore"; +import { + openSessionDirectory, + resetSessionDirectory, + type SessionDirectory, +} from "./opfsSession"; /** Minimal shape of the synchronous access handle (Worker-only API). The * read/write buffers are narrowed to ``Uint8Array`` (all we pass) to avoid @@ -43,7 +48,7 @@ type SyncCapableFileHandle = FileSystemFileHandle & { * Worker-only. Call {@link init} once before use (idempotent). */ export class OpfsSyncObjectStore implements ObjectByteStore { - private root: FileSystemDirectoryHandle | null = null; + private session: SessionDirectory | null = null; private dir: FileSystemDirectoryHandle | null = null; private readonly sizes = new Map(); private initialised = false; @@ -72,8 +77,10 @@ export class OpfsSyncObjectStore implements ObjectByteStore { if (!OpfsSyncObjectStore.isSupported()) { throw new Error("synchronous OPFS access handles are unavailable here"); } - this.root = await navigator.storage.getDirectory(); - this.dir = await this.root.getDirectoryHandle(STORE_DIR, { create: true }); + this.session = await openSessionDirectory( + await navigator.storage.getDirectory(), + ); + this.dir = this.session.dir; this.initialised = true; } @@ -142,16 +149,11 @@ export class OpfsSyncObjectStore implements ObjectByteStore { this.sizes.delete(oid); } - /** Remove every stored payload and reset the directory. */ + /** Remove every payload of this store (and only those). */ async clear(): Promise { - if (!this.root) return; - try { - await this.root.removeEntry(STORE_DIR, { recursive: true }); - } catch { - // Directory may not exist yet. - } + if (!this.session) return; this.sizes.clear(); - this.dir = await this.root.getDirectoryHandle(STORE_DIR, { create: true }); + this.dir = await resetSessionDirectory(this.session); } /** Total on-disk byte size of all payloads written through this instance. */ diff --git a/src/storage/pluginWheelStore.ts b/src/storage/pluginWheelStore.ts index 405e770..7eff738 100644 --- a/src/storage/pluginWheelStore.ts +++ b/src/storage/pluginWheelStore.ts @@ -87,8 +87,9 @@ function parseIndex(text: string): PluginWheelIndex | null { /** * OPFS owner for user plugin wheel bytes and their committed metadata. * - * The directory is deliberately distinct from `dlw-object-store`, whose - * workspace clear operation must never remove installed plugins. + * The directory is deliberately distinct from those of the on-disk object + * stores, whose workspace clear operation must never remove installed + * plugins. */ export class PluginWheelStore { private dir: FileSystemDirectoryHandle | null = null; diff --git a/tests/ts/storage/opfsSession.test.ts b/tests/ts/storage/opfsSession.test.ts new file mode 100644 index 0000000..755939a --- /dev/null +++ b/tests/ts/storage/opfsSession.test.ts @@ -0,0 +1,230 @@ +/** + * Unit tests for the per-session directories of the on-disk object stores + * ({@link openSessionDirectory}), with a fake OPFS tree and a fake Web Locks + * manager: the stores of two sessions never share or clear each other's + * files, and the directories of ended sessions are reclaimed. + */ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { OpfsObjectStore } from "../../../src/storage/opfsObjectStore"; +import { + SESSION_LOCK_PREFIX, + SESSIONS_DIR, + openSessionDirectory, +} from "../../../src/storage/opfsSession"; + +class FakeFile { + bytes = new Uint8Array(0); +} + +class FakeFileHandle { + readonly kind = "file"; + constructor( + readonly name: string, + private readonly file: FakeFile, + ) {} + async createWritable() { + const file = this.file; + let swap: Uint8Array | null = null; + return { + write: async (data: ArrayBuffer) => { + swap = new Uint8Array(data).slice(); + }, + close: async () => { + if (swap) file.bytes = swap; + }, + abort: async () => { + swap = null; + }, + }; + } + async getFile(): Promise { + return new Blob([this.file.bytes.slice()]); + } +} + +class FakeDirHandle { + readonly kind = "directory"; + files = new Map(); + subdirs = new Map(); + constructor(readonly name = "") {} + async getDirectoryHandle( + name: string, + opts?: { create?: boolean }, + ): Promise { + let d = this.subdirs.get(name); + if (!d) { + if (!opts?.create) throw new Error("NotFound"); + d = new FakeDirHandle(name); + this.subdirs.set(name, d); + } + return d; + } + async getFileHandle( + name: string, + opts?: { create?: boolean }, + ): Promise { + let f = this.files.get(name); + if (!f) { + if (!opts?.create) throw new Error("NotFound"); + f = new FakeFile(); + this.files.set(name, f); + } + return new FakeFileHandle(name, f); + } + async removeEntry(name: string): Promise { + if (!this.files.delete(name) && !this.subdirs.delete(name)) { + throw new Error("NotFound"); + } + } + async *values(): AsyncIterable { + for (const dir of this.subdirs.values()) yield dir; + for (const [name, file] of this.files) yield new FakeFileHandle(name, file); + } +} + +/** Fake Web Locks manager: exclusive locks, held while their callback's + * promise is pending. */ +class FakeLockManager { + readonly held = new Set(); + async request( + name: string, + options: { mode?: string; ifAvailable?: boolean }, + callback: (lock: unknown) => unknown, + ): Promise { + if (this.held.has(name)) { + if (options.ifAvailable) return callback(null); + throw new Error("Lock contention is not simulated"); + } + this.held.add(name); + try { + return await callback({ name, mode: options.mode }); + } finally { + this.held.delete(name); + } + } +} + +const ORIGINAL = { + storage: (navigator as { storage?: unknown }).storage, + locks: (navigator as { locks?: unknown }).locks, + secure: globalThis.isSecureContext, +}; + +let opfsRoot: FakeDirHandle; +let locks: FakeLockManager | undefined; + +function setLocks(manager: FakeLockManager | undefined): void { + locks = manager; + Object.defineProperty(navigator, "locks", { + configurable: true, + value: manager, + }); +} + +function sessions(): FakeDirHandle { + const parent = opfsRoot.subdirs.get(SESSIONS_DIR); + expect(parent).toBeDefined(); + return parent as FakeDirHandle; +} + +beforeEach(() => { + opfsRoot = new FakeDirHandle(); + Object.defineProperty(navigator, "storage", { + configurable: true, + value: { getDirectory: async () => opfsRoot }, + }); + Object.defineProperty(globalThis, "isSecureContext", { + configurable: true, + value: true, + }); + setLocks(new FakeLockManager()); +}); + +afterEach(() => { + Object.defineProperty(navigator, "storage", { + configurable: true, + value: ORIGINAL.storage, + }); + Object.defineProperty(navigator, "locks", { + configurable: true, + value: ORIGINAL.locks, + }); + Object.defineProperty(globalThis, "isSecureContext", { + configurable: true, + value: ORIGINAL.secure, + }); +}); + +describe("OPFS session directories", () => { + it("gives each store its own directory, owned by a held lock", async () => { + const first = new OpfsObjectStore(); + const second = new OpfsObjectStore(); + await first.init(); + await second.init(); + + const names = Array.from(sessions().subdirs.keys()); + expect(names).toHaveLength(2); + expect(locks?.held).toEqual( + new Set(names.map((name) => SESSION_LOCK_PREFIX + name)), + ); + }); + + it("never clears the files of another session", async () => { + const first = new OpfsObjectStore(); + const second = new OpfsObjectStore(); + await first.init(); + await second.init(); + await first.put("a", new Uint8Array([1, 2, 3])); + await second.put("b", new Uint8Array([4, 5])); + + // e.g. the second tab switches back to RAM or replaces its workspace + await second.clear(); + + expect(Array.from(await first.get("a"))).toEqual([1, 2, 3]); + expect(await second.has("b")).toBe(false); + await second.put("c", new Uint8Array([6])); + expect(Array.from(await second.get("c"))).toEqual([6]); + }); + + it("reclaims the directories of ended sessions only", async () => { + const parent = await opfsRoot.getDirectoryHandle(SESSIONS_DIR, { + create: true, + }); + const ended = await parent.getDirectoryHandle("ended", { create: true }); + await ended.getFileHandle("x.npy", { create: true }); + await parent.getDirectoryHandle("running", { create: true }); + locks?.held.add(SESSION_LOCK_PREFIX + "running"); + const legacy = await opfsRoot.getDirectoryHandle("dlw-object-store", { + create: true, + }); + await legacy.getFileHandle("old.npy", { create: true }); + + const session = await openSessionDirectory( + opfsRoot as unknown as FileSystemDirectoryHandle, + ); + + expect(new Set(sessions().subdirs.keys())).toEqual( + new Set(["running", session.name]), + ); + // The shared directory of earlier versions is left to them + expect(opfsRoot.subdirs.get("dlw-object-store")?.files.has("old.npy")).toBe( + true, + ); + }); + + it("keeps every directory without the Web Locks API", async () => { + setLocks(undefined); + const parent = await opfsRoot.getDirectoryHandle(SESSIONS_DIR, { + create: true, + }); + await parent.getDirectoryHandle("unknown", { create: true }); + + const store = new OpfsObjectStore(); + await store.init(); + await store.put("a", new Uint8Array([1])); + + expect(sessions().subdirs.size).toBe(2); + expect(sessions().subdirs.has("unknown")).toBe(true); + }); +}); From 3ef194563e989f58e95e87f5abda8920ab459ea2 Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:16:42 +0200 Subject: [PATCH 7/8] fix(sdk): check the origin of the messages exchanged with the iframe The SDK accepted every message whose source was the iframe window, without checking its origin: a window keeps its identity when it navigates, so a document of another origin loaded in the iframe could resolve pending requests or emit events. The bridge, for its part, broadcast its events to the known host windows with the "*" target origin, so that a host window that had navigated to another document still received them. The SDK now also requires the origin to be targetOrigin (any origin with "*", for local development only). The bridge records the origin of the requests of each host window and sends its responses and events to that origin only; opaque origins ("null") are only targeted through "*", which only the "*" wildcard of allowedOrigins accepts. Documented in the SDK README. Assisted-by: Claude Opus 5.5 --- packages/sdk/README.md | 2 + packages/sdk/src/DataLabWebClient.ts | 9 ++++- src/runtime/remoteBridge.ts | 28 ++++++++----- tests/ts/DataLabWebClient.test.ts | 51 +++++++++++++++++++++++ tests/ts/remoteBridge.test.ts | 60 ++++++++++++++++++++++++++++ 5 files changed, 137 insertions(+), 13 deletions(-) diff --git a/packages/sdk/README.md b/packages/sdk/README.md index b05dac4..727e1f9 100644 --- a/packages/sdk/README.md +++ b/packages/sdk/README.md @@ -34,6 +34,8 @@ const data = await client.getSignalXY(ids[0]); The DataLab-Web iframe **must** be loaded with `?allowedOrigins=` in its URL (or `?allowedOrigins=*` for development) so the bridge accepts incoming messages. +`targetOrigin` must be the origin of the iframe document: the client only accepts responses and events coming from the iframe window **and** from that origin, so that another document loaded in the iframe (after a navigation) cannot answer in its place. `targetOrigin: "*"` accepts any origin and is intended for local development only. Likewise, the bridge sends its responses and events to each host window with the origin of its requests. + ## Wire-protocol versioning `client.ready()` negotiates a wire-protocol version with the iframe (semver `MAJOR.MINOR`). It rejects if the iframe reports a MAJOR incompatible with the SDK (`SUPPORTED_PROTOCOL_MAJOR`). diff --git a/packages/sdk/src/DataLabWebClient.ts b/packages/sdk/src/DataLabWebClient.ts index 70d07ef..f1882df 100644 --- a/packages/sdk/src/DataLabWebClient.ts +++ b/packages/sdk/src/DataLabWebClient.ts @@ -24,8 +24,10 @@ export interface DataLabWebClientOptions { /** Origin of the DataLab-Web iframe. Required so we don't leak - * request payloads to the wrong document. Use the iframe's - * ``src`` origin (e.g. ``"https://datalab.example.com"``). */ + * request payloads to the wrong document, nor accept responses and + * events from another document loaded in the iframe. Use the iframe's + * ``src`` origin (e.g. ``"https://datalab.example.com"``); ``"*"`` + * accepts any origin and is intended for local development only. */ targetOrigin: string; /** Default request timeout in ms. ``0`` disables. Defaults to 30 s. */ defaultTimeoutMs?: number; @@ -492,6 +494,9 @@ export class DataLabWebClient { private handleMessage = (event: MessageEvent) => { if (event.source !== this.iframe.contentWindow) return; + // The iframe window keeps its identity when it navigates: also check that + // the message comes from the expected origin (any origin with "*") + if (this.targetOrigin !== "*" && event.origin !== this.targetOrigin) return; const data = event.data as RpcResponse | RpcEvent | null; if (!data || typeof data !== "object") return; if (data.type === "response") { diff --git a/src/runtime/remoteBridge.ts b/src/runtime/remoteBridge.ts index 0f88288..8a7ac64 100644 --- a/src/runtime/remoteBridge.ts +++ b/src/runtime/remoteBridge.ts @@ -132,6 +132,15 @@ function isOriginAllowed(origin: string, allowed: readonly string[]): boolean { return allowed.includes(origin); } +/** Target origin of the messages sent to a peer whose requests come from + * *origin*: that origin, so that a peer window that navigated to another + * document does not receive them, or ``"*"`` for an opaque origin + * (``"null"``, e.g. a sandboxed iframe or a ``file:`` page), which only + * the ``"*"`` wildcard allows. */ +function peerTargetOrigin(origin: string): string { + return origin === "" || origin === "null" ? "*" : origin; +} + /** * Walk an arbitrary value and collect every ``ArrayBuffer`` reachable * through ``TypedArray.buffer``, so binary payloads transfer zero-copy. @@ -163,7 +172,9 @@ export function activateRemoteBridge( const allowedOrigins = parseAllowedOrigins(source); if (allowedOrigins.length === 0) return null; - const peers = new Set(); + // Peer windows, with the origin of their last request: messages are only + // delivered to a peer still showing that origin + const peers = new Map(); const queuedEvents: RpcEvent[] = []; // ``post`` is defined further down; declare a forward reference so @@ -179,8 +190,8 @@ export function activateRemoteBridge( const flushEvents = () => { if (!post || queuedEvents.length === 0 || peers.size === 0) return; const drained = queuedEvents.splice(0); - for (const peer of peers) { - for (const evt of drained) post(peer, "*", evt); + for (const [peer, origin] of peers) { + for (const evt of drained) post(peer, origin, evt); } }; @@ -228,7 +239,7 @@ export function activateRemoteBridge( post = (target: MessageEventSource, origin: string, msg: OutboundMessage) => { const transfer = collectTransferables(msg); - const targetOrigin = origin === "" ? "*" : origin; + const targetOrigin = peerTargetOrigin(origin); try { // ``MessageEventSource`` has a narrower type than ``Window`` but // ``postMessage`` is structurally compatible at runtime. @@ -256,7 +267,7 @@ export function activateRemoteBridge( } const source = event.source; if (!source) return; - peers.add(source); + peers.set(source, event.origin); const fn = bridge[data.method]; let response: RpcResponse; @@ -301,12 +312,7 @@ export function activateRemoteBridge( }, emit(name, payload) { const evt: RpcEvent = { type: "event", name, payload }; - for (const peer of peers) { - // Origin is unknown here (we only know ``event.origin`` per peer), - // so we re-broadcast with ``"*"`` — restricted in practice because - // ``peers`` only contains sources whose origin was whitelisted. - post(peer, "*", evt); - } + for (const [peer, origin] of peers) post(peer, origin, evt); }, }; } diff --git a/tests/ts/DataLabWebClient.test.ts b/tests/ts/DataLabWebClient.test.ts index e52a4a2..6b0687c 100644 --- a/tests/ts/DataLabWebClient.test.ts +++ b/tests/ts/DataLabWebClient.test.ts @@ -140,6 +140,57 @@ describe("DataLabWebClient", () => { expect(cb).toHaveBeenCalledWith({ oid: "x" }); }); + it("ignores messages of the iframe window from another origin", async () => { + // The iframe window keeps its identity when it navigates to another + // document: its messages must also come from the expected origin. + const promise = client.call("a"); + const id = ( + iframe.contentWindow.postMessage.mock.calls[0][0] as { id: number } + ).id; + const cb = vi.fn(); + client.on("model-changed", cb); + for (const data of [ + { id, type: "response", result: "spoofed" }, + { type: "event", name: "model-changed", payload: { panel: "x" } }, + ]) { + window.dispatchEvent( + new MessageEvent("message", { + data, + source: iframe.contentWindow as unknown as MessageEventSource, + origin: "https://different.example", + }), + ); + } + expect(cb).not.toHaveBeenCalled(); + + reply(iframe, { id, type: "response", result: "ok" }); + await expect(promise).resolves.toBe("ok"); + }); + + it("accepts any origin when targetOrigin is the wildcard", async () => { + const anyIframe = makeFakeIframe(); + const anyClient = new DataLabWebClient( + anyIframe as unknown as HTMLIFrameElement, + { targetOrigin: "*" }, + ); + try { + const promise = anyClient.call("a"); + const id = ( + anyIframe.contentWindow.postMessage.mock.calls[0][0] as { id: number } + ).id; + window.dispatchEvent( + new MessageEvent("message", { + data: { id, type: "response", result: "ok" }, + source: anyIframe.contentWindow as unknown as MessageEventSource, + origin: "http://localhost:5173", + }), + ); + await expect(promise).resolves.toBe("ok"); + } finally { + anyClient.dispose(); + } + }); + it("dispose() rejects pending calls and stops listening", async () => { const promise = client.call("never"); client.dispose(); diff --git a/tests/ts/remoteBridge.test.ts b/tests/ts/remoteBridge.test.ts index 84149e2..80eb3ad 100644 --- a/tests/ts/remoteBridge.test.ts +++ b/tests/ts/remoteBridge.test.ts @@ -297,4 +297,64 @@ describe("activateRemoteBridge", () => { win.removeEventListener(REMOTE_MODEL_CHANGED_EVENT, onCustom); handle?.dispose(); }); + + it("sends events to each peer with the origin of its requests", async () => { + const handle = activateRemoteBridge(runtime, { + allowedOriginsSource: + "?allowedOrigins=https://a.example.com,https://b.example.com", + win, + }); + const peerA: MockPeer = { postMessage: vi.fn() }; + const peerB: MockPeer = { postMessage: vi.fn() }; + for (const [peer, origin, id] of [ + [peerA, "https://a.example.com", 1], + [peerB, "https://b.example.com", 2], + ] as const) { + win.dispatchEvent( + fakeMessageEvent({ + origin, + source: peer, + data: { id, type: "request", method: "get_version" }, + }), + ); + } + await new Promise((r) => setTimeout(r, 0)); + + handle?.emit("ready", { ok: true }); + + // Regression: events were posted with the "*" target origin, so a peer + // window that navigated to another document still received them. + for (const [peer, origin] of [ + [peerA, "https://a.example.com"], + [peerB, "https://b.example.com"], + ] as const) { + const targets = peer.postMessage.mock.calls.map((c) => c[1]); + expect(targets).toEqual([origin, origin]); + expect(peer.postMessage.mock.calls[1][0]).toMatchObject({ + type: "event", + name: "ready", + }); + } + handle?.dispose(); + }); + + it("targets opaque origins only through the wildcard", async () => { + const handle = activateRemoteBridge(runtime, { + allowedOriginsSource: "?allowedOrigins=*", + win, + }); + const peer: MockPeer = { postMessage: vi.fn() }; + win.dispatchEvent( + fakeMessageEvent({ + origin: "null", + source: peer, + data: { id: 3, type: "request", method: "get_version" }, + }), + ); + await new Promise((r) => setTimeout(r, 0)); + handle?.emit("ready"); + + expect(peer.postMessage.mock.calls.map((c) => c[1])).toEqual(["*", "*"]); + handle?.dispose(); + }); }); From a6817266cacfec8cf10ee4c21153a025d027e70e Mon Sep 17 00:00:00 2001 From: Pierre Raybaut <1311787+PierreRaybaut@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:17:35 +0200 Subject: [PATCH 8/8] docs: changelog entries for the review fixes and data locality wording Add the user-visible fixes of this branch to the [Unreleased] section (on-disk storage write failures and multi-tab isolation, HDF5 complex and integer import, broken links, link cycles and large files, embedding message origins). Replace the absolute "your data never leaves your machine" claim of the README by a precise statement: processing never requires sending data anywhere, and the network services that can be enabled (AI assistant provider, capsule registry) are named. Assisted-by: Claude Opus 5.5 --- CHANGELOG.md | 11 +++++++++++ README.md | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2078836..6f3b8cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,17 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) - **Security — opened notebooks:** an opened `.ipynb` file can no longer change the look of DataLab-Web, cover it or contact a remote server. Its markdown cells and its HTML and SVG outputs are now displayed in their own isolated block: their styles no longer apply to the rest of the interface, and content positioned over the whole window stays inside the block, so it cannot hide commands or imitate a DataLab-Web dialog. Form fields are removed, and nothing is loaded from the network (remote images, style sheets, fonts or backgrounds), which prevents a notebook from tracking its opening. Images embedded in the notebook and the tables and styling of Sigima results are displayed as before. - **Unsaved changes:** several edits did not mark the workspace as modified, so closing or reloading the page discarded them without warning: editing signal or image values in the _Data_ array editor, duplicating objects, moving an object up or down, pasting or importing a region of interest, changing the resampling method of an image, plotting analysis results, committing an interactive fit, importing nodes from an HDF5 file and running a plugin menu action. They now mark the workspace as modified, like the other edits. +- **On-disk storage — failed writes:** when the browser refused to write an object to disk (storage quota exceeded, write error), the object lost its data: it had already been removed from memory. The object now stays in memory with its data, and the error is reported. An interrupted write no longer replaces the previous on-disk copy either. +- **On-disk storage — several tabs:** with DataLab-Web open in two tabs in on-disk storage mode, switching one tab back to memory storage or replacing its workspace deleted the data stored on disk by the other tab, whose objects could then no longer be displayed or processed. Each tab now keeps its data in its own storage area; the areas of closed tabs are reclaimed when the on-disk mode is next enabled. +- **HDF5 import — complex and integer data:** single-precision complex signals (`complex64`) lost their imaginary part, and unsigned 32-bit images were clipped to 2 147 483 647. Complex data is now imported as complex objects and integer data keeps its values exactly (images with large values become 64-bit floating-point images); a warning now reports integers beyond 2⁵³, which cannot be represented exactly. Previews show the real part of complex data. +- **HDF5 browser — broken links:** files containing a link to a missing object or to a missing file (e.g. synchrotron master files without their detector files) could not be opened at all. Broken links are now skipped and the rest of the file can be browsed and imported. +- **HDF5 browser — link cycles:** files containing a group linked into itself or into one of its subgroups were browsed down to a hundred phantom levels, or took too long to open. Such links back to a parent group are now skipped; other aliases are still shown at each of their paths. +- **HDF5 browser — large files:** opening an HDF5 file no longer reads every dataset entirely, including those that cannot be imported (e.g. 3D cubes): datasets are described from a small sample, and their data is read only when previewed or imported. Large files open faster and with much less memory. +- **Embedding — message origins:** the JavaScript SDK accepted responses and events from any document loaded in the DataLab-Web iframe, and DataLab-Web sent its events to host windows whatever document they showed. The SDK now only accepts messages from the configured `targetOrigin`, and DataLab-Web sends its responses and events to the origin from which each host window made its requests. + +### Changed + +- **Data locality:** the README now states precisely what stays local: processing never requires sending data anywhere, and the network services that can be enabled (AI assistant provider, capsule registry) are named. ## [0.10.0] - 2026-10-20 diff --git a/README.md b/README.md index c09e902..e39edba 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ DataLab Web embeds the [Sigima](https://github.com/DataLab-Platform/Sigima) comp **👉 ** -The latest release is deployed automatically to GitHub Pages. Open the link in any modern browser (Chrome, Edge, Firefox, Safari) and the full DataLab application — Sigima, NumPy, SciPy, scikit-image, h5py — runs locally inside the browser tab. No server, no account, no upload: your data never leaves your machine. +The latest release is deployed automatically to GitHub Pages. Open the link in any modern browser (Chrome, Edge, Firefox, Safari) and the full DataLab application — Sigima, NumPy, SciPy, scikit-image, h5py — runs locally inside the browser tab. No server, no account, no upload: processing your data never requires sending it anywhere. Network services are used only when you enable them: the AI assistant sends your requests (and the context you share) to the provider you configure, and capsules and their reviews are sent to a capsule registry only when you configure one and publish them. > First load downloads Pyodide and installs Sigima via `micropip` (~30–60 s). Subsequent loads are cached by the browser. On Microsoft Edge the first load may be much slower — see [doc/troubleshooting.md](doc/troubleshooting.md) for the one-line fix.