MILAB-6303: tree-sync introspection metrics + stop-marker follow-up fix#1760
Draft
xnacly wants to merge 6 commits into
Draft
MILAB-6303: tree-sync introspection metrics + stop-marker follow-up fix#1760xnacly wants to merge 6 commits into
xnacly wants to merge 6 commits into
Conversation
Extend the MI_LOG_TREE_STAT counters with per-cycle re-fetch classification computed in updateFromResourceData: new, changed and unchanged resources, wasted downlink bytes, BFS fetches spent on unchanged resources, and changes that re-streamed stable metadata. Quantifies how much of each poll re-fetches resources the client already holds. Reuses the existing changed flag, no extra work per resource.
Expand MI_LOG_TREE_STAT into full per-path introspection: streaming detail (rounds, resource/stop-marker frames, follow-up seeds, traverse-stopped), BFS detail (resources requested, not found), and a per-cycle change breakdown (fields added/removed/changed, kv, ready flips, locks, errors, duplicates resolved, resources marked final), alongside the existing cross-cycle duplication counters. Fix the resourceTree stop-marker follow-up to propagate traverseStopRules so the retry stops at the same boundaries instead of traversing the stop-marked subtrees unbounded. Assert that one follow-up round resolves every stop marker (throws if a deeper stop-marker chain remains, which would mean the retry must become a loop).
🦋 Changeset detectedLatest commit: f329cc3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request instruments tree-sync statistics (MI_LOG_TREE_STAT) to provide detailed introspection on cross-cycle duplication, per-path details, and cycle changes, while also fixing stop-marker follow-up propagation. The review comments correctly identify a critical issue where adding, modifying, or deleting KV entries does not set the changed flag to true. This omission causes incorrect state versioning, inaccurate introspection metrics, and prevents proper final state evaluation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+759
to
+766
| if (stat) stat.kvChanged++; | ||
| } else if (Buffer.compare(current, kv.value) !== 0) { | ||
| resource.kv.set(kv.key, kv.value); | ||
| notEmpty(resource.kvChangedPerKey).markChanged( | ||
| kv.key, | ||
| `kv changed for ${resourceIdToString(resource.id)}: ${kv.key}`, | ||
| ); | ||
| if (stat) stat.kvChanged++; |
Contributor
There was a problem hiding this comment.
When KV entries are added or modified, the changed flag is not set to true. This leads to several issues:
- Incorrect State Versioning:
resource.dataVersionis not updated toresource.version, meaning external observers or caching layers won't detect the KV changes. - Incorrect Introspection Metrics: The resource is incorrectly counted under
resourcesUnchangedinstead ofresourcesChangedin the newly addedResourceUpdateStatmetrics, which also incorrectly inflatesbytesUnchangedandbfsRequestsWasted. - Final State Evaluation: Bumping
changedensures thatisFinalPredicateis re-evaluated, which is important if the finality of a resource depends on its KV values.
We should set changed = true when KV entries are added or modified.
changed = true;
if (stat) stat.kvChanged++;
} else if (Buffer.compare(current, kv.value) !== 0) {
resource.kv.set(kv.key, kv.value);
notEmpty(resource.kvChangedPerKey).markChanged(
kv.key,
`kv changed for ${resourceIdToString(resource.id)}: ${kv.key}`,
);
changed = true;
if (stat) stat.kvChanged++;| key, | ||
| `kv deleted for ${resourceIdToString(resource!.id)}: ${key}`, | ||
| ); | ||
| if (stat) stat.kvChanged++; |
Contributor
There was a problem hiding this comment.
Similarly to KV additions and modifications, when a KV entry is deleted, the changed flag should be set to true to ensure proper state versioning, final state evaluation, and accurate introspection metrics.
changed = true;
if (stat) stat.kvChanged++;
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
KV mutations previously did not set the changed flag, so they never bumped dataVersion, skipped isFinalPredicate re-evaluation, and were miscounted as unchanged re-fetches in the tree-sync stats. Set changed on KV add, modify, and delete so versioning, finality, and the duplication metrics are correct. Addresses PR review.
Comment out the throw on unresolved stop-marker seeds after the follow-up round while the deeper-chain case is under investigation. Keep the traverseStopRules propagation and leave re-enable instructions inline.
…hains Propagating traverseStopRules to the follow-up bounded the retry but left stop-marker chains deeper than one level unresolved: a loaded resource referenced a deeper stop-marked resource that was never fetched, so updateFromResourceData threw "orphan resource" on open. Loop the follow-up (each round with the same stop rules, deduping fetched ids) until no stop-marker seeds remain, so everything referenced is loaded. Replaces the disabled one-shot assertion.
Reapplying traverseStopRules on the follow-up re-flagged the very resources being retried as stop markers (the rule is finality-based and they are server-final), so their state never loaded and resources referencing them threw "orphan resource" on open. Fetch follow-up seeds plainly; keep the loop only to resolve any residual stop markers. Update the changeset accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tree-sync instrumentation and a stop-marker follow-up fix, backing the MILAB-6303 investigation (Desktop App on slow / distant networks). Spec: milaboratory/text#190.
What this adds
Full introspection of the tree-sync load via
MI_LOG_TREE_STAT, computed by reusing the state machine's existingchangedflag (negligible per-resource cost):This measured that 52-76% of every polling cycle re-fetches unchanged resources (trees changed by only 2-9 resources over 3 min) on a heavy project.
Fix
The
resourceTreestop-marker follow-up now propagatestraverseStopRules, so the retry stops at the same boundaries instead of traversing the stop-marked subtrees unbounded. It also asserts that one follow-up round resolves every stop marker (throws if a deeper stop-marker chain remains, which would mean the retry must become a loop).Status
Draft. Type-check clean; pl-tree unit tests pass (the one skipped failure needs a live
PL_ADDRESS). No end-to-end validation of the stop-rule change under a deep tree yet; the assertion is the tripwire for that.