Repository navigation
fix(#5135): an owned connection's release terminates attached subscribers; one upload ceiling - #5647
Conversation
…bers; one upload ceiling for every transport AutoConnectOwnedBy refused subscribers arriving after the owner's release, but a subscriber already attached while the one-shot was in flight got nothing: disposing the connection detaches the replay subject from its upstream and emits no terminal. An MCP upload waiting on ContentService.GetCollection when its hub tore down waited forever. The owned handle now fires a release terminal (ObjectDisposedException, on the ThreadPool) only when the OWNER is disposed; a settled chain that drops its handle keeps replaying. UploadLimits (MeshWeaver.Mesh.Operations) is the one upload ceiling (200 MiB decoded). POST /api/mesh/upload now declares its request-body limit from it — FormOptions alone left Kestrel's 30 MB default in force — and MeshOperations.Upload refuses a larger payload by name. The /mcp half follows in MeshWeaver.Plugins. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved owned-connection behavior, documentation inaccuracies, localization concerns, and missing operation-level coverage remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (4)
What changed in this PR
Fixes silent subscriber teardown and centralizes the 200 MiB upload ceiling for REST uploads.
Changes:
- Adds owner-release termination signaling and regression tests.
- Applies shared upload limits to REST metadata and operation validation.
- Updates disposal and upload documentation.
| File | Summary |
|---|---|
test/MeshWeaver.Messaging.Hub.Test/OwnedConnectionTest.cs |
Tests connection-release behavior. |
test/MeshWeaver.ContentCollections.Test/InFlightCollectionReleasedWithItsHubTest.cs |
Tests collection teardown during initialization. |
test/Memex.Portal.Shared.Test/MeshApiUploadBodyTest.cs |
Tests REST route size metadata. |
src/MeshWeaver.Messaging.Hub/OwnedConnectionExtensions.cs |
Propagates owner-release terminals. |
src/MeshWeaver.Mesh.Operations/UploadLimits.cs |
Defines shared upload limits and refusal handling. |
src/MeshWeaver.Mesh.Operations/MeshOperations.cs |
Rejects oversized uploads. |
src/MeshWeaver.Documentation/Data/DataMesh/UnifiedPath.md |
Documents upload limits. |
src/MeshWeaver.Documentation/Data/Architecture/HubDisposalModel.md |
Documents disposal semantics. |
memex/Memex.Portal.Shared/Api/MeshApiEndpoints.cs |
Applies the REST request-body limit. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// and every subscriber — one still attached when the release happens as much as one arriving after | ||
| /// it — terminates with <see cref="ObjectDisposedException"/> instead of being parked on a replay | ||
| /// that nothing will ever feed (#5135: the attached half was silent before). |
There was a problem hiding this comment.
Narrowed, in 5e9b544. The class summary now scopes the attached-subscriber guarantee to the shared observable AutoConnectOwnedBy hands out. It also says ConnectOwnedBy releases the upstream only, because its readers subscribe to the connectable itself and this type never wraps it. Extending the terminal to ConnectOwnedBy would need a wrapper around every eager feed's read path. That is a separate change with its own callers, and it is out of scope for #5135.
| its request-body limit from it for its own encoding — REST `POST /api/mesh/upload` the file plus its | ||
| multipart envelope, the MCP `/mcp` endpoint the file's base64 length plus the JSON-RPC envelope — so | ||
| a request inside the ceiling is never cut at Kestrel's 30 MB default, and a larger file is refused | ||
| by name (`Error: the file is N bytes; the upload ceiling is …`). Both transports buffer the whole | ||
| file in memory, and base64 adds a third; prefer the REST route for large files. |
There was a problem hiding this comment.
Fixed in 5e9b544. The paragraph now says the REST route declares the limit. It says /mcp is mapped in MeshWeaver.Plugins and takes its limit from the same constant in its own change. It also says that on an image without that change, /mcp still carries Kestrel's 30 MB default, which is about 22 MB of file.
| public static string TooLarge(long bytes) | ||
| => $"Error: the file is {bytes:N0} bytes; the upload ceiling is {MaxUploadBytes:N0} bytes " | ||
| + $"({MaxUploadBytes / (1024 * 1024)} MiB). Larger files need a streaming ingest path, not this tool."; |
There was a problem hiding this comment.
Numbers fixed in 5e9b544: the message is now built with string.Create(CultureInfo.InvariantCulture, …) and no N0, so it no longer depends on the process culture. Not localized, on purpose. This is an Error: … tool/API sentinel like every other MeshOperations result. It is read by a model or a script (the MCP tool result, the REST body) and never rendered as UI. AGENTS.md's i18n rule excludes model-facing text. The remark on TooLarge now says so.
| /// error. Each route now declares a body limit computed from <see cref="MaxUploadBytes"/> for its | ||
| /// own encoding, and <see cref="MeshOperations.Upload"/> refuses a larger payload by name.</para> |
There was a problem hiding this comment.
Fixed in 5e9b544. The XML doc now describes the REST declaration as current. It describes Base64JsonRequestBodyLimit as the same ceiling for the MCP endpoint, which MeshWeaver.Plugins maps and declares in its companion change.
…tOwnedBy; the /mcp limit is the Plugins half; invariant refusal numbers Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
🔀 Merge-queue steward: re-queued alone (bisecting) — bisecting: your group of 3 pull requests failed on an uncatalogued assertion while this PR's own run was green; re-queued ALONE — if the solo build fails too, the PR stays out (group: #5646, #5640).
enqueued |
Refs #5135 — the two remaining code items in core. The
/mcpbody limit follows in MeshWeaver.Plugins once this is in a sealed set (it referencesUploadLimits).1. A subscriber attached when an owned connection is released was parked forever
OwnedConnectionExtensions.AutoConnectOwnedByrefused subscribers that arrived after the owner's release. A subscriber already attached while the one-shot was still running got nothing: disposing the connection detaches the replay subject from its upstream and emits no terminal to its observers. Nothing upstream of theReplaycan see an unsubscription, soContentService's evictionCatchnever ran either.This is the silent leg in
upload. An MCP upload waits onContentService.GetCollectionwhile the collection's initial scan runs. If the hub it rides tears down during that wait, the upload gets no value, no error and no completion.Fix. The owned handle fires a release terminal,
ObjectDisposedException(ownerName), the same exception a late subscriber already gets. It fires only when the owner is disposed. A settled chain that drops its handle from a live owner keeps replaying its value. The terminal is delivered on the ThreadPool, never on the disposing hub's turn, which is the same trade as the IoPool refusal (#4530).What the issue's other suspects turned out to be (read from source):
IStreamProviderFactory.Create(FileSystem, EmbeddedResource, AzureBlob) returnsObservable.Return.RequestTimeout(per the 2026-09-22 comment).ContentCollection.Initialize's pool leaf ends in a value or a fault. A cancelled or refused leaf errors.So none of them can be silent unless a provider's I/O hangs forever. The owned-connection release was the one structural silence.
Tests (negative control run by hand, both fail on the timeout without
TakeUntil(released)):OwnedConnectionTest.ASubscriberAttachedWhileInFlight_IsTerminatedByTheReleaseInFlightCollectionReleasedWithItsHubTest: a realContentServicewith a parked scan, torn down mid-creation.OwnedConnectionTest.AConnectStillQueuedWhenTheOwnerIsReleased_NeverRunsnow asserts the queued subscriber gets the refusal. It used to pin the silence as correct.ASettledPromise_KeepsReplaying_AfterItsHandleLeavesALiveOwnerguards the other direction.2. One upload ceiling, declared on the route
UploadLimits(MeshWeaver.Mesh.Operations) sets the ceiling at 200 MiB of decoded bytes.POST /api/mesh/uploadnow declares its request-body limit from it viaIRequestSizeLimitMetadata.FormOptions.MultipartBodyLengthLimitalone never lifted Kestrel's 30 MB whole-request default, so the documented 200 MB was unreachable.MeshOperations.Uploadrefuses a larger payload by name.Test:
MeshApiUploadBodyTest.The_upload_route_declares_the_shared_upload_ceiling. The negative control, run by hand without.WithMetadata, fails.Local verification
dotnet build -c Release -warnaserror, one project at a time, 0 errors: Messaging.Hub, Mesh.Operations, Memex.Portal.Shared, ContentCollections.Test, Messaging.Hub.Test, Memex.Portal.Shared.Test.MeshApiUploadBodyTest6/6,PackageListingCache*25/25.QueryConnections*2/2.*Guard*363/363.Not established
/mcpstill has no declared limit until the Plugins half lands.AsCallerseam is a design question and is not touched here (stated on the issue).Pairs-with: none — additive only (a new public type
UploadLimits; no public surface removed)🤖 Generated with Claude Code