-
Notifications
You must be signed in to change notification settings - Fork 576
feat(container-loader): add readOnly option to loadFrozenContainerFromPendingState #27211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
3d57157
2991cf6
8f2774f
e771980
fb6a0fd
f4a202e
a08917f
3e767b8
7c31d95
ff37f19
411863f
d1fd645
7866ef5
cdcf203
849c0ed
ce7f4ed
e93eb26
ac44bd1
ebb5b29
8f15eef
86c509a
2035078
4ddd5c1
a3360ee
102c584
87fe785
dfcb644
78c9ef4
731aaea
e8dd9b9
7422e30
7312def
858990e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| "@fluidframework/container-loader": minor | ||
| "__section": feature | ||
| --- | ||
| Add readOnly option to loadFrozenContainerFromPendingState | ||
|
Check failure on line 5 in .changeset/frozen-container-readonly-option.md
|
||
|
|
||
| `loadFrozenContainerFromPendingState` and `createFrozenDocumentServiceFactory` now accept an optional `readOnly` parameter (default `true`, preserving existing behavior). | ||
|
|
||
| When `readOnly: false`, the frozen container loads as writable so the runtime accepts DDS submissions. The first runtime submit triggers an internal read→write upgrade attempt that cannot succeed (no upstream, no quorum join op), so the container settles into `Disconnected`. DDS local apply continues, and submitted ops accumulate in the runtime's pending-state manager — this is the state needed to accrue and capture additional pending state without publishing it. | ||
|
Check failure on line 9 in .changeset/frozen-container-readonly-option.md
|
||
|
|
||
| Use `readOnly: false` when the caller wants to load a frozen container, apply additional local changes, and capture the resulting pending state via `getPendingLocalState()`. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -229,6 +229,21 @@ export interface ILoadFrozenContainerFromPendingStateProps | |
| * Pending local state to be applied to the container. | ||
| */ | ||
| readonly pendingLocalState: string; | ||
|
|
||
| /** | ||
| * Controls whether the frozen container is surfaced as read-only. | ||
| * | ||
| * Defaults to `true`. When `true`, the container reports `readOnlyInfo.readonly === true` | ||
| * with `storageOnly === true`, matching the historical behavior of frozen loads. | ||
| * | ||
| * When `false`, the container loads as writable so the runtime will accept DDS submissions. | ||
| * The first submission triggers an internal read→write upgrade attempt that cannot succeed | ||
| * (no upstream, no quorum join op), so the container settles into a `Disconnected` state. | ||
| * Local DDS state continues to update via optimistic apply, and submitted ops accumulate in | ||
| * the runtime's pending-state manager. Use this when callers want to accrue and capture | ||
| * pending state without publishing it. | ||
| */ | ||
| readonly readOnly?: boolean; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or if you want negative polarity (I just read the remarks), then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Went with positive
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On reflection, going back to The |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -241,7 +256,10 @@ export async function loadFrozenContainerFromPendingState( | |
| ): Promise<IContainer> { | ||
| return loadExistingContainer({ | ||
| ...props, | ||
| documentServiceFactory: createFrozenDocumentServiceFactory(props.documentServiceFactory), | ||
| documentServiceFactory: createFrozenDocumentServiceFactory( | ||
| props.documentServiceFactory, | ||
| props.readOnly, | ||
| ), | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,36 +30,53 @@ import { | |
| import type { IConnectionStateChangeReason } from "./contracts.js"; | ||
|
|
||
| /** | ||
| * Creation of a FrozenDocumentServiceFactory which wraps an existing | ||
| * DocumentServiceFactory to provide a storage-only document service. | ||
| * Creates an `IDocumentServiceFactory` that produces a "frozen" document service: one whose | ||
| * delta stream never sends or receives ops, and whose storage service only supports | ||
| * `IDocumentStorageService.readBlob`. Used to load a container from pending local state | ||
| * without re-establishing a live connection. | ||
| * | ||
| * @param documentServiceFactory - The underlying DocumentServiceFactory to wrap. | ||
| * @returns A FrozenDocumentServiceFactory | ||
| * @param factory - The underlying factory to wrap. Its storage backs blob reads; all other | ||
| * storage operations throw. May be omitted when blob fetches are not required. | ||
| * @param readOnly - When `true` (the default), the document service advertises the | ||
| * `IDocumentServicePolicies.storageOnly` policy, which causes the loader to surface the | ||
| * container as read-only (see `IContainer.readOnlyInfo`). | ||
| * | ||
| * When `false`, the container is loaded as writable so the runtime will accept DDS submissions. | ||
| * The first such submission triggers the connectionManager's read→write upgrade attempt. Since | ||
| * there is no real upstream and we will not fabricate a quorum join op, that upgrade hangs and | ||
| * the container settles into a `Disconnected` state. Local DDS state continues to update via | ||
| * optimistic apply, and submitted ops accumulate in the runtime's pending-state manager — which | ||
| * is exactly the state needed to capture pending local state. Use `false` when callers want to | ||
| * accrue and capture pending state without publishing it. | ||
| * @returns A factory that produces frozen document services. | ||
| * @legacy @alpha | ||
| */ | ||
| export function createFrozenDocumentServiceFactory( | ||
| factory?: IDocumentServiceFactory | Promise<IDocumentServiceFactory>, | ||
| readOnly: boolean = true, | ||
| ): IDocumentServiceFactory { | ||
| // Sync path | ||
| return factory instanceof FrozenDocumentServiceFactory | ||
| ? factory | ||
| : new FrozenDocumentServiceFactory(factory); | ||
| if (factory instanceof FrozenDocumentServiceFactory) { | ||
| // Already wrapped. Reuse if readOnly matches; otherwise unwrap and rewrap so the caller's | ||
| // most recent readOnly intent wins (silently honoring caller intent rather than dropping | ||
| // the new argument). | ||
| return factory.readOnly === readOnly | ||
| ? factory | ||
| : new FrozenDocumentServiceFactory(readOnly, factory.inner); | ||
| } | ||
| return new FrozenDocumentServiceFactory(readOnly, factory); | ||
| } | ||
|
|
||
| export class FrozenDocumentServiceFactory implements IDocumentServiceFactory { | ||
| constructor( | ||
| private readonly documentServiceFactory?: | ||
| | IDocumentServiceFactory | ||
| | Promise<IDocumentServiceFactory>, | ||
| public readonly readOnly: boolean, | ||
| public readonly inner?: IDocumentServiceFactory | Promise<IDocumentServiceFactory>, | ||
| ) {} | ||
|
|
||
| async createDocumentService(resolvedUrl: IResolvedUrl): Promise<IDocumentService> { | ||
| let factory = this.documentServiceFactory; | ||
| if (isPromiseLike(factory)) { | ||
| factory = await this.documentServiceFactory; | ||
| } | ||
| const factory = isPromiseLike(this.inner) ? await this.inner : this.inner; | ||
| return new FrozenDocumentService( | ||
| resolvedUrl, | ||
| this.readOnly, | ||
| await factory?.createDocumentService(resolvedUrl), | ||
| ); | ||
| } | ||
|
|
@@ -72,26 +89,67 @@ class FrozenDocumentService | |
| extends TypedEventEmitter<IDocumentServiceEvents> | ||
| implements IDocumentService | ||
| { | ||
| private disposed = false; | ||
| private readonly pendingConnectRejecters = new Set<(reason: Error) => void>(); | ||
|
|
||
| constructor( | ||
| public readonly resolvedUrl: IResolvedUrl, | ||
| private readonly readOnly: boolean, | ||
| private readonly documentService?: IDocumentService, | ||
| ) { | ||
| super(); | ||
| // When readOnly, advertise the storageOnly policy. The connectionManager short-circuits | ||
| // on it: it synthesizes a FrozenDeltaStream itself and never calls | ||
| // connectToDeltaStream, and the readOnlyInfo getter forces the container to read-only | ||
| // because the live connection is a FrozenDeltaStream. | ||
| this.policies = readOnly ? { storageOnly: true } : {}; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deep Review: Writable-frozen loads with New correctness gap on a path the prior reviews didn't trace. Walking it end-to-end:
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 ( Suggested options:
Whichever you pick, please add a regression test in the writable-frozen suite that loads from pending state containing Open design questions:
|
||
| } | ||
|
|
||
| public readonly policies: IDocumentServicePolicies = { | ||
| storageOnly: true, | ||
| }; | ||
| public readonly policies: IDocumentServicePolicies; | ||
| async connectToStorage(): Promise<IDocumentStorageService> { | ||
| return new FrozenDocumentStorageService(await this.documentService?.connectToStorage()); | ||
| } | ||
| async connectToDeltaStorage(): Promise<IDocumentDeltaStorageService> { | ||
| return frozenDocumentDeltaStorageService; | ||
| } | ||
| async connectToDeltaStream(client: IClient): Promise<IDocumentDeltaConnection> { | ||
| return new FrozenDeltaStream(); | ||
| if (this.readOnly) { | ||
| // connectionManager short-circuits via policies.storageOnly before reaching here in | ||
| // the read-only path; this is a defensive fallback. | ||
|
markfields marked this conversation as resolved.
Outdated
|
||
| return new FrozenDeltaStream(); | ||
| } | ||
| if (client.mode !== "write") { | ||
|
anthony-murphy marked this conversation as resolved.
Outdated
anthony-murphy marked this conversation as resolved.
Outdated
|
||
| // Initial / read-mode connect: hand the runtime a writable-surface FrozenDeltaStream | ||
| // (DocWrite scope + not matched by isFrozenDeltaStreamConnection, so readOnlyInfo | ||
| // reports `readonly: false` and the runtime will accept DDS submissions). | ||
| return new FrozenDeltaStream({ readOnly: false }); | ||
|
anthony-murphy marked this conversation as resolved.
Outdated
|
||
| } | ||
| // Write upgrade: triggered the moment the runtime tries to send (sendMessages sees | ||
| // connectionMode === "read"). We can't honor it — there's no upstream and we won't | ||
| // fabricate a quorum join op. Hang the promise. The container settles into Disconnected | ||
| // (Connected → reconnecting → never resolves), DDS local apply continues to work, and | ||
| // submitted ops accumulate in the runtime's pendingStateManager (the outbox sees | ||
|
anthony-murphy marked this conversation as resolved.
Outdated
|
||
| // shouldSend() return false and skips actual send). That's the right representation | ||
| // for "load to accrue and capture pending state without publishing". | ||
| return new Promise<IDocumentDeltaConnection>((_, reject) => { | ||
| if (this.disposed) { | ||
| reject(new Error("FrozenDocumentService disposed")); | ||
| return; | ||
| } | ||
| this.pendingConnectRejecters.add(reject); | ||
| }); | ||
| } | ||
| dispose(): void { | ||
| this.disposed = true; | ||
| // Unblock any hung connect attempts so connectCore can exit cleanly. Without this, | ||
| // container.dispose() leaves the connectionManager's connect loop awaiting a promise | ||
| // that never resolves until garbage collection cleans up the closure. | ||
| const rejecters = [...this.pendingConnectRejecters]; | ||
| this.pendingConnectRejecters.clear(); | ||
| for (const reject of rejecters) { | ||
| reject(new Error("FrozenDocumentService disposed")); | ||
| } | ||
| } | ||
| dispose(): void {} | ||
| } | ||
|
|
||
| const frozenDocumentStorageServiceHandler = (): never => { | ||
|
|
@@ -129,60 +187,89 @@ const clientFrozenDeltaStream: IClient = { | |
| const clientIdFrozenDeltaStream: string = "storage-only client"; | ||
|
|
||
| /** | ||
| * Implementation of IDocumentDeltaConnection that does not support submitting | ||
| * or receiving ops. Used in storage-only mode and in frozen loads. | ||
| * Inert `IDocumentDeltaConnection` for frozen container loads. Has no server upstream: | ||
| * op and signal streams are empty, and `initialClients` contains only its own synthetic | ||
| * read-only client — which lets the connection state handler observe "self" in the audience | ||
| * and transition the container to Connected without waiting for a real join op or signal. | ||
| * | ||
|
markfields marked this conversation as resolved.
|
||
| * Two variants, selected via `options.readOnly` (default `true`): | ||
| * | ||
| * - **Read-only (default)** — claims show only `DocRead`. Used by storage-only loads (where connectionManager synthesizes one directly via `policies.storageOnly`) and by the forbidden / out-of-storage fallback paths. {@link isFrozenDeltaStreamConnection} matches this variant and drives the read-only forcing in `ConnectionManager.readOnlyInfo`. | ||
| * - **Writable (`{ readOnly: false }`)** — claims include `DocWrite` so the container surfaces as writable; not matched by `isFrozenDeltaStreamConnection`, so `readOnlyInfo` reports `readonly: false`. Connection mode stays `"read"`: advertising `"write"` would imply quorum membership, which we cannot honor. The connectionManager's read→write upgrade attempt that follows the first runtime submit is intercepted in `FrozenDocumentService.connectToDeltaStream` and hung indefinitely; the container then settles into Disconnected. | ||
| * | ||
| * Both variants nack any incoming `submit`: this connection has no upstream and | ||
| * `ConnectionManager.sendMessages` short-circuits read-mode ops to reconnect rather than calling | ||
| * `submit`, so under normal flow it should never fire. A nack reaching the connectionManager | ||
| * surfaces the misuse — and may close the container — which is the right defensive signal that | ||
| * something has bypassed the expected flow. | ||
| * | ||
| * `submitSignal` is a silent no-op for both variants. Signals are ephemeral and best-effort — | ||
| * runtime/presence subsystems may submit them at any point in the writable-frozen lifetime, and | ||
| * dropping them is the correct behavior here (we have no upstream). Closing the container or | ||
| * triggering a reconnect on a stray signal would be strictly worse than dropping it. | ||
| */ | ||
| export class FrozenDeltaStream | ||
| extends TypedEventEmitter<IDocumentDeltaConnectionEvents> | ||
| implements IDocumentDeltaConnection, IDisposable | ||
| { | ||
| clientId = clientIdFrozenDeltaStream; | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| claims = { | ||
| scopes: [ScopeType.DocRead], | ||
| } as ITokenClaims; | ||
| mode: ConnectionMode = "read"; | ||
| existing: boolean = true; | ||
| maxMessageSize: number = 0; | ||
| version: string = ""; | ||
| initialMessages: ISequencedDocumentMessage[] = []; | ||
| initialSignals: ISignalMessage[] = []; | ||
| initialClients: ISignalClient[] = [ | ||
| public readonly clientId: string = clientIdFrozenDeltaStream; | ||
| public readonly claims: ITokenClaims; | ||
| public readonly mode: ConnectionMode = "read"; | ||
| public readonly existing: boolean = true; | ||
| public readonly maxMessageSize: number = 0; | ||
| public readonly version: string = ""; | ||
| public readonly initialMessages: ISequencedDocumentMessage[] = []; | ||
| public readonly initialSignals: ISignalMessage[] = []; | ||
| public readonly initialClients: ISignalClient[] = [ | ||
| { client: clientFrozenDeltaStream, clientId: clientIdFrozenDeltaStream }, | ||
| ]; | ||
| serviceConfiguration: IClientConfiguration = { | ||
| public readonly serviceConfiguration: IClientConfiguration = { | ||
| maxMessageSize: 0, | ||
| blockSize: 0, | ||
| }; | ||
| checkpointSequenceNumber?: number | undefined = undefined; | ||
| public readonly checkpointSequenceNumber?: number | undefined = undefined; | ||
|
|
||
| public readonly readOnly: boolean; | ||
| public readonly storageOnlyReason: string | undefined; | ||
| public readonly readonlyConnectionReason: IConnectionStateChangeReason | undefined; | ||
|
|
||
| /** | ||
| * Connection which is not connected to socket. | ||
| * @param storageOnlyReason - Reason on why the connection to delta stream is not allowed. | ||
| * @param readonlyConnectionReason - reason/error if any which lead to using FrozenDeltaStream. | ||
| * @param options - Configuration: | ||
| * | ||
| * - `readOnly`: when `true` (the default), claims include only `DocRead` and {@link isFrozenDeltaStreamConnection} matches this instance (forcing the container read-only). When `false`, claims include `DocWrite` and the container surfaces as writable. | ||
| * - `storageOnlyReason`: surfaced via `IContainer.readOnlyInfo.storageOnlyReason` for the read-only variant. | ||
| * - `readonlyConnectionReason`: error/reason that led to using this stream as a fallback (e.g. forbidden delta stream connection); surfaced via the same readOnlyInfo path. | ||
| */ | ||
| constructor( | ||
| public readonly storageOnlyReason?: string, | ||
| public readonly readonlyConnectionReason?: IConnectionStateChangeReason, | ||
| ) { | ||
| constructor(options?: { | ||
| readOnly?: boolean; | ||
| storageOnlyReason?: string; | ||
| readonlyConnectionReason?: IConnectionStateChangeReason; | ||
| }) { | ||
| super(); | ||
| this.readOnly = options?.readOnly ?? true; | ||
| this.storageOnlyReason = options?.storageOnlyReason; | ||
| this.readonlyConnectionReason = options?.readonlyConnectionReason; | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| this.claims = { | ||
| scopes: this.readOnly ? [ScopeType.DocRead] : [ScopeType.DocRead, ScopeType.DocWrite], | ||
| } as ITokenClaims; | ||
| } | ||
|
|
||
| submit(messages: IDocumentMessage[]): void { | ||
| // Defensive nack: nothing should send on a frozen delta stream. If this fires, an | ||
| // invariant in connectionManager has changed and we want it to surface loudly. | ||
| this.emit( | ||
| "nack", | ||
| this.clientId, | ||
| messages.map((operation) => { | ||
| return { | ||
| operation, | ||
| content: { message: "Cannot submit with storage-only connection", code: 403 }, | ||
| }; | ||
| }), | ||
| messages.map((operation) => ({ | ||
| operation, | ||
| content: { message: "Cannot submit on a frozen delta stream", code: 403 }, | ||
| })), | ||
| ); | ||
| } | ||
| submitSignal(message: unknown): void { | ||
| this.emit("nack", this.clientId, { | ||
| operation: message, | ||
| content: { message: "Cannot submit signal with storage-only connection", code: 403 }, | ||
| }); | ||
|
|
||
| submitSignal(_message: unknown): void { | ||
| // Intentional no-op. See class JSDoc for rationale. | ||
| } | ||
|
|
||
| private _disposed = false; | ||
|
|
@@ -193,8 +280,14 @@ export class FrozenDeltaStream | |
| this._disposed = true; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Recognizes the read-only variant of {@link FrozenDeltaStream}. Drives the storage-only forcing | ||
| * in `ConnectionManager.readOnlyInfo`: only the read-only variant should make the container | ||
| * surface as read-only. | ||
| */ | ||
| export function isFrozenDeltaStreamConnection( | ||
| connection: unknown, | ||
| ): connection is FrozenDeltaStream { | ||
| return connection instanceof FrozenDeltaStream; | ||
| return connection instanceof FrozenDeltaStream && connection.readOnly; | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.