reject_pending_entry bypasses FavaClient write lock #23

New issue

Open

opened 2026-06-06 11:00:33 +00:00 by padreug · 0 comments

padreug commented

2026-06-06 11:00:33 +00:00

Owner

Bug

reject_pending_entry (views_api.py:2860-2956) mutates the Beancount source file by calling Fava's PUT /api/source directly with httpx.AsyncClient, bypassing FavaClient entirely. It does not acquire FavaClient._write_lock.

Every other mutation path in the codebase goes through FavaClient and takes the lock (add_entry, update_entry_source, delete_entry, add_account). This is the only writer that doesn't.

Failure mode

Concurrent reject + add (or two concurrent rejects against entries in the same file) race on the file's sha256sum. Fava's optimistic-CC will 412 the loser; the call site has no retry/queue. Net result: the admin's reject silently fails with no recovery path. Hasn't bitten in practice because rejects are rare and admin-driven, but it's a real correctness gap independent of any architectural improvement.

Fix

Two viable shapes:

Stopgap — wrap the existing direct /api/source call in async with fava._write_lock: to close the lock gap without changing the data model.
Together with #2 (the reversing-entry refactor) — the void path stops mutating source entirely and posts a new reversing transaction via FavaClient.add_entry, which already takes the lock. Lock gap disappears as a side-effect of consolidation.

If #2 is imminent, do this fix as part of it. If #2 slips, ship (1) standalone — this is a bug today, half a day of work, and it should not be hostage to a larger refactor's timeline.

Scope

views_api.py:2913-2954 — the direct httpx block in reject_pending_entry.

Half-day estimate for the stopgap.

## Bug `reject_pending_entry` (`views_api.py:2860-2956`) mutates the Beancount source file by calling Fava's `PUT /api/source` directly with `httpx.AsyncClient`, bypassing `FavaClient` entirely. It does not acquire `FavaClient._write_lock`. Every other mutation path in the codebase goes through `FavaClient` and takes the lock (`add_entry`, `update_entry_source`, `delete_entry`, `add_account`). This is the only writer that doesn't. ## Failure mode Concurrent reject + add (or two concurrent rejects against entries in the same file) race on the file's `sha256sum`. Fava's optimistic-CC will 412 the loser; the call site has no retry/queue. Net result: the admin's reject silently fails with no recovery path. Hasn't bitten in practice because rejects are rare and admin-driven, but it's a real correctness gap independent of any architectural improvement. ## Fix Two viable shapes: 1. **Stopgap** — wrap the existing direct `/api/source` call in `async with fava._write_lock:` to close the lock gap without changing the data model. 2. **Together with #2 (the reversing-entry refactor)** — the void path stops mutating source entirely and posts a new reversing transaction via `FavaClient.add_entry`, which already takes the lock. Lock gap disappears as a side-effect of consolidation. If #2 is imminent, do this fix as part of it. If #2 slips, ship (1) standalone — this is a bug today, half a day of work, and it should not be hostage to a larger refactor's timeline. ## Scope - `views_api.py:2913-2954` — the direct `httpx` block in `reject_pending_entry`. Half-day estimate for the stopgap.

padreug referenced this issue

2026-06-06 11:04:26 +00:00

Replace tag-based voiding with reversing entries (single mutation surface) #24

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

aiolabs/libra#23

No description provided.

Rows
Columns