Repository navigation
fix(optimum): validate referenced Optimum assemblies and surface fatal log exceptions - #661
Conversation
Translation statusen-US is the source and carries 920 keys.
Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in |
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Thanks for taking #578 on. The reference check does what a player needs most while 0.3.19 is the newest Optimum release: a patch from that overlay is rolled back instead of leaving a build that cannot enter a world. Three things have to change before this goes in. The restore change now conflicts with dev, which already does it through #662, the way Optimum's own rollback does. The version rule behind the row badge comes from a release history that did not happen. And the translation status page needs its refresh.
Line numbers are at d07333ba. S, C, R, F and V refer to probes run on made-up data and on the public v0.3.19 overlays; each result is in the table under Validation.
Blocking
1. Rebase on dev and drop the restore change
dev is now 682751bd, with #662 in it. Its restoreVanillaBuild copies .optimum/vanilla/Optimum.GameContent.dll back over the live file when Optimum backed one up, and removes the live file otherwise, before the state folder goes. That follows RestoreGameContentAsset in StratumServer/Optimum#131, whose patch copies a file already in place into .optimum/vanilla/, or writes Optimum.GameContent.dll.absent there, the first time it deploys. A restore puts back what the backup recorded, as it does for the four assemblies, and agrees with Optimum's own --rollback wherever Optimum recorded something.
This head removes the file whatever the backup says (src/ipc/optimumInstall.ts:210-212) and then deletes .optimum, the copy with it. In R1, a folder whose backup holds the file the build had before the patch (for example one placed by hand to get a 0.3.19 build into a world, then updated) ends with no Optimum.GameContent.dll and no copy of it anywhere.
Against 682751bd, src/ipc/optimumInstall.ts conflicts on the import line and in the restore body, and src/ipc/optimumOverlay.ts merges without a conflict into a second OPTIMUM_GAME_CONTENT_ASSEMBLY (src/domain/optimum/plan.ts:51 and dev's in optimumOverlay.ts). Please drop the restore hunk, OPTIMUM_DEPLOYED_ASSEMBLIES and the restore test lines, and keep one definition of each file name. The row badge needs them in the renderer, so src/domain/optimum/plan.ts is the place, with optimumOverlay.ts importing them; dev's doc comment can move along.
One test needs a change once rebased (R2). On dev the fake CLI deploys Optimum.GameContent.dll in every run that reaches the deploy step, as the patcher does from Optimum#131 on, so "rolls back when patched assemblies reference a missing Optimum assembly" gets { ok: true }. The missing-ref mode has to skip that deploy to stand for the 0.3.19 overlay.
2. The file was not introduced in 0.3.19, and the badge's version rule follows from that
src/domain/optimum/plan.ts:43, :50 and :56-57 say Optimum.GameContent.dll was introduced in 0.3.19, that 0.3.19 and newer need it, and that older releases need the contracts only. The release history says otherwise:
- No overlay up to 0.3.19 contains it.
scripts/package-overlay.shstages only the contracts at the root at v0.3.18 and v0.3.19, and the 0.3.19 manifests list no such file. It ships for the first time with the release after Optimum#131. - What needs it is the patched
Mods/VSEssentials.dll. The VSEssentials donor has carried the reference since the overlay packaging appeared in 0.3.15 (add_reference ... "Optimum.GameContent"inscripts/prepare-runtime-donors.sh), for theChunkMapLayerfield initializer. - v0.3.18 and v0.3.19 differ only in the version number, the README and the two bootstrap scripts, so the 0.3.18 overlay has the same hole.
So requiredOptimumAssemblies asks for the contracts alone on a row that is just as broken (0.3.18), and its split only matters for rows no released launcher can have. 0.3.15 to 0.3.17 published no overlay and no manifest. beta.11 asked for optimum-manifest.json, which none of the releases it could see as latest (0.3.17 to 0.3.19) publishes, so beta.12 is the first release that can install Optimum, and it came out the day after 0.3.19. A 0.3.18 row can only come from a dev build run between 29 and 30 September, and it gets no badge.
Dropping the split is less code: check both files for every Optimum row, and have the comments say that the patched VSEssentials of every overlay so far references the file, that 0.3.18 and 0.3.19 leave it out, and that it ships from the release after Optimum#131. The hook's comment ("such as Optimum.GameContent.dll in 0.3.19") can name both releases.
3. The translation status page
The two new en-US keys raise its total to 904 and the Missing count of the twelve other locales by two, and docs/contribute/translation-status.md still says 902. The status comment on this PR says so too. Run npm run i18n:status -- --write docs/contribute/translation-status.md and commit what changes.
Not blocking
The reference scan, measured
I ran findReferencedOptimumAssemblies over all 38 DLLs of the linux-x64 and win-x64 0.3.19 overlays and compared each answer with the file's AssemblyRef table read from its metadata (S1).
- On the three donors whose code is transplanted it is exact. For the VintagestoryLib and VSSurvivalMod donors it returns
Optimum.Api.Contracts.dll, and for the VSEssentials donor that andOptimum.GameContent.dll, which are their Optimum references. The API target is patched from the contracts assembly, whose own scan returns its name andOptimum.Tests.dll, butapi-patcher.csimports members from it, so what reaches the target is a reference toOptimum.Api.Contracts, which sits at the root. Both names also come back from a target that Mono.Cecil writes after importing the two references, in either order (S2). So a 0.3.19 run should be refused and a run of the next overlay, which leaves both files at the root, should pass, and since the badge does not scan, this cannot flag a working row. - It still reads strings rather than references. Across the 38 files it returns 17 names that are not references of the file they come from: namespaces (
Optimum.Bootstrap.Core.Install.dll,Optimum.Cli.dll), the file's own assembly name, PDB names from the debug directory (Optimum.Patcher.pdb.dll), andOptimum.Tests.dllfrom anInternalsVisibleToblob, where the length byte in front of the 13-byte string is 0x0D and the pattern takes it for a line break. None of these is in a file the check reads today, and for one target that comes down to a missing dot: the patcher injects five whole types intoVintagestoryLib.dllwith their namespace,Optimum(Optimum.Patcher/Program.cs:52-56,MemberInjector.InjectTypes), which the pattern skips only because no dot follows. Move them toOptimum.Runtimeand every install is rolled back asoutput-unverified. The match also swallows the separator, so a name right after anotherOptimum.name is skipped (S3); neither the donors' heaps nor the Cecil-written ones put two such names side by side. - From the release after Optimum#131 on, the CLI checks this itself, and exactly:
GamePatcherends every patch withRuntimeValidator, which loads each patched target's real references, looks for them by assembly name in the game root,Lib,Modsand the runtime folder, and fails the run withverification-failedbeforeverifyPatchedOutputruns. What the launcher adds is a guard against the 0.3.19 overlay, and a second line. If the check stays for good, reading the AssemblyRef table makes it exact without a dependency: the table stream header, the row counts, and the row sizes of the tables before 0x23, about 150 lines. If the scan stays, its comment should say what it cannot tell apart, and the doc comment ofverifyPatchedOutputand the module header still describe the hash checks only. - Cost: once per patch run, in the main process, after the CLI exits 0. Each target is streamed for its hash, then read again whole and decoded to a latin1 string, so memory peaks near twice the largest target: 20 ms and 120 MB more resident memory for a 60 MB file (C1). Small next to the run, but the note on
sha256File(optimumOverlay.ts:64) says the hash is streamed so that a large assembly is never held in memory, and the new pass holds each one whole right after. The badge reads no assembly: two IPC calls per file per Optimum row, on every mount of the page and every change of the version list or the backup set, so twice on open.
What the player is told
With the 0.3.19 overlay, every attempt now runs the whole patch and ends on "Applying Optimum didn't finish, so the original game files were put back." The run did finish; the launcher refused what it wrote, and nothing tells the player that trying again will end the same way. #578 asks for a clear reason. A log line naming the target and the missing file costs a couple of lines in verifyPatchedOutput, and a reason of its own would let the toast say that this Optimum release is missing a file. Refusing before the run would save the wait, but only with the exact table: the same scan over the staged API donor returns Optimum.Tests.dll and would refuse every overlay.
[Fatal] in the session report
- What it feeds: the mod groups and their error counts, the "Anything else" list, the verdict, the page and the clipboard text. No notification reads it; the play notification goes by exit code (
playOutcomeNotifications.ts:48). A log with[Fatal]lines and no crash file used to read "No errors were logged." and now reads "The game exited with errors.", which is the point. A crashed session keeps its verdict and now lists its[Fatal]lines as well, if it logged any. - They are drawn in amber, the warning colour:
SessionReport.tsx:53paints onlyerrorred. - "Anything else" keeps the first twelve entries (
attribution.ts:69), and a[Fatal]line tends to be the last one. With twelve unattributed warnings ahead of it, the line namingOptimum.GameContentis in neither the page nor the clipboard text, though the verdict counts it (F1). Letting fatal entries past the cap, or putting them first, keeps the one line #578 is after. - The
unattributeddoc (attribution.ts:25) still says errors and warnings.
Where the file is looked for
The root, Lib and Mods are where the game's own resolver looks (plus the data path's Mods, as read in Optimum#129) and where Optimum's validator looks (plus the runtime folder), and Optimum deploys to the root, so the rule holds. The badge only checks the root, though, and no test puts the file in Lib or Mods: probing the root alone survives the suite. Testing those two, or checking the root in both places, would make the check and the badge agree.
Strings
Both keys sit at the same place in en-US and fr-FR, and all 14 locale files round-trip identically through the Weblate writer. drafted.json is untouched, which is right for new keys, though the description says it was synced. On the wording:
- en-US: the other strings say VS Version and "put back", and Remove Optimum leaves the shaders and language lines in place, as
restoreVanillaIsPartialsays. Perhaps "This VS Version is missing a file Optimum needs, so it cannot enter a world. Remove Optimum to put the original game files back." - fr-FR: the button reads "Retirer Optimum", so "Retirez Optimum" rather than "Supprimez Optimum", and "cette VS Version" as elsewhere. Perhaps "Il manque à cette VS Version un fichier dont Optimum a besoin : elle ne peut entrer dans aucun monde. Retirez Optimum pour restaurer les fichiers de jeu d'origine."
- The explanation lives only in
title, which a keyboard never reaches, and SonarCloud flagsrole="status"on the badge (S6819).
Smaller
|| isBrokenOptimumin the Remove Optimum condition (ListVersions.tsx:228) cannot add a row, since the hook only flags rows that have a variant or a backup. Removing it survives the suite.optimumBackups: ReadonlySet<string> = new Set()(useBrokenOptimumVersions.ts:14) builds a new set on every render, so a caller that leaves it out would loop through the effect. Make it required.- SonarCloud's gate passes, with 97.8% coverage on new code and five new issues: cognitive complexity 21 in
verifyPatchedOutput(critical; the reference pass in a helper of its own fixes it), S6819 above, anawaitin a loop and two re-export notes, which go with point 1. - Every row's label lost
justify-centerandw-fulland gainedtruncate, so a long label now ends in an ellipsis. Not seen in a running launcher. Fixes #578closes the issue on merge, but the restore item came in through #662 and the dedicated-server start in the pre-release test (and CI if it fits) is not here. "Part of #578" keeps it open.
Tests
21 mutants of the new lines, each run against the eleven test files that exercise them: 12 killed, 9 survived, one of them equivalent (the || isBrokenOptimum above). The other eight: a [Fatal] line counted as a Mod's warning, the pattern's leading or trailing boundary dropped (two mutants), the root alone probed, no variant asking for the contracts only, older versions asking for both (gone with point 2), rows with a backup and no variant never checked, and a folder that cannot be read flagged. A [Fatal] line with a [modid] prefix, a backup-only row, and the real cases from S1 (a namespace, the attribute blob) would pin what matters.
Validation
At d07333ba, whose parent is 9013f93c, dev before #662:
npm ci,npm run typecheckandnpm run format:checkpass.npm run lint:cipasses with 0 errors and 12 warnings, all in files this PR does not touch.npm run test:coverage: 279 files, 5,024 tests passed, 4 skipped. 94.35% statements, 90.47% branches, 95.32% functions, 96.2% lines, above every floor.- CI is green at this head: build on Ubuntu and Windows, lint, typecheck, test, the four test-matrix jobs and sonarcloud. The macOS build was skipped.
- Against dev at
682751bd: one conflict, insrc/ipc/optimumInstall.ts. With dev's side of that file kept, typecheck passes and 136 of the 137 tests in the Optimum, game log and VS Versions files pass; the other is themissing-refcase of point 1 (R2). - Locale files: 14 of 14 identical through the Weblate writer; the status page regenerates as in point 3.
- Mutation: as under Tests, against 235 tests.
- The AssemblyRef tables in S1 come from a reader of the metadata tables that agrees with
monodis --assemblyrefon all 38 files.
| Probe | Setup | Observed |
|---|---|---|
| S1 | the scan over the 38 DLLs of both 0.3.19 overlays, against each file's AssemblyRef table | donors: their Optimum references, nothing else; elsewhere 17 names that are not references (namespaces, own names, PDB names, an attribute blob); no reference missed |
| S2 | made-up target written by Mono.Cecil 0.11.1 after importing a type from Optimum.GameContent and one from Optimum.Api.Contracts, both orders (the overlay's patcher ships 0.11.6) |
both names, either order |
| S3 | made-up bytes | \0Optimum.Api.Contracts\0Optimum.GameContent\0 gives the contracts only; a 13-byte Optimum.Tests attribute string gives Optimum.Tests.dll; a 14-byte one gives nothing; the namespace Optimum.GameContent.Worldmap gives Optimum.GameContent.Worldmap.dll |
| C1 | read whole and scan 5, 20 and 60 MB files cut from the donors | 3, 6 and 20 ms; resident memory up 10, 40 and 120 MB |
| R1 | restoreVanillaBuild at this head, .optimum/vanilla/ holding the file the build had before the patch |
{ ok: true }; live file, .optimum and the copy all gone |
| R2 | this PR applied on 682751bd, dev's optimumInstall.ts kept |
typecheck passes; the missing-ref test gets { ok: true } |
| F1 | twelve unattributed [Warning] lines, then a [Fatal] naming Optimum.GameContent |
verdict errors; the [Fatal] line is in neither the list nor the clipboard text |
| F2 | [Fatal] [madeupmod] ... with that Mod installed |
one error for that Mod |
| V1 | requiredOptimumAssemblies for 0.3.15 to 0.3.20, and for no version |
the contracts alone up to 0.3.18; both from 0.3.19, and for no version |
Not run: a real patch of a game build, the game itself, or the page in a running launcher.
Verdict: changes requested. Point 1 cannot merge as it stands and would throw away a file Optimum backed up; point 2 writes a wrong release history into the code, along with a rule that never applies; point 3 is the usual status page refresh.
d07333b to
820846e
Compare
|
Updated to address review feedback across blocking points and suggestions:
In addition:
All five repository gates pass locally (typecheck, lint:ci with 0 errors, format:check, test:coverage with 94.18% statements and 89.75% branches, build:unpack). |
820846e to
0c4d690
Compare
|
Updated to address review feedback:
Verification:
|
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Thanks for the rework. Round one's three blocking points are settled, and the new AssemblyRef reader gives the same references as monodis on all 3,757 assemblies I put through it. Two things in the new code still need a change: the sentence a player reads after the launcher rolls back a 0.3.19 run, and the server smoke, which prints PASS on the failure it was written to catch.
Line numbers are at 0c4d6904. Probe names refer to the table under Validation.
Round one
- Rebase, restore and file names (blocking 1): settled. The branch sits on dev's tip
ab5ad066,restoreVanillaBuildis dev's own, and both names live inplan.tsonly. R1 puts the backed-upOptimum.GameContent.dllback, and themissing-reftest now ends inmissing-assemblywith a rollback (R2). - Release history and version split (blocking 2): settled. The comments say what the releases did, and every row asks for both files (V1).
- Status page (blocking 3): settled. Regenerating it changes nothing.
- The reference scan: replaced by the reader, which is exact (X1 to X3). The 17 names round one found that were not references are gone.
- What the player is told: the run has a reason of its own and a log line naming the target and the file (
optimumInstall.ts:162-164). The toast is blocking point 1 below. [Fatal]past the twelve-entry cap: settled (F1).[Fatal]colour: still amber.SessionReport.tsx:53paints onlyerrorred.- The badge wording, in both languages: adopted.
|| isBrokenOptimum, thenew Set()default and "Part of #578": done.- The badge checking the root only: settled. Both sides look in the root,
LibandMods, and tests cover the last two on each side. - Tests: the survivors round one listed are pinned now (the fatal line counted for its Mod, backup-only rows, an unreadable folder, the folders probed) or went with the scan and the split.
- Sonar: under Smaller.
Blocking
1. After a rollback, the toast points at a button that is not there
describeOptimumFailure (adapters/optimum.ts:54-56) handles missing-assembly before it looks at rolledBack, so the player always reads "Remove Optimum to put the original game files back." By then the main process has rolled the run back (optimumInstall.ts:164), useOptimumActions has taken the variant off the row (useOptimumActions.ts:118) and the restore has removed .optimum, so the row offers no Remove Optimum. In T1, adding Optimum 0.3.19 to a plain 1.22.7 row ends on that sentence, with no Remove Optimum and Add Optimum 0.3.19 offered again. While 0.3.19 is the published overlay, every player who tries it will see this.
The rolled-back run needs its own sentence, one that says the files are back and that this build will fail the same way next time. The current one fits a failed rollback, where Remove Optimum is on the row. "Overlay" appears in no other player string, and the page calls Optimum's releases builds. Perhaps:
- en-US: "This Optimum build needs {{assembly}} but does not include it, so the original game files were put back. Trying again with this build ends the same way."
- fr-FR: "Ce build d'Optimum a besoin de {{assembly}} mais ne le fournit pas, les fichiers de jeu d'origine ont donc été restaurés. Réessayer avec ce build donnera le même résultat."
The test "names a missing Optimum assembly after the rollback" pins the current sentence and will need the new one.
2. The server smoke passes the failure it is there to catch
scripts/smoke/optimum-dedicated-server.mjs:158 prints PASS as soon as server-main.log holds "Entering runphase RunGame", before it looks for an Optimum load failure at :162. The log in #578 has that line first and the FileNotFoundException from WorldMapManager.OnLvlFinalize right after it. Against a made-up server that writes those two lines, or that exits right after RunGame, the script prints PASS (P1). It fails only when the exception comes without RunGame. For 0.3.19 the file check at :130 catches it first, but a later build that leaves out some other file would pass.
Either keep reading for some seconds after RunGame and fail on the pattern or on an early exit, or take the script and its section of headless-checks.md out of this PR until they have run against a real build. The PR does not say they have, and I did not run them on one.
Not blocking
The reader
Reading the table was the right move, and it holds up.
- Exact on all 38 DLLs of both 0.3.19 overlays (X1), on ten targets written by Mono.Cecil 0.11.6, the version the overlay's patcher ships (X2), and on 3,709 distinct managed assemblies from a Mono and a .NET SDK install (X3). Each time the whole AssemblyRef table was compared, name by name and in order, with
monodisand with an independent reader. The Cecil targets include the four donors written back with both references added, and two whose TypeRef, Field, MethodDef, Param, MemberRef, Constant and CustomAttribute tables go past 65,535 rows. - Malformed input (H): 0-byte, text, random, ELF and native PE files, every truncation of a 2 KB target and 5,727 of a 1.2 MB one, broken PE, CLI and stream headers, row counts of 0xFFFFFFFF and 40,000 randomly corrupted images. Nothing threw and the slowest call took 4.7 ms. A truncated file is refused or read in full, and where corruption changed the answer, the independent reader gives the same one (316 checked). A target that is empty, not an image, or a directory ends in
unverified. - One crafted shape is slow.
readStringscans to the next zero for every AssemblyRef row, so rows that all point into one longOptimum.string cost rows × length: an 8.4 MB file holds the main process for 10.5 s and returns an 8 MB name, which then goes into the log line and the toast (Q). Refusing a name longer than a kilobyte or so fixes both. Only a file the CLI itself wrote that way gets this far, since its hash is checked first. - Cost (C2): on a 61.5 MB target, reading and parsing take 13 ms and hold one copy of the file, 64 MB. The whole check, hash included, takes 76 to 128 ms. Round one's scan held about twice that.
- The tests build images with three tables, Module, Assembly and AssemblyRef, so the row sizes of the 33 tables in between are never exercised, and Sonar counts 54 of the file's 125 conditions as uncovered. Seven mutants in that arithmetic survive the suite (M): a column dropped from TypeRef, MethodDef, Constant or CustomAttribute, the string or blob width read from the wrong bit, and row counts read for the first 36 tables only. Each gives no answer or an empty one on five or six of the six target files I tried (the published VSEssentials donor, the four donors written back by Cecil, the 9 MB target), which means every real patch rolled back as
output-unverified, or a target missing its file let through. A small real assembly as a fixture, written by Cecil with populated tables and a few 4-byte columns, would pin that. A target that matches its hash but is not an image is not tested either.
Smaller
- The doc comment of
verifyPatchedOutputnow sits on thePatchedOutputVerificationtype (optimumOverlay.ts:126-136) and still says "@returns true". requiredOptimumAssembliesignores its argument (plan.ts:62), so its test passes versions nobody reads. The constant would do.- The adapter's header (
adapters/optimum.ts:5-7) says nothing there renders a string the CLI wrote. The toast now shows a name read out of a file the CLI wrote; the reader's pattern keeps it toOptimum.and[A-Za-z0-9_.-], which the comment could say. The"Optimum assembly"fallback is English inside a translated sentence and is never reached. - Fatal lines go to the front with
unshift(attribution.ts:71,:83), so two of them read newest first, and past twelve the first one is dropped, which is usually the cause (F3, F4). A Mod's own cap with a fatal line behind twelve entries is not tested. - The badge (
ListVersions.tsx:214-216):tabIndex={0}on a span trades S6819 for S6845, and focusing it shows a sighted keyboard user nothing, sincetitleonly appears on hover. The description is in the flow assr-onlytext and again througharia-describedby, so a screen reader can read it twice; withoutrole,tabIndexandaria-describedbyit is read once. On a flagged row the label also losesw-full(:206), and the parent'sjustify-centermoves label and badge to the middle of the cell, 188 px in where the other labels start at 4 px (L1).w-full min-w-0 truncatekeeps it at 4 px and still truncates a long label. - The smoke's own file check (
:130) looks at the root only and names one file. Once the server decides, it can go. - SonarCloud: the gate passes, with 81.3% coverage on new code. Seven new issues: cognitive complexity 20 in
verifyPatchedOutput, still above 15 (the nestedifin the loop keeps it there), and 16 inreadOptimumAssemblyReferences; S6845 above; a character class holdingA-Zanda-zunder theiflag; two nested ternaries;Array().
Validation
At 0c4d6904, on ab5ad066, which is still dev's tip, so there is nothing to merge in:
npm ci,npm run typecheckandnpm run format:checkpass.npm run lint:ci: 0 errors, 12 warnings, none in a file this PR touches.npm run test:coverage: 281 files, 5,062 tests passed, 4 skipped; 94.21% statements, 89.8% branches, 95.28% functions, 96.16% lines. A first run, on a busy machine, had one failure intests/ipc/compression.test.ts, which this PR does not touch; it passed alone three times and in the second full run.- CI is green at this head. The macOS build was skipped.
- Locale files: the three keys sit after
restoreVanillaIsPartialin en-US and fr-FR,drafted.jsonis untouched, and all 14 files come back identical through the Weblate writer.
| Probe | Setup | Observed |
|---|---|---|
| X1 | the reader over all 38 DLLs of both 0.3.19 overlays, whole AssemblyRef table | identical to monodis and to an independent reader, 38 of 38 |
| X2 | ten targets written by Mono.Cecil 0.11.6: a made-up target in each order, the two made-up libraries they reference, the four donors written back with both references added, and two with 70,000-row tables (9 MB, and 61.5 MB with a padding resource) | identical, 10 of 10 |
| X3 | 3,709 distinct managed assemblies from a Mono and a .NET SDK install, two of them with tables past 65,535 rows | identical, 3,709 of 3,709; slowest parse 0.41 ms |
| H | the malformed inputs listed above, 40,000 of them random | nothing thrown; slowest 4.7 ms; truncations refused or read in full; changed answers equal to the independent reader's (316 checked) |
| Q | AssemblyRef rows all pointing at one long Optimum. string |
1 MB × 1,000 rows: 0.7 s; 4 MB × 1,000: 2.7 s; 8 MB × 2,000: 10.5 s, 290 MB peak, an 8 MB name returned; through verifyPatchedOutput, missing-assembly carrying that name |
| C2 | the 61.5 MB Cecil target in a made-up game folder | read and parse 13 ms, one copy of the file held; whole check 76 to 128 ms |
| M | 30 mutants of the new lines against the 11 related test files (235 tests); the reader's mutants also run over six target files | 15 killed, 15 survived; 7 survivors wrong on 5 or 6 of the six files, 6 giving the original answers on all six |
| R1 | restoreVanillaBuild, .optimum/vanilla/ holding the file the build had before the patch |
{ ok: true }; that file is live again; contracts and .optimum gone |
| R2 | the missing-ref test on this head |
{ ok: false, reason: "missing-assembly", missingAssembly: "Optimum.GameContent.dll", rolledBack: true }, and the log line naming Mods/VSEssentials.dll |
| V1 | requiredOptimumAssemblies for 0.3.15 to 0.3.20 and for no version |
both files every time |
| F1 | twelve unattributed [Warning] lines, then a [Fatal] naming Optimum.GameContent |
verdict errors; the [Fatal] line first in the list and in the clipboard text |
| F2 | [Fatal] [madeupmod] ... with that Mod installed |
one error for that Mod |
| F3, F4 | two [Fatal] lines; thirteen of them |
newest first; the first of the thirteen dropped |
| T1 | Add Optimum 0.3.19 on a plain row, the main process answering missing-assembly with a rollback |
the toast quoted above; no Remove Optimum on the row; Add Optimum 0.3.19 offered again |
| P1 | the smoke against a made-up server: RunGame then an Optimum FileNotFoundException; RunGame then an exit; the exception alone |
PASS; PASS; failure |
| L1 | the row's markup in a headless browser, 800 px wide | plain label 4 px into its cell; flagged label 188 px into a 670 px cell; with w-full min-w-0 truncate, 4 px |
Not run: a real patch of a game build, the game, its dedicated server, or the page in a running launcher.
Verdict: changes requested. Round one's points are settled. The toast after a rollback (1) sends every player who tries 0.3.19 to a button that is not there, and the server smoke (2) cannot fail on the failure #578 asked it to catch. Both are small changes.
d8e358c to
9807d98
Compare
Patch output verification previously verified SHA-256 hashes against the overlay manifest, but did not check whether assemblies referenced by patched binaries (such as Optimum.GameContent.dll in Optimum 0.3.19+) were actually deployed to disk. When an overlay omitted an assembly, hash checking passed and produced an unrunnable build that crashed on RunGame. In addition, restoreVanillaBuild only deleted Optimum.Api.Contracts.dll, leaving newer assemblies behind. In session reports, isReportable and anyErrors only inspected error and warning severities, silently omitting [Fatal] crash lines from Logs/client-main.log and reporting a false clean status. On the VS Versions page, broken Optimum versions with missing assemblies had no indicator explaining why they could not run. This change: 1. Updates verifyPatchedOutput to scan patched target DLLs for referenced Optimum.* assemblies and verify that each exists on disk, rolling back the patch run with output-unverified if any are missing. 2. Updates restoreVanillaBuild to remove all deployed Optimum assemblies (Optimum.Api.Contracts.dll and Optimum.GameContent.dll). 3. Adds fatal severity to isReportable and anyErrors so unhandled fatal exceptions surface in session reports and error group totals. 4. Adds useBrokenOptimumVersions hook and a "Missing Optimum files" row indicator in ListVersions, keeping Remove Optimum accessible to restore the original vanilla files. 5. Adds unit and DOM regression tests in optimumPatch, optimumInstall, report, and versionsListVersions suites. Fixes #578
…k, and logging Rebase on dev and preserve existing vanilla backup files during rollback. Drop the Optimum version split so required assemblies are checked across all releases. Update translation files with refined copy and refresh the status document. In addition: 1. Parse the PE/CLI AssemblyRef table directly instead of scanning raw strings to avoid false positives from namespaces, debug records, or custom attributes. 2. Prioritize fatal crash entries in attribution so they remain visible in session reports and error groups ahead of the warning cap. 3. Improve accessibility attributes on the missing assemblies badge. 4. Add a dedicated server smoke script for release validation. Part of #578
…, and badge markup
0659640 to
1fec806
Compare
|
Updated to address review feedback from round two:
|
Addressed all feedback in commit 1fec806 (verified all CI checks green)
|
Signed commit Fatal lines retain the first cause and log order ahead of warnings in both Mod groups and the unattributed list; fatal entries now use the error color. I removed the unused version helper and unreachable English fallback, corrected the adapter documentation, extracted per-target verification, and removed the nested PE-layout ternaries. The smoke lets the server resolve its own dependencies instead of requiring GameContent at the root. Its subprocess regressions fail on an Optimum error after RunGame and on early exit, and pass only after stable startup. Timeout races no longer keep a finished smoke process alive. The rollback toast and badge changes from round two remain covered. All five local gates passed on Linux with Node 26.10.0: 5,138 tests passed, 5 skipped; coverage is 94.40% statements, 90.38% branches, 95.47% functions and 96.39% lines. Lint has 0 errors and 12 existing warnings. The packaged headless check showed the missing-file badge, enabled Remove Optimum and a 4 px label offset; placing GameContent in Lib removed the badge on a fresh process. I did not run the dedicated server against a real game build, so that pre-release check remains outstanding. |
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Thanks for the third round. Both of round two's blocking points are settled for the case they described: a rolled-back 0.3.19 run now reads right, and the server smoke fails on the #578 exception after RunGame. The reader refuses the long-name shape at once, and your Cecil fixtures kill all seven table mutants that survived last time. One small thing is worth taking before merge. The sentence written for the rolled-back run is now also what the player reads when the rollback failed and nothing was put back. That path needs the rollback itself to fail, so it is rare, and the row's badge already tells the truth there, which is why I am not holding the merge on it.
Line numbers are at ad4eeccf. Probe names refer to the table under Validation.
Earlier rounds
- Rebase, restore and file names (round one, blocking 1): still settled.
restoreVanillaBuildis dev's, both names live only inplan.ts, and R1 puts the backed-upOptimum.GameContent.dllback. - Release history and version split (round one, blocking 2): settled.
requiredOptimumAssembliesis gone and the hook reads the constant (V1). - Status page (round one, blocking 3): settled. Regenerating it changes nothing.
- Toast after a rolled-back run (round two, blocking 1): settled for that case (T1). The sentence has a new problem, under Blocking.
- Server smoke (round two, blocking 2): settled. Against made-up servers it fails on the #578 lines after RunGame, with or without the logger's prefix, on exit 134 or 0 after RunGame, and on a hang. It passes a clean run after 5.3 s and leaves no process or data folder behind (P1).
- Reader: exact on all 38 DLLs of both 0.3.19 overlays, on 14 Cecil-written targets including your four fixtures (X1), and on six PE32+ copies (X2). Malformed input is refused as before (H).
- Long-name shape: settled. The 8.4 MB file is refused in 4 ms and no name comes back. The longest name that can come back is now 1,028 characters (Q).
- Table arithmetic tested on three tables only: settled. The seven mutants that survived round two are killed (M).
- Cost: unchanged, 103 to 108 ms for the whole check on a 61.5 MB target (C2).
[Fatal]colour: settled, red, with a test.- Fatal lines newest first, first one dropped past twelve: settled. They keep log order and the first cause, in both caps (F3, F4, F5).
requiredOptimumAssembliesignoring its argument: settled, removed.- Badge: settled. No
role,tabIndexoraria-describedby, and a flagged label starts 4 px into its cell like the others (L1). verifyPatchedOutputdoc comment, adapter header, English fallback: settled.- The smoke's own file check: removed.
- Sonar: the gate passes with 89.9% coverage on new code and no new issue.
Worth taking before merge
A failed rollback reads as a successful one
describeOptimumFailure (adapters/optimum.ts:53-55) returns the new sentence for any missing-assembly that names a file, before it looks at rolledBack. When restoreVanillaBuild fails, rollBackFailedRun (optimumInstall.ts:186-189) hands the refusal back without rolledBack, and the player still reads that "the original game files were put back".
In R2b the patch leaves the 0.3.19 shape and the rollback cannot write the folder. The result is { ok: false, reason: "missing-assembly", missingAssembly: "Optimum.GameContent.dll" }, and the folder keeps the patched VintagestoryLib.dll, the contracts and .optimum. In T1b that result reaches the page: the toast says the files were put back, while the row shows Missing Optimum files and Remove Optimum, and the badge's description says to remove Optimum to put the original game files back. The comment above rollBackFailedRun (:181-183) says rolledBack is what decides which sentence is true about the folder.
Adding && failure.rolledBack to that branch is enough. A failed rollback then falls through to "Optimum couldn't patch this VS Version.", which is true, and the badge on the row says the rest. If you would rather name the file there too, that is one more pair of strings. Nothing pins this case today: the suite passes with that condition added (D2), so a test for the run without rolledBack should come with it.
Not blocking
The smoke
It does what round two asked. Three things still get through (P2), for whenever it next changes:
- Only file-load failures count. A
TypeLoadExceptionnaming an Optimum type passes while the server stays up. Matching any exception line that namesOptimum.would cover it. - Only
server-main.logis searched (:154-155). The server's own output, which the script already captures, is not, so the #578 lines printed there pass while the server stays up. - The five seconds count against the start limit, so a server that reaches RunGame in its last five seconds fails with "did not reach RunGame" (
:172).
The number of names
The length bound settles the long-name shape, but the count is the other axis. verifyOptimumReferences (optimumOverlay.ts:207-212) probes every name in all three folders at once, so a 13 MB target with 350,000 different Optimum. names holds the check for 10.5 s and takes more than 3 GB of memory (Q). Checking the names one at a time and stopping at the first missing one keeps that flat. As before, only a file the CLI wrote and recorded gets this far.
Tests
39 of 50 mutants are killed, including round two's survivors O1, T2 and X13 besides the seven table ones. Of the eleven left, X3, X4, X5, X10 and X14 give the original answers on all six target files, as in round two, and K3 is equivalent. The PE32+ branch (assemblyReferences.ts:91-92) runs in no test (E1). It reads the PE32+ copies correctly (X2), so one such fixture would be enough to pin it.
Validation
At ad4eeccf, on 86e2d4dd, which is still dev's tip, so there is nothing to merge in:
npm ci,npm run typecheckandnpm run format:checkpass.npm run lint:ci: 0 errors, 12 warnings, none in a file this PR touches.npm run test:coverage: 283 files, 5,139 tests passed, 4 skipped; 94.41% statements, 90.39% branches, 95.47% functions, 96.39% lines.- CI is green at this head. The macOS build was skipped.
- Locale files: the three keys sit after
restoreVanillaIsPartialin en-US and fr-FR,drafted.jsonis untouched, the status page is current, and all 14 files come back identical through the Weblate writer.
| Probe | Setup | Observed |
|---|---|---|
| T1 | Add Optimum 0.3.19 on a plain row, the main process answering missing-assembly with a rollback |
the new sentence; no Remove Optimum on the row; Add Optimum 0.3.19 offered again |
| T1b | the same answer without rolledBack, the folder keeping .optimum/vanilla and the contracts, as R2b leaves it |
the same sentence; Missing Optimum files and Remove Optimum on the row |
| R1 | restoreVanillaBuild, .optimum/vanilla/ holding the file the build had before the patch |
{ ok: true }; that file is live again; contracts and .optimum gone |
| R2 | the 0.3.19 shape through applyOptimumOverlay |
missing-assembly naming Optimum.GameContent.dll, rolledBack: true, a log line naming Mods/VSEssentials.dll; original files back |
| R2b | the same run, the game folder read-only when the rollback starts | missing-assembly naming the file, no rolledBack; patched VintagestoryLib.dll, contracts and .optimum still there |
| P1 | the smoke against made-up servers: the #578 lines after RunGame, bare or behind [Server Fatal]; exit 134 or 0 after RunGame; a clean run; a hang, also one ignoring SIGTERM |
failure for the first four; PASS after 5.3 s; failure at the 4 s limit, after 14.1 s with the forced kill; nothing left running |
| P2 | the same, with a crash or the #578 lines 6 s after RunGame, a TypeLoadException naming an Optimum type, the #578 lines on stderr only, RunGame 1.5 s before a 4 s limit |
PASS for the first four; "did not reach RunGame within 4000 ms" for the last |
| X1 | the reader over the 38 overlay DLLs and 14 Cecil-written targets (round two's eight, the 9 MB and 61.5 MB ones, your four fixtures), whole AssemblyRef table | identical to monodis and to an independent reader, 52 of 52; slowest 0.64 ms |
| X2 | x64 and ARM64 copies of the made-up target, the 9 MB target and your wide fixture | identical, 6 of 6 |
| H | round two's malformed inputs, 40,000 of them random | nothing thrown; slowest 4.7 ms; truncations refused or read in full; changed answers equal to the independent reader's (316 checked) |
| Q | 2,000 rows pointing at one 8 MB Optimum. name; 350,000 rows pointing at one 1,024-byte name; 350,000 different names |
refused in 4 ms, 16 ms through verifyPatchedOutput, no name; 0.7 s and a 1,028-character name; 0.16 s in the reader, 10.5 s and more than 3 GB through verifyPatchedOutput |
| C2 | the 61.5 MB Cecil target in a made-up game folder | whole check 103 to 108 ms; read and parse 23 to 28 ms, one copy of the file held |
| M | 50 mutants against the 11 related test files, the smoke ones against the smoke test; reader mutants also over six target files | 39 killed; survivors X3, X4, X5, X10, X14 (original answers on all six files), K3 (equivalent), B2 and B3 (the exact bound), E1, D2, S5 (a one-second window) |
| F1 to F5 | twelve unattributed warnings then a [Fatal]; a [Fatal] for an installed Mod; two, then thirteen [Fatal] lines; a Mod's fifteen warnings then its [Fatal] |
verdict errors and the line first in the list and the clipboard text; one error for that Mod; log order; the first twelve kept; the Mod's fatal line first |
| L1 | the row's markup in a headless browser, 800 px wide | flagged label 4 px into its cell like a plain one; a long label truncates and the badge stays in the cell |
| V1 | OPTIMUM_DEPLOYED_ASSEMBLIES |
both files; no requiredOptimumAssemblies left |
Not run: a real patch of a game build, the game, its dedicated server, or the page in a running launcher.
Verdict: approve. Everything from rounds one and two is settled. The one condition and its test above are worth taking before merge, so that a rollback that failed does not tell the player their files are back.
2 participants
Summary
Refuse an Optimum install whose patched CLR AssemblyRef entries name a missing Optimum file, roll the run back, and tell the player which file the build omits. Surface fatal log entries in session reports and flag existing Optimum rows missing required assemblies in the game root, Lib or Mods.
Assembly names are bounded to 1,024 bytes. Fatal entries retain the first cause and chronological order ahead of warnings, and use the error color. The dedicated-server smoke waits five seconds after RunGame and fails on an Optimum assembly load failure or early process exit. It lets the server resolve its dependencies and exits promptly after completion.
Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run test:coveragepasses, coverage at or above the floor invitest.config.ts.npm run build:unpackpasses.Testing
Linux, Node 26.10.0, head
ad4eeccf: all five repository gates passed. Lint reports 0 errors and 12 existing warnings in untouched files. Full coverage suite: 283 files, 5,138 passed, 5 skipped; 94.40% statements, 90.38% branches, 95.47% functions, 96.39% lines. An earlier full run failed an untouched ModDB browse-selection test; it passed in isolation on both branches and in both complete repeats.Cecil-written fixtures cover populated metadata tables, separate 4-byte string/blob heaps, widened coded indices, and row counts after AssemblyRef. Regressions cover oversized names, hash-matching non-managed targets, fatal ordering and both entry caps, and fatal rendering. Subprocess smoke tests cover stable startup, post-RunGame assembly failure, and early exit.
The packaged headless app showed the missing-file badge with Remove Optimum enabled and a label offset of 4 px. A fresh process accepted GameContent in Lib and removed the badge. These checks used fabricated files, not a patched Vintage Story build. The dedicated-server smoke has not been run against licensed game files in this pass; that pre-release check remains required. Windows was not run locally; the final-head CI matrix covers it.
Related issues
Part of #578