Skip to content

fix(table): dedupe transaction requirements by semantics#1328

Open
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix/transaction-requirement-semantic-dedupe
Open

fix(table): dedupe transaction requirements by semantics#1328
fallintoplace wants to merge 2 commits into
apache:mainfrom
fallintoplace:fix/transaction-requirement-semantic-dedupe

Conversation

@fallintoplace

Copy link
Copy Markdown
Contributor

Summary

  • dedupe transaction requirements by their full semantic payload instead of GetType() alone
  • preserve distinct requirements of the same type, including multiple AssertRefSnapshotID checks for different refs
  • add focused transaction regressions for distinct same-type requirements and repeated identical requirements

Why

Transaction.apply currently dedupes requirements by GetType() only. That drops semantically distinct requirements that share a type, especially AssertRefSnapshotID checks for different refs. Operations such as expiring snapshots can build multiple ref assertions, and collapsing them weakens optimistic concurrency control by letting commits proceed without validating every referenced branch or tag.

Using the full requirement payload as the dedupe key keeps equivalent duplicates collapsed while preserving distinct assertions.

Testing

  • go test ./table -run 'TestTransactionApply(KeepsDistinctRequirementsOfSameType|DedupesEquivalentRequirementsWithinAndAcrossCalls)$' -count=1
  • go test ./table -count=1
  • go test ./... -run '^$' -count=1
  • go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m ./table/...
  • git diff --check

@fallintoplace fallintoplace requested a review from zeroshade as a code owner June 27, 2026 10:17

@tanmayrauth tanmayrauth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR, confirmed this is a real concurrency fix, not cleanup, the old GetType() key dropped every ref assertion after the first across apply calls. Two small non-blocking things inline.

Comment thread table/transaction.go
}

func requirementSemanticKey(r Requirement) (string, error) {
data, err := json.Marshal(r)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The dedupe key relies on encoding/json being deterministic. That holds for all current requirement types, fields serialize in declaration order and none contain maps — but a future requirement that adds a map field would silently break dedupe (Go sorts map keys, so it'd still be stable... actually that's fine — the real risk is any field whose marshaling isn't canonical). Could you add a one-line doc comment on requirementSemanticKey noting it assumes canonical/deterministic marshaling, so the constraint is visible to whoever adds the next Requirement type?

Comment thread table/transaction_internal_test.go Outdated
return txn
}

func requireContainsRequirement(t *testing.T, requirements []Requirement, expected Requirement) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

requireContainsRequirement asserts membership via requirementSemanticKey — the same function the production change uses. If that function had a bug, both sides would agree and the test would still pass.
Comparing on the concrete fields (ref + snapshot id) instead would make the assertion independent of the code under test.

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.

Thank you, I push the fix here. e35a1e1

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.

2 participants