test(kite3d): pin the manifest hash as an If-Match token - #22
Draft
pythonlearner1025 wants to merge 1 commit into
Draft
pythonlearner1025 wants to merge 1 commit into
pythonlearner1025 wants to merge 1 commit into
Conversation
The plan was to delete the whole tree hash at startup. It cannot go. The editor seeds If-Match from the manifest row for files it writes but never reads. A scene save proves it. After a page reload the editor sends If-Match eb2b94df155c for assets/main.scene.bin, which is exactly the manifest sha256 for that path. The PUT route compares it against a fresh hash of the file. Publishing a size and mtime revision instead would 412 every scene save. Skipping the seed would send If-Match: * and overwrite a file another writer had changed. This commit deletes nothing. It adds the tripwire and writes the reason next to the field, so the next attempt fails loudly. The lazy seed half of the plan is not the blocker. Measured on 300 files rewritten with identical bytes: the seed gives 0 editor reloads, a lazy seed gives 300 on the first build and 0 on the second. That matches the stated bar. It saves nothing though, because the startup walk still has to hash every file to fill the manifest rows. Verified: npm run build, npm run typecheck, npm run lint, test:kite3d (12 passed), test:runtime (1 passed), test:editor (8 passed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T623SnndzSVwCSmuQrVRj2
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
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.
What part this touches
GET /api/filesreturns the project manifest. Every row carries a path, a size, a sha256 and an mtime.The brief was to delete the whole tree hash at startup, which still costs about 2 seconds on the owner's project:
The plan had two halves. Publish a size and mtime revision for
?v=instead of a content hash. Seed the no-op event filter lazily.I was asked to verify one thing before taking either: does the editor seed
If-Matchfrom the manifest for a file it has not read yet.It does. That kills the first half, and the first half is what carried the saving.
The problem
Not a bug. A constraint that was invisible in the code, and is now written down.
ViewerInstanceManager.replaceManifestseeds a hash for every row it receives:writeSerializedScenethen uses that map as its write precondition:serialized.filesare the buffers and images thatserializeSceneGltfDocumentextracts out of the exported glTF.assets/main.scene.binis one. The editor writes them on every scene save. It never reads them, because it loads the scene throughviewer.load(fileUrl(...)), which is a plain GET and records no ETag.So after a page reload, the manifest is the only place that hash can come from.
I ran it rather than argued it. A real editor, headless, on a real dev server. Save the scene, reload the page, save again, and log the
If-Matchheader on every PUT next to the manifest row for that path:The server compares that token against a fresh hash of the file on
PUT. So the manifest row has to be a real content hash.Publishing a size and mtime revision there gives one of two outcomes. The PUT route cannot match it, so every scene save returns 412. Or the editor stops seeding from the manifest, the write falls back to
If-Match: *, and a scene save silently overwrites a buffer another writer had changed.The second half of the plan, lazy seeding, is fine on its own terms. It just saves nothing. The startup walk must still hash every file to fill the manifest rows, so the seed comes out of that walk for free.
The fix
There is no fix here, because there is nothing safe to delete. There is a tripwire and a written reason.
The test pins the contract. The manifest sha256 must work as an
If-Matchtoken, and a stale one must be refused.The comment puts the reason on the field itself, so the next reader does not have to trace it through two packages.
Nothing else changed. 26 lines added, 0 removed, 0 behaviour changed.
The risk trade
Doing nothing keeps the 2 seconds. I take that over a silent overwrite on scene save.
What it would actually take to remove the startup hash, so the owner can price it:
The editor must stop treating the manifest as an
If-Matchsource.writeSerializedScenewould read each companion file it has no hash for, to get a real ETag first. That is a read per extracted buffer per save.replaceManifestmust stop seedinghashesfrom manifest rows, or it will keep overwriting good ETags with revision tokens.The server can then publish a size and mtime revision for
?v=, seed the no-op filter lazily, and make the startup walk stat only.That is four coordinated changes across two packages, and it changes what a failed precondition means on a scene save. It is a redesign of the revision token, not a deletion, so I left it out as instructed.
I also looked for a smaller deletion and rejected the one I found.
GET /files/*hashes the file on every request to build its ETag, and the manifest already holds that hash. Serving the ETag from memory would make a missed watcher event serve stale bytes to the browser behind a 304. A stale listing is recoverable. Stale content behind a correct-looking ETag is not.Tests
Suites run, mirroring
.github/workflows/ci.yml:One test is new, in
packages/kite3d/test/server.test.ts:accepts the manifest hash as an If-Match precondition and rejects a stale oneMaking the manifest publish
${size}-${mtimeMs}instead of a content hash fails it, which is the whole point:It also fails the add and delete test from #21, which checks the row against a known sha256:
This test failed correctly on its first version. No rewrite was needed.
The reload storm, measured as asked. 300 files rewritten with identical bytes, counting the change events an editor would receive, twice in a row on one server:
The
lazy-seedrow is the proposed change, simulated by removing the startup seed and rebuilding. The storm is one time, not per build, so it clears the stated bar. It is still worth nothing, for the reason above.Speed, unchanged, and unchanged by construction. The diff adds a comment and a test and touches no execution path:
Startup is the 2 seconds this PR failed to remove.
/api/filesstays where #21 left it. Play was not re-measured, because no code on that path changed, and reporting two identical numbers as a before and after would be theatre.Edges checked:
A first ever write still works. The
.bindid not exist on the first save, so the editor sentIf-Match: *and created it. That is in the evidence above..kite3dwrites are unaffected. They are not in the manifest after #21, and they re-read the file for their own precondition.Publish is unaffected. It hashes its own set in
packages/kite3d/src/manifest.ts.Deploy
The
kite3dnpm package, with #20 and #21. This commit changes no shipped behaviour, so it needs no separate release.No editor bundle change. No engine change. No backend change. No migration.
Merge #20, then #21, then this. Release with the repository script:
Rollback: revert this commit. It removes a test and a comment and nothing else.
🤖 Generated with Claude Code
https://claude.ai/code/session_01T623SnndzSVwCSmuQrVRj2