refactor(container-runtime): extract feature/manager classes from ContainerRuntime

[anthony-murphy]

Description

Drives containerRuntime.ts from 5582 → 4900 lines (-682, ~12%) by introducing an IRuntimeFeature framework and extracting purpose-built classes. Motivation: the ContainerRuntime class is a monolith — too many concerns are tangled inside it, message dispatch is split across multiple residual switches, and inline closures plumb runtime state into nearly every subsystem constructor. This PR establishes a feature pattern that subsystems opt into, then migrates twelve subsystems / concerns onto it.

Feature framework (runtimeFeature.ts, runtimeFeatureCollection.ts): IRuntimeFeature<TOps> interface + composite RuntimeFeatureCollection that fans out lifecycle calls and routes ops O(1) via a Map<ContainerMessageType, IRuntimeFeature> built from each feature's supportedOps. Hooks: onLoadFromSnapshot, setConnectionState, notifyStagingMode, dispose, contributeSummary, handleOp, applyStashedOp, reSubmitOp, rollbackStagedOp. The op-routing hooks are parameterized on TOps so each feature's message/messagesContent parameters narrow to the variants it claims (helpers: InboundRuntimeMessageFor, LocalRuntimeMessageFor, RuntimeMessagesContentFor).

Subsystems migrated to features (own their own routing instead of being switched-on inside the runtime):

• ChannelCollection, BlobManager, GarbageCollector, DocumentsSchemaController — op routing + summary contribution
• PendingStateManager, RemoteMessageProcessor, DuplicateBatchDetector, InboundBatchAggregator, DeltaScheduler
• SignalTelemetryManager — self-resets via setConnectionState

New feature classes:

• IdCompressorFeature — owns the compressor instance, pending-range queue, on-boot/delayed load, IdAllocation message routing, summary blob, and outbound LocalBatchMessage shape
• RuntimeOpsFeature — Rejoin / ChunkedOp routing
• SummarizerSubsystem — election summary blob

New helper classes (not features, just monolith reduction):

• ReconnectTracker — consecutive-reconnects-with-no-progress detector
• LoadingGroupSnapshotFetcher — per-loadingGroupId snapshot cache + catch-up wait
• ExtensionsManager — ContainerExtension store, layer-compat plumbing, lazy event emitter wiring

Op-routing collapse: validateAndProcessRuntimeMessages, applyStashedOp, reSubmit, rollbackStagedChange all converged on the same shape — features.<verb>Op decides ownership; runtime only handles the unknown-type fail. The IdAllocation / Rejoin / ChunkedOp residual switches are gone.

Inline closure cleanup: outbox generateIdAllocationOp (17-line closure) moved into IdCompressorFeature.generateAllocationOp; runtime's applyStashedOp + parseLocalOpContent removed (PSM dispatches directly through features.applyStashedOp).

Reviewer Guidance

The review process is outlined on this wiki page.

This is a draft for early feedback. Specific things I'd appreciate eyes on:

• Feature framework shape — IRuntimeFeature is intentionally optional-everything; the runtime calls features.<hook>(...) and each member implements only what applies. Is the surface right? Anything missing for future migrations?
• Op-routing dispatch ownership — each ContainerMessageType has exactly one owning feature; duplicate claims throw at construction time (registerOpClaims). Registration order matters only for fan-out hooks (onLoadFromSnapshot, contributeSummary, setConnectionState, etc.), which iterate features in order.
• SignalTelemetryManager.setConnectionState reset moved from explicit call inside setConnectionStateCore to a self-tracked transition (was→is canSendOps). Behaviorally equivalent, but worth a check.
• Skipped after analysis (and noted at extraction time):
  • Folding notifyReadOnlyState into setConnectionState — separate signal path from the legacy _deltaManager.on("readonly") listener; risks regressions.
  • IRuntimeContext shared object — would touch every subsystem constructor; deserves its own branch.
  • submitSummary (375 lines) and processInboundMessageOrBatch (143 lines) — too tightly coupled to runtime state for clean extraction without the shared-context refactor.

Test results:

• @fluidframework/container-runtime mocha — 1966 passing, 2 pending, 0 failing
• @fluid-internal/local-server-stress-tests — 199 passing, 1 pending, 0 failing
• @fluid-internal/local-server-tests — 65 passing, 4 pending; 8 unrelated captureFullContainerState failures (function lives on a different branch)

