Skip to content

[codex] fix(outbox): preserve local FIFO event order - #1321

Merged
kkroo merged 1 commit into
masterfrom
codex/fix-plugin-outbox-fifo
Aug 24, 2026
Merged

[codex] fix(outbox): preserve local FIFO event order#1321
kkroo merged 1 commit into
masterfrom
codex/fix-plugin-outbox-fifo

Conversation

@kkroo

@kkroo kkroo commented Aug 12, 2026

Copy link
Copy Markdown

Thinking Path

  • Paperclip uses a durable plugin-event outbox to bridge API-tier activity writes to the worker-tier plugin event bus.
  • The outbox promises FIFO delivery by its monotonic seq column.
  • logActivity intentionally keeps normal outbox publication fire-and-forget, so sequential callers can overlap their inserts.
  • Overlapping inserts can allocate seq in completion order rather than call order, delivering issue.updated before issue.created.
  • This pull request serializes detached outbox inserts within each process while preserving best-effort failure semantics.
  • The regression test delays the first insert and proves the persisted sequence remains causal.

Linked Issues or Issue Description

Related: #1031, which protects transactional outbox writes. This is a separate normal detached-publication path.

Bug report:

  • Expected: sequential domain events issued by one Paperclip process retain that causal order in plugin_event_outbox.seq.
  • Actual: a delayed first detached insert can allow a subsequent insert to claim an earlier sequence number.
  • Reproduction: delay the first outbox insert, call logActivity for issue.created and then issue.updated, and observe reversed persisted order without this fix.
  • Impact: worker-side plugin consumers can receive an update before its corresponding create event.

What Changed

  • Chain fire-and-forget plugin outbox inserts through a per-process enqueue tail.
  • Keep insert failures logged and swallowed so ordinary activity callers retain best-effort behavior.
  • Add a deterministic embedded-Postgres regression covering a delayed first insertion and exact seq ordering.

Verification

  • pnpm exec vitest run server/src/__tests__/plugin-event-outbox.test.ts --no-file-parallelism --maxWorkers=1
  • pnpm exec vitest run server/src/__tests__/plugin-event-outbox.test.ts server/src/__tests__/plugin-orchestration-apis.test.ts --no-file-parallelism --maxWorkers=1
  • pnpm --filter @paperclipai/server exec tsc --noEmit
  • git diff --check

Risks

Low operational risk. Ordering is serialized per process only; independent replicas can still interleave database inserts. Transactional callers should continue using the existing atomic/deferred publication paths when they need cross-replica causal coupling. The queue tail adds at most one pending promise per detached event and does not make the activity request await the outbox write.

Model Used

OpenAI Codex (GPT-5 family; exact deployment identifier is not exposed by the runtime), using repository inspection, test execution, and a delegated coding subagent whose local proposal was independently reviewed and validated.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this bug fix does not duplicate planned core work
  • I have searched GitHub for duplicate or related PRs and linked the related PR above
  • I have described the bug in-PR
  • I have run focused tests locally and they pass
  • I have added or updated tests where applicable
  • I have updated relevant documentation; no command or user-facing documentation changes are needed
  • I have considered and documented risks above
  • All Paperclip CI gates are green
  • Greptile is 5/5 with no open P2s, recommendations, or follow-ups
  • I will address all Greptile and reviewer comments before requesting merge

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@kkroo
kkroo marked this pull request as ready for review August 12, 2026 01:32
@cursor

cursor Bot commented Aug 12, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@github-actions

Copy link
Copy Markdown

@ally head a06190e has been awaiting review for 278.8h with no review on either surface (pulls/1321/reviews carries no consolidated report for this head, no ## Ally comment either) -- automated sweep (BLO-22892 / BLO-28203), not a human/agent re-ask.

Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head a06190e.

@github-actions
github-actions Bot requested a review from allyblockcast August 23, 2026 16:17

@allyblockcast allyblockcast Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a06190e

Critical Issues (0)

Important Issues (0)

Suggestions (1)

  • [pr-review-toolkit/tests] server/src/__tests__/plugin-event-outbox.test.ts:51 — make the first insert wait on an explicit promise released after the second insert is scheduled, rather than only one setImmediate turn. As written, the first insert may complete before the second logActivity reaches the outbox, so the test can pass without exercising the out-of-order completion race that the new queue is intended to prevent.

Strengths

  • The enqueue tail serializes database insert completion while preserving the detached publishPluginDomainEvent API.
  • Insert failures are logged and swallowed inside each queued operation, so one failed write does not poison subsequent events.
  • The regression test checks persisted seq order rather than only invocation order.

Recommended Action

  1. Consider making the delayed-insert test deterministic before relying on it as coverage for the race.
  2. The implementation is safe to merge.

@kkroo
kkroo added this pull request to the merge queue Aug 23, 2026
Merged via the queue into master with commit b50744f Aug 24, 2026
34 of 35 checks passed
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.

1 participant