Hold detached entries weakly in the change tracker

[ajcvickers]

Fixes #33557: A long-lived DbContext leaked entities that were detached via ctx.Entry(x).State = Detached.

• Root cause: _detachedReferenceMap held entities with references for the context's lifetime.
• Fix: make _detachedReferenceMap a ConditionalWeakTable
• The originally-reported InvalidOperationException is already fixed on main; the remaining defect was purely the leak.
• No API change, ~12 lines
• No measurable perf impact--see below--but does add some some extra memory pressure.

Full details from Claude

Background

When ctx.Entry(entity) is called for an untracked entity, the StateManager creates an InternalEntityEntry in the Detached state and caches it in EntityReferenceMap._detachedReferenceMap so that subsequent calls for the same
instance return the same entry (and a later Add/Attach/Update acts on it).

That map was a Dictionary<object, InternalEntityEntry> keyed by the entity instance, so it held the entity with a strong reference until the entry transitioned to a tracked state or ChangeTracker.Clear()/dispose was called.
Detaching an entity (ctx.Entry(x).State = Detached) is a no-op state change that never transitions the entry out of the map, so the entity stayed rooted by the context. In the reported scenario the principal's detach cascade-detaches
the dependents, so the subsequent ctx.Entry(...) calls in the user's detach loop ran against already-detached entities, caching - and thus leaking - them.

Rejected approaches

1. Evict the cached entry on the no-op SetEntityState(Detached).
   Breaks the entry-stability contract pinned by 15 existing tests
   (DbSetTest / DbContextTest / SharedTypeDbSetTest.Can_use_*_to_change_entity_state):
   var entry = ctx.Entry(e); // caches entry Added inital solution structure, early metadata support, and utilities. #1
   entry.State = Detached; // no-op
   ctx.Add(e); // must transition entry Added inital solution structure, early metadata support, and utilities. #1 -> Added
   Evicting Added inital solution structure, early metadata support, and utilities. #1 makes Add create a new entry, so the caller's entry no longer reflects the Add.

2. Any other purely state/flag-based heuristic for "release on detach".
   Same problem. The ONLY thing that distinguishes the leak case (the user no longer references the entity) from the stability case (the user still holds and reuses the entity) is whether the entity is still reachable. EF cannot
   know that from change-tracking state - only the GC does. That is precisely what a weak reference expresses, so weak references are required, not just one option among several.

Fix

Change _detachedReferenceMap to ConditionalWeakTable<object, InternalEntityEntry>.
CWT holds the key (entity) weakly via a DependentHandle, so:

• While the entity is referenced elsewhere, the cached entry is found and reused (entry-stability contract preserved).
• Once nothing else references the entity, the entity and its cached entry become collectable even though the context is still alive (leak fixed).

The change is contained to the field and the add site (AddOrUpdate); TryGetValue,Remove and the Clear (= null) paths are syntactically identical. Detached entries are never enumerated - GetCountForState / GetEntriesForState / GetNonDeletedEntities all exclude Detached - so no enumeration-based code is affected. CWT uses reference
equality, matching the previous ReferenceEqualityComparer.

Perf

Measured on the InMemory provider (Release, min of repeated runs), comparing this
change against main:

• Attach a 400k-entity graph (exercises detached-map add + transition-remove per
  node): ~416 ms (main) vs ~403-415 ms (this change).
• ctx.Entry() on 500k untracked entities (pure detached-map add): ~335 ms vs
  ~324-335 ms.
  No measurable regression; the detached-map operations are a small fraction of the attach cost and CWT add/remove/lookup is comparable to Dictionary for this workload. Note: CWT allocates a GC-tracked DependentHandle per cached detached entry, so a workload creating millions of distinct untracked entities via ctx.Entry() without
  tracking them would create more handles than the old Dictionary.

Tests

Added to StateManagerTest:

• Entry_for_untracked_entity_is_cached_while_the_entity_is_referenced - the stability contract (same entry returned while the entity is referenced).
• Entry_for_untracked_entity_does_not_keep_the_entity_alive - GC/WeakReference test proving a cached detached entry no longer roots its entity.
• Detaching_a_tracked_graph_does_not_retain_references_to_detached_entities - GC/WeakReference test reproducing the issue's detach-loop pattern.
• Can_add_an_equivalent_graph_after_detaching_a_graph_with_the_same_keys - pins the already-fixed no-exception / no-save-detached behavior.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds coverage and fixes around caching InternalEntityEntry instances for detached/untracked entities without preventing GC, addressing weak-reference behavior regressions (issue #33557).

Changes:

• Switch detached-entry caching from a strong Dictionary to ConditionalWeakTable in EntityReferenceMap.
• Add multiple GC-focused tests verifying detached-entry caching semantics for normal and shared-type entities, plus graph detach/add scenarios.
• Extend the test model with shared-type entities and new test-only entity types for reproductions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
test/EFCore.Tests/ChangeTracking/Internal/StateManagerTest.cs	Adds regression tests for weak detached-entry cache behavior and graph detach/add scenarios.
src/EFCore/ChangeTracking/Internal/EntityReferenceMap.cs	Updates detached entity cache to use ConditionalWeakTable to avoid keeping entities alive.

Comments suppressed due to low confidence (1)

src/EFCore/ChangeTracking/Internal/EntityReferenceMap.cs:1

• ConditionalWeakTable<TKey, TValue> in the BCL exposes Add, Remove, TryGetValue, and GetValue APIs, but not AddOrUpdate. Unless this repo defines an AddOrUpdate extension for ConditionalWeakTable, this will fail to compile. Prefer using Remove+Add (or TryGetValue + Remove/Add) to update an existing key, or use GetValue(key, ...) where appropriate.

// Licensed to the .NET Foundation under one or more agreements.

[AndriySvyryd]

There's no need to create new test entity type classes for these tests, prefer to use existing ones when possible

[ajcvickers]

Fixed.