Persist outbound federation with a transactional outbox#359
Conversation
Persist outbound ActivityPub fanout and delivery messages in an application-owned outbox that commits with the domain changes that produced them. Add ordered leasing, retry and dead-letter handling, payload redaction, pruning, and dedicated worker integration. Wrap federation-producing operations in transactions, defer best-effort AI work until commit, and keep remote inbox dereferencing outside database transactions. Fixes hackers-pub#342 hackers-pub#336 Assisted-by: Codex:gpt-5.6-sol
dahlia
added
enhancement
|
@codex review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a PostgreSQL-backed transactional outbox, worker queue processing, transactional domain operations, deferred background work, and centralized transactional inbox dispatch. Federation delivery now supports leasing, ordering, retries, dead letters, replay, pruning, and at-least-once semantics. ChangesTransactional outbox and federation delivery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GraphQL
participant Models
participant Database
participant OutboxWorker
participant Fedify
GraphQL->>Models: execute transactional mutation
Models->>Database: commit domain state and outbox event
OutboxWorker->>Database: claim and lease event
OutboxWorker->>Fedify: deliver ActivityPub work
Fedify-->>OutboxWorker: report completion or retry
OutboxWorker->>Database: complete, retry, or dead-letter event
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a transactional outbox queue to ensure atomic commits of application state changes and outgoing ActivityPub federation work. It adds the outbox_event table, implements TransactionalOutboxQueue for queue management, refactors inbox dispatching and model operations to run within transactions, and adds background pruning. The feedback identifies a critical issue in the outbox queue processor where database errors aborting a transaction prevent immediate persistence of failure/retry states, and non-database errors commit partial changes, violating atomicity. Additionally, there is a style guide violation regarding import ordering in federation/outbox-queue.ts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Let handler failures escape the fanout transaction before recording a retry or dead letter through the root database. This prevents partial fanout deliveries from committing and keeps recovery available after PostgreSQL aborts the worker transaction. Cover both ordinary handler failures and aborted database transactions with regression tests against the committed queue path. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
Keep runtime dependencies before application modules and sort both groups according to the repository import convention. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request implements a transactional outbox queue system to ensure that state changes and their corresponding outgoing ActivityPub activities are committed atomically. It introduces the outbox_event table and the TransactionalOutboxQueue class, refactoring operations such as account deletion, following, blocking, and moderation actions to use transactional enqueuing. Feedback on the changes highlights two potential runtime errors in the inbox dispatching logic: a missing null guard on followObject in onFollowed which could cause a crash, and an incorrect routing condition in onDeleted when both the object and actor IDs are null.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Require a Delete object IRI before taking the actor-deletion shortcut. Activities without object and actor IDs now continue through the general object dispatch path instead of matching two missing values. Cover the missing-ID case with a regression test that verifies object dereferencing still occurs. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a transactional outbox pattern to guarantee atomic commits of application state changes and outgoing ActivityPub activities. It introduces a custom TransactionalOutboxQueue backed by a new outbox_event table, refactors federation inbox handlers and model operations to run within database transactions, and defers best-effort background tasks (such as AI summarization and translation) until after transaction commits. A critical issue was identified in the outbox queue processor where a lost or reclaimed lease during fanout processing does not trigger a transaction rollback, which could lead to duplicate processing and inconsistent state; throwing an error when completeOutboxEvent returns false is recommended to resolve this.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (8)
drizzle/20260714090834_boring_nebula/migration.sql (1)
12-12: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider a CHECK constraint tying
statustocompleted/failed/lease fields.Elsewhere in this schema (
flag_action_suspension_check,moderation_notification_ref_check, etc.) state-shape invariants are enforced via CHECK constraints.outbox_eventhas none linkingstatus='completed'⇒completed IS NOT NULL,status='dead'⇒failed IS NOT NULL, etc. Given this table is the crux of the durability guarantee, an invariant guard would catch corrupt writes early. Please confirm the exact intended state machine before adding one, since an overly strict constraint could break valid transitions not visible in this diff.🔒 Example constraint (adjust to the real state machine)
CREATE INDEX "outbox_event_lease_idx" ON "outbox_event" ("status","leased"); +--> statement-breakpoint +ALTER TABLE "outbox_event" ADD CONSTRAINT "outbox_event_terminal_state_check" CHECK ( + ("status" = 'completed' AND "completed" IS NOT NULL) OR + ("status" = 'dead' AND "failed" IS NOT NULL) OR + ("status" IN ('pending', 'processing')) +);Also applies to: 19-20, 24-25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drizzle/20260714090834_boring_nebula/migration.sql` at line 12, Confirm the intended outbox state machine, then add a CHECK constraint on outbox_event that enforces the required relationship between status and completed, failed, and lease fields, including the analogous status-column definitions noted in the comment. Ensure the constraint permits every valid transition while rejecting inconsistent state combinations, and follow the schema’s existing CHECK-constraint naming conventions.models/outbox.ts (1)
65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSequence name duplicated as a raw string.
'outbox_event_sequence'here has no compile-time link tooutboxEventSequencedefined inmodels/schema.ts. If the sequence is ever renamed there, this silently breaks at runtime with a "relation does not exist" error instead of failing at build/type-check time.Consider exporting the DB name as a shared constant (or referencing the
pgSequenceobject viasql.identifier) so both files stay in sync.♻️ Suggested fix
+// models/schema.ts +export const OUTBOX_EVENT_SEQUENCE_NAME = "outbox_event_sequence"; +export const outboxEventSequence = pgSequence(OUTBOX_EVENT_SEQUENCE_NAME);-async function nextSequence(db: OutboxDatabase): Promise<bigint> { - const rows = await db.execute( - sql`select nextval('outbox_event_sequence')::text as sequence`, - ) as unknown as Array<{ sequence: string }>; +async function nextSequence(db: OutboxDatabase): Promise<bigint> { + const rows = await db.execute( + sql`select nextval(${OUTBOX_EVENT_SEQUENCE_NAME})::text as sequence`, + ) as unknown as Array<{ sequence: string }>; return BigInt(rows[0].sequence); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/outbox.ts` around lines 65 - 70, Update nextSequence to reuse the exported database sequence name or identifier from outboxEventSequence in models/schema.ts instead of the duplicated 'outbox_event_sequence' literal. Ensure the SQL references the shared symbol so renaming the schema sequence keeps this query synchronized.models/pin.ts (1)
73-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isTransactionbranch is now dead code.
pinPostis only reachable throughtransactional(pinPostOperation)(line 87), andtransactional()always supplies an already-openTransactionviawithTransaction. SoisTransaction(db)at line 73 is now alwaystrue, and thedb.transaction(pinInTransaction)fallback branch can never execute. Keeping it around risks a future reader assumingpinPostOperationstill needs to handle a non-transactionaldb, and papers over the fact the function now has a hard transactional precondition.♻️ Suggested simplification
- const pinInTransaction = async (tx: Transaction) => { + const result = await (async (tx: Transaction) => { ... - }; - - const result = isTransaction(db) - ? await pinInTransaction(db) - : await db.transaction(pinInTransaction); + })(db as Transaction);Based on learnings,
transactional()(models/tx.ts) wraps operations so downstream code always receives a bound transaction context, which is the same contract this branch was defending against before the wrapper existed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/pin.ts` around lines 73 - 89, Remove the now-unreachable isTransaction helper and conditional in pinPostOperation; use the provided db directly as the transaction when invoking pinInTransaction. Preserve the existing result handling and pinPost transactional wrapper while making the function’s transaction-only contract explicit.models/schema.ts (1)
1696-1774: 🚀 Performance & Scalability | 🔵 TrivialSolid outbox schema design. Columns, unique index, and status enum line up correctly with every consumer in
models/outbox.ts(ordering key, lease token, message-id dedup target, etc.).One index-coverage nit worth a look: the crash-recovery branch of
claimOutboxEvent's WHERE clause (status = 'processing' AND leased <= expired) isn't fully served by eitheroutbox_event_ready_idx(lines 1755-1761, noleasedcolumn) oroutbox_event_lease_idx(line 1768, noeventType). This is a cold path today, but if per-event-type volume grows, consider a composite(eventType, status, leased)index for that branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/schema.ts` around lines 1696 - 1774, Add a composite index on outboxEventTable covering eventType, status, and leased to support the crash-recovery WHERE clause in claimOutboxEvent. Keep the existing ready, ordering, and lease indexes unchanged, and give the new index a distinct descriptive name.models/tx.ts (1)
38-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider including task position in the failure log.
The catch swallows and logs every after-commit failure identically; with multiple queued tasks there's no way to tell which one failed from the log alone.
♻️ Optional: add task index to the log context
- for (const task of afterCommit) { + for (const [index, task] of afterCommit.entries()) { try { await task(); } catch (error) { - logger.error("Failed to run after-commit task: {error}", { error }); + logger.error( + "Failed to run after-commit task {index}/{total}: {error}", + { index: index + 1, total: afterCommit.length, error }, + ); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/tx.ts` around lines 38 - 46, Include the afterCommit task’s position in the failure log within the loop around the after-commit task execution. Iterate with an index or otherwise track each task’s position, then add that index to the logger.error context while preserving the existing error details and continuation behavior.graphql/account.ts (1)
243-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "update + send actor update + refetch" sequence across both alias mutations.
addAccountMigrationAliasandremoveAccountMigrationAliasnow each inline the same "send actor Update, then refetchaccountTable" tail that a shared helper previously covered. Consider extracting a small helper that takes the actor-mutating callback and does the common send/refetch/guard steps once.♻️ Sketch of a shared helper
+async function finishMigrationAliasUpdate( + fedCtx: ApplicationContext<Transaction>, + accountId: Uuid, + actorUpdated: Date, +) { + await sendAccountActorUpdate(fedCtx, accountId, actorUpdated); + const account = await fedCtx.db.query.accountTable.findFirst({ + where: { id: accountId }, + }); + if (account == null) throw new InvalidInputError("accountId"); + return account; +}Then both functions call
return await finishMigrationAliasUpdate(fedCtx, accountId, actor.updated);after their respective.update(actorTable)calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graphql/account.ts` around lines 243 - 294, Extract the duplicated actor-update completion logic from addAccountMigrationAlias and removeAccountMigrationAlias into a shared helper, such as finishMigrationAliasUpdate, that validates the updated actor, sends sendAccountActorUpdate, refetches accountTable, validates the account, and returns it. Have both mutation functions invoke this helper after their actorTable update while preserving their existing alias-specific SQL and transaction flow.models/moderation.ts (1)
665-665: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify by removing redundant transaction closures.
Both of these operations are now exclusively wrapped by
transactional(), guaranteeing thatfedCtx.dbis already initialized as aTransaction. The legacy internal closures andisTransactionchecks are redundant and can be removed to flatten the logic.
models/moderation.ts#L665-L665: inline therunclosure, drop theisTransaction(db)check, and remove the unusedisTransactionhelper if it exists below.models/poll.ts#L620-L622: inline thevoteInTransactionclosure, drop theisTransaction(db)check, and delete the unusedisTransactionhelper function at the bottom of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/moderation.ts` at line 665, In models/moderation.ts at lines 665-665, inline the run closure, use the already initialized fedCtx.db transaction directly, remove the isTransaction(db) check, and delete the unused isTransaction helper if present. In models/poll.ts at lines 620-622, apply the same change by inlining voteInTransaction, removing the conditional transaction handling, and deleting that file’s unused isTransaction helper.models/post/lifecycle.ts (1)
36-38: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid nested
transactionalwrapper calls during recursion.Since
deletePostis the exportedtransactionalwrapper, calling it recursively re-invokes the transaction setup. We are already executing within the transaction bounds here, so it is safer and more efficient to call the internaldeletePostOperationdirectly, bypassing unnecessary re-entrancy checks or savepoint exhaustion.♻️ Proposed refactor
for (const reply of replies) { - await deletePost(fedCtx, { ...reply, replyTarget: post }); + await deletePostOperation(fedCtx, { ...reply, replyTarget: post }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@models/post/lifecycle.ts` around lines 36 - 38, Update the recursive reply-deletion loop to call the internal deletePostOperation directly instead of the exported transactional deletePost wrapper. Preserve the existing fedCtx and reply arguments while bypassing nested transaction setup during recursion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@drizzle/20260714090834_boring_nebula/migration.sql`:
- Line 12: Confirm the intended outbox state machine, then add a CHECK
constraint on outbox_event that enforces the required relationship between
status and completed, failed, and lease fields, including the analogous
status-column definitions noted in the comment. Ensure the constraint permits
every valid transition while rejecting inconsistent state combinations, and
follow the schema’s existing CHECK-constraint naming conventions.
In `@graphql/account.ts`:
- Around line 243-294: Extract the duplicated actor-update completion logic from
addAccountMigrationAlias and removeAccountMigrationAlias into a shared helper,
such as finishMigrationAliasUpdate, that validates the updated actor, sends
sendAccountActorUpdate, refetches accountTable, validates the account, and
returns it. Have both mutation functions invoke this helper after their
actorTable update while preserving their existing alias-specific SQL and
transaction flow.
In `@models/moderation.ts`:
- Line 665: In models/moderation.ts at lines 665-665, inline the run closure,
use the already initialized fedCtx.db transaction directly, remove the
isTransaction(db) check, and delete the unused isTransaction helper if present.
In models/poll.ts at lines 620-622, apply the same change by inlining
voteInTransaction, removing the conditional transaction handling, and deleting
that file’s unused isTransaction helper.
In `@models/outbox.ts`:
- Around line 65-70: Update nextSequence to reuse the exported database sequence
name or identifier from outboxEventSequence in models/schema.ts instead of the
duplicated 'outbox_event_sequence' literal. Ensure the SQL references the shared
symbol so renaming the schema sequence keeps this query synchronized.
In `@models/pin.ts`:
- Around line 73-89: Remove the now-unreachable isTransaction helper and
conditional in pinPostOperation; use the provided db directly as the transaction
when invoking pinInTransaction. Preserve the existing result handling and
pinPost transactional wrapper while making the function’s transaction-only
contract explicit.
In `@models/post/lifecycle.ts`:
- Around line 36-38: Update the recursive reply-deletion loop to call the
internal deletePostOperation directly instead of the exported transactional
deletePost wrapper. Preserve the existing fedCtx and reply arguments while
bypassing nested transaction setup during recursion.
In `@models/schema.ts`:
- Around line 1696-1774: Add a composite index on outboxEventTable covering
eventType, status, and leased to support the crash-recovery WHERE clause in
claimOutboxEvent. Keep the existing ready, ordering, and lease indexes
unchanged, and give the new index a distinct descriptive name.
In `@models/tx.ts`:
- Around line 38-46: Include the afterCommit task’s position in the failure log
within the loop around the after-commit task execution. Iterate with an index or
otherwise track each task’s position, then add that index to the logger.error
context while preserving the existing error details and continuation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3083322-753e-4512-9a72-cc2427d4f6a5
📒 Files selected for processing (48)
ARCHITECTURE.mdFEDERATION.mddrizzle/20260714090834_boring_nebula/migration.sqldrizzle/20260714090834_boring_nebula/snapshot.jsonfederation/context.test.tsfederation/context.tsfederation/deno.jsonfederation/inbox/dispatch.test.tsfederation/inbox/dispatch.tsfederation/inbox/following.test.tsfederation/inbox/following.tsfederation/inbox/mod.tsfederation/inbox/quote.tsfederation/inbox/subscribe.tsfederation/outbox-queue.test.tsfederation/outbox-queue.tsfederation/outbox.tsfederation/tags-pub.tsgraphql/account.tsgraphql/hashtag.tsgraphql/moderation.tsgraphql/schema.graphqlgraphql/worker.tsmodels/account.delete.test.tsmodels/account.tsmodels/article.background.test.tsmodels/article.summary.test.tsmodels/article.tsmodels/blocking.tsmodels/deno.jsonmodels/following.tsmodels/moderation-action.test.tsmodels/moderation.tsmodels/note.tsmodels/organization.tsmodels/outbox.test.tsmodels/outbox.tsmodels/pin.tsmodels/poll.tsmodels/post/engagement.tsmodels/post/lifecycle.tsmodels/post/sharing.tsmodels/reaction.tsmodels/relay.tsmodels/schema.tsmodels/tx.test.tsmodels/tx.tsruntime/resources.ts
Treat a failed completion CAS as a processing error so a stale fanout worker cannot commit deliveries after another worker has reclaimed the parent event. The outer recovery path still observes the stale lease and leaves ownership with the current worker. Cover the race by replacing the lease during fanout and verifying that its nested delivery is rolled back. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust transactional outbox pattern to ensure that database state changes and outgoing ActivityPub federation work commit atomically. It implements a custom TransactionalOutboxQueue conforming to Fedify's MessageQueue interface, adds the outbox_event table schema and migration, refactors inbox dispatching and various GraphQL mutations (such as account deletion, moderation actions, and hashtag following) to use atomic transactions, and sets up a background worker cron job to prune expired outbox events. The feedback focuses on improving the robustness and diagnostics of the outbox queue listener, specifically by handling clean shutdowns gracefully to avoid logging AbortError as an error, and logging a warning if an outbox lease renewal fails or expires during processing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
federation/outbox-queue.test.ts (1)
156-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
AbortControllerinstead of a fixed timeout to prevent test flakiness.Using
AbortSignal.timeout(50)introduces a race condition on slow CI runners. If the handler logic (the enqueue and database update) takes longer than 50ms, the listener will abort prematurely, causing the worker's transaction to roll back due to the abort rather than the lease mismatch. While the test assertions will still pass, it will be a false positive because the lease validation logic wasn't actually tested.Explicitly aborting the controller at the end of the handler ensures the test always fully evaluates the lease rejection path and stops polling immediately afterward without relying on a fixed timeout.
♻️ Proposed refactor to avoid test race conditions
try { + const controller = new AbortController(); await fanoutQueue.enqueue({ type: "fanout", id: parentId }); await fanoutQueue.listen(async () => { await deliveryQueue.enqueue(message(childId)); await testDb.update(outboxEventTable).set({ leaseToken: reclaimedLeaseToken, }).where(eq(outboxEventTable.messageId, parentId)); + controller.abort(); - }, { signal: AbortSignal.timeout(50) }); + }, { signal: controller.signal });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@federation/outbox-queue.test.ts` around lines 156 - 164, Replace the fixed AbortSignal.timeout(50) in the fanoutQueue.listen test with an AbortController. Pass its signal to the listener and abort the controller only after deliveryQueue.enqueue and the testDb.update operation complete, ensuring the lease-rejection path is evaluated before polling stops.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@federation/outbox-queue.test.ts`:
- Around line 156-164: Replace the fixed AbortSignal.timeout(50) in the
fanoutQueue.listen test with an AbortController. Pass its signal to the listener
and abort the controller only after deliveryQueue.enqueue and the testDb.update
operation complete, ensuring the lease-rejection path is evaluated before
polling stops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3688a6ec-ff97-48b7-ab4d-1870d04b4b1d
📒 Files selected for processing (4)
federation/inbox/dispatch.test.tsfederation/inbox/dispatch.tsfederation/outbox-queue.test.tsfederation/outbox-queue.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- federation/outbox-queue.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e66f58bfea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Treat listener cancellation as normal shutdown after releasing the leased event, and stop reporting subsequent polling failures once the abort signal is set. Warn while an active heartbeat can no longer renew its lease so operators can distinguish lease loss from database errors. hackers-pub#359 (comment) hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
Require both Delete IRIs before dispatch and only dereference post objects when the actor and object share an origin. This preserves actor and quote deletions while preventing arbitrary or incomplete Delete activities from forcing remote object fetches. Cover missing-ID and cross-origin activities with dispatch regressions. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
Keep a translated placeholder when transactional persistence fails, but reset its claim timestamp through the root database using the original claim as a CAS guard. The existing stale-placeholder path can then retry immediately without deleting prior locale state or waiting 30 minutes. Exercise a second translation and outbox attempt immediately after the first persistence failure. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a transactional outbox pattern to ensure that database state changes and outgoing ActivityPub activities commit atomically. It introduces the outbox_event table, a TransactionalOutboxQueue adapting Fedify's message queue, and wraps various model operations in transactions. The review feedback highlights a critical performance issue where slow AI translation and summarization promises are returned inside queueAfterCommit blocks, which would block the post-commit phase of the transaction. The reviewer suggests triggering these as fire-and-forget background tasks instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
federation/inbox/dispatch.ts (1)
140-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unreachable
onActorDeletedfallback.The
await onActorDeleted(txCtx, del)fallback at line 141 is unreachable dead code.onActorDeletedinternally requiresdel.objectId?.href === del.actorId?.href, but this exact condition is already handled by the early return at line 123. By the time execution reaches here, the IDs are guaranteed to differ, meaningonActorDeletedwill immediately returnfalse.♻️ Proposed refactor to remove dead code
- await onPostDeleted(txCtx, del, { object }) || - await onActorDeleted(txCtx, del) || - logger.warn("Unhandled Delete object: {delete}", { delete: del }); + await onPostDeleted(txCtx, del, { object }) || + logger.warn("Unhandled Delete object: {delete}", { delete: del });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@federation/inbox/dispatch.ts` around lines 140 - 142, Remove the unreachable onActorDeleted(txCtx, del) fallback from the delete dispatch chain, leaving onPostDeleted followed by the existing unhandled-delete warning. Preserve the earlier onActorDeleted handling and current warning behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@federation/inbox/dispatch.ts`:
- Line 138: Update federation/inbox/dispatch.ts lines 138-138 to call getObject
with explicit documentLoader and contextLoader values from fedCtx, retaining
suppressError: true and removing the fedCtx spread. Update
federation/inbox/dispatch.test.ts lines 55-67 so the mock tracks dereferencing
with a boolean flag instead of asserting options.data.db; both sites require
changes.
---
Nitpick comments:
In `@federation/inbox/dispatch.ts`:
- Around line 140-142: Remove the unreachable onActorDeleted(txCtx, del)
fallback from the delete dispatch chain, leaving onPostDeleted followed by the
existing unhandled-delete warning. Preserve the earlier onActorDeleted handling
and current warning behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cad4c60-5446-42f1-bd00-d88cf695e991
📒 Files selected for processing (5)
federation/inbox/dispatch.test.tsfederation/inbox/dispatch.tsfederation/outbox-queue.tsmodels/article.background.test.tsmodels/article.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- models/article.background.test.ts
- federation/outbox-queue.ts
- models/article.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7856ff30f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Resolve remote actors and quote instruments before opening the inbox transaction. Persist any remote quote candidate during this preparation phase, then keep quote state changes and outbound replies atomic inside the transaction. Cover the listener boundary so future changes cannot move instrument fetches back onto a transaction-scoped database context. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a transactional outbox queue system to ensure that application state changes and outgoing ActivityPub tasks are committed atomically. It adds the outbox_event table, implements TransactionalOutboxQueue to adapt Fedify's message queue, and decorates key operations (such as account deletion, moderation actions, and posts) with transactions. Feedback on the changes highlights a performance issue where returning promises inside queueAfterCommit callbacks (for AI translation and summarization) causes them to be awaited sequentially, blocking the HTTP response. The reviewer suggests using the void operator to run these heavy network operations asynchronously in the background.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07923b2daa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Persist a prepared quote instrument inside the inbox transaction so the remote post and its pending request roll back together. Add a database-only persistence mode that rejects further remote fetches while preserving any existing post enrichment. Cover forced transaction rollback to prevent orphaned quote posts from returning. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a transactional outbox pattern to ensure that application state changes and outgoing ActivityPub federation work are committed atomically. It adds the outbox_event database table, implements a TransactionalOutboxQueue adapting Fedify's MessageQueue, and refactors inbox dispatching and various model operations to run within database transactions. The review feedback suggests attaching a dummy .catch() handler to the abort promise in runHandlerBounded to prevent unhandled promise rejections after a race settles, and adding a defensive check for follow.actorId == null in prepareFollower to avoid potential runtime errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Stop malformed Follow activities before account lookup or actor dereferencing. This avoids unnecessary remote work for an activity that cannot identify its follower. Cover the inbox dispatch boundary so missing actor IDs remain a no-fetch case. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust Transactional Outbox Pattern to ensure that state changes and outgoing ActivityPub activities are committed atomically. It adds a new outbox_event table and sequence to the database schema, implements a TransactionalOutboxQueue adapting Fedify's MessageQueue interface, and updates various domain models (such as accounts, articles, blocking, following, moderation, notes, polls, reactions, and shares) to use a new transactional decorator. Additionally, the inbox dispatch flow has been refactored to dereference remote objects before opening database transactions, preventing long-lived transactions. A daily cron job has also been added to prune completed and dead outbox events. As there are no review comments provided, I have no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67181499c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Hold the shared test database lock while post-commit article summary tests create, inspect, and remove root database fixtures. This prevents parallel aggregate tests from observing those transient committed rows. hackers-pub#359 Assisted-by: Codex:gpt-5.6-sol
Move pending fanout and delivery messages from Fedify's legacy shared queue into the transactional outbox before the new queues start. The copy and delete share a table lock and transaction so ordering keys, delays, and inbox messages survive the handoff without delivery loss. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
Resolve and persist both Block actors before opening the relationship transaction. The shorter database-only phase still commits the block, relationship cleanup, and resulting outbox writes atomically without holding a connection during remote profile dereferences. hackers-pub#359 (comment) Assisted-by: Codex:gpt-5.6-sol
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust transactional outbox queue system to ensure atomic database commits and outgoing ActivityPub message enqueues. It adds the outbox_event table, implements a lease-based TransactionalOutboxQueue conforming to Fedify's MessageQueue interface, and refactors various application operations (such as account deletion, article creation/updates, follow/unfollow, blocking, moderation actions, and reactions) to run within transactions using a new transactional decorator. Additionally, it handles legacy outbox migration, background worker cron pruning, and ensures inbox dispatching dereferences remote objects before transactions. As there are no review comments provided, I have no further feedback to offer on this pull request.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Run legacy delivery migration only in processes that consume the federation queue, immediately before their listeners start. Static Fresh builds can now construct runtime resources without requiring PostgreSQL, while queue handoff still completes before legacy rows are consumed. https://github.com/hackers-pub/hackerspub/actions/runs/29384330167/job/87254831249?pr=359 Assisted-by: Codex:gpt-5.6-sol
1 participant
Part of #336. Closes #342.
Why the queue write belongs in the transaction
Hackers' Pub previously updated application state and handed the corresponding ActivityPub work to Fedify across separate persistence boundaries. Both writes were durable once complete, but a queue failure or process exit between them could leave a follow, post, moderation decision, or account deletion committed without the activity that tells remote servers about it.
The existing after-commit mechanism remains a good fit for replaceable work such as article summaries. Federation delivery has a stricter requirement: once a domain change is visible, its initial fanout message must already be durable. This change therefore makes that message part of the domain transaction instead of an in-memory callback after it.
How the boundary is kept acyclic
The generic persistence and leasing code lives in models/outbox.ts. It stores versioned events and retry state without importing Fedify. The adapter in federation/outbox-queue.ts implements Fedify's
MessageQueuecontract and binds each enqueue to the database handle carried by the current application context. federation/context.ts preserves that binding when a context is cloned for a transaction.This split lets model operations commit their state and the initial fanout row together while keeping the package direction unchanged. The worker later expands fanout rows into per-inbox delivery rows. That expansion is also transactional, so acknowledging the parent and storing all children is one database operation.
Model operations that produce outbound federation now establish this boundary at their public entry points. Best-effort AI work is queued for after commit, using the root database rather than a transaction handle that is about to close. Incoming federation follows the same rule in reverse: remote objects and actors are resolved before opening the transaction that changes local relationships and stores any response activity.
How workers coordinate
Claims use
FOR UPDATE SKIP LOCKED, renewable leases, and lease-token comparisons on completion. An expired claim can be recovered by another worker, while the stale worker can no longer acknowledge it. Stable message IDs make enqueue idempotent.Ordering keys need stronger coordination than row claiming alone because two transactions may enqueue the same key at the same time. Enqueue transactions take a transaction-scoped advisory lock before allocating their sequence. Claims also reject a key while another row for it is processing or an earlier sequence/position is pending. Nested fanout enqueues reserve distinct positions synchronously, including when Fedify creates them in parallel.
A handler is bounded to 180 seconds. Transient failures use capped exponential backoff, Fedify-requested retries update the leased row in place, and permanent failures become dead letters with structured diagnostics. Completed payloads are removed immediately. The worker retains completed metadata for one day and dead letters for 30 days, then prunes them through the existing cron process.
Operational notes
The migration in drizzle/20260714090834_boring_nebula/migration.sql creates the
outbox_eventtable, its ordering sequence, and the indexes used for claiming and lease recovery. The GraphQL worker remains the only consumer: it drains the existing Fedify inbox queue alongside the new fanout/delivery queues. API processes only enqueue.Dead rows retain their payload and last error for inspection. Operators can requeue one with
replayOutboxEvent; replay keeps the event identity and resets its processing-attempt count.Verification
mise run checkandmise run testpass.