feat(container-loader): add readOnly option to loadFrozenContainerFromPendingState by anthony-murphy · Pull Request #27211 · microsoft/FluidFramework

anthony-murphy · 2026-04-30T19:59:20Z

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 is updated.

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 API shape (readOnly?: boolean, default true, 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). readOnly aligns with the established state name at the container, runtime, and datastore layers (also the version-history / storage-only signal).

…mPendingState Currently a frozen container is always read-only via the storageOnly policy. This adds a `readOnly` option (default `true`) so callers can load a frozen container that surfaces as writable in order to accrue and capture additional pending state without publishing it. When `readOnly: false`: - The initial connect returns a `FrozenDeltaStream` configured with `DocWrite` scope so `readOnlyInfo.readonly === false`. - The runtime accepts DDS submissions; the first submit triggers a read→write upgrade attempt that the document service intercepts and hangs (we have no upstream and won't fabricate a quorum join op). - The container settles into Disconnected. DDS local apply continues, and ops accumulate in the runtime's pendingStateManager so getPendingLocalState() can capture them. Other changes: - `FrozenDeltaStream` constructor switched from positional args to an options bag with `readOnly`, `storageOnlyReason`, and `readonlyConnectionReason`. - Both variants nack on submit/submitSignal — under normal flow neither is ever called (sendMessages short-circuits read-mode ops to reconnect), so a nack reaching the connectionManager is the right defensive signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions · 2026-04-30T20:07:55Z

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

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

- submitSignal becomes a silent no-op for both FrozenDeltaStream variants. The old read-only behavior emitted a non-array nack payload (would crash nackHandler's `messages[0]` access) and, on the writable variant, would trigger reconnect/close on stray runtime/presence signals. Dropping is the correct behavior since signals are ephemeral and we have no upstream. - createFrozenDocumentServiceFactory now re-wraps an already-wrapped factory when its readOnly differs from the caller's argument, so the caller's most recent intent wins instead of being silently dropped. - Surface FrozenDocumentServiceFactory.readOnly/inner so the rewrap path can unwrap; rename the inner field for clarity. - Tests: - signal submission on the writable frozen container does not close it. - getPendingLocalState() round-trips writable-frozen edits through a second writable-frozen load (the load-bearing invariant for this PR). - readOnly: false wins over an already-wrapped readOnly: true factory. - Fix a misleading comment that claimed writable variant "silently drops" submissions; ops actually accumulate in pendingStateManager. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

anthony-murphy · 2026-04-30T23:05:21Z

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 · 2026-04-30T23:37:18Z

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.

…ial connect Container.connectToDeltaStream forces args.mode = "write" on the very first connect when allowReconnect is false or the client is non-interactive (container.ts:1517-1523). The previous FrozenDocumentService logic gated on client.mode alone, so that initial connect would be intercepted by the upgrade-hang path and the load would never complete. Track first-vs-subsequent connect on FrozenDocumentService and always return the writable FrozenDeltaStream on the first call, regardless of client.mode. Mode-based hang now applies only to subsequent write-mode connects, which is the runtime-driven read→write upgrade we actually want to intercept. Subsequent read-mode connects still return a fresh writable stream. Tests: - loadFrozenContainerFromPendingState({readOnly: false, allowReconnect: false}) asserts readonly === false, container stays open, and pending edits round-trip through getPendingLocalState(). - Same assertions for the non-interactive-client variant (clientDetailsOverride.capabilities.interactive === false). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ect is hung The dispose ordering on the hung write-upgrade promise is the most invariant-relevant scenario in writable-frozen — FrozenDocumentService.dispose() and pendingConnectRejecters exist specifically to make container.dispose() complete in this state — and it had no test coverage. Two regression tests: - dispose() completes while the read→write upgrade connect is hung — triggers the upgrade via a DDS write, awaits the disconnect that precedes the hang, then asserts dispose() returns and disposed === true. - close() does not hang while the read→write upgrade connect is hung — same flow via close(); pins the documented benign-leak tradeoff (close() does not propagate to service.dispose(), so the hung promise stays pending until GC). Also document the close-vs-dispose tradeoff in connectToDeltaStream's hung-promise block so future readers know which path drains pendingConnectRejecters and why. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Blocked CI lint with @typescript-eslint/no-unnecessary-type-arguments — void is the default for timeoutPromise's T parameter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ose/close tests The new dispose-during-hung-upgrade and close-during-hung-upgrade tests were timing out at mocha's ~2s default while waiting 5s for a "disconnected" event. The writable-frozen container does not transition to a "saved" state and does not necessarily emit "disconnected" before the upgrade-connect call lands — connection state may transition through EstablishingConnection instead, or the runtime's outbox flush is async enough that nothing visible happens in 2s. Switch to a bounded 500ms sleep that yields long enough for the upgrade attempt to register the hung promise on pendingConnectRejecters. Test still validates the dispose drain path; it just doesn't require an explicit disconnected signal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… deep review Six independent items, each previously held as Tier 3 polish: - connectionManager.sendMessages: tripwire comment near the read-mode short-circuit so a future refactor surfaces writable-frozen's dependency on this exact path (the deferred reconnect-to-write is what FrozenDocumentService.connectToDeltaStream intercepts to hang). - FrozenDeltaStream constructor: runtime asserts that storageOnlyReason and readonlyConnectionReason are not passed when readOnly: false — the writable variant has no readOnlyInfo to surface them on, so silent acceptance was misleading. Per repo convention, asserts use string literal messages, not hex codes. - FrozenDeltaStream constructor: JSDoc the `as ITokenClaims` cast — only scopes drives observable behavior (DocRead vs DocWrite gates readOnlyInfo); inventing sentinels for tenant/document/user/iat/exp/ver would imply quorum membership we cannot honor. - ILoadFrozenContainerFromPendingStateProps.readOnly: JSDoc rationale for the negative polarity (alignment with IContainer.readOnlyInfo), preempting the alpha-rename-window question. - New writes-after-disconnect integration test: writes performed AFTER the upgrade-connect has hung still apply locally and round-trip through getPendingLocalState() into a second writable-frozen load. - New unit-test file for FrozenDeltaStream covering submit's nack payload shape (array, code 403, one entry per op), submitSignal as a silent no-op for both variants, and the constructor guards above. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…am connections Without this, loadFrozenContainerFromPendingState({ readOnly: false, allowReconnect: false }) closes asynchronously after the first DDS write: the read-mode reconnect scheduled by sendMessages → reconnect("write") → ReconnectMode.Never path → closeHandler() fires on a microtask. The existing allowReconnect:false test masked this because it captured pending state before the close microtask could be observed. ConnectionManager.sendMessages now recognizes a FrozenDeltaStream as the live connection and drops outbound messages at the network layer — the runtime's outbox keeps them in pendingStateManager for getPendingLocalState to capture. This is the design invariant the writable-frozen flow already depended on, made explicit and enforced rather than load-bearing on a side-effect of the read-mode short-circuit. Other consequences: - Container stays Connected for writable-frozen (previously settled into Disconnected after the upgrade-connect hung). JSDocs in frozenServices.ts and createAndLoadContainerUtils.ts updated to reflect the new mechanism. - The upgrade-hang at FrozenDocumentService.connectToDeltaStream({ mode: "write" }) is now defense-in-depth — unreachable in normal flow but retained for invariant breaks. JSDoc updated accordingly. Tests: - Extended `loads with allowReconnect: false` to flush microtasks after the initial writes, assert closed === false, perform another batch of writes, and assert all writes round-trip through pending state into a second writable-frozen load. Without the sendMessages fix this would fail on the closed assertion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…trings + changeset After the sendMessages FrozenDeltaStream short-circuit landed, the integration "dispose() / close() while upgrade-connect is hung" tests no longer exercise the path they were written for: the short-circuit returns before any read→write reconnect fires, so pendingConnectRejecters is empty when those tests call dispose() / close(), and disposed === true / closed === true assertions pass on the synchronous setter alone. Restoration: add focused unit tests in frozenServices.spec.ts that drive FrozenDocumentService.connectToDeltaStream({mode: "write"}) directly to exercise the rejecter: - 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 Also align the now-stale narrative across artifacts: - Changeset: replace the "first runtime submit triggers an internal read→write upgrade attempt that cannot succeed, so the container settles into Disconnected" narrative with the actual flow (sendMessages drops outbound at the FrozenDeltaStream short-circuit; container stays Connected). Call out the submitSignal read-only-variant behavior change (was 403 nack, now silent no-op) explicitly. - Integration tests: rename "dispose() while the read→write upgrade connect is hung" → "dispose() runs cleanly on a writable-frozen container after a local write" (and same for close); rename "captures writes performed after the upgrade-connect has hung" → "captures writes batched across timer/microtask boundaries". Update inline comments to describe the actual short-circuit-first flow and cross-link the focused unit test for the dispose-rejecter coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

anthony-murphy · 2026-05-01T22:57:21Z

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

The unit test typed `rejection` as `unknown` and called `instanceof Error`, which TypeScript rejects for `unknown` LHS. Type rejection as `Error | undefined` and check for `undefined` instead — equivalent semantics, no instanceof on unknown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

assert.strictEqual is declared with `asserts actual is T`, which narrowed `rejection` to type `undefined` for the rest of the function. The subsequent `assert(rejection !== undefined)` then narrowed to `never`, making `rejection.message` a TS2339 error. Capture into a separate const before the pending-state assertion so the narrowing scopes to that const and the post-dispose check sees rejection as Error|undefined again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The lint rule requires catch handler parameters to be named 'error'. Bare 'e' was caught by eslint --quiet at 178:22. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

anthony-murphy · 2026-05-01T23:59:31Z

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.

Pre-fix, every FrozenDeltaStream instance shared the constant clientId "storage-only client". A subsequent connect through FrozenDocumentService.connectToDeltaStream's `client.mode !== "write"` branch (e.g. an explicit container.disconnect/connect cycle on a writable-frozen container with dirty pending ops) would replay pending ops against the same clientId twice and trip pendingStateManager's 0x173 assert ("replayPendingStates called twice for same clientId"). Mint a fresh `frozen-delta-stream/<uuid>` clientId per instance, mirror it in initialClients so the audience handler still observes "self" and transitions to Connected. Adjusted two existing tests in noDeltaStream.spec.ts that asserted the literal "storage-only client" string — they now assert the "frozen-delta-stream/" prefix instead. New regression test in loadFrozenContainerFromPendingState.spec.ts forces a disconnect/reconnect cycle on a writable-frozen container with dirty pending ops and verifies the container survives, all writes remain locally visible, and pending state still round-trips. Pre-fix this would have tripped 0x173. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ts; fix changeset wording Address Copilot review feedback on timing-dependent test waits and stale wording: - Changeset: "synthetic FrozenDeltaStream" → "synthetic WritableFrozenDeltaStream" in the writable-context paragraph (the writable path uses WritableFrozenDeltaStream specifically, and sendMessages short-circuits on that). - captures writes batched across timer/microtask boundaries: 500ms sleep → setTimeout(0). Crossing one macrotask boundary is sufficient for the test's intent (runtime continues to capture pending state across timer boundaries); 500ms was overkill. Renamed map keys preDisconnect/postDisconnect → preDelay/postDelay since no disconnect occurs in this test. Updated comment "FrozenDeltaStream short-circuit" → "WritableFrozenDeltaStream short-circuit". - dispose() / close() runs cleanly: dropped the 200ms delays. The simplification that collapsed FrozenDocumentService.connectToDeltaStream branches removed the async hung-promise machinery, so there's no deferred work to wait for; dispose()/close() can run immediately after the synchronous root.set(). - forceReadonly(true): dropped the 200ms delay. forceReadonly toggles _forceReadonly synchronously, so readOnlyInfo updates without awaiting a reconnect cycle (the disconnect/reconnect-as-read triggered behind it is exercised by the survives-disconnect/reconnect test). - survives a disconnect/reconnect cycle: replaced the 200ms sleep with Promise.race against the "connected" event and a 500ms safety-net timeout. Resolves immediately if the event fires; bounded otherwise. The safety net is documented. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

CI biome check on `a3360ee43c` flagged the multiline `new Promise<void>((resolve) => frozenContainer.once("connected", () => resolve()))` arrow body. Reformat to one line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ctToDeltaStream Address markfields' review feedback: the read-only branch was a "defensive fallback" — connectionManager short-circuits via policies.storageOnly before reaching connectToDeltaStream, so reaching this branch indicates a non-connectionManager consumer or a regression of the short-circuit. Throwing surfaces the misuse instead of silently producing a working stream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Address markfields' review feedback (line 198 was 800+ chars, line 199 1000+). Move per-variant detail out of the base-class bullet list (which was forcing single-line bullets to satisfy jsdoc/check-indentation) into the FrozenDeltaStream and WritableFrozenDeltaStream class JSDoc, where each line wraps naturally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ames Address markfields' nit: the class was renamed in microsoft#25538 but the comments and test descriptions still referred to "NoDeltaStream" / "No Delta Stream". - connectionManager.ts:599 comment ("NoDeltaStream object" → "FrozenDeltaStream object") - local-server-tests noDeltaStream.spec.ts: describe + test name ("No Delta Stream" → "Frozen Delta Stream") - test-end-to-end-tests noDeltaStream.spec.ts: describeCompat name ("No Delta stream loading mode testing" → "Frozen Delta stream loading mode testing") Filenames left alone — file rename was held as out-of-scope earlier (would require either a broader rename to frozenContainer.spec.ts or splitting the suite, with 0 churn on the file in 6 months making rebase collision risk minimal). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

markfields · 2026-05-08T18:43:08Z

+		readonlyConnectionReason?: IConnectionStateChangeReason;
+	}) {
+		// Constant clientId: preserves the pre-PR `"storage-only client"` identity for any
+		// consumer that keys off it. The 0x173 replay-assert risk that motivates per-instance


Trying to think of a clearer naming here. The DeltaStream is not writeable (as evidenced by the short-circuit when connectionManager tries to send a message), it just presents itself that way.

Even if you don't rename, making this comment up front about this will help.

Suggested change

// consumer that keys off it. The 0x173 replay-assert risk that motivates per-instance

* Variant of {@link FrozenDeltaStreamBase} that appears to support writing, but remains "Frozen" - no messages are sent or received. Differs from {@link FrozenDeltaStream}

Btw It's too bad the abstractions don't line up right to hide the "don't actually send" part inside this impl

Applied your suggestion in e8dd9b9790. The class JSDoc now leads with "appears to support writing but remains frozen — no messages are actually sent or received", then explains where the no-send is actually enforced (ConnectionManager.sendMessages short-circuit), then the "appears writable" mechanics (DocWrite claims, mode="read"). Kept the class name — per your "Even if you don't rename", the upfront framing carries the clarification.

Agreed — would have been nicer if submit could enforce it locally, but that path emits a nack event which is exactly the tripwire we want preserved on the read-only variant (a stray submit on a storage-only frozen connection signals an upstream invariant break and should remain observable). The writable variant inherits the same submit, but sendMessages short-circuits before it can fire under normal flow. So both variants share an identical inert submit that does preserve the contract — the writable variant just relies on connectionManager to never reach it. Documented that on the JSDoc; the in-class fix would have to thread a "writable but inert" flag through which is what the prior readOnly constructor option did before the sibling-class split.

…pears writable" framing Address markfields' review feedback: the class isn't actually writable — it just presents itself that way. The no-send guarantee lives in ConnectionManager.sendMessages, not in the stream itself. Restructured the JSDoc opening per markfields' suggested wording: lead with "appears to support writing but remains frozen — no messages are actually sent or received", then explain where the no-send actually enforces (sendMessages short-circuit), then the "appears writable" mechanics (DocWrite claims, mode="read"). The class name stays the same per markfields' "Even if you don't rename" — the upfront framing carries the clarification. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

markfields · 2026-05-08T18:52:23Z

+	 * 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`.
+	 */
+	readonly readOnly?: boolean;


The term "readonly" typically makes me think of a state where you can't write but you can receive remote changes.

I'd call the parameter "allowLocalChanges" (defaulting to false).

Or if you want negative polarity (I just read the remarks), then disallowLocalChanges...?

Done in 7422e300f5. Renamed to allowLocalChanges with default false (positive opt-in — caller passes allowLocalChanges: true to opt into the writable-frozen behavior; preserves the previous default behavior of "no local writes" when no arg is passed). Updated the props interface, factory function, internal class fields, JSDoc, comments, tests, and the API report.

Went with positive allowLocalChanges (default false) over negative disallowLocalChanges (default true) — the double-negative in disallowLocalChanges: false is harder to parse than allowLocalChanges: true. The earlier "negative polarity" rationale (alignment with IContainer.readOnlyInfo.readonly) was specific to the readOnly name; once the name itself was wrong, that justification dropped.

On reflection, going back to readOnly?: boolean (default true) in 858990ee5b. Rationale: readOnly is the established state name across the container, runtime, and datastore layers — IContainer.readOnlyInfo.readonly, the readonly signals on the runtime and datastore, and version-history / storage-only mode all use the same word for the same concept. It determines whether ops are allowed; matching that convention here keeps the surface aligned rather than introducing a parallel name for the same idea.

The allowLocalChanges framing is more literal about what the flag toggles in this specific path, but the cost is splitting readers between two conventions for "this thing controls whether ops can be submitted." For consistency with the rest of the stack I'd rather keep readOnly.

markfields · 2026-05-08T18:57:00Z

+		// WritableFrozenDeltaStream as the live connection. Its `mode` is "read" (advertising
+		// "write" would imply quorum membership we cannot honor), so a runtime submit
+		// would otherwise fall into the read-mode reconnect branch below. That branch
+		// schedules `reconnect("write")`, which under `ReconnectMode.Never`


I wonder if there are other cases in the runtime that trigger reconnect that would cause trouble?

Walked through the runtime-reconnect triggers — they all funnel into ConnectionManager.connectCore → docService.connectToDeltaStream → returns a fresh WritableFrozenDeltaStream. The failure modes the writable-frozen path needs to defend against:

Runtime submit → read-mode reconnect branch in sendMessages — blocked by the isWritableFrozenDeltaStreamConnection short-circuit (this hunk).

Container forceReadonly(true) mid-life — disconnect/reconnect-as-read; the new connection is another WritableFrozenDeltaStream; _forceReadonly = true overrides readOnlyInfo.readonly. Pinned by the forceReadonly(true) surfaces ... as readonly integration test.

Runtime-driven disconnect() + connect() cycle (e.g. via IContainer.disconnect() / connect() in the host) — same path: fresh WritableFrozenDeltaStream, runtime replays pending state. Pinned by the survives a disconnect/reconnect cycle test (per-instance UUID clientId defends pendingStateManager 0x173).

Connection error / disconnect events from the live stream — the WritableFrozenDeltaStream is inert (no upstream emits these), so no event-driven reconnect fires in normal flow.

NACK on outbound submit — sendMessages drops at the short-circuit before reaching the connection's submit, so no nack-driven reconnectOnError("write").

So the only path that would still trigger a runtime-driven write reconnect is a stray submit hitting the connection's submit directly (bypassing sendMessages) — which would emit a 403 nack and could drive reconnectOnError("write"). That path is unreachable in normal flow because sendMessages is the sole runtime → connection submit path. The 403-nack tripwire on the read-only variant's submit deliberately stays as the loud-failure signal if any future change opens that path.

Are you thinking of a specific scenario I haven't covered? Happy to add a test if there's a path I'm missing.

…tive opt-in) Address markfields' review feedback: "readOnly" implies a state where remote changes still arrive — but here the connection is frozen in both directions. The flag actually controls whether the runtime accepts *local* DDS submissions (which then accumulate in pendingStateManager). Renaming makes the parameter describe what it does. Polarity flips with the rename: - ILoadFrozenContainerFromPendingStateProps.readOnly?: boolean → allowLocalChanges?: boolean - createFrozenDocumentServiceFactory(factory, readOnly?: boolean = true) → createFrozenDocumentServiceFactory(factory, allowLocalChanges?: boolean = false) - Default behavior unchanged: undefined / no arg → no local changes (same as previous default `readOnly: true`). - New default-restrictive polarity: caller must explicitly opt in to allow local changes by passing `allowLocalChanges: true` (was `readOnly: false`). Internal field/parameter names and JSDoc updated throughout (FrozenDocumentServiceFactory.allowLocalChanges, FrozenDocumentService.allowLocalChanges, the storageOnly policy assignment now reads `allowLocalChanges ? {} : { storageOnly: true }`). API report regenerated. Tests, describe/it names, and comments updated for the new wording. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

markfields

Approving, but would really suggest reconsidering the readonly term, since that state typically implies remote changes will still be processed which is not the case.

anthony-murphy · 2026-05-08T19:23:49Z

+			await Promise.race([
+				new Promise<void>((resolve) => frozenContainer.once("connected", () => resolve())),
+				new Promise<void>((resolve) => setTimeout(resolve, 500)),
+			]);


Deep Review: Disconnect/reconnect regression test races a resolving 500ms timeout — can pass without connected ever firing.

The wait at lines 876-879 is

await Promise.race([ new Promise<void>((resolve) => frozenContainer.once("connected", () => resolve())), new Promise<void>((resolve) => setTimeout(resolve, 500)), ]);

The timeout resolves rather than rejects, so the race always settles cleanly even when connected never fires. Post-race assertions (lines 881-901) check only frozenContainer.closed === false, local DDS state via root.get(...) (already in memory from the writes at line 865), and getPendingLocalState() !== initialPending. None of these are reconnect-only observables — all are satisfiable by state written before the disconnect/reconnect cycle. There is no assertion that connected actually fired, no assertion on a new frozen-delta-stream/<uuid> clientId, and no assertion that replayPendingStates was invoked.

This is the only end-to-end guard for the per-instance-UUID 0x173 (replayPendingStates called twice for same clientId!) mitigation — a hazard neither blind design proposal surfaced. Weakening the assertion surface here materially undermines that guard.

Suggested fix: Make the timeout reject so the test fails when connected does not fire, and add a reconnect-only assertion. For example, capture the writable stream's frozen-delta-stream/<uuid> clientId before disconnect and assert it differs after reconnect — that directly pins the per-instance-UUID 0x173 mitigation rather than incidental local state.

const clientIdBefore = (frozenContainer as any).deltaManager.connectionManager.connection?.clientId; frozenContainer.disconnect(); frozenContainer.connect(); await Promise.race([ new Promise<void>((resolve) => frozenContainer.once("connected", () => resolve())), new Promise<void>((_, reject) => setTimeout(() => reject(new Error("connected never fired after reconnect")), 500)), ]); const clientIdAfter = (frozenContainer as any).deltaManager.connectionManager.connection?.clientId; assert.notStrictEqual(clientIdAfter, clientIdBefore, "writable stream clientId should be fresh per reconnect");

(Adjust the property access path to match whatever surface is reachable from the test fixture.)

You're right that the resolving timeout was vacuous. I tried the suggested reject-on-timeout (Promise.race against once("connected") with a 2s reject) and the test consistently fails: connected never fired after reconnect. Also tried polling frozenContainer.clientId for 10s waiting for it to change after disconnect() + connect() — clientId stays pinned at the original value the entire time.

So IContainer.disconnect() + connect() doesn't actually rotate the live connection on a writable-frozen container within a bounded wait. Whether the disconnect propagates but the reconnect never reaches Connected, or the disconnect is a no-op because of how this container's reconnect mode is wired, the observable we'd need (a fresh clientId) isn't reachable from the integration surface.

Given that, an integration test gated on that observable would either be vacuous (resolving timeout — what we had) or always fail. Dropped the test in 7312defdcb and replaced it with a comment block pointing at the unit test in frozenServices.spec.ts → "hands out distinct WritableFrozenDeltaStream instances with distinct clientIds on subsequent connects" — which directly asserts the contract the 0x173 mitigation depends on (each FrozenDocumentService.connectToDeltaStream() call mints a fresh frozen-delta-stream/<uuid> clientId).

Net result: 0x173 mitigation is still fully covered, but at the unit-test layer where the observable is actually reachable, rather than dressed up as integration coverage that never exercised the path.

Deep Review: Acknowledged — dropping the integration test in 7312defd and pointing at the unit test (hands out distinct WritableFrozenDeltaStream instances with distinct clientIds on subsequent connects in frozenServices.spec.ts) is the right call once the integration observable turned out to be unreachable from the test fixture. The unit test directly asserts the contract the 0x173 mitigation depends on, which is what the integration test was trying to dress up. No further action on this thread.

… test; rely on unit test for 0x173 coverage Address Deep Review finding: the disconnect/reconnect test was passing for the wrong reason — `IContainer.disconnect()` + `connect()` doesn't actually rotate the live connection on a writable-frozen container within a bounded wait (clientId stays pinned for >10s), so any assertion that "the new connection has a fresh clientId" is either vacuous (resolving timeout) or fails (clientId never changes). The 0x173 (`replayPendingStates called twice for same clientId!`) mitigation is the contract that each `FrozenDocumentService.connectToDeltaStream()` call mints a fresh `frozen-delta-stream/<uuid>` clientId — that's directly asserted by the unit test in `frozenServices.spec.ts`. The integration test was attempting to layer a second guard on top, but the observable it was checking is unreachable from this container surface. Replaced the test with a comment block pointing future readers at the unit test, so we don't lose the explanation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions · 2026-05-08T19:42:53Z

🔗 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

…es (positive opt-in)" Per-author preference: keep the original `readOnly?: boolean` (default `true`) shape on `ILoadFrozenContainerFromPendingStateProps` and `createFrozenDocumentServiceFactory`. Polarity restored to negative-with-default-true (aligned with `IContainer.readOnlyInfo.readonly`). Internal field/parameter names and JSDoc reverted throughout. API report regenerated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

github-actions · 2026-05-08T19:55:38Z

🔗 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 · 2026-05-08T20:20:51Z

Deep Review

Reviewed commit 858990e on 2026-05-08.

Readiness: 6/10 — MAKING PROGRESS

Not sign-off ready. New correctness gap found this round on a path no prior review traced: a writable-frozen load (readOnly: false) whose pending state contains pendingAttachmentBlobs either closes the container or hangs forever, because ContainerRuntime.loadRuntime always invokes runtime.sharePendingBlobs() once connected && readOnlyInfo.readonly !== true, and the writable-frozen variant satisfies that gate while still rejecting createBlob (closes via closeFn) and routing blob-attach ops into WritableFrozenDeltaStream's drop-on-the-floor sendMessages (load hangs). The allowLocalChanges rename from the prior round was reverted at this sha — the API is back to readOnly?: boolean (default true), and the PR description matches. Detail and suggested fixes inline on frozenServices.ts. Fix is contained but absent and uncovered by the new writable-frozen suite.

Path to Ready

Resolve inline threads

Context for Reviewers

The WritableFrozenDeltaStream short-circuit in ConnectionManager.sendMessages is the first deliberate exception to vladsud's Remove two types of nacks - "Nonexistent client" & "Readonly client" #7753 read-mode-no-submits invariant. isWritableFrozenDeltaStreamConnection at connectionManager.ts:1092-1112 MUST stay above the read-mode reconnect branch; under ReconnectMode.Never the read-mode branch would call closeHandler on the first DDS write. 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"; only WritableFrozenDeltaStream mints a per-instance frozen-delta-stream/<uuid> to dodge pendingStateManager 0x173 (replayPendingStates called twice for same clientId!) on reconnect with dirty pending ops. Neither blind design proposal surfaced this hazard.
initialClients mirroring is load-bearing for the audience/Connected interaction. 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 and explicitly flagged NoDeltaConnection as an open question; the frozen-stream base mirrors each synthetic clientId into initialClients so the audience handler observes self without a real join signal.
submitSignal is now a silent no-op for both variants (was 403 nack on the read-only variant). Signals are ephemeral and dropping is the right behavior with no upstream; submit stays defensive (403 nack on the read-only variant) since ops are durable state.
jatgarg-owned call sites are reshaped from positional args to options bag. PR Handle sessoionforbidden error and add storageOnlyReason to Readonly info #15907 (jatgarg, 2023-06-12 — storageOnlyReason) and PR Handle out of storage error from driver and make the container loadable #17131 (jatgarg, 2023-08-31 — outOfStorageError) established the two FrozenDeltaStream call sites this PR rewrites. The IReadOnlyInfo.storageOnlyReason and outOfStorage diagnostics still propagate through the options-bag form on the read-only variant.
readOnly?: boolean (default true) was restored at 858990e after the prior sha's allowLocalChanges rename — convention argument grounded in IContainer.readOnlyInfo.readonly, runtime/datastore readonly state names, and IDocumentServicePolicies.storageOnly. Negative polarity is now defended in an in-code @remarks block at createAndLoadContainerUtils.ts:229-269.
frozenServices.ts was a low-churn file before this PR. 0 commits in the last 6 months and 8 historical PRs (Integrate Strongly Typed Events from DeltaConnection Thru DeltaManager #1820, Implementation for resolving data corruption issue for distributed data ordering service #5484, Fixing potential issues around client having connection that does not broadcast ops #6928, Remove redundant maxMessageSize property #7949, DeltaManager: refactor out connectivity manager #8423, Simplified model for Audience; Leverage join signal's referenceSequenceNumber for catch up logic #12164, Handle sessoionforbidden error and add storageOnlyReason to Readonly info #15907, Handle out of storage error from driver and make the container loadable #17131), none in 2024–2026. The +203/-59 reshape doubles its surface — re-derive FrozenDeltaStream invariants (clientId identity, audience self-observation, nack vs no-op semantics) from first principles rather than skim the in-code commentary.

For human reviewer

vladsud sign-off on the ConnectionManager.sendMessages WritableFrozenDeltaStream short-circuit (connectionManager.ts:1092-1112) — first deliberate exception to Remove two types of nacks - "Nonexistent client" & "Readonly client" #7753's read-mode-no-submits invariant. Verify ordering vs the read-mode reconnect branch, that the read-only FrozenDeltaStream 403-nack tripwire remains observable, and the audience/Connected interaction with the per-instance UUID clientId (Simplified model for Audience; Leverage join signal's referenceSequenceNumber for catch up logic #12164 territory). Not currently in the requested-reviewers list.
ChumpChief sign-off on the @legacy @alpha API shape (readOnly?: boolean, default true, positional arg on createFrozenDocumentServiceFactory). The earlier allowLocalChanges rename per markfields was reverted at this sha — confirm the negative-polarity readOnly shape is the intended final form. Not currently in the requested-reviewers list.
jatgarg (optional) review of the FrozenDeltaStream constructor reshape 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 outOfStorageError) — confirm IReadOnlyInfo.storageOnlyReason and outOfStorage diagnostics still flow end-to-end through the options-bag form.
Sibling-class split vs single-class-with-flag — taste call. Both blind design proposals chose single-class + flag; the PR's sibling-class shape preserves a type-discriminant invariant a flag would scatter.
Cannot be assessed by the pipeline — CI status on 858990e; 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.

Review history (10 prior reviews)

7422e30 2026-05-08 · 5/10 — allowLocalChanges rename landed in code but not in PR title/description/reviewer guidance
87fe785 2026-05-08 · 10/10 — sign-off ready; no Tier 1–3 findings on 87fe785
102c584 2026-05-06 · 10/10 — sign-off ready; no Tier 1–3 findings; soft polish + human sign-offs only
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

anthony-murphy · 2026-05-08T20:20:54Z

+		// live connection — not the policy. So the writable-frozen container is intentionally
+		// indistinguishable from a normal container at the policies layer; downstream behavior
+		// flows through the live `WritableFrozenDeltaStream` instead.
+		this.policies = readOnly ? { storageOnly: true } : {};


Deep Review: Writable-frozen loads with pendingAttachmentBlobs in pending state will close or hang the container via BlobManager.sharePendingBlobs.

New correctness gap on a path the prior reviews didn't trace. Walking it end-to-end:

this.policies = readOnly ? { storageOnly: true } : {} (this line) — the writable-frozen variant has empty policies, so Container reports connected && readOnlyInfo.readonly !== true.

ContainerRuntime.loadRuntime (containerRuntime.ts:1299) always calls runtime.sharePendingBlobs() after load. The implementation (containerRuntime.ts:5331-5356) proceeds when connected and not readonly, and routes errors to closeFn via .catch(this.closeFn).

For pending state containing pendingAttachmentBlobs, BlobManager.sharePendingBlobs() either:

calls storage.createBlob() — which the frozen storage wrapper at frozenServices.ts:151 is wired to throw on. The error propagates to closeFn and closes the container, or

sends a blob-attach op via this.submit (containerRuntime.ts:2014-2023). The op then hits ConnectionManager.sendMessages whose new isWritableFrozenDeltaStreamConnection short-circuit (connectionManager.ts:1111-1112) drops it. BlobManager waits indefinitely for a processed BlobAttach (blobManager.ts:666-703), so the attach promise never resolves and the load hangs.

Both behaviors violate the PR's stated invariant that writable-frozen loads "do not publish anything" and that storage operations remain unsupported.

The new writable-frozen suite (loadFrozenContainerFromPendingState.spec.ts:328-593) covers DDS writes, signal drops, pending-op round-trip, rewrap, and reconnect, but no test constructs pending state with pendingAttachmentBlobs — so this path is uncovered.

Suggested options:

Suppress sharePendingBlobs for frozen document services even in writable mode — e.g. via an additional policy flag distinct from storageOnly (so connectToDeltaStream still runs but sharePendingBlobs does not).

Short-circuit BlobManager.sharePendingBlobs when the storage's createBlob is the throw-handler.

If supporting pending state containing pendingAttachmentBlobs is out of scope, fail fast at load time with a diagnosable error rather than relying on the silent drop / closeFn path.

Whichever you pick, please add a regression test in the writable-frozen suite that loads from pending state containing pendingAttachmentBlobs with readOnly: false and asserts the load resolves without closing the container and without hanging.

Open design questions:

For writable-frozen with pendingAttachmentBlobs, what is the intended behavior — suppress sharePendingBlobs, fail fast, or out-of-scope?

If sharePendingBlobs is intentionally allowed to run, how should the dropped blob-attach ops in the writable-frozen sendMessages path resolve their attach promises (or surface a diagnosable failure) rather than hanging indefinitely?

anthony-murphy and others added 2 commits April 30, 2026 13:11

add changeset and regenerate api-report

2991cf6

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

fix biome formatting and unresolved @link references

8f2774f

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

anthony-murphy added the deep-review label Apr 30, 2026

anthony-murphy commented Apr 30, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/frozenServices.ts Outdated

anthony-murphy commented Apr 30, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/frozenServices.ts Outdated

anthony-murphy commented May 1, 2026

View reviewed changes

Comment thread packages/test/local-server-tests/src/test/loadFrozenContainerFromPendingState.spec.ts

anthony-murphy and others added 4 commits May 1, 2026 09:55

fix(test): drop redundant <void> type argument on timeoutPromise calls

a08917f

Blocked CI lint with @typescript-eslint/no-unnecessary-type-arguments — void is the default for timeoutPromise's T parameter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

anthony-murphy commented May 1, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/frozenServices.ts Outdated

anthony-murphy and others added 2 commits May 1, 2026 15:15

anthony-murphy and others added 3 commits May 1, 2026 16:06

fix(test): rename catch parameters to satisfy unicorn/catch-error-name

cdcf203

The lint rule requires catch handler parameters to be named 'error'. Bare 'e' was caught by eslint --quiet at 178:22. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

anthony-murphy commented May 1, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/frozenServices.ts Outdated

anthony-murphy commented May 2, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/frozenServices.ts Outdated

anthony-murphy commented May 2, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/connectionManager.ts

anthony-murphy commented May 2, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/test/frozenServices.spec.ts

anthony-murphy commented May 2, 2026

View reviewed changes

Comment thread packages/test/local-server-tests/src/test/noDeltaStream.spec.ts Outdated

anthony-murphy and others added 3 commits May 5, 2026 19:27

Merge branch 'main' into users/anthonm/frozen-container-readonly-option

4ddd5c1

anthony-murphy requested a review from Copilot May 6, 2026 15:13

Copilot started reviewing on behalf of anthony-murphy May 6, 2026 15:14 View session

anthony-murphy requested review from dannimad, markfields and sonalideshpandemsft May 6, 2026 15:30

chore: remove changeset

87fe785

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

markfields reviewed May 8, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/frozenServices.ts Outdated

markfields reviewed May 8, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/frozenServices.ts

markfields reviewed May 8, 2026

View reviewed changes

Comment thread packages/loader/container-loader/src/connectionManager.ts Outdated

markfields reviewed May 8, 2026

View reviewed changes

anthony-murphy commented May 8, 2026

View reviewed changes

anthony-murphy changed the title ~~feat(container-loader): add readOnly option to loadFrozenContainerFromPendingState~~ feat(container-loader): add allowLocalChanges option to loadFrozenContainerFromPendingState May 8, 2026

anthony-murphy changed the title ~~feat(container-loader): add allowLocalChanges option to loadFrozenContainerFromPendingState~~ feat(container-loader): add readOnly option to loadFrozenContainerFromPendingState May 8, 2026

anthony-murphy commented May 8, 2026

View reviewed changes

	// consumer that keys off it. The 0x173 replay-assert risk that motivates per-instance
	* Variant of {@link FrozenDeltaStreamBase} that appears to support writing, but remains "Frozen" - no messages are sent or received. Differs from {@link FrozenDeltaStream}

Conversation

anthony-murphy commented Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Guidance

Uh oh!

github-actions Bot commented Apr 30, 2026 • edited by anthony-murphy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

anthony-murphy commented Apr 30, 2026

Review fixes — addressed in e771980

1 + 3: submitSignal payload shape and writable failure mode

2: Wrapped-factory readOnly mismatch

4: getPendingLocalState() round-trip test

Other

Uh oh!

anthony-murphy commented Apr 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anthony-murphy commented May 1, 2026

Tier 1 finding addressed in 411863f

Uh oh!

anthony-murphy commented May 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markfields May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markfields left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 8, 2026

linkcheck output

Uh oh!

github-actions Bot commented May 8, 2026

linkcheck output

Uh oh!

anthony-murphy commented May 8, 2026

Deep Review

Path to Ready

Context for Reviewers

Uh oh!

Choose a reason for hiding this comment

anthony-murphy commented Apr 30, 2026 •

edited

Loading

github-actions Bot commented Apr 30, 2026 •

edited by anthony-murphy

Loading

Review fixes — addressed in `e771980`

1 + 3: `submitSignal` payload shape and writable failure mode

2: Wrapped-factory `readOnly` mismatch

4: `getPendingLocalState()` round-trip test

Tier 1 finding addressed in `411863f`

markfields May 8, 2026 •

edited

Loading