{{ message }}
fix(api): propagate Redis errors and bound bulk commands - #4549
Open
mogery wants to merge 8 commits into
Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Member
Author
Contributor
@mogery I have started the AI code review. It will take a few minutes to complete. |
mogery
changed the title
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 26 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
cubic-dev-ai
Bot
dismissed
their stale review
September 5, 2026 18:57
Dismissed because Cubic found issues in a newer review.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 31 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Contributor
There was a problem hiding this comment.
2 issues found across 25 files (changes from recent commits).
Confidence score: 2/5
apps/api/src/controllers/v2/scrape-browser.ts—markBrowserSessionCreationFailedcan erase a billing record after a webhook has completed when destruction races with persistence cleanup, creating a concrete billing/data-integrity risk; make the failure update conditional on the row still being eligible for that update.apps/api/src/lib/browser-sessions.ts— If the post-claimBrowserSessionDestroyedupdate fails afterbillTeamsucceeds, the request rejects whilecredits_usedremains null and the prompt flag remains set, leaving inconsistent state for subsequent DELETE/webhook handling; make this cleanup reliably retryable or otherwise recoverable.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/lib/browser-sessions.ts">
<violation number="1" location="apps/api/src/lib/browser-sessions.ts:206">
P2: When this update fails after `claimBrowserSessionDestroyed` and `billTeam` succeed, the request rejects but leaves the destroyed row with `credits_used` null and the prompt flag uncleared. Subsequent DELETE/webhook attempts see the failed claim and skip persistence, so add an idempotent reconciliation path for this post-billing failure.</violation>
</file>
<file name="apps/api/src/controllers/v2/scrape-browser.ts">
<violation number="1" location="apps/api/src/controllers/v2/scrape-browser.ts:846">
P1: When a destroyed webhook races with persistence cleanup, `markBrowserSessionCreationFailed` can erase a billing record that the webhook already completed. Make this failure update conditional on the row still being active, preserving fields after another teardown claims it.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 18 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Contributor
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Member
Author
|
@cubic-dev-ai re-review please |
Contributor
Contributor
There was a problem hiding this comment.
1 issue found across 23 files (changes from recent commits).
Confidence score: 3/5
apps/api/src/lib/browser-session-finalization.tscan report success again when a completed keyless browser teardown is retried, causing the scrape-browser caller to record duplicate credits. Make finalization expose whether billing transitioned from pending and only bill on that first transition.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/lib/browser-session-finalization.ts">
<violation number="1" location="apps/api/src/lib/browser-session-finalization.ts:87">
P2: When a keyless browser teardown is retried after completion, this branch returns success again and the scrape-browser caller records the same credits again. Return whether billing transitioned from pending, or make the usage receipt idempotent, so repeated teardown requests cannot inflate keyless usage.</violation>
</file>
Tip: instead of fixing issues one by one fix them 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
1 participant
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.
Redis pipelines can resolve while individual commands fail. Check every reply in backlog, crawl tracking, URL deduplication, sitemap, upload-quota, Zscaler reply, and index-cache operations, and split variadic commands below Dragonfly limits. Failed commands throw their original error objects; transport failures propagate unchanged. No error-code allowlists or replacement error messages.
Individual Redis failures now reject browser activity, billing flags, keyless quotas, token/index caches, semaphore work, and Zscaler lookups. Sitemap callback failures propagate through fetch/parser fallback layers, and upload-quota release completes before parsing can send success. Callers await these operations instead of dropping their promises or reporting successful fallback results. Cleanup still runs after failure, preserving the original error or aggregating it with cleanup failures. Zscaler drainers process one shared batch before callers inspect replies, so a continuously nonempty queue cannot prevent completion. Callers join in-flight shared work to preserve failures; cancellation can therefore wait for that work to settle.
Browser creation rolls back its database row, provider session, and concurrency slot when Redis finalization fails. Research responses wait for keyless charging and preserve upstream errors if charging also fails. Security configuration changes are audited even when replacement sync fails. Sitemap tracking keys expire on failure, preserving both processing and expiry errors.
Browser teardown serializes completion with a database row lock and shares a frozen billing plan across DELETE, scrape teardown, and webhooks. An atomic Redis enqueue receipt allows retrying failed queue writes or database completion without enqueuing the charge twice. Incomplete sessions remain retryable, and prompt cleanup runs after commit. Ambiguous legacy external tracking remains pending for reconciliation because that provider has no idempotency contract; it is never silently treated as successful or blindly charged again. Search quota compensation uses idempotent adjustment identifiers to handle a committed adjustment whose response was lost.
Validation: The final review follow-up passes all 90 harness tests, including rollback retries, usage logging, and billing error classification. 53 cleanup/audit/search/sitemap tests pass through the harness. The final browser/billing harness run passes 206 of 208 tests; the two failures are unchanged arithmetic expectations in browser-billing.test.ts (expected 2/4 versus the existing 7-credit minimum). All new regressions pass, including concurrent teardown, failed enqueue, lost queue acknowledgement, failed database completion, strict tracking errors, and persistent quota receipts. All 18 finalization/quota tests also pass on isolated Dragonfly v1.36.0, including the ACL-induced partial-write rollback test. Full TypeScript checking, Knip, formatting, and independent review pass. No billing-rate calculation is changed.