fix: wrap webhook handler's arq enqueue in try/except#41
Conversation
Matches the error-handling pattern already used in the merge endpoint (lines 203-213). Without this, a Redis failure during webhook processing returns 500 to GitHub, causing delivery failures and retries. Closes #24 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe GitHub push webhook handler in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GitHub as GitHub (push webhook)
participant Handler as Webhook Handler
participant DB as Database
participant ARQ as ARQ Pool / Queue
participant Log as Logger
GitHub->>Handler: POST push event
Handler->>Handler: find matching RemoteSyncConfig
alt file_path in touched files
Handler->>Handler: snapshot old_status
Handler->>DB: set sync_config.status = "checking" and commit
Handler->>ARQ: await get_arq_pool()
alt pool != None
Handler->>ARQ: enqueue run_remote_check_task
ARQ-->>Handler: enqueue success
else pool is None
Handler->>Log: warn "ARQ pool not available"
Handler->>DB: restore old_status and commit
end
alt enqueue raises exception
Handler->>Log: warn with exc_info=True
Handler->>DB: restore old_status and commit
end
else no match
Handler->>Log: debug "no sync needed"
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ontokit/api/routes/pull_requests.py`:
- Around line 682-693: The code sets sync_config.status = "checking" and commits
before attempting to enqueue the remote job, which can leave state stuck if
get_arq_pool() fails or returns None; change the flow so the status is only set
and service.db.commit() is called after a successful enqueue (i.e., after
get_arq_pool() returns a non-None pool and pool.enqueue_job completes), or if
you must keep the pre-commit, add a rollback in the except and the pool None
path that resets sync_config.status to its prior value and commits, and ensure
logger.warning is called when pool is None; key symbols: sync_config,
get_arq_pool, pool.enqueue_job, service.db.commit, logger.warning.
🪄 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: ce2bc496-a737-496d-91ab-924b8c8ebad6
📒 Files selected for processing (1)
ontokit/api/routes/pull_requests.py
Move the status update and db commit to after pool.enqueue_job() succeeds, so a failed or unavailable ARQ pool doesn't leave sync_config.status stuck as 'checking'. Also log a warning when the pool is None. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ontokit/api/routes/pull_requests.py`:
- Around line 682-696: Scope the ARQ error handling to only the pool/enqueue
calls and persist the "checking" state before awaiting the enqueue: set
previous_status = sync_config.status; set sync_config.status = "checking" and
call await service.db.commit() immediately, then wrap only get_arq_pool() and
await pool.enqueue_job("run_remote_check_task", str(project_id)) in a try/except
that logs ARQ errors; if enqueue fails, revert sync_config.status =
previous_status and persist that rollback (await service.db.commit() or
rollback) so the remote_sync_service.py sees the updated state and we don't
leave a queued job inconsistent with DB state. Use the existing names
sync_config.status, get_arq_pool(), enqueue_job(), and service.db.commit() to
locate the code.
🪄 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: f835bc19-f966-4f7c-ac69-27238a7dbb63
📒 Files selected for processing (1)
ontokit/api/routes/pull_requests.py
Set sync_config.status = "checking" and commit before attempting the enqueue so that remote_sync_service sees the updated state immediately. If the ARQ pool is unavailable or enqueue fails, revert the status to its previous value and commit the rollback. This avoids a race where the worker starts before the status commit, and ensures the DB stays consistent on failure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
get_arq_pool()andpool.enqueue_job()calls in the GitHub webhook push handler with try/except error handlingpool is not Noneguard before enqueueing, matching the defensive pattern used elsewhereCloses #24
Test plan
{"status": "ok"}even when Redis is unavailablerun_remote_check_taskjob is still enqueued correctly🤖 Generated with Claude Code
Summary by CodeRabbit