fix: token_spend calculation incorrect with concurrent webhooks#1081
Merged
fix: token_spend calculation incorrect with concurrent webhooks#1081
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughDev-mode toggles for Uvicorn were added and GitHub request-counting was reworked: requester wrappers now support shared counters/locks and are rewrapped per webhook to track absolute API call counts; helpers short-circuit single-token selection; tests and a context docstring were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Webhook as rect rgba(200,230,255,0.5) Webhook
participant Requester as rect rgba(200,255,200,0.5) CountingRequester
participant PyGithub as rect rgba(255,230,200,0.5) PyGithub/API
participant Metrics as rect rgba(255,200,200,0.5) WebhookContext
Webhook->>PyGithub: receive API client (may be wrapped)
Webhook->>Requester: unwrap existing wrapper -> create new per-webhook CountingRequester
Webhook->>PyGithub: perform API call via Requester
Requester->>Requester: increment shared_count under shared_lock
Requester->>PyGithub: forward actual HTTP request
PyGithub-->>Requester: response
Requester-->>Webhook: return response
Webhook->>Metrics: record requester.count as token_spend / final_count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Collaborator
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@entrypoint.py`:
- Around line 62-64: The call currently passes both workers=int(_max_workers)
and reload=_dev_mode (using _max_workers and _dev_mode), which is invalid for
uvicorn; change the invocation so that the workers parameter is only included
when reload is False (i.e., when _dev_mode is falsy). Locate the call that
supplies workers=int(_max_workers) and reload=_dev_mode and add conditional
logic to omit workers when _dev_mode is true so only reload is used in dev and
workers is used in production.
In `@webhook_server/libs/github_api.py`:
- Around line 217-224: The docstring for WebhookContext.token_spend is now
misleading because the code assigns token_spend = self.requester_wrapper.count
(absolute calls counted by this wrapper) instead of a rate-limit delta; update
the docstring in webhook_server/utils/context.py for class WebhookContext
(token_spend) to state it represents "API calls counted by this webhook's
requester_wrapper (absolute count)" or similar wording, and mention that
final_rate_limit is computed heuristically as initial_rate_limit -
requester_wrapper.count to keep docs consistent with the logic in github_api.py
(references: WebhookContext.token_spend, requester_wrapper.count,
final_rate_limit).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b553f696-9b6e-4000-a2b5-ae2b212695a8
📒 Files selected for processing (5)
entrypoint.pywebhook_server/libs/github_api.pywebhook_server/tests/test_github_api_metrics.pywebhook_server/tests/test_helpers.pywebhook_server/utils/helpers.py
myakove
force-pushed
the
fix/issue-970-token-spend-concurrency
branch
from
30c9817 to
e73d9fe
Compare
myakove-bot
added
commented-myakove
and removed
changes-requested-coderabbitai[bot]
labels
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
myakove
force-pushed
the
fix/issue-970-token-spend-concurrency
branch
from
e73d9fe to
fc45543
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
webhook_server/tests/test_github_api_metrics.py (1)
145-180: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winHIGH: This still doesn't lock in the issue
#970regression.This test proves the old wrapper is replaced, but it never exercises two
GithubWebhookinstances against the samemock_github_api. The bug was cross-webhook count contamination, so please add a direct two-instance test that performs requests through both webhook-local wrappers/repositories and asserts the counts stay independent.🧪 Suggested regression shape
gh1 = GithubWebhook(minimal_hook_data, minimal_headers, logger) gh2 = GithubWebhook(minimal_hook_data, minimal_headers, logger) gh1.requester_wrapper.requestJsonAndCheck("a") gh1.requester_wrapper.requestJsonAndCheck("b") gh2.requester_wrapper.requestJsonAndCheck("c") assert gh1.requester_wrapper.count == 2 assert gh2.requester_wrapper.count == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webhook_server/tests/test_github_api_metrics.py` around lines 145 - 180, Update the test to reproduce the cross-webhook-count regression by creating two GithubWebhook instances against the same mock_github_api and exercising their local wrappers independently: in test_init_unwraps_existing_wrapper (or a new test), instantiate gh1 = GithubWebhook(...) and gh2 = GithubWebhook(...) using the same mock_get_api return (mock_github_api with existing inner requester), call gh1.requester_wrapper.requestJsonAndCheck("a"), gh1.requester_wrapper.requestJsonAndCheck("b"), then gh2.requester_wrapper.requestJsonAndCheck("c"), and finally assert gh1.requester_wrapper.count == 2 and gh2.requester_wrapper.count == 1; ensure the mock inner_requester used by mock_github_api supports the called method so the CountingRequester increments independently for each wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/tests/test_github_api_metrics.py`:
- Around line 311-320: The test test_del_cleanup_without_clone_dir should call
the destructor directly to make failures deterministic: create the
partially-initialized instance via gh = GithubWebhook.__new__(GithubWebhook) and
then invoke GithubWebhook.__del__(gh) (rather than using del gh) so any
exceptions in GithubWebhook.__del__ are raised in the test; update the
assertion/expectation accordingly to ensure __del__ handles the missing
clone_repo_dir without raising.
In `@webhook_server/utils/helpers.py`:
- Around line 495-510: The single-token fast path still makes a live GitHub call
via _api.get_rate_limit(); remove that call and instead use the cached
_api.rate_limiting data (or skip rate-limit logging) before returning. In the
block handling apis_and_tokens == 1, avoid calling _api.get_rate_limit() and
either construct a minimal rate_limit object from _api.rate_limiting (then call
log_rate_limit(rate_limit=..., api_user=_api_user)) or simply omit the call to
log_rate_limit, keeping the existing checks that read _api.rate_limiting and the
final logger.info and return of _api, _token, _api_user.
---
Outside diff comments:
In `@webhook_server/tests/test_github_api_metrics.py`:
- Around line 145-180: Update the test to reproduce the cross-webhook-count
regression by creating two GithubWebhook instances against the same
mock_github_api and exercising their local wrappers independently: in
test_init_unwraps_existing_wrapper (or a new test), instantiate gh1 =
GithubWebhook(...) and gh2 = GithubWebhook(...) using the same mock_get_api
return (mock_github_api with existing inner requester), call
gh1.requester_wrapper.requestJsonAndCheck("a"),
gh1.requester_wrapper.requestJsonAndCheck("b"), then
gh2.requester_wrapper.requestJsonAndCheck("c"), and finally assert
gh1.requester_wrapper.count == 2 and gh2.requester_wrapper.count == 1; ensure
the mock inner_requester used by mock_github_api supports the called method so
the CountingRequester increments independently for each wrapper.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ecbe836-85e4-458a-ae2d-ec1ccf228763
📒 Files selected for processing (7)
entrypoint.pywebhook_server/libs/github_api.pywebhook_server/tests/conftest.pywebhook_server/tests/test_github_api_metrics.pywebhook_server/tests/test_helpers.pywebhook_server/utils/context.pywebhook_server/utils/helpers.py
myakove
force-pushed
the
fix/issue-970-token-spend-concurrency
branch
from
fc45543 to
41d31e6
Compare
Collaborator
Author
|
@coderabbitai[bot] The following review comments were reviewed and a decision was made:
|
myakove-bot
added
commented-myakove
and removed
changes-requested-coderabbitai[bot]
labels
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@entrypoint.py`:
- Line 19: Add documentation for the new runtime env var WEBHOOK_SERVER_DEV_MODE
(referenced by the _dev_mode variable in entrypoint.py) to the runtime
environment variables list in docs/docker-deployment.md: describe accepted
values ("1", "true", "yes" case-insensitive) and default behavior when unset,
explain its effect (enables webhook server dev mode) and give an example usage
line for docker/compose and exported shell variable form.
In `@webhook_server/tests/test_helpers.py`:
- Around line 172-180: The test includes hard-coded token-like literals (e.g.,
"token1") that trigger S105; refactor by extracting these literals into named
constants at the top of the test module (e.g., TEST_TOKEN_1) and append the
allowlist pragma comment "# pragma: allowlist secret" to each constant, then
update usages in get_api_with_highest_rate_limit() test and the other affected
block (around the assertions at lines 191-196) to reference those constants
instead of inline string literals; ensure constants are clearly named and used
wherever the token-like values appear.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 42d7f1a2-4461-4548-aa7b-ef83c3c94090
📒 Files selected for processing (7)
entrypoint.pywebhook_server/libs/github_api.pywebhook_server/tests/conftest.pywebhook_server/tests/test_github_api_metrics.pywebhook_server/tests/test_helpers.pywebhook_server/utils/context.pywebhook_server/utils/helpers.py
CountingRequester was broken in two ways: 1. withLazy() bypass: PyGithub's Github.get_repo() calls requester.withLazy() which returns a NEW raw Requester, completely bypassing the CountingRequester wrapper. Only Github-level calls (get_rate_limit) were counted — all Repository-level API calls (labels, check runs, comments) were invisible. Fixed by overriding withLazy() to propagate the shared counter. 2. Shared counter between concurrent webhooks: if a Github instance were reused, multiple webhooks would share the same CountingRequester and corrupt each other's token_spend. Fixed by always creating a fresh CountingRequester per webhook. Also includes: - Single-token optimization: skip rate limit comparison when only one token is configured - WEBHOOK_SERVER_DEV_MODE env var for uvicorn auto-reload - Remove hasattr guard on _Github__requester (PyGithub >=2.4.0 guaranteed) Closes #970
myakove
force-pushed
the
fix/issue-970-token-spend-concurrency
branch
from
41d31e6 to
e180859
Compare
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
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
2 participants
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.
Problem
CountingRequesterwas broken in two ways:withLazy()bypass: PyGithub'sGithub.get_repo()callsrequester.withLazy()which returns a NEW rawRequester, completely bypassing theCountingRequesterwrapper. OnlyGithub-level calls (get_rate_limit) were counted — all Repository-level API calls (labels, check runs, comments) were invisible. A PR that ran for 2m50s doing dozens of operations reported only 1 API call.Shared counter between concurrent webhooks: If a
Githubinstance were reused, multiple webhooks would share the sameCountingRequesterand corrupt each other'stoken_spend.Fix
withLazy()inCountingRequesterto return a new wrapper sharing the same counter (_shared_countlist +_thread_lock)CountingRequesterper webhook — unwrap any existing wrapper to get the real requesterinitial_wrapper_count— each webhook starts at 0, no delta neededAlso includes
WEBHOOK_SERVER_DEV_MODEenv var for uvicorn auto-reload during developmenthasattr(_Github__requester)guard — PyGithub >=2.4.0 guaranteed, direct access per AGENTS.md fail-fast principleVerified on live dev server
Testing
Closes #970
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation