feat(pl-middle-layer): use server-side LoadSubtree for tree refreshes when available#1583
feat(pl-middle-layer): use server-side LoadSubtree for tree refreshes when available#1583PoslavskySV wants to merge 1 commit into
Conversation
… when available
Replaces the client-driven BFS tree loader with a single RPC to the
new ResourceAPI.LoadSubtree when the backend advertises the
"loadSubtree:v1" capability, falling back transparently to the
existing loader otherwise.
The old loader made one round-trip per graph layer, dominating
project-open and post-mutation refresh time on high-latency
connections. The new path collapses the walk to one RTT regardless
of depth.
Changes:
- pl-client: PlClient.hasServerCapability helper reading
MaintenanceAPI.Ping.Response.capabilities; PlTransaction.loadSubtree
method; declarative PruningRule/PruningSpec/LoadSubtreeOptions/
LoadedSubtreeNode types; tx stat fields for the new call. Minimal
proto regen - only our additions in the generated api.ts (no drift
on unrelated messages).
- pl-tree: loadTreeStateServerSide as a peer to loadTreeState;
pruningSpec field on TreeLoadingRequest and SynchronizedTreeOps;
SynchronizedTreeState.refresh picks the server-side path when a
spec is supplied and the capability is present.
- pl-middle-layer: projectTreePruningSpec and
ProjectsListTreePruningSpec added alongside the existing
PruningFunctions, kept in lock-step so both old and new backends
see the same pruning behavior.
Wire protocol unchanged on old backends. No desktop-app changes
required; the new loader is enabled automatically when it connects
to a backend advertising the capability.
Depends on the pl repo change adding ResourceAPI.LoadSubtree.
🦋 Changeset detectedLatest commit: 91ad225 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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}
There was a problem hiding this comment.
Code Review
This pull request introduces server-side tree synchronization via the ResourceAPI.LoadSubtree RPC, which optimizes project opening and refreshing by collapsing multi-round-trip client-driven BFS walks into a single request when the backend supports the loadSubtree:v1 capability. The implementation includes new protobuf definitions, client-side capability detection, declarative pruning rules, and updated transaction statistics. Review feedback suggests that knownFinals signatures should be preserved rather than hardcoded to empty to avoid redundant data transfers, and that the loadSubtreeBytes statistic should include field value lengths for more accurate data tracking.
| const knownFinals = opts.knownFinals | ||
| ? [...opts.knownFinals].map((rid) => ({ | ||
| resourceId: toResourceId(rid), | ||
| resourceSignature: new Uint8Array(0), | ||
| })) | ||
| : []; |
There was a problem hiding this comment.
The knownFinals signatures are currently hardcoded to empty Uint8Array. According to the protobuf definition and comments for LoadSubtree.Request, each entry in known_finals must carry a signature obtained from a prior response to allow the server to verify and skip it. By sending empty signatures, the server may be forced to re-send data for resources the client already holds, which would significantly reduce the performance benefits of this optimization on high-latency connections. If opts.knownFinals contains ResourceData objects, their signatures should be preserved and sent to the server.
| for (const n of result) { | ||
| this._stat.loadSubtreeBytes += n.resource.data?.length ?? 0; | ||
| this._stat.loadSubtreeFields += n.resource.fields.length; | ||
| for (const kv of n.kv) { | ||
| this._stat.loadSubtreeBytes += kv.key.length + kv.value.length; | ||
| } | ||
| } |
There was a problem hiding this comment.
The loadSubtreeBytes statistic is currently undercounting the actual data transferred because it does not include the size of the field values. In a typical resource tree, the bulk of the data resides within the fields. To maintain consistency with other byte-tracking statistics in TxStat (like kvGetBytes), the lengths of all field values should be added to the total.
for (const n of result) {
this._stat.loadSubtreeBytes += n.resource.data?.length ?? 0;
this._stat.loadSubtreeFields += n.resource.fields.length;
for (const f of n.resource.fields) {
this._stat.loadSubtreeBytes += f.value.length;
}
for (const kv of n.kv) {
this._stat.loadSubtreeBytes += kv.key.length + kv.value.length;
}
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const knownFinals = opts.knownFinals | ||
| ? [...opts.knownFinals].map((rid) => ({ | ||
| resourceId: toResourceId(rid), | ||
| resourceSignature: new Uint8Array(0), | ||
| })) | ||
| : []; |
There was a problem hiding this comment.
resourceSignature in knownFinals may silently defeat the optimization
All knownFinals entries are sent with resourceSignature: new Uint8Array(0). The proto doc says "Each entry must carry a signature obtained from a prior response." If the backend validates the signature and rejects empty bytes (treating the entry as a cache-miss rather than a known-final), the server will re-fetch and re-descend through every supposedly-skipped resource on every refresh — reducing the server-side path to a full tree walk rather than the incremental walk that justifies the feature. The same empty signature is sent for the root resource itself. Worth cross-checking against the paired backend PR to confirm the server accepts Uint8Array(0) as "no signature / skip check".
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/node/pl-client/src/core/transaction.ts
Line: 847-852
Comment:
**Empty `resourceSignature` in `knownFinals` may silently defeat the optimization**
All `knownFinals` entries are sent with `resourceSignature: new Uint8Array(0)`. The proto doc says "Each entry must carry a signature obtained from a prior response." If the backend validates the signature and rejects empty bytes (treating the entry as a cache-miss rather than a known-final), the server will re-fetch and re-descend through every supposedly-skipped resource on every refresh — reducing the server-side path to a full tree walk rather than the incremental walk that justifies the feature. The same empty signature is sent for the root resource itself. Worth cross-checking against the paired backend PR to confirm the server accepts `Uint8Array(0)` as "no signature / skip check".
How can I resolve this? If you propose a fix, please make it concise.| action: "includeAll", | ||
| }, | ||
| // Catch-all: drop fields on any other resource and stop descending. | ||
| { typeMatch: "prefix", typePattern: "", action: "dropAll" }, |
There was a problem hiding this comment.
{ typeMatch: "prefix", typePattern: "", action: "dropAll" } is intended as a catch-all that drops every non-Projects resource. An empty prefix matches every string, so this works in standard string semantics — but the backend's prefix-matching logic should explicitly handle "" as "match all". If the server treats an empty type_pattern as "no match" or as a no-op, the catch-all silently stops firing and the walk continues unbound into all subtrees. Worth a one-line comment (or a companion integration test) confirming the server treats "" as the "match any type" wildcard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/node/pl-middle-layer/src/middle_layer/project_list.ts
Line: 36
Comment:
**Catch-all rule relies on empty-string prefix behavior being well-defined**
`{ typeMatch: "prefix", typePattern: "", action: "dropAll" }` is intended as a catch-all that drops every non-Projects resource. An empty prefix matches every string, so this works in standard string semantics — but the backend's prefix-matching logic should explicitly handle `""` as "match all". If the server treats an empty `type_pattern` as "no match" or as a no-op, the catch-all silently stops firing and the walk continues unbound into all subtrees. Worth a one-line comment (or a companion integration test) confirming the server treats `""` as the "match any type" wildcard.
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (13.15%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1583 +/- ##
==========================================
- Coverage 55.98% 53.96% -2.03%
==========================================
Files 198 256 +58
Lines 10781 14659 +3878
Branches 2306 3045 +739
==========================================
+ Hits 6036 7911 +1875
- Misses 3986 5735 +1749
- Partials 759 1013 +254 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Problem
Opening a project and applying block mutations (reorder/delete) walk the resource graph client-side: one gRPC round-trip per graph layer. On LAN this is invisible; on high-latency/lossy connections it compounds into multi-second hangs and
networktimeout errors. The bottleneck is architectural — not bandwidth, not payload size — so a browser app loading a heavy page over the same connection is unaffected.See the paired pl PR for the full problem framing.
Change
When the backend advertises the
loadSubtree:v1capability (viaMaintenanceAPI.Ping.Response.capabilities), tree synchronization uses the new server-sideResourceAPI.LoadSubtreeRPC instead of the client-driven BFS. The walk happens locally on the backend against the open read transaction and streams results back in a single RPC regardless of graph depth.Fallback is transparent — when the capability is absent, the loader uses the existing client-driven BFS with no behavior change.
Packages touched
@milaboratories/pl-clientPlClient.hasServerCapability(capability: string): boolean— reads the newcapabilitiesfield from Ping.PlTransaction.loadSubtree(opts: LoadSubtreeOptions): Promise<LoadedSubtreeNode[]>— wire wrapper over the new RPC.PruningRule/PruningSpec/LoadSubtreeOptions/LoadedSubtreeNode— declarative wire types exported publicly so pl-tree and middle-layer can describe pruning without touching proto-generated identifiers.loadSubtreeRequests,loadSubtreeNodes,loadSubtreeFields,loadSubtreeBytes.api.ts. Unrelated proto messages are unchanged.ll_client.tsdefaultscapabilitiesto[]so the REST path tolerates older openapi schemas.@milaboratories/pl-treeloadTreeStateServerSide(tx, request, stats)peer to existingloadTreeState. Same signature, same return type.TreeLoadingRequestgainsrootand optionalpruningSpecfields.SynchronizedTreeOpsgains an optionalpruningSpec.SynchronizedTreeState.refresh()chooses the loader: if a spec is supplied and the client reports the capability, it callsloadTreeStateServerSide; otherwise it falls back to the classic loader.LoadSubtreeCapability = "loadSubtree:v1"constant exported for symmetry with the backend token.@milaboratories/pl-middle-layerprojectTreePruningSpec(inproject.ts) — declarative equivalent ofprojectTreePruning, coveringStreamWorkdir/*,BlockPackCustom.template,UserProject.__serviceTemplate*,Blob.ProjectsListTreePruningSpec(inproject_list.ts) — declarative equivalent ofProjectsListTreePruningFunction.SynchronizedTreeState.initcall sites now pass bothpruning(function, for fallback) andpruningSpec(declarative, for server-side path).projectTreePruning; it fires only on the fallback path. Parity-logging on the server-side path is out of scope for this PR.Why both
PruningFunctionandPruningSpec?The two are maintained in lock-step: the function is the authoritative filter on the classic loader, the spec is the authoritative filter on the server-side path. Unit tests in the paired pl PR cover the exact translation, so regressing one and not the other is detectable.
Keeping both avoids a breaking change for any external consumer still wiring a
PruningFunctiondirectly, and lets new/old backends coexist without the middle layer having to branch.Compatibility
Zero-impact on old backends: the switch inside
refresh()keys on the explicit capability, so a backend that doesn't advertise it keeps using the client-driven BFS just like today. No wasted round-trip, noUNIMPLEMENTEDprobes.Also forward-compatible: if a future feature needs similar gating,
hasServerCapabilityis reusable with a different token.Test plan
pnpm types:checkgreen in pl-client, pl-tree.pnpm buildgreen in pl-client, pl-tree, pl-middle-layer, and the desktop app (via pnpm overrides).pnpm fmtapplied to all three packages.curl /v1/pingshowscapabilities: ["loadSubtree:v1"].tc qdiscor equivalent).Changeset
.changeset/load-subtree-rpc.md— minor bumps for pl-client, pl-tree, pl-middle-layer.Depends on
milaboratory/pl#1776 — the backend change adding
ResourceAPI.LoadSubtreeand thecapabilitiesPing field. This PR is safe to merge and ship against older pl releases (fallback kicks in); the fast path only activates once a pl with the new RPC is deployed.Greptile Summary
This PR introduces server-side subtree loading (
ResourceAPI.LoadSubtree) as a capability-gated fast path for tree synchronization, collapsing a multi-round-trip client-driven BFS into a single RPC. The fallback to the existing client-driven path is transparent and the dualpruning/pruningSpecstrategy is well-designed for old/new backend coexistence. Three P2 items worth confirming before or after merging: the server's handling of emptyresourceSignaturebytes inknownFinals, the server's prefix-matching behaviour for an emptytypePattern(catch-all inProjectsListTreePruningSpec), and the missingprunedFields/finalResourcesSkippedstat increments on the server-side code path.Confidence Score: 5/5
Safe to merge; all remaining findings are P2 clarifications that do not block correctness or fallback behavior.
All three comments are P2: empty signatures are a potential optimization gap (not a functional bug), the catch-all prefix is a protocol question rather than a current failure, and missing stat counters are a monitoring gap. The fallback to the client-driven BFS is fully preserved, so the worst-case scenario on a new backend is a slightly less efficient tree walk.
lib/node/pl-client/src/core/transaction.ts(empty signatures in knownFinals) andlib/node/pl-middle-layer/src/middle_layer/project_list.ts(empty-string prefix catch-all) deserve a cross-check against the paired backend PR.Important Files Changed
loadSubtreemethod wiring the new RPC; knownFinals always sent with empty signatures which may silently bypass the skip optimizationhasServerCapabilityhelper that safely readscapabilitiesfrom ping response; handles null/undefined server info correctlycapabilitiesto[]so older openapi schemas don't break the capability checkloadTreeStateServerSideandLoadSubtreeCapability; prunedFields and finalResourcesSkipped stats are not populated on the server-side pathprojectTreePruningSpecin lock-step withprojectTreePruning; all four pruning rules match their function equivalentsProjectsListTreePruningSpec; catch-all rule uses empty-string prefix which needs server confirmation that""matches all type namesComments Outside Diff (1)
lib/node/pl-tree/src/sync.ts, line 1281-1315 (link)stats.prunedFieldsandstats.finalResourcesSkippedsilently stay at 0loadTreeStateServerSidenever updatesstats.prunedFieldsorstats.finalResourcesSkipped. When the server-side path is active, any log or dashboard that watches these counters will always read 0, making it impossible to tell how much pruning or known-final skipping actually happened on the server. The classicloadTreeStateincrements both. This doesn't affect correctness, but monitoring parity between the two paths is already called out in the PR as a future concern for the "excessive field count" warning — the stat gap compounds that blind spot.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(pl-middle-layer): use server-side L..." | Re-trigger Greptile