Arvados

The bug is not in the actual locking code, the bug is in crunch-dispatch's handling of setting the git tags for locking. This used to work (in a slightly ugly way) but the 4297-dispatch-load branch converted various "bail out and try again" errors into fatal errors, which resulted in this regression.

At 7d6454e

Now that it's normal (during a race) for the "git tag" command to fail, but "git rev-list" failure really means a bad thing, the 2>/dev/null should be removed from "git rev-list" and could be added to "git tag".

Minor things:

These (existing) statements are either debug printfs (which should be removed) or real log messages (which should be prefixed with at least "dispatch: " like [most of] crunch-dispatch's other log messsages):

+        $stderr.puts cmd
+        $stderr.puts `#{cmd}`

Could be more specific/helpful and say "any existing tag" instead of "the tag" here, since "found other tag" would have resulted in the other "existing tag points to blah" error.

+            fail_job job, "'git tag' for #{job.uuid} failed but did not find the tag using 'git rev-list'" 

Tom Clegg wrote:

At 7d6454e

Now that it's normal (during a race) for the "git tag" command to fail, but "git rev-list" failure really means a bad thing, the 2>/dev/null should be removed from "git rev-list" and could be added to "git tag".

Fixed.

Minor things:

These (existing) statements are either debug printfs (which should be removed) or real log messages (which should be prefixed with at least "dispatch: " like [most of] crunch-dispatch's other log messsages):
[...]

Fixed, although we could probably do with a real logger in crunch-dispatch.

Could be more specific/helpful and say "any existing tag" instead of "the tag" here, since "found other tag" would have resulted in the other "existing tag points to blah" error.
[...]

Fixed.

LGTM @ f3b3935, thanks!

At 3e1ef3e

LGTM, except that it'll never work until you call it ArvadosModel::AlreadyLockedError. :)