diff --git a/packages/runtime/container-runtime/src/containerRuntime.ts b/packages/runtime/container-runtime/src/containerRuntime.ts index 259d772552e7..cee3edfe8c8b 100644 --- a/packages/runtime/container-runtime/src/containerRuntime.ts +++ b/packages/runtime/container-runtime/src/containerRuntime.ts @@ -302,7 +302,6 @@ import { type ISummaryConfiguration, type ISummaryMetadataMessage, metadataBlobName, - OrderedClientCollection, OrderedClientElection, recentBatchInfoBlobName, RetriableSummaryError, @@ -2260,14 +2259,10 @@ export class ContainerRuntime logger: this.baseLogger, namespace: "OrderedClientElection", }); - const orderedClientCollection = new OrderedClientCollection( + const orderedClientElectionForSummarizer = new OrderedClientElection( orderedClientLogger, this.innerDeltaManager, this._quorum, - ); - const orderedClientElectionForSummarizer = new OrderedClientElection( - orderedClientLogger, - orderedClientCollection, this.electedSummarizerData ?? this.innerDeltaManager.lastSequenceNumber, SummarizerClientElection.isClientEligible, this.mc.config.getBoolean( diff --git a/packages/runtime/container-runtime/src/summary/index.ts b/packages/runtime/container-runtime/src/summary/index.ts index 49d725f3bb5a..5133fc9690ef 100644 --- a/packages/runtime/container-runtime/src/summary/index.ts +++ b/packages/runtime/container-runtime/src/summary/index.ts @@ -44,11 +44,9 @@ export { type ISummaryBaseConfiguration, } from "./summarizerTypes.js"; export { - type IOrderedClientCollection, type IOrderedClientElection, type ISerializedElection, type ITrackedClient, - OrderedClientCollection, OrderedClientElection, } from "./orderedClientElection.js"; export { diff --git a/packages/runtime/container-runtime/src/summary/orderedClientElection.ts b/packages/runtime/container-runtime/src/summary/orderedClientElection.ts index 88afdbeb8921..b5fbcccc6b2c 100644 --- a/packages/runtime/container-runtime/src/summary/orderedClientElection.ts +++ b/packages/runtime/container-runtime/src/summary/orderedClientElection.ts @@ -3,31 +3,20 @@ * Licensed under the MIT License. */ -/* eslint-disable @rushstack/no-new-null */ - import { TypedEventEmitter } from "@fluid-internal/client-utils"; import type { IDeltaManager } from "@fluidframework/container-definitions/internal"; -import type { - IEvent, - IEventProvider, - ITelemetryBaseLogger, -} from "@fluidframework/core-interfaces"; -import { assert } from "@fluidframework/core-utils/internal"; +import type { IEvent, IEventProvider } from "@fluidframework/core-interfaces"; import type { IClient, IQuorumClients, ISequencedClient, } from "@fluidframework/driver-definitions"; -import { - type ITelemetryLoggerExt, - UsageError, - createChildLogger, -} from "@fluidframework/telemetry-utils/internal"; +import type { ITelemetryLoggerExt } from "@fluidframework/telemetry-utils/internal"; import { summarizerClientType } from "./summarizerTypes.js"; // helper types for recursive readonly. -// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type +// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type, @rushstack/no-new-null export type ImmutablePrimitives = undefined | null | boolean | string | number | Function; export type Immutable = T extends ImmutablePrimitives ? T @@ -48,209 +37,6 @@ export interface ITrackedClient { readonly client: Immutable; } -/** - * Common contract for link nodes within an OrderedClientCollection. - */ -export interface ILinkNode { - readonly sequenceNumber: number; - youngerClient: ILinkedClient | undefined; -} - -/** - * Placeholder root node within an OrderedClientCollection; does not represent a client. - */ -export interface IRootLinkNode extends ILinkNode { - readonly sequenceNumber: -1; - readonly olderClient: undefined; -} - -/** - * Additional information required to keep track of the client within the doubly-linked list. - */ -export interface ILinkedClient extends ILinkNode, ITrackedClient { - olderClient: LinkNode; -} - -/** - * Any link node within OrderedClientCollection including the placeholder root node. - */ -export type LinkNode = IRootLinkNode | ILinkedClient; - -/** - * Events raised by an OrderedClientCollection. - */ -export interface IOrderedClientCollectionEvents extends IEvent { - /** - * Event fires when client is being added. - */ - ( - event: "addClient" | "removeClient", - listener: (client: ILinkedClient, sequenceNumber: number) => void, - ); -} - -/** - * Contract for a sorted collection of all clients in the quorum. - */ -export interface IOrderedClientCollection - extends IEventProvider { - /** - * Count of clients in the collection. - */ - readonly count: number; - /** - * Pointer to the oldest client in the collection. - */ - readonly oldestClient: ILinkedClient | undefined; - /** - * Returns a sorted array of all the clients in the collection. - */ - getAllClients(): ILinkedClient[]; -} - -/** - * Tracks clients in the Quorum. It maintains their order using their join op - * sequence numbers. - * Internally, the collection of clients is maintained in a doubly-linked list, - * with pointers to both the first and last nodes. - * The first (root) node is a placeholder to simplify logic and reduce null checking. - */ -export class OrderedClientCollection - extends TypedEventEmitter - implements IOrderedClientCollection -{ - /** - * Collection of ALL clients currently in the quorum, with client ids as keys. - */ - private readonly clientMap = new Map(); - /** - * Placeholder head node of linked list, for simplified null checking. - */ - private readonly rootNode: IRootLinkNode = { - sequenceNumber: -1, - olderClient: undefined, - youngerClient: undefined, - }; - /** - * Pointer to end of linked list, for optimized client adds. - */ - private _youngestClient: LinkNode = this.rootNode; - private readonly logger: ITelemetryLoggerExt; - - public get count(): number { - return this.clientMap.size; - } - public get oldestClient(): ILinkedClient | undefined { - return this.rootNode.youngerClient; - } - - constructor( - logger: ITelemetryBaseLogger, - deltaManager: Pick, "lastSequenceNumber">, - quorum: Pick, - ) { - super(); - this.logger = createChildLogger({ logger, namespace: "OrderedClientCollection" }); - const members = quorum.getMembers(); - for (const [clientId, client] of members) { - this.addClient(clientId, client); - } - - quorum.on("addMember", (clientId, client) => { - const newClient = this.addClient(clientId, client); - this.emit("addClient", newClient, deltaManager.lastSequenceNumber); - }); - quorum.on("removeMember", (clientId) => { - const sequenceNumber = deltaManager.lastSequenceNumber; - const removeClient = this.removeClient(clientId); - if (removeClient === undefined) { - this.logger.sendErrorEvent({ - eventName: "ClientNotFound", - clientId, - sequenceNumber, - }); - } else { - this.emit("removeClient", removeClient, sequenceNumber); - } - }); - } - - private addClient(clientId: string, client: ISequencedClient): ITrackedClient { - // Normal case is adding the latest client, which will bypass loop. - // Find where it belongs otherwise (maybe possible during initial load?). - assert( - client.sequenceNumber > -1, - 0x1f6 /* "Negative client sequence number not allowed" */, - ); - let currClient = this._youngestClient; - while (currClient.sequenceNumber > client.sequenceNumber) { - assert( - currClient.olderClient !== undefined, - 0x1f7 /* "Previous client should always be defined" */, - ); - // Note: If adding a client older than the elected client, it will not be automatically elected. - currClient = currClient.olderClient; - } - - // Now currClient is the node right before where the new client node should be. - const newClient: ILinkedClient = { - clientId, - sequenceNumber: client.sequenceNumber, - client: { ...client.client }, // shallow clone - olderClient: currClient, - youngerClient: currClient.youngerClient, - }; - - // Update prev node to point to this new node. - newClient.olderClient.youngerClient = newClient; - - if (newClient.youngerClient === undefined) { - // Update linked list end pointer to youngest client. - this._youngestClient = newClient; - } else { - // Update next node to point back to this new node. - newClient.youngerClient.olderClient = newClient; - } - - this.clientMap.set(clientId, newClient); - return newClient; - } - - private removeClient(clientId: string): ITrackedClient | undefined { - const removeClient = this.clientMap.get(clientId); - if (removeClient === undefined) { - return; - } - - // Update prev node to point to next node. - removeClient.olderClient.youngerClient = removeClient.youngerClient; - - if (removeClient.youngerClient === undefined) { - // Update linked list end pointer to youngest client. - this._youngestClient = removeClient.olderClient; - } else { - // Update next node to point back to previous node. - removeClient.youngerClient.olderClient = removeClient.olderClient; - } - - this.clientMap.delete(clientId); - return removeClient; - } - - /** - * Returns an array of all clients being tracked in order from oldest to newest. - */ - public getAllClients(): ILinkedClient[] { - const result: ILinkedClient[] = []; - let currClient: LinkNode = this.rootNode; - while (currClient.youngerClient !== undefined) { - result.push(currClient.youngerClient); - currClient = currClient.youngerClient; - } - return result; - } -} - /** * Events raised by an OrderedClientElection. */ @@ -347,74 +133,92 @@ export interface IOrderedClientElection extends IEventProvider = { ...client.client }; // shallow clone + return { + clientId, + sequenceNumber: client.sequenceNumber, + client: clientClone, + }; +} + /** - * Adapter for OrderedClientCollection, with the purpose of deterministically maintaining - * a currently elected client, excluding ineligible clients, in a distributed fashion. - * This can be true as long as incrementElectedClient and resetElectedClient calls - * are called under the same conditions for all clients. + * Deterministically maintains a currently elected client by reading quorum members directly, + * excluding ineligible clients. Observes quorum membership events to detect when clients join + * or leave, enabling the graceful handoff protocol. + * + * This class tracks electedClient and electedParent separately. This allows us to handle the case + * where a new interactive parent client has been elected, but the summarizer is still doing work, so + * a new summarizer should not yet be spawned. In this case, changing electedParent will cause SummaryManager + * to stop the current summarizer, but a new summarizer will not be spawned until the old summarizer client has + * left the quorum. + * + * Details: + * + * electedParent is the interactive client that has been elected to spawn a summarizer. It is typically the oldest + * eligible interactive client in the quorum. Only the electedParent is permitted to spawn a summarizer. + * Once elected, this client will remain the electedParent until it leaves the quorum or the summarizer that + * it spawned stops producing summaries, at which point a new electedParent will be chosen. + * + * electedClient is the non-interactive summarizer client if one exists. If not, then electedClient is equal to + * electedParent. If electedParent === electedClient, this is the signal for electedParent to spawn a new + * electedClient. Once a summarizer client becomes electedClient, a new summarizer will not be spawned until + * electedClient leaves the quorum. + * + * A typical sequence looks like this: + * + * i. Begin by electing A. electedParent === A, electedClient === A. + * + * ii. SummaryManager running on A spawns a summarizer client, A'. electedParent === A, electedClient === A' + * + * iii. A' stops producing summaries. A new parent client, B, is elected. electedParent === B, electedClient === A' + * + * iv. SummaryManager running on A detects the change to electedParent and tells the summarizer to stop, but A' + * is in mid-summarization. No new summarizer is spawned, as electedParent !== electedClient. + * + * v. A' completes its summary, and the summarizer and backing client are torn down. + * + * vi. A' leaves the quorum, and B takes its place as electedClient. electedParent === B, electedClient === B + * + * vii. SummaryManager running on B spawns a summarizer client, B'. electedParent === B, electedClient === B' */ export class OrderedClientElection extends TypedEventEmitter implements IOrderedClientElection { - private _eligibleCount: number = 0; - private _electedClient: ILinkedClient | undefined; - private _electedParent: ILinkedClient | undefined; + private _electedClient: ITrackedClient | undefined; + private _electedParent: ITrackedClient | undefined; private _electionSequenceNumber: number; public get eligibleCount(): number { - return this._eligibleCount; + return this.getAllEligibleClients().length; } public get electionSequenceNumber(): number { return this._electionSequenceNumber; } - /** - * OrderedClientCollection tracks electedClient and electedParent separately. This allows us to handle the case - * where a new interactive parent client has been elected, but the summarizer is still doing work, so - * a new summarizer should not yet be spawned. In this case, changing electedParent will cause SummaryManager - * to stop the current summarizer, but a new summarizer will not be spawned until the old summarizer client has - * left the quorum. - * - * Details: - * - * electedParent is the interactive client that has been elected to spawn a summarizer. It is typically the oldest - * eligible interactive client in the quorum. Only the electedParent is permitted to spawn a summarizer. - * Once elected, this client will remain the electedParent until it leaves the quorum or the summarizer that - * it spawned stops producing summaries, at which point a new electedParent will be chosen. - * - * electedClient is the non-interactive summarizer client if one exists. If not, then electedClient is equal to - * electedParent. If electedParent === electedClient, this is the signal for electedParent to spawn a new - * electedClient. Once a summarizer client becomes electedClient, a new summarizer will not be spawned until - * electedClient leaves the quorum. - * - * A typical sequence looks like this: - * - * i. Begin by electing A. electedParent === A, electedClient === A. - * - * ii. SummaryManager running on A spawns a summarizer client, A'. electedParent === A, electedClient === A' - * - * iii. A' stops producing summaries. A new parent client, B, is elected. electedParent === B, electedClient === A' - * - * iv. SummaryManager running on A detects the change to electedParent and tells the summarizer to stop, but A' - * is in mid-summarization. No new summarizer is spawned, as electedParent !== electedClient. - * - * v. A' completes its summary, and the summarizer and backing client are torn down. - * - * vi. A' leaves the quorum, and B takes its place as electedClient. electedParent === B, electedClient === B - * - * vii. SummaryManager running on B spawns a summarizer client, B'. electedParent === B, electedClient === B' + * Currently elected client. + * @see {@link IOrderedClientElection.electedClient} */ - public get electedClient(): ILinkedClient | undefined { + public get electedClient(): ITrackedClient | undefined { return this._electedClient; } - public get electedParent(): ILinkedClient | undefined { + /** + * Currently elected parent client. + * @see {@link IOrderedClientElection.electedParent} + */ + public get electedParent(): ITrackedClient | undefined { return this._electedParent; } constructor( private readonly logger: ITelemetryLoggerExt, - private readonly orderedClientCollection: IOrderedClientCollection, + deltaManager: Pick, "lastSequenceNumber">, + private readonly quorum: Pick, /** * Serialized state from summary or current sequence number at time of load if new. */ @@ -423,58 +227,132 @@ export class OrderedClientElection private readonly recordPerformanceEvents: boolean = false, ) { super(); - let initialClient: ILinkedClient | undefined; - let initialParent: ILinkedClient | undefined; - for (const client of orderedClientCollection.getAllClients()) { - this.addClient(client, 0); - if (typeof initialState !== "number") { - if (client.clientId === initialState.electedClientId) { - initialClient = client; - if ( - initialState.electedParentId === undefined && - client.client.details.type !== summarizerClientType - ) { - // If there was no elected parent in the serialized data, use this one. - initialParent = client; + + if (typeof initialState === "number") { + this._electionSequenceNumber = initialState; + this._electedParent = this.findOldestEligibleParent(); + this._electedClient = this._electedParent; + // Check if a summarizer is already in quorum and should supersede + const summarizer = this.findSummarizerInQuorum(); + if (summarizer !== undefined) { + this._electedClient = summarizer; + } + } else { + this._electionSequenceNumber = initialState.electionSequenceNumber; + const members = quorum.getMembers(); + + // Try to restore the elected parent + let initialParent: ITrackedClient | undefined; + if (initialState.electedParentId !== undefined) { + const member = members.get(initialState.electedParentId); + if (member !== undefined) { + const tracked = toTrackedClient(initialState.electedParentId, member); + if (this.isEligibleFn(tracked)) { + initialParent = tracked; } } - if (client.clientId === initialState.electedParentId) { - initialParent = client; + } + + // Try to restore the elected client + let initialClient: ITrackedClient | undefined; + if (initialState.electedClientId !== undefined) { + const member = members.get(initialState.electedClientId); + if (member === undefined) { + // Cannot find initially elected client, so elect undefined. + this.logger.sendErrorEvent({ + eventName: "InitialElectedClientNotFound", + electionSequenceNumber: initialState.electionSequenceNumber, + expectedClientId: initialState.electedClientId, + electedClientId: undefined, + clientCount: members.size, + }); + } else { + const tracked = toTrackedClient(initialState.electedClientId, member); + if (this.isEligibleFn(tracked)) { + initialClient = tracked; + } else { + // Initially elected client is ineligible — elect next eligible after it. + const fallback = this.findNextEligibleParentAfter(tracked.sequenceNumber); + initialClient = fallback; + initialParent = fallback; + this.logger.sendErrorEvent({ + eventName: "InitialElectedClientIneligible", + electionSequenceNumber: initialState.electionSequenceNumber, + expectedClientId: initialState.electedClientId, + electedClientId: initialClient?.clientId, + }); + } } } - } - orderedClientCollection.on("addClient", (client, seq) => this.addClient(client, seq)); - orderedClientCollection.on("removeClient", (client, seq) => - this.removeClient(client, seq), - ); - if (typeof initialState === "number") { - this._electionSequenceNumber = initialState; - } else { - // Override the initially elected client with the initial state. - if (initialClient?.clientId !== initialState.electedClientId) { - // Cannot find initially elected client, so elect undefined. - this.logger.sendErrorEvent({ - eventName: "InitialElectedClientNotFound", - electionSequenceNumber: initialState.electionSequenceNumber, - expectedClientId: initialState.electedClientId, - electedClientId: initialClient?.clientId, - clientCount: orderedClientCollection.count, - }); - } else if (initialClient !== undefined && !isEligibleFn(initialClient)) { - // Initially elected client is ineligible, so elect next eligible client. - initialClient = initialParent = this.findFirstEligibleParent(initialParent); - this.logger.sendErrorEvent({ - eventName: "InitialElectedClientIneligible", - electionSequenceNumber: initialState.electionSequenceNumber, - expectedClientId: initialState.electedClientId, - electedClientId: initialClient?.clientId, - }); + // If no parent was found but we have an interactive client, use it + if ( + initialParent === undefined && + initialClient !== undefined && + initialClient.client.details.type !== summarizerClientType + ) { + initialParent = initialClient; } + this._electedParent = initialParent; this._electedClient = initialClient; - this._electionSequenceNumber = initialState.electionSequenceNumber; } + + // Updates tracking when a new client joins the quorum. + // Will automatically elect the new client if none is currently elected. + quorum.on("addMember", (clientId: string, client: ISequencedClient) => { + const sequenceNumber = deltaManager.lastSequenceNumber; + const tracked = toTrackedClient(clientId, client); + if (!this.isEligibleFn(tracked)) { + return; + } + + const newClientIsSummarizer = client.client.details.type === summarizerClientType; + const electedClientIsSummarizer = + this._electedClient !== undefined && isSummarizerClient(this._electedClient); + + if ( + this._electedClient === undefined || + (!electedClientIsSummarizer && newClientIsSummarizer) + ) { + // Elect this client: either no one is elected, or a summarizer supersedes an interactive client. + this.tryElectingClient(tracked, sequenceNumber, "AddClient"); + } else if (this._electedParent === undefined && !newClientIsSummarizer) { + // This is an odd case. If the _electedClient is set, the _electedParent should be as well. + this.tryElectingParent(tracked, sequenceNumber, "AddClient"); + } + }); + + // Updates tracking when a client leaves the quorum. + // Will automatically elect the next oldest client if the currently elected client is removed. + quorum.on("removeMember", (clientId: string) => { + const sequenceNumber = deltaManager.lastSequenceNumber; + + // Removing the _electedClient. There are 2 possible cases: + if (this._electedClient?.clientId === clientId) { + if (this._electedParent?.clientId === clientId) { + // 1. The _electedClient is an interactive client that has left the quorum. + // Automatically shift to next oldest client. + // In this case _electedClient === _electedParent, so the next parent is also the next client. + const nextClient = this.findNextEligibleParent(); + this.tryElectingClient(nextClient, sequenceNumber, "RemoveClient"); + } else { + // 2. The _electedClient is a summarizer that we've been allowing to finish its work. + // Let the _electedParent become the _electedClient so that it can start its own summarizer. + this.tryElectingClient( + this._electedParent, + sequenceNumber, + "RemoveSummarizerClient", + ); + } + } else if (this._electedParent?.clientId === clientId) { + // Removing the _electedParent (but not _electedClient). + // Shift to the next oldest parent, but do not replace the _electedClient, + // which is a summarizer that is still doing work. + const nextParent = this.findNextEligibleParent(); + this.tryElectingParent(nextParent, sequenceNumber, "RemoveClient"); + } + }); } /** @@ -483,7 +361,7 @@ export class OrderedClientElection * we will set _electedClient, and we will set _electedParent if this is an interactive client. */ private tryElectingClient( - client: ILinkedClient | undefined, + client: ITrackedClient | undefined, sequenceNumber: number, reason: string, ): void { @@ -495,9 +373,9 @@ export class OrderedClientElection reason, ); let change = false; - const isSummarizerClient = client?.client.details.type === summarizerClientType; + const isSummarizer = client !== undefined && isSummarizerClient(client); const prevClient = this._electedClient; - if (this._electedClient !== client) { + if (this._electedClient?.clientId !== client?.clientId) { this.sendPerformanceEvent( "ClientElected", client, @@ -510,7 +388,7 @@ export class OrderedClientElection this._electedClient = client; change = true; } - if (this._electedParent !== client && !isSummarizerClient) { + if (this._electedParent?.clientId !== client?.clientId && !isSummarizer) { this.sendPerformanceEvent( "InteractiveClientElected", client, @@ -528,7 +406,7 @@ export class OrderedClientElection } private tryElectingParent( - client: ILinkedClient | undefined, + client: ITrackedClient | undefined, sequenceNumber: number, reason: string, ): void { @@ -539,7 +417,7 @@ export class OrderedClientElection false /* forceSend */, reason, ); - if (this._electedParent !== client) { + if (this._electedParent?.clientId !== client?.clientId) { this.sendPerformanceEvent( "ParentElected", client, @@ -553,101 +431,59 @@ export class OrderedClientElection } /** - * Helper function to find the first eligible parent client starting with the passed in client, - * or undefined if none are eligible. - * @param client - client to start checking - * @returns oldest eligible client starting with passed in client or undefined if none. + * Find the oldest eligible interactive (non-summarizer) client in the quorum. + * @returns the oldest eligible parent client, or undefined if none are eligible. */ - private findFirstEligibleParent( - client: ILinkedClient | undefined, - ): ILinkedClient | undefined { - let candidateClient = client; - while ( - candidateClient !== undefined && - (!this.isEligibleFn(candidateClient) || - candidateClient.client.details.type === summarizerClientType) - ) { - candidateClient = candidateClient.youngerClient; - } - return candidateClient; + private findOldestEligibleParent(): ITrackedClient | undefined { + return this.findNextEligibleParentAfter(-1); } /** - * Updates tracking for when a new client is added to the collection. - * Will automatically elect that new client if none is elected currently. - * @param client - client added to the collection - * @param sequenceNumber - sequence number when client was added + * Find the next eligible parent after the current one, wrapping around to + * the oldest if no younger client is found. + * @returns the next eligible parent client, or undefined if none are eligible. */ - private addClient(client: ILinkedClient, sequenceNumber: number): void { - this.sendPerformanceEvent("AddClient", client, sequenceNumber); - if (this.isEligibleFn(client)) { - this._eligibleCount++; - const newClientIsSummarizer = client.client.details.type === summarizerClientType; - const electedClientIsSummarizer = - this._electedClient?.client.details.type === summarizerClientType; - // Note that we allow a summarizer client to supersede an interactive client as elected client. + private findNextEligibleParent(): ITrackedClient | undefined { + const currentParentSeq = this._electedParent?.sequenceNumber ?? -1; + return ( + this.findNextEligibleParentAfter(currentParentSeq) ?? this.findOldestEligibleParent() + ); + } + + /** + * Find the next eligible interactive client after the given sequence number. + * @param sequenceNumber - sequence number to start searching after. + * @returns the next eligible parent client, or undefined if none are found with a higher sequence number. + */ + private findNextEligibleParentAfter(sequenceNumber: number): ITrackedClient | undefined { + let nextOldest: ITrackedClient | undefined; + for (const [clientId, client] of this.quorum.getMembers()) { + const tracked = toTrackedClient(clientId, client); if ( - this._electedClient === undefined || - (!electedClientIsSummarizer && newClientIsSummarizer) + this.isEligibleFn(tracked) && + client.client.details.type !== summarizerClientType && + client.sequenceNumber > sequenceNumber && + (nextOldest === undefined || client.sequenceNumber < nextOldest.sequenceNumber) ) { - this.tryElectingClient(client, sequenceNumber, "AddClient"); - } else if (this._electedParent === undefined && !newClientIsSummarizer) { - // This is an odd case. If the _electedClient is set, the _electedParent should be as well. - this.tryElectingParent(client, sequenceNumber, "AddClient"); + nextOldest = tracked; } } + return nextOldest; } /** - * Updates tracking for when an existing client is removed from the collection. - * Will automatically elect next oldest client if currently elected is removed. - * @param client - client removed from the collection - * @param sequenceNumber - sequence number when client was removed + * Find any summarizer-type client currently in the quorum. */ - private removeClient(client: ILinkedClient, sequenceNumber: number): void { - this.sendPerformanceEvent("RemoveClient", client, sequenceNumber); - if (this.isEligibleFn(client)) { - this._eligibleCount--; - if (this._electedClient === client) { - // Removing the _electedClient. There are 2 possible cases: - if (this._electedParent === client) { - // 1. The _electedClient is an interactive client that has left the quorum. - // Automatically shift to next oldest client. - const nextClient = - this.findFirstEligibleParent(this._electedParent?.youngerClient) ?? - this.findFirstEligibleParent(this.orderedClientCollection.oldestClient); - this.tryElectingClient(nextClient, sequenceNumber, "RemoveClient"); - } else { - // 2. The _electedClient is a summarizer that we've been allowing to finish its work. - // Let the _electedParent become the _electedClient so that it can start its own summarizer. - if (this._electedClient.client.details.type !== summarizerClientType) { - throw new UsageError("Elected client should be a summarizer client 1"); - } - this.tryElectingClient( - this._electedParent, - sequenceNumber, - "RemoveSummarizerClient", - ); - } - } else if (this._electedParent === client) { - // Removing the _electedParent (but not _electedClient). - // Shift to the next oldest parent, but do not replace the _electedClient, - // which is a summarizer that is still doing work. - if (this._electedClient?.client.details.type !== summarizerClientType) { - throw new UsageError("Elected client should be a summarizer client 2"); + private findSummarizerInQuorum(): ITrackedClient | undefined { + for (const [clientId, client] of this.quorum.getMembers()) { + if (client.client.details.type === summarizerClientType) { + const tracked = toTrackedClient(clientId, client); + if (this.isEligibleFn(tracked)) { + return tracked; } - const nextParent = - this.findFirstEligibleParent(this._electedParent?.youngerClient) ?? - this.findFirstEligibleParent(this.orderedClientCollection.oldestClient); - this.tryElectingParent(nextParent, sequenceNumber, "RemoveClient"); } } - } - - public getAllEligibleClients(): ITrackedClient[] { - return this.orderedClientCollection - .getAllClients() - .filter((client) => this.isEligibleFn(client)); + return undefined; } /** @@ -655,10 +491,11 @@ export class OrderedClientElection * and no client has been elected. */ public resetElectedClient(sequenceNumber: number): void { - const firstClient = this.findFirstEligibleParent( - this.orderedClientCollection.oldestClient, - ); - if (this._electedClient === undefined || this._electedClient === this._electedParent) { + const firstClient = this.findOldestEligibleParent(); + if ( + this._electedClient === undefined || + this._electedClient.clientId === this._electedParent?.clientId + ) { this.tryElectingClient(firstClient, sequenceNumber, "ResetElectedClient"); } else { // The _electedClient is a summarizer and should not be replaced until it leaves the quorum. @@ -668,10 +505,18 @@ export class OrderedClientElection } public peekNextElectedClient(): ITrackedClient | undefined { - return ( - this.findFirstEligibleParent(this._electedParent?.youngerClient) ?? - this.findFirstEligibleParent(this.orderedClientCollection.oldestClient) - ); + return this.findNextEligibleParent(); + } + + public getAllEligibleClients(): ITrackedClient[] { + const result: ITrackedClient[] = []; + for (const [clientId, client] of this.quorum.getMembers()) { + const tracked = toTrackedClient(clientId, client); + if (this.isEligibleFn(tracked)) { + result.push(tracked); + } + } + return result.sort((a, b) => a.sequenceNumber - b.sequenceNumber); } public serialize(): ISerializedElection { @@ -684,7 +529,7 @@ export class OrderedClientElection private sendPerformanceEvent( eventName: string, - client: ILinkedClient | undefined, + client: ITrackedClient | undefined, sequenceNumber: number, forceSend: boolean = false, reason?: string, diff --git a/packages/runtime/container-runtime/src/test/summary/orderedClientElection.spec.ts b/packages/runtime/container-runtime/src/test/summary/orderedClientElection.spec.ts index 7501ee6e2ab6..72b62acff664 100644 --- a/packages/runtime/container-runtime/src/test/summary/orderedClientElection.spec.ts +++ b/packages/runtime/container-runtime/src/test/summary/orderedClientElection.spec.ts @@ -9,18 +9,15 @@ import type { ISequencedClient } from "@fluidframework/driver-definitions"; import { MockLogger } from "@fluidframework/telemetry-utils/internal"; import { - type IOrderedClientCollection, type IOrderedClientElection, type ISerializedElection, type ITrackedClient, - OrderedClientCollection, OrderedClientElection, } from "../../summary/index.js"; import { TestQuorumClients } from "./testQuorumClients.js"; describe("Ordered Client Collection", () => { - let orderedClients: IOrderedClientCollection; const mockLogger = new MockLogger(); const testQuorum = new TestQuorumClients(); @@ -47,38 +44,6 @@ describe("Ordered Client Collection", () => { currentSequenceNumber += opCount; testQuorum.removeClient(clientId); } - function createOrderedClientCollection( - initialClients: [id: string, seq: number, int: boolean][] = [], - ): IOrderedClientCollection { - for (const [id, seq, int] of initialClients) { - addClient(id, seq, int); - } - orderedClients = new OrderedClientCollection(mockLogger, testDeltaManager, testQuorum); - return orderedClients; - } - function assertCollectionState(expectedCount: number, message = "") { - const prefix = message ? `${message} - ` : ""; - assert.strictEqual( - orderedClients.count, - expectedCount, - `${prefix}Invalid client count: ${orderedClients.count} !== ${expectedCount}`, - ); - } - function assertOrderedClientIds(...expectedIds: string[]) { - const actualIds = orderedClients.getAllClients(); - assert.strictEqual( - actualIds.length, - expectedIds.length, - `Unexpected count of ordered client ids: ${actualIds.length} !== ${expectedIds.length}`, - ); - for (let i = 0; i < actualIds.length; i++) { - assert.strictEqual( - actualIds[i].clientId, - expectedIds[i], - `Unexpected ordered client id at index ${i}: ${actualIds[i].clientId} !== ${expectedIds[i]}`, - ); - } - } afterEach(() => { mockLogger.clear(); @@ -86,36 +51,6 @@ describe("Ordered Client Collection", () => { currentSequenceNumber = 0; }); - describe("Initialize", () => { - it("Should initialize with empty quorum", () => { - createOrderedClientCollection(); - assertCollectionState(0); - assertOrderedClientIds(); - }); - - it("Should initialize with correct count", () => { - createOrderedClientCollection([ - ["a", 1, true], - ["b", 2, true], - ["s", 5, false], - ["c", 9, true], - ]); - assertCollectionState(4); - assertOrderedClientIds("a", "b", "s", "c"); - }); - - it("Should initialize in correct order", () => { - createOrderedClientCollection([ - ["c", 9, true], - ["b", 2, true], - ["a", 1, true], - ["s", 5, false], - ]); - assertCollectionState(4); - assertOrderedClientIds("a", "b", "s", "c"); - }); - }); - describe("Ordered Client Election", () => { let election: IOrderedClientElection; let electionEventCount = 0; @@ -123,7 +58,9 @@ describe("Ordered Client Collection", () => { initialClients: [id: string, seq: number, int: boolean][] = [], initialState?: ISerializedElection, ): IOrderedClientElection { - createOrderedClientCollection(initialClients); + for (const [id, seq, int] of initialClients) { + addClient(id, seq, int); + } if ( initialState !== undefined && initialState.electionSequenceNumber > currentSequenceNumber @@ -132,7 +69,8 @@ describe("Ordered Client Collection", () => { } election = new OrderedClientElection( mockLogger.toTelemetryLogger(), - orderedClients, + testDeltaManager, + testQuorum, initialState ?? currentSequenceNumber, (c: ITrackedClient) => c.client.details.capabilities.interactive, ); @@ -146,13 +84,11 @@ describe("Ordered Client Collection", () => { election.resetElectedClient(sequenceNumber); } function assertElectionState( - expectedTotalCount: number, expectedEligibleCount: number, expectedElectedClientId: string | undefined, expectedElectionSequenceNumber: number, message = "", ) { - assertCollectionState(expectedTotalCount, message); const prefix = message ? `${message} - ` : ""; assert.strictEqual( election.eligibleCount, @@ -198,15 +134,9 @@ describe("Ordered Client Collection", () => { }); describe("Initialize", () => { - const emptySerializedElection = { - electedClientId: undefined, - electedParentId: undefined, - electionSequenceNumber: 101, - }; - it("Should initialize with empty quorum", () => { createOrderedClientElection(); - assertElectionState(0, 0, undefined, 0); + assertElectionState(0, undefined, 0); assertOrderedEligibleClientIds(); }); @@ -217,28 +147,24 @@ describe("Ordered Client Collection", () => { ["s", 5, false], ["c", 9, true], ]); - assertElectionState(4, 3, "a", 9); + assertElectionState(3, "a", 9); assertOrderedEligibleClientIds("a", "b", "c"); }); it("Should initialize with empty quorum at specific sequence number", () => { currentSequenceNumber = 99; createOrderedClientElection(); - assertElectionState(0, 0, undefined, 99); + assertElectionState(0, undefined, 99); assertOrderedEligibleClientIds(); }); it("Should initialize with empty quorum and initial state", () => { - createOrderedClientElection(undefined, emptySerializedElection); - assertElectionState(0, 0, undefined, 101); - assertOrderedEligibleClientIds(); - }); - - it("Should log error with empty quorum and initially elected client", () => { - const clientId = "x"; - createOrderedClientElection(undefined, emptySerializedElection); - assertElectionState(0, 0, undefined, 101); - mockLogger.matchEvents([{ eventName: "InitialElectedClientNotFound", clientId }]); + createOrderedClientElection(undefined, { + electedClientId: undefined, + electedParentId: undefined, + electionSequenceNumber: 101, + }); + assertElectionState(0, undefined, 101); assertOrderedEligibleClientIds(); }); @@ -252,7 +178,7 @@ describe("Ordered Client Collection", () => { ], { electedClientId: "b", electedParentId: "b", electionSequenceNumber: 4321 }, ); - assertElectionState(4, 3, "b", 4321); + assertElectionState(3, "b", 4321); assertOrderedEligibleClientIds("a", "b", "c"); }); @@ -267,11 +193,12 @@ describe("Ordered Client Collection", () => { ], { electedClientId: "s", electedParentId: "s", electionSequenceNumber: 4321 }, ); - assertElectionState(5, 3, "c", 4321); + assertElectionState(3, "c", 4321); mockLogger.matchEvents([ { eventName: "InitialElectedClientIneligible", - clientId: "s", + electionSequenceNumber: 4321, + expectedClientId: "s", electedClientId: "c", }, ]); @@ -288,11 +215,12 @@ describe("Ordered Client Collection", () => { ], { electedClientId: "s", electedParentId: "s", electionSequenceNumber: 4321 }, ); - assertElectionState(4, 2, undefined, 4321); + assertElectionState(2, undefined, 4321); mockLogger.matchEvents([ { eventName: "InitialElectedClientIneligible", - clientId: "s", + electionSequenceNumber: 4321, + expectedClientId: "s", electedClientId: undefined, }, ]); @@ -309,8 +237,16 @@ describe("Ordered Client Collection", () => { ], { electedClientId: "x", electedParentId: "x", electionSequenceNumber: 4321 }, ); - assertElectionState(4, 3, undefined, 4321); - mockLogger.matchEvents([{ eventName: "InitialElectedClientNotFound", clientId: "x" }]); + assertElectionState(3, undefined, 4321); + mockLogger.matchEvents([ + { + eventName: "InitialElectedClientNotFound", + electionSequenceNumber: 4321, + expectedClientId: "x", + electedClientId: undefined, + clientCount: 4, + }, + ]); assertOrderedEligibleClientIds("a", "b", "c"); }); }); @@ -324,7 +260,7 @@ describe("Ordered Client Collection", () => { ["c", 9, true], ]); addClient("n", 100, false); - assertElectionState(5, 3, "a", 9); + assertElectionState(3, "a", 9); assertEvents(0); assertOrderedEligibleClientIds("a", "b", "c"); }); @@ -332,7 +268,7 @@ describe("Ordered Client Collection", () => { it("Should add ineligible client to empty quorum without impacting eligible clients", () => { createOrderedClientElection(); addClient("n", 100, false); - assertElectionState(1, 0, undefined, 0); + assertElectionState(0, undefined, 0); assertEvents(0); assertOrderedEligibleClientIds(); }); @@ -340,7 +276,7 @@ describe("Ordered Client Collection", () => { it("Should add and elect eligible client to empty quorum", () => { createOrderedClientElection(); addClient("n", 100); - assertElectionState(1, 1, "n", 100); + assertElectionState(1, "n", 100); assertEvents(1); assertOrderedEligibleClientIds("n"); }); @@ -353,7 +289,7 @@ describe("Ordered Client Collection", () => { ["c", 9, true], ]); addClient("n", 100); - assertElectionState(5, 4, "a", 9); + assertElectionState(4, "a", 9); assertEvents(0); assertOrderedEligibleClientIds("a", "b", "c", "n"); }); @@ -367,7 +303,7 @@ describe("Ordered Client Collection", () => { ["c", 9, true], ]); addClient("n", 3); - assertElectionState(5, 4, "a", 9); + assertElectionState(4, "a", 9); assertEvents(0); assertOrderedEligibleClientIds("a", "b", "n", "c"); }); @@ -381,38 +317,13 @@ describe("Ordered Client Collection", () => { ["c", 9, true], ]); addClient("n", 0); - assertElectionState(5, 4, "a", 9); + assertElectionState(4, "a", 9); assertEvents(0); assertOrderedEligibleClientIds("n", "a", "b", "c"); }); }); describe("Remove Client", () => { - it("Should log error when removing a client from empty quorum", () => { - createOrderedClientElection(); - const clientId = "x"; - removeClient(clientId); - mockLogger.matchEvents([{ eventName: "ClientNotFound", clientId }]); - assertElectionState(0, 0, undefined, 0); - assertEvents(0); - assertOrderedEligibleClientIds(); - }); - - it("Should log error when removing a client that doesn't exist", () => { - createOrderedClientElection([ - ["a", 1, true], - ["b", 2, true], - ["s", 5, false], - ["c", 9, true], - ]); - const clientId = "x"; - removeClient(clientId); - mockLogger.matchEvents([{ eventName: "ClientNotFound", clientId }]); - assertElectionState(4, 3, "a", 9); - assertEvents(0); - assertOrderedEligibleClientIds("a", "b", "c"); - }); - it("Should remove ineligible client", () => { createOrderedClientElection([ ["a", 1, true], @@ -421,7 +332,7 @@ describe("Ordered Client Collection", () => { ["c", 9, true], ]); removeClient("s", 5); - assertElectionState(3, 3, "a", 9); + assertElectionState(3, "a", 9); assertEvents(0); assertOrderedEligibleClientIds("a", "b", "c"); }); @@ -434,7 +345,7 @@ describe("Ordered Client Collection", () => { ["c", 9, true], ]); removeClient("c", 5); - assertElectionState(3, 2, "a", 9); + assertElectionState(2, "a", 9); assertEvents(0); assertOrderedEligibleClientIds("a", "b"); }); @@ -447,7 +358,7 @@ describe("Ordered Client Collection", () => { ["c", 9, true], ]); removeClient("b", 5); - assertElectionState(3, 2, "a", 9); + assertElectionState(2, "a", 9); assertEvents(0); assertOrderedEligibleClientIds("a", "c"); }); @@ -460,7 +371,7 @@ describe("Ordered Client Collection", () => { ["c", 9, true], ]); removeClient("a", 5); - assertElectionState(3, 2, "b", 14); + assertElectionState(2, "b", 14); assertEvents(1); assertOrderedEligibleClientIds("b", "c"); }); @@ -476,9 +387,9 @@ describe("Ordered Client Collection", () => { { electedClientId: "s", electedParentId: "s", electionSequenceNumber: 4321 }, ); removeClient("s", 1111); - assertElectionState(3, 3, "c", 4321); + assertElectionState(3, "c", 4321); removeClient("c", 1111); - assertElectionState(2, 2, "a", 6543); + assertElectionState(2, "a", 6543); assertEvents(1); }); }); @@ -494,9 +405,9 @@ describe("Ordered Client Collection", () => { ], { electedClientId: "s", electedParentId: "s", electionSequenceNumber: 4321 }, ); - assertElectionState(4, 3, "b", 4321); + assertElectionState(3, "b", 4321); resetElectedClient(7777); - assertElectionState(4, 3, "a", 7777); + assertElectionState(3, "a", 7777); assertEvents(1); }); }); diff --git a/packages/runtime/container-runtime/src/test/summary/summarizerClientElection.spec.ts b/packages/runtime/container-runtime/src/test/summary/summarizerClientElection.spec.ts index e6c8e853ed52..fd2801d6a344 100644 --- a/packages/runtime/container-runtime/src/test/summary/summarizerClientElection.spec.ts +++ b/packages/runtime/container-runtime/src/test/summary/summarizerClientElection.spec.ts @@ -21,7 +21,6 @@ import { type ISerializedElection, type ISummarizer, type ISummaryCollectionOpEvents, - OrderedClientCollection, OrderedClientElection, SummarizerClientElection, SummaryManager, @@ -176,7 +175,8 @@ describe("Summarizer Client Election", () => { summaryCollectionEmitter, new OrderedClientElection( mockLogger.toTelemetryLogger(), - new OrderedClientCollection(mockLogger, testDeltaManager, testQuorum), + testDeltaManager, + testQuorum, initialState ?? currentSequenceNumber, SummarizerClientElection.isClientEligible, ),