Deep Review: UTF-8/base64 encoding split — process sign-off ask, no code change.

captureReferencedAttachmentBlobs writes base64 (bufferToString(buffer, "base64")) into a new IBase64BlobContents; readReferencedSnapshotBlobs writes UTF-8 into ISerializableBlobContents (serializedStateManager.ts:284-299, decode in containerStorageAdapter.ts:515-527). The split is technically correct and aligns with the runtime's existing pending-blob serializer (blobManager.ts:340-343, :951-960); the non-UTF-8 round-trip e2e test at captureFullContainerState.spec.ts:351-427 covers the binary case.

Process concern: vladsud's "we should stick to one format across layers for blobs" position from PR #20507 (with anthony-murphy explicitly looped in) hasn't been revisited on the record. vladsud is already requested; please also tag anthony-murphy so the area's "one format" stance is consciously superseded rather than silently bypassed. The binary-safety argument for the split is sound — no code change expected.

Deep Review: Add abortSignal?: AbortSignal to ICaptureFullContainerStateProps before @alpha graduation.

ICaptureFullContainerStateProps exposes only urlResolver, documentServiceFactory, request, logger. The capture body concurrently reads snapshot/attachment blobs (bounded concurrency 32) and drains the op stream without a cancellation check. IDocumentDeltaStorageService.fetchMessages already accepts abortSignal?: AbortSignal (packages/common/driver-definitions/src/storage.ts:103-109), so the underlying primitives support it.

Suggested fix: add abortSignal?: AbortSignal to ICaptureFullContainerStateProps, propagate it into the mapWithConcurrency blob/group loops and the fetchMessages op-drain loop, and call signal.throwIfAborted() between bounded batches. Additive and cheaper to land before the API graduates from @alpha.

This is a good idea, adding it to follow-ups mentioned in PR description

Deep Review: Thanks — accepting as a follow-up is reasonable given the API is still @alpha @legacy. One ask: gate the follow-up to land before graduation. The prop is additive, so it's cheaper now than once the surface is frozen; please link the ADO item here when filed and resolve the thread.

Deep Review: Op-stream drain trusts the driver silently — assert contiguity to fail closed.

captureFullContainerState drains connectToDeltaStorage().fetchMessages(attributes.sequenceNumber + 1, …) into savedOps and emits whatever the driver yields. Other parts of the same function fail-closed on suspicious states (UsageError on loading groups; GenericError on empty snapshot fetch); ops are the one place that trusts the driver silently. A buggy driver / network truncation would produce an artifact whose ops cannot be replayed, surfacing as a confusing rehydration-time failure pointing at the loader rather than at the capture.

Suggested fix: track a previous-sequence-number across the drain loop and assert each op's sequenceNumber === prev + 1; throw on a gap with the offending pair in the message. Optional: also assert the snapshot's latestSequenceNumber >= attributes.sequenceNumber at fetch time.

Another good point. A downside to not using the DeltaManager code in the container loader, I guess. Tracking as a follow-up.

Deep Review: Thanks for tracking this. The contiguity assert keeps the artifact's replay invariants honest at capture time and matches the fail-closed posture used elsewhere in captureFullContainerState (UsageError on loading groups, GenericError on empty snapshot fetch). Please link the follow-up ADO item here when filed and resolve the thread; ideally land before @alpha graduation alongside #3204969959.

Deep Review: Loader↔runtime constant duplication — gatekeeper nod ask, no code change.

This PR introduces a wireFormatConstants block in the loader mirroring seven runtime-side constants (.blobs, .redirectTable, _blobs, gc, __gc, __tombstones, __deletedNodes) and a cross-package deepStrictEqual contract test (wireFormatConstants.spec.ts). PR #21729 (anthony-murphy, 2024-07-02) was the deliberate refactor that produced these constants in their current form, with markfields reviewing for path-format discipline. This PR is the first cross-layer consumer; the chosen pattern (duplicate + contract test) sets precedent for any future shared wire-format constant.

Tag anthony-murphy for an explicit nod on the duplicate+contract-test layering choice, or merge with a clear note that AB#72934 will revisit (per thread #3192306888). No code change expected.

@anthony-murphy thinking about replacing these 3 params with just documentService and having caller piece that together (maybe in follow-up PR). Does that work? I don't totally understand the environment in which this will be called and where these params will come from

its tricky. overall i would love to move in a direction where fluid takes a document service, but it could be hard right now due lack of usage that way. there won't be any app examples, and i'm not sure the odsp driver supports it. i would be 100% down to support both maybe via overload or union to start moving in that direction, as we need to start somewhere

I'd vote for this direction. I think we can expose both - the core function that takes instance, and wrapper that takes factory. Might need to make some changes to the service interface. For examlpe, it would be nice to get resolved URI from service (assuming it's not exposed today) and thus get rid of URI resolver dependency as well

It's probably much larger change (and should be considered on its own), but I wonder if something like directory / map should support arrays as payloads? In addition to strings. But that would likely require massive changes across whole stack.

any opportunities to dedup some code with Container.load()?

If I got it right, this flow does not have websocket connection and fetching ops from websocket.
It might be enough for prototype, but we would need ops from socket (replace with REST call once/if we have such API from SPO in the future).

L390 we connect to delta storage and fetch messages. I'll have to look at what that's doing... is it possible it's already that REST API we're talking about? I forget and on mobile at the moment.

Ohhh that's Storage's deltas, not PUSH, right? So would be missing the very latest messages?

Do we need some kind of stress test system here? I.e. a collection of random snapshots that go through new code path vs. full container load (with container runtime instantiation) and check that they produce same result in terms of ops fetched, blobs captured?
That assumes that we have equvalent flow for full container load (which we do not), so maybe we can't imlement it directly. But maybe some parts of it can be implemented, like asking GC subsystem for referenced blobs and compare that with what container-loader layer computed.

I just worry that as we make changes in runtime, they may not reflect in this layer.

this could be integrated into local-server-stress, which already does pending state capture from live containers

I would mention that it's a free function somewhere in the longer description as opposed to the title.

I'm going to drop the changeset, we don't need to advertise this at the moment. We can do so when promoting to beta.

Deep Review: Loading-group fail-fast — confirm follow-up tracking against a target consumer.

snapshotHasLoadingGroups(baseSnapshot) throws UsageError whenever any referenced subtree carries a groupId, and captureFullContainerState leaves loadedGroupIdSnapshots: undefined. Fail-fast is the right interim for an @alpha @legacy surface — preferable to emitting an incomplete artifact — and consistent with PR #20565's pending-state model for loading groups (tyler-cai-microsoft).

No code change requested. Two asks:

1. Confirm the deferral is tracked against an open ADO/work item with a target consumer. The PR risk-section text alone ("no known consumer + no e2e harness") does not auto-prioritize; once a consumer surfaces, the per-groupId storage.getSnapshot({ loadingGroupIds: [groupId], cacheSnapshot: false }) path needs to follow PR [Data Virtualization & Offline]: Reimplement groupId offline with the loader #20565's storage convention rather than inventing a new one.
2. Tag tyler-cai-microsoft to confirm the parity gap with PR [Data Virtualization & Offline]: Reimplement groupId offline with the loader #20565 is acceptable scope for v1.