diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-09-06 21:25:23 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-09-06 21:25:23 +0200 |
| commit | 4c4e9ba7a17e058dc12cb10171743329201dd7e6 (patch) | |
| tree | 721d8a0c81fe56b89cf03f19db84c1b01d0258e4 /packages | |
| parent | 37225762109ad8d11e073f8661359860d678f84a (diff) | |
| download | meshbay-4c4e9ba7a17e058dc12cb10171743329201dd7e6.tar.gz | |
fix(node): adding a root reached node.toml but not the running node
`add_root` appended to the config and to node.toml and stopped there.
`_retarget_indexer` — the MNP path — then re-points the indexer at
`groups_ctx[gid]["roots"]`, an object nobody had touched, so it was retargeted
at exactly what it already had. The directory was in the config file and
invisible everywhere else until a restart.
Worse than invisible: the ack does carry the new set, so the client showed the
directory for one paint and the next index_sync took it away again — which
reads as a UI bug and is not one. Adding it a second time was then refused as
colliding with itself, which is the only reason anyone found out.
`remove_root` and `update_root` already updated the live set; this one was
missed. The loopback API hid it, because `ui/app.py` fires `reload_fn()` after
the op and that re-reads node.toml from disk. The MNP path does not, and the
shared-directories table only started offering Add over MNP in this refactor —
a latent bug made reachable.
The ack now describes the set the node will actually serve rather than one
built on the side, so the two cannot disagree.
test_root_ops_reach_the_live_set.py holds all three ops to it, including the
counter-property that the same directory is still refused twice and that
node.toml and the live set stay in step — the two halves drifting is how an
operator's next restart silently undoes their last change. Four of its seven
fail against the code above.
Recovery on a node already in this state is `meshbay-node reload`: node.toml
has everything, nothing was lost.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us
Diffstat (limited to 'packages')
| -rw-r--r-- | packages/meshbay-node/src/meshbay_node/ops.py | 23 | ||||
| -rw-r--r-- | packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py | 192 |
2 files changed, 214 insertions, 1 deletions
diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 11694c3..10f6a8f 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -682,9 +682,30 @@ 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. + # + # `_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. + # + # 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) + 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": built.describe()} + "group_id": group_id, + "roots": (live_roots or built).describe()} async def remove_root(state: dict, group_id: str, root_name: str) -> dict: 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 new file mode 100644 index 0000000..d45a5b2 --- /dev/null +++ b/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py @@ -0,0 +1,192 @@ +""" +A root operation changes what the node is *serving*, not only what it will +serve after a restart. + +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. + +`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. + +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. +""" + +from dataclasses import asdict +from pathlib import Path +from types import SimpleNamespace + +import pytest +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.roots import RootSet +from meshbay_node.roster import Roster + +pytestmark = pytest.mark.asyncio + +GROUP = "g" * 32 + + +async def _state(tmp_path: Path) -> tuple[dict, Roster]: + """A node hosting one group with two roots, as node.toml and as live state.""" + for name in ("one", "two"): + (tmp_path / name).mkdir() + + cfg = GroupConfig(id=GROUP, name="plop", roots=[ + RootSpec(path=str(tmp_path / "one"), name="one"), + RootSpec(path=str(tmp_path / "two"), name="two"), + ]) + node_cfg = NodeConfig.__new__(NodeConfig) + node_cfg.groups = [cfg] + + conf = tmp_path / "node.toml" + conf.write_text( + f'[[groups]]\nid = "{GROUP}"\nname = "plop"\n\n' + f' [[groups.roots]]\n path = "{(tmp_path / "one").as_posix()}"\n' + f' name = "one"\n\n' + f' [[groups.roots]]\n path = "{(tmp_path / "two").as_posix()}"\n' + f' name = "two"\n') + + live = RootSet.build([asdict(r) for r in cfg.roots]) + index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + state = { + "config": node_cfg, + "config_path": str(conf), + "groups_ctx": {GROUP: {"index": index, "roots": live}}, + "roster": roster, + "node_user_id": "operator", + } + return state, roster + + +def _live(state) -> RootSet: + return state["groups_ctx"][GROUP]["roots"] + + +# ── Adding ─────────────────────────────────────────────────────────────────── + +async def test_adding_a_root_reaches_the_running_node(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"] + finally: + await roster.close() + + +async def test_the_ack_describes_the_set_the_node_will_serve(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. + """ + state, roster = await _state(tmp_path) + (tmp_path / "uploads").mkdir() + try: + result = await ops.add_root(state, GROUP, str(tmp_path / "uploads")) + assert result["roots"] == _live(state).describe() + finally: + await roster.close() + + +async def test_adding_the_same_directory_twice_is_still_refused(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. + """ + 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" + finally: + await roster.close() + + +async def test_a_second_different_root_still_lands(tmp_path): + state, roster = await _state(tmp_path) + (tmp_path / "uploads").mkdir() + (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)] == [ + "one", "two", "uploads", "incoming"] + assert _live(state).by_name("incoming").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): + state, roster = await _state(tmp_path) + state["config"] = SimpleNamespace(groups=state["config"].groups) + try: + result = await ops.update_root(state, GROUP, "two", + writable=True, removable=True) + live = _live(state).by_name("two") + assert live.writable is True and live.removable is True + assert result["roots"] == _live(state).describe() + finally: + 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): + """ + 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. + """ + state, roster = await _state(tmp_path) + (tmp_path / "uploads").mkdir() + try: + await ops.add_root(state, GROUP, str(tmp_path / "uploads"), + 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)]) + + text = Path(state["config_path"]).read_text() + assert text.count("[[groups.roots]]") == 2 + assert "uploads" in text + finally: + await roster.close() |