improvement(id-compressor): Add resetUnfinalizedCreationRange API and use in CR.replayPendingStates#26784
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the ID allocation replay/resubmit flow to avoid submitting an extra IdAllocation op up-front during ContainerRuntime.replayPendingStates, by introducing a new IdCompressor.releaseUnfinalizedCreationRange() API that rewinds the next-take cursor to include all unfinalized IDs in the next naturally-submitted allocation range.
Changes:
- Add
IIdCompressorCore.releaseUnfinalizedCreationRange(): voidand implement it inIdCompressor. - Refactor
takeUnfinalizedCreationRange()to delegate toreleaseUnfinalizedCreationRange()+takeNextCreationRange(). - Update runtime/test logic to rely on the released cursor and validate ranges/finalization across reconnect.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/id-compressor/src/types/idCompressor.ts | Adds the new core API contract and docs for releasing unfinalized ranges. |
| packages/runtime/id-compressor/src/idCompressor.ts | Implements cursor rewind via releaseUnfinalizedCreationRange and simplifies takeUnfinalizedCreationRange. |
| packages/runtime/id-compressor/src/test/idCompressor.spec.ts | Adds coverage ensuring released ranges are included on the next takeNextCreationRange. |
| packages/runtime/id-compressor/replay-id-allocation-approaches.md | Adds design notes comparing alternative replay approaches. |
| packages/runtime/id-compressor/api-report/id-compressor.legacy.beta.api.md | Updates API report to include the new method on IIdCompressorCore. |
| packages/runtime/container-runtime/src/containerRuntime.ts | Switches replay behavior to release ranges instead of submitting a dedicated resubmit allocation op. |
| packages/runtime/container-runtime/src/test/containerRuntime.spec.ts | Updates reconnect test to validate IDs from a “lost take” are finalized after reconnect with a single allocation op. |
packages/runtime/container-runtime/src/test/containerRuntime.spec.ts
Outdated
Show resolved
Hide resolved
anthony-murphy
left a comment
There was a problem hiding this comment.
this change seems like a good direction to me, but i don't have deep id-compressor knowledge
packages/runtime/id-compressor/api-report/id-compressor.legacy.beta.api.md
Outdated
Show resolved
Hide resolved
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
1 similar comment
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
|
@markfields I thought you were going to deprecate (or, honestly, just remove--I've audited all external users repos) takeUnfinalizedCreationRange? I'd like to not let the surface grow. |
@taylorsw04 - I'm hoping to deprecate this whole IIdCompressorCore interface for moving to I'll either do that, or have a 1-liner follow-up PR that deprecates the new thing, and then remove it in 2.100. |
Description
Background
Managing ID Allocation ops/batches in the ContainerRuntime has been a perennial source of difficulty, including some current efforts around Batch tracking, and potentially Staging Mode as well.
One special case that we can simplify is what happens for "resubmit". The current flow submits an ID Allocation op before replaying pending ops. This is divergent from the typical semantics of ID Allocation ops which is that they're only submitted right before an op that uses a new compressed ID.
The new API
So we are introducing
IdCompressor.releaseUnfinalizedCreationRange. It's similar totakeUnfinalizedCreationRange, but instead of returning the range, it merely resets the internal statenextRangeBaseGenCountto be before any unfinalized ranges, since we know they won't be finalilzed (since the connection the ID Allocations were sent on closed without those acks).Now, the next call to
takeNextCreationRangewill include those unfinalized ranges, and we don't need to interject an extra op before replay.takeUnfinalizedCreationRangeitself becomes a two-line shortcut. Maybe we would want to deprecate it since additionally, it's unused internally?Reconnect Flow
By the time we get to
replayPendingStates, we know everything pending has not been sequenced, and never will be until we resubmit. I.e. we've finalized ALL successfully submitted ranges. So we can safely movenextRangeBaseGenCountback according to the local clusters - those IDs will not be finalized by any in-flight ops, and should be included in the next creation range, triggered whenever the resubmitted main content (or subsequent changes) demands.Testing
Updated a test that was added for that
replayPendingStatescodepath to demonstrate that this works (if you comment out the new call, the test fails withRanges Finalized Out Of Order).Here's the summary of the design discussion Claude and I had, comparing this ("Approach B") to 2 other alternatives (Approach A was very bad so omitted here):
Approaches for Deferring ID Allocation Op During Replay
Problem
During
replayPendingStates, the container runtime callstakeUnfinalizedCreationRangeand 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
takeNextCreationRangereturns IDs generated since the last range was taken, starting from an internal cursor (nextRangeBaseGenCount), and advances the cursor forward.takeUnfinalizedCreationRangereturns ALL unfinalized IDs (going back to the last finalized cluster), and also advances the cursor forward.generateCompressedIddoes not interact with the range-taking cursor at all -- it only toucheslocalGenCount, cluster state, and the normalizer.Approach B:
releaseUnfinalizedCreationRange(IdCompressor change)Add a new void method to
IIdCompressorCore/IdCompressor:The container runtime calls this during replay instead of submitting an op. The next
takeNextCreationRangecall naturally produces a range covering both the old unfinalized IDs and any new IDs generated in the interim.Pros
takeNextCreationRangehandles everythingnormalizer.getRangesBetweenworks correctly for the expanded rangeCons
IIdCompressorCore(a@legacy @betapublic interface)nextRangeBaseGenCountgoing backward is a new pattern (currently it only advances)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. InsubmitIdAllocationOpIfNeeded, check the flag:Pros
takeUnfinalizedCreationRange)nextRangeBaseGenCountcontinues to only advanceCons
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).