Skip to content
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

rc.2 Port: Fix "savedOp" metadata property propagation for grouped ops (#20837) #20902

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 26 additions & 23 deletions packages/runtime/container-runtime/src/containerRuntime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ import {
getLongStack,
} from "./opLifecycle/index.js";
import { DeltaManagerSummarizerProxy } from "./deltaManagerSummarizerProxy.js";
import { IBatchMetadata, IIdAllocationMetadata } from "./metadata.js";
import { IBatchMetadata, ISavedOpMetadata } from "./metadata.js";
import {
ContainerMessageType,
type InboundSequencedContainerRuntimeMessage,
Expand Down Expand Up @@ -652,11 +652,13 @@ type MessageWithContext =
message: InboundSequencedContainerRuntimeMessage;
modernRuntimeMessage: true;
local: boolean;
savedOp?: boolean;
}
| {
message: InboundSequencedContainerRuntimeMessageOrSystemMessage;
modernRuntimeMessage: false;
local: boolean;
savedOp?: boolean;
};

const summarizerRequestUrl = "_summarizer";
Expand Down Expand Up @@ -1017,7 +1019,7 @@ export class ContainerRuntime
// Id Compressor serializes final state (see getPendingLocalState()). As result, it needs to skip all ops that preceeded that state
// (such ops will be marked by Loader layer as savedOp === true)
// That said, in "delayed" mode it's possible that Id Compressor was never initialized before getPendingLocalState() is called.
// In such case we have to process all ops, including those marked with saveOp === true.
// In such case we have to process all ops, including those marked with savedOp === true.
private readonly skipSavedCompressorOps: boolean;

/**
Expand Down Expand Up @@ -2369,21 +2371,28 @@ export class ContainerRuntime
// We do not need to make a deep copy. Each layer will just replace message.contents itself,
// but will not modify the contents object (likely it will replace it on the message).
const messageCopy = { ...messageArg };
const savedOp = (messageCopy.metadata as ISavedOpMetadata)?.savedOp;
for (const message of this.remoteMessageProcessor.process(messageCopy)) {
if (modernRuntimeMessage) {
this.processCore({
// Cast it since we expect it to be this based on modernRuntimeMessage computation above.
// There is nothing really ensuring that anytime original message.type is Operation that
// the result messages will be so. In the end modern bool being true only directs to
// throw error if ultimately unrecognized without compat details saying otherwise.
message: message as InboundSequencedContainerRuntimeMessage,
local,
modernRuntimeMessage,
});
} else {
// Unrecognized message will be ignored.
this.processCore({ message, local, modernRuntimeMessage });
}
const msg: MessageWithContext = modernRuntimeMessage
? {
// Cast it since we expect it to be this based on modernRuntimeMessage computation above.
// There is nothing really ensuring that anytime original message.type is Operation that
// the result messages will be so. In the end modern bool being true only directs to
// throw error if ultimately unrecognized without compat details saying otherwise.
message: message as InboundSequencedContainerRuntimeMessage,
local,
modernRuntimeMessage,
}
: // Unrecognized message will be ignored.
{
message,
local,
modernRuntimeMessage,
};
msg.savedOp = savedOp;

// ensure that we observe any re-entrancy, and if needed, rebase ops
this.ensureNoDataModelChanges(() => this.processCore(msg));
}
}

Expand Down Expand Up @@ -2466,13 +2475,7 @@ export class ContainerRuntime
// stashed ops flow. The compressor is stashed with these ops already processed.
// That said, in idCompressorMode === "delayed", we might not serialize ID compressor, and
// thus we need to process all the ops.
if (
!(
this.skipSavedCompressorOps &&
(messageWithContext.message.metadata as IIdAllocationMetadata)?.savedOp ===
true
)
) {
if (!(this.skipSavedCompressorOps && messageWithContext.savedOp === true)) {
const range = messageWithContext.message.contents;
if (this._idCompressor === undefined) {
this.pendingIdCompressorOps.push(range);
Expand Down
4 changes: 2 additions & 2 deletions packages/runtime/container-runtime/src/metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ export interface IBlobMetadata {
}

/**
* The IdCompressor needs to know if this is a replayed savedOp as those need to be skipped in stashed ops scenarios.
* ContainerRuntime needs to know if this is a replayed savedOp as those need to be skipped in stashed ops scenarios.
*/
export interface IIdAllocationMetadata {
export interface ISavedOpMetadata {
savedOp?: boolean;
}