chart/docs/async_refactor.md

Async refactor — findings, priorities, and how to keep it that way

Status: P0, P1 and the loop-lag probe are done (2026-08-11). P2 and P3 outstanding. To be implemented once the in-flight chart work has landed. Everything below is from reading the code on 2026-08-11 and measuring the running app; each finding names the path it was found on.

The goal is not "more async"

The target is nothing blocks the event loop, which is not the same thing as converting everything to async def. Getting this backwards would make the app worse, so state it plainly:

• FastAPI runs a sync def route in a threadpool. Blocking work inside one never touches the loop. That is protection, not a defect.
• Converting those routes to async def removes the protection: any blocking call inside then stalls the market stream and every WebSocket.
• So the rule is per-function. A handler that only awaits should be async def. A handler doing blocking or CPU work should stay def — and must not touch loop-owned objects (see P0).

What is already right

• Every outbound HTTP call is async: httpx.AsyncClient in notify/ntfy.py and market/yahoo.py; the Schwab StreamClient is built with asyncio=True.
• The market stream is an asyncio.Task owned by the app lifespan, and the WebSocket endpoint is a coroutine.
• ManualLineStore guards itself with a threading.RLock, which is the correct primitive precisely because both threadpool routes and the loop reach it. Do not "modernise" it to asyncio.Lock — that would protect only one of them.

---

P0 — Cross-thread access to asyncio.Queue (correctness) — DONE

Sync route handlers reach loop-owned objects from a worker thread:

create_line / create_price_alert / create_comment / patch_line / delete_line   (sync def, threadpool)
  -> runtime.rebuild_levels()
    -> broadcast_level_delta() -> broadcast()
      -> queue.put_nowait(event)          # asyncio.Queue, owned by the loop

asyncio.Queue is not thread-safe. It wakes a waiting consumer by setting a Future's result, and Futures must be resolved on the loop thread — from another thread that requires loop.call_soon_threadsafe. Writing directly can drop the wakeup or corrupt internal state.

Why nobody has noticed: the market stream broadcasts roughly once a second, so a dropped wakeup is papered over by the next event almost immediately. The visible symptom would be a drawing made in one browser not appearing in another until the next tick — easy to misread as network lag.

Fix. Give Runtime the loop it belongs to and post from the correct thread:

self._loop = asyncio.get_running_loop()          # captured in start()

def broadcast(self, event: dict) -> None:
    if threading.current_thread() is threading.main_thread() and self._loop.is_running():
        self._publish(event)                      # already on the loop
    else:
        self._loop.call_soon_threadsafe(self._publish, event)

Prefer this over making the routes async def: that would move level rebuilding (P1) onto the loop, trading a rare correctness bug for a guaranteed latency one.

Verify. A test that calls a mutating route through TestClient while a WebSocket subscriber waits, asserting the event arrives without another tick intervening. Today that passes by luck.

---

P1 — Level rebuilding is CPU-bound on the loop (latency) — DONE

Measured on the dev stack, Yahoo source:

	before	after
port accepting connections	121s	4s
worst loop lag	19,545ms	526ms
steady-state lag	—	0.2–0.6ms

Two changes did it. Seeding now accumulates into the store and derives price, ATR and the level set once at the end (settle_after_seed) instead of rebuilding per replayed bar. And request_rebuild coalesces rebuilds to at most one per 250ms with a guaranteed trailing pass, which matters because Yahoo's first poll emits a whole day of minutes in one burst — that burst, not the seed, was most of the remaining 20 seconds. Bar counts, all five daily MAs, prior-day levels and VWAP are unchanged; 125 python tests and 31 e2e tests pass.

Still open from the original list: incremental moving averages, and diffing levels by fingerprint rather than by re-serialising every point. Neither is needed while lag sits under a second.

Original analysis

rebuild_levels() recomputes all five daily moving averages and re-serialises their points to diff them, on every closed bar. Measured consequence: the seed replay runs the same path per bar and takes ~82 seconds, during which the port is closed. In steady state it is once a minute, which is survivable but is the largest single thing the loop does.

Threads do not help — it is genuine CPU under the GIL. The fix is algorithmic:

