285 lines
13 KiB
Markdown
285 lines
13 KiB
Markdown
# CheddahBot Architecture Review: Data Consistency and Robustness
|
|
|
|
**Date:** 2026-02-19
|
|
**Scope:** Full codebase review focusing on ClickUp data consistency, failure modes, and thread safety
|
|
|
|
---
|
|
|
|
## 1. Data Flow Map: Complete Task Lifecycle
|
|
|
|
### 1.1 ClickUp Auto-Execution Path (Scheduler)
|
|
|
|
```
|
|
ClickUp API (source of truth for task creation)
|
|
|
|
|
v
|
|
Scheduler._poll_clickup() [scheduler.py:235]
|
|
|-- get_tasks_from_space() [traverses all folders/lists]
|
|
|-- For each task:
|
|
| |-- Check KV store for existing state (executing/completed/failed -> skip)
|
|
| |-- Verify task_type in skill_map
|
|
| |-- Verify auto_execute == true
|
|
| |-- Verify due_date within 3-week window
|
|
| v
|
|
| Scheduler._execute_task() [scheduler.py:313]
|
|
| |-- Set KV state to "executing"
|
|
| |-- Update ClickUp status to "in progress"
|
|
| |-- Call ToolRegistry.execute() with mapped tool
|
|
| |-- On success:
|
|
| | |-- If tool handled sync ("## ClickUp Sync" in output):
|
|
| | | state = "completed" in KV
|
|
| | |-- Else (fallback path):
|
|
| | | extract docx paths -> upload attachments
|
|
| | | state = "completed" in KV
|
|
| | | ClickUp status -> "internal review"
|
|
| | | Post completion comment
|
|
| |-- On failure:
|
|
| | state = "failed" in KV
|
|
| | ClickUp status -> "to do"
|
|
| | Post error comment
|
|
```
|
|
|
|
### 1.2 Press Release Pipeline (Tool-Internal)
|
|
|
|
```
|
|
write_press_releases() [tools/press_release.py:489]
|
|
|-- If no clickup_task_id passed, auto-lookup via _find_clickup_task()
|
|
|-- Set ClickUp to "in progress", post starting comment
|
|
|-- Step 1: Generate 7 headlines (chat brain)
|
|
|-- Step 2: AI judge picks best 2 (chat brain)
|
|
|-- Step 3: Write 2 PRs (execution brain x2)
|
|
|-- Upload docx attachments to ClickUp mid-pipeline
|
|
|-- Step 4: Generate 2 JSON-LD schemas (execution brain x2)
|
|
|-- Set ClickUp to "internal review"
|
|
|-- Update KV state to "completed"
|
|
|-- Output includes "## ClickUp Sync" marker
|
|
```
|
|
|
|
### 1.3 Link Building Pipeline (Tool-Internal)
|
|
|
|
```
|
|
run_cora_backlinks() [tools/linkbuilding.py:455]
|
|
|-- If no clickup_task_id, auto-lookup via _find_clickup_task()
|
|
|-- Step 1: subprocess call to Big-Link-Man ingest-cora
|
|
|-- Step 2: subprocess call to Big-Link-Man generate-batch
|
|
|-- On success: _complete_clickup_task() -> status "complete", KV "completed"
|
|
|-- On failure: _fail_clickup_task() -> status "internal review", KV "failed"
|
|
```
|
|
|
|
### 1.4 Folder Watcher Path
|
|
|
|
```
|
|
Scheduler._folder_watch_loop() [scheduler.py:461]
|
|
|-- Scan watch folder for *.xlsx
|
|
|-- For each file:
|
|
| |-- Check KV "linkbuilding:watched:{filename}" (skip completed/processing/failed)
|
|
| |-- Match filename to ClickUp task via fuzzy keyword match
|
|
| |-- Set KV to "processing"
|
|
| |-- Call run_cora_backlinks via tool registry
|
|
| |-- On success: move file to processed/, KV "completed"
|
|
| |-- On failure: KV "failed"
|
|
```
|
|
|
|
---
|
|
|
|
## 2. Single Points of Failure
|
|
|
|
### 2.1 CRITICAL: Crash During Execution Leaves Task in "executing" Limbo
|
|
|
|
**File:** `scheduler.py:328-344`
|
|
|
|
When `_execute_task()` sets the KV state to `"executing"` and updates ClickUp to `"in progress"`, then the process crashes before the tool completes, the task ends up in a permanent dead state:
|
|
|
|
- **KV store** says `"executing"` -- the scheduler will never pick it up again (skips `executing`, `completed`, `failed`)
|
|
- **ClickUp** says `"in progress"` -- the poll only queries `["to do"]` status
|
|
- **Result:** The task is permanently orphaned. No automatic recovery. No human alert.
|
|
|
|
**Severity:** CRITICAL. A crashed process could silently drop a paid client deliverable with no notification.
|
|
|
|
**Mitigation today:** The `clickup_reset_task` and `clickup_reset_all` tools exist (`clickup_tool.py`), but they require someone to manually notice and intervene.
|
|
|
|
### 2.2 CRITICAL: ClickUp API Down During Status Update
|
|
|
|
**File:** `scheduler.py:343`
|
|
|
|
If `update_task_status()` fails (returns `False`), the code continues execution anyway. The ClickUp status stays at `"to do"` while the KV says `"executing"`. On next poll cycle, the task is skipped in KV but still visible as `"to do"` in ClickUp.
|
|
|
|
The `update_task_status` method in `clickup.py` catches all exceptions and returns `False` on failure, but the caller at `scheduler.py:343` discards this return value.
|
|
|
|
**Severity:** CRITICAL. Silent data inconsistency between ClickUp and KV store.
|
|
|
|
### 2.3 HIGH: Execution Brain 5-Minute Hard Timeout
|
|
|
|
**File:** `llm.py:193`
|
|
|
|
The press release pipeline involves 4+ sequential execution brain calls. Each has a 300-second timeout, but none of the callers in `press_release.py` handle `TimeoutExpired`. If the execution brain times out mid-pipeline, intermediate files may have been written and partial docx uploaded to ClickUp.
|
|
|
|
The link building tool **does** handle `subprocess.TimeoutExpired` properly with cleanup.
|
|
|
|
**Severity:** HIGH. Partial deliverables uploaded to ClickUp with a failed state could confuse client review.
|
|
|
|
### 2.4 HIGH: SQLite DB Corruption Recovery
|
|
|
|
The SQLite database uses WAL mode, which is robust. However:
|
|
- There is **no backup mechanism**
|
|
- There is **no integrity check on startup**
|
|
- If the DB file is corrupted, the entire KV store and all task state history is lost
|
|
|
|
**Severity:** HIGH. For a production system managing paid deliverables, a single SQLite file with no backups is fragile.
|
|
|
|
### 2.5 MEDIUM: Tool Execution Has No Timeout
|
|
|
|
**File:** `tools/__init__.py:175`
|
|
|
|
`ToolRegistry.execute()` calls tool functions synchronously with no timeout wrapper. If a tool hangs, the scheduler thread blocks forever. Since there is only one ClickUp polling thread, this halts all future ClickUp processing.
|
|
|
|
### 2.6 MEDIUM: Memory Files Can Be Silently Truncated
|
|
|
|
**File:** `memory.py:78-85`
|
|
|
|
The memory system does unsynchronized read-modify-write on markdown files. If two threads both try to log at the same time, one write can overwrite the other's data.
|
|
|
|
---
|
|
|
|
## 3. ClickUp Sync Robustness
|
|
|
|
### 3.1 Source of Truth: It is Ambiguous
|
|
|
|
The system has **two** sources of truth that can disagree:
|
|
|
|
1. **KV store** (`clickup:task:{id}:state`) -- the scheduler's decision engine
|
|
2. **ClickUp API** (`task.status`) -- the human-facing project management system
|
|
|
|
Neither is authoritative:
|
|
|
|
- A task can be `"completed"` in KV but still `"in progress"` on ClickUp if the status update API call failed
|
|
- A task can be `"to do"` on ClickUp but `"executing"` in KV if the process crashed
|
|
- A human can manually move a task back to `"to do"` on ClickUp, but if KV says `"completed"`, the scheduler will never pick it up
|
|
|
|
### 3.2 Tasks Can Get Lost
|
|
|
|
**Scenario 1:** Someone moves a task to `"in progress"` manually on ClickUp -- it will never be polled (only `"to do"` is polled).
|
|
|
|
**Scenario 2:** Tasks with no due date are skipped by the scheduler. Tasks without a due date that are never assigned one are permanently invisible.
|
|
|
|
**Scenario 3:** The `DUE_DATE_WINDOW_WEEKS = 3` filter means tasks with due dates more than 3 weeks out are invisible.
|
|
|
|
### 3.3 Duplicate Executions CAN Happen
|
|
|
|
If the same tool is invoked both from the scheduler AND from the chat UI (via `_find_clickup_task` auto-lookup), both paths can set up KV state independently. Two press releases for the same client could be generated simultaneously.
|
|
|
|
**Severity:** HIGH.
|
|
|
|
### 3.4 Failed Task Retry Mechanism
|
|
|
|
The retry path works but is manual:
|
|
1. On failure, scheduler sets KV to `"failed"` and ClickUp back to `"to do"`
|
|
2. But since KV says `"failed"`, the scheduler skips it on next poll
|
|
3. To retry, someone must call `clickup_reset_task(task_id)` via chat
|
|
|
|
**Discrepancy:** The scheduler sets failed tasks to `"to do"`, but the link building tool sets them to `"internal review"`. This inconsistency means retry behavior depends on which code path handles the failure.
|
|
|
|
### 3.5 Phantom Completion
|
|
|
|
The `"## ClickUp Sync"` string check (`scheduler.py:385`) is fragile -- it could false-positive on debug output. Also, in the fallback path, a task is marked `"completed"` even if zero docx files were uploaded.
|
|
|
|
---
|
|
|
|
## 4. Thread Safety
|
|
|
|
### 4.1 Threads in Play
|
|
|
|
| Thread | Name | Purpose |
|
|
|--------|------|---------|
|
|
| Main | uvicorn | Gradio UI + FastAPI |
|
|
| Daemon | `scheduler` | Cron-based scheduled tasks |
|
|
| Daemon | `heartbeat` | Periodic checklist check |
|
|
| Daemon | `clickup` | ClickUp polling + execution |
|
|
| Daemon | `folder-watch` | CORA file watcher |
|
|
|
|
### 4.2 Shared ClickUp Client Instance
|
|
|
|
The scheduler lazy-initializes `self._clickup_client` once and reuses it across the ClickUp thread. The folder watcher also calls `self._get_clickup_client()`, which shares the same instance. If both threads are active simultaneously, this could cause request interleaving bugs in httpx.
|
|
|
|
Tool functions create their **own** ClickUpClient instances, so tool-level calls are isolated.
|
|
|
|
### 4.3 KV Store Race Conditions
|
|
|
|
Individual `kv_set` and `kv_get` calls are safe (atomic SQL). But the **read-check-modify-write** pattern in the scheduler is NOT atomic. The real risk is the chat-vs-scheduler race described in section 3.3.
|
|
|
|
### 4.4 Memory System File Writes
|
|
|
|
`remember()` and `log_daily()` both do unsynchronized read-modify-write on markdown files with no file locking. Concurrent calls can lose data.
|
|
|
|
---
|
|
|
|
## 5. Error Recovery After Crash and Restart
|
|
|
|
### 5.1 What Survives a Restart
|
|
|
|
- KV store, ClickUp task statuses, scheduled tasks, conversation history, memory files, notification history -- all persist.
|
|
|
|
### 5.2 What Does NOT Recover
|
|
|
|
- **In-flight task execution:** KV says `"executing"`, ClickUp says `"in progress"`. Task is stuck forever.
|
|
- **Folder watcher "processing" state:** KV says `"processing"`. File is stuck forever.
|
|
- There is no startup reconciliation, no stale-task detector, no watchdog for stuck states.
|
|
|
|
---
|
|
|
|
## 6. Recommendations
|
|
|
|
### CRITICAL Priority
|
|
|
|
| # | Issue | Fix | Effort |
|
|
|---|-------|-----|--------|
|
|
| C1 | Stuck "executing" after crash | On startup, scan KV for stale `"executing"` states (>1h old), move to `"failed"`, push notification | 30-50 lines |
|
|
| C2 | Ignored ClickUp API failures | Check `update_task_status()` return value before proceeding with execution | 10-20 lines |
|
|
| C3 | Chat-vs-scheduler duplicate execution | Check KV store in `_find_clickup_task()` before creating new entries | 15-25 lines/file |
|
|
|
|
### HIGH Priority
|
|
|
|
| # | Issue | Fix | Effort |
|
|
|---|-------|-----|--------|
|
|
| H1 | No DB backup | Copy DB to `.bak` on startup; periodic backups from heartbeat | 5-10 lines |
|
|
| H2 | Inconsistent failure status | Unify: scheduler and tools both use same failure status | 10-15 lines |
|
|
| H3 | Fragile "## ClickUp Sync" detection | Use structured return type or unique token instead of string parsing | 20-40 lines |
|
|
| H4 | No KV-ClickUp reconciliation | Add heartbeat check comparing ClickUp `"in progress"` vs KV states | 40-60 lines |
|
|
|
|
### MEDIUM Priority
|
|
|
|
| # | Issue | Fix | Effort |
|
|
|---|-------|-----|--------|
|
|
| M1 | Memory file race condition | Add `threading.Lock()` to MemorySystem for file writes | 10-15 lines |
|
|
| M2 | Shared httpx client across threads | Make `_get_clickup_client()` return thread-local instances | 10-15 lines |
|
|
| M3 | No tool execution timeout | Wrap tool execution in a thread with timeout | 15-25 lines |
|
|
| M4 | Poor auto-flush summary quality | Use chat brain for summarization instead of truncated concat | 20-30 lines |
|
|
| M5 | Unbounded KV growth | Add periodic cleanup of completed/failed states older than N days | 20-30 lines |
|
|
|
|
### LOW Priority
|
|
|
|
| # | Issue | Fix | Effort |
|
|
|---|-------|-----|--------|
|
|
| L1 | No programmatic health check | Add `/health` endpoint checking thread liveness and DB access | 20-30 lines |
|
|
| L2 | Unstructured state transition logging | Log every state transition as structured JSON event | 20-40 lines |
|
|
| L3 | Test coverage gaps | No tests for crash recovery, concurrent execution, folder watcher | 200+ lines |
|
|
|
|
---
|
|
|
|
## Risk Matrix
|
|
|
|
| # | Issue | Severity | Likelihood | Impact |
|
|
|---|-------|----------|------------|--------|
|
|
| C1 | Stuck "executing" after crash | CRITICAL | Medium | Task permanently lost |
|
|
| C2 | Ignored ClickUp API failures | CRITICAL | Medium | Silent inconsistency |
|
|
| C3 | Chat-vs-scheduler duplicate | CRITICAL | Low | Duplicate deliverables |
|
|
| H1 | No DB backup | HIGH | Low | Total state loss |
|
|
| H2 | Inconsistent failure status | HIGH | Medium | Confusing retry behavior |
|
|
| H3 | Fragile sync detection | HIGH | Low | Phantom completion |
|
|
| H4 | No KV-ClickUp reconciliation | HIGH | Medium | Undetected inconsistency |
|
|
| M1 | Memory file race condition | MEDIUM | Low | Lost memory entries |
|
|
| M2 | Shared httpx client | MEDIUM | Low | Corrupted HTTP requests |
|
|
| M3 | No tool execution timeout | MEDIUM | Low | Thread deadlock |
|
|
| M4 | Poor auto-flush summaries | MEDIUM | High | Lost context |
|
|
| M5 | Unbounded KV growth | MEDIUM | Low | Performance degradation |
|