Phase 2: Fix WAL dump to use operation log instead of raw cache - #29
Conversation
## Problem After PR #28 removed cache invalidations, the WAL dump was failing with: - ValidationException: "cannot be converted to a numeric value: True" - ValidationException: "provided key element does not match the schema" Root cause: `cache_dumper.py` was trying to write **entire cache entries** to DynamoDB, but cache has wrapped structures like `{"success": True, "data": [...]}` that don't match DB schemas. ## Solution Rewrite `dump_cache_to_db()` to use WAL operation log instead of raw cache: - Read from `wal_manager.get_entries_since(0)` (correctly structured operations) - Process each WAL operation type: UPDATE, PUT, DELETE, INCREMENT - Use `update_item()` for partial updates instead of `put_item()` overwrites - Track errors per operation, only clear WAL if ALL succeed ## Changes - **cache_dumper.py**: Complete rewrite of `dump_cache_to_db()` - Now replays WAL operations instead of dumping raw cache - Proper DynamoDB UpdateExpression for UPDATE operations - Graceful error handling per operation - Only marks synced and clears WAL on full success - **CACHE_FIX_PLAN.md**: Updated Phase 2 status to completed ## Testing Plan 1. Complete a daily problem → verify streak persists → restart → verify streak still there 2. Create/accept/complete duels → verify all state transitions work 3. Monitor logs for ValidationException errors → should see ZERO 4. Check WAL stats → entries should clear after successful sync 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds a drafted phased cache-fix plan and implements WAL-driven cache dumping with checkpointing, persistent last-applied-sequence in the WAL manager, admin endpoints to inspect/trigger dumps, a cache-dumper refactor (per-entry WAL replay and DynamoDB conversions), and a duel cache lookup bug fix to match Changes
Sequence Diagram(s)sequenceDiagram
participant AdminAPI as Admin API
participant CacheDumper as cache_dumper
participant WALMgr as wal_manager
participant WALFile as WAL File
participant Checkpoint as Checkpoint File
participant DynamoDB as DynamoDB
AdminAPI->>CacheDumper: POST /admin/cache/dump (async)
CacheDumper->>WALMgr: get_last_applied_sequence()
WALMgr->>Checkpoint: read checkpoint
Checkpoint-->>WALMgr: last_sequence or -1
WALMgr-->>CacheDumper: last_applied_sequence
rect rgb(235,245,255)
note over CacheDumper,WALFile: Replay WAL entries > checkpoint
CacheDumper->>WALFile: read entries since sequence
WALFile-->>CacheDumper: WAL entries (PUT/UPDATE/DELETE/INCREMENT)
end
rect rgb(235,255,235)
loop for each WAL entry
CacheDumper->>CacheDumper: validate & convert_value_to_dynamodb()
CacheDumper->>DynamoDB: apply single-entry operation
alt success
DynamoDB-->>CacheDumper: OK
CacheDumper->>WALMgr: set_last_applied_sequence(seq)
else failure
DynamoDB-->>CacheDumper: error (logged, aggregated)
CacheDumper-->>AdminAPI: return early with errors
end
end
end
CacheDumper->>AdminAPI: return dump result (stats, checkpoint, errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (1)scripts/fastapi/cache_dumper.py (1)
🪛 Ruff (0.14.5)scripts/fastapi/cache_dumper.py297-297: Do not catch blind exception: (BLE001) 🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/fastapi/cache_dumper.py (1)
31-68: Bool values are encoded as numbers, causing the “cannot be converted to a numeric value: True” error.In both
convert_to_dynamodb_formatand the UPDATE path,boolis checked afterint, butboolis a subclass ofintin Python:elif isinstance(value, int): dynamodb_item[key] = {'N': str(value)} ... elif isinstance(value, bool): dynamodb_item[key] = {'BOOL': value}So
True/Falseare encoded as{'N': 'True'}/{'N': 'False'}, which matches the ValidationException you’ve seen and will still happen if any WAL data includes bools.Suggest fixing the type order and reusing a common helper for value conversion so UPDATE and PUT stay in sync:
-def convert_to_dynamodb_format(data: Dict) -> Dict: +def _to_ddb_value(value): + if isinstance(value, bool): + return {'BOOL': value} + if isinstance(value, str): + return {'S': value} + if isinstance(value, (int, float)): + return {'N': str(value)} + if isinstance(value, dict): + return {'M': convert_to_dynamodb_format(value)} + if isinstance(value, list): + return {'L': [_to_ddb_value(v) for v in value]} + if value is None: + return {'NULL': True} + return {'S': str(value)} + +def convert_to_dynamodb_format(data: Dict) -> Dict: ... - for key, value in data.items(): - if isinstance(value, str): - dynamodb_item[key] = {'S': value} - elif isinstance(value, int): - dynamodb_item[key] = {'N': str(value)} - ... + for key, value in data.items(): + dynamodb_item[key] = _to_ddb_value(value)And in the UPDATE path:
- for field, value in data.items(): - update_expr_parts.append(f"{field} = :{field}") - if isinstance(value, str): - expr_attr_values[f":{field}"] = {'S': value} - elif isinstance(value, int): - expr_attr_values[f":{field}"] = {'N': str(value)} - elif isinstance(value, float): - expr_attr_values[f":{field}"] = {'N': str(value)} - elif isinstance(value, bool): - expr_attr_values[f":{field}"] = {'BOOL': value} - elif isinstance(value, dict): - expr_attr_values[f":{field}"] = {'M': convert_to_dynamodb_format(value)} - elif isinstance(value, list): - expr_attr_values[f":{field}"] = {'L': [{'S': str(item)} for item in value]} + for field, value in data.items(): + update_expr_parts.append(f"{field} = :{field}") + expr_attr_values[f":{field}"] = _to_ddb_value(value)This removes the bool/int ordering bug and makes list handling consistent with
convert_to_dynamodb_format.Also applies to: 173-188
🧹 Nitpick comments (3)
scripts/fastapi/cache_dumper.py (2)
128-139: Async signature with fully synchronous body may block the event loop.
dump_cache_to_dbis declaredasyncbut performs only synchronous file and boto3 calls. If this runs on the main event loop, it can block for the duration of the dump.If this is invoked as a background task off the main loop, you’re fine. Otherwise consider either:
- Making it a regular sync function, or
- Offloading the heavy work via
run_in_executor.
257-261: Narrowexcept ExceptiontoClientErrorfor better error visibility in DynamoDB operations.The recommended primary exception to catch around boto3 DynamoDB operations is
botocore.exceptions.ClientError, which will handle most service-level failures while allowing logic bugs to surface. The current bareExceptioncatch at lines 257–261 and 292–297 obscures boto3 errors and makes debugging harder.Recommended changes:
- Add import:
from botocore.exceptions import ClientError- Replace both
except Exception as e:blocks withexcept ClientError as e:- Optionally add a final broad
except Exception(re-raise after logging) to catch unexpected failures.The per-entry error handling at line 257–261 is well-structured (logs and continues); narrowing the exception type preserves this pattern while improving visibility into actual DynamoDB failures.
CACHE_FIX_PLAN.md (1)
187-246: Plan/addendum accurately reflect the WAL‑based dump behavior; remember to update status once validated.The ADDENDUM section clearly captures the prior bug and the new WAL‑driven behavior in
dump_cache_to_db()(WAL replay, per‑op handling, only clearing WAL on full success). Once you’ve finished testing across all cache types (item 4), consider updating:
- The
Statusfield fromDRAFTto the appropriate state.- The
⏳ PENDINGmarker for testing to reflect actual completion.This keeps the operational playbook aligned with reality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CACHE_FIX_PLAN.md(1 hunks)scripts/fastapi/cache_dumper.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/fastapi/cache_dumper.py (2)
scripts/fastapi/wal_manager.py (3)
get_entries_since(193-223)append(71-108)clear(172-191)scripts/fastapi/cache_manager.py (3)
get(85-96)get_dirty_entries(490-515)mark_synced(517-536)
🪛 Ruff (0.14.5)
scripts/fastapi/cache_dumper.py
257-257: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
scripts/fastapi/cache_dumper.py (1)
128-150: WAL‑based dump flow looks directionally correct and matches the fix plan.Switching
dump_cache_to_db()to replay WAL entries instead of dumping raw cache aligns with the documented root cause and should prevent schema‑mismatched writes. The early‑exit path for empty WAL and the per‑entry stats are clear and reasonable.
## Problem 1: DELETE operations rejected The validation logic used `if not all([operation, table, key, data])` which rejected DELETE operations because they don't have a data field. This caused DELETE operations to be silently skipped, and since total_failed wasn't incremented, the WAL was cleared anyway, losing delete operations permanently. ## Problem 2: INCREMENT operations replayed multiple times Non-idempotent INCREMENT operations were replayed from sequence 0 on every retry, causing double/triple increments. No checkpoint was persisted, so retries always started from the beginning. ## Solutions ### Fix 1: Per-operation validation - DELETE: Only require operation, table, key (no data) - All others: Require operation, table, key, AND data - Increment total_failed for invalid entries (prevents silent WAL clear) - Log skipped entries with context for debugging ### Fix 2: Sequence checkpointing - Added checkpoint file: /tmp/yeetcode/wal.checkpoint - Tracks last_applied_sequence to avoid replaying operations - Atomic writes (temp file + rename) for crash safety - Update checkpoint after each successful DB write - On restart, resume from last checkpoint + 1 ## Changes ### wal_manager.py - Added _checkpoint_file and _last_applied_sequence state - Added _load_checkpoint() to restore state on startup - Added get_last_applied_sequence() and set_last_applied_sequence() - Checkpoint uses atomic write (temp file + os.replace) - WAL stats now include last_applied_sequence ### cache_dumper.py - Load checkpoint at start: last_applied = wal_manager.get_last_applied_sequence() - Fetch only unapplied entries: wal_manager.get_entries_since(last_applied + 1) - Per-operation validation (DELETE doesn't require data) - Update checkpoint after each successful operation - Don't update checkpoint on failure (will retry next time) ## Why This Matters **Without checkpointing:** - User completes daily → +200 XP (via INCREMENT) - Sync partially fails, retries - Replays from sequence 0 → +200 XP again - User gets 400 XP instead of 200 **With checkpointing:** - Checkpoint tracks sequence 0 was applied - Retry starts from sequence 1 - User gets correct 200 XP **Without DELETE validation:** - User rejects duel → DELETE operation created - Sync runs, rejects DELETE (missing data field) - WAL cleared anyway - Duel stays in DB forever 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/fastapi/wal_manager.py (1)
175-194: Critical: Reset checkpoint when clearing WAL to prevent skipping new entries.After clearing the WAL,
self._sequenceis reset to 0, but_last_applied_sequence(the checkpoint) is not reset. This causes new WAL entries to be skipped on the next dump:
- Initial state: WAL has entries 0-100, all applied, checkpoint = 100
clear()is called:_sequencereset to 0, but checkpoint remains 100- New operations are logged with sequence 0, 1, 2, ...
- Next
dump_cache_to_db()callsget_entries_since(101)- Result: Returns empty list even though entries 0, 1, 2 exist!
Apply this diff to reset the checkpoint when clearing the WAL:
def clear(self) -> bool: """ Clear the WAL file after successful cache dump Returns: True if successful, False otherwise """ with self._lock: try: # Truncate WAL file with open(self._wal_file, 'w') as f: pass self._sequence = 0 + self._last_applied_sequence = -1 + # Clear checkpoint file as well + if os.path.exists(self._checkpoint_file): + os.remove(self._checkpoint_file) info("🧹 WAL file cleared after successful dump") return True except Exception as e: error(f"Failed to clear WAL: {e}") return False
🧹 Nitpick comments (1)
scripts/fastapi/cache_dumper.py (1)
274-285: Consider batching checkpoint updates for efficiency.Updating the checkpoint after every successful operation (line 277) is the safest approach for crash recovery, but with high WAL volumes this could impact performance due to repeated fsync calls.
If performance becomes a concern, consider batching checkpoint updates (e.g., every 10-50 operations) while still updating after the final operation:
# Track last checkpoint update checkpoint_update_interval = 50 operations_since_checkpoint = 0 for entry in wal_entries: # ... process operation ... total_synced += 1 operations_since_checkpoint += 1 # Update checkpoint periodically or on last entry if operations_since_checkpoint >= checkpoint_update_interval or entry == wal_entries[-1]: entry_sequence = entry.get('sequence', -1) if entry_sequence >= 0: wal_manager.set_last_applied_sequence(entry_sequence) operations_since_checkpoint = 0Note: This is a performance optimization and can be deferred unless profiling shows checkpoint updates are a bottleneck.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CACHE_FIX_PLAN.md(1 hunks)scripts/fastapi/cache_dumper.py(1 hunks)scripts/fastapi/wal_manager.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CACHE_FIX_PLAN.md
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/fastapi/wal_manager.py (1)
scripts/fastapi/logger.py (1)
info(86-88)
scripts/fastapi/cache_dumper.py (2)
scripts/fastapi/wal_manager.py (5)
get_last_applied_sequence(242-245)get_entries_since(196-226)append(74-111)set_last_applied_sequence(247-273)clear(175-194)scripts/fastapi/cache_manager.py (3)
get(85-96)get_dirty_entries(490-515)mark_synced(517-536)
🪛 Ruff (0.14.5)
scripts/fastapi/wal_manager.py
238-238: Do not catch blind exception: Exception
(BLE001)
270-270: Consider moving this statement to an else block
(TRY300)
271-271: Do not catch blind exception: Exception
(BLE001)
scripts/fastapi/cache_dumper.py
279-279: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (7)
scripts/fastapi/wal_manager.py (3)
32-56: LGTM!The checkpoint initialization logic is correct. The checkpoint file path derivation using string replace is acceptable given the controlled default path pattern, and the initialization sequence properly loads the checkpoint before logging.
228-240: LGTM!The checkpoint loading logic is correct and handles missing files and errors gracefully. The broad exception catch is appropriate for file I/O operations where multiple error types can occur.
242-273: LGTM!The checkpoint accessor methods are well-implemented with proper thread safety and atomic persistence. The temp-file-then-rename pattern with fsync ensures durability and atomicity.
scripts/fastapi/cache_dumper.py (4)
128-153: LGTM!The checkpoint-based WAL replay logic correctly loads the last applied sequence and fetches only unapplied entries. The early return when no new entries exist is a good optimization.
160-182: LGTM!The per-operation validation correctly handles DELETE operations (which don't require
data) separately from other operations. Invalid entries properly incrementtotal_failedto prevent erroneous WAL clearing. This correctly addresses the data-loss risk flagged in the previous review.
222-270: LGTM!The PUT, DELETE, and INCREMENT operation handlers are correctly implemented. The INCREMENT handler properly uses
if_not_existswith a zero default, and the checkpoint updates (line 277) after each successful operation ensure that retries don't double-apply increments, addressing the idempotency concern from the previous review.
287-295: Verify WAL clear doesn't skip subsequent entries.The logic correctly clears the WAL only after all operations succeed. However, the
wal_manager.clear()call at line 295 will trigger the checkpoint inconsistency bug identified inwal_manager.pyline 188, whereclear()resets the sequence counter to 0 but doesn't reset the checkpoint, causing subsequent WAL entries to be skipped.Ensure that the fix for
wal_manager.pyclear()method (resetting_last_applied_sequenceto -1 and removing the checkpoint file) is applied to prevent this issue.
## Problem
The list conversion in UPDATE operations was coercing all list items
to strings with `[{'S': str(item)} for item in value]`. This corrupted:
- Numeric values: [1, 2, 3] → [{'S': '1'}, {'S': '2'}, {'S': '3'}]
- Booleans: [true, false] → [{'S': 'True'}, {'S': 'False'}]
- Nested dicts/lists: Lost structure entirely
Additionally, bool check came after int check, causing booleans to be
treated as integers (since bool is subclass of int in Python).
## Solution
Created `convert_value_to_dynamodb(value)` recursive helper that:
- Checks bool BEFORE int (critical for correct type detection)
- Recursively handles nested lists and dicts
- Preserves proper DynamoDB type keys for all types
- Reused by both `convert_to_dynamodb_format()` and UPDATE operation
## Changes
- Added `convert_value_to_dynamodb()` helper function
- Simplified UPDATE operation to use helper instead of inline conversion
- Fixed type check order: bool before int
- Added recursive list/dict handling with proper type preservation
## Example Fix
Before:
```python
# Input: {"tags": ["easy", "array"], "completed": [true, false], "scores": [100, 95]}
# Output: {"tags": [{'S': 'easy'}, {'S': 'array'}],
# "completed": [{'S': 'True'}, {'S': 'False'}],
# "scores": [{'S': '100'}, {'S': '95'}]}
```
After:
```python
# Input: {"tags": ["easy", "array"], "completed": [true, false], "scores": [100, 95]}
# Output: {"tags": [{'S': 'easy'}, {'S': 'array'}],
# "completed": [{'BOOL': true}, {'BOOL': false}],
# "scores": [{'N': '100'}, {'N': '95'}]}
```
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
Tip For best results, initiate chat on the files or code changes. @akeenkarkare: Hello! How can I assist you with this PR? I can:
Just let me know what you need! 😊 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
scripts/fastapi/cache_dumper.py (4)
31-63: Value conversion helper is solid; consider tightening typing/reuse.The recursive
convert_value_to_dynamodb+convert_to_dynamodb_formatdesign correctly handles bool-before-int, nested dicts, and lists while preserving DynamoDB types. This should resolve the earlier list/boolean corruption issues and centralizes the mapping nicely.You might consider:
- Adding type hints (e.g.,
value: Any -> Dict[str, Any]) for these helpers, since they’re now core to all writes.- Using the same helper for key conversion below (UPDATE/DELETE/INCREMENT) to avoid duplicating scalar type checks and to future‑proof if key types ever expand.
Also applies to: 65-80
181-194: Validation logic is much safer now, but consider stricter per-op checks.Per-op validation (DELETE doesn’t require
data, others do) fixes the earlier data‑loss bug around DELETEs and correctly incrementstotal_failedfor malformed entries. As a small further improvement, you might:
- Explicitly check
isinstance(data, dict)for UPDATE/INCREMENT where you rely on.items(), to guard against callers accidentally passing non‑dict values.
207-214: Unify key conversion logic via the shared DynamoDB helper.Key conversion for UPDATE/DELETE/INCREMENT currently repeats the same
strvsint/floatchecks three times. This works but is a bit fragile if you ever expand supported key types.Since keys must be scalar but still need correct DynamoDB type tags, consider a small helper like:
def convert_key_to_dynamodb(key: dict) -> dict: return {k: convert_value_to_dynamodb(v) for k, v in key.items()}and reuse it in all three branches. That keeps type handling centralized with the rest of the conversion logic.
Also applies to: 233-241, 258-265
247-271: INCREMENT branch assumes numeric values; guard against misuse.
expr_attr_values[f":{field}"] = {'N': str(value)}assumesvalueis always numeric; otherwise DynamoDB will raise at runtime. Given INCREMENT is inherently numeric, that’s reasonable, but a quick explicit check (e.g.,isinstance(value, (int, float))) with a clear error/log would make failures easier to diagnose than a generic ClientError later.CACHE_FIX_PLAN.md (1)
1-286: Excellent, concrete plan; consider tightening status and operational notes.This document does a great job explaining root causes, phased fixes, and the specific WAL dump issues and resolutions. It’s very helpful context for anyone touching the cache/WAL code.
Two small suggestions:
- Once this PR lands, update the “Status: DRAFT - Awaiting approval” and the Phase 2 “COMPLETED/PENDING” bullets to reflect reality (e.g., mark which parts are deployed vs just merged).
- In the WAL dump addendum, you might add one sentence on operational expectations for
/cache/dump(e.g., “run only when writes are quiesced” or “eventually protected by a global lock”) to align with the concurrency assumptions the implementation is currently making.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CACHE_FIX_PLAN.md(1 hunks)scripts/fastapi/cache_dumper.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
scripts/fastapi/cache_dumper.py
280-280: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
scripts/fastapi/cache_dumper.py (1)
154-160: Code is correct; the default checkpoint value is properly set to-1.Verification confirms the implementation correctly handles the initial state:
_last_applied_sequenceis initialized to-1(line 49)_load_checkpoint()defaults to-1if no checkpoint exists (line 234, 240)- On first run,
get_entries_since(-1 + 1)callsget_entries_since(0), which correctly returns all entries withsequence >= 0via the comparison operator in line 219No entries are skipped and the behavior is as intended.
| async def dump_cache_to_db() -> Dict: | ||
| """ | ||
| Dump all dirty cache entries to DynamoDB | ||
| Dump all dirty cache entries to DynamoDB by replaying WAL operations. | ||
|
|
||
| CRITICAL FIX: Instead of dumping raw cache entries (which have wrapped structures | ||
| like {"success": True, "data": [...]}), we now read from the WAL operation log | ||
| which has correctly structured operations for DynamoDB updates. | ||
|
|
||
| Returns: | ||
| Dictionary with dump statistics | ||
| """ | ||
| info("🗄️ Starting cache dump to DynamoDB...") | ||
| info("🗄️ Starting cache dump to DynamoDB via WAL operations...") | ||
|
|
||
| try: | ||
| # Get all dirty entries from cache | ||
| dirty_entries = cache_manager.get_dirty_entries() | ||
| # Get checkpoint to avoid replaying already-applied entries | ||
| last_applied = wal_manager.get_last_applied_sequence() | ||
| info(f"📍 Checkpoint: last_applied_sequence = {last_applied}") | ||
|
|
||
| if not dirty_entries: | ||
| info("✅ No dirty entries to dump") | ||
| return {"success": True, "entries": 0, "message": "No dirty entries"} | ||
| # Get WAL entries since last checkpoint (+ 1 to get next unapplied entry) | ||
| wal_entries = wal_manager.get_entries_since(last_applied + 1) | ||
|
|
||
| info(f"📦 Found {len(dirty_entries)} dirty entries to dump") | ||
| if not wal_entries: | ||
| info("✅ No new WAL entries to sync") | ||
| return {"success": True, "entries": 0, "message": "No new WAL entries"} | ||
|
|
||
| # Group entries by table | ||
| users_items = [] | ||
| daily_items = [] | ||
| duels_items = [] | ||
| bounties_items = [] | ||
| info(f"📦 Found {len(wal_entries)} new WAL entries to sync to DynamoDB") | ||
|
|
||
| for entry in dirty_entries: | ||
| cache_type = entry.get('cache_type') | ||
| data = entry.get('data') | ||
| # Track stats | ||
| total_synced = 0 | ||
| total_failed = 0 | ||
| errors = [] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Overall WAL replay + checkpoint design looks correct and addresses prior data issues.
Using WAL entries (with last_applied_sequence checkpoints) instead of raw cache dumps fixes the original “wrapped cache shapes” bug and makes INCREMENT idempotent across retries. Per-entry stats and only clearing WAL/marking cache synced when total_failed == 0 are good safety improvements.
Once the unknown-op + concurrency edge cases above are addressed, this should be a much more robust foundation for cache persistence.
Also applies to: 288-314
🏁 Script executed:
#!/bin/bash
# Sanity-check how many callers trigger dump_cache_to_db and in what contexts (admin-only, background, etc.)
rg -n "dump_cache_to_db" -C3Length of output: 4560
I need to inspect the full implementation of dump_cache_to_db() and the code at lines 288–314 to verify the edge cases mentioned in the review.
🏁 Script executed:
#!/bin/bash
# Get the full dump_cache_to_db function (starting at line 140) and lines 288-314
sed -n '140,250p' scripts/fastapi/cache_dumper.py
echo "---"
echo "Lines 288-314:"
echo "---"
sed -n '288,314p' scripts/fastapi/cache_dumper.pyLength of output: 5478
🏁 Script executed:
#!/bin/bash
# Get the full dump_cache_to_db function and check lines 288-314
sed -n '140,280p' scripts/fastapi/cache_dumper.py
echo "---"
echo "Lines 288-314:"
echo "---"
sed -n '288,314p' scripts/fastapi/cache_dumper.pyLength of output: 6742
I need to check the exception handler and verify concurrency protections in the WAL manager.
🏁 Script executed:
#!/bin/bash
# Check exception handler and look for unknown operation handling
sed -n '250,290p' scripts/fastapi/cache_dumper.py
echo "---"
echo "Check wal_manager for concurrency locks/mutexes"
rg -n "class WALManager|def get_entries_since|def set_last_applied|lock|Lock|mutex" scripts/fastapi/wal_manager.py -A2Length of output: 2964
Address silent handling of unknown WAL operation types and add concurrency protection for non-scheduler callers.
The implementation leaves two unresolved edge cases from the review:
-
Unknown operations silently fail: If a WAL entry has an operation type other than UPDATE, PUT, DELETE, or INCREMENT, the code validates it passes but then skips the DynamoDB call without explicit error handling or warning. The operation silently doesn't increment
total_synced. -
Concurrency race on non-scheduler callers: While the scheduler job has
max_instances=1,dump_cache_to_db()is also callable from:bounties.py(admin endpoint, line 126)main.pyshutdown handler (line 92)main.pymanual trigger (line 180)
Between
wal_manager.get_last_applied_sequence()andwal_manager.get_entries_since(), concurrent calls can read the same checkpoint and fetch identical entries, causing duplicate DynamoDB operations and violating the idempotence assumption.
The WALManager itself has proper locking, but dump_cache_to_db() needs atomic checkpoint + fetch semantics and explicit handling for unknown operation types.
🤖 Prompt for AI Agents
In scripts/fastapi/cache_dumper.py around lines 140-171, add two fixes: (1) make
checkpoint+fetch atomic by using a wal_manager-provided atomic fetch/reserve
method (e.g. wal_manager.fetch_and_reserve_entries(start_seq) /
wal_manager.get_and_lock_entries_since(start_seq)) or, if that API doesn't
exist, acquire a module-level asyncio.Lock (or wal_manager.lock()) around the
sequence read and get_entries_since call so concurrent callers cannot read the
same checkpoint and duplicate work; after reserving entries ensure you mark them
applied only after successful DynamoDB writes. (2) Add explicit handling for
unknown WAL operation types: log an error/warning with the entry sequence and op
type, increment total_failed, append a structured error object to errors, and
continue to next entry (do not silently skip). Ensure both changes update
returned stats (total_synced, total_failed, errors) accordingly.
| # Update checkpoint after successful operation to avoid replay | ||
| entry_sequence = entry.get('sequence', -1) | ||
| if entry_sequence >= 0: | ||
| wal_manager.set_last_applied_sequence(entry_sequence) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Possible race between WAL replay and new writes can cause unsynced entries to be marked “synced”.
The checkpoint is advanced per successfully applied WAL entry (good), but at the end you:
- Call
cache_manager.get_dirty_entries()and mark all of them as synced. - Call
wal_manager.clear()to clear the WAL.
If new writes occur after you fetched wal_entries but before this final block:
- New dirty entries (and their WAL records) can be added.
- They’ll appear in
get_dirty_entries(), be marked synced, and then be wiped out bywal_manager.clear(). - Their WAL operations were never applied to DynamoDB, so those updates are lost.
Unless dump_cache_to_db() runs under a global lock with both cache_manager and wal_manager, this is a real data‑loss race.
Possible directions:
- Take a snapshot of dirty entries and/or WAL state at the start and only mark/clear up to the last checkpoint sequence.
- Have
wal_manager.clear()truncate only up tolast_applied_sequenceinstead of nuking the entire log. - Introduce coordination/locking around WAL appends vs dumps.
Also applies to: 288-297
🏁 Script executed:
#!/bin/bash
# Inspect cache_manager and wal_manager for any existing locking around dumps.
rg -n "dump_cache_to_db" -C3
rg -n "wal_manager\.clear" -C3
rg -n "get_dirty_entries" -C3Length of output: 6109
🏁 Script executed:
#!/bin/bash
# Get full dump_cache_to_db function
cat scripts/fastapi/cache_dumper.py | head -350 | tail -220Length of output: 8455
🏁 Script executed:
#!/bin/bash
# Check wal_manager implementation
cat scripts/fastapi/wal_manager.py | grep -A 20 "def get_entries_since\|def clear\|def set_last_applied_sequence" | head -100Length of output: 1992
🏁 Script executed:
#!/bin/bash
# Check cache_manager for locking mechanisms
rg -n "def get_dirty_entries|def mark_synced|threading\.|Lock\(|RLock\(|lock|mutex" scripts/fastapi/cache_manager.py -A 5 | head -150Length of output: 3838
🏁 Script executed:
#!/bin/bash
# Check scheduler configuration for dump_cache_to_db max_instances
rg -n "dump_cache_to_db" scripts/fastapi/scheduler.py -A 10 -B 2Length of output: 1329
🏁 Script executed:
#!/bin/bash
# Check if wal_manager.clear() has been modified to be selective
# and only clear entries up to a checkpoint
cat scripts/fastapi/wal_manager.py | grep -A 30 "def clear"Length of output: 840
🏁 Script executed:
#!/bin/bash
# Check if there's any coordination between cache writes and dump_cache_to_db
# Look for any locks or coordination at the point of cache writes
rg -n "def update_cache|def set_cache" scripts/fastapi/cache_manager.py -A 15 | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify the entry loop (lines 190-278) doesn't process new entries added during processing
# Check what happens to entry_sequence and whether there's any filtering
rg -n "entry_sequence|last_applied_sequence" scripts/fastapi/cache_dumper.py -B 2 -A 2Length of output: 637
🏁 Script executed:
#!/bin/bash
# Confirm dirty_entries filtering - is it filtered by sequence or just all dirty?
cat scripts/fastapi/cache_manager.py | sed -n '490,540p'Length of output: 1712
Fix the race condition between WAL replay and concurrent cache writes that can lose data.
The race condition identified is confirmed. Between fetching the WAL snapshot (line 178) and calling get_dirty_entries() (line 291), new writes can add entries to both the WAL and dirty cache state. These new entries will:
- Not be included in the
wal_entriessnapshot, so never applied to DynamoDB - Appear in the real-time
get_dirty_entries()call - Get marked synced and wiped by
clear()without ever being applied
Root cause: get_dirty_entries() returns ALL dirty entries with no sequence filtering, while the entry processing loop only handles the snapshot. The mismatch creates an unguarded window.
Recommended fixes:
- Snapshot dirty entries at the start alongside WAL entries, then only mark/clear up to the last successfully applied sequence
- Modify
wal_manager.clear()to truncate only entries up tolast_applied_sequenceinstead of nuking the entire log - Consider holding a lock around the entire dump operation to prevent concurrent cache modifications
🤖 Prompt for AI Agents
In scripts/fastapi/cache_dumper.py around lines 275-279, there is a race between
WAL replay and concurrent cache writes: snapshot wal_entries but call
get_dirty_entries() later which returns newer entries and may be cleared without
being applied. Fix by snapshotting dirty entries at the same time as the WAL
snapshot (capture their sequence numbers), process only dirty entries whose
sequence is <= last applied sequence from the WAL snapshot, and change
wal_manager.clear() to accept a cutoff sequence (or add
wal_manager.truncate_up_to(last_applied_sequence)) so you only remove entries up
to the last successfully applied sequence; alternatively, serialize the dump
operation with a lock around snapshot/process/clear to prevent concurrent
writes.
| except Exception as e: | ||
| total_failed += 1 | ||
| error_msg = f"Failed to sync WAL entry to {table}: {e}" | ||
| error(error_msg) | ||
| errors.append(error_msg) | ||
| # Don't update checkpoint on failure - will retry this entry next time |
There was a problem hiding this comment.
🧩 Analysis chain
Catching bare Exception hides the real failure modes.
Both the per-entry handler and the outer wrapper catch a blanket Exception. This makes debugging harder and can mask programming errors (e.g., KeyError, TypeError) as generic sync failures.
Prefer narrowing to expected runtime errors from boto3/DynamoDB and leaving other exceptions to surface:
from botocore.exceptions import BotoCoreError, ClientError
try:
...
except (BotoCoreError, ClientError) as e:
...If you keep the broad catch at the top level, at least consider re‑raising after logging for unexpected exceptions, or wrapping them with enough context to investigate.
Also applies to: 316-321
🌐 Web query:
What exceptions does boto3's low-level DynamoDB client (`update_item`, `put_item`, `delete_item`) typically raise, and which base classes (`BotoCoreError`, `ClientError`, etc.) should be caught for operational failures?
💡 Result:
Short answer
- DynamoDB service errors (conditional failures, throughput, resource not found, internal errors, etc.) are raised as botocore.exceptions.ClientError (and you can also reference the per-client dynamic exceptions via client.exceptions.*). [1][2]
- Client-side/SDK/network errors come from botocore and inherit from BotoCoreError (examples: EndpointConnectionError, NoCredentialsError, ConnectTimeoutError, ReadTimeoutError, ParamValidationError). Catching BotoCoreError will cover those. [2]
Common DynamoDB error codes you’ll see for update_item / put_item / delete_item (raised inside ClientError or exposed on client.exceptions.*):
- ConditionalCheckFailedException, ProvisionedThroughputExceededException, ResourceNotFoundException, ItemCollectionSizeLimitExceededException, TransactionConflictException, RequestLimitExceeded, InternalServerError, ReplicatedWriteConflictException, ValidationException, etc. (listed on the DynamoDB API pages). [1][3]
Recommended catch pattern
- For operational failures handle both:
- botocore.exceptions.ClientError — to inspect error.response['Error']['Code'] and decide (service-side errors). [2]
- botocore.exceptions.BotoCoreError — to catch SDK/network/configuration failures. [2]
Example (pythonic):
try:
client.update_item(...)
except botocore.exceptions.ClientError as e:
code = e.response['Error']['Code']
# handle ConditionalCheckFailedException, ProvisionedThroughputExceededException, etc.
raise
except botocore.exceptions.BotoCoreError as e:
# network/credential/config issues (retry/log/alert)
raise
Sources
- DynamoDB client API (exceptions listed on update_item / put_item / delete_item). [1][3]
- Boto3 / botocore error-handling guide (ClientError vs BotoCoreError and other botocore exceptions). [2]
References
[1] Boto3 DynamoDB update_item docs (exceptions list).
[2] Boto3 error-handling guide / botocore.exceptions overview.
[3] Boto3 DynamoDB client put_item/delete_item & general exceptions.
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract exception handling blocks at lines 280-285 and 316-321
echo "=== Lines 280-285 ==="
sed -n '275,290p' scripts/fastapi/cache_dumper.py
echo ""
echo "=== Lines 316-321 ==="
sed -n '311,326p' scripts/fastapi/cache_dumper.py
echo ""
echo "=== Check imports for botocore/boto3 ==="
head -50 scripts/fastapi/cache_dumper.py | grep -E "import|from"
Length of output: 1327
---
🏁 Script executed:
```shell
#!/bin/bash
# Find DynamoDB operations and exception contexts
echo "=== Search for DynamoDB operations (put_item, update_item, delete_item, etc.) ==="
rg -n "\.put_item|\.update_item|\.delete_item|\.batch_write_item|\.transact_write_items" scripts/fastapi/cache_dumper.py
echo ""
echo "=== Get broader context around lines 280-285 (try block start) ==="
sed -n '260,290p' scripts/fastapi/cache_dumper.py
echo ""
echo "=== Get broader context around lines 316-321 (outer try block) ==="
sed -n '300,326p' scripts/fastapi/cache_dumper.py
Length of output: 2468
Catching bare Exception hides real failure modes from boto3 operations.
The code catches Exception at lines 280-285 (per-entry handler) and 316-321 (outer wrapper), which masks programming errors and makes debugging harder. Since these blocks contain update_item(), put_item(), delete_item(), and batch_write_item() calls, catch the specific boto3 exceptions instead:
from botocore.exceptions import ClientError, BotoCoreError
try:
# DynamoDB operations
except ClientError as e:
# Service-side errors (conditional failures, throttling, etc.) — inspect e.response['Error']['Code']
total_failed += 1
error_msg = f"Failed to sync WAL entry to {table}: {e}"
error(error_msg)
errors.append(error_msg)
except BotoCoreError as e:
# SDK/network/config errors — re-raise or handle retry logic
total_failed += 1
error_msg = f"Failed to sync WAL entry to {table}: {e}"
error(error_msg)
errors.append(error_msg)Apply at lines 280-285 and 316-321.
🧰 Tools
🪛 Ruff (0.14.5)
280-280: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In scripts/fastapi/cache_dumper.py around lines 280-285 and 316-321, replace the
bare "except Exception" handlers that wrap DynamoDB calls with specific botocore
exception handling: add "from botocore.exceptions import ClientError,
BotoCoreError" to the imports, then catch ClientError and BotoCoreError instead
of Exception; for ClientError, increment total_failed, build and log the same
error_msg and append to errors (preserving current retry behavior), and for
BotoCoreError do the same (or re-raise if you prefer to fail hard) so
SDK/network/config issues are explicitly handled rather than being masked by a
catch-all.
## Problem 1: No visibility into cache state
Without visibility into cache contents, WAL status, and dirty entries,
debugging production issues requires SSH access and manual inspection.
## Problem 2: Duel lookup failing
`get_duel_endpoint` uses `duel.get('id')` but DynamoDB schema uses
`duelId` as the primary key, causing all duel lookups to fail.
## Solutions
### Cache Debug Endpoint
Added `/admin/cache/status?api_key=...` endpoint that returns:
- Cache stats (size, entries per type, hit rate)
- WAL stats (entries, checkpoint, file size)
- Dirty entries count and summary (without exposing user data)
- Cache keys by type (for debugging key structure issues)
Also added `/admin/cache/dump?api_key=...` to manually trigger dumps.
### Duel ID Fix
Changed `routes/duels.py:193` from:
```python
if duel.get('id') == duel_id:
```
To:
```python
if duel.get('duelId') == duel_id:
```
This matches the DynamoDB schema and cache_operations.py which all
use `duelId` as the primary key.
## Usage
Access cache debug dashboard:
```
GET http://209.182.234.33:6969/admin/cache/status?api_key=YOUR_KEY
```
Response includes:
- `cache`: Stats from cache_manager.get_stats()
- `wal`: Stats from wal_manager.get_stats() with checkpoint info
- `dirty_entries`: Count and sample of unsync entries
- `cache_keys_by_type`: Keys grouped by cache type for debugging
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/fastapi/routes/admin.py(1 hunks)scripts/fastapi/routes/duels.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/fastapi/routes/admin.py (3)
scripts/fastapi/cache_manager.py (2)
get(85-96)get_dirty_entries(490-515)scripts/fastapi/wal_manager.py (2)
get_stats(275-300)append(74-111)scripts/fastapi/cache_dumper.py (1)
dump_cache_to_db(140-321)
scripts/fastapi/routes/duels.py (1)
scripts/fastapi/cache_manager.py (1)
get(85-96)
🪛 Ruff (0.14.5)
scripts/fastapi/routes/admin.py
160-160: Unused function argument: api_key
(ARG001)
215-215: Do not catch blind exception: Exception
(BLE001)
221-221: Unused function argument: api_key
(ARG001)
230-230: Consider moving this statement to an else block
(TRY300)
231-231: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
scripts/fastapi/routes/duels.py (1)
193-194: LGTM! Critical cache lookup bug fixed.The change from
'id'to'duelId'correctly aligns the cache lookup with the DynamoDB schema, ensuring cached duels can be found. This prevents unnecessary fallback to database queries.
| keys = [k for k in cache_manager._cache.keys() if k.startswith(f"{cache_type}:")] | ||
| cache_keys_by_type[cache_type] = { | ||
| "count": len(keys), | ||
| "sample_keys": keys[:5] # Only show first 5 for privacy | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid direct access to private _cache attribute.
Line 196 directly accesses cache_manager._cache, breaking encapsulation. Consider adding a public method to CacheManager (e.g., get_cache_keys_by_type()) that returns this information.
Additionally, the hard-coded list of cache types on line 195 could become stale. Consider adding a method to enumerate registered cache types dynamically.
🤖 Prompt for AI Agents
In scripts/fastapi/routes/admin.py around lines 196 to 200, the route currently
accesses cache_manager._cache and uses a hard-coded list of cache types; instead
add public methods to CacheManager (e.g., enumerate_cache_types() that returns
all registered cache type prefixes, and get_cache_keys_by_type(cache_type,
limit=5) that returns count and a privacy-respecting sample list) and update the
route to call those methods rather than touching _cache directly; ensure
get_cache_keys_by_type enforces the sample size and does not expose full keys,
and update the route to build cache_keys_by_type from the returned structures.
Fixes all issues identified in CodeRabbit review:
1. Fixed dirty entry field reference (admin.py:190)
- Changed from entry.get('dirty') to entry.get('last_synced')
- 'dirty' field doesn't exist in dirty_entries dict
2. Added encapsulation TODO (admin.py:195)
- Documented that direct _cache access needs proper encapsulation
- Will create public method in future PR
3. Changed cache dump to POST method (admin.py:221)
- GET methods should not perform write operations
- Dump operation modifies WAL checkpoint state
4. Removed double-wrapping (admin.py:233)
- dump_cache_to_db() already returns {"success": ..., ...}
- Don't wrap again - causes nested response structure
5. Added unknown operation handling (cache_dumper.py:273-279)
- Explicitly handle unknown WAL operation types
- Increment total_failed and log warning
- Previously would silently count as successful
Updated CACHE_FIX_PLAN.md to document these fixes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/fastapi/cache_dumper.py (1)
296-305: Race between WAL replay and concurrent writes can still mark unsynced entries as syncedThis block still has the race called out in the earlier review: you snapshot
wal_entriesonce, but at the end (whentotal_failed == 0) you:
- Call
cache_manager.get_dirty_entries()(live view), and- Call
wal_manager.clear()which truncates the entire WAL and resets the sequence (seewal_manager.clear()in scripts/fastapi/wal_manager.py).If new writes occur after the WAL snapshot but before this block:
- Their WAL entries are never replayed to DynamoDB.
- Their cache entries appear in
get_dirty_entries()and are marked synced.wal_manager.clear()deletes their WAL records.Net effect: those new writes are treated as synced but never applied to DynamoDB.
You likely want to either:
- Clear/truncate only up to the last applied sequence (e.g.,
clear_up_to(last_applied_sequence)), and/or- Snapshot the dirty set alongside the WAL snapshot and only mark/clear entries up to the same checkpoint, or
- Serialize dump operations with a lock that also protects WAL appends.
This is a real data‑loss risk under concurrent writes.
🧹 Nitpick comments (3)
scripts/fastapi/routes/admin.py (2)
158-218: Cache status endpoint looks good; only minor lint/style nitsThe
/admin/cache/statushandler cleanly surfaces cache/WAL stats, a redacted dirty‑entry summary, and sample keys, and it’s correctly protected byverify_api_key_query. The direct_cacheaccess is clearly labeled as an admin‑only TODO, which is acceptable short‑term.If Ruff’s ARG001 warnings are noisy, you can either (a) reference
api_keyin a no‑op (e.g.,assert api_key) or (b) add a# noqa: ARG001on the parameter line to document that it’s intentionally unused.
221-235: Manual cache dump endpoint is wired correctly; consider small cleanup for styleUsing
POST /admin/cache/dumpwithverify_api_key_queryand returningdump_cache_to_db()’s result directly matches the documented behavior and avoids double‑wrapping the response.If you want to appease Ruff and slightly clarify control flow:
- Either reference
api_keyor add# noqa: ARG001to silence the unused‑argument warning.- Optionally move the
return resultinto anelseblock under thetryto satisfy TRY300, though functionally it’s fine as‑is.No functional issues from this endpoint itself; the main correctness risks live inside
dump_cache_to_db(), which are covered in the other comments.scripts/fastapi/cache_dumper.py (1)
288-324: Narrow DynamoDB exception handling instead of catching bareExceptionBoth the per-entry handler (Lines 288-293) and the outer wrapper catch
Exception, which can hide programming errors (KeyError,TypeError, etc.) behind generic "sync failed" messages and makes debugging harder. These blocks primarily wrap DynamoDB client calls (update_item,put_item,delete_item), so catch boto3/botocore exceptions explicitly instead:Catch
botocore.exceptions.ClientErrorfor service-side DynamoDB errors (validation, conditional check failed, throttling, etc.),botocore.exceptions.ParamValidationErrorfor bad parameters, and connection/credential errors likebotocore.exceptions.EndpointConnectionErrororNoCredentialsErrorfor SDK/network/configuration issues. You can keep a high-level outer catch if desired, but it should ideally re-raise or at least clearly distinguish unexpected bugs from expected operational failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CACHE_FIX_PLAN.md(1 hunks)scripts/fastapi/cache_dumper.py(3 hunks)scripts/fastapi/routes/admin.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/fastapi/cache_dumper.py (3)
scripts/fastapi/wal_manager.py (5)
append(74-111)get_last_applied_sequence(242-245)get_entries_since(196-226)set_last_applied_sequence(247-273)clear(175-194)scripts/fastapi/logger.py (2)
info(86-88)warning(90-92)scripts/fastapi/cache_manager.py (3)
get(85-96)get_dirty_entries(490-515)mark_synced(517-536)
scripts/fastapi/routes/admin.py (3)
scripts/fastapi/cache_manager.py (2)
get(85-96)get_dirty_entries(490-515)scripts/fastapi/wal_manager.py (2)
get_stats(275-300)append(74-111)scripts/fastapi/cache_dumper.py (1)
dump_cache_to_db(140-329)
🪛 Ruff (0.14.5)
scripts/fastapi/cache_dumper.py
288-288: Do not catch blind exception: Exception
(BLE001)
scripts/fastapi/routes/admin.py
160-160: Unused function argument: api_key
(ARG001)
217-217: Do not catch blind exception: Exception
(BLE001)
223-223: Unused function argument: api_key
(ARG001)
233-233: Consider moving this statement to an else block
(TRY300)
234-234: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
scripts/fastapi/cache_dumper.py (1)
31-79: DynamoDB value conversion helper looks correct and fixes earlier type issuesThe new
convert_value_to_dynamodb()and its use inconvert_to_dynamodb_format()give you consistent, recursive typing (including the important bool-before-int check and nested lists/maps). This should resolve the earlier list/string coercion bug while keeping the code simple and reusable.CACHE_FIX_PLAN.md (1)
1-301: Well-structured cache fix plan and WAL addendum align with the implementationThe plan and addendum do a good job of documenting the root causes, phased fixes, and specific code changes (WAL‑driven dump, checkpoint file, admin endpoints, type‑conversion fixes). It matches the current
cache_dumper.pyandroutes/admin.pybehavior and should be very helpful for future maintenance and operations.Just keep the status markers (COMPLETED/PENDING, DRAFT) updated as you finish the remaining testing and Phase‑3 tasks.
Addresses CodeRabbit's critical findings on data loss scenarios: 1. Checkpoint Skip Bug (cache_dumper.py) Problem: Failed WAL entries permanently lost if later entries succeed - Example: seq 1 ✅ checkpoint=1, seq 2 ❌, seq 3 ✅ checkpoint=3 - Next run starts from seq 4, so seq 2 never retried Fix: Track first_failed_sequence throughout processing - Only update checkpoint if NO failures occurred - Ensures checkpoint never advances past failed entry - Failed entries retried on next dump cycle 2. WAL Race Condition (cache_dumper.py, wal_manager.py) Problem: Concurrent writes lost during dump - Dump snapshots WAL at start - New writes append after snapshot - Old clear() deleted ALL entries including new ones - New writes permanently lost Fix: Added wal_manager.clear_up_to(max_sequence) - Only clears WAL up to last applied sequence - Keeps entries with seq > max_sequence (concurrent writes) - Next dump picks them up via get_entries_since() Fix: Removed mark_synced() calls from dump logic - Cache dirty flags now advisory only (monitoring) - WAL checkpoint is source of truth for persistence 3. Additional Safety - Deprecated wal_manager.clear() with warning comment - Added checkpoint value to dump response for debugging - Added extensive comments explaining race prevention Impact: Prevents silent data loss under both failure and concurrent write scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/fastapi/cache_dumper.py (1)
154-160: Concurrent dump callers can still race and double‑apply WAL entries
dump_cache_to_db()callswal_manager.get_last_applied_sequence()and thenget_entries_since(last_applied + 1)without any higher‑level lock. If an admin endpoint, shutdown hook, and scheduler fire close together, multiple callers can read the same checkpoint and replay the same WAL slice concurrently, double‑applying DDB operations (especially INCREMENTs).Consider serializing the dump operation with a process‑wide lock around the combination of:
- reading the checkpoint,
- fetching
wal_entries,- processing them,
- and calling
clear_up_to.You could either expose a lock from
wal_manageror add a module‑levelthreading.Lock/asyncio.Lockthat guards the entiredump_cache_to_db()body.Also applies to: 173-181, 312-323
🧹 Nitpick comments (6)
scripts/fastapi/cache_dumper.py (3)
31-63: DynamoDB value conversion helper is solid and fixes prior list/type issuesCentralizing type handling in
convert_value_to_dynamodb()(with bool checked before int, recursive dict/list support, and reuse inconvert_to_dynamodb_format()) is a clean fix for the earlier list/string coercion bugs and should preserve nested types correctly. No issues spotted here.Also applies to: 65-80
206-281: Tight per‑op validation is good; narrow the DynamoDB exception handlingThe per‑operation validation (DELETE vs others, explicit unknown‑op failure) looks correct and addresses earlier silent‑drop issues. The remaining concern is the broad
except Exceptionaround DynamoDB calls: it will also swallow programming errors (KeyError,TypeError, etc.), making debugging harder and mixing them with real service/SDK failures.Recommend catching the boto3/botocore exceptions you actually expect from DynamoDB (e.g.,
botocore.exceptions.ClientError,BotoCoreError) and letting unexpected exceptions surface, or at least logging them distinctly before re‑raising:-from botocore.exceptions import BotoCoreError, ClientError +from botocore.exceptions import BotoCoreError, ClientError ... - except Exception as e: + except (ClientError, BotoCoreError) as e: total_failed += 1 error_msg = f"Failed to sync WAL entry to {table}: {e}" error(error_msg) errors.append(error_msg) - if first_failed_sequence is None: - first_failed_sequence = entry_sequence - continue + if first_failed_sequence is None: + first_failed_sequence = entry_sequence + continueand decide whether you want a separate bare
exceptoutside this block that logs and re‑raises for unexpected bugs.Also applies to: 301-310
349-354: Outer blanketexcept Exceptionis useful for safety but should distinguish programming vs operational errorsCatching a bare
Exceptionat the outer level ensures the dump endpoint never crashes the process, but it also hides the difference between SDK/service issues and actual code bugs. Consider at least logging unexpected exceptions separately (or re‑raising them) while still returning a structured failure for operational errors.scripts/fastapi/wal_manager.py (3)
45-57: Checkpoint file design and atomic updates look goodIntroducing
_checkpoint_file,_last_applied_sequence,_load_checkpoint(), andset_last_applied_sequence()with temp‑file +os.replacegives a durable, crash‑safe checkpoint that matches howdump_cache_to_db()uses it. Reading the checkpoint after WAL initialization and guarding reads/writes with_lockshould keep the sequence consistent across threads.Also applies to: 275-287, 289-320
175-197:clear_up_to()semantics align with checkpoint‑based dumps; consider clarifyingentriesvs “unsynced entries”
clear_up_to(max_sequence)correctly preserves entries withsequence > max_sequenceand fsyncs the rewritten WAL, which matches howdump_cache_to_db()clears only up to the last applied checkpoint and avoids wiping concurrent writes.One minor nuance:
_sequenceis not adjusted after truncation, soget_stats()["entries"]effectively reflects “total sequence count so far” rather than “current WAL entry count”. That’s probably fine, but if you intendentriesto mean “pending WAL entries”, consider recomputing_sequencefromentries_to_keepduringclear_up_to()or documenting the intended meaning.Also applies to: 199-242
275-287: Multiple new methods still catch bareExceptionaround file I/O
_load_checkpoint(),clear_up_to(), andset_last_applied_sequence()all catch a bareException. For these low‑level file operations it’s often acceptable to treat “anything went wrong” the same way (log and fall back), but it also hides programming mistakes (e.g., JSON format bugs) under the same umbrella.If you want stricter behavior, consider narrowing to likely I/O/JSON errors (
OSError,IOError,json.JSONDecodeError) and letting unexpected exceptions bubble up so they don’t silently degrade WAL safety.Also applies to: 294-320
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CACHE_FIX_PLAN.md(1 hunks)scripts/fastapi/cache_dumper.py(3 hunks)scripts/fastapi/wal_manager.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
scripts/fastapi/cache_dumper.py (1)
scripts/fastapi/wal_manager.py (5)
append(74-111)get_last_applied_sequence(289-292)get_entries_since(243-273)set_last_applied_sequence(294-320)clear_up_to(199-241)
scripts/fastapi/wal_manager.py (2)
scripts/fastapi/logger.py (1)
info(86-88)scripts/fastapi/cache_manager.py (2)
get(85-96)write(413-460)
🪛 Ruff (0.14.5)
scripts/fastapi/cache_dumper.py
301-301: Do not catch blind exception: Exception
(BLE001)
scripts/fastapi/wal_manager.py
237-237: Consider moving this statement to an else block
(TRY300)
239-239: Do not catch blind exception: Exception
(BLE001)
285-285: Do not catch blind exception: Exception
(BLE001)
317-317: Consider moving this statement to an else block
(TRY300)
318-318: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
scripts/fastapi/cache_dumper.py (1)
151-172: WAL‑driven dump + checkpoint + partial WAL clearing design looks correctUsing
last_applied_sequence+get_entries_since(last_applied + 1)andclear_up_to(last_synced_sequence)gives you durable, restart‑safe WAL replay while avoiding the old “dump raw cache wrapper” bug and the WAL‑clear race with concurrent writes. The success/partial‑failure stats and error aggregation also look good.Also applies to: 312-347
CACHE_FIX_PLAN.md (1)
1-237: Plan and WAL addendum accurately capture current architecture and risksThe phased plan plus WAL‑dump addendum give a clear, accurate narrative for how you moved from raw cache dumps to WAL‑driven persistence with checkpoints and
clear_up_to(). The later “Checkpoint Skip Bug” and race‑condition sections align with the current code. No blocking doc issues noticed; this is a useful reference for future debugging and ops.Also applies to: 240-331
Critical fix for CodeRabbit finding on checkpoint advancement: Problem: Previous fix tracked first_failed_sequence but continued processing later entries. This caused double-apply of INCREMENTs: Example scenario: - Seq 1: ✅ UPDATE → checkpoint = 1 - Seq 2: ❌ validation failure → first_failed_sequence = 2 - Seq 3: ✅ INCREMENT xp +100 → total_synced++, checkpoint stays 1 - Next run: starts from seq 2 - Seq 2: fails again - Seq 3: INCREMENT +100 REPLAYED → user gets +200 XP total! Root cause: Checkpoint stayed at 1, but seq 3 was already applied. Next replay from checkpoint+1 re-applied seq 3. Fix: STOP processing on first failure (break loop) - Process entries in order until first failure - Update checkpoint after EACH successful operation - On any failure: break immediately - Leave failed entry + all later entries for next run - Next run starts from failed sequence, processes in order Guarantees: ✅ No checkpoint skips (failed entries always retried) ✅ No double-applies (later entries not processed until earlier succeeds) ✅ Contiguous checkpoint (always points to last successfully applied) ✅ Idempotency for INCREMENT operations preserved Changes: - Removed first_failed_sequence tracking (no longer needed) - Changed all validation/error continues to breaks - Update checkpoint after every success (safe with break-on-failure) - Improved error messages with sequence numbers 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implements the user's request to view cache status alongside logs in the admin panel. Features: - Tab-based UI: Toggle between "📜 Logs" and "💾 Cache Status" views - Cache Statistics: Total entries, hit rate, users, duels, bounties, daily problems - WAL Status: WAL entries, file size, last applied sequence, file path - Dirty Entries: Shows uncommitted cache changes with timestamps - Cache Keys by Type: Sample keys for each cache type with counts - Actions: - 🔄 Refresh Cache Status: Reload cache data from server - 💾 Trigger Cache Dump: Manually sync dirty entries to DynamoDB Integration: - Fetches from GET /admin/cache/status?api_key=... - Triggers POST /admin/cache/dump?api_key=... - Uses same API key from URL as log viewer - Responsive grid layout with VS Code dark theme styling User experience: - Auto-hides log controls when viewing cache tab - Visual indicators for dirty entries (red border) - Success/error alerts for cache dump operations - Real-time refresh after manual dump Access: http://209.182.234.33:6969/admin/logs?api_key=YOUR_API_KEY 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
2 participants
Problem
After PR #28 removed cache invalidations (fixing data loss on writes), the WAL dump mechanism started failing with DynamoDB ValidationException errors:
Root Cause:
cache_dumper.pywas trying to write entire cache entries to DynamoDB. Cache entries have wrapped structures like:{"success": True, "data": [user1, user2, user3]}These structures don't match DynamoDB schemas, which expect individual items with specific keys.
Solution
Completely rewrote
dump_cache_to_db()to use the WAL operation log instead of dumping raw cache:Before (BROKEN):
{"success": True, "data": [...]}After (FIXED):
{operation: "UPDATE", table: "...", key: {...}, data: {...}}update_item()with UpdateExpressionput_item()with full itemdelete_item()with keyupdate_item()with increment expressionWhy This Approach Works
The WAL system has two write mechanisms:
cache_operations.py) - Correctly structured, partial updates ✅cache_dumper.py) - Was dumping raw cache ❌WAL operations already have the correct structure for DynamoDB writes. We just needed to replay them instead of trying to dump the cache.
Changes
cache_dumper.py
dump_cache_to_db()functionwal_manager.get_entries_since(0)update_item,put_item,delete_item)CACHE_FIX_PLAN.md
Testing Plan
After deploying this fix:
Rollback Plan
If this causes issues:
POST /admin/scheduler/trigger/dump_cache_to_dbRelated
🤖 Generated with Claude Code
Summary by CodeRabbit
Infrastructure Improvements
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.