Skip to content

[api] Reparse extended configs cached under another resolution stack - #65303

Open
Christopher Plieger (cplieger) wants to merge 2 commits into
microsoft:mainfrom
cplieger:fix-api-extended-config-stack-key
Open

Christopher Plieger (cplieger) wants to merge 2 commits into
microsoft:mainfrom
cplieger:fix-api-extended-config-stack-key

Conversation

@cplieger

Copy link
Copy Markdown
Contributor

The extended config cache keys an entry by path and expires it when the hash of the config's content and of the configs it extends changes. When a config is parsed with a config it extends already on the resolution stack, getExtendedConfig parses that config without the cache and parseConfig stops there with TS18000, so the entry has none of the options or configs the cycle brings in. A later load of the same config under a stack without that config gets the same hash and is handed the incomplete entry.

Impact: in a repository with a circular extends chain, a project that extends into the cycle can get the wrong compiler options on the API, depending on which project was opened first, where tsc -p gives it the full set.

The hash now also takes the name of each extended config that is on the resolution stack when the entry is parsed or checked, so a load under a stack that differs in this respect parses the config again. A chain without a cycle never has an extended config on the stack, so its hash is unchanged. tsc -b has the same gap in its own extended config cache, which this change leaves alone because #64835 is rewriting that file.

This depends on #65302 and is based on its branch, so this diff also shows that commit until #65302 merges and I rebase onto it. On main, LoadAndAcquire parses an expired entry while holding the entry's lock, so the reparses this change adds let two concurrent snapshots that enter an already opened cycle at different configs each hold one entry and wait for the other's. A test that does this hangs in 6 of 10 runs with this change on main and passes in 10 of 10 on top of #65302.

TestOpenProjectsExtendingIntoCircularExtendsKeepExtendedOptions opens p1, which extends shared/base.json, and then p2, which extends shared/a.json. base.json extends a.json and strict.json, and a.json extends base.json. It expects strict in p2's compiler options, and on main strict is unset in 10 of 10 runs. With a tsc built from this branch, the reproduction in the issue prints strict=true in both sessions in 3 of 3 runs.

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.

  • There is an associated issue in the Backlog milestone (required)
  • Code is up-to-date with the main branch
  • You've successfully run npx hereby test
  • You've successfully run npx hereby lint
  • You've successfully run npx hereby check:format
  • There are new or updated tests validating the change

Fixes #65301

LoadAndAcquire still returns a live, unexpired entry under its lock. Otherwise it parses with no lock held, then stores the value and adds the owner under the entry's lock, so two loads that re-enter the cache for each other no longer wait on each other's lock. Concurrent loads of one key that find no live, unexpired entry may each parse it; the first value stored wins and a later parse is dropped.

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. TestOwnerCacheConcurrentlyLoadKeysThatLoadEachOther fails on main with its deadlock message and passes here. TestOwnerCacheConcurrentLoadsKeepFirstStoredValue fails on main after 10 seconds because the second load waits on the first parse's lock, fails on this change with every parse stored because the slower parse replaces the first value, and passes here.
The extended config cache hash now also takes the name of each extended config on the resolution stack, so an entry parsed with a cycle config on the stack is parsed again under a stack without it.

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. TestOpenProjectsExtendingIntoCircularExtendsKeepExtendedOptions loses strict on main and passes here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Reentrant cache locking and stack-dependent expiry interact across concurrent snapshots, warranting human review and concurrency/race validation because project tests cannot be run in this environment.

No open issues 🎉

What changed in this PR

Fixes #65301 by making the API’s extended-config cache account for circular extends resolution context, preventing inherited compiler options from depending on project-open order.

Changes:

  • Includes extended configs already on the resolution stack in cache hashes, triggering reparsing when that context changes.
  • Includes the prerequisite #65302 change to parse owner-cache entries outside locks while preserving a live, unexpired value stored concurrently.
  • Adds regression tests for concurrent cache loads and inherited options when projects enter a circular config chain.
File Description
tsc/​internal/​project/​ownercache.go Moves parsing outside entry locks to avoid recursive-load deadlocks.
tsc/​internal/​project/​ownercache_test.go Tests concurrent recursive loads and first-stored-value preservation.
tsc/​internal/​project/​extendedconfigcache.go Makes cache hashes sensitive to circular resolution context.
tsc/​internal/​api/​session_apistate_test.go Verifies inherited strict survives sequential project opens into a cycle.

Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Status: Not started

Development

Successfully merging this pull request may close these issues.

[api] createSnapshot drops options a project inherits through a circular extends chain

2 participants