Add point-in-time container loading by sequence number#27669
Add point-in-time container loading by sequence number#27669sonalideshpandemsft wants to merge 8 commits into
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (1785 lines, 21 files), I've queued these reviewers:
How this works
|
|
Deep Review: On the generic
PR #16152 formalized requiring Question for the author: is the returned container on the generic path intended to stop at the target, or is exact point-in-time materialization only supported via
Update the changeset headline example in |
| onClose = (error?: IErrorBase): void => reject(error); | ||
|
|
||
| // We need to setup a listener to stop op processing once we reach the desired sequence number (if specified). | ||
| opHandler = (): void => { |
There was a problem hiding this comment.
Deep Review: The central new behavior of this PR has no test coverage. The added tests exercise only ODSP base-snapshot selection (fetchSnapshot.spec.ts:345-468) and option plumbing (serializedStateManager.spec.ts:321-366).
There is no test for loadContainerPaused replaying from a historical base snapshot and pausing exactly at loadToSequenceNumber (loadPaused.ts:114-120), nor for Container.canMaterializePointInTime delegating through the storage-adapter chain (ContainerStorageAdapter → RetriableDocumentStorageService/ProtocolTreeStorageService → driver, container.ts:678-682). This is exactly the untested path where the generic-contract gap in the other thread lives — a target-assertion test would have caught it.
Please add:
- A test driving
loadContainerPausedwith a mocked delta stream asserting the container pauses at the target, including the "already at target" and "snapshot newer than target" branches. - A test asserting container-level
canMaterializePointInTimereturnsunknownUnavailablewhen the underlying driver lacks the method.
(The opHandler dropped the loadToSequenceNumber !== undefined guard at loadPaused.ts:114-119, but the early return at loadPaused.ts:86-93 keeps that safe — the removed guard is not itself a live bug; the untested-path concern stands independently.)
This change is for "generic path intended to stop at the target". By generic path, I mean the generic alpha historical API path:
|
| * If the underlying driver cannot answer, implementations should return `notAvailable` rather than claiming | ||
| * the point is unavailable for a specific reason. | ||
| */ | ||
| canMaterializePointInTime( |
There was a problem hiding this comment.
i think this would be better as a free function, rather than being built into the IContainer interface if possible
| type IContainerLoadDriverProps, | ||
| type ICreateAndLoadContainerProps, | ||
| type ICreateDetachedContainerProps, | ||
| type ILoadExistingContainerPropsAlpha, |
There was a problem hiding this comment.
i do not feel like this should just be added to the existing load flow. it is fundamentally different from normal loading, and i think should have its own independent path with its own types. pieces can be reused where it makes sense.
also is the plan for these version to always be readonly? like do with local versions?
There was a problem hiding this comment.
I figured it's best to keep it readonly (similar to local versions) rather than open this old point and keep editing from there. My thinking was, for NITL, copilot changes are auto-applied, and this change lets us inspect the document exactly at the sequence number produced by that copilot change, even if more edits happen afterward.
example:
- seq 10: copilot auto applies change A
- seq 11-20: other users make more edits
Historical load reconstructs state at seq 10.
If it writes, the new op is not inserted at seq 11.
It gets appended at seq 21, after edits 11-20.
There was a problem hiding this comment.
Also, edits from a old point would be a larger change? but I'm open to thoughts around this
| * Alpha storage service extension for point-in-time materialization probes. | ||
| * @legacy @alpha | ||
| */ | ||
| export interface IDocumentStorageServiceAlpha extends IDocumentStorageService { |
There was a problem hiding this comment.
i do no think this should be combined with IDocumentStorageService. it should be a separate interface, as the load path and usage is fundamentally different. it can still be alpha, it can still be in driver definitions, it should just be separate from the existing interfaces
There was a problem hiding this comment.
this is partially due to the fact that we do not expect other drivers to support this flow generally
| ); | ||
| const alphaProps = loadExistingContainerProps as ILoadExistingContainerPropsAlpha; | ||
| const headers = { ...loadExistingContainerProps.request.headers }; | ||
| if (alphaProps.loadToSequenceNumber !== undefined) { |
There was a problem hiding this comment.
this is probably the wrong mechanism to do this. i think your want to be more like frozen load
|
Deep Review: Thanks for confirming — |
| ); | ||
| break; | ||
| } | ||
| case "sequenceNumber": { |
There was a problem hiding this comment.
Deep Review: This targeted-load path can hang uncancellably when the requested loadToSequenceNumber is unreachable, and the connect() fallback in loadContainerPaused is now dead code.
The paused/targeted load forces opsBeforeReturn: "sequenceNumber" with deltaConnection: "none" (loadPaused.ts:63-66). The new case "sequenceNumber" builds opsBeforeReturnP = Promise.all([attachDeltaManagerOpHandler(..., "sequenceNumber", ...), targetReachedP]), and targetReachedP resolves only when an op with sequenceNumber >= loadToSequenceNumber is processed. Because deltaConnection is "none", DeltaManager drains delta storage bounded to loadToSequenceNumber + 1 and stops without opening the WebSocket. If storage cannot supply an op reaching the target — target in the future, ops not yet flushed, or a sustained 429 — targetReached() is never called and await opsBeforeReturnP blocks forever. No AbortSignal is threaded into Container.load; the signal param in loadContainerPaused (loadPaused.ts:126) is wired only into the opHandler/connect() block.
Separately, after loadExistingContainer(...) returns, Container.load has already paused at the target, so lastProcessedSequenceNumber === loadToSequenceNumber and the "already there" early return (loadPaused.ts:88-96) always fires — making the opHandler + container.connect() block (loadPaused.ts:107-158), including the comment explaining why connect() is needed to fetch ops, unreachable dead code.
Your clarification that loadExistingContainer({ loadToSequenceNumber }) is intended to stop at the target confirms this is a robustness gap, not an ambiguity: a path that must stop at the target hangs when the target is unreachable.
Recommendation: decide and implement the unreachable-target behavior — (a) detect delta-storage exhaustion (the targeted fetch drains without reaching loadToSequenceNumber) and reject with a clear "requested sequence number is not yet available" error; and/or (b) thread the AbortSignal/a timeout into the "sequenceNumber" wait; and/or (c) gate the load behind the new canMaterializePointInTime availability probe. Then remove the now-unreachable opHandler + connect() block (loadPaused.ts:107-158) and its stale comment, or restore a path that keeps it reachable. Add an end-to-end test asserting the targeted load fails clearly rather than hangs when the target exceeds available ops.
There was a problem hiding this comment.
Deep Review: Related to the hang above, the same "sequenceNumber" / deltaConnection: "none" path leaves the read-only historical contract enforced only by convention. pauseContainer() pauses the queues, but handleDeltaConnectionArg("none") is a bare break that never clears inboundQueuePausedFromInit — unlike the undefined/"delayed" cases, which set it false. A later Container.connect() → resumeInternal() sees inboundQueuePausedFromInit === true, resumes inbound/inboundSignal, and advances the container past loadToSequenceNumber — so a host can silently walk the "historical" container forward off the target. The predecessor loadContainerPaused deliberately calls container.forceReadonly?.(true); this path never does, and the new tests assert only paused queues, not readonly state.
You confirmed readonly is the intended contract (like local versions — a later write appends after latest rather than at the historical point). Convention-only pausing doesn't enforce that intent. Suggest failing closed on the historical path: call forceReadonly(true) on the returned container mirroring loadContainerPaused, and reject connect()/getPendingLocalState() on historical containers with a typed UsageError, while still allowing local read/UI interaction. Add a test asserting readonly state and that connect() does not advance the sequence number.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Bundle size comparisonBase commit: Notable changes
Per-bundle deltas
|
Deep ReviewReviewed commit Readiness: 5/10 — MAKING PROGRESS Not ready for sign-off, but the core architecture is validated — no rework required. Three contained correctness gaps remain, all flagged inline: the read-only historical contract is enforced only by convention (a host can Path to Ready
Context for Reviewers
For human reviewer
Review history (2 prior reviews)
|
2 participants
This PR threads a point-in-time load target through the loader stack using an alpha API:
LoaderHeader.sequenceNumbersupport for point-in-time load targets.ILoadContainerToSequenceNumberPropsandloadContainerToSequenceNumber.loadExistingContaineras the normal/latest-load helper and does not add historical-load props to its beta API.Loader.resolve,Container.load,SerializedStateManager.fetchSnapshot, andISnapshotFetchOptionsAlpha."sequenceNumber"load mode so the container can replay to the requested target, pause inbound/outbound queues, and return a read-only historical view.loadContainerPausedpath as a separate internal helper; it does not use the new sequence-number load mode.For ODSP, this adds point-in-time base snapshot selection. When
loadToSequenceNumberis supplied, ODSP skips the latest snapshot path, lists recent versions without cache, reads candidate snapshot attributes, and selects the closest usable base snapshot at or before the requested target. If no usable base snapshot exists, ODSP fails clearly instead of returning latest.The availability probe is exposed through the standalone alpha
IPointInTimeMaterializationStorageServicecapability. ODSP uses it to report whether a point appearsmaterializable, is missing a base version, is missing required replay ops, is inaccessible due to permissions, or cannot be determined.