Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions packages/runtime/container-runtime/src/containerRuntime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2699,12 +2699,10 @@ export class ContainerRuntime
this.emitDirtyDocumentEvent = false;

try {
// Any ID Allocation ops that failed to submit after the pending state was queued need to have
// the corresponding ranges resubmitted (note this call replaces the typical resubmit flow).
// Since we don't submit ID Allocation ops when staged, any outstanding ranges would be from
// before staging mode so we can simply say staged: false.
this.submitIdAllocationOpIfNeeded({ resubmitOutstandingRanges: true, staged: false });
this.scheduleFlush();
// Any ID Allocation ops that failed to submit need to have their ranges included
// in the next allocation op. Release the unfinalized ranges back so that the next
// call to takeNextCreationRange (during replay) will include them.
this._idCompressor?.releaseUnfinalizedCreationRange();

// replay the ops
this.pendingStateManager.replayPendingStates();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -618,8 +618,8 @@ describe("Runtime", () => {
);
});

it("IdAllocation op from replayPendingStates is flushed, preventing outboxSequenceNumberCoherencyCheck error", async () => {
// Start out disconnected since step 1 is to trigger ID Allocation op on reconnect
it("IDs generated before a lost range are finalized after reconnect", async () => {
// Start out disconnected since step 1 is to generate IDs before reconnect
const connected = false;
const mockContext = getMockContext({ connected }) as IContainerContext;
const mockDeltaManager = mockContext.deltaManager as MockDeltaManager;
Expand All @@ -632,33 +632,45 @@ describe("Runtime", () => {
provideEntryPoint: mockProvideEntryPoint,
});

// 1st compressed id – queued while disconnected (goes to idAllocationBatch).
containerRuntime.idCompressor?.generateCompressedId();
const compressor = containerRuntime.idCompressor;
assert(compressor !== undefined, "Expected idCompressor to be defined");

// Generate an ID while disconnected.
const id1 = compressor.generateCompressedId();
// Simulate the range being taken but lost (e.g. nack'd / disconnect before submit).
compressor.takeNextCreationRange();

// Re-connect – replayPendingStates will submit only an idAllocation op.
// It's now in the Outbox and a flush is scheduled (including this flush was a bug fix)
// Re-connect – replayPendingStates releases unfinalized ranges
// (no IdAllocation op is submitted at this point).
changeConnectionState(containerRuntime, true, mockClientId);

// Simulate a remote op arriving before we submit anything else.
// Bump refSeq and continue execution at the end of the microtask queue.
// This is how Inbound Queue works, and this is necessary to simulate here to allow scheduled flush to happen
// This is how Inbound Queue works, and this is makes sure we get coverage of ref seq coherency in this test.
++mockDeltaManager.lastSequenceNumber;
await Promise.resolve();

// 2nd compressed id – its idAllocation op will enter Outbox *after* the ref seq# bumped.
const id2 = containerRuntime.idCompressor?.generateCompressedId();

// This would throw a DataProcessingError from codepath "outboxSequenceNumberCoherencyCheck"
// if we didn't schedule a flush after the idAllocation op submitted during the reconnect.
// (On account of the two ID Allocation ops having different refSeqs but being in the same batch)
// Generate another ID and submit a data store op referencing it.
// This triggers submitIdAllocationOpIfNeeded → takeNextCreationRange.
// Because the unfinalized range was released during replay, both IDs
// are included in the resulting allocation range.
const id2 = compressor.generateCompressedId();
submitDataStoreOp(containerRuntime, "someDS", genTestDataStoreMessage({ id: id2 }));

// Let the Outbox flush so we can check submittedOps length
await Promise.resolve();
assert(
submittedOps.length === 3,
"Expected 3 ops to be submitted (2 ID Allocation, 1 data)",
);
assert.strictEqual(submittedOps.length, 2, "Expected 1 ID Allocation + 1 data op");

// Simulate processing the ID Allocation ack — finalize the range.
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-unsafe-member-access
compressor.finalizeCreationRange(submittedOps[0].contents);

// Both IDs should now be finalized (positive final IDs in op space).
// Without releaseUnfinalizedCreationRange, id1 would remain a local-only
// ID (negative) that could never be shared with other clients.
const id1OpSpace = compressor.normalizeToOpSpace(id1);
const id2OpSpace = compressor.normalizeToOpSpace(id2);
assert(id1OpSpace >= 0, "id1 should be finalized after allocation range is sequenced");
assert(id2OpSpace >= 0, "id2 should be finalized after allocation range is sequenced");
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export interface IIdCompressor {
export interface IIdCompressorCore {
beginGhostSession(ghostSessionId: SessionId, ghostSessionCallback: () => void): void;
finalizeCreationRange(range: IdCreationRange): void;
releaseUnfinalizedCreationRange(): void;
serialize(withSession: true): SerializedIdCompressorWithOngoingSession;
serialize(withSession: false): SerializedIdCompressorWithNoSession;
takeNextCreationRange(): IdCreationRange;
Expand Down
63 changes: 63 additions & 0 deletions packages/runtime/id-compressor/replay-id-allocation-approaches.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# Approaches for Deferring ID Allocation Op During Replay

## Problem

During `replayPendingStates`, the container runtime calls `takeUnfinalizedCreationRange` and immediately submits an IdAllocation op. Submitting this op during replay is problematic. We want to defer the allocation so it is included in the next naturally-submitted IdAllocation op instead.

## Background

- `takeNextCreationRange` returns IDs generated since the last range was taken, starting from an internal cursor (`nextRangeBaseGenCount`), and advances the cursor forward.
- `takeUnfinalizedCreationRange` returns ALL unfinalized IDs (going back to the last finalized cluster), and also advances the cursor forward.
- `generateCompressedId` does not interact with the range-taking cursor at all -- it only touches `localGenCount`, cluster state, and the normalizer.

## Approach B: `releaseUnfinalizedCreationRange` (IdCompressor change)

Add a new void method to `IIdCompressorCore` / `IdCompressor`:

```ts
public releaseUnfinalizedCreationRange(): void {
// Reset nextRangeBaseGenCount back to the start of the unfinalized region
}
```

The container runtime calls this during replay instead of submitting an op. The next `takeNextCreationRange` call naturally produces a range covering both the old unfinalized IDs and any new IDs generated in the interim.

### Pros

- State tracking is consolidated inside the IdCompressor
- No merge logic needed -- `takeNextCreationRange` handles everything
- Only a single range is ever produced, so no ordering/overlap issues
- `normalizer.getRangesBetween` works correctly for the expanded range

### Cons

- New method on `IIdCompressorCore` (a `@legacy @beta` public interface)
- `nextRangeBaseGenCount` going backward is a new pattern (currently it only advances)
- The reserved range is opaque -- caller cannot inspect it for logging/debugging

## Approach C: Boolean flag in container runtime (no IdCompressor change)

Add a boolean flag (`needsUnfinalizedResubmit`) to the container runtime. Set it during replay instead of submitting an op. In `submitIdAllocationOpIfNeeded`, check the flag:

```ts
const idRange = this.needsUnfinalizedResubmit
? this._idCompressor.takeUnfinalizedCreationRange()
: this._idCompressor.takeNextCreationRange();
this.needsUnfinalizedResubmit = false;
```

### Pros

- Zero changes to IdCompressor or its public API
- Uses existing, well-tested methods (`takeUnfinalizedCreationRange`)
- No new invariants -- `nextRangeBaseGenCount` continues to only advance
- Simpler to review and reason about

### Cons

- State is split across two components (boolean in container runtime, range state in compressor)
- Container runtime must know about the distinction between the two take methods (though it already does today)

## Recommendation

Both approaches are functionally equivalent and correct. Approach C is the lower-risk path (no API change, uses existing methods). Approach B is the more principled one (compressor owns its own state). The right choice depends on how much the team values keeping the IdCompressor API minimal vs. consolidating state management.
39 changes: 11 additions & 28 deletions packages/runtime/id-compressor/src/idCompressor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,35 +244,18 @@ export class IdCompressor implements IIdCompressor, IIdCompressorCore {
}

public takeUnfinalizedCreationRange(): IdCreationRange {
const lastLocalCluster = this.localSession.getLastCluster();
let count: number;
let firstGenCount: number;
if (lastLocalCluster === undefined) {
firstGenCount = 1;
count = this.localGenCount;
} else {
firstGenCount = genCountFromLocalId(
(lastLocalCluster.baseLocalId - lastLocalCluster.count) as LocalCompressedId,
);
count = this.localGenCount - firstGenCount + 1;
}

if (count === 0) {
return {
sessionId: this.localSessionId,
};
}
this.releaseUnfinalizedCreationRange();
return this.takeNextCreationRange();
}

const range: IdCreationRange = {
ids: {
count,
firstGenCount,
localIdRanges: this.normalizer.getRangesBetween(firstGenCount, this.localGenCount),
requestedClusterSize: this.nextRequestedClusterSize,
},
sessionId: this.localSessionId,
};
return this.updateToRange(range);
public releaseUnfinalizedCreationRange(): void {
const lastLocalCluster = this.localSession.getLastCluster();
this.nextRangeBaseGenCount =
lastLocalCluster === undefined
? 1
: genCountFromLocalId(
(lastLocalCluster.baseLocalId - lastLocalCluster.count) as LocalCompressedId,
);
}

private updateToRange(range: IdCreationRange): IdCreationRange {
Expand Down
89 changes: 89 additions & 0 deletions packages/runtime/id-compressor/src/test/idCompressor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,95 @@ describe("IdCompressor", () => {
);
});
});

describe("by reserving unfinalized ranges for the next take", () => {
it("produces equivalent range to takeUnfinalizedCreationRange on next takeNextCreationRange", () => {
const compressor = CompressorFactory.createCompressor(Client.Client1, 2);
generateCompressedIds(compressor, 1);
compressor.takeNextCreationRange();

// Reserve instead of take
compressor.releaseUnfinalizedCreationRange();

// Next takeNextCreationRange should cover the unfinalized IDs
const range = compressor.takeNextCreationRange();
assert.deepEqual(range.ids, {
firstGenCount: 1,
count: 1,
localIdRanges: [[1, 1]],
requestedClusterSize: 2,
});
});

it("includes new IDs generated after reserving", () => {
const compressor = CompressorFactory.createCompressor(Client.Client1, 2);
generateCompressedIds(compressor, 1);
compressor.takeNextCreationRange();

compressor.releaseUnfinalizedCreationRange();
generateCompressedIds(compressor, 1);

const range = compressor.takeNextCreationRange();
assert.deepEqual(range.ids, {
firstGenCount: 1,
count: 2,
localIdRanges: [[1, 2]],
requestedClusterSize: 2,
});
});

it("is a no-op when there are no unfinalized IDs", () => {
const compressor = CompressorFactory.createCompressor(Client.Client1, 2);
generateCompressedIds(compressor, 1);
compressor.finalizeCreationRange(compressor.takeNextCreationRange());

compressor.releaseUnfinalizedCreationRange();
const range = compressor.takeNextCreationRange();
assert.equal(range.ids, undefined);
});

it("is idempotent", () => {
const compressor = CompressorFactory.createCompressor(Client.Client1, 2);
generateCompressedIds(compressor, 2);
compressor.takeNextCreationRange();

compressor.releaseUnfinalizedCreationRange();
compressor.releaseUnfinalizedCreationRange();
compressor.releaseUnfinalizedCreationRange();

const range = compressor.takeNextCreationRange();
assert.deepEqual(range.ids, {
firstGenCount: 1,
count: 2,
localIdRanges: [[1, 2]],
requestedClusterSize: 2,
});
});

it("works with multiple outstanding ranges", () => {
const compressor = CompressorFactory.createCompressor(Client.Client1, 2);
generateCompressedIds(compressor, 1);
const range1 = compressor.takeNextCreationRange();
generateCompressedIds(compressor, 1); // one local
compressor.finalizeCreationRange(range1);
compressor.takeNextCreationRange();
generateCompressedIds(compressor, 1); // one eager final
compressor.takeNextCreationRange();
generateCompressedIds(compressor, 1); // one local
compressor.takeNextCreationRange();

compressor.releaseUnfinalizedCreationRange();
const range = compressor.takeNextCreationRange();
assert.deepEqual(range.ids?.firstGenCount, 2);
assert.deepEqual(range.ids?.count, 3);
assert.deepEqual(range.ids?.localIdRanges, [
[2, 1],
[4, 1],
]);

compressor.finalizeCreationRange(range);
});
});
});

describe("Finalizing", () => {
Expand Down
12 changes: 12 additions & 0 deletions packages/runtime/id-compressor/src/types/idCompressor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,18 @@ export interface IIdCompressorCore {
*/
takeUnfinalizedCreationRange(): IdCreationRange;

/**
* Resets the next creation range to include all unfinalized IDs.
* After calling this, the next call to `takeNextCreationRange` will produce a range
* covering all unfinalized IDs (equivalent to what `takeUnfinalizedCreationRange` would
* have returned) plus any IDs generated after this call.
*
* Unlike `takeUnfinalizedCreationRange`, this method does not produce or return a range,
* and does not advance the internal range counter. It is useful when the caller wants to
* defer the actual range submission to the next natural `takeNextCreationRange` call.
*/
releaseUnfinalizedCreationRange(): void;

/**
* Finalizes the supplied range of IDs (which may be from either a remote or local session).
* @param range - the range of session-local IDs to finalize.
Expand Down
Loading