diff options
Diffstat (limited to 'packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py')
| -rw-r--r-- | packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py | 295 |
1 files changed, 232 insertions, 63 deletions
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() |