Skip to content

test(e2e): guard destructive spend-log truncate behind explicit opt-in - #33725

Closed
devin-ai-integration[bot] wants to merge 1 commit into
litellm_internal_stagingfrom
litellm_e2e_guard_spend_truncate
Closed

test(e2e): guard destructive spend-log truncate behind explicit opt-in#33725
devin-ai-integration[bot] wants to merge 1 commit into
litellm_internal_stagingfrom
litellm_e2e_guard_spend_truncate

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Relevant issues

Linear ticket

Resolves LIT-4555

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • 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 (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Screenshots / Proof of Fix

Harness-internal change with no live-proxy behavior to demonstrate; see the QA runbook below for the gates that were run

Type

🧹 Refactoring
✅ Test

Changes

tests/e2e/conftest.py's pytest_sessionfinish truncated LiteLLM_SpendLogs against whatever DATABASE_URL resolved to, gated only by "an e2e test body ran". Pointed at a shared or staging DB, a routine local run would wipe real spend data. It also reached the truncate helper through a sys.path.insert into quota_management/spend_tracking, a cross-suite import-by-path hack it then had to unwind in a finally

This adds a real guard and requires an explicit opt-in before anything destructive happens. The truncate now runs only when E2E_RESET_SPEND_LOGS=1 is set, and the existing "a spend test actually ran" stash check stays as an additional necessary condition on top of the opt-in. Without the env var the session-finish hook returns early and leaves the DB untouched

def pytest_sessionfinish(session, exitstatus):
    if os.environ.get("E2E_RESET_SPEND_LOGS") != "1":
        return
    if not session.stash.get(_E2E_TEST_RAN, False):
        return
    try:
        reset_spend_logs()
    ...

The sys.path hack is gone. reset_spend_logs moved into a new shared module tests/e2e/e2e_db.py that sits alongside e2e_config.py and lifecycle.py, so it imports normally with from e2e_db import reset_spend_logs from both the top-level conftest and anywhere else. The implementation is unchanged (same TRUNCATE TABLE "LiteLLM_SpendLogs", same DATABASE_URL default). spend_e2e_client.py no longer defines it or lists it in __all__; nothing else imported reset_spend_logs from that module, so there is no duplicated logic and no other caller relied on the old path

QA runbook

Harness-internal change; there is no new live-proxy behavior to reproduce, so the validation is the harness gates rather than a proxy call

  • make lint-e2e-basedpyright -> 0 errors, 0 warnings, 0 notes
  • from tests/e2e, PYTHONPATH=. python -m coverage_registry.collector -> still runs and prints the coverage table
  • from tests/e2e, pytest --collect-only -qq -p no:cacheprovider . -> collects with no import or fixture errors
  • make pre-commit -> passes on the staged changes

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/51a83116b250445cba40d78568532b6a
Requested by: @yassin-berriai

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@yassin-berriai yassin-berriai self-assigned this Jul 17, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • 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.

@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR guards the destructive TRUNCATE TABLE \"LiteLLM_SpendLogs\" in pytest_sessionfinish behind an explicit E2E_RESET_SPEND_LOGS=1 env-var opt-in, preventing accidental data loss when running locally against a shared or staging database. It also eliminates the sys.path hack by moving reset_spend_logs into a new tests/e2e/e2e_db.py shared module.

  • tests/e2e/e2e_db.py is a new file that holds the re-homed reset_spend_logs function; the implementation is identical to what was in spend_e2e_client.py.
  • tests/e2e/conftest.py now returns early when E2E_RESET_SPEND_LOGS is not \"1\", keeping both the stash guard and the opt-in as necessary conditions before any DB write occurs.
  • spend_e2e_client.py has the duplicate reset_spend_logs definition and its __all__ entry removed; no other file imported it from there.

Confidence Score: 4/5

Safe to merge once the bob_the_builder.remediate scoping is resolved; the core opt-in guard and module restructuring are correct.

The bob_the_builder.remediate(session) call has been accidentally pulled inside the E2E_RESET_SPEND_LOGS=1 early-return guard, silently disabling it in pipelines that don't set the new env-var.

tests/e2e/conftest.py — the bob_the_builder.remediate call should be moved outside the E2E_RESET_SPEND_LOGS guard.

Important Files Changed

Filename Overview
tests/e2e/conftest.py Adds E2E_RESET_SPEND_LOGS=1 opt-in guard and cleans up the sys.path hack, but now gates bob_the_builder.remediate behind the new env-var check as an unintended side-effect.
tests/e2e/e2e_db.py New shared module that re-homes reset_spend_logs from the spend_tracking subdirectory; implementation is unchanged and the lazy psycopg import keeps startup cost minimal.
tests/e2e/quota_management/spend_tracking/spend_e2e_client.py Removes the now-relocated reset_spend_logs function and trims the unused os import and all entry; no callers remain on the old path.

Comments Outside Diff (1)

  1. tests/e2e/conftest.py, line 121-135 (link)

    P1 The bob_the_builder.remediate(session) call is now gated behind E2E_RESET_SPEND_LOGS=1, but that was not its original behaviour. Previously it ran whenever any e2e test body had executed; now it only runs when the operator also opts in to the destructive spend-log reset. Any CI pipeline that doesn't set this env-var will silently stop running remediation after every e2e session.

Reviews (1): Last reviewed commit: "test(e2e): guard destructive spend-log t..." | Re-trigger Greptile

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed-hq

codspeed-hq Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing litellm_e2e_guard_spend_truncate (b2c05fd) with litellm_internal_staging (561b679)1

Open in CodSpeed

Footnotes

  1. No successful run was found on litellm_internal_staging (ea48ded) during the generation of this report, so 561b679 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@yassin-berriai

Copy link
Copy Markdown
Contributor

Closing in favor of #33751, which supersedes this. Same fix (opt-in env guard on the destructive spend-log truncate plus dropping the sys.path hack via a top-level e2e_db module), and additionally extracts the guard into a dependency-injected pure helper with an injected-spy regression test, restores the bob_the_builder remediation gating, and includes a real-Postgres before/after proof

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