Skip to content

Keep Slack terminal replies durable and idempotent - #2415

Open
3mdistal wants to merge 16 commits into
mainfrom
codex/slack-terminal-delivery-custody
Open

Keep Slack terminal replies durable and idempotent#2415
3mdistal wants to merge 16 commits into
mainfrom
codex/slack-terminal-delivery-custody

Conversation

@3mdistal

@3mdistal 3mdistal commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Problem

A Slack request can successfully mutate a connected app such as Content and still lose its terminal Slack reply across a serverless process boundary. Replaying the original request is unsafe because it can repeat the successful mutation.

A second ambiguity exists after Slack accepts a reply: if the SQL receipt write fails, stale recovery must distinguish “already visible in Slack” from “never posted” without creating a duplicate terminal reply.

Approach

Keep the existing one-campaign, one-continuation architecture and make terminal delivery recoverable without mutation replay:

  1. retain the receiver-verified downstream result;
  2. atomically claim terminal delivery;
  3. update a known Slack stream or placeholder timestamp in place when available;
  4. for fresh or overflow posts, attach a deterministic Block Kit delivery marker;
  5. before every fresh-post retry, reconcile those markers through conversations.replies; and
  6. persist the provider receipt and conversation history before completing the parent campaign.

If reconciliation is unavailable or exceeds its bounded scan, delivery fails closed and retries later. Recovery may reread an already-terminal downstream task, but it never reinvokes the successful mutation.

What changed

  • Added additive continuation fields for verified artifact checkpoints, provider delivery receipts, and bounded terminal history custody, with conservative legacy backfill.
  • Replaced processor-boundary bare failure with bounded continuation recovery and mutation-backed fallback.
  • Gated parent completion on provider-confirmed terminal delivery and idempotent history persistence.
  • Added scoped campaign/continuation health diagnostics and orphan reason classes.
  • Added stable delivery options to the platform adapter boundary.
  • Exposed Slack’s native stream timestamp as an update target and prohibited a strict target from silently falling through to a fresh post.
  • Replaced the undocumented client_msg_id assumption with documented thread reconciliation: fresh chunks carry deterministic hashed Block Kit markers, retries read the thread from the continuation creation time, reuse accepted message timestamps, and post only missing chunks.
  • Propagated cancellation through Slack reconciliation, response-body consumption, fresh posts, and native stream completion; the processor now waits for aborted delivery work to settle before releasing its SQL claim.
  • Added regressions for receipt-write failure followed by recovery, Slack chunk reconciliation, fail-closed and body-stalled reconciliation, strict stream-target updates, timeout/claim ordering, rollout disable, mixed siblings, and finalized-parent behavior.
  • Added the Core package changeset and Dispatch changelog entry.

Safety and operations

  • Schema changes are additive for SQLite and Postgres. Legacy failed rows remain conservative.
  • Stored custody is bounded and contains only the already-formatted assistant reply, opaque provider references, and verified artifact identities.
  • Delivery markers contain a one-way hash of the continuation delivery key and chunk index; they contain no message content, credentials, or user identifiers.
  • The existing AGENT_INTEGRATION_DURABLE_DISPATCH flag and production scope slack:C0BHWJK7BCY are unchanged. No production configuration or data changed in this Work pass.
  • Provider I/O and response-body reads receive the processor abort signal, and the processor waits for the aborted delivery to settle before releasing its SQL claim.
  • A failed reconciliation performs no fresh Slack post. Successful Content mutations remain delivery-only recovery inputs.

Verification

  • Previous head 435100b06 passed 120 focused continuation/campaign tests and the complete GitHub Actions matrix. That evidence covers the unchanged custody, recovery, migration, and parent-finalization behavior, but predates the Slack reconciliation repair.
  • Current exact head: 9e8a5ba71.
  • git diff --check and pinned oxfmt 0.56.0 pass on the current head.
  • Local dependency-backed tests are unavailable because the Framework compute host is offline and this worktree has no installed dependencies.
  • Exact-head GitHub CI is green: required Build, Content DB, Content parity, Core integration, Fast tests, Lint & format, Plan E2E, Scaffold E2E, and standalone Chat all passed.
  • Independent exact-head technical review is clean with no actionable P1/P2 findings. The scoped production acceptance run remains pending for Land.

Review focus

  • Does every fresh Slack retry reconcile the same deterministic chunk markers before posting?
  • Can a timeout release the SQL delivery claim while any provider response body or I/O is still running?
  • Can any receipt-write failure reenter connected-app mutation execution rather than delivery-only recovery?
  • Is the fail-closed bias correct when Slack thread reconciliation or strict target update is unavailable?
  • Are stored custody, delivery identifiers, and diagnostics bounded and tenant-safe?

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@netlify

This comment has been minimized.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 3 potential issues 🔴

Review Details

Code Review Summary

This incremental review verified that the four previously reported issues were addressed: receipt-backed rows reset to pending are now finalizable, missing thread mappings fail custody rather than silently succeeding, adapter exhaustion is bounded, and sibling history writes now have additional protection. Those stale review threads were resolved before this review.

The remaining implementation still has concerns in the queue-health query and in the boundary between provider receipt, thread-history persistence, and parent completion. Risk: High because this PR changes durable delivery and scoped operational data.

  • 🔴 HIGH: waiting-campaign queue stats use unqualified owner_email/org_id after joining campaigns and pending tasks, which can produce ambiguous SQL errors or incorrect scope behavior.
  • 🟡 MEDIUM: the parent is completed before terminal history cleanup; a process failure after parent completion can strand receipt custody because recovery only discovers processing parents.
  • 🟡 MEDIUM: campaign-resume paths treat provider receipt confirmation as terminal-delivered before history persistence/finalization, allowing a waiting campaign to advance while the reply remains only in the temporary custody payload.

The receipt-kind idempotency and bounded recovery logic otherwise remain sound. Browser verification is not applicable because the changes are backend/integration-only.

🧪 Browser testing: Skipped — PR only modifies backend/integration/config surfaces, no user-facing browser UI impact

Comment thread packages/core/src/integrations/task-queue-stats.ts
Comment thread packages/core/src/integrations/a2a-continuation-processor.ts Outdated
Comment thread packages/core/src/integrations/webhook-handler.ts
builder-io-integration[bot]

This comment was marked as outdated.

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 2 potential issues 🔴

Review Details

Code Review Summary

This incremental review verified that the receipt-finalization lease fix is present: confirmed deliveries now receive a future next_check_at, preventing immediate scheduler reclaim. The previously open receipt-only campaign advancement finding remains unchanged and was not reposted.

Two new issues were identified. First, the processor’s failure-recovery callback can itself reject, which escapes the per-continuation handler and aborts the rest of the batch. Second, the migration backfills only legacy completed rows; legacy failed rows now classify as terminal-without-delivery even though the old implementation could mark a delivered failure notice as failed, potentially preventing a waiting parent from completing after upgrade.

Risk: High — this PR changes durable migration semantics and queue failure recovery.

  • 🔴 HIGH: unhandled recovery errors can stop processing later continuations in the same batch.
  • 🟡 MEDIUM: ambiguous legacy failed rows can become permanently blocking after migration.

🧪 Browser testing: Skipped — PR only modifies backend/integration/config surfaces, no user-facing browser UI impact

Comment thread packages/core/src/integrations/a2a-continuation-processor.ts Outdated
Comment thread packages/core/src/integrations/a2a-continuations-store.ts

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 3 potential issues 🔴

Review Details

Code Review Summary

This incremental review verified that the processor recovery-error containment fix is present and that the receipt finalization lease is now applied. The existing receipt-only campaign advancement and legacy failed-continuation comments remain open and were not reposted.

New issues remain around receipt persistence failure and mixed-sibling recovery. Both terminal delivery paths can receive a provider delivery receipt, fail to persist that receipt after retries, and then silently return while leaving the continuation in delivering; the next recovery can resend to the provider. Additionally, an exhausted missing-adapter sibling can fail the parent while another sibling still has confirmed delivery custody awaiting history persistence, making that sibling undiscoverable.

Risk: High — these cases can cause duplicate provider replies or strand confirmed delivery history.

  • 🔴 HIGH: successful delivery receipt recording failure is silently abandoned.
  • 🔴 HIGH: failure-notice receipt recording failure is silently abandoned.
  • 🔴 HIGH: missing-adapter containment can terminate a parent while a confirmed sibling still needs history-only recovery.

🧪 Browser testing: Skipped — PR only modifies backend/integration/config surfaces, no user-facing browser UI impact

Comment thread packages/core/src/integrations/a2a-continuation-processor.ts
Comment thread packages/core/src/integrations/a2a-continuation-processor.ts
Comment thread packages/core/src/integrations/a2a-continuation-processor.ts

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Code Review Summary

This incremental review verified the new receipt-retention and mixed-sibling recovery changes, while leaving all existing unresolved findings unchanged and un reposted: receipt-only waiting campaign advancement, recovery containment, legacy failed compatibility, silent receipt-recording returns, and missing-adapter sibling custody.

The latest flow correctly retains custody while history persistence is incomplete, but one new handoff gap remains. After history is finalized, repeated parent-completion failures trigger only a best-effort ordinary task dispatch. If that dispatch is lost and rollout scope is disabled before the next sweep, parent recovery consults the legacy parent payload marker rather than the continuation receipt, so it can fail a parent whose provider reply and history were already durably confirmed.

Risk: High — this affects durable terminal recovery after a process boundary.

  • 🟡 MEDIUM: persist parent-level delivery proof or make recovery consult continuation custody, and use the campaign-continuation wake path for this handoff.

🧪 Browser testing: Skipped — PR only modifies backend/integration/config surfaces, no user-facing browser UI impact

Comment thread packages/core/src/integrations/a2a-continuation-processor.ts
@3mdistal 3mdistal changed the title Keep Slack replies in durable delivery custody Keep Slack terminal replies durable and idempotent Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants