From 5fee6909f2814e782f66ca4a090c38864e6f4a5b Mon Sep 17 00:00:00 2001 From: Hugh Kaznowski Date: Mon, 7 Sep 2026 13:14:19 +0100 Subject: [PATCH] Update actor split idea file --- AGENTS.md | 4 +- CLAUDE.md | 2 + ideas/0001-actor-split.md | 169 +++++++++++++++++++++----------------- 3 files changed, 99 insertions(+), 76 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 13380c1..15f076b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -5,7 +5,7 @@ ## Codebase Intelligence for ottyl (Repowise) -Indexed by [Repowise](https://repowise.dev). Last indexed: 2026-09-07 (commit dbe2ea9). Confidence: 100%. +Indexed by [Repowise](https://repowise.dev). Last indexed: 2026-09-07 (commit c7173cf). Confidence: 100%. ### How to work in this repo - **Trust the index.** `verified: true` and `_meta.complete` mean the bytes were checked against the live tree, so never re-read them. Re-read only what `bounds: "approximate"` or `_meta.stale_warning` names. `confidence` rates the prose, not the evidence: on `low` read the `fallback_targets` or `best_guesses` the reply names, and run `repowise update` and ask again if `_meta.hint` says the index is behind HEAD. `index_behind: true` alone is informational. @@ -70,7 +70,7 @@ Covers the 3 source files in 2 directories under… - `webapp/src/components/Docs.tsx` — 1 bug fix, last fix today; 17 commits/90d ### Code health -Three co-equal signals: defect risk 5.95/10 avg, hotspot health 4.94/10 (stable), worst `src/engine/snapshot/mod.rs` at 2.0/10 · maintainability 7.48/10 · performance risk 16 open static I/O-in-loop / N+1 findings. Detail: `get_health()`. +Three co-equal signals: defect risk 5.95/10 avg, hotspot health 4.94/10 (declining), worst `src/engine/snapshot/mod.rs` at 2.0/10 · maintainability 7.48/10 · performance risk 16 open static I/O-in-loop / N+1 findings. Detail: `get_health()`. Critical files: - `webapp/src/components/LogPreview.tsx` — untested hotspot — impact −2.0 diff --git a/CLAUDE.md b/CLAUDE.md index 6d77ce7..4b85505 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,3 +43,5 @@ Frontend structure: `webapp/src/App.tsx` is the shell; `webapp/src/components/` - **`tickets/`** holds scoped, written-up work items (problem statement, pointers to exact call sites with `file.rs:line` references, and an explicit scope section) — check here for planned/in-flight work before starting something that might overlap. Tickets are removed from this directory once implemented (see recent commit history — e.g. "Remove ticket 047 (implemented: ...)"). - Declaration-related features (params, metrics) consistently need three coordinated changes: an `EngineMessage` variant + handler in `engine.rs`, a persistence method in `settings.rs`/`profile.rs`, and an `api.rs` route wiring them together (plus a webapp UI affordance). Grep for an existing similar declaration (e.g. how metric declaration is wired) as the template before adding a new one. - Doc comments in this codebase tend to explain *why* a non-obvious choice was made (e.g. ordering constraints, a workaround, a past ticket's rationale) rather than restating what the code does — match that style rather than adding descriptive comments. +- All markdown files (`*.md`) in this repo should follow one-sentence-per-line: each sentence starts on its own line, even within the same paragraph. + diff --git a/ideas/0001-actor-split.md b/ideas/0001-actor-split.md index ddc6119..6327111 100644 --- a/ideas/0001-actor-split.md +++ b/ideas/0001-actor-split.md @@ -26,6 +26,54 @@ > Values in the primary index should be arcs, to prevent cloning > allocations. +## Resolved decisions (2026-09-07) + +Clarified interactively; supersedes the open questions raised below where +they conflict. + +- **No profiling gate.** Proceeding on the code-reading evidence already in + this doc (double/triple string clones per line, full-`EngineSnapshot` + deep-clone on every `left_right` reconciliation) rather than waiting on a + `samply` profile first. +- **Three actors, not two.** The split is: **reader actor** (tails + stdin/file, splits into lines, timestamps, wraps each line in an `Arc`, + sends downstream) → **primary actor** (ingests timestamped `Arc` lines, + extracts declared + auto params only, maintains the `param=value` index — + no metrics, no profile logic) → **profile actor** (holds its own + `left_right` reader over the primary's index; builds metrics, custom + params, timeseries, and PromQL evaluation for the one active profile). + This resolves the "per-profile message format" question below: there is + no per-profile parsing fork on the primary — format/shape parsing of an + already-classified line is entirely the profile actor's concern, on top + of the shared raw text. +- **Still exactly one active profile at a time.** No concurrent + multi-profile indexing; the split changes *how* the one active profile's + data is computed relative to the primary, not the user-facing + single-active-profile model. +- **Alerts move to the profile actor.** Alert rules are profile-scoped and + read metric buckets, so they move with metric computation rather than + staying on the primary. +- **Primary→profile signaling:** the primary actor notifies subscribers + (e.g. a `tokio::sync::watch` carrying the latest processed + position/index) right after each `left_right` sync completes. The + profile actor wakes on that signal and re-reads whatever's newly visible + through its own `left_right` reader — no busy-polling, no second copy of + the data in transit. +- **Eviction wins over profile-actor lag.** If the profile actor is + behind (e.g. mid-backfill on a newly declared metric) when the primary + evicts old lines (`SetMaxLogs`), the primary evicts on schedule + regardless — the profile actor silently misses data for the evicted + range. Simplest option, matches today's single-actor retention model. +- **System param suppressions: dropped from scope.** Not needed right now; + revisit as a separate idea if it becomes a real requirement. +- **Migration path: incremental**, in this order: + 1. Arc-ify stored values (`Arc`/`Arc` for log text and + metric-captured strings) — independent win, and the load-bearing + precondition for cheap cross-actor reads in steps 2–3. + 2. Extract metric computation into a separately-callable stage, still + within the single actor. + 3. Split into the reader/primary/profile actors described above. + ## Current architecture (confirmed in code) - **One process, one actor, today.** `spawn_engine` @@ -188,12 +236,16 @@ `left_right` pair. Splitting introduces: a defined "primary perspective" (raw logs + derived/auto params only, no profile-specific metrics) as its own bounded module, a - "profile perspective" abstraction that can be instantiated per - active profile, a notification/subscription mechanism from primary - to profile workers (the note's "profiles need to be informed of new - messages to index" — doesn't exist in any form today; the closest - analogue, `ingest.rs`'s producer tasks feeding one consumer, runs the - opposite direction), and a query-fan-out/merge layer for any request + "profile perspective" abstraction for the one active profile, a + notification/subscription mechanism from primary to the profile actor + (the note's "profiles need to be informed of new messages to index" — + doesn't exist in any form today; the closest analogue, `ingest.rs`'s + producer tasks feeding one consumer, runs the opposite direction). + **Resolved** (see "Resolved decisions"): the primary signals via e.g. a + `tokio::sync::watch` after each `left_right` sync completes, and the + profile actor re-reads through its own `left_right` reader rather than + the primary pushing data. A query-fan-out/merge layer is still needed + for any request that needs both raw logs and profile-derived data in one response (today `Logs`/`Timeline` queries read both from a single `EngineSnapshot` in one pass, e.g. `query.rs:154`'s @@ -202,10 +254,9 @@ across two snapshots/workers instead of one borrow). - Alert evaluation (`handle.rs:199-211`) currently depends on running immediately after metric bucketing in the same message, against the - same snapshot. If metrics move to per-profile workers, alerts need to - either move with them (probably correct, since alert rules are - profile-scoped conceptually) or gain a defined way to read - cross-worker metric state that doesn't exist today. + same snapshot. **Resolved** (see "Resolved decisions"): alerts move + with metric computation into the profile actor, since alert rules are + profile-scoped and read the metric buckets computed there. - `EngineHandle`'s current API (`handle.rs:49-73` for reads, the mpsc `EngineMessage` send for writes) would need to become two APIs, or one handle type parameterized by "primary" vs. "profile" — @@ -222,75 +273,45 @@ ### Functionality -- **Per-profile message format** (the note's "What if message needs to - be different format? Thats per-profile.") — today, log-stream header - detection and logfmt/JSON auto-param extraction happen once, in the - primary, against the raw line (`derive_log_stream_params_from_row`, - `auto_declare_params`, both in the current `Ingest` arm). If - different profiles can expect genuinely different message shapes - from the *same* piped process (unclear from the note whether this - means "different profiles parse the same stream differently" or - "each profile only ever sees its own stream"), that's a real design - fork: either the primary's parsing must become profile-aware (which - undercuts "primary only extracts params" as a clean, profile-agnostic - boundary), or each profile worker does its own message-format - parsing independently on top of the shared raw text — the latter fits - the stated architecture better but means format detection isn't a - primary-side concern at all, just param extraction of an - already-classified line is. -- **System param suppressions** (the note's fragment) is undeveloped — - unclear whether this means "some auto-derived params should never be - promoted to profile-visible params" (a filtering rule between primary - and profile perspectives) or "a way to suppress a param a profile - inherited but doesn't want" (a per-profile override). This needs a - sentence or two from you before it can be scoped at all; flagging - here rather than guessing. -- Multiple simultaneously-active profile workers (implied by "a - different worker" being per-profile, plural) raises a question the - current single-profile-at-a-time model never had to answer: can two - profiles be "active" and indexing concurrently, or is it still - strictly one active profile with the split only changing *how* that - one profile's data is computed relative to the primary? The - perf/functionality tradeoffs are very different depending on the - answer (concurrent multi-profile indexing multiplies the backfill - and per-line cost discussed above by the number of concurrently - active profiles). +- **Per-profile message format** — **resolved**, see "Resolved decisions" + above: there's no per-profile parsing fork on the primary. Log-stream + header detection and auto-param extraction stay a one-time, primary-side, + profile-agnostic concern (`derive_log_stream_params_from_row`, + `auto_declare_params`); any further message-shape interpretation the + profile actor needs happens independently, on top of the shared raw + `Arc` text, in the profile actor itself. +- **System param suppressions** — **dropped from scope** (see "Resolved + decisions"). Revisit as a separate idea if it becomes a real need. +- **Concurrency** — **resolved**: still strictly one active profile at a + time. The split changes *how* that one profile's data is computed + relative to the primary, not the user-facing model — no multi-profile + concurrent indexing. ### Not yet considered -- **Failure/restart semantics**: if a profile worker crashes or lags - behind the primary, what's the recovery story? Today there's one - actor with one failure mode (the whole engine). A split needs an - answer for "profile worker is behind — does it catch up from the - primary's retained history (bounded by the same retention limit the - primary enforces), or is data permanently lost for the gap?" -- **Retention/eviction coordination**: the primary already evicts old - logs (`evict_excess_logs`, `handle.rs:195`, `SetMaxLogs` message). - If a profile worker is reading "the primary log sequence" lazily or - asynchronously, eviction on the primary could remove a line before a - lagging profile worker has processed it — there's no coordination - primitive today (like a per-consumer low-water-mark) to prevent this - silently dropping data for a slow profile worker. +- **Failure/restart semantics** — partially resolved: the profile actor + falling behind the primary during a backfill is expected and accepted + (see "Resolved decisions" — eviction wins over lag, silent gaps for the + evicted range). Still open: what happens if the profile actor *crashes* + outright (not just lags) — does it restart and replay from whatever the + primary currently retains, or is there a persisted checkpoint? Not yet + decided; likely fine to punt until the split is actually implemented, + since "restart and rebuild from currently-retained history" is a + reasonable default consistent with the accepted-gaps decision above. +- **Retention/eviction coordination** — **resolved**, see "Resolved + decisions": no low-water-mark. The primary evicts on its own schedule + (`evict_excess_logs`, `handle.rs:195`, `SetMaxLogs`) regardless of the + profile actor's progress; a lagging profile actor silently misses data + for the evicted range rather than blocking primary eviction. - **Testing story**: `src/engine/tests.rs` (2348 lines) and the `test_handle` pattern (see ticket 099) are built entirely around one actor with one channel — a split roughly doubles the kinds of integration tests needed (primary alone, profile worker alone, primary+profile interaction) and the existing test harness would - need real rework, not just extension. -- **Migration path**: this is a rewrite of the highest-risk, largest, - most-churned file in the codebase (per `CLAUDE.md`'s repowise - summary) — no plan yet for whether this ships as one big change or - incrementally (e.g. Arc-ing values first as an independent, - lower-risk step; then extracting metric computation into a - separately-callable-but-still-single-actor stage; then finally - splitting into separate tasks) — given the size, an incremental path - is very likely necessary rather than optional. -- **Whether this is worth it at all without profiling evidence**: per - this project's own established norm (tickets 079/080 were removed - for "no profiling evidence to justify the fix"), this is a large - structural bet motivated by the note's stated concern ("primary - worker is doing too much") rather than a measured bottleneck — worth - a `samply` profile of realistic ingest load *before* committing to - this, to confirm metric-bucketing-on-the-primary is actually a - measurable cost relative to param extraction, storage, and alert - evaluation, rather than assuming it from code-reading alone. + need real rework, not just extension. Still open — no resolution yet. +- **Migration path** — **resolved**, see "Resolved decisions": incremental, + in three steps (Arc-ify stored values → extract metric computation into + a still-single-actor stage → split into reader/primary/profile actors). +- **Whether this is worth it at all without profiling evidence** — + **resolved**, see "Resolved decisions": proceeding without a `samply` + profile, on the code-reading evidence already documented in this file.