Repository navigation
fix(azure_storage): name Data Lake objects without base64 padding or slashes - #43914
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
There was a problem hiding this comment.
Devin Review found 1 potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| def adls_safe_file_name(payload_id: str | None) -> str: | ||
| """`=` padding and `/` in a base64 payload id are what the Data Lake service rejects, so the name drops the | ||
| padding and maps `/` to `_`. Standard base64 has no `_` and its padding is fixed by the length, so ids from | ||
| that alphabet stay distinct; anything else is left as is.""" | ||
| return f"{(payload_id or str(uuid.uuid4())).translate(_ADLS_SAFE_NAME)}.json" |
There was a problem hiding this comment.
🔴 Distinct payload IDs overwrite one object
When IDs differ only by / versus _, adls_safe_file_name gives both the same name. The later upload overwrites the earlier audit record.
Learn more
The Azure callback handles every StandardLoggingPayload, whose id contract permits any string. Replacing / with _ is injective only for IDs restricted to standard base64, but this shared callback also receives provider IDs and LiteLLM call IDs. Deleting every = creates additional collisions for IDs outside canonical padded base64.
Example: Payload IDs provider/a_b and provider/a/b both become provider_a_b.json. The second create_file() call reuses that path, so storage retains only the second payload.
Recommended fix: Encode the complete ID with an injective filename-safe encoding, such as unpadded URL-safe base64 of its UTF-8 bytes. Apply the same helper in both writers and all readers.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed the mapping is not injective over arbitrary strings, and the docstring says so (distinct only within one base64 alphabet). Keeping it as is on purpose:
- Every id this callback sees is a litellm call id (uuid4), a provider id, or a gateway
resp_id (resp_plus standard base64). Standard base64 never contains_, and the padding is fixed by the length, so two gateway ids cannot collide. A collision needs a provider id and a gateway id that differ only in/vs_at the same position, which does not occur in practice - An injective encoding of the whole id would rename every object, including
chatcmpl-...and uuid names that land fine today, and make the object name unreadable next to the logs page id. That is a larger observable change than this bug fix; the leftover collision case is noted as a Low caveat in the PR body
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
bugbot run |
1 similar comment
|
bugbot run |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
bugbot run |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
…slashes Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
0764cdc to
7218279
Compare
Comments Outside DiffThese findings could not be posted inline.
|
|
The reversed TTL check is pre-existing main code, fixed separately in #43082 with its reuse tests; this PR only changes object names |
|
@greptileai review latest head |
|
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 7218279. Configure here.
…ject_key_prefix * upstream/main: (62 commits) fix(guardrails): scan Responses API input in Azure Prompt Shield (BerriAI#43786) feat(lens): investigate sampled traces and retain batch results (BerriAI#43942) fix(proxy): restore pre-config-wins handling of pass-through endpoints (BerriAI#43962) fix(cost-map): raise baseten DeepSeek-V4.1-Flash max output to 262144 (BerriAI#43916) chore(cost-map): add deprecation date for anthropic claude-sonnet-4-5 (BerriAI#43898) chore(cost-map): add fireworks inkling priority prices from the prices api (BerriAI#43949) feat(guardrails): honor litellm_params.timeout in every HTTP guardrail (BerriAI#43134) test(e2e): typed per-test metadata for the e2e suite (BerriAI#42044) fix(caching): write the response-cache SET to Redis at once instead of on the post-call batch (BerriAI#43973) feat(ui): filter tags by name and description on the Tag Management page (BerriAI#42949) feat(providers): add Cortecs as an OpenAI-compatible provider (BerriAI#43872) feat(e2e): record each e2e test's steps, starting with ProxyClient (BerriAI#42393) test(ci): repair stale tests and move retired OpenAI text-completion fixtures (BerriAI#43958) feat(proxy): record in spend logs whether a request used a client-forwarded Anthropic OAuth token (BerriAI#43063) fix(azure_storage): keep the DataLakeServiceClient alive until its TTL elapses (BerriAI#43082) chore(deps): bump gitpython and tornado, extend diskcache osv ignore to Nov 1 (BerriAI#43961) fix(guardrails): treat an unknown straiker api_version as unset instead of skipping the guardrail (BerriAI#43956) fix(azure_storage): name Data Lake objects without base64 padding or slashes (BerriAI#43914) fix(grayswan): send request conversation and tool calls to post-call monitor (BerriAI#43770) chore(cost-map): sync openrouter prices from the models API (BerriAI#43950) ...
TLDR
Problem this solves:
/v1/responsesids areresp_plus standard base64, so they end in=or==and may contain/{id}.jsonas the Data Lake object name%3Dbut sends a bare=, so Azure answersAuthenticationFailed/in the name is anInvalidUrion both auth pathsHow it solves it:
=and maps/to_; theidfield inside the JSON is unchanged/v1/responsesid itself is untouched, soprevious_response_idkeeps workingIntentional product change: on the Entra ID path a padded id used to land as
resp_...==.jsonand now lands asresp_....json; the account key path never landed those objects at all. Tooling that looks objects up by{response_id}.jsonhas to apply the same rewrite (the e2e reader added in #43082 will pick it up once that PR rebases on this one)User Flow
Before: a proxy admin with
success_callback: [azure_storage]and an account key sees every/v1/responsescall in the logs page but most are missing from storage{"model": "gpt-5.6", "input": "hi"}and get 200 with an id likeresp_bGl0ZWxsbTpj...ZmFi==ClientAuthenticationError: (AuthenticationFailed) Server failed to authenticate the requestresp_...==objectAfter: the same call lands in storage under a name Azure accepts
resp_bGl0ZWxsbTpj...ZmFi.json, and itsidfield is the exact id the client receivedLinear ticket
Resolves LIT-9079
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/unit/<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
Real Azure Data Lake Gen2 account (isolated resource group), the real
AzureBlobStorageLogger, sequential uploads, ids shaped exactly like the gateway's/v1/responsesids (resp_plus standard base64 oflitellm:custom_llm_provider:...;response_id:..., padded to 0, 1 and 2=), plus one id whose base64 contains//, one whose base64 contains+, and achatcmpl-control. Each object is then listed back through the Data Lake SDK. Same script, same account, before and afterDeterministic proof in the same PR:
tests/integration/observability/test_azure_storage_file_names.pyruns a real proxy (Postgres, Redis, scripted upstream) and points the callback at a local TLS Data Lake sink that verifies the SharedKey signature the way the service does. Audit overtest_azure_storage_file_names.pyplus the four observability suites that share the changedtests/integration/_support/wire.py(41 nodes) withuv run --no-sync python tests/integration/run.py, same--seedand--order-seedon every leg, run idsphase12/72182792/base,head1,head2. The live Azure runs below were captured on the previous tip, whoseazure_storage.pynaming code is byte-identical to this one (the rebase ontomainonly dropped the unrelated client TTL hunks from the stacked base)Before (merge base e61733b,
main; live runs on 0b7c9ee, same naming code)Account key path
python lit9079_logger.py <unfixed worktree>(uploads the six ids, listsYYYY-MM-DD/)Entra ID path
AZURE_STORAGE_ACCOUNT_KEYunset and the tenant, client id and client secret set=names, then the/id fails and the run stopsLive proxy, account key path
success_callback: ["azure_storage"],general_settings: disable_responses_id_security: true(so the gateway's ownresp_id reaches the callback instead of the encrypted per-key rewrite the id security hook applies by default), modelgpt-5.6on the real OpenAI API, real account keyfor n in $(seq 0 11); do curl -s http://127.0.0.1:42177/v1/responses -H "Authorization: Bearer $KEY" -d "{\"model\":\"gpt-5.6\",\"input\":\"lit9079pad $n\"}"; doneplus one/v1/chat/completionscontrol. All 13 return 200, every/v1/responsesid isresp_bGl0ZWxsbTpjdXN0b21fbGxtX3Byb3ZpZGVyOm9wZW5haTttb2RlbF9...=, one=of padding2026-09-30/after the flush: thechatcmpl-control landed, none of the 12resp_objects exist.proxy.logcarries 12ClientAuthenticationErrorentries,Error occurred: (AuthenticationFailed) Server failed to authenticate the request. Make sure the value of Authorization header is formed correctly including the signature.Deterministic integration test
uv run --no-sync pytest tests/integration/observability/test_azure_storage_file_names.py -qagainst a proxy started from the unfixed treeassert sink.unauthenticated_targets() == ()fails with four/litellm-logs/<day>%2Fresp_...=.json?resource=filetargets, 1 failed in 15sthrottled_sink(an s3 throttle timing cell, red on every leg on this box)After (7218279; live runs on 0764cdc, same naming code)
Account key path
python lit9079_logger.py <this branch>(rerun at 7218279 after the rebase: all six rows land again)Entra ID path
Live proxy, account key path
/v1/responsesid again ends in one=. Listing2026-09-30/: all 12 land asresp_bGl0ZWxsbTpjdXN0b21fbGxtX3Byb3ZpZGVyOm9wZW5haTttb2RlbF9....jsonwith the=removed, and each stored JSONidis byte-equal to the padded id the client received.proxy.loghas zeroAuthenticationFailedand zeroClientAuthenticationErrorDeterministic integration test
1 passed in 16.74s; six/v1/responsesids with one and two=all land, every stored payload carries the original id, all names match^[A-Za-z0-9._+-]+\.json$and are distincttest_azure_storage_file_names.pygreen on every leg. Two or three s3 and passthrough timing cells (slow_sink,throttled_sink,cache_hit_twins,upstream_failure_events,passthrough_sigkill) flip between legs withhttpx.ReadTimeoutin the request burst; each passes when run alone on the head rig andthrottled_sinkandpassthrough_sigkillalso fail on the base rig, so they read as box load rather than this diff, which does not touch their code pathMutation checks on the unit tests (
tests/unit/integrations/azure_storage/test_azure_storage.py, 25 passed): keeping=in the translation table turns the three padded cases plus both upload path tests red; putting the raw f-string back at the SDK call site turns the account key upload test red; both restoredType
🐛 Bug Fix
Caveats (if any)
Medium
{response_id}.jsonsee padded ids under a new name from this release on (see the intentional product change above)Low
+is left in the name on purpose, a+id lands today on both paths, so rewriting it would rename working objects/and a URL-safe id with_at the same position could map to one name, which needs two upstream ids that differ in exactly that character?,#, space) pass through untouched as before; no gateway id contains themresp_id with an encrypted URL-safe one before the callback sees it, so the earlier live A/B on that default landed every record on both sides; the A/B above setsdisable_responses_id_security: trueto expose the padded id, which is the shape the ticket reportsQA runbook
/v1/responsesis found under the padding-free object namesuccess_callback: ["azure_storage"],AZURE_STORAGE_ACCOUNT_NAME,AZURE_STORAGE_FILE_SYSTEMandAZURE_STORAGE_ACCOUNT_KEY{"model": "<your model>", "input": "hi"}and note theresp_...=id=removed, whose JSONidequals the id you receivedAZURE_STORAGE_ACCOUNT_KEYunset andAZURE_STORAGE_TENANT_ID,AZURE_STORAGE_CLIENT_ID,AZURE_STORAGE_CLIENT_SECRETset, and expect the same object at the container rootFinal Attestation
ran /live-pr-risk and found no regressions/backward incompatible risks
REVIEWER MUST KNOW BEFORE APPROVING
/v1/responsesrecord with idresp_...==lands asresp_...==.jsonresp_....json, the JSONidfield is unchanged/versus_, or by=, share one object name/is rejected by the service (InvalidUri) on both paths and its record is dropped_name and overwrites a record whose id already had_there, if one exists; no gatewayresp_, litellm uuid orchatcmpl-id can produce this pairLink to Devin session: https://app.devin.ai/sessions/5c920e07636044cebaef8e7a723300fe
Open in Devin Desktop: https://app.devin.ai/desktop/session/5c920e07636044cebaef8e7a723300fe?variant=devin
Requested by: @yucheng-berri
Note
Medium Risk
Changes observability object naming for padded or slash-containing ids (operators looking up
{response_id}.jsonmust apply the same rewrite), but fixes failed uploads that previously dropped audit records.Overview
Fixes silent audit log loss when the Azure Storage callback uploads
/v1/responsesrecords whose ids include base64=padding or/characters.Adds
adls_safe_file_name, which strips=and maps/→_for the Data Lake object name (both account-key SDK and Entra ID REST upload paths now use it). The JSON body still stores the originalidunchanged.Test coverage adds unit cases for the rewrite and upload paths, plus integration harness (TLS wire peer, SharedKey-verifying Data Lake sink) and an end-to-end proxy test that padded
resp_ids authenticate and land under ADLS-safe names.Reviewed by Cursor Bugbot for commit 7218279. Bugbot is set up for automated code reviews on this repo. Configure here.