Repository navigation
Give mod detail lookups catalog timeout and track update check failures - #658
Conversation
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed at 51e8442, which sits on dev's tip. The scan now tells a failed lookup from a Mod ModDB does not know, the new rule stays on /api/mod and /api/mod/<id>, and the rewritten comment in useModSuggestions.ts matches what ModSuggestions.tsx:153-154 renders. But in the case #618 describes, the lookup is still cut at 15 s, and the notice still says 1 Mod when two have updates. Two things to settle before this can close the issue.
Blocking
-
The 90 s ceiling never reaches a lookup that is still waiting.
requestBoundedBufferarms its inactivity window when the request is created (network.ts:187), resets it only when a chunk arrives (:202, throughcollectBoundedat:125-126), and caps it atMath.min(REQUEST_TIMEOUT_MS, timeoutMs), so never more than 15 s whatever the rule says (:159). The comment above that line already says a widened ceiling "still gets cut after 15s of dead air", and the description's "15-second inactivity resets per chunk" is the same window. A lookup starved behind the catalog transfer receives nothing at all, so for that window it is dead air from the start. Driving the realqueryUrlwith a fakenet.requestwhose response and chunks land on a fixed schedule, dev's tip against this head:/api/mod/123, what arrivesdev this head nothing for 16, 20, 30 or 60 s, then the whole body cut at 15 s cut at 15 s headers at once, first byte at 20 s cut at 15 s cut at 15 s first byte at 5 s, then a chunk every 10 s until 45 s cut at 15 s answered at 45 s nothing, ever cut at 15 s cut at 15 s a byte every 10 s, never ending cut at 15 s cut at 90 s The first row is the wait #618 points at (
network.ts:171-187). What the rule saves is a lookup whose first byte comes within 15 s and whose body then takes longer, which is the shape of the new test: its first chunk lands at 10 s. I tried arming the window on theresponseevent instead of at creation. The 16 to 60 s rows then answer,/api/tagsstill stops at 15 s, and the whole suite passes. The price is that a request that never answers waits for its rule's whole ceiling: 90 s instead of 15 s for a detail lookup, and a catalog request that never answers falls back to its cached copy 75 s later than today. The issue also lists options that go after the overlap itself and leave the timers alone. Whichever you pick, a test where nothing arrives for 20 s before the body would hold it; on this head it fails. -
The notice still undercounts, and still says nothing. The failure count reaches
GlobalModUpdateChecker, where it only decides whether the installation is recorded (GlobalModUpdateChecker.tsx:28-30). The notice is the same. With the real checker over the real scan, three installed Mods, two of them with an update and the lookup of one of those two failing, the first notice reads "The selected Installation has 1 Mod with an update available." on dev and on this head. #618's "Why it matters" names both halves: the count can be short, and nothing tells the player a lookup failed. Either let the notice say the count may be short whenfailedLookups > 0(new strings in en-US and fr-FR only), or change "Closes #618" to "Part of #618" so the issue stays open for it.
Not blocking
-
The guard changes the revisit, and only within one launch.
_notifiedModUpdatesInstallationsnever reaches disk (configReducer.ts:322-327), so dev already checks again at every launch, and "permanently suppressing future notifications" in the description is not what dev does. With the same setup, leaving the installation and coming back: when the failed lookup succeeds on the second check, dev keeps the single "1 Mod" notice and this head adds "2 Mods", which is the gain. When it fails again, this head posts "1 Mod" a second time. When the failed lookup was the Mod that has no update, the first notice was already right ("2 Mods") and this head posts it again, word for word. And when every lookup fails, or both updatable ones do, the first check shows nothing on either side. Once the notice can say it may be short, a repeat carries news. Today it can be the same sentence twice. -
The failure count is not pinned. Counting failures per installed file instead of per id, or capping the count at 1, leaves all 5017 tests green. Nothing reads the number beyond zero yet, so it starts to matter when the notice uses it. A test with two failing ids, one of them installed twice, expecting
onFinish(0, 2), catches both; I checked. -
One value, two ways to call
onFinish. Theif/elseatuseGetCompleteInstalledMods.ts:110-113and the= 0default atGlobalModUpdateChecker.tsx:26exist so thattoHaveBeenCalledWith(1)(useGetCompleteInstalledMods.test.tsx:143) keeps passing: callingonFinish(availableModUpdates, failedLookups)every time fails that assertion and nothing else in the suite. Passing both numbers always and updating that line is shorter. -
Comments that still say catalog only.
getApiUrlTimeoutMs's doc says no rule but the mods/authors catalog sets a timeout (validation.ts:486), the comment overMODS_CATALOG_TIMEOUT_MSspeaks of "the same two endpoints" (:47), andnetwork.ts:53-57and:155-156name only the catalog. The constant's name now covers detail lookups too.
Validation
Timers. The table comes from a throwaway test file that runs the real queryUrl, requestBoundedBuffer and rules under fake timers, the way moddb-slow-transfer-timeout.test.ts does, on dev's tip and on this head. The catalog itself, waiting 20 s, is cut at 15 s on both sides too, despite its 90 s rule. This stands in for the network at net.request: it shows what each timer does with a given arrival schedule, not how long the real wait behind the catalog lasts.
Where the rule applies. pathMatches stops at a path segment (validation.ts:444-447), so the rule takes /api/mod and /api/mod/<id> and nothing else. /api/mods and /api/authors still match the rule above it, /api/tags keeps 15 s, and the byte ceiling stays at the generic 4 MB (api-url-ceiling.test.ts:20). Downloads have their own rules and worker. Every /api/mod/<id> lookup through QUERY_URL gets the 90 s, not only the scan's: the Mods page rows, the release lists, installing the newest release, the modpack import and the suggestions. The ModDB listing count's own detail request (netHandlers.ts:127-128) does not read the rule and keeps 15 s.
Concurrency. No new fan-out: the scan still runs two lookups at a time through installedModLookups, a share it has with the Mods page rows. Opening the Mods page also asks for the grid's catalog, the suggestions' copy of it when suggestions are on, and the three filter lists, /authors among them at about 4 MB as well (validation.ts:43-45). With the scan's two lookups that is seven requests for QUERY_URL's six slots, so one can wait in the launcher's own queue, which does not count against its timer (netHandlers.ts:52-53). A detail lookup can now keep its slot for 90 s instead of 15 s, and only while bytes keep coming.
Scan and notice. The real GlobalModUpdateChecker over the real useGetCompleteInstalledMods, only the bridge faked, in six cases: all lookups succeed, one updatable Mod's lookup fails once, it fails twice, the current Mod's lookup fails, both updatable ones fail, all fail. A failing lookup rejects the way a timed-out QUERY_URL does. Each case runs a first check and a revisit, on dev and on this head, and gives the outcomes quoted above. useGetCompleteInstalledMods.ts has every line and all 24 branches covered. Both probe files are ready to share.
Mutations. 17 changes to the rule, the scan's count and the checker's guard, run against the four test files the PR touches and five neighbours (155 tests). 15 are caught, among them the rule removed, placed after /api, at 40 s, given the catalog's byte ceiling or widened to all of /api, a failed lookup counted as fine, a found or not-found one counted as failed, the count never handed over, and the guard removed, inverted or defaulting to a failure. The two left are in Not blocking 2, and both survive the whole suite.
Gates and CI. npm ci, typecheck, lint:ci (0 errors, the 12 known warnings), format:check and test:coverage pass: 279 files, 5017 passed, 4 skipped, 94.35 / 90.47 / 95.3 / 96.19. The first run here failed one unrelated test, "compresses a source past the old 2 GiB archive ceiling", for lack of free space in the temp folder; it passes with TMPDIR pointed at a folder with room. CI is green on this head, with the macOS build skipped. The branch sits on dev's tip, so there is nothing to merge. No locale file changes: the status page is already up to date, and the Weblate writer round trip gives identical files for all 14 locales.
Not run. The game, a packaged build, Windows and macOS by hand, a real slow connection.
Request changes. The groundwork is right: the scan knows a failed lookup from an unknown Mod, and the record is no longer taken from a short count. But in #618's own case the lookup is still cut at 15 s, and the player still reads "1 Mod" with nothing saying a lookup failed.
Translation statusen-US is the source and carries 911 keys.
Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in |
|
Addressed Pixnop's review feedback in
|
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed again at 3945159. Both blocking points from the first round are settled: a lookup that receives nothing for 16 to 60 s and then its answer is no longer cut, and the notice now says when its count may be short. Arming the window on the response costs what the first round said it would; the numbers are below, and I think a follow-up can deal with them. One new thing needs a change before merge: with no network, every launch and every switch of installation now ends in a banner that stays until dismissed, and it opens with a count the scan does not have.
First round
- Blocking 1, the ceiling never reached a waiting lookup: settled. With the same fake
net.requestas last time,/api/mod/123and/api/modsthat receive nothing for 16, 20, 30 or 60 s and then their body are answered at 16, 20, 30 and 60 s (cut at 15 s on dev),/api/tagsandapi.vintagestory.at/stable.jsonare still cut at 15 s in every row, and the new test (lets /api/mod/123 wait more than 15s for response headers) fails on the first round's code. What still cuts a lookup at 15 s is a response that has started and then sends nothing for 15 s, which is what the window is for. - Blocking 2, the notice undercounted and said nothing: settled. In #618's case (three Mods, two with an update, the lookup of one of those two failing) the first notice now reads "The selected Installation has 1 Mod with an update available. Some installed Mods could not be checked, so more updates may be available.", and the revisit adds "2 Mods" once the lookup works.
- Not blocking 1, the revisit: settled. A repeat of an incomplete check now says it is incomplete, and a check that becomes complete says so: "2 Mods ... could not be checked", then "2 Mods with updates available." Where the repeats become a problem is under Blocking.
- Not blocking 2, the count: settled. The new per-id test catches both changes that survived last time, counting per installed file and capping at 1.
- Not blocking 3, one way to call
onFinish: settled (useGetCompleteInstalledMods.ts:110, the default gone, the test expects(1, 0)). - Not blocking 4, comments: settled (
network.ts:52-58,:156-159,validation.ts:47-51,:484). One sentence kept a claim that was already off on dev:validation.ts:51calls 90 s "six times the generic timeout, matching the catalog byte-ceiling headroom", but 16 MB is four times the generic 4 MB.
Blocking
- With no network, every launch and every switch ends in a banner that stays. The checker now posts when nothing was found but a lookup failed (
GlobalModUpdateChecker.tsx:27), and with no network every lookup fails. The banner carries "View updates", so it has no timeout (NotificationsContext.tsx:119) and is never folded into an identical one (duplicateToast.ts:36), and since an incomplete check is not recorded, each switch posts another. With the real checker over the real scan, three installed Mods and every QUERY_URL rejecting at once: one banner after the first check, still there 10 s later, three after leaving the installation and coming back twice. Dev shows none. Each reads "The selected Installation has 0 Mods with updates available. Some installed Mods could not be checked, so more updates may be available.", so it opens with a count the scan does not have. Online, a Mod whose lookup keeps failing does the same whenever nothing else has an update, since any answer but a clean 404 counts as failed (useQueryMod.ts:41). The smallest fix I can see: whenupdates === 0, a sentence with no count and no action, something like "Some installed Mods could not be checked for updates.", which then times out like any info banner and folds into a twin still on screen. That is one new key in en-US and fr-FR, and the "0 Mods" assertion in the new checker test moves to it. I tried it with a plain string in place of the key: the banner is gone 10 s later, two switches leave one banner marked as shown twice, and that assertion is the only test that fails.
Not blocking
-
What arming on the response costs. Measured with ModDB taking every request and never answering (no response, no byte, no error), the real
queryUrland its six slots, dev's tip against this head:ModDB takes the request and never answers dev this head one detail lookup fails at 15 s fails at 90 s /api/mods, cached copy servedat 15 s at 90 s /api/authorsfails at 15 s fails at 90 s /api/tags,/api/gameversionsfail at 15 s fail at 15 s scan of 10 Mods, two lanes done at 75 s done at 450 s scan of 40 Mods done at 300 s done at 1800 s modpack import, 30 lookups done at 75 s done at 450 s stable.json, asked 1 s into that importanswered at 75 s answered at 450 s Mods page opened at launch, install popup at 10 s fails at 30 s fails at 105 s A single request now waits its rule's 90 s, as the catalog already could for a slow transfer, and every other rule keeps 15 s. The serial paths multiply it. A first visit to Manage Mods shows only its spinner until the whole scan is done (
NoInstalledModsNotice.tsx:14-16), the modpack import keeps Import disabled on "Checking the mod database..." (ImportModpackPopup.tsx:368), and the six slots are shared across hosts and served in order, so Add Version's list waits behind the import's lookups. A refused connection or an offline machine still fails at once: only a server that takes the request and sends nothing pays this, and in that case dev gives the same empty result, only sooner. So I would not hold the PR on it, but it deserves a follow-up issue. The bound belongs with the callers that ask in series, the scan and the import (for instance, stop asking once a lookup has timed out), rather than in the timer, where a shorter wait would give part of #618 back. -
Two behaviours nothing pins. Arming the window on the first byte instead of the headers (a response that starts and then sends nothing would only be cut at 90 s), and posting the incomplete sentence for a complete check, both leave the whole suite green. A test with headers at once and nothing after, expecting
/api/mod/123cut at 15 s, holds the first. Asserting innotifies once per installation and does not renotify on a revisitthat "could not be checked" is absent holds the second.
Validation
Timers. A throwaway test file drives the real queryUrl, requestBoundedBuffer and rules under fake timers, with the catalog cache stubbed: four paths (/api/mod/123, /api/mods, /api/tags, api.vintagestory.at/stable.json) by ten arrival schedules, the first round's among them, on dev's tip and on this head. Only the two widened rules change. Every other caller of requestBoundedBuffer has a ceiling of 15 s or less, and its overall timer always fires before the window could, so arming the window later changes nothing for them. The cost table comes from a second file where ModDB never answers and every other host answers at once, with the scan's two lanes rebuilt from the same ConcurrencyLimiter.
Scan and notice. The real GlobalModUpdateChecker over the real scan, only the bridge faked, the six first-round cases with a revisit. On this head: all lookups succeed, "2 Mods" once, as on dev. One updatable Mod's lookup fails once: the incomplete "1 Mod", then "2 Mods". It fails twice: the incomplete "1 Mod" twice. The current Mod's lookup fails: the incomplete "2 Mods", then "2 Mods". Both updatable lookups fail: the incomplete "0 Mods", then "2 Mods". All fail: the incomplete "0 Mods" twice, where dev shows nothing.
Mutations. 12 changes to the new commit and to the scan's count, run against the four test files the PR touches and five neighbours (158 tests). 9 are caught there, among them the window armed at creation again, the detail rule removed, no notice when only lookups failed, the incomplete sentence never used, an incomplete check recorded, and the two that survived last time. The overall timer moved to the response is caught by the whole suite (fetchReleaseNotes.test.ts). The two in Not blocking 2 survive the whole suite.
Gates and CI. npm ci, typecheck, lint:ci (0 errors, the 12 known warnings), format:check and test:coverage pass: 279 files, 5022 passed, 4 skipped, 94.34 / 90.43 / 95.3 / 96.18, with the temp folder on disk as last time. CI is green on this head, the macOS build skipped. The branch is one commit behind dev's tip (682751b, Optimum files only) and merges cleanly. Only en-US and fr-FR change, at the same position, drafted.json is untouched, the status page is up to date, and the Weblate writer round trip gives identical files for all 14 locales.
Not run. The game, a packaged build, Windows and macOS by hand, a real slow or silent ModDB, a real launch without network.
Request changes, for the banner a launch without network now gets. Everything the first round asked for is in: #618's lookup waits for its answer, and the notice says when its count may be short.
3945159 to
8db6ed5
Compare
|
Updated to address review feedback:
Verification:
|
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed again at 8db6ed5, which sits on dev's tip (ab5ad06). The second round's blocking point is settled: with no network, the update check now ends in a short notice with no count and no action, which leaves after 4.5 s and folds into a twin still on screen. Both test gaps are closed and the validation.ts comment is right. CI is red on this head, but on a Windows test this PR does not reach, so it needs a re-run rather than a change.
Second round
- Blocking 1, the banner a launch without network got: settled. Same probe as last time, every QUERY_URL rejecting at once: one banner after the first check, "Some installed Mods could not be checked for updates.", with no View updates and the 4.5 s info timeout; nothing left 10 s later; two switches leave one banner marked x2. Online, with one Mod answered by anything but a clean 404 and nothing else to update, the same. Dev shows none in either case.
- Not blocking 1, what arming on the response costs: unchanged. The cost table comes out the same row for row (a detail lookup ModDB never answers fails at 90 s instead of 15 s, a scan of 10 Mods ends at 450 s instead of 75 s). No issue tracks it yet.
- Not blocking 2, two behaviours nothing pinned: settled. Arming the window on the first byte now fails
cuts /api/mod/123 at 15s when headers arrive at once and nothing follows, and the incomplete sentence posted for a complete check failsnotifies once per installation and does not renotify on a revisit. - The
validation.ts:51comment: settled. It now says "providing headroom for slow catalog transfers", and 90 s is six times 15 s.
The first round's answers still hold. The timer table gives the same 40 rows as last time on both sides: /api/mod/123 and /api/mods that receive nothing for 16 to 60 s are answered (cut at 15 s on dev), and /api/tags and stable.json are still cut at 15 s. The six checker cases with a revisit give what they gave last time, except the two where the scan finds nothing. Both updatable lookups failing now gives the no-count notice, then "2 Mods". Every lookup failing gives the no-count notice, and the revisit folds into it, where dev shows nothing.
CI
test-matrix (windows-latest, 22) failed one test, and the test gate with it: in importModpackPopup.test.tsx, "labels a resolved row with the mod database name and keeps the modid as a second line" found the row still reading "Pending" where it expects "New install". Nothing this PR changes runs there. The test fakes the bridge, and the popup, useQueryMod and the domain code they use are untouched; the other three legs passed the same suite. The manifest entry in that test already carries the name rowFor looks for (name: "Traders Expansion", line 82), so the row can be found before any lookup answers, and the lines after it check the status synchronously. With the fake bridge answering 200 ms late, the test fails the same way on dev. A re-run of the job should clear it, and the race deserves its own issue.
Not blocking
- Two things the new branch does that nothing pins. That the no-count notice leaves by itself: posting it with
{ duration: null }keeps the whole suite green, and the banner would stay until closed, as it did last round. And that a check which found nothing but missed some Mods is not recorded: adding|| updates === 0to the guard atGlobalModUpdateChecker.tsx:28keeps it green too, and a player who launched offline would then hear nothing about updates on a later visit in the same session. Extendingreports an incomplete scan even when no update was confirmedholds both: wait until the sentence is gone (waitFor, 6 s), then leave and revisit the installation and expect it back. I checked: it passes here and fails under each of the two changes. - The French sentence reads as translated. "n'ont pas pu être vérifiés pour les mises à jour" follows "checked for updates" word for word. "Les mises à jour de certains mods installés n'ont pas pu être vérifiées." says the same thing the way the neighbouring strings do.
Validation
Banner. The real GlobalModUpdateChecker over the real scan, with the real notifications provider and overlay and only the bridge faked; three installed Mods. Offline is every QUERY_URL rejecting at once. Online is one Mod answered with statuscode "500", which the scan counts as failed. With the two switches 10 s apart instead of 3 s, each notice has gone before the next one arrives, so none is folded and the Activity Center ends with three rows, one per check, as for any other info notice.
Mutations. 17 changes, run against the four test files the PR touches and five neighbours (159 tests): the two that survived last time, four to the new no-count branch, the window left uncapped for the widened rules, and last round's other ten. 14 are caught there. The overall timer moved to the response is caught by the whole suite (fetchReleaseNotes.test.ts), and the two in Not blocking 1 survive it.
Gates. npm ci, typecheck, lint:ci (0 errors, the 12 known warnings), format:check and test:coverage pass: 280 files, 5050 passed, 4 skipped, 94.38 / 90.48 / 95.35 / 96.22. The branch sits on dev's tip, so there is nothing to merge. Only en-US and fr-FR change, at the same position (after updatesAvailableInstallationIncomplete_other), drafted.json is untouched, the status page is up to date, and the Weblate writer round trip gives identical files for all 14 locales.
Not run. The game, a packaged build, Windows and macOS by hand, a real slow or silent ModDB, a real launch without network.
Approve. Before merging, re-run the Windows Node 22 job.
Opening the Mods page shortly after launch triggers the catalog download while the deferred update check runs mod detail lookups concurrently. The generic 15s API timeout cut mod detail lookups starved behind the catalog download on slow connections, and failed lookups were collapsed into not-found, causing GlobalModUpdateChecker to falsely record the installation as notified with an undercounted update total. Give /api/mod lookups the catalog 90s timeout allowance in API_URL_RULES, distinguish failed lookups from missing mods in useGetCompleteInstalledMods, and refrain from dispatching ADD_NOTIFIED_MOD_UPDATE in GlobalModUpdateChecker when lookups fail. Also update the stale unmount comment on the folded state in useModSuggestions.
Drop the action button from the notification when zero updates are confirmed with failed lookups, allowing the notification to timeout and fold into duplicate notifications instead of persisting indefinitely. Add updatesCheckFailedSome localization strings for en-US and fr-FR, update translation status, refine the timeout comment in validation.ts, and add a test verifying that /api/mod/123 cuts off at 15s when headers arrive immediately and no body data follows.
8db6ed5 to
e9e67fe
Compare
2 participants
Summary
Mod detail requests can time out while waiting behind large catalog downloads. Failed lookups were also treated like missing mods, which could hide available updates for that scan.
Changes
/api/mod/<id>requests the 90-second ModDB API ceiling. The 15-second inactivity timeout begins after response headers and resets as data arrives, while the overall timeout still bounds the request.Type
Verification
Ran the RiftLauncher gates on Linux x64:
npm run typecheck: passed across all TypeScript configurations.npm run lint:ci: passed with 0 errors and 12 existing warnings.npm run format:check: passed.npm run test:coverage: passed, 94.35% statements, 90.46% branches, 95.30% functions, 96.19% lines; 5 tests skipped.npm run build:unpack: passed.Closes #618