Keep Slack terminal replies durable and idempotent - #2415
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed: Open the full interactive recap |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
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_idafter 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-deliveredbefore 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
There was a problem hiding this comment.
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
failedrows can become permanently blocking after migration.
🧪 Browser testing: Skipped — PR only modifies backend/integration/config surfaces, no user-facing browser UI impact
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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

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:
conversations.replies; andIf 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
client_msg_idassumption 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.Safety and operations
AGENT_INTEGRATION_DURABLE_DISPATCHflag and production scopeslack:C0BHWJK7BCYare unchanged. No production configuration or data changed in this Work pass.Verification
435100b06passed 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.9e8a5ba71.git diff --checkand pinnedoxfmt 0.56.0pass on the current head.Review focus