fix: edge indexes become invalid when deleting and recreating same edge (#3097)

[robfrank]

In TransactionIndexContext.addIndexKeyLock(), when a unique index entry is deleted (REMOVE) and recreated (ADD) with the same key within the same transaction on the same bucket-level index, the REMOVE entry was silently replaced by a REPLACE entry — losing the old RID. At commit time the old persisted index entry was never removed, causing stale entries to accumulate. After 2+ iterations, checkUniqueIndexKeys detected >2 entries for the same unique key and threw DuplicatedKeyException.

Fix: store the old RID in the REPLACE entry (IndexKey.oldRid). At commit time, call index.remove(key, oldRid) for REPLACE entries with a non-null oldRid to properly clean up the old persisted entry. Also fix getTxDeletedEntries() to use oldRid as the deleted RID for correctness in checkUniqueIndexKeys. Update TxForwardRequest serialization to include oldRid for HA replication.

[gemini-code-assist]

Summary of Changes

Hello @robfrank, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical bug where unique edge indexes could become invalid when an edge was deleted and then immediately recreated with the same key within the same transaction. Previously, this sequence of operations would lead to stale index entries and eventually "DuplicatedKeyException". The fix introduces a mechanism to correctly track and remove the old Record ID during such "REPLACE" operations, ensuring index integrity and proper cleanup, and extends this correction to high-availability replication.

Highlights

• IndexKey Enhancement: Introduced an "oldRid" field within the "IndexKey" class to correctly track the original Record ID (RID) when an index entry is replaced due to a delete-then-add operation within the same transaction.
• Commit Logic Refinement: Modified the transaction commit process to explicitly remove the "oldRid" from the persisted index when a "REPLACE" operation occurs, preventing stale index entries.
• Transaction Index Context Update: Ensured that the "oldRid" is properly captured and assigned to "REPLACE" operations in "addIndexKeyLock()" when a "REMOVE" operation is superseded by an "ADD" for the same key.
• Deleted Entries Retrieval Correction: Updated "getTxDeletedEntries()" to correctly identify the actual deleted RID by utilizing the "oldRid" for "REPLACE" entries, which is crucial for unique index checks.
• HA Replication Support: Extended the "TxForwardRequest" serialization and deserialization logic to include the new "oldRid" field, ensuring high-availability replication correctly handles these index changes.
• Robust Test Case: Enhanced the "EdgeIndexDuplicateKeyTest" to repeatedly delete and recreate an edge, thoroughly validating that the fix prevents "DuplicatedKeyException" under such scenarios.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• engine/src/main/java/com/arcadedb/database/TransactionIndexContext.java
  • Added "oldRid" field to "IndexKey" to store the RID being replaced.
  • Modified "commit()" to remove the "oldRid" for "REPLACE" operations.
  • Updated "addIndexKeyLock()" to set "oldRid" when a "REMOVE" operation is replaced by an "ADD".
  • Adjusted "getTxDeletedEntries()" to use "oldRid" for "REPLACE" entries to correctly identify deleted RIDs.
• engine/src/test/java/com/arcadedb/graph/EdgeIndexDuplicateKeyTest.java
  • Updated SQL commands to use "sqlscript" for multi-statement execution.
  • Expanded the test case to loop the delete and recreate edge operation multiple times, ensuring the fix is robust.
• server/src/main/java/com/arcadedb/server/ha/message/TxForwardRequest.java
  • Modified "writeIndexKeysToBuffer" to serialize the "oldRid" for "REPLACE" operations.
  • Modified "readIndexKeysFromBuffer" to deserialize the "oldRid" for "REPLACE" operations.

Activity

• No human activity has been recorded on this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request fixes an issue where deleting and recreating an edge with a unique index within the same transaction could lead to stale index entries and DuplicatedKeyException in subsequent operations. The fix introduces an oldRid field in IndexKey to track the RID of a replaced entry, ensuring it's properly removed at commit time. The changes also update HA transaction forwarding to include this new field.

The core logic seems correct, and the accompanying test has been improved to be more robust. I've found one potential issue in TransactionIndexContext where the oldRid is not propagated correctly in all cases, which could still lead to stale entries in more complex transaction scenarios. Please see my detailed comment.

[claude]

Code Review

This PR fixes a real and subtle bug: when an edge (or any record with a unique index) is deleted and recreated within the same transaction, the in-memory index context merges the REMOVE+ADD into a REPLACE, but the old persisted RID was never cleaned up at commit time. After the second such round-trip the stale entry causes a false DuplicatedKeyException. The root cause analysis and fix are sound.

Correctness

getTxDeletedEntries() — minor inconsistency in fallback RID

In the updated getTxDeletedEntries() (line 433), the fallback branch uses entry.getKey().rid instead of entry.getValue().rid. Since the map is built with values.put(v, v) (key and value are the same object), both references are identical at runtime — so there is no functional bug. However, mixing .getKey() and .getValue() within the same expression is misleading. Prefer entry.getValue().rid throughout for consistency.

checkUniqueIndexKeys — REPLACE with oldRid

checkUniqueIndexKeys(Index, IndexKey, RID deleted) uses deleted to skip the uniqueness violation when the found entry equals the deleted one. For a REPLACE that originated from REMOVE→ADD, getTxDeletedEntries() now correctly returns oldRid as the deleted RID, suppressing the false positive. Correct.

addIndexKeyLock — oldRid capture scope

The oldRid is only captured when entry != null && entry.operation == REMOVE (lines 297-299). If the existing entry is already a REPLACE (e.g. three or more calls for the same key within one transaction), the intermediate oldRid is not propagated. This is likely not reachable in practice, but worth a comment noting the assumption.

HA Serialization

The oldRid is serialized only for REPLACE entries in TxForwardRequest, using a presence flag byte. This is clean and consistent with the rest of the serialization style.

Backward compatibility concern: The comment in IndexKeyOperation documents that REPLACE (ordinal=2) was introduced in 25.3.2. The new oldRid flag byte is appended inside the REPLACE branch. If a newer node sends a TxForwardRequest with REPLACE+oldRid to an older follower that does not know to read that flag byte, the follower's deserialization cursor will be misaligned. This is only a risk during a rolling upgrade, and may be acceptable by policy — but it should be explicitly noted (e.g. with a version marker in the compatibility comment or in the changelog/documentation).

Code Style

The oldRid field is public and mutable (non-final), which is consistent with the existing mutable operation field but widens the API surface. This is acceptable given existing precedent.

Test Coverage

Looping 10 times rather than just 2 is a meaningful improvement — it validates the fix across multiple iterations, not just the first recurrence. Using sqlscript for multi-statement setup is cleaner.

One gap: there is no assertion that the edge is still correctly queryable after each recreate cycle. The test only checks that no exception is thrown. Adding a SELECT COUNT(*) FROM trs_duct assertion after the loop would give stronger confidence.

There is also no HA-level test verifying that the oldRid round-trips correctly through TxForwardRequest serialization, though a cluster test setup is understandably out of scope for this fix.

Summary

The fix is correct and well-targeted. Items to consider before merging:

1. (Minor) In getTxDeletedEntries() line 433: change entry.getKey().rid to entry.getValue().rid for readability (functionally equivalent).
2. (Important) Document the backward-compat impact of the new flag byte in TxForwardRequest REPLACE serialization for rolling upgrades.
3. (Nice-to-have) Add an assertion in the test to verify the edge count is 1 after the delete-recreate loop, not just that no exception is thrown.

Overall, this is a solid fix for a tricky transactional index bug. Good work.

[mergify]

🧪 CI Insights

Here's what we observed from your CI run for 585c393.

🟢 All jobs passed!

But CI Insights is watching 👀

[claude]

Code Review

This PR fixes a real and subtle bug in TransactionIndexContext where a REMOVE→ADD sequence on the same unique index key within a single transaction silently merged into a REPLACE, discarding the old RID. The approach is sound. Below are observations ranging from correctness concerns to minor style points.

---

Correctness / Potential Issues

1. oldRid field is mutable and not final — breaks the immutable design of IndexKey

IndexKey has all other fields declared final. The new oldRid is a mutable public field, which is inconsistent and risky:

// Current:
public       RID               oldRid; // for REPLACE created from same-bucket REMOVE→ADD: the old RID being replaced
public       IndexKeyOperation operation;

operation is also mutable (already pre-existing), but oldRid is a new field with no compelling reason to remain non-final after construction. Consider initialising both in a second constructor overload, or at least document clearly why it must remain mutable.

---

2. getTxDeletedEntries() — the if condition change may silently drop REPLACE-based deletions

Before the fix, only REMOVE or null-existent entries were added. Now REPLACE entries can also appear in the deleted-entries map, but the outer if condition still guards on REMOVE:

if (existent == null || entry.getValue().operation == IndexKey.IndexKeyOperation.REMOVE) {
    final RID deletedRid = (entry.getValue().operation == IndexKey.IndexKeyOperation.REPLACE && entry.getValue().oldRid != null)
        ? entry.getValue().oldRid
        : entry.getKey().rid;
    entries.put(key, deletedRid);
}

A REPLACE with a non-null oldRid will only be processed when existent == null. If a prior iteration already placed a value in entries for the same key, a subsequent REPLACE entry will be silently skipped. Is this the intended semantics? A comment clarifying the priority (REMOVE beats REPLACE beats ADD) would help reviewers, and a test that exercises multi-bucket scenarios would confirm correctness.

---

3. HA serialisation — no version guard or backward compatibility handling

TxForwardRequest.writeIndexKeysToBuffer now writes an extra byte (and potentially two unsigned numbers) for every REPLACE entry. Replicas running an older build will misread the stream and produce corrupt index state or throw a deserialization error. For a clustered deployment with a rolling upgrade, this is a data-integrity risk.

Consider bumping the serialization format version (if one exists) or adding a note in the PR description about the requirement for a full-cluster restart when upgrading.

---

Design / Style

4. oldRid initialisation site — consider using the existing IndexKey constructor

The new field is set immediately after construction:

final TransactionIndexContext.IndexKey v = new TransactionIndexContext.IndexKey(index.isUnique(), operation, keyValues, rid);
if (operation == TransactionIndexContext.IndexKey.IndexKeyOperation.REPLACE) {
    final byte hasOldRidFlag = uniqueKeysBuffer.getByte();
    if (hasOldRidFlag == 1)
        v.oldRid = new RID(database, ...);
}

Adding a constructor that accepts oldRid would make construction atomic and keep the field final. This is consistent with the rest of IndexKey.

---

5. Comment style — inline uppercase explanatory comments are non-standard

// REMOVE THE OLD RID THAT WAS REPLACED BY A NEW ONE IN THE SAME BUCKET
// SAVE THE OLD RID SO IT CAN BE PROPERLY REMOVED FROM THE PERSISTED INDEX AT COMMIT TIME

The rest of the file uses mixed-case comments. Shouting-case is typically reserved for TODO/FIXME/HACK markers. Prefer normal sentence-case to match the surrounding code.

---

6. hasOldRidFlag — use boolean, not byte, for a boolean flag

final byte hasOldRidFlag = uniqueKeysBuffer.getByte();
if (hasOldRidFlag == 1)

A local boolean hasOldRid = uniqueKeysBuffer.getByte() == 1; is both more readable and avoids magic literals.

---

Test Coverage

7. Test improvements are good — a few suggestions for completeness

The loop-10 approach nicely exercises the repeated delete/recreate scenario. Two additional cases worth considering:

• Multi-bucket scenario: When multiple edges with the same key land in different buckets (exercising the REMOVE vs REPLACE priority logic in getTxDeletedEntries).
• HA replication path: A test (or at least an integration test note) confirming the new TxForwardRequest serialization round-trips correctly with oldRid populated.

---

8. Test assertion message

assertThat(result.<Long>getProperty("edgeCount")).isEqualTo(1);

Adding .as("should have exactly one edge after repeated delete/recreate cycles") would give a clearer failure message if the assertion ever fails.

---

Summary

The core fix is correct and the logic in addIndexKeyLock() and commit() is clean. The main concerns are:

• Backward-compatibility risk in HA serialization (correctness risk in rolling upgrades)
• getTxDeletedEntries() semantics under multi-bucket REPLACE scenarios need verification or a comment
• Minor style inconsistencies (oldRid mutability, comment casing, byte-as-bool)

Overall this is a well-reasoned fix for a tricky transactional index bug. Addressing the HA versioning concern before merge would be prudent.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ -10.00%	✅ 59.09%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (533c7c5)	86378	62318	72.15%
Head commit (585c393)	115985 (+29607)	72076 (+9758)	62.14% (-10.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3482)	22	13	59.09%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

[codecov]

Codecov Report

❌ Patch coverage is 40.90909% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.88%. Comparing base (533c7c5) to head (585c393).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...m/arcadedb/server/ha/message/TxForwardRequest.java	0.00%	8 Missing and 2 partials ⚠️
...com/arcadedb/database/TransactionIndexContext.java	75.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3482      +/-   ##
==========================================
+ Coverage   63.15%   63.88%   +0.72%     
==========================================
  Files        1299     1299              
  Lines       86378    86427      +49     
  Branches    17767    17789      +22     
==========================================
+ Hits        54552    55212     +660     
+ Misses      24087    23419     -668     
- Partials     7739     7796      +57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.