aiolabs/castle
1
0
Fork 0
Code Issues 5 Pull requests 2 Projects Releases 5 Packages Wiki Activity Actions

Critical: No concurrency protection for Fava/Beancount ledger writes #4

New issue
Closed
opened 2026-01-06 16:35:24 +00:00 by padreug · 0 comments
padreug commented 2026-01-06 16:35:24 +00:00
Owner
Copy link

Summary

The Castle extension has no synchronization mechanisms protecting concurrent writes to the Fava/Beancount ledger. This can lead to data corruption, lost entries, duplicate payments, and unbalanced accounting records under concurrent load.

Problem Analysis

No Locks Found

After thorough code analysis, no asyncio.Lock, threading.Lock, or any mutual exclusion mechanism exists in the codebase for Fava write operations.

Critical Race Conditions

1. fava_client.py:add_entry() (Lines 51-110)

The core method that submits entries to Fava is completely unprotected:

async def add_entry(self, entry: Dict[str, Any]) -> Dict[str, Any]:
    async with httpx.AsyncClient(timeout=self.timeout) as client:
        response = await client.put(
            f"{self.base_url}/add_entries",
            json={"entries": [entry]},
        )
        # No locking - concurrent calls can corrupt ledger

Impact: Two concurrent expense submissions can:

  • Both read the same ledger state
  • Both write their entries independently
  • Overwrite each other's changes
  • Lose data silently

2. fava_client.py:add_account() (Lines 1217-1340)

Account creation has a check-then-act race condition:

# Step 1: Read file with sha256sum
# Step 2: Check if account exists (UNPROTECTED GAP)
# Step 3-6: Modify and write back with OLD sha256sum

When two requests try to create the same user account:

  1. Both read file (sha256: ABC123)
  2. Both check if account exists (both get false)
  3. Request A modifies and writes (sha256 now XYZ789)
  4. Request B writes with stale checksum ABC123 → 409 Conflict

3. Payment Settlement Race (tasks.py:on_invoice_paid)

Background task processing paid invoices:

async def on_invoice_paid(payment: Payment) -> None:
    balance = await fava.get_user_balance(user_id)  # Read
    entry = format_net_settlement_entry(...)
    result = await fava.add_entry(entry)  # Write - UNPROTECTED

If user pays invoice AND submits expense simultaneously, settlement entry uses stale balance data.

4. Duplicate Invoice Processing

Idempotency check followed by unprotected insert:

# Check if payment already recorded
for entry in entries:
    if link_to_find in entry_links:
        return  # Not atomic with insert!

# Another task could insert between check and write
result = await fava.add_entry(entry)  # DUPLICATE!

Failure Scenarios

Scenario Risk Impact
Two users submit expenses simultaneously HIGH One entry lost, balance wrong
Concurrent user account creation MEDIUM API errors, orphaned accounts
Invoice paid during expense submission HIGH Settlement with wrong amount
Same invoice processed twice MEDIUM Double payment, corrupt balances

Recommended Solutions

Option 1: Application-Level Locking (Recommended)

Add an asyncio.Lock in FavaClient:

class FavaClient:
    def __init__(self, ...):
        self._write_lock = asyncio.Lock()
    
    async def add_entry(self, entry: Dict[str, Any]) -> Dict[str, Any]:
        async with self._write_lock:  # Serialize all writes
            async with httpx.AsyncClient(...) as client:
                response = await client.put(...)

Pros: Simple, effective, no external dependencies
Cons: Serializes ALL writes (potential bottleneck), doesn't scale horizontally

Option 2: Per-User Locking

Use a lock manager with per-user locks:

class LockManager:
    def __init__(self):
        self._locks: Dict[str, asyncio.Lock] = {}
    
    def get_user_lock(self, user_id: str) -> asyncio.Lock:
        if user_id not in self._locks:
            self._locks[user_id] = asyncio.Lock()
        return self._locks[user_id]

# Usage
async with lock_manager.get_user_lock(user_id):
    await fava.add_entry(entry)

Pros: Better parallelism, users don't block each other
Cons: More complex, still doesn't help global operations

Option 3: Distributed Locking (Production Scale)

For multi-instance deployments, use Redis-based locking:

from redis import asyncio as aioredis

async def with_fava_lock(key: str, ttl: int = 30):
    lock = await redis.lock(f"castle:fava:{key}", timeout=ttl)
    async with lock:
        yield

Pros: Works across multiple LNbits instances
Cons: Requires Redis dependency

Option 4: Optimistic Concurrency with Retry

For add_account, implement retry with exponential backoff:

async def add_account(self, account_name: str, max_retries: int = 3):
    for attempt in range(max_retries):
        try:
            # Read current state
            sha256sum, source = await self._read_source()
            # Modify
            new_source = self._add_open_directive(source, account_name)
            # Write with checksum (Fava validates)
            await self._write_source(new_source, sha256sum)
            return
        except ChecksumConflict:
            if attempt == max_retries - 1:
                raise
            await asyncio.sleep(0.1 * (2 ** attempt))  # Backoff

Option 5: Fava Source of Truth + Idempotency Keys

Leverage Beancount links for idempotency:

async def add_entry_idempotent(self, entry: Dict, idempotency_key: str):
    # Check if entry with this link already exists in Fava
    existing = await self.query(f"SELECT * WHERE 'castle-{idempotency_key}' in links")
    if existing:
        return existing[0]  # Already recorded
    
    entry["links"] = [f"castle-{idempotency_key}"]
    async with self._write_lock:
        return await self.add_entry(entry)

Implementation Priority

  1. Immediate: Add global write lock to FavaClient (Option 1)
  2. Short-term: Implement retry logic for add_account (Option 4)
  3. Medium-term: Add idempotency keys to all entry creation (Option 5)
  4. Long-term: Per-user locking or distributed locking for scale

Affected Files

  • fava_client.py - Core write operations
  • views_api.py - All entry creation endpoints
  • tasks.py - Background payment processing
  • crud.py - get_or_create_user_account()

Severity

CRITICAL - Under moderate concurrent load (>5 users), accounting data corruption is likely. The ledger can become unbalanced, entries can be lost, and duplicate payments can occur.

## Summary The Castle extension has **no synchronization mechanisms** protecting concurrent writes to the Fava/Beancount ledger. This can lead to data corruption, lost entries, duplicate payments, and unbalanced accounting records under concurrent load. ## Problem Analysis ### No Locks Found After thorough code analysis, **no asyncio.Lock, threading.Lock, or any mutual exclusion mechanism** exists in the codebase for Fava write operations. ### Critical Race Conditions #### 1. `fava_client.py:add_entry()` (Lines 51-110) The core method that submits entries to Fava is completely unprotected: ```python async def add_entry(self, entry: Dict[str, Any]) -> Dict[str, Any]: async with httpx.AsyncClient(timeout=self.timeout) as client: response = await client.put( f"{self.base_url}/add_entries", json={"entries": [entry]}, ) # No locking - concurrent calls can corrupt ledger ``` **Impact**: Two concurrent expense submissions can: - Both read the same ledger state - Both write their entries independently - Overwrite each other's changes - Lose data silently #### 2. `fava_client.py:add_account()` (Lines 1217-1340) Account creation has a **check-then-act race condition**: ```python # Step 1: Read file with sha256sum # Step 2: Check if account exists (UNPROTECTED GAP) # Step 3-6: Modify and write back with OLD sha256sum ``` When two requests try to create the same user account: 1. Both read file (sha256: ABC123) 2. Both check if account exists (both get false) 3. Request A modifies and writes (sha256 now XYZ789) 4. Request B writes with stale checksum ABC123 → **409 Conflict** #### 3. Payment Settlement Race (`tasks.py:on_invoice_paid`) Background task processing paid invoices: ```python async def on_invoice_paid(payment: Payment) -> None: balance = await fava.get_user_balance(user_id) # Read entry = format_net_settlement_entry(...) result = await fava.add_entry(entry) # Write - UNPROTECTED ``` If user pays invoice AND submits expense simultaneously, settlement entry uses stale balance data. #### 4. Duplicate Invoice Processing Idempotency check followed by unprotected insert: ```python # Check if payment already recorded for entry in entries: if link_to_find in entry_links: return # Not atomic with insert! # Another task could insert between check and write result = await fava.add_entry(entry) # DUPLICATE! ``` ## Failure Scenarios | Scenario | Risk | Impact | |----------|------|--------| | Two users submit expenses simultaneously | HIGH | One entry lost, balance wrong | | Concurrent user account creation | MEDIUM | API errors, orphaned accounts | | Invoice paid during expense submission | HIGH | Settlement with wrong amount | | Same invoice processed twice | MEDIUM | Double payment, corrupt balances | ## Recommended Solutions ### Option 1: Application-Level Locking (Recommended) Add an asyncio.Lock in FavaClient: ```python class FavaClient: def __init__(self, ...): self._write_lock = asyncio.Lock() async def add_entry(self, entry: Dict[str, Any]) -> Dict[str, Any]: async with self._write_lock: # Serialize all writes async with httpx.AsyncClient(...) as client: response = await client.put(...) ``` **Pros**: Simple, effective, no external dependencies **Cons**: Serializes ALL writes (potential bottleneck), doesn't scale horizontally ### Option 2: Per-User Locking Use a lock manager with per-user locks: ```python class LockManager: def __init__(self): self._locks: Dict[str, asyncio.Lock] = {} def get_user_lock(self, user_id: str) -> asyncio.Lock: if user_id not in self._locks: self._locks[user_id] = asyncio.Lock() return self._locks[user_id] # Usage async with lock_manager.get_user_lock(user_id): await fava.add_entry(entry) ``` **Pros**: Better parallelism, users don't block each other **Cons**: More complex, still doesn't help global operations ### Option 3: Distributed Locking (Production Scale) For multi-instance deployments, use Redis-based locking: ```python from redis import asyncio as aioredis async def with_fava_lock(key: str, ttl: int = 30): lock = await redis.lock(f"castle:fava:{key}", timeout=ttl) async with lock: yield ``` **Pros**: Works across multiple LNbits instances **Cons**: Requires Redis dependency ### Option 4: Optimistic Concurrency with Retry For `add_account`, implement retry with exponential backoff: ```python async def add_account(self, account_name: str, max_retries: int = 3): for attempt in range(max_retries): try: # Read current state sha256sum, source = await self._read_source() # Modify new_source = self._add_open_directive(source, account_name) # Write with checksum (Fava validates) await self._write_source(new_source, sha256sum) return except ChecksumConflict: if attempt == max_retries - 1: raise await asyncio.sleep(0.1 * (2 ** attempt)) # Backoff ``` ### Option 5: Fava Source of Truth + Idempotency Keys Leverage Beancount links for idempotency: ```python async def add_entry_idempotent(self, entry: Dict, idempotency_key: str): # Check if entry with this link already exists in Fava existing = await self.query(f"SELECT * WHERE 'castle-{idempotency_key}' in links") if existing: return existing[0] # Already recorded entry["links"] = [f"castle-{idempotency_key}"] async with self._write_lock: return await self.add_entry(entry) ``` ## Implementation Priority 1. **Immediate**: Add global write lock to `FavaClient` (Option 1) 2. **Short-term**: Implement retry logic for `add_account` (Option 4) 3. **Medium-term**: Add idempotency keys to all entry creation (Option 5) 4. **Long-term**: Per-user locking or distributed locking for scale ## Affected Files - `fava_client.py` - Core write operations - `views_api.py` - All entry creation endpoints - `tasks.py` - Background payment processing - `crud.py` - `get_or_create_user_account()` ## Severity **CRITICAL** - Under moderate concurrent load (>5 users), accounting data corruption is likely. The ledger can become unbalanced, entries can be lost, and duplicate payments can occur.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
fix/authorization-security-refactor
fix/concurrency-protection
v0.0.5
v0.0.4
v0.0.3
v0.0.2
v0.0.1
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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/castle#4
No description provided.
Delete branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?