Skip to content

feat(ordered-collection): implement reSubmitSquashed hook (identity transform)#27407

Closed
anthony-murphy wants to merge 1 commit into
microsoft:mainfrom
anthony-murphy:prep-ordered-collection
Closed

feat(ordered-collection): implement reSubmitSquashed hook (identity transform)#27407
anthony-murphy wants to merge 1 commit into
microsoft:mainfrom
anthony-murphy:prep-ordered-collection

Conversation

@anthony-murphy
Copy link
Copy Markdown
Contributor

Description

Implements SharedObjectCore.reSubmitSquashed on ConsensusOrderedCollection as the identity transform — each pending op is replayed in submit order via reSubmitCore.

Why

ConsensusOrderedCollection ops (add / acquire / complete / release) participate in a server-ordered queue with externally observable positions. Collapsing pending ops would change the queue's observable state, so identity is the right squash.

The base reSubmitSquashed falls back to reSubmitCore only while the Fluid.SharedObject.AllowStagingModeWithoutSquashing flag is true; otherwise it throws. Explicitly opting in protects this DDS from breaking when the flag is flipped.

A known limitation is noted in the implementation comment: a staging-mode sequence such as add(secret) -> acquire -> complete still transmits the add op on commit, because identity squash replays it in order. Callers that need leak-free staging for queue values should hold the add locally and only call it after committing the staging session.

Notes

Updates ordered-collection.legacy.beta.api.md to reflect the new protected member. These api-report files are normally generated; if CI's api-extractor disagrees with the change, regenerate via build:api-reports and push a fixup.

Prep for upcoming DDS-wide staging-mode squash work.

@github-actions
Copy link
Copy Markdown
Contributor

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (18 lines, 2 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

How this works

  • Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

  • Tick Start review below to dispatch the review fleet.

  • After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

  • This comment updates as new commits land; your reviewer selections are preserved.

  • Start review

@anthony-murphy
Copy link
Copy Markdown
Contributor Author

Closing: this implements
eSubmitSquashed (the squash hook) rather than prep. Will be revisited as part of the actual squash work later.

@anthony-murphy anthony-murphy deleted the prep-ordered-collection branch May 27, 2026 22:06
@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:
  288859 links
    1925 destination URLs
    2175 URLs ignored
       0 warnings
       0 errors


@anthony-murphy
Copy link
Copy Markdown
Contributor Author

Deep Review

Reviewed commit 2666984 on 2026-05-27.

Readiness: 8/10 — ALMOST READY

The reSubmitSquashed identity override is correct and aligns with the queue's externally-observable position semantics. PR is closed as draft, so these notes are recorded for the eventual replacement PR. Two polish items: no regression test pins the strict-flag path this override exists to enable, and the documented "Known leak" caveat has no linked tracking artifact.

Path to Ready

  • Resolve inline threads
  • Add an AB# or GitHub-issue link in the TSDoc (or PR description) anchoring the "Known leak" caveat and the broader DDS-wide staging-mode squash work this preps

Context for Reviewers

For human reviewer
  • Needs human judgment — Owner sign-off on the documented "Known leak" being an acceptable shipped limitation, or whether the staging-mode contract for consensus DDSes needs a written design doc before the broader DDS-wide squash work lands. vladsud / CraigMacomber appropriate.
  • Cannot be assessed by the pipeline — Cross-fork / runtime confirmation that identity replay preserves queue state under AllowStagingModeWithoutSquashing=false across ConsensusOrderedCollection / ConsensusQueue / ConsensusStack, including that the rollback-compatible localOpMetadata path still resolves pending-op promises. scottn12 best positioned.
  • Cannot be assessed by the pipeline — Whether CI's api-extractor agrees with the hand-edited ordered-collection.legacy.beta.api.md; only a non-draft CI run can confirm.

*/
protected override reSubmitSquashed(content: unknown, localOpMetadata: unknown): void {
this.reSubmitCore(content, localOpMetadata);
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deep Review: No regression test guards the strict-flag squashed-resubmit path this override exists to enable.

The PR description states the override exists because the base reSubmitSquashed "otherwise throws. Explicitly opting in protects this DDS from breaking when the flag is flipped" — i.e. the guarantee is that the strict AllowStagingModeWithoutSquashing=false path no longer throws for this DDS. No file under packages/dds/ordered-collection/src/test/ is touched in this PR. The ordered-collection fuzz defaults at fuzzUtils.ts:53-61 do not set testSquashResubmit, and ddsFuzzHarness.ts:688-690 shows that path only runs when explicitly enabled — so existing fuzz coverage doesn't exercise the strict-flag scenario either.

Without a targeted test, a future refactor of reSubmitCore or of the base SharedObject.reSubmitSquashed contract can silently re-break the strict-flag scenario for ConsensusOrderedCollection / ConsensusQueue / ConsensusStack without CI noticing — defeating the PR's stated purpose.

Suggested coverage in the eventual replacement PR:

  • Add a focused unit spec in packages/dds/ordered-collection/src/test/consensusOrderedCollection.spec.ts that drives a mock runtime with allowStagingModeWithoutSquashing: false, stages an add/acquire/complete/release mix, triggers squashed resubmit, and asserts (a) no throw and (b) equivalent final queue + jobTracking state.
  • Optionally enable testSquashResubmit: true in fuzzUtils.ts for ongoing fuzz coverage.

* `add(secret) -> acquire -> complete` (or `add(secret) -> acquire -> release`) still
* transmits the `add` op on commit because identity squash replays it in order. Callers
* that need leak-free staging behavior for queue values should hold the `add` locally
* and only call `add` after committing the staging session.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Deep Review: The documented "Known leak" caveat ships without a tracking artifact.

The new TSDoc explicitly classifies the behavior as a leak: add(secret) -> acquire -> complete still transmits the add op on commit because identity squash replays it in order. The PR description also notes "Prep for upcoming DDS-wide staging-mode squash work" without a linked tracking item. The closest precedent on the same class — PR #25747, which added rollback under the same "consensus DDS = state only changes on ack" framing — anchored to [AB#47480].

Without a tracked handle, the caller-side workaround ("hold the add locally and only call add after committing the staging session") becomes tribal knowledge, and the documented data-confidentiality caveat is not auditable from the issue tracker.

Add an AB# or GitHub-issue link in the TSDoc (or PR description) covering both the leak-on-commit limitation and the broader DDS-wide staging-mode squash work this prep enables. Existing ordered-collection TODOs (e.g. consensusOrderedCollection.ts:110, consensusOrderedCollectionFactory.ts:80) use GitHub-issue style — either format works.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant