dds-map: subdir identity + keyset lifetime + ack metadata identity + side-maps + keysets list#27413
dds-map: subdir identity + keyset lifetime + ack metadata identity + side-maps + keysets list#27413anthony-murphy wants to merge 8 commits into
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (641 lines, 4 files), I've queued these reviewers:
How this works
|
anthony-murphy
changed the title
| 0xc72 /* Unexpected pending data for deleteSubDirectory op */, | ||
| ); | ||
| this.pendingSubDirectoryData.splice(pendingEntryIndex, 1); | ||
| this.removePendingSubDirectoryEntry(pendingEntry, pendingNode); |
There was a problem hiding this comment.
Deep Review: Subdirectory rollback ignores localOpMetadata.pendingEntry and still removes by name+type.
This PR plumbs pendingEntry through ICreateSubDirLocalOpMetadata / IDeleteSubDirLocalOpMetadata specifically to disambiguate same-name lifecycle ops, and resubmitSubDirectoryMessage now uses it — pendingSubDirectoryData.includes(localOpMetadata.pendingEntry) plus resubmission of that exact entry. Rollback here calls findLastPendingSubDirectoryNode(subdirName, "createSubDirectory" | "deleteSubDirectory") and then removePendingSubDirectoryEntry(pendingEntry, pendingNode) without ever consulting localOpMetadata.pendingEntry.
On a pending delete("x") → create("x") → delete("x") sequence, rolling back the first delete removes the last delete entry instead of the metadata's own entry — the exact same-name ambiguity the resubmit-side fix was authored to prevent, and the failure mode Abe27342 flagged on #15493 (2023-05-26, inline on directory.ts:1116). The asymmetry is logically indefensible: both rollback and resubmit must use identity, or neither does.
Fix: in both create-rollback and delete-rollback branches, locate and remove localOpMetadata.pendingEntry directly. Mirror what resubmitSubDirectoryMessage already does. At minimum, add assert(pendingNode.data === localOpMetadata.pendingEntry) before removePendingSubDirectoryEntry so the asymmetry fails loud rather than silently corrupting the pending queue. Storing the owning ListNode on pendingEntry at push time makes removal O(1) without an indexOf scan.
…k regressions Add two regressions to lock in the prep-map fixes: - Reconnection: delete -> create -> delete the same SharedDirectory subdir while disconnected, then reconnect. Guards the reference-identity resubmit path against the 0xc31 assert. - Rollback: set -> delete -> set the same SharedMap key, then rollback. Exercises the new PendingKeySet.lifetime back-pointer and the per-type rollback branches.
Deep ReviewReviewed commit Readiness: 3/10 — GETTING STARTED Not ready for sign-off. The structural refactor — identity back-pointers on subdir Path to Ready
Context for Reviewers
For human reviewer
Review history (3 prior reviews)
|
1 participant
Description
Bundles five structural changes in
packages/dds/map/:pendingEntryback-pointers onICreateSubDirLocalOpMetadata/IDeleteSubDirLocalOpMetadata, threads them through the submit helpers, and rewritesresubmitSubDirectoryMessageto use the back-pointer instead offindLast(name && type).PendingKeySet.lifetimeback-pointer — adds areadonly lifetime: PendingKeyLifetimeback-pointer onPendingKeySetin bothmapKernel.tsanddirectory.ts, populates it at construction sites, and converts rollback consumers to use the back-pointer for O(1) parent lookup instead offindLastIndexoverpendingData.pendingData.findIndex(... entry.key === op.key)at the 4 local-op-ack sites with reference-identity (delete:localOpMetadataIS thePendingKeyDelete; set: uses thelifetimeback-pointer from Address re-entrancy when playing ops (#3242) #2 directly).pendingSubDirectoryByNameside-map — addsMap<string, DoublyLinkedList<PendingSubDirectoryEntry>>next topendingSubDirectoryData, maintained symmetrically at every mutation. Converts 10+ scan sites (findIndex/findLast/someforsubdirName === name) to O(1) / O(per-name-length). New private helpers onSubDirectory:pushPendingSubDirectoryEntry,findLastPendingSubDirectoryNode,removePendingSubDirectoryEntry.PendingKeyLifetime.keySets: DoublyLinkedList<PendingKeySet>— replaces the array with a doubly-linked list in both files. ACKshift()is O(1) instead ofArray.shift()'s O(k); rollbackpop()similarly; last-element reads uselist.last!.data.Why
0xc31assert on same-name lifecycle cycles (delete → create → delete with the same subdirectory name), wherefindLast(name && type)could pair the resubmit with the wrong pending entry. Also removes two O(N) scans per resubmit.PendingKeyLifetimeand itsPendingKeySets explicit, eliminatesfindLastIndexscans during rollback, and lets the local-ack lookups (Sample: type-race switch to use webpack-dev-server from prague-dev-server #3) be O(1) instead of linear key scans.subdirectories()was O(n²) today (n entries ×findLastper name). The side-map collapses those scans without changing observable behavior or insertion-order semantics. The flatpendingSubDirectoryDataarray is preserved to minimize blast radius; only the lookup-scans (the hot paths) become O(1).Array.shift()is O(k) per ack on a lifetime with k queued keySets — quadratic when draining a hot key written k times before any ack.DoublyLinkedList.shift()is O(1).Notes
Pure structural prep — no
reSubmitSquashed, no squash logic, no tests touched, no api-report changes.