Page MenuHomePhabricator

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

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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?

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.

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.

pulkit added a subscriber: pulkit.Feb 10 2021, 7:02 AM

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.

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.

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
marmoute accepted this revision.Mar 1 2021, 3:54 AM
marmoute added a subscriber: marmoute.

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.

baymax updated this revision to Diff 26110.Mar 5 2021, 1:17 PM

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

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.