Repository navigation
[api] Own the configs a cached extended config extends - #65304
Open
Christopher Plieger (cplieger) wants to merge 1 commit into
Open
Christopher Plieger (cplieger) wants to merge 1 commit into
Christopher Plieger (cplieger) wants to merge 1 commit into
Conversation
A cache hit in GetExtendedConfig now adds the snapshot as an owner of every config the entry extends (TryAddOwner), skipping the configs on the resolution stack. When one has no live entry, it parses the config again through ParseAndAcquire, which stores the result in the cache entry with the snapshot as an owner unless another load stored a live, unexpired entry during the parse. Tested with go vet and go test -race on internal/project and internal/api, npx hereby lint, npx hereby check:format and npx hereby test, all passing. Both new tests panic on main with OwnerCache.AddOwner: entry not found and pass here; without the store, the second outer open loses strict.
6 tasks done
Open
2 of 7 tasks
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The unlocked reparse and repair/store paths have ownership and lifetime invariants that sequential regression tests do not validate, requiring targeted concurrency testing and human review.
No open issues 🎉
What changed in this PR
This PR addresses #64824 by ensuring snapshots own the dependencies of cached extended configs, preventing createSnapshot panics when those dependencies lack live cache entries.
Changes:
- Adds owner-cache methods to acquire existing entries safely and store reparsed configs.
- Acquires cached configs’ dependencies, reparsing when an entry is missing while skipping configs on the current resolution stack.
- Adds regression tests for circular
extendschains and reopening projects after an extended-config change.
| File | Description |
|---|---|
tsc/internal/project/ownercache.go |
Adds TryAddOwner and ParseAndAcquire for dependency ownership recovery. |
tsc/internal/project/configfileregistrybuilder.go |
Acquires extended-config dependencies and reparses when needed. |
tsc/internal/api/session_apistate_test.go |
Tests circular-config diagnostics, inherited options, and reopening after config changes. |
Review effort: Balanced
This branch has not been deployed
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.
On a cache hit,
GetExtendedConfigreturns the cached extended config throughOwnerCache.LoadAndAcquire, which makes the snapshot an owner of that entry only. The configs the entry extends still reach the snapshot'sExtendedSourceFiles, andSnapshot.ClonecallsAddOwnerfor each of them, which panics withOwnerCache.AddOwner: entry not foundwhen one has no entry. This happens in two ways: the config that closes a circularextendschain is parsed without the cache, so it never gets an entry, and an entry acquired only by a released snapshot is deleted while a cached config still lists it, which needs no cycle.Impact: a repository with a circular
extendschain and another config that extends into it makescreateSnapshotpanic depending on the order its projects are opened and disposed in, wheretsc -preports TS18000 for each project. A client that changes a shared extended config and releases snapshots can hit the same panic with no cycle.A cache hit in
GetExtendedConfignow adds the snapshot as an owner of every config the entry extends, through a newOwnerCache.TryAddOwner. It skips the configs on the current resolution stack, which the caller is still parsing. If one of them has no live entry,GetExtendedConfigparses the config again through a newOwnerCache.ParseAndAcquire, which stores the result in the cache entry with the snapshot as an owner unless another load stored a live, unexpired entry during the parse, and that parse acquires each config it extends through the cache. A cache miss is unchanged.TestOpenProjectsExtendingIntoCircularExtendsReportCircularityopenscycle/tsconfig.json, which setsstrictand extendsa.json, which setsnoUnusedLocalsand extends it back, and then opensouter/tsconfig.json, which extendsa.json, twice. Bothoutersnapshots must have both options, and the second one checks that the parse was stored. It also expects TS18000 as the only config file parsing diagnostic.TestReopenProjectAfterExtendsChangeUsesNewExtendedConfigneeds no cycle: it opensp1, which extendsshared/a.json, changesa.jsonto extendc.jsoninstead ofb.json, opensp2, which also extendsa.json, releases that snapshot and opensp2again, expectingstrict: falsefromc.json. Onmaineach test panics withOwnerCache.AddOwner: entry not foundin 10 of 10 runs. With atscbuilt from this branch, the reproduction in the issue prints TS18000 for both orders and opensouteragain after the dispose in 3 of 3 runs.Two other defects on the same kind of tree have separate causes: two concurrent snapshots can deadlock, fixed in #65302, and a config cached under another project's resolution stack can lose options, fixed in #65303.
This replaces part of #64825. I met it while building deadset-ts, the TypeScript analyzer of deadset, on the TypeScript 7 API.
An AI coding agent wrote this patch. I have read, built and tested it and will handle the review.
Backlogmilestone (required)mainbranchnpx hereby testnpx hereby lintnpx hereby check:formatFixes #64824