search

pre-commit-ci / issues

public issues for https://pre-commit.ci
16 stars 3 forks source link

luacheck: false positives due to cache write race condition #114

Closed hunger closed 2 years ago

hunger commented 2 years ago

Hi pre-commit-ci Team!

I really like pre-commit and was delighted when I learned how easy it is to enable for projects hosted on github. The auto-update also rocks.

But I keep getting failed checks like this one for no reason I can make out:

https://results.pre-commit.ci/run/github/454957552/1646418179.oAkOxRg4QH27-pp7x5qJTg

The check runs just fine locally and the commit is definitely green when I run pre-commit locally.

It is always this test here that fails in CI: https://github.com/lunarmodules/luacheck

Any ideas what I might be doing wrong?

Best Regards, Tobias

asottile commented 2 years ago

that looks like a problem with luacheck itself -- pre-commit.ci is just running pre-commit run --all-files and it looks like it is not multiprocessing safe and so it is erroring on (re)writing a cache file?

asottile commented 2 years ago

I can reproduce the same thing locally by repeatedly trying this command:

rm -rf ~/.cache/luacheck; PATH=$PWD/opt/lua/bin/:$PATH pre-commit run luacheck --all-files

please report the issue to luacheck

asottile commented 2 years ago

yeah -- this bit of code is not safe: https://github.com/lunarmodules/luacheck/blob/81e0835cf5b54b9cb2101b21edeec011dec5fff3/src/luacheck/fs.lua#L177-L201

it needs to work similar to mkdir -p (idempotent) -- or os.makedirs(..., exist_ok=True) -- I don't know enough lua to fix the code but perhaps you do and can submit a PR to fix the issue there

hunger commented 2 years ago

Thank you for looking into this so thoroughly. My Lua is unfortunately extremely limited as well, but I will raise an issue with luacheck. Maybe they can help:-)

alerque commented 2 years ago

Luacheck maintainer here.

Thanks for raising this issue on our repo. I see the trouble and agree the lack of idempotence there makes it not safe to run multiple times. Unfortunately as I noted the issue is actually upstream from us in the luafilesystem library. I will look into whether we can hack in a workaround, but already the "unsafe" block in our code is a workaround for not having an idempotent function to work with.

That being said the reason pre-commit seems to be running into this is it is using luacheck "wrong". It seems like you are instantiating one instance per file to check, but the better way to do it would be to pass a glob of all the files that need processing in one command. There is a --jobs option already so you can multithread the actual linting operation in a safe way. If you do keep calling multiple instances on one file at time there isn't really much point to the cache mechanism and you can use --no-cache to avoid the potential problem. Also, if the cache is being blown away in between runs anyway it also doesn't serve a purpose and should be disabled.

asottile commented 2 years ago

pre-commit runs similar to xargs -P ... (not one per file, that would be very slow!)

the typical way to write a correct makedirs is to avoid TOCTOU by attempting creation and then checking validity (rather than the other way around)

alerque commented 2 years ago

Thanks again. I have a patch that should fix this even without upstream support in the lfs module. It will land in 0.26.0 soon.