fix: the four defects the client campaigns left open - #228
Merged
Merged
Conversation
Nothing here is a regression — all four shipped in `2.0.0-beta.0` — and all four are cleared before the tag rather than carried into it. **#224, a role separator cutting inside parentheses.** `split_on` applied its separators in passes over the whole value, with no idea where it was. A `COMPOSER` reading `Kobee (Melange / INHOUSE), Holy M (Melange / INHOUSE)` names two people; the two slashes made three entities, each carrying a parenthesis it never opened, and they reached the catalogue as artists that search answered with. It is one pass now, counting parenthesis depth and cutting only at zero. The rule the reference asks for is untouched: `Bach/Gounod` is still two people and `AC/DC` is still one band. Depth is clamped at zero so a stray `)` cannot make the rest of a value uncuttable, and a `(` that never closes protects only the tail it opened. **#225, `song` where the schema says `entry`.** `getPlayQueue` and `getNowPlaying` named their children as if the rename that applies inside a playlist did not apply to them. Three campaigns passed over it because every client that met it was lenient; one that decodes against the schema reads an empty queue. `playQueue` also gains the `username` the schema requires beside `changed` and `changedBy`. This is an observable wire change, made deliberately now: the freeze protects the clients that were validated, not a divergence from the specification, and a beta is what it is for. **#226, a 416 announcing a length of zero.** Two refusals wear that status and they do not mean the same thing. A complete file knows its length, so it says which range would have been satisfiable — and it accepts ranges, which every other answer for that same file already says, where this one claimed `none`. A transcode still being produced knows how many bytes it has written, never how many it will; `bytes */0` did not say "unknown", it said "empty", which a client may believe and never ask again. That header is now left out rather than filled with a falsehood. **#219, a refusal that outlived its session.** A sign-out and a sign-in fit inside one round trip, so a 401 raised for one account could arrive after another had signed in — renewing spent the new account's rotating refresh token for a request the old one made, and the replay went out under the new account's token. Five routes did this, not the four the issue names: the scan event stream renews the same way and was missed. One `renewFor` now holds the shape, checked before the renewal and again after it, because the renewal is itself a round trip a sign-out can happen inside. Every test here was run against the defect as well as against the fix. Three of them fail with the parenthesis guard removed or made too broad; the contract test fails on the rename and, separately, on `username` alone; the range assertions fail on `bytes */0` and on the wrong `Accept-Ranges`; the session test fails with the generation check neutralised, and it is the only one of the nineteen that does, which is what makes it new coverage rather than a second opinion. Two review findings were not taken. Rechecking the generation a second time between `renewFor` resolving and the replay closes a window one microtask wide that nothing in the module can write to — every writer sits behind an await, and a user gesture is a task, which cannot preempt a microtask drain; no scheduling distinguishes it, which is this repository's own bar for keeping a guard. And making the empty `getPlayQueue` schema-conforming would mean omitting the element, a third wire change on the one call every client makes at startup, with no observed harm and none of it asked for. That shape is pinned by a test instead, and left for #225 to settle. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
github-actions
Bot
added
scope: server
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughLa modification corrige les réponses 416 des transcodages, les noms d’éléments Subsonic, le découpage des crédits entre parenthèses et les renouvellements de session après changement de compte. Des tests couvrent chaque comportement. ChangesRéponses de plages média
Contrat Subsonic
Découpage des crédits
Protection du renouvellement de session
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant API as api.ts
participant Generation as sessionGeneration
participant Refresh as renewFor
participant Request as Requête 401
API->>Generation: Capture la génération courante
Request-->>API: Retourne 401
API->>Generation: Vérifie la génération
alt Génération inchangée
API->>Refresh: renewFor(generation)
Refresh->>Generation: Vérifie la génération après renouvellement
Refresh-->>API: Session renouvelée
else Génération modifiée
API-->>Request: Rejette sans renouvellement ni rejeu
end
Merge Risk: ⚪ Minimal · up to The protocol, streaming, credit parsing, and session-retry fixes are covered by aligned implementation and regression tests. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 56.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 8 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
InstaZDLL
added a commit
that referenced
this pull request
Sep 16, 2026
…nged the wire #228 merged, so this branch was saying the opposite of the truth: it listed #219, #224, #225 and #226 as known issues of the release that fixes them. They move into **Fixed**, and #225 moves further than that. Renaming `getPlayQueue` and `getNowPlaying` children from `song` to `entry`, and giving `playQueue` the `username` the schema requires, is an observable change for anyone already reading `song` — so it belongs beside the two `/api/v2` shape changes, not buried in a list of corrections. The section is no longer about `/api/v2` alone. It also records what was deliberately not done: both names are not emitted together, because a response carrying `entry` and `song` conforms to neither contract and would need a second wire change later to undo. And `getPlayQueue` with nothing saved still answers a bare `playQueue`, which is not schema-conforming either — making it so means omitting the element, on the one call every client makes at startup, and that is a decision for after the beta. The client campaign is recorded as what it was: the whole replayed set, four clients on three platforms, not the two this file named. Substreamer is said to be outside the set rather than left to look forgotten. And the commit count is 361, not 354. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The four defects the client campaigns left open, cleared before the tag. None is a regression — all four ship in
2.0.0-beta.0— and none is carried into2.0.0-beta.1.Closes #219. Closes #224. Closes #226. Closes #225.
#224 — a role separator cutting inside parentheses
split_onapplied its separators in passes over the whole value, with no idea where in the string it was. ACOMPOSERreadingnames two people. The two slashes made three entities, each carrying a parenthesis it never opened, and they reached the catalogue as artists that
search3answered with — 43 names appeared in search that the browse index never held, four of them these fragments.It is one pass now, counting parenthesis depth and cutting only at zero. The reference's rule is untouched:
Bach/Gounodis still two people,AC/DCis still one band. Depth is clamped at zero so a stray)cannot make the rest of a value uncuttable, and a(that never closes protects only the tail it opened.#225 —
songwhere the schema saysentrygetPlayQueueandgetNowPlayingnamed their children as though the rename that applies inside a playlist did not apply to them. Three client campaigns passed over it because every client that met it was lenient; one that decodes against the schema reads an empty queue.playQueuealso gains theusernamethe schema requires besidechangedandchangedBy.#226 — a 416 announcing a length of zero
Two refusals wear that status and they do not mean the same thing.
Content-RangeAccept-Rangesbytes */<real length>bytes— only the range was wrongnonebytes */0did not say "length unknown". It said the resource is empty, which a client may believe and never ask about again. And the first case answerednone, contradicting every other answer the same file gives.#219 — a refusal that outlived its session
A sign-out and a sign-in fit inside one round trip, so a 401 raised for account A could arrive after account B signed in: renewing spent B's rotating refresh token for a request A made, and the replay went out under B's access token.
Five routes did this, not the four the issue names — the scan event stream renews the same way and was missed. One
renewFornow holds the shape, checked before the renewal and again after it, because the renewal is itself a round trip a sign-out can happen inside.Every test was run against the defect, not only against the fix
usernameremoved alonebytes */0restoredAccept-Ranges: noneon a known lengthThat last line is the point: it is new coverage, not a second opinion on something already covered.
Two review findings not taken
Rechecking the generation between
renewForresolving and the replay. It closes a window one microtask wide that nothing in the module can write to — every writer of the session sits behind anawait, and a user gesture is a task, which cannot preempt a microtask drain. No scheduling distinguishes the guard from its absence, which is this repository's own bar for keeping one. Say the word and I will add it anyway; it costs one&&.Making the empty
getPlayQueueschema-conforming. The schema has no way to say "empty queue" — the reference omits the element — so conforming means removingplayQueue, a third wire change on the one call every client makes at startup, with no observed harm and none of it asked for. That shape is pinned by a test instead, and left for #225 to settle.After this merges
CHANGELOG.mdlives on thechore/release-2-0-0-beta-1branch (#227) and lists all four of these under Known issues. Whichever merges second needs that section replaced by the fixes, and#225announced as a wire change beside the two/api/v2shape changes already there.Checks
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warnings— cleancargo test --all-features— 289 passed (was 285)bun --cwd=webapp run test— 135 passed (was 134)Summary by CodeRabbit
Nouvelles fonctionnalités
getNowPlayingetgetPlayQueueutilisent désormais le formatentryet incluent l’identifiant utilisateur approprié.Corrections