From ad1cfc919d7fffa66742e1fba7a9376d29874516 Mon Sep 17 00:00:00 2001 From: Mark Fields Date: Wed, 22 Oct 2025 14:55:51 -0700 Subject: [PATCH 1/5] Don't assert if flush early returns --- .../src/opLifecycle/outbox.ts | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/runtime/container-runtime/src/opLifecycle/outbox.ts b/packages/runtime/container-runtime/src/opLifecycle/outbox.ts index f0b2937eece2..31a9d0de2c8e 100644 --- a/packages/runtime/container-runtime/src/opLifecycle/outbox.ts +++ b/packages/runtime/container-runtime/src/opLifecycle/outbox.ts @@ -316,7 +316,7 @@ export class Outbox { // If we're configured to flush partial batches, do that now and return (don't throw) if (this.params.config.flushPartialBatches) { - this.flushAll(); + this.flush(); return; } @@ -365,14 +365,6 @@ export class Outbox { * @param resubmitInfo - Key information when flushing a resubmitted batch. Undefined means this is not resubmit. */ public flush(resubmitInfo?: BatchResubmitInfo): void { - assert( - !this.isContextReentrant(), - 0xb7b /* Flushing must not happen while incoming changes are being processed */, - ); - this.flushAll(resubmitInfo); - } - - private flushAll(resubmitInfo?: BatchResubmitInfo): void { const allBatchesEmpty = this.idAllocationBatch.empty && this.blobAttachBatch.empty && this.mainBatch.empty; if (allBatchesEmpty) { @@ -387,6 +379,11 @@ export class Outbox { return; } + assert( + !this.isContextReentrant(), + 0xb7b /* Flushing must not happen while incoming changes are being processed */, + ); + // Don't use resubmittingBatchId for idAllocationBatch. // ID Allocation messages are not directly resubmitted so don't pass the resubmitInfo this.flushInternal({ @@ -409,6 +406,11 @@ export class Outbox { resubmittingBatchId: BatchId, resubmittingStagedBatch: boolean, ): void { + assert( + !this.isContextReentrant(), + "Flushing must not happen while incoming changes are being processed", + ); + const referenceSequenceNumber = this.params.getCurrentSequenceNumbers().referenceSequenceNumber; assert( From fbeadf2fbd3fe6584864cf33f6ea414c2f8a3431 Mon Sep 17 00:00:00 2001 From: Mark Fields Date: Wed, 22 Oct 2025 15:06:44 -0700 Subject: [PATCH 2/5] Add tests --- .../src/test/opLifecycle/outbox.spec.ts | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/packages/runtime/container-runtime/src/test/opLifecycle/outbox.spec.ts b/packages/runtime/container-runtime/src/test/opLifecycle/outbox.spec.ts index 70321eb46fb3..1767df4de1e7 100644 --- a/packages/runtime/container-runtime/src/test/opLifecycle/outbox.spec.ts +++ b/packages/runtime/container-runtime/src/test/opLifecycle/outbox.spec.ts @@ -17,6 +17,7 @@ import type { ISequencedDocumentMessage, } from "@fluidframework/driver-definitions/internal"; import { MockLogger } from "@fluidframework/telemetry-utils/internal"; +import { validateAssertionError } from "@fluidframework/test-runtime-utils/internal"; import type { ICompressionRuntimeOptions } from "../../compressionDefinitions.js"; import { CompressionAlgorithms } from "../../compressionDefinitions.js"; @@ -1092,6 +1093,58 @@ describe("Outbox", () => { assert.strictEqual(state.opsResubmitted, opsResubmitted, "unexpected opsResubmitted"); } + it("should not assert when flushing while reentrant with empty batches", () => { + const outbox = getOutbox({ + context: getMockContext(), + opGroupingConfig: { + groupedBatchingEnabled: true, + }, + }); + + // Mark context as reentrant + state.isReentrant = true; + + // Flush with no messages - should not throw + assert.doesNotThrow(() => { + outbox.flush(); + }, "Should not assert when flushing empty batches while reentrant"); + + validateCounts(0, 0, 0); + }); + + it("should assert when flushing while reentrant with non-empty batches", () => { + const outbox = getOutbox({ + context: getMockContext(), + opGroupingConfig: { + groupedBatchingEnabled: true, + }, + }); + + const messages = [createMessage(ContainerMessageType.FluidDataStoreOp, "0")]; + + // Submit a message (not reentrant) + state.isReentrant = false; + outbox.submit(messages[0]); + + // Now mark context as reentrant and try to flush - should throw + state.isReentrant = true; + + assert.throws( + () => outbox.flush(), + (error: Error) => { + return ( + validateAssertionError(error, + "Flushing must not happen while incoming changes are being processed", + ) + ); + }, + "Should assert when flushing non-empty batches while reentrant", + ); + + // Verify nothing was submitted + validateCounts(0, 0, 0); + }); + it("batch has reentrant ops, but grouped batching is off", () => { const outbox = getOutbox({ context: getMockContext(), From ac49a7a451207940cb5377c9712a198939536452 Mon Sep 17 00:00:00 2001 From: Mark Fields Date: Wed, 22 Oct 2025 16:21:33 -0700 Subject: [PATCH 3/5] lint --- .../container-runtime/src/test/opLifecycle/outbox.spec.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/runtime/container-runtime/src/test/opLifecycle/outbox.spec.ts b/packages/runtime/container-runtime/src/test/opLifecycle/outbox.spec.ts index 1767df4de1e7..cb2128e2fd54 100644 --- a/packages/runtime/container-runtime/src/test/opLifecycle/outbox.spec.ts +++ b/packages/runtime/container-runtime/src/test/opLifecycle/outbox.spec.ts @@ -1132,10 +1132,9 @@ describe("Outbox", () => { assert.throws( () => outbox.flush(), (error: Error) => { - return ( - validateAssertionError(error, - "Flushing must not happen while incoming changes are being processed", - ) + return validateAssertionError( + error, + "Flushing must not happen while incoming changes are being processed", ); }, "Should assert when flushing non-empty batches while reentrant", From 775aa7c389210f7891baecd40245b5c84bd7b668 Mon Sep 17 00:00:00 2001 From: Mark Fields Date: Thu, 23 Oct 2025 10:25:42 -0700 Subject: [PATCH 4/5] More targeted fix. Previous fix opened up a possible regression under `maybeFlushPartialBatch` --- .../src/opLifecycle/outbox.ts | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/runtime/container-runtime/src/opLifecycle/outbox.ts b/packages/runtime/container-runtime/src/opLifecycle/outbox.ts index 31a9d0de2c8e..da15089093ce 100644 --- a/packages/runtime/container-runtime/src/opLifecycle/outbox.ts +++ b/packages/runtime/container-runtime/src/opLifecycle/outbox.ts @@ -316,7 +316,7 @@ export class Outbox { // If we're configured to flush partial batches, do that now and return (don't throw) if (this.params.config.flushPartialBatches) { - this.flush(); + this.flushAll(); return; } @@ -365,6 +365,25 @@ export class Outbox { * @param resubmitInfo - Key information when flushing a resubmitted batch. Undefined means this is not resubmit. */ public flush(resubmitInfo?: BatchResubmitInfo): void { + // We have nothing to flush if all batchManagers are empty, and we we're not needing to resubmit an empty batch placeholder + if ( + this.idAllocationBatch.empty && + this.blobAttachBatch.empty && + this.mainBatch.empty && + resubmitInfo?.batchId === undefined + ) { + return; + } + + assert( + !this.isContextReentrant(), + 0xb7b /* Flushing must not happen while incoming changes are being processed */, + ); + + this.flushAll(resubmitInfo); + } + + private flushAll(resubmitInfo?: BatchResubmitInfo): void { const allBatchesEmpty = this.idAllocationBatch.empty && this.blobAttachBatch.empty && this.mainBatch.empty; if (allBatchesEmpty) { @@ -379,11 +398,6 @@ export class Outbox { return; } - assert( - !this.isContextReentrant(), - 0xb7b /* Flushing must not happen while incoming changes are being processed */, - ); - // Don't use resubmittingBatchId for idAllocationBatch. // ID Allocation messages are not directly resubmitted so don't pass the resubmitInfo this.flushInternal({ @@ -406,11 +420,6 @@ export class Outbox { resubmittingBatchId: BatchId, resubmittingStagedBatch: boolean, ): void { - assert( - !this.isContextReentrant(), - "Flushing must not happen while incoming changes are being processed", - ); - const referenceSequenceNumber = this.params.getCurrentSequenceNumbers().referenceSequenceNumber; assert( From 38a6152f80eca58ca6b9cfaa3c8b2d0f76d841ec Mon Sep 17 00:00:00 2001 From: Mark Fields Date: Thu, 23 Oct 2025 10:29:31 -0700 Subject: [PATCH 5/5] Revert whitespace change --- packages/runtime/container-runtime/src/opLifecycle/outbox.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/runtime/container-runtime/src/opLifecycle/outbox.ts b/packages/runtime/container-runtime/src/opLifecycle/outbox.ts index da15089093ce..102580b2baa1 100644 --- a/packages/runtime/container-runtime/src/opLifecycle/outbox.ts +++ b/packages/runtime/container-runtime/src/opLifecycle/outbox.ts @@ -379,7 +379,6 @@ export class Outbox { !this.isContextReentrant(), 0xb7b /* Flushing must not happen while incoming changes are being processed */, ); - this.flushAll(resubmitInfo); }