bvandeusenandClaude Opus 5
8826be7a91
CI & Build / Python lint (push) Failing after 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 30s
CI & Build / TypeScript typecheck (push) Successful in 33s
CI & Build / Python tests (push) Successful in 1m5s
CI & Build / Build & push image (push) Skipped
Milestone 333 step 1. The write-path standing-rule arm is the only retrieval surface in Scribe whose usefulness cannot be observed — and, not coincidentally, the only one that has never declined to fire. 296 calls, zero zero-result, 100% clearing its threshold, while every other surface declines most of the time (#3311, and re-measured in note #3430). `retrieval_logs` gives it scores; scores say what the ranker thought, never whether the hint landed. WHY A SIBLING TABLE AND NOT A COLUMN ON note_usage_events. The row carries no note-specific field and the readout is the same shape, which is the strongest case for sharing that note #3163 admits. What decides against it is identity at RESTORE: the note importer maps note_id through note_id_map, so a rule id parked in that column comes back attached to whatever note holds that number in the target database. Not dropped — reattached. The restore reports success, the counters are populated, and every one is about the wrong record, with no other field to disagree with. rule_versions made the same call for the same reason; this is the third rule-side sibling and it reads like the first two. FK-free on rule_id and user_id, matching note_usage_events / retrieval_logs / app_logs, and deliberately unlike rule_versions. A version belongs to a rule's history and dies with it; telemetry outlives what it describes. Deleting a rule must not erase the evidence that it was surfaced forty times and opened never, because that evidence is the case for having deleted it. The service uses `background.spawn` rather than a third copy of the strong-reference dance — that module's own docstring says new callers should, and a fourth copy is how one of them drifts. The AppLog canary #2663 demands is kept, and since `rule_usage` needed exactly `note_usage`'s semantics, that canary moved into `background.report_telemetry_failure` and note_usage now calls it. `retrieval_telemetry` deliberately keeps its own: its canary is a different shape (one process-wide flag, no AppLog row), so repointing it would change behaviour rather than consolidate it. No ambient bucket, and that is a decision. The note twin splits ranked from ambient surfacings because enter_project and the skill sync deliver records without choosing them (#2477). Rules have the same problem waiting — list_always_on_rules loads them wholesale — but nothing emits here yet, so an empty AMBIENT_SOURCES would be machinery pretending to a distinction the data does not contain. `source` stays granular, so the split stays a readout-level change needing no migration. Backup carries it (v14). The round-trip test seeds a NOTE alongside the rule so the target database has a note id to collide with — without that decoy, a restore running rule ids through the wrong map would merely drop them and the test would pass by absence, rather than failing on the populated-and-wrong result that is the actual hazard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcCs1CcQ1ormdnzSshKqvN
115 lines
4.5 KiB
Python
115 lines
4.5 KiB
Python
"""Fire-and-forget background tasks that actually run.
|
|
|
|
The event loop holds only a WEAK reference to a task, so a bare
|
|
``create_task`` with no other holder can be garbage-collected mid-flight — a
|
|
write that never errors and never lands (the #2663 GC footgun). This module is
|
|
the one place that gets the pattern right: strong references in ``_pending``,
|
|
discarded on completion, with failures logged at WARNING instead of vanishing.
|
|
|
|
``retrieval_telemetry`` predates this module and keeps its own copy, because
|
|
its canary is a genuinely different shape — one process-wide flag and no
|
|
AppLog row. ``note_usage`` and ``rule_usage`` share ``report_telemetry_failure``
|
|
below. New fire-and-forget callers use ``spawn`` rather than writing another
|
|
copy of the strong-reference dance.
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import asyncio
|
|
import logging
|
|
import traceback
|
|
from collections.abc import Coroutine
|
|
|
|
logger = logging.getLogger(__name__)
|
|
|
|
_pending: set[asyncio.Task] = set()
|
|
|
|
# Sites that have already dropped their once-per-process AppLog row, keyed
|
|
# "<subsystem>:<site>". A readout can run on every list render — without this,
|
|
# a broken table turns the error log into a firehose that buries the finding it
|
|
# exists to surface.
|
|
_reported: set[str] = set()
|
|
|
|
|
|
async def report_telemetry_failure(subsystem: str, site: str) -> None:
|
|
"""Make a swallowed telemetry failure visible. Call from an except block.
|
|
|
|
WARNING to the process log every time; one AppLog error row per process per
|
|
(subsystem, site) so the admin UI shows the outage without host access.
|
|
|
|
THIS IS NOT DECORATION. #2663 is the record of a telemetry subsystem running
|
|
at zero for weeks — every counter reading empty, indistinguishable from
|
|
"nobody uses this" — because every failure went to ``logger.debug``. A
|
|
subsystem whose failures are all invisible cannot report its own death.
|
|
|
|
The AppLog write is itself guarded: when the database is down it fails too,
|
|
and that is fine. The WARNING already said so, and a canary must never take
|
|
down the surface it watches.
|
|
"""
|
|
logger.warning("%s telemetry %s failed", subsystem, site, exc_info=True)
|
|
key = f"{subsystem}:{site}"
|
|
if key in _reported:
|
|
return
|
|
_reported.add(key)
|
|
try:
|
|
from scribe.services.logging import log_error
|
|
|
|
await log_error(
|
|
endpoint=subsystem,
|
|
error_type=f"{subsystem}_{site}_failed",
|
|
error_message=f"{subsystem} telemetry {site} is failing; "
|
|
"usage counters will read zero until this is fixed",
|
|
traceback=traceback.format_exc(),
|
|
)
|
|
except Exception:
|
|
logger.debug("%s canary write failed", subsystem, exc_info=True)
|
|
|
|
|
|
def spawn(coro: Coroutine, *, site: str) -> None:
|
|
"""Schedule ``coro`` fire-and-forget; ``site`` names it in failure logs.
|
|
|
|
No running loop (sync context outside the app) closes the coroutine and
|
|
skips — every app path runs on the loop, and blocking would be worse.
|
|
"""
|
|
try:
|
|
task = asyncio.get_running_loop().create_task(coro)
|
|
except RuntimeError:
|
|
coro.close()
|
|
logger.debug("background task %s skipped — no running event loop", site)
|
|
return
|
|
_pending.add(task)
|
|
|
|
def _done(t: asyncio.Task) -> None:
|
|
_pending.discard(t)
|
|
if not t.cancelled() and t.exception() is not None:
|
|
logger.warning(
|
|
"background task %s failed", site, exc_info=t.exception()
|
|
)
|
|
|
|
task.add_done_callback(_done)
|
|
|
|
|
|
def start_periodic(interval_s: float, work, *, label: str) -> asyncio.Task:
|
|
"""A forever loop that sleeps ``interval_s`` then awaits ``work()``, logging
|
|
(never raising) when a tick fails — the one shape the hourly/daily
|
|
retention sweeps share (log retention, notification sweep, auth-token
|
|
purge). Sleeps FIRST so startup isn't a sweep; holds a strong reference
|
|
like spawn() so the loop cannot be garbage-collected mid-flight."""
|
|
async def _loop() -> None:
|
|
while True:
|
|
await asyncio.sleep(interval_s)
|
|
try:
|
|
await work()
|
|
except Exception:
|
|
logger.exception("periodic task %s failed", label)
|
|
|
|
task = asyncio.get_running_loop().create_task(_loop(), name=f"periodic-{label}")
|
|
_pending.add(task)
|
|
task.add_done_callback(_pending.discard)
|
|
return task
|
|
|
|
|
|
async def drain() -> None:
|
|
"""Await everything in flight — for tests that need the writes landed."""
|
|
while _pending:
|
|
await asyncio.gather(*list(_pending), return_exceptions=True)
|