Skip to content

Skip non-positive provider ids instead of forwarding them - #37

Open
MarcFauser wants to merge 1 commit into
intro-skipper:mainfrom
MarcFauser:fix/skip-non-positive-provider-ids
Open

MarcFauser wants to merge 1 commit into
intro-skipper:mainfrom
MarcFauser:fix/skip-non-positive-provider-ids

Conversation

@MarcFauser

Copy link
Copy Markdown

A negative placeholder is an established convention in Jellyfin libraries for "do not match this item": the anime providers only search by name when the stored id is empty (AniDbSeriesProvider, AniListSeriesProvider), so writing a deliberately invalid id suppresses a wrong match. Jellyfin stores provider ids as strings and does not validate them as numbers - ProviderIdsExtensions has format validators for IMDb, TMDb, AudioDb and MusicBrainz only.

TryGetIntProviderId forwards such a value because NumberStyles.Integer allows a leading sign, so "-1" parses happily. The API then rejects the whole batch and names only the first offending index, so one item costs every lookup in it.

Returning null instead drops the field from the request body, because the lookup models use int? and SkipMeApiClient sets
JsonIgnoreCondition.WhenWritingNull. No lookup is lost by this: shows are only abandoned when tvdb_series_id, tmdb_id, imdb_series_id AND anilist_id are all null (TryBuildShowLookup), and on the library this was measured against, all 81 affected series carried a Tvdb, Tmdb or Imdb id as well.

A negative placeholder is an established convention in Jellyfin libraries for
"do not match this item": the anime providers only search by name when the
stored id is empty (AniDbSeriesProvider, AniListSeriesProvider), so writing a
deliberately invalid id suppresses a wrong match. Jellyfin stores provider ids
as strings and does not validate them as numbers - ProviderIdsExtensions has
format validators for IMDb, TMDb, AudioDb and MusicBrainz only.

TryGetIntProviderId forwards such a value because NumberStyles.Integer allows a
leading sign, so "-1" parses happily. The API then rejects the whole batch and
names only the first offending index, so one item costs every lookup in it.

Returning null instead drops the field from the request body, because the
lookup models use int? and SkipMeApiClient sets
JsonIgnoreCondition.WhenWritingNull. No lookup is lost by this: shows are only
abandoned when tvdb_series_id, tmdb_id, imdb_series_id AND anilist_id are all
null (TryBuildShowLookup), and on the library this was measured against, all 81
affected series carried a Tvdb, Tmdb or Imdb id as well.
@kilo-code-bot

kilo-code-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (1 file)
  • SkipMe.Db.Plugin/Tasks/SyncSegmentsTask.cs

Reviewed by gpt-5.6-luna · Input: 36 · Output: 4.9K · Cached: 250.4K

@MarcFauser

Copy link
Copy Markdown
Author

Two follow-ups, both about the 500 in the last section rather than about this change.

The response body is bare. Every 500 we saw, from both endpoints, carried exactly
Internal Server Error and nothing else — which is why the daily free tier limit check in
SkipMeApiClient can never match, whatever the actual cause was. Across 69 server log files
on the library in question, usage limit reached appears 0 times while returned 500
appears 13.

There is a proxy in front of the API. From the thread on #32, a direct request came back
with server: cloudflare. That is secondhand — I have not queried the API myself — so I
would not present it as measured. But if an error body can be replaced or shortened in front
of the origin, then matching on a message substring is fragile regardless of what the backend
emits. A status code (429) or a response header would survive that; a sentence in the body
may not.

This matters a little beyond the log wording, because usageLimitExceeded is what stops the
remaining batches. Undetected, a sync spends all of them against a budget that may already be
gone — which is the opposite of what you want on a metered backend.

On process: I saw how #32 ended — "the API is being updated to provide a better
response"
. If the same applies here and you would rather handle the non-positive id
server-side (ignore it rather than reject the batch), that is a good outcome from where I sit
and I am happy to close this PR. The client-side > 0 is offered as the cheap half, not as
the preferred one, and I have no attachment to it being merged.

Either way, thanks for the service — and #33 is noted: if a library of this size is simply
outside what the stopgap is meant to carry, that is a fine answer and worth stating publicly
so others stop chasing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant