fix(engine): keep a/v in sync when ffprobe misses a late stream's start - #5462

Review at d8d5e84a2b0abccfa089d2f0dacf538ba9076f57, against effective merge-base/API base b211fc7cb845c6838cbdb1860506c2fda2804787: both changed files read end-to-end, two modifications, +409/−21, no additions/deletions/renames. The submitted review and inline histories were empty when checked; private review claims are not GitHub receipts.

The explicit far-end interval, partial-row discard and abort propagation are good safeguards (packages/engine/src/utils/ffprobe.ts:75,105–135). The shared raw probe also avoids independently choosing fallback timing while its entry survives (:460–493).

Blocker — the equality fingerprint can shorten a valid AVI loop period

packages/engine/src/utils/ffprobe.ts:60–72,87–96 treats an aligned stream whose start/duration match its container as unobserved and overwrites its duration when packet measurement succeeds. Equality is not sufficient evidence that the summary was filled: the inspected FFmpeg AVI demuxer takes video duration from the declared frame count, skips empty chunks while advancing the frame offset, and does not extend the preceding real packet to include those slots (n7.1.1 avidec, empty chunks). The muxer's final skip-frame writing supports this representation (avienc). For an AVI exposing that declared-duration/packet-span distinction, this fingerprint discards the declared cycle length even though it was not inferred from a missing packet summary. This is a source-derived contract regression, not a claim that I decoded this representation with the bundled runtime.

Concrete source-derived control: aligned 10-second MJPEG/PCM AVI metadata, 25 fps, 250 declared video slots, with synthetic packet replies representing 242 nonempty frames followed by eight null slots. Declared duration is 10 seconds and last real packet end is 9.68. Executing the extracted refinement preserves 10 on base and substitutes 9.68 on HEAD. The positive substituted duration wins over declared nb_frames (ffprobe.ts:886–898). The actual extraction-window source keeps a loop cycle using that playable duration (packages/engine/src/services/videoFrameExtractor.ts:1419–1434), and resolvePlayableVideoDuration / getFrameIndexAtTime use it for modulo (:1303–1308,2643–2675); the final-frame-only repair is not selected by that loop branch. Given 242 delivered paths, a 20-second looping slot at time 9.8 holds frame 241 on base but wraps to frame 3 on HEAD in the modeled production lookup. That is a 320 ms cycle change for these inputs, not the disclosed sub-frame rounding cost; the actual native delivered-path count remains unverified.

I checked the default producer coverage gate against the same modeled delivery count, rather than claiming a completed render. With 242 synthetic delivered paths the extracted report/assert passes: 242/250 = 96.8% on base and 242/242 = 100% on HEAD, both above default 95% (packages/producer/src/services/render/videoFrameCoverage.ts:125–133,169–220). This shows the gate does not necessarily exclude the source-derived contract regression; it is not a native passing-render receipt. The ≤60% extraction reconciliation does not restore the duration for these inputs. An earlier 200/250 control does not establish default producer rendering because base fails that gate; that larger-tail example is not the reachability basis for this finding.

Please preserve trustworthy hold-bearing stream duration, or narrow/corroborate the filled-metadata inference before replacing it. Add an AVI trailing-null-slot regression that pins the declared playable duration and loop phase, alongside the late-stream tests.

Important follow-up — the disclosed cache-eviction boundary is real

As the PR body already notes, raw eviction can separate video/audio timing generations. Raw is capped at 128, but normalized video/audio caches outlive it (ffprobe.ts:233–247,432–493,794–797,1049–1086). A failed refinement can remain in normalized video while a later first audio read measures successfully, or vice versa; source-extracted controls show leads 0/7 or 7/0 and the corresponding input-seek versus timestamp-trim argument split (packages/engine/src/services/audioMixer.ts:624–657). Producer awaits video probes, then image probes, extraction, coverage and audio, so intervening images can supply eviction pressure. I am treating this narrow, explicitly disclosed boundary as follow-up, not as an additional blocker or a measured PCM desync. Tie timing decisions to a common generation/lifetime and add eviction coverage.

Evidence limits: two independent full source audits and subsequent cross-falsification completed. I read their controls and the finding-bearing production/upstream paths; my additional offline control executes extracted refinement, lookup and coverage bodies with synthetic metadata, packet replies, delivered frame counts and constant-rate collaborators. It is not a decoded AVI, native FFmpeg/bundled-version verification or a completed CLI render. Native repository/media/browser tests and CI were not run; the author's measurements remain separate evidence claims.

— tai

Verdict: REQUEST CHANGES
Reasoning: For the AVI declared-duration/packet-span representation supported by the inspected demuxer source, this fingerprint replaces the declared cycle with packet extent and changes the loop period; the modeled narrower case passes the extracted coverage gate. Narrow/corroborate the inference and preserve that duration contract before merging; this verdict rests on the source-derived regression, not a native rendered-output claim.

Re-review at 10a544653515152268dea5e77d9a39a72867eb3c, against effective merge-base b211fc7cb845c6838cbdb1860506c2fda2804787. Two modified files, +535/−22; both changed HEAD files and base originals were read, with extraction/audio/cache callers inspected.

Resolved from my previous review:

• The exact AVI witness with first packet at zero now retains its declared 10 s: ffprobe.ts:168–171; the new assertion is at streamStartOffset.test.ts:591–595. I am not carrying that resolved example forward as an active blocker.
• The separate measured-timing cache fixes the previously disclosed ordinary 128-entry raw-LRU eviction witness (ffprobe.ts:79–82): the same simulated fallback remains shared after 130 intervening probes. The missing-duration fallback also avoids double-counting stream lead (ffprobe.ts:926–955).

Blocker [P2] — Bind shared measured streams to the raw probe's generation. packages/engine/src/utils/ffprobe.ts:65–72.

The raw probe fingerprints A before asynchronous ffprobe (:509), but refinement fingerprints the path again after its response (:65) and caches the entire A stream array under that later identity. If an external writer replaces A with B in that window, a subsequent call correctly performs a fresh raw probe of B, then :67 discards B's streams and substitutes cached A streams. The normalized cache now retains A metadata under B: stale metadata is reused across subsequent calls while B's fingerprint remains unchanged, not merely during the pre-existing transient observation of a concurrent replacement.

An independently executed, source-extracted public extractMediaMetadata control simulated width64 A → width128 B during the first raw response. Base returned [64,128,128]; HEAD returned [64,64,64], with two raw probes in each. Without replacement both returned [64,64,64] with one probe; ordinary replacement between completed calls invalidates correctly. Width is a production-consumed field (videoFrameExtractor.ts:1006), and caching the whole array can also preserve stale codec/fps/color/timing fields.

Carry the raw probe's immutable generation into refinement and prevent publishing/reusing results under a different generation. Add a replacement-during-probe regression through the public metadata reader; reducing the cached fields alone does not establish generation correctness.

Blocker [P2] — A late first packet does not justify truncating AVI's declared end. packages/engine/src/utils/ffprobe.ts:103–108,168–177.

The first-packet guard fixes the zero-start example, but the same valid null-slot representation with leading empty slots still passes it. A 25fps AVI can declare 250 slots / 10 s, contain two leading null slots, 240 real packets from .08 through 9.64 (last packet end9.68), and eight trailing null slots. .08 > .05 triggers replacement with start .08 and duration 9.6; playable duration becomes 9.68, losing the declared trailing320ms hold. Base keeps the 10 s cycle. The declared 10 s already includes the leading80ms; it is not 10 s plus an additional lead.

This is grounded in the inspected FFmpeg n7.1.1 AVI contract: the demuxer derives duration from declared frame count and skips empty chunks while advancing frame offset; the muxer accepts explicitly supplied empty packets. Its automatic skipped-frame insertion does not establish initial null slots. This is an explicitly leading-null representation, not a claim that the new test fixture automatically generates it, nor confirmation of the bundled binary version.

Extracted production window/final-frame/reconciliation/coverage/lookup controls with 240 modeled delivered paths preserve a 10 s loop on base versus9.68 on HEAD. At9.8 s lookup selects final frame239 versus wrapped frame3. Both modeled default coverage checks pass:240/250=.96 versus240/242≈.9917; the ≤60% correction does not run, and the loop window does not request final-frame repair. Thus those source guards do not restore the declared cycle in this control. Do not shorten a valid declared AVI cycle merely because its first/last nonempty packets exclude null slots: preserve its declared media end (the 10 s playable cycle here) when correcting start, or require evidence that the declaration is invalid. Add the leading-plus-trailing-null case alongside the existing test.

Evidence limits: controls used extracted source bodies with stat/process/media collaborators simulated. No native ffprobe/FFmpeg, real file-replacement experiment, repository suite, browser/CLI render, decoded audio or CI verification was performed. Modeled packet/frame outcomes are not native delivery receipts; full-render success and occurrence frequency remain unmeasured. The verdict is based on the reproduced metadata-generation defect and the source-derived duration-contract regression, not missing native evidence or CI status.

— tai

Verdict: REQUEST CHANGES
Reasoning: The original AVI example and ordinary raw-LRU follow-up are resolved, but the new cache can persist a replaced file's old metadata, and the first-packet guard still permits truncating a declared AVI cycle with leading null slots. Correct these generation and duration contracts before approval.

Verdict: REQUEST CHANGES · 1 blocker · 1 important · 0 nits — head 7b93d64e

Both earlier change requests are resolved: AVI is no longer measured (only mpegts|nut|matroska|webm), and measured timings are now cached under the raw probe's pre-probe identity, so a file replaced mid-probe is re-probed on the next call. What's left is CI and one cost regression.

Blocker: the required Tests on windows-latest check fails because of the new test. In streamStartOffset.test.ts:597-618 ("does not keep a replaced file's measured timing…"), the fake ffprobe is a #!/bin/sh script. Windows can't spawn it: ffprobe not found at HYPERFRAMES_FFPROBE_PATH="…\fake-ffprobe-swap" (engine-cli job, 1 failed / 2553 passed). Either write the wrapper as a Node script launched through process.execPath, or it.skipIf(process.platform === "win32").

Important: a URL with stored probe facts now downloads the media when a stream is late. buildProbeFactsText stores the raw, unmeasured ffprobe JSON. readProbeJson returns that JSON, and withMeasuredFilledStreams (ffprobe.ts:66, called at :669-671) still sees the fingerprint, so it runs packet probes against the remote URL. Repro: a TS whose video is 20 s late, served with declareContentAddressedUrl + declareProbeFactsUrl. extractMediaMetadata(url) made 15 range requests (~7 MB for a 0.73 MB file). An aligned TS made none. Possible fixes: store the measured streams in the facts and bump PROBE_FACTS_VERSION, or skip measuring when the JSON came from stored facts. If the fetch fails, the container fallback is also cached under the sha256: identity for the rest of the process.