From 232fd2a8d15f63b6bf6ad26286dd9836d6819668 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 21:43:00 +0200 Subject: fix(node): the MNP root path never reloaded, and my first repair made it worse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit was the wrong fix. `add_root` did leave the running node unchanged, but editing the live `RootSet` in place — which is what I did — is wrong in the other direction. `DirectoryIndexer.retarget` decides what to scan by diffing the names it already holds against the ones it is handed, and `_retarget_indexer` hands it `groups_ctx[gid]["roots"]`: the very object the op had edited. So the new root sat on both sides of the comparison, nothing was scanned, and the directory would have appeared in the table permanently empty. `_reload_config_inner` diffs the same way and would have concluded nothing changed. `remove_root` had the same shape and would have kept serving a removed directory's files. The real defect is that two front doors did different things. `ui/app.py` has always fired the daemon's `reload_fn` after these ops, which re-reads node.toml and builds a *fresh* set; the MNP path retargeted a stale object instead. That asymmetry is exactly what `ops.py` exists to prevent, and it is why the bug survived until an operator added a directory from a browser — the loopback path worked all along. So: the ops leave the live set alone, `_retarget_indexer` asks the daemon to reload, and `update_root` keeps editing in place because flags change no files and the synchronous upload handler reads that object on the next request. The tests now check the files rather than `describe()`, which proves nothing about whether anything was scanned. One of them demonstrates the failure mode instead of describing it, so the rule is checkable and will say so if `retarget` ever changes. Two more cover the seam itself — that the MNP path reloads, and that a context with no daemon still retargets. Diagnosed by reading the running node's journal rather than the source: the first add logged "Reloading config" and a rescan, the two later ones logged neither. I should have looked there before the first attempt. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- packages/meshbay-node/src/meshbay_node/ops.py | 50 ++++++++-------------- .../src/meshbay_node/transport/webrtc_server.py | 28 +++++++++++- 2 files changed, 45 insertions(+), 33 deletions(-) (limited to 'packages/meshbay-node/src') 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: -- cgit v1.2.3