fix(odsp-driver): preserve epoch in pending container state - #28156
fix(odsp-driver): preserve epoch in pending container state#28156Alex Villarreal (alexvy86) wants to merge 8 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
area: tools
area: loader
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (382 lines, 17 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
🟡 Changes recommended
The frozen-service path can lose the persisted epoch, and the moderate API-contract and telemetry issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Preserves ODSP epochs in pending container state so stale operations are rejected after server-side restoration.
Changes:
- Adds optional driver-state serialization and restoration hooks.
- Restores ODSP epochs before storage or delta-stream connections.
- Adds tests, API updates, and release metadata.
File summaries
| File | Review |
|---|---|
packages/test/test-service-load/src/faultInjectionDriver.ts |
Forwards the new hooks in test services. |
packages/loader/driver-utils/src/documentServiceProxy.ts |
Proxies the driver-state hooks. |
packages/loader/container-loader/src/test/serializedStateManager.spec.ts |
Tests driver-state serialization. |
packages/loader/container-loader/src/serializedStateManager.ts |
Serializes opaque driver state. |
packages/loader/container-loader/src/frozenServices.ts |
Critical: Discards restored state without a wrapped service, allowing offline re-serialization to remove the epoch. |
packages/loader/container-loader/src/createAndLoadContainerUtils.ts |
Captures driver state during full-state serialization. |
packages/loader/container-loader/src/container.ts |
Restores state before connecting. |
packages/drivers/odsp-driver/src/test/joinSessionCacheTests.spec.ts |
Nit: Lacks integration coverage for reloading stale state against a mismatched ODSP epoch. |
packages/drivers/odsp-driver/src/odspDocumentService.ts |
Moderate: Restored epochs are incorrectly reported as not originating from cache. |
packages/common/driver-definitions/src/storage.ts |
Moderate: The public contract does not document JSON-serializability, undefined, or restoration timing requirements. |
packages/common/driver-definitions/api-report/driver-definitions.legacy.beta.api.md |
Updates the legacy-beta API surface; API Council review is required. |
.changeset/every-feet-post.md |
Nit: Omits consumer-meaningful container-loader and odsp-driver package entries. |
Review details
Suppressed comments (2)
.changeset/every-feet-post.md:2
- The changeset only assigns the changelog entry to
driver-definitions, although the consumer-visible fix is implemented incontainer-loaderandodsp-driver. Repository guidance says to include every package whose change is meaningful to consumers (.changeset/README.md:107-111); otherwise those packages ship this behavior without a changelog entry.
"@fluidframework/driver-definitions": minor
packages/drivers/odsp-driver/src/test/joinSessionCacheTests.spec.ts:66
- This test only invokes the two methods on one service instance; it never serializes pending state, reloads it through
Container, or verifies that a different server epoch rejects the stale state before storage/delta connection. The critical cross-package restoration path can regress while this test still passes. Add an integration test that reloads serialized pending state against a mismatched ODSP epoch and asserts the non-retryable epoch failure.
const service = await odspDocumentServiceFactory.createDocumentService(odspResolvedUrl);
service.setDriverState?.("epoch1");
assert.strictEqual(service.getDriverState?.(), "epoch1");
- Files reviewed: 12/12 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check |
|
/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - common-utils,Build - eslint-config-fluid |
|
Azure Pipelines: 6 pipeline(s) were filtered out due to trigger conditions. |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 6 pipeline(s) were filtered out due to trigger conditions. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2fe1f5e-932c-4ea8-9681-14d8fd2d0709
|
/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check |
|
/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - common-utils,Build - eslint-config-fluid |
|
Azure Pipelines: 6 pipeline(s) were filtered out due to trigger conditions. |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 6 pipeline(s) were filtered out due to trigger conditions. |
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate issues could allow incomplete restoration contracts or stale driver state to be re-serialized.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The persistence API, frozen-state fallback, and timing-dependent rejection test need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
packages/common/driver-definitions/src/storage.ts:381
- The two independently optional methods allow a service to expose state that the loader serializes but can never restore (or to expose only a setter), and wrappers that always define both methods hide that mismatch further. Model this as one optional persistence capability whose getter and setter are both required so a partial implementation is not representable.
getDriverState?(): unknown;
packages/loader/container-loader/src/frozenServices.ts:124
- Using
??treats a valid JSON-serializablenullreturned by the inner getter as absent, and it also replaces an inner getter's deliberateundefined(“no state”) with the wrapper's cached value. This can omit valid state or reserialize stale state; fall back only when the inner getter itself is not implemented.
return this.documentService?.getDriverState?.() ?? this.driverState;
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2fe1f5e-932c-4ea8-9681-14d8fd2d0709
|
Regarding the suggested legacy-pending-state telemetry: I am not adding it in this PR. The ODSP service cannot distinguish an absent legacy value from a load that did not originate from pending state without adding another loader-to-driver signal, and the legacy pending-state population is transient. The default remains backward-compatible and permissive. |
|
/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check |
|
/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - common-utils,Build - eslint-config-fluid |
|
Azure Pipelines: 6 pipeline(s) were filtered out due to trigger conditions. |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 6 pipeline(s) were filtered out due to trigger conditions. |
|
Deep Review: Reasonable call — the legacy-absent case can't be distinguished from a non-pending-state load without adding another loader-to-driver signal, and that population is transient, so declining the telemetry here is fine and the permissive default preserves back-compat. Two narrower observability points are still open and captured inline rather than here: the restored epoch is logged as a cache source in |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2fe1f5e-932c-4ea8-9681-14d8fd2d0709
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2fe1f5e-932c-4ea8-9681-14d8fd2d0709
|
/azp run Build - protocol-definitions,Build - test-tools,server-gitrest,server-gitssh,server-historian,server-routerlicious,Build - client packages,repo-policy-check |
|
/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - common-utils,Build - eslint-config-fluid |
|
Azure Pipelines: 6 pipeline(s) were filtered out due to trigger conditions. |
|
Azure Pipelines: Successfully started running 2 pipeline(s). 6 pipeline(s) were filtered out due to trigger conditions. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2fe1f5e-932c-4ea8-9681-14d8fd2d0709
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2fe1f5e-932c-4ea8-9681-14d8fd2d0709
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2fe1f5e-932c-4ea8-9681-14d8fd2d0709
|
🔗 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: 9/10 — ALMOST READY Close to sign-off; no blocking defects against the changed files. Since the last review the identity-ownership request landed — pending state carrying Path to Ready
Context for Reviewers
For human reviewer
Review history (5 prior reviews)
|
Description
AB#82272
ODSP uses an epoch to detect when a file has been restored server-side. Pending container state did not preserve that epoch, so loading stale pending state could learn the restored file's current epoch and replay stale operations instead of rejecting them.
This change lets document services provide opaque state for pending-state serialization and restores it before connecting to storage or the delta stream. ODSP uses the hook to preserve its epoch. Existing pending state and drivers remain compatible because the hooks and serialized field are optional.
Tests cover pending-state serialization and ODSP epoch restoration.
Reviewer Guidance
The review process is outlined in the pull request guidelines.
The
IDocumentServiceadditions affect the@legacy @betaAPI surface and require API Council review.