>>> [!note] Migrated issue <!-- Drupal.org comment --> <!-- Migrated from issue #3591878. --> Reported by: [mably](https://www.drupal.org/user/3375160) Related to !8 >>> <h3 id="summary-problem-motivation">Problem/Motivation</h3> <p>The module uses six distinct lock names. After the deadline-bug fix and renewal pattern introduced in #3591871, an audit of every remaining lock surfaced two issues that deserve attention as a follow-up.</p> <h3>Lock inventory</h3> <p>Each row: lock name -&gt; acquirer -&gt; typical critical-section duration -&gt; TTL -&gt; status.</p> <ul> <li><code>audit_trail.write:[chain]</code> via <code>ChainArchiver::acquireChainWriteLock()</code>. Most callers (purge / file-purge / createBareSegment / transientPurgeSegment) hold the lock for one bounded operation, ~50ms. Two callers run a per-row loop under the lock whose duration is data-dependent: <code>archiveSegment()</code> writes NDJSON line-by-line for the segment range, and <code>restoreLocked()</code> re-INSERTs each line back into the live table. Both can exceed the 30s TTL on multi-million-row segments. <strong>Issue A below.</strong></li> <li><code>audit_trail.coverage:[chain]</code> via <code>ChainArchiver::acquireCoverageLock()</code>, up to 50 INSERTs. TTL 60s + 20s renewal. Fixed in #3591871.</li> <li><code>audit_trail.write:[chain]</code> via <code>AuditTrailChainWriter::writeChainedRow()</code>, ~10-20ms per chain row. TTL 5s. <strong>Issue B below.</strong></li> <li><code>audit_trail.checkpoint:[chain]</code> via <code>AuditTrailVerifier::mintCheckpoint()</code>, ~milliseconds. TTL 5s. Single try-acquire, no deadline loop. No issue.</li> <li><code>audit_trail.chain_writer.dropped:counter</code> via <code>AuditTrailChainWriter</code>, ~microseconds (one State::set). TTL 1s. Single try-acquire, no issue.</li> <li><code>audit_trail.cron_archive_throttle:[chain]</code> via <code>CronArchiveHook::cron()</code>, throttle gate only. TTL 1s. Single try-acquire, no issue.</li> </ul> <h3>Issue A: chain-write lock TTL can expire mid-loop in archiveSegment() and restoreLocked()</h3> <p>Two chain-write-lock holders run a per-row loop whose runtime scales linearly with row count, and can therefore outlive the 30s TTL on large segments:</p> <ul> <li><code>ChainArchiver::archiveSegment()</code> writes one NDJSON line per row in the segment range, then hashes the whole file with <code>hash_file('sha256')</code>. Disk-bandwidth bound.</li> <li><code>ChainArchiver::restoreLocked()</code> <code>fgets()</code>-loops through the archive file, re-canonicalizes and re-INSERTs each row.</li> </ul> <p>Realistic durations:</p> <ul> <li>100 rows: ~1s.</li> <li>1000 rows: ~10s.</li> <li>10000+ rows: 100s+. Crosses the 30s TTL.</li> </ul> <p>For <code>restoreLocked()</code> specifically, the lock is explicitly documented as <strong>defensive</strong> at <code>ChainArchiver::restore()</code> (lines 2201-2210): chain integrity is actually protected at the DB level by <code>UNIQUE(chain, previous_hash)</code>. Restore touches a disjoint id window from the live head, so even if the TTL expires mid-loop and a concurrent logger write claims the chain-write lock, the two writers operate on disjoint slots and disjoint <code>previous_hash</code> regions. No fork, no corruption -- but the defensive layer is temporarily gone.</p> <p>For <code>archiveSegment()</code>, the consequence of TTL expiry is more concerning: a concurrent logger write on the chain could land WHILE the archive writer is mid-file. The archive's NDJSON envelope captures the live row state at archive time; if a sibling write lands at the head between the start of the archive loop and its end, the chain row sequence the archive snapshotted may not be the sequence on disk in the live table at the moment the segment row is stamped. Verifier reconciliation still works (the NDJSON file is the source of truth for the archived range), but operators inspecting timing forensically could see an off-by-one between archive boundaries and concurrent activity.</p> <p>Real correctness risk: low for both. Defensive guarantee: degraded.</p> <h3>Issue B: AuditTrailChainWriter has the same deadline-vs-short-circuit bug fixed in #3591871</h3> <p><code>AuditTrailChainWriter::writeChainedRow()</code> line ~131:</p> <p>The retry loop reads roughly: set <code>deadline = now + 5s</code>; while we can't acquire the lock, check whether the wait returned <code>FALSE</code> (lock free) AND the deadline has passed; only then bump the drop counter and throw. The conjunction is the bug.</p> <p>Same bug shape as <code>ChainArchiver::acquireChainWriteLock()</code> had before #3591871: when <code>wait()</code> returns TRUE (timed out without the lock becoming available), <code>!wait()</code> is FALSE and the "and" short-circuits before the deadline check. If a foreign holder keeps the lock for longer than 5s, every <code>wait()</code> returns TRUE, the deadline check never fires, the loop spins forever instead of giving up and bumping the drop counter.</p> <p>Real correctness risk: medium. The drop-counter contract -- "give up after 5s and surface the drop on /admin/reports/status" -- is silently violated; chained writes block indefinitely under sustained contention instead.</p> <h3>Proposed resolution</h3> <p>For Issue A, apply the same TTL + renewal pattern that #3591871 introduced for the coverage lock: <code>CHAIN_WRITE_LOCK_TTL_S</code> + <code>CHAIN_WRITE_LOCK_RENEW_INTERVAL_S</code> constants, plus a <code>renewChainWriteLockIfStale()</code> helper called inside the two per-row loops -- one call site in <code>archiveSegment()</code> (per-line NDJSON write loop) and one in <code>restoreLocked()</code> (per-line replay loop). Every other chain-write-lock holder is bounded and does not need the renewer.</p> <p>For Issue B, replace the the "wait result AND deadline" conjunction conjunction with an unconditional deadline check, identical to the fix already shipped for <code>ChainArchiver::acquireChainWriteLock()</code> in #3591871. Use <code>wait()</code> only for its back-off side effect; the deadline check stands on its own.</p> <h3>Why not bump the TTL for Issue A instead of adding renewal?</h3> <p>A 600s TTL covers practical restore sizes but leaves the lock held for 10 minutes after a crashed restore worker -- blocking every legitimate caller for the same window. The renewal pattern gives both bounded recovery on crash (lock frees within TTL) and unbounded happy-path runtime.</p> <h3>Related</h3> <ul> <li>#3591871 (parent: introduced the TTL + renewal pattern on the coverage lock; fixed the same deadline short-circuit bug on <code>ChainArchiver::acquireChainWriteLock()</code>).</li> <li>#3591838 (grandparent: cron coverage-pass refactor).</li> </ul> <h3>Remaining tasks</h3> <ul> <li>Issue A: add chain-write TTL + renew constants, <code>renewChainWriteLockIfStale()</code> helper, wire into BOTH <code>archiveSegment()</code>'s NDJSON-write loop AND <code>restoreLocked()</code>'s per-line replay loop. Kernel tests for long-archive and long-restore renewal.</li> <li>Issue B: fix the deadline short-circuit in <code>AuditTrailChainWriter::writeChainedRow()</code>. Kernel test pinning that a contended chain-write actually drops after 5s instead of blocking forever.</li> </ul> <h3>API changes</h3> <p>None for external callers.</p> <h3>Data model changes</h3> <p>None.</p>