Skip to content

fix(tree): track chunkIndex on field entry so getField finds parent #27084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
brrichards wants to merge 1 commit into
base: main
Choose a base branch
from
+108 −15
Open

fix(tree): track chunkIndex on field entry so getField finds parent #27084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 27 additions & 12 deletions packages/dds/tree/src/feature-libraries/chunked-forest/basicChunk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor
// Compute the number of nodes deep the current depth is.
// We want the floor of the result, which can computed using a bitwise shift assuming the depth is less than 2^31, which seems safe.
// eslint-disable-next-line no-bitwise
const halfHeight = (this.siblingStack.length + 1) >> 1;
const halfHeight = this.siblingStack.length >> 1;
Copy link
Copy Markdown
Contributor

@CraigMacomber CraigMacomber Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the old code wrong, but just never getting called in cases where that caused an assert before?

If so, https://github.com/microsoft/FluidFramework/pull/27084/changes#r3103181969 to make it easy to run this in more places seems extra valuable. I think that might have caught this bug.

assert(
this.indexOfChunkStack.length === halfHeight,
0x51c /* unexpected indexOfChunkStack */,
Expand Down Expand Up @@ -203,8 +203,15 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor
return this.indexStack[height] ?? oob();
}

private getStackedNode(height: number): BasicChunk {
const index = this.getStackedNodeIndex(height);
private getStackedChunkIndex(height: number): number {
assert(height % 2 === 1, "must be node height");
assert(height >= 0, "must not be above root");
// eslint-disable-next-line no-bitwise
Copy link
Copy Markdown
Contributor

@CraigMacomber CraigMacomber Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When suppressing the linter, its a good idea to document why its ok to suppress it in this case, like we do in https://github.com/microsoft/FluidFramework/pull/27084/changes#diff-e637d8b2f919c742dad1d0d98d18b101019ade1f3b748623aed966ba4b1119d7R169-R170

That said, we should probably deduplicate that code with this.

Introducing a getNodeOnlyHeightFromHeight method, which takes an optional height defaulting to this.siblingStack.length seems like it would be a good way to deduplicate this logic, and make it a bit more clear what is going on in it.

return this.indexOfChunkStack[height >> 1] ?? oob();
}

private getStackedChunk(height: number): BasicChunk {
const index = this.getStackedChunkIndex(height);
return (this.siblingStack[height] as readonly TreeChunk[])[index] as BasicChunk;
}

Expand Down Expand Up @@ -322,6 +329,11 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor
assert(this.mode === CursorLocationType.Nodes, 0x528 /* must be in nodes mode */);
this.siblingStack.push(this.siblings);
this.indexStack.push(this.index);
// Save the chunk array position of the current node. When siblings contain
// multi node chunks, the flat node index diverges from the array position,
// so getField needs this to locate the parent in the sibling array.
this.indexOfChunkStack.push(this.indexOfChunk);
this.indexWithinChunkStack.push(this.indexWithinChunk);

// For fields, siblings are only used for key lookup and
// nextField and which has arbitrary iteration order,
Expand Down Expand Up @@ -355,6 +367,8 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor

this.siblingStack.push(this.siblings);
this.indexStack.push(this.index);
this.indexOfChunkStack.push(this.indexOfChunk);
this.indexWithinChunkStack.push(this.indexWithinChunk);
Copy link
Copy Markdown
Contributor

@CraigMacomber CraigMacomber Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The places in the code where we edit these parallel arrays could call our validation method afterward ( see https://github.com/microsoft/FluidFramework/pull/27084/changes#r3103181969 ) which could assert they are properly in sync to catch future similar bugs.

this.index = 0;
this.siblings = [...fields.keys()]; // TODO: avoid this copy
return true;
Expand Down Expand Up @@ -422,8 +436,6 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor
}
this.siblingStack.push(this.siblings);
this.indexStack.push(this.index);
this.indexOfChunkStack.push(this.indexOfChunk);
this.indexWithinChunkStack.push(this.indexWithinChunk);
this.index = 0;
this.siblings = siblings;
this.indexOfChunk = 0;
Expand Down Expand Up @@ -486,6 +498,10 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor
this.siblings =
this.siblingStack.pop() ?? fail(0xaf0 /* Unexpected siblingStack.length */);
this.index = this.indexStack.pop() ?? fail(0xaf1 /* Unexpected indexStack.length */);
this.indexOfChunk =
this.indexOfChunkStack.pop() ?? fail("Unexpected indexOfChunkStack.length");
this.indexWithinChunk =
this.indexWithinChunkStack.pop() ?? fail("Unexpected indexWithinChunkStack.length");
Comment on lines +502 to +504
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fail() messages in this package are consistently passed as numeric assert codes (e.g. fail(0xaf0 /* ... */) a couple of lines above). These two new fail("Unexpected …") calls break that convention and make failures harder to triage consistently. Please use new numeric codes here (with brief comments) to match the surrounding error-tagging pattern.

Suggested change
this.indexOfChunkStack.pop() ?? fail("Unexpected indexOfChunkStack.length");
this.indexWithinChunk =
this.indexWithinChunkStack.pop() ?? fail("Unexpected indexWithinChunkStack.length");
this.indexOfChunkStack.pop() ?? fail(0xaf4 /* Unexpected indexOfChunkStack.length */);
this.indexWithinChunk =
this.indexWithinChunkStack.pop() ??
fail(0xaf5 /* Unexpected indexWithinChunkStack.length */);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@brrichards brrichards Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I am just moving this in the file I don't know if I should be re-using the fail codes.

Copy link
Copy Markdown
Contributor

@CraigMacomber CraigMacomber Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is ok to reuse codes when moving the assert/fail around (so long as you don't introduce another copy of it), but it's also ok to remove them.

}

public exitNode(): void {
Expand All @@ -502,16 +518,15 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor
this.siblings =
this.siblingStack.pop() ?? fail(0xaf2 /* Unexpected siblingStack.length */);
this.index = this.indexStack.pop() ?? fail(0xaf3 /* Unexpected indexStack.length */);
this.indexOfChunk =
this.indexOfChunkStack.pop() ?? fail(0xaf4 /* Unexpected indexOfChunkStack.length */);
this.indexWithinChunk =
this.indexWithinChunkStack.pop() ??
fail(0xaf5 /* Unexpected indexWithinChunkStack.length */);
// At the Fields level these aren't semantically used, but reset for consistent state
// (so a fully-iterated cursor matches a fresh cursor at the same logical position).
this.indexOfChunk = 0;
this.indexWithinChunk = 0;
}

private getNode(): BasicChunk {
assert(this.mode === CursorLocationType.Nodes, 0x52f /* can only get node when in node */);
return (this.siblings as TreeChunk[])[this.index] as BasicChunk;
return (this.siblings as TreeChunk[])[this.indexOfChunk] as BasicChunk;
}

private getField(): readonly TreeChunk[] {
Expand All @@ -522,7 +537,7 @@ export class BasicChunkCursor extends SynchronousCursor implements ChunkedCursor
this.mode === CursorLocationType.Fields,
0x530 /* can only get field when in fields */,
);
const parent = this.getStackedNode(this.indexStack.length - 1);
const parent = this.getStackedChunk(this.siblingStack.length - 1);
const key: FieldKey = this.getFieldKey();
const field = parent.fields.get(key) ?? [];
return field;
Expand Down
84 changes: 81 additions & 3 deletions packages/dds/tree/src/test/feature-libraries/chunked-forest/basicChunk.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ import { strict as assert } from "node:assert";

import {
EmptyKey,
type FieldKey,
type ITreeCursor,
type ITreeCursorSynchronous,
type JsonableTree,
type ChunkedCursor,
} from "../../../core/index.js";
// eslint-disable-next-line import-x/no-internal-modules
import { BasicChunk } from "../../../feature-libraries/chunked-forest/basicChunk.js";
import {
BasicChunk,
BasicChunkCursor,
// eslint-disable-next-line import-x/no-internal-modules
} from "../../../feature-libraries/chunked-forest/basicChunk.js";
Comment on lines +16 to +20
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// eslint-disable-next-line import-x/no-internal-modules is currently placed inside the named-import list, which typically won’t suppress a lint finding reported on the ImportDeclaration line. Move the disable comment to immediately precede the import { ... } from ".../basicChunk.js"; statement (or use an eslint-disable block) so CI linting is reliably suppressed for this internal-module import.

Copilot uses AI. Check for mistakes.
import {
basicChunkTree,
basicOnlyChunkPolicy,
Expand All @@ -22,9 +26,14 @@ import {
// eslint-disable-next-line import-x/no-internal-modules
} from "../../../feature-libraries/chunked-forest/chunkTree.js";
// eslint-disable-next-line import-x/no-internal-modules
import { uniformChunk } from "../../../feature-libraries/chunked-forest/index.js";
import { dummyRoot, uniformChunk } from "../../../feature-libraries/chunked-forest/index.js";
// eslint-disable-next-line import-x/no-internal-modules
import { SequenceChunk } from "../../../feature-libraries/chunked-forest/sequenceChunk.js";
import {
TreeShape,
UniformChunk,
// eslint-disable-next-line import-x/no-internal-modules
Comment on lines +32 to +35
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above: the // eslint-disable-next-line import-x/no-internal-modules comment is inside the import specifier list, so it may not suppress the rule on the ImportDeclaration. Place the disable comment immediately above the import { TreeShape, UniformChunk } ... statement to avoid lint failures.

Suggested change
import {
TreeShape,
UniformChunk,
// eslint-disable-next-line import-x/no-internal-modules
// eslint-disable-next-line import-x/no-internal-modules
import {
TreeShape,
UniformChunk,

Copilot uses AI. Check for mistakes.
} from "../../../feature-libraries/chunked-forest/uniformChunk.js";
import {
type TreeChunk,
chunkTree,
Expand Down Expand Up @@ -155,6 +164,75 @@ describe("basic chunk", () => {
assert(!cursor.atChunkRoot());
});

it("getField resolves parent via chunk array index when preceded by a multi-node chunk", () => {
// A root field with 3 logical nodes split across 2 chunks:
// chunks[0]: UniformChunk with 2 number nodes.
// chunks[1]: BasicChunk holding a single subField leaf.
const numberShape = new TreeShape(brand(numberSchema.identifier), true, []);
const uniform = new UniformChunk(numberShape.withTopLevelLength(2), [10, 20]);

const subField: FieldKey = brand("subField");
const subLeaf = new BasicChunk(brand(numberSchema.identifier), new Map(), 42);
const trailingBasic = new BasicChunk(brand("Trailing"), new Map([[subField, [subLeaf]]]));

const cursor = new BasicChunkCursor(
[uniform, trailingBasic],
// siblingStack, indexStack, indexOfChunkStack, indexWithinChunkStack
[],
[],
[],
[],
[dummyRoot],
// index, indexOfChunk, indexWithinChunk
0,
0,
0,
undefined,
);

// Logical node index 2 maps to chunk array index 1 (the trailing BasicChunk).
cursor.enterNode(2);

// enterField drives getField, which on main looks up the parent at the
Copy link
Copy Markdown
Contributor

@CraigMacomber CraigMacomber Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code is going to be on main soon. If you want to comment on some specific comparison between this branch and main which will no longer apply after the PR merges, you can do so in a PR comment, or if putting it in the code, you need to phrase it differently.

Anyway, thanks for the explanation.

// logical node index (2) instead of the chunk array index (1). siblings[2]
// is out of bounds in the 2-chunk array, so the cast returns undefined and
// the subsequent field read throws.
Comment on lines +196 to +199
Copy link
Copy Markdown
Contributor

@CraigMacomber CraigMacomber Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say "looks up the parent", that's a bit confusing.

I think this is coming from the use of the variable name "parent" in get field, which is also rather poor.

Our trees and cursors do provide the ability to look up the parents of nodes, which sounds like what is being done here, but isn't.

Its instead simply looking up the current node, which it happens to call parent, since it's the parent of the field that method is operating on.

Also, you should make it clear why it was wrong to use a node index.

Suggested change
// enterField drives getField, which on main looks up the parent at the
// logical node index (2) instead of the chunk array index (1). siblings[2]
// is out of bounds in the 2-chunk array, so the cast returns undefined and
// the subsequent field read throws.
// enterField uses getField, which must looks up the current node (who's field should be gotten).
// This must handle when the index of the node does not match the index of the chunk containing that node.
// This test is a node index 2 and chunk index 1.

cursor.enterField(subField);
assert.doesNotThrow(() => cursor.getFieldLength());
Copy link
Copy Markdown
Contributor

@CraigMacomber CraigMacomber Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than simply asserting it doesn't throw, it would be better to assert that enter field actually worked correctly (its easy to have bugs that don't throw).

Asserting the field length is 1, and that if you enter that node you get a value of 42 should do it I think.

});

it("getField resolves parent via chunk array index when preceded by two multi-node chunks", () => {
// Root field with 5 logical nodes across 3 chunks:
// chunks[0]: UniformChunk with 2 number nodes.
// chunks[1]: UniformChunk with 2 number nodes.
// chunks[2]: BasicChunk with a single subField leaf.
const numberShape = new TreeShape(brand(numberSchema.identifier), true, []);
const uniformA = new UniformChunk(numberShape.withTopLevelLength(2), [10, 20]);
const uniformB = new UniformChunk(numberShape.withTopLevelLength(2), [30, 40]);

const subField: FieldKey = brand("subField");
const subLeaf = new BasicChunk(brand(numberSchema.identifier), new Map(), 99);
const trailingBasic = new BasicChunk(brand("Trailing"), new Map([[subField, [subLeaf]]]));

const cursor = new BasicChunkCursor(
[uniformA, uniformB, trailingBasic],
[],
[],
[],
[],
[dummyRoot],
0,
0,
0,
undefined,
);

// Logical node index 4 maps to chunk array index 2 (the trailing BasicChunk).
cursor.enterNode(4);
cursor.enterField(subField);
assert.doesNotThrow(() => cursor.getFieldLength());
});

describe("SequenceChunk", () => {
it("root", () => {
validateChunkCursor(new SequenceChunk([numericBasicChunk(0)]), numberSequenceField(1));
Expand Down
Loading