Skip to content

fix(credits): restore 402 return for insufficient_credits in credit_middleware#58

Merged
MoltyCel merged 4 commits into
MoltyCel:mainfrom
HaraldeRoessler:fix/restore-credit-402
May 22, 2026
Merged

fix(credits): restore 402 return for insufficient_credits in credit_middleware#58
MoltyCel merged 4 commits into
MoltyCel:mainfrom
HaraldeRoessler:fix/restore-credit-402

Conversation

@HaraldeRoessler
Copy link
Copy Markdown
Contributor

Summary

PR#17 (security hardening pass) lost the deduct_failed → 402 return block during rebase conflict resolution on the credit middleware.

The Bug

The deduct_failed flag was correctly set when UPDATE ... WHERE balance >= cost returned 0 rows (insufficient balance / race condition), but it was never evaluated — the code fell through to return response, shipping the original 200 OK back to the caller.

Consequence: Billing bypass under parallel calls — paid endpoints returned 200 instead of 402 when credits ran out mid-batch.

The Fix

Restores the exact 7-line block that was lost (originally from PR#27):

if deduct_failed:
    return JSONResponse(
        status_code=402,
        content={
            "error": "insufficient_credits",
            "detail": "Not enough credits for this call.",
        },
    )

This produces consistent 402 semantics across both the pre-check (line 553) and the atomic deduct (line 582) paths.

Test

test_concurrent_deducts_respect_balance now passes:

  • Before: 10×200 (all calls succeed, credits overspent by 7)
  • After: 3×200 + 7×402 (only 3 calls succeed, balance=3 enforced)

Diff

+        if deduct_failed:
+            return JSONResponse(
+                status_code=402,
+                content={
+                    "error": "insufficient_credits",
+                    "detail": "Not enough credits for this call.",
+                },
+            )
+
     return response

7 lines, no new code — exact restore of the block lost from the PR#17 rebase.

Credit

Discovered and diagnosed by Kersten Kroehl (kk). Fix applied from his diagnosis on api.moltrust.ch.

…iddleware

PR#17 (security hardening) lost the deduct_failed → 402 return block
during rebase conflict resolution on the credit middleware. The
deduct_failed flag was set on UPDATE ... WHERE balance >= cost
returning 0 rows, but never evaluated — causing paid endpoints to
return 200 instead of 402 under parallel calls (billing bypass).

This commit restores the exact 7-line block that was lost, producing
consistent 402 semantics across both the pre-check and the atomic
deduct paths.

Found by: Kersten Kroehl (kk)
Original commit reference: bbe4ad7 (credit-middleware PR MoltyCel#27)
Test: test_concurrent_deducts_respect_balance now passes
(10 parallel calls → 3×200 + 7×402 vs previous 10×200)
Previously the fork CI only ran pytest --collect-only, never executing
any tests. This meant regressions like the PR#17 credit-middleware
402-block loss were invisible to CI — the concurrency test
(test_concurrent_deducts_respect_balance) would have caught it
immediately, but it never ran.

New job:
- pytest-run: spins up a Postgres 16 service container, runs
  init_db.sql for the schema, executes tests/test_credit_middleware.py
  with proper env vars (CREDITS_ENABLED=true, MOLTSTACK_DB_PW, etc.)

This is intentionally scoped to credit middleware tests only — the
test_caep.py and protocol compliance tests need additional
infrastructure (sandbox, admin keys) and can be added separately.

Also adds httpx to the collect job's pip install (needed for import
to succeed).
@MoltyCel
Copy link
Copy Markdown
Owner

Small post-merge process note — not a knock on the change itself, the CI job is sound:

The PR description's Diff section shows only the app/main.py block, but #58 also rewrote .github/workflows/fork-ci.yml (+63/-1) — a new DB-backed pytest-run job. A reviewer going by the description alone wouldn't know a workflow file changed.

Worth keeping as a review norm: list every changed file (or area) in the description, especially when a fix(...) PR also touches CI/workflow files — those carry different risk than app code and deserve an explicit callout.

Two follow-ups surfaced while reviewing this PR, both now open:

Thanks for the quick 402 fix.

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