Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't address a revision by change id #924

Closed
ilyagr opened this issue Dec 21, 2022 · 12 comments
Closed

Can't address a revision by change id #924

ilyagr opened this issue Dec 21, 2022 · 12 comments

Comments

@ilyagr
Copy link
Collaborator

ilyagr commented Dec 21, 2022

I can't seem to refer to some revisions by change id in
this repository state.

$ jj log -r 87e04
o d9deb76cecc7 ilyagr@users.noreply.github.com 2022-12-20 17:36:28.000 -08:00 87e04ac37424
~ cmd: Add `--to` and `--from` arguments to `jj touchup`
$ jj log -r d9deb7
Error: Revision "d9deb7" doesn't exist

@yuja , might this have to do with some of your recent changes to change id resolution?

Let me know if this is a crazy typo I can't see.

@yuja
Copy link
Collaborator

yuja commented Dec 21, 2022

might this have to do with some of your recent changes to change id resolution?

Nah, but the undiscoverable revision is entry.change_id() != store.get_commit(entry.commit_id()).change_id().
Maybe you have multiple change ids somehow associated with a single git commit? @martinvonz?

ChangeId("164f07b0822b426684cd355a67983c1d"), ChangeId("164f07b0822b426684cd355a67983c1d"), CommitId("0595387ef76f59a7138f1abc890f4bc1117b33b5")
ChangeId("978ce5729598410d9b1bd3e87ee0d262"), ChangeId("978ce5729598410d9b1bd3e87ee0d262"), CommitId("a133a9cdf92e27761a9818811cbe1f740c1b9dc9")
ChangeId("b79ef7a3dc1f849ea06ba57060ee258d"), ChangeId("b79ef7a3dc1f849ea06ba57060ee258d"), CommitId("f879344257856759d78ab908c00b3a03522addcf")
ChangeId("f518cdb60c14a52174e19e7aba02d65c"), ChangeId("f518cdb60c14a52174e19e7aba02d65c"), CommitId("b7ea512ff67d0577ccddcafbf5861a06bc862bff")
ChangeId("45b3b056842d45a273932590b227dd79"), ChangeId("45b3b056842d45a273932590b227dd79"), CommitId("888b4666d83382c3c06811ba6b5c8fc39a3f8cd1")
ChangeId("b99e18206ec470cca650a6b4cab138a7"), ChangeId("d9deb76cecc7350e75b9a7a88349242e"), CommitId("87e04ac3742492c115e59dae70ace33736ed7b9b")
...
diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 12245cab44...ef9ed47765 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -124,6 +124,12 @@
         // TODO: Create a persistent lookup from change id to (visible?) commit ids.
         for index_entry in RevsetExpression::all().evaluate(repo, None).unwrap().iter() {
             let change_id = index_entry.change_id();
+            let commit = repo.store().get_commit(&index_entry.commit_id()).unwrap();
+            eprintln!(
+                "{change_id:?}, {change_id2:?}, {commit_id:?}",
+                change_id2 = commit.change_id(),
+                commit_id = index_entry.commit_id()
+            );
             if change_id.hex().starts_with(hex_prefix.hex()) {
                 if let Some(previous_change_id) = found_change_id.replace(change_id.clone()) {
                     if previous_change_id != change_id {

@ilyagr
Copy link
Collaborator Author

ilyagr commented Dec 21, 2022 via email

@martinvonz
Copy link
Owner

Nah, but the undiscoverable revision is entry.change_id() != store.get_commit(entry.commit_id()).change_id(). Maybe you have multiple change ids somehow associated with a single git commit? @martinvonz?

ChangeId("164f07b0822b426684cd355a67983c1d"), ChangeId("164f07b0822b426684cd355a67983c1d"), CommitId("0595387ef76f59a7138f1abc890f4bc1117b33b5")
ChangeId("978ce5729598410d9b1bd3e87ee0d262"), ChangeId("978ce5729598410d9b1bd3e87ee0d262"), CommitId("a133a9cdf92e27761a9818811cbe1f740c1b9dc9")
ChangeId("b79ef7a3dc1f849ea06ba57060ee258d"), ChangeId("b79ef7a3dc1f849ea06ba57060ee258d"), CommitId("f879344257856759d78ab908c00b3a03522addcf")
ChangeId("f518cdb60c14a52174e19e7aba02d65c"), ChangeId("f518cdb60c14a52174e19e7aba02d65c"), CommitId("b7ea512ff67d0577ccddcafbf5861a06bc862bff")
ChangeId("45b3b056842d45a273932590b227dd79"), ChangeId("45b3b056842d45a273932590b227dd79"), CommitId("888b4666d83382c3c06811ba6b5c8fc39a3f8cd1")
ChangeId("b99e18206ec470cca650a6b4cab138a7"), ChangeId("d9deb76cecc7350e75b9a7a88349242e"), CommitId("87e04ac3742492c115e59dae70ace33736ed7b9b")
...

Good find!

You should be able to work around it by removing the index, letting it be recalculated on next access. rm .jj/repo/index/operations/* would do that.

I don't know what would have caused this to happen. The fact that your report came so soon after PR #883 makes me think that I may have missed a reason that caching the store is not safe. I'll have to think more about that another day. Let me know if your jj binary is from before that PR.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Dec 21, 2022

No, it's newer I'm pretty sure. OTOH, I only upgraded the jj binary that day for that version of the repo.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Jan 7, 2023

I ran into this again. Unfortunately, currently my .jj is 80 MB, and I can only upload up to 25MB to Github. I'm not sure if it's worth the trouble to find another way to share it.

@martinvonz
Copy link
Owner

Two possibly useful pieces of information:

  • d9deb76cecc7350e75b9a7a88349242e, i.e. the value from the store, is change ID that's automatically generated from the commit ID when no extra metadata is found
  • The extra metadata was actually written to disk, but the .jj/repo/store/extra/heads/ was not updated. I haven't figured out which file has the data, but if you do cp .jj/repo/store/extra/??????* .jj/repo/store/extra/heads/ in the repo you shared above, you'll see that jj log -r 87e04 produces the correct result afterwards. If it made no sense why that command helped, see my unfinished architecture doc at https://github.com/martinvonz/jj/blob/30e057ed222f9446990484a7af3be74bc07b30c8/docs/technical/architecture.md#stackedtable has some information about how that works. By adding all files as heads, we cause the store to merge in all tables into one.

@ilyagr
Copy link
Collaborator Author

ilyagr commented Feb 22, 2023

In case it helps, here's another example. Here, a bunch of revisions are affected such as uzyo, kpun, etc. The cp command you suggested changed all the affected change ids, and everything started to work.

jj_no_uzyo_kpun.tar.gz

@ilyagr
Copy link
Collaborator Author

ilyagr commented May 12, 2023

I am not 100% sure it's the same bug, but it seems so. I got into this repo state:

jj_weirdly_obsolete2.tar.gz

(I ran git maintenance run to make the archive small enough to put here, but the problem existed before that as well)

jj log shows the following. Note how an un-abandoned commit is a child of abandoned commits. I didn't abandon any commits. Checking the past repo states (jj log --at-op) seems to show these commits as abandoned at every past operation, even though they definitely weren't.

image

@yuja
Copy link
Collaborator

yuja commented May 15, 2023

Ok, minimal script to create corrupted index:

for i in `seq 10`; do
    jj new root -m a &
done

wait
jj log --no-pager

Maybe something wrong with concurrent & hash-conflicting commits?

@yuja
Copy link
Collaborator

yuja commented May 15, 2023

Maybe something wrong with concurrent & hash-conflicting commits?

Wild guess:

  • A commit created by jj has a change id assigned by jj,
  • but git backend doesn't hash the change id (because it isn't a part of the git commit.)
  • So concurrent jj processes may observe (temporarily) different jj commit objects identified by a same commit id.

@ilyagr
Copy link
Collaborator Author

ilyagr commented May 15, 2023

If I understand correctly, the theory is that two things happen simultaneously:

  1. one jj process assigns a random change id to a new change and writes a git commit, but doesn't yet have the time to write down that change id.
  2. a simultaneous jj process notices the new git commit, and loads it into the jj repository with a change id generated from the git commit id.

Then, both processes write the change id they think is correct to the index, and havoc ensues.

This sounds very plausible to me, thanks @yuja!

@yuja
Copy link
Collaborator

yuja commented May 16, 2023

1. one jj process assigns a random change id to a new change and writes a git commit, but doesn't yet have the time to write down that change id.

2. a simultaneous jj process notices the new git commit, and loads it into the jj repository with a change id generated from the git commit id.

Just for the record, quick and dirty repro script based on this scenario (might need to adjust parameters):

set -e
set -x

rm -Rf repo
git init repo && jj init --git-repo=repo repo
#jj init --git repo
cd repo

export JJ_TIMESTAMP=1970-01-02T00:00:00+00:00
#jj new && jj branch set master
for i in `seq 10`; do
    jj new -m "_${i}_"
    #jj branch set "b$i"
done

for i in `seq 10`; do
    rev="$(jj --ignore-working-copy log --no-graph -T commit_id -r 'description(_1_)')"
    if [ "$(expr $i % 2)" -eq 0 ]; then
        jj rebase -s "$rev" -d master &
    else
        jj rebase -s "$rev" -d root &
    fi
    for j in `seq 40`; do
        #( jj git import; jj git export ) &
        jj status > /dev/null &
    done
    wait
    if jj --ignore-working-copy log -r 'all()' | grep hidden; then
        break
    fi
done

#wait
jj log --no-pager
#jj op log --no-pager

yuja added a commit to yuja/jj that referenced this issue May 19, 2023
Since non-Git metadata isn't hashed, we can't rely on the consistency
provided by content-addressed storage. The problem is also described in
martinvonz#3 (comment)

martinvonz#924
yuja added a commit to yuja/jj that referenced this issue May 19, 2023
My first attempt was to fix up corrupted index when merging, but it turned
out to be not easy because the self side may contain corrupted data. It's
also possible that two concurrent commit operations have exactly the same
view state (because change id isn't hashed into commit id), and only the
table heads diverge.

martinvonz#924
yuja added a commit to yuja/jj that referenced this issue May 19, 2023
yuja added a commit to yuja/jj that referenced this issue May 19, 2023
Since non-Git metadata isn't hashed, we can't rely on the consistency
provided by content-addressed storage. The problem is also described in
martinvonz#3 (comment)

martinvonz#924
yuja added a commit to yuja/jj that referenced this issue May 19, 2023
My first attempt was to fix up corrupted index when merging, but it turned
out to be not easy because the self side may contain corrupted data. It's
also possible that two concurrent commit operations have exactly the same
view state (because change id isn't hashed into commit id), and only the
table heads diverge.

martinvonz#924
yuja added a commit to yuja/jj that referenced this issue May 19, 2023
yuja added a commit that referenced this issue May 20, 2023
Since non-Git metadata isn't hashed, we can't rely on the consistency
provided by content-addressed storage. The problem is also described in
#3 (comment)

#924
yuja added a commit that referenced this issue May 20, 2023
My first attempt was to fix up corrupted index when merging, but it turned
out to be not easy because the self side may contain corrupted data. It's
also possible that two concurrent commit operations have exactly the same
view state (because change id isn't hashed into commit id), and only the
table heads diverge.

#924
@yuja yuja closed this as completed in a942246 May 20, 2023
yuja added a commit to yuja/jj that referenced this issue May 21, 2023
If I spawned ~20 "jj status &" processes, some of them panicked there.
Spotted when debugging martinvonz#924.
yuja added a commit that referenced this issue May 21, 2023
If I spawned ~20 "jj status &" processes, some of them panicked there.
Spotted when debugging #924.
yuja added a commit to yuja/jj that referenced this issue Jun 26, 2023
yuja added a commit to yuja/jj that referenced this issue Jun 26, 2023
yuja added a commit to yuja/jj that referenced this issue Jan 26, 2024
This is the source of the problem I noticed when debugging martinvonz#924 and martinvonz#1608. I
don't think this can be easily fixed, so let's document it.
yuja added a commit that referenced this issue Jan 26, 2024
This is the source of the problem I noticed when debugging #924 and #1608. I
don't think this can be easily fixed, so let's document it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants