test(azure_sentinel): pin batch_size as a per-request bound under concurrent events - #40320
Conversation
…current events Adds a regression test to the mapped Azure Sentinel test file for the concurrency scenario from LIT-6920: 40 records logged concurrently at batch_size=5 while each ingestion request is still in flight. Asserts no request carries more than batch_size records, every record arrives exactly once in order, and the queue is empty afterwards. Runs for both the standard log queue and the audit log queue. The test fails on the tree before #39880 (whole shared queue serialized per threshold send, then cleared) and passes on current staging. It is independent of the size-split coverage that #39880 added for LIT-5899. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis test-only PR strengthens the Azure Sentinel batching regression coverage by replacing timing-based overlap with explicit event synchronization.
Confidence Score: 5/5The PR appears safe to merge because the revised synchronization directly exercises the in-flight-send condition without introducing production changes or new actionable issues. No new defects or applicable rule violations remain. The earlier timing-dependence thread was manually resolved without explanation; the current event-based synchronization nevertheless addresses its concern. The other previous findings were correctly withdrawn after repository scope and local test conventions were clarified.
|
| Filename | Overview |
|---|---|
| tests/test_litellm/integrations/test_azure_sentinel.py | Adds deterministic synchronization and intermediate assertions to the concurrent batch-size regression test for standard and audit log queues. |
Reviews (2): Last reviewed commit: "test(azure_sentinel): gate the first sen..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
bugbot run |
…provably arrive while it is in flight Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2a9c860. Configure here.
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a platform team on a release without #39880 lowers
DEFAULT_BATCH_SIZEto 50 to stay under Sentinel's 1 MB ingestion cap, and still loses recordsDEFAULT_BATCH_SIZE=50andcallbacks: ["azure_sentinel"], then restart the proxygpt-5-mini, all 400 return 200 with achatcmpl-...idAfter: the same team on a release with #39880 sees every request stay under batch_size and every record arrive once
DEFAULT_BATCH_SIZE=50andcallbacks: ["azure_sentinel"], then restart the proxyThis PR adds the test that keeps the After flow from regressing. The merge base already behaves like After
Relevant issues
Pylon #7652
Linear ticket
Resolves LIT-6920
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Shared setup. Real proxy per tree, real Postgres (
--use_prisma_db_push), 4 uvicorn workers on both sides, real OpenAIgpt-5-minicalls.PYTHONPATHis pinned to the tree under test andlitellm.__file__is asserted to sit inside it before boot. Sentinel is a local HTTP ingestion endpoint on 127.0.0.1 that takes 2 s per request, answers 204, and answers 413 for any body over 1,000,000 bytes, which is Azure's documented cap. It logs bytes, record count, status, and record ids per request, so the same numbers a customer reads off Sentinel diagnostics are read back hereThe merge base d9b63ef already contains the fix from #39880, so a Before at the merge base would match After. Before is captured at 56a61cf, the last staging commit without that behavior, which is the tree the ticket reproduced against
No screenshots: the behavior this test pins, how many records ride in each Sentinel ingestion request, is visible only at the ingestion endpoint. The Admin UI Logs page shows the same 400 rows on both trees and no dashboard page renders ingestion batches, so the endpoint-side tally above is the proof
Config:
Environment on both sides:
DEFAULT_BATCH_SIZE=50 DEFAULT_FLUSH_INTERVAL_SECONDS=30 AZURE_SENTINEL_ENDPOINT=http://127.0.0.1:25920 AZURE_SENTINEL_AUTHORITY_HOST=http://127.0.0.1:25920plus the usualAZURE_SENTINEL_*idsBoot (per tree):
Burst (400 concurrent chat completions, each response id is the Sentinel record id):
Then wait 40 s for the periodic flush and compare the 400 response ids against the ids Sentinel accepted
Before (56a61cf)
grep -c "Started server process" proxy.loggrep -c "413 Request Entity Too Large" proxy.logAfter (2a9c860)
grep -c "Started server process" proxy.loggrep -c "413 Request Entity Too Large" proxy.logRegression evidence for the test itself: at 2a9c860 the new test fails on 56a61cf (
2 failed, both parametrizations, the held first send carried 26 records instead of 5) and passes on staging (85 passedfor the file, 3 repeated runs). Mutants that drop the flush lock, drop the count split, or send from the live queue instead of a detached copy each fail at least one test in the fileType
✅ Test
Caveats (if any)
Final Attestation
Note
Low Risk
Test-only change in the Azure Sentinel integration test suite; no runtime or configuration behavior is modified.
Overview
Adds a regression test so Azure Sentinel ingestion never sends more than
batch_sizerecords in one HTTP request when new events keep arriving while a batch is still on the wire.The test fires 40 concurrent log/audit callbacks with
batch_size=5, blocks the first ingestion call until mid-flight assertions run, then checks the held request carried exactly five IDs, the remainder stayed queued, every later request stayed ≤5 records, all 40 IDs were delivered once, and the queue drained. It runs for both standard and audit queues via existingQUEUE_CASESparametrization.No production code changes—this locks in behavior from the prior concurrency fix (LIT-6920) so future batching changes cannot reintroduce oversized bodies, 413s, duplicates, or lost records.
Reviewed by Cursor Bugbot for commit 2a9c860. Bugbot is set up for automated code reviews on this repo. Configure here.
Link to Devin session: https://app.devin.ai/sessions/7fdc48a030e94795bb06e1ad3b0af942
Open in Devin Desktop: https://app.devin.ai/desktop/session/7fdc48a030e94795bb06e1ad3b0af942?variant=devin
Requested by: @yucheng-berri