Fix federation error handling to prevent 500 responses #1328
Conversation
When USE_MQ=false, federation happens synchronously during the HTTP request. If delivery to any follower fails (e.g., deleted accounts, down servers), it causes the entire request to return 500 even though the action itself succeeded.
WalkthroughAdds a Logger dependency to FediverseBridge and updates its constructor accordingly. Wraps the send-to-followers path in a try/catch around the existing sendActivity call. On failure, logs a structured federation error with account details instead of letting the error propagate. Successful behavior remains unchanged; failures are now handled and recorded. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Single-file change with a small control-flow addition (try/catch) and a constructor signature update; low logic density and limited scope. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/activitypub/fediverse-bridge.ts (1)
58-73: Inbox path still throws: wrapsendActivityToInboxto avoid 500s on block/reject flows.This path can still bubble errors during synchronous delivery (USE_MQ=false), reintroducing 500s when blocking accounts.
await ctx.sendActivity( - { username: account.username }, - { - id: recipient.apId, - inboxId: recipient.apInbox, - }, - activity, - ); + try { + await ctx.sendActivity( + { username: account.username }, + { + id: recipient.apId, + inboxId: recipient.apInbox, + }, + activity, + ); + } catch (error) { + this.logger.error('Failed to federate activity to inbox: {error}', { + senderAccountId: account.id, + senderAccountUsername: account.username, + recipientAccountId: recipient.id, + recipientAccountUsername: recipient.username, + activityId: activity.id?.href, + error, + }); + }
🧹 Nitpick comments (1)
src/activitypub/fediverse-bridge.ts (1)
81-99: Add actor and activity identifiers to error log
Extend the logger payload withactorApId,activityId, andactivityTypefor easier correlation.- this.logger.error('Failed to federate activity to followers: {error}', { - accountId: account.id, - accountUsername: account.username, - error, - }); + this.logger.error('Failed to federate activity to followers: {error}', { + accountId: account.id, + accountUsername: account.username, + actorApId: account.apId, + activityId: activity.id?.href, + activityType: (activity as any).type ?? activity.constructor?.name, + error, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/activitypub/fediverse-bridge.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash-based lookups instead
Use Result helper functions (isError, getError, getValue) and avoid destructuring Result values directly
Awilix uses CLASSIC injection: constructor parameter names must match their registration names
Define routes using decorators rather than direct registration
Prefer immutable entities that emit domain events over mutable entities with dirty flags
Use structured error objects with context in Result types instead of string literals
Prefer class-based handlers with dependency injection over function factories
Files:
src/activitypub/fediverse-bridge.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
| Reject, | ||
| Update, | ||
| } from '@fedify/fedify'; | ||
| import type { Logger } from '@logtape/logtape'; |
There was a problem hiding this comment.
🧩 Analysis chain
Logger DI: type‑only import looks good; verify Awilix CLASSIC registration name is logger.
Type import is correct. Ensure the container registers a dependency named exactly logger (CLASSIC injection is name‑based) and update tests/mocks accordingly.
Also applies to: 28-33
🏁 Script executed:
#!/bin/bash
# Verify Awilix registration for `logger`
rg -nP --type=ts -C3 '\bregister\(\{[^}]*\blogger\b'
rg -nP --type=ts -C3 '\blogger\s*:\s*(?:asClass|asFunction|asValue)\('
# Also list DI/container files for manual inspection
fd -t f -e ts -a | rg -nP '(awilix|container|di|ioc)'Length of output: 847
🏁 Script executed:
#!/bin/bash
# Inspect Awilix container registration for `logger`
rg -nP 'register\(\s*\{' -C5 src/configuration/container.ts
rg -nP '\blogger\s*:' -C3 src/configuration/container.ts
# Check if loadModules is used instead of register
rg -nP 'loadModules' -C5 src/configuration/container.tsLength of output: 0
Register logger in the DI container
No logger registration found in src/configuration/container.ts—add a classic Awilix registration named exactly logger (e.g.
container.register({
logger: asClass(LogTapeLogger).singleton()
});) and update tests/mocks accordingly.
🤖 Prompt for AI Agents
In src/activitypub/fediverse-bridge.ts around line 11 the code expects a DI
binding named "logger" but no registration exists; open
src/configuration/container.ts and add a classic Awilix registration named
exactly logger (e.g. register the real LogTape logger with
asClass(...).singleton()) so the container provides the dependency, then update
test setup/mocks to register a mock or stub logger under the same name (or use
container.register in tests with asValue(mockLogger)) so all imports that expect
Logger resolve correctly.
1 participant
When USE_MQ=false, federation happens synchronously during the HTTP request. If delivery to any follower fails (e.g., deleted accounts, down servers), it causes the entire request to return 500 even though the action itself succeeded.