Skip to content

test(proxy): make the stagger-offset and linear-dedup guards independent of runner identity and load - #45031

Merged
mateo-berri merged 3 commits into
mainfrom
litellm_deflake_20261007
Oct 7, 2026
Merged

mateo-berri merged 3 commits into
mainfrom
litellm_deflake_20261007

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

  • Two unit tests flaked in CI on 2026-10-06/07, passing on same-SHA reruns
  • One depended on the runner's hostname and pid hashing to a nonzero offset
  • One asserted a 1.0s wall-clock budget that loaded runners exceeded

How it solves it:

  • Inject the cleanup job's stagger offset through scheduled_job_stagger.offsets
  • Count traced line events instead of timing the attachment resolver
  • Test-only changes; no production code touched

Flakes covered (run date 2026-10-06 to 2026-10-07)

tests/unit/proxy/proxy_server/test_proxy_config.py::test_ProxyConfig__update_general_settings_runtime_interval_job_carries_the_stagger_offset

CI evidence, 5 failures in the window, 3 of them passing on a rerun of the same SHA:

  1. run 37569184199 on main at 62dee3d, attempt 1 failed (proxy-server job), attempt 2 passed
  2. run 37538950707 at 57e1dd0, attempt 1 failed, attempt 2 passed
  3. run 37535137407 at 36ef783, attempt 1 failed, attempt 2 passed
  4. run 37581498422 at d56ac0d, attempt 1 failed
  5. run 37544268078 at 893e065, attempt 1 failed

Failure: AssertionError: <IntervalTrigger (interval=datetime.timedelta(days=1), ...)> where an _OffsetTrigger was expected

Mechanism: offset_seconds hashes "spend_log_cleanup_job\x00{host}:{pid}" into [0, window) with a 300s window, and stagger_trigger correctly returns the bare trigger when that offset is 0. About 1 in 300 test processes land on 0, so the test's result depended on the runner's hostname and the xdist worker's pid. Reproduced locally by forking until a pid hashed to offset 0: the test fails there and passes everywhere else, with no edits in between

Fix: the test sets scheduled_job_stagger: {offsets: {spend_log_cleanup_job: 120}} on the proxy settings, which _spend_log_cleanup_trigger reads through parse_stagger_settings, and now asserts the job's trigger is an _OffsetTrigger with offset == 120s wrapping the one-day IntervalTrigger. The hash placement itself stays covered by tests/unit/proxy/common_utils/test_scheduled_job_stagger.py

tests/unit/proxy/policy_engine/test_attachment_registry.py::TestGetAttachedPolicies::test_many_distinct_policies_resolve_in_linear_time

CI evidence, 2 failures in the window, both passing on a rerun of the same SHA:

  1. run 37582238587 at 9a8ba0f, attempt 1 failed (proxy-hooks-client job) with 20000 attachments took 1.00s, attempt 2 passed
  2. run 37572462589 at 90f2bd0, attempt 1 failed with 20000 attachments took 1.30s, attempt 2 passed

Mechanism: a wall-clock budget (elapsed < 1.0 around get_attached_policies for 20,000 policies). The call takes about 0.25s on an idle machine, so CPU contention on a shared runner was enough to cross the budget. tests/unit/AGENTS.md also rules out the clock in unit tests

Fix: the guard the test exists for is the one-pass dedup from 311d9bb, which replaced a per-policy scan that was quadratic. The test now traces get_attached_policies with sys.settrace and counts line events at 1,000 and 4,000 policies, asserting the larger run does less than 8x the work of the smaller one. The current resolver measures 69,145 and 260,026 (3.8x). With the pre-311d9bba37 per-policy scan put back, the counts are 568,642 and 8,258,023 (14.5x), and the test fails. The counts do not depend on machine speed or load

Not fixed here

Pre-Submission checklist

  • I have added meaningful tests
  • The handful of test files covering my change pass locally
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review

Screenshots / Proof of Fix

No end-user behavior changes; this PR only changes two tests. The mechanism of the first flake can be seen directly in the production helper, outside pytest, by asking it for the cleanup job's trigger under two runner identities:

