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

An invalid setter argument poisons the in-memory config, so nothing saves until restart #231

New Issue
Open
opened 2026-08-30 08:41:11 +00:00 by logan · 2 comments
logan commented 2026-08-30 08:41:11 +00:00
Collaborator
Copy Link
Copy Source

Report

Config.SetTrackListColumns assigns the new column list to the
in-memory config before validating it, and returns the validation
error without putting the old list back:

c.TrackList.Columns = columns

if err := c.TrackList.Validate(); err != nil {
    return fmt.Errorf("invalid track-list columns: %w", err)
}

Config.Save() validates the whole config, so a rejected list does
not merely fail its own call — it makes every later call to any setter
fail too, because they all end in Save().

Reproduction (backend/config, a temp YJ_HOME, one invalid id):

SetTrackListColumns = invalid track-list columns: unknown track-list column ID: "titleArtist"
in-memory columns after failure = [{trackName} {artistName} {album} {trackLength} {titleArtist}]
later, unrelated SetThemeAccentColor = could not save config: invalid config: one or more config parts are invalid: unknown track-list column ID: "titleArtist"
later, plain Save = invalid config: one or more config parts are invalid: unknown track-list column ID: "titleArtist"

So one rejected column list and no setting can be saved for the rest
of the session — theme, default page, shortcuts, libraries, all of
them. Nothing is said to the user: config-page logs the first failure
to the console, and every subsequent failure belongs to a different
control that simply does not stick. A restart clears it, since the
poisoned list was never written to disk.

How it was reached, and why that route is now closed

Settings offered a column the backend does not accept (#197,
titleArtist), so a single tick reproduced this. That row is gone, so
today there is no known way to send an invalid list from our own UI.
The defect is not: Validate() also rejects a duplicate id, and the
frontend is free to send one — handleColumnMove and
handleColumnToggle both post lists assembled in the client — and a
stale frontend against a newer backend is the same shape.

