Skip to content

feat(counter): implement reSubmitSquashed hook (identity transform)#27404

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

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

Conversation

@anthony-murphy
Copy link
Copy Markdown
Contributor

Description

Implements SharedObjectCore.reSubmitSquashed on SharedCounter as the identity transform — each pending increment is resubmitted unchanged via reSubmitCore.

Why

Counter increments are commutative and additive (a later +3 never cancels an earlier +5), so the squash on resubmit is the identity transform. 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.

Notes

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 (10 lines, 1 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-counter branch May 27, 2026 22:06
@anthony-murphy
Copy link
Copy Markdown
Contributor Author

Deep Review

Reviewed commit b592c62 on 2026-05-27.

Readiness: 8/10 — ALMOST READY

The 10-line reSubmitSquashed identity-transform override on SharedCounter mirrors the established SharedSegmentSequence pattern and is correct. Two Tier 3 polish items remain — both surgical and relevant when this work is revived as part of the actual squash work. Acknowledged the PR is closed in draft per the close comment.

Path to Ready

  • Resolve inline threads
  • When revived, add ChumpChief (area owner, last approver on counter.ts) and scottn12 (author of the surrounding pendingOps / messageId / rollback machinery) as reviewers

Context for Reviewers

  • The identity-transform shape matches SharedSegmentSequence (packages/dds/sequence/src/sequence.ts:796-798); the base SharedObject.reSubmitSquashed (packages/dds/shared-object-base/src/sharedObject.ts:490-502) only forwards to reSubmitCore while Fluid.SharedObject.AllowStagingModeWithoutSquashing=true and throws otherwise — so an explicit opt-in is what protects this DDS when the flag flips.
  • The forwarded localOpMetadata is load-bearing: ChumpChief's design comment on PR feat(counter): Add rollback support for SharedCounter #25771 (implemented by scottn12) established the unique-metadata discipline that lets repeated identical increments (+1, +1) be distinguished in the pending-op queue across squash/resubmit.
  • Author has previously shaped this pipeline (PR Align apply stashed ops with re-submit #19518 "Align apply stashed ops with re-submit", merged 2024-02-09), so low risk of misunderstanding the ContainerRuntime → DataStoreRuntime → IChannel layering.
For human reviewer
  • Needs human judgment — Doc-shape convention for new SharedObject hooks repo-wide: SharedString/Sequence's reSubmitSquashed omits {@inheritDoc} and @sealed, while the adjacent rollback in SharedCounter (added in PR feat(counter): Add rollback support for SharedCounter #25771) includes both. Which convention should new hooks follow is a taste/standardization call best made by an area owner.
  • Needs human judgment — Scope: author's close comment frames this as the squash hook itself rather than prep. Whether the identity transform belongs in this prep PR or in the actual squash work PR is a sequencing decision for the area owner when revived.

*/
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 test exercises the path this override exists to protect. The new method only matters when Fluid.SharedObject.AllowStagingModeWithoutSquashing=false — in that branch the base SharedObject.reSubmitSquashed (packages/dds/shared-object-base/src/sharedObject.ts:490-502) calls throwUnsupported("reSubmitSquashed"). The PR adds 10 lines, 0 deletions, touches zero test files. Existing packages/dds/counter/src/test/counter.spec.ts:244-306 covers reconnection resubmit and counter.fuzz.spec.ts:20-31 covers rebase fuzzing, but packages/dds/counter/src/test contains no AllowStagingModeWithoutSquashing reference, and packages/test/local-server-tests/src/test/stagingMode.spec.ts:301-303 pins the flag to true.

Precedent: PR #25771 added the rollback hook with UT + fuzz + stress coverage. The urgency here is lower (this is a one-line identity delegation vs. novel arithmetic), which is why this is polish rather than blocking — but the contract it asserts is exactly what motivates the PR, so pinning it pays off when the upcoming flag flip lands.

Suggested test in packages/dds/counter/src/test/counter.spec.ts:

  1. Set Fluid.SharedObject.AllowStagingModeWithoutSquashing=false.
  2. Queue multiple pending increments, including a negative.
  3. Trigger a squashed resubmit.
  4. Assert every increment is resubmitted unchanged.

Alternative: wire SharedCounter into packages/test/local-server-tests/src/test/stagingMode.spec.ts with the flag flipped off. Either pins the contract so a future refactor that drops override or changes the base-class default fails CI.

* intent to add that amount to the counter. No increment is "subsumed" by a later one (a later
* `+3` doesn't cancel an earlier `+5`), so the squash on resubmit is the identity transform:
* each pending increment is resubmitted unchanged.
*/
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 doc comment diverges from the convention established for adjacent overrides in this same class. applyStashedOp (counter.ts:197-199) uses {@inheritdoc @fluidframework/shared-object-base#SharedObjectCore.applyStashedOp}, and rollback (counter.ts:220-223, added in PR #25771, approved by ChumpChief) uses {@inheritDoc @fluidframework/shared-object-base#SharedObject.rollback} plus @sealed. The new reSubmitSquashed carries only the free-form rationale paragraph — no {@inheritDoc}, no @sealed.

SharedString/Sequence's equivalent override (packages/dds/sequence/src/sequence.ts:796-798) also omits both tags, so this is not a repo-wide convention — but it is the local convention for this class set by the most recent precedent on this exact file.

Keep the rationale paragraph (it documents the Counter-specific commutative/additive justification that an inherit-doc alone wouldn't carry). Add {@inheritDoc @fluidframework/shared-object-base#SharedObject.reSubmitSquashed} and @sealed so the override matches the adjacent rollback.

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