Service: check project logical timestamp when getting cached results

[auduchinok]

Fixes remaining known repro of #11291.

We've debugged it with @TIHan and discovered that stale results could be cached in the service and returned without checking timestamps of prior files in the project. When some prior file is changed and other files in the project are checked afterwards, a file with previously cached results may return stale results. This PR forces checking prior timestamps up to the file that has cached results.

[dsyme]

@TIHan Would you have a chance to review this please

@auduchinok Is there an automated test we can be adding for this?

[auduchinok]

@auduchinok Is there an automated test we can be adding for this?

@dsyme Nope, unfortunately. We could only reliably reproduce it in a bigger project, using the file system shim and accessing results concurrently.

[TIHan]

Passing -1 is a little weird since we have operated on 0 and above for file slots. But I understand why, as -1 means look at all the stamps and anything greater than that you want to only look at that many file stamps. So maybe fileSlot could be renamed to fileCount and add a comment explaining what happens when -1 is passed.

[auduchinok]

Thanks, will do.

[TIHan]

This is more or less what @auduchinok and I talked about last time, except for the changes in MaxTimeStampInDependencies.

Creating a test for this is challenging as we cannot seem to create a simple repo, it only happens in more complex projects.

I'm confident this will fix the issue, though it would mean that we will be computing timestamps for files on every call since we must compute the logical timestamp every time. A call to computeStampedFileNames looks at all files for their last write time. This might affect performance, so I advise a little bit of caution regarding this change.

[vzarytovskii]

As @TIHan said, it may affect performance.

Will it be possible to measure latency - now vs with this change?

[dsyme]

This might affect performance, so I advise a little bit of caution regarding this change.

Given this comment, we need some kind of measure on scenarios that may be of concern

@TIHan @auduchinok Could you iterate on how we can resolve this? Either through an adhoc manual test, or measured manual test?

[TIHan]

@dsyme We could do a measured test in our benchmarkdotnet tests. It could look something like this (pseudo):

Setup() =
    options <- .. // FSharpProjectOptions could have as an example, 1000 files.
    checker.ParseAndCheckProject(options) // check entire project so it builds up a cache
    
Execute() =
    // Cause all files to have their timestamps re-evaluated
    let updatedFirstFileContents = .. // updated contents of the first source file, content should be different with each execution
    checker.CheckFileInProject("first_file.fs", updatedFirstFileContents, options) // check first file in the project with the new contents
    checker.CheckFileInProject("last_file.fs", sameLastFileContents, options) // check last file in the project

[KevinRansom]

@auduchinok can you offer some insight about how we can verify performance impact to this change.

[dsyme]

I talked with @vzarytovskii - we will take this change