tahoe check on LIT produces KeyError

[kmarkley86]

If I "tahoe check" a path that is actually a DIR-IMM (an empty directory), or a LIT file, I get an exception:

Traceback (most recent call last):
  File "/usr/local/bin/tahoe", line 9, in <module>
    load_entry_point('allmydata-tahoe==1.9.1', 'console_scripts', 'tahoe')()
  File "/usr/local/lib/python2.7/site-packages/allmydata/scripts/runner.py", line 113, in run
    rc = runner(sys.argv[1:], install_node_control=install_node_control)
  File "/usr/local/lib/python2.7/site-packages/allmydata/scripts/runner.py", line 99, in runner
    rc = cli.dispatch[command](so)
  File "/usr/local/lib/python2.7/site-packages/allmydata/scripts/cli.py", line 589, in check
    rc = tahoe_check.check(options)
  File "/usr/local/lib/python2.7/site-packages/allmydata/scripts/tahoe_check.py", line 80, in check
    stdout.write("Summary: %s\n" % quote_output(data["summary"], quotemarks=False))
KeyError: 'summary'

On the other hand, "tahoe deep-check" is fine for the directory, but gives an unhelpful error if given the path to a LIT file:

ERROR: 400 Bad Request POST to file: bad t=stream-deep-check

[davidsarah]

Putting this in 1.10 (not 1.9.2 since I don't think it's a regression).

[killyourtv]

Replying to davidsarah:

Putting this in 1.10 (not 1.9.2 since I don't think it's a regression).

I can confirm that it happens in 1.8.3.

[davidsarah]

Note that it is only checking a LIT file/dir directly that fails, not deep-checking a non-literal directory that contains LIT files or dirs. (The latter is tested, for the case of a LIT file, in test_cli.Check.test_deep_check.)

[davidsarah]

In git/src/allmydata/scripts/tahoe_check.py, DeepCheckOutput.lineReceived does this:

summary = cr.get("summary", "Healthy (LIT)")

whereas in the check function, data["summary"] is used without any fallback for a missing field (and it would also fail a little further on for other fields such as cr["count-shares-good"]).

There are two ways to fix the bug:

a) include "summary", "count-shares-*", "list-corrupt-shares" fields in the output even for LIT files/dirs;

b) make 'tahoe check' tolerate lack of these fields.

Arguably, "summary" makes sense for LIT files and the other fields do not, but that doesn't really help a lot in choosing between a) and b). Any preference? I have a patch for b) but it seems slightly ugly to do it that way.

[davidsarah]

Make 'tahoe check' tolerate lack of certain fields

[davidsarah]

Please review ​https://github.com/tahoe-lafs/tahoe-lafs/pull/28 instead (it includes a test for literal directories, and fixes a minor error in another of the new tests).

[davidsarah]

Note that the pull request doesn't fix the

ERROR: 400 Bad Request
POST to file: bad t=stream-deep-check

mentioned in the Description. That's arguably a different bug, which happens for a deep-check on any non-directory.

[davidsarah]

Replying to davidsarah:

Note that the pull request doesn't fix the ERROR: 400 Bad Request... for a deep-check on any non-directory.

Split to #1898.

[amiller]

Reviewed +1.

I reproduced this KeyError by putting a tiny LIT file and checking it, and checked that the patch fixes it. I also ran through all the variations that the tests cover, --raw and --verify. I couldn't think of anything else that isn't covered.

[davidsarah]

I will commit to trunk.

[warner]

Replying to davidsarah:

There are two ways to fix the bug:

a) include "summary", "count-shares-*", "list-corrupt-shares" fields in the output even for LIT files/dirs;

b) make 'tahoe check' tolerate lack of these fields.

Arguably, "summary" makes sense for LIT files and the other fields do not, but that doesn't really help a lot in choosing between a) and b). Any preference? I have a patch for b) but it seems slightly ugly to do it that way.

Yeah, I didn't want to add non-sensical fake information (like pretending that LIT files have one share that's always present, or suggesting that LIT files have shares at all). It might be ok to have LIT results return empty lists and share-counts of "0", but it'd be confusing and somewhat scary if the renderer shows those numbers in the same way it shows them for CHK and SSK files ("huh? oh no! zero shares?!? Oh, wait, right, LIT."). So we'd want the renderer to not display share-counts/etc for LIT files. Which means we need special-casing code in the renderer anyways. And that code could switch on the file being of type "LIT", but that's disappointingly hard-coded (*any* filetype that doesn't use shares should get the special-case code, not just LITs, it's just that so far LITs are the only non-distributed filetype). So the code should probably switch on things like res["list-corrupt-shares"] is None or "list-corrupt-shares" not in res.

So I guess I'd lean +0 towards (b), and have the 'tahoe check' renderer tolerate missing fields (by not printing anything about them).

[davidsarah]

Replying to warner:

So I guess I'd lean +0 towards (b), and have the 'tahoe check' renderer tolerate missing fields (by not printing anything about them).

I adjusted the patch at ​https://github.com/tahoe-lafs/tahoe-lafs/pull/28 to make it print the share information conditional on presence of the count-* fields (and separately, the list-corrupt-shares field) rather than on lack of the summary field.

[davidsarah]

Fixed in a9272522d5aff534.