summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorChristophe Besson <cbesson@gmail.com>2026-09-02 17:31:50 +0200
committerChristophe Besson <cbesson@gmail.com>2026-09-02 17:31:50 +0200
commit7b25f1c09ba1b8692988d9616f3c33c97af9f3ca (patch)
tree1a7d346ea6a3847269df4aeff3c6b41d515b5d7b
parent10f8266e7152d7dc38dbfe2449327829bf020ad1 (diff)
downloadmeshbay-7b25f1c09ba1b8692988d9616f3c33c97af9f3ca.tar.gz
fix(hub): stop the maintenance loop racing the tests, and pin _ASSETS
Two defects found while closing out the Search merge, neither of them in that feature. The maintenance loop. create_app's lifespan starts cleanup_loop as an asyncio task, so every test — each entering that lifespan — ran a purge pass concurrently with its own requests. On SQLite :memory: that is not merely noisy: the engine uses a StaticPool, one connection for the whole process, so the request's session and the cleanup task's session interleave transactions on the same connection. A registration could commit and then be invisible to the login three lines later, surfacing as 401 Invalid credentials for an account created moments before, in roughly one run of test_node_ws_auth.py in four. The purge itself is not at fault and this is not a production condition. A passive SQL listener caught the DELETE removing 0 rows, and the INSERT carrying status='active' — so neither the pending-account mechanism nor the purge filter is involved, and PostgreSQL gives every session its own connection. What the fixture removes is the second user of the shared one. 60 runs of the previously flaky file, 0 failures; reproductions before the fix landed on attempts 4, 6, 13 and 29 of separate loops, so a clean run of 60 has about a 1% chance of being luck. _ASSETS. source-merge.js shipped missing from webapp._ASSETS, the cache-busting hash's input list — exactly the silent failure docs/apps.md §4 step 5 warns about: the file changes, the asset URL does not, and a browser holding the old page keeps the old copy. Harmless this time only because search-page.js changed in the same commit and is listed, which is the worst way for it to go unnoticed. Found by re-reading that checklist for the doc pass, not by any test — so there is a test now, holding _ASSETS to every .js in static/ (sw.js excepted, unversioned on purpose). It was the only one missing. Phase 9 of docs/refactoring-search.md also lands here: mediacenter.md §10.6, musicbay.md §9b, photos.md §10b, apps.md §2b and step 5. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AbwJDbNTkiRUh7HTWEoyss
-rw-r--r--docs/apps.md40
-rw-r--r--docs/mediacenter.md43
-rw-r--r--docs/musicbay.md34
-rw-r--r--docs/photos.md23
-rw-r--r--docs/refactoring-search.md10
-rw-r--r--packages/meshbay-hub/src/meshbay_hub/api/webapp.py1
-rw-r--r--packages/meshbay-hub/tests/conftest.py27
-rw-r--r--packages/meshbay-hub/tests/test_asset_versioning.py27
8 files changed, 200 insertions, 5 deletions
diff --git a/docs/apps.md b/docs/apps.md
index 1cd6339..ef8cc1a 100644
--- a/docs/apps.md
+++ b/docs/apps.md
@@ -80,6 +80,39 @@ needs — a new app does not get a bespoke prop list. Notable ones:
| `transportRef`, `gekRef` | refs to the live MNP transport and the imported group key | never state — a ref, so reconnects don't force a re-render of every app |
| `mayUpload` | `memberUpload || isNodeAdmin`, computed once | Files' toolbar and Chat's composer both gate on it; a second derivation would eventually disagree with the first |
+### 2b. The same app, rendered by the Search page
+
+Videos, Music and Photos are mounted twice: by `group-page.js` for one group,
+and by `search-page.js` across every group the reader belongs to. The second
+caller passes the same prop shape, and the difference lives entirely on the
+**entries**, in underscore-prefixed fields the group page never sets:
+
+| Field | What it is |
+|---|---|
+| `groupId`, `groupName`, `groupOwner` | which group serves this entry |
+| `_tRef`, `_gRef` | that group's transport and key — read as `entry._tRef \|\| transportRef`, which is why a single-group mount needs no special case |
+| `_connGen` | bumped when that group reconnects; use it as a refetch key so a tile recovers instead of staying a spinner |
+| `_sources` | every group that has this file, after the de-duplication below |
+
+**A file shared by two groups is one entry, not two** (`source-merge.js`,
+`docs/refactoring-search.md`). Entries are folded on their content hash and one
+source is resolved per *unit* — a film, a show, an album — using each app's own
+grouping function to decide what a unit is. Two consequences for a new app:
+
+- if it renders a group name, use **`SourceTag`** from `group-name.js` rather
+ than `entry.groupName`: a merged entry has several groups and must say
+ `N sources` instead of naming one. Pass it the **whole unit** (a show's
+ episodes, an album's tracks), not the entry the card was drawn from — that
+ entry is usually chosen for its thumbnail, and would under-report;
+- if it needs a merge unit key of its own, add a `<name>Units()` helper to
+ `search-page.js` that calls the app's **exported** grouping function. Never
+ re-derive the keys there: a copy keeps agreeing until one of them changes,
+ and the symptom is a show whose episodes stream from two different nodes.
+
+The Files explorer is deliberately **not** merged — there each group is a
+top-level folder, and merging would remove a file from one of them.
+`test_search_files_unmerged.py` refuses a build that changes this.
+
An app that needs **local** state (Files' `selecting`/`sortKey`/`currentPath`,
for instance) owns it itself with `useState`, same as before the split. One
thing worth keeping if you add a tab with a notion of "current location within
@@ -160,8 +193,11 @@ registry, so a newly-registered app gets a checkbox for free.
5. **`webapp.py`'s `_ASSETS`** tuple: add the new file. This is the
cache-busting hash's input list — a file imported by the page but missing
here can change without the served URL changing, which is the exact bug
- class `test_asset_versioning.py` exists for. Forgetting this step is
- silent: nothing errors, a browser just keeps an old copy.
+ class `test_asset_versioning.py` exists for. Forgetting this step used to be
+ silent: nothing errors, a browser just keeps an old copy. It is now caught —
+ `test_every_static_script_participates_in_the_fingerprint` holds `_ASSETS`
+ to every `.js` in `static/` (`sw.js` excepted, unversioned on purpose).
+ Written after `source-merge.js` shipped missing from the list.
6. **Test coverage that scans the file set**: `test_hook_ordering.py`
(`STATIC_FILES`) and `test_transport_contracts.py`
(`test_no_setter_survives_the_state_it_belonged_to`, `SPLIT_FILES`) walk a
diff --git a/docs/mediacenter.md b/docs/mediacenter.md
index dbdf85c..d714366 100644
--- a/docs/mediacenter.md
+++ b/docs/mediacenter.md
@@ -904,6 +904,49 @@ only when there is nothing else, and the lowest *number* rather than the first
entry so it does not quietly depend on `buildSeasons` keeping its sort.
`test_video_default_season.py` — no input it takes can carry a thumbnail.
+### 10.6 The same film twice in the cross-group Search view (2026-09-02)
+
+An operator hosting two groups gave both the *same* video directory — which is
+the point of having two groups: different people are invited to different
+libraries, and one library may be in several of them. **Search files** then
+showed every film as two poster cards and every episode twice in the season
+list, one copy badged per group. Flat list too.
+
+Not a Videos bug. Inside a group it cannot happen: `GroupIndex` is keyed by
+blake3, so the same bytes at two paths are already one entry. `search-page.js`
+concatenates *N* independently keyed indexes into one list, and that is where
+the duplication is born.
+
+`source-merge.js` folds entries on the content hash and resolves **one source
+per unit** — a film, a whole show — rather than per file: a season split across
+two nodes would open two connections and two metadata lookups for one show. A
+group hosted by the reader's own node wins (read from `handshake_ack`'s
+`is_node_admin`, which the node computes from its own record of who it belongs
+to, never a hub claim); failing that the pick is `hash(unitKey + userId)`,
+stable for one reader across renders and reloads — a source that changed
+mid-stream would tear down the connection under a film that is playing — and
+spread across readers.
+
+The units come from **`groupVideoEntries` itself**, called on the un-merged
+list purely to learn them, never a second copy of its keys in the Search page.
+A copy would keep agreeing until one of them changed, and the symptom would be
+a show whose episodes stream from two different nodes.
+
+Two consequences worth knowing:
+
+- **An operator's "Fix match" and "Rematch" go to the chosen source's node.**
+ On the operator's own libraries that is their node, which is what they mean.
+ On a merged entry they do not host, the override lands on whichever group was
+ picked — and the other source keeps its own match.
+- **`PosterGrid.mergedShows` still merges two differently-parsed titles once
+ both resolve to the same TMDB id.** Those were two units when the source was
+ picked, so a merged card can hold two sources. Left alone deliberately:
+ re-picking under a card the reader is looking at is worse than a mixed one.
+
+Full design, the adversary this names, and what must not change:
+`docs/refactoring-search.md`. `test_search_source_merge.py` holds the rules,
+`test_search_media_merge.py` holds this symptom end to end.
+
## 11. Acceptance before shipping
1. Re-run the §3 validation (real TMDB calls, same corpus, same script
diff --git a/docs/musicbay.md b/docs/musicbay.md
index 2ca8b23..d9794d2 100644
--- a/docs/musicbay.md
+++ b/docs/musicbay.md
@@ -434,6 +434,40 @@ actually implement (§3.2), not a property the protocol gives for free.
| M5 | Gapless playback, crossfade, lyrics, waveform seek preview | Nice-to-haves, no architectural prerequisite from this plan either way |
| M6 | Photos app | Out of scope of this document, per `apps.md`'s own list — unaffected by anything here |
+## 9b. One album, not two, in the cross-group Search view (2026-09-02)
+
+Reported against Videos and true here by construction: an operator hosting two
+groups that share one directory saw every track listed twice inside one album.
+Inside a group it cannot happen (`GroupIndex` is keyed by blake3); the Search
+page concatenates *N* indexes, and that is where the duplication is born.
+
+The fix is `source-merge.js`, applied identically to all three media views —
+fold on the content hash, resolve **one source per unit**. For Music the unit
+is an album, and a track loose enough to carry no artist at all is a unit of
+its own (or it would be dropped from the merge entirely, since
+`groupMusicEntries` never puts it in an album bucket).
+
+**`foldKey` is exported for this**, and the reason is worth keeping. §5.1's
+grouping folds case and `&`/`and` for the *key* while keeping the first-seen
+spelling for display — and which group is seen first is whichever index
+happened to arrive first. Keying a merge unit on the display strings would
+therefore let the chosen source change between page loads. The folded key is
+the one grouping actually uses, and is stable.
+
+The units come from `groupMusicEntries` itself, called on the un-merged list
+purely to learn them — never a second copy of its keys in the Search page.
+
+**The player needed no change.** `music-player.js` resolves a connection from
+`entry.groupId` (§2.3), and a merged entry carries exactly one. The queue built
+in `search-page.js`'s `onPreview` needed none either: it filters by `groupId`
+and is reachable only from the Files explorer, which is deliberately *not*
+merged — there each group is a folder and merging would remove a track from
+one of them.
+
+Design and the adversary it names: `docs/refactoring-search.md`.
+`test_search_media_merge.py` covers the album cases, including a differently-
+cased tag not splitting the unit.
+
## 10. Acceptance before shipping
1. Tag-coverage measurement against a real local library (not committed —
diff --git a/docs/photos.md b/docs/photos.md
index ed4a826..c00a7e0 100644
--- a/docs/photos.md
+++ b/docs/photos.md
@@ -456,6 +456,29 @@ than Videos, not more.
| P4 | Wrap-around vs. stop-at-ends for next/previous at album boundaries | UI choice, mirror whatever `video-player.js`'s own controls already do for consistency |
| P5 | Album cover selection (always "first photo" vs. an operator/member choice) | Product call; "first photo, stable" is a reasonable, zero-config default and is what this document assumes |
+## 10b. One photo, not two, in the cross-group Search view (2026-09-02)
+
+Reported against Videos and true here by construction: two groups sharing one
+directory listed every photo twice inside one album. `source-merge.js` folds
+entries on the content hash and resolves one source per album; the units come
+from `groupPhotoAlbums` itself (exported for this), called on the un-merged
+list purely to learn them.
+
+**One consequence is deliberate and is not a bug.** Albums are keyed by
+directory (§2.5), so two groups whose roots have *different basenames* put the
+same photo in two differently-named albums, and the merge — scoped to a unit —
+leaves it in both. That is correct: they are two albums. Only same-named albums
+collapse, which is the reported shape. Videos and Music do not have this case,
+their units being title- and tag-based rather than path-based.
+
+The album card's source badge counts the union over **the album**, not the
+cover photo: the cover is `photos.find((p) => p.thumb_hash) || photos[0]`, so
+an album in two groups whose cover sits in only one would have claimed a single
+source.
+
+Design: `docs/refactoring-search.md`. `test_search_media_merge.py` covers both
+the collapsing and the non-collapsing case.
+
## 11. Acceptance before shipping
1. Orientation correction verified against a real EXIF-rotated phone photo
diff --git a/docs/refactoring-search.md b/docs/refactoring-search.md
index 2210397..d39bd36 100644
--- a/docs/refactoring-search.md
+++ b/docs/refactoring-search.md
@@ -1,6 +1,7 @@
# Refactor: one file, one entry — merging duplicate sources in the Search view
-> Status: **phases 1–8 built** (2026-09-02), 9 (the doc pass) outstanding.
+> Status: **complete** (2026-09-02) — all nine phases landed. This stays as
+> the decision record.
> Branch `feat/search-source-merge`. Videos, Music and Photos are merged,
> failover is in, and a card with several sources says `N sources` instead of a
> group name. Confirmed live for Videos on 2026-09-02: one card per film,
@@ -335,6 +336,11 @@ be easy to ship and hard to notice.
override lands on whichever node was picked.
7. **`cacheGroupIndex`.** The IndexedDB cache is per group and stores raw index
entries. Merged entries must never be written to it.
+8. **`webapp.py`'s `_ASSETS`.** A new static file missing from it changes
+ without moving the asset URL, so a browser that cached the page keeps the
+ old copy — and nothing errors. `source-merge.js` shipped missing from it;
+ caught afterwards, and `test_every_static_script_participates_in_the_fingerprint`
+ now holds the list to every `.js` in `static/` so the next one cannot.
---
@@ -352,7 +358,7 @@ Each phase is independently testable and leaves the tree working.
| 6 ✅ | Same for Photos | `search-page.js` |
| 7 ✅ | `downGroups` and failover | `search-page.js` |
| 8 ✅ | `SourceTag` at the four display sites; `search.n_sources` in ten catalogues | `group-name.js`, `video-app.js`, `music-app.js`, `photos-app.js`, `locales/*.js` |
-| 9 | Docs: `mediacenter.md` §10, `musicbay.md`, `photos.md`, `apps.md` if the props contract moves | `docs/` |
+| 9 ✅ | Docs: `mediacenter.md` §10.6, `musicbay.md` §9b, `photos.md` §10b, `apps.md` §2b + checklist step 5 | `docs/` |
Phase 4 alone was enough to confirm the reported bug is gone; phases 5–8 are the
same mechanism applied outward.
diff --git a/packages/meshbay-hub/src/meshbay_hub/api/webapp.py b/packages/meshbay-hub/src/meshbay_hub/api/webapp.py
index 6dfd3ed..a1807ea 100644
--- a/packages/meshbay-hub/src/meshbay_hub/api/webapp.py
+++ b/packages/meshbay-hub/src/meshbay_hub/api/webapp.py
@@ -34,6 +34,7 @@ _ASSETS = ("style.css", "keyderive.js", "crypto.js", "transport.js", "app.js",
# app.js or group-page.js, so a change to any of them is a change
# to what the browser must fetch.
"icon.js", "file-utils.js", "hub-client.js", "apps.js",
+ "source-merge.js",
"chat-app.js", "files-app.js", "video-player.js", "video-app.js",
"music-app.js", "music-player.js", "photos-app.js",
"group-settings.js", "group-page.js",
diff --git a/packages/meshbay-hub/tests/conftest.py b/packages/meshbay-hub/tests/conftest.py
index 2769f7e..cb595c5 100644
--- a/packages/meshbay-hub/tests/conftest.py
+++ b/packages/meshbay-hub/tests/conftest.py
@@ -96,3 +96,30 @@ def _skip_email_verification(monkeypatch):
monkeypatch.setattr(
"meshbay_hub.api.users._create_and_send_verification", _noop)
monkeypatch.setattr("meshbay_hub.mail._send", lambda msg: True)
+
+
+@pytest.fixture(autouse=True)
+def _no_cleanup_task(monkeypatch):
+ """
+ Do not run the maintenance loop under test.
+
+ `create_app`'s lifespan starts `cleanup_loop` as an asyncio task, so every
+ test — each of which enters that lifespan — ran a purge pass concurrently
+ with its own requests. On SQLite `:memory:` that is not merely noisy: the
+ engine uses a **StaticPool**, one connection for the whole process, so the
+ request's session and the cleanup task's session interleave their
+ transactions on the *same* connection. A registration could commit and then
+ not be visible to the login three lines later, which surfaced as
+ `401 Invalid credentials` for an account created moments before, in about
+ one run of `test_node_ws_auth.py` in four.
+
+ The purge itself is not at fault and this is not a production condition:
+ the DELETE was measured removing 0 rows, and PostgreSQL gives every session
+ its own connection. What is removed here is the second user of the shared
+ one. Tests that want the maintenance behaviour call the `purge_*` functions
+ directly, which is how they are covered.
+ """
+ async def _noop(get_session):
+ return
+
+ monkeypatch.setattr("meshbay_hub.tasks.cleanup.cleanup_loop", _noop)
diff --git a/packages/meshbay-hub/tests/test_asset_versioning.py b/packages/meshbay-hub/tests/test_asset_versioning.py
index e5889f0..3d84d99 100644
--- a/packages/meshbay-hub/tests/test_asset_versioning.py
+++ b/packages/meshbay-hub/tests/test_asset_versioning.py
@@ -23,7 +23,7 @@ import re
import pytest
from fastapi.testclient import TestClient
-from meshbay_hub.api.webapp import ASSET_V, STATIC_DIR, _asset_version
+from meshbay_hub.api.webapp import _ASSETS, ASSET_V, STATIC_DIR, _asset_version
from meshbay_hub.app import create_app
@@ -101,3 +101,28 @@ def test_the_fingerprint_follows_the_content(tmp_path, monkeypatch):
finally:
target.write_bytes(original)
assert _asset_version() == before, "the fingerprint is not reproducible"
+
+
+def test_every_static_script_participates_in_the_fingerprint():
+ """
+ `_ASSETS` is hand-maintained, and forgetting an entry fails silently: the
+ file is imported by the page, so the browser fetches it, but it does not
+ feed the content hash — so a change confined to that one file ships at the
+ URL a cache already holds. Nothing errors, and the symptom is a fix that
+ "doesn't work" on exactly the machines that visited before.
+
+ Found by `source-merge.js`, added to the Search view and left out of the
+ list. It happened to be harmless that day because `search-page.js` changed
+ in the same commit and *is* listed — which is the worst way for this to go
+ unnoticed. The checklist in docs/apps.md §4 step 5 names this trap; this
+ enforces it instead of relying on remembering.
+
+ `sw.js` is the one deliberate exclusion — the service worker is served
+ unversioned on purpose (`test_the_service_worker_is_not_versioned`).
+ """
+ on_disk = {p.name for p in STATIC_DIR.glob("*.js")} - {"sw.js"}
+ missing = sorted(on_disk - set(_ASSETS))
+ assert not missing, (
+ f"static scripts missing from webapp._ASSETS: {missing}. A change to "
+ "one of these will not move the asset URL, so a browser that cached "
+ "the page keeps running the old copy")