Fix ODSP file-version base selection and harden the version manager

[lindsnguyen]

Description

Follow-up to #27689 addressing review feedback on the internal ODSP file-version manager in
@fluidframework/odsp-driver. As before, everything here is internal to the package (not exported from
index.ts, not wired into a call site), so there is no consumer-facing change.

Base selection — tip exclusion retained. An earlier iteration of this PR removed the tip-drop
(versions.slice(1)) and scanned all versions. Per review feedback (anthony-murphy), that was reverted:
the tip (newest version, index 0) is the one version whose sequence number is not yet static, so it cannot
serve as a stable base. findBaseForSeq scans the sealed versions (index 1+) newest-first and returns the
closest one with seq <= target; a file whose only version is the tip returns noBaseVersion, and a
consumer loads the live file instead (M-TIP-01/M-TIP-02).

Robustness — failed fetches. refresh() is removed. The version list is re-enumerated on every query
(it changes as new versions are cut), pruning cached sequence numbers for versions that have left the list
(lastKnownVersionIds). Resolved sequence numbers are cached via PromiseCache, which evicts rejected
entries by default, so a transient 5xx/429 no longer memoizes a permanent failure and a later call retries
(M-ERR-02, M-CACHE-04). This mirrors how Page History pulls the version list fresh per navigation.

Robustness — content-type. resolveSequenceNumber now handles application/json and
application/ms-fluid explicitly and throws a clear error on anything else (e.g. an HTML error page)
rather than mis-parsing (F-RESOLVE-04).

Test fidelity. Manager fixtures exercise the real /versions shape (a distinct tip plus sealed
versions). Added coverage for percent-encoding of version labels in the snapshot URL (F-RESOLVE-05).
DEV.md is updated to match.

Work item: https://dev.azure.com/fluidframework/internal/_workitems/edit/77292

[github-actions]

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (466 lines, 5 files), I've queued these reviewers:

• Correctness — logic errors, race conditions, lifecycle issues
• Security — vulnerabilities, secret exposure, injection
• API Compatibility — breaking changes, release tags, type design
• Performance — algorithmic regressions, memory leaks
• Testing — coverage gaps, hollow tests

How this works

• Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

• Tick Start review below to dispatch the review fleet.

• After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

• This comment updates as new commits land; your reviewer selections are preserved.

• Start review

[github-actions]

Fleet Review — Clean

No issues found across the reviewer fleet for this run.

View run

[github-actions]

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-website@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-website@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  298156 links
    1937 destination URLs
    2187 URLs ignored
       0 warnings
       0 errors

[anthony-murphy]

Deep Review

Reviewed commit 0eece2d on 2026-07-20.

Readiness: 8/10 — ALMOST READY

The load-bearing base-selection concern is fully settled: the tip-drop was reverted, M-TIP-01/M-TIP-02 restored, and ODSP owner anthony-murphy confirmed inline that the tip's sequence number isn't static so it can't be a stable base. The PR description now matches the shipped code, clearing the prior review's only action. What remains are four contained polish items in unwired, package-internal prototype code — flagged inline, none blocking.

Path to Ready

• Resolve inline threads

Context for Reviewers

• Builds on Add an internal ODSP file-version manager for point-in-time load #27689 (first component of point-in-time document load) — still an internal prototype, not exported from index.ts or wired to a call site, so no api-report/changeset.
• Tip-exclusion was reverted per review feedback and is owner-confirmed: the tip is the one version whose sequence number isn't yet static, so it can't be a stable base. A single-tip file returning noBaseVersion is deliberate; treat the index = 1 scan start as the invariant to protect.
• Rejected-resolution caching now uses PromiseCache (evicts rejected entries by default), closing anthony-murphy's Add an internal ODSP file-version manager for point-in-time load #27689 eviction thread and work item 77401 — pinned by M-ERR-02 / M-CACHE-04.
• Op-trimming is deliberately deferred to work item 77293; review this as base-selection/fetcher hardening, not trimmed-op materialization.
• Suggested reviewers: anthony-murphy (design authority on tip-exclusion, export, and PromiseCache), lindsnguyen (authored the original manager and the list-fetch coalescing this PR drops), sonalideshpandemsft (Add an internal ODSP file-version manager for point-in-time load #27689 co-reviewer on API tags/work items, raised the trimmed-ops question DEV.md now answers).

For human reviewer

• Needs human judgment — anthony-murphy confirmed inline that the tip should be excluded; a reviewer should confirm the reverted findBaseForSeq plus restored M-TIP-01/M-TIP-02 fully satisfy that contract before the component is wired into a load path.
• Cannot be assessed by the pipeline — whether the content-type allowlist (application/json, application/ms-fluid) is exhaustive for what ODSP returns in production, and whether per-query version-list re-enumeration (no coalescing) is acceptable network cost once the loader is wired up. Verify against the live service.

Review history (2 prior reviews)

• 4bd31d9 2026-07-17 · 6/10 — base-selection concern resolved; PR description still narrated the abandoned iteration
• 7003041 2026-07-15 · 5/10 — base-selection reversal contract unconfirmed; rejected-promise eviction and content-type hardening resolved

[anthony-murphy]

Deep Review: The rewrite drops the concurrent-fetch coalescing #27689 added. findBaseForSeq/listVersions now call this.fetcher.listFileVersions() unconditionally, so N concurrent callers issue N overlapping /versions enumerations and interleave mutations of the shared lastKnownVersionIds/seqCache. This is not a wrong-base bug — each caller acts on the concrete list it fetched, and failed seq resolutions auto-evict via PromiseCache — but it is extra round-trips plus an untested interleaving edge on the lastKnownVersionIds prune/repopulate path.

Related resolved thread: r3590146089 — anthony-murphy asked to "store the fetch promise, ensure only a single fetch promise is ever active"; the seq cache moved to PromiseCache, but the list fetch itself is no longer coalesced.

Wrap listFileVersions() in a single-flight promise (e.g. this.pendingFetch) cleared on settle so simultaneous callers join one in-flight enumeration before lastKnownVersionIds is updated — preserving per-query freshness for sequential calls while restoring coalescing for concurrent bursts. If concurrent-query volume is provably low, add an explicit "concurrent callers not expected" note in DEV.md instead. Are concurrent findBaseForSeq/listVersions calls on a single instance expected in practice?

[anthony-murphy]

Deep Review: M-CACHE-03 is repurposed here to document the new "version leaves the list" behavior and M-CACHE-02 is removed, but DEV.md's own convention (lines 7-13) states scenario IDs are append-only and are never renumbered — reusing or removing IDs breaks traceability between the doc and the tests that reference these IDs.

Keep the old IDs (M-CACHE-02, M-CACHE-03) reserved/deprecated rather than renumbering, and assign new, unused IDs for the added cache behaviors so existing references stay valid — or drop the append-only clause if it is no longer intended.

[anthony-murphy]

Deep Review: In #27689 anthony-murphy asked to export a factory rather than the class — "you should export a function which returns the interface and internally constructs the class. this keeps the implementation off our exposed api surface." The follow-up added createOdspVersionManager returning IOdspVersionManager, but export class OdspVersionManager remains — because tests construct it directly with a mock fetcher (new OdspVersionManager(fetcher) at test/odspVersionManager.spec.ts:59, 214, 249), so a naive "just drop export" would break the suite. The class never reaches the folder barrel (odspVersionManager/index.ts) nor the package public index, so this is narrow, not a public-API leak.

Add a test-injection factory overload accepting IOdspFileVersionFetcher so tests inject the mock without importing the class, then drop export; or add a one-line comment documenting that the class export is retained deliberately for test injection.

[anthony-murphy]

packages/drivers/odsp-driver/src/odspVersionManager/odspFileVersionFetcher.ts:220

Deep Review: The new else branch throws a plain new Error(...) naming the version and content-type. The sibling snapshot path records contentType/accept in propsToLog (fetchSnapshot.ts:372-380) then throws new NonRetryableError("Unknown snapshot content type", OdspErrorTypes.incorrectServerResponse, propsToLog) (fetchSnapshot.ts:502-507), and odspUtils.ts:103-119 treats incorrectServerResponse specially in the token-refresh retry wrapper — so typed classification has observable behavior beyond the message text. A generic Error won't be classified as non-retryable and carries no structured telemetry.

Swap the plain Error for a NonRetryableError with OdspErrorTypes.incorrectServerResponse and contentType/accept telemetry, mirroring fetchSnapshot.ts.

[github-actions]

Bundle size comparison

Base commit: a18ada444eb5c9696ebd8d66691c1772acd9194e
Head commit: 0eece2d7cf0dc733886f6b259bcdd798afb7b70d

Notable changes

No bundles changed by ≥ 500 bytes parsed.

Per-bundle deltas

@fluid-example/bundle-size-tests

• azureClient.js: parsed 620215 → 620271 (+56), gzip 165029 → 165074 (+45)
• odspClient.js: parsed 592941 → 592997 (+56), gzip 159136 → 159178 (+42)
• aqueduct.js: parsed 526704 → 526739 (+35), gzip 140901 → 140930 (+29)
• fluidFramework.js: parsed 395545 → 395566 (+21), gzip 112324 → 112336 (+12)
• sharedTree.js: parsed 384932 → 384946 (+14), gzip 109707 → 109714 (+7)
• containerRuntime.js: parsed 305054 → 305068 (+14), gzip 83403 → 83410 (+7)
• sharedString.js: parsed 175984 → 175991 (+7), gzip 49445 → 49452 (+7)
• experimentalSharedTree.js: parsed 160798 → 160798 (0), gzip 45804 → 45804 (0)
• matrix.js: parsed 159845 → 159852 (+7), gzip 45411 → 45418 (+7)
• loader.js: parsed 145256 → 145270 (+14), gzip 39063 → 39075 (+12)
• odspDriver.js: parsed 104329 → 104350 (+21), gzip 32625 → 32635 (+10)
• directory.js: parsed 66616 → 66623 (+7), gzip 18532 → 18540 (+8)
• 748.js: parsed 58793 → 58793 (0), gzip 17827 → 17827 (0)
• map.js: parsed 46709 → 46716 (+7), gzip 14310 → 14317 (+7)
• odspPrefetchSnapshot.js: parsed 45642 → 45656 (+14), gzip 15277 → 15284 (+7)
• 985.js: parsed 44491 → 44491 (0), gzip 13726 → 13726 (0)
• summarizerDelayLoadedModule.js: parsed 30749 → 30749 (0), gzip 7753 → 7753 (0)
• socketModule.js: parsed 26476 → 26483 (+7), gzip 7887 → 7894 (+7)
• createNewModule.js: parsed 12480 → 12480 (0), gzip 4786 → 4786 (0)
• summaryModule.js: parsed 3797 → 3797 (0), gzip 1860 → 1860 (0)
• connectionState.js: parsed 724 → 724 (0), gzip 429 → 429 (0)
• sharedTreeAttributes.js: parsed 666 → 673 (+7), gzip 433 → 441 (+8)
• debugAssert.js: parsed 429 → 429 (0), gzip 299 → 299 (0)
• FluidFramework-HashFallback.js: parsed 422 → 422 (0), gzip 316 → 316 (0)