Repository navigation
fix(relay): a relayed Discord interaction carries the text lane's chat and user labels #128797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
40322be
cd581c7
6aad417
0f3aa86
fa9259f
10b955a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,6 +148,11 @@ def __init__( | |
| self._auto_thread_waiters: Dict[str, asyncio.Event] = {} | ||
| # Bounded FIFO seen-set for inbound replay dedupe (insertion-ordered dict). | ||
| self._seen_inbound: Dict[str, None] = {} | ||
| # Discord chat labels the text lane carries (connector-resolved) and a forwarded interaction | ||
| # does not: (scope, chat) -> (chat_name, chat_topic). The second map is what the session | ||
| # store is known to hold, so a label costs a write only when it is new or changed. | ||
| self._discord_chat_labels: Dict[tuple, tuple] = {} | ||
| self._discord_labels_recorded: Dict[tuple, tuple] = {} | ||
| # Live cards: draft_key -> draft_id of the OPEN native stream. Armed by | ||
| # send_draft; consumed by send() to convert the turn-final into | ||
| # draft(final=true) instead of a duplicate post. Keyed by _draft_key (chat + | ||
|
|
@@ -881,16 +886,80 @@ async def _on_inbound(self, event) -> None: | |
| return | ||
| self._seen_inbound[dedupe_key] = None | ||
| self._evict_oldest(self._seen_inbound, self._SEEN_INBOUND_MAX) | ||
| self._capture_scope(event) | ||
| self._stamp_slack_session_thread(event) | ||
| # A structured prompt answer resolves its waiting primitive and is CONSUMED β | ||
| # never also dispatched as chat. | ||
| if await self._consume_prompt_response(event): | ||
| return | ||
| await self._localize_inbound_media(event) | ||
| try: | ||
| self._capture_scope(event) | ||
| await self._remember_discord_labels(event.source) | ||
| self._stamp_slack_session_thread(event) | ||
| # A structured prompt answer resolves its waiting primitive and is CONSUMED β | ||
| # never also dispatched as chat. | ||
| if await self._consume_prompt_response(event): | ||
| return | ||
| await self._localize_inbound_media(event) | ||
| except asyncio.CancelledError: | ||
| # Teardown cancels the reader mid-frame, before admission and before the ACK, so the | ||
| # connector replays this frame; the replay must be admitted, not dropped as seen. | ||
| if dedupe_key is not None: | ||
| self._seen_inbound.pop(dedupe_key, None) | ||
| raise | ||
| await self.handle_message(event) | ||
|
|
||
| _SEEN_INBOUND_MAX = 512 | ||
| _DISCORD_LABELS_MAX = 2048 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: 2048 per map is generous but cheap (a tuple of two short strings). The only behavioral cost of a miss is one extra re-render on the first interaction after eviction/restart, which is the same as today. No change requested. |
||
|
|
||
| async def _remember_discord_labels(self, source) -> None: | ||
| """Keep the text lane's Discord chat labels for the interaction lane: the pinned | ||
| session-context prompt renders them, so a slash turn without them re-rendered the cached | ||
| prefix and the next message rendered it back.""" | ||
| # A message carrying no labels is an observation too (a topic or name removed upstream): | ||
| # the interaction must render what the text lane last rendered, not an older label. | ||
| if getattr(source, "platform", None) != Platform.DISCORD or not source.chat_id: | ||
| return | ||
| key, labels = (str(source.scope_id or ""), str(source.chat_id)), (source.chat_name, source.chat_topic) | ||
| self._discord_chat_labels[key] = labels | ||
| self._evict_oldest(self._discord_chat_labels, self._DISCORD_LABELS_MAX) | ||
| store = getattr(self, "_session_store", None) | ||
| if store is None or self._discord_labels_recorded.get(key) == labels: | ||
| return | ||
| # Recorded when observed, so the interaction lane has them after a restart whatever order | ||
| # the chat's sessions were created or reset in. Awaited before dispatch (the reader delivers | ||
| # events one at a time, so records land in observation order) but off the loop: it is a | ||
| # disk write. | ||
| try: | ||
| recorded = await asyncio.to_thread(store.record_chat_labels, source) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P1] Keep this
|
||
| except Exception: | ||
| logger.debug("relay: Discord chat labels not recorded", exc_info=True) | ||
| recorded = False | ||
| if not recorded: | ||
| # Not written (store fault, or no database handle right now): leave the key out so the | ||
| # next message with these labels tries again. | ||
| self._discord_labels_recorded.pop(key, None) | ||
| return | ||
| self._discord_labels_recorded[key] = labels | ||
| self._evict_oldest(self._discord_labels_recorded, self._DISCORD_LABELS_MAX) | ||
|
|
||
| async def _discord_chat_labels_for(self, scope: str, chat_id: str) -> tuple: | ||
| """The text lane's last (chat_name, chat_topic) for a chat. After a restart (or eviction) the | ||
| map is empty until the text lane speaks again, so ask the session store for what that lane | ||
| last recorded; otherwise a first interaction re-renders the prompt the cache still holds.""" | ||
| key = (scope, chat_id) | ||
| if key in self._discord_chat_labels: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Invalidate positive and negative warm caches across peer adapters. Revalidating the existing peer-cache finding: with two adapters sharing a real SessionStore, warm the peer via _on_passthrough, then let the writer observe a new label. The peer keeps the old label, or None if it was warmed before any record existed. Both current-head and full-composition variants fail. Moving cold reads off the loop and making unavailable storage retryable does not invalidate an already populated local entry. Give label observations a shared generation/invalidation mechanism or revalidation policy; preserve both positive and negative controls. The body includes the runnable real-consumer check with inert transport delivery.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The check still needs two RelayAdapters writing labels into one SessionStore, and the gateway doesn't run that topology. Under multiplex, I'm leaving this unchanged in this PR. If you can show a supported configuration where a second writer exists (for example a native Discord adapter and Relay both recording into the same store), I'll reproduce it and add the invalidation with both controls. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] One RelayAdapter is enough to reproduce the generation splitThis new evidence supersedes my earlier withdrawal of the peer-cache finding in the 12:35 UTC current-head follow-up. I withdrew it there because the two-RelayAdapter fixture used a topology the gateway does not configure and, at that point, I had not established another supported writer/generation transition. The live-restore path below supplies exactly that missing reachability evidence. The topology reply is correct that normal multiplex startup does not intentionally create a second RelayAdapter. That is not the only supported way the backing record changes. This cache hit returns the local value without consulting the SessionStore or a store generation. Hermes explicitly supports live state.db restore: So the supported sequence is: observe A β snapshot β same adapter observes B β restore snapshot while the gateway stays live β durable That is the same cache-coherence failure the earlier positive/negative peer probe exposed; the supported live restore supplies the legal generation transition. Bind warm entries to a durable generation/restore epoch, invalidate/revalidate them, or explicitly define and test why live observations intentionally survive restore and how eviction preserves that policy. |
||
| return self._discord_chat_labels[key] | ||
| store = getattr(self, "_session_store", None) | ||
| try: | ||
| # Off the loop: the read takes the database's writer lock, which a write may be holding. | ||
| recorded = (await asyncio.to_thread(store.chat_labels, Platform.DISCORD, scope, chat_id) | ||
| if store is not None else None) | ||
| except Exception: | ||
| # Labels only keep the prompt cache warm; a store fault must not drop the interaction. | ||
| # Not cached either: the next interaction asks the store again. | ||
| logger.debug("relay: recorded Discord chat labels unreadable", exc_info=True) | ||
| return (None, None) | ||
| if recorded is not None: | ||
| self._discord_labels_recorded[key] = recorded | ||
| self._evict_oldest(self._discord_labels_recorded, self._DISCORD_LABELS_MAX) | ||
| self._discord_chat_labels[key] = recorded or (None, None) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Invalidate peer adapters' positive and negative label cachesThis result, including a successful Reproduction: construct two relay adapters A and B over one real SessionStore. A converts an interaction before any observation, memoizing Use a shared observation generation/invalidation owner, or another mechanism that makes a changed authoritative record visible to all adapter readers. Cover both first observation after a successful miss and a rename after a positive hit. Avoid fixing only exception retry: these reads succeeded. Verified on
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not reachable in a supported topology, so not changed. One gateway process runs one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] F07 β A recoverable DB-open failure becomes an indefinitely cached absent-label readAttribution: introduced. The guarantee in chat_labels' docstring is not upheld by its missing-handle branch. RecoverableHandleCache intentionally returns None on transient open failure, in-flight retry or backoff. The getter lookup then converts that unavailable state into the same None as an absent record. This assignment caches it. Probe: record labels using real SQLite; replace only the registry opener with a one-failure wrapper and drive the real RecoverableHandleCache with a controlled clock. First cold interaction returns (None,None). Advance beyond backoff; direct store read returns saved labels, and a fresh adapter returns them too. The original adapter still returns (None,None), with exactly two real open attempts overall. In the paired control, a raised getter exception correctly retries on the second interaction. Consequence: storage recovery does not repair this adapter's prompt inputs until another observation, eviction or restart. This is not the documented no-record fallback limitation and does not require multiple adapters. The record API is newly introduced; the base has no equivalent API, so no baseline read-recovery pass is invented. Repair/closure: propagate unavailable/indeterminate read state distinctly from an authoritative missing row and do not memoize it as permanent absence. Test failed open, backoff/in-flight miss, healthy reopen and a second interaction without intervening text, alongside the existing getter-exception case. Fresh receipt: state-head-final: unavailable-read fails; getter-exception control passes; base record API absent. Head Code under discussion β producer, state owner, and consumers:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reproduced: no handle meant |
||
| self._evict_oldest(self._discord_chat_labels, self._DISCORD_LABELS_MAX) | ||
| return self._discord_chat_labels[key] | ||
|
|
||
| def _inbound_dedupe_key(self, event) -> Optional[str]: | ||
| """Stable replay identity: (platform, chat, platform message id). The platform | ||
|
|
@@ -1129,6 +1198,9 @@ async def _on_passthrough(self, forward, buffer_id: Optional[str] = None) -> Non | |
| # A prompt-token component press is consumed (same gate as _on_inbound). | ||
| if await self._consume_prompt_response(event): | ||
| return | ||
| src = event.source | ||
| src.chat_name, src.chat_topic = await self._discord_chat_labels_for( | ||
| str(src.scope_id or ""), src.chat_id) | ||
| await self.handle_message(event) | ||
| return | ||
| logger.info( | ||
|
|
@@ -1170,17 +1242,33 @@ def _discord_interaction_to_event(self, forward): | |
| if not isinstance(user, dict): | ||
| user = {} | ||
| guild_id = payload.get("guild_id") | ||
| # The text lane's user_display_name is the native author.display_name: guild nick, else | ||
| # global name, else username. The interaction carries all three as of now, so derive the | ||
| # same name from it; a remembered one would outlive a nickname change. | ||
| user_name = next( | ||
| (str(v) for v in ((member.get("nick") if isinstance(member, dict) else None), | ||
| user.get("global_name"), user.get("username")) if v), None) | ||
| chat_id = str(payload.get("channel_id") or "") | ||
| # The text lane keys a message inside a thread on chat_type "thread" + thread_id (both session-key | ||
| # fields), so an interaction sent there must carry the same or it lands in a per-user "group" | ||
| # session beside the thread's. The partial channel object marks a thread by type (10 announcement, | ||
| # 11 public, 12 private: what discord.py's Thread covers) and names the parent. | ||
| channel = payload.get("channel") if isinstance(payload.get("channel"), dict) else {} | ||
| is_thread = bool(guild_id) and channel.get("type") in (10, 11, 12) | ||
| source = SessionSource( | ||
| # The LOGICAL platform, not RELAY: session keys must match the connector's | ||
| # capability binding (platform="discord"), /sethome must file under the | ||
| # logical platform, and _capture_scope skips the generic "relay". | ||
| platform=Platform.DISCORD, | ||
| chat_id=str(payload.get("channel_id") or ""), | ||
| chat_id=chat_id, | ||
| # "group", not "channel": both the connector's capability binding and the | ||
| # native Discord adapter key guild channels as "group". | ||
| chat_type="group" if guild_id else "dm", | ||
| chat_type="thread" if is_thread else ("group" if guild_id else "dm"), | ||
| thread_id=chat_id if is_thread else None, | ||
| parent_chat_id=str(channel["parent_id"]) if is_thread and channel.get("parent_id") else None, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Preserve a known thread parent when the partial channel omits itThe new thread derivation fixes the session split, but unconditionally loses a known parent when Reproduction: admit and persist a text source with Recover an already-known parent from an authoritative thread source, or resolve it at the boundary, and preserve it consistently across prompt and routing consumers. Add a partial-channel regression with tools enabled; the tools-disabled renderer hides this difference. Parent omission predates this PR's thread repair; it remains part of the class being repaired here. Verified on
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not changed. Discord's channel docs say interactions "always receive full channel data" for channels the app can act on, and |
||
| user_id=str(user["id"]) if user.get("id") else None, | ||
| user_name=str(user["username"]) if user.get("username") else None, | ||
| user_name=user_name, | ||
| # chat_name / chat_topic: filled by _on_passthrough from what the text lane recorded. | ||
| scope_id=str(guild_id) if guild_id else None, | ||
| message_id=str(payload.get("id")) if payload.get("id") else None, | ||
| # Same upstream-trust marker the relay text lane stamps. Set locally, never | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Keep interaction identity separate from actual message identityThis source still uses Reproduction: forward a type-3 component with A second real-wire probe provides a text frame's root Normalize event/source message identity at the boundary and represent the interaction identifier separately. Use the attached actual message for components; slash invocations with no platform message must not invent one. Regress both component identity and root-only text frames with tools enabled. Both probes also fail on the merge base; they require repair but are not new regressions introduced by this PR. Verified on
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, and older than this PR (same on the merge base). Filed as #130306 with your probes, crediting you. It stays out of this PR on purpose. teknium1's review of #129032 measured what an identity split does to the slash prompt when |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ | |
| from utils import atomic_json_write | ||
|
|
||
| if TYPE_CHECKING: | ||
| from gateway.session import SessionEntry | ||
| from gateway.session import SessionEntry, SessionSource | ||
|
|
||
| # Log-record parity with the origin module. | ||
| logger = logging.getLogger("gateway.session") | ||
|
|
@@ -234,6 +234,38 @@ def _routing_db_method(self, name: str): | |
| method = getattr(self._routing_db or None, name, None) | ||
| return method if callable(method) else None | ||
|
|
||
| @staticmethod | ||
| def _chat_labels_meta_key(platform, scope_id, chat_id) -> str: | ||
| return f"gateway_chat_labels:{getattr(platform, 'value', platform)}:{scope_id or ''}:{chat_id}" | ||
|
|
||
| def record_chat_labels(self, source: SessionSource) -> bool: | ||
| """Persist *source*'s ``chat_name`` / ``chat_topic`` as the chat's current labels, at the | ||
| moment they are observed; True once written. A session origin cannot answer "what is this | ||
| chat called now": it is written when its session is created, which can be after a newer | ||
| observation (a delayed first turn), and a reset inherits it under a fresh ``created_at``. | ||
|
|
||
| False when there is no database handle right now (open failed, backoff, JSONL fallback): | ||
| nothing was written, so the caller must not treat these labels as recorded.""" | ||
| setter = self._routing_db_method("set_meta") | ||
| if setter is None: | ||
| return False | ||
| setter(self._chat_labels_meta_key(source.platform, source.scope_id, source.chat_id), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Give this observation write the short label/activity contention budget
This call is awaited before message dispatch. The websocket reader processes inbound frames serially, and its The failure semantics already make a short budget safe: if this observation cannot be written, the adapter catches the exception, leaves Use a specialized observation-meta setter (or a Regression contract: hold |
||
| json.dumps([source.chat_name, source.chat_topic])) | ||
| return True | ||
|
|
||
| def chat_labels(self, platform, scope_id, chat_id) -> Optional[tuple]: | ||
| """The ``(chat_name, chat_topic)`` last recorded for a chat, or None when none was. Raises | ||
| when the store cannot be read, including when there is no database handle right now: | ||
| "unreadable now" is not "never labelled".""" | ||
| getter = self._routing_db_method("get_meta") | ||
| if getter is None: | ||
| raise LookupError("session database unavailable") | ||
| raw = getter(self._chat_labels_meta_key(platform, scope_id, chat_id)) | ||
| if not raw: | ||
| return None | ||
| chat_name, chat_topic = json.loads(raw) | ||
| return chat_name, chat_topic | ||
|
|
||
| def _load_routing_rows_locked(self) -> bool: | ||
| """Load state.db routing entries into ``_entries``; False when there is no loader or the | ||
| load failed (warned). Lock held.""" | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified closure of the pre-admission cancellation finding. The stable-message review check that produced 0 deliveries after cancellation/replay at fa9259f now passes and produces one delivery at 10b955a. The supplied cancelled case also exercises the real label write boundary. Keep this RED-to-GREEN regression with the cache and database-retry repairs in this cohesive Relay PR. This establishes cancellation before admission only; settlement after gateway handoff and the wider transport ledger still need their own explicit acceptance contracts.