Fava client hardening: write-lock coverage, shared HTTP client, JSON assertions #58

Open

padreug wants to merge 2 commits from fix/fava-client-hardening into fix/settlement-balance-and-decimal

padreug commented

2026-07-12 13:43:11 +00:00

Owner

Fourth PR of the refactor series (stacked on #57 → #56 → #55; merge in order).

Problems (CODE-REVIEW-2026-06 #7, #14, #15, #19 + libra-#23, libra-#39, libra-#53):

Approve/reject endpoints did raw httpx read-checksum-modify-write against Fava /source with no lock at all — two concurrent mutations raced each other and every other ledger writer (libra-#23).
ChecksumConflictError existed but was never raised; 409/412 leaked as raw HTTPStatusError 500s.
12 per-call httpx.AsyncClient instantiations — a TCP handshake per Fava request.
Account names interpolated into BQL string literals unvalidated.
Integer-only SATS regex silently dropped decimal-SATS postings from balances.
POST /assertions submitted a Beancount source string to Fava's JSON API — every assertion create 500'd (libra-#39).
add-account verified its own write with a second serialized get_all_accounts round-trip (libra-#53).

Changes:

New FavaClient.transform_source_line(filename, lineno, fn) — the whole read-modify-write under _write_lock, 409/412 → ChecksumConflictError. Approve/reject rewired through it; conflicts surface as HTTP 409.
update_entry_source/delete_entry raise ChecksumConflictError on 409/412.
One shared httpx.AsyncClient per FavaClient with aclose() wired into libra_stop; health probes keep a 2s per-request timeout.
_validate_bql_account guards both interpolation sites (^[A-Za-z0-9:_-]+$).
Amount-string regexes consolidated to module-level compiled patterns, all decimal-tolerant.
format_balance returns Fava's {"t": "Balance", "amount": {number, currency}} dict — the seven strict-xfail reconciliation tests flip to passing.
sync_single_account_from_beancount grows assume_exists so the add-account endpoint skips the redundant verification round-trip.

Not addressed: libra-#43 (newer-Fava "non-source file" 500) — could not reproduce against fava 1.30.13 in the test harness (all add-account tests pass); it may predate the _resolve_target_file fix in d82443d. Worth re-testing on the deployed Fava version before closing.

Tests: new concurrent approve+reject test (both mutations must land); 7 reconciliation xfails now real passes. Full suite: 176 passed, 3 skipped, 0 xfailed.

🤖 Generated with Claude Code

Fourth PR of the refactor series (stacked on #57 → #56 → #55; merge in order). **Problems** (CODE-REVIEW-2026-06 #7, #14, #15, #19 + libra-#23, libra-#39, libra-#53): - Approve/reject endpoints did raw httpx read-checksum-modify-write against Fava `/source` with **no lock at all** — two concurrent mutations raced each other and every other ledger writer (libra-#23). - `ChecksumConflictError` existed but was never raised; 409/412 leaked as raw `HTTPStatusError` 500s. - 12 per-call `httpx.AsyncClient` instantiations — a TCP handshake per Fava request. - Account names interpolated into BQL string literals unvalidated. - Integer-only SATS regex silently dropped decimal-SATS postings from balances. - `POST /assertions` submitted a Beancount source **string** to Fava's JSON API — every assertion create 500'd (libra-#39). - add-account verified its own write with a second serialized `get_all_accounts` round-trip (libra-#53). **Changes:** - New `FavaClient.transform_source_line(filename, lineno, fn)` — the whole read-modify-write under `_write_lock`, 409/412 → `ChecksumConflictError`. Approve/reject rewired through it; conflicts surface as HTTP 409. - `update_entry_source`/`delete_entry` raise `ChecksumConflictError` on 409/412. - One shared `httpx.AsyncClient` per FavaClient with `aclose()` wired into `libra_stop`; health probes keep a 2s per-request timeout. - `_validate_bql_account` guards both interpolation sites (`^[A-Za-z0-9:_-]+$`). - Amount-string regexes consolidated to module-level compiled patterns, all decimal-tolerant. - `format_balance` returns Fava's `{"t": "Balance", "amount": {number, currency}}` dict — the seven strict-xfail reconciliation tests flip to passing. - `sync_single_account_from_beancount` grows `assume_exists` so the add-account endpoint skips the redundant verification round-trip. Not addressed: libra-#43 (newer-Fava "non-source file" 500) — could not reproduce against fava 1.30.13 in the test harness (all add-account tests pass); it may predate the `_resolve_target_file` fix in d82443d. Worth re-testing on the deployed Fava version before closing. **Tests:** new concurrent approve+reject test (both mutations must land); 7 reconciliation xfails now real passes. Full suite: 176 passed, 3 skipped, 0 xfailed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

padreug added 2 commits

2026-07-12 13:43:11 +00:00

fix(assertions): send Balance directives in Fava's JSON shape (libra-#39) c0c8acbe30

format_balance returned a Beancount source string, but fava.add_entry
feeds PUT /add_entries whose deserialiser expects
{"t": "Balance", "amount": {"number", "currency"}, ...} — every
assertion create 500'd. Returns the dict shape now.

The seven strict-xfail reconciliation tests tracking this flip to
regular passing tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

fix(fava): serialize source mutations, share HTTP client, validate BQL input 4d63e08a69

Fava-client hardening cluster (CODE-REVIEW-2026-06 #7, #14, #15, #19
+ libra-#23, libra-#53):

- New FavaClient.transform_source_line does the whole
  read-checksum-modify-write under the global write lock and maps
  Fava 409/412 to ChecksumConflictError. The approve and reject
  endpoints used to do this dance with raw httpx and no lock — two
  concurrent mutations raced each other and every other ledger
  writer (libra-#23). They now route through the new method and
  translate conflicts to HTTP 409.
- update_entry_source / delete_entry raise ChecksumConflictError on
  409/412 instead of leaking raw HTTPStatusError.
- One shared httpx.AsyncClient per FavaClient (12 per-call
  instantiations removed — no more TCP handshake per request);
  closed via libra_stop. Health probes keep their 2s timeout
  per-request.
- Account names/patterns are validated against ^[A-Za-z0-9:_-]+$
  before interpolation into BQL string literals.
- The posting amount regexes are consolidated into module-level
  compiled patterns, all decimal-tolerant — the old integer-only
  SATS pattern silently dropped decimal-SATS postings (Fava's @@->@
  normalisation emits them) from balances.
- add-account no longer verifies its own write with a second
  serialized get_all_accounts round-trip (libra-#53):
  sync_single_account_from_beancount grows an assume_exists path.

New test: concurrent approve+reject must both land (was
lost-update/412 before the lock).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

padreug referenced this pull request

2026-07-12 13:52:54 +00:00

Auth exact-matching, review-race guards, centralized account-name validation #59

padreug referenced this pull request

2026-07-12 14:01:56 +00:00

Hygiene sweep: dead code, role-assignment race, cached user lookup, stale files #60

padreug referenced this pull request

2026-07-12 14:13:07 +00:00

Split views_api into a domain package; unshadow /accounts/hierarchy #61

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix/fava-client-hardening:fix/fava-client-hardening

git switch fix/fava-client-hardening

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.