Skip to content

fix(storage): add first-writer-wins write intent conflict detection for AP transactions - #153

Merged
liuuys merged 7 commits into
TuGraph-family:masterfrom
liuuys:fix-ap-write-write-conflict
May 29, 2026
Merged

fix(storage): add first-writer-wins write intent conflict detection for AP transactions#153
liuuys merged 7 commits into
TuGraph-family:masterfrom
liuuys:fix-ap-write-write-conflict

Conversation

@liuuys

@liuuys liuuys commented May 17, 2026

Copy link
Copy Markdown
Contributor

Prevent concurrent transactions from silently overwriting each other's write intent on the same edge, which caused committed values to be lost when interleaved with an aborting transaction.

  • set_edge_property_in_txn / delete_edge_in_txn: check edge.commit_ts for an external txn_id before overwriting, return WriteWriteConflict
  • commit_at: validate all undo entries before setting self.commit_ts so a failed commit can still be aborted; promote all matching property versions instead of only last_mut(); return explicit errors for missing edge_id_map / block / offset mismatches
  • abort: verify write intent ownership before restoring commit_ts; update edge block min_ts/max_ts on rollback
  • Tests: sequential abort-restore test, sequential write-conflict test with specific error type assertion, deterministic 3-barrier concurrent test that guarantees WriteWriteConflict detection

fix(storage): add first-writer-wins write intent conflict detection for AP transactions

Type

  • feat: (new feature)
  • fix: (bug fix)
  • docs: (doc update)
  • refactor: (refactor code)
  • test: (test code)
  • chore: (other updates)

Scope

  • query: (query engine)
    • parser: (frontend parser)
    • planner: (frontend planner)
    • optimizer: (query optimizer)
    • executor: (execution engine)
    • op: (operators)
  • storage: (storage engine)
    • mvcc: (multi version concurrency control)
    • schema: (graph model and topology)
  • tool: (tools)
    • cli: (cli)
    • sdk: (sdk)
  • none: (N/A)

Description

Issue: #

FAIL [   0.016s] minigu-storage::integration_tests ap::transaction_test::test_concurrent_commit_and_abort_preserve_committed_value
  stdout ───

    running 1 test
    test ap::transaction_test::test_concurrent_commit_and_abort_preserve_committed_value ... FAILED

    failures:

    failures:
        ap::transaction_test::test_concurrent_commit_and_abort_preserve_committed_value

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 103 filtered out; finished in 0.00s
    
  stderr ───

    thread 'ap::transaction_test::test_concurrent_commit_and_abort_preserve_committed_value' (465640) panicked at minigu/storage/tests/ap/transaction_test.rs:1230:5:
    assertion `left == right` failed: Committed value should win over aborted update
      left: Some(Int32(Some(10)))
     right: Some(Int32(Some(1111)))
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

  Cancelling due to test failure: 7 tests still running

Checklist

  • I have prepared the pull request title according to the requirements.
  • I have successfully run all unit tests and integration tests.
  • I have already rebased the latest master branch.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.

…or AP transactions

Prevent concurrent transactions from silently overwriting each other's
write intent on the same edge, which caused committed values to be
lost when interleaved with an aborting transaction.

- set_edge_property_in_txn / delete_edge_in_txn: check edge.commit_ts
  for an external txn_id before overwriting, return WriteWriteConflict
- commit_at: validate all undo entries before setting self.commit_ts
  so a failed commit can still be aborted; promote all matching
  property versions instead of only last_mut(); return explicit errors
  for missing edge_id_map / block / offset mismatches
- abort: verify write intent ownership before restoring commit_ts;
  update edge block min_ts/max_ts on rollback
- Tests: sequential abort-restore test, sequential write-conflict test
  with specific error type assertion, deterministic 3-barrier
  concurrent test that guarantees WriteWriteConflict detection
Copilot AI review requested due to automatic review settings May 17, 2026 08:26

Copilot AI 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.

Pull request overview

This PR strengthens AP MVCC transaction correctness by adding first-writer-wins detection for write intents on edges, and by hardening commit/abort bookkeeping to avoid losing committed updates under interleavings with aborting transactions.

Changes:

  • Add write-write conflict detection in AP edge mutation paths so a second writer can’t overwrite an existing uncommitted write intent.
  • Rework commit_at/abort to validate and enforce write-intent ownership, and to promote/rollback versions more consistently.
  • Add new integration tests covering sequential abort-restore, sequential write-conflict, and a deterministic concurrent barrier scenario.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.

File Description
minigu/storage/src/ap/olap_graph.rs Adds write-intent ownership checks to edge set/delete in txn to detect write-write conflicts.
minigu/storage/src/ap/transaction.rs Validates undo entries before committing; promotes all matching property versions; adds intent-ownership checks during abort and updates block ts metadata.
minigu/storage/tests/ap/transaction_test.rs Adds/updates tests for abort-restore behavior and write-write conflict detection, including a barrier-coordinated concurrent scenario.
.gitignore Ignores local .claude/ files.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread minigu/storage/src/ap/transaction.rs Outdated
Comment thread minigu/storage/src/ap/transaction.rs
Comment thread minigu/storage/src/ap/olap_graph.rs Outdated
Comment thread minigu/storage/src/ap/transaction.rs
Comment thread minigu/storage/tests/ap/transaction_test.rs
liuuys added 6 commits May 17, 2026 16:38
…arly conflict check

- commit_at: use TransactionError::InvalidState for missing edge_id_map /
  block / eid mismatch (internal invariant errors), keep WriteWriteConflict
  only for genuine owner mismatch
- abort: clone undo entries and drop undo_buffer lock before acquiring
  edges/property locks, fixing a lock-order inversion with write paths
  that lock edges first then push_undo
- set_edge_property_in_txn: move write-intent conflict check to right
  after edge lookup, before scanning property columns, so conflicting
  writers fail fast without extra property work
Use SetPropsOp.indices from the undo entry to iterate only the property
columns that were actually touched, instead of scanning all columns for
every SetEdgeProps operation. Also skip min_ts/max_ts update when no
version was promoted (both CreateEdge and SetEdgeProps), preventing
spurious max_ts inflation on untouched columns.
# Conflicts:
#	minigu/storage/tests/ap/transaction_test.rs
@liuuys
liuuys requested a review from WhiteGive-base May 29, 2026 09:15
@liuuys
liuuys merged commit fc2ccf5 into TuGraph-family:master May 29, 2026
46 of 49 checks passed
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.

3 participants