fix(#6264): fourteen more algo.* iteration knobs get the minimum and the checkpoint #6216 gave three

[lvca]

Closes #6264. Follow-up to #6216 (PR #6222).

#6216 established that an iteration-shaped knob needs two things - a domain minimum rejected by name, and a checkpoint inside the loop it drives - and gave both to the three procedures its parent review had named. Thirteen more carried the identical defect, untouched. Every one extracted its knob with a plain extractInt(n, "maxIterations") and contained zero guard. calls:

algo.pageRank, algo.personalizedPageRank, algo.articleRank, algo.eigenvector, algo.hits, algo.katz, algo.louvain, algo.leiden, algo.labelPropagation, algo.slpa, algo.simRank, algo.fastrp, algo.hashgnn.

Domain

CALL algo.pageRank({maxIterations: 0}) returned the uniform initial rank vector as though it were a PageRank result; algo.louvain returned every node in its own community; algo.fastrp the untouched random projection. The silent half is the more serious one, as the issue says: an un-iterated centrality is not obviously wrong to a caller, unlike an exception. All thirteen now reject below 1, naming the procedure, the parameter and the value. maxIterations with a minimum of 1 also matches Neo4j GDS, where it is declared @Configuration.IntegerRange(from = 1).

Time

No honest ceiling exists for time, so a large value stays legal and becomes abortable instead. Each knob-driven loop calls the shared WorkGuard (thread interrupt + arcadedb.command.timeout), and a per-node checkpoint inside each pass bounds the abort latency below a whole graph sweep. Following the rule #6222 arrived at over three review cycles - throttle only where one iteration can be cheaper than the check itself - the per-node checkpoint is checkPeriodically where a node's own work is O(deg) or O(dim), and unthrottled where it is already O(n) (Louvain's getCommunityDegree scan, SimRank's inner v loop).

Five of the thirteen have no convergence test at all - the CSR PageRank kernel, simRank, fastrp, hashgnn, slpa - so the knob alone decided when they stopped.

The two CSR delegates

algo.pageRank and algo.labelPropagation hand a CSR-backed graph to GraphAlgorithms, which sits below the query layer and knew nothing about deadlines. Rather than couple the OLAP kernels to the query engine, both gained an overload taking the new WorkCheckpoint - a one-method interface in com.arcadedb.graph.olap that the procedures satisfy with guard::check. Existing callers are unchanged and get WorkCheckpoint.NONE.

Heap: algo.slpa

Alone among the thirteen, SLPA's iterations buys memory as well as time: every node keeps a label-memory row of iterations + 1 ints, so {iterations: 1000000} on a 10k-node graph asks for 40 GB, and at Integer.MAX_VALUE the iterations + 1 wrapped to Integer.MIN_VALUE and died as a bare NegativeArraySizeException naming nothing. The footprint is now estimated in saturating long arithmetic and checked against the same arcadedb.cypher.algoMaxWalkMemory budget the walk buffers use, before the first row is allocated. checkWalkBudget() becomes the walk-shaped special case of a new checkBufferBudget().

An alternative better than what the issue proposed

The issue framed this as "mechanical application" of #6216's mechanism. Two places it is not:

• The CSR delegates. A mechanical pass would have added a checkpoint to the twelve inline loops and left algo.pageRank's CSR path - the single worst offender, since that kernel never converges - unguarded, because the loop is in another package. WorkCheckpoint fixes it without inverting the layering.
• SLPA's heap. A mechanical pass would have given iterations a minimum and a time checkpoint and left {iterations: 1000000} as a 40 GB allocation. Unbounded CPU-shaped config knobs in the OpenCypher algo procedures (follow-up to #6065) #6216's actual principle is bound a knob by the resource it spends, and this knob spends two.

Tests

New Issue6264AlgoIterationKnobGuardTest, 62 cases:

• 13 parameterised zero-rejections, one per procedure, plus 3 negative-count rejections
• 13 over-reach cases running each knob at exactly its minimum and asserting a full result set
• 13 interrupt aborts (deterministic: the flag is armed before the call, so the first checkpoint must observe it) and 11 command-deadline aborts. The two omitted from the deadline list, louvain and leiden, settle on the fixture before the clock is read - noted in the test rather than papered over
• simRankHonoursTheCommandTimeoutInsideASingleIteration: maxIterations: 1 on an 800-node degree-400 graph, so the outer checkpoint runs once before any work and only the per-node checkpoint can fire. Per fix(#6216): a graph-algorithm knob is bounded by the resource it spends, not by a guessed cap #6222's latency-test trap, "it throws" is non-vacuous here (the unguarded version returns a similarity rather than throwing) and the elapsed-time bound is measured from both sides: ~1.5 s guarded, ~29 s unguarded
• 3 direct tests on the CSR kernels (checkpoint called once per iteration; abort propagates), plus one end-to-end that leaves tolerance at its default so that only the CSR path - which ignores tolerance - can still be running when the deadline hits
• 3 SLPA heap cases: over budget, past Integer.MAX_VALUE with the budget disabled, and an under-budget over-reach guard

Sensitivity, measured against the unmodified sources (git show HEAD:<file> for the 13 procedures only): 20 of 35 non-abort cases go red, and the 24 abort cases hang rather than fail, which is the defect itself.

Full runs: 562 green across com.arcadedb.query.opencypher.procedures.algo.* + com.arcadedb.graph.olap.*; 4963 green across all com.arcadedb.query.opencypher.** + com.arcadedb.graph.**.

Follow-ups spotted, not in scope

• AlgoSLPA.mostFrequent() is O(len^2) in the listener's degree, so a supernode makes one round quadratic in a way no knob controls. Graph-driven rather than knob-driven, so it belongs with arcadedb.command.timeout is described as the timeout for commands but is honoured by only two code paths #6266's family rather than here.
• algo.simRank allocates new double[n][n] twice - node-count-driven and outside every budget, the same shape as Embedding matrices in the algo.* procedures are sized nodeCount x dimension and sit outside every budget (follow-up to #6216) #6263.

🤖 Generated with Claude Code

https://claude.ai/code/session_016yjKJXMNMQeLPKdCno7cFX

[mergify]

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

• Queue this pull request

[codacy-production]

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]

Review

Read through the full diff plus the current state of WorkGuard/WorkCheckpoint, AbstractAlgoProcedure, and every touched algo.* procedure. This is careful, well-scoped work; the checkpoint-granularity choices are correctly derived from the actual per-node complexity of each kernel rather than applied mechanically, and the test suite is unusually rigorous (sensitivity-measured against the unmodified sources, interrupt + deadline + in-pass abort all covered, over-reach guards at the minimum boundary).

Correctness spot-checks (all held up)

• Louvain vs. Leiden checkpoint throttling: Louvain uses unthrottled guard.check() per node because getCommunityDegree() rescans all n nodes per candidate community (confirmed - node cost is O(deg × n)), while Leiden uses checkPeriodically(i) because its per-candidate cost is an O(1) array lookup (communityDegree[candidateComm]). The distinction in the PR description is accurate, not just asserted.
• SimRank's outer u loop is unthrottled for the same reason (O(n² × deg²) total, so per-u checking is already a 1/n fraction of the work) - checked against the actual sim[a][b] double loop, correct.
• The two GraphAlgorithms overloads (pageRank, labelPropagation) only checkpoint once per outer iteration, not per-node - coarser than the inline OLTP/CSR paths in the procedures themselves. This looked at first like a gap, but it's justified: those kernels parallelize the per-node work internally via parallelForRange, which submits to QueryEngineManager's dedicated executor (not the common pool, so it's consistent with the engine-concurrency rule), and pushing Thread.interrupted() checks into worker-thread closures wouldn't reliably observe the calling thread's interrupt anyway. Worth a one-line callout in the javadoc that the coarser granularity is why abort latency there is "one full sweep" rather than "~1024 nodes," since it's a real (if minor) asymmetry with the other eleven procedures.
• SLPA's heap accounting: rowCapacity = iterations + 1L computed in long, saturatingProduct used at both multiplication steps, the Integer.MAX_VALUE guard sits after the budget check - verified the numbers in the test (4 nodes × 1000001 ints ≈ 15.3MB against a 1MB budget) actually produce a rejection.

One gap in scope

AlgoGraphSAGE.layers (AlgoGraphSAGE.java:98) has the identical defect this PR fixes everywhere else: extracted with a bare extractInt(n, "layers") (no minimum), and the for (int layer = 0; layer < layers; layer++) loop has no WorkGuard/checkpoint at all - not even the coarse one. {layers: 0} silently returns the untouched random-Gaussian initial features as if they were trained embeddings (same "silent half" shape as the fastrp/hashgnn bug described in the PR), and {layers: 2000000000} has nothing to interrupt or time out. It sits in the same package, right next to AlgoFastRP/AlgoHashGNN which got fixed, and wasn't mentioned in the "Follow-ups spotted, not in scope" section. Given the PR's own framing ("thirteen more carried the identical defect, untouched"), this looks like a fourteenth that was missed rather than a deliberate exclusion - worth a fast follow-up (or folding in here, since the fix shape is now copy-paste from AlgoFastRP).

Minor

• In AlgoSLPA, when a finite CYPHER_ALGO_MAX_WALK_MEMORY budget is configured and iterations is huge enough to overflow int, checkBufferBudget will always fire first (its saturated estimate trivially exceeds any finite budget), so the more specific "more than a Java array can hold" message only ever surfaces when the budget is disabled (as the test correctly sets up). Not a bug, just means the array-capacity message is effectively dead code under default configuration - fine as defense in depth, not worth changing.
• The per-procedure comments ("maxIterations is a caller-supplied knob and the tolerance break only fires if the graph converges...") are duplicated near-verbatim across ~10 files. Consistent and each one is locally justified by the surrounding loop shape, so this reads as intentional rather than copy-paste drift.

Style/process

Follows the repo conventions well: @Tag("slow") correctly applied only to the one genuinely slow SimRank in-pass test, StallAwareStopwatch.assertStayedUnder used instead of a raw wall-clock assertion, assertThat(...).isTrue()-style AssertJ throughout, no System.out debug left behind, and the interrupt-flag cleanup in @AfterEach is a nice touch for isolating tests from each other.

Nothing here blocks merge; the GraphSAGE gap is the only actionable item.

[lvca]

Review actioned.

The GraphSAGE gap — valid, folded in (commit fa5304d). You are right that it is a fourteenth of the same thing, not a deliberate exclusion: AlgoGraphSAGE.layers is a bare extractInt driving a checkpoint-free loop, and {layers: 0} returns the random-Gaussian init as though it were a trained embedding. Fixed the same way as fastrp/hashgnn, plus four new parameterised cases (zero rejection, minimum accepted, interrupt abort, deadline abort). The zero-rejection case is red against the unmodified AlgoGraphSAGE, green with the fix.

One judgement call inside it worth stating: the Xavier initialisation in the layer loop gets no checkpoint of its own. Both its bounds are embedding dimensions, capped at MAX_EMBEDDING_DIMENSION, so it is bounded work whatever the caller asks for — unlike the per-node loop after it. Said so in a comment rather than leaving it to look like an omission.

The javadoc callout on the granularity asymmetry — valid, added. Both GraphAlgorithms overloads now state why one sweep is as fine as those kernels can be: the per-node work runs on parallelForRange worker threads, which would not observe an interrupt aimed at the calling thread, and throwing out of a chunk closure would leave its siblings running.

Counts corrected throughout (title, release notes, test javadoc): fourteen procedures, six of them with no convergence test at all.

The two minor points — agreed, no change. Your own reading is right on both: the SLPA array-capacity message is unreachable under a finite budget and is defence in depth, and the near-verbatim per-loop comments are intentional, since each is locally justified by the loop it sits above and a reader arriving at one file should not have to find another to know why the checkpoint is there.

Tests after the change: 119 green across Issue6264AlgoIterationKnobGuardTest (66), AlgoGraphSAGETest, Issue6216AlgoWorkKnobBoundsTest and GraphAlgorithmsTest.

[claude]

Review of #6288

I read through the full diff (20 files, ~780 additions) plus the current merged state of the affected sources (AbstractAlgoProcedure, WorkGuard, WorkCheckpoint, GraphAlgorithms, and all fourteen algo.* procedures). Overall this is a solid, disciplined follow-up to #6216/#6222 - it mechanically (and in two cases, non-mechanically and correctly so) extends an established pattern rather than inventing a new one per procedure.

Code quality

• extractInt(value, name, minimum), WorkGuard/checkPeriodically, and checkBufferBudget are all reused as-is from the infrastructure Unbounded CPU-shaped config knobs in the OpenCypher algo procedures (follow-up to #6065) #6216 already built - no duplicated logic, matching the "reuse existing components" guidance in CLAUDE.md.
• The WorkCheckpoint interface is a clean, minimal way to let com.arcadedb.graph.olap (below the query layer) stay free of a hard dependency on com.arcadedb.query WorkGuard, while still getting a cooperative abort hook via guard::check. The layering rationale is spelled out directly in the interface javadoc, which is helpful for future maintainers.
• Checkpoint placement is thoughtful, not copy-pasted blindly:
  • AlgoLouvain and AlgoLeiden local-move loops call guard.check() UNTHROTTLED per node because getCommunityDegree/community-weight lookups are themselves O(n) or O(deg) - throttling to every 1024 nodes there would have left abort latency proportional to whole-graph scans. Good catch, correctly reasoned in the inline comments.
  • AlgoSimRank outer u loop is also unthrottled for a symmetric reason (O(n^2 x deg^2) work per outer step).
  • GraphAlgorithms.pageRank/labelPropagation checkpoints stay on the calling thread between parallelForRange phases rather than inside worker-thread closures - correctly avoids the two failure modes called out in the javadoc (interrupt not observed on worker threads, and an exception thrown from one chunk leaving sibling chunks running).
• AlgoSLPA heap-budget check (checkBufferBudget before the first int[] row is allocated, iterations + 1 computed in long to avoid the Integer.MAX_VALUE wraparound) is a genuinely different failure mode than the other 13 procedures, and it is correctly handled as a separate case rather than shoehorned into the same time-only guard.

Bugs
No correctness bugs found in the checkpoint/minimum-validation logic itself. A couple of small things worth a second look, neither blocking:

• In AlgoSLPA, checkBufferBudget(...) runs before the rowCapacity > Integer.MAX_VALUE check. That ordering is fine functionally (the budget check uses saturating long arithmetic so it cannot itself overflow), but it means that with the default budget (which auto-scales with heap and is rarely negative) the NegativeArraySizeException-shaped case is normally caught by the budget message rather than the "more than a Java array can hold" message. That path is only reachable when a user explicitly sets CYPHER_ALGO_MAX_WALK_MEMORY negative (as the regression test does), so this is a minor overlap in error messages rather than a bug.
• AlgoLouvain per-node guard.check() (unthrottled) calls System.currentTimeMillis() once per node once a deadline is configured - for a large graph with many maxIterations, that is O(n x iterations) timer reads. Given getCommunityDegree is already O(n) per candidate community per node, this is dwarfed by existing algorithmic cost, so it is the right call, just flagging that it is a different cost profile than the O(1)-per-1024 throttled sites elsewhere in the same package.

Performance

• The Arrays.fill(newSim[i], 0.0) reset in AlgoSimRank (replacing an O(n^2) element-by-element loop with per-row Arrays.fill) is a real, low-risk win - Arrays.fill is intrinsic-optimized and this runs every iteration.
• No new allocations were introduced inside hot loops beyond what already existed; the checkpoint calls themselves are O(1) (a Thread.interrupted() check plus, only when a deadline is configured, one System.currentTimeMillis()).

Security
Nothing security-sensitive here - this is resource-exhaustion hardening (unbounded CPU time and, for SLPA, unbounded heap) triggered only by parameters the caller already fully controls. No new external input parsing, no injection surface.

Test coverage
The new Issue6264AlgoIterationKnobGuardTest (62 cases) is thorough and well-targeted:

• Zero/negative rejection and minimum-acceptance are parameterized across all 14 procedures (13 named in the issue + algo.graphsage, correctly folded in during review since it has the identical defect shape).
• The interrupt-abort test is deterministic (flag armed before the call), and the command-timeout test correctly excludes louvain/leiden with a documented reason (they converge before the clock is read on the 4-node fixture) rather than silently omitting them.
• simRankHonoursTheCommandTimeoutInsideASingleIteration is the standout test - it specifically isolates the per-node checkpoint from the outer per-iteration checkpoint (maxIterations: 1 so the outer checkpoint only fires once, before any work), and asserts on elapsed time using StallAwareStopwatch.assertStayedUnder per the repo own CLAUDE.md guidance rather than a raw wall-clock assertion. It is correctly tagged @tag("slow").
• Direct kernel-level tests (thePageRankKernelCallsTheCheckpointOncePerIteration, ...PropagatesAnAbortFromTheCheckpoint) verify the WorkCheckpoint contract independently of the Cypher layer, which is good isolation.
• The PR description sensitivity analysis (20/35 non-abort cases red, 24/24 abort cases hanging against the unmodified sources) is a nice touch that gives confidence the tests are not vacuously passing.

One minor gap: no test asserting that algo.simRank Arrays.fill refactor produces bit-identical results to the old element-by-element reset (it is mentioned only in the commit message, not the issue). pageRankStillConvergesToTheRightAnswerWithTheCheckpointInPlace covers the same "did the algorithm output change" concern for PageRank, but there is no equivalent test for SimRank buffer-reset change specifically - likely low risk since Arrays.fill(x, 0.0) followed by x[i]=1.0 is behaviorally identical to the loop it replaces, but worth a quick sanity check if not already covered elsewhere.

Overall this closes a real gap (silent wrong answers from {maxIterations: 0}-shaped calls, plus genuinely unbounded CPU/heap) across the entire algo.* surface, and the two departures from the issue "mechanical application" framing (the WorkCheckpoint interface for the CSR kernels, and SLPA heap accounting) are both justified rather than convenient.

[lvca]

Round 2 actioned — thanks, the SimRank gap was a fair catch.

The one suggestion — added (e6a92d3). You're right that the Arrays.fill reset is the only place this PR changes what a kernel computes rather than only when it stops, and that nothing pinned its result. Note the four-node cycle cannot carry that assertion: every SimRank of two distinct nodes there is 0, so a broken reset would go unnoticed. The new test builds a hub pointing at two leaves instead, where the leaves share their only in-neighbour and sim(A, B) = decay × sim(hub, hub) = 0.8.

It asserts at 1 and 3 iterations, which is what makes it non-vacuous: the reset exists only for the diagonal (the u < v loop writes every off-diagonal cell itself) and the buffers swap each round, so dropping it leaves sim(hub, hub) at 0 from the second iteration on. Measured rather than assumed — with newSim[i][i] = 1.0 removed the run still returns 0.8 at 1 iteration and fails at 3.

The two "worth a second look" items — no change, and I think your own reading of both is right.

• SLPA check ordering. The array-capacity message being shadowed by the budget message under a default budget is the intended precedence, not an accident: the budget is the bound an operator can tune and its message names the setting to raise, so it should win when both apply. The capacity refusal exists for the one case the budget cannot cover — algoMaxWalkMemory explicitly accepts "negative = no limit", and 2^31 rows do not fit a Java array whatever the heap is.
• Louvain's unthrottled check(). Agreed it is a different cost profile, and deliberately so. getCommunityDegree() is O(n) per candidate community per node, so the per-node body is already Ω(n) — one currentTimeMillis() against that is the cheap side of the trade, and throttling to 1024 would make the abort latency 1024 whole-graph scans. Same reasoning as SimRank's u loop; both carry it in an inline comment so the asymmetry with the throttled sites reads as a choice.

Suite after the change: 634 green across com.arcadedb.query.opencypher.procedures.** and com.arcadedb.graph.olap.**.

[claude]

Review

I read through the full diff (14 procedures + GraphAlgorithms, WorkCheckpoint, AbstractAlgoProcedure, and the 495-line regression test) plus the untouched surrounding code in each file. Overall this is a well-executed, mechanical-but-not-lazy application of the pattern #6216 established, and the test suite is unusually thorough. A few notes below, nothing blocking.

Code quality / correctness

• The guard.check() vs guard.checkPeriodically(i) placement is judged per-procedure rather than templated, and the judgment is correct in the cases I checked most carefully:
  • AlgoLouvain's per-node loop uses unthrottled guard.check() because getCommunityDegree() is an O(n) scan per candidate community, so one node's own work already dominates a flag test - throttling to every 1024 nodes here would genuinely blow up abort latency to ~1024 whole-graph scans, as the comment says.
  • AlgoSimRank's u loop is likewise unthrottled because a single outer iteration is O(n²·deg²); n flag tests are a 1/n fraction of that regardless of density.
  • Everywhere else (PageRank, HITS, Katz, Eigenvector, LabelPropagation, FastRP, HashGNN, GraphSAGE, SLPA) the per-node work is O(deg) or O(dim), so the throttled checkPeriodically (mask 1023) is the right call.
• GraphAlgorithms.pageRank/labelPropagation gaining a WorkCheckpoint-taking overload rather than pulling WorkGuard/CommandContext down into com.arcadedb.graph.olap is the right layering call, and existing callers are untouched (verified there are only two call sites for each, both updated).
• SLPA's heap-budget check is genuinely necessary and correctly ordered: checkBufferBudget (the common, budget-enabled path) runs before the rowCapacity > Integer.MAX_VALUE fallback (the budget-disabled path), matching the two dedicated tests. iterations + 1L is done in long specifically to avoid the Integer.MAX_VALUE wraparound that used to throw a nameless NegativeArraySizeException - confirmed by slpaRejectsMoreLabelEntriesThanAJavaArrayCanHoldEvenWithTheBudgetDisabled.
• extractInt(value, name, minimum) and checkBufferBudget/checkWalkBudget are clean, minimal generalizations of what Unbounded CPU-shaped config knobs in the OpenCypher algo procedures (follow-up to #6065) #6216 already had - no gratuitous refactor.
• The SimRank matrix-reset change (Arrays.fill + diagonal instead of an O(n²) element-by-element write) is a real, correctly-tested behavior-preserving optimization, and the PR is honest that it's the one place semantics rather than only timing changed.

Minor/non-blocking observations

• checkBufferBudget's saturatingProduct(rowCapacity, WALK_ENTRY_BYTES) + WALK_ROW_OVERHEAD_BYTES (AlgoSLPA.java:127) adds WALK_ROW_OVERHEAD_BYTES after the saturating multiply, without saturating the addition itself. Not exploitable today - rowCapacity is int-bounded (≤ ~2.1e9) so the product can't get anywhere near Long.MAX_VALUE - but if checkBufferBudget is ever reused for a knob whose per-entry byte size is large enough to make saturatingProduct return Long.MAX_VALUE, the +32 would silently wrap to a large negative number and defeat the budget check entirely (a negative estimatedBytes always passes estimatedBytes <= budget). Might be worth a saturatingAdd or just clamping, cheaply, for future callers.
• The unthrottled guard.check() calls in AlgoLouvain's and AlgoSimRank's inner loops read System.currentTimeMillis() once per node when a deadline is configured - negligible next to the O(n) / O(deg²) work per node, but worth knowing it's not free the way the throttled path is.
• docs/release-26.9.1.md is a nice touch for discoverability, and its "six of the fourteen have no convergence test" list matches the code.

Test coverage

Excellent. The 62-case matrix (zero/negative rejection × 14, minimum-acceptance × 14, interrupt-abort × 14, deadline-abort × 12, plus targeted CSR-kernel and SLPA-heap tests) is exactly the right shape, and a few details stand out as careful rather than boilerplate:

• simRankHonoursTheCommandTimeoutInsideASingleIteration specifically isolates the per-node checkpoint from the outer one by setting maxIterations: 1, and uses StallAwareStopwatch.assertStayedUnder per the repo's guidance against raw wall-clock assertions.
• Omitting louvain/leiden from the deadline-test list with a documented reason (they converge too fast on the 4-node fixture to exercise the deadline) rather than papering over it with a bigger fixture is a good call given the interrupt test already covers the same checkpoint.
• simRankStillComputesTheSameSimilarityAfterTheMatrixResetRefactor correctly picks a hub-and-two-leaves fixture instead of the shared 4-node cycle specifically because the cycle can't distinguish a correct vs. broken diagonal reset - and asserts at 1 and 3 iterations, which is what actually catches the bug (the buffers swap, so a broken reset only surfaces from the second iteration on).
• The PR description's sensitivity claim (20/35 non-abort cases red, 24/24 abort cases hang against the unmodified sources) is a good practice to see documented, even though I didn't independently re-verify it.

No functional issues found. Nice work.

[lvca]

Round 3 actioned (de3340b).

The non-saturating addition — valid, fixed. You're right that saturatingProduct(...) + WALK_ROW_OVERHEAD_BYTES defeats itself in the one case it matters: a saturated product plus +32 wraps to a large negative number, and a negative estimate satisfies estimatedBytes <= budget for any budget — the check silently disabled by exactly the input it exists to refuse. Added saturatingSum() as the companion to saturatingProduct() and used it there.

I also applied it to AlgoNode2Vec's bytesPerWalk, which has the identical raw-arithmetic shape and the identical proof that it cannot overflow today. Not because it is reachable, but so that a reader does not have to redo that proof per call site to work out which ones are safe — the value of the fix is entirely in closing the shape, and one site left in the old form is what makes the shape ambiguous again. Both carry a comment saying they are bounded today and why they are written this way anyway.

The currentTimeMillis() cost note — acknowledged, no change, for the reason you give: it is dwarfed by the O(n) per-node work at both unthrottled sites. Flagged in the inline comments already so the asymmetry with the throttled sites reads as deliberate.

Suite: 634 green across com.arcadedb.query.opencypher.procedures.** and com.arcadedb.graph.olap.**, unchanged by this commit.

[claude]

Review

I read through the full diff (14 procedure files, GraphAlgorithms.java, the new WorkCheckpoint interface, AbstractAlgoProcedure.java, and the 495-line test class). Overall this is a solid, mechanically consistent fix that closes a real defect class cleanly. A few notes:

Code quality / correctness

• The unthrottled guard.check() vs. throttled guard.checkPeriodically(i) choice is applied correctly and matches actual per-node complexity, not just copy-pasted: AlgoLouvain's per-node loop calls getCommunityDegree(), an O(n) scan per candidate community, so it rightly gets an unthrottled check every node. AlgoLeiden's equivalent loop uses an O(1) communityDegree[] array lookup instead, so the throttled checkPeriodically there is correct too - nice attention to detail rather than a blanket rule.
• AlgoSLPA's heap-budget check order is right: checkBufferBudget runs first (a no-op when the budget is disabled via a negative value), then the rowCapacity > Integer.MAX_VALUE check catches the case the budget can't (budget disabled + iterations at Integer.MAX_VALUE). The iterations + 1L widening avoids the exact NegativeArraySizeException this PR is trying to kill.
• The WorkCheckpoint interface is a clean way to let GraphAlgorithms (below the query layer) stay decoupled from the query engine's WorkGuard, and the javadoc on the two new overloads honestly documents why the abort latency there is coarser (one sweep) than the inline OLTP loops (~1024 nodes) - the parallelForRange worker-thread reasoning is correct and worth having in writing.
• saturatingSum closing the "saturated product + plain addition wraps to negative, and a negative estimate passes the budget check unconditionally" gap is a good catch, and applying the same shape to AlgoNode2Vec's bytesPerWalk for consistency (even though it can't overflow today) is reasonable defensive-by-shape practice rather than defensive-by-instance.

Minor / stylistic

• AlgoPersonalizedPageRank's OLTP-path guard.check()/guard.checkPeriodically(i) calls (lines ~924, ~932) lack the explanatory comment that every other procedure's checkpoint carries. Not a bug, just a small consistency gap given how deliberately commented the rest of the PR is.
• The PR description itself flags two intentionally-deferred follow-ups (AlgoSLPA.mostFrequent() O(len²), SimRank's new double[n][n] double allocation) - good that these are called out explicitly rather than silently left for later discovery.

Test coverage

Very thorough: zero/negative rejection, minimum-accepted over-reach guard, interrupt abort, deadline abort, and CSR-kernel-specific direct tests, all parameterized across the fourteen procedures. Two things stood out as good practice:

• The SimRank matrix-reset refactor (element-by-element i==j?1.0:0.0 → Arrays.fill + diagonal write) gets its own value-pinning regression test on a fixture (hub + two leaves) specifically chosen because the existing four-node-cycle fixture couldn't distinguish a correct reset from a broken one (all cross-node SimRank values are 0 there). Good awareness of test vacuity.
• simRankHonoursTheCommandTimeoutInsideASingleIteration uses StallAwareStopwatch.assertStayedUnder per the repo's stall-aware timing convention rather than a raw wall-clock assertion, and is correctly tagged @Tag("slow").
• The PR description's "sensitivity" note (20/35 non-abort cases red, 24/24 abort cases hang against unmodified sources) is a good way to demonstrate the tests actually exercise the defect rather than just exercising the fix.

One coverage gap worth a sentence in the class javadoc if not already there: the negative-count test (anIterationKnobRejectsANegativeCount) only samples 3 of the 14 procedures rather than all 14. Given extractInt(..., minimum) is shared code, that's a defensible sampling choice, but explicitly noting "shared validation, sampled rather than exhaustive" would save a future reader from wondering if the other 11 were overlooked.

Security / performance

No concerns. The added checks are cheap (an AND + branch for the throttled path, a Thread.interrupted() + currentTimeMillis() compare for the outer one) relative to the O(n)/O(n+m) work they guard, and nothing here touches user-facing input handling beyond validation that was already missing.

Nice follow-through on #6216's pattern, including catching the algo.graphsage fourteenth instance and the SLPA heap dimension that a purely mechanical pass would have missed.

[lvca]

Round 4 actioned (8037429) — both points valid, both comment-only, no behaviour change.

The uncommented checkpoints. You spotted the real cause of the inconsistency: AlgoPersonalizedPageRank and AlgoArticleRank are the only two whose OLTP path duplicates the CSR path's loop, and only the CSR copy carried the note. In a PR where every other checkpoint explains itself, the OLTP copy read like an unexplained call. Both now say it, and so does the fallback branch inside the CSR path.

The negative-count sampling. Written down rather than left to be re-derived: what separates a negative value from zero lives entirely in the shared extractInt(value, name, minimum), and the per-procedure half — that the knob is extracted with a minimum at all — is what the zero case covers for all fourteen. Fourteen rows there would re-test one shared comparison fourteen times, which is the argument for sampling rather than an excuse for it.

634 green across com.arcadedb.query.opencypher.procedures.** and com.arcadedb.graph.olap.**, unchanged.

From my side this is complete: four review rounds, every actionable point either implemented (GraphSAGE as a fourteenth instance, the SimRank value-pinning test, saturatingSum, these comments) or declined with a reason the review itself agreed with. Ready to merge as far as I'm concerned.

[claude]

Review of #6288

I read through the full diff (GraphAlgorithms, WorkCheckpoint, AbstractAlgoProcedure, all 14 modified algo.* procedures, and the new Issue6264AlgoIterationKnobGuardTest) plus the current state of AbstractAlgoProcedure.WorkGuard/extractInt. Overall this is a solid, mechanically consistent follow-up to #6216 - the pattern (extractInt(n, name, 1) for the domain minimum + guard.check()/checkPeriodically() for the abort path) is applied uniformly across all fourteen procedures, and the two harder cases are handled thoughtfully:

Correctness spot-checks that passed:

• WorkGuard.check() only reads System.currentTimeMillis() when a deadline is actually configured (deadline < Long.MAX_VALUE), so the no-timeout default stays one cheap Thread.interrupted() per iteration - good attention to the performance mandate in this repo's guidelines.
• The WorkCheckpoint overloads on GraphAlgorithms.pageRank/labelPropagation correctly call the checkpoint from the calling thread between parallelForRange phases rather than from worker threads - avoids the half-finished-siblings problem a throw from inside a parallel chunk would create.
• AlgoSLPA's heap-budget check runs before the int-overflow guard (rowCapacity > Integer.MAX_VALUE), and checkBufferBudget's saturating arithmetic (saturatingProduct + new saturatingSum) correctly avoids the wrap-to-negative case that would otherwise silently disable the budget check - a negative estimate satisfies estimatedBytes <= budget for any budget, so this is a real edge case worth closing even though no current caller can reach it.
• Louvain's unthrottled per-node guard.check() (vs. the checkPeriodically throttling used elsewhere) is justified by getCommunityDegree()'s O(n) scan per candidate community - traced through the loop and the O(deg x n) claim in the comment checks out.
• SimRank's per-node Arrays.fill + diagonal write is a faithful refactor of the old element-by-element reset, and the new simRankStillComputesTheSameSimilarityAfterTheMatrixResetRefactor test is a genuine pinning test, correctly built on a hub/two-leaves fixture rather than the four-node cycle where the old bug would have been invisible.
• The zero/negative-knob rejection message format matches extractInt(value, name, minimum) exactly, and the CSR vs. OLTP dispatch in AlgoPageRank/AlgoArticleRank/AlgoPersonalizedPageRank/AlgoLabelPropagation extracts and validates maxIterations once before branching, so there is no risk of the two paths validating differently.

Minor, non-blocking observations:

1. Comment duplication: the rationale comment ("maxIterations is a caller-supplied knob and the tolerance break only fires if the graph converges, so the outer loop carries the checkpoint...") is copy-pasted near-verbatim into at least 5-6 procedures (AlgoArticleRank, AlgoHITS, AlgoKatz, AlgoPageRank, AlgoPersonalizedPageRank, ...). It reads well per-file, but with 14 procedures touched it is a lot of repeated prose that could drift if the rationale ever needs correcting in one place but not thirteen others. Might be worth trimming to a short pointer back to WorkGuard's class doc in a follow-up, not a blocker here.
2. docs/release-26.9.1.md is edited in-place, same pattern as the parent PR - worth a quick check that no other in-flight PR touches the same section (merge-conflict risk, not a content problem).
3. The negative-count test (anIterationKnobRejectsANegativeCount) samples 3 of the 14 procedures rather than all - reasonable given the shared extractInt implementation, and it is explicitly called out as deliberate in the Javadoc rather than silently under-tested.

Test coverage: the new Issue6264AlgoIterationKnobGuardTest is thorough - zero-rejection and minimum-acceptance for all 14, interrupt/deadline abort coverage (with a documented exclusion of louvain/leiden from the deadline list since they converge before the clock is read on the fixture graph), a dedicated test proving the per-node checkpoint fires inside a single long-running SimRank iteration and not just the outer one, direct kernel-level tests for the two WorkCheckpoint overloads, and the SLPA heap-budget cases. The @Tag(\"slow\")/@Timeout usage on the SimRank latency test matches this repo's convention (StallAwareStopwatch.assertStayedUnder, not a raw wall-clock assertion).

No security or dependency concerns - no new third-party dependencies, and the changes only add validation/abort paths rather than removing any.

Nice work - this closes a real gap (silent wrong answers plus unabortable OLAP kernels) and the test suite gives good confidence it does not regress the algorithms' actual output.

[lvca]

Round 5 — nothing implemented, and here is the reasoning for each of the three.

1. Comment duplication — declining, and this one has history worth recording. Round 2 raised the same observation and concluded it read as intentional; round 4 then asked me to add the note to the two sites that lacked it (AlgoPersonalizedPageRank/AlgoArticleRank's OLTP paths), which I did in 8037429. So the rounds pull in opposite directions, and the tie-breaker is what a maintainer actually does with these files: they arrive at one procedure while debugging it, and the question "why is there a checkpoint here and why is this one throttled and that one not" has a different answer per file — it is derived from that loop's per-node complexity, not from a shared rule. A pointer to WorkGuard's class doc would explain the mechanism but not the placement, which is the part that varies and the part #6222 spent three review cycles getting wrong. The drift risk is real but bounded: if the rationale needs correcting it needs correcting per-site anyway, because each site states its own complexity claim.

2. Release-notes conflict risk — checked, and thank you, this was the actionable one. Seven open PRs touch docs/release-26.9.1.md (#6271, #6272, #6275, #6277, #6284, #6285, #6287) and two touch AbstractAlgoProcedure.java (#6285, #6291). GitHub reports this PR MERGEABLE against main right now, so there is nothing to resolve yet.

The one to watch is #6285 (#6263), which renames arcadedb.cypher.algoMaxWalkMemory → algoMaxWorkingMemory while this PR extends that same setting's description and generalises checkWalkBudget into checkBufferBudget. Whichever lands second needs a textual merge in GlobalConfiguration and AbstractAlgoProcedure — additive in the same place, nothing conflicting semantically. I'll resolve it here if #6285 goes first.

3. Negative-count sampling — already documented in the Javadoc as deliberate, as you note.

That closes every point raised across five rounds: implemented where actionable (algo.graphsage as a fourteenth instance, the SimRank value-pinning test, saturatingSum, the missing checkpoint comments), declined with reasons where not. No round found a correctness defect in the fix itself.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

🟢 Coverage 94.29% diff coverage · -6.57% coverage variation

Metric	Results
Coverage variation	✅ -6.57% coverage variation
Diff coverage	✅ 94.29% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (a03af88)	159043	125523	78.92%
Head commit (c603ded)	191468 (+32425)	138531 (+13008)	72.35% (-6.57%)

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 (#6288)	105	99	94.29%

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

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

[claude]

Review

I read through the full diff (all 14 procedure files, AbstractAlgoProcedure, the new WorkCheckpoint interface, the two GraphAlgorithms overloads, and the 500-line regression test), plus AbstractAlgoProcedure.WorkGuard and the two call sites where the per-node checkpoint granularity differs, to check the reasoning holds up. Overall this is a clean, well-scoped follow-up to #6216 - solid work.

What looks right

• Layering is respected. WorkCheckpoint in com.arcadedb.graph.olap avoids pulling the query layer's WorkGuard below com.arcadedb.graph, and existing GraphAlgorithms.pageRank/labelPropagation callers get WorkCheckpoint.NONE with zero behavior change - verified the two new overloads correctly delegate.
• The throttled vs. unthrottled checkpoint choice is actually justified by the code, not just asserted in comments. I checked this specifically because it's easy to get wrong: AlgoLouvain's per-node loop calls guard.check() unthrottled because getCommunityDegree() is a genuine O(n) scan per candidate community, while AlgoLeiden's structurally similar loop uses guard.checkPeriodically(i) because its per-node cost is O(deg) via an IntIntHashMap lookup, not a scan. That distinction is correct and non-obvious enough to be worth the comment it got.
• SLPA's heap-budget fix is properly ordered and tested, including the tricky edge case: checkBufferBudget runs before the rowCapacity > Integer.MAX_VALUE guard, and there's a dedicated test (slpaRejectsMoreLabelEntriesThanAJavaArrayCanHoldEvenWithTheBudgetDisabled) that disables the budget to prove the overflow guard still fires independently. Good catch on saturatingSum too - a saturated product plus a raw + overhead silently produces a negative estimate that would've slipped past estimatedBytes <= budget unconditionally.
• Test coverage is thorough and each test earns its place: zero/negative rejection, minimum-accepted over-reach guard, interrupt abort, deadline abort, two CSR-kernel-level unit tests, and the SimRank result-pinning test for the one place the PR changes output rather than just when it stops (the Arrays.fill refactor of the similarity-matrix reset). The simRankHonoursTheCommandTimeoutInsideASingleIteration test correctly uses @Tag("slow") and StallAwareStopwatch.assertStayedUnder per the repo's stall-aware timing convention rather than a raw wall-clock assertion.
• Confirmed via grep that all 14 procedures named in the issue (plus algo.graphsage, found in review) now contain at least one guard.check/guard.checkPeriodically call - nothing was missed.
• Default-path performance is preserved: WorkGuard.check() only calls System.currentTimeMillis() when a deadline is actually configured, so the common case (timeout disabled) costs one Thread.interrupted() test per checkpoint.

Minor nit

• docs/release-26.9.1.md: the new #6264 section's added text ends directly with [#6264](...) immediately followed by the next ## ... heading with no blank line in between, unlike every other entry in that file (e.g. the #6216 section right above it has a blank line before its own heading). Purely cosmetic, but worth a quick fix for consistency with the rest of the changelog.

One thing worth confirming (not a blocker)

extractInt(n, "maxIterations"/"iterations"/"layers", 1) is a behavior change for any existing caller passing 0 today - it now throws instead of silently returning the untouched initial state. That's clearly the intended fix (and is the whole point of the issue), and it's called out in the release notes, just flagging it explicitly as an intentional breaking change to the procedure's accepted input domain in case anyone's downstream code relies on the old silent-passthrough behavior.

I wasn't able to run mvn compile/mvn verify in this environment (sandboxing blocked the invocation), so this is a static review rather than a build-verified one - the PR description's own reported counts (562/4963 green) are the build evidence here.

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.17%. Comparing base (2a88019) to head (c603ded).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...ncypher/procedures/algo/AbstractAlgoProcedure.java	66.66%	2 Missing ⚠️
...encypher/procedures/algo/AlgoLabelPropagation.java	71.42%	2 Missing ⚠️
...pher/procedures/algo/AlgoPersonalizedPageRank.java	77.77%	1 Missing and 1 partial ⚠️
...ry/opencypher/procedures/algo/AlgoArticleRank.java	88.88%	1 Missing ⚠️
...b/query/opencypher/procedures/algo/AlgoFastRP.java	83.33%	0 Missing and 1 partial ⚠️
...uery/opencypher/procedures/algo/AlgoGraphSAGE.java	75.00%	0 Missing and 1 partial ⚠️
...b/query/opencypher/procedures/algo/AlgoLeiden.java	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6288      +/-   ##
==========================================
+ Coverage   70.12%   70.17%   +0.04%     
==========================================
  Files        1839     1840       +1     
  Lines      158956   159115     +159     
  Branches    33534    33556      +22     
==========================================
+ Hits       111463   111652     +189     
+ Misses      33599    33596       -3     
+ Partials    13894    13867      -27     

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

🚀 New features to boost your workflow:

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