From 6dab239eb2e8c2914591809f278fca6e226ec32c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 21 Oct 2026 20:27:19 -0400 Subject: [PATCH] docs(6205): note_usage comments state the reason, not the instance that taught it The silent-outage story (#2663), the ranked/ambient split (#2477), the transfer evidence (#3735) and the attach_usage consolidation (#4230) are stated as the property each guards, without the incident. Co-Authored-By: Claude Opus 5.5 --- src/scribe/services/note_usage.py | 55 ++++++++++++++----------------- 1 file changed, 24 insertions(+), 31 deletions(-) diff --git a/src/scribe/services/note_usage.py b/src/scribe/services/note_usage.py index 3416bd37..e04cc884 100644 --- a/src/scribe/services/note_usage.py +++ b/src/scribe/services/note_usage.py @@ -15,13 +15,12 @@ Design notes (mirrors retrieval_telemetry, for the same reasons): - Writes are fire-and-forget. `record_surfaced` / `record_pulled` extract plain ints synchronously and schedule the insert as a background task, so telemetry never adds latency to — or can break — the surface it observes. - - Failures degrade, but they must not degrade SILENTLY. The original version - swallowed everything into logger.debug, and the deployed instance ran with - every counter at zero for weeks while surfacing demonstrably fired — a - total outage indistinguishable from "nobody uses this" (#2663). A - subsystem whose every failure mode is invisible cannot report its own - death, so failures now log at WARNING and drop one AppLog error row per - process per site, where the admin UI shows it. + - Failures degrade, but they must not degrade SILENTLY. Swallowed into a + debug log, a failure leaves every counter at zero while surfacing fires — + a total outage indistinguishable from "nobody uses this". A subsystem + whose every failure mode is invisible cannot report its own death, so + failures log at WARNING and drop one AppLog error row per process per + site, where the admin UI shows it. - Reads (`usage_for_notes`) are NOT fire-and-forget — a readout the caller awaits, aggregated in one round-trip for a whole page of snippets rather than per row. @@ -151,7 +150,7 @@ def record_pulled( `project_id` is where the READER was — the caller's active project, never the record's own. Compared against the record's `project_id`, it answers whether this was opened somewhere other than where it was written, which - is the evidence that a record TRANSFERRED (milestone 385, #3735). + is the evidence that a record TRANSFERRED. Unlike a surfacing arm, a getter only knows what it was handed, so this stays optional and null is an ordinary answer meaning "not reported". A @@ -178,7 +177,7 @@ def record_pulled( # top-N-by-recency happen to be; the skill sync installs every Process the # operator can reach. Counting those alongside auto-inject would make a note's # surfaced_count dominated by "it was recently updated in a project you -# opened", and dead-weight detection would read that as popularity (#2477). +# opened", and dead-weight detection would read that as popularity. # They still matter — a pull that follows one must not float unattributed — so # they land in their own bucket rather than not landing at all. AMBIENT_SOURCES = ("enter_project", "process_skill_sync") @@ -191,14 +190,13 @@ def empty_usage() -> dict: reads as "never surfaced, never pulled" rather than as a missing key. `surfaced_count` is RANKED surfacings only — a scored surface chose this - record. `ambient_count` is the rest (see AMBIENT_SOURCES). The split is the - readout half of #2477: the "high surfaced, zero pulls → dead weight" - reading is only valid over surfacings that were choices. + record. `ambient_count` is the rest (see AMBIENT_SOURCES). The split exists + because the "high surfaced, zero pulls → dead weight" reading is only valid over surfacings that were choices. `surfaced_away_count` / `pulled_away_count` are the subsets that happened on a project other than the one the record was written on — the evidence - that a record TRANSFERRED, which is the claim the lesson kind rests on - (milestone 385, #3735). Counted only where both projects are known: an + that a record TRANSFERRED, which is the claim the lesson kind rests on. + Counted only where both projects are known: an event with no reader project, or a record with no project of its own, cannot speak to "away" and is left out rather than guessed. """ @@ -231,9 +229,8 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]: # distinct source. ONE labelled expression, grouped by its label — a second # case() instance in GROUP BY renders with its own expanding-IN bind names # under asyncpg, so the database sees two DIFFERENT expressions and rejects - # the query with a GroupingError. That rejection was swallowed, which is - # how every counter read zero in production while the writes were landing - # fine (#2663). + # the query with a GroupingError — and a swallowed rejection reads as + # every counter at zero while the writes land fine. ambient = case( (NoteUsageEvent.source.in_(AMBIENT_SOURCES), True), else_=False, @@ -262,7 +259,7 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]: # it needs the join to `notes` and the first must keep counting # events whose note has since been deleted (the table is FK-free # so that evidence outlives the row). Ranked surfacings only, for - # the same reason surfaced_count is (#2477). + # the same reason surfaced_count is. away_rows = ( await session.execute( select( @@ -284,7 +281,7 @@ async def usage_for_notes(note_ids: list[int]) -> dict[int, dict]: except Exception: # A telemetry readout must not be able to break the list it decorates — # but it must say it failed, or a broken readout is indistinguishable - # from a corpus nobody uses (#2663). + # from a corpus nobody uses. await _report_failure("readout") return out @@ -335,15 +332,12 @@ def _row_id(row: dict, key: str) -> int | None: async def attach_usage(rows: Sequence[dict], *, key: str = "id") -> None: - """Add `usage` to every row of a payload a door is about to return (#4230). + """Add `usage` to every row of a payload a door is about to return. - The one seam both doors and every record kind share. Before this, four call - sites carried their own copy of the same lines — two list routes and two - detail routes — and `/api/knowledge`, which is the list a person ACTUALLY - browses notes and lessons in, was about to become a fifth. That is how the - chip came to reach two record kinds out of four while a service named - `usage_for_notes` worked on all of them: each door read fine on its own, - and nobody was comparing them. + The one seam both doors and every record kind share — list routes, detail + routes and `/api/knowledge`, the list a person actually browses notes and + lessons in. A copy per call site lets the chip reach some record kinds and + not others while each door reads fine on its own. ONE AGGREGATE FOR THE WHOLE PAGE. `usage_for_notes` is a single GROUP BY over the id set; calling it per row would be N+1 by construction, which is @@ -360,11 +354,10 @@ async def attach_usage(rows: Sequence[dict], *, key: str = "id") -> None: reports it through `_report_failure("readout")` and returns the zero-filled map, so a broken readout degrades without breaking the list it decorates. Wrapping it again would swallow the REPORT along with the error, and a - silently-swallowed readout failure is exactly #2663 — every counter reading - zero in production for weeks while the writes landed fine. + silently-swallowed readout failure reads as every counter at zero while + the writes land fine. - Mutates in place and returns None, matching how the call sites already used - it: these rows are the payload, not a copy of it. + Mutates in place and returns None: these rows are the payload, not a copy of it. A detail payload is just a one-row list — `await attach_usage([data])` — so the single-record doors share this seam rather than keeping a second