fix(provider): bound screenshot cache by active references - #172
Merged
Merged
Conversation
sarath-menon
marked this pull request as ready for review
October 5, 2026 07:32
There was a problem hiding this comment.
All reported issues were addressed across 3 files
This PR changes concurrency-sensitive code such as locks, queues, or retries. Ultrareviews find 2.4x more serious bugs than standard reviews. Comment @cubic-dev-ai ultrareview to run one.
Fix all with cubic | Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screenshot-heavy runs failed after 64 lifetime uploads, including follow-ups that re-uploaded historical images. This replaces that lifetime limit with a bounded cache of 300 active references when the gateway advertises support, so rolling history can continue beyond 300 total uploads without changing current prompt content.
Requests pin their working set until generation finishes or their stream ends/cancels/errors. Eviction only removes unpinned entries; same-content reuse, image ordering/detail, tool history, account routing, and provider/model isolation are preserved. Failed uploads do not poison the cache. Explicit 429/502/503/504 responses receive at most three attempts; ambiguous network/500 upload outcomes are not blindly retried. Cleanup has a one-second total deadline, bounded retryable tombstones, and expiry-aware closed-cache reclamation.
Capability-negotiated
_image_file_proofsbody metadata avoids oversized proof headers. Legacy gateways retain the 64-reference contract and a 12KiB proof-header guard. Deploy gateway cloud#6307 first.Validation: nine original regression cases fail against unchanged main, then pass with the fix. An additional model-isolation regression failed before its fix. All 44 focused OpenAI/Anthropic/lifecycle tests pass, package
bun typecheckpasses, and formatting/diff checks pass. The push hook also passed all 17 typecheck tasks. Coverage includes 300 images, rolling lifetime uploads, concurrent request and stream pinning, cancellation, retries, hung deletion deadlines, bounded cleanup backlog, expired global cache entries, and body proof transport. Cubic's model-isolation and cached-request head-of-line findings were addressed with regressions; final review reported one P2 about duplicate lease registration/cleanup. Duplicate registration is removed; both bounded cleanup phases are intentional (recover backlog capacity before eviction, then reclaim newly evicted files). No remaining blocking review findings. Cache hits can proceed while unrelated uploads are pending; cache-miss preparation remains serialized to reserve capacity safely, with upload concurrency capped at two and the existing 30-second preparation deadline.Limitations: one prompt needing 301 distinct images still rejects explicitly; no screenshots are silently discarded and session compaction behavior is unchanged. New calls can also reject while concurrent requests pin all 300 slots. Explicit OpenAI stateful chains (
previousResponseIdorstore:true) conservatively retain known files until closure because deletion safety for provider-side history is unverified; such chains remain bounded at 300 known images even when the explicit prompt is smaller. Staging verification is pending coordinated gateway/agent deployment and separate AWS approval. No merge, release, deployment, or AWS change is included.Summary by cubic
Replaces the lifetime 64-upload limit for screenshots with a bounded cache of 300 active references when the gateway advertises support, so rolling history can continue beyond 300 total uploads without changing prompt content. Requests pin their working set until generation finishes or their stream ends, cancels, or errors; eviction only removes unpinned entries, and failed uploads do not poison the cache.
Retry and cleanup
Gateway contract and limitations
_image_file_proofsbody metadata avoids oversized proof headers; legacy gateways keep the 64-reference contract and a 12KiB proof-header guard.Written for commit 2dfd70b. Summary will update on new commits.
How we tested in staging
Final staging API suite: 61 passed. Released BrowserCode
0.1.21-screenshot-files.4on AgentCore 288 passed owned OpenAI GPT-5.5 and direct Anthropic Fable-5 worker runs with 65 screenshot file references, then 66 in the same session, zero inline images, and SHA256-verified visual labels plus prior-label recall. Each native adapter also processed 325 unique generated PNGs across 14 requests with 25 active images, bounded body proofs, correct visual answers, and no duplicate uploads on reuse.All owned sessions/workspaces were cleaned up; adapter plus explicit fallback deletion left zero fixture files (OpenAI: 324 delete successes + 1 already deleted; Anthropic: 323 + 2 already deleted). These live cleanup counts are aggregate, not proof that the adapter alone deleted every file; local 1,000-image/100-active churn tests cover its background drain. Ten paid runs, including diagnosed harness/provider failures, cost $10.283591. Sonnet via Bedrock remains an intentionally unsupported file route with inline fallback.
Testing began with stable matched integration
406ea87a590fe32c55a0799df2beca571a310581, preserving staging’s existing migrations; backend and worker stayed there. Concurrent control-plane deployments later advanced to50a3ba0candb8f69adad, both retaining the screenshot gateway change; the latter has identical relevant gateway files and was still rolling at final inspection. Production was unchanged. Deployment evidence: backend, control plane, worker, verified release.