Skip to content

fix(inkless:retention): decouple enforce_retention_v2 from the log lock - #705

Merged
ivanyu merged 4 commits into
mainfrom
jeqo/fix-enforcer
Jul 20, 2026
Merged

fix(inkless:retention): decouple enforce_retention_v2 from the log lock#705
ivanyu merged 4 commits into
mainfrom
jeqo/fix-enforcer

Conversation

@jeqo

@jeqo jeqo commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

enforce_retention_v2 took logs FOR UPDATE before its O(depth) retention boundary scan and looped every request in a single transaction. A concurrent commit_file on a locked partition therefore waited for the whole enforcement pass, and in production the commit-latency tail tracked the enforce latency (commit max was roughly equal to enforce max).

Change

Enforcement is restructured so the lock is held only for the short delete, not the scan:

  • The retention boundary is computed in a read-only step with no FOR UPDATE. byte_size is read inside that scan statement (a selected_log CTE) so the reverse-aggregate stays consistent with the batches rows under READ COMMITTED. The lock is taken only afterwards, for the delete.
  • EnforceRetentionJob runs one transaction per request instead of wrapping all requests in one. A plpgsql function cannot release a row lock before it returns, so per-request transactions are what actually free the lock between partitions rather than accumulating it until the end. now is computed once per cycle and shared. Each transaction holds at most one partition lock, so enforce can no longer self-deadlock.
  • Deleted batches/bytes are recounted under the lock so the reported counts stay accurate when a concurrent delete advances log_start between the unlocked scan and the locked delete.
  • The max_batches_per_request cap probes for the (cap+1)-th deletable batch with LIMIT/OFFSET instead of COUNT(*) over all deletable rows, keeping the locked section O(delete-size) rather than O(depth) when a small cap meets a large deletable set.
  • The boundary scan is skipped entirely when the oldest retained batch provably survives every enabled policy, using logs.byte_size and logs.earliest_batch_timestamp. A NULL earliest_batch_timestamp means "unknown", so the short-circuit does not fire and the scan runs.

Safety

Computing the boundary unlocked is safe: retention deletes at the tail while commit appends at the head, and delete_records_v1 re-clamps to the current log_start under its own lock, so a concurrent commit or delete cannot cause over-deletion. Partial progress across partitions is fine; retention is idempotent and re-runs next cycle.

Results

Indicative local numbers (testcontainers, single machine) from the committed @Tag("benchmark") harness, maxBatchesPerRequest=1 so delete work is tiny and the measured tail is lock coupling. max/enforce is the concurrent commit's max latency as a fraction of the enforce latency; commits is how many commits completed during one enforce pass.

Single partition, commit to the same partition being enforced:

rows before (max, commits) after (max, commits)
200k 147 ms, 1 13 ms, 52
400k 312 ms, 1 44 ms, 121
800k 731 ms, 1 18 ms, 285

Multi-partition (8 partitions enforced in one cycle), commit to the first partition:

rows/partition before (max, commits) after (max, commits)
50k 271 ms, 1 29 ms, 135
100k 776 ms, 1 15 ms, 223

Commit p50/p95 during enforcement drop to roughly 1-5 ms, and commit max no longer scales with partition depth.

Testing

  • Full control-plane contract (PostgresControlPlaneTest, including the Retention size/time suites) and the in-memory contract remain green; the SQL semantics are unchanged.
  • New PG concurrency tests in EnforceRetentionJobTest: log deleted between scan and delete returns unknown_topic_or_partition; log_start advanced between scan and delete returns the recounted deletion; and a NULL earliest_batch_timestamp falls back to the scan instead of short-circuiting.

Stacking / dependencies

Stacked on the find_batches bounded-scan change and on the logs.earliest_batch_timestamp change (the short-circuit reads that column, so this must apply after it).

Out of scope (follow-ups)

  • Bound the boundary scan itself to O(result) with a reverse walk (the scan is now unlocked, so its cost is background-job latency, not commit blocking).
  • FOR UPDATE SKIP LOCKED with a fairness backstop, if a residual enforce-vs-commit tail remains after this change.
  • Leader-scheduled enforcement once managed replicas provide a per-partition owner.

Copilot AI 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.

Pull request overview

Restructures Postgres retention enforcement (enforce_retention_v2) to reduce commit tail-latency by avoiding holding the logs row lock during the O(depth) boundary scan, and updates the Java job to avoid lock accumulation across partitions by running one transaction per request.

Changes:

  • Updates enforce_retention_v2 to compute the retention boundary without FOR UPDATE, then acquire the lock only for the bounded delete/recount section.
  • Changes EnforceRetentionJob to execute one DB transaction per retention request (shared now per cycle).
  • Adds Postgres concurrency regression tests plus an opt-in benchmark to measure commit blocking during enforcement.

Reviewed changes

Copilot reviewed 4 out of 122 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
storage/inkless/src/main/java/io/aiven/inkless/control_plane/postgres/EnforceRetentionJob.java Runs retention enforcement in per-request transactions and expects a single response per call.
storage/inkless/src/main/resources/db/migration/V21__Retention_enforcement_lock_decoupling.sql Rewrites enforce_retention_v2 to lock late (delete phase only) and adds short-circuit + capped-delete logic.
storage/inkless/src/test/java/io/aiven/inkless/control_plane/postgres/EnforceRetentionJobTest.java Adds concurrency-focused tests for scan/delete interleavings and NULL earliest_batch_timestamp behavior.
storage/inkless/src/test/java/io/aiven/inkless/control_plane/postgres/EnforceRetentionCommitBlockingBenchmarkTest.java Adds an opt-in @Tag("benchmark") harness to measure commit blocking during enforcement.
storage/inkless/src/main/jooq/org/jooq/generated/DefaultSchema.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/Domains.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/Indexes.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/Keys.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/Routines.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/Tables.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/UDTs.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/AdvanceCrossTierLogStartResponseErrorV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/CommitBatchResponseErrorV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/DeleteRecordsResponseErrorV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/EnforceRetentionResponseErrorV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/FileReasonT.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/FileStateT.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/FindBatchesResponseErrorV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/InitDisklessLogErrorV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/ListOffsetsResponseErrorV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/enums/PruneBatchesBelowHighestTieredOffsetErrorV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/routines/BatchTimestamp.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/routines/DeleteBatchV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/routines/DeleteFilesV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/routines/DeleteTopicV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/routines/FlushCommitRunV2.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/routines/MarkFileToDeleteV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/AdvanceCrossTierLogStartV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/Batches.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/CommitFileV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/CommitFileV2.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/DeleteRecordsV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/EnforceRetentionV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/EnforceRetentionV2.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/Files.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/FindBatchesV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/FindBatchesV2.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/InitDisklessLogV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/ListOffsetsV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/Logs.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/ProducerState.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/PruneBatchesBelowHighestTieredOffsetV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/RepairDisklessLogV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/AdvanceCrossTierLogStartV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/BatchesRecord.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/CommitFileV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/CommitFileV2Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/DeleteRecordsV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/EnforceRetentionV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/EnforceRetentionV2Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/FilesRecord.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/FindBatchesV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/FindBatchesV2Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/InitDisklessLogV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/ListOffsetsV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/LogsRecord.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/ProducerStateRecord.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/PruneBatchesBelowHighestTieredOffsetV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/tables/records/RepairDisklessLogV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/AdvanceCrossTierLogStartRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/AdvanceCrossTierLogStartResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/BatchInfoV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/BatchMetadataV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/CommitBatchRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/CommitBatchResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/DeleteRecordsRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/DeleteRecordsResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/EnforceRetentionRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/EnforceRetentionResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/FindBatchesRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/FindBatchesResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/InitDisklessLogProducerStateV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/InitDisklessLogRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/InitDisklessLogResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/ListOffsetsRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/ListOffsetsResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/PruneBatchesBelowHighestTieredOffsetRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/PruneBatchesBelowHighestTieredOffsetResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/RepairDisklessLogRequestV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/RepairDisklessLogResponseV1.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/AdvanceCrossTierLogStartRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/AdvanceCrossTierLogStartResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/BatchInfoV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/BatchMetadataV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/CommitBatchRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/CommitBatchResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/DeleteRecordsRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/DeleteRecordsResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/EnforceRetentionRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/EnforceRetentionResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/FindBatchesRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/FindBatchesResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/InitDisklessLogProducerStateV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/InitDisklessLogRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/InitDisklessLogResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/ListOffsetsRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/ListOffsetsResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/PruneBatchesBelowHighestTieredOffsetRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/PruneBatchesBelowHighestTieredOffsetResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/RepairDisklessLogRequestV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/paths/RepairDisklessLogResponseV1Path.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/AdvanceCrossTierLogStartRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/AdvanceCrossTierLogStartResponseV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/BatchInfoV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/BatchMetadataV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/CommitBatchRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/CommitBatchResponseV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/DeleteRecordsRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/DeleteRecordsResponseV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/EnforceRetentionRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/EnforceRetentionResponseV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/FindBatchesRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/FindBatchesResponseV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/InitDisklessLogProducerStateV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/InitDisklessLogRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/InitDisklessLogResponseV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/ListOffsetsRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/ListOffsetsResponseV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/PruneBatchesBelowHighestTieredOffsetRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/PruneBatchesBelowHighestTieredOffsetResponseV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/RepairDisklessLogRequestV1Record.java jOOQ regen metadata bump to schema version 21.
storage/inkless/src/main/jooq/org/jooq/generated/udt/records/RepairDisklessLogResponseV1Record.java jOOQ regen metadata bump to schema version 21.

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 122 changed files in this pull request and generated no new comments.

