fix(EN-1501): bound live query checkpoints with a configurable, Raft-replicated limit

[Azorlogh]

EN-1501: bound the number of live query checkpoints

Query checkpoints were unbounded — a scheduler or client loop grows disk (hard-linked SST checkpoints) and ListQueryCheckpoints payloads without limit. This caps the number of live query checkpoints.

The limit

Cluster-wide, Raft-committed, runtime-configurable (default 10). Set/read via ledgerctl query-checkpoint set-limit <n> / get-limit; enforced in the FSM at a precise applied index so every node agrees — deliberately not a node-local flag (that would break FSM determinism under config drift / rolling upgrade). Creation fails at the limit with CHECKPOINT_LIMIT_REACHED (ResourceExhausted / 429); no eviction — delete one or raise the limit. Zero is rejected (INVALID_QUERY_CHECKPOINT_LIMIT).

How

• Enforcement — FSMState.QueryCheckpointLimit + a live-ID set on FSMState, both rehydrated at recovery; the cap gate reads them in the apply path (no Pebble reads on the hot path, invariant Add basic script system #3).
• Delete is existence-aware — a non-live id returns CHECKPOINT_NOT_FOUND; only a real delete emits a log, keeping created−deleted == live.
• Orphan reclamation — recovery sweeps query-checkpoints/* against the live set, reclaiming a row-less directory a snapshot-installed follower would otherwise leak.
• Checker (invariant feat(CI): Add GoReleaser #8) — compareQueryCheckpoints verifies stored rows against the audit chain both ways, including max_sequence / created_at / key-vs-payload id; created_at is carried on CreatedQueryCheckpointLog; RebuildDelta recreates the rows and restores the monotonic next-ID counter.
• The setting mirrors the existing query-checkpoint schedule mechanism (not ClusterConfig, which is flag-reconciled at leadership and would revert runtime changes).

Tests

Unit (processor cap/limit/delete, WriteSet overlay + intra-bulk, checker, recovery sweep, rebuild) + e2e (cap, delete-not-found, set/get-limit → enforce new cap → reject-0 → raise). golangci-lint clean on both modules.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

• main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5721ffdc-f031-486e-af3e-ffb225fbff9f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[NumaryBot]

🛑 Changes requested — automated review

The new checker pass still cannot detect deletion of an explicitly persisted limit when its audited value equals the default.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1660 (comment)

[codecov]

Codecov Report

❌ Patch coverage is 71.55963% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.73%. Comparing base (a931282) to head (3163e7f).
⚠️ Report is 5 commits behind head on release/v3.0.

Files with missing lines	Patch %	Lines
internal/application/check/checker.go	51.51%	11 Missing and 5 partials ⚠️
internal/infra/state/query_checkpoint_scheduler.go	54.54%	5 Missing ⚠️
internal/domain/processing/skip_safe_scope.go	0.00%	4 Missing ⚠️
internal/infra/attributes/baseline.go	66.66%	1 Missing and 1 partial ⚠️
internal/infra/state/fsmstate.go	81.81%	1 Missing and 1 partial ⚠️
internal/query/query_checkpoint.go	75.00%	1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (71.55%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@               Coverage Diff                @@
##           release/v3.0    #1660      +/-   ##
================================================
+ Coverage         74.39%   74.73%   +0.33%     
================================================
  Files               447      447              
  Lines             47566    47695     +129     
================================================
+ Hits              35389    35645     +256     
+ Misses             8984     8841     -143     
- Partials           3193     3209      +16     

Flag	Coverage Δ
e2e	74.73% <71.55%> (+0.33%)	⬆️
scenario	74.73% <71.55%> (+0.33%)	⬆️
unit	74.73% <71.55%> (+0.33%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[flemzord]

Revalidated current head 3163e7fc. I confirm the existing NumaryBot finding as a correctness/integrity blocker: recovery, cap enforcement, and the new checker derive the live set from embedded checkpoint IDs without validating them against the persisted Pebble key IDs. A mismatched row can therefore under-count live rows and evade the projection check. I am not duplicating the inline evidence. The NumaryBot and Codecov failures are not the basis of this review.

[flemzord]

One additional correctness/integrity blocker on current head 3163e7fc: compareQueryCheckpoints explicitly checks only stored ⊆ audit-derived and treats an audit-live checkpoint with no stored row as legitimate. That lets deletion or loss of a checkpoint projection pass Check() silently, while recovery then seeds the FSM cap/existence set from that incomplete store and changes subsequent create/delete outcomes. This is not full verification of a persisted projection under invariant #8. Please compare both directions, or represent the restore-time invalidation in authoritative state so an arbitrary missing row cannot be indistinguishable from an intentional restore.

[Azorlogh]

@flemzord addressed in 4fdc99064:

• Checkpoint IDs are now read from the Pebble key, not the payload checkpoint_id — recovery, cap enforcement, and the checker all go through ReadLiveQueryCheckpointIDs, which iterates keys. A key≠payload row can no longer under-count or evade the check.
• compareQueryCheckpoints now verifies both directions. To make the reverse direction sound rather than false-positive after a restore, the audit-rebuild path now recreates the checkpoint metadata rows from the CreatedQueryCheckpoint/DeletedQueryCheckpoint logs (rebuild.go) — like every other projection it rebuilds. The physical files still can't be reconstructed, so a rebuilt checkpoint reads Unavailable until deleted (the existing EN-1460 state). A missing row is therefore corruption, not a legitimate restore artifact.

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #1660 (comment)

[NumaryBot]

NumaryBot posted 2 new inline findings.

Summary: #1660 (comment)

[paul-nicolas]

Multi-model review — Claude + Codex

Reviewed at head 5e2601ed1 from a dedicated worktree pinned to that SHA. GOROOT= go build ./... is green and all changed packages pass (internal/domain/processing, internal/infra/state, internal/application/check, internal/query, internal/infra/backup, internal/infra/attributes, internal/domain).

Findings: 3 posted inline — 2 Major, 1 Medium. Both models converged on the same cluster of issues; each was then verified against the real code before posting, and everything below is mechanically confirmed rather than inferred.

The three findings share one root: the set of live checkpoints is now rigorously audited, but the contents of each checkpoint row and the filesystem behind it are not. max_sequence is audit-backed yet uncompared, created_at is not audited at all, and the new existence-aware delete silently drops the only cleanup path for orphaned checkpoint directories.

What I checked and found sound

Worth stating explicitly, since these are the parts most likely to hide a determinism or consistency bug:

• Live-set consistency across every reconstruction path. Recovery.RecoverState is the single path that rebuilds FSMState, and it is called on both restart and follower sync (recovery.go:49), so the new LiveQueryCheckpointIDs rehydration is reached on every boot path. FSMState is not proto-serialized into the Raft snapshot — the snapshot is the Pebble checkpoint — so Pebble rehydration is the correct and only mechanism; there is no snapshot field left to update.
• Concurrency. The map is touched only in fsmstate.go, write_set.go, and tests — never by the query path, the scheduler, or a health path. No unsynchronised map read/write.
• Overlay semantics. The lazy copy makes rollback correct (Reset nils it, so an aborted proposal leaves FSMState untouched), and the cap check precedes IncrementNextQueryCheckpointID, so a rejected create consumes no ID. No Pebble read is added on the hot path (invariant #3), and the gate reads only len/lookups, so there is no map-iteration determinism hazard (invariant #2).
• Rebuild counter restore. RebuildDelta is incremental on top of a restored Pebble checkpoint that already carries SubGlobNextQueryCheckpointID, so the "delta has no create logs" case correctly leaves the counter alone, and when the delta does have creates, max+1 is exactly right. No ID-reuse or row-overwrite window — this addresses the concern the head commit was written for.
• No checker false positive on restore. The bidirectional check is safe because restore carries the rows and RebuildDelta patches the delta; the inDerived && !inStored direction cannot fire on a legitimate restore.
• Error wiring. Both reasons are complete end to end — the string→enum conversion is generic (ErrorReason_value["ERROR_REASON_"+…]), so no per-reason registration is missing, and KindForReason handles both. Freezability is correct: CHECKPOINT_LIMIT_REACHED → ResourceExhausted is not freezable (IsFreezableFailure, errors.go:151), sidestepping the idempotency-freeze hazard as intended, while CHECKPOINT_NOT_FOUND → NotFound is freezable, which is right since IDs are monotonic and never reused.
• Proto and generated code are consistent and sequentially numbered; mocks for the two new Scope methods are properly regenerated.
• Docs are accurate. query-checkpoints.md, cli.md, api-comparison.md, and the invariant-#8 pass list in AGENTS.md all correctly describe the bidirectional pass. No REST/openapi surface is affected (gRPC ClusterService only), so that claim holds too.
• E2E test is properly isolated — a dedicated single node on its own ports (9224/8224), so the global cap cannot leak across specs, and it asserts the typed reasons rather than just "an error occurred". Good test.

Note on the PR description

The description still says the checker pass is "One-directional (stored ⊆ derived): the reverse is intentionally not flagged because checkpoint rows are deliberately not rebuilt on restore." The code, the docs, and AGENTS.md are all bidirectional, and rows are rebuilt on restore — the last commit changed this. Worth updating the body so it does not contradict the merged design.

Minor / Nit (not posted inline)

• internal/infra/state/query_checkpoint_scheduler.go:131 — the limitReached state machine itself (log-once, stay-armed, reset-on-success) has no test; only the isCheckpointLimitReached classifier is covered. "Stays armed so creation resumes after a delete" is the behavioural claim in the PR body and the docs, and nothing asserts it at any level. Consider a small loop-level test driving proposeFn through limit → limit → success.
• internal/infra/state/write_set.go:1745 — QueryCheckpointCount/QueryCheckpointExists call ensureLiveQueryCheckpoints(), so even a rejected create (cap full) or a read-only existence check allocates a full copy of the set and makes Merge swap in a content-identical map. Harmless, but the copy could be deferred to the first mutation by having the read paths fall back to b.fsm.State.LiveQueryCheckpointIDs when the overlay is nil.

[paul-nicolas]

[Major] Existence-aware delete removes the only path that reclaimed stray checkpoint directories

Returning CHECKPOINT_NOT_FOUND before emitting DeletedQueryCheckpointLog also suppresses every downstream cleanup, because all of it is log-driven:

• Machine.deleteQueryCheckpointFiles runs post-commit from pb.checkpointDeletes (internal/infra/state/machine.go:889-891), which is populated from r.QueryCheckpointDeleted (machine.go:789-793) — a signal derived from the delete log.
• the read-index cleanup is gated the same way (internal/application/indexbuilder/process_logs.go:159).

I checked for any other reclamation path and there is none: DeleteQueryCheckpointFiles (internal/storage/dal/store.go:955) has exactly one caller, queryCheckpointsDir is referenced only by the create/delete/path helpers in store.go, and cleanupOldCheckpoints reaps checkpointsDir (Pebble backup checkpoints), not query-checkpoints/. process_logs.go:252 states outright that there is no reconciler.

Reachable case: a follower that falls far enough behind to be caught up by a snapshot install receives the leader's Pebble state (row already absent) and never executes the per-entry post-commit hook, so its local query-checkpoints/<id>/ survives. Before this PR an operator could reclaim it by re-issuing delete <id> — the log was emitted unconditionally and every node deleted its files. Now that call returns NotFound and does nothing, so the directory leaks permanently with no API or boot-time sweep to recover it. Each one is a full db.Checkpoint() whose hard-linked SSTs keep pinning disk as the live store compacts — the same unbounded-disk failure mode EN-1501 is closing, reintroduced through a different door.

Fix: keep the typed NotFound for the caller (it is the right contract and the live-count exactness argument holds), but do not let it be the only signal. Either emit a file-cleanup-only side effect on the not-found branch, or add a boot-time sweep that reconciles query-checkpoints/* against LiveQueryCheckpointIDs (which recovery already loads) and removes directories with no live row — the analogue of purgeOrphanVersions for the read index.

[Azorlogh]

Fixed: RecoverState now sweeps query-checkpoints/* against LiveQueryCheckpointIDs and reclaims row-less dirs (best-effort). 858e115c1

[paul-nicolas]

[Major] Checker pass verifies only checkpoint IDs, leaving the audit-backed max_sequence unverified

compareQueryCheckpoints reads ReadLiveQueryCheckpointIDs, which decodes IDs from the Pebble key and discards the payload. So the pass verifies set membership only. QueryCheckpointState.max_sequence is a persisted field of this projection, and it is audit-backed — CreatedQueryCheckpointLog.max_sequence (field 2, misc/proto/common.proto:559) carries it, and the rebuild path already reads it back (rebuild.go:412).

Why it matters: invariant #8 requires the checker to verify every projection it persists, and no other pass touches SubGlobQueryCheckpoint (grep: only baseline.go, query_checkpoint.go, batch.go, and this file). A store whose max_sequence is edited in place therefore passes Check() clean, and the tampered value is served to clients by ListQueryCheckpoints / GetQueryCheckpointInfo (internal/adapter/grpc/server_cluster.go:473 and :577) and printed by ledgerctl query-checkpoint list/info. It does not change what the read index serves (the indexbuilder materializes from the log's value inline), so this is an integrity/reporting gap rather than a query-correctness one — but it is exactly the "projection the checker does not verify is a tampering vector" case, and the PR adds this pass precisely to close that gap.

Fix: make derived a map[uint64]*commonpb.CreatedQueryCheckpointLog (or a small struct carrying maxSequence) instead of map[uint64]struct{}, load the full stored rows with query.ListQueryCheckpoints, and compare max_sequence per ID, emitting CHECK_STORE_ERROR_TYPE_QUERY_CHECKPOINT_MISMATCH on divergence. The baseline seed can carry the stored max_sequence the same way it carries the ID today.

[Azorlogh]

Fixed: compareQueryCheckpoints now verifies max_sequence (and created_at) per row, both directions. 858e115c1

[paul-nicolas]

[Medium] Rebuild silently drops created_at, and the field has no audit backing at all

The rebuilt row is constructed with only CheckpointId and MaxSequence, so QueryCheckpointState.created_at (misc/proto/raft_cmd.proto:217) comes back as nil after any restore + RebuildDelta. That value is user-visible: it is returned by ListQueryCheckpoints / GetQueryCheckpointInfo (internal/adapter/grpc/server_cluster.go:473, :577) and rendered by ledgerctl query-checkpoint list / info, so every live checkpoint reports an empty creation time post-restore.

The root cause is that CreatedQueryCheckpointLog only carries checkpoint_id and max_sequence (misc/proto/common.proto:557-560) — the timestamp is never audited, so it is not merely dropped here, it is unrecoverable and permanently unverifiable by any checker pass. Combined with the finding on compareQueryCheckpoints, created_at is a persisted projection field with zero audit binding, which is the invariant-#8 gap in its strongest form.

Fix: add common.Timestamp created_at = 3; to CreatedQueryCheckpointLog, populate it in processCreateQueryCheckpoint from the same proposal date the row already uses, run just generate-proto, set it on the rebuilt row here, and extend the checker comparison to cover it. v3 is unreleased, so there is no wire-compat cost. Worth a rebuild regression test asserting CreatedAt survives — rebuild_test.go currently only asserts the ID set.

[Azorlogh]

Fixed: created_at added to CreatedQueryCheckpointLog, populated at create, restored on rebuild, verified in the checker. 858e115c1

[gfyrag]

Blocking design concern: EN-1501 establishes why query-checkpoint cardinality must be bounded, but neither the ticket nor this PR provides a legitimate basis for the value 10. The fact that --pebble-max-checkpoints defaults to 10 is not sufficient evidence: maintenance checkpoints and query checkpoints have different lifecycle, storage cost, and product semantics. There is no sizing estimate, retention requirement, workload analysis, or API/SLA constraint showing that 10 is the correct boundary. Hard-coding it makes an arbitrary number part of the v3 protocol contract.

This should not be “fixed” by simply reusing --pebble-max-checkpoints or introducing another node-local CLI/env flag in the FSM path. The existing flag controls local Pebble housekeeping; the query-checkpoint limit determines whether a replicated command succeeds or fails. During a rolling upgrade—or with any configuration drift—nodes could run with different values and apply the same Raft entry differently, which would violate FSM determinism and can desynchronize replicated state.

If the limit is configurable, it needs cluster-wide replicated semantics: the value must be committed through Raft, take effect at a precise applied index on every node, and have explicit compatibility/validation rules for rolling upgrades. If it remains a protocol constant, the chosen value needs a documented product/operational rationale rather than being copied from an unrelated default.

Please document both the derivation of the limit and the safe configuration/rollout model before merging.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1660 (comment)

[Azorlogh]

Reworked to a cluster-wide, Raft-committed configurable limit (default 10), applied deterministically in the FSM at a precise applied index — not a node-local flag. ledgerctl query-checkpoint set-limit/get-limit; rationale + rollout documented. 858e115c1

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1660 (comment)

[NumaryBot]

🔴 [blocker] Verify the persisted checkpoint limit against the audit chain

When the audited limit was explicitly set to the default value 10, deleting SubGlobQueryCheckpointLimit is invisible here because ReadQueryCheckpointLimit substitutes the same default for a missing key. The checker therefore accepts a lost persisted projection instead of verifying its presence; track whether the audit contains a set operation and distinguish an absent row accordingly, as required by AGENTS.md:21.

[gfyrag]

I think the limit should be enforced at admission time rather than by the FSM.

The important rolling-upgrade property is that once a CreateQueryCheckpoint command is committed, every node applies it identically, regardless of its local version or infrastructure configuration. During a mixed-version rollout, different leaders may make different admission decisions, but this only affects whether a command is proposed—not how a committed Raft entry is applied.

Proposed changes:

1. Remove the replicated SetQueryCheckpointLimit setting and its Raft order, API, ledgerctl set-limit/get-limit commands, persisted projection, and checker pass.
2. Keep CreateQueryCheckpoint unconditional in the FSM: no node-local or version-dependent limit check on the apply path.
3. Make the limit an infrastructure-owned startup configuration, with no runtime mutation through ledgerctl.
4. Before proposing CreateQueryCheckpoint, admission checks the leader's current live checkpoint count and rejects the request when the configured limit is reached.
5. Accept that concurrent admissions may exceed the configured limit until a checkpoint is deleted. The possible overshoot is tied to the number of concurrent in-flight creations. Avoid adding reservation or serialization machinery unless we decide that the limit must be strict.
6. Document the rolling-upgrade behavior explicitly: an old leader may not enforce the limit, while an upgraded leader does; committed commands remain deterministic on every replica.
7. Add admission tests for rejection at the limit and acceptance below it. Keep the existing checker coverage for checkpoint rows and IDs; only the replicated-limit verification should disappear.

This makes the limit an operational safeguard rather than a Raft protocol invariant, which is the intended boundary here.