Page MenuHomeSoftware Heritage

Fix crash on None snapshot.
ClosedPublic

Authored by vlorentz on Mar 5 2020, 12:21 PM.

Details

Summary

Fixes Sentry issue SWH-LOADER-SVN-C

Test Plan

I can't find how to reproduce this

Diff Detail

Repository
rDLDSVN Subversion (SVN) loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Thanks for looking into it.

Something sounds off though. Not sure exactly what yet.

First the report's code is out of sync with the diff's code (took me while to see it) [1]

Anyway. I'm not sure this is the right fix (as i fear we might sweap some more profound issue under the rug).

I'm wondering whether we should have a partial snapshot in any case or not...
I'm wondering why we see only _missing endpoints calls (up to revision_missing) without further storage interations.
If nothing is missing, we still should have, client side, the last revision reference from the list sent to the storage to build a snapshot...
... Thus the snapshot should be able to be created...

i can't find how to reproduce

One way i see is finding a small repository as a dump (the svn repository reported is a big one).
(Or create one?)

Then add it to the loader-svn's data test.
Then create a test scenario which loads that repository.
The scenario should fail the same way the production does.
It's tedious but that's how previous issues got fixed with tests.


[1] diff's code is snapshot.id and not snapshot['id'] (report).
(I sometimes do not rely on the version reported as it sometimes misguided me)
Still, the code reports v0.0.51, v0.0.52 is running now.
I expect the issue to still be there though.

[2] Note (for later):
when i look into the loader-svn code, i still see dicts instead of model objects.
Which means there is still that migration to finish (out-of-scope here).
I remember that i only fixed the ci by migrating the necessary dict to models (without breaking implementation).

Sorry for the blurb...

douardda added a subscriber: douardda.

The annotation part should be done on the whole module and, most importantly, in a dedicated revision.

The actual fix consist in a single 'if snapshot:' but this not what the diffstat "shows".

This revision now requires changes to proceed.Mar 12 2020, 1:57 PM

The annotation part should be done on the whole module and, most importantly, in a dedicated revision.

The actual fix consist in a single 'if snapshot:' but this not what the diffstat "shows".

I only added annotations on code I changed.

The annotation part should be done on the whole module and, most importantly, in a dedicated revision.

The actual fix consist in a single 'if snapshot:' but this not what the diffstat "shows".

I only added annotations on code I changed.

Wait, i found those annotation change on the code changed reasonable.
Against spend an insane amount of time typing all the module in one go (which must be a pita).
I prefer the incremental approach.

@douardda So what's wrong here?

The fact that val did not say he added types it in the description or that he did add the types?

I kinda do the same.
I increase the types of the code when i'm incrementally changing it.
I'd like to know if that's wrong.

For my part, i think we should always end up with a snapshot.
So the real fix is finding out why the snapshot ends up None.

If nothing new, that should be the same as previous snapshot.
Else, well, it's a new one.

Well, this case was explicitly written (else: return None), so I assumed there was a good reason.

But I don't even understand why revision may be None in generate_and_load_snapshot

Well, this case was explicitly written (else: return None), so I assumed there was a good reason.

I don't think there was.
I'm most probably the author. And i'm disagreeing with young me on that one ;)
I probably did not see everything clearly then.

See D2860 btw which is somewhat related for the package loaders :)

But I don't even understand why revision may be None in generate_and_load_snapshot

Same here


Btw, another solution to test this is to patch the loader...

mmm, it appears i'm wrong about the None snapshot.
We do have case where we can have it, D2860 showed it to me!

The annotation part should be done on the whole module and, most importantly, in a dedicated revision.

The actual fix consist in a single 'if snapshot:' but this not what the diffstat "shows".

I only added annotations on code I changed.

Wait, i found those annotation change on the code changed reasonable.
Against spend an insane amount of time typing all the module in one go (which must be a pita).
I prefer the incremental approach.

@douardda So what's wrong here?

The fact that val did not say he added types it in the description or that he did add the types?

I kinda do the same.
I increase the types of the code when i'm incrementally changing it.
I'd like to know if that's wrong.

Sorry I missed the notif here.

My point is the git revision for that fix should only be the fix itself (be it a one liner). The annotation should be in a dedicated one.

Whether the annotation should be only the modified part or the whole file is another discussion. I prefer doing the whole file at once, because it's a pain in the neck to have inconsistent stuff everywhere, (but doing the whole package at once is a bit extreme.)

Same as we did when converting unit tests to pytest, we did not convert tests one at a time, we did (at least) one test file as a whole (in fact we often did much more than just one file).

The annotation part should be done on the whole module and, most importantly, in a dedicated revision.

The actual fix consist in a single 'if snapshot:' but this not what the diffstat "shows".

I only added annotations on code I changed.

Also this is not even (strictly) true here. The modified code is SvnLoader.store_data() but the annotated one is SvnLoader.henerate_and_load_snapshot(). But meh.

This revision is now accepted and ready to land.Mar 26 2020, 10:49 AM
This revision was automatically updated to reflect the committed changes.