3f23bb4396f61cf70b8ab77fa864b20f8a984847
380
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 yonlu
|
af2ff17342 | Merge remote-tracking branch 'origin/fix/263-slskd-transfer-lifecycle' into batch/258-272 | ||
|
yonlu
|
613901847a | Merge remote-tracking branch 'origin/feat/264-explore-cards' into batch/258-272 | ||
 yonluandClaude Opus 5.5
|
9710c11476 |
feat(download): one grab per Soulseek peer, several peers at once
slskd was capped at one transfer per daemon, on the grounds that Soulseek peers punish clients that ask for too much. That politeness is per peer: two different users do not compete for anyone's upload slot. So one slow peer serialised every other Soulseek download behind it. The manager now takes a per-(provider, peer) lock before any slot, so a grab waiting on a busy peer does not hold a provider slot another peer could use, and the slskd default rises to 3, which now counts peers. The help text says so. Running grabs at once exposed the folder collision: slskd names a download's directory after the remote leaf folder, so two peers' "Greatest Hits" (or any two rips' "CD1") share one directory, and collect finds files by name there. Grabs whose local folders overlap now take a package-level lock per folder, in sorted order, keyed on the full path because two clients can share one daemon. Closes #272 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT |
||
|
yonluandClaude Opus 5.5
|
5e3ac8fb1b |
fix(download): search Soulseek more than once, and read file lengths
The slskd search asked one question and ignored part of the answer. Two queries. Soulseek matches every term against a file's full path, so every extra word is a filter, and several filter wrongly: an edition qualifier from the catalog title that no one puts in a folder name, a term with a leading "-", which Soulseek reads as an exclusion, and "Various Artists", which is in no one's path. When a normalised form of the request differs, it runs alongside the original and the candidates are merged by peer and folder. Concurrently, not as a fallback: the manager gives a provider one search budget, and a Soulseek search spends most of it waiting. A query the user typed is searched as written. Stated options. The search carried only its id and text, so slskd's own defaults for its timeout and response limits applied. Its timeout is now set inside our wait, the limits are well above a popular album, and slskd drops folders below the file floor and peers with a queue we would not reach today. A state-only poll. Every one-second poll re-sent every response; the responses are now fetched once at the end, falling back to the old includeResponses form for a daemon without that endpoint. Durations. slskd reports each file's length and it was discarded. It is now carried as CandidateFile.LengthMillis and scored against the expected tracks as DurationFit, which takes 0.15 of title fit's weight when at least half the aligned pairs are timed: a title says which song a file claims to be, a length says whether it is that recording. Without lengths the score is exactly the previous formula. freeUploadSlots is removed from the response type; slskd sends hasFreeUploadSlot and nothing by that name. Closes #271 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT |
||
|
yonluandClaude Opus 5.5
|
f81a950916 |
fix(download): score a multi-disc rip as one album, and count tracks
CI / check (push) Skipped
CI / e2e (push) Skipped
Three faults in how candidates are shaped and scored, one commit because they meet in the same completeness number. Multi-disc albums were split in two. Soulseek shares them as Album/CD1 and Album/CD2, and candidates were grouped by the immediate parent, so each disc became its own candidate titled "CD1": about half complete, with an album title that could not match. Such a release essentially never cleared auto-pick. AlbumDir groups a disc folder under its parent, ParsePath takes the disc number from the folder (a disc in the filename still wins), and collect keeps the disc folders in staging, where flattened, disc 2's "01 Intro.flac" overwrote disc 1's. A single-track request could never be served from Soulseek. A track search matches one file per folder, and the two-file floor that screens out noise for an album screened out every result. A recording request takes one. Completeness counted files. Ten files against a ten-track album scored full marks whether or not they were its tracks, and title fit is the mean over the files that did align, so a folder where three titles matched read as near-perfect on both. Coverage is now counted in aligned tracks, with the file count still setting the penalty for extras. Closes #270 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT |
||
|
yonluandClaude Opus 5.5
|
7fbfd9c105 |
test(library): skip the cover-tier scan test when fixtures are absent
TestScan_StoresOnlyCoverTiers built the fixture path by hand, so in a tree where make testdata had not run it failed on a missing covers directory, where every other fixture test skips via testfixtures.Load. In a fresh worktree that failure blocked the pre-push hook for every branch. Closes #266 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT |
||
|
yonluandClaude Opus 5.5
|
7b42b9ce56 |
feat(explore): sample the one-album-owned shelf instead of ranking it
"More from artists you own one album by" drew its artists and their albums most-popular first, so it was a second leaderboard: the same handful of big names every time the page opened, which is not what the shelf is saying. The pool is still bounded, but which artists and which albums fill it is RANDOM(), so each visit is a different sample. The test asserts the set rather than the order. Refs #264 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT |
||
|
yonluandClaude Opus 5.5
|
0a33b9d653 |
fix(download): try the next acceptable copy when a transfer fails
A failed transfer failed the whole download. On Soulseek the usual failure is one peer being offline or refusing, and a popular album has several other peers offering the same folder; the ranked list that names them was already held in m.results and nothing walked it. grab now loops: when a candidate's transfer fails, or delivers too little of the album to import, the next candidate is tried in its place, up to three in all. Three rules keep that honest: - Only a candidate auto-pick would itself have accepted is offered, so a second choice clears the same match, quality and guardrail gates as the first. - On Soulseek the failure is the peer's, so every folder that peer offered is skipped with it; elsewhere only the failed release is. - A candidate the user picked by hand does not fall back. They chose that copy, and quietly substituting another is a decision they did not make. The same change fixes auto-pick grabbing the wrong candidate. AutoPickVeto judges the best candidate inside the user's guardrails, but Start and Attempt then grabbed ranked[0] -- so when the overall best was over the size ceiling, the veto passed on the strength of the second and the first was downloaded anyway: the one copy the user had said not to take unattended. autoPick returns the candidate the veto actually judged. Closes #263 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT |
||
|
yonluandClaude Opus 5.5
|
fc0121228e |
fix(download): give up on a stalled slskd peer and cancel its transfers
awaitTransfers waited on every requested file reaching a terminal state with no bound but the caller's six-hour context. slskd's transfer limit is one, so a peer that queued us and never sent a byte held every other Soulseek download behind it for the whole six hours. A grab now gives up after ten minutes with no bytes moving; a folder that stalls on its last tracks goes forward with what arrived, as a partial failure always has. Three smaller faults on the same path: - A file slskd never lists (refused at enqueue) could never reach a terminal state, so the wait could not end. It counts as failed after a short grace period. - A terminal record left by an earlier attempt at the same file from the same peer was read as this attempt's answer on the first poll. The ids already terminal before enqueue are ignored. - Giving up, for any reason, left slskd downloading for a request nobody was waiting on. The live transfers are cancelled and removed there, on a context of their own so a cancelled caller still sends it. Usernames are now path-escaped; they may carry spaces and slashes. Refs #263 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017HJiuc3ZZhxsPXz3ozTirT |
||
|
yonlu
|
5d9c677cf7 |
ci(index-artifact): import the exported artifact before publishing it
Nothing should be published until the code that imports it on a user's machine has imported it here. The exporter and the importer are two descriptions of one storage format, and every other tier tests the importer against a *fixture* rather than against the file being shipped — a second description free to be wrong in the same direction as the code reading it. That is how #258 reached everyone: the importer positioned its batch walk with a Go `string` cursor against this file's 16-byte `mbid` column, and SQLite neither coerces between TEXT and BLOB nor complains about the comparison, so the walk merged no rows and never advanced. The fixture guarding that walk writes the old text encoding, and the only compact fixture is one row, below the batch size, so the bound query never ran. Both were green throughout, and no install could finish a first index build. `TestImportPublishedArtifact` takes the published file and runs the client's own path over it — checksum, decompress, merge — and asserts that what the artifact holds is what the client ends up with: the row count, the rows carrying a listen count, an FTS index in step with the table, and one row read back through the app's own MBID conversions. It skips without `YJ_CORE_INDEX_ARTIFACT`, so an ordinary run pays nothing. It needs no Wails, which is why the step runs it under the indexbuild tag: that container has no GTK. Measured on the current artifact, 64.8 MB compressed: 39 seconds including the decompress. Verified against the pre-fix comparison behaviour, the step goes red in about 3 seconds — the strictly-advancing guard fails the import with a named reason rather than the day-long spin it used to produce. Refs #258 |
||
|
yonlu
|
1e3a490c12 |
fix(explore): refuse a catalog merge that does not land every row
The walk's predicates partition the artifact's key space, so a merge that ends with fewer rows than the artifact declares does not mean the artifact was smaller than it said — it means a predicate filtered rows out, and the catalog is quietly partial while reporting complete. Equality rather than a lower bound: RowsAffected counts an upsert that changes nothing, and a row already merged locally is counted again here. One reachable case, so this is not merely a tripwire. A row whose mbid is empty is excluded by `mbid > ?` in both encodings, so an artifact carrying one imports as a success with a row missing — which is the shape #258 had, one cause over. The test covers exactly that artifact. Refs #258 |
||
|
yonlu
|
d4ea14ca5c |
fix(explore): merge the catalog artifact in its own mbid encoding
The prebuilt catalog never merged. `mergeArtifactRows` positions itself with `WHERE mbid > ? ORDER BY mbid LIMIT 1 OFFSET ?` against the attached artifact, and it bound that cursor as a Go `string` while `cmd/indexexport` publishes `explore_index.mbid` as 16 raw bytes — the storage change that took the table from 677 MB to 389 MB. SQLite does not coerce between TEXT and BLOB and orders every blob after every text value, so against a byte column the predicate was not wrong but unconditional: `mbid > <text>` matched the whole artifact, so the bound the walk looked up was the same row every time and the cursor never advanced, and `mbid <= <text>` matched nothing, so no batch merged. No error, no rows, no state change — a fresh install sat at "0 of 1,077,893 rows" burning a core indefinitely, which is what it did here for a day, while Explore showed only the rows the library scan and the lazy artist enrichment had produced and popularity for none of the catalog. The cursor is now an `artifactKey`, typed to the encoding `artifactStoresText` reports for the file it is attached to, so the comparison is made in the same type as the column it is made against. Two things guard the class rather than the instance: a nil key binds as an empty value instead of SQL NULL, because `mbid > NULL` agrees with nothing and would import nothing just as silently; and the walk returns an error when its bound does not strictly advance, because the failure here is silence and the next one should be a failed job with a reason. It was never caught because the fixture that guards the walk writes the old text encoding, and the only compact fixture is a single row — below `artifactMergeBatch`, so the bound query never ran at all. The walk is now covered on both encodings, across several batch boundaries. Closes #258 |
||
|
yonlu
|
8dbdb7ad75 |
Merge branch 'fix/249-unbounded-growth' into batch/248-249
Both branches add to the same two registries, so the conflicts are between the two fixes rather than with main: - backend/app.go: both register a janitor job. Both are registered. - backend/maintenance/sweeps.go: both append a job at the end of the file. Both are kept, each with its own closing tail. - backend/maintenance/maintenance_test.go: both append a test. Both are kept as separate functions. - backend/library/library.go: #249's orphan-path lyrics delete was written against the loop variable before #250 renamed it, so its `audioFile.ID` no longer exists in that function. Adapted to `f.ID`. Closes #248 Closes #249 |
||
|
yonlu
|
a96cc9be1f | Merge remote-tracking branch 'origin/main' into fix/248-artist-metadata-sweep | ||
|
yonlu
|
53f480980f | Merge remote-tracking branch 'origin/main' into fix/247-cover-art-orphans | ||
|
yonlu
|
88f5524aa2 |
fix(maintenance): bound lyrics search and clicks, clear queue source
Three unbounded or stale surfaces, each small on its own: - lyrics_index rows were never pruned on track removal, so the FTS index grew forever. Delete the entry where the library search FTS entry is already deleted, on the orphan and RemoveFromLibrary paths. - search_clicks had no ceiling; age out ranking rows after a retention window via a daily janitor job. - queue.source_* kept a "Playing from X" label after its playlist was deleted. Drop the source when the queue's own playlist goes, wired through a playlist-service hook like Library.SetRemovalHooks. Closes #249 |
||
|
yonlu
|
e745acf88a |
fix(maintenance): sweep artist_metadata rows nothing references
artist_metadata was classified Cache/Swept but had no sweep and no DELETE anywhere, so long-lived entity data (no TTL by design) grew for the life of the install. Sweep rows whose MBID is neither a library artist nor holding cached artwork, and register the job with the janitor. Closes #248 |
||
|
yonlu
|
32bb64918c |
fix(library): sweep orphaned cover art when an album empties
pruneEmptyEntities deleted empty albums but never the cover_art rows they referenced, so removing the last track of an album leaked the row and its files forever — the janitor's covers sweep computes its live set from cover_art.file_path, which keeps the orphaned row's files exempt. Extract sweepOrphanedCoverArt/removeCoverArtFiles as one implementation and run it from pruneEmptyEntities (scan orphan path and RemoveFromLibrary) and RemoveLibrary alike. Closes #247 |
||
|
yonlu
|
1b9868ddd0 |
fix(library): preserve playlist phantoms on incremental scan and removal
The incremental scan's orphan cleanup and RemoveFromLibrary deleted audio_files rows without first filling the playlist phantom columns, so a track removed from the library folder outside YellowJacket (or removed from the library) became a permanently empty playlist row that nothing could re-link — the same bug #183 fixed on the full-rescan and retire paths, on the two paths it missed. Add a scoped PreservePlaylistPhantomsForFiles and run it in the same transaction as the deletes on both paths. Closes #246 |
||
|
yonlu
|
68e7edb8c9 |
feat(database): listening-events log with skip counters
Replace play_history with listening_events — one row per track exit, kind (complete/play/skip) plus raw position/duration — and add skip_count/last_skipped to audio_files beside play_count/last_played. The classifier that writes these lands later (plan 021); this is the schema it records into. Also drop the dead queue.source_playlist_id column and remove the stale references to the squashed migration chain in download_*.sql and tagging_items.sql, declaring the missing download-request indexes inline. |
||
|
yonlu
|
a3b5b43777 |
fix(database): preserve playlist phantoms across a stale audio_files retire
Retiring a stale audio_files dropped every playlist entry to an empty row: ON DELETE SET NULL ran before the phantom_* columns were filled, whereas the manual rescan path populates them first. Run the same phantom population inside the retire transaction, before the drop, only when audio_files is among the tables going, so ResolvePhantomTracksAfterScan can re-link the entries. Closes #183 |
||
 logan
|
f8800ca1f8 | Merge remote-tracking branch 'origin/main' into fix/231-setter-rollback | ||
|
logan
|
49445ded77 |
test(download): write the yt-dlp stub under ForkLock
The kernel refuses to exec a file that is open for writing anywhere in the process, and these tests are parallel: a sibling's fork duplicates stubYtDlp's write descriptor in the moment it is open and carries it past our close, so the exec a moment later fails with ETXTBSY. That is the flake seen once locally and once in CI, both times on a tree with no Go in its diff. Closing sooner is not available -- os.WriteFile has already closed the file before anything execs it -- and O_CLOEXEC does not help, because the window is between another goroutine's fork and its own exec. syscall.ForkLock is the lock forkExec takes across that fork, so holding it over the write means no child can exist while the descriptor does. Measured on the helper itself under 12 concurrent writers: 176-189 of 2400 execs refused before, 0 of 2400 after, three runs each. Closes #146 |
||
|
logan
|
bcf3856b6f |
fix(config): put the old value back when a setter is rejected
Config.Save() validates the whole config, so a setter that assigned before validating did not merely fail its own call: the rejected value stayed in memory and failed every later save, of every unrelated setting, silently and for the rest of the session. Nothing reached disk, so a restart cleared it — which is what made the fault invisible and unreportable. The defect is precisely "assignment precedes a validation that can reject that argument", and that predicate enumerates seven setters rather than the whole file. Each snapshots the field and restores it on the error path. The remaining setters were read rather than assumed and are unchanged: shortcuts.Config.Validate returns nil unconditionally, the bools and SetFavoritesPlaylistID pass through no validation that inspects them, Config.Validate does not validate Downloads at all, and SetViewVisible refuses an unknown, non-hideable or launch-page view before assigning. SetLibraryDirectory was already correct and is the precedent the new comment points at: it validates a candidate before assigning, so there is nothing to undo. The rationale sits above the setter section rather than on Save(), which is bound — a doc comment there renders into frontend/bindings for an audience with no use for it. Closes #231 |
||
|
logan
|
a113b7bd62 |
fix(riff): grow a chunk buffer with what arrives
Parse sized its buffer from the chunk header, which is four bytes read off the file, so a truncated or malformed WAV declaring a 4 GB data chunk in a 2 kB file got 4 GB from the allocator before the read discovered there was nothing to put in it. The error was always right; the allocation happened first. io.CopyN into a bytes.Buffer is what ID3Chunk beside it has done since #104, and needs nothing new: the reader stays an io.Reader and the buffer grows with what actually arrives. The regression test measures rather than asserts the error, because the error is identical on a build that allocates the gigabyte. Measured on the pre-fix build: 1,073,750,920 bytes of TotalAlloc for a 42-byte container whose data chunk claimed 1 GiB. Closes #216 |
||
|
logan
|
c56eae2959 |
fix(metadata): read a WAV's tags out of its RIFF id3 chunk
tagwriter has always written a WAV's tags into a RIFF "id3 " chunk correctly, and dhowden/tag -- which metadata.ExtractTags is built on -- has no RIFF reader at all. So the app could not see tags it had just written: editing tags on a WAV, autotagging a WAV folder or importing a WAV download all appeared to succeed and changed nothing the library could show, while the file on disk really was tagged and other players read it. backend/riff is a new package rather than a move into either half, because tagwriter already imports metadata: reaching back for parseRIFF is an import cycle, not merely the wrong direction. backend/tagtotals is the precedent. Its two readers are deliberately different. Parse holds every chunk in memory, which is what rewriting a file needs -- and a WAV's audio *is* a chunk, so doing that on the scan path would read every WAV in the library in full. ID3Chunk seeks over what it is not looking for. The container is asked before tag.ReadFrom rather than after it fails, because that library's last resort is an ID3v1 trailer and a WAV carrying both would otherwise be read by the wrong one. An untagged WAV -- no chunk, an RF64 container, a tag with every frame cleared -- reads as empty metadata with no TagReadWarning: the scanner's filename fallback is the right answer there, and a warning would report a fault on a healthy file. The gap was pinned by TestWAVTagsAreNotReadableYet, which failed the moment the reader learned and said in its own comment what to update. So it goes, TestFixturesMatchManifest no longer skips wav, and totals_test.go's WAV case reads through metadata.ExtractTags like the other three formats -- a round trip asserted through the writer's own parser was a test of the writer, which is why nothing caught this. Closes #104 |
||
|
logan
|
30c6b665f1 |
fix(system): give the process a temp directory that exists
Android has no /tmp and hands an app no TMPDIR. Go's os.TempDir() falls
back to "/tmp" when the variable is unset, so every library in this
process that wants scratch space was being handed a path that has never
existed.
SQLite is the one that noticed, and it said so precisely:
W/yellowjacket: msg="champion index rebuild failed"
explore.search-index.error="populate champion fts: disk I/O error (6410)"
6410 is not a generic I/O error. `6410 & 0xff` is 10, SQLITE_IOERR, and
`6410 >> 8` is 25 -- SQLITE_IOERR_GETTEMPPATH. SQLite could not work out
where to put a temporary file. Two measurements on the device say why:
`ls -d /tmp` does not exist, and the app process's environment carries
no TMPDIR. A shell's does (/data/local/tmp), which is why this is easy
to miss from `adb shell`.
The cost was a silent performance cliff on the slowest device this app
runs on: `championReady` stayed false, so every Explore search took the
generic path over the whole 1,079,667-row index instead of the champion
subset, and the rebuild was re-attempted on every launch.
**The class is fixed rather than the statement.** The trigger is the
*size* of the work, not that query -- anything that spills fails the
same way there, so large sorts, large joins and VACUUM were all waiting
their turn. The repair belongs at the process's one answer to "where do
temporary files go".
`PRAGMA temp_store = MEMORY` was the alternative: cheaper, more local,
and a promise that every future spill fits in RAM on a phone. The
catalog is the largest thing in this app and that is not a promise
worth making silently.
UseTempDir sits beside UseHomeOverride and carries its two rules for
the same reasons. **An empty base is a no-op**, because that is what
application.Mobile.StoragePath() returns on desktop -- so this needs no
build tag and changes nothing off mobile, where /tmp is real. And **an
explicit TMPDIR wins**, so anyone who set one deliberately gets it;
nothing sets it on the platform this exists for. It needs no new Wails
API and no Java change: StoragePath() is already what YJ_HOME is
pointed at, and the directory goes under it.
Two things beyond the rename of a variable.
**Writability is probed, not assumed.** MkdirAll on an existing
unwritable directory succeeds, so without the probe this could set
TMPDIR to a directory nothing can use -- which is the same bug one
directory over, and just as quiet.
**It returns its error, and main logs it.** A temp directory that could
not be created is the same silent failure one step earlier. A failure
is not fatal: it leaves the platform's answer in place, which is what
every release before this one ran with. That log line is readable on
the platform only because of #160.
Verified on the reference device, where the same launch that used to
print the failure now prints:
I/yellowjacket: msg="champion index rebuilt"
explore.search-index.elapsed=6.496s
Closes #190
|
||
|
logan
|
d034d6e571 |
fix(explore): resolve pending release MBIDs against the real table
The release-group MBID backfill queried `release_groups`, which plan 013
renamed to `albums`. It failed on its first statement on every launch
since
|
||
|
logan
|
842fe47e9e |
feat(player): count what the ring buffer misses
An underrun is audible and nothing counted it. When the ring is empty BufferedStreamer.Stream zeroes the caller's buffer and returns ok, so a run of zeros is spliced into the waveform and the step discontinuity at each edge is a click; a series of short ones is static. That is the one candidate in #135 whose audible signature matches the report. `starved` and `starvedSince` already existed, from #122's stall fix, and could not answer this: they are a *stall* detector, reset by every arriving sample, because their job is to end a track whose source has died and a merely slow source must not be cut short. That reset is exactly what made the audible case invisible -- a hundred 20ms underruns a minute never approach the give-up threshold, so they were invisible to the log, to the UI and to every tier. UnderrunStats counts runs, calls and samples for the life of the streamer. Runs is the number that means something audible: one episode is one pop however many callbacks it spans, and the ratio of runs to calls is what separates clicking from dropping out. Three things about the reporting are deliberate. **Counting is in Stream and reporting is not.** Stream runs on the speaker callback's real-time deadline, so a log line there would allocate, format and write on the exact path whose missed deadline is the defect -- measuring by making it worse. The count is two increments under a lock that was already held; the report is on the 1 Hz position ticker, which only runs while playing. **An unchanged count is not logged**, which is emitStatus' rule one package over. A healthy player is silent, so anything in the log is news and the line appears exactly while it is popping. **It is Info rather than Debug.** The default level is Info and a phone has no convenient way to set YJ_LOG_LEVEL, so a debug line here would be a counter nobody on the affected platform could read -- which is the shape of the bug that made #160 necessary. underrunDelta clamps at zero because the counter belongs to the streamer and the streamer is replaced on every track: a baseline carried across that boundary is the previous track's total subtracted from a fresh zero. The baseline is reset at the load as well; a negative count in a log line reads as a broken instrument and would discredit the measurement this exists to make. This is the instrument, not a fix. What it measures is on the issue. |
||
|
logan
|
168e588387 |
feat(android): route slog to logcat
Every slog line the app wrote on Android went to /dev/null, including the one naming the error it was about to os.Exit on. #52 is what that cost: a process that vanished with no tombstone, no AndroidRuntime stack and nothing in `logcat -b crash`, at Priority/Critical for months, whose entire diagnosis was one sLogger.Error main.go was already writing. backend/androidlog is a slog.Handler over __android_log_write, chosen in main() by build tag rather than by a runtime check so that a desktop binary links no cgo for a platform it cannot run on. **Everything except the write itself is untagged.** That is androidpayload.go's discipline pushed as far as it goes: the only toolchain that compiles the android tag is a cross-compiler and the only thing that runs it is a phone, so the priority mapping, the formatting, the chunking and the handler's own attr and group bookkeeping are ordinary Go that `go test` exercises everywhere, and android.go is fifteen lines that hand a string to liblog. Four things in it are load-bearing. **The tag is a fixed string, not the application id.** The debug build carries `applicationIdSuffix ".dev"` so it can be installed beside the release app, and it is the only build whose WebView can be inspected -- so a tag derived from the id is a different tag on the one build anybody debugging this app is running, and the filter meant to show these lines would hide them exactly where they were being looked for. **The priorities are android/log.h's own values, asserted twice.** android.go carries constant expressions that do not compile as uint if the header renumbers; the untagged test writes the six numbers out longhand, because comparing a constant to itself passes on any renumbering. A wrong priority is the failure that hides rather than breaks -- logcat prints whatever number it is handed, so an Error filed as Info is present, correct, and invisible to every filter. **Formatting is delegated to slog's TextHandler.** WithAttrs and WithGroup are the half of slog.Handler that is easy to get subtly wrong, and a logger whose groups are wrong is a logger nobody reads. The derived handlers share the parent's buffer *and its mutex*: a second mutex would guard nothing, and two loggers derived from one would splice their bytes into a single line under load. **A line is chunked, because liblog drops what does not fit.** The kernel logger's entry is 4068 bytes for tag and message together and the remainder goes without comment, so a long record would be truncated in the middle of the thing worth reading. Time and level are dropped from the formatted line, since logcat stamps every entry with both -- and dropping them by *key* also ate a caller's own "level" attribute, which the on-device probe caught and TestACallersOwnLevelAttrSurvives now holds. ReplaceAttr sees an empty group path for the built-ins and for every top-level attribute alike, so the kinds are what separate them. Verified on the reference device (TLP301, Android 14): a debug build logs I/W/E under the `yellowjacket` tag at the right priorities, and the first thing it surfaced was a real warning nobody could previously see -- `champion index rebuild failed ... disk I/O error (6410)`. Closes #160 |
||
|
logan
|
867ced8c81 |
feat(player): leave the volume to the system where the system owns it
On Android the hardware keys are the volume control and the framework mixes our stream against the device level, so a second control inside the app moves something the user already moved. Where that is true the player's level sits at maximum, SetVolume / ChangeVolume / MuteToggle are refused, and nothing persists a level nobody chose: restore remembers the stored value instead of applying it, and saveState writes that same value back rather than recording the synthetic maximum. Mute is in that list because it is a level of zero by another name -- and because with no control rendered it would be the one state on such a platform the user could not get out of. The predicate is named after the capability rather than the platform, because that is what makes it testable. Only platformOwnsVolume is behind a build tag, in two files that declare nothing else; everything else is decided against Player.systemVolume, a field a test sets either way. That is mediacontrols' split, with androidpayload.go's reasoning for keeping the contract out of a tagged file, and the tagged pair is covered by a source sweep since no tier here compiles both halves. SetDuck is deliberately untouched: it applies its attenuation by re-applying the *user's* level through setVolumeLocked, so pinning that level to maximum leaves the offset arithmetic exactly as it was. It is the only thing that may still move the output on such a platform, and TestSystemVolumeStillDucks is that property rather than a comment. |
||
|
logan
|
dc8db159f9 |
feat(player): centre the transport and show the volume inline
Two issues over one bar, because they are one relayout. #42's own findings say so: giving wa-slider a label grows it 6px to 14px and moves the transport, which is #23's subject, so doing them in sequence means measuring the bar twice and throwing the first set away. The bar was `320px 1fr auto`, so the transport sat in the middle of what the metadata and the queue button did not use — its centre was ~140px right of the window's at every width. The outer two tracks are the same expression now, so the middle is centred by construction. The side width is the metadata's, capped at a quarter of the bar, and the cap was measured as a regression before it was a decision: reserving the full `--now-playing-width` on both sides is perfectly centred and takes the seek bar's track from 257px to 61px at 800px, and to 0 at 200% text. The control you drag was paying for the symmetry. With the cap it is 246, which is parity. It is a `min()` rather than a breakpoint because that variable is user state — the metadata has a drag handle — and tying both sides to it is also what keeps dragging meaningful; a plain `1fr … 1fr` centres just as well and silently makes the handle a no-op. The volume moved out of `audio-player` into the bar because the transport column has to hold the transport and nothing else, and it joins the queue button in one cell rather than a second column, since the centring compares columns. It is a slider by default and a popup by setting. The stored flag names the *popup*, which is this config's polarity rule — the zero value has to be the intended answer, so an existing config.toml gets the new default with no migration. Inline, the icon is the mute toggle and is named after that action rather than the state, because with the slider beside it there is nothing to disclose; the component tier now covers both presentations rather than whichever is default. Three nested rules in this block began with a bare element selector, which Chrome 120 relaxed and the phone's Chrome 113 **silently drops** — including the ellipsis on the bar's own title and artist, which has therefore never truncated on the device. They are `&`-prefixed now. Filed as #154 for the class and for a check. `bottom-bar.spec.ts` pins both halves separately on purpose: an uncapped build is perfectly centred and fails only the seek-bar width, so a spec asserting centring alone would have passed the regression above. Both were verified by mutation. Closes #23 Closes #42 |
||
|
logan
|
8efed2dd2b |
refactor(shell): retire the Jobs destination
Nothing it carried is gone -- the two commits before this put all of it somewhere the work is already being done. A retired destination is the one shape #25's storage decision does not make free. A visibility entry is a map key and an unknown key is dropped on load; a launch page is a *value*, and an unknown one fails validation -- which on the load path means the app refuses to start for whoever had Jobs selected. `RetiredViews` is that list, read by `ApplyDefaults`, which treats a retired name as a zero value. An unknown-but-not-retired name still errors, because that is a typo and saying so is the useful answer. |
||
|
logan
|
a3926704cc |
feat(config): make the shell's destinations configurable
The sidebar's eleven entries are more than most libraries need, and Autotag rewrites tags on disk, which is not what a fresh install should be one click from. Stored as a map keyed by view id, where an absent key means that view's own default. A `HiddenViews []string` cannot express "Autotag off by default" -- its zero value is *hide nothing* -- and a boolean per view turns a view that later stops existing into stored garbage. With a map, an unknown key is dropped on load, a view added later gets its own default, and no install needs migrating in either direction. Same polarity as AllowMeteredCatalogDownload: the zero value is the intended answer. `Views` is also what DefaultPage now validates against, so which views exist and which may be the launch page are one list rather than two. Two states the user could not get out of are refused rather than allowed: Settings is never hideable, and the launch page is not hideable while it is the launch page. Both refuse in the *config*, not in the UI, because `config.toml` is hand-editable. On load the launch page is instead un-hidden -- there is nobody to tell, and the honest reading of "my launch page is Autotag" is that this user wants Autotag, not that their launch page should be silently reset. |
||
|
logan
|
018d857746 |
feat(shell): global back and forward in the top bar
The history stack has been global since the Android back gesture landed -- every navigation is an entry and `popstate` restores any of them in either direction. What the report describes as "back is tab-scoped" is that the only way back was a detail view's own button, which leaves the screen with the view it belongs to: click over to Tracks and the album you were reading is still one entry away with nothing on screen saying so. `<nav-history>` is that affordance, plus `nav.back` / `nav.forward` on Alt+Left / Alt+Right -- the browser's own combination, and clear of the bare arrows that seek, since a binding matches on its full canonical string. Forward is not back negated, which is why the old `pushedEntries` counter is gone rather than extended: `popstate` carries no direction and fires identically both ways, so one counter decremented on every pop reads a forward as a second back. Each entry carries its index and the shell keeps the current one and a high-water mark, which also survives a jump of more than one. The buttons dispatch the events the rest of the app already dispatches rather than calling `history` themselves -- the shell owns the guard that stops a press at the root leaving the app, and a second caller reaching for history is how the old `navStack` came to disagree with the platform. Below 900px the control stands down: the top bar is what runs out of room first below that, and nothing becomes unreachable -- the shortcuts are global at every width and the phone has the platform's gesture. Closes #6 |
||
|
yonlu
|
23f3d4b3b0 |
fix(explore): clear in_library on a row that has no local id
CI / check (push) Skipped
CI / e2e (push) Skipped
`in_library = 1 AND local_*_id IS NULL` was a fixed point. upsertBatch's conflict clause is `MAX(in_library, excluded.in_library)`, so it can only ever raise the flag, and pruneStaleLocalCrossReferences — which its own comment calls the only place a removal from the library is reflected back into the index — was gated on the id being present. So nothing in the app could clear such a row, ever: a permanent claim of ownership with no local row to check it against. The gate is now the flag *or* the id, for all three entity types. A NULL id fails the existence test on its own, so this needs no second clause to say what "not owned" means. Nothing in the tree writes that shape today — collectLibraryEntities sets both together — which is why this is worth closing rather than leaving: the exposure is a database written by a version whose local-id columns were populated differently, and the next writer that sets the flag without an id, which nothing structurally prevents and which this shape made permanent rather than merely wrong until the next scan. The test seeds the row with raw SQL on purpose. upsertBatch writes a zero LocalArtistID as literal 0, and 0 satisfies `IS NOT NULL`, so the old gate already caught that shape — a fixture built through the upsert cannot reproduce this at all. NULL is what the artifact importer and any older writer leave behind, the columns being nullable with no default. Reverted against the old gate, it fails on all three types. Closes #118 |
||
|
logan
|
ede183d026 |
test(shell): check 900x600, which is narrower than the minimum
The sidebar collapses to icons *below* 900, so the main panel is 843px at 899 and 700px at 900: the narrowest content area any desktop width produces is at the top of the Compact band, not at the enforced floor. A viewport list that stopped at "the minimum" was missing its own worst case. MinWidth's comment loses both reasons it used to give, because neither mechanism can happen any more — the subtitle is display:none from 899 down, and the sidebar host is overflow-y:auto (at 600x460 its scrollHeight is 434 against a 332px client, and Settings is reachable after scrolling). The value does not change: 800x600 is where desktop chrome stops being comfortable, not where the app breaks, and below 600 the phone layout takes over. A floor defended by two expired mechanisms is a number nobody can argue with, which is worse than either answer. Closes #24 |
||
|
yonlu
|
2b84bc53e9 |
fix(player): stop reporting one track's state against another
Five faults found while auditing the play/pause and position path for a desktop report of the pause icon showing over a seek bar that was not moving. They are one commit because they are one file's worth of tangled state, and two of them do not compile apart. The finished callback did not know which chain it came from. It is dispatched as a goroutine from the beep callback and then queues for p.mu, so a user pressing Next in the last second of a track had it wake up holding the lock for a player that had loaded something else -- and rewind it, stop it, and hand a stale finish to the queue's auto-advance. updateStreamers now stamps a chainID and the callback carries the one it was registered with. (#123) It also emitted PlaybackFinished and PlaybackStateChanged(stopped) *after* releasing p.mu, alone in this file, so a Play() taking the lock in that gap emitted `playing` first and the stale `stopped` landed last -- the button showing play over a track that was audibly running. Both emits are back under the lock. (#123) A source that failed mid-track was reported to the queue as a natural end, so a broken file auto-advanced in silence and was counted as played. The handler takes the reason now: the player cannot name the track, because the metadata is the queue's, so the queue emits PlaybackFailed and skips recording the play. (#123) p.format was assigned once, in the constructor, to the *speaker's* rate, and never again -- so it claimed 44.1 kHz for every file. The replay-after-finish path resamples from it, meaning a finished track played a second time was resampled from a rate the decoder never produced: audibly wrong speed and pitch, and the length and position fallbacks wrong with it. The fixtures are 22050 Hz, which is what lets a test see this at all. (#124) p.trackLengthMs was written only when the database had a row and cleared only by UnloadTrack, so a file with no row inherited the previous track's duration -- and every position report is scaled by it, so the bar reported one track's progress on another's scale. (#125) Queue.OnPlaybackFinished indexed q.tracks[currentIndex] having checked only that the queue was non-empty. currentIndex is -1 whenever the queue has been exhausted, and onQueueExhausted deliberately leaves the finished track loaded -- so playing it from there and letting it end panicked, on a goroutine with no caller to recover it. (#126) The position readers guarded the decoder with the speaker lock, which the read-ahead goroutine has no reason to hold and never takes -- so Position() raced readAhead's Stream() on every position emit, once a second for the whole of playback. srcMu is the lock that excludes that goroutine, and taking it naively deadlocks, because seekLocked already holds it and then emits the landing position from inside that region. seekSourceLocked is that region extracted, so the lock is released before anything is emitted. Found by the race detector, via the test added here for the chain guard: the existing suite never loads a file outside the integration guard, so make test was green over it. (#127) OnPlaybackFinished picks up //wails:ignore along with its error parameter: v3's generator segfaults on a bound method taking an error, and this was never IPC. That removes a binding the frontend could have called to force an auto-advance. Closes #123 Closes #124 Closes #125 Closes #126 Closes #127 |
||
|
yonlu
|
282dab43eb |
fix(player): end the stream when the audio source stops producing
BufferedStreamer.Stream treated an empty ring buffer as a momentary underrun and answered with silence and ok. That is right while the read-ahead is still going to deliver something, and two of its three exit paths left it never going to: a Close, and a source returning (0, true) in a loop. Neither set done, so the ring drained and every call after it was silence claiming to be audio, for the life of the process. Nothing above this type could tell that from healthy playback. The beep.Seq chain never ended, so the player stayed in Playing with the button showing pause; the decoder's position never moved, so the 1 Hz report pinned the seek bar at a constant -- and since every report resets the bar's interpolation, the report actively suppressed the one thing that would still have moved it. A frozen bar over a track that was not playing, with no watchdog anywhere to notice. Every exit now marks the stream done, and the silence fill is bounded by a duration *and* a run of calls. It needs both. Wall clock is the real measure, because the speaker paces itself and a stall is a question about time -- but a caller draining in a tight loop makes hundreds of calls in microseconds and would outrun a duration alone. A call count alone is the opposite failure, and not a hypothetical one: the first attempt used one and spent the whole budget before the read-ahead goroutine had been scheduled once, ending a perfectly good stream at sample zero and breaking TestBufferedStreamer_BasicStream. Err is plumbed out at the same time, because a drained source and a failed one both arrive as (0, false) and are not the same event. Reading it is a separate change; without it there is nothing to read. Closes #122 |
||
|
logan
|
9118c16fe3 |
feat(autotag): answer whether an album has a confident match
`MatchForAlbum(albumID)` is the question the album detail page needs to ask on open: does the autotagger already have something confident to say about this album, and what would applying it do. **It costs no MusicBrainz request.** Everything it needs is on disk — `tagging_items` carries the top score and release from the background prefetch, `tagging_candidates` durably holds the scored list. The rate limiters here are shared with every page the user can open, so a lookup that fires on page load must not join that queue; a folder nobody has scored yet answers "nothing", rather than scoring it now. **The tier is computed, not read.** `tagging_items.score` is the raw number and `Recommend` is what turns it into a claim, capping it for an ambiguous runner-up, an incomplete alignment or a folder too small to corroborate itself. Filtering on the stored score would promise confidence the scorer had explicitly withheld — which the two-track test pins. **Nothing is said about an album the user has already answered for.** Only a `pending` group qualifies: `confirmed` covers both a finished apply and an explicit "leave as is", and arguing with the second would be actively wrong. The join is `audio_files.group_key`, not a key derived from the folder path, because a group carved out of a mixed-bag folder is keyed on its tags — so a path-derived key would find nothing for exactly the messiest libraries this helps. `GroupCount` is returned because a multi-disc album is one group per disc: a caller that applied to "the album" from a single button would retag one disc of three. |
||
|
logan
|
fe67849e57 |
feat(autotag): name the confidence tier two features have to share
`ConfidentTier` and `Confident()` are a name for what was about to be written as `== RecommendationStrong` at two call sites: the album page telling the user unprompted that there is a match for what they are looking at (#28), and strict auto-accept rewriting files without asking (#90). A page that claims confidence the auto-accept pass would decline is the app contradicting itself, and #90 asks for exactly this — that the two agree on what "high confidence" means rather than computing it twice. What they do not share is written down beside it. Surfacing a match is a suggestion with a confirm dialog behind it; auto-accept is an irreversible on-disk rewrite gated on further conditions the tier cannot express — exact track count, every title matching, lengths within a couple of seconds, no cover replacement, no MBID conflict. So this is the floor both stand on, not the whole of either test. `Confident` is a rank comparison rather than an equality, so a tier added above "strong" later does not silently stop qualifying. |
||
|
logan
|
41c41a860e |
feat(explore): carry the local row id on a top result
`TopResult` was the one projection here that shipped `inLibrary` and no local id, so the top-results cards had no choice but to read the weaker flag. Every sibling model — `MBArtist`, `MBReleaseGroup`, `MBRecording` — already carries `LocalID`, and the candidate builders had the value in hand at every construction site. `LocalID` is set and cleared by a test against `audio_files`, so it means "there is something of mine here". `InLibrary` is written by the same pass but is a one-way ratchet the prune can only clear alongside a local id; it stays for scoring, which is where an approximate answer is fine. |
||
|
logan
|
4bf59b45b7 |
feat(library): answer album completeness for a screenful in one query
A card grid has to know how much of an album is here — an album held 2 tracks of 10 wearing the same green tick as one held whole is the complaint the badge-accuracy work was filed about — and `GetAlbumCompleteness` is one query per album, which is fifty round trips for a grid of fifty. `GetAlbumsCompleteness` is the same question over a slice. It is two grouping levels rather than the single-album form's correlated subqueries, because a correlated subquery in the FROM clause is not something SQLite will reliably do, and because the slice may only be spelled once or sqlc expands it twice with independently numbered placeholders. An album with no files is absent from the result rather than zeroed: "I have none of this" and "I have no idea" are the third state `Known` exists to keep apart. The test that matters is that the two spellings never disagree — they are genuinely different SQL, so the risk is a drift in meaning (a disc's total counted once per file, a duplicate counted twice) rather than a typo. |
||
|
logan
|
4b9114fd8d |
fix(tagwriter): declare the track and disc totals when tagging
An album the user holds 2 of 10 tracks of showed a green tick reading "is in your library", and the mechanism was our own writer. tagwriter wrote track and disc *numbers* and dropped the totals, so autotagging a folder made the release MBID-matched -- which is what earns the tick -- while erasing the one field GetAlbumCompleteness reads. The evidence for "2 of 10" was destroyed by the act that produced the tick. FieldTotalTracks and FieldTotalDiscs are written as the ID3 "n/N" form and as Vorbis TRACKTOTAL/DISCTOTAL; the autotag apply pass and the download importer fill them from the release's own tracklist; and dbsync persists the track total to audio_files.total_tracks so the album page agrees with the file without waiting for a rescan. Five things about it are load-bearing, and four fail silently: - The total is per *disc*, not per release, because that is what the tag form declares and what GetAlbumCompleteness sums per disc. A release total on every file multiplies a two-disc album's expectation by two, which no library can satisfy. backend/tagtotals is that derivation once, since the two callers must not import the writer or each other. - The Vorbis names are TRACKTOTAL and DISCTOTAL and no other spelling. dhowden/tag reads exactly those two keys, so TOTALTRACKS -- which xiph lists and several taggers write -- or a "1/12" packed into TRACKNUMBER writes successfully and reads back as no total at all. The tests therefore assert the round trip through the reader the scan uses, not through the bytes. - ID3's number and total share one frame, so writing either alone must read the other off the existing tag or discard it. A total with no number is not written: "/12" parses as track 0. - The totals are written unconditionally rather than on a diff. The case this exists for is a file declaring no total at all, which compares equal to nothing and is exactly what a "only if it changed" guard skips. - A single-track download is not totalled. A RecordingMBID anchor resolves Expected to that one track, so the same code would tag a track off a twelve-track album "1 of 1" -- and a declared total outranks the catalog total that would have answered correctly. autotag's field constants are a second copy of tagwriter's, deliberately so autotag stays out of the write pipeline's import graph. A key that drifts neither fails to compile nor fails to write -- the writer simply finds nothing under the name it looks for -- so autotagservice, the one package importing both, now pins them. Steps 2 and 3 of the issue stay open under #38: the catalog fallback already landed as completenessAnswer(), and the badge call-site audit is the part that overlaps it. Closes #16 |
||
|
logan
|
10660c8168 | Merge remote-tracking branch 'origin/fix/wanted-without-client' into integration/small-fixes | ||
|
logan
|
760021ea5a |
fix(downloads): stop searching a list there is nothing to search with
Every pass attempted every request, each came back "no download clients are enabled", and RecordAttempt wrote that down as an attempt and put a retry on the clock -- so a wanted list built deliberately without a client accrued failures and announced "next check in 6 hours" about a check that cannot happen. Wanting something with no way to fetch it is supported. Being told it is being looked for is a lie, and the row says what is true instead. Everything above the attempt still runs: an artist subscription still expands, and a request satisfied by some other route -- ripped, bought, copied in -- is still retired. Neither needs a provider. TestReconcileRespectsBatchSize now installs a client that finds nothing, because a batch size is about how many requests one pass searches for and that only means something when there is something to search with. Refs #37 |
||
|
yonlu
|
63ec068add | Merge branch 'main' into fix/small-issue-batch | ||
|
yonluandClaude Opus 5
|
185eb1b125 |
feat(smartplaylist): let a rule set match any rule, not only all of them
The conditions were joined with " AND " and nothing else, so a smart playlist could only ever narrow: "jazz released after 1960" was expressible and "jazz or blues" was not, which is most of what anyone reaches for a second rule to say. `RuleSet.Match` is "all" or "any", and an empty match is "all" — which is what every playlist saved before the field existed carries, so an upgrade cannot silently widen one. ParseRuleSet rejects anything else rather than falling through to AND, since a playlist quietly returning the wrong tracks is worse than one that refuses to be saved. Under OR each condition is parenthesised and under AND it is not: AND is the tighter operator, so an OR-join has to protect a condition carrying a top-level AND of its own — `days_since_played less_than` is two predicates belonging to one rule. The editor shows the choice as a sentence with the control in the middle, and hides it while there is one rule: with nothing to combine, all and any are the same query. Closes #35 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
yonluandClaude Opus 5
|
b3556d825c |
fix(mediacontrols): always send an art URL, even when there is no art
Every other key in the MPRIS metadata map can be omitted safely, because a client reading it renders a track with no title as a track with no title. Art is different: KDE's applet treats an absent mpris:artUrl as no news about the art and keeps drawing whatever the last track had, so playing something without a cover left the previous album's sleeve on screen — which reads as the wrong track playing rather than as missing artwork. The map's construction moves out of UpdateMetadata into a pure metadataMap so it can be asserted on at all: everything else in this file needs a live session bus, which is the same reason the Android contract lives in an untagged file. Closes #41 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
yonluandClaude Opus 5
|
bf4f352117 |
fix(queue): stop claiming a queue came from somewhere it no longer does
`q.source` was written by SetQueue and cleared in exactly one place, Clear, so no append path touched it: adding a track to a queue built from an album left the page still offering "Playing from <that album>", and since the source is persisted alongside the queue state the wrong label outlived the session that earned it. Every add and insert path drops it now. Removing and reordering deliberately do not — a queue with a track taken out of it is still that album, and the link still goes somewhere true. Only the arrival of a track from elsewhere makes the claim false. The delta event carries the source for the same reason it carries the current index: an append emits nothing else, so the frontend would keep the label it was last given until something forced a full state. Closes #14 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> |