fix(#9694): stop concurrent inserts into one bucket from conflicting at commit - #9703

This pull request does not currently match the merge queue conditions, so it cannot be queued from here. The box comes back if it matches again.

Review

I read the diff only. I did not build the project or run the tests.

The fix is well targeted and the root-cause analysis is convincing.

What looks good

• addNewPage() now handles every page-creation path (createRecordInternal, createRecordsBulk, writeMultiPageRecord, updateMultiPageRecord), so the registration cannot be missed.
• The page is registered in inFlightNewPages before anything is written to it, so its creator cannot list it in the free-space map before it is protected.
• The guard in three places (findAvailableSpace, findAvailableSpaceFromStatistics, claimInsertSlot) gives defence in depth, and the isEmpty() check keeps the single-writer case cheap.
• writeRecordAtSlot now uses findContentInsertionOffset on the page it is given. This matches the rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401 delete-replay fix and removes the maxRecordsInPage early exit.
• The tests are deterministic (latches, no wall-clock assertions) and cover both bugs.

Suggestions (minor)

1. Map<Integer, Object> with creator != tx relies on reference identity with an untyped value. Map<Integer, TransactionContext> would be clearer. Keep it consistent with insertReservations if you change one.
2. Please confirm releaseNewPage is reached on every way a transaction ends: commit, rollback, a failed commit before replay, and a commit-retry reset. If a path clears the trackNewPageReservation entries without calling release, the page stays blocked for other transactions until close(). A test that forces a commit failure and then checks the page is allocatable again would lock this in.
3. After a rollback the page number stays reserved (as before this PR) and the page is an empty page that any transaction may fill. Please check that this still holds when txPageCounter has advanced past it.
4. A test for the multi-page record path and for createRecordsBulk with concurrent writers would be nice, since both now go through addNewPage(). A small concurrent-insert stress test asserting no retries would guard against regressions (tag it @Tag("slow") if it is slow).
5. Style: some of the comment blocks are long and could be trimmed.

I found no security concerns. The performance impact looks negligible: one ConcurrentHashMap put and remove per new page, and a lookup per candidate page only while another transaction has a page in flight.

Overall this looks good to merge once point 2 is confirmed.

No actionable comments were generated in the recent review. 🎉

Merge Risk: ⚪ Minimal · up to a9bc8

No actionable merge-blocking issue is established for this change; it is ready for normal merge checks.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 78.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check		The title clearly and concisely identifies the main change: preventing conflicting concurrent inserts into one bucket at commit.
Description check		The description provides detailed motivation, related issue context, implementation scope, measured results, and test coverage. It does not use every template heading and does not explicitly confirm t…
Linked Issues check		Direct issue [#9694] requires preventing lost batches in the default 9,742-insert, four-worker, one-bucket scenario. LocalBucket excludes in-flight pages, replays inserts with `findContentInsertionO…
Out of Scope Changes check		The production changes address the three concurrent-insert causes described in [#9694]. The transaction cleanup changes support new-page reservation ownership. The guidance update and regression tests…

---

• Fix all pre-merge checks with AI

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Up to standards ✅

_{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.}

Review round 1, addressed in 283509e:

1. inFlightNewPages is now Map<Integer, TransactionContext>.
2. Confirmed: the release runs in TransactionContext.releaseInsertSlotReservations(), called from reset() (every commit and rollback ends there, including a commit that fails in phase 1, which rolls back) and from kill(). The new test aCreatedPageIsGivenBackWheneverItsTransactionEnds pins it for commit, rollback and a commit that fails with a ConcurrentModificationException.
3. The same test covers the abandoned page: after the rollback and the failed commit the next insert fills the bucket's pages again and reads back correctly.
4. Not added: the multi-page and bulk paths only changed in how the page number is reserved (one helper), and a 'no retries' stress assertion would be timing-dependent on a shared CI runner. The two deterministic tests fail without the fix.
5. Trimmed the long comment blocks.

Review of #9703 (fix for #9694)

Summary: Both fixes target the right root causes.

• inFlightNewPages keeps other transactions' allocators off pages that are still private to their creator.
• writeRecordAtSlot now measures the committed page with findContentInsertionOffset, as rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401 already did for the delete replay.

Good

• Every page-creation site goes through one addNewPage(): single insert, bulk, and both multi-page paths. The page is registered before anything is written to it, so it cannot appear in freeSpaceInPages unguarded.
• Cleanup reuses the existing reservation arrays (primitives, no per-record allocation) and runs on every teardown path. NEW_PAGE_RESERVATION = -1 is a cheap sentinel. isEmpty() makes the hot-path check nearly free.
• inFlightNewPages.clear() in close() is handled.
• The regression test covers the exact interleaving, and the PR includes before/after measurements.

Points to check

1. claimInsertSlot returns -1 for another transaction's new page. Please confirm every caller treats -1 as "pick another page / create a new one" and none of them loops or reads it as a slot index. The comment calls it a backstop, so a test or assertion that it is never reached in practice would help.
2. In findAvailableSpace, the last-page check marks createNewPage = true when the last page belongs to another transaction. With many writers on one bucket, every writer then creates its own page. That is probably intended, since it avoids conflicts, but it can leave more partially filled pages. Is this reflected in the measurements, for example page count after the Async executor silently discards records at parallel level > 1: 9,742 submitted, 2,436 stored, no error reported #7615 load?
3. Between the creator's publish at commit and releaseNewPage at teardown, the page is still skipped. That is harmless (a short, conservative window), but it deserves a one-line comment.
4. getInFlightNewPagesForTesting is package-private and test-only. Fine, but check that the test also asserts the map is empty after both rollback and commit.
5. In the test, database.isTransactionActive() and rollback run in the worker threads' finally blocks. Please make sure those threads have a bound database context (DatabaseContext) so they do not roll back or commit the wrong transaction. The await(30s) values are used as hang detectors, which is correct per the CLAUDE.md timing rules.
6. Style: the engine/CLAUDE.md update is good. Please also check for a TX_PAGE_SLOT_MERGE mention of the getAvailableSpaceInPage caveat, so nobody reintroduces it.

No security concerns. The change is performance-neutral to positive, since it removes replays. I'd also run the existing slot-merge and concurrency suites (#5279, #5381, #8401 tests) before merging.

Overall LGTM with the minor confirmations above.

Review round 2, in 09e1f8d:

1. claimInsertSlot has one caller, createRecordInternal, which treats -1 as "use a new page" (the same contract the existing reservation-overflow path already used). It never loops on it or reads it as a slot.
2. The last-page branch only matters when the last page the transaction can see belongs to another transaction, which is rare (that page is either the caller's own or a committed one). The pages concurrent writers open were opened before this change too; the difference is that nobody else writes into them until they are committed, after which the free-space map hands their room to everyone. In the Async executor silently discards records at parallel level > 1: 9,742 submitted, 2,436 stored, no error reported #7615 load the bucket ends with the same handful of pages.
3. Added the one-line note on releaseNewPage.
4. Already covered: aCreatedPageIsGivenBackWheneverItsTransactionEnds asserts the map is empty after a rollback, after a failed commit and after a commit.
5. The worker threads each get their own transaction context: ArcadeDB binds transactions per thread, so begin/commit/rollback in a thread only touch that thread's transaction.
6. The getAvailableSpaceInPage caveat is in the TX_PAGE_SLOT_MERGE bullet of engine/CLAUDE.md.

The #5279, #5381, #5569, #8401 and other slot-merge suites pass (292 tests).

Review

The change is well scoped and the explanation is clear. I read the diff only and did not run the tests.

What looks good

• Every page creation goes through one addNewPage(). That covers createRecordInternal, createRecordsBulk and both multi-page paths, so no creation site is left out.
• The creator's page is registered in inFlightNewPages before anything is written to it, so it cannot appear in the free-space map unprotected.
• New-page reservations reuse the existing reservation arrays, with a -1 sentinel for the slot. That adds no per-record allocation.
• Releasing in releaseInsertSlotReservations means commit, rollback and kill all clean up, and the existing try/catch keeps a release failure from aborting teardown.
• writeRecordAtSlot now uses findContentInsertionOffset, as the delete replay already does after rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401. That removes the maxRecordsInPage early exit from the slot-merge replay.
• The regression test is a targeted two-transaction scenario with latches.

Points to check

1. Window after commit: the page is still skipped between the commit's publish and releaseNewPage. You note this costs the others only a short window. That is acceptable.
2. claimInsertSlot returning -1: please confirm every caller treats -1 as "try another page or create a new one". It must not be read as "no slot, abort the insert".
3. findAvailableSpace fallback: forcing createNewPage = true when the last page belongs to another transaction means that under heavy writer contention each writer gets its own page. That is the intended trade, but it can increase page fragmentation with short records and many workers. If you measured page counts, they would be worth adding to the Measured section.
4. Rolled-back pages: after a rollback the page is empty but may still be listed in freeSpaceInPages. Please confirm its stats entry is not stale (for example a free-space value recorded after the creator's partial writes). Otherwise a later allocator could trust an entry that no longer matches the empty page.
5. Test waits: the 30-second latch timeouts act as hang detectors, which fits the guideline against asserting on elapsed time.
6. engine/CLAUDE.md: the diff shows a one-line change (-1/+1) where the visible text looks identical. Please make sure it is intentional (whitespace only, or a truncated line).

Overall this looks correct and safe. Points 2 and 4 are the ones I would verify before merging.

Review round 3 (no code change needed):

2. claimInsertSlot has a single caller, createRecordInternal, which sets createNewPage = true on -1: the insert goes to a fresh page and never aborts. It is the same contract the reservation-overflow path already used.
3. A rolled-back page's free-space entry can only UNDER-state the room: the creator recorded what was left after its own writes, and the page is now empty. findAvailableSpaceFromStatistics never trusts an entry without re-reading the page ("CHECK IF THE SPACE AVAILABLE IS REAL") and rewrites it from the measurement, and an over-stated entry is corrected by the A commit walks every slot of each bucket page it modified to prove the page has no hole: 56% to 58% of the commit of a one-update transaction, 29% of a three-statement transaction's CPU #9483 branch. A stale entry costs at most a missed reuse until the next read, never a wrong placement. aCreatedPageIsGivenBackWheneverItsTransactionEnds inserts into the bucket after both abandoned pages and reads the record back.
4. Intentional: the TX_PAGE_SLOT_MERGE bullet in engine/CLAUDE.md is one long markdown line, and the new sentences were appended to it, so the diff shows -1/+1.
5. Same answer as round 2: the pages concurrent writers open existed before this change too; the fix only keeps others out of them until they are committed, after which the free-space map hands their room to everyone.

Review of PR 9703 (diff read only, not built or run)

Overall: Focused change, well explained, and it fixes two real causes. The PR description documents the measurements.

What looks good

• addNewPage() puts the page-number reservation, tx.addPage and the in-flight registration in one place. It replaces four copy-pasted blocks.
• Registration happens before anything is written to the page, so the creator can't list the page in freeSpaceInPages unprotected.
• Release reuses the existing releaseInsertSlotReservations teardown (commit, rollback and kill). The -1 sentinel in the primitive arrays adds no per-record allocation. Nothing else reads insertReservationSlots, so it is safe.
• isNewPageOfAnotherTransaction has a cheap isEmpty() fast path, and close() clears the map.
• writeRecordAtSlot now measures the page it is given with findContentInsertionOffset, which removes the maxRecordsInPage early-out problem. This matches the earlier delete-replay fix.
• The deterministic two-thread regression test and the engine CLAUDE.md update are good.

Points to consider

1. Page lifetime window. releaseNewPage runs at transaction teardown, after the commit publishes the page. Other transactions skip the page until then. A long-lived creator (large batch, multi-page records) keeps its pages unusable for everyone else. That is the intended trade-off, but a short note on the space cost would help.
2. Rollback. After a rollback the reserved page numbers were never written. Please check they get picked up again, since nothing re-lists them. This was likely already the case, so it is a question rather than a regression.
3. claimInsertSlot returning -1. Please confirm every caller treats it as 'pick another page'. The comment says it is only a safety net, so it is probably fine.
4. Test robustness. The join(60_000) results are not checked. Add assertThat(creator.isAlive()).isFalse() (and the same for other) so a hang fails clearly. Tag any multi-second stress test @Tag("slow").
5. Style. Imports and final usage are consistent, and there are no System.out calls.

No blocking issues found. A test for the rollback case (point 2) would be a good addition.

Review round 4, in f022880:

4. Every join is now followed by an isAlive() assertion, so a hung worker fails the test by name. No test in the class takes more than a fraction of a second, so none needs @Tag("slow").
5. The pages a creator holds are the tail pages it is filling itself; the others allocate around them, and after the commit the free-space map offers whatever room is left, so there is no lasting space cost.
6. Covered by aCreatedPageIsGivenBackWheneverItsTransactionEnds: after a rollback and after a failed commit the map is empty, and the next insert lands in the bucket and reads back correctly. The creator's free-space entry for an abandoned page only under-states its room, and the allocator re-reads a page before trusting an entry.
7. Single caller (createRecordInternal), which turns -1 into a new page.

Review

Overall this is a well-scoped, well-explained fix. I read the diff only and did not build or run the tests.

What looks good

• Every page creation path (createRecordInternal, createRecordsBulk, both multi-page writers) now goes through a single addNewPage(). That removes four copies of the reservation snippet, and the in-flight registration cannot be missed on a new path.
• Registering in inFlightNewPages before the page is written, and releasing it via the existing insert-slot reservation teardown, reuses cleanup that already runs on commit, rollback and kill.
• isNewPageOfAnotherTransaction starts with a cheap isEmpty() check, so the uncontended path costs almost nothing.
• Measuring the committed page with findContentInsertionOffset in writeRecordAtSlot is consistent with the rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401 delete-replay fix.
• The regression test is deterministic: it uses latches rather than sleeps and avoids wall-clock assertions.

Points to consider

1. Sentinel reuse. NEW_PAGE_RESERVATION = -1 shares the insertReservationSlots array with real slots. Fine today since slots are >= 0, but a comment or assertion at trackInsertSlotReservation stating that slot < 0 is reserved would protect against future collisions.
2. Long-lived bulk transactions. One transaction creating thousands of pages now holds thousands of inFlightNewPages entries (boxed Integer keys plus a map node each) until it ends. Probably acceptable, but a quick before/after check on a bulk-import benchmark would be reassuring.
3. Window after commit. The page stays skipped between the commit publish and releaseNewPage. Benign, as the Javadoc says, but worth a line in the engine CLAUDE.md update.
4. findAvailableSpace last-page branch. Forcing createNewPage = true when the last page belongs to another transaction is correct, but under heavy contention it may create more pages (fragmentation). The "Measured" table shows the replay drop but not page count or file size. Adding that would help.
5. Test tagging. The new test looks fast enough for the regular lane. If a multi-run measurement scenario is added, tag it @Tag("slow") or @Tag("benchmark") per CLAUDE.md.

No security concerns, no new dependencies, no new external input paths.

Good to merge from my side once point 1 is considered and, ideally, point 4 is quantified.

Review round 5, in 48c9e28:

1. trackInsertSlotReservation now asserts the slot is not negative; the new-page sentinel goes through a private method of its own.
2. One map entry per page a transaction creates, i.e. one entry per 64 KB page it already holds in memory: well under 0.1% of what the page itself costs.
3. Quantified, and it found a third problem. On the Async executor silently discards records at parallel level > 1: 9,742 submitted, 2,436 stored, no error reported #7615 load (9,742 small records, 4 async workers, one bucket) the bucket ended with ~100 pages, against 5 for a single worker, both before and after the first two fixes: pages 5..102 held ONE record each. When the page the allocator offered had no room once running transactions' reservations were counted, the insert opened a new page straight away, and the free-space map kept offering it the same page for the next record. The rejected page is now dropped from the map and the allocator asked again before opening a new page. Same load, 30 runs: 8 pages (5 + one partly filled tail page per worker), 360 batch commits, 0 replays. Pinned by concurrentInsertsDoNotOpenAPagePerRecord, which fails without the fix.
4. The window is documented on releaseNewPage.

Related suites (async executor, slot merge, bucket iterators, transactions; 264 tests) pass.

Review of #9703 (fix #9694: in-flight new pages and slot-merge replay)

I read the engine diff (LocalBucket, TransactionContext) and the start of the new test. I did not build the PR or run any tests.

Overall

The diagnosis is clear and the fix is targeted. Both root causes are addressed at the right layer:

• inFlightNewPages keeps the allocator off pages that another transaction created and has not finished.
• writeRecordAtSlot now measures the committed page with findContentInsertionOffset. That is the same approach rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401 used for the delete replay.

Funnelling every page creation through addNewPage() is a good consolidation. It replaces four copies of the reservedPageCounter.getAndIncrement() plus addPage pattern. Reusing the existing primitive-array reservation storage, with a NEW_PAGE_RESERVATION sentinel, also fits the repo's performance guidance.

Points to consider

1. Window between publish and release. releaseNewPage runs when the transaction ends, so a committed page is skipped by others for a short time. The Javadoc acknowledges this. Under heavy load it only costs some extra new pages, but it would be good to see that in your measurements.
2. Dropping the rejected page from the free-space map. freeSpaceInPages.remove(rejectedPage, -1) relies on later commits re-listing the page. If a page is rejected only because of in-flight reservations that then roll back, its room stays unlisted until some other commit touches it. This is probably acceptable, but it is worth stating that it can leave space unused for a while. A test that rolls back the reserving transactions and checks the page is reused would pin the behaviour down.
3. MAX_INSERT_PAGE_ATTEMPTS = 3. The loop is bounded and falls back to a new page, which is good. Each attempt takes the freeSpaceInPages monitor, so it is worth confirming there is no contention regression on the single-writer path. The reported before/after table suggests it is fine.
4. inFlightNewPages.clear() in close(). This matches insertReservations. If a transaction ends after close(), the release is a no-op remove, which is safe.
5. The assert slot >= 0 in trackInsertSlotReservation. This is only a guard when assertions are enabled, which is fine since all callers are internal. A negative slot is read back as the sentinel, so a comment on that coupling, which you already have, is enough.
6. getInFlightNewPagesForTesting() is package-private. It matches existing test-hook conventions.

Tests

• The regression test is thorough. It uses latches to build the exact interleaving, and it is named after the issue.
• Please double-check that there is no wall-clock assertion. CLAUDE.md requires StallAwareStopwatch, and I only saw the first part of the file.
• The test also needs one case for the second root cause, a full record table with free bytes. I did not see its full body, so ignore this if it is already there.
• Please run LocalBucket-related suites (slot merge, edge append, delete replay) and an async-insert test with several workers before merging.

Style

The code follows the repo conventions: final locals, single-statement if without braces, and imports instead of fully qualified names. There is no stray System.out.

No blocking issues from my read. LGTM, pending CI and the points above.

Review round 6 (no code change needed):

2. A rejected page whose reservations later roll back is re-listed by the next gatherPageStatistics (it runs on the next allocation that finds the map empty or after 5 s with changes pending, and scans the file), or sooner by any write to the page. The cost is room left unlisted for at most one gather cycle, against the page per record the old path opened.
   1, 3. Measured on the 4-worker load: 8 pages (5 + one tail page per worker) and no replays. The single-writer path never enters the retry: a claim only fails on a page another transaction holds reservations on.
   Tests: no wall-clock assertion anywhere (latches and joins are hang detectors only). The full-record-table case is anInsertIsMergedIntoAPageWhoseRecordTableAnotherTransactionFilled, and the 4-worker async load is concurrentInsertsDoNotOpenAPagePerRecord. The slot-merge, delete-replay (rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401), edge-append (Page-level ConcurrentModificationException on bulk edge CREATE_UPDATE to a shared single-bucket type under concurrent load (remote HTTP, explicit tx) — reinforces #5279 #5381) and async suites pass (264 tests).

Up to standards ✅

_{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.}

Review of #9703 (in-flight new pages, slot-merge space check, bounded page retry)

I read the diff only. I did not build it or run the tests.

Overall: the diagnosis is clear and the three fixes are narrow and each has a regression test. The addNewPage() consolidation also removes five copies of the page-reservation snippet, which is a good change.

Correctness and design

• writeRecordAtSlot: findContentInsertionOffset on the committed page matches what rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401 did for the delete replay. The spaceNeeded > getMaxContentSize() - contentPos check still refuses a page with no room. Please check that findContentInsertionOffset handles a record count of exactly maxRecordsInPage and a slot index above the current count. positionInPage + 1 > recordCountInPage bumps the count, so the new count must not exceed the table size.
• inFlightNewPages lifetime: the entry is released from releaseInsertSlotReservations, so it is only as reliable as that teardown. The existing try/catch there logs and continues, so a failed release leaves a page unusable for other transactions until close(). That is safe but could be a quiet space leak. It may be worth a WARNING-level log, or a test that forces a release failure.
• Post-commit window: between publish and releaseNewPage, the page stays hidden from other transactions' allocators. The Javadoc says this is cheap, and I agree. Under heavy load it does mean each worker briefly opens its own page, which fits the "5 + one tail page per worker" numbers.
• delistReservedPage and relistPage:
  • freeSpaceInPages.remove(pageNumber, -1) is only correct if that map's remove(key, value) has the semantics you intend. Please confirm it isn't a conditional remove that silently does nothing when the stored value differs.
  • relistPage inserts an upper-bound value (the whole usable page). The comment says the next allocation corrects it. That is fine, but it means one wasted page read per relist. Also, the size() < MAX_PAGES_GATHER_STATS guard can drop a relist silently.
  • delisted is a plain boolean, always accessed under synchronized (res), so that is fine.
• delistReservedPage race: if res == null, you relist immediately. A concurrent claimer could create a new reservation just after that. This is benign, because the next allocation corrects the entry, but a comment would help.
• Retry loop: only the latest rejected page is excluded via avoidPageNumber. Earlier rejects are protected only by being delisted. If another thread relists one in between, attempt 3 could pick it again. That is bounded by MAX_INSERT_PAGE_ATTEMPTS, so it is acceptable.
• Assertion in trackInsertSlotReservation: the assert slot >= 0 only helps with -ea. Surefire enables it, so that is fine. A sentinel in a shared int array is a bit fragile, though. A separate boolean[], or encoding the new-page case as Integer.MIN_VALUE, would be harder to misuse. This is only a nit.

Performance

• isNewPageOfAnotherTransaction is cheap: an isEmpty() check, then one ConcurrentHashMap lookup. It is called per candidate in findAvailableSpaceFromStatistics, which is fine for at most 100 candidates.
• createRecordsBulk and the multi-page paths now add one reservation entry per page. The arrays grow by doubling, so there is no per-record cost.

Tests

• The three causes are covered, and so is release on commit, rollback and failed commit. The latch-based tests use bounded waits (30s) and no wall-clock assertions, which follows CLAUDE.md.
• The 4-worker page-count bound could be flaky if the bound is tight. Please confirm it has headroom (it should depend on the worker count, not on timing), and that the class is tagged slow if it takes more than a few seconds.
• A test where the creator's transaction commits while another is mid-allocation (the post-commit window) would be a nice addition, but it is not required.

Style

• The code follows the project conventions: final locals, braces-less single statements, imports used, no System.out.
• The engine/CLAUDE.md update is helpful.

Nothing blocking from what I can see. I would confirm the freeSpaceInPages.remove(key, -1) semantics and the findContentInsertionOffset edge case at a full record table before merging.

Review round 7 (no code change needed), confirming the two points:

• freeSpaceInPages.remove(pageNumber, -1): IntIntHashMap.remove(key, defaultValue) removes unconditionally; the second argument is only what it returns when the key is absent. It is the same call the allocator already uses to drop full pages.
• findContentInsertionOffset at a full record table: it walks the slots below the record count and takes the end of the last live record, so a count of exactly maxRecordsInPage is fine (that is the point of using it instead of the allocator's walker, see rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401). The slot replayed is one reserveInsertSlot handed out, which is always < maxRecordsInPage, so positionInPage + 1 never exceeds the table. anInsertIsMergedIntoAPageWhoseRecordTableAnotherTransactionFilled replays slot 2040 onto a page whose count is 2048.

Others: a failed release can only happen when the bucket was dropped meanwhile (that catch already exists for the slot reservations), and then its pages no longer matter. The page-count bound in concurrentInsertsDoNotOpenAPagePerRecord is 12 against 8 measured (5 + one tail page per worker) and ~100 before; it depends on the worker count, not on timing, and the test runs in well under a second.

Codecov Report

❌ Patch coverage is 87.27273% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.69%. Comparing base (9478dfe) to head (a9bc8af).
⚠️ Report is 4 commits behind head on main.

@@             Coverage Diff              @@
##               main    #9703      +/-   ##
============================================
+ Coverage     77.67%   77.69%   +0.02%     
- Complexity     4436     4439       +3     
============================================
  Files          2183     2183              
  Lines        226795   226924     +129     
  Branches      48981    49008      +27     
============================================
+ Hits         176155   176302     +147     
+ Misses        32026    31995      -31     
- Partials      18614    18627      +13     

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

CI round on d7a1b6c, unit-tests lane:

• Issue6339FreedSpaceStatisticsTest (2 failures) was caused by d7a1b6c: the page re-listing used the whole usable page as an upper bound, and a transaction that fills its own page goes through that path too, so the statistics advertised ~64 KB on a full page. Fixed in ec6d3a6: the page is measured on its committed image and listed under the same thresholds updatePageStatistics applies, and a page another transaction is still creating is left alone.
• Issue9680RunningQueryTerminationTest.cypherForeachStopsWhenTerminatedAndWritesNothing: timing-dependent (the statement finished before the test could terminate it), unrelated to page allocation; passes 15/15 locally.
• WalCommitOrderingTest.explicitLockTransactionWithLateJoiningFileIsRejectedNotSilentlyRelocked: fails on unmodified main too.

Full engine unit suite on ec6d3a6 (benchmark/slow/vector excluded): 19,603 tests, only that pre-existing WalCommitOrderingTest failure.

Review of the #9694 fix. Overall it is well targeted and clearly documented, and the new addNewPage() choke point nicely consolidates five duplicated reserve-and-add sites.

Correctness and risks:

1. inFlightNewPages entries are only removed via releaseInsertSlotReservations() at tx end. Please confirm it runs on every terminal path (commit, rollback, failure after phase 1, tx reset or close without commit, async teardown). A missed path leaves a page permanently off-limits until close(). A test that forces a commit failure and asserts getInFlightNewPagesForTesting() == 0 would pin this.
2. Between commit publish and releaseNewPage the page is skipped (acknowledged in the Javadoc). Check that a committed, partly filled new page is still reachable from freeSpaceInPages afterwards.
3. relistPage swallows all exceptions silently. It is only a hint, but a FINE log line would help diagnose pages that are never reused.
4. writeRecordAtSlot now uses findContentInsertionOffset, consistent with rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401. The old code also rejected an unusable slot table via a negative position; please confirm findContentInsertionOffset cannot return a bogus offset on a corrupt table.
5. The NEW_PAGE_RESERVATION = -1 sentinel stored in the slot array is slightly implicit. Acceptable with the assert; optional to make explicit.

Performance: isNewPageOfAnotherTransaction has a cheap isEmpty() fast path, and the insert retry loop is bounded by MAX_INSERT_PAGE_ATTEMPTS. The measured 9,742 records over about 100 pages versus 5 pages justifies it.

Tests: Issue9694InFlightNewPageAllocationTest is a deterministic, latch-driven regression for cause 1 with no wall-clock assertions. Causes 2 (replay on a page at maxRecordsInPage) and 3 (one page per record) are distinct fixes, so please make sure each has its own regression test (tag with @tag(slow) if the multi-writer load is long). Since allocation changed for all inserts, please also run the existing slot-merge, LocalBucket and concurrent/async insert tests.

Style follows the CLAUDE.md conventions. No blockers from my side, pending confirmation of point 1.

Review round 8 (no code change needed):

1. Confirmed and pinned: the release runs in releaseInsertSlotReservations(), called from reset() (every commit and every rollback, including a commit that fails after phase 1, which rolls back; the async executor ends its batches through the same commit/rollback) and from kill(). aCreatedPageIsGivenBackWheneverItsTransactionEnds forces a commit failure (a ConcurrentModificationException) and asserts getInFlightNewPagesForTesting() is 0 after it, as well as after a rollback and a commit.
2. The creator lists its page in freeSpaceInPages while writing it, and that entry stays; the allocator only skips it while the creator runs. After the release the entry is used as any other (concurrentInsertsDoNotOpenAPagePerRecord shows the effect: 8 pages, not 100).
3. Left as is: the only failure there is the page read, and the next gather measures the page anyway.
4. findContentInsertionOffset goes through getRecordPositionInPage, which raises on a slot pointing inside the header; rebaseRecordOnPage turns that into a DatabaseOperationException that aborts the commit instead of writing at a bogus offset (unchanged behaviour of the rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401 helper).
   Tests: cause 2 is anInsertIsMergedIntoAPageWhoseRecordTableAnotherTransactionFilled, cause 3 concurrentInsertsDoNotOpenAPagePerRecord (well under a second, so not tagged slow). The full engine unit suite passes except the pre-existing WalCommitOrderingTest failure.

305a32f: the slow lane on ec6d3a6 took 81 minutes (the run before took 44, main 40-66 today). That is most likely runner noise, but the one thing ec6d3a6 added on a hot-ish path was a page read inside TransactionContext.reset() (re-measuring a page whose last reservation was released). Teardown should not do page I/O, so the release now only queues the page number and the next allocation, which already reads pages under the freeSpaceInPages monitor, measures it and lists the room it really has. The test now pins both halves (queued at release, measured and listed by the next allocation, never the whole page). Related suites: 272 tests green.

Review of #9703

I read the diff only. I didn't build the project or run any tests.

Overall: The root-cause analysis is convincing and the fix is well scoped. Routing every new-page creation through addNewPage() is a clean way to guarantee that no creation path skips the registration. The comments explain the "why", and the regression tests cover each of the three problems.

Correctness and design

• NEW_PAGE_RESERVATION = -1 reuses the slot arrays as a sentinel. The assert slot >= 0 guards this, but asserts are usually off in production. A negative slot would then release the wrong thing, and it would do so silently. Consider an explicit if that throws IllegalArgumentException. The check is cheap and not on a hot path.
• releaseNewPage runs when the transaction ends, which is after the commit publishes the page. The Javadoc says this leaves a short window where the page is skipped. That is safe and only costs a little space. Please confirm that every teardown path (commit, rollback, failed commit, and a transaction closed abnormally, such as by a thread dying or the database closing) reaches the reservation-release loop. If one doesn't, inFlightNewPages leaks the entry, and that page is permanently excluded from reuse. The failed-commit case is tested. A test for an exception thrown mid-commit would harden this.
• relistQueuedPages() reads pages through tx.getPage while holding the freeSpaceInPages monitor. Other threads' allocations are blocked for the duration of these reads. They are bounded by MAX_PAGES_GATHER_STATS, but pagesToRelist is unbounded, so a burst of delisting could make one allocation do many page reads. Consider capping how many pages are processed per call and leaving the rest queued.
• The catch (Exception) in relistQueuedPages swallows errors silently. A LogManager fine-level log would help when debugging.
• In the retry loop, delistReservedPage is called on a rejected page that may be the caller's own tail page. Please confirm the delisting and relisting path can't let a page flap between listed and unlisted under contention. The delisted flag is set under synchronized (res), which looks right, but the race with removed deserves a targeted concurrency test.
• writeRecordAtSlot now measures the page itself with findContentInsertionOffset. This is the right fix and is consistent with rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401. Its comment is much better than the old one.

Performance

• isNewPageOfAnotherTransaction has an isEmpty() fast path, which is good. It is called per candidate page in the free-space scan. That is acceptable, but the ConcurrentHashMap<Integer,...> boxes the page numbers. Boxing is small next to the page reads here, so I wouldn't worry about it.
• The extra attempts (MAX_INSERT_PAGE_ATTEMPTS = 3) are bounded, which is fine.

Style

• This follows CLAUDE.md: it imports classes, uses final, and the tests use the assertThat(...) syntax.
• The test-only accessors (...ForTesting) are package-private, which is acceptable.
• The PR does not tag the new tests. If the 2048-record prefill test or the multi-writer test takes more than a few seconds, it should be @Tag("slow"). Also confirm that none of the tests assert on raw wall-clock time.

Tests

• Coverage is good. It includes the concurrent-creator scenario, the release on rollback, commit and conflict, and the full record table merge.
• I'd add a stress test with several writers on one bucket. It would assert that no batch exhausts txRetries, which is the original symptom. It should use @Tag("slow") if it's long.
• The engine/CLAUDE.md change looks fine.

Nice work. I'd merge after the sentinel hardening and a check of the teardown paths.

Review round 9, in d83a985:

• Sentinel: trackInsertSlotReservation now throws IllegalArgumentException on a negative slot instead of asserting.
• relistQueuedPages measures at most 8 queued pages per allocation (the rest stays queued for the next one), since it runs under the monitor every allocation of the bucket waits on, and logs at FINE a page it cannot measure.
• Teardown paths: the release runs in releaseInsertSlotReservations(), called from reset() and kill(). Every commit and rollback ends in reset(), including an exception thrown mid-commit: commit1stPhase and commit2ndPhase roll back (and so reset) on any failure, which is the path aCreatedPageIsGivenBackWheneverItsTransactionEnds exercises with a conflict. A database close clears the map in LocalBucket.close(); a dead thread's transaction context is not reused, and its pages are skipped only until the database closes, i.e. a page of space, never a correctness issue.
• Flapping: a page is delisted only by an insert its reservations turned away, and listed again only once the last of those reservations is gone (both decided under the entry's monitor; a release that raced with the delisting is caught by the removed check and lists it at once). The 4-worker load (concurrentInsertsDoNotOpenAPagePerRecord) is the contention case: 8 pages, no replays.
• Tests: none asserts on wall-clock time and all run in well under a second, so none is tagged slow. The multi-writer case asserting no batch exhausts its retries is Issue9529AsyncBatchReplayBackoffTest.fourWorkersOnOneBucketLoseNoBatch (already in the suite) together with concurrentInsertsDoNotOpenAPagePerRecord.

Review of #9703

Well-reasoned fix: each of the three causes has its own regression test and the measured before/after numbers are convincing. I read the main-source diff only and did not run the tests. Notes are mostly minor.

Good

• Routing every page creation through addNewPage() removes four duplicated page-number reservation blocks and registers the page before anything is written to it.
• Reusing the primitive-array reservation storage in TransactionContext (sentinel NEW_PAGE_RESERVATION = -1, plus the slot < 0 guard) adds no per-record allocation.
• writeRecordAtSlot now measures with findContentInsertionOffset, as rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401 did for deletes. contentEndOffset fails loudly on a corrupted table, so dropping the old negative-position check is safe.
• The relist queue records page numbers at teardown and reads pages inside the allocation, capped at 8 per call.

Points to consider

1. Delist vs relist modes: delistReservedPage always removes the page from freeSpaceInPages, but relistPage only queues at REUSE_SPACE_MODE.HIGH or above, while the allocator uses the map from MEDIUM. In MEDIUM a delisted page stays out until the next full gather. Align the conditions or document it.
2. Silent drops in relistQueuedPages: entries leave the queue before the checks, so a page is lost if the map is at MAX_PAGES_GATHER_STATS or its creator is still in flight. Fine as a hint, but worth a comment.
3. Page I/O under the monitor: tx.getPage(...) can hit disk while holding the freeSpaceInPages lock all allocations wait on. Consider a smaller cap (2 to 4) or measuring outside the lock.
4. Leak window in addNewPage(): inFlightNewPages.put runs before trackNewPageReservation. If tracking throws (e.g. OOM growing the arrays), the entry is never released and the page is skipped by everyone until close(). Track first, or remove the entry in a catch. Unlikely but cheap to fix.
5. Post-commit window: the page stays skipped between publish and releaseNewPage. Fine, as documented.
6. Style: MAX_PAGES_RELISTED_PER_ALLOCATION breaks the column alignment of the neighbouring constants.

Tests

• The deterministic latch-based tests and the commit/rollback/failed-commit release test are the right approach.
• Please check the 4-worker test is tagged @Tag("slow") if it takes noticeable time, and that its page-count bound (8) is not load-sensitive. No raw wall-clock assertions.
• Consider a small test for point 1 (MEDIUM vs HIGH after a delist).
• WalCommitOrderingTest.explicitLockTransactionWithLateJoiningFileIsRejectedNotSilentlyRelocked is reported failing on HEAD; unrelated, but worth a tracking issue.

Security / performance
No security concerns. Hot-path cost is one ConcurrentHashMap put per new page plus an isEmpty() fast path on lookups, which is negligible.

Non-blocking overall; I would fix point 4 and clarify point 1 before merge.

Review round 10, in a9bc8af:

4. addNewPage now tracks the page in the transaction before registering it in inFlightNewPages, so nothing can leave an entry that no release reaches.
5. Pages are queued for relisting from REUSE_SPACE_MODE.MEDIUM, the mode the allocator reads freeSpaceInPages from, so the delist and relist conditions match.
6. Commented: a queued page is dropped when there is nothing to do now (already listed, map full, or still being created by another transaction whose own writes list it); the next gather measures every page anyway.
7. Cap lowered to 4 pages per allocation. These pages were just written by the transaction that released them, so they are normally in the read cache.
8. Constant renamed to MAX_RELISTS_PER_ALLOCATION and aligned.
   Tests: the 4-worker test runs in well under a second; its bound is 12 pages (measured 8, ~100 before), set by the worker count rather than timing.

Up to standards ✅

_{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.}

Review of #9703 (in-flight new pages / slot-merge replay)

I read the diff but did not build it or run the tests.

Overall: The fix is well-targeted and the root-cause analysis is convincing. Replays dropping from 1,478 to 0 and pages from ~100 to 8 is a strong result. The three causes are separated cleanly, and each has its own regression test.

Good

• addNewPage() is now the single entry point for page creation. All the call sites are converted, including bulk, multi-page write and multi-page update. It tracks the reservation before the inFlightNewPages.put, so nothing is left unreleased.
• trackInsertSlotReservation rejects negative slots, so the NEW_PAGE_RESERVATION = -1 sentinel cannot collide with a real slot.
• writeRecordAtSlot now uses findContentInsertionOffset, consistent with the rebaseRecordDeleteOnPage reports -1 free bytes to updatePageStatistics under concurrent vertex deletes #8401 delete replay.
• relistQueuedPages is bounded (MAX_RELISTS_PER_ALLOCATION), and the queue is filled from teardown without reading pages, which keeps teardown cheap.
• close() clears the new maps, and the engine/CLAUDE.md update is useful.

Points to consider

1. relistQueuedPages calls tx.getPage, which can do disk I/O, while holding the freeSpaceInPages monitor that every allocation on the bucket waits on. The cap of 4 pages limits the damage, but please check the multi-worker benchmark with a cold cache.
2. releaseNewPage runs at teardown, after the commit publishes the page. The Javadoc acknowledges this short window and it is harmless. Please confirm releaseInsertSlotReservations runs in a finally on every commit and rollback path, including the replication/Raft apply path, otherwise a page stays locked out forever.
3. isNewPageOfAnotherTransaction compares TransactionContext by identity. If any path swaps the context object after the page is created (nested or re-begun transaction on the same thread), the creator would be locked out of its own tail page. A test that creates a page, then commits and begins again on the same thread, would pin this down.
4. In the 3-attempt loop in createRecordInternal, a short comment on why the last attempt delists and then forces a new page would help readers.
5. The tests carry no @Tag. If the 4-worker load test takes more than a few seconds, tag it @Tag("slow"). Also confirm it has no raw wall-clock assertions.
6. The test computes the page as position / maxRecordsInPage. A brief comment saying that is intentional would help.

Style: This matches CLAUDE.md. There is no leftover System.out and no new dependencies, and the code uses final and imports.

LGTM pending the checks above, mainly 2 and 3.

Review round 11 (no code change needed), confirming 2 and 3:

2. Every transaction end goes through TransactionContext.reset() (successful commit after phase 2, rollback, and the rollback a failed phase 1 or phase 2 performs), plus kill(); both call releaseInsertSlotReservations(), whose per-entry try/catch keeps one failed release from skipping the others. The replication apply path never allocates bucket pages (a replica applies the leader's page images; addNewPage runs only in the leader/embedded insert paths), so it holds no entries to release.
3. The context only has to be the same object for the lifetime of one transaction, which it is: entries are released at that transaction's end, so after a commit and a new begin() on the same thread the page is an ordinary committed page and the creator uses it like anyone else (aCreatedPageIsGivenBackWheneverItsTransactionEnds commits and then inserts again on the same thread). A nested transaction has its own context and cannot see the parent's uncommitted page in any case (it lives in the parent's newPages), so skipping it there is the correct answer, not a lockout.
4. The pages relisted are ones the releasing transaction just wrote, so normally in the read cache; at most 4 per allocation.
   5/6. The tests take well under a second each and assert no wall-clock time; position / maxRecordsInPage is the RID-to-page mapping LocalBucket itself uses.