🤖 Generated with Claude Code

[github-actions]

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

Based on the diff (3197 lines, 23 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

Toggle the reviewer checkboxes above to adjust, then tick the box below to start:

• Start review

[anthony-murphy]

Deep Review: RuntimeFeatureCollection.applyStashedOp returns undefined for two distinct conditions: (a) no feature owns the op type, and (b) the owning feature has no applyStashedOp:

const feature = this.opOwners.get(opContents.type as ContainerMessageType);
if (feature?.applyStashedOp === undefined) { return undefined; }
return feature.applyStashedOp(opContents);

PendingStateManager.dispatchStashedOp then treats undefined as "unknown op type" and calls this.stateHandler.closeFn(error); throw error;. chunkedOpsGuardFeature (in runtimeOpsFeature.ts) claims ContainerMessageType.ChunkedOp via supportedOps: CHUNKED_OPS but provides only handleOp — so its claim is silently downgraded to "no handler" inside this collapse and the container closes. The sibling rejoinFeature in the same file does the right thing: it provides an explicit-throw applyStashedOp. The two guards in the same file diverge on this contract.

Well-formed local replay can't carry ChunkedOp (it's excluded from LocalContainerRuntimeMessage in messageTypes.ts), so the trigger requires corrupt or forward-incompatible pending JSON — but the asymmetry is real, and it is also present symmetrically in handleOp / reSubmitOp / rollbackStagedOp.

Suggestions, in order of effort:

1. Smallest — give chunkedOpsGuardFeature.applyStashedOp an explicit throw matching rejoinFeature. Closes the same-file divergence at minimum cost.
2. More thorough — distinguish the two return cases at the collection level, e.g. add a hasOwner(type: ContainerMessageType): boolean helper and let PSM check it before treating undefined as unknown, or switch to a tagged result { kind: "no-owner" | "no-apply" | "applied", result?: unknown }. Apply uniformly across handleOp / applyStashedOp / reSubmitOp / rollbackStagedOp so the contract is consistent.

[anthony-murphy]

Deep Review: The PR description says "Op-routing dispatch order — feature registration order determines who claims each message type first… The channelCollection claim is intentionally before idCompressorFeature so FluidDataStoreOps short-circuit fastest." That's not how this code works. registerOpClaims builds a Map<ContainerMessageType, IRuntimeFeature> and throw new Error(`RuntimeFeatureCollection: multiple features claim ${type}`) on a second claim — there is at most one owner per op type, and registration order is irrelevant for op routing. channelCollection.supportedOps = [FluidDataStoreOp, Attach, Alias] and idCompressorFeature.supportedOps = [IdAllocation] don't even overlap; even if they did the second add() would throw at construction time.

The in-source comment at the top of this file (the Order matters for fan-out hooks / Op-routing dispatch is type-keyed and order-independent block) already documents the correct invariant. Only the PR description disagrees.

Since this PR is deep-review-labelled, reviewers will read the metadata first and look for an ordering invariant that doesn't exist. Suggest editing the description to:

Op-routing dispatch order — each ContainerMessageType has exactly one owning feature; duplicate claims throw at construction time (registerOpClaims). Registration order matters only for fan-out hooks (onLoadFromSnapshot, contributeSummary, setConnectionState, etc.), which iterate features in order.

[anthony-murphy]

Deep Review: PR #20174 (vladsud, signed off by andre4i) deliberately authored the rule on schema transitions for the id compressor: "this will only work for 'off' -> 'delayed' transitions. Anything else is too risky, and requires ability to initialize ID compressor synchronously!" The warning still lives at containerRuntime.ts above the schema-transition decision point (onSchemaChange), but the actual loading site is now this loadDelayed method, which only documents "Lazy load for 'delayed' mode — finalizes any ranges that piled up" with no reference to the constraint.

It's true that loadDelayed() has multiple non-transition callers (load-time and connection-time) — but that is exactly why a future second transition caller could miss the invariant now that the implementation moved files.

Suggest copying a one-line warning into the docstring, e.g.:

NOTE: only safe for off → delayed schema transitions; any other transition requires synchronous ID-compressor initialization (see containerRuntime.onSchemaChange).

That way the constraint travels with the implementation.

[anthony-murphy]

Deep Review: The pre-PR runtime did if (canSendOpsChanged) { this.signalTelemetryManager.resetTracking(); } — reset on both false→true and true→false transitions. The new self-tracked behavior is if (canSendOps && !this.wasCanSendOps) { this.resetTracking(); } — only resets on false→true. The disconnect path no longer triggers a reset; any tracking state polluted while canSendOps === false is carried forward until the next connect.

The PR description flags this as "behaviorally equivalent, but worth a check" — it isn't strictly equivalent. Practical impact is narrow because telemetry is scoped via message.clientId === this.clientId and the next-send window is wiped before reuse, but the truth-table changed and nothing pins it: no test in runtimeFeatureCollection.spec.ts or containerRuntime.spec.ts asserts which canSendOps transitions trigger reset (read-only ↔ read-write while still connected, forced read-only toggle, reconnect during read-only).

Suggestions:

1. Preferred — restore byte-equivalence with if (canSendOps !== this.wasCanSendOps) { this.resetTracking(); }.
2. Alternative — confirm with telemetry owners (vladsud / ChumpChief, who debated canSendOps semantics in PR Move op replay count telemetry to the runtime layer #16104) that "reset on reconnect only" is intentional.

In either case, add a focused unit test on SignalTelemetryManager.setConnectionState that asserts reset fires exactly on the chosen transition set and not on the others — covering the four (wasCanSendOps, canSendOps) combinations.

[anthony-murphy]

Deep Review: replace<T> swaps this.features[index] = replacement, deletes the old feature's op-claims, then registerOpClaims(replacement) and returns. oldFeature.dispose?.() is never called, even though dispose is part of IRuntimeFeature and the collection's own dispose() (lines 103-105) calls f.dispose?.() on each feature — so replace is the one lifecycle path that leaks.

Migrated subsystems implementing dispose include BlobManager (with internal emitters), SummarizerSubsystem (SummaryCollection / SummaryManager listeners), and ChannelCollection. The JSDoc says "Primarily a test-fixture seam" / "production code should rarely need this" — language that invites non-test use without forbidding it.

Suggestions:

1. Preferred — call oldFeature.dispose?.() inside replace to bring the contract in line with the rest of the collection's lifecycle treatment. One line, removes the footgun entirely.
2. Alternative — tighten the JSDoc to "test-only — do not use in production", optionally rename to replaceForTesting, and assert/guard against production callers.

[anthony-murphy]

Deep Review: The pre-PR initializeBaseState ran await this.initializeSummarizer(loader); await this.garbageCollector.initializeBaseState(); — summarizer first, GC second. The new features.onLoadFromSnapshot() iterates in registration order, and garbageCollector is registered before summarizerSubsystem, so GC initializes first now. The PR description claims behavioral equivalence; on this hook that claim is false.

No concrete bug is triggered today: SummarizerSubsystem.onLoadFromSnapshot doesn't currently read GC base state — it builds election machinery from injected summary, delta-manager, quorum, loader, and runtime dependencies (see summarizerSubsystem.ts:137-269). But the ordering is observably inverted, and no test pins the registration order, so a future change could silently re-invert it.

This combines with a more general concern: the registration sequence in ContainerRuntime has no comment explaining why each feature is in its current slot, even though several fan-out hooks are order-sensitive (SignalTelemetryManager's transition reset, contributeSummary summary-tree key ordering, GC vs. ChannelCollection on connect, GC vs. SummarizerSubsystem on load).

Suggestions:

1. Either register summarizerSubsystem before garbageCollector to preserve the prior load-order, or document explicitly that onLoadFromSnapshot ordering between these two is independent.
2. Add a comment at the registration site listing why order matters for each fan-out hook, plus a test that pins the registration order so refactors don't silently change setConnectionState / contributeSummary / onLoadFromSnapshot semantics.

[anthony-murphy]

Deep Review: IRuntimeStateHandler gains closeFn; dispatchStashedOp now calls this.stateHandler.closeFn(error); throw error; when no feature claims the op type. Pre-PR this lived in ContainerRuntime.applyStashedOp's default branch — PSM is now both dispatcher and closer for an entire class of corruption-grade errors.

No demonstrated defect: the coupling is narrow (Pick<RuntimeFeatureCollection, "applyStashedOp">), PSM already owned corruption-detection semantics for forked-container / mismatched pending-message conditions, and existing tests cover stashed-op application, empty-batch handling, replay batching, batchId persistence, staged batches, and local-op metadata preservation. The concern is coverage proportional to the historical pattern: PSM has been a dense locus of forked-container / batch-id correctness work (#21714, #21767, #22310, #19802), and large mechanical moves through containerRuntime.ts have repeatedly required follow-up PRs.

Suggestion: add a unit/integration test tracing one local/stashed batch through PendingStateManager.dispatchStashedOp → RuntimeFeatureCollection.applyStashedOp → features.reSubmitOp → duplicate-batch summary contribution. Confirm batch boundaries, opMetadata/localOpMetadata separation, and batchId persistence are all preserved across the four-hook re-routing, and that closeFn here doesn't race with the existing disposeOnce path. Worth tagging markfields on the dispatchStashedOp change.

[anthony-murphy]

Deep Review: The top-level summary-tree blob/tree key ordering shifted vs. the deleted addContainerStateToSummary.

Old (deleted at pr.diff lines ~1147-1190) added in fixed order: idCompressor → chunks → recentBatchInfo → aliases → electedSummarizer → blobs tree → gc tree.

New features.contributeSummary iterates registration order: chunks (RemoteMessageProcessor) → recentBatchInfo (DuplicateBatchDetector) → gc tree (GarbageCollector) → aliases (ChannelCollection) → blobs tree (BlobManager) → idCompressor (IdCompressorFeature) → electedSummarizer (SummarizerSubsystem).

Looked into whether this is a wire-format break: PR #17285's "set the properties in the order they appear here … stringified values can be compared" invariant applies to TypedContainerRuntimeMessage and PSM pending-message stringification (messageTypes.ts:64-82, pendingStateManager.ts:156-162), not to summary-tree key order; the upload site (containerRuntime.ts:4057-4060) sends summaryTree as a named-entry object. So no demonstrated break — but the ordering observably changed without being called out, and no test pins it.

Suggestions:

1. Preferred — reorder feature registration in containerRuntime.ts to match the previous summary-tree key sequence; the registration-order pin test (separate thread) then guards against future drift.
2. Alternative — document the new canonical order in RuntimeFeatureCollection.contributeSummary and add a snapshot test pinning the top-level summary-tree key sequence.

A quick grep across storage drivers / snapshot-diff tests outside container-runtime would settle whether anything downstream relies on the historical order.

[anthony-murphy]

Deep Review: Two cheap test-only additions would close a class of risk that this round's findings keep surfacing — the dispose and onLoadFromSnapshot order inversions, the summary-tree key reorder, the duplicate-instance add gap, and the chunkedOpsGuardFeature completeness gap are all instances where add() order or registration completeness silently shifted observable behavior.

1. Completeness assertion — iterate Object.values(ContainerMessageType) in a freshly-constructed ContainerRuntime's feature collection and assert each has an owner. Forces the registry to be the single source of truth for op-type ownership. Could also tighten registerOpClaims to require each owning feature to implement at least one op-routing hook for each claimed type (i.e. surface the chunkedOpsGuard "claims but no applyStashedOp" case at construction).
2. Ordering pin — assert the registration array (or each fan-out hook's call order) matches a hand-curated expected sequence. So that any future re-register that silently reorders dispose / contributeSummary / onLoadFromSnapshot / setConnectionState surfaces in CI.

The existing runtimeFeatureCollection.spec.ts covers fan-out order on the abstract collection (e.g. :13-33, :90-105, :166-184) but no test pins the ContainerRuntime registration array. That's the gap.

[anthony-murphy]

Deep Review: add() calls registerOpClaims(feature), and registerOpClaims only throws when existing !== undefined && existing !== feature. The existing !== feature short-circuit silently accepts a re-add of the same instance — the feature ends up in the this.features array twice, so all fan-out hooks (lifecycle, contributeSummary, setConnectionState, dispose) fire twice for that subsystem.

PR #17285 (noencke) and PR #20174 (vladsud) established exactly-once ownership of message types and exactly-once schema/blob writes; a duplicated feature would silently double-write summary blobs and double-fire connection-state callbacks. No current caller exercises this, but the contract gap is real.

Suggestion — one-line guard in add():

if (this.features.includes(feature)) {
    throw new Error(`RuntimeFeatureCollection: feature already registered`);
}

or drop the existing !== feature short-circuit in registerOpClaims and require callers to use replace() for the legitimate re-registration case.

[anthony-murphy]

Deep Review: The diff replaces a live callback (getClientId: () => this.clientId) with a cached field currentClientId that's updated only by setConnectionState. Previously the localBatch flag was computed against the live clientId on every comparison; now any inbound op processed before the first setConnectionState callback compares against undefined.

Narrowing impact: currentClientId is read only in error-detail objects (localBatch for the system-message-during-batch error, localBatch/localMessage for interleaved-client batch corruption errors), so it's telemetry-on-error-paths only. But it's an unnecessary semantic change in a "behavior-preserving" refactor — and on the rare path where one of these errors fires before the first setConnectionState callback, the telemetry will misreport localBatch/localMessage.

Suggestions:

1. Smallest — initialize currentClientId from the runtime at construction so the first read is never undefined.
2. Cleanest — retain the original getClientId callback (or expose clientId through a runtime-context object) so the value is always live and there's no caching-sync hazard.

[anthony-murphy]

Deep Review

Reviewed commit 6a013ab on 2026-05-05.

Readiness: 7/10 — ALMOST READY

A disciplined refactor: 12 subsystems extracted from ContainerRuntime onto a new IRuntimeFeature framework, in-package mocha green, and an independent design pass converged on essentially the same shape. No Tier 1/2 issues survive analysis. Remaining items are doc/test polish on top of the existing inline thread cluster, plus two behavior diffs the PR description already calls out (SignalTelemetryManager reset and summary-tree blob iteration order) that are worth pinning explicitly before promotion out of draft.

Path to Ready

• Resolve inline threads
• Add a Known behavior diffs (telemetry-only) section to the PR description covering (a) SignalTelemetryManager reset now firing only on false → true canSendOps (was: any canSendOps change while attached; detached-reset precluded by the existing setConnectionStateCore assert) and (b) summary-tree blob iteration order moving from fixed (metadata → idCompressor → chunks → recentBatchInfo → aliases → electedSummarizer → blobs → gc) to feature-registration order (metadata → chunks → recentBatchInfo → gc → aliases → blobs → idCompressor → electedSummarizer) — ISummaryTreeWithStats is dictionary-keyed so tree hashes are unaffected, but ordered-iteration consumers (snapshot diff harnesses, stringify-based telemetry) would observe a delta.
• Add a comment block at the feature-registration site naming the cross-feature ordering constraints (specifically DocumentsSchemaController precedes IdCompressorFeature for onLoadFromSnapshot/contributeSummary, since schema must be applied before id-compressor observes off → delayed).
• Add a one- or two-line comment beside IRuntimeStateHandler.closeFn (or beside dispatchStashedOp's closeFn(error); throw error; site) noting alignment with the existing close-then-throw pattern at containerRuntime.ts:2956-2968 / :4511-4522. PR Support applyingStashedOps while detached #19802's detached invariant is preserved by applyStashedOpsAt's isAttached() gate; the layering rationale is currently undocumented.
• Add a construction-time exhaustiveness assertion verifying every ContainerMessageType enum value is present in opOwners (mirrors the existing duplicate-claim throw in registerOpClaims).
• Add (or link from a comment in IdCompressorFeature) a stash → rehydrate → resubmit test driving a mixed FluidDataStoreOp + IdAllocation batch — assert exactly one fresh range is generated by generateAllocationOp and no IdAllocation op is double-submitted. The no-op reSubmitOp/applyStashedOp contract is load-bearing for offline/forked-container scenarios (PR Offline: Add batchId to batch metadata on resubmit #21767) and currently unpinned at the new feature-collection boundary.

Context for Reviewers

• Highest-churn region in the runtime — bugs introduced here will be hard to localize via blame. containerRuntime.ts shows 30 commits in 6mo across 18 PRs; the containerRuntime.ts hunk (+235/-921) touches code introduced or rewritten by nearly all of the offline/forked-container work merged in 2024 (Offline: Provide incoming batch's starting CSN to the PendingStateManager #21714, Offline: Add batchId to batch metadata on resubmit #21767, Send empty group batch on resubmit #21854, Offline: Detect duplicate sequenced batches and close the container #22310, Offline: Add recent batch info from DuplicateBatchDetector to summary #22454). Most lines here are moved, not authored.
• Domain experts whose prior decisions this PR re-homes — request reviews from markfields (PSM / RemoteMessageProcessor / DuplicateBatchDetector / Outbox-resubmit; Offline: Provide incoming batch's starting CSN to the PendingStateManager #21714, Offline: Add batchId to batch metadata on resubmit #21767, Offline: Detect duplicate sequenced batches and close the container #22310, Offline: Add recent batch info from DuplicateBatchDetector to summary #22454), vladsud (DocumentsSchemaController feature + schema-change resubmit semantics; Create framework for safe rollout of back-compatible runtime document schema changes #20174, and canSendOps semantics in Move op replay count telemetry to the runtime layer #16104), noencke (op-type generics on IRuntimeFeature; Make container runtime message types stricter #17285); courtesy ping tyler-cai-microsoft on offline back-compat for Offline: Add batchId to batch metadata on resubmit #21767's enableOfflineLoad gate now reading through RuntimeFeatureCollection.reSubmitOp, and andre4i on schema-change documentation location.
• Loader/runtime back-compat surface to verify — enableOfflineLoad is the single gate for all offline scenarios (per Offline: Add batchId to batch metadata on resubmit #21767 review thread). PSM, RemoteMessageProcessor, DuplicateBatchDetector, and resubmit logic all moved onto features; IRuntimeStateHandler adds closeFn and removes applyStashedOp. Specifically check (1) old stashed-state formats still parse through the new dispatchStashedOp path; (2) enableOfflineLoad-gated behavior in resubmit is preserved through RuntimeFeatureCollection.reSubmitOp; (3) the IRuntimeStateHandler interface change is internal-only (no external implementers).
• IRuntimeFeature is a new exported surface (src/index.ts adds IRuntimeFeature and helper types). Rollback infrastructure #10067's rollback-infrastructure review pushed back on exposing new runtime extension shape before it was fully supported. Confirm the export is intentionally @internal and downstream consumers are not being offered a premature extension API. The added RuntimeFeatureCollection unit tests align with the historical request for test seams.

For human reviewer

• markfields sign-off on PendingStateManager.dispatchStashedOp calling closeFn — author of Support applyingStashedOps while detached #19802, owner of offline/forked-container infrastructure. Is the close-then-throw pattern at the PSM boundary acceptable given the Support applyingStashedOps while detached #19802 detached invariant is preserved by applyStashedOpsAt's isAttached() gate?
• vladsud sign-off on DocumentsSchemaController implements IRuntimeFeature<ContainerMessageType.DocumentSchemaChange> and the resubmit/applyStashed semantics (preserved verbatim, but now living in documentSchema.ts rather than the runtime resubmit switch). Is the per-feature documentation location sufficient for the PR Create framework for safe rollout of back-compatible runtime document schema changes #20174 commitment?
• vladsud sign-off on IdCompressorFeature ownership of the off → delayed transition, the onLoadFromSnapshot ordering relative to DocumentsSchemaController, and the no-op reSubmitOp contract.
• noencke sanity-check on InboundRuntimeMessageFor / LocalRuntimeMessageFor / RuntimeMessagesContentFor and the unavoidable widening to IRuntimeMessagesContent[] at the collection-dispatch boundary — does the new helper layer regress the typed-boundary work from PR Make container runtime message types stricter #17285?
• Design taste call — single combined IRuntimeFeature vs. split message+lifecycle contracts, and direct subsystem implementation vs. adapter classes. Both independent design proposals preferred the alternatives; the PR's choices are defensible. Reviewer judgment, not a code-fix item.
• Load-latency check — the lifecycle fan-out (for (const f of this.features) { await f.onLoadFromSnapshot?.(); }) is sequential; verify nothing in the prior implementation ran in parallel and would regress load latency.