Skip to content

Analyze retrieved video evidence while saving it - #217

Merged
devhims merged 6 commits into
mainfrom
fix/run-scoped-asset-cache
Oct 20, 2026
Merged

devhims merged 6 commits into
mainfrom
fix/run-scoped-asset-cache

Conversation

@devhims

@devhims devhims commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner

Problem / Motivation

Answering questions about newly extracted video content waits for saving, another model decision, and a storage download before analysis begins. The dashboard also calls combined retrieval and analysis “Loading Transcript” throughout.

What changed

Transcript, frame and storyboard tools accept an optional analysis question. They analyze available bytes while saving continues, then return findings after both succeed. Transcript research uses this path for missing inputs. Retrieval-only calls remain available.

%%{init: {"themeVariables": {"actorBkg": "#e2e8f0", "actorTextColor": "#0f172a", "signalColor": "#64748b", "signalTextColor": "#64748b", "sequenceNumberColor": "#ffffff"}}}%%
sequenceDiagram
    autonumber
    participant App
    participant Storage as D1 and R2
    participant Analyst
    Note over App: Extraction has returned the requested bytes
    alt Before
        App->>Storage: Save inputs
        Storage-->>App: Saved
        Note over App: Research model chooses analysis (removed)
        App->>Storage: Download inputs again (removed)
        Storage-->>App: Input bytes
        App->>Analyst: Analyze
    else After
        par Persistence retained
            App->>Storage: Save inputs
            Storage-->>App: Saved references
        and Analysis starts immediately
            App->>Analyst: Analyze retained bytes
            Analyst-->>App: Findings
        end
        Note over App: Return findings only after saving succeeds
    end
Loading

Analysis overlaps saving without another model decision or download.

Dashboard labels now follow actual activity: loading, analyzing, finishing, then analyzed. Analysis failures show a warning instead of a green check. Saved results also receive accurate completion labels.

Preview saving overlaps unfinished analysis. Rejected calls remove unpublished previews, with failed deletions queued for retry. Failed analysis preserves saved inputs for retries without another extraction or input charge. Saved-analysis warnings stay within the 50-warning limit.

Retrieval and inference have separate concurrency limits. Required tools wait for temporary credit holds and recheck quotas. Deletion, cancellation, failed saves and changed content prevent publication.

Progress responses gain optional activity and analysis outcome fields. No migration is required. The live labels require both backend and dashboard deployment.

Tests

  • Builds, type checking and generated documentation checks passed.
  • 2,055 platform unit tests, 94 runtime integration tests, 115 web tests and three focused browser tests passed.
  • Earlier production smoke tests on 3b40ce0 passed for all three content types and confirmed overlap. The new UI changes are not deployed. Overall speedup remains unmeasured.

@vercel

vercel Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
video2ctx-web Ready Ready Preview Oct 20, 2026 9:31am UTC

devhims commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

Verdict: approve 7ecc923.

No blocking regression found in the reviewed changes. Checked cache snapshot immutability/LRU limits, per-store and per-run isolation, deletion-generation invalidation, late work after cancellation, current asset metadata/duration limits, explicit refresh, persistence-before-seeding and the unchanged billing path.

Independent verification at this head:

  • Platform Node unit suite: 2,005 passed, 56 skipped.
  • Workers session/runtime/billing integration suite: 365 passed.
  • Video-catalog integration: 11 passed.
  • Processor and frame container Node suites: 49 passed each.
  • Five additional adversarial Workers probes passed: mutation isolation, eviction back to the exact historical version after refresh, delete/restore during an outstanding read, cancellation preventing late fill, and another store instance rejecting the original cache.
  • TypeScript, generated documentation freshness and diff whitespace checks passed.
  • GitHub Verify run 39377427868 and Vercel passed.

The cache is correctly an optimization over existing verified storage reads, not a source of ownership or billing authority. Hits still check current session metadata; a generation change invalidates retained entries. The wrapper closes the cache on completion/failure/cancellation, and late writes cannot reopen it.

Verification limits: no production latency or heap measurements, no live provider calls. The 16 MiB limit bounds retained serialized snapshots rather than total Worker memory or transient JSON/model allocations. Cache hits intentionally trust earlier same-run shared-source verification. The preexisting post-analysis packet-hash delete/restore race documented on PR216 remains outside these changes. Local auth-integration execution did not produce a completed result; the corresponding GitHub integration/auth checks passed. No implementation changes, pushes, merge or deployment performed.

@devhims devhims changed the title Reuse saved analysis inputs within each research run Analyze retrieved video evidence while saving it Oct 20, 2026

devhims commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner Author

Verdict: changes needed for 392a2fe. This supersedes my earlier approval. Additional review feedback exposed cases the green suites missed.

Merge blockers:

  1. P2 — Failed combined transcript analysis is cached as completed retrieval, preventing an identical same-run retry. research-video-transcripts.ts:71-76 calls retrieval with the question in its semantic key, then replaces the returned failure packet with a generic error. retrieval-analysis.ts:78-83 returns that packet successfully, so agent-runtime-do.ts:763-775 reuses it on the next call instead of rerunning the analyst. A real AgentRuntimeDO/SQLite probe confirmed one analyst invocation across two different toolCallIds, a completed stored packet with the saved asset version, and no asset version in either batch failure result. Preserve an actionable saved retry handle and allow analysis retry without repeating source retrieval or billing.

  2. P2 — Combined calls hold analysis slots during slow retrieval. research-agent.ts:258-261,304 routes includesAnalysis to the four-slot analysisLimiter for the entire execution. Four waiting combined fetches prevent a ready saved-asset analysis from starting; eight-source research no longer pipelines four fetches independently of four analyses. A focused scheduling probe using the real ConcurrencyLimiter and the exact dispatch condition reproduced the blockage until a fetch slot was released. Mixed retrieval-only and combined calls can also bypass the previous shared four-fetch ceiling. Keep extraction and inference admission separate; simply moving the whole combined call to the provider limiter risks deadlock when an analyst needs metadata through that limiter.

Additional confirmed regression:

  1. P2 — Saved multi-sheet storyboard analysis drops later sheets' warnings. analyze-video-storyboard.ts:56-63 copies meta from the first sheet, and :154-157 now emits only that meta's warnings. The previous implementation flattened warnings across every saved value. A valid two-sheet probe retained the first warning and lost the second. Aggregate/deduplicate all selected sheets' warnings while respecting the output cap.

Other feedback, qualified:

  • All requested frames failing is not a newly introduced diagnostic regression as described: youtube-frames-contract.ts already requires frames.min(1). Both retrieval-only and combined calls reject an all-empty response; the latter can surface a less specific missing-save error.
  • Appending a superseded warning after the cap is fragile, but a 51-warning result was not reproduced with valid current sheet/analyst limits. An artificially over-limit input is not evidence of a production failure. Keep the final cap after all warnings when fixing aggregation.
  • retrieval-analysis.ts:81 discards the caught reason when converting failure to a successful retrieval packet, bypassing the failed-tool record/log. Preserve sanitized failure classification/correlation. This does not mean every underlying failure has no logs: transcript_analysis instrumentation can still record a failed operation.
  • Previews start only after awaited inference; full image content is serialized/hashed twice; the retrieval-only storyboard has nested identical deadlines. These are performance/cleanup opportunities, not independently measured production regressions. The three-way transcript forwarding branch is cleanup and also restores diagnostic forwarding omitted on the base.

Kiro independently reached changes-needed for this exact head and agrees on the retry and scheduling blockers plus warning loss. Its repository suite was blocked by package-registry policy; it used code review and focused limiter/helper probes, not a full pass. The full suites below were run locally in this review.

Verification: 2,021 unit tests passed / 59 skipped (three opt-in Fireworks tests not run); 367 session/runtime Workers integration tests, 11 catalog tests, 170 auth integration tests, 49 tests in each container suite passed. TypeScript/docs/whitespace checks and GitHub Verify 39386775043/Vercel passed. Six original safety probes passed, followed by four targeted characterization probes confirming the retry, warning, scheduler and empty-frame findings/qualifications above. No production changes, push, merge or deployment. Production latency/memory benefits remain unmeasured.

devhims commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

Verdict: approve 2fac745. The three previously confirmed regressions are resolved; this supersedes the changes-needed verdict for 392a2fe, not its historical findings.

Rechecked fixes:

  • agent-runtime-do.ts:774-781 and tools/retrieval-analysis.ts:133-153: an identical request with a new tool-call ID retries saved analysis rather than returning the cached analysis failure. Same-call recovery remains idempotent. The normal analysis record coalesces concurrent retries and reuses a completed recovery. A real Workers/SQLite reviewer probe confirmed one extraction, one source charge, and one recovery analysis across simultaneous retries. Deletion, cancellation and repeated failure remain guarded.
  • tools/research-video-transcripts.ts:98-108: failed analyses now return an actionable saved assetVersion instead of disappearing behind a generic error.
  • runtime/evidence-scheduler.ts:11-45 and research/research-agent.ts:503,658: provider calls share four retrieval slots without enclosing the whole combined analysis. Saved analysis can proceed during slow retrieval; mixed requests retain the shared fetch limit, nested metadata completes without deadlock, and queued phase-canceled work does not call the provider. An extra interleaved-phase probe confirmed cancellation of one queued caller does not cancel another.
  • tools/analyze-video-storyboard.ts:73-81,155: warnings from all selected sheets are aggregated/deduplicated, with the superseded warning included before the final 50-warning cap and schema validation.
  • tools/retrieval-analysis.ts:113-128: failures retain allowlisted classification/correlation without exposing the private error message. The diagnostic artifact being first changes where visualDiagnostics is attached, but does not remove it from the persisted packet.

Independent verification at this head:

  • 2,031 unit tests passed; 59 skipped, including opt-in live-provider checks.
  • 371 Workers session/runtime/billing integration tests, 11 catalog tests, 170 auth integration tests passed.
  • Processor and frame container suites: 49 passed each.
  • Eight additional reviewer probes passed: concurrent recovery/billing, isolated queued-phase cancellation, and six save/publication/content/deletion/cancellation boundaries.
  • TypeScript, generated documentation freshness and whitespace checks passed.
  • GitHub Verify 39391707033 completed successfully, including platform integration, auth E2E and both dashboard E2E jobs; Vercel preview is ready.

No new blocking correctness or security finding. Non-blocking performance work remains: previews still start after inference, image identity checks serialize/hash twice, and some redundant validation/deadline work remains. These are not measured production latency regressions. A saved-analysis retry intentionally has no new preview payload; the original retrieval retains its previews.

Verification limits: no live inference rerun or production latency/heap measurement in this review, and no end-to-end production validation of early callbacks across the real deployed Durable Object boundary. The preexisting post-analysis packet-hash delete/restore race noted in the earlier cache review remains outside this change. Only reviewer test probes were added locally; no production edits, pushes, merge or deployment.

devhims commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

Verdict: approve a8d6c89. Reviewed the new credit-admission and preview-overlap commit against approved 2fac745; no new blocking correctness, billing or security finding.

Verified behavior:

  • runtime/evidence-billing.ts:228-260 distinguishes temporarily held credits from genuine exhaustion. Required callers wait without owning a hold; cancellation removes their waiter/listener. Wake-ups recheck affordability rather than overbooking.
  • agent-runtime-do.ts:811-847 rechecks active run, signal, session generation and durable tool quota immediately before admission. Failed rechecks release any acquired hold. Optional nested metadata remains fail-fast, avoiding a wait on its own parent; saved-analysis retries still do not require a second source charge.
  • runtime/evidence-scheduler.ts:32 forwards the current signal into credit admission. Waiting for credits occurs before provider work, so it does not consume a fetch slot.
  • tools/retrieval-analysis.ts:76-84 and the frames/storyboard afterSaved callbacks start previews only after source persistence, alongside unfinished inference. Findings wait for both. A non-cancellation preview failure preserves analysis with a warning; failed source saves never start previews. Deletion or changed content still prevents publication.

Independent checks at this exact head:

  • 2,037 unit tests passed, 59 skipped (including opt-in live-provider tests).
  • 379 Workers session/runtime/billing tests, 11 catalog tests and 170 auth integration tests passed.
  • Both container suites passed: 49 processor and 49 frame tests.
  • Twelve extra reviewer probes passed: four credit wait/cancellation/mixed-price/lifecycle cases and eight persistence/content/cancellation/preview boundaries. These include deletion between credit release and waiter continuation, cancellation before a hold releases, deletion while previews finish, and analysis failure while preview work is still pending.
  • TypeScript, generated documentation freshness and diff whitespace checks passed. GitHub Verify 39395985778 completed successfully; Vercel preview is ready.

The prior retry, scheduler and storyboard-warning fixes remain covered and passing. No failed completed check. Limits: provider work was mocked in adversarial tests; no live inference rerun or deployed Durable Object end-to-end test. Production wait time, latency and heap benefits remain unmeasured. Full-image hashing and preexisting orphan-preview cleanup/race limitations are not solved by this commit. No production edits, pushes, merge or deployment performed.

devhims commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

Verdict: approve 3b40ce0, with the non-blocking cleanup limitation below explicitly retained. Reviewed this delta against approved a8d6c89; no new merge-blocking regression found. This is not verification of an unconditional durable-cleanup guarantee.

What is verified:

  • runtime/preview-lifecycle.ts:7-37 tracks previews returned by successful saver calls until the enclosing tool completes, drains outstanding writes before discarding, and retains only previews referenced by the returned packet. Successful analysis-failure fallback retains its retrieval previews; cached/coalesced calls do not acquire ownership of another caller's previews.
  • agent-runtime-do.ts:717-723 durably queues abandoned private preview keys before deletion; a failed deletion leaves those queued keys available for cleanup after restart. Shared source JPEGs are not targeted.
  • tools/analyze-video-frames.ts:60-65 and analyze-video-transcripts.ts:95-100 preserve the superseded warning before applying the final 50-warning cap and validating the packet.

P3 / non-blocking existing cleanup gap: runtime/preview-lifecycle.ts:17 records descriptors only when the saver fulfills. In frame-previews.ts:80-82, if cancellation or a partial PUT failure triggers rollback and that rollback DELETE also fails, the saver rejects without returning its attempted keys. The wrapper therefore sees no created previews and never reaches discardPreviews at :35-36. Two failure-injection tests using the real saver reproduced one orphaned private object and zero queued cleanup keys: (1) cancel after successful PUT, then fail DELETE; (2) first PUT succeeds, second PUT fails, then fail DELETE. Restart cleanup cannot retry keys that were never queued.

This is an incomplete hardening of the preexisting orphan-preview limitation, not a new leak introduced by this commit. The reproduced impact is unreferenced private storage, not demonstrated public exposure or loss of published previews. Recommend narrowing the PR/reference documentation's blanket failed-deletion claim, or following up with durable tracking of attempted keys before the saver can lose ownership. A process restart before finally runs remains another unverified interruption boundary. Do not describe all orphan paths as fixed.

Verification at this head:

  • Passed: 2,052 unit tests; 380 Workers session/runtime/billing tests; 11 catalog; 170 auth integration; 49 processor and 49 frame container tests. 59 unit tests skipped, including live-provider opt-ins.
  • Passed: eight additional reviewer persistence/content/cancellation/preview boundary probes. Focused ownership and restart-cleanup tests also passed.
  • Failed: the two added complete-cleanup invariant probes described above. These document the retained limitation, rather than a failing required repository suite.
  • Passed: TypeScript, generated docs freshness, whitespace checks and GitHub Verify 39397853038, including CI E2E coverage.

Limits: no live inference rerun, deployed end-to-end test, production heap/latency measurement or crash-injection test. No production edits, pushes, merge or deployment performed.

devhims commented Oct 20, 2026

Copy link
Copy Markdown
Owner Author

Verdict: approve 31e989f. Reviewed the new progress/activity/outcome and dashboard changes against approved 3b40ce0. No blocking regression found.

Verified:

  • tools/retrieval-analysis.ts:46,68-73 emits loading/analyzing/finishing at the actual combined-work boundaries, suppressing finishing after abort. Activity carries only a tool-call ID and an enum, not the analysis question or video content.
  • agent-runtime-do.ts:514-529 restores the latest persisted activity per call. runtime/run-progress.ts:63-73 derives analysis completion/failure from the saved packet, prioritizing RETRIEVAL_ANALYSIS_FAILED rather than treating successful retrieval as successful inference. Legacy retrieval-only packets do not invent an analysis outcome.
  • web/lib/agent-tool-labels.ts:30-44 keeps running phases distinct from terminal outcomes; SessionsClient.tsx:407-408 uses a warning rather than a green check when retrieval completes but analysis fails. Saved frame reuse does not imply another capture.
  • Optional backend/OpenAPI fields and tolerant web parsing preserve old/new client compatibility. Focused concurrency, cancellation, failure-projection and compatibility probes found no cross-call activity contamination or late finishing on cancellation.

Non-blocking display qualifications: a recovered analysis is a new successful analysis row; the original failed attempt remains accurately marked failed. Analysis-budget rejection can still show the generic Loading Failed label. A resumed unfinished call can retain its last recorded phase until new activity is emitted. These do not affect evidence or billing.

Independent local checks passed: 2,055 platform unit tests (59 skipped), 381 Workers session/runtime/billing tests, 11 catalog, 170 auth integration, 49 processor and 49 frame container tests, and all 115 web tests. Platform TypeScript, generated documentation freshness and whitespace checks passed.

Local dashboard build/browser verification was blocked by the execution environment: Next.js networkInterfaces failed with uv_interface_addresses; the allowed outside-sandbox retry failed before launch. No local visual/Playwright pass is claimed. Exact-commit GitHub Verify 39401265933 completed successfully, including Web build, both Dashboard E2E groups and Auth E2E. Thus applicable CI checks are green despite that local limitation.

The previously documented orphan-preview rollback limitation is untouched, not fixed by this UI commit. I did not independently repeat the author's production smoke tests, run live inference, or measure production performance. Live phase labels require both backend and dashboard deployment. No production edits, pushes, merge or deployment performed.

@devhims
devhims merged commit c2727e2 into main Oct 20, 2026
11 checks passed
@devhims
devhims deleted the fix/run-scoped-asset-cache branch October 20, 2026 09:38

This branch was successfully deployed

1 active deployment
Preview — 31e989fb Deployed Oct 20, 2026 by vercel[bot]
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