1. Incremental moving averages. indicators.sma is already a rolling sum; the waste is recomputing every window from scratch each rebuild rather than advancing the last one.
2. Bulk seeding. Load seeded bars into the store directly and rebuild levels once at the end, rather than replaying each bar through on_bar.
3. Diff without re-serialising. broadcast_level_delta() compares to_dict() output including hundreds of points per MA. Compare a cheap fingerprint (last point plus length) and serialise only what changed.

Do (2) first — it is contained, testable, and removes most of the 82 seconds.

Verify. A test asserting a seed of N bars completes under a threshold, and a loop-lag probe (below) staying under ~50ms while a bar closes.

---

P2 — Blocking disk write on the loop (small, real)

on_bar (coroutine)
  -> rebuild_clusters(evaluate_alerts=True) -> dispatch_alerts() -> disarm()
    -> manual_lines.update() -> save()      # write_text + atomic replace

A ~1KB write, usually sub-millisecond, but it lands on the loop at the exact moment an alert fires, and it is unbounded on a contended disk.

Fix. Either await asyncio.to_thread(self.manual_lines.update, ...) on that path, or mark the line disarmed in memory and flush outside the tick. The same applies to any future persistence work — see the cold-restart notes, which will add far more writing than this.

---

P3 — Seeding blocks startup (architectural)

Runtime.start() awaits both seeds before uvicorn binds, so the port refuses connections for the whole ~82 seconds and any open browser logs a wall of ERR_CONNECTION_REFUSED. Fixing P1(2) may reduce this enough on its own. If it does not, seed in a background task and serve immediately — but note that changes what /api/status's warm flags mean to every consumer, so it needs its own thought rather than being bolted on.

---

Explicitly not doing

• Converting sync routes to async def. They do in-memory work behind a threadpool hop, which is correct and cheap. Changing them adds risk for no gain, and would drag P1's CPU cost onto the loop.
• asyncio.Lock in ManualLineStore. It is reached from both the loop and threadpool threads; only a threading primitive covers both.
• Multiple uvicorn workers as a performance fix. See Procfile — a second process opens a second Schwab stream and duplicates every alert.

---

Keeping it this way

Findings decay unless something enforces them. Three layers, cheapest first.

1. Rules where agents actually read them

AGENTS.md is loaded automatically by both Claude Code (via the CLAUDE.md symlink) and OpenCode every session; nothing else in the repo is guaranteed to be read. Add a short Async rules section stating:

• Nothing blocking or CPU-heavy runs on the event loop.
• Sync def routes stay sync; they run in a threadpool by design.
• A threadpool thread must never touch asyncio objects directly — post through loop.call_soon_threadsafe.
• ManualLineStore's lock is a threading lock deliberately.

Keep it to a handful of lines. A long section is skimmed; a short one is read.

2. Comments at the point of danger

A rule in a document does not stop an edit; a comment on the line does. Already done for the worker count in Procfile. Add the same at:

• Runtime.broadcast — why it posts through the loop.
• ManualLineStore._lock — why it is a threading lock.
• Each mutating route — why it is def and not async def.

3. Make a regression visible

• Loop-lag probe. DONE — Runtime.loop_lag_watch samples 100ms scheduling drift and /api/status reports loop_lag_ms. It found P1 on its first run: 19,441ms worst at startup against 1.5ms in steady state. A stall then shows up as a number instead of as "the chart feels laggy". This is the single highest-value addition here, and it costs about ten lines.
• loop.set_debug(True) in dev, which logs any callback over 100ms with a traceback — it would have named P1 immediately.
• A seed-duration test, so P1 cannot silently regress once fixed.

4. Where the record lives

The risk register in IMPLEMENTATION_PLAN.md §15 gets one row per finding, so a reader looking for known hazards finds them. This file holds the detail; the register holds the pointer.

---

Suggested order

1. P0 — correctness, small, self-contained.
2. Loop-lag probe — so P1's improvement is measurable rather than asserted.
3. P1(2) bulk seeding, then P1(1) and P1(3) if the probe still shows stalls.
4. P2 — trivial once P0 has established how work leaves the loop.
5. P3 — only if P1 leaves the startup window unacceptable.
6. Docs and comments alongside each change, not as a final sweep.