yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

Explore: in_library can never be cleared on a row that has no local id #118

New Issue
Closed
opened 2026-08-19 04:42:52 +00:00 by logan · 1 comment
logan commented 2026-08-19 04:42:52 +00:00
Collaborator
Copy Link
Copy Source

Report

explore_index.in_library is a one-way ratchet with a blind spot: a row
that carries the flag with no local_*_id beside it can never have it
cleared. Nothing in the app can un-set it, and the ordinary repair pass
skips it by construction.

Findings

upsertBatch's conflict clause only ever raises the flag:

in_library = MAX(in_library, excluded.in_library),

and pruneStaleLocalCrossReferences — which its own comment calls "the
only place a removal from the library is ever reflected back into the
index" — is gated on the id being present:

UPDATE explore_index
   SET in_library = 0, local_release_group_id = NULL
 WHERE entity_type = ? AND local_release_group_id IS NOT NULL
   AND NOT EXISTS (...)

So in_library = 1 AND local_release_group_id IS NULL is a fixed point.
The same is true for all three entity types.

Does it arise? Not from any writer in the tree today:
collectLibraryEntities is the only thing that sets InLibrary = true,
and it sets a local id in the same struct. The exposure is (a) any
database written by a version whose local-id columns did not exist or
were populated differently, and (b) the next writer that sets the flag
without an id — which nothing structurally prevents, and which this
shape makes permanent rather than merely wrong until the next scan.

Why it matters even though the badges no longer read it. #38 routed
the UI onto local_*_id, which is set and cleared by a file test. But
in_library is not decorative — it still decides:

  • search ranking (fwInLibrary, worth 0.50);
  • the popularity-floor bypass, WHERE popularity >= ? OR in_library = 1,
    in four queries — so a phantom row is permanently searchable when a
    real one below the floor would not be;
  • two of Explore's shelves (shelves.go joins back through it, and one
    shelf is explicitly in_library = 0, which a phantom silently
    excludes from "things you do not have").

Reproduction

-- against a YJ_HOME's library.db
UPDATE explore_index
   SET in_library = 1, local_release_group_id = NULL
 WHERE entity_type = 2 AND mbid = <some release group>;

Then run a full library scan — PopulateLocalCrossReferences runs at
the end of it — and observe the row still reads in_library = 1. It
will outrank correctly-scored rows and will be missing from the
"discover" shelf forever.

Direction

One extra statement in pruneStaleLocalCrossReferences: clear
in_library on any row of that entity type whose local id is NULL,
after the id-gated pass has run (so the ordering is: retire stale
ids, then retire flags with no id). That is one UPDATE per entity
type and needs no new state.

The alternative — dropping MAX() from the conflict clause so the flag
follows the last write — is not it: the upsert is also how the dump
importer and the artifact merge touch these rows, and neither of them
knows anything about the library.

Deliberately not done in #38, which only had to stop the UI
believing the flag. Filed so the reasoning survives the PR.

**Report** `explore_index.in_library` is a one-way ratchet with a blind spot: a row that carries the flag with no `local_*_id` beside it can never have it cleared. Nothing in the app can un-set it, and the ordinary repair pass skips it by construction. **Findings** `upsertBatch`'s conflict clause only ever raises the flag: ```sql in_library = MAX(in_library, excluded.in_library), ``` and `pruneStaleLocalCrossReferences` — which its own comment calls "the only place a removal from the library is ever reflected back into the index" — is gated on the id being present: ```sql UPDATE explore_index SET in_library = 0, local_release_group_id = NULL WHERE entity_type = ? AND local_release_group_id IS NOT NULL AND NOT EXISTS (...) ``` So `in_library = 1 AND local_release_group_id IS NULL` is a fixed point. The same is true for all three entity types. **Does it arise?** Not from any writer in the tree today: `collectLibraryEntities` is the only thing that sets `InLibrary = true`, and it sets a local id in the same struct. The exposure is (a) any database written by a version whose local-id columns did not exist or were populated differently, and (b) the next writer that sets the flag without an id — which nothing structurally prevents, and which this shape makes permanent rather than merely wrong until the next scan. **Why it matters even though the badges no longer read it.** #38 routed the UI onto `local_*_id`, which is set and cleared by a file test. But `in_library` is not decorative — it still decides: - search ranking (`fwInLibrary`, worth 0.50); - the popularity-floor bypass, `WHERE popularity >= ? OR in_library = 1`, in four queries — so a phantom row is permanently searchable when a real one below the floor would not be; - two of Explore's shelves (`shelves.go` joins back through it, and one shelf is explicitly `in_library = 0`, which a phantom silently excludes from "things you do not have"). **Reproduction** ```sql -- against a YJ_HOME's library.db UPDATE explore_index SET in_library = 1, local_release_group_id = NULL WHERE entity_type = 2 AND mbid = <some release group>; ``` Then run a full library scan — `PopulateLocalCrossReferences` runs at the end of it — and observe the row still reads `in_library = 1`. It will outrank correctly-scored rows and will be missing from the "discover" shelf forever. **Direction** One extra statement in `pruneStaleLocalCrossReferences`: clear `in_library` on any row of that entity type whose local id is NULL, *after* the id-gated pass has run (so the ordering is: retire stale ids, then retire flags with no id). That is one `UPDATE` per entity type and needs no new state. The alternative — dropping `MAX()` from the conflict clause so the flag follows the last write — is not it: the upsert is also how the dump importer and the artifact merge touch these rows, and neither of them knows anything about the library. **Deliberately not done in #38**, which only had to stop the UI believing the flag. Filed so the reasoning survives the PR.
logan added the Kind/BugArea/Explore
Priority
Medium
3
Reviewed
Confirmed
1
 labels 2026-08-19 04:42:52 +00:00
yonlu self-assigned this 2026-08-19 17:18:26 +00:00
yonlu added the
Status
In Progress
 label 2026-08-19 17:18:27 +00:00
yonlu commented 2026-08-19 17:18:27 +00:00
Owner
Copy Link
Copy Source

Widening pruneStaleLocalCrossReferences so a row carrying in_library with a NULL local id is also cleared, for all three entity types, with a regression test seeding exactly that fixed point.

Widening pruneStaleLocalCrossReferences so a row carrying in_library with a NULL local id is also cleared, for all three entity types, with a regression test seeding exactly that fixed point.
logan closed this issue 2026-08-19 19:08:46 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-19 19:09:18 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Explore Kind/Bug
Priority
Medium
3
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees yonlu
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#118