$ LITELLM_LOCAL_MODEL_COST_MAP=True uv run --no-sync python - <<'PY'
from apscheduler.triggers.interval import IntervalTrigger
from litellm.proxy.common_utils.scheduled_job_stagger import ScheduledJobStaggerSettings, offset_seconds, stagger_trigger
zero = next(f"fv-az1:{p}" for p in range(1, 100000) if offset_seconds(job_id="spend_log_cleanup_job", identity=f"fv-az1:{p}", window_seconds=300) == 0)
for identity in (zero, "fv-az1:2"):
    t = stagger_trigger(job_id="spend_log_cleanup_job", trigger=IntervalTrigger(days=1), period_seconds=86400, settings=ScheduledJobStaggerSettings(), identity=identity)
    print(identity, "->", type(t).__name__)
print("zero-offset identities among 30000 pids:", sum(offset_seconds(job_id="spend_log_cleanup_job", identity=f"fv-az1:{p}", window_seconds=300) == 0 for p in range(30000)))
PY
fv-az1:61 -> IntervalTrigger
fv-az1:2 -> _OffsetTrigger
zero-offset identities among 30000 pids: 118

The second flake's mechanism is in the CI logs linked above: the same SHA measured 1.00s and 1.30s on attempt 1 and passed on attempt 2

  • 588b0db passes /audit (skipped: no product code on a request path, tests only)

Type

Test

Caveats (if any)

Low

  • The traced window hides those lines from coverage.py for that one test
    • Other tests in the file still cover get_attached_policies_with_reasons
  • The 8x bound assumes the resolver stays linear; a deliberate superlinear change must update it

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

Link to Devin session: https://app.devin.ai/sessions/5db7c48d582940a2b79292fc4fdea782
Open in Devin Desktop: https://app.devin.ai/desktop/session/5db7c48d582940a2b79292fc4fdea782?variant=devin


Note

Low Risk
Only unit test assertions and harness code changed; no production behavior is modified.

Overview
Test-only changes to stop two CI flakes that depended on runner identity and wall-clock load.

In test_attachment_registry.py, the large-policy performance guard no longer uses a <1.0s timer on 20k attachments. It adds a sys.settrace helper that counts line events while resolving attachments, runs 1,000 vs 4,000 global-scope policies, and asserts the larger run stays below 8× the smaller run’s line count (still checking one-pass linear dedup).

In test_proxy_config.py, the stagger test now loads scheduled_job_stagger.offsets.spend_log_cleanup_job: 120 into proxy settings so the registered job always gets an _OffsetTrigger with offset == 120s wrapping a one-day IntervalTrigger, instead of relying on hostname/pid hashing that sometimes yields offset 0 and a bare interval trigger.

Reviewed by Cursor Bugbot for commit 588b0db. Bugbot is set up for automated code reviews on this repo. Configure here.

…c test no longer depends on the runner's host and pid

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…he linear dedup guard is deterministic under CI load

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

@greptile-apps

greptile-apps Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Low risk] Test changes for policy engine and proxy configuration.

The PR appears safe to merge, with both previous findings cleared and no new actionable issues

Summary

This test-only PR removes runner-dependent failures without changing production code

  • The cleanup-job test supplies a fixed stagger offset and checks the wrapped interval
  • The policy test compares traced work instead of elapsed time
  • The latest commit addresses the variable-annotation finding. No new actionable issues were found
  • devin-ai-integration[bot] accepted the trace guard’s inability to detect comparisons inside C operations because it targets the earlier Python-level scan. Greptile withdrew that concern

Reviews (2) · Last reviewed commit: "test(policy_engine): annotate the line-c..." · Reviewed by Greptile

Comment thread tests/unit/proxy/policy_engine/test_attachment_registry.py


def _lines_run_resolving(registry: AttachmentRegistry, context: PolicyMatchContext) -> tuple[list[str], int]:
lines_run = 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Variable rules are unmet

New locals lack Final, and lines_run is rebound without justification. AGENTS.md requires Final annotations and # rebind-ok: <reason> for unavoidable rebinding. Apply these conventions to the helper and new test setup before merging

Context Used: AGENTS.md (source)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 588b0db. The new locals are annotated Final, and the lines_run counter has a # rebind-ok reason, since the trace hook has to increment it

@codecov

codecov Bot commented Oct 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@mateo-berri

Copy link
Copy Markdown
Contributor

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 588b0db. Configure here.

@mateo-berri

Copy link
Copy Markdown
Contributor

@greptileai

@mateo-berri mateo-berri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mateo-berri
mateo-berri merged commit 498a3e1 into main Oct 7, 2026
102 checks passed
@mateo-berri
mateo-berri deleted the litellm_deflake_20261007 branch October 7, 2026 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants