[lvca]

Fixes a silent data-corruption bug in the commutative edge-append merge (GRAPH_EDGE_APPEND_MERGE, on by default since 26.7.2), found while investigating a customer-reported BufferUnderflowException on a high-degree vertex. Internal tracking: https://github.com/ArcadeData/arcadedb-operations/issues/565

The bug

When concurrent transactions append edges to the same vertex, a commit-time page conflict on the edge chunk is resolved by re-deriving the page from the current committed version and replaying the transaction's appends, instead of retrying the whole transaction.

That re-derivation is unsound when the chunk is stored as a multi-page record (a chunk allocated near the end of a page continues on the next page, FIRST_CHUNK/NEXT_CHUNK):

1. The drain writes the record's continuation slice through a page copy created only at drain time, so that page passes the MVCC version check while carrying the transaction's stale view of the continuation bytes.
2. The rebase evicts only the conflicted first page before re-reading the chunk, so the re-read walks the chunk chain through the stale in-transaction copy of the continuation page.
3. The result is committed: concurrently committed appends on the continuation page are silently reverted - zeroed chunk tails on young chunks, shifted/aliased edge-vertex pairs on filled ones, lost edges, invalidLinks/corruptedRecords in CHECK DATABASE, and BufferUnderflowException on later traversals (a shifted region can end mid-varint at the used boundary).

Only chunks that straddle a page boundary are exposed (~1/8 of cap-size chunks at the default 64 KB page), which is why this only shows up at scale, on hot vertices, under concurrent insertion.

The fix

A chunk stored as a multi-page or indirected (placeholder) record is not rebasable: rebaseEdgeAppends now checks the record's layout marker on the current committed page (LocalBucket.isRecordStoredInSinglePage) and falls back to the standard retryable ConcurrentModificationException. Single-page chunks - the vast majority, including all super-node stripe chunks - keep the merge.

Tests

• EdgeAppendMergeMultiPageChunkTest (regression): a 16 KB bucket page makes cap-size chunks straddle a boundary every other chunk; 8 threads x 1500 concurrent inserts on one hub, then the hub's raw pair stream is verified byte-exact (no zeroed bytes, no duplicates, no alien pairs, exact count) with the rebase asserted to have actually fired. Fails on every run without the fix; passes with it.
• EdgeAppendMergeConcurrentStressTest, EdgeAppendMergeRaceTest: concurrent add/remove/read races on the classic layout with the merge active.
• SuperNodeDefaultThresholdTest: super-node promotion at the shipped default threshold (4096), which promotes through the bounded chain-walk branch no test covered before.

Also verified at customer scale during the investigation: a 300k-edge hub with 8 concurrent writers and ~12k rebases fired ends with a byte-exact pair stream, and the full graph/database/engine test packages pass.

Please review before merging.

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request

[gemini-code-assist]

Code Review

This pull request addresses issue #565 by preventing edge-append rebasing on records that span multiple pages, falling back to a full transaction retry instead. It introduces isRecordStoredInSinglePage in LocalBucket to identify these multi-page records and adds several robust concurrent stress and race tests to verify the fix. The review feedback suggests adding defensive checks for null or invalid RIDs in the new method and preserving the original exception cause when wrapping IOException in DatabaseOperationException.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

To prevent potential NullPointerException or IndexOutOfBoundsException (due to negative index calculation when reading from the page buffer), we should add defensive checks to ensure the rid is non-null and has a valid non-negative position.

Suggested change

	public boolean isRecordStoredInSinglePage(final RID rid) {
	final int pageId = (int) (rid.getPosition() / maxRecordsInPage);
	final int positionInPage = (int) (rid.getPosition() % maxRecordsInPage);
	public boolean isRecordStoredInSinglePage(final RID rid) {
	if (rid == null || rid.getPosition() < 0)
	return false;
	final int pageId = (int) (rid.getPosition() / maxRecordsInPage);
	final int positionInPage = (int) (rid.getPosition() % maxRecordsInPage);

[gemini-code-assist]

When catching IOException and wrapping it in a DatabaseOperationException, please pass the original exception e as the cause. This preserves the stack trace and makes troubleshooting underlying I/O or page-reading issues much easier.

Suggested change

	} catch (final IOException e) {
	throw new DatabaseOperationException("Error on checking record layout for " + rid);
	}
	} catch (final IOException e) {
	throw new DatabaseOperationException("Error on checking record layout for " + rid, e);
	}

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 9 complexity

Metric	Results
Complexity	9

View in Codacy

🟢 Coverage 76.19% diff coverage · -6.93% coverage variation

Metric	Results
Coverage variation	✅ -6.93% coverage variation
Diff coverage	✅ 76.19% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (ce50b05)	140251	104470	74.49%
Head commit (996a46c)	172161 (+31910)	116307 (+11837)	67.56% (-6.93%)

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 (#5302)	21	16	76.19%

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%

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Review: edge-append merge on multi-page edge chunks (PR 5302)

Thanks for the very clear write-up. I traced the fix against TransactionContext.rebaseEdgeAppends, the LocalBucket record-layout markers, and the sibling tests. Overall this is a correct, well-scoped, and conservative fix with strong regression coverage. A few points below, one of which is a repo-convention issue worth addressing before merge.

Correctness (looks good)

• isRecordStoredInSinglePage returns true only when recordSize[0] > 0, which is exactly the in-place, single-page marker. Every other marker (RECORD_PLACEHOLDER_POINTER = -1, FIRST_CHUNK = -2, and the surrogate-content case < RECORD_PLACEHOLDER_CONTENT) falls to false -> ConcurrentModificationException -> full retry. That is the safe direction, and it matches the existing decoding in existsRecord / scan.
• Reading the current committed state via database.getTransaction().getPage(...) after modifiedPages / immutablePages were evicted for pageId is right: the rebase must decide based on what it is about to write over, not on the stale tx copy. Guarding on the current layout also correctly catches a chunk that became multi-page concurrently.
• The guard only inspects the segment whose page equals the conflicted pageId, which is sufficient: a single-page chunk has no continuation slice, so re-writing it cannot touch another (drain-time-copied) page. The chunk-fill case is still handled downstream by segment.add(...) == false.
• The pageId >= pageCount.get() && pageId >= getTotalPages() short-circuit is logically equivalent to the nested form in existsRecord and avoids the extra getTotalPages() call on the common path.

Convention issue: tests should be @tag("slow")

The four new tests each drive thousands of concurrent transactions (e.g. EdgeAppendMergeMultiPageChunkTest = 8x1500, SuperNodeDefaultThresholdTest = 12k + 20k edges with two reopens). Per CLAUDE.md, functional regression tests that take noticeably long should carry @tag("slow") so CI can skip them from the fast build. The directly analogous sibling ConcurrentEdgeAppendMergeTest is already annotated @tag("slow") at the class level - these new tests should follow suit (import org.junit.jupiter.api.Tag;).

Minor

• Debug System.out.println left in tests. CLAUDE.md asks to remove debug System.out. There are several: EdgeAppendMergeConcurrentStressTest (>>> expected=..., >>> TOP ROW: ...), EdgeAppendMergeRaceTest (>>> added=...). Consider dropping them or routing through the logger; the AssertJ .as(...) descriptions already carry the diagnostic context on failure.
• Small duplication. isRecordStoredInSinglePage repeats ~15 lines of page/marker decoding from existsRecord. Not blocking, but a shared private helper returning the raw size marker (or a sentinel) would keep the two in lockstep if the layout ever changes.
• getStats().edgeAppendMerges is read without synchronization at the end of each test. It is a post-join read so it is fine here; just noting the counter is a plain field.

Test coverage (strong)

Good that each test asserts edgeAppendMerges > 0 so a green run cannot pass vacuously, and that EdgeAppendMergeMultiPageChunkTest verifies the raw pair stream byte-exact (no zeroed bytes, no dupes, no alien pairs, exact count) rather than just the aggregate count - that directly targets the reported corruption shape. The full-retry fallback path is exercised implicitly (multi-page chunks fall back while single-page ones rebase) and the end state is verified, which is what matters.

Nice fix. The @tag("slow") addition is the one thing I would want before merge.

[lvca]

Addressed the review feedback in 996a46c:

• Defensive RID guard - isRecordStoredInSinglePage now returns false for a null RID or negative position before any index arithmetic.
• Exception cause preserved - the IOException is passed as the cause when wrapping in DatabaseOperationException.
• Shared marker decoding - extracted readRecordSizeMarker as the single place that decodes a record slot's size marker; existsRecord and isRecordStoredInSinglePage both use it now, so they stay in lockstep if the page layout ever changes. existsRecord's external behavior is unchanged.
• Test conventions - all four new tests are now @Tag("slow") (matching ConcurrentEdgeAppendMergeTest) and the debug System.out left from the investigation is removed; the AssertJ .as(...) descriptions carry the diagnostic context on failure.

Re-ran the four new tests plus the full com.arcadedb.engine and com.arcadedb.database packages after the refactoring - all green. Ready for another look.

[claude]

Code Review: edge-append merge on multi-page chunks (#5302)

Reviewed the fix, the refactor, and the four new tests. This is a well-diagnosed, well-documented, correctness-over-throughput fix. The core reasoning is sound and I'd be comfortable merging it. Notes below.

Correctness ✅

• The gate is exactly right: isRecordStoredInSinglePage returns true only for a positive size marker (recordSize[0] > 0), which correctly excludes FIRST_CHUNK (-2, multi-page) and RECORD_PLACEHOLDER_POINTER (-1, indirection), plus RECORD_PLACEHOLDER_CONTENT (large negative). All non-in-place layouts fall back to the standard retryable ConcurrentModificationException. Conservative and safe.
• No TOCTOU: rebaseEdgeAppends drops the stale copies (modifiedPages.remove / immutablePages.remove) before the check, so isRecordStoredInSinglePage observes the current committed layout via getPage, and the subsequent bucket.getRecord(segmentRID) reads the same transaction-cached page. Both run under the edge file's commit lock, so the layout can't shift between the two reads. Good.
• The loop only inspects segments whose home page equals the conflicted pageId (edgeSegmentPageKey(segmentRID) == pageKey), so the marker read targets the freshly-reloaded page. Correct.
• Null/negative-RID guard in the new method defaults to "not single page" -> full retry, which is the safe direction.

Refactor (existsRecord -> readRecordSizeMarker) ✅

Behavior is preserved: the old inline path returned false for missing page / out-of-range slot / deleted slot, which now maps to null -> false, and the marker predicate (> 0 || RECORD_PLACEHOLDER_POINTER || FIRST_CHUNK) is unchanged. Nice extraction that removes the duplication between the two callers.

Performance

• The extra isRecordStoredInSinglePage read is a second page fetch per segment, but only on the edge-append conflict path under the commit lock (not a general hot path), and it hits the transaction page cache that getRecord reuses immediately after - negligible.
• The tradeoff (multi-page chunks, ~1/8 of cap-size chunks, now take a full retry instead of a merge) is clearly the right call vs. silent corruption, and super-node stripe chunks stay single-page so they keep the merge. Well-scoped.

Tests ✅

Strong coverage: the regression test (EdgeAppendMergeMultiPageChunkTest) forces the multi-page straddle with a 16 KB page and verifies the raw pair stream byte-exact, and every test asserts edgeAppendMerges > 0 so a green run can't pass without the rebase actually firing. The race/stress tests and the default-threshold super-node test close real gaps. @Tag("slow") is applied per the project convention.

Minor suggestions (non-blocking)

1. Slow tests won't run in normal CI. All four tests, including the actual #565 regression, are @Tag("slow") and thus excluded from the regular build. That's consistent with the CLAUDE.md convention, but it means the one test that fails-without-the-fix won't guard against a future regression unless the slow suite is run on a schedule. Worth confirming the slow tag is part of a periodic CI job.
2. edgeAppendMerges > 0 is timing-dependent. Under adversarial schedulers a run could theoretically produce zero rebases and fail the precondition assert rather than the property under test. With 8 threads on one hub and retry counts of 50-100 it's near-certain to fire, so this is low risk, just flagging it.
3. Tiny: the double read (isRecordStoredInSinglePage then getRecord) could be folded into one marker+record fetch if this ever shows up on a profile, but it's not worth the added coupling today.

Overall: clear root-cause analysis, minimal and conservative fix, and tests that genuinely exercise the failing path. LGTM. 👍

[codecov]

Codecov Report

❌ Patch coverage is 61.90476% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.60%. Comparing base (ce50b05) to head (996a46c).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...src/main/java/com/arcadedb/engine/LocalBucket.java	57.89%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5302      +/-   ##
============================================
+ Coverage     65.58%   65.60%   +0.02%     
+ Complexity      972      969       -3     
============================================
  Files          1714     1714              
  Lines        140251   140260       +9     
  Branches      30023    30025       +2     
============================================
+ Hits          91986    92022      +36     
+ Misses        35822    35768      -54     
- Partials      12443    12470      +27     

☔ View full report in Codecov by Harness.
📢 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.