feat(buzz): add reaction lifecycle and presence publishing - #99451
KostaGorod wants to merge 7 commits into
Conversation
Signed-off-by: Kosta Gorod <35299380+KostaGorod@users.noreply.github.com>
Buzz presence records expire unless republished — bots with live websockets still showed offline because no code path ever published presence (round-trip proven: manual 'buzz users set-presence online' made the record appear; it vanished within ~13 min without a refresh). connect() now publishes online and refreshes it every 60s; disconnect() cancels the refresh first, then publishes offline with a 5s cap so a wedged buzz-cli can never stall shutdown. All presence calls are best-effort and fail quiet. Signed-off-by: Kosta Gorod <35299380+KostaGorod@users.noreply.github.com>
The reactions toggle lives in config.yaml at gateway.platforms.buzz.extra.reactions and defaults to true, per the repo rule that behavioral settings are config.yaml-only. The website docs already cover this setting.
33f29e2 to
f6b770b
Compare
The lifecycle coordinator is well-engineered: transitions are serialized per message behind a tail-task chain, replacement is remove-gated so reactions never stack on the relay (plugins/platforms/buzz/adapter.py:1238-1266), terminal cleanup pops state in a Non-blocking: Verdict: LGTM |
Expire abandoned lifecycle state with one shared sweeper, preserve active transitions, and document the cleanup contract. Clamp presence refreshes to a conservative relay-expiry margin and simplify best-effort shutdown handling.
Verdict: PASS at Gate: worktree clean; A — Abandoned lifecycle state ( B — Redundant tuple ( C — Presence margin ( Independent verification run by the reviewer at this exact head:
CI (exact head, reported separately — external gate, not a code verdict): runs 33552516713 (CI), 33552515542 (Docker), 33552515545 (Nix) are terminal Non-blocking note: there is a one-event-loop-slice window where a TTL-expired terminal tail cancelled by the sweeper pops a key that a duplicate |
…cket The CLI-based presence loop (`buzz-cli users set-presence`) can never keep Hermes agents visible in Buzz: each invocation runs the CLI as a fresh process, publishes a kind-20001 event, and closes its socket — and the relay drops the presence lease with the socket. A client that subscribes after that sees no presence records at all (verified live against the relay: read-only REQ for all five profile pubkeys returned ABSENT across three rounds 70s apart, while a connected subscriber saw only the single connect-time publish). Presence now rides the adapter's own long-lived authenticated WebSocket: a signed kind-20001 `online` event published inline right after the NIP-42 handshake — before the connection is reported ready — then refreshed by a heartbeat task at the expiry-aware cadence (every publish lands at least the configured margin before the relay's 180s TTL lapses). `offline` is published only on graceful shutdown, on the still-open socket, before the WebSocket task is cancelled; a transient disconnect never flaps offline because the relay clears the record itself and the reconnect's first publish restores it. Signing runs in the default executor (pure-Python schnorr is CPU-bound, ~50ms/event) and every frame shares a send lock so the inbound pump is never stalled. Two shutdown hazards surfaced while hardening this and are fixed here as well: - Cancelling during the initial publish (or inside `wait_for` on the NIP-42 handshake) can be silently consumed on Python 3.11 and the loop resumes with a pending cancel that is never observed as terminal — shutdown then wedges with orphan heartbeat/discovery tasks. The loop now re-arms an observed-but-consumed cancellation before spawning any child task, and the reconnect loop's generic `except Exception` re-raises when the task is cancelling. - The read-loop `finally` retired the heartbeat but left a window where a duplicate heartbeat task could run per connection; the reconnect test now pins exactly one heartbeat per connection and zero after the loop exits (the duplicate-heartbeat red test at the PR base is the first failing test). Kind-20001 events are rejected by the relay's HTTP bridge, so presence publishing cannot regress to the CLI path; the adapter tests pin that presence never spawns a CLI process. TDD: all new behavior covered by failing tests first (8 websocket + 4 adapter tests red at 96dd89a, green after the fix). Live verification: read-only relay probes recorded on kanban card t_5eabc763; post-deploy observation pending user confirmation.
|
Presence root cause + fix — Found: the CLI presence loop ( Fixed:
Verification: 12 new tests (8 websocket + 4 adapter) written red-first at |
KostaGorod
left a comment
There was a problem hiding this comment.
Frozen-SHA independent review — PASS
Head: ce59b6091652a23d96400ed54f8b156e5c10b28a · Base: 375ce8eee51b9d76714cb6fd1f200c4c9ef83c4a · Reviewed by the reviewer agent profile (read-only on this tree; verdict via COMMENT because the acting account is the PR author).
Freshness gates — all pass
- Workspace clean (
git status --porcelainempty);HEAD=origin/buzz-reaction-lifecycle-upstream= live PR head =ce59b609; merge-base with the recorded base is the base itself. - Live PR files = local diff inventory: 6 files (+1724/−19); the fix commit alone touches 5 (+600/−86), scoped to presence + its tests/docs. Reaction-lifecycle behavior unchanged by the fix commit.
Independently re-verified at this head
scripts/run_tests.sh tests/gateway/test_buzz_websocket.py tests/gateway/test_buzz_adapter.py tests/gateway/test_buzz_reaction_lifecycle.py→ 254/254 passed; six sibling Buzz suites → 69/69 passed. Ruff clean on all five changed Python files,py_compileOK,git diff --checkclean.- RED receipts re-checked (
/tmp/red_base_result.txt,/tmp/red_reconnect_result.txt): at96dd89ac+ new tests,build_presence_eventAttributeError and the duplicate-heartbeat guard failure ("first connection never heartbeated") fail for the expected mechanism reason — TDD discipline confirmed. - Live relay probe re-run (read-only subscriber, 140 s): all five profiles published recurring same-socket
onlineheartbeats at ~60 s cadence (2 events each across two windows) —PROBE_PASS, independently reproducing the deployed-tree evidence.
Code findings on the presence lifecycle and shared-socket boundaries
- Same-socket guarantee: presence publishes only via
_send_wson the connection stored in_ws_connection; every frame on the shared socket (subscription REQs, presence, discovery) goes through the per-adapter send lock. The one remaining rawsendis the NIP-42AUTHframe, sent strictly before any concurrent task exists on that connection. Nousers set-presenceCLI path remains (pinned by tests). - Heartbeat lifecycle: first
onlinepublishes inline before_ws_ready.set(); heartbeat starts only after successful auth; reconnect'sfinallycancels and settles the old heartbeat before the old socket exits and arms exactly one for the new connection (test_presence_heartbeat_reconnect_replaces_task_exactly_oncedrives a real forced-close reconnect and asserts retired-heartbeat quiescence). Transient drops deliberately do not publishoffline— the relay clears the lease with the socket; correct anti-flap design. - Orderly teardown:
disconnect()stops the heartbeat and publishes bounded best-effortofflineon the still-open socket before cancelling the WS task (test_graceful_offline_publish_before_socket_closeasserts offline-before-__aexit__). - Cancellation hardening: the Python-3.11 consumed-cancellation re-arm (
loop_task.cancelling()→ raise) before spawning child tasks, and theexcept Exceptionre-raise when cancelling, are both real hazards with real tests (test_cancel_during_first_presence_does_not_leak_heartbeatalso asserts zero stray asyncio tasks).wait_forauth-timeout path cancels and reaps the loop. - Event shape:
build_presence_eventfollows NIP-01 serialization exactly (kind 20001,["status", s]tag, bare-status content, Schnorr sig over the id); signing off-loop in the default executor.
Non-blocking notes
- With
transport="poll"pinned, the adapter no longer publishes presence at all (the old CLI loop covered both transports). Justified: the CLI path is the defect being removed (its socket-close drops the lease), and the WS transport is the default. Not documented explicitly, but the poll fallback is already described as degraded. _stop_presenceclears_ws_connectionbefore the offline publish; a transient failure there leavesNone— consistent with best-effort semantics, no correctness impact.
Residuals (disclosed, not blockers for this verdict)
- Upstream main advanced past the recorded base; the PR is currently
mergeStateStatus: DIRTY(media-cache refactor touchedadapter.py— incidental overlap, no competing presence implementation). Reconciliation + fresh verification is follow-up work after this verdict; it does not affect the frozen-SHA mechanics reviewed here. - Upstream CI for this PR is
action_required(first-time contributor approval); same-diff fork run at the identical head: Docker success, CI 14/17 green — the 3 remaining jobs + nix target larger-runner labels (ubuntu-latest-96-core,ubuntu-latest-32-core,windows-latest-32-core) the fork cannot execute, verified against the workflow definitions at this head. Local exact-head suites stand in for those lanes.
Resolve adapter.py import conflict: keep our ProcessingOutcome (reaction lifecycle) alongside upstream's cache_media_bytes_async from the media-cache offload sweep (568b161) — its call sites already migrated to the async seam, so the sync cache_media_bytes import is dropped. Presence fix (ce59b60) invariants preserved: same-socket kind-20001 publishing via _send_ws send-lock, executor-signed events, single heartbeat-per-connection with reconnect replace, offline-before-close.
|
Found→Fixed→Verified @ d344c58 Found: upstream main advanced 375ce8e → 6327930; the media-cache offload sweep (568b161) collided with this PR's adapter.py import block → PR was Fixed: normal merge commit of upstream/main into the published branch (fast-forward push ce59b60..d344c58, no force, history preserved). Sole conflict = import block: kept our ProcessingOutcome (reaction lifecycle) alongside upstream's cache_media_bytes_async — the sweep had already migrated all call sites, so the sync cache_media_bytes import is dropped. Presence fix invariants intact: same-socket kind-20001 via _send_ws send-lock, executor-signed events, single heartbeat per connection with reconnect replace, offline-before-close. Diff vs upstream/main is purely additive. Verified: all 8 buzz suites via scripts/run_tests.sh — 309/309 passed, 0 failed (websocket 26, adapter 191, reaction-lifecycle 37, authz 11, forum-kinds 6, mention-resolution 19, progress-thread-routing 2, thread-topology 17). Ruff clean, py_compile OK, git diff --check clean, git merge-tree upstream/main HEAD clean. GitHub: mergeable MERGEABLE (no conflicts). Supersession scan of 375ce8e..6327930 found no competing presence work (upstream has no kind-20001 code; only the media-cache sweep + an off-loop read tweak touched buzz). Fresh exact-head review at d344c58 pending — prior PASS is commit-bound to ce59b60. |
KostaGorod
left a comment
There was a problem hiding this comment.
Fresh independent exact-head review at d344c5865111a4606c8661b6ff96a30ef4046bae (COMMENT-form: GitHub blocks verdict reviews on own PRs). Prior PASS (pullrequestreview-5098189286) was commit-bound to ce59b60916 and is superseded.
Verdict: REVIEW_PASS
1. Merge-delta review (ce59b60916..d344c58651)
- Merge shape verified: parents
ce59b60916+63279301bc, merge-base375ce8eee5; no history rewritten. - Only buzz file touched:
plugins/platforms/buzz/adapter.py, and the combined diff (--cc) shows the single conflicted hunk was the import block — resolved as claimed: ours'ProcessingOutcome+ upstream'scache_media_bytes_async(568b161); synccache_media_bytesimport dropped. - Zero remaining sync
cache_media_bytesreferences anywhere underplugins/platforms/buzz/; all call sites async (adapter.py:2823, 3343). - Strongest check — PR diff vs upstream main is byte-identical pre-merge (
375ce8eee5..ce59b60916) vs post-merge (63279301bc..d344c58651) modulo the one import line, blob-index lines, and hunk-offset shifts. The merge introduced no hidden semantic change to the PR payload. git merge-tree upstream/main HEADclean (exit 0, no conflicts): future merge is trivial.
2. Presence invariants survived the merge (spot-checked live at this head)
- Same-socket kind-20001 via
_send_wssend-lock: adapter.py:1032–1050; presence and subscription frames serialize on_ws_send_lock. - Executor-signed events:
build_presence_eventruns viarun_in_executorin_publish_presence— loop never stalled by the CPU-bound Schnorr sign. - Single heartbeat per connection with reconnect replace: spawned once at adapter.py:2317 after the inline first publish; reaped in the per-connection
finally(2408–2413); graceful path via_stop_presenceindisconnect(). - Offline-before-close:
disconnect()awaits_stop_presence()before_ws_task.cancel()(adapter.py:1246–1254); transient reconnect drops deliberately do not flap offline. - NIP-01 event shape: serialization array
[0, pubkey, created_at, 20001, tags, content],id= sha256 of serialized,sig= 64-byte Schnorr hex (nostr_auth.py:186–225). Correct.
3. No semantic drift from upstream's media-cache sweep
- Upstream's adapter change is confined to the media-cache seam (async migration + off-loop
read_bytesviaasyncio.to_thread, e056777/568b16122d). Presence/reaction code paths reference neither — no interaction surface.
Independent re-verification at this exact head (not trusted from the implementer)
- All 8 buzz suites via
scripts/run_tests.sh: 309/309 passed — websocket 26, adapter 191, reaction-lifecycle 37, authz 11, forum-kinds 6, mention-resolution 19, progress-thread-routing 2, thread-topology 17 (per-file counts match the claim exactly). - Ruff clean on changed buzz files;
py_compileOK;git diff --checkon the PR delta vs upstream clean (two EOF blank-line warnings in photon/CLI tests are inherited from pure upstream375ce8eee5..63279301bc, not introduced here). - GitHub state at review time: head =
d344c58651, OPEN, MERGEABLE. No check runs exist at this head — consistent with the stated fork-CI ceiling (14/17 + Docker); local gates are the verification evidence per the PR constraints.
Follow-up hardening
Exact head
d344c5865111a4606c8661b6ff96a30ef4046baeresolves the presence-offline symptom reported during live dogfooding, plus all three non-blocking notes from the automated review:Summary
onlinekind-20001 event immediately after authentication, refreshed on a 60 s heartbeat (well inside the relay's 180 s presence lease), and a best-effortofflineon graceful shutdown before the socket closes.gateway.platforms.buzz.extra.reactions(defaulttrue), plus behavioral tests and user documentation (website/docs/user-guide/messaging/buzz.md).Root cause of the offline-agent symptom
Buzz presence is a relay-side lease. Two bugs combined to make connected agents appear offline:
buzz users set-presence).buzz users set-presenceinvocation opens its own WebSocket, publishes one event, and closes. The relay deregisters the identity when that socket closes, so the freshly-published lease was dropped again moments later. A live relay probe showed a fresh subscriber receiving zero presence records for all five multiplexed profiles — the Desktop's exact "all agents offline" view — while the gateway itself stayed healthy.The fix publishes kind-20001
onlineheartbeats on the same long-lived authenticated socket that carries the inbound subscription. The relay keeps the lease alive for as long as that socket stays registered; the 60 s heartbeat only refreshes it. Presence events are built and Schnorr-signed in-process (plugins/platforms/buzz/nostr_auth.py, dependency-free, same event shape the Buzz SDK emits), sent through a per-connection send lock shared with subscription traffic, and re-armed as exactly one heartbeat task per reconnect.Live relay evidence (old vs fixed)
Read-only probes against the local relay; the AUTH event is client authentication, nothing is published by the probe.
Old code (installed tree
8e4fad97, CLI loop):Fixed code (deployed tree
4bb6c0b8, byte-identical patch, 140 s live fan-out):Recurring same-socket heartbeats at ~60 s cadence, each well inside the 180 s lease.
Verification
RED at the pre-fix head (
96dd89ac+ new tests only): 12 failed / 205 passed across the Buzz websocket/adapter suites — including the duplicate-heartbeat guardtest_presence_heartbeat_reconnect_replaces_task_exactly_once("first connection never heartbeated") and the event-builder contract (build_presence_event). GREEN atce59b609, re-verified GREEN at the reconciled headd344c5865111a4606c8661b6ff96a30ef4046bae(merge of upstreammain@63279301bc, see Reconciliation below):Exact-head test evidence (d344c58)
All via
scripts/run_tests.sh(never direct pytest):tests/gateway/test_buzz_websocket.py+tests/gateway/test_buzz_adapter.py— 217/217 passed (same-socket presence, send-lock serialization, single-heartbeat-per-connection, offline-before-close, no_run_clipresence path, event shape/signature).tests/gateway/test_buzz_reaction_lifecycle.py— 37 passed;test_buzz_authz.py— 11;test_buzz_forum_kinds.py— 6;test_buzz_mention_resolution.py— 19;test_buzz_progress_thread_routing.py— 2;test_buzz_thread_topology.py— 17 → 92/92 passed across all eight Buzz test files (309 tests total).py_compileOK;git diff --checkclean.CI evidence (frozen head ce59b60 — superseded by d344c58, pending re-run)
Upstream fork-PR workflows for this PR are gated behind first-time-contributor workflow approval (
action_required), so exact-head CI was executed as a same-diff run on the fork: KostaGorod/hermes-agent PR #3 (headce59b6091652a23d96400ed54f8b156e5c10b28a, base375ce8eee51b9d76714cb6fd1f200c4c9ef83c4a— identical base/head to this PR).Python tests / Run tests,Windows-only tests,Playwright E2E (Linux)) plus the queued nix job target larger-runner labels (ubuntu-latest-96-core,ubuntu-latest-32-core,windows-latest-32-core) that the fork cannot execute — they will never start there. Verified against the workflow definitions at this head. Local exact-head suites (254/254 + 69/69 viascripts/run_tests.sh) stand in for the full-suite lane.Open gaps
Upstream main advanced past the recorded base; this PR wasResolved atmergeStateStatus: DIRTYagainst itd344c58651— see Reconciliation below. GitHub now reportsmergeable: MERGEABLE.d344c58651is still needed (the prior PASS is commit-bound toce59b609).mainand does not depend on that change.Reconciliation (2026-09-03)
Upstream
mainadvanced375ce8eee5 → 63279301bc; the media-cache offload sweep (568b16122d) migrated buzz's media caching to the async seam (cache_media_bytes_async), colliding with this PR's import block inplugins/platforms/buzz/adapter.py. Reconciled without rewriting published history via a normal merge commit:d344c58651= merge ofupstream/main(63279301bc) into the published branch (fast-forward pushce59b60916..d344c58651, no force).ProcessingOutcome(reaction lifecycle) alongside upstream'scache_media_bytes_async; the synccache_media_bytesimport is dropped because the sweep already migrated all call sites (adapter.py:2827/3347 useawait cache_media_bytes_async(...)). Diff vs upstream/main is purely additive (presence + reactions); upstream'sreaction_onlypath and profile-scoped config reads are untouched._send_wssend-lock, executor-signed events, single heartbeat per connection with reconnect replace, offline-before-close, NIP-01-correct event shape.py_compile+git diff --checkgreen atd344c58651(receipts above).git merge-tree upstream/main HEADconfirms a clean merge.375ce8eee5..63279301bc(supersession scan: only the media-cache sweep and an off-loop read tweak touched buzz; upstream has no kind-20001/presence code).action_requireduntil maintainer approval; local exact-head suites stand in.History
ce59b60916d344c5865163279301bc— media-cache conflict resolved; fresh verification aboveCloses #99611
Comparison with related implementations
Reaction lifecycle
For the reaction-lifecycle behavior in #99611, this implementation is more complete and concurrency-correct than the related open alternatives:
(chat_id, message_id)message_idperchat_id; later input overwrites earlier stateextra.reactionsThe important correctness difference is message identity. #97610 stores one pending 👀 target per chat, so two messages arriving in the same channel before a reply can overwrite the first target and strand or remove the wrong reaction. This PR maintains an independent state machine per inbound message and completes it from the gateway's actual
ProcessingOutcome, rather than treating an outbound send as proof of successful processing.This comparison is scoped to reactions. #97610 also proposes typing indicators and NIP-AO observer telemetry, which are separate capabilities and are not superseded by this PR.
Presence
The presence half keeps a deliberately narrow shape: one dependency-free event builder + signer (
nostr_auth.py), one send lock per connection, one heartbeat task per connection.offlineon the live socket before closure, bounded by a timeout.