Findings

  • GetTrackListColumns() returns the poisoned list while it is in
    memory, so a frontend that re-reads the config gets a column set the
    backend would not accept and the track list would try to draw.
  • It is not specific to this setter. SetLibraryDirectory,
    SetThemeAccentColor, SetDefaultPage, SetQueueFallback and the
    rest have the same read-modify-write-then-Save() shape; whether
    each restores its previous value on a rejected write is worth one
    pass over the file rather than one fix.
  • Adjacent but separate: Config has no lock at all (#97), which is
    about two setters racing rather than one failing.

Direction

Validate the candidate before it is installed, and leave the struct
untouched when it is rejected — for SetTrackListColumns that is a
tracklist.Config{Columns: columns}.Validate() on a copy, and the same
question asked of the other setters. A test that drives a setter with
input it must reject and then asserts an unrelated setter still saves
is what turns this from an argument into a fact; that assertion, not
the error string, is the one that fails today.

Found while doing #197.

**Report** `Config.SetTrackListColumns` assigns the new column list to the in-memory config **before** validating it, and returns the validation error without putting the old list back: ```go c.TrackList.Columns = columns if err := c.TrackList.Validate(); err != nil { return fmt.Errorf("invalid track-list columns: %w", err) } ``` `Config.Save()` validates the *whole* config, so a rejected list does not merely fail its own call — it makes every later call to any setter fail too, because they all end in `Save()`. **Reproduction** (`backend/config`, a temp `YJ_HOME`, one invalid id): ``` SetTrackListColumns = invalid track-list columns: unknown track-list column ID: "titleArtist" in-memory columns after failure = [{trackName} {artistName} {album} {trackLength} {titleArtist}] later, unrelated SetThemeAccentColor = could not save config: invalid config: one or more config parts are invalid: unknown track-list column ID: "titleArtist" later, plain Save = invalid config: one or more config parts are invalid: unknown track-list column ID: "titleArtist" ``` So one rejected column list and **no setting can be saved for the rest of the session** — theme, default page, shortcuts, libraries, all of them. Nothing is said to the user: `config-page` logs the first failure to the console, and every subsequent failure belongs to a different control that simply does not stick. A restart clears it, since the poisoned list was never written to disk. **How it was reached, and why that route is now closed** Settings offered a column the backend does not accept (#197, `titleArtist`), so a single tick reproduced this. That row is gone, so today there is no known way to send an invalid list from our own UI. The defect is not: `Validate()` also rejects a *duplicate* id, and the frontend is free to send one — `handleColumnMove` and `handleColumnToggle` both post lists assembled in the client — and a stale frontend against a newer backend is the same shape. **Findings** - `GetTrackListColumns()` returns the poisoned list while it is in memory, so a frontend that re-reads the config gets a column set the backend would not accept and the track list would try to draw. - It is not specific to this setter. `SetLibraryDirectory`, `SetThemeAccentColor`, `SetDefaultPage`, `SetQueueFallback` and the rest have the same read-modify-write-then-`Save()` shape; whether each restores its previous value on a rejected write is worth one pass over the file rather than one fix. - Adjacent but separate: `Config` has no lock at all (#97), which is about two setters racing rather than one failing. **Direction** Validate the candidate before it is installed, and leave the struct untouched when it is rejected — for `SetTrackListColumns` that is a `tracklist.Config{Columns: columns}.Validate()` on a copy, and the same question asked of the other setters. A test that drives a setter with input it must reject and then asserts an *unrelated* setter still saves is what turns this from an argument into a fact; that assertion, not the error string, is the one that fails today. Found while doing #197.
logan self-assigned this 2026-08-30 09:33:17 +00:00
logan added the
Status
In Progress
 label 2026-08-30 09:33:17 +00:00
logan commented 2026-08-30 09:33:17 +00:00
Author
Collaborator
Copy Link
Copy Source

Picking this up on fix/231-setter-rollback.

Approach: restore the previous value when the setter's own validation rejects it, rather than the larger "validate a candidate copy" refactor. The defect is precisely assignment precedes validation, and that predicate enumerates the affected setters mechanically rather than by judgement.

Scope, decided explicitly, since the Findings ask whether this is one fix or one pass over the file. It is a pass, but a narrower one than "every setter":

  • Fixing (7): SetScanConcurrency, SetThemeAccentColor, SetThemeBackgroundShade, SetDefaultPage, SetQueueFallback, SetTrackListColumns, SetFavoritesIconStyle — each assigns, then calls a sub-Validate() that can reject that very argument.
  • Not fixing, because they cannot be poisoned by their own argument (checked, not assumed): SetViewVisible already refuses an unknown/non-hideable/launch-page view before assigning, and GeneralConfig.Validate only normalizes the visibility map rather than rejecting it. SetShortcuts/SetShortcut — shortcuts.Config.Validate returns nil unconditionally. SetFavoritesPlaylistID, SetPinDefaultPlaylist, SetAllowMeteredCatalogDownload, SetPopupVolume — bools and an int64 that no Validate inspects. SetDownloadPreferences — Config.Validate does not validate Downloads at all.
  • SetLibraryDirectory is untouched and is the precedent: it builds and validates a candidate via library.NewConfig before assigning, which is the shape the other seven were missing.

Deliberately out of scope: the separate question of a Save() failure (a disk error) leaving memory ahead of disk. The value there has already passed validation, so nothing is poisoned and no later save is blocked — that is a different defect needing its own argument.

Picking this up on `fix/231-setter-rollback`. **Approach: restore the previous value when the setter's own validation rejects it**, rather than the larger "validate a candidate copy" refactor. The defect is precisely *assignment precedes validation*, and that predicate enumerates the affected setters mechanically rather than by judgement. **Scope, decided explicitly, since the Findings ask whether this is one fix or one pass over the file.** It is a pass, but a narrower one than "every setter": - **Fixing (7):** `SetScanConcurrency`, `SetThemeAccentColor`, `SetThemeBackgroundShade`, `SetDefaultPage`, `SetQueueFallback`, `SetTrackListColumns`, `SetFavoritesIconStyle` — each assigns, then calls a sub-`Validate()` that can reject that very argument. - **Not fixing, because they cannot be poisoned by their own argument** (checked, not assumed): `SetViewVisible` already refuses an unknown/non-hideable/launch-page view *before* assigning, and `GeneralConfig.Validate` only *normalizes* the visibility map rather than rejecting it. `SetShortcuts`/`SetShortcut` — `shortcuts.Config.Validate` returns nil unconditionally. `SetFavoritesPlaylistID`, `SetPinDefaultPlaylist`, `SetAllowMeteredCatalogDownload`, `SetPopupVolume` — bools and an int64 that no `Validate` inspects. `SetDownloadPreferences` — `Config.Validate` does not validate `Downloads` at all. - **`SetLibraryDirectory` is untouched and is the precedent**: it builds and validates a candidate via `library.NewConfig` *before* assigning, which is the shape the other seven were missing. Deliberately out of scope: the separate question of a *`Save()`* failure (a disk error) leaving memory ahead of disk. The value there has already passed validation, so nothing is poisoned and no later save is blocked — that is a different defect needing its own argument.
logan commented 2026-08-30 09:55:04 +00:00
Author
Collaborator
Copy Link
Copy Source

Fixed in PR #233 (fix/231-setter-rollback) — CI green (run 18472: check ✔, e2e ✔ on both chromium and webkit). Not merged.

Scope, since the Findings left it open. The previous run's note that every setter shares this shape turns out to be false, and checking it is what bounded the diff. The defect is precisely assignment precedes a validation that can reject that argument, which enumerates seven setters: SetScanConcurrency, SetThemeAccentColor, SetThemeBackgroundShade, SetDefaultPage, SetQueueFallback, SetTrackListColumns, SetFavoritesIconStyle. Each snapshots the field and restores it on the error path.

The rest genuinely cannot be poisoned by their own argument, and this was measured rather than assumed — on the unfixed code the two already-correct setters passed while all seven others failed:

  • SetShortcuts/SetShortcut — shortcuts.Config.Validate returns nil unconditionally.
  • SetFavoritesPlaylistID, SetPinDefaultPlaylist, SetAllowMeteredCatalogDownload, SetPopupVolume — an int64 and bools no Validate inspects.
  • SetDownloadPreferences — Config.Validate does not validate Downloads at all.
  • SetViewVisible — refuses an unknown, non-hideable or launch-page view before assigning; GeneralConfig.Validate only normalizes the visibility map rather than rejecting it.
  • SetLibraryDirectory — already correct, and the precedent the fix points at: it validates a candidate before assigning, so there is nothing to undo.

backend/config/setter_rollback_test.go covers all of it, including two cases pinning the setters deliberately left alone. The duplicate-id case is the one that still matters day to day: #197 closed the UI route to an invalid id, but a client assembling the list itself is free to send a duplicate.

One thing deliberately not done, noted here so it does not get lost: a Save() failure (a disk error rather than a rejection) still leaves memory ahead of disk. That value has already passed validation, so nothing is poisoned and no later save is blocked — whether the in-memory config should roll back to match the file is a different question needing its own argument. Not filed as an issue, because it is not clearly a defect; raising it here rather than inventing a decision.

Fixed in **PR #233** (`fix/231-setter-rollback`) — CI green (run 18472: `check` ✔, `e2e` ✔ on both chromium and webkit). Not merged. **Scope, since the Findings left it open.** The previous run's note that *every* setter shares this shape turns out to be false, and checking it is what bounded the diff. The defect is precisely *assignment precedes a validation that can reject that argument*, which enumerates **seven** setters: `SetScanConcurrency`, `SetThemeAccentColor`, `SetThemeBackgroundShade`, `SetDefaultPage`, `SetQueueFallback`, `SetTrackListColumns`, `SetFavoritesIconStyle`. Each snapshots the field and restores it on the error path. The rest genuinely cannot be poisoned by their own argument, and this was measured rather than assumed — on the unfixed code the two already-correct setters passed while all seven others failed: - `SetShortcuts`/`SetShortcut` — `shortcuts.Config.Validate` returns `nil` unconditionally. - `SetFavoritesPlaylistID`, `SetPinDefaultPlaylist`, `SetAllowMeteredCatalogDownload`, `SetPopupVolume` — an `int64` and bools no `Validate` inspects. - `SetDownloadPreferences` — `Config.Validate` does not validate `Downloads` at all. - `SetViewVisible` — refuses an unknown, non-hideable or launch-page view *before* assigning; `GeneralConfig.Validate` only *normalizes* the visibility map rather than rejecting it. - `SetLibraryDirectory` — already correct, and the precedent the fix points at: it validates a candidate before assigning, so there is nothing to undo. `backend/config/setter_rollback_test.go` covers all of it, including two cases pinning the setters deliberately left alone. The **duplicate-id** case is the one that still matters day to day: #197 closed the UI route to an invalid id, but a client assembling the list itself is free to send a duplicate. **One thing deliberately not done**, noted here so it does not get lost: a *`Save()`* failure (a disk error rather than a rejection) still leaves memory ahead of disk. That value has already passed validation, so nothing is poisoned and no later save is blocked — whether the in-memory config should roll back to match the file is a different question needing its own argument. Not filed as an issue, because it is not clearly a defect; raising it here rather than inventing a decision.
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/Settings Kind/Bug Platform/Android Platform/Desktop
Priority
Medium
3
Reviewed
Confirmed
1
Status
In Progress
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees logan
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#231