Repository navigation
fix(producer): plan hash no longer depends on the scratch folder - #5782
Conversation
Edit accuracy: accurate 271 (base branch 271), smooth 175 of thoseThe gate passes. Quarantined, measured but not gated (0) |
77ce046 to
b66afc6
Compare
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at b66afc69. meta/videos.json srcPath is now written relative to the plan: video-frames/…, then compiled/…, then project-relative, and absolute paths stay only for files outside the project (plan.ts:958-968, 1301-1312). The scratch folder was <planDir>/.plan-work, so the old absolute path carried a per-run component into the hash.
There's no collision or cache-poisoning risk. freezePlan hashes the bytes of compiled/, video-frames/ and video-sources/, so two projects with the same relative path but different media still hash differently. The change removes only the random component. srcPath is never used to open a file: renderChunk.ts:397 only passes it along, and deferred extraction reads deferredRange.sourcePath. The new test plans one project into two plan folders and asserts the same hash and identical videos.json bytes.
Nit: HAS_FFMPEG only checks that ffmpeg exists, but the fixture needs libx264, so on a build without it the test throws instead of skipping.
Required CI is green at this head. No live run.
— Rames
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving the merge successor at 8129af41. This replaces my approval at b66afc69.
- The only change is the conflict fix. Merging main brought in a second
import { spawnSync } from "node:child_process"inplan.test.ts. One import is left (:19), and it serves both the PR's fixture (:791,:793) and the ffmpeg-gatedstitched mp4 plansblock from main (:891onward). - Apart from that, the PR's diff against main is the same as at
b66afc69. I compared the two patches with index and hunk headers stripped, and the only differences were that import and the surrounding context from main. The plan-hash fix itself hasn't changed.
Required CI is green at this head: CI run 39077463966 (Typecheck, Build, Test, runtime contract, reachability, edit accuracy gate), Windows run 39077463972, regression run 39077463926 and PR captures 39077463948 all passed. No live run.
— Rames
What changed
A distributed plan now gets the same plan hash every time it is built from the same inputs, including plans with a video.
meta/videos.jsonrecorded each video's source as an absolute path inside the planner's temporary work folder. That folder has a random name on every run, and the file feeds the plan hash, so any plan with a video got a new hash each run. That broke the contract inplanHash.ts("two invocations with identical inputs MUST produce the same hash"), which replay short-circuits and cross-version checks rely on.The source is now written without the work folder:
compiled/...orvideo-frames/...relative to the plan, project-relative for project files, and unchanged only for files outside the project (those are real inputs, so they are already stable). Nothing reads this field to open a file: chunks rebuild frame paths fromvideo-frames/<videoId>, and deferred extraction usesdeferredRange.sourcePath, which was already plan-relative.The test plans one project with a real (generated) video into two different plan folders and checks the plan hash, the
meta/videos.jsonbytes and the recorded source. The existing determinism test used a composition with no video, which is why this was never caught. The two ffmpeg-fixture tests in the file now share one small helper.What I measured
plan.tschange reverted it fails on the plan hash itself (two different hashes).plan.test.ts55 pass,renderChunk.test.ts19 pass,planV2.test.ts62 pass,rebuildExtractedFrames.test.ts5 pass, all 0 fail. Producer typecheck, oxlint, oxfmt, the comment checks and the Fallow audit are clean.What I did NOT exercise
Main integration after merge-queue failure
Current signed head
8129af41685c6c910d725d3522829876796645b2merges main7e00f37be2c76b4a9294c81aed79cf49667709f1. Main and this PR each added the samespawnSyncimport toplan.test.ts; Git merged them textually, but the actual queue lint rejected the duplicate declaration. Removed only that duplicate import, retaining both branches' fixtures and all plan behavior.Identifier spawnSync has already been declaredat lines 18/20; this matches queue job117525476189. With one import, lint and format pass.eslint-scope/@jridgewell/trace-mappingin the reused local dependency cache. This is not a full local build pass. No dependencies were installed or shared caches changed; current-head locked-dependency hosted CI remains required.b66afc69; successor review and fresh hosted checks are pending.