fix: API-key usage counter must not deny authentication (intermittent 403s) - #8
Merged
Merged
Conversation
`_check_credentials` bumps x_last_used / x_use_count with one UPDATE per authenticated call. Two concurrent requests on the same key update that row at once, and at REPEATABLE READ the loser gets a SerializationFailure. Odoo's retrying() lists SerializationFailure as retryable, but never sees this one: the counter runs inside authentication, and _authenticate_explicit() wraps auth in `except Exception: raise AccessDenied()`. The concurrency blip is converted to a hard 403 before the retry layer is reached, so a perfectly valid key is rejected. Observed on production 2026-07-15 08:15 (key id 55) — MCP clients making parallel calls got intermittent 403s. Run the UPDATE in a savepoint and swallow concurrency errors, so a lost increment can't fail the request. x_use_count and x_last_used are readonly display fields and gate nothing, so dropping one is free. Verified against PostgreSQL that ROLLBACK TO SAVEPOINT recovers a transaction after a 40001 and lets it commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rutgerhofste
force-pushed
the
fix/apikey-counter-must-not-deny-auth
branch
from
September 7, 2026 17:22
d92c6a7 to
3a54560
Compare
Member
Author
|
Rebased on 19.0 (now at d3f8d2c, after #9). Only the manifest version conflicted; the code applied clean. Version resolved to 19.0.1.20.4 on top of 19.0.1.20.3. The bug is still live: production hit it again today, 2026-09-07 17:01 UTC, five SerializationFailures in 200ms on key id 33. Different key from the July report (33, was 55), same failure mode. 🤖 Generated with Claude Code |
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
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.
The bug
_check_credentialsbumps the usage counter with oneUPDATEper authenticated call:When two requests using the same API key arrive concurrently, both update that row. At Odoo's
REPEATABLE READisolation the loser getsSerializationFailure(SQLSTATE 40001), which aborts the request transaction.Odoo does know how to handle this —
PG_CONCURRENCY_EXCEPTIONS_TO_RETRYinodoo/service/model.pylistsSerializationFailureas retryable. Butretrying()never sees it. The counter runs inside authentication, and_authenticate_explicit()inodoo/addons/base/models/ir_http.pydoes:So a retryable concurrency blip is converted to a hard 403 before the retry layer is reached, and a perfectly valid key is rejected. Bookkeeping takes down auth.
Seen in production
2026-07-15 08:15, key id 55, parallel
crm.lead/res.partnercalls from an MCP client:Note this is intermittent and load-dependent — it only bites when the same key is used in parallel, which is exactly what a busy MCP client does.
The fix
Wrap the counter in a savepoint and swallow concurrent-update errors. The counter is best-effort; authentication is not.
Dropping an increment is free:
x_use_countandx_last_usedarereadonlydisplay fields, referenced only by the form view. Nothing reads them for cap enforcement or any other decision, so a lost count under contention has no downstream effect. A denied API call does.Unexpected
psycopg2.Errors are also swallowed (atwarning, with traceback) rather than failing auth — same reasoning.Verification
The load-bearing assumption is that
ROLLBACK TO SAVEPOINTactually recovers a transaction after a 40001, rather than leaving it unusable. Confirmed against PostgreSQL with two concurrent sessions reproducing the exact production error:Also confirmed on the Odoo 19 runtime: all three
psycopg2.errorsclasses resolve,cr.savepoint(flush=False)matches the signature inodoo/sql_db.py, andSavepoint.__exit__rolls back but re-raises — hence the explicittry/except.ruff checkandruff format --checkpass.No unit test.
tests/test_apikeys.pydocuments that_check_credentialsneeds a liveodoo.http.requestand is therefore covered by manual smoke plus staging rather thanTransactionCase; this change doesn't alter that. Reproducing a genuine serialization failure additionally needs two concurrent transactions, which the single-cursor test harness can't express. Suggest a staging smoke: hammer one key with parallel calls and confirm no 403s.Notes for review
19.0, independent of fix: two install/runtime crashes from the DCBO report (TOTP column leak + auditlog rule collision) #6 — but fix: two install/runtime crashes from the DCBO report (TOTP column leak + auditlog rule collision) #6 touches the same file and claims version19.0.1.20.3, so this one takes19.0.1.20.4. Whichever merges second may need a trivial manifest bump.CONCURRENT_UPDATE_ERRORSmirrorsodoo.service.model.PG_CONCURRENCY_EXCEPTIONS_TO_RETRYrather than importing it, to keep a model from reaching into a service layer.🤖 Generated with Claude Code