fix: cancel media uploads when mask_otel_spans drops the batch - #1818
fix: cancel media uploads when mask_otel_spans drops the batch#1818uuzzrm wants to merge 1 commit into
Conversation
Media attributes are processed and their upload jobs enqueued before the mask_otel_spans hook runs. When the hook raises or returns an invalid result, the whole export batch is dropped -- but the media bytes behind the spans were already queued and still got uploaded, so the documented fail-closed guarantee did not hold for media. Track the media IDs enqueued while processing a batch and cancel them when the batch is dropped, so the consumer skips those uploads. Fixes #16102. Signed-off-by: Ruiming Zhao <uuzzrm@gmail.com>
| if upload_job["media_id"] in self._cancelled_media_ids: | ||
| self._cancelled_media_ids.discard(upload_job["media_id"]) | ||
| logger.debug( | ||
| f"Media: Skipping cancelled upload for media_id={upload_job['media_id']} in trace_id={upload_job['trace_id']}" | ||
| ) | ||
| self._queue.task_done() | ||
| return |
There was a problem hiding this comment.
Cancellation misses active uploads
When a media consumer dequeues a job before or while mask_otel_spans runs, it can pass this sole cancellation check and begin the network upload before the hook drops the batch, causing media from a redacted span to be uploaded. How this was verified: Media is enqueued before the synchronous mask hook runs, while independent consumer threads dequeue jobs and never re-check cancellation inside the upload path.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_task_manager/media_manager.py
Line: 69-75
Comment:
**Cancellation misses active uploads**
When a media consumer dequeues a job before or while `mask_otel_spans` runs, it can pass this sole cancellation check and begin the network upload before the hook drops the batch, causing media from a redacted span to be uploaded. **How this was verified:** Media is enqueued before the synchronous mask hook runs, while independent consumer threads dequeue jobs and never re-check cancellation inside the upload path.
**Knowledge Base Used:**
- [OTel Span Processing and Export Pipeline](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/otel-pipeline.md)
- [Task Manager: Media Upload and Score Ingestion](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/task-manager.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if upload_job["media_id"] in self._cancelled_media_ids: | ||
| self._cancelled_media_ids.discard(upload_job["media_id"]) |
There was a problem hiding this comment.
Content IDs misroute cancellation
When an exported batch and a dropped batch enqueue identical media bytes, both jobs have the same deterministic media_id, so this one-shot marker can skip the earlier legitimate job and then allow the dropped job to upload. A stale marker can likewise suppress a later legitimate upload of the same content.
Knowledge Base Used: Task Manager: Media Upload and Score Ingestion
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_task_manager/media_manager.py
Line: 69-70
Comment:
**Content IDs misroute cancellation**
When an exported batch and a dropped batch enqueue identical media bytes, both jobs have the same deterministic `media_id`, so this one-shot marker can skip the earlier legitimate job and then allow the dropped job to upload. A stale marker can likewise suppress a later legitimate upload of the same content.
**Knowledge Base Used:** [Task Manager: Media Upload and Score Ingestion](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/task-manager.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
What does this PR do?
Fixes #16102
EventSerializerhandles media extraction before themask_otel_spanshook runs, and that ordering is deliberate (the hook must see@@@langfuseMedia:reference strings, not raw bytes). The problem is the consequence: when the hook raises or returns an invalid result, the whole batch is dropped as documented, but the media upload jobs were already enqueued during attribute processing and still get uploaded. The fail-closed guarantee that "dropping the batch" covers the spans does not extend to the media behind them.This tracks the media IDs enqueued while processing a batch's attributes and cancels them when the mask hook drops the batch. The upload consumer skips cancelled media IDs, so a redacted attribute never produces an upload.
Type of change
Verification
The new regression test
test_mask_otel_spans_drop_batch_cancels_enqueued_media_uploadsfails without the fix (the upload job is processed despite the batch being dropped) and passes with it.test_pathfails on this machine only (POSIX-vs-Windows path separators) and passes in CI's Linux runner.Checklist
code_review.md..env.templateif needed.Greptile Summary
This PR attempts to extend fail-closed OpenTelemetry span masking to separately queued media uploads.
Confidence Score: 2/5
This PR is not yet safe to merge because dropped spans can still upload media under normal consumer concurrency, and content-based cancellation can suppress or misapply uploads across batches.
Media jobs are visible to background consumers before masking completes, and the sole pre-upload cancellation check can be passed before cancellation is recorded; additionally, deterministic media IDs cannot distinguish individual queued jobs from different batches.
Files Needing Attention: langfuse/_task_manager/media_manager.py, langfuse/_client/span_exporter.py, tests/unit/test_mask_otel_spans.py
Security Review
The new cancellation check does not close the concurrent upload window: a background consumer can dequeue and begin uploading media before the masking failure records the cancellation, allowing media from a dropped span to leave the process.
Sequence Diagram
sequenceDiagram participant E as Export thread participant Q as Media queue participant C as Media consumer participant M as Mask hook participant B as Backend E->>Q: Enqueue extracted media Q-->>C: Dequeue job C->>C: Check cancellation marker E->>M: Invoke mask_otel_spans C->>B: Begin media upload M-->>E: Raise / invalid result E->>C: Record cancelled media ID Note over C,B: Cancellation arrives after the only checkPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: cancel media uploads when mask_otel..." | Re-trigger Greptile
Context used: