fix: verify-and-retry entry add/update against read-modify-write races - #12
Open
nate-kelley-buster wants to merge 9 commits into
Open
nate-kelley-buster wants to merge 9 commits into
nate-kelley-buster wants to merge 9 commits into
Conversation
Requesting "profile offline_access openid" during device-code login causes Yoto's auth server to reject the authorization with access_denied as soon as the user confirms the code (see TheBestMoshe#6). The CLI only needs an access/refresh token to call the Yoto API, so trim the request to the minimal "offline_access" scope. Verified end-to-end: device flow now reaches the consent screen and completes successfully. Fixes TheBestMoshe#6 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0182auKxRRCcLdZrBotvbJ6K
The Yoto API returns display.icon16x16 as null for chapters/tracks that don't have a custom icon set, but DisplaySchema only allowed string|undefined for that field. This made `playlist show` (and any other command that parses card content) throw a Zod validation error on any playlist with unset icons instead of rendering. Verified against a real 25-chapter playlist that previously failed to display. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0182auKxRRCcLdZrBotvbJ6K
\`icon list\`, \`icon list --mine\`, and \`icon upload\` all displayed
\`displayIconId\` in their "ID" column / success message. But
\`entry|chapter|track --icon\` (per its own --help text: "file path,
mediaId, or yoto:#mediaId") requires the \`mediaId\`, a different,
longer token — not the displayIconId.
Copy-pasting the ID shown by these commands into --icon silently
builds an invalid \`yoto:#{displayIconId}\` reference, which the API
rejects with:
icon16x16 must be in format "yoto:#{mediaId}" where mediaId is 43
characters
Fix: print \`mediaId\` (renamed column to "Media ID") everywhere an
icon reference is surfaced to the user, since that's the value
--icon actually consumes.
## Verification
Reproduced end-to-end against the live API: copying a displayIconId
from \`icon list\` into \`entry update --icon\` failed with the error
above. After this fix, \`icon list\`/\`icon upload\` print the mediaId,
and passing that value to \`entry update --icon\` succeeds.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0182auKxRRCcLdZrBotvbJ6K
Fixes TheBestMoshe#3 entry add, track upload, and track status all polled transcode status against a hardcoded allow-list of "in progress" phase names (queued/processing/transcoding, or queued/processing). Any other phase the API returned was treated as a terminal failure and the command exited immediately with "Transcoding failed with status: <phase>" — even though the file was still being processed. In practice the API emits more phases than the allow-list knew about ("analyzing", "downloading" both reproduced live), so most uploads failed instantly instead of polling through to completion, exactly as described in TheBestMoshe#3. ## Fix Invert the check: only treat a phase as terminal failure if it's explicitly "failed" or "error". Any other phase (known or not) is treated as still-in-progress and polling continues until the file reaches "complete" or the timeout elapses. Also bumped the polling timeout from 5 to 10 minutes across all three call sites, since the old timeout was sized around the old (incomplete) phase list and some files legitimately spend real time in earlier phases like downloading/analyzing before transcoding starts. ## Verification Reproduced against the live API: `entry add` on a real audiobook chapter failed immediately with "Transcoding failed with status: analyzing", then again with "status: downloading" after a partial fix. After this change the same file uploads and transcodes successfully end-to-end (56-minute file, "Transcoding complete"). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0182auKxRRCcLdZrBotvbJ6K
Every mutation to a playlist (entry add, entry update, etc.) does a non-atomic read-modify-write against POST /content: fetch the whole card, mutate it locally, POST the entire document back. The API has no optimistic-concurrency check (no version/etag), and a getContent() shortly after a write can race the server's own propagation and return stale data. A second read-modify-write built on that stale snapshot silently clobbers the first write when it POSTs the whole document back — the CLI reports success on both calls, but one chapter/icon update quietly vanishes. This is not theoretical: it was hit twice in real usage, uploading large multi-chapter audiobook playlists strictly one `entry add` / `entry update` call at a time (no client-side concurrency) — a chapter went missing after `entry add` reported success, and separately 12 icon updates got silently reverted to a placeholder after `entry update` reported success on each one. ## Fix Added `YotoClient.updateContentSafely(cardId, mutate, verify)`, which wraps the read-modify-write in a verify-and-retry loop: after POSTing, it re-fetches the card and checks the caller's `verify` predicate actually holds. If not, it retries the full fetch-mutate-write cycle (up to 4 attempts, with backoff) instead of trusting a single write. `addEntry` and `updateEntry` in src/commands/entry.ts now go through this helper, verifying the new chapter's title/trackUrl (add) or the updated title/icon (update) actually landed before considering the command done. ## Verification Reproduced the addEntry/updateEntry race in real usage (see above). Rebuilt with this fix and re-ran the exact update-icon workflow that previously silently failed against a live test playlist; confirmed via a direct API read (not just the CLI's own success message) that the icon change was actually persisted. ## Scope This fix covers `entry add`/`entry update`, the two commands that triggered the bug in practice. The same read-modify-write pattern exists in `chapter add/update`, `track add/update`, and `playlist update` (src/commands/content.ts) — those would benefit from the same `updateContentSafely` treatment as a follow-up, but are out of scope here to keep this fix focused and reviewable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0182auKxRRCcLdZrBotvbJ6K
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.
Problem
Every mutation to a playlist (
entry add,entry update, etc.) does a non-atomic read-modify-write againstPOST /content: fetch the whole card, mutate it locally, POST the entire document back. The API has no optimistic-concurrency check (no version/etag), and agetContent()shortly after a write can race the server's own propagation and return stale data. A second read-modify-write built on that stale snapshot silently clobbers the first write when it POSTs the whole document back — the CLI reports success on both calls, but one chapter/icon update quietly vanishes.This is not theoretical: it was hit twice in real usage, uploading large multi-chapter audiobook playlists strictly one
entry add/entry updatecall at a time (no client-side concurrency):entry addreported success.entry updatereported success on each one.Fix
Added
YotoClient.updateContentSafely(cardId, mutate, verify), which wraps the read-modify-write in a verify-and-retry loop: after POSTing, it re-fetches the card and checks the caller'sverifypredicate actually holds. If not, it retries the full fetch-mutate-write cycle (up to 4 attempts, with backoff) instead of trusting a single write.addEntryandupdateEntrynow go through this helper, verifying the new chapter's title/trackUrl (add) or the updated title/icon (update) actually landed before considering the command done.Verification
Reproduced the race in real usage (see above). Rebuilt with this fix and re-ran the exact update-icon workflow that previously silently failed against a live test playlist; confirmed via a direct API read (not just the CLI's own success message) that the icon change was actually persisted.
Scope
This fix covers
entry add/entry update, the two commands that triggered the bug in practice. The same read-modify-write pattern exists inchapter add/update,track add/update, andplaylist update— those would benefit from the sameupdateContentSafelytreatment as a follow-up, but are out of scope here to keep this fix focused and reviewable.