This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11238

rust-nodemap: falling back to C impl as mitigation
ClosedPublic

Authored by gracinet on Aug 2 2021, 6:22 AM.

Details

Reviewers
Alphare
pulkit
Group Reviewers
hg-reviewers
Commits
rHG3fffb48539ee: rust-nodemap: falling back to C impl as mitigation
Summary

This is a mitigation for https://bz.mercurial-scm.org/show_bug.cgi?id=6554

We see sometimes almost all data except the most recent revisions
removed from the persistent nodemap, but we don't know how to
reproduce yet.

This has sadly repercussions beyond just needing to reconstruct the
persistent nodemap: for instance, this automatically filters out
all bookmarks pointing to revisions that the nodemap cannot resolve.
If such filtering happens in a transaction, the update of the
bookmarks file that happens at the end of transaction loses all
bookmarks that have been affected. There may be similar consequences
for other data.

So this is a data loss, something that we have to prevent as soon as
possible.

As a mitigation measure, we will now fallback to the C implementation
in case nodemap lookups failed. This will add some latency, e.g., in
discovery, yet less than disabling the persistent nodemap entirely.

We considered implementing the fallback directly on the Python
side, but revlog.get_rev() is not systematically used, there are
also several direct calls to the index method (self.index.rev() for
a revlog instance). It is therefore more direct to implement the
mitigation in the rust-cpython wrapper.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gracinet created this revision.Aug 2 2021, 6:22 AM
gracinet added a comment.Aug 2 2021, 6:25 AM

This is for the stable branch (mitigation for a bug labeled as blocker)

Alphare accepted this revision as: Alphare.Aug 2 2021, 9:49 AM
Alphare added a subscriber: Alphare.

I'll give other reviewers a little time to review as well, but this looks fine as a workaround. I hope we'll be able to figure out the underlying cause soon.

rust/hg-cpython/src/revlog.rs
68

nit: I'd have added a small comment to reference issue6554 to remove the fallback once it's fixed.

gracinet updated this revision to Diff 29775.Aug 2 2021, 3:00 PM
gracinet marked an inline comment as done.Aug 2 2021, 3:01 PM
gracinet added inline comments.
rust/hg-cpython/src/revlog.rs
68

Good point, thanks.

pulkit accepted this revision.Aug 4 2021, 4:36 AM
This revision is now accepted and ready to land.Aug 4 2021, 4:36 AM
gracinet marked an inline comment as done.Aug 4 2021, 4:43 PM
gracinet added a commit: rHG3fffb48539ee: rust-nodemap: falling back to C impl as mitigation.
Closed by commit rHG3fffb48539ee: rust-nodemap: falling back to C impl as mitigation (authored by gracinet). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 29797

rust/hg-cpython/src/revlog.rs

def get_cindex(&self) -> PyResult<PyObject> { def get_cindex(&self) -> PyResult<PyObject> {
Ok(self.cindex(py).borrow().inner().clone_ref(py)) Ok(self.cindex(py).borrow().inner().clone_ref(py))
} }
// Index API involving nodemap, as defined in mercurial/pure/parsers.py // Index API involving nodemap, as defined in mercurial/pure/parsers.py
/// Return Revision if found, raises a bare `error.RevlogError` /// Return Revision if found, raises a bare `error.RevlogError`
/// in case of ambiguity, same as C version does /// in case of ambiguity, same as C version does
def get_rev(&self, node: PyBytes) -> PyResult<Option<Revision>> { def get_rev(&self, pynode: PyBytes) -> PyResult<Option<Revision>> {
let opt = self.get_nodetree(py)?.borrow(); let opt = self.get_nodetree(py)?.borrow();
let nt = opt.as_ref().unwrap(); let nt = opt.as_ref().unwrap();
let idx = &*self.cindex(py).borrow(); let idx = &*self.cindex(py).borrow();
let node = node_from_py_bytes(py, &node)?; let node = node_from_py_bytes(py, &pynode)?;
nt.find_bin(idx, node.into()).map_err(|e| nodemap_error(py, e)) match nt.find_bin(idx, node.into())
{
AlphareUnsubmitted
Done

nit: I'd have added a small comment to reference issue6554 to remove the fallback once it's fixed.

Alphare: nit: I'd have added a small comment to reference issue6554 to remove the fallback once it's…
gracinetAuthorUnsubmitted
Done

Good point, thanks.

gracinet: Good point, thanks.
Ok(None) =>
// fallback to C implementation, remove once
// https://bz.mercurial-scm.org/show_bug.cgi?id=6554
// is fixed (a simple backout should do)
self.call_cindex(py, "get_rev", &PyTuple::new(py, &[pynode.into_object()]), None)?
.extract(py),
Ok(Some(rev)) => Ok(Some(rev)),
Err(e) => Err(nodemap_error(py, e)),
}
} }
/// same as `get_rev()` but raises a bare `error.RevlogError` if node /// same as `get_rev()` but raises a bare `error.RevlogError` if node
/// is not found. /// is not found.
/// ///
/// No need to repeat `node` in the exception, `mercurial/revlog.py` /// No need to repeat `node` in the exception, `mercurial/revlog.py`
/// will catch and rewrap with it /// will catch and rewrap with it
def rev(&self, node: PyBytes) -> PyResult<Revision> { def rev(&self, node: PyBytes) -> PyResult<Revision> {
match nt.unique_prefix_len_node(idx, &node_from_py_bytes(py, &node)?) match nt.unique_prefix_len_node(idx, &node_from_py_bytes(py, &node)?)
{ {
Ok(Some(l)) => Ok(l), Ok(Some(l)) => Ok(l),
Ok(None) => Err(revlog_error(py)), Ok(None) => Err(revlog_error(py)),
Err(e) => Err(nodemap_error(py, e)), Err(e) => Err(nodemap_error(py, e)),
} }
} }
def partialmatch(&self, node: PyObject) -> PyResult<Option<PyBytes>> { def partialmatch(&self, pynode: PyObject) -> PyResult<Option<PyBytes>> {
let opt = self.get_nodetree(py)?.borrow(); let opt = self.get_nodetree(py)?.borrow();
let nt = opt.as_ref().unwrap(); let nt = opt.as_ref().unwrap();
let idx = &*self.cindex(py).borrow(); let idx = &*self.cindex(py).borrow();
let node_as_string = if cfg!(feature = "python3-sys") { let node_as_string = if cfg!(feature = "python3-sys") {
node.cast_as::<PyString>(py)?.to_string(py)?.to_string() pynode.cast_as::<PyString>(py)?.to_string(py)?.to_string()
} }
else { else {
let node = node.extract::<PyBytes>(py)?; let node = pynode.extract::<PyBytes>(py)?;
String::from_utf8_lossy(node.data(py)).to_string() String::from_utf8_lossy(node.data(py)).to_string()
}; };
let prefix = NodePrefix::from_hex(&node_as_string).map_err(|_| PyErr::new::<ValueError, _>(py, "Invalid node or prefix"))?; let prefix = NodePrefix::from_hex(&node_as_string).map_err(|_| PyErr::new::<ValueError, _>(py, "Invalid node or prefix"))?;
nt.find_bin(idx, prefix) match nt.find_bin(idx, prefix) {
// TODO make an inner API returning the node directly Ok(None) =>
.map(|opt| opt.map( // fallback to C implementation, remove once
|rev| PyBytes::new(py, idx.node(rev).unwrap().as_bytes()))) // https://bz.mercurial-scm.org/show_bug.cgi?id=6554
.map_err(|e| nodemap_error(py, e)) // is fixed (a simple backout should do)
self.call_cindex(
py, "partialmatch",
&PyTuple::new(py, &[pynode]), None
)?.extract(py),
Ok(Some(rev)) =>
Ok(Some(PyBytes::new(py, idx.node(rev).unwrap().as_bytes()))),
Err(e) => Err(nodemap_error(py, e)),
}
} }
/// append an index entry /// append an index entry
def append(&self, tup: PyTuple) -> PyResult<PyObject> { def append(&self, tup: PyTuple) -> PyResult<PyObject> {
if tup.len(py) < 8 { if tup.len(py) < 8 {
// this is better than the panic promised by tup.get_item() // this is better than the panic promised by tup.get_item()
return Err( return Err(
PyErr::new::<IndexError, _>(py, "tuple index out of range")) PyErr::new::<IndexError, _>(py, "tuple index out of range"))

tests/test-persistent-nodemap.t

$ hg debugupdatecache $ hg debugupdatecache
$ hg debugnodemap --metadata $ hg debugnodemap --metadata
uid: * (glob) uid: * (glob)
tip-rev: 5002 tip-rev: 5002
tip-node: b355ef8adce0949b8bdf6afc72ca853740d65944 tip-node: b355ef8adce0949b8bdf6afc72ca853740d65944
data-length: 121088 data-length: 121088
data-unused: 0 data-unused: 0
data-unused: 0.000% data-unused: 0.000%
Sub-case: fallback for corrupted data file
------------------------------------------
Sabotaging the data file so that nodemap resolutions fail, triggering fallback to
(non-persistent) C implementation.
$ UUID=`hg debugnodemap --metadata| grep 'uid:' | \
> sed 's/uid: //'`
$ FILE=.hg/store/00changelog-"${UUID}".nd
$ python -c "fobj = open('$FILE', 'r+b'); fobj.write(b'\xff' * 121088); fobj.close()"
The nodemap data file is still considered in sync with the docket. This
would fail without the fallback to the (non-persistent) C implementation:
$ hg log -r b355ef8adce0949b8bdf6afc72ca853740d65944 -T '{rev}\n' --traceback
5002
The nodemap data file hasn't been fixed, more tests can be inserted:
$ hg debugnodemap --dump-disk | f --bytes=256 --hexdump --size
size=121088
0000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0010: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0020: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0060: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0070: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
0090: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
00f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff |................|
$ mv ../tmp-data-file $FILE $ mv ../tmp-data-file $FILE
$ mv ../tmp-docket .hg/store/00changelog.n $ mv ../tmp-docket .hg/store/00changelog.n
Check transaction related property Check transaction related property
================================== ==================================
An up to date nodemap should be available to shell hooks, An up to date nodemap should be available to shell hooks,