Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an IdCompressor "Ranges finalized out of order" error that occurs when forked containers submit IdAllocation operations in parallel. The fix adds range-level duplicate detection to catch overlapping ID ranges from different clients sharing the same session ID, which the existing DuplicateBatchDetector cannot catch because IdAllocation ops use the ignoreBatchId flag.
Changes:
- Added duplicate range detection logic in
ContainerRuntimeto track and validate IdAllocation ranges per session - Added comprehensive unit tests for the duplicate detection logic covering overlapping, sequential, and cross-session scenarios
- Enabled and expanded e2e test coverage for parallel forked containers with ID allocation operations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/runtime/container-runtime/src/containerRuntime.ts | Added finalizedIdAllocationRanges Map to track expected genCount per session, and validation logic before finalizing ranges to detect and reject overlaps |
| packages/runtime/container-runtime/src/test/containerRuntime.spec.ts | Added unit test suite covering overlapping ranges (error), sequential ranges (allowed), different sessions (allowed), and partial overlap detection |
| packages/test/test-end-to-end-tests/src/test/offline/stashedOps.spec.ts | Implemented previously-skipped parallel fork test and added new variant that includes ID allocation ops to exercise the new duplicate detection path |
Comments suppressed due to low confidence (1)
packages/test/test-end-to-end-tests/src/test/offline/stashedOps.spec.ts:2366
- Typo in comment: "contianer1" should be "container1".
// All containers close: contianer1, container2, container3
| }, | ||
| ); | ||
| } | ||
| this.finalizedIdAllocationRanges.set( |
There was a problem hiding this comment.
What would it look like to move this duplicate tracking into ID Compressor itself? (Inside finalizeCreationRange)
There was a problem hiding this comment.
I've been playing with it and by doing so it breaks some other id compressor tests. I think this is a better place to target the forked scenario better, given that finalizeCreationRange is used in different places and id compressor already has other mechanisms to detect similar scenarios.
There was a problem hiding this comment.
Ok. Yeah I can see how trying to modify the core finalizeCreationRange fn could be troublesome.
I'm always leery of adding a too much niche logic to this file/class. Can we add a new fn to IIdCompressorCore like tryFinalizeCreationRange that returns status about whether it succeeded (rather than throwing), and includes this check?
That type is legacy-beta so you'd have to add it as optional but I know Taylor wants to move it to internal anyway so we can consider it pretty flexible.
There was a problem hiding this comment.
@taylorsw04 can you please review this new logic? Any thoughts on moving it into the ID Compressor impl rather than adding it here?
| }); | ||
|
|
||
| it("Allows IdAllocation ranges from different sessions", () => { | ||
| const otherSessionId = "aaaaaaaa-bbbb-4ccc-dddd-eeeeeeeeeeee"; |
There was a problem hiding this comment.
| const otherSessionId = "aaaaaaaa-bbbb-4ccc-dddd-eeeeeeeeeeee"; | |
| const otherSessionId = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"; |
Haha I'm kidding, it doesn't matter what it is but I thought it was funny the "4" in the middle
There was a problem hiding this comment.
I wonder what on earth Claude was thinking writing that 4 mmm
| false /* local */, | ||
| ); | ||
|
|
||
| // Overlapping range starting at genCount 3 (before nextExpected=6) |
There was a problem hiding this comment.
I wonder if we should make this case an assert - no known way to trigger this case in reality is there?
There was a problem hiding this comment.
I don't think so but given I'm no expert on id compressor I wouldn't assert that. Seems only relevant to internal id compressor logic so I wouldn't add that assert in this PR
| ] as const; | ||
|
|
||
| itExpects( | ||
| `Parallel Forks: Closes (ForkedContainerError and DuplicateBatchError) when hydrating twice and submitting in parallel (via Counter DDS)`, |
There was a problem hiding this comment.
How do you know this won't fail for t9s? Isn't it equivalent to the former single test?
There was a problem hiding this comment.
Wait, previously it was only run for local. Shouldn't you keep it that way for both new tests?
Bug scenario:
When a container forks (two instances load from the same stashed state), the ID Compressor in each fork has the same session ID. Both independently generate IDs and submit IdAllocation ops:
Fork 1 sends: "Session ABC claims IDs 1-10"
Fork 2 sends: "Session ABC claims IDs 1-10" (same range — they started from the same state)
When finalizeCreationRange processes both, the compressor sees overlapping ranges for the same session, hitting "Ranges finalized out of order" inside the compressor itself, which is a confusing error with no indication it's a fork.
Fix
Detect duplicate (overlapping) IdAllocation ranges from forked containers that share the same session ID but submit from different clients. IdAllocation batches use ignoreBatchId, so the DuplicateBatchDetector cannot catch these — this adds range-level overlap detection before finalizeCreationRange.