diff options
3 files changed, 277 insertions, 96 deletions
diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 10f6a8f..5b10452 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -682,30 +682,21 @@ async def add_root(state: dict, group_id: str, path: str, *, writable=added.writable, removable=added.removable, direct=added.direct)) - # The *live* set, not only the config — the same thing `remove_root` and - # `update_root` do, and the one this was missing. + # Deliberately *not* mutating the live RootSet in place. # - # `_retarget_indexer` (the MNP path) re-points the indexer at - # `groups_ctx[gid]["roots"]`, so leaving that object untouched retargeted - # it at exactly what it already had: node.toml gained the directory, the - # index went on reporting the old set, and the next `index_sync` overwrote - # whatever the ack had just told the client. The root was there, invisible, - # until a restart — and adding it again was refused as a duplicate of - # itself, which is the only reason anyone found out. + # `DirectoryIndexer.retarget` decides what to scan by diffing the names it + # already has against the ones it is given — so handing it the same object, + # edited, means the new root is in both sides of the comparison and is + # never scanned. It would appear in the table and stay permanently empty. + # `_reload_config_inner` diffs the same way and would likewise conclude + # nothing changed. The caller reloads instead, which builds a fresh set + # from the file this just wrote. # - # Safe to append rather than rebuild: `RootSet.build(specs)` above already - # validated the whole set, this root included, for name collisions and - # nesting. - live_roots: RootSet | None = state.get("groups_ctx", {}).get( - group_id, {}).get("roots") - if live_roots is not None and not any( - r.folded == added.folded for r in live_roots.roots): - live_roots.roots.append(added) - + # `built` is that set, computed here only to validate and to answer with; + # what the node serves comes from the reload. log.info("Root added: %s → group %s", added.name, group_id[:8]) return {"status": "added", "name": added.name, "path": str(added.path), - "group_id": group_id, - "roots": (live_roots or built).describe()} + "group_id": group_id, "roots": built.describe()} async def remove_root(state: dict, group_id: str, root_name: str) -> dict: @@ -741,20 +732,15 @@ async def remove_root(state: dict, group_id: str, root_name: str) -> dict: cfg.roots.pop(match_idx) - # Update the live RootSet so GET /api/groups returns correct data - # immediately, without waiting for the async reload. - live_roots = state.get("groups_ctx", {}).get( - group_id, {}).get("roots") - if live_roots: - live_roots.roots = [ - r for r in live_roots.roots if fold(r.name) != target] - - # Built from config when there is no live set, never returned empty: an + # Not mutating the live set here either — see `add_root`. Dropping the + # root from it would leave `retarget` unable to tell that its entries + # should go, so the removed directory's files would stay in the index. + # + # Built from the config this just edited, and never returned empty: an # empty list is a *valid answer* meaning "this group has no directories", - # and the client cannot tell it from "the node could not say". It would + # which the client cannot tell from "the node could not say" — it would # blank the operator's table on an op that succeeded. - result_roots = (live_roots.describe() if live_roots - else RootSet.build([asdict(r) for r in cfg.roots]).describe()) + result_roots = RootSet.build([asdict(r) for r in cfg.roots]).describe() log.info("Root removed: %s from group %s", root_name, group_id[:8]) return {"status": "removed", "name": root_name, "group_id": group_id, diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index c4d053e..d341d8c 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -2773,10 +2773,36 @@ class WebRTCPeerSession: return await fn(state, *args, **kwargs) async def _retarget_indexer(self, group_id: str) -> None: - """Tell the indexer to rescan after roots changed.""" + """ + Pick up a root that was just added to or removed from node.toml. + + Through the daemon's own reload, which is what the loopback API has + always done after the same operations (`ui/app.py`). This used to + re-point the indexer at `groups_ctx[gid]["roots"]` instead — the very + object the op had just edited — so `retarget` diffed a set against + itself, found no new names, scanned nothing, and dropped nothing. A + directory added over MNP reached node.toml and was invisible until a + restart; one removed kept serving its files. + + Two front doors doing different things is the shape `ops.py` exists to + prevent, and this was it: the loopback path worked and the MNP path did + not, which is why it survived until the operator added a directory from + a browser. + + Not awaited: a reload rescans, and a new library is minutes. The ack + the caller sends carries the set the node is moving to, and the + `index_sync` that follows the scan carries what it found. + """ state = self._ctx.get("daemon_state") if not state: return + reload_fn = state.get("reload_fn") + if reload_fn: + self._spawn(reload_fn()) + return + # No daemon to ask — a test harness, or a context assembled by hand. + # Retarget directly, which is correct as long as the caller did not + # edit the live set in place. indexer = state.get("indexers", {}).get(group_id) roots = state.get("groups_ctx", {}).get(group_id, {}).get("roots") if indexer and roots: diff --git a/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py b/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py index d45a5b2..976af82 100644 --- a/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py +++ b/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py @@ -1,24 +1,25 @@ """ -A root operation changes what the node is *serving*, not only what it will -serve after a restart. +Adding or removing a root has to reach the running node, not only node.toml. -Every root op writes two places: `node.toml`, which survives a restart, and the -live `RootSet` in `groups_ctx[gid]["roots"]`, which is what the running node -answers from. The index payload is built from the second, so an op that updates -only the first is invisible until the daemon is restarted — and worse than -invisible, because the ack it sends *does* carry the change, so the client shows -it for a moment and the next `index_sync` takes it away again. +Two front doors do this — the loopback API and a signed MNP op — and `ops.py` +exists so they behave identically. They did not. The loopback path fired the +daemon's `reload_fn`, which re-reads node.toml and builds a fresh `RootSet`; +the MNP path instead re-pointed the indexer at `groups_ctx[gid]["roots"]`, the +very object the op had just been asked about. `DirectoryIndexer.retarget` +decides what to scan by diffing the names it holds against the ones it is +given, so a set compared against itself scans nothing and drops nothing. -`add_root` was like that. It appended to the config and to node.toml, and -`_retarget_indexer` then re-pointed the indexer at a `RootSet` object nobody had -touched — retargeting it at exactly what it already had. Found by an operator -adding a directory, seeing nothing, and being told on the second attempt that -its name collided with itself. +A directory added from a browser therefore reached node.toml and was invisible +everywhere else until a restart — and adding it again was refused as colliding +with itself, which is the only reason anyone found out. One removed would have +kept serving its files. -The loopback API hid it: `ui/app.py` fires `reload_fn()` after the op, which -re-reads node.toml from disk. The MNP path does not, and the shared-directories -table started offering Add over MNP in this refactor — a latent bug made -reachable. +**The obvious repair is wrong in the other direction**, and was committed once +before this file said so: making the op edit the live set in place puts the new +root on *both* sides of retarget's comparison. The table would show it and it +would stay permanently empty. So the ops leave that object alone, the MNP path +reloads like the loopback one always did, and the tests below check the files — +`describe()` agreeing proves nothing about whether anything was scanned. """ from dataclasses import asdict @@ -31,6 +32,7 @@ from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey from meshbay_node import ops from meshbay_node.config import GroupConfig, NodeConfig, RootSpec from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.indexer.indexer import DirectoryIndexer from meshbay_node.roots import RootSet from meshbay_node.roster import Roster @@ -77,50 +79,159 @@ def _live(state) -> RootSet: return state["groups_ctx"][GROUP]["roots"] -# ── Adding ─────────────────────────────────────────────────────────────────── +async def _indexer(state) -> DirectoryIndexer: + idx = DirectoryIndexer(roots=_live(state), group_id=GROUP, + sk_node=Ed25519PrivateKey.generate(), gek=None) + await idx.initial_scan() + return idx -async def test_adding_a_root_reaches_the_running_node(tmp_path): + +def _rebuilt(state) -> RootSet: + """What a reload produces: a fresh set from the config the op just wrote.""" + return RootSet.build([asdict(r) for r in state["config"].groups[0].roots]) + + +# ── What the op writes ─────────────────────────────────────────────────────── + +async def test_adding_a_root_reaches_node_toml_and_the_ack(tmp_path): state, roster = await _state(tmp_path) (tmp_path / "uploads").mkdir() try: result = await ops.add_root(state, GROUP, str(tmp_path / "uploads")) - assert [r.name for r in _live(state)] == ["one", "two", "uploads"], ( - "the live root set did not learn about the new directory, so the " - "index will keep reporting the old one until a restart") assert [r["name"] for r in result["roots"]] == ["one", "two", "uploads"] + assert [r.name for r in state["config"].groups[0].roots] == [ + "one", "two", "uploads"] + assert "uploads" in Path(state["config_path"]).read_text() + finally: + await roster.close() + + +async def test_removing_a_root_reaches_node_toml_and_the_ack(tmp_path): + state, roster = await _state(tmp_path) + try: + result = await ops.remove_root(state, GROUP, "two") + assert [r["name"] for r in result["roots"]] == ["one"] + assert Path(state["config_path"]).read_text().count( + "[[groups.roots]]") == 1 finally: await roster.close() -async def test_the_ack_describes_the_set_the_node_will_serve(tmp_path): +async def test_the_op_does_not_edit_the_live_set_in_place(tmp_path): """ - Not a set built on the side. Describing something the node is not actually - using is how the client shows a directory for one paint and loses it on the - next index push — which reads as a UI bug and is not one. + The property that made the original bug, and then made the first repair for + it wrong in the other direction. + + `retarget` diffs the names it holds against the ones it is handed. Editing + that same object and passing it back puts a new root on both sides of the + comparison: nothing is scanned, and the directory shows in the table + permanently empty. `_reload_config_inner` diffs the same way and would + likewise conclude nothing had changed. """ state, roster = await _state(tmp_path) + before = [r.name for r in _live(state)] (tmp_path / "uploads").mkdir() try: - result = await ops.add_root(state, GROUP, str(tmp_path / "uploads")) - assert result["roots"] == _live(state).describe() + await ops.add_root(state, GROUP, str(tmp_path / "uploads")) + assert [r.name for r in _live(state)] == before, ( + "add_root edited the live RootSet, which is the object retarget " + "diffs against — the new root would never be scanned") + + await ops.remove_root(state, GROUP, "two") + assert [r.name for r in _live(state)] == before, ( + "remove_root edited the live RootSet, so retarget cannot tell the " + "removed root's entries should go") finally: await roster.close() -async def test_adding_the_same_directory_twice_is_still_refused(tmp_path): +# ── What the node then serves ──────────────────────────────────────────────── + +async def test_a_retarget_from_the_config_scans_the_new_root(tmp_path): + """ + The half no assertion about `describe()` can reach: the files. + + A root that appears in the table and holds nothing is the same bug one step + later, and it is what editing the live set in place would produce. + """ + state, roster = await _state(tmp_path) + (tmp_path / "one" / "kept.txt").write_bytes(b"kept") + fresh = tmp_path / "uploads" + fresh.mkdir() + (fresh / "new.txt").write_bytes(b"new") + + idx = await _indexer(state) + assert {e.name for e in idx.index.entries} == {"kept.txt"} + try: + await ops.add_root(state, GROUP, str(fresh)) + await idx.retarget(_rebuilt(state)) + + assert {e.name for e in idx.index.entries} == {"kept.txt", "new.txt"}, ( + "the added directory was not scanned — it would show in the table " + "and stay empty") + assert [r["name"] for r in idx.index.roots] == ["one", "two", "uploads"] + finally: + await roster.close() + + +async def test_handing_retarget_the_edited_set_scans_nothing(tmp_path): """ - The counter-property. The live set gaining the root must not make the - collision check pass the second time — a group with the same path under two - names indexes every file in it twice. + The failure mode above, demonstrated rather than described — so the reason + the ops leave the live set alone is checkable instead of asserted in a + comment. If this ever starts failing, `retarget` has changed and the rule + in `add_root` can be revisited. """ state, roster = await _state(tmp_path) + fresh = tmp_path / "uploads" + fresh.mkdir() + (fresh / "new.txt").write_bytes(b"new") + + idx = await _indexer(state) + try: + await ops.add_root(state, GROUP, str(fresh)) + # What editing in place would have left behind. + _live(state).roots.append(_rebuilt(state).roots[-1]) + await idx.retarget(_live(state)) + + assert {e.name for e in idx.index.entries} == set(), ( + "retarget now scans a root it was handed on both sides of its own " + "diff — the constraint this file is built on has changed") + finally: + await roster.close() + + +async def test_a_retarget_from_the_config_drops_a_removed_root(tmp_path): + """The mirror: a removed directory's files must stop being served.""" + state, roster = await _state(tmp_path) + (tmp_path / "one" / "kept.txt").write_bytes(b"kept") + (tmp_path / "two" / "going.txt").write_bytes(b"going") + + idx = await _indexer(state) + assert {e.name for e in idx.index.entries} == {"kept.txt", "going.txt"} + try: + await ops.remove_root(state, GROUP, "two") + await idx.retarget(_rebuilt(state)) + assert {e.name for e in idx.index.entries} == {"kept.txt"}, ( + "the removed directory's files are still being served") + finally: + await roster.close() + + +# ── The invariants around them ─────────────────────────────────────────────── + +async def test_adding_the_same_directory_twice_is_still_refused(tmp_path): + """A group with one path under two names indexes every file in it twice.""" + state, roster = await _state(tmp_path) (tmp_path / "uploads").mkdir() try: await ops.add_root(state, GROUP, str(tmp_path / "uploads")) with pytest.raises(ops.OpError): await ops.add_root(state, GROUP, str(tmp_path / "uploads")) - assert len(_live(state)) == 3, "the refused add left something behind" + assert len(state["config"].groups[0].roots) == 3, ( + "the refused add left something behind") + assert Path(state["config_path"]).read_text().count( + "[[groups.roots]]") == 3 finally: await roster.close() @@ -131,28 +242,22 @@ async def test_a_second_different_root_still_lands(tmp_path): (tmp_path / "incoming").mkdir() try: await ops.add_root(state, GROUP, str(tmp_path / "uploads")) - await ops.add_root(state, GROUP, str(tmp_path / "incoming"), - writable=True) - assert [r.name for r in _live(state)] == [ + result = await ops.add_root(state, GROUP, str(tmp_path / "incoming"), + writable=True) + assert [r["name"] for r in result["roots"]] == [ "one", "two", "uploads", "incoming"] - assert _live(state).by_name("incoming").writable is True + assert result["roots"][-1]["writable"] is True finally: await roster.close() -# ── The other two, which already did this ──────────────────────────────────── - -async def test_removing_a_root_reaches_the_running_node(tmp_path): - state, roster = await _state(tmp_path) - try: - result = await ops.remove_root(state, GROUP, "two") - assert [r.name for r in _live(state)] == ["one"] - assert result["roots"] == _live(state).describe() - finally: - await roster.close() - - -async def test_updating_a_root_reaches_the_running_node(tmp_path): +async def test_updating_flags_may_edit_the_live_set(tmp_path): + """ + The exception, and why it is one: `writable` and `removable` change nothing + about which files exist, so there is nothing for retarget to scan or drop. + Editing in place is what makes the flag true for the upload handler on the + very next request, which is synchronous and reads the live set. + """ state, roster = await _state(tmp_path) state["config"] = SimpleNamespace(groups=state["config"].groups) try: @@ -165,13 +270,11 @@ async def test_updating_a_root_reaches_the_running_node(tmp_path): await roster.close() -# ── And node.toml, so a restart agrees with the running node ───────────────── - -async def test_the_config_file_and_the_live_set_say_the_same_thing(tmp_path): +async def test_the_file_on_disk_and_the_config_in_memory_agree(tmp_path): """ - The two halves must not drift: what the node serves now and what it will - serve after a restart are the same answer, or the operator's next restart - silently undoes their last change. + A reload re-reads the file, so a config edited in memory but not on disk is + undone by the next restart — and one written to disk but not in memory + makes the *next* op validate against a stale picture. """ state, roster = await _state(tmp_path) (tmp_path / "uploads").mkdir() @@ -180,13 +283,79 @@ async def test_the_config_file_and_the_live_set_say_the_same_thing(tmp_path): writable=True) await ops.remove_root(state, GROUP, "one") - from_disk = RootSet.build([ - asdict(r) for r in state["config"].groups[0].roots]) - assert ([r.name for r in from_disk] - == [r.name for r in _live(state)]) + import tomllib + on_disk = tomllib.loads(Path(state["config_path"]).read_text()) + disk_paths = [str(r["path"]) for r in on_disk["groups"][0]["roots"]] + memory_paths = [Path(r.path).as_posix() + for r in state["config"].groups[0].roots] + assert disk_paths == memory_paths + assert Path(state["config_path"]).read_text().count( + "[[groups.roots]]") == 2 + finally: + await roster.close() + + +# ── The seam that was actually broken ──────────────────────────────────────── - text = Path(state["config_path"]).read_text() - assert text.count("[[groups.roots]]") == 2 - assert "uploads" in text +async def test_the_mnp_path_reloads_like_the_loopback_one(tmp_path): + """ + The two front doors, doing the same thing. + + `ui/app.py` has always fired the daemon's `reload_fn` after a root op. + `_retarget_indexer` did not — it re-pointed the indexer at the live set + instead, which is the object the ops leave alone, so nothing happened at + all. That divergence *is* the bug: the loopback path worked, the MNP path + did not, and it survived until an operator added a directory from a + browser. + + Not awaited: a reload rescans, and a new library is minutes. The ack + already carries the set the node is moving to. + """ + from meshbay_node.transport.webrtc_server import WebRTCPeerSession + + state, roster = await _state(tmp_path) + reloaded: list[bool] = [] + + async def fake_reload(): + reloaded.append(True) + + state["reload_fn"] = fake_reload + spawned = [] + + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = {"daemon_state": state} + session._spawn = lambda coro: spawned.append(coro) + try: + await session._retarget_indexer(GROUP) + assert spawned, "the MNP path did not ask the daemon to reload" + await spawned[0] + assert reloaded == [True] + finally: + await roster.close() + + +async def test_without_a_daemon_it_still_retargets(tmp_path): + """ + A context assembled by hand — a harness, or a test — has no `reload_fn`. + Falling through to a direct retarget keeps those working, and is correct + precisely because the ops no longer edit the set being passed. + """ + from meshbay_node.transport.webrtc_server import WebRTCPeerSession + + state, roster = await _state(tmp_path) + fresh = tmp_path / "uploads" + fresh.mkdir() + (fresh / "new.txt").write_bytes(b"new") + idx = await _indexer(state) + state["indexers"] = {GROUP: idx} + + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = {"daemon_state": state} + try: + await ops.add_root(state, GROUP, str(fresh)) + # What a reload would have installed, done by hand here. + state["groups_ctx"][GROUP]["roots"] = _rebuilt(state) + await session._retarget_indexer(GROUP) + assert {e.name for e in idx.index.entries} == {"new.txt"} finally: await roster.close() |