Skip to content

fix(#5066): map the 'fast' async queue to the MPMC DisruptorBlockingQueue - #5081

Merged
lvca merged 5 commits into
mainfrom
fix/5066-fast-queue-shutdown-undo
Jul 7, 2026
Merged

fix(#5066): map the 'fast' async queue to the MPMC DisruptorBlockingQueue#5081
lvca merged 5 commits into
mainfrom
fix/5066-fast-queue-shutdown-undo

Conversation

@lvca

@lvca lvca commented Jul 7, 2026

Copy link
Copy Markdown
Member

Fixes #5066.

Verdict per option in the issue

The issue offered two shapes: a second drain pass on shutdown, or documenting the limitation. Investigation found the real defect one level deeper, which makes both options the wrong fix:

  • The fast impl violated its own concurrency contract in normal operation. PushPullBlockingQueue is an explicit single-producer/single-consumer design ("Transfers from a single thread writer to a single thread reader", no CAS on the tail sequence). Every async worker queue here has MANY producers: any application thread calling scheduleTask, cross-scheduling workers, waitCompletion markers, the closing thread. This is not theoretical - the new multi-producer regression test demonstrated lost tasks empirically on the old impl (8 producers x 250 no-op tasks, completion latch never reached zero). Note the high-performance profile selects fast, so this was not an obscure opt-in corner.
  • A second drain pass cannot be made total. A producer that snapshotted executorThreads before shutdown can offer arbitrarily late, so no finite number of drain passes closes the window; the producer-side remove-undo is the only shape that works regardless of timing - and it needs remove(Object).

The fix

arcadedb.asyncOperationsQueueImpl=fast now maps to DisruptorBlockingQueue, the flagship MPMC variant from the same Conversant jar (no new dependency, same Apache 2.0 artifact):

  • lock-free CAS-claimed sequences honor the multi-producer contract;
  • remove(Object) is implemented (cursor-blocked scan-and-shift), so scheduleTask's post-shutdown undo now works identically on both queue impls - the fix(#4953,#4954,#4961): async executor stall detector, shutdown draining, concurrency cleanups #5062-documented residual gap is gone;
  • capacity 1 is explicitly supported (Capacity.getCapacity), matching small test configs; capacities round up to the next power of two, as the old ring already did.

removeQuietly's UnsupportedOperationException catch is kept as defensive-only for hypothetical future impls, with its comment and operator WARNING updated.

Red-first evidence

AsyncFastQueueShutdownUndoTest (both red on the old impl, green after the swap):

  • fastQueueMustSupportRemoveForThePostShutdownUndo: the exact primitive the undo depends on - threw UnsupportedOperationException before.
  • fastQueueMustNotLoseTasksUnderConcurrentProducers: timed out with lost tasks before; deterministically green on a compliant MPMC impl.

Verification

mvn -pl engine compile; the full com.arcadedb.database.async battery plus AsyncTest run twice: default config (36 tests) and the fast-queue variant via -Darcadedb.asyncOperationsQueueImpl=fast (36 tests) - all green, so every existing shutdown/drain/stall/helping regression also passes on the Disruptor queue. Release notes updated to record that the formerly documented fast-queue gap is fixed.

…ueue

The issue offered a second drain pass or documenting the limitation.
Investigation found the real defect one level deeper, making both
options wrong:

- PushPullBlockingQueue (what 'fast' mapped to) is an explicit
  single-producer/single-consumer design ("Transfers from a single
  thread writer to a single thread reader") with no CAS on its tail
  sequence. Every async worker queue has MANY producers (any
  application thread, cross-scheduling workers, waitCompletion markers,
  the closing thread), so the contract was violated in normal
  operation, not just at shutdown. The new multi-producer regression
  test demonstrated it empirically: 8 producers x 250 no-op tasks LOST
  tasks on the old impl (latch never reached zero). The 'fast' setting
  is also selected by the high-performance profile, so this was not an
  obscure corner.
- A second drain pass cannot be made total anyway: a producer that
  snapshotted executorThreads before shutdown can offer arbitrarily
  late, and no finite number of passes covers that; the producer-side
  remove-undo is the only shape that works regardless of timing, and it
  needs remove(Object).

Fix: 'fast' now maps to DisruptorBlockingQueue, the flagship MPMC
variant from the same Conversant jar (no new dependency): lock-free
CAS-claimed sequences honor the multi-producer contract, remove(Object)
is implemented (cursor-blocked scan), so scheduleTask's post-shutdown
undo now works identically on both impls, and capacity 1 is supported
(explicitly allowed by Capacity.getCapacity), matching the test
configs. Capacities round up to the next power of two, as they already
did with the old ring. removeQuietly's UnsupportedOperationException
catch is kept as defensive-only for hypothetical future impls, with the
comment updated.

Red-first evidence (AsyncFastQueueShutdownUndoTest):
- fastQueueMustSupportRemoveForThePostShutdownUndo: threw
  UnsupportedOperationException on the old impl; green after the swap.
- fastQueueMustNotLoseTasksUnderConcurrentProducers: timed out with
  lost tasks on the old impl; deterministically green on the MPMC one.

Verified: mvn -pl engine compile; com.arcadedb.database.async battery
plus AsyncTest twice - default config (36 tests) and the fast-queue
variant via -Darcadedb.asyncOperationsQueueImpl=fast (36 tests) - all
green, so every existing shutdown/drain/stall regression now also
passes on the Disruptor queue. Release notes updated: the formerly
documented fast-queue residual gap is fixed.
@mergify

mergify Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

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

  • Queue this pull request

@codacy-production

codacy-production Bot commented Jul 7, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

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

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review — PR #5081: map the 'fast' async queue to DisruptorBlockingQueue

Overall this is a well-diagnosed, well-scoped fix. The root-cause analysis (the fast queue was an SPSC design used in a many-producer context) is convincing, the red-first tests are genuinely red-then-green, and the change reuses the same Conversant jar so no new dependency or license concern is introduced. The comments and release-note update are unusually thorough. Nice work.

I could not run Maven in this review sandbox, so the notes below are from static analysis and reading the surrounding code; I relied on the PR's reported test runs for the green result.

Correctness — looks sound

  • waitCompletion accounting is not broken by the manual remove in test 1. waitCompletion works by offering a per-worker DatabaseAsyncCompletion marker and awaiting it, not by comparing counterScheduledTasks against an executed counter. So removing the probe out from under the executor (which bumps counterScheduledTasks but never executes it) does not leave waitCompletion hanging. This is subtle and worth calling out as intentionally safe.
  • No spurious offer failures. DisruptorBlockingQueue.offer() (via MPMCConcurrentQueue) retries on CAS contention and only returns false when genuinely full, so the waitIfQueueIsFull=false callers (e.g. index compaction) won't see a task dropped just because two producers raced. This is exactly the property the old SPSC queue lacked.
  • No remaining code references to PushPullBlockingQueue — only explanatory comments/javadoc.

Points worth addressing

  1. Capacity 1 is claimed but not covered by a test. The PR body and the code comment both assert "capacity 1 is explicitly supported" / "capacity rounded up to a power of 2", but neither new test exercises it (they use per-worker capacities of 4 and 32). Capacity 1 is reachable in production: queueSize = ASYNC_OPERATIONS_QUEUE_SIZE / parallelLevel is floored to 1 (lines 152-153) when the pool is large. Since it is stated as a guarantee and the Conversant ring / Capacity.getCapacity minimum-size behavior is version-specific (disruptor 1.2.21), consider adding a tiny test that runs with an effective capacity of 1 and confirms offer/poll/remove behave, so a future library bump cannot silently regress it.

  2. remove(Object) on DisruptorBlockingQueue is an O(n), whole-queue-locking scan. This is fine here because removeQuietly is only called on the rare !target.isAlive() post-shutdown race (line 1030), never on the steady-state hot path — but it is worth being explicit that this must not migrate onto a hot path. A one-line note at the removeQuietly call site would prevent a future refactor from turning a rare stop-the-world scan into a per-schedule cost.

Minor / nits

  • CPU tradeoff (not a regression, just for the release note's benefit): DisruptorBlockingQueue busy-spins before parking, so idle workers cost more CPU than standard. This matches the prior fast (PushPullBlockingQueue also spun), so it is not a regression, and fast is opt-in with standard as default — but operators reading the note may appreciate the reminder that fast trades CPU for latency.
  • Test thread-name coupling: findWorkerThread locates the worker by scanning Thread.getAllStackTraces() for "AsyncExecutor-<db>-<slot>". Brittle if the naming scheme ever changes, but acceptable for a white-box regression test and consistent with the pattern already used here.
  • t.join(30_000) return value is ignored in the concurrent-producers test; a stuck producer would be caught later by the executed.await(30, SECONDS) assertion, so this is fine, just noting it.
  • Per CLAUDE.md's tagging guidance, the concurrent-producers test (2000 tasks) is bounded and typically sub-second, so it likely does not need @Tag("slow") — the @Timeout(60) safety net is the right call.

Verdict

Correct root cause, minimal blast radius, good regression coverage. The one thing I would genuinely want before merge is an explicit capacity-1 test to back the stated guarantee. Everything else is optional polish.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.76%. Comparing base (f3fab92) to head (c4cd00b).
⚠️ Report is 54 commits behind head on main.

Files with missing lines Patch % Lines
...dedb/database/async/DatabaseAsyncExecutorImpl.java 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5081      +/-   ##
============================================
+ Coverage     65.63%   65.76%   +0.13%     
- Complexity      841      892      +51     
============================================
  Files          1687     1691       +4     
  Lines        135373   135819     +446     
  Branches      28969    29042      +73     
============================================
+ Hits          88846    89323     +477     
+ Misses        34411    34320      -91     
- Partials      12116    12176      +60     

☔ 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.

@lvca lvca self-assigned this Jul 7, 2026
@lvca lvca added this to the 26.7.2 milestone Jul 7, 2026
… cold path; CPU note

The review's one genuine pre-merge want, taken: capacity 1 is
production-reachable (ASYNC_OPERATIONS_QUEUE_SIZE / parallelLevel floors
to 1 on large pools) and the Conversant ring's minimum-size rounding is
library-version-specific, so the stated guarantee is now pinned by
fastQueueSupportsEffectiveCapacityOne - a parked-worker probe of the real
1-slot queue: offer, reject-when-full (not block/throw), remove(Object),
slot reuse. A future disruptor bump cannot silently regress the
post-shutdown undo primitive at the production floor.

Point 2 taken as the requested note at the removeQuietly call site:
remove(Object) on the Disruptor queue is an O(n) whole-queue-locking scan,
acceptable only on the rare dead-worker post-shutdown race - it must never
migrate onto the steady-state scheduling path.

Nit taken: the release note now states the fast-profile CPU tradeoff
(busy-spin before park, same as the previous implementation) for
operators. The thread-name coupling, ignored join return and no-slow-tag
observations are the reviewer's own verifications, kept as-is.

AsyncFastQueueShutdownUndoTest 3/3 plus the async battery under
-Darcadedb.asyncOperationsQueueImpl=fast, all green.
@lvca

lvca commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@lvca

lvca commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

@claude review the latest commit (6580d19) - the round-1 response adding the capacity-1 guarantee test, the removeQuietly cold-path note, and the fast-profile CPU tradeoff in the release note.

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @lvca's task in 1s —— View job


I'll analyze this and get back to you.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request resolves issue #5066 by replacing the single-producer/single-consumer PushPullBlockingQueue with the multi-producer DisruptorBlockingQueue for the 'fast' queue implementation. This change prevents silent task loss under concurrent producers and restores support for the remove(Object) operation needed during post-shutdown undo. A comprehensive regression test suite has been added to verify these behaviors. The review feedback suggests optimizing the test helper method findWorkerThread by traversing the ThreadGroup instead of calling Thread.getAllStackTraces(), which incurs high JVM overhead by triggering a global safepoint.

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.

Comment on lines +123 to +128
private static DatabaseAsyncExecutorImpl.AsyncThread findWorkerThread(final DatabaseInternal db, final int slot) {
final String name = "AsyncExecutor-" + db.getName() + "-" + slot;
return (DatabaseAsyncExecutorImpl.AsyncThread) Thread.getAllStackTraces().keySet().stream()
.filter(t -> t.getName().equals(name)).findFirst()
.orElseThrow(() -> new IllegalStateException("Worker thread " + name + " not found"));
}

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.

medium

Using Thread.getAllStackTraces() is a heavy operation because it forces the JVM to dump stack traces for all active threads, which triggers a global safepoint. In a large test suite, this can lead to significant performance overhead and transient timing/flakiness issues under heavy CI load.\n\nInstead, you can traverse the active threads in the thread group using Thread.currentThread().getThreadGroup(), which is much more efficient and avoids capturing stack traces.

  private static DatabaseAsyncExecutorImpl.AsyncThread findWorkerThread(final DatabaseInternal db, final int slot) {
    final String name = "AsyncExecutor-" + db.getName() + "-" + slot;
    ThreadGroup group = Thread.currentThread().getThreadGroup();
    while (group.getParent() != null) {
      group = group.getParent();
    }
    Thread[] threads = new Thread[group.activeCount() * 2];
    final int count = group.enumerate(threads);
    for (int i = 0; i < count; i++) {
      if (threads[i] != null && name.equals(threads[i].getName())) {
        return (DatabaseAsyncExecutorImpl.AsyncThread) threads[i];
      }
    }
    throw new IllegalStateException("Worker thread " + name + " not found");
  }

@lvca

lvca commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

@claude can you review the latest commits?

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @lvca's task in 2m 42s —— View job


Reviewing latest commits on PR #5081

  • Set up todo list
  • Gather context (git log/diff, changed files)
  • Review commit 3a8d269 (fast queue → DisruptorBlockingQueue)
  • Review commit 6580d19 (round-1 response: capacity-1 test, removeQuietly note, CPU tradeoff note)
  • Post review feedback

(Note: mvn compile/tests could not be run in this sandbox - permission for the Maven command was not granted. Findings below are from reading the diff and surrounding code, cross-checked against the actual method signatures in DatabaseAsyncExecutorImpl and TestHelper.)


Commit 3a8d269 - fast → DisruptorBlockingQueue

Already reviewed in detail in my prior comment on this PR - stands: correct root-cause diagnosis (SPSC queue used in a many-producer context), no new dependency, red-first tests. Nothing new to add here.

Commit 6580d19 - round-1 response

This addresses all three points from the prior review, and each holds up:

  1. Capacity-1 guarantee is now pinned (AsyncFastQueueShutdownUndoTest.java:129-163). fastQueueSupportsEffectiveCapacityOne parks a worker, then drives the raw 1-slot DisruptorBlockingQueue directly: offer succeeds once, a second offer is rejected (not blocked/thrown), remove(Object) frees the slot, and the freed slot is reusable. This is exactly the guarantee the release note and code comments assert, and it's now regression-tested against a future Conversant version bump. Verified setParallelLevel/recreateThreadsForTests are real methods on DatabaseAsyncExecutorImpl (lines 843, 885), so the test compiles against the actual API.

    • One subtlety worth confirming (not a bug): first/second are offered directly onto worker.queue, bypassing scheduleTask, so counterScheduledTasks is never incremented for them. That's fine because waitCompletion() (lines 428-479) tracks completion via a per-worker marker task offered onto the same queue and awaited, not by comparing scheduled vs. executed counters - so the direct-offer bypass doesn't desync anything the test then asserts on.
  2. removeQuietly cold-path note added at the call site (DatabaseAsyncExecutorImpl.java:1030-1032), explicit that the O(n) whole-queue-locking scan is acceptable only on the rare dead-worker post-shutdown race and must never migrate onto the steady-state scheduling path. Matches the ask exactly.

  3. CPU tradeoff documented in docs/release-26.7.2.md:313-315: busy-spin-before-park cost of the Disruptor queue vs. standard, noted as unchanged from the previous fast implementation. Accurate and appropriately scoped (not a regression, just an operator-facing reminder).

Minor / nit (non-blocking)

  • Missing blank line in AsyncFastQueueShutdownUndoTest.java between findWorkerThread (ends line 128) and the new fastQueueSupportsEffectiveCapacityOne test (line 129) - no separating blank line, and the new test method now sits after the private helper rather than grouped with the other @Test methods. Purely cosmetic (Prettier won't touch it per this repo's requirePragma: true setup), but worth a quick tidy for readability.
  • Gemini's suggestion to replace Thread.getAllStackTraces() with ThreadGroup traversal in findWorkerThread is reasonable (avoids the safepoint-triggering stack dump) but is a pre-existing pattern from the original commit, not something introduced by the round-1 response - optional polish, not a blocker.

Verdict

Round-1 response fully closes out my prior review's asks: the capacity-1 guarantee is now test-backed, the O(n) scan is explicitly scoped to the cold path, and the release note is more complete for operators. No correctness issues found. The blank-line nit above is the only thing I'd touch, and it's optional.

…methods

Round 2 fully closed out the round-1 asks with no correctness issues; the
lone cosmetic nit taken: the capacity-1 test I added had landed after the
private findWorkerThread helper without a separating blank line. Moved it
up with the other @test methods and left the helper last, matching the
class layout convention. Test-only reordering, no behavior change.

AsyncFastQueueShutdownUndoTest 3/3.
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review: fix(#5066): map the 'fast' async queue to the MPMC DisruptorBlockingQueue

Thorough, well-argued fix. The root-cause analysis is correct and valuable: PushPullBlockingQueue is explicitly SPSC (no CAS on the tail), yet every worker queue here genuinely has many producers (application threads, cross-scheduling workers, completion markers, the closing thread). Swapping to DisruptorBlockingQueue from the same Conversant jar (no new dependency, both already on the classpath) is the right shape, and it also brings a working remove(Object) so the post-shutdown undo is no longer a documented gap. The red-first tests, docs update, and honest CPU trade-off note are all appreciated.

Verified locally: no dangling PushPullBlockingQueue references remain, the import swap is clean, id == slot so findWorkerThread's name matching is correct, and the release-notes file (docs/release-26.7.2.md) matches the current 26.7.2-SNAPSHOT version.

Note: I could not run Maven in this review environment (blocked), so statements about test-green status rely on the PR's stated verification rather than a re-run here.

Points worth a look

1. Confirm the capacity-1 guard is actually green in CI (medium).
fastQueueSupportsEffectiveCapacityOne asserts the effective capacity is exactly 1 (offer(second) returns false). Conversant's DisruptorBlockingQueue rounds up to the next power of two via Capacity.getCapacity(...); some library versions enforce a minimum ring size larger than 1. If 1.2.21 floors the ring above 1, the second offer returns true and this test fails. The test is a good regression guard either way - just please make sure CI shows it green on this exact library version, since the behavior is library-version-specific (as the test comment itself acknowledges).

2. Pre-existing divide-by-zero in the back-pressure calc (low, not introduced here).
Lines ~1013 and ~1145:
```java
final int queueFullAt = 100 - (queue.remainingCapacity() * 100 / (queue.remainingCapacity() + queue.size()));
```
remainingCapacity() and size() are two separate calls. If the queue goes full then empty between them (count_A == cap, then size() reads 0), the denominator is 0 -> ArithmeticException. This is reachable today even with standard/ArrayBlockingQueue when applyBackPressureOnPercentage > 0 and the queue is small, so it is not caused by this PR. But it is worth flagging because DisruptorBlockingQueue's size()/remainingCapacity() are weakly-consistent estimates (unlike ArrayBlockingQueue's exact counts), which widens the window. A single snapshot or a Math.max(1, ...) guard on the denominator would close it. Consider a follow-up.

3. Test message overstates what is verified (nit).
In fastQueueMustNotLoseTasksUnderConcurrentProducers, the assertion message says tasks execute "exactly once", but a CountDownLatch only detects under-counting (loss), not duplicate execution (it floors at 0). Since the SPSC failure mode is loss, the test is still sound - just consider softening the message to "must all execute" to match what is actually asserted.

4. findWorkerThread via Thread.getAllStackTraces() (nit).
Building a full JVM stack-dump to locate the worker and then reaching into worker.queue is heavyweight and couples the test to thread-naming internals. A small package-visible test accessor for the slot's queue would be cleaner and less fragile, but this is fine as-is for a regression test.

Performance / operational

The high-performance profile selects fast, so this ships to real deployments. The doc correctly records the busy-spin-before-park CPU trade-off; since the previous fast impl also spun/parked, idle-CPU behavior should be broadly comparable. The one genuinely new steady-state cost to keep an eye on is remove(Object)'s whole-queue lock - the #5081 review comment correctly fences it to the rare dead-worker post-shutdown race, which is the right call.

Overall this is a solid, correct fix that closes the documented gap rather than papering over it. My only "please confirm" is point 1 (CI-green on capacity-1 against this library version); the rest are follow-ups/nits.

…akly-consistent 0/0 snapshot

Round 3 endorsed the fix; taking the pre-existing divide-by-zero it
flagged (point 2), because THIS PR widens its window: the back-pressure
gauge reads remainingCapacity() and size() as separate calls, and the new
'fast' DisruptorBlockingQueue reports them as weakly-consistent ESTIMATES
(unlike ArrayBlockingQueue's exact counts), so a full-then-drained race
between the two reads can present remaining=0, size=0 -> ArithmeticException
on a live back-pressure path.

Extracted both duplicated inline forms into queueFullPercentage(queue):
each of remainingCapacity()/size() is read ONCE into a local (the old form
called each twice, compounding the skew) and the denominator is guarded
with Math.max(1, ...). Package-visible so the racy snapshot is testable.

Red-first: backPressureGaugeSurvivesTheRacyZeroDenominatorSnapshot drives
the gauge with a fake queue reporting 0/0 (the real TOCTOU cannot be
forced) - ArithmeticException without the guard, a sane 0-100 with it;
plus half-full=50 and full=100 sanity points.

Also took the nit (point 3): the concurrent-producers assertion message
now says 'must all execute' - a CountDownLatch detects loss, not
duplicates. Point 1 (capacity-1 green on disruptor 1.2.21) confirmed by
the local run; point 4 (findWorkerThread stack-dump) left as-is per the
review ('fine as-is for a regression test').

AsyncFastQueueShutdownUndoTest 4/4, AsyncTest and the helping/bail
backstops green.
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Code Review — PR #5081: map the fast async queue to the MPMC DisruptorBlockingQueue

Overall this is a strong, well-reasoned fix. The root-cause analysis is the right call: rather than papering over #5066 with a second drain pass, it identifies that PushPullBlockingQueue is an explicit SPSC design being used in an unambiguously multi-producer context, and swaps it for the MPMC variant from the same Conversant jar. No new dependency, remove(Object) now works on both shipped impls, and the previously "documented residual gap" is genuinely closed. The red-first tests and the twice-run verification (default + -Darcadedb.asyncOperationsQueueImpl=fast) are exactly what I'd want to see for a concurrency fix.

What's good

  • Correct diagnosis. The high-performance profile selects fast, so this was shipping a lost-task hazard on a non-obscure path, not an opt-in corner. The empirical multi-producer test (8 producers x 250 tasks, completion latch never reaching zero on the old impl) is convincing evidence, not just a theoretical argument.
  • No new dependency / license risk. com.conversantmedia:disruptor:1.2.21 is already a dependency (engine/pom.xml:123) and already listed in ATTRIBUTIONS.md:100 (Apache 2.0). Nothing to add.
  • queueFullPercentage(...) extraction is a nice cleanup: it reads remainingCapacity()/size() once each (the old inline form called each twice, and on the Disruptor queue those are weakly-consistent estimates), dedups two call sites, and the Math.max(1, ...) guard closes a real latent divide-by-zero. Good that it's package-visible and directly unit-tested with a fake queue rather than left to a non-deterministic race.
  • Comments are honest about tradeoffs - the O(n) whole-queue-locking remove(Object) is flagged as "must never migrate onto the steady-state path," and removeQuietly's catch is correctly demoted to defensive-only.

Minor points (non-blocking)

  1. queueFullPercentage(0, 0) returns 100, not 0. A racy remaining=0, size=0 snapshot is reported as "100% full," which drives the maximum back-pressure sleep for that iteration. The test only asserts isBetween(0, 100), so this passes, but over-throttling on an ambiguous/empty estimate is a slightly surprising direction. It's harmless given the rarity and self-correcting nature (next iteration re-reads), but a one-line Javadoc note that the 0/0 case biases toward "full" would prevent a future reader assuming it biases toward "empty."

  2. Effective-capacity semantics differ from standard. DisruptorBlockingQueue rounds capacity up to the next power of two, so remainingCapacity() reflects the rounded-up size, and the back-pressure threshold is computed against that rather than the exact configured ASYNC_OPERATIONS_QUEUE_SIZE / parallelLevel (as ArrayBlockingQueue does). This is not a regression - the old fast/PushPullBlockingQueue rounded identically - but fast and standard now trigger back-pressure at slightly different real fill levels for non-power-of-two sizes. Worth a sentence in the release note since it is operator-visible.

  3. Idle CPU cost. The release note already calls out that the Disruptor queue busy-spins before parking, so idle workers cost more CPU than standard. Good that it is documented; just flagging that with the high-performance profile spinning one thread per parallel slot, operators sizing many-core boxes should be aware. No action needed.

  4. findWorkerThread via Thread.getAllStackTraces() is a global scan keyed on the thread name AsyncExecutor-<db>-<slot>. It is fine here because the DB name is per-test, but it is a slightly fragile coupling to the naming scheme in the constructor - if that format ever changes these tests fail obscurely. A brief comment pointing at the super("AsyncExecutor-...") call site would help the next maintainer.

Test coverage

Coverage is thorough and appropriately red-first: remove(Object) support, multi-producer no-loss, capacity-1 offer/reject/reuse (nicely pinning the library-version-specific rounding so a future bump cannot silently regress it), and the 0/0 divide-by-zero guard. This is a good example of the "write the test first, prove it red on the old impl" approach in CLAUDE.md.

Nice work - the analysis quality here is well above a typical bug-fix PR. None of the above blocks merge.

…l-level note, thread-name coupling

Round 4 was another endorsement; the three doc-only minors taken (no code
behavior change):

1. queueFullPercentage javadoc now states the guarded 0/0 snapshot returns
   100 ('full'), an ambiguous estimate biasing toward MORE back-pressure
   for that one self-correcting iteration - so a future reader does not
   assume it biases toward 'empty'.
2. Release note: for a non-power-of-two ASYNC_OPERATIONS_QUEUE_SIZE /
   parallelLevel, 'fast' (Disruptor rounds up to a power of two) applies
   back-pressure at a slightly different real fill level than 'standard'
   (exact size) - operator-visible, not a regression.
3. findWorkerThread carries a comment pointing at the AsyncThread
   constructor's thread-name format it couples to.

Point 3 from the review (idle CPU on high-performance profile) needed no
action - already documented. AsyncFastQueueShutdownUndoTest 4/4.
@lvca
lvca merged commit 6a69a38 into main Jul 7, 2026
12 of 14 checks passed
@lvca
lvca deleted the fix/5066-fast-queue-shutdown-undo branch July 7, 2026 18:27
@codacy-production

codacy-production Bot commented Jul 7, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric Results
Complexity 0

View in Codacy

🟢 Coverage 83.33% diff coverage · -7.09% coverage variation

Metric Results
Coverage variation -7.09% coverage variation
Diff coverage 83.33% diff coverage

View coverage diff in Codacy

Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (f3fab92) 135373 100999 74.61%
Head commit (c4cd00b) 167657 (+32284) 113202 (+12203) 67.52% (-7.09%)

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 (#5081) 6 5 83.33%

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

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Code Review: fix(#5066) map the 'fast' async queue to DisruptorBlockingQueue

Overall this is a high-quality, well-reasoned fix. It correctly diagnoses the root cause one level deeper than the issue suggested: PushPullBlockingQueue is an explicit single-producer/single-consumer design, but every async worker queue here has many producers (application threads calling scheduleTask, cross-scheduling workers, completion markers, the closing thread). Swapping to DisruptorBlockingQueue (MPMC, same jar, no new dependency) both honors the multi-producer contract and provides the remove(Object) primitive the post-shutdown undo needs. The red-first tests and the double test run (default + -Darcadedb.asyncOperationsQueueImpl=fast) are exactly the kind of evidence I'd want to see.

Strengths

  • Root-cause fix, not a workaround. The reasoning for rejecting both options in the issue (a second drain pass cannot be total; producer-side remove-undo is the only timing-independent shape) is sound.
  • No new dependency. Same Conversant artifact, Apache 2.0, so no ATTRIBUTIONS/NOTICE churn required.
  • Strong test coverage. The multi-producer lost-task test (8 x 250), the capacity-1 pin, and the TOCTOU queueFullPercentage test all target real failure modes. Extracting queueFullPercentage to a package-visible static so the racy 0/0 snapshot can be tested deterministically via FixedSnapshotQueue is a clean approach.
  • Math.max(1, ...) guard genuinely closes a divide-by-zero that was reachable only on the weakly-consistent fast queue estimates. Good catch, well documented.
  • Import fully swapped, no dangling PushPullBlockingQueue reference in main; test helpers (AsyncTestTasks, recreateThreadsForTests) already exist.

Considerations (minor, mostly for the record)

  1. Idle CPU cost. DisruptorBlockingQueue busy-spins before parking, and high-performance selects fast by default with parallelLevel = CPU-1 workers each doing a timed poll(500ms). This is correctly documented in the release notes, and it is not a regression (PushPull also spun), but it is worth operators knowing that the fast profile trades idle CPU for latency.
  2. remove(Object) is an O(n) whole-queue-locking scan on the Disruptor queue, which briefly blocks producers on that queue. The inline NOTE (#5081 review) comment correctly bounds this to the rare dead-worker path (!target.isAlive()), so it never touches the steady-state schedule path. Good that this is called out explicitly to prevent future misuse.
  3. queueFullPercentage returns 100 on the guarded 0/0 snapshot, biasing toward one extra back-pressure sleep iteration. Benign and self-correcting on the next re-read, as the Javadoc notes.
  4. Capacity-1 behavior is library-version-specific. The fastQueueSupportsEffectiveCapacityOne test wisely pins offer/reject/remove/reuse semantics so a future Conversant bump that changes ring rounding or reserves a slot fails loudly here rather than silently regressing back-pressure.
  5. Back-pressure fill level shifts because Disruptor rounds capacity up to the next power of two while standard uses the exact ASYNC_OPERATIONS_QUEUE_SIZE / parallelLevel. Documented in the release notes; just a behavioral note for anyone tuning ASYNC_BACK_PRESSURE.

Nits

  • In fastQueueMustNotLoseTasksUnderConcurrentProducers, the t.join(30_000) return value is not checked; if a producer hung, the subsequent executed.await(30, SECONDS) would catch it anyway, so this is fine as-is.

No blocking concerns. The concurrency contract fix is correct, the change is minimal and well-scoped, and the tests are convincing. LGTM.

robfrank pushed a commit that referenced this pull request Aug 14, 2026
… cold path; CPU note

The review's one genuine pre-merge want, taken: capacity 1 is
production-reachable (ASYNC_OPERATIONS_QUEUE_SIZE / parallelLevel floors
to 1 on large pools) and the Conversant ring's minimum-size rounding is
library-version-specific, so the stated guarantee is now pinned by
fastQueueSupportsEffectiveCapacityOne - a parked-worker probe of the real
1-slot queue: offer, reject-when-full (not block/throw), remove(Object),
slot reuse. A future disruptor bump cannot silently regress the
post-shutdown undo primitive at the production floor.

Point 2 taken as the requested note at the removeQuietly call site:
remove(Object) on the Disruptor queue is an O(n) whole-queue-locking scan,
acceptable only on the rare dead-worker post-shutdown race - it must never
migrate onto the steady-state scheduling path.

Nit taken: the release note now states the fast-profile CPU tradeoff
(busy-spin before park, same as the previous implementation) for
operators. The thread-name coupling, ignored join return and no-slow-tag
observations are the reviewer's own verifications, kept as-is.

AsyncFastQueueShutdownUndoTest 3/3 plus the async battery under
-Darcadedb.asyncOperationsQueueImpl=fast, all green.

(cherry picked from commit 6580d19)
robfrank pushed a commit that referenced this pull request Aug 14, 2026
…methods

Round 2 fully closed out the round-1 asks with no correctness issues; the
lone cosmetic nit taken: the capacity-1 test I added had landed after the
private findWorkerThread helper without a separating blank line. Moved it
up with the other @test methods and left the helper last, matching the
class layout convention. Test-only reordering, no behavior change.

AsyncFastQueueShutdownUndoTest 3/3.

(cherry picked from commit e82693e)
robfrank pushed a commit that referenced this pull request Aug 14, 2026
…akly-consistent 0/0 snapshot

Round 3 endorsed the fix; taking the pre-existing divide-by-zero it
flagged (point 2), because THIS PR widens its window: the back-pressure
gauge reads remainingCapacity() and size() as separate calls, and the new
'fast' DisruptorBlockingQueue reports them as weakly-consistent ESTIMATES
(unlike ArrayBlockingQueue's exact counts), so a full-then-drained race
between the two reads can present remaining=0, size=0 -> ArithmeticException
on a live back-pressure path.

Extracted both duplicated inline forms into queueFullPercentage(queue):
each of remainingCapacity()/size() is read ONCE into a local (the old form
called each twice, compounding the skew) and the denominator is guarded
with Math.max(1, ...). Package-visible so the racy snapshot is testable.

Red-first: backPressureGaugeSurvivesTheRacyZeroDenominatorSnapshot drives
the gauge with a fake queue reporting 0/0 (the real TOCTOU cannot be
forced) - ArithmeticException without the guard, a sane 0-100 with it;
plus half-full=50 and full=100 sanity points.

Also took the nit (point 3): the concurrent-producers assertion message
now says 'must all execute' - a CountDownLatch detects loss, not
duplicates. Point 1 (capacity-1 green on disruptor 1.2.21) confirmed by
the local run; point 4 (findWorkerThread stack-dump) left as-is per the
review ('fine as-is for a regression test').

AsyncFastQueueShutdownUndoTest 4/4, AsyncTest and the helping/bail
backstops green.

(cherry picked from commit 248acdd)
robfrank pushed a commit that referenced this pull request Aug 14, 2026
…l-level note, thread-name coupling

Round 4 was another endorsement; the three doc-only minors taken (no code
behavior change):

1. queueFullPercentage javadoc now states the guarded 0/0 snapshot returns
   100 ('full'), an ambiguous estimate biasing toward MORE back-pressure
   for that one self-correcting iteration - so a future reader does not
   assume it biases toward 'empty'.
2. Release note: for a non-power-of-two ASYNC_OPERATIONS_QUEUE_SIZE /
   parallelLevel, 'fast' (Disruptor rounds up to a power of two) applies
   back-pressure at a slightly different real fill level than 'standard'
   (exact size) - operator-visible, not a regression.
3. findWorkerThread carries a comment pointing at the AsyncThread
   constructor's thread-name format it couples to.

Point 3 from the review (idle CPU on high-performance profile) needed no
action - already documented. AsyncFastQueueShutdownUndoTest 4/4.

(cherry picked from commit c4cd00b)
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.

[engine] Async post-shutdown task undo is best-effort with asyncOperationsQueueImpl=fast (Conversant queue lacks remove)

1 participant