Skip to content

test(kite3d): pin the manifest hash as an If-Match token - #22

Draft
pythonlearner1025 wants to merge 1 commit into
perf/dev-server-manifest-in-memoryfrom
perf/dev-server-lazy-hash
Draft

pythonlearner1025 wants to merge 1 commit into
perf/dev-server-manifest-in-memoryfrom
perf/dev-server-lazy-hash

Conversation

@pythonlearner1025

Copy link
Copy Markdown
Member

Stacked on #21, which is stacked on #20. Base branch is perf/dev-server-manifest-in-memory. Review #20 and #21 first.

Read this one first, though. It does not do what it was asked to do. It deletes nothing, and this is why.

What part this touches

GET /api/files returns 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:

{ "label": "current", "bootMs": 2702.5, "entryCount": 3496, "hashedMegabytes": 1557.7 }

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-Match from 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.replaceManifest seeds a hash for every row it receives:

private replaceManifest(entries: ProjectFileEntry[]) {
    this.manifest = entries
    for (const entry of entries) this.hashes.set(entry.path, entry.sha256)

writeSerializedScene then uses that map as its write precondition:

const result = await this.source.write(file.path, file.bytes, this.hashes.get(file.path) || '*')

serialized.files are the buffers and images that serializeSceneGltfDocument extracts out of the exported glTF. assets/main.scene.bin is one. The editor writes them on every scene save. It never reads them, because it loads the scene through viewer.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-Match header on every PUT next to the manifest row for that path:

[after reload] each PUT, and whether its If-Match is the manifest sha256:
  .kite3d/state.json               If-Match=*            manifest=not listed   -> STAR (overwrite regardless)
  .kite3d/state.json               If-Match=051172d12fc9 manifest=not listed   -> other (read or write response)
  assets/main.scene.bin            If-Match=eb2b94df155c manifest=eb2b94df155c -> MANIFEST sha256
  assets/main.scene.gltf           If-Match=2cb0b9c1549c manifest=2cb0b9c1549c -> MANIFEST sha256

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-Match token, 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-Match source. writeSerializedScene would read each companion file it has no hash for, to get a real ETag first. That is a read per extracted buffer per save.

replaceManifest must stop seeding hashes from 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:

npm run build       OK
npm run typecheck   OK
npm run lint        OK
npm run test:kite3d   Test Files  6 passed (6)    Tests  12 passed (12)
npm run test:runtime  1 passed (2.3s)
npm run test:editor   8 passed (25.7s)

One test is new, in packages/kite3d/test/server.test.ts:

accepts the manifest hash as an If-Match precondition and rejects a stale one

Making the manifest publish ${size}-${mtimeMs} instead of a content hash fails it, which is the whole point:

 FAIL  test/server.test.ts > accepts the manifest hash as an If-Match precondition and rejects a stale one
AssertionError: expected 412 to be 200

It also fails the add and delete test from #21, which checks the row against a known sha256:

AssertionError: expected '23-1789358403213.4553' to be '0c13ee5bb1ad5998f970a37dd4cb2bba661ec…'

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:

{"mode":"seeded",     "filesRewritten":300, "identicalBytes":true, "editorReloadEventsFirstBuild":0,   "editorReloadEventsSecondBuild":0}
{"mode":"lazy-seed",  "filesRewritten":300, "identicalBytes":true, "editorReloadEventsFirstBuild":300, "editorReloadEventsSecondBuild":0}

The lazy-seed row 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:

{ "label": "current", "bootMs": 2702.5, "filesSamplesMs": [ 108.4, 11.5, 6.8 ], "entryCount": 3496 }

Startup is the 2 seconds this PR failed to remove. /api/files stays 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 .bin did not exist on the first save, so the editor sent If-Match: * and created it. That is in the evidence above.

.kite3d writes 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 kite3d npm 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:

npm run release:patch

Rollback: revert this commit. It removes a test and a comment and nothing else.

🤖 Generated with Claude Code

https://claude.ai/code/session_01T623SnndzSVwCSmuQrVRj2

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
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