feat: Lock and rate limit webhook audio bytes - #11353
Conversation
…audio bytes webhook Added a Redis-based locking mechanism with a 5-second TTL in `backend/database/redis_db.py` to handle rate limiting and concurrency locking for the developer audio bytes webhook. Updated `send_audio_bytes_developer_webhook` in `backend/utils/webhooks.py` to enforce this lock, validate the webhook URL uses HTTP/HTTPS via regex (`re`), and shortens the audio data segments strictly to a max of 5 seconds (assuming 16-bit PCM). Verified through tests. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3160ba46-d26b-4daf-a5bf-986b365a3a1e) |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df791c294a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return | ||
|
|
||
| max_bytes = sample_rate * 2 * 5 | ||
| data = data[:max_bytes] |
There was a problem hiding this comment.
When the configured interval exceeds five seconds, this slice permanently discards the remainder: the producer queues and then clears the entire interval buffer in backend/routers/pusher.py lines 628-646, so a 10-second interval delivers only its first five seconds. Even the default interval loses the frame that crosses the strict > 5 seconds threshold. Shorten chunks at the producer while retaining overflow rather than truncating after the buffer has been cleared.
Useful? React with 👍 / 👎.
|
|
||
| def try_acquire_audio_bytes_webhook_lock(uid: str, ttl: int = 5) -> bool: | ||
| """Atomically try to acquire audio bytes webhook rate limit lock. Returns True if acquired (not rate limited), False if already rate limited.""" | ||
| result = r.set(f'users:{uid}:audio_bytes_webhook_lock', '1', ex=ttl, nx=True) |
There was a problem hiding this comment.
When Redis is unavailable or SET raises, this new helper propagates the exception through run_blocking before the webhook's try block; the pusher consumer then catches it and drops that queued audio segment without attempting delivery. Redis-backed locks and rate limits are explicitly required to catch errors and let requests proceed, so this helper should log the failure and return True.
AGENTS.md reference: backend/AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
|
|
||
| def try_acquire_audio_bytes_webhook_lock(uid: str, ttl: int = 5) -> bool: | ||
| """Atomically try to acquire audio bytes webhook rate limit lock. Returns True if acquired (not rate limited), False if already rate limited.""" | ||
| result = r.set(f'users:{uid}:audio_bytes_webhook_lock', '1', ex=ttl, nx=True) |
There was a problem hiding this comment.
When a delivery takes more than five seconds and the same UID has concurrent pusher sessions, this TTL expires while the first POST is still in flight, allowing another worker to acquire the key and send concurrently. Webhook requests have a documented 30-second timeout and can also retry, so a five-second lease cannot provide the claimed concurrency lock; release a token-owned lock in finally, with a safety TTL covering the complete delivery window.
AGENTS.md reference: backend/AGENTS.md:L329-L329
Useful? React with 👍 / 👎.
| from database.redis_db import ( | ||
| get_user_webhook_db, | ||
| user_webhook_status_db, | ||
| try_acquire_audio_bytes_webhook_lock, |
There was a problem hiding this comment.
This newly added direct function import violates the backend's required Redis boundary and snapshots the helper binding at module import time, so replacing or patching database.redis_db does not affect this caller. Import the module as prescribed and invoke redis_db.try_acquire_audio_bytes_webhook_lock(...) instead.
AGENTS.md reference: backend/AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| return | ||
| webhook_url = webhook_url.split(',')[0] | ||
| if not webhook_url: | ||
| if not webhook_url or not re.match(r"^https?://", webhook_url): |
There was a problem hiding this comment.
For existing webhook URLs whose scheme uses valid uppercase or mixed-case spelling, such as HTTPS://example.com/audio, this new case-sensitive regex returns silently and stops all audio delivery. URI schemes are case-insensitive, so validate the parsed, normalized scheme or use a case-insensitive match rather than rejecting these URLs.
Useful? React with 👍 / 👎.
| if not await run_blocking(db_executor, try_acquire_audio_bytes_webhook_lock, uid): | ||
| return |
There was a problem hiding this comment.
When one account has multiple active listen/pusher sessions, their chunks contain distinct audio, but this UID-only lock lets one session return without delivery whenever their intervals overlap. The listen handshake explicitly does not rate-limit concurrent sessions (backend/tests/unit/test_ws_auth_handshake.py lines 250-257), and the pusher has already cleared each queued buffer before this call, so every rejected chunk is permanently lost; serialize or queue deliveries instead of dropping the losing session's data.
Useful? React with 👍 / 👎.
| return | ||
| webhook_url = webhook_url.split(',')[0] | ||
| if not webhook_url: | ||
| if not webhook_url or not re.match(r"^https?://", webhook_url): |
There was a problem hiding this comment.
When the webhook setter receives a scheme-less or unsupported URL, it stores the value, enables the webhook, resets its health state, and returns success, but this new branch silently skips every delivery without recording a failure. Previously the attempted request flowed through webhook health handling and eventually auto-disabled and notified the user; validate and reject the URL in the setter, or record this dispatch failure, so the integration is not shown as enabled forever while doing nothing.
Useful? React with 👍 / 👎.
undivisible
added
human
undivisible
left a comment
undivisible
left a comment
There was a problem hiding this comment.
The new webhook guard can overlap on slow deliveries and silently truncate configured audio. Please hold or tokenise the per-user lock for the full delivery/retry lifecycle, derive the byte cap from the configured segment length instead of hard-coding five seconds, and add tests for lock contention, invalid URLs, truncation, and Redis-outage behavior.
Failure-Class: none
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_3e19ba27-720f-4e36-a472-3707f4ac3d3d) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 467fb1459d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except Exception as e: | ||
| logger.warning('audio bytes webhook lock unavailable; proceeding without lock error=%s', type(e).__name__) | ||
| return token |
There was a problem hiding this comment.
When Redis raises during lock acquisition, this branch deliberately changes correctness mode by sending the webhook without concurrency protection, but it only logs a warning. Record this through the shared record_fallback helper so unlocked deliveries are observable and attributable during Redis incidents.
AGENTS.md reference: backend/AGENTS.md:L334-L334
Useful? React with 👍 / 👎.
| max_bytes = sample_rate * 2 * configured_seconds | ||
| data = data[:max_bytes] |
There was a problem hiding this comment.
When an audio webhook is configured as https://example.com/audio,0 or with a negative interval, the setter accepts it and this calculation produces a zero or negative slice bound. The pusher threshold then fires on every incoming frame; zero sends an empty POST each time, while a negative value sends a frame with an arbitrary suffix removed. Validate the parsed interval as positive or fall back to the documented default before using it for buffering and truncation.
Useful? React with 👍 / 👎.
| except Exception as e: | ||
| if cb is not None: | ||
| cb.record_failure() | ||
| should_disable = await run_blocking( | ||
| db_executor, record_dev_webhook_failure, uid, WebhookType.audio_bytes, 0, type(e).__name__ | ||
| ) | ||
| await _handle_dev_webhook_disable(uid, WebhookType.audio_bytes, should_disable) | ||
| logger.error(f"Error sending audio bytes to developer webhook: {e}") | ||
| else: | ||
| return | ||
| should_disable = await run_blocking( | ||
| db_executor, record_dev_webhook_failure, uid, WebhookType.audio_bytes, 0, type(e).__name__ | ||
| ) |
There was a problem hiding this comment.
When user_webhook_status_db or get_user_webhook_db raises, the newly widened try reaches this handler even though no HTTP request was attempted, incrementing the endpoint's failure count and potentially auto-disabling it after repeated Redis read/decode failures. Keep configuration/enablement failures separate from webhook health, or only record a delivery failure after the endpoint and circuit breaker have been resolved.
Useful? React with 👍 / 👎.
Implemented concurrency locking, shortened audio segment lengths, and regex validation for developer audio webhooks as detailed in the task description. Failure-Class: none Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
This reverts commit 120735e.
Implemented concurrency locking, shortened audio segment lengths, and regex validation for developer audio webhooks as detailed in the task description. Failure-Class: none Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
This reverts commit 3683dae.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 883c73d244
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| r.eval(_RELEASE_AUDIO_BYTES_WEBHOOK_LOCK_SCRIPT, 1, f'users:{uid}:audio_bytes_webhook_lock', token) | ||
| except Exception as e: | ||
| logger.warning('release audio bytes webhook lock failed error=%s', type(e).__name__) |
There was a problem hiding this comment.
When EVAL fails transiently without deleting the key and Redis then recovers, this helper swallows the error while the acquired lock remains for at least 180 seconds; every subsequent audio segment sees SET ... NX return false and is silently dropped until expiry. Retry the token-owned release or otherwise ensure a release failure cannot turn a brief Redis error into several minutes of fail-closed webhook delivery.
AGENTS.md reference: backend/AGENTS.md:L191-L195
Useful? React with 👍 / 👎.
Git-on-my-level
removed
the
needs-tests
|
Thanks for tightening the audio-bytes developer webhook path. I reviewed the current head ( Specific code notes:
Validation seen:
I removed the stale Automated review by glm-5.2 for Omi maintainers. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Failure-Class: none
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d8c0648e-b9ec-40e8-9fe7-b805029b58a4) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32c39210dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| request_seconds = _WEBHOOK_REQUEST_TIMEOUT_SECONDS * (len(retry_delays) + 1) | ||
| delay_seconds = sum(max(delay, 0) for delay in retry_delays) | ||
| return max(_AUDIO_BYTES_WEBHOOK_LOCK_MIN_TTL_SECONDS, int(request_seconds + delay_seconds) + 1) |
There was a problem hiding this comment.
When the shared 64-request webhook semaphore remains saturated long enough, _post_dev_webhook waits at line 83 without a timeout before entering client.post, but this lease budgets only the HTTP attempts and retry sleeps. Fresh evidence after the earlier lease change is that semaphore wait time is still excluded from the calculation; if it consumes the 180-second minimum lease, another worker can acquire the same UID lock while the first delivery is still queued or running, defeating the serialization guarantee. Renew the lock through delivery or bound and include semaphore waiting in its lifetime.
AGENTS.md reference: backend/AGENTS.md:L329-L329
Useful? React with 👍 / 👎.
| max_bytes = sample_rate * 2 * configured_seconds | ||
| data = data[:max_bytes] |
There was a problem hiding this comment.
This changes the externally visible audio-webhook streaming contract by adding per-UID serialization and configured-duration payload truncation, but the commit does not update docs/doc/developer/backend/listen_pusher_pipeline.mdx. The backend guide explicitly requires that document to move in the same PR whenever audio streaming changes, so update it to describe the new chunk sizing and delivery-lock behavior.
AGENTS.md reference: backend/AGENTS.md:L177-L177
Useful? React with 👍 / 👎.
Git-on-my-level
added
the
positive-signal
|
Thanks for the follow-up on this webhook path. I reviewed the current head ( Specific code notes:
Validation seen:
Keeping Automated review by glm-5.2 for Omi maintainers. by AI on behalf of David — security-sensitive webhook/rate-limit behavior needs maintainer sign-off before merge. |
2 participants
Implemented concurrency locking, shortened audio segment lengths, and regex validation for developer audio webhooks as detailed in the task description.
PR created automatically by Jules for task 1937260442649585048 started by @undivisible
Note
Medium Risk
Changes real-time webhook delivery semantics (drops concurrent sends, caps bytes, blocks bad URLs) and uses fail-open locking when Redis fails, which could allow duplicate deliveries during outages.
Overview
Developer audio-bytes webhooks are now serialized per user with a Redis lock (token-based release, TTL tied to retry/timeouts) so overlapping deliveries are skipped instead of stacking; lock acquisition fails open if Redis is down.
send_audio_bytes_developer_webhookacquires the lock up front, rejects non-HTTP(S) URLs, truncates the body using theurl,secondsconfig (shared withget_audio_bytes_webhook_seconds), and always releases the lock in afinallyblock.Unit tests cover lock contention, invalid URLs, truncation, failure handling, and Redis lock acquire/release behavior.
Reviewed by Cursor Bugbot for commit 32c3921. Configure here.
Failure-Class: none