Slack delivery: fast group-DM addressing without directory clobbering, and no duplicate replies - #170
Merged
Merged
Conversation
…nces clobbering the directory * fix(slack): resync the directory when a message arrives from an unseen group DM Group DMs are addressed by their participant set, resolved against directory_group_members, which the Slack surface pushes. Group membership only refreshed on the 30-minute private-channel TTL, so a group DM created moments ago was unaddressable: /v1/reach returned group_not_found until the timer came round. An mpim created with the bot already among its initial members fires no group_join subtype and no member_joined_channel, so the existing membership hooks never ran. Its first plain message fell straight through to the turn handler and synced nothing. Track the mpim ids the last listing saw and force a directory sync when a message arrives from one we have not seen. The sync is already coalesced, and tracking every listed mpim (not only the ones surviving the all-internal filter) keeps an unaddressable group from resyncing on every message. The failure mode this fixes: the agent could already reply inside the new group's own session, but a concurrent session's reach call to that group kept failing, so the caller was told the group did not exist. * fix(slack): bound the unseen-group resync to one attempt per group Review of the previous commit found two faults it introduced. The seen-set was assigned only after `listBotGroupDms` resolved, so any persistent listing failure — a missing mpim:read scope, a 429 — left every mpim permanently unseen and made every message trigger another full directory sync. The coalescer serializes those but does not bound them, so a rate-limit error caused the API burn that provokes more rate-limit errors. The set also recorded what was listed rather than what was published, so a group whose push failed was marked seen and never retried. Collapse both into one set marked optimistically at the trigger, and move the policy into the directory beside the sync it guards. A group now costs at most one forced sync, whatever happens to it downstream, and a message burst in a new group collapses to one sync instead of one per message. A sync that fails still self-heals on the ordinary TTL refresh, which is the behaviour that predates this change. The tests now count mpim listings rather than directory pushes: the earlier assertions passed against an implementation with the trigger removed. * fix(directory): refuse a directory swap stamped older than the stored snapshot The directory push is a full per-org swap gated only by a roster hash, and each core instance pushes from its own in-memory cache. With multiple instances, the one that resyncs for a new group DM fixes the table and another — pushing every few minutes from a cache up to thirty minutes old — deletes the group again. The previous commits' prompt resync therefore held only until a stale instance's next push, and the group flapped in and out until every cache expired. Carry the snapshot's fetch time with each push (members from the user snapshot, channels and groups from the membership cache) and have the store refuse a swap stamped older than the one it already holds. A refused members push also skips the deactivation side-effects, which would otherwise act on the stale roster. An unstamped push keeps today's last-write-wins behaviour, so nothing changes for callers that do not stamp. The losing instance's pushes are dropped wholesale until its cache refreshes; new public channels still land promptly via the winning instance, which lists them fresh on every push. * fix(directory): review fixes — stale fallback stamps, idempotent-push ordering, refusal logging Review of the stale-swap guard found two holes in the scenario it exists to close. A failed group listing fell back to the previous rows but the rebuilt cache was stamped with the current time, so stale (or, after a forced sync, empty) group data shipped under a stamp that beat a genuinely fresh sync from the other instance. The group stamp now travels with the group data: a fallback keeps the old stamp, and a fallback with no prior cache stamps zero, which loses to any real sync. An identical push returned early without advancing the stamp, so the stored stamp froze at the last content-changing write and a swap older than the newest snapshot seen could still win; the in-memory store, which advances on every accepted push, disagreed with Postgres about the same sequence. The hash-equal path now advances the stamp, aligning the two stores. Refused swaps were silent; both stores now log the section and how far behind the refused stamp was, since a clock-skewed instance losing every push would otherwise be invisible. * style: prettier
…plicate post On an addressed surface-tools turn that ends without a post, core injected a 'reply with post or stay_silent' nudge even when the model had already written its reply as plain assistant text. The nudged model re-posts near-identical text, and surfaces that render assistant entries (the web chat) show the message twice. Now, when the primary turn ends with reply text, that text is delivered directly through the existing fallback delivery path — no second model call, no duplicate. The nudge still fires when the turn ends with no reply text, and the post-nudge fallback is unchanged. Tests: text-bearing endings assert direct delivery with no nudge request; a new !shedmute mock command ends with no reply text so the nudge machinery (stateless rebuild, tape continuation, image/environment preservation) keeps end-to-end coverage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two Slack-delivery correctness fixes.
Fast group-DM addressing, without stale instances clobbering the directory. Group DMs are addressed by participant set against the pushed directory, which only refreshed on the 30-minute TTL — a group DM created moments ago was unaddressable (
group_not_found), and an mpim created with the bot already in it fires no join events at all. A message from an unseen mpim now forces one coalesced directory sync (bounded to one attempt per group, so listing failures can't turn a message burst into an API burn). Because each core instance pushes the directory from its own cache, a multi-instance deployment could still flap: a stale instance's periodic push deleted the just-synced group again. Directory swaps now carry the snapshot's fetch time and a store refuses a swap stamped older than what it holds, with review fixes for fallback stamps, idempotent-push stamp advancement, and refusal logging.No duplicate replies from the post nudge. On an addressed turn that ended without a post, core nudged the model to reply even when the turn had already produced reply text — surfaces rendering assistant entries showed the message twice. A text-bearing ending now delivers that text directly through the existing fallback path; the nudge still fires when a turn ends with no reply text.
Tests cover the unseen-group resync trigger and bounding, stale-swap refusal in both stores, and direct delivery vs nudge behavior.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.