fix(editor): keep fetches a refresh overtook out of the refreshed column and field path caches - #3094
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
Two
SQLSchemaProvidercaches wrote stale data after a refresh:fieldPaths(for:)samples a MongoDB collection's field paths for MQL completion and the filter panel's column menu.resetForDatabaseclears that cache and the in-flight tasks, but a sample already in flight did not notice. When it finished it wrote the pre-refresh paths into the refreshed cache, and setfieldPathTasks[key] = nileven when that slot now held a newer sample started after the refresh. So a third caller could start a duplicate$sample. Shipped since v0.65.0 (feat(plugin-mongodb): nested field paths, write options, and MQL formatting #2105).getColumns(for:schema:)had the same stale write with no in-flight task at all. A column fetch that a refresh overtook wrote the pre-refresh columns into the refreshed column cache, so column completion showed dropped or renamed columns until the next refresh. If the eager preload filled the key during the await, the key was also appended tocolumnAccessOrdertwice, which skews LRU eviction.The fix
The task-identity check
onDemandTables(inSchema:)got in #3070, applied to both:fieldPaths: after the await, a sample only clears its slot and writes the cache whilefieldPathTasks[key]is still its own task. Sampling errors are now logged withpublicLogShapeinstead of being swallowed bytry?.getColumns: gets an in-flight task per cache key (columnTasks), cleared byresetForDatabaseandclearColumnCache, with the same check. Concurrent requests for one table now share a fetch, andupdateValuekeeps a key from being appended to the access order twice.An overtaken fetch still answers its own caller. Cancelling overtaken tasks and returning
[]to that caller are separate follow-ups.Tests
Two cases in
SQLSchemaProviderUnqualifiedScopeTests, one per cache. Each holds the first fetch, refreshes, starts a second fetch, releases the first, then checks that a third caller joins the second rather than starting another.overtakenFieldPathSampleLeavesTheRefreshedScopeAlone: with its guard line removed it fails and the other 10 cases pass.overtakenColumnFetchLeavesTheRefreshedScopeAlone: with its guard line removed it fails and the other 11 cases pass.The shared test helper (
ScriptedFetch) now holds any number of calls, and the two "database switch" test names say "refresh", which is the case that actually reaches one provider twice.With both fixes, the provider suites (
SQLSchemaProviderUnqualifiedScopeTests,SQLSchemaProviderTests,SQLSchemaProviderFallbackTests,CachedColumnOrderTests,MongoCompletionCaseTests,SQLCompletionProviderTests,SchemaProviderRegistryTests,CopilotPreambleBuilderTests,SchemaContextForAITests): 223 executed, 223 passed.swiftlint lint --stricton both Swift files: 0 violations.Review
Skill(code-review)read thefieldPathsdiff. Fixed from it:It also found the same stale write in
getColumns, now fixed here. ThegetColumnschange has not had a separate review.Found while building #3070.