add back invalid cache reporting

[kkmuffme]

which was removed in #9889 - now we won't report for every error but if there are at least 10 errors (chosen arbitrarily)

Cherry picked from #9914

[kkmuffme]

@orklah this restores psalm's current behavior for the cache (5.12.0 and before), except the failure limit is 10 instead of 1.

If we really want to ignore errors in cache reads/writes, someone who wants this needs to create a PR for that to make it consistent in all locations where cache data is used (and either add a configuration option to enable/disable this behavior OR report a single message at the end of psalm's run to stderr that invalid cache data was found/writes failed)

But for the time being, I think it's a bad idea that some cache errors are ignored while many (most?) are not.

[ro0NL]

IMHO cache should be disposable

[ygottschalk]

I think it's a bad idea that some cache errors are ignored while many (most?) are not.

if ($this->error_count > 10)

Why then ignore 10 arbitrary errors...?

After reading through the discussion in the original PR, I felt like there are four things important for the ppl discussing (in no particular order):

1. User should be informed about issues with cache
2. Cache should be disposable
3. Handling of cache errors should be consistent
4. Cache should be self-healing / repairing

(Not sure if I do understand 'disposable' in this context correctly, but I assume it is smth along the lines of 'if not there / corrupted do not fail execution')

I 100% agree with 1 and 4.
IMO:

• Emit an information that cache is not found / corrupted or has self-repaired itself when --debug or --verbose given, else just be silent.
• Throw if a serious error happened (eg psalm is explicitly configured to use cache but cache dir is not writeable)

For 3: Well optimal case would be that. But require that every little possibility is handled in a consistent manner before accepting / merging that bit of code is overkill. Every little step will help and step-by-step we will get there.

For 2: Yes, but not 100% silent (see my points about 1&4)

[kkmuffme]

This PR is not to discuss about whether cache should be disposable or not. I totally agree that it's fine to make the cache disposable, please create a PR for that.

This PR just adds back consistent behavior as it has been

Why then ignore 10 more errors...?

As a compromise. It was 1 error originally, then it was PRed that it inconsistently ignores some errors but others not. This is just a preliminary compromise.
If that's not wanted though, I will change it back to throw immediately on the first error - how it has been all the time before anyway.

Emit an information that cache is not found / corrupted or has self-repaired itself when --debug or --verbose given, else just be silent.

I wouldn't necessarily report only when --debug is set, but generally report these (in stderr?), as otherwise you might not be aware that there is an issue with cache at all.
This is like how we currently report some config options will be set to "true" in psalm 6 - in that kind of style it would make sense I guess.

Throw if a serious error happened (eg psalm is explicitly configured to use cache but cache dir is not writeable)

I don't think so. Either it's disposable or not; otherwise we will end up in a discussion on what is "serious" and what is not? e.g. is a E_WARNING serious? (when we (un)serialize/compress)

For 3: Well optimal case would be that. But require that every little possibility is handled in a consistent manner before accepting / merging that bit of code is overkill. Every little step will help and step-by-step we will get there.

Making it consistent is just removing all the "throw" where cache data is used explicitly. This is not a big task/PR, so shouldn't be an issue to PR this as one.

Why this is important that it's done as one: otherwise the origin of the issue is hidden. E.g. it will throw at a place X, but the issue already happened 5 steps prior when reading from cache, but you don't know that (since that is ignored/not thrown)
It will make identifying any actual cache issues impossible.

[kkmuffme]

@ro0NL yes, but it is not disposable now. It is only disposable in a random number of cases if you look at the code base, e.g. https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Provider/FileReferenceCacheProvider.php#LL355C1-L357C88

Everybody has understood that cache should be disposable at this point. It only needs somebody to ACTUALLY make it disposable. It's really not that hard to remove all the "throw" where cache data is used, perhaps a good first issue for you?

[ro0NL]

so it was intendted to be disposable, and this pr reverts that? why not focus on fixing the remaning cases here? instead of saying others should do it.

(Not sure if I do understand 'disposable' in this context correctly, but I assume it is smth along the lines of 'if not there / corrupted do not fail execution')

@ygottschalk yes, and it's a core semantic of cache IMHO

@kkmuffme sorry if i annoy you 🥺 im just invested in psalm/GHA and cache :)

[dkarlovi]

The intent of #9889 was to make the cache not absurdly large and, while doing so, a sizeable refactor was required and #8679 was fixed in the refactor. The intent of the latter was to uniformly make it forgiving / self healing and that might have been missed in some specific cases which you've noticed.

The direction here should be to fix those remaining inconsistencies, not to undo the #8679 fix. @orklah made a good call to separate these two discussions in #9914 because now the intent of this PR is more clearly visible and I can say 👎 on it without dismissing the compression configuration I had no issues with.

[kkmuffme]

The direction here should be to fix those remaining inconsistencies, not to undo the #8679 fix

Exactly.
I think we're on the same page about it anyway. Could you just fix those few places you missed and add a message to the output (like those we have for psalm 6) if cache encountered a self-healable error (= we ignored the cache) or if it's something that is unhealable (e.g. couldn't write some files to cache or the write wasn't complete)

[dkarlovi]

Good we're on the same page, it seems we needed to have a sizeable discussion involving 5 people to get here. 👍

Could you just fix those few places you missed and add a message to the output

Unfortunately I don't have the time to work on Psalm currently, sorry. Feel free to fix any issues you noticed or at least list them in an issue so I (or somebody) will work on them when we get the time. Thanks!

[ygottschalk]

This PR is not to discuss about ....

Sorry for making this a discussion....

I wouldn't necessarily report only when --debug is set, but generally report these (in stderr?), as otherwise you might not be aware that there is an issue with cache at all.

My thoughts were focused around a clean output but having the option to investigate when someone notices a problem. My vote would go to having the issues hidden unless --debug / --verbose is set, but I am fine with the other option, too. Was just worried that every time I change some files I get like 1,000 warnings about 'Cache miss file xyz.php. Generating cache.' or even worse in case of a first run getting warnings about every file in the project. That is something we should avoid...

But while we are at adding some warnings: I would like to have some warnings to show up (not errors failing execution and in this case show them always regardless of --debug / --verbose) for the cases where config says 'use compressor lz4 / gzip' but the required extensions / functions are not present in the environment. Same for 'serilizer = igbinary' and no ext-igbinary present. I would expect changes to be made here: https://github.com/vimeo/psalm/pull/9924/files#diff-8d07f6a86af4900069be3c80774a29e2e7e2ffaffe434c781ba95de859cf874dR1192-R1207

[dkarlovi]

I agree output shouldn't be swamped with cache-related output by default, they're transitive and should be there only if I requested them. Focus of Psalm is not to debug its cache, it's to warn the user about errors in their code.

Having Psalm spew out what's basically infrastructural notifications in the same stream actual error messages which the user is actually interested in (which is exactly the reason why they're using it to begin with) is like you're working as maintenance in a hotel and then run around yelling at guests a lightbulb in lobby #2 burnt out. They don't care, it's not why they're here, you should care because that's your job, fix it and move on.

[orklah]

My two cents on that:

• If I have to choose between two solutions, I prefer having a cache that doesn't crash Psalm when unavailable
• However, it should be consistent and be applied everywhere
• I can understand why @kkmuffme would not want to work on a feature he doesn't agree with
• I can perfectly understand @dkarlovi not having time to work on it right now

So, while it's not my first preference, I'd be inclined to merge this, unless someone tells me he wants to work on the opposite direction.

It's fine, we can always change it later if it still an itch we want to scratch, but at least in the meantime, it will be consistent

Any disagreement?

[dkarlovi]

@orklah the current solution as proposed in this PR will actually increase the possible number of crashes than the current master, as I'm reading it.

[orklah]

Yeah, more than current master but not more than the last release if I understood correctly. And cache corruption doesn't happen in the wild that often so it doesn't seem that big a deal for now

[dkarlovi]

@orklah that's incorrect, the changes done in the cache.gz PR reduced / disabled the errors thrown, this PR is trying to undo that. 😃

[orklah]

yeah, that's what I was saying, your PR was not yet released so it would make no difference with the current stable release.

As I said above, I'm okay with reducing errors, but only if we're consistent

[dkarlovi]

But this PR doesn't increase consistency. 🤔

It's up to you, if you merge here, make sure to reopen #8679 because this PR re-breaks that.

[orklah]

@kkmuffme just for clarity, could you put here examples of where a corrupted cache will make Psalm crash on current master?

[ro0NL]

Here, take this.

Uncaught TypeError: Psalm\Internal\Analyzer\StatementsAnalyzer::analyzeStatement(): Argument #2 ($stmt) must be of type _HumbugBox0d94293eab9c\PhpParser\Node\Stmt, __PHP_Incomplete_Class given, called in phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php on line 167 and defined in phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php:246
Stack trace:
#0 phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php(167): Psalm\Internal\Analyzer\StatementsAnalyzer::analyzeStatement(Object(Psalm\Internal\Analyzer\StatementsAnalyzer), Object(__PHP_Incomplete_Class), Object(Psalm\Context), NULL)
#1 phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Analyzer/FileAnalyzer.php(140): Psalm\Internal\Analyzer\StatementsAnalyzer->analyze(Array, Object(Psalm\Context), NULL, true)
#2 phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Codebase/Analyzer.php(1088): Psalm\Internal\Analyzer\FileAnalyzer->analyze()
#3 phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Codebase/Analyzer.php(379): Psalm\Internal\Codebase\Analyzer->analysisWorker(0, '/app/bin/consol...')
#4 phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Codebase/Analyzer.php(222): Psalm\Internal\Codebase\Analyzer->doAnalysis(Object(Psalm\Internal\Analyzer\ProjectAnalyzer), 1)
#5 phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php(375): Psalm\Internal\Codebase\Analyzer->analyzeFiles(Object(Psalm\Internal\Analyzer\ProjectAnalyzer), 1, false, true)
#6 phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/src/Psalm/Internal/Cli/Psalm.php(272): Psalm\Internal\Analyzer\ProjectAnalyzer->check('/app/', false)
#7 phar:///usr/local/etc/tools/vendor/psalm/phar/psalm.phar/psalm(7): Psalm\Internal\Cli\Psalm::run(Array)
#8 /usr/local/etc/tools/vendor/psalm/phar/psalm.phar(14): require('phar:///usr/loc...')
#9 /usr/local/etc/tools/vendor/bin/psalm(120): include('/usr/local/etc/...')
#10 {main}
(Psalm 5.12.0@f90118cdeacd0088e7215e64c0c99ceca819e176 crashed due to an uncaught Throwable)

It was caused by swapping src to phar, and solved by wiping previous cache 🙄 aka manual labor.

(edit: i assume this was fixed on current master)

[ygottschalk]

I do not have a lot of time but I put #9932 out there.

It is just a small starting point, but I think that is the direction to go.

Can fix (if wanted) the case given here #9925 (comment), just leave some feedback or a👍 on the relevant comment in PR.

Would be nice if someone can confirm I did not miss anything relevant....

[kkmuffme]

Thanks @ygottschalk, with the PR #9932 I think we can get the consistency needed.

I'll leave this PR open until the other one gets merged.