Skip to content

fix: harden SCQIndexing seek logic and de-flake queue cleanup tests#1700

Open
peter-lawrey wants to merge 4 commits intodevelopfrom
adv/scqindexing
Open

fix: harden SCQIndexing seek logic and de-flake queue cleanup tests#1700
peter-lawrey wants to merge 4 commits intodevelopfrom
adv/scqindexing

Conversation

@peter-lawrey
Copy link
Member

@peter-lawrey peter-lawrey commented Mar 10, 2026

Drawing on this PR for inspiration #1699

This PR contains a manually curated subset of the broader AIDE-driven changes, focused on two areas:

  • hardening SCQIndexing.sequenceForPosition(...)
  • improving cleanup determinism across queue tests

The selected changes keep the indexing fix and its focused regression coverage, while also consolidating test cleanup around a shared background-release helper to reduce file-handle and deletion flakiness.

Functional changes

SCQIndexing: safer and faster position-to-sequence lookup

SCQIndexing.sequenceForPosition(...) has been reworked to improve both correctness and search behaviour.

Changes include:

  • use a read-only secondary-address lookup on the read path via getSecondaryAddressReadOnly(...)

  • replace the previous reverse linear slot search with:

    • findBestSecondarySlotForPosition(...)
    • binary search for the common case
    • findBestSecondarySlotByReverseScan(...) fallback when holes are detected
  • extract toSequenceIndex(...) to make index reconstruction clearer

  • improve variable naming in the indexing path:

    • usedPrimarySlots
    • primarySlot
    • usedSecondarySlots
    • secondarySlot
    • firstPosition
    • candidateAddress
  • tidy setPositionForSequenceNumber(...) naming for readability (secondarySlot, existingPosition)

This addresses two practical issues in the old flow:

  • the read path should not create secondary index blocks as a side effect
  • secondary-index lookup should not always pay the cost of reverse linear scan when a binary search is sufficient

The implementation also preserves correctness in the presence of index holes by falling back to reverse scan when a zero slot is encountered during binary search.

Test changes

New SCQIndexing regression coverage

Added SCQIndexingTest with focused behavioural coverage for the selected indexing changes:

  • sequenceForPositionMustNotCreateSecondaryIndexBlocks
  • sequenceForPositionMatchesLinearAndBinarySearch
  • tailerCanReadAllEntriesWhenIndexHasHoles
  • sequenceForPositionWithInteriorHoleStillFindsCorrectAnchor
  • sequenceForPositionMustNotCreateSecondaryIndexBlocksWithLargeQueue
  • concurrentTailersDoNotCorruptOrThrow
  • tableStoreWriteLockSubprocessStartsWithin15Seconds

Together these tests protect:

  • no write-on-read regression
  • correctness across normal seek/read flows
  • behaviour with sparse or holey secondary indexes
  • behaviour across larger queues / multiple index blocks
  • concurrent tailer seek/read stability
  • subprocess timing assumptions used by lock-related tests

Test cleanup and de-flaking

This PR also introduces a shared cleanup helper in QueueTestCommon:

  • drainBackgroundCleanup()

The helper centralises:

  • BackgroundResourceReleaser.releasePendingResources()
  • GC-cycle waiting
  • a second pending-resource drain

It is now used across a wide set of tests that previously:

  • called BackgroundResourceReleaser.releasePendingResources() directly, or
  • attempted best-effort deletion while swallowing cleanup-related exceptions

This makes teardown more deterministic and reduces flakiness around:

  • lingering mapped files
  • delayed background resource release
  • directory/file deletion on slower or stricter environments

QueueTestCommon updates

QueueTestCommon now:

  • exposes drainBackgroundCleanup()
  • uses it before target-dir artifact checks
  • uses it during general teardown before deleting temp dirs

Tests migrated to shared cleanup

The selected changes update a broad group of tests to use the shared cleanup path before deleting files or asserting release state, including areas such as:

  • acquire/release and rolling tests
  • metadata deletion tests
  • EOF / incomplete-write tests
  • file deletion and roll-cycle issue tests
  • builder and reader tests
  • append/tailer file-handle leak tests

Additional test hardening

A few additional adjustments were kept as part of this curated PR:

  • ChronicleHistoryReaderMainTest only resets the security manager on pre-Java-17 runtimes

  • some tests now extend QueueTestCommon to gain consistent cleanup and exception-expectation support

  • AppenderFileHandleLeakTest was tightened with:

    • shared cleanup usage
    • explicit tailer closure
    • pre-after shutdown
    • longer future timeout
    • clearer retry constants for mapped-file closure checks

Why this PR

The chosen subset keeps the highest-value pieces of the original work:

  • the SCQIndexing read/seek improvement
  • targeted regression tests around that behaviour
  • a practical cleanup de-flake pass across the test suite

It deliberately avoids bundling unrelated planning artefacts or broader changes not needed for the final reviewed PR.

@peter-lawrey peter-lawrey requested review from sam-ross and tgd March 10, 2026 10:12
@peter-lawrey peter-lawrey changed the title Adv/scqindexing fix: harden SCQIndexing seek logic and de-flake queue cleanup tests Mar 10, 2026
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants