feat: video visual extraction (FFmpeg + Vision API)#751
Conversation
Implements video_extract_visual semantic task type that extracts searchable text from video files by sampling frames with FFmpeg and describing them via a Vision model (OpenAI-compatible). - New TaskTypeVideoExtractVisual in pkg/semantic/task.go - VideoTextExtractor interface + ProcessVideoExtractTask handler - OpenAIVideoTextExtractor: FFmpeg frame sampling + multi-image Vision API - Content-type allowlist: mp4, mov, avi, webm, mkv - Full backend wiring: options, enqueue, LLM cost tracking - Semantic worker dispatch + tenant worker task type registration - Env var configuration: DRIVE9_VIDEO_EXTRACT_* - Unit tests for handler, MIME detection, enqueue logic Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable asynchronous video-to-text extraction using durable semantic tasks, FFmpeg frame sampling, OpenAI-compatible vision APIs, transactional ChangesVideo visual extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Upload
participant SemanticTasks
participant TenantWorker
participant Dat9Backend
participant OpenAIVideoTextExtractor
participant OpenAICompatibleAPI
Upload->>SemanticTasks: Confirm video revision
SemanticTasks->>SemanticTasks: Enqueue video_extract_visual
TenantWorker->>Dat9Backend: ProcessVideoExtractTask
Dat9Backend->>OpenAIVideoTextExtractor: Extract video text
OpenAIVideoTextExtractor->>OpenAICompatibleAPI: Send sampled frames
OpenAICompatibleAPI-->>OpenAIVideoTextExtractor: Return visual text and usage
OpenAIVideoTextExtractor-->>Dat9Backend: Return extracted text
Dat9Backend-->>TenantWorker: Write content text and acknowledge task
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
R1 review from @adversary-1 on exact head Blocking findings:
Local checks I ran:
Required before re-review: deterministic merge/ownership semantics for audio+visual text, a regression covering both tasks on the same MP4 revision without losing visual terms, and an end-to-end or tightly mocked pipeline test that proves visual content is searchable. |
When video visual extraction is enabled, skip audio_extract_text enqueue for video MIME types to prevent last-writer-wins race on content_text. Pure audio files (mp3, wav) still enqueue audio. Also fix unchecked os.RemoveAll return in video_extract_openai.go. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
R2 review from @adversary-1 on exact head R1 status:
The new test For this task, direct Local package tests remain blocked in my environment by the existing rootless-Docker TestMain/testcontainers setup, so I cannot substitute local test execution for the missing regression. |
Proves the full pipeline: HTTP PUT .mp4 → video_extract_visual task
enqueued → semantic worker dispatches → stub Vision extractor returns
visual description → content_text written back with visual-only term
("golden retriever") → task succeeds.
Also verifies no audio_extract_text task is enqueued for MP4 when
video extraction is enabled (mutual exclusion).
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
R3 verdict: GREEN for exact head No blocking findings found in this round. The R2 blocker is closed by Validation:
Scope boundary: this is code/test GREEN for visual video extraction into Drive9 search text. It does not assert audio+visual transcript merging; current MP4 semantics deliberately make visual extraction own video files to avoid |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/backend/video_extract.go (1)
126-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the sentinel to
ErrVideoExtractSourceTooLarge.Update its
errors.Iscall sites accordingly. As per coding guidelines, “Name sentinel errors with theErrFoopattern.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/backend/video_extract.go` around lines 126 - 127, Rename the sentinel error variable errVideoExtractSourceTooLarge to the exported ErrVideoExtractSourceTooLarge, and update every errors.Is call site and other references to use the new name while preserving the existing error value and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/backend/llm_usage.go`:
- Around line 98-126: Update recordVideoExtractUsage to accept ctx
context.Context as its first parameter, then use that context for logging and
syncCentralLLMCostRecord instead of backgroundWithTrace(). Update
ProcessVideoExtractTask to pass its existing context when invoking
recordVideoExtractUsage, preserving the current usage-recording behavior.
In `@pkg/backend/video_extract_test.go`:
- Around line 40-42: Replace assertion-only t.Fatal/t.Fatalf calls with the
specified t.Error/t.Errorf variants throughout pkg/backend/video_extract_test.go
at lines 40-42, 44-46, 59-61, 62-64, 85-87, 107-109, 130-132, 161-163, 164-166,
167-169, 194-196, 206-208, 209-211, 231-233, 237-239, 244-246, and 247-249. In
pkg/server/upload_test.go, change lines 684-686, 687-689, and 690-692 to
t.Errorf, and add an immediate return after the length assertion at 684-686 to
prevent indexing an empty result; keep fatal calls only for setup failures.
In `@pkg/backend/video_extract.go`:
- Around line 258-291: The video extraction flow must enforce an end-to-end
memory budget instead of retaining the full source, extracted JPEGs, base64
data, and JSON simultaneously. Update Dat9Backend.loadVideoBytesForExtract in
pkg/backend/video_extract.go (lines 258-291) to stream or bound S3/source data
through a temporary file rather than returning the entire source in memory, and
update the frame-processing/request assembly flow in
pkg/backend/video_extract_openai.go (lines 153-238) to cap or scale frame data
before constructing the OpenAI payload.
---
Nitpick comments:
In `@pkg/backend/video_extract.go`:
- Around line 126-127: Rename the sentinel error variable
errVideoExtractSourceTooLarge to the exported ErrVideoExtractSourceTooLarge, and
update every errors.Is call site and other references to use the new name while
preserving the existing error value and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81e3bd19-6cb4-471f-b790-49830e3b30b4
📒 Files selected for processing (13)
cmd/drive9-server/main.gopkg/backend/dat9.gopkg/backend/llm_usage.gopkg/backend/options.gopkg/backend/semantic_tasks.gopkg/backend/video_extract.gopkg/backend/video_extract_openai.gopkg/backend/video_extract_test.gopkg/semantic/task.gopkg/server/tenant_worker.gopkg/server/tenant_worker_helpers_test.gopkg/server/tenant_worker_semantic.gopkg/server/upload_test.go
| func (b *Dat9Backend) recordVideoExtractUsage(taskID string, usage VideoExtractUsage) { | ||
| totalTokens := int64(usage.TotalTokens()) | ||
| cost := b.imageTokenCostMillicents(totalTokens) // reuse vision cost rate | ||
| if cost == 0 && totalTokens == 0 { | ||
| cost = b.fallbackImageCostMillicents | ||
| } | ||
| if cost <= 0 { | ||
| return | ||
| } | ||
| if !b.UseServerQuota() { | ||
| if err := b.store.InsertLLMUsage("video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil { | ||
| logger.Warn(backgroundWithTrace(), "llm_usage_insert_failed", | ||
| zap.String("tenant_id", b.tenantID), | ||
| zap.String("task_type", "video_extract_visual"), | ||
| zap.String("task_id", taskID), | ||
| zap.Error(err)) | ||
| metrics.RecordTenantOperation(b.tenantID, "llm_cost_budget", "usage_insert", "error", 0) | ||
| } | ||
| } | ||
| if err := b.syncCentralLLMCostRecord(backgroundWithTrace(), "video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil { | ||
| logger.Warn(backgroundWithTrace(), "central_quota_llm_cost_record_failed", | ||
| zap.String("tenant_id", b.tenantID), | ||
| zap.String("task_type", "video_extract_visual"), | ||
| zap.String("task_id", taskID), | ||
| zap.Error(err)) | ||
| metrics.RecordTenantOperation(b.tenantID, "central_quota", "llm_cost_record", "log_error", 0) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Accept and pass context.Context to propagate tracing and cancellation.
As per coding guidelines, pass context.Context as the first parameter to all I/O and database calls. The recordVideoExtractUsage function initiates database and quota operations but does not accept a context parameter, relying instead on a disconnected backgroundWithTrace() context.
Please update the function signature to accept ctx context.Context as the first parameter and pass it to the logger and syncCentralLLMCostRecord. Note that you will also need to update its call site in ProcessVideoExtractTask to pass the context.
💡 Proposed fix
-func (b *Dat9Backend) recordVideoExtractUsage(taskID string, usage VideoExtractUsage) {
+func (b *Dat9Backend) recordVideoExtractUsage(ctx context.Context, taskID string, usage VideoExtractUsage) {
totalTokens := int64(usage.TotalTokens())
cost := b.imageTokenCostMillicents(totalTokens) // reuse vision cost rate
if cost == 0 && totalTokens == 0 {
cost = b.fallbackImageCostMillicents
}
if cost <= 0 {
return
}
if !b.UseServerQuota() {
if err := b.store.InsertLLMUsage("video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil {
- logger.Warn(backgroundWithTrace(), "llm_usage_insert_failed",
+ logger.Warn(ctx, "llm_usage_insert_failed",
zap.String("tenant_id", b.tenantID),
zap.String("task_type", "video_extract_visual"),
zap.String("task_id", taskID),
zap.Error(err))
metrics.RecordTenantOperation(b.tenantID, "llm_cost_budget", "usage_insert", "error", 0)
}
}
- if err := b.syncCentralLLMCostRecord(backgroundWithTrace(), "video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil {
+ if err := b.syncCentralLLMCostRecord(ctx, "video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil {
- logger.Warn(backgroundWithTrace(), "central_quota_llm_cost_record_failed",
+ logger.Warn(ctx, "central_quota_llm_cost_record_failed",
zap.String("tenant_id", b.tenantID),
zap.String("task_type", "video_extract_visual"),
zap.String("task_id", taskID),
zap.Error(err))
metrics.RecordTenantOperation(b.tenantID, "central_quota", "llm_cost_record", "log_error", 0)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (b *Dat9Backend) recordVideoExtractUsage(taskID string, usage VideoExtractUsage) { | |
| totalTokens := int64(usage.TotalTokens()) | |
| cost := b.imageTokenCostMillicents(totalTokens) // reuse vision cost rate | |
| if cost == 0 && totalTokens == 0 { | |
| cost = b.fallbackImageCostMillicents | |
| } | |
| if cost <= 0 { | |
| return | |
| } | |
| if !b.UseServerQuota() { | |
| if err := b.store.InsertLLMUsage("video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil { | |
| logger.Warn(backgroundWithTrace(), "llm_usage_insert_failed", | |
| zap.String("tenant_id", b.tenantID), | |
| zap.String("task_type", "video_extract_visual"), | |
| zap.String("task_id", taskID), | |
| zap.Error(err)) | |
| metrics.RecordTenantOperation(b.tenantID, "llm_cost_budget", "usage_insert", "error", 0) | |
| } | |
| } | |
| if err := b.syncCentralLLMCostRecord(backgroundWithTrace(), "video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil { | |
| logger.Warn(backgroundWithTrace(), "central_quota_llm_cost_record_failed", | |
| zap.String("tenant_id", b.tenantID), | |
| zap.String("task_type", "video_extract_visual"), | |
| zap.String("task_id", taskID), | |
| zap.Error(err)) | |
| metrics.RecordTenantOperation(b.tenantID, "central_quota", "llm_cost_record", "log_error", 0) | |
| } | |
| } | |
| func (b *Dat9Backend) recordVideoExtractUsage(ctx context.Context, taskID string, usage VideoExtractUsage) { | |
| totalTokens := int64(usage.TotalTokens()) | |
| cost := b.imageTokenCostMillicents(totalTokens) // reuse vision cost rate | |
| if cost == 0 && totalTokens == 0 { | |
| cost = b.fallbackImageCostMillicents | |
| } | |
| if cost <= 0 { | |
| return | |
| } | |
| if !b.UseServerQuota() { | |
| if err := b.store.InsertLLMUsage("video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil { | |
| logger.Warn(ctx, "llm_usage_insert_failed", | |
| zap.String("tenant_id", b.tenantID), | |
| zap.String("task_type", "video_extract_visual"), | |
| zap.String("task_id", taskID), | |
| zap.Error(err)) | |
| metrics.RecordTenantOperation(b.tenantID, "llm_cost_budget", "usage_insert", "error", 0) | |
| } | |
| } | |
| if err := b.syncCentralLLMCostRecord(ctx, "video_extract_visual", taskID, cost, totalTokens, "tokens"); err != nil { | |
| logger.Warn(ctx, "central_quota_llm_cost_record_failed", | |
| zap.String("tenant_id", b.tenantID), | |
| zap.String("task_type", "video_extract_visual"), | |
| zap.String("task_id", taskID), | |
| zap.Error(err)) | |
| metrics.RecordTenantOperation(b.tenantID, "central_quota", "llm_cost_record", "log_error", 0) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/backend/llm_usage.go` around lines 98 - 126, Update
recordVideoExtractUsage to accept ctx context.Context as its first parameter,
then use that context for logging and syncCentralLLMCostRecord instead of
backgroundWithTrace(). Update ProcessVideoExtractTask to pass its existing
context when invoking recordVideoExtractUsage, preserving the current
usage-recording behavior.
Source: Coding guidelines
| if result != VideoExtractResultWritten { | ||
| t.Fatalf("result=%q, want %q", result, VideoExtractResultWritten) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use t.Errorf/t.Error for assertion failures.
As per coding guidelines, tests must use t.Fatal/t.Fatalf only for setup failures and t.Errorf/t.Error for assertion failures.
pkg/backend/video_extract_test.go#L40-L42: change tot.Errorf.pkg/backend/video_extract_test.go#L44-L46: change tot.Errorf.pkg/backend/video_extract_test.go#L59-L61: change tot.Error.pkg/backend/video_extract_test.go#L62-L64: change tot.Errorf.pkg/backend/video_extract_test.go#L85-L87: change tot.Errorf.pkg/backend/video_extract_test.go#L107-L109: change tot.Errorf.pkg/backend/video_extract_test.go#L130-L132: change tot.Errorf.pkg/backend/video_extract_test.go#L161-L163: change tot.Error.pkg/backend/video_extract_test.go#L164-L166: change tot.Error.pkg/backend/video_extract_test.go#L167-L169: change tot.Error.pkg/backend/video_extract_test.go#L194-L196: change tot.Error.pkg/backend/video_extract_test.go#L206-L208: change tot.Error.pkg/backend/video_extract_test.go#L209-L211: change tot.Error.pkg/backend/video_extract_test.go#L231-L233: change tot.Error.pkg/backend/video_extract_test.go#L237-L239: change tot.Error.pkg/backend/video_extract_test.go#L244-L246: change tot.Error.pkg/backend/video_extract_test.go#L247-L249: change tot.Error.pkg/server/upload_test.go#L684-L686: change tot.Errorfand append an immediatereturnto prevent a panic on the next line if the length is0.pkg/server/upload_test.go#L687-L689: change tot.Errorf.pkg/server/upload_test.go#L690-L692: change tot.Errorf.
📍 Affects 2 files
pkg/backend/video_extract_test.go#L40-L42(this comment)pkg/backend/video_extract_test.go#L44-L46pkg/backend/video_extract_test.go#L59-L61pkg/backend/video_extract_test.go#L62-L64pkg/backend/video_extract_test.go#L85-L87pkg/backend/video_extract_test.go#L107-L109pkg/backend/video_extract_test.go#L130-L132pkg/backend/video_extract_test.go#L161-L163pkg/backend/video_extract_test.go#L164-L166pkg/backend/video_extract_test.go#L167-L169pkg/backend/video_extract_test.go#L194-L196pkg/backend/video_extract_test.go#L206-L208pkg/backend/video_extract_test.go#L209-L211pkg/backend/video_extract_test.go#L231-L233pkg/backend/video_extract_test.go#L237-L239pkg/backend/video_extract_test.go#L244-L246pkg/backend/video_extract_test.go#L247-L249pkg/server/upload_test.go#L684-L686pkg/server/upload_test.go#L687-L689pkg/server/upload_test.go#L690-L692
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/backend/video_extract_test.go` around lines 40 - 42, Replace
assertion-only t.Fatal/t.Fatalf calls with the specified t.Error/t.Errorf
variants throughout pkg/backend/video_extract_test.go at lines 40-42, 44-46,
59-61, 62-64, 85-87, 107-109, 130-132, 161-163, 164-166, 167-169, 194-196,
206-208, 209-211, 231-233, 237-239, 244-246, and 247-249. In
pkg/server/upload_test.go, change lines 684-686, 687-689, and 690-692 to
t.Errorf, and add an immediate return after the length assertion at 684-686 to
prevent indexing an empty result; keep fatal calls only for setup failures.
Source: Coding guidelines
| func (b *Dat9Backend) loadVideoBytesForExtract(ctx context.Context, f *datastore.File) ([]byte, error) { | ||
| if b.videoExtractMaxSize > 0 && f.SizeBytes > b.videoExtractMaxSize { | ||
| return nil, errVideoExtractSourceTooLarge | ||
| } | ||
| if f.StorageType == datastore.StorageDB9 { | ||
| if b.videoExtractMaxSize > 0 && int64(len(f.ContentBlob)) > b.videoExtractMaxSize { | ||
| return nil, errVideoExtractSourceTooLarge | ||
| } | ||
| return append([]byte(nil), f.ContentBlob...), nil | ||
| } | ||
| if f.StorageType != datastore.StorageS3 { | ||
| return nil, fmt.Errorf("unsupported storage type: %s", f.StorageType) | ||
| } | ||
| if b.s3 == nil { | ||
| return nil, fmt.Errorf("s3 client not configured") | ||
| } | ||
| rc, err := b.s3.GetObject(ctx, f.StorageRef) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer func() { _ = rc.Close() }() | ||
|
|
||
| reader := io.Reader(rc) | ||
| if b.videoExtractMaxSize > 0 { | ||
| reader = io.LimitReader(rc, b.videoExtractMaxSize+1) | ||
| } | ||
| data, err := io.ReadAll(reader) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if b.videoExtractMaxSize > 0 && int64(len(data)) > b.videoExtractMaxSize { | ||
| return nil, errVideoExtractSourceTooLarge | ||
| } | ||
| return data, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add an end-to-end memory budget for video extraction.
The pipeline retains the full source, extracted JPEGs, base64 strings, and the JSON payload in memory at once. Stream or bound the source into a temp file, and cap/scale frame data before assembling the OpenAI request.
📍 Affects 2 files
pkg/backend/video_extract.go#L258-L291(this comment)pkg/backend/video_extract_openai.go#L153-L238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/backend/video_extract.go` around lines 258 - 291, The video extraction
flow must enforce an end-to-end memory budget instead of retaining the full
source, extracted JPEGs, base64 data, and JSON simultaneously. Update
Dat9Backend.loadVideoBytesForExtract in pkg/backend/video_extract.go (lines
258-291) to stream or bound S3/source data through a temporary file rather than
returning the entire source in memory, and update the frame-processing/request
assembly flow in pkg/backend/video_extract_openai.go (lines 153-238) to cap or
scale frame data before constructing the OpenAI payload.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b0dfaa2fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isImage := b.hasAsyncImageTextSource(path, contentType) | ||
| isAudio := b.shouldEnqueueAudioExtractTask(path, contentType) | ||
| if !isImage && !isAudio { | ||
| isVideo := b.shouldEnqueueVideoExtractTask(path, contentType) |
There was a problem hiding this comment.
When video extraction is enabled, this adds video_extract_visual tasks to the same quota-gated enqueue path, but the currentMediaDelta passed by all callers is still computed with isQuotaMediaContentType, which only counts image/ and audio/ content types. A tenant at its MaxMediaLLMFiles limit can therefore keep uploading videos and each one will enqueue a Vision job without increasing the media count, bypassing the intended LLM-file cap.
Useful? React with 👍 / 👎.
| if isVideo { | ||
| created, err := b.enqueueVideoExtractTaskTx(tx, fileID, revision, path, contentType) |
There was a problem hiding this comment.
For TiDB tenants whose embedding profile resolves to non-auto/FTS-only, tenant.Pool.createBackend currently clears AsyncImageExtract and AsyncAudioExtract but not the new AsyncVideoExtract. With this enqueue path, those tenants will still create and process video_extract_visual tasks whenever the server template has video extraction enabled, unexpectedly running Vision work (and incurring LLM cost) in the same mode where image/audio extraction was explicitly disabled.
Useful? React with 👍 / 👎.
| if cost == 0 && totalTokens == 0 { | ||
| cost = b.fallbackImageCostMillicents |
There was a problem hiding this comment.
When the Vision provider omits token usage, video extraction records only one fallbackImageCostMillicents even though the request can include multiple frames (FramesExtracted, default max 10). In that provider mode the monthly LLM budget and central cost accounting can undercount each video by the number of sampled frames, allowing tenants to exceed the intended spend cap before monthlyLLMCostExceededCheck starts skipping work.
Useful? React with 👍 / 👎.
FFmpeg is needed by the video visual extraction pipeline to sample frames from uploaded videos before sending them to the Vision API. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
GREEN at exact head 9972980.\n\nPost-R3 delta reviewed: only Dockerfile changes from prior GREEN head 2b0dfaa, adding Alpine ffmpeg alongside ca-certificates for the product-enabled video extraction container path. This is consistent with the FFmpeg dependency introduced by frame extraction.\n\nValidation:\n- git diff --name-status 2b0dfaa..origin/pr-751 => Dockerfile only\n- git diff --check 2b0dfaa..origin/pr-751 passed\n- GitHub ci success\n- GitHub drive9-server-local e2e smoke success\n- CodeRabbit success\n\nBoundary remains unchanged: this approves the default-off product pipeline plus Docker image dependency. It does not claim real Vision API/content-effect staging smoke has been performed, and it does not add tenant allowlist/count quota controls.\n\nNote: formal GitHub approval from this machine failed with "Review Can not approve your own pull request"; this comment is the review record. |
Global default remains off. When DRIVE9_VIDEO_EXTRACT_ENABLED=true, only tenants listed in DRIVE9_VIDEO_EXTRACT_TENANT_ALLOWLIST (comma- separated) may enqueue video extraction tasks. Omitting the env var allows all tenants (backwards-compatible). An empty value blocks all. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
REVISE at exact head 05f2ce1.\n\nBlocking issue: the tenant allowlist is fail-open when the env allowlist is unset/empty. In cmd/drive9-server/main.go lines 934-949, tenantAllowlist remains nil unless DRIVE9_VIDEO_EXTRACT_TENANT_ALLOWLIST has a non-empty value. In pkg/backend/semantic_tasks.go lines 107-113, nil means no filtering, so DRIVE9_VIDEO_EXTRACT_ENABLED=true with no allowlist enables video extraction for every tenant. That violates the stated rollout requirement: global default false, and when enabled only explicitly selected tenants should receive video visual extraction.\n\nThe new test also codifies the wrong contract: pkg/backend/video_extract_test.go lines 243-255 asserts nil allowlist allows all tenants. That is incompatible with the requested shared-production safety gate.\n\nExpected gate for this PR/follow-up: with video extraction globally enabled but no tenant allowlist configured, either startup must fail closed or no tenants may enqueue video extraction. Non-allowlisted tenants must upload/list/stat/download normally, with no video_extract_visual task and no FFmpeg/Vision call. Allowlisted tenants should retain the happy path. Also ensure logs distinguish missing/empty allowlist from actual allowlist size, because tenant_allowlist_size=0 currently could mean either all tenants allowed or none allowed depending on nil vs empty map semantics.\n\nValidation performed: git diff --check 9972980..origin/pr-751 passed; GitHub ci/e2e are still in progress, but this is a logic/spec blocker independent of CI. |
When DRIVE9_VIDEO_EXTRACT_ENABLED=true but DRIVE9_VIDEO_EXTRACT_TENANT_ALLOWLIST is unset, default to empty map so no tenant can enqueue. Operators must explicitly list tenant IDs. Addresses adversary-1 review: nil map no longer means "all tenants allowed". Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Adversarial review: GREEN for head The previous tenant-allowlist blocker is closed. The updated code now fails closed for nil/empty tenant allowlists, only enqueues Validation:
Local focused runtime Review boundary remains explicit: this GREEN covers the default-off, tenant-allowlisted Drive9 product pipeline and Docker FFmpeg dependency. It still does not prove a real Bailian/DashScope Vision API smoke with a real MP4, real model response, final bilingual |
Replace DRIVE9_VIDEO_EXTRACT_ENABLED + DRIVE9_VIDEO_EXTRACT_TENANT_ALLOWLIST with just DRIVE9_VIDEO_EXTRACT_TENANT_ALLOWLIST: - empty/unset = off (no Vision/FFmpeg init) - "*" = all tenants - "tenant-a,tenant-b" = only listed tenants Validation: "*" must be sole token (no mixing with IDs), glob patterns like "tenant-*" are rejected. Parse errors fail startup. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Adversarial re-review for head Blocker: the new single-config parser is not covered by regression tests. The rollout safety contract now lives in This is exactly the code path that prevents another fail-open rollout bug, so it needs tests in
Validation I ran:
The implementation direction is acceptable. The missing parser tests are the blocker. |
Extract allowlist parser from main.go into backend package as a pure function. Add table-driven tests covering: empty, single/multi tenant, whitespace, wildcard "*", trailing comma, mixed wildcard+tenant (error), glob patterns like "tenant-*" (error). Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Adversarial re-review for head The previous parser-test blocker is mostly addressed, but one required empty-token edge is still undefined and currently behaves inconsistently.
This exact class was in the prior requested matrix: define and test empty-token behavior for Required fix: make all-empty token lists either explicitly off or explicit startup/config errors; I recommend config error for Validation I ran:
This is a small fix, but it is still in the rollout-control parser. I would not merge until this ambiguity is removed. |
All-empty tokens (e.g. "," or ", ,") now return off instead of
initializing video runtime with an empty allowlist. Interior empty
tokens ("a,,b") are silently skipped, keeping "a" and "b". Added
three new test cases: only-commas, commas-and-spaces, interior-empty.
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
REVISE at Blocking issue remains: comma-only allowlist is documented/tested as off in Evidence:
This violates the chosen single-var contract: comma-only values are supposed to behave like off / no Vision runtime initialization. The parser unit tests are not enough because they do not cover the env-to-runtime builder. Local proof: DRIVE9_VIDEO_EXTRACT_TENANT_ALLOWLIST=, go test ./cmd/drive9-server -run TestBuildBackendOptionsFromEnvAudioDisabled -count=1Fails with: Required fix:
|
Move the early-return check after ParseVideoExtractTenantAllowlist so that all-comma inputs like "," or ", ," are treated as off and do not require API key/model env vars. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
REVISE at The runtime bug appears fixed, but the regression test is still missing. What changed correctly:
Manual proof now passes: DRIVE9_VIDEO_EXTRACT_TENANT_ALLOWLIST=, go test ./cmd/drive9-server -run TestBuildBackendOptionsFromEnvAudioDisabled -count=1But this is not a committed regression test. Required fix before GREEN:
Validation run:
|
Tests buildVideoExtractOptionsFromEnv directly with t.Setenv: - off cases: unset, only-commas, commas-and-spaces, whitespace - invalid: glob prefix "tenant-*", mixed wildcard "*,tenant-a" - missing API envs with tenant allowlist set - missing API envs with wildcard "*" No Docker/testcontainers dependency — runs locally. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
GREEN at The previous single-config allowlist blockers are fixed:
Validation:
Boundary unchanged: this GREEN covers the default-off / allowlisted video visual extraction product pipeline and regression coverage. It still does not prove real Bailian/DashScope Vision quality/effect with a real MP4, real model response, final bilingual Note: formal GitHub approve is unavailable from this local identity because GitHub reports it as the PR author. |
Add MaxVideoLLMFiles quota independent from the existing media LLM quota. Counts distinct resource_ids with video_extract_visual tasks. Default 50 per tenant, configurable via DRIVE9_VIDEO_EXTRACT_MAX_FILES. When exceeded, video files still upload normally but no extraction tasks are enqueued. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
REVISE at exact head Blocking issues:
Local validation I ran: |
- Change count > max to count >= max (check is pre-enqueue, so >= is correct boundary for the Nth file) - Video uses its own quota, not the general media LLM quota. Image and audio still share the media quota independently. - Add tests for default (50) and custom MaxVideoLLMFiles values. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
REVISE at exact head Good movement: the shared media quota is no longer applied to pure video extraction ( Blocking issues:
Required next fix: add behavioral tests around Validation run locally: |
- videoLLMQuotaExceededTx now excludes the current fileID so re-uploads of existing videos are never blocked by the quota (only new distinct files count toward the limit). - Add env tests for DRIVE9_VIDEO_EXTRACT_MAX_FILES default (50) and custom value parsing. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
REVISE at exact head Findings:
What changed:
Validation:
Verdict: REVISE. Do not merge until the quota behavior itself is tested. |
TestVideoQuotaSkipsEnqueueAtLimit verifies: - First video (within quota) enqueues and succeeds - Second distinct video (exceeds quota) uploads OK but no task created - Re-upload of existing video at full quota still re-enqueues (fileID excluded from count) Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/drive9-server/video_extract_env_test.go (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
t.Errorffor assertion failures.As per coding guidelines, use
t.Fatal/t.Fatalffor setup failures andt.Errorffor assertion failures. Since these checks are validating the final outcome of the test rather than a prerequisite setup step, they should be changed to uset.Errorf.
cmd/drive9-server/video_extract_env_test.go#L31-L33: Changet.Fataltot.Errorf.cmd/drive9-server/video_extract_env_test.go#L55-L57: Changet.Fatalftot.Errorf.cmd/drive9-server/video_extract_env_test.go#L69-L71: Changet.Fataltot.Errorf.cmd/drive9-server/video_extract_env_test.go#L81-L83: Changet.Fataltot.Errorf.cmd/drive9-server/video_extract_env_test.go#L97-L99: Changet.Fatalftot.Errorf.cmd/drive9-server/video_extract_env_test.go#L113-L115: Changet.Fatalftot.Errorf.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/drive9-server/video_extract_env_test.go` around lines 31 - 33, Replace the assertion-failure calls in cmd/drive9-server/video_extract_env_test.go at lines 31-33, 55-57, 69-71, 81-83, 97-99, and 113-115: use t.Errorf instead of t.Fatal or t.Fatalf, preserving each existing assertion message and arguments.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/backend/quota.go`:
- Around line 381-391: Update Dat9Backend.videoLLMQuotaExceededTx to accept
context.Context as its first parameter, use QueryRowContext with that context,
and pass it to the warning logger instead of backgroundWithTrace(). Update the
existing caller in semantic_tasks.go to forward its available context.
In `@pkg/server/upload_test.go`:
- Around line 744-746: Change the task-count assertion for tasks2 in the upload
test from t.Fatalf to t.Errorf, preserving the existing failure message and
allowing subsequent re-upload assertions to run.
---
Nitpick comments:
In `@cmd/drive9-server/video_extract_env_test.go`:
- Around line 31-33: Replace the assertion-failure calls in
cmd/drive9-server/video_extract_env_test.go at lines 31-33, 55-57, 69-71, 81-83,
97-99, and 113-115: use t.Errorf instead of t.Fatal or t.Fatalf, preserving each
existing assertion message and arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb6d3aa0-93e3-4894-ad99-2d4dd2d321f6
📒 Files selected for processing (8)
cmd/drive9-server/main.gocmd/drive9-server/video_extract_env_test.gopkg/backend/dat9.gopkg/backend/options.gopkg/backend/quota.gopkg/backend/semantic_tasks.gopkg/backend/video_extract_test.gopkg/server/upload_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/backend/dat9.go
- cmd/drive9-server/main.go
- pkg/backend/options.go
- pkg/backend/video_extract_test.go
- pkg/backend/semantic_tasks.go
| func (b *Dat9Backend) videoLLMQuotaExceededTx(tx *sql.Tx, fileID string) bool { | ||
| if b.maxVideoLLMFiles <= 0 { | ||
| return false | ||
| } | ||
| var count int64 | ||
| if err := tx.QueryRow(`SELECT COUNT(DISTINCT resource_id) FROM semantic_tasks WHERE task_type = 'video_extract_visual' AND resource_id != ?`, fileID).Scan(&count); err != nil { | ||
| logger.Warn(backgroundWithTrace(), "video_llm_quota_check_fail_open", zap.Error(err)) | ||
| return false | ||
| } | ||
| return count >= b.maxVideoLLMFiles | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Pass context.Context as the first parameter to database calls.
As per coding guidelines, context.Context must be passed as the first parameter to all I/O and database calls. This enables proper cancellation and request tracing. Please update the function signature to accept ctx and use it with QueryRowContext and the logger, rather than falling back to backgroundWithTrace().
Note: The downstream caller in pkg/backend/semantic_tasks.go already has a context available and will need to be updated to pass it.
🛠️ Proposed fix to propagate context
-func (b *Dat9Backend) videoLLMQuotaExceededTx(tx *sql.Tx, fileID string) bool {
+func (b *Dat9Backend) videoLLMQuotaExceededTx(ctx context.Context, tx *sql.Tx, fileID string) bool {
if b.maxVideoLLMFiles <= 0 {
return false
}
var count int64
- if err := tx.QueryRow(`SELECT COUNT(DISTINCT resource_id) FROM semantic_tasks WHERE task_type = 'video_extract_visual' AND resource_id != ?`, fileID).Scan(&count); err != nil {
- logger.Warn(backgroundWithTrace(), "video_llm_quota_check_fail_open", zap.Error(err))
+ if err := tx.QueryRowContext(ctx, `SELECT COUNT(DISTINCT resource_id) FROM semantic_tasks WHERE task_type = 'video_extract_visual' AND resource_id != ?`, fileID).Scan(&count); err != nil {
+ logger.Warn(ctx, "video_llm_quota_check_fail_open", zap.Error(err))
return false
}
return count >= b.maxVideoLLMFiles
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/backend/quota.go` around lines 381 - 391, Update
Dat9Backend.videoLLMQuotaExceededTx to accept context.Context as its first
parameter, use QueryRowContext with that context, and pass it to the warning
logger instead of backgroundWithTrace(). Update the existing caller in
semantic_tasks.go to forward its available context.
Source: Coding guidelines
|
REVISE at head New requirement from qiffang: repeated video uploads should consume repeated semantic-search quota. Example: tenant default quota 50, the same video uploaded 50 times should exhaust the quota. Current implementation does not match that contract:
If the intended product contract is per extraction/upload attempt rather than per distinct video file, this PR needs a quota model/test update before GREEN. Keep upload success behavior if desired, but the over-quota path must not enqueue FFmpeg/Vision, and tests should prove same-file repeated uploads exhaust/skip exactly according to the new rule. |
Per @qiffang's decision (option 2): each Vision API call consumes one quota unit, including re-extractions of the same file. This prevents abuse via repeated overwrites of the same video path. - COUNT(DISTINCT resource_id) → COUNT(*) - Remove resource_id exclusion from quota check - Update test: re-upload of same file now blocked when quota exhausted - Update option comment to reflect per-extraction semantics Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/backend/quota.go (1)
381-391: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPass
context.Contextas the first parameter to database calls.As per coding guidelines,
context.Contextmust be passed as the first parameter to all I/O and database calls. This enables proper cancellation and request tracing. Please update the function signature to acceptctxand use it withQueryRowContextand the logger, rather than falling back tobackgroundWithTrace().Note: The downstream caller in
pkg/backend/semantic_tasks.gowill need to be updated to pass its available context.🛠️ Proposed fix to propagate context
-func (b *Dat9Backend) videoLLMQuotaExceededTx(tx *sql.Tx) bool { +func (b *Dat9Backend) videoLLMQuotaExceededTx(ctx context.Context, tx *sql.Tx) bool { if b.maxVideoLLMFiles <= 0 { return false } var count int64 - if err := tx.QueryRow(`SELECT COUNT(*) FROM semantic_tasks WHERE task_type = 'video_extract_visual'`).Scan(&count); err != nil { - logger.Warn(backgroundWithTrace(), "video_llm_quota_check_fail_open", zap.Error(err)) + if err := tx.QueryRowContext(ctx, `SELECT COUNT(*) FROM semantic_tasks WHERE task_type = 'video_extract_visual'`).Scan(&count); err != nil { + logger.Warn(ctx, "video_llm_quota_check_fail_open", zap.Error(err)) return false } return count >= b.maxVideoLLMFiles }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/backend/quota.go` around lines 381 - 391, Update Dat9Backend.videoLLMQuotaExceededTx to accept context.Context as its first parameter, use that context with QueryRowContext and the quota-check warning logger, and remove the backgroundWithTrace fallback. Update the downstream caller in semantic_tasks.go to pass its available context while preserving the existing quota behavior.Source: Coding guidelines
♻️ Duplicate comments (1)
pkg/server/upload_test.go (1)
744-746: 📐 Maintainability & Code Quality | 🟡 Minor | 💤 Low valueUse
t.Errorffor assertion failures. As per coding guidelines, uset.Fatal/t.Fatalffor setup failures andt.Errorffor assertion failures so the test can report multiple failures before aborting.
pkg/server/upload_test.go#L744-L746: changet.Fatalftot.Errorffor this task count assertion.pkg/server/upload_test.go#L765-L767: changet.Fatalftot.Errorffor this task count assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/upload_test.go` around lines 744 - 746, In the task-count assertions in pkg/server/upload_test.go at lines 744-746 and 765-767, replace t.Fatalf with t.Errorf while preserving the existing failure messages and checks, so both assertion failures can be reported without aborting the test.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/backend/quota.go`:
- Around line 381-391: Update Dat9Backend.videoLLMQuotaExceededTx to accept
context.Context as its first parameter, use that context with QueryRowContext
and the quota-check warning logger, and remove the backgroundWithTrace fallback.
Update the downstream caller in semantic_tasks.go to pass its available context
while preserving the existing quota behavior.
---
Duplicate comments:
In `@pkg/server/upload_test.go`:
- Around line 744-746: In the task-count assertions in pkg/server/upload_test.go
at lines 744-746 and 765-767, replace t.Fatalf with t.Errorf while preserving
the existing failure messages and checks, so both assertion failures can be
reported without aborting the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec6238a6-a20c-40de-8196-4df8a5066f80
📒 Files selected for processing (4)
pkg/backend/options.gopkg/backend/quota.gopkg/backend/semantic_tasks.gopkg/server/upload_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/backend/semantic_tasks.go
|
GREEN at exact head No remaining blocking findings from my gate. The final quota blocker is fixed against qiffang's option-2 contract: video quota is now per extraction attempt / Vision call, not per distinct file. Validation:
Local focused Scope boundary remains unchanged: GREEN covers the default-off / tenant-allowlisted video visual extraction pipeline, Docker FFmpeg dependency, and per-extraction video quota. It is not proof of a real Bailian/DashScope Vision smoke with production credentials and final content/search quality. |
1 participant
Summary
video_extract_visualsemantic task type for extracting searchable text from video filesImplementation
pkg/semantic/task.go:TaskTypeVideoExtractVisual+VideoExtractTaskPayloadpkg/backend/video_extract.go:VideoTextExtractorinterface,ProcessVideoExtractTaskhandler, MIME allowlistpkg/backend/video_extract_openai.go:OpenAIVideoTextExtractorwith FFmpeg + multi-image Vision APIpkg/backend/options.go:AsyncVideoExtractOptionsconfigpkg/backend/semantic_tasks.go: enqueue helpers + routingpkg/backend/llm_usage.go: cost tracking (reuses vision token rate)pkg/server/tenant_worker_semantic.go: dispatch + spec conversionpkg/server/tenant_worker.go: task type registrationcmd/drive9-server/main.go:DRIVE9_VIDEO_EXTRACT_*env var wiringConfiguration
DRIVE9_VIDEO_EXTRACT_ENABLED=true
DRIVE9_VIDEO_EXTRACT_API_BASE, DRIVE9_VIDEO_EXTRACT_API_KEY, DRIVE9_VIDEO_EXTRACT_MODEL
DRIVE9_VIDEO_EXTRACT_FRAME_INTERVAL=5, DRIVE9_VIDEO_EXTRACT_MAX_FRAMES=10
DRIVE9_VIDEO_EXTRACT_MAX_BYTES=200MB, DRIVE9_FFMPEG_PATH=ffmpeg
Test plan
Summary by CodeRabbit