feat(container-loader): add readOnly option to loadFrozenContainerFromPendingState

[anthony-murphy]

Description

Adds an optional readOnly parameter (default true, preserving existing behavior) to loadFrozenContainerFromPendingState and createFrozenDocumentServiceFactory. When readOnly: false, the frozen container loads as writable so the runtime accepts DDS submissions and accumulates them in pendingStateManager for getPendingLocalState() to capture — without publishing them.

The mechanism: a sibling WritableFrozenDeltaStream is the live connection (claims include DocWrite, mode stays "read"). ConnectionManager.sendMessages matches it via isWritableFrozenDeltaStreamConnection and short-circuits before the read-mode reconnect branch — outbound writes are dropped at the network layer instead of triggering a reconnect (which under ReconnectMode.Never would close the container).

Other notable changes:

• FrozenDeltaStream is split into two sibling classes (FrozenDeltaStream read-only, WritableFrozenDeltaStream writable) over a shared abstract base. Both internal-only. Type discrimination at consumer call sites uses predicate functions (isFrozenDeltaStreamConnection, isWritableFrozenDeltaStreamConnection) — no raw instanceof.
• WritableFrozenDeltaStream mints a per-instance frozen-delta-stream/<uuid> clientId to avoid pendingStateManager 0x173 (replayPendingStates called twice for same clientId!) on reconnect with dirty pending ops. The read-only variant keeps the historical constant "storage-only client".
• submitSignal is now a silent no-op on both variants (was a 403 nack on the read-only variant). Signals are ephemeral and dropping is the right behavior with no upstream; the read-only variant's submit stays defensive (403 nack) since ops are durable state.
• createFrozenDocumentServiceFactory rewraps an already-wrapped factory if its readOnly differs from the caller — most recent intent wins.

API report and a changeset are included.

Reviewer Guidance

• vladsud sign-off welcome on the ConnectionManager.sendMessages WritableFrozenDeltaStream short-circuit — sits in front of Remove two types of nacks - "Nonexistent client" & "Readonly client" #7753's read-mode invariant.
• ChumpChief sign-off welcome on the @legacy @alpha shape (readOnly?: boolean polarity / positional arg on createFrozenDocumentServiceFactory).
• jatgarg (optional) review of the FrozenDeltaStream constructor reshape at her two historical call sites.

The submit / submitSignal asymmetry on FrozenDeltaStream is intentional: ops are durable (nack to surface invariant breaks); signals are ephemeral (drop).

fixes AB#72108

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (334 lines, 4 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

Toggle the reviewer checkboxes above to adjust, then tick the box below to start:

• Start review

[anthony-murphy]

Review fixes — addressed in e771980

Picking up the four findings from the Deep Review (only one was an inline thread; the other three were summary-only):

1 + 3: submitSignal payload shape and writable failure mode

Resolved together by changing submitSignal to a silent no-op for both variants. The old behavior was wrong on two axes:

• The read-only variant emitted a non-array nack payload ({ operation, content }), and nackHandler does messages[0] — that would crash before the diagnostic could surface, so the "loud failure" intent wasn't actually achievable.
• The writable variant nacking on a stray runtime/presence signal would close the container (when _readonlyPermissions is true) or trigger reconnect spam (when false). Either is strictly worse than dropping.

Signals are ephemeral by nature; a frozen container has no upstream; dropping is the correct behavior. submit stays defensive because ops are durable state and we want to surface invariant breaks for those. JSDoc updated to spell out the asymmetry.

Regression test added: submitting a signal does not close the container — calls runtime.submitSignal post-load and asserts the container stays open and undisposed.

2: Wrapped-factory readOnly mismatch

See inline reply on the existing thread. Short version: rewrap on mismatch, with a regression test.

4: getPendingLocalState() round-trip test

Added: captures local writes in getPendingLocalState() and round-trips through a second frozen load — N writable DDS sets → capture pending state → load a second writable-frozen container from that blob → assert the layered edits are visible alongside the original snapshot's data. This is the load-bearing invariant for the whole feature, and the test was a real gap.

Other

Stale "drops them silently" comment in the existing accepts local writes test is now corrected to describe the actual mechanism (ops accumulate in pendingStateManager; the upgrade-hang in connectToDeltaStream is what blocks the wire).

Holding the polish items (close-vs-dispose hung-promise leak, dispose-test gap on the hung promise, readonlyConnectionReason dropped on writable path) for follow-up — none of them affect correctness in current usage. PR description update to follow.

[anthony-murphy]

Deep Review: Thanks for the rapid turnaround on e771980 — the submitSignal no-op (with submit staying defensive), the wrapped-factory rewrap-on-mismatch, and the getPendingLocalState() round-trip test all land cleanly and address the prior findings. The asymmetry rationale you wrote up (signals ephemeral → drop; ops durable → nack to surface invariant breaks) is the right call.

One new concern surfaced this round, posted as an inline thread on frozenServices.ts: the writable initial connect hangs for allowReconnect: false and non-interactive callers, because Container.connectToDeltaStream forces args.mode = "write" on the very first connect — bypassing the client.mode !== "write" early return that hands out the writable FrozenDeltaStream. The new tests don't cover this combination. Details in the thread; the updated review summary above has the path-to-ready.

[anthony-murphy]

Tier 1 finding addressed in 411863f

The dispose/close-during-hung-upgrade integration tests were vacuous after the sendMessages short-circuit landed — confirmed. Restored real coverage via focused unit tests in `packages/loader/container-loader/src/test/frozenServices.spec.ts` that drive `FrozenDocumentService.connectToDeltaStream({mode: "write"})` directly, then assert dispose() rejects the hung promise. Three cases:

• `rejects the hung upgrade-connect when service.dispose() is called` — flips `handedOutInitialConnection` via an initial read connect, then captures the second (write-mode) connect's promise, asserts it stays pending across microtask flushes, calls `service.dispose()`, asserts it rejects with the expected message.
• `rejects synchronously when called after service.dispose()` — pins the disposed-first branch (`if (this.disposed) reject(...)`) of the upgrade-hang.
• `drains multiple pending rejecters on a single dispose() call` — three concurrent hung upgrade promises, single dispose, asserts all three reject.

Picked option (b) — focused unit test against a directly-constructed `FrozenDocumentService` — over option (a) — driving connectToDeltaStream from the integration test — because (b) is lighter weight and doesn't require accessing the underlying `IDocumentService` from inside the integration container.

Also addressed Tier-3 polish in the same commit:

• Changeset reconciled. Replaced the "first runtime submit triggers an internal read→write upgrade attempt that cannot succeed" / "settles into Disconnected" narrative with the actual flow (sendMessages drops outbound at the FrozenDeltaStream short-circuit; container stays Connected). Added an explicit call-out for the submitSignal read-only-variant behavior change (was 403 nack, now silent no-op for both variants — same rationale: signals are ephemeral and dropping is the right behavior with no upstream).
• Test docstrings reconciled. Renamed `dispose()/close() while the read→write upgrade connect is hung` → `dispose()/close() runs cleanly on a writable-frozen container after a local write`, and `captures writes performed after the upgrade-connect has hung` → `captures writes batched across timer/microtask boundaries`. Inline comments updated to describe the actual short-circuit-first flow and cross-link the new unit test for the rejecter coverage.
• PR description was already updated in ff37f19 to match the short-circuit-first design.

Held for the next iteration / human reviewer:

• vladsud signoff on the connectionManager hunk (would prefer their judgment on `instanceof` vs a `policies`-style flag for refactor resilience)
• Summarizer / id-compressor / blob JSDoc note on `readOnly?: boolean` — small addition; happy to do this in a follow-up commit if the next review still flags it
• Flag-polarity decision — already JSDoc'd with rationale (alignment with `IContainer.readOnlyInfo.readonly`); awaiting human signoff

[anthony-murphy]

Deep Review: Thanks for the rapid turnaround on 411863f — confirmed:

• The focused unit tests in frozenServices.spec.ts (rejects the hung upgrade-connect when service.dispose() is called, rejects synchronously when called after service.dispose(), drains multiple pending rejecters on a single dispose() call) restore real coverage of the pendingConnectRejecters drain. Driving connectToDeltaStream({mode: "write"}) directly against a constructed FrozenDocumentService is the lighter-weight option (b) and gets meaningful assertions back — the integration-test version was always going to be awkward once the sendMessages short-circuit prevented the upgrade from firing.
• Changeset reconciliation matches the actual flow now (sendMessages short-circuit; container stays Connected; submitSignal read-only-variant 403→no-op called out explicitly).
• Test docstring renames (...while the read→write upgrade connect is hung → ...runs cleanly on a writable-frozen container after a local write, and the timer/microtask-batching rename) match what the tests actually exercise post-short-circuit.

One new concern surfaced this round, posted as an inline thread on frozenServices.ts:133-137: the writable-variant reconnect branch (client.mode !== "write" → fresh FrozenDeltaStream({readOnly:false})) reuses the constant "storage-only client" clientId across instances, which will trip pendingStateManager's 0x173 (replayPendingStates called twice for same clientId!) the first time a writable-frozen container reconnects with dirty pending ops. The fix is either per-instance clientId minting (mirrored in initialClients) or — if the branch is dead-by-construction now that sendMessages short-circuits the read-mode upgrade — dropping/asserting against the branch entirely. Details, evidence, and the open design question in the inline thread; updated review summary above has the path-to-ready.

[anthony-murphy]

Quick status note on the 8/10 verdict on ce7f4ed:

The "held subsystem JSDoc note" item in Path to Ready is already addressed in ce7f4ed4d1. See createAndLoadContainerUtils.ts:265-272:

Subsystem behavior is unchanged from the read-only frozen path regardless of readOnly: storage operations still throw (only readBlob is supported); summarizer / id-compressor never fire because no acks arrive; the quorum is whatever was captured in pending state and gains no members during the writable-frozen lifetime. The only behavioral delta is that the runtime accepts DDS submissions and accumulates them in pendingStateManager.

Remaining items from the Path to Ready are all on the human reviewer side:

• Human sign-offs: vladsud (connectionManager short-circuit), ChumpChief (@legacy @alpha polarity / arg order), dannimad (frozen-container test pattern), jatgarg (optional, constructor refactor at her two call sites).
• Optional Tier-3 follow-ups I am holding: focused mocked unit test for the sendMessages instanceof FrozenDeltaStream && !readOnly early-return (requires a ConnectionManager test double — agreed it would tighten regression detection but is heavier than the integration coverage already in place); noDeltaStream.spec.ts filename rename (cosmetic, file covers paths beyond just FrozenDeltaStream).

All 7 prior Deep Review iterations have closed; no fresh correctness findings on this commit. Standing down on autonomous changes until human review lands.

[Copilot]

Pull request overview

This PR adds a readOnly?: boolean option (defaulting to true) to loadFrozenContainerFromPendingState and createFrozenDocumentServiceFactory, enabling a new “writable frozen” load mode where the runtime can accept local DDS submissions while still preventing any outbound ops from reaching a real service.

Changes:

• Add readOnly?: boolean plumbing for frozen-from-pending-state loads and factory wrapping (default preserves existing read-only behavior).
• Split frozen delta stream behavior into read-only vs writable variants, and add a ConnectionManager.sendMessages short-circuit to drop outbound messages for the writable-frozen connection.
• Add targeted unit + local-server tests for the writable-frozen flow and regression coverage (pending state accrual, reconnect safety, signals, etc.).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
packages/test/local-server-tests/src/test/loadFrozenContainerFromPendingState.spec.ts	Adds extensive local-server coverage for readOnly: false flow, reconnects, pending state round-trip, and lifecycle behaviors.
packages/loader/container-loader/src/test/frozenServices.spec.ts	New unit tests for frozen delta stream nack/no-op behavior and per-instance clientId uniqueness for writable connections.
packages/loader/container-loader/src/frozenServices.ts	Introduces writable frozen delta stream variant + per-instance clientId; adds readOnly wrapping behavior to factory/service.
packages/loader/container-loader/src/createAndLoadContainerUtils.ts	Extends frozen-load props with readOnly?: boolean and forwards to frozen document service factory wrapper.
packages/loader/container-loader/src/connectionManager.ts	Adds writable-frozen sendMessages short-circuit; updates FrozenDeltaStream construction to new options shape.
packages/loader/container-loader/api-report/container-loader.legacy.alpha.api.md	Updates legacy alpha API report for the new optional readOnly parameter.
.changeset/frozen-container-readonly-option.md	Adds release note + minor bump for the new option and behavior changes.

[github-actions]

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output

> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  288641 links
    1922 destination URLs
    2172 URLs ignored
       0 warnings
       0 errors

[anthony-murphy]

Deep Review

Reviewed commit 102c584 on 2026-05-06.

Readiness: 10/10 — READY

Sign-off ready. No Tier 1–3 findings on 102c584; same code position as the a3360ee review with the surface concerns (assert(!isWritableFrozenDeltaStreamConnection) invariant guard, as ITokenClaims casts, option polarity) reading as soft polish rather than blockers. Remaining motion is the human sign-offs the PR description already requests — vladsud on the ConnectionManager.sendMessages short-circuit, ChumpChief on the @legacy @alpha API shape, jatgarg (optional) on the constructor reshape at her two historical call sites.

Context for Reviewers

• The WritableFrozenDeltaStream short-circuit in sendMessages is the first deliberate exception to vladsud's Remove two types of nacks - "Nonexistent client" & "Readonly client" #7753 read-mode-no-submits invariant. Ordering is the entire correctness story — isWritableFrozenDeltaStreamConnection at connectionManager.ts:1093-1120 MUST stay above the read-mode reconnect branch; under ReconnectMode.Never the read-mode branch would call closeHandler and tear the container down on the first DDS write. Tripwire comment is in place; no automated enforcement. The branch is gated to isWritableFrozenDeltaStreamConnection (not FrozenDeltaStream) so the read-only variant retains its submit 403-nack tripwire.
• clientId ownership change is asymmetric by design. Read-only variant keeps the constant "storage-only client" (hard-coded in noDeltaStream.spec.ts); only WritableFrozenDeltaStream mints a per-instance frozen-delta-stream/<uuid>. Targets the pendingStateManager 0x173 (replayPendingStates called twice for same clientId!) assert that only fires on the writable path when reconnecting with dirty pending ops. Disconnect/reconnect integration test pins this. Inline JSDoc captures the rationale; redirect any "give every frozen connection a uuid clientId for symmetry" suggestions there.
• initialClients mirroring is load-bearing. PR Simplified model for Audience; Leverage join signal's referenceSequenceNumber for catch up logic #12164 (vladsud, 2022-11-13) made the connected transition occur only after "self" appears in Audience. The frozen-stream base mirrors each synthetic clientId into initialClients so the audience handler observes self without a real join signal; the new unit test asserts each writable stream's initialClients[0]?.clientId matches its fresh clientId.
• allowReconnect: false and non-interactive-client first-connect interaction is also covered by the sendMessages short-circuit. Container.connectToDeltaStream forces mode = "write" on the very first connect under those conditions; the same short-circuit catches both the runtime upgrade path and the ReconnectMode.Never close path with a single early return. Integration tests pin both allowReconnect: false and non-interactive-client variants.
• submitSignal is now a silent no-op for both variants (was 403 nack on the read-only variant in ContainerLoader: Load pending state as a frozen container #25538). Documented in the changeset; signals are ephemeral and dropping is the right behavior with no upstream. submit stays defensive (403 nack on the read-only variant) — ops are durable state, a stray submit signals an invariant break.
• Writable-frozen intentionally diverges from Lie to ContainerRuntime about connected state when in readonly mode #11655's "lie disconnected" approach. Container stays Connected, runtime submits, sendMessages drops at the network layer. The new forceReadonly(true) integration test pins the interaction with Lie to ContainerRuntime about connected state when in readonly mode #11655's mechanism — a writable-frozen container with forceReadonly(true) surfaces as readonly.
• Type-system enforcement of readOnly-only options matches the author's own established review standard. On PR Handle sessoionforbidden error and add storageOnlyReason to Readonly info #15907, anthony-murphy pushed back on as casts in this area. The sibling-class split (FrozenDeltaStream / WritableFrozenDeltaStream) plus isFrozenDeltaStreamConnection / isWritableFrozenDeltaStreamConnection predicates and the explicit "no raw instanceof checks" PR-description note directly mirror that standard. The surviving as ITokenClaims casts at frozenServices.ts:173-184,296-307 are explicitly bracketed by JSDoc rationale.
• @legacy @alpha shape extends an existing props bag. PR ContainerLoader: Move getPendingLocalState to legacy/alpha #25513 review (ChumpChief, 2025-09-24): "how does this scale if there's a second thing that we want to export an alpha version for?" — anthony-murphy: "it should become an overload". The PR adds readOnly?: boolean to the existing ILoadFrozenContainerFromPendingStateProps rather than introducing a new function; positional-arg createFrozenDocumentServiceFactory(readOnly?, factory?) is the asymmetry worth scrutiny.
• Rewrap-on-mismatch and per-instance UUID are extra defensive depth. Independent design proposals missed both hazards: createFrozenDocumentServiceFactory unwraps and rewraps when an already-wrapped factory's readOnly differs from the caller's argument so the most recent intent wins; the per-instance writable clientId defends against pendingStateManager 0x173 on reconnect with dirty pending ops.
• Writable-frozen is a scoped opt-in layered on the original storage-only design. PR ContainerLoader: Load pending state as a frozen container #25538 introduced FrozenDocumentServiceFactory / FrozenDocumentService / FrozenDeltaStream for read-only, storage-only frozen loads. This PR preserves that default while adding readOnly?: boolean and gates the non-storage-only policy to readOnly: false.

For human reviewer

• vladsud sign-off on the ConnectionManager.sendMessages short-circuit (packages/loader/container-loader/src/connectionManager.ts:1093-1120) — first deliberate exception to his Remove two types of nacks - "Nonexistent client" & "Readonly client" #7753 read-mode-no-submits invariant. Verify (a) the writable-frozen short-circuit precedes every submit-path branch, (b) whether the tripwire comment is sufficient or an assert(!isWritableFrozenDeltaStreamConnection(this.connection)) guard above the read-mode reconnect branch is wanted, (c) the read-only FrozenDeltaStream 403-nack tripwire remains observable. Also: confirm intended forceReadonly(true) × writable-frozen fall-through semantics pinned by the new test.
• ChumpChief sign-off on the @legacy @alpha API shape — readOnly?: boolean polarity (negative, default true) on ILoadFrozenContainerFromPendingStateProps; positional-arg vs. options-bag asymmetry on createFrozenDocumentServiceFactory(readOnly?, factory?); whether overloading loadFrozenContainerFromPendingState({ readOnly }) is the right axis or the writable case warrants its own entry point (the ContainerLoader: Move getPendingLocalState to legacy/alpha #25513 "second thing" question now exercised). Both independent proposals chose positive polarity (loadWritable / allowLocalEdits); the JSDoc explicitly notes a positive-polarity option "can layer on top later without breaking callers."
• jatgarg (optional) review of (a) the FrozenDeltaStream constructor positional → options-bag refactor at her two historical call sites (Handle sessoionforbidden error and add storageOnlyReason to Readonly info #15907 storageOnlyReason, Handle out of storage error from driver and make the container loadable #17131 readonlyConnectionReason); confirm the readonly event payload still carries error.errorType === DriverErrorType.outOfStorage end-to-end after the reshape; (b) the read-only variant losing its submitSignal 403-nack tripwire on the storage-only / forbidden / out-of-storage paths.
• Sibling-class split vs single class with mode flag — taste call. PR has documented rationale (type-system narrowing of read-only-only options + instanceof predicate excluding the writable variant). Independent design proposals both chose a single-class + flag shape; PR's choice preserves a type-discriminant invariant.
• Locus of decoupling — connection-layer (PR's choice) vs Container-layer (localEditsOnly field, canSendOps independent of readonly). Both achieve the same observable behavior; PR's blast radius is smaller.
• Persisted as ITokenClaims cast pattern at frozenServices.ts:173-184 and :296-307. A reviewer who wants a makeFrozenClaims(scopes) helper rather than the eslint-disable bracketing should call it out explicitly. No current-correctness bug — only claims.scopes is read downstream.
• submitSignal harmonization to silent no-op on the read-only variant — pre-existing public path; documented in the changeset. Reviewers wanting a positive consumer audit (grep results) rather than a negative claim ("no known consumer") can call it out.
• Cannot be assessed by the pipeline — whether the disconnect/reconnect regression test currently passes in CI on 102c584; runtime behavior of presence/signal subsystems on a writable-frozen connection over time; cross-fork determinism of the per-instance UUID clientId under concurrent reconnects; production drift in PendingStateManager / Outbox ordering that would change the "ops are durable before sendMessages drops them" invariant.

Review history (10 prior reviews)

• a3360ee 2026-05-05 · 10/10 — sign-off ready; no Tier 1–3 findings; soft-polish items survive as Tier 4
• 4ddd5c1 2026-05-05 · 10/10 — sign-off ready; no Tier 1–3 findings; remaining motion is human sign-offs
• 2035078 2026-05-05 · 9/10 — no Tier 1–3 findings; gap to 10/10 attributed to pending human sign-offs
• 86c509a 2026-05-05 · 9/10 — no Tier 1–3 findings; gap to 10/10 is human sign-offs
• ac44bd1 2026-05-04 · 8/10 — three new Tier-3 polish items (stale PR-description paragraph, policies.storageOnly consumer audit, forceReadonly(true) pinning test)
• e93eb26 2026-05-04 · 8/10 — design holds on re-review; new disposed-asymmetry inline thread; polish + sign-offs remaining
• ce7f4ed 2026-05-01 · 8/10 — short-circuit narrowing to writable-only and per-instance-UUID unit test landed; surviving items polish + sign-offs
• 849c0ed 2026-05-01 · 8/10 — per-instance clientId fix lands; instanceof short-circuit narrowing and unit-test polish raised
• cdcf203 2026-05-01 · 5/10 — writable-frozen reconnect 0x173 clientId-reuse risk on frozenServices.ts:133-137
• ff37f19 2026-05-01 · 3/10 — vacuous dispose/close-during-hung-upgrade tests after the sendMessages short-circuit