Fix accounting race conditions and run the addclass concurrency test

[willgearty]

Description

AddClassConcurrencyRegressionTest has never run, on any database. Once enabled, it uncovered a race in how accounting rows are lazily created. Auditing that pattern across the accounting code turned up several more races, some of which charge students twice or grant aid twice. This PR fixes them and turns the test on, with no test-side workaround.

1. The concurrency test was always skipped

The skip guard checked connection.features.supports_select_for_update. Django has no such flag; the real one is has_select_for_update. Because the check used getattr(..., False), it always returned False, so the test skipped on Postgres too, with a message blaming SQLite. It now reads has_select_for_update directly, so a wrong name would raise instead of silently skipping.

2. The race classes

Lazy get_or_create() without a unique constraint. Two requests that both find no row each create one. From then on every get_or_create() for it raises MultipleObjectsReturned, so the page keeps returning 500 until someone deletes the duplicate by hand. This is what the concurrency test hit:

• DonationModule.students() created the donation line item on a read path: Program.user_can_join() → _students_in_program(), run when program_size_max is set and the student isn't already in the program.
• So both racing registration requests returned 500.

Hand-written check-then-create. The same race, but the result is duplicate charges rather than errors. @transaction.atomic doesn't prevent it under Postgres' default READ COMMITTED isolation, because a concurrent transaction's uncommitted inserts are invisible to each statement.

3. Every site fixed

Site	Triggered by	What went wrong	Lock
IndividualAccountingController.ensure_required_transfers() (runs on every amount_due() / amount_requested())	students	admission charged twice	user
apply_preferences() / set_preference()	students double-submitting the extra-costs or donation form	optional items / donations charged twice (each delete misses the other's inserts)	user
FinancialAidGrant.finalize()	admins	financial aid granted twice	user, plus a re-check of finalized from the database
SplashInfo.execute_sibling_discount()	students	duplicate discount transfer, then MultipleObjectsReturned	user
set_finaid_params()	admins	duplicate request/grant, then MultipleObjectsReturned	user
Donation LineItemType (both modules' line_item_type(), Stripe check_setup() / payonline(), donation_lineitemtype())	students	MultipleObjectsReturned → 500 on registration	program
"Student refund" LineItemType (default_refund_lineitemtype(), called by amount_refunded() on the Stripe payment page)	students	MultipleObjectsReturned	program
setup_lineitemtypes() items and options, sibling-discount LineItemType, program Account	admins	duplicates, then MultipleObjectsReturned	program

The mechanism (esp/db/locking.py)

• lock_row(instance) takes SELECT ... FOR NO KEY UPDATE on the user or program row. That serializes callers that lock the same row, without blocking ordinary inserts that merely reference it.
  • It resolves the model through _meta, because request.user is a SimpleLazyObject. The existing suite caught type() getting this wrong.
• get_or_create_locked(model, lock_on, defaults=None, **kwargs) has the same arguments and (obj, created) return as get_or_create(), so every call site keeps its exact lookup.
  • For example, a new base cost still creates a new "Program admission" row.
  • Fast path: plain lookup. When the row is missing, it locks, re-checks, and creates.
  • If earlier races left duplicates, it returns the newest, matching the existing default_*_lineitemtype() readers (which order by -id), instead of raising.
  • Lookup kwargs are passed straight to create(), so it doesn't support field__lookup keys. No caller uses them.
• LineItemType.get_or_create_for_program() delegates to it.
• ensure_required_transfers() takes the lock only when a transfer is missing, so the common path stays lock-free.
• Lock order is always user, then program, never the reverse, so these locks can't deadlock each other.
• DonationModule.students() no longer writes. It filters on transfer__line_item__program and transfer__line_item__text, which also counts donors on duplicate rows.

One deliberate behavior change: Stripe's check_setup() used to look up with required=False. If an admin had marked the donation item required, it would silently create a second row; now it reuses the existing one.

Alternative considered: unique constraints. Lookups like "Program admission" deliberately include amount_dec, so the same text can legitimately repeat. Constraints would also need migrations that merge existing duplicates and repoint Transfer/LineItemOptions rows. Row locks fix the races without a schema change.

Reviewed and deliberately left alone

• Stripe charge_payment: a double submit can't double-charge. Stripe tokens are single-use, so the second Charge.create fails with InvalidRequestError (the code already expects this), and its transaction.atomic() rolls back the second payment record.
• GlobalAccountingController.setup_accounts() (the global receivable/payable/grants accounts): runs only from install() on post_migrate, so it can't race in normal operation.
• SplashInfo.getForUser() (check-then-create): not accounting, and its readers already tolerate duplicates (q[0]).

Other things found (not changed)

• ATOMIC_REQUESTS = True sits at the top level of django_settings.py. Django only honors it inside a DATABASES entry, so in the repo's settings requests run in autocommit, and these locks are held only for their short atomic() blocks. If production local_settings enables it properly, locks are held until the end of the request. That's still correct, but requests that create rows for the same user or program would queue behind each other.
• DonationModule's POST handler calls iac.set_preference('Donation to Learning Unlimited', ...) with a hard-coded text. If a program customizes donation_text, that lookup won't find the line item.

Related Issue

None. This came out of an audit of the frontend tests (learning-unlimited/ESP-Website#6130).

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Refactor / cleanup
• CI/CD or build change

Testing

All runs used CI's tool versions (pytest==7.4.4, pytest-django==4.5.2, pytest-xdist==3.3.1, pytest-split==0.11.0) on Postgres 16.

Concurrency regression tests (accounting/tests/test_concurrency.py)

Each test races two threads, with save() slowed down to widen the race window. Each was checked by removing its fix:

Test	With fix	With its fix removed
test_line_item_type_created_once	passes	fails 3 of 3 (2 rows)
test_admission_charged_once	passes	fails 3 of 3 (2 admission charges)
test_double_submitted_preference_kept_once	passes	fails 3 of 3 (2 T-shirt charges)
test_financial_aid_granted_once	passes	original finalize() fails 3 of 3 (2 aid transfers)

Unit tests

• LineItemTypeGetOrCreateForProgramTest: creates when missing, reuses an existing row, newest of duplicates, ignores other programs, matches on every lookup field, applies defaults only on create, refund lookup with duplicates doesn't raise.
• DonationModuleTest: students() creates no line item and counts donors across duplicates.

The addclass concurrency test

Code	Result
Flag fix only, without the line item fix	fails about 6 in 10 (both requests 500)
This PR	passes 30 of 30, and 10 of 10 again after the final commit, with no workaround
This PR with the seat locks removed	caught a double enrollment 1 time in 10. Timing-dependent, so a weak detector, but its failures are the right ones

Suites

• Accounting, donation, Stripe, credit-card, financial-aid, extra-costs, student registration and program tests: 292 passed, on fresh databases.

• Full suite: ran deploy/ci/setup_test_db (fresh per-worker databases, as in CI), then deploy/ci/script.

  • 2,642 passed, nothing skipped.
  • 11 failures, the same 11 that fail on main in this container (no LaTeX/ghostscript, which CI installs). No new failures.

• deploy/lint passes.

• Existing tests pass

• New tests added (if applicable)

• Manually verified

AI Disclosure

Written by Claude Code (an AI assistant) at the request of @willgearty, including the investigation and local verification above.

Checklist

• Self-reviewed the code
• Updated documentation (if needed)
• No new warnings or errors introduced

🤖 Generated with Claude Code

https://claude.ai/code/session_019PKPRE8AGqavuf4cvm4v2o

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability when multiple requests occur simultaneously, preventing duplicate accounting records and duplicate financial-aid transfers.
  • Prevented donation lookups from creating unnecessary accounting records.
  • Improved handling of existing or duplicate accounting entries while preserving accurate donation and donor information.

• Tests

  • Added regression coverage for concurrent accounting, financial-aid, preference, admission, and donation scenarios.
  • Expanded coverage for program-specific accounting records and duplicate handling.

[willgearty]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 77833737-8f23-40f1-9d09-6854e2c70bdf

📥 Commits

Reviewing files that changed from the base of the PR and between 54249ff and 8dcec05.

📒 Files selected for processing: 11

• esp/.test_durations
• esp/esp/accounting/controllers.py
• esp/esp/accounting/models.py
• esp/esp/accounting/tests/test_concurrency.py
• esp/esp/accounting/tests/test_models.py
• esp/esp/db/locking.py
• esp/esp/program/models/__init__.py
• esp/esp/program/modules/handlers/creditcardmodule_stripe.py
• esp/esp/program/modules/handlers/donationmodule.py
• esp/esp/program/modules/tests/test_addclass_concurrency.py
• esp/esp/program/modules/tests/test_donationmodule.py

Limit details: You’ve used all 10 included reviews currently available.

📝 Walkthrough

Accounting record creation and updates now use row locking in several paths. Program modules use program-scoped line-item creation, and new tests cover concurrent accounting operations and donation lookups.

Changes: Accounting concurrency

Layer / File(s)	Summary
Locking helpers and line-item creation esp/esp/db/locking.py, esp/esp/accounting/models.py, esp/esp/accounting/controllers.py, esp/esp/accounting/tests/test_models.py, esp/esp/program/models/__init__.py	Added locked retrieval or creation helpers. Accounting setup and refund, donation, and sibling-discount line-item paths use the program-scoped helper. Tests cover line-item creation, lookup, defaults, and duplicate handling.
Serialized accounting operations esp/esp/accounting/controllers.py, esp/esp/accounting/models.py, esp/esp/accounting/tests/test_concurrency.py, esp/esp/program/models/__init__.py, esp/.test_durations, esp/esp/program/modules/tests/test_addclass_concurrency.py	User rows are locked before preference, required-transfer, and financial-aid updates. Financial-aid finalization rechecks persisted state before creating a transfer. Concurrency tests check single creation and finalization. Test-duration metadata and the lock-support feature check are updated.
Donation handlers and lookup tests esp/esp/program/modules/handlers/creditcardmodule_stripe.py, esp/esp/program/modules/handlers/donationmodule.py, esp/esp/program/modules/tests/test_donationmodule.py	Donation handlers use program-scoped line-item creation. DonationModule.students() filters by program and donation text without creating a line item. Tests cover no creation during lookup and donor counts with duplicate line-item types.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ProgramAccountingController
  participant lock_row
  participant Database
  ProgramAccountingController->>lock_row: Lock the user's row
  lock_row->>Database: Request FOR NO KEY UPDATE
  Database-->>lock_row: Return the locked row
  lock_row-->>ProgramAccountingController: Complete the lock
  ProgramAccountingController->>Database: Replace or create preference transfers

Loading

Merge Risk: ⚪ Minimal · up to 8dcec

The change serializes accounting record creation and updates to prevent duplicates under concurrent requests, and it enables the previously skipped concurrency test. No concrete merge-blocking risk was identified in the supplied evidence.

Security architecture risk: 🔵 Low · up to 8dcec

The changes strengthen protection against duplicate charges and grants. No newly introduced security weakness was established in the examined paths. Remaining uncertainty concerns recovery, historical duplicates, and coordination with operations outside the new locking protocol.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The directly affected assets are financial records and registration-derived donor membership. Program locks coordinate participating metadata creators across users in that program; user locks coordinate participating financial mutations for that user, including across programs. Lock contention can therefore extend beyond one request, without itself granting additional authority.

Trust Boundaries and Controls

• observed — The examined payment entrypoint continues to construct accounting identity from the program and request.user. Before invoking Stripe, it compares the submitted purchase-order identity and amount against server-derived values. These existing controls remain separate from the new locking mechanism; locking is not authorization.

Resilience and Maintainability Implications

• observed — The existing Stripe path performs an external charge within a local database transaction. The PR does not add cross-system commit recovery or payment replay protection. Its improved database serialization must not be interpreted as an exactly-once guarantee for external payments.

Hardening Proposals

• proposed — Separate lifecycle hardening could coordinate revocation and sibling-discount removal with their creators, and define explicit refund replay handling. These pre-existing paths remain outside the newly added owner-lock protocol; they are not retained as introduced PR concerns.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 28.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 10 files. (1 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarizes the two main changes: fixing accounting race conditions and running the addclass concurrency test.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 28.85% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 10 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

• Mode: Continue automatically
• Reviewed files: 11
• Waived: $2.75 (charged $0.00)
• View usage details

Note

This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. Manage usage-based reviews.

_{Comment @coderabbitai help to get the list of available commands.}