Skip to content

[api] Own the configs a cached extended config extends - #65304

Open
Christopher Plieger (cplieger) wants to merge 1 commit into
microsoft:mainfrom
cplieger:fix-api-extended-config-ownership
Open

Christopher Plieger (cplieger) wants to merge 1 commit into
microsoft:mainfrom
cplieger:fix-api-extended-config-ownership

Conversation

@cplieger

Copy link
Copy Markdown
Contributor

On a cache hit, GetExtendedConfig returns the cached extended config through OwnerCache.LoadAndAcquire, which makes the snapshot an owner of that entry only. The configs the entry extends still reach the snapshot's ExtendedSourceFiles, and Snapshot.Clone calls AddOwner for each of them, which panics with OwnerCache.AddOwner: entry not found when one has no entry. This happens in two ways: the config that closes a circular extends chain 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 extends chain and another config that extends into it makes createSnapshot panic depending on the order its projects are opened and disposed in, where tsc -p reports 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 GetExtendedConfig now adds the snapshot as an owner of every config the entry extends, through a new OwnerCache.TryAddOwner. It skips the configs on the current resolution stack, which the caller is still parsing. If one of them has no live entry, GetExtendedConfig parses the config again through a new OwnerCache.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.

TestOpenProjectsExtendingIntoCircularExtendsReportCircularity opens cycle/tsconfig.json, which sets strict and extends a.json, which sets noUnusedLocals and extends it back, and then opens outer/tsconfig.json, which extends a.json, twice. Both outer snapshots 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. TestReopenProjectAfterExtendsChangeUsesNewExtendedConfig needs no cycle: it opens p1, which extends shared/a.json, changes a.json to extend c.json instead of b.json, opens p2, which also extends a.json, releases that snapshot and opens p2 again, expecting strict: false from c.json. On main each test panics with OwnerCache.AddOwner: entry not found in 10 of 10 runs. With a tsc built from this branch, the reproduction in the issue prints TS18000 for both orders and opens outer again 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.

  • 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 #64824

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.

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

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 extends chains 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

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

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

Status: Not started

Development

Successfully merging this pull request may close these issues.

[api] createSnapshot panics opening a project that extends into a circular extends chain

3 participants