A journal that could not be applied was deleted, and nobody was told either way - #207
Merged
Conversation
…either way
Item 5 of Docs/FIX-PLAN-FROM-SITE-FINDINGS-2026-08-15, in the shape the plan recommended:
the failure is reported, the file is kept, and opening still does not fail.
RollbackJournal.Recover wrapped every journal in try { ... } catch { } under the comment
"Skip corrupted journals" and deleted the file on the successful path only - so a journal
that threw half way left the database carrying half a transaction, took the only evidence
with it, and told nobody. The intent is sound and is kept: one unreadable file must not
lock somebody out of the rest of their data. What changes is that the failure reaches
ITransactionJournal.RecoveryFailures and TransactionalStore.RecoveryFailures, and that the
file survives to be looked at. A journal that WAS applied is still removed.
NOTHING LOGS IT, and that is written into the type rather than left to be found out:
OutWit.Database.Core has no ILogger anywhere, and taking a logging dependency for this is a
decision of a different size than the fix. A caller who never reads the property is no
better off than before. RecoveryFailures is a default interface member returning empty, so
an implementation that recovers all or nothing needs no opinion about it and no existing
implementer had to change.
THE SECOND HALF WAS FOUND BY A TEST AND IS THE MORE INTERESTING ONE. The entry reader's own
catch { break; } stopped at the first unreadable entry and returned what it had, which the
caller could not tell from a journal that ended there - so a torn tail was applied in part
and then deleted as though it had been applied in full. The first attempt to distinguish
them compared the stream position against the length and was WRONG: BinaryReader.ReadBytes
on a truncated entry returns a SHORT array and leaves the stream at its end, so a torn tail
looks exactly like a clean end to anything counting bytes. The damage is now an explicit
flag set at every exit that is not "the file ended", and the short reads are checked.
Checkpoint() claimed in a comment to ensure no orphan journals exist. It never did, and it
must not: since recovery now keeps what it could not apply, deleting orphans would throw
away exactly the evidence just preserved. The comment says that instead.
AND ONE PART HAD AN EMPTY RED SET, which is what sabotaging part by part is for. Restoring
the old empty catch left every case green: a file that is not a journal never throws - it
fails the magic check and comes back as damage - so nothing reached the exception path at
all. A journal held open by somebody else does reach it, which is what a backup agent or a
scanner looks like from here, and that case is now in the fixture. With it, the two parts
have different red sets: one case each.
Core 2376.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Item 5 of
Docs/FIX-PLAN-FROM-SITE-FINDINGS-2026-08-15.md, in the shape the plan recommended: the failure is reported, the file is kept, and opening still does not fail.What was there
…and the file was deleted on the successful path only. So a journal that threw half way left the database carrying half a transaction, took the only evidence with it, and told nobody.
The intent is sound and is kept — one unreadable file must not lock somebody out of the rest of their data. What changes is that the failure reaches
ITransactionJournal.RecoveryFailuresandTransactionalStore.RecoveryFailures, and that the file survives to be looked at. A journal that was applied is still removed.Nothing logs it, and that is written into the type rather than left to be found out.
OutWit.Database.Corehas noILoggeranywhere, and taking a logging dependency for this is a decision of a different size than the fix. A caller who never reads the property is no better off than before.RecoveryFailuresis a default interface member returning empty, so no existing implementer had to change.The second half was found by a test, and it is the more interesting one
The entry reader's own
catch { break; }stopped at the first unreadable entry and returned what it had — which the caller could not tell from a journal that ended there. So a torn tail was applied in part and then deleted as though it had been applied in full.The first attempt to distinguish them compared the stream position against the length, and that was wrong:
BinaryReader.ReadByteson a truncated entry returns a short array and leaves the stream at its end, so a torn tail looks exactly like a clean end to anything counting bytes. The damaged-tail case went red and said so. Damage is now an explicit flag set at every exit that is not "the file ended", and the short reads are checked.Checkpoint()claimed something it never didIts comment said it "ensures no orphan journals exist". It never did — and must not: since recovery now keeps what it could not apply, deleting orphans would throw away exactly the evidence just preserved. The comment says that instead.
One part had an empty red set, which is what sabotaging part by part is for
Restoring the old empty
catchleft every case green. A file that is not a journal never throws — it fails the magic check and comes back as damage — so nothing in the fixture reached the exception path at all.A journal held open by somebody else does reach it, which is what a backup agent or a scanner looks like from here. That case is now in the fixture, and with it the two parts have different red sets, one case each:
catchrestoredAJournalThatCannotBeOpenedIsReportedTestAJournalWithADamagedTailAppliesThePrefixAndReportsTheRestTestPlus the control that keeps them honest: a journal that can be applied is applied, restores the value from before the change, reports nothing, and is deleted.
Core 2376.
🤖 Generated with Claude Code