Base automatically changed from jeqo/logs-earliest-batch-timestamp to main July 17, 2026 08:57
Measures commit latency while enforce_retention_v2 runs on the same
partition, as an A/B harness for the lock-decoupling change. Two shapes:
single-partition (commit and enforce target the same partition, isolating
the scan-under-lock effect) and multi-partition (enforce runs over N
partitions while a commit targets the first, isolating lock accumulation
across partitions in one transaction). Tagged @tag("benchmark") so it is
excluded from CI and run on demand via the benchmarkTest task.
@jeqo
jeqo force-pushed the jeqo/fix-enforcer branch 2 times, most recently from 4f5ffbf to 55f56bc Compare July 17, 2026 10:04
@jeqo
jeqo requested a review from Copilot July 17, 2026 10:10

Copilot AI 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.

Pull request overview

Copilot reviewed 6 out of 124 changed files in this pull request and generated 1 comment.

Copilot AI 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.

Pull request overview

Copilot reviewed 6 out of 124 changed files in this pull request and generated 2 comments.

Copilot AI 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.

Pull request overview

Copilot reviewed 6 out of 124 changed files in this pull request and generated 2 comments.

enforce_retention_v2 took logs FOR UPDATE before its O(depth) retention
boundary scan and looped every request in one transaction, so a
concurrent commit_file on a locked partition waited for the whole
enforcement pass. In production the commit-latency tail tracked the
enforce latency (commit max ~ enforce max).

Restructure enforcement so the lock is held only for the short delete:

- The retention boundary is now computed in a read-only step with no FOR
  UPDATE. byte_size is read inside that scan statement (a selected_log
  CTE) so the reverse-aggregate stays consistent with the batches rows
  under READ COMMITTED. The lock is taken only afterwards, for the delete.
- EnforceRetentionJob runs one transaction per request instead of wrapping
  all requests in one. A plpgsql function cannot release a row lock before
  it returns, so per-request transactions are what actually free the lock
  between partitions rather than accumulating it until the end. now is
  computed once per cycle and shared. Each transaction holds at most one
  partition lock, so enforce can no longer self-deadlock and the loop's
  ordering is no longer needed for deadlock avoidance on this path.
- Deleted batches/bytes are recounted under the lock so the reported
  counts stay accurate when a concurrent delete advances log_start between
  the unlocked scan and the locked delete.
- The max_batches_per_request cap now probes for the (cap+1)-th deletable
  batch with LIMIT/OFFSET instead of COUNT(*) over all deletable rows,
  keeping the locked section O(delete-size) rather than O(depth) when a
  small cap meets a large deletable set.
- The boundary scan is skipped entirely when the oldest retained batch
  provably survives every enabled policy, using logs.byte_size and
  logs.earliest_batch_timestamp. A NULL earliest_batch_timestamp means
  "unknown", so the short-circuit does not fire and the scan runs.

Computing the boundary unlocked is safe: retention deletes at the tail
while commit appends at the head, and delete_records_v1 re-clamps to the
current log_start under its own lock, so a concurrent commit or delete
cannot cause over-deletion. Partial progress across partitions is fine;
retention is idempotent and re-runs next cycle.

Adds PG concurrency tests for the interleavings this enables: log deleted
between scan and delete returns unknown_topic_or_partition; log_start
advanced between scan and delete returns the recounted deletion; and a
NULL earliest_batch_timestamp falls back to the scan instead of
short-circuiting.

Regenerates jOOQ sources for the schema-version stamp bump introduced by
the migration.
@jeqo
jeqo force-pushed the jeqo/fix-enforcer branch from 4d4e98c to 032458b Compare July 17, 2026 11:23
jeqo added 2 commits July 17, 2026 14:35
… control plane

Mirror logs.earliest_batch_timestamp into InMemoryControlPlane for parity
with the Postgres control plane, so the enforce_retention short-circuit
behaves identically across both backends.

- LogInfo.earliestBatchTimestamp (nullable; null == "unknown, must scan",
  matching the SQL NULL sentinel), maintained under the same rules as V20:
  commit populates only when unset, delete/prune/enforce recompute from the
  new oldest batch when the oldest changed (null when the log empties).
- enforceRetention gains the same short-circuit: when the oldest retained
  batch survives every enabled policy, skip the O(depth) selection scans.
  It skips only the scans and falls through to the unchanged tail, so the
  reported log_start_offset and deletion counts are identical (pure no-op
  result-wise; the in-memory backend has no O(depth) scan to optimize, this
  is parity only).
- InMemoryControlPlaneEarliestBatchTimestampTest mirrors the Postgres
  LogsEarliestBatchTimestampTest across commit/delete/prune/enforce. The
  field is intentionally not on the ControlPlane interface, so it is read
  via reflection.
…art backward

InMemoryControlPlane.enforceRetention recomputed log_start_offset from the
oldest batch's base offset on every call, even when it deleted nothing. If a
prior deleteRecords had advanced log_start into the middle of a still-retained
batch (e.g. offset 5 inside batch [0,9]), a later enforce that deleted nothing
would reset log_start back to that batch's base (0). That diverges from
delete_records_v1 (Postgres) and re-exposes reads for offsets that should be
below the log start.

Only touch log_start (and the mirrored earliest_batch_timestamp) when a
deletion actually occurred, and make the recompute forward-only. Adds a parity
test in AbstractControlPlaneTest.Retention that runs against both backends:
after deleteRecords advances log_start mid-batch, an enforce that deletes
nothing must leave log_start unchanged.

Copilot AI 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.

Pull request overview

Copilot reviewed 7 out of 125 changed files in this pull request and generated no new comments.

@jeqo
jeqo marked this pull request as ready for review July 17, 2026 11:50
@jeqo
jeqo requested a review from ivanyu July 17, 2026 12:25

@ivanyu ivanyu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, very nice!

@ivanyu
ivanyu merged commit 8c0c452 into main Jul 20, 2026
9 checks passed
@ivanyu
ivanyu deleted the jeqo/fix-enforcer branch July 20, 2026 05:46
jeqo added a commit that referenced this pull request Jul 20, 2026
…ck (#705)

* test(inkless:retention): add enforce commit-blocking benchmark

Measures commit latency while enforce_retention_v2 runs on the same
partition, as an A/B harness for the lock-decoupling change. Two shapes:
single-partition (commit and enforce target the same partition, isolating
the scan-under-lock effect) and multi-partition (enforce runs over N
partitions while a commit targets the first, isolating lock accumulation
across partitions in one transaction). Tagged @tag("benchmark") so it is
excluded from CI and run on demand via the benchmarkTest task.

* fix(inkless:retention): decouple enforce_retention_v2 from the log lock

enforce_retention_v2 took logs FOR UPDATE before its O(depth) retention
boundary scan and looped every request in one transaction, so a
concurrent commit_file on a locked partition waited for the whole
enforcement pass. In production the commit-latency tail tracked the
enforce latency (commit max ~ enforce max).

Restructure enforcement so the lock is held only for the short delete:

- The retention boundary is now computed in a read-only step with no FOR
  UPDATE. byte_size is read inside that scan statement (a selected_log
  CTE) so the reverse-aggregate stays consistent with the batches rows
  under READ COMMITTED. The lock is taken only afterwards, for the delete.
- EnforceRetentionJob runs one transaction per request instead of wrapping
  all requests in one. A plpgsql function cannot release a row lock before
  it returns, so per-request transactions are what actually free the lock
  between partitions rather than accumulating it until the end. now is
  computed once per cycle and shared. Each transaction holds at most one
  partition lock, so enforce can no longer self-deadlock and the loop's
  ordering is no longer needed for deadlock avoidance on this path.
- Deleted batches/bytes are recounted under the lock so the reported
  counts stay accurate when a concurrent delete advances log_start between
  the unlocked scan and the locked delete.
- The max_batches_per_request cap now probes for the (cap+1)-th deletable
  batch with LIMIT/OFFSET instead of COUNT(*) over all deletable rows,
  keeping the locked section O(delete-size) rather than O(depth) when a
  small cap meets a large deletable set.
- The boundary scan is skipped entirely when the oldest retained batch
  provably survives every enabled policy, using logs.byte_size and
  logs.earliest_batch_timestamp. A NULL earliest_batch_timestamp means
  "unknown", so the short-circuit does not fire and the scan runs.

Computing the boundary unlocked is safe: retention deletes at the tail
while commit appends at the head, and delete_records_v1 re-clamps to the
current log_start under its own lock, so a concurrent commit or delete
cannot cause over-deletion. Partial progress across partitions is fine;
retention is idempotent and re-runs next cycle.

Adds PG concurrency tests for the interleavings this enables: log deleted
between scan and delete returns unknown_topic_or_partition; log_start
advanced between scan and delete returns the recounted deletion; and a
NULL earliest_batch_timestamp falls back to the scan instead of
short-circuiting.

Regenerates jOOQ sources for the schema-version stamp bump introduced by
the migration.

* feat(inkless:retention): mirror earliest_batch_timestamp to in-memory control plane

Mirror logs.earliest_batch_timestamp into InMemoryControlPlane for parity
with the Postgres control plane, so the enforce_retention short-circuit
behaves identically across both backends.

- LogInfo.earliestBatchTimestamp (nullable; null == "unknown, must scan",
  matching the SQL NULL sentinel), maintained under the same rules as V20:
  commit populates only when unset, delete/prune/enforce recompute from the
  new oldest batch when the oldest changed (null when the log empties).
- enforceRetention gains the same short-circuit: when the oldest retained
  batch survives every enabled policy, skip the O(depth) selection scans.
  It skips only the scans and falls through to the unchanged tail, so the
  reported log_start_offset and deletion counts are identical (pure no-op
  result-wise; the in-memory backend has no O(depth) scan to optimize, this
  is parity only).
- InMemoryControlPlaneEarliestBatchTimestampTest mirrors the Postgres
  LogsEarliestBatchTimestampTest across commit/delete/prune/enforce. The
  field is intentionally not on the ControlPlane interface, so it is read
  via reflection.

* fix(inkless:retention): stop in-memory enforceRetention moving log_start backward

InMemoryControlPlane.enforceRetention recomputed log_start_offset from the
oldest batch's base offset on every call, even when it deleted nothing. If a
prior deleteRecords had advanced log_start into the middle of a still-retained
batch (e.g. offset 5 inside batch [0,9]), a later enforce that deleted nothing
would reset log_start back to that batch's base (0). That diverges from
delete_records_v1 (Postgres) and re-exposes reads for offsets that should be
below the log start.

Only touch log_start (and the mirrored earliest_batch_timestamp) when a
deletion actually occurred, and make the recompute forward-only. Adds a parity
test in AbstractControlPlaneTest.Retention that runs against both backends:
after deleteRecords advances log_start mid-batch, an enforce that deletes
nothing must leave log_start unchanged.
jeqo added a commit that referenced this pull request Jul 20, 2026
…ck (#705)

* test(inkless:retention): add enforce commit-blocking benchmark

Measures commit latency while enforce_retention_v2 runs on the same
partition, as an A/B harness for the lock-decoupling change. Two shapes:
single-partition (commit and enforce target the same partition, isolating
the scan-under-lock effect) and multi-partition (enforce runs over N
partitions while a commit targets the first, isolating lock accumulation
across partitions in one transaction). Tagged @tag("benchmark") so it is
excluded from CI and run on demand via the benchmarkTest task.

* fix(inkless:retention): decouple enforce_retention_v2 from the log lock

enforce_retention_v2 took logs FOR UPDATE before its O(depth) retention
boundary scan and looped every request in one transaction, so a
concurrent commit_file on a locked partition waited for the whole
enforcement pass. In production the commit-latency tail tracked the
enforce latency (commit max ~ enforce max).

Restructure enforcement so the lock is held only for the short delete:

- The retention boundary is now computed in a read-only step with no FOR
  UPDATE. byte_size is read inside that scan statement (a selected_log
  CTE) so the reverse-aggregate stays consistent with the batches rows
  under READ COMMITTED. The lock is taken only afterwards, for the delete.
- EnforceRetentionJob runs one transaction per request instead of wrapping
  all requests in one. A plpgsql function cannot release a row lock before
  it returns, so per-request transactions are what actually free the lock
  between partitions rather than accumulating it until the end. now is
  computed once per cycle and shared. Each transaction holds at most one
  partition lock, so enforce can no longer self-deadlock and the loop's
  ordering is no longer needed for deadlock avoidance on this path.
- Deleted batches/bytes are recounted under the lock so the reported
  counts stay accurate when a concurrent delete advances log_start between
  the unlocked scan and the locked delete.
- The max_batches_per_request cap now probes for the (cap+1)-th deletable
  batch with LIMIT/OFFSET instead of COUNT(*) over all deletable rows,
  keeping the locked section O(delete-size) rather than O(depth) when a
  small cap meets a large deletable set.
- The boundary scan is skipped entirely when the oldest retained batch
  provably survives every enabled policy, using logs.byte_size and
  logs.earliest_batch_timestamp. A NULL earliest_batch_timestamp means
  "unknown", so the short-circuit does not fire and the scan runs.

Computing the boundary unlocked is safe: retention deletes at the tail
while commit appends at the head, and delete_records_v1 re-clamps to the
current log_start under its own lock, so a concurrent commit or delete
cannot cause over-deletion. Partial progress across partitions is fine;
retention is idempotent and re-runs next cycle.

Adds PG concurrency tests for the interleavings this enables: log deleted
between scan and delete returns unknown_topic_or_partition; log_start
advanced between scan and delete returns the recounted deletion; and a
NULL earliest_batch_timestamp falls back to the scan instead of
short-circuiting.

Regenerates jOOQ sources for the schema-version stamp bump introduced by
the migration.

* feat(inkless:retention): mirror earliest_batch_timestamp to in-memory control plane

Mirror logs.earliest_batch_timestamp into InMemoryControlPlane for parity
with the Postgres control plane, so the enforce_retention short-circuit
behaves identically across both backends.

- LogInfo.earliestBatchTimestamp (nullable; null == "unknown, must scan",
  matching the SQL NULL sentinel), maintained under the same rules as V20:
  commit populates only when unset, delete/prune/enforce recompute from the
  new oldest batch when the oldest changed (null when the log empties).
- enforceRetention gains the same short-circuit: when the oldest retained
  batch survives every enabled policy, skip the O(depth) selection scans.
  It skips only the scans and falls through to the unchanged tail, so the
  reported log_start_offset and deletion counts are identical (pure no-op
  result-wise; the in-memory backend has no O(depth) scan to optimize, this
  is parity only).
- InMemoryControlPlaneEarliestBatchTimestampTest mirrors the Postgres
  LogsEarliestBatchTimestampTest across commit/delete/prune/enforce. The
  field is intentionally not on the ControlPlane interface, so it is read
  via reflection.

* fix(inkless:retention): stop in-memory enforceRetention moving log_start backward

InMemoryControlPlane.enforceRetention recomputed log_start_offset from the
oldest batch's base offset on every call, even when it deleted nothing. If a
prior deleteRecords had advanced log_start into the middle of a still-retained
batch (e.g. offset 5 inside batch [0,9]), a later enforce that deleted nothing
would reset log_start back to that batch's base (0). That diverges from
delete_records_v1 (Postgres) and re-exposes reads for offsets that should be
below the log start.

Only touch log_start (and the mirrored earliest_batch_timestamp) when a
deletion actually occurred, and make the recompute forward-only. Adds a parity
test in AbstractControlPlaneTest.Retention that runs against both backends:
after deleteRecords advances log_start mid-batch, an enforce that deletes
nothing must leave log_start unchanged.
jeqo added a commit that referenced this pull request Jul 27, 2026
…O(cap)

The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every
deletable batch, O(partition depth). PR #705 moved it out of the log lock so it
no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain
(retention lowered so almost the whole partition is deletable) a single scan on
a multi-million-batch partition runs for seconds-to-minutes and exceeds the
enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the
still-running backend so the partition never drains.

V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The
boundary can never be deeper than the cap because the delete is clamped there
regardless; the +1 distinguishes an in-window boundary (delete up to it) from a
deeper one that falls through to high_watermark and is re-clamped by the
existing under-lock cap-probe. Each pass is O(cap), index-only on
batches_by_last_offset_covering_idx, and a deep partition drains over
successive enforcement cycles. max_batches_per_request = 0 keeps the original
unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes
oldest-first, so a snapshot-stale boundary can only under-delete).

Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the
bound is on out of the box; it must stay above the per-interval expiry rate or
retention falls behind.

Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via
-Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k
batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms
(flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is
the sweet spot: short passes at 10x the drain throughput of 100.

[KC-354]
jeqo added a commit that referenced this pull request Jul 28, 2026
…O(cap)

The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every
deletable batch, O(partition depth). PR #705 moved it out of the log lock so it
no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain
(retention lowered so almost the whole partition is deletable) a single scan on
a multi-million-batch partition runs for seconds-to-minutes and exceeds the
enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the
still-running backend so the partition never drains.

V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The
boundary can never be deeper than the cap because the delete is clamped there
regardless; the +1 distinguishes an in-window boundary (delete up to it) from a
deeper one that falls through to high_watermark and is re-clamped by the
existing under-lock cap-probe. Each pass is O(cap), index-only on
batches_by_last_offset_covering_idx, and a deep partition drains over
successive enforcement cycles. max_batches_per_request = 0 keeps the original
unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes
oldest-first, so a snapshot-stale boundary can only under-delete).

Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the
bound is on out of the box; it must stay above the per-interval expiry rate or
retention falls behind.

Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via
-Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k
batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms
(flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is
the sweet spot: short passes at 10x the drain throughput of 100.

[KC-354]
viktorsomogyi pushed a commit that referenced this pull request Jul 28, 2026
…O(cap) (#722)

The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every
deletable batch, O(partition depth). PR #705 moved it out of the log lock so it
no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain
(retention lowered so almost the whole partition is deletable) a single scan on
a multi-million-batch partition runs for seconds-to-minutes and exceeds the
enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the
still-running backend so the partition never drains.

V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The
boundary can never be deeper than the cap because the delete is clamped there
regardless; the +1 distinguishes an in-window boundary (delete up to it) from a
deeper one that falls through to high_watermark and is re-clamped by the
existing under-lock cap-probe. Each pass is O(cap), index-only on
batches_by_last_offset_covering_idx, and a deep partition drains over
successive enforcement cycles. max_batches_per_request = 0 keeps the original
unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes
oldest-first, so a snapshot-stale boundary can only under-delete).

Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the
bound is on out of the box; it must stay above the per-interval expiry rate or
retention falls behind.

Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via
-Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k
batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms
(flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is
the sweet spot: short passes at 10x the drain throughput of 100.

[KC-354]
jeqo added a commit that referenced this pull request Jul 28, 2026
EnforceRetentionJob runs one transaction per partition (post-#705), yet
EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side
RetentionEnforcementTotalTime, which already measures the whole enforce wave.

Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per
partition) instead, matching the other *QueryTime metrics (one DB operation each)
and yielding per-partition enforce latency without a new metric. The whole-wave
total stays RetentionEnforcementTotalTime on the broker.

Note: this redefines two existing metrics -- EnforceRetentionQueryTime from
per-wave to per-partition, and EnforceRetentionQueryRate from wave count to
partitions-enforced count (wave count remains RetentionEnforcementRate).
jeqo added a commit that referenced this pull request Jul 28, 2026
EnforceRetentionJob runs one transaction per partition (post-#705), yet
EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side
RetentionEnforcementTotalTime, which already measures the whole enforce wave.

Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per
partition) instead, matching the other *QueryTime metrics (one DB operation each)
and yielding per-partition enforce latency without a new metric. The whole-wave
total stays RetentionEnforcementTotalTime on the broker.

Note: this redefines two existing metrics -- EnforceRetentionQueryTime from
per-wave to per-partition, and EnforceRetentionQueryRate from wave count to
partitions-enforced count (wave count remains RetentionEnforcementRate).
giuseppelillo pushed a commit that referenced this pull request Jul 29, 2026
…#725)

* feat(inkless:retention): surface enforcement schedule lag

Add a RetentionEnforcementScheduleLagMs gauge (0 when on schedule; a sustained
positive value means enforcement is falling behind its per-partition cadence),
fed by the scheduler's oldest past-due queue entry read under a lock so the
metrics/JMX thread and the enforcement thread do not race the PriorityQueue.

* feat(inkless:retention): meter enforce latency per partition

EnforceRetentionJob runs one transaction per partition (post-#705), yet
EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side
RetentionEnforcementTotalTime, which already measures the whole enforce wave.

Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per
partition) instead, matching the other *QueryTime metrics (one DB operation each)
and yielding per-partition enforce latency without a new metric. The whole-wave
total stays RetentionEnforcementTotalTime on the broker.

Note: this redefines two existing metrics -- EnforceRetentionQueryTime from
per-wave to per-partition, and EnforceRetentionQueryRate from wave count to
partitions-enforced count (wave count remains RetentionEnforcementRate).

* feat(inkless:retention): make enforcement and file-cleaner logs intent/result pairs

Retention enforcer: promote the per-cycle summary from DEBUG to INFO and add a
matching intent line before the (potentially blocking) enforce call, so a stuck
enforce shows as intent-without-completion:
  "Enforcing retention for {n} partitions"
  "Enforced retention for {n} partitions in {ms} ms: {b} batches, {y} bytes deleted"
One pair per non-empty cycle; empty cycles stay silent (no per-tick heartbeat).
Also fix a latent partition misattribution: the per-partition log lines indexed
readyPartitions while responses align with the delete-policy-filtered requests, so a
filtered-out partition could be mislabeled -- keep an enforcedPartitions list aligned
1:1 with requests/responses instead.

File cleaner: replace the per-run "Running file cleaner at {now}" heartbeat
(redundant with the framework timestamp and the outcome line) with a work-gated
intent/result pair:
  "Running file cleaner: deleting {n} of {m} marked files"  (m-n still within grace)
  "File cleaner deleted {n} files in {ms} ms"
Partition/size counts are not available at this layer (FileToDelete carries only the
object key; batches are already deleted by cleanup time).
giuseppelillo pushed a commit that referenced this pull request Jul 29, 2026
…O(cap) (#722)

The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every
deletable batch, O(partition depth). PR #705 moved it out of the log lock so it
no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain
(retention lowered so almost the whole partition is deletable) a single scan on
a multi-million-batch partition runs for seconds-to-minutes and exceeds the
enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the
still-running backend so the partition never drains.

V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The
boundary can never be deeper than the cap because the delete is clamped there
regardless; the +1 distinguishes an in-window boundary (delete up to it) from a
deeper one that falls through to high_watermark and is re-clamped by the
existing under-lock cap-probe. Each pass is O(cap), index-only on
batches_by_last_offset_covering_idx, and a deep partition drains over
successive enforcement cycles. max_batches_per_request = 0 keeps the original
unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes
oldest-first, so a snapshot-stale boundary can only under-delete).

Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the
bound is on out of the box; it must stay above the per-interval expiry rate or
retention falls behind.

Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via
-Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k
batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms
(flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is
the sweet spot: short passes at 10x the drain throughput of 100.

[KC-354]
giuseppelillo pushed a commit that referenced this pull request Jul 29, 2026
…#725)

* feat(inkless:retention): surface enforcement schedule lag

Add a RetentionEnforcementScheduleLagMs gauge (0 when on schedule; a sustained
positive value means enforcement is falling behind its per-partition cadence),
fed by the scheduler's oldest past-due queue entry read under a lock so the
metrics/JMX thread and the enforcement thread do not race the PriorityQueue.

* feat(inkless:retention): meter enforce latency per partition

EnforceRetentionJob runs one transaction per partition (post-#705), yet
EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side
RetentionEnforcementTotalTime, which already measures the whole enforce wave.

Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per
partition) instead, matching the other *QueryTime metrics (one DB operation each)
and yielding per-partition enforce latency without a new metric. The whole-wave
total stays RetentionEnforcementTotalTime on the broker.

Note: this redefines two existing metrics -- EnforceRetentionQueryTime from
per-wave to per-partition, and EnforceRetentionQueryRate from wave count to
partitions-enforced count (wave count remains RetentionEnforcementRate).

* feat(inkless:retention): make enforcement and file-cleaner logs intent/result pairs

Retention enforcer: promote the per-cycle summary from DEBUG to INFO and add a
matching intent line before the (potentially blocking) enforce call, so a stuck
enforce shows as intent-without-completion:
  "Enforcing retention for {n} partitions"
  "Enforced retention for {n} partitions in {ms} ms: {b} batches, {y} bytes deleted"
One pair per non-empty cycle; empty cycles stay silent (no per-tick heartbeat).
Also fix a latent partition misattribution: the per-partition log lines indexed
readyPartitions while responses align with the delete-policy-filtered requests, so a
filtered-out partition could be mislabeled -- keep an enforcedPartitions list aligned
1:1 with requests/responses instead.

File cleaner: replace the per-run "Running file cleaner at {now}" heartbeat
(redundant with the framework timestamp and the outcome line) with a work-gated
intent/result pair:
  "Running file cleaner: deleting {n} of {m} marked files"  (m-n still within grace)
  "File cleaner deleted {n} files in {ms} ms"
Partition/size counts are not available at this layer (FileToDelete carries only the
object key; batches are already deleted by cleanup time).
giuseppelillo pushed a commit that referenced this pull request Jul 30, 2026
…O(cap) (#722)

The enforce_retention_v2 boundary scan reverse-aggregates byte_size over every
deletable batch, O(partition depth). PR #705 moved it out of the log lock so it
no longer blocks commit, but the scan itself stayed O(depth): on a bulk drain
(retention lowered so almost the whole partition is deletable) a single scan on
a multi-million-batch partition runs for seconds-to-minutes and exceeds the
enforcer's socket.timeout.ms (default 5s), aborting the call and orphaning the
still-running backend so the partition never drains.

V22 caps the scan to the oldest (max_batches_per_request + 1) batches. The
boundary can never be deeper than the cap because the delete is clamped there
regardless; the +1 distinguishes an in-window boundary (delete up to it) from a
deeper one that falls through to high_watermark and is re-clamped by the
existing under-lock cap-probe. Each pass is O(cap), index-only on
batches_by_last_offset_covering_idx, and a deep partition drains over
successive enforcement cycles. max_batches_per_request = 0 keeps the original
unbounded full scan via LIMIT NULL. Correctness is unchanged from V21 (deletes
oldest-first, so a snapshot-stale boundary can only under-delete).

Flip retention.enforcement.max.batches.per.request default 0 -> 1000 so the
bound is on out of the box; it must stay above the per-interval expiry rate or
retention falls behind.

Benchmark (EnforceRetentionCommitBlockingBenchmarkTest, now cap-overridable via
-Dinkless.benchmark.maxBatchesPerEnforce). enforce ms vs depth at 200k/400k/800k
batches: unbounded 359/729/1465 ms (linear O(depth)); bounded cap=1 21/21/29 ms
(flat). A cap sweep shows per-pass cost is flat from cap 1 to 1000, so 1000 is
the sweet spot: short passes at 10x the drain throughput of 100.

[KC-354]
giuseppelillo pushed a commit that referenced this pull request Jul 30, 2026
…#725)

* feat(inkless:retention): surface enforcement schedule lag

Add a RetentionEnforcementScheduleLagMs gauge (0 when on schedule; a sustained
positive value means enforcement is falling behind its per-partition cadence),
fed by the scheduler's oldest past-due queue entry read under a lock so the
metrics/JMX thread and the enforcement thread do not race the PriorityQueue.

* feat(inkless:retention): meter enforce latency per partition

EnforceRetentionJob runs one transaction per partition (post-#705), yet
EnforceRetentionQueryTime wrapped the whole batch -- duplicating the broker-side
RetentionEnforcementTotalTime, which already measures the whole enforce wave.

Record EnforceRetentionQueryTime/QueryRate per enforce_retention_v2 call (one per
partition) instead, matching the other *QueryTime metrics (one DB operation each)
and yielding per-partition enforce latency without a new metric. The whole-wave
total stays RetentionEnforcementTotalTime on the broker.

Note: this redefines two existing metrics -- EnforceRetentionQueryTime from
per-wave to per-partition, and EnforceRetentionQueryRate from wave count to
partitions-enforced count (wave count remains RetentionEnforcementRate).

* feat(inkless:retention): make enforcement and file-cleaner logs intent/result pairs

Retention enforcer: promote the per-cycle summary from DEBUG to INFO and add a
matching intent line before the (potentially blocking) enforce call, so a stuck
enforce shows as intent-without-completion:
  "Enforcing retention for {n} partitions"
  "Enforced retention for {n} partitions in {ms} ms: {b} batches, {y} bytes deleted"
One pair per non-empty cycle; empty cycles stay silent (no per-tick heartbeat).
Also fix a latent partition misattribution: the per-partition log lines indexed
readyPartitions while responses align with the delete-policy-filtered requests, so a
filtered-out partition could be mislabeled -- keep an enforcedPartitions list aligned
1:1 with requests/responses instead.

File cleaner: replace the per-run "Running file cleaner at {now}" heartbeat
(redundant with the framework timestamp and the outcome line) with a work-gated
intent/result pair:
  "Running file cleaner: deleting {n} of {m} marked files"  (m-n still within grace)
  "File cleaner deleted {n} files in {ms} ms"
Partition/size counts are not available at this layer (FileToDelete carries only the
object key; batches are already deleted by cleanup time).
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.

3 participants