Skip to content

feat(map): add opt-in sort-key iteration to SharedDirectory#27145

Draft
tylerbutler wants to merge 9 commits intomicrosoft:mainfrom
tylerbutler:directory-iteration-order
Draft

feat(map): add opt-in sort-key iteration to SharedDirectory#27145
tylerbutler wants to merge 9 commits intomicrosoft:mainfrom
tylerbutler:directory-iteration-order

Conversation

@tylerbutler
Copy link
Copy Markdown
Member

Summary

Adds a replicated, opt-in custom iteration order to SharedDirectory driven by string "sort keys" attached to individual keys and child subdirectories. The existing default iteration order is unchanged.

What's new

API additions on IDirectory / ISharedDirectory:

  • setSortKey(key, sortKey | undefined) / setSubDirectorySortKey(name, sortKey | undefined) — set or clear (via undefined) a sort key.
  • keysByOrder() / valuesByOrder() / entriesByOrder() — iterate entries in sort-key order.
  • subdirectoriesByOrder() — iterate child subdirectories in sort-key order.
  • New events sortKeyChanged / subDirectorySortKeyChanged (path-carrying) on ISharedDirectoryEvents; containedSortKeyChanged / containedSubDirectorySortKeyChanged on IDirectoryEvents.

Semantics

Sort-keyed entries iterate first, in lexicographic order of the sort key (JS <, UTF-16 code points). Entries without a sort key follow, in the existing iteration order. Ties on sort key break by the default iteration order — so ordering is deterministic across clients.

Sort keys live alongside the entry they annotate: delete(k) clears the entry's sort key, clear() clears all sort keys in the directory, and deleteSubDirectory(name) clears the child's sort key on the parent.

Replication and back-compat

  • Two new ops (setSortKey, setSubDirectorySortKey) are wired through the existing pending/sequenced model with reference-identity ack matching. Rollback, reconnect-resubmit, and applyStashedOp paths are extended symmetrically.
  • Snapshot gains optional additive fields sortKeys? / subdirectorySortKeys? on IDirectoryDataObject. The directory snapshot format stays at 0.1 — older readers silently ignore the new fields.
  • Adding methods to IDirectory is a forward-compat break; declared in package.json typeValidation.broken. Back-compat is preserved.

Docs and plan

  • Full plan and test specification in packages/dds/map/SORT-KEYS-PLAN.md (includes implementation status, deviations from the original design, and a resume-in-fresh-session guide).
  • Internal architecture notes in packages/dds/map/ARCHITECTURE.md §9.4 + §11.

Status

47 of 67 planned tests landed in packages/dds/map/src/test/mocha/directory.sortKey.spec.ts. 20 follow-on tests (deferred rollback / snapshot round-trip / fuzz, plus a few edge-case spec entries) are enumerated at the top of SORT-KEYS-PLAN.md for a follow-up PR; all their implementation paths are covered by the landed code.

Draft — API Council review required (surfaces @public @legacy and @beta @legacy types).

tylerbutler and others added 7 commits April 22, 2026 14:49
Adds an opt-in `detect_changes` job to the client build pipeline that
computes the merge-base vs the PR's target branch and emits a pnpm
`--filter "...[<sha>]"` expression as an output variable. Downstream
test jobs consume it via `npm_config_filter`, so pnpm natively scopes
recursive runs to packages changed in the PR (plus their transitive
dependents) — no wrapper scripts, no package.json changes.

Activates when one of:
  * `enableChangedPackageTestScoping` pipeline parameter is true, OR
  * the PR source branch name contains `test/filtered-ci/` — a
    branch-name convention that lets contributors exercise the feature
    from an auto-triggered PR build without flipping parameters.

When the detect job is skipped (non-PR builds, or PR builds that
haven't opted in) its output variables resolve to empty strings, which
downstream jobs interpret as "no filter — run every package" — so
pipeline behavior is effectively byte-identical to today for non-opt-in
runs.

The merge-base SHA is passed to pnpm's selector instead of a branch ref
because pnpm's `[ref]` uses a two-dot diff (pnpm/pnpm#9907), which
would pick up unrelated commits from the target branch as "changed".
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
Addresses review feedback on the scoping PR:

- Skip Coverage_tests and Test_* jobs entirely (not just their test steps)
  when detect_changes reports shouldRunTests=false. Empty (detect_changes
  skipped) still runs, preserving the historical non-opt-in behavior.
  Saves agent allocation + checkout + install for no-package-change PRs.
- Remove the now-redundant step-level Start Test condition and orphaned
  `shouldRunTests` job variable; `scopedPnpmFilter` is still exposed as a
  job variable because test-task steps consume it.
- Shallow-clone the detect_changes checkout (fetchDepth: 200) with a
  runtime unshallow fallback guarded by .git/shallow presence, so full
  clones don't error on --unshallow.
- Cross-reference `pr: paths: include:` in build-client.yml from
  FULL_RUN_PATTERNS to flag the convention-based overlap.
- Condense duplicated "historical behavior" narration across the diff.
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
PR microsoft#27134 revealed that `succeeded()` on `Coverage_tests` and `Test_*`
false-skips the job when `detect_changes` is Skipped, contrary to the
documented "skipped deps count as successful" behavior. Check
`dependencies.build.result` directly so the test jobs run for non-opt-in
PRs (where `detect_changes` is intentionally skipped) while still
respecting a 'false' output signal when scoping is active.
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
The parameter only takes effect on manually-queued builds, where an operator
can tick it in the ADO UI. Auto-triggered PR builds always run with the
`false` default, so the only practical opt-in is the 'test/filtered-ci/'
branch-name substring. Removing the parameter eliminates a lever that
can't be pulled from a developer's normal workflow and simplifies the
detect_changes activation condition.
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
…tory

Onboarding reference for contributors: covers the pending-state / lifetime
model shared by both DDSes, per-DDS internals (kernel split, snapshot
formats, tree topology, dispose lifecycle, ordering), wire-op shapes, event
suppression rules, and known subtleties. The README stays user-facing; this
is the doc you want when you're editing the code.
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
Adds a replicated, opt-in custom iteration order to SharedDirectory
driven by string "sort keys" attached to individual keys and child
subdirectories. The default iteration order is unchanged.

New API on IDirectory / ISharedDirectory:

- setSortKey(key, sortKey | undefined) / setSubDirectorySortKey
- keysByOrder / valuesByOrder / entriesByOrder / subdirectoriesByOrder
- sortKeyChanged / subDirectorySortKeyChanged (path-carrying) on
  ISharedDirectoryEvents plus containedSortKeyChanged /
  containedSubDirectorySortKeyChanged on IDirectoryEvents

Ordering semantics: sort-keyed entries first in lexicographic (JS <)
order of their sort keys, ties broken by the default iteration order;
unkeyed entries follow in default order. Sort keys live alongside
their entry's identity — delete(k), clear(), and deleteSubDirectory
clear associated sort-key state.

Replication: two new op types (setSortKey, setSubDirectorySortKey)
wired through the existing pending/sequenced model with reference-
identity ack matching. Rollback, reconnect-resubmit, and
applyStashedOp paths are extended symmetrically.

Snapshot: optional additive fields sortKeys / subdirectorySortKeys on
IDirectoryDataObject. The directory snapshot format stays at 0.1;
older readers silently ignore the new fields.

Plan and test specification at packages/dds/map/SORT-KEYS-PLAN.md.
Adding methods to IDirectory is a forward-compat break; declared in
typeValidation.broken.
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
@github-actions
Copy link
Copy Markdown
Contributor

Hey! You look nice today! Want me to review this PR?

Based on the diff (4007 lines, 15 files), I've queued these reviewers:

  • Correctness — logic errors, race conditions, lifecycle issues
  • Security — vulnerabilities, secret exposure, injection
  • API Compatibility — breaking changes, release tags, type design
  • Performance — algorithmic regressions, memory leaks
  • Testing — coverage gaps, hollow tests

Toggle checkboxes to adjust, then reply yes to start — or ask me anything!

@tylerbutler
Copy link
Copy Markdown
Member Author

yes

tylerbutler and others added 2 commits April 23, 2026 16:15
Adds six mocha tests exercising edge cases of the SharedDirectory sort-key
API that were deferred from the initial feature commit:

- T33: rollback of a pending delete restores the key and its sort key.
- T34: a local clear while a setSortKey is pending leaves no leftover
  sequenced sort key after both ops ack.
- T42: deleting one subdirectory leaves sibling subdirectorySortKeys
  entries intact.
- T45a/T45b: concurrent delete + setSortKey on the same key from two
  clients converges; split into two cases covering both sequencing
  orders, with T45a documenting that a delete-first race leaves the
  remote setSortKey behaving as pre-registration for a future rebirth
  of the key (same semantics as T46).
- T56: a pending setSortKey whose subdirectory was remotely deleted
  during a disconnect is dropped silently by the resubmit path when
  the client reconnects.

Also updates SORT-KEYS-PLAN.md to mark these items done in the status
header, adjust the test-coverage table, and document the T45
pre-registration clarification.

Full @fluidframework/map mocha suite: 973 passing (was 967).
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
Covers rollback of setSortKey (T50-T52), detached no-op (T53), and
setSubDirectorySortKey mirrors (T54a/b/c) in directory.rollback.spec.ts.
T52/T54c exercise pending-entry splice-by-reference-identity via
flushSomeMessages(1) + processOneMessage before rollback.
🤖 Generated with [Nori](https://noriagentic.com)

Co-Authored-By: Nori <contact@tilework.tech>
@github-actions
Copy link
Copy Markdown
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  287071 links
    1898 destination URLs
    2148 URLs ignored
       0 warnings
       0 errors


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant