This is an archive of the discontinued Mercurial Phabricator instance.

tests: demonstrate a case where a corrupt tag cache causes an abort
ClosedPublic

Authored by mharbison72 on Jan 17 2021, 4:54 PM.

Download Raw Diff

Details

Reviewers

Alphare
marmoute

Group Reviewers

hg-reviewers

Commits

rHG9ea6b75b4a95: tests: demonstrate a case where a corrupt tag cache causes an abort

Summary

I happened to hit this trying to cover other cases around valid vs missing
entries. I have no idea if this is something that could occur more naturally
(similar to how a missing file node in hgtagsfnodes1 can occur after a strip).
There is a test just above this added in f5a7cf0adb12 mentioning it "overwrites
the junk", though that tests truncation instead of actual garbage.

But since this is just a cache, it probably shouldn't abort with a cryptic
message like this. The two options I see both have downsides- either rebuild
the cache (and potentially take a long time), or hint to the user to run a debug
command.

Diff Detail

Repository

rHG Mercurial

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

mharbison72 created this revision.Jan 17 2021, 4:54 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptJan 17 2021, 4:54 PM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

Maybe rebuilding the cache with a warning that it's being rebuilt while saying that they can abort, since I'm assuming rebuilding the cache is atomic?

In D9812#149217, @Alphare wrote:

Maybe rebuilding the cache with a warning that it's being rebuilt while saying that they can abort, since I'm assuming rebuilding the cache is atomic?

Even that would be tricky because at least for the problem I’m trying to solve, you have to delete the cache before rebuilding it. Otherwise, you still get the wrong answer (for the thg problem). It makes me wonder if there are other issues with rebuilding an existing cache. If you’re going to spend the time rebuilding, I guess it should be from scratch. But then aborting it should restore the original cache.

In D9812#149224, @mharbison72 wrote:

Even that would be tricky because at least for the problem I’m trying to solve, you have to delete the cache before rebuilding it. Otherwise, you still get the wrong answer (for the thg problem). It makes me wonder if there are other issues with rebuilding an existing cache. If you’re going to spend the time rebuilding, I guess it should be from scratch. But then aborting it should restore the original cache.

Sure, I was expecting a rebuild from scratch in a separate file with an atomic rename at the very end, so aborting would still work.

Hm, we should probably rebuild the cache or try use the slow path without the cache.

The previous and this patch are of interest to me since I am trying to fix a similar error message related to tags.

In D9812#151055, @pulkit wrote:

Hm, we should probably rebuild the cache or try use the slow path without the cache.
The previous and this patch are of interest to me since I am trying to fix a similar error message related to tags.

@joerg.sonnenberger mentioned that he was going to rewrite the tag cache stuff, and I'm swamped in the short term with other things. So if you want to take this over and do what you need, feel free so that you aren't waiting for me to find the time to get back to this.

pulkit edited parent revisions, added: D10014: debugcommands: prevent using `is False`; removed: D9811: tagcache: distinguish between invalid and missing entries.Feb 17 2021, 2:44 PM

pulkit added a child revision: D10015: debugtagscache: verify that filenode is correct.

pulkit updated this revision to Diff 25652.

In D9812#151121, @mharbison72 wrote:

In D9812#151055, @pulkit wrote:

Hm, we should probably rebuild the cache or try use the slow path without the cache.
The previous and this patch are of interest to me since I am trying to fix a similar error message related to tags.

@joerg.sonnenberger mentioned that he was going to rewrite the tag cache stuff, and I'm swamped in the short term with other things. So if you want to take this over and do what you need, feel free so that you aren't waiting for me to find the time to get back to this.

Thank you, I applied this patch, fixed the failure and based my fixes on this. After my series hg debugtagscache show a message about invalid node and we rebuild the cache for those parts which have corrupted node.

Alphare accepted this revision.Feb 23 2021, 4:31 AM

In D9812#151121, @mharbison72 wrote:

In D9812#151055, @pulkit wrote:

Hm, we should probably rebuild the cache or try use the slow path without the cache.
The previous and this patch are of interest to me since I am trying to fix a similar error message related to tags.

@joerg.sonnenberger mentioned that he was going to rewrite the tag cache stuff, and I'm swamped in the short term with other things. So if you want to take this over and do what you need, feel free so that you aren't waiting for me to find the time to get back to this.

Even if we rework cache behavior in newer format, the older format and cache will remains et we need to have a non-broken behavior for them.

✅ refresh by Heptapod after a successful CI run (🐙 💚)

mharbison72 added a commit: rHG9ea6b75b4a95: tests: demonstrate a case where a corrupt tag cache causes an abort.Mar 8 2021, 3:26 PM

This revision was not accepted when it landed; it landed in state Needs Review.

Closed by commit rHG9ea6b75b4a95: tests: demonstrate a case where a corrupt tag cache causes an abort (authored by mharbison72). · Explain Why

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			tests/test-tags.t (43 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	25090		Jan 17 2021, 4:54 PM	★	★
Diff 2	25652		Feb 17 2021, 2:43 PM	★	★
Diff 3	26110		Mar 5 2021, 1:17 PM	★	★
Diff 4	26143	rHG9ea6b75b4a95f9f98aed6c895bb92a80f6e824e9	Dec 24 2020, 12:23 PM	★	★

Commit	Parents	Author	Summary	Date
957f879ee4cc	cb489598e841	Matt Harbison		Dec 24 2020, 12:23 PM

Status	Author	Revision
Closed	pulkit	D10083 tags: validate nodes in _getfnodes() and update cache in case of unknown nodes
Closed	pulkit	D10015 debugtagscache: verify that filenode is correct
Closed	mharbison72	D9812 tests: demonstrate a case where a corrupt tag cache causes an abort
Closed	pulkit	D10014 debugcommands: prevent using `is False`
Closed	pulkit	D10013 hgtagsfnodes: refactor code to compute fnode into separate fn
Closed	pulkit	D9994 error: remove shortening of node in error message

Diff 25090

tests/test-tags.t

	$ hg debugtagscache \| tail -2			$ hg debugtagscache \| tail -2
	4 0c192d7d5e6b78a714de54a2e9627952a877e25a 0c04f2a8af31de17fab7422878ee5a2dadbc943d			4 0c192d7d5e6b78a714de54a2e9627952a877e25a 0c04f2a8af31de17fab7422878ee5a2dadbc943d
	5 8dbfe60eff306a54259cfe007db9e330e7ecf866 invalid			5 8dbfe60eff306a54259cfe007db9e330e7ecf866 invalid

	$ hg tags			$ hg tags
	tip 5:8dbfe60eff30			tip 5:8dbfe60eff30
	bar 1:78391a272241			bar 1:78391a272241

				BUG: If the filenode part of an entry in hgtagsfnodes is corrupt and
				tags2-visible is missing, `hg tags` aborts. Corrupting the leading 4 bytes of
				node hash (as above) doesn't seem to trigger the issue. Also note that the
				debug command hides the corruption, both with and without tags2-visible.

				$ mv .hg/cache/hgtagsfnodes1 .hg/cache/hgtagsfnodes1.bak
				$ hg debugupdatecaches

				>>> import os
				>>> with open(".hg/cache/hgtagsfnodes1", "rb+") as fp:
				... fp.seek(-16, os.SEEK_END) and None
				... fp.write(b'\xde\xad') and None

				$ f --size --hexdump .hg/cache/hgtagsfnodes1
				.hg/cache/hgtagsfnodes1: size=144
				0000: bb d1 79 df 00 00 00 00 00 00 00 00 00 00 00 00 \|..y.............\|
				0010: 00 00 00 00 00 00 00 00 78 39 1a 27 0c 04 f2 a8 \|........x9.'....\|
				0020: af 31 de 17 fa b7 42 28 78 ee 5a 2d ad bc 94 3d \|.1....B(x.Z-...=\|
				0030: 7a 94 12 77 0c 04 f2 a8 af 31 de 17 fa b7 42 28 \|z..w.....1....B(\|
				0040: 78 ee 5a 2d ad bc 94 3d 6f a4 50 21 7d 3b 71 8c \|x.Z-...=o.P!};q.\|
				0050: 96 4e f3 7b 89 e5 50 eb da fd 57 89 e7 6c e1 b0 \|.N.{..P...W..l..\|
				0060: 0c 19 2d 7d 0c 04 f2 a8 af 31 de 17 fa b7 42 28 \|..-}.....1....B(\|
				0070: 78 ee 5a 2d ad bc 94 3d 8d bf e6 0e 0c 04 f2 a8 \|x.Z-...=........\|
				0080: de ad de 17 fa b7 42 28 78 ee 5a 2d ad bc 94 3d \|......B(x.Z-...=\|

				$ hg debugtagscache \| tail -2
				4 0c192d7d5e6b78a714de54a2e9627952a877e25a 0c04f2a8af31de17fab7422878ee5a2dadbc943d
				5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d

				$ rm -f .hg/cache/tags2-visible
				$ hg debugtagscache \| tail -2
				4 0c192d7d5e6b78a714de54a2e9627952a877e25a 0c04f2a8af31de17fab7422878ee5a2dadbc943d
				5 8dbfe60eff306a54259cfe007db9e330e7ecf866 0c04f2a8deadde17fab7422878ee5a2dadbc943d

				$ hg tags
				abort: data/.hgtags.i@0c04f2a8dead: no match found
				[50]

				BUG: Unless this file is restored, the `hg tags` in the next unix-permissions
				conditional will fail: "abort: data/.hgtags.i@0c04f2a8dead: no match found"

				$ mv .hg/cache/hgtagsfnodes1.bak .hg/cache/hgtagsfnodes1

	#if unix-permissions no-root			#if unix-permissions no-root
	Errors writing to .hgtags fnodes cache are silently ignored			Errors writing to .hgtags fnodes cache are silently ignored

	$ echo dummy2 > foo			$ echo dummy2 > foo
	$ hg commit -m throwaway2			$ hg commit -m throwaway2

	$ chmod a-w .hg/cache/hgtagsfnodes1			$ chmod a-w .hg/cache/hgtagsfnodes1
	$ rm -f .hg/cache/tags2-visible			$ rm -f .hg/cache/tags2-visible