fix(container-runtime): prevent invalid handle reuse after parent skip recursion#27659
fix(container-runtime): prevent invalid handle reuse after parent skip recursion#27659jzaffiro wants to merge 1 commit into
Conversation
…p recursion When a parent summarizer node emits a handle (skips recursion), its children's content is not directly stored in the new summary version - only transitively accessible via the parent's handle chain. Previously, children would still have their _lastSummaryReferenceSequenceNumber updated, allowing them to emit handles in subsequent summaries. This caused fluidElementNotFound (404) errors from the ODSP storage service because it cannot resolve nested paths through handle references. The fix clears _lastSummaryReferenceSequenceNumber for all descendants when parentSkipRecursion is true, forcing them to fully re-summarize the next time their content is needed directly in a summary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (131 lines, 3 files), I've queued these reviewers:
How this works
|
Bundle size comparisonBase commit: Notable changes
Per-bundle deltas
|
|
I don't understand what the bug is. What's described in |
| // _lastSummaryReferenceSequenceNumber so that this node does not attempt to emit a | ||
| // handle in future summaries pointing to this version, because the storage service | ||
| // cannot resolve nested paths through handle references. | ||
| this._lastSummaryReferenceSequenceNumber = undefined; |
There was a problem hiding this comment.
This is not right. This breaks incremental summary - basically whenever a parent hasn't changed, in the next summary, all of it's children will re-summarize.
The comment is also incorrect / unclear. Handles are a client / runtime concept - when the service sees handles, it basically gets the summary tree from the previous summary and replaces the handle with it. So, in the next summary, if a child doesn't change and uses handle to point to this summary, the server should find the contents just fine.
2 participants
Description
Fixes a summarizer bug that caused ODSP
404 fluidElementNotFounderrors during incremental summarization, most visibly for containers using SharedTree as the root DDS:Root cause
When a
SummarizerNodeemits aSummaryType.Handle(skips recursion), its subtree's content is not directly stored in the resulting summary version — the child paths are only reachable transitively through the parent's handle reference. However,completeSummaryCorewas still promoting each descendant's_lastSummaryReferenceSequenceNumber(viarefreshLatestSummaryFromPending) as if their content had been written.On a later summary, if the parent became dirty and re-summarized while a descendant had not changed, the descendant would emit a handle pointing back to the version where its content only lived behind the parent's handle. The ODSP snapshot upload path (
odspSummaryUploadManager.writeSummaryTree) then constructed an id like${parentHandle}/${rootNodeName}${handlePath}that the server cannot resolve, because it does not follow nested handle references, yieldingfluidElementNotFound.Why this is disproportionately visible on SharedTree
SharedTree's
ForestIncrementalSummaryBuilderemits many nested chunk-level handles inside the DDS itself — a pattern unique among current DDSes. Workloads with large trees and frequent GC state churn on the containing data store made the bad handle emission a near-certainty. Other DDSes only reuse handles at the DDS-root boundary and largely dodge the bug.Fix
In
SummarizerNode.completeSummaryCore, whenparentSkipRecursionis true, clear_lastSummaryReferenceSequenceNumberon the entire descendant subtree (not just newly-created nodes, which was the prior guard). This forces those descendants to emit a fullSummaryType.Treethe next time they are summarized instead of an unresolvable handle. Once they've been directly summarized and acked, normal handle reuse resumes.The clearing also indirectly disables SharedTree's chunk-level incremental handles for that next summary, since
summarizerNode.tspassesincrementalSummaryContext: undefinedwhen there is no tracked reference sequence number.Cost
Bounded and rare: it only triggers on the summary immediately after a parent used a handle and then became dirty. Cost is one extra full serialization of the affected subtree, one time per cycle. The prior behavior produced broken summaries and forced full retries, which is strictly worse.
Repro / verification
Two regression tests were added in
summarizerNode.spec.tsunderHandle Reuse After Parent Skip Recursion:All 998
container-runtimetests pass locally. Linting and formatting are clean.Reviewer Guidance
The review process is outlined on this wiki page.
SummarizerNodeWithGc.completeSummaryCore. With the fix, when the parent skips recursion,super.completeSummaryCoredoes not add the descendant topendingSummaries; the GC subclass's lookup then gracefully no-ops, and the 15 GC-focused tests still pass.