insert_stream doesn't check references are satisfied

This is a little complex to do well, but not doing it will lead to silent corruption (like in bug 406597 where the output repository was corrupted)

The linked branch does an ok job at noticing missing CHK root entries for added inventories, which would have caught the problem that inspired this bug report.

After chatting with Robert today it would be good to be more paranoid, though: essentially do a reasonably complete "bzr check" on every added revision. The end goal is that if commit_write_group accepts it, we have a very high degree of confidence that the data is sound, so we can be confident that we'll be intercepting corruptions before they damage repositories. This gives better user experience and better bug reports.

Specifically for each added revision verify that an inventory is present (not necessarily a fulltext), and that the set of chks and texts referenced from that inventory (vs. the present parents) are also present. This is essentially what Repository.check already does (although it does even more besides, but perhaps we want that too). Unfortunately, bug 406597 means that simply asking Repository.check to verify new revisions won't work, because it is overly strict with stacked branches. Currently the linked branch is using Repository.check in _commit_write_group and so demonstrates this problem.

So, my short term plan is to propose a merge for a patch doesn't reuse check, but will at least catch missing chk entries. This will insulate us from gross bugs to do with using the wrong generic fetch code (like bug 406597), and is a good incremental change in the desired direction. (Perhaps this partial fix will be considered adequate for the 2.0 release, but I think Robert does not consider that enough by itself.)

After that, I'll try to reuse check, which means fixing bug 406597 and possibly other undiscovered bugs (I've already found that check sometimes explodes rather than reports cleanly in the presence of some problems like missing chk entries). Hopefully that goes well; if not I may just reimplement the logic for detecting missing chk entries and referenced texts and hope we can unify with check later.

On Mon, 2009-08-31 at 10:03 +0000, Andrew Bennetts wrote:
>
> So, my short term plan is to propose a merge for a patch doesn't reuse
> check, but will at least catch missing chk entries. This will
> insulate
> us from gross bugs to do with using the wrong generic fetch code (like
> bug 406597), and is a good incremental change in the desired
> direction.
> (Perhaps this partial fix will be considered adequate for the 2.0
> release, but I think Robert does not consider that enough by itself.)

Right. We do need to merge that patch, or streams from 1.16<->1.18
servers will corrupt local 2a repositories. Its not complete, we know
of other things that can go wrong, and I think we really should nail the
door closed as much as possible.

Ian's just made a couple of windows bugs release blockers, so we may
have time to do this before they are fixed anyway.

-Rob

To be clear "that patch" means one yet to be written that will walk down through all referenced chkinventory pages?

On Mon, 2009-08-31 at 23:09 +0000, Martin Pool wrote:
> To be clear "that patch" means one yet to be written that will walk down
> through all referenced chkinventory pages?

In my last reply here, 'that patch' meant the current one, which closes
the door on the known buggy behaviour of 1.16/1.17.

-Rob

The part of this bug about ensuring just that new revisions have their inventories present, and that the inventories for those revisions have their chk root records present is now bug 423506. The fix is on the way to PQM.

The rest of this bug, checking that all the relevant chk_bytes records are present, and that the corresponding texts referenced by those are present, is still In Progress.

lp:~spiv/bzr/insert-stream-check-chks-part-2 checks for all relevant chk records and text references. It doesn't go deeper than checking that the records are present in the index, but anything more is out of scope for this bug :)

mp https://code.edge.launchpad.net/~spiv/bzr/insert-stream-check-chks-part-2/+merge/11290

spiv says https://code.edge.launchpad.net/~spiv/bzr/insert-stream-check-references has the code that tries to reuse code from check, so it's not up for review at this time.

Landed in r4661 of lp:bzr/2.0