insert_stream doesn't check references are satisfied

Bug #406687 reported by Robert Collins on 2009-07-30
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Bazaar
Critical
Andrew Bennetts
2.0
Critical
Andrew Bennetts

Bug Description

StreamSink.insert_stream doesn't make sure that the references an inventory has are satisified by the stream, leading to repositories with insufficient data when bugs happen in the stream generator.

Robert Collins (lifeless) wrote :

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)

Changed in bzr:
importance: Undecided → High
milestone: none → 2.0
status: New → Triaged
Changed in bzr:
importance: High → Critical
Andrew Bennetts (spiv) on 2009-08-28
Changed in bzr:
assignee: nobody → Andrew Bennetts (spiv)
Martin Pool (mbp) on 2009-08-28
Changed in bzr:
status: Triaged → In Progress
Andrew Bennetts (spiv) wrote :

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

Martin Pool (mbp) wrote :

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

Robert Collins (lifeless) wrote :

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

Andrew Bennetts (spiv) wrote :

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.

Changed in bzr:
milestone: 2.0 → 2.0rc2
Changed in bzr:
milestone: 2.0 → none
Andrew Bennetts (spiv) wrote :

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 :)

Changed in bzr:
status: In Progress → Fix Committed
Martin Pool (mbp) wrote :
Martin Pool (mbp) on 2009-09-09
Changed in bzr:
status: Fix Committed → In Progress
Andrew Bennetts (spiv) wrote :

Landed in r4661 of lp:bzr/2.0

Andrew Bennetts (spiv) on 2009-09-18
Changed in bzr:
status: In Progress → Fix Released
John A Meinel (jameinel) on 2009-10-14
Changed in bzr:
milestone: none → 2.0.0
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers