fix: #3591949 Cron auto-coverage snaps gap bounds to actual chain rows

Frank Mablyrequested to merge
issue/audit_trail-3591949:3591949-cron-auto-coverage-fails into 1.x

Summary

Cron coverage was failing every tick on chains with lifecycle attestations interleaved between segments, leaving orphan rows in audit_trail indefinitely. Root cause: uncoveredRanges() slices in absolute id space; when the slice boundary fell on an id that belonged to another chain (the audit_trail PRIMARY KEY is shared across chains), createBareSegment()'s anchor lookups threw with "no row id=X" and the bucket was never minted.

Closes #3591949.

Problem

The cron coverage pass (CronArchiveHook::coveragePass()) groups orphan rows past the configured cutoff into closed time buckets and passes each bucket's envelope to ChainArchiver::ensureSegmentCoverage(). When the envelope straddles an existing segment, uncoveredRanges() slices it around the obstacle. Each gap is then materialised via createBareSegment(), which calls readRowHash(chain, to_id) to compute the segment's anchor_after.

The slice boundary min(obstacle.from_id - 1, envelope.to_id) is computed in absolute audit_trail.id space. When that boundary id corresponds to a row on a different chain, readRowHash() finds no row on the target chain at that id and throws:

Cannot resolve anchor_after: chain "[chain]" has no row id=[N].

coveragePass catches the throw, logs a watchdog warning, and continues. The bucket is never minted. Subsequent cron ticks repeat the same failure forever; the orphan rows accumulate on the chain indefinitely and break the "every row past retention is in a segment" invariant the architecture relies on.

The same shape applies to the envelope's own boundary: an operator passing archive(chain, 1, 99999) against a chain whose head is at id=10 (with ids 11..99999 belonging to other chains) would trip createBareSegment()'s to_id > head_id guard.

Fix

Snap each gap's [from_id, to_id] to the actual target-chain row ids inside it before calling createBareSegment(). One bounded SELECT MIN(d.id), MAX(d.id) FROM audit_trail WHERE d.chain = :chain AND d.id BETWEEN :gap_from AND :gap_to per gap. The composite (chain, id) index on audit_trail makes each snap two O(log N) lookups -- sub-millisecond per call, negligible cron-tick overhead.

When the snap returns NULL (a phantom gap whose entire range is on other chains), the gap is silently skipped -- no bare needed, no throw.

Performance impact

  • Index already exists ((chain, id) composite on audit_trail); the snap is two B-tree lookups per gap.
  • Worst-case cron tick: MAX_COVERAGE_PER_TICK = 50 x ~3-5 gaps/bucket = 150-250 SELECTs at ~0.5 ms each ~= ~125 ms additional latency per cron tick, all async.
  • Interactive callers (drush audit_trail:archive, segments admin form): 1-3 extra SELECTs ~= sub-millisecond.
  • Negligible. The pre-fix cost was unbounded (orphans never collected); the fix's cost is fixed-rate.

Test coverage

Multi-chain by default per the new testing convention (tests/src/Traits/AuditTrailKernelTestHelperTrait.php class docblock). Single-chain tests systematically miss cross-chain id-space bugs -- this issue is the canonical example.

Tests added or restructured:

  • testEnsureSegmentCoverageMintsSingleBareForObstacleFreeRange -- envelope past head; snap clamps to webdav-only bounds (entity row excluded).
  • testEnsureSegmentCoverageSlicesAroundMultipleObstacles -- 4 obstacles, 3 gaps; each snap to webdav-only bounds, NOT to the absolute slice boundaries (which fall on entity rows).
  • testEnsureSegmentCoverageSkipsPhantomGapWithNoTargetChainRows (new) -- pins the phantom-gap skip.
  • testArchiveClampsEnvelopePastChainHeadToTargetChainRows (new) -- drush-style large envelope; snap clamps the bare to the two webdav rows.
  • testArchiveRefusesInvertedRange (split from old testArchiveRefusesInvalidRange) -- from > to still rejected upfront by archive().
  • testEnsureSegmentCoverageReleasesCoverageLockOnThrow -- pre-fix used to_id > head_id inducer that the snap now bypasses; switched to acknowledgment-bisection inducer so the lock-release-on-throw invariant is still pinned.

Tests fail without the fix (verified)

All four bug-shape tests were re-run against the pre-fix code (snap stashed); each fails with the production error message:

  • testArchiveClampsEnvelopePastChainHeadToTargetChainRows: Bare-segment to_id=99 exceeds the chain head (id=3).
  • testEnsureSegmentCoverageMintsSingleBareForObstacleFreeRange: Bare-segment to_id=10 exceeds the chain head (id=9).
  • testEnsureSegmentCoverageSlicesAroundMultipleObstacles: Cannot read previous_hash: chain "webdav" has no row id=2. (exact production bug shape)
  • testEnsureSegmentCoverageSkipsPhantomGapWithNoTargetChainRows: Bare-segment to_id=3 exceeds the chain head (id=1).

Restoring the snap turns all four green.

Verification

  • 7/7 testEnsureSegmentCoverage* + testArchive* tests green.
  • 33/33 across the two neighbor classes that also exercise ensureSegmentCoverage: ChainArchiverOverlapTest, CronArchiveHookTest.
  • phpcs clean (Drupal + DrupalPractice standards).
  • Production install: orphan rows confirmed cleared on the next cron run after deployment.

Testing convention added

tests/src/Traits/AuditTrailKernelTestHelperTrait.php class docblock now documents that any test exercising an id-range code path defaults to MULTI-CHAIN setup (provision a second chain, interleave logChained() calls, assert on chain-scoped row sets). Single-chain tests cannot reproduce cross-chain id-space bugs and should not be used as the primary coverage for code paths that scan id ranges.

Edited by Frank Mably

Merge request reports