You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Await sibling deliveries after a fan-out failure - #1388
The immediate delivery paths in sendActivity() and forwardActivity() used Promise.all() alone. If one recipient failed, the method rejected while the other deliveries were still running. On Cloudflare Workers the isolate can be terminated at that point, which can cancel the remaining deliveries.
This change makes both paths drain every delivery they have already started before propagating the original error.
In packages/fedify/src/federation/middleware.ts, the fan-out loop and Promise.all() in FederationImpl.sendActivity() and InboxContextImpl.forwardActivityInternal() now sit inside a try block, with Promise.allSettled() in finally. The original rejection still propagates after the siblings settle.
Each started promise gets a no-op catch() so an early rejection cannot surface as an unhandled rejection while a later recipient's Collection-Synchronization header is being prepared.
packages/fedify/src/federation/middleware.test.ts adds regression tests for the two immediate paths and for a failure while preparing a later recipient. The tests gate the second delivery and assert that the call is still pending while the first one has already failed.
changes.d/fedify/await-sibling-deliveries.md records the fix, and CHANGES.md is materialized from it.
Benefits
Already-started sibling deliveries now settle before the caller receives the original error. Queue-backed delivery is unchanged, and the error that callers see is still the original one.
The immediate fan-out paths in FederationImpl.sendActivity() and
InboxContextImpl.forwardActivityInternal() used Promise.all() alone.
When one delivery rejected, the method returned while the remaining
deliveries were still in flight, so Cloudflare Workers could terminate
the isolate and cancel them.
Put the fan-out loop and Promise.all() in a try block and drain the
started promises with Promise.allSettled() in finally. The original
error still propagates after the siblings settle, and a synchronous
failure while preparing a later recipient no longer abandons earlier
deliveries. Attach a no-op catch to each started promise so an early
rejection cannot surface as an unhandled rejection while a later
Collection-Synchronization header is being built.
Add regression tests for the sendActivity() and forwardActivity()
immediate paths and for a fan-out setup failure. Document the fix in
the 2.0.36 changelog.
fedify-dev#1380
Assisted-by: OpenCode:deepseek-flash
Assisted-by: Codex:gpt-6-astra
Assisted-by: Claude Code:claude-fable-5-1
Assisted-by: OpenCode:gemini-4-argon
No actionable comments were generated in the recent review.
Walkthrough
Immediate activity sends and forwards now wait for started deliveries to settle before propagating an error. Regression tests cover recipient failures and a fan-out setup failure. Changelog entries describe the behavior.
Changes: Immediate delivery handling
Layer / File(s)
Summary
Drain started deliveries before error propagation packages/fedify/src/federation/middleware.ts, packages/fedify/src/federation/middleware.test.ts, CHANGES.md, changes.d/fedify/await-sibling-deliveries.md
Immediate sends and forwards attach rejection handlers as deliveries start and drain started deliveries before propagating errors. Tests cover recipient failures and fan-out setup failure. Changelog entries document the behavior and link issue #1380 and pull request #1388.
Immediate deliveries are intended to finish before an error is returned. No actionable issue remains from this review; normal checks can proceed.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly and concisely describes the main change: waiting for sibling deliveries after a fan-out failure.
Description check
The description directly explains the delivery failure issue, the implementation, the tests, and the changelog updates. It is fully related to the changeset.
Linked Issues check
Issue #1380 requires that the immediate sendActivity() and forwardActivity() paths wait for every already-started delivery before propagating the original error. The head diff of packages/fedify/src/f…
Out of Scope Changes check
All changed files map to the objective. middleware.ts holds the fan-out fix in both immediate paths (FederationImpl.sendActivity and InboxContextImpl.forwardActivityInternal). middleware.test.ts holds…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Linked Issues check
Explanation
Issue #1380 requires that the immediate sendActivity() and forwardActivity() paths wait for every already-started delivery before propagating the original error. The head diff of packages/fedify/src/federation/middleware.ts shows both paths now place the fan-out loop and Promise.all() inside try, with await Promise.allSettled(promises) in finally. This matches the fix in the issue. The loop sits inside try, so a synchronous throw while preparing a later recipient also drains earlier deliveries (issue acceptance criterion). Each started promise gets promise.catch(() => {}), which does not alter the rejection seen by Promise.all(). The issue's regression test requirement (rejecting and delayed destination, assert delayed delivery settled before rejection) is covered by middleware.test.ts additions described in the PR summary. Verification limit: the middleware.test.ts diff could not be read (changed blob unavailable), and the file read was truncated before the new tests. Test content is therefore supported only by the summary and PR description, not by direct reading. Queue-backed delivery is unchanged in the diff. No non-coding requirements exist in the issue.
Full details: Out of Scope Changes check
Explanation
All changed files map to the objective. middleware.ts holds the fan-out fix in both immediate paths (FederationImpl.sendActivity and InboxContextImpl.forwardActivityInternal). middleware.test.ts holds regression tests for those paths. changes.d/fedify/await-sibling-deliveries.md and CHANGES.md record the fix as a changelog entry. Changelog and test changes support the fix and do not introduce unrelated behavior. The diff shows no public API change, consistent with the issue scope. The test file content was not directly readable, so this verdict relies on the PR summary for the test changes.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files. (2 skipped: 2 unsupported.)
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Repository UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 264bece9-fb9d-4018-84b6-4dd4f71e5f74
Commits
Reviewing files that changed from the base of the PR and between 90504ae and 37beba8.
Files selected for processing: 4
CHANGES.md
changes.d/fedify/await-sibling-deliveries.md
packages/fedify/src/federation/middleware.test.ts
packages/fedify/src/federation/middleware.ts
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
dahlia
linked an issue
Nov 5, 2026
that may be
closed
by this pull request
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dahlia
added
component/federation
Summary
The immediate delivery paths in
sendActivity()andforwardActivity()usedPromise.all()alone. If one recipient failed, the method rejected while the other deliveries were still running. On Cloudflare Workers the isolate can be terminated at that point, which can cancel the remaining deliveries.This change makes both paths drain every delivery they have already started before propagating the original error.
Fixes #1380. Targets the
2.0-maintenancebranch.Changes
Promise.all()inFederationImpl.sendActivity()andInboxContextImpl.forwardActivityInternal()now sit inside atryblock, withPromise.allSettled()infinally. The original rejection still propagates after the siblings settle.catch()so an early rejection cannot surface as an unhandled rejection while a later recipient'sCollection-Synchronizationheader is being prepared.Benefits
Already-started sibling deliveries now settle before the caller receives the original error. Queue-backed delivery is unchanged, and the error that callers see is still the original one.