Report a compaction retry spent by a failed attempt (#739) - #741
Conversation
… in silence (#739) LLP 0217 made a partition maintenance deliberately leaves fragmented a stated outcome, but the skip report needs a recorded effectiveness verdict and the failure path deliberately records none: a rewrite that threw proves nothing about whether the partition can be shrunk. So a retry that failed stamped the cursor, spent its writer generation, and from the next tick on the partition was skipped with no report, no `hyp query maintain` line and no span attribute, indistinguishable from a healthy converged one while it was still fragmented and still held the torn file that broke the rewrite. The stamp now records `attemptFailedAt` beside `writerGeneration`, and the skip branch reports `compactionAttemptFailed` when the stamp is there without an effectiveness record. A recorded verdict still outranks it: a rewrite that committed a verdict and then threw is described by the verdict, which says something about the partition, and not by the error, which does not. A stamp from a writer generation this build does not run is suppressed too, because that partition is owed a fresh attempt rather than frozen. Nothing about dueness changes. The new field is read only by the reporting branch, never by the baseline gate or `compactionVerdictStale`, so it cannot grant a retry and cannot reopen the LLP 0199 rewrite-forever loop. The recognition path drops it where it already drops effectiveness, and a rewrite that commits writes a fresh record, so the note clears itself with the same write that supersedes it. An older build ignores the extra string key, and a cursor without it reads as one whose last attempt is not known to have failed, which is every cursor written before this change. LLP 0218 records the extension; LLP 0217 carries the forward-ref.
… unpinned precedence fixture
An empty-string compaction.attemptFailedAt used to pass the guard in
compactionAttemptFailedAt and print an operator line with a hole
("...failed under this writer at , --force to retry"). Reject '' too.
The precedence anti-regression test asserted only one side of the
competing-records fixture (dataFilesBefore === 8) without pinning that
the late-throw also wrote attemptFailedAt, so a future stamp change that
silently stopped writing it could pass the test while no longer testing
precedence. Assert the fixture invariant explicitly.
|
Review round 1 of This review was briefed with a lesson from sibling PR #740, which described a mechanism wrongly five times because each claim was verified against internals and never against the surface an operator actually sees. So this one drove real 1. minor - an empty-string
|
…o reason
The `|| c.attemptFailedAt === ''` guard added to
`compactionAttemptFailedAt` landed unpinned: deleting it left the whole
suite green, so the malformed operator line ("failed under this writer
at , --force to retry") could come back unnoticed. This is deterministic
cursor-reading logic in a file that already has a dedicated test, which
is exactly what CLAUDE.md asks to be covered traditionally.
The branch is hard to reach by accident, which is the reason to pin it
rather than a reason not to: the obvious reproduction plants a record
that still carries an effectiveness verdict, and that short-circuits at
the verdict guard before the timestamp is ever consulted. The cursor
here is the shape a failed retry actually leaves - this build's writer
generation, taken from the real record so a future generation bump
cannot silently stop the test exercising the branch, and no verdict,
because a rewrite that threw records none.
The second half pins the other direction: the same cursor carrying a
real timestamp is still reported, so a future guard that rejects too
much fails too.
|
Review round 2 of 1. minor - round 1's empty-string fix landed with no test. FIXEDVerified by mutation: deleting Three reasons this was worth raising rather than waving through:
Fixed with a test that discriminates in both directions: it passes at head, and with the guard reverted it fails on exactly the intended assertion and only that one ( Verified from round 1
Also checked, clean
The head has moved to |
|
Triage after the review budget (LLP 0017). Two rounds ran (3 findings, all fixed). Judged mergeable. One deferred item in #742. The operator message, judged as an operator wouldObserved with a torn-parquet fixture and real It clears the bar. The bare ISO timestamp is deliberately the right payload, not a weaker choice than elapsed time or a count: it is the correlation key into the failing tick's And the advice was tested rather than assumed. Following Two preferences, neither blocking: a failed On the #737 interactionThis PR is the mitigation, not undercut by it. The failing tick still aborts the whole walk with a raw error and prints nothing - but the stamp written on the way out means that happens once per writer generation. Verified: every subsequent tick completes the walk, returns exit 0, and prints the standing line. Until #737 lands a per-partition catch, this line is exactly the durable evidence that the one-shot lost tick cannot be. Precedence and lifecycle, verifiedThe verdict-outranks-error rule is enforced twice and consistently: the report branch is an Deferred to #742The daemon discards the report object, so the daemon-side surface is the span attribute alone. That is byte-for-byte the same surface |
philcunliffe
added
the
neutral:approved
1 participant
A compaction retry spent by a failed attempt is now reported on every later tick, instead of leaving the partition indistinguishable from a healthy converged one.
The gap
PR #735 made a stale-verdict retry that throws re-stamp the cursor before rethrowing, so a persistently failing partition is attempted once per writer generation rather than once per tick.
stampWriterGenerationdeliberately records no effectiveness claim, because a failed rewrite proves nothing about the partition.But
compactionKnownIneffectiverequirescompactionReducedFiles === false, which requires a numericdataFilesBefore- deliberately absent after a failure. So the skip-report branch never fired, and later ticks skipped the partition with nocompactionIneffectivereport, no line inhyp query maintain, and no span attribute.LLP 0217 promises a partition maintenance deliberately leaves fragmented is skipped for a stated reason. That held for the recorded-ineffective case and not for the failure case. The failing tick itself logs
daemon.maintenance_failedand its span carries the error, so the evidence exists once - but the ongoing frozen state was invisible, which is the same legibility gap #723 was about, one layer down.The fix
stampWriterGenerationnow also writesattemptFailedAt, a new readercompactionAttemptFailedAtexposes it, and a new skip branch reportscompaction skipped: the retry failed under this writer at <ts>, --force to retry, with acompaction_attempt_failedspan attribute.Deliberately weaker than an ineffective verdict. The failure stamp still records no
dataFilesBefore, socompactionKnownIneffectiveis unchanged and the new report is a separate, weaker statement. Where both could apply, the committed verdict outranks the error that followed it: a rewrite that commits its verdict and then throws reportscompactionIneffective: true, notcompactionAttemptFailed.No extra retries.
attemptFailedAtis read in exactly one place, the reporting branch guarded by!shouldCompact. It is not consulted bygrewSinceCompaction,compactionVerdictStale,needsCompaction, orcompactionDue.stampWriterGenerationstill setswriterGeneration, socompactionVerdictStalestays false exactly as before. Retry-once-per-generation is untouched and--forceremains the override, so LLP 0199's rewrite-forever loop cannot return.Evidence
Test written first, driving the real
maintainCachepath with the merged suite's own torn-file technique (truncate a live parquet data file so the retry's scan throws). Against unmodified source, independently re-derived by the reconciler:It fails for the right reason: the fixture invariants (
assert.rejectson the failing tick, andrecord.dataFilesBefore === undefinedproving the stamp recorded no effectiveness) both pass first, so the failure is precisely "the later tick reports nothing". After the fix: 8/8.Anti-regression is covered two ways. The merged suite's existing tests all pass unchanged, including the one whose second tick asserts
compactionIneffective === true. And a new test,a committed ineffective verdict outranks the failed attempt that followed it, uses the read-only-directory late-throw fixture to prove precedence - it passes both before and after the fix, which is what makes it an anti-regression rather than a driver.Cursor compatibility, both directions
compactionAttemptFailedAtreturnsundefinedunlesstypeof c.attemptFailedAt === 'string', so every pre-existing cursor reads as "last attempt not known to have failed". Covered by every other test in the file, all of which use cursors without the key.compactionobject.tryReadCursorSyncpasses it through untyped and every reader probes only the fields it needs, so an older build ignores it. Nothing validates the record's key set.cursorAfterbuilds the compaction record fresh rather than spreading the old one, so a successful rewrite drops the note in the same write that supersedes it (asserted on the--forcepath).rebaselineCursorspreads, so the key is deleted there explicitly, next to where it already deletesdataFilesBefore.LLP route
Minted LLP 0218 (Decision) with an
Extended-by:forward-ref on LLP 0217, rather than editing 0217. It is Accepted, and CLAUDE.md treats an Accepted doc as a record. This is genuinely additive - a new cursor key and a new report field, for a case 0217's two decisions create between them but neither settles - and it needs prose of its own for the verdict-outranks-error precedence rule and the recognition-path drop, which is more than a forward-ref note can carry. Nothing 0217 settled is contradicted. Checked against every open branch: no number collision.Suite 3996 pass / 0 fail / 1 pre-existing skip; typecheck clean;
llp-ref-hygiene11/11;cache-retention-maintenance38/38 with the LLP 0207 test intact.Fixes #739