fix: harden API-cost webhook outbox retries (MYX-79) - #869
fix: harden API-cost webhook outbox retries (MYX-79)#869groupthinking wants to merge 35 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔍 PR Validation |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
📝 WalkthroughWalkthroughThe API-cost monitor now uses a persisted, scheduled webhook outbox with managed worker lifecycle, bounded retries, stale-claim recovery, atomic delivery claims, and idempotency headers. FastAPI starts and closes the monitor through application lifespan management. ChangesWebhook outbox delivery
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FastAPI
participant APICostMonitor
participant OutboxWorker
participant WebhookEndpoint
FastAPI->>APICostMonitor: start()
APICostMonitor->>OutboxWorker: run managed worker
OutboxWorker->>APICostMonitor: process due outbox item
APICostMonitor->>WebhookEndpoint: POST with idempotency header
WebhookEndpoint-->>APICostMonitor: delivery result
APICostMonitor->>OutboxWorker: persist sent or retry state
FastAPI->>APICostMonitor: close()
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
groupthinking
added
async-fix
bug
ChatGPT Codex Connector
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/youtube_extension/backend/services/api_cost_monitor.py (1)
651-717: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove the outbox worker off the event loop.
recover_stale_deliveries(), claim, and completion all use a synchronousSessioninside async worker methods. With the 30-second SQLite timeout, lock contention can freeze the FastAPI loop and delay unrelated requests/shutdown. Switch this path toAsyncSession/create_async_engine, or wrap each transaction inasyncio.to_thread()with a context-managed session.🤖 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 `@src/youtube_extension/backend/services/api_cost_monitor.py` around lines 651 - 717, Move the synchronous database work in recover_stale_deliveries and the outbox claim/completion methods out of the event loop by using AsyncSession with an async engine, or by wrapping each transaction in asyncio.to_thread with a context-managed Session. Ensure all queries, updates, commits, rollbacks, and session cleanup in these worker paths execute off-loop while preserving their existing concurrency and recovery behavior.Sources: Coding guidelines, Path instructions
🤖 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 `@src/youtube_extension/backend/services/api_cost_monitor.py`:
- Line 220: Update the _worker_task attribute and the start() method’s
worker-task typing to use asyncio.Task[None] instead of bare asyncio.Task. Apply
the parameterized type consistently at the declaration and task
creation/assignment path while preserving existing worker behavior.
In `@src/youtube_extension/main.py`:
- Around line 49-67: Update _app_lifespan with the AsyncIterator[None] return
annotation and import the required typing symbol. In its shutdown finally block,
wrap cost_monitor.close() in exception handling, log cleanup failures with
logger.exception, and preserve any exception raised by the application body.
---
Outside diff comments:
In `@src/youtube_extension/backend/services/api_cost_monitor.py`:
- Around line 651-717: Move the synchronous database work in
recover_stale_deliveries and the outbox claim/completion methods out of the
event loop by using AsyncSession with an async engine, or by wrapping each
transaction in asyncio.to_thread with a context-managed Session. Ensure all
queries, updates, commits, rollbacks, and session cleanup in these worker paths
execute off-loop while preserving their existing concurrency and recovery
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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93728548-6ec0-452b-955a-3c078e02c588
⛔ Files ignored due to path filters (3)
tests/unit/test_api_cost_monitor.pyis excluded by!tests/**tests/unit/test_api_cost_monitor_lifecycle.pyis excluded by!tests/**tests/unit/test_api_cost_outbox_worker.pyis excluded by!tests/**
📒 Files selected for processing (2)
src/youtube_extension/backend/services/api_cost_monitor.pysrc/youtube_extension/main.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
groupthinking/uvai-skills(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Vercel Agent Review
⚠️ CI failures not shown inline (1)
Commit Status: Vercel: Vercel
Conclusion: failure
Canceled from the Vercel Dashboard
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{py,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,jsx,ts,tsx}: Use Python 3.9+ and Node 18+ for development
Never hardcode API keys, database URLs, or secrets in code
Make minimal, surgical changes and avoid deleting working code unless fixing security issues
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for Python functions
Use Black formatter with 88 character line length for Python code
Follow PEP 8 conventions for Python code
Use descriptive variable names and add docstrings to all public functions in Python
Group imports in Python: standard library, third-party, local
Use SQLAlchemy ORM and never write raw SQL queries
Use environment variables via os.getenv() or pydantic-settings to access configuration
Wrap database operations in try-except blocks and use context managers or FastAPI dependencies for connection cleanup
Implement comprehensive error handling with proper logging in all functions
Version APIs using /api/v1/ prefix for stability
Use JSON-RPC 2.0 protocol for all MCP communication
Follow the single-flow workflow: YouTube link → context extraction → agent dispatch → outputs
Use context managers for resource management in Python code
Implement comprehensive input validation and sanitize outputs for security
Use parameterized queries and SQLAlchemy ORM to prevent SQL injection
Define API request/response models using Pydantic for FastAPI endpoints
Store the single unified workflow as the only workflow; never introduce alternate flows or manual triggers
Use SQLAlchemy with connection pooling for database connections and manage sessions with context managers
Provide sensible defaults for non-sensitive configuration in Python settings
Maintain backward compatibility and do not break existing API endpoints
Include comprehensive logging for debugging in MCP implementations
**/*.py: Format Python code with Black using an 88-character line length.
Use Ruff with rules E, W, F, I, B, C4, and UP; E501 is ignored.
Use strict mypy checking with untyped function definitions disallowed.
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
⚙️ CodeRabbit configuration file
Python backend code. Check for type hints, proper exception handling, async context manager usage, and potential blocking calls in async functions. Flag any bare except clauses or missing timeout parameters on network calls. CRITICAL: Flag any file that contains placeholder/stub implementations — especially in unified_ai_sdk. Any class or function that says "TODO: Replace with production implementation" or returns mock/fake data must be flagged as a blocking issue. Flag any code generation output that reaches users without AST validation or syntax checking.
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,ts,tsx}: Keep frontend and backend data models synchronized using matching Pydantic (backend) and TypeScript (frontend) interfaces
Use type-safe interfaces for backend-frontend data exchange
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Strictly verify that GitHub Copilot has explicitly reviewed and approved the pull request; human approvals alone must not satisfy this check.
Before allowing a merge, require thecopilot-rabbitlabel and AI-generated unit tests committed alongside the code changes; fail the check if either is missing.
**/*: Follow the documented event naming convention<domain>.<entity>.<action>, such asyoutube.video.captured.
Use the service-container dependency injection pattern for backend dependencies.
Never infer SDK types from tests or API documentation alone; use backend response models as the authority.
When auditing branches, use thebranch-cleanupskill and its six-gate fail-test harness; archive branches withgit tag archive/<branch>before deletion, and do not rely on three-dot diffs orgit merge-treefor orphaned branches.For Vercel-specific work, include
https://vercel.com/docs/llms-full.txtin the AI assistant context set.
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Format Python code with Black using an 88-character line length.
Sort Python imports with isort using the Black profile.
Use Ruff with E, W, F, I, B, C4, and UP rules; E501 is ignored.
Use mypy strict mode; untyped function definitions are disallowed.
Target Python 3.9 or newer.
Validate backend inputs with Pydantic and sanitize subprocess arguments.
Maintain strict mypy type safety in the Python backend.
Use the required Anthropic SDK parametersthinking={"type": "adaptive"}andoutput_config={"effort": "..."}with the current model stringclaude-opus-4-8; do not addTypeErrorcompatibility fallbacks.
src/**/*.py: Do not introduce alternative workflows or manual triggers that bypass the single YouTube link → transcript → events → agents → outputs pipeline.
Use event names in the<domain>.<entity>.<action>format.
Use the service-container dependency-injection pattern for dependencies.
Use Pydantic input validation and sanitize subprocess arguments.
Production code must use real behavior only; do not add mock delays or fake data.
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{py,ts,tsx,js,jsx}: Do not use mock delays, fake data, or simulated responses in production code; production must remain REAL_MODE_ONLY.
Do not hard-code secrets, keys, or credentials; store them in.envfiles that are gitignored.Do not include secrets or API keys in source code; load them from environment variables instead.
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
**/*.{py,pyw}
📄 CodeRabbit inference engine (AGENTS.md)
Write Python code to remain compatible with Linux and Windows where possible, including correct handling of
asyncioevent loops.
Files:
src/youtube_extension/main.pysrc/youtube_extension/backend/services/api_cost_monitor.py
🪛 ast-grep (0.44.1)
src/youtube_extension/backend/services/api_cost_monitor.py
[info] 635-635: use secrets package over random package
Context: random.uniform(0, half_cap)
Note: [CWE-330] Use of Insufficiently Random Values.
(avoid-random-python)
🔍 Remote MCP GitHub Copilot
Most useful review context
- PR
#869is an open draft fromagent/harden-api-cost-outboxintomain, with 5 commits, 5 changed files, and +868/-89 lines. It explicitly says it advances MYX-79 but must stay draft until#868/ MYX-81 is complete. - The touched files are
src/youtube_extension/backend/services/api_cost_monitor.py,src/youtube_extension/main.py, and three test files. The new tests cover schema upgrade, worker lifecycle, compare-and-swap claims, cancellation/stale recovery, backoff, and idempotency headers. - CodeRabbit’s auto-review only selected the 2 source files; all 3 test files were excluded by
!tests/**, and the review is still marked as in progress / incremental. - The prerequisite issue
#868is open and says production still lacks shared persistent storage, an executable migration path, and a reliable worker runtime; it also calls out the current main branch as process-local SQLite with import-timecreate_all()and no durable worker deployment model. - Related PR
#859was merged on 2026-07-17 and already added once-per-UTC-day gating plus Slack/Discord payload compatibility in the same module, so#869is hardening the retry/outbox path on top of an earlier webhook change. - Repo search shows
track_api_callis used inofficial_api.pyandreal_ai_processor.py, and the production entrypoint isyoutube_extension.main:appin the Dockerfile/CLI/docs.
🔇 Additional comments (4)
src/youtube_extension/main.py (1)
9-9: LGTM!Also applies to: 21-22
src/youtube_extension/backend/services/api_cost_monitor.py (3)
11-15: LGTM!Also applies to: 30-38, 56-61, 105-112
206-219: LGTM!Also applies to: 221-221, 577-580, 592-649, 916-935
287-314: 🗄️ Data Integrity & IntegrationKeep the SQLite compatibility shim.
WebhookOutboxalready declaresnext_attempt_atandix_webhook_outbox_duefor new databases; this startup path only patches older SQLite files so existing installs keep working.> Likely an incorrect or invalid review comment.
🔍 PR Validation |
|
Addressed the current CodeRabbit findings on head
Local Black, Ruff, and @coderabbitai review |
|
✅ Action performedReview finished.
|
Controller receipt — CI compatibility correction
The prior exact head failed CI because six established callers intentionally omit a model and therefore use the legacy sentinel Applied the smallest focused correction on the canonical branch:
This supersedes |
Controller receipt — exact-head completion
Completed and verified:
The exact-head Vercel preview was canceled by unverified-commit policy. #869 was automatically promoted after checks and has been returned to draft because protected staging migration, revision-replacement durability, production-shaped worker scenarios, webhook activation review, and rollback proof remain incomplete. This partial implementation progresses #898 and does not close #861. |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Suggestion:
A supported/selectable model (gemini-2.5-flash) missing from the pricing table causes calculate_cost to raise ValueError, converting an already-successful, already-billed Gemini response into success=False and discarding the paid result.
Controller reconciliation — 2026-07-27T21:53Z
Execution receipt
Next executable action: wait for the exact-head workflows, then record their terminal results. Do not merge or activate production delivery. |
Exact-head workflow completion — 2026-07-27T21:58ZExact head remains
Keep #869 draft. The two outbox findings remain fixed and all 11 threads resolved, but exact-head CI cannot become green until the separately owned TypeScript rollback blocker is repaired through a valid focused canonical unit. |
Closing in favour of #1376Both PRs implement the durable API-cost outbox / canonical usage tracking. Measured against current
This branch has drifted to the point where its diff is no longer a review artifact — 301 files and 29k lines for a focused outbox change means the branch has absorbed most of Keeping the more complete implementation, which here means the one whose diff actually describes the change. Closing this; #1376 is canonical for GRV-79 / the API-cost outbox work. No code is lost — the branch is retained and will be archive-tagged before any branch deletion. Generated by Claude Code |
|
Correction to the numbers above. The close stands, but I gave the wrong mechanism. I described this branch as having "drifted" to 301 files. That file count was wrong — it came from a diff computed against an empty merge base, which silently degraded to a working-tree diff. The actual situation is starker: This branch shares no ancestry at all with current That strengthens rather than weakens the disposition: #1376 is not just the tidier implementation, it is the only one rooted in current Generated by Claude Code |
Status: draft — do not merge
This advances #861 / #861 and progresses parent program #898. All current inline review findings are resolved, including the atomic usage/outbox transaction and canonical v1/HybridProcessor Gemini usage instrumentation. The exact head is green. The PR remains draft only until the protected staging/production proof gates below are complete.
Implemented
next_attempt_atand due/stale-work indexes;API_COST_WEBHOOK_URLis unset;api-cost:{utc_date}:{alert_type}event ID asIdempotency-KeyandX-Event-ID;StaticPoolpluscheck_same_thread=Falsefor supported in-memory SQLite worker-thread tests;track_api_callfrom the canonical HybridProcessor path without allowing telemetry failure to discard a paid result.Verification
Previous green head
cd9964a05f200b5aa1d9fa71152a20ce44eaf23b:Historical execution head
45edc01037d72e7d2d9a56e18b2d5c2f6bb4ba76:3b4d66eand1c3ee57preserve Gemini provider usage metadata and call the durable API-cost tracker from the canonical HybridProcessor path;adbfb6band45edc01add provider-metadata and non-fatal telemetry regression tests;29810092858, API cost PostgreSQL29810092890, Coverage29810092885, CodeQL29810092856, Security Scan29810092921, Secret Scan29810092855, and Dependency Review29810092888passed; E2E29810092846was repository-skipped;Remaining merge blockers
Scope
Closes #861 only after every remaining gate is complete; progresses #898 and #861. Builds on merged #877 and reopened corrective issue #868. The PR must remain draft and unmerged until those gates are complete.
Coverage evidence correction — 2026-07-22
The previously listed
Coveragesuccess predates the authoritative workflow installed by #921. That workflow could return green after pytest collection failed because failures were suppressed. Treat the old Coverage run as non-authoritative evidence, not as proof. This PR must receive a truthful Coverage result on a current synchronized exact head before it advances; this correction does not imply a regression in this PR's code.Current exact-head reconciliation — 2026-07-22T10:23Z
1fd74ffc6fb791797428a071f75f12b0afa2a013immediately before fix(ci): make coverage and gh-aw canary authoritative #927 merged tomain. It was incorrectly marked ready while protected proof remained incomplete.main@5da61c595aa9dc848786e9e1fe99e40ad2a4fce0into this existing canonical branch without force.fd7c82d268224709487cbf0f1c5865feeb77da2d; exact tree:62bfb5ba79ed0b57b17aaa30926398cef405eaa4.7e70286585bc730ff9ea1fc7751ddefb783a517bd54defe0a85923c83bd33376.dpl_ooDwJ5UBLUgiD2S9kFTFv6Coc8dawas canceled because the controller commit is unverified; obsolete intermediate head1fd74ffc…had a verified READY preview but is not current evidence.Controller execution receipt
groupthinkingeventrelay-blocker-watch-20260722-1011zagent/harden-api-cost-outbox/ fix: harden API-cost webhook outbox retries (MYX-79) #8692026-07-22T10:11:32Z2026-07-22T10:23:12Zfd7c82d268224709487cbf0f1c5865feeb77da2dKeep draft. The next executable transition is current-head review followed by protected staging/worker proof; do not merge, activate delivery, or create another implementation.