Bind the SQLite stack at the floor each server line ships - #339
Merged
Merged
Conversation
The archive a server installs holds meta.json and one assembly, because this project is a library and no package assembly is copied to its build output. So the Microsoft.Data.Sqlite the plugin references is a demand on the server rather than something it carries, and 0.1.0.0 demanded 9.0.11.0 while a 10.11.0 server ships 9.0.10.0. A strong-named assembly found below the referenced version is not found at all, so that server started the plugin, failed to open the store, created no data folder and stored no play, while build.yaml went on offering the archive to it. Issue #330. The two floors are now declared once, in Directory.Build.props beside the Jellyfin ones, with the reading that produced them: I copied Microsoft.Data.Sqlite.dll out of four server images and read its assembly version, and 10.11.0 gives 9.0.10.0 and 12.0-rc1 gives 10.0.9.0. The plugin references them with ExcludeAssets runtime, so the project file now reads the way it behaves, and no version is written in a project file. The suite gains the two references as well. Nothing reached it through the project reference once the plugin excluded the runtime asset, and every store test failed to load the assembly until those lines existed. It takes the floor rather than a later patch, unlike the Jellyfin references above it, because nothing in the fakes stops it: the store is exercised against exactly the assembly a floor server supplies. What refuses a repeat, and each was proved by putting the mistake back: - ThePluginBindsTheSqliteTheFloorServerShips reads the reference out of the built assembly and compares it against the table. Built with the floor raised one patch, it alone goes red, expected 9.0.10 and actual 9.0.11. - EverySqliteFloorInTheTableIsTheFloorTheBuildUses compares the new column of docs/support-matrix.md against Directory.Build.props. With the column moved to 9.0.11 and the property left alone, both of those checks go red and the other five pass. - The ABI floor job reads the resolved Microsoft.Data.Sqlite out of packages.lock.json against a floor of its own, the way it already reads the two Jellyfin packages. Against a lock file saying 9.0.11 it prints FAIL and exits 1. The SQLitePCLRaw.lib.e_sqlite3 reference stays for the advisory and its comment now says it ships nothing, since the native library that runs is the server's. I ran the invariant scan over the tree at this commit: 21 rule(s) checked, no match. Signed-off-by: Nils Lehnen <30603423+iderex@users.noreply.github.com>
iderex
added
bug
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
2 participants
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.
Closes #330.
What was wrong
The archive a server installs holds
meta.jsonand one assembly. That is not apackaging choice:
Jellyfin.Plugin.Stats.csprojis a library, so no packageassembly is copied to its build output at all, and the output is what the
packaging tool zips.
So the
Microsoft.Data.Sqlitethe assembly binds is a demand on the server, andmasterdemanded9.0.11.0while the floor server of the 10.11 line ships9.0.10.0. A strong-named assembly found below the referenced version is notfound at all, so the plugin loads, the store never opens, no data folder is
created and no play is stored - on the one server the manifest's
targetAbi 10.11.0.0promises the loudest.The floors, read rather than recalled
I copied
Microsoft.Data.Sqlite.dllout of four server images and read itsassembly version:
SqliteFloorNet9is9.0.10andSqliteFloorNet10is10.0.9, declared oncein
Directory.Build.propsbeside the two Jellyfin floors with that reading nextto them. Both project files read the properties and neither names a version of
its own. The plugin references the package with
ExcludeAssetsruntime, so theproject file now reads the way it already behaved.
Done-when, one at a time
The assembly references of the built package. Read out of the assembly
inside the zip
jprmproduced, not out of a build tree:and the net10.0 build of the same commit:
refs.csis fifteen lines overSystem.Reflection.Metadatathat print everyassembly reference of a file; nothing about it is in this tree.
The package installed on a
jellyfin/jellyfin:10.11.0server. The zipunpacked into
/config/plugins/Jellyfin.Plugin.Stats, a fresh config and cache,one generated video in
/media, and the reading drove the server over its ownAPI: wizard, sign-in, add the folder as a library, report a play, read it back.
The data folder:
The log:
No
[ERR]line at all, and the grep for the failing line counts zero ratherthan being left unrun.
The same reading against the version that was wrong. I did not want to
inherit the failure from the issue, so I built the identical assembly with the
floor raised back to
9.0.11and gave it a second fresh 10.11.0 server. Sameimage, same media, same script:
No
plays.db, the play answered 503, and the plugin loaded cleanly right up tothe moment the store was touched. That is the failure this change removes, on
this machine rather than on somebody's report of it.
The suite compares the floor against the assembly. Two new checks, and I
proved each by putting the mistake back.
ThePluginBindsTheSqliteTheFloorServerShipsreads the reference out of thebuilt assembly. Built one patch above the floor, with the table and the property
untouched, it alone goes red:
EverySqliteFloorInTheTableIsTheFloorTheBuildUsescompares the new column ofdocs/support-matrix.mdagainstDirectory.Build.props. With the column movedto
9.0.11and the property left alone, both new checks bite and the other fivepass:
The ABI floor job. It now reads the resolved
Microsoft.Data.Sqliteout ofpackages.lock.jsonagainst a floor of its own, the way it already reads thetwo Jellyfin packages. Run at this commit:
and against a lock file whose resolved SQLite is one patch higher:
floorcheck.pyis that step's own body, cut out ofbuild.yamlby a scriptrather than retyped.
The floor job going red on a runner.
scratch/330-the-floor-job-goes-redcarries this branch with one difference: the SQLite reference is a literal
9.0.11in the project file again,Directory.Build.propsand the table leftalone, the lockfile regenerated so the restore reaches the comparison. Run
33966016363, dispatched on that ref:
Two things to read out of that. The 12.0 leg stayed green, so the refusal is the
line that broke rather than the job, and the near miss is one patch rather than a
whole major. And
call / buildwent red beside it for a second reason nobodydesigned: with the plugin raised and the suite still at the floor, the suite's
reference is a downgrade and
NU1605refuses it.That is the suite reference earning its place a second time: it also pins the
plugin down to the floor from below.
That branch is not for merging and is left where it is; its head is the evidence.
The suite gained the two references, and that is a scope line the issue does not carry
Jellyfin.Plugin.Stats.Tests.csprojis the one file I touched that the issue'sScope:does not name. It is not optional and it is not a second topic: once theplugin excludes the runtime asset, nothing reaches the suite through the project
reference, and 300 store tests failed to load
Microsoft.Data.Sqlitebeforethose two lines existed. It is the same shape the file already carries for
Jellyfin.ControllerandJellyfin.Modeldirectly above it.It differs from those in one way worth stating. The suite cannot follow the
plugin down to the Jellyfin floor, because
FakeUserManagerdoes not compileagainst
IUserManageron 10.11.0. Nothing stops it here, so it takes the floorpackage: the store is exercised against exactly the assembly a floor server
supplies, rather than against something newer that happens to satisfy the same
reference.
The means
No new means. This is MSBuild properties, a
PackageReference, a step in aworkflow that already exists and xUnit checks in the suite that already holds
this table - every one of them a thing this tree already carries, and every claim
above is backed by a command run at this commit. Adding a version to a project
file, which is the alternative, is the shape that produced the defect.
What is green
The suite was 1048 and 1049 before this; the two added checks are the difference.
No second reader
Nobody else has read this branch. The evidence above stands in place of a second
reader and does not replace one, and every number in it carries the command that
produced it so that a reader can disagree with the reading rather than with me.