Skip to content

Commit 27cf340

Browse files
committed
Harden persistence boundaries and shard lifecycles
Preserve the base Redis connector alias for typed registrations. Prevent Redis sequence reset-range overflow at the Int64 boundary. Normalize EF save guards without redundant per-context state. Quote Npgsql notification channels with provider semantics. Avoid shard initialization/removal lock inversion and dispose async-only child services.
1 parent 74b16e2 commit 27cf340

11 files changed

Lines changed: 492 additions & 82 deletions

File tree

docs/plans/fusion-library-audit.md

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1050,8 +1050,8 @@ Status: **completed**. Confidence: **Confirmed by source and focused DI regressi
10501050
- Test: `PersistenceAuditRegressionTest.TypedRedisDatabasesShouldRetainTheirOwnConnectors` resolves two typed databases and finds the same connector instance.
10511051
- **Maintainer decision:** associate each connector with its context type and inject `RedisConnector<TContext>` or an equivalent typed holder.
10521052
- **Recommended:** key the connector by context type and inject `RedisConnector<TContext>` or an equivalent typed holder.
1053-
- **Resolution:** typed Redis registrations now create and resolve `RedisConnector<TContext>`, isolating each context's connector while preserving the untyped single-database API.
1054-
- **Validation:** the focused DI regression now resolves two typed databases with distinct connector instances.
1053+
- **Resolution:** typed Redis registrations now create and resolve `RedisConnector<TContext>`, isolating each context's connector. Each registration also exposes its typed connector through the legacy base `RedisConnector` service, preserving the former last-registration-wins compatibility without using that ambiguous base service to construct typed databases.
1054+
- **Validation:** focused DI regressions resolve two typed databases with distinct connector instances, resolve the typed connector through the base service for a single context, and retain the last typed registration as the base connector when multiple contexts are registered.
10551055

10561056
### PERS3. `RedisSequenceSet.Next` reset is not atomic
10571057

@@ -1061,8 +1061,8 @@ Status: **completed**. Confidence: **Confirmed by source and focused Redis concu
10611061
- Failure: concurrent callers can interleave two resets and return the same sequence number, violating the advertised atomic sequence contract.
10621062
- **Maintainer decision:** make the complete conditional reset and increment atomic with one Redis Lua script or an equivalent compare transaction.
10631063
- **Recommended:** perform the complete conditional reset/increment in one Redis Lua script or transaction with a compare condition.
1064-
- **Resolution:** `Next` now performs increment, range validation, conditional reset, and the post-reset increment in one Redis Lua script.
1065-
- **Validation:** the Redis concurrency regression reproduces duplicate values with the former multi-command reset and now returns a unique contiguous range for 100 simultaneous callers; the existing sequence test also passes.
1064+
- **Resolution:** `Next` now performs increment, range validation, conditional reset, and the post-reset increment in one Redis Lua script. Negative reset ranges are rejected, and the upper reset boundary saturates at `Int64.MaxValue` instead of wrapping.
1065+
- **Validation:** the Redis concurrency regression reproduces duplicate values with the former multi-command reset and now returns a unique contiguous range for 100 simultaneous callers. The sequence regression also preserves integer precision above `2^53`, advances through the final two valid `Int64` values without a duplicate, and rejects a negative reset range.
10661066

10671067
### PERS4. The `DbEvent` resolver is registered with the wrong key type
10681068

@@ -1084,8 +1084,8 @@ Status: **completed**. Confidence: **Confirmed by source and focused regression
10841084
- Failure: disable/disable/enable still throws on `SaveChanges`, which is especially hazardous for pooled contexts.
10851085
- Test: `PersistenceAuditRegressionTest.SaveChangesGuardShouldBeIdempotent` reproduces the remaining guard.
10861086
- **Maintainer decision:** track the disabled state explicitly so disabling and enabling are idempotent; irreversible disabling is acceptable only if a safe reversible implementation proves infeasible.
1087-
- **Resolution:** `EnableSaveChanges` now tracks each context's enabled state in a weak table and normalizes the private `SavingChanges` subscription on every call, keeping repeated disable and enable calls reversible and idempotent without retaining contexts or trusting event state cleared by pooling.
1088-
- **Validation:** the original focused regression failed 0/1 before the change and passes after exercising both disable and enable twice. A second regression reproduced EF's pooled-context event reset against the first state-only implementation and now passes; the EntityFramework/Npgsql graph builds for all nine supported target frameworks.
1087+
- **Resolution:** `EnableSaveChanges` normalizes the private `SavingChanges` subscription on every call, keeping repeated disable and enable calls reversible and idempotent. EF pooling clears event subscriptions independently, so the reviewed implementation deliberately avoids a redundant weak-table state lookup and lock that cannot reliably describe the pooled context's current event state.
1088+
- **Validation:** the original focused regression failed 0/1 before the change and passes after exercising both disable and enable twice. A second regression reproduced EF's pooled-context event reset against the first state-only implementation and now passes with direct subscription normalization; the EntityFramework/Npgsql graph builds for all nine supported target frameworks.
10891089
- **Recommended:** make state idempotent, using a tracked flag rather than event-subscription count.
10901090

10911091
### PERS6. Npgsql locking clauses are emitted in caller order
@@ -1108,8 +1108,8 @@ Status: **completed**. Confidence: **Confirmed by source and focused formatter r
11081108
- Failure: valid shard names such as `us-west` produce invalid SQL; quotes and overlength names also lack escaping/normalization.
11091109
- Test: `PersistenceProviderAuditRegressionTest.DefaultNpgsqlChannelNamesShouldBeValidUnquotedIdentifiers` produces `AuditDbContext_String_us-west`.
11101110
- **Maintainer decision:** quote identifiers through the provider helper or map arbitrary inputs to bounded safe identifiers.
1111-
- **Resolution:** the default formatter now reuses `FilePath.GetHashedName` to normalize the complete channel name to a distinct ASCII identifier bounded to PostgreSQL's 63-character limit, adding a stable hash when sanitization or truncation is required.
1112-
- **Validation:** the original formatter regression failed before the change and now passes for hyphenated, quote-containing, and overlength shard names while checking syntax, uniqueness, and length; the Npgsql project builds for all nine supported target frameworks.
1111+
- **Resolution:** the default formatter reuses `FilePath.GetHashedName` to normalize the complete channel name to a bounded ASCII identifier, adding a stable hash when sanitization or truncation is required. `LISTEN` and `NOTIFY` now quote every formatted channel through `NpgsqlCommandBuilder`, preserving case-sensitive shard distinctions and safely handling custom formatter output.
1112+
- **Validation:** formatter regressions pass for hyphenated, quote-containing, and overlength shard names while checking syntax, uniqueness, and length. A SQL-emission regression verifies provider quoting for case-distinct names and embedded quotes; the Npgsql project builds for all nine supported target frameworks.
11131113
- **Recommended:** quote through the provider's identifier helper or map inputs to a bounded safe identifier.
11141114

11151115
### PERS8. An empty custom Npgsql hint crashes SQL generation
@@ -1151,8 +1151,8 @@ Status: **completed**. Confidence: **Confirmed by source and focused regression
11511151
- Source: `src/ActualLab.Fusion.EntityFramework/Sharding/ShardDbContextBuilder.cs:115-124` builds a provider per shard and returns only its `IDbContextFactory`.
11521152
- Failure: provider-owned pooled factories and disposable dependencies live indefinitely, even when shard factories are no longer used.
11531153
- **Maintainer decision:** retain provider ownership in a disposable shard-factory entry and dispose it when the entry is evicted or the root factory stops.
1154-
- **Resolution:** action-configured shard factories now return an internal provider-owning factory entry and dispose the new provider if factory resolution fails. The root shard factory uses lifecycle-aware cache entries so initialization racing shard eviction or root shutdown cannot leak a newly created provider, and implements synchronous and asynchronous disposal; container-owned custom factories remain untouched.
1155-
- **Validation:** focused lifecycle tests failed with per-shard dependencies alive, including a deterministically gated creation-versus-eviction race and a provider whose factory resolution failed after creating a disposable singleton. They now observe cleanup on resolution failure, immediate disposal for removed shards, exact owned-provider cleanup across eviction and root-shutdown races, and no disposal of externally supplied factories.
1154+
- **Resolution:** action-configured shard factories return an internal provider-owning factory entry and use async-capable cleanup even from synchronous resolution-failure paths. Cache entries retain lock-free reads after initialization, but no longer hold their entry lock while building a factory or marking its shard used; eviction starts tracked asynchronous disposal without holding the shard-registry callback, and every later asynchronous disposal observes the shared completion. The initializer disposes any provider produced after eviction. The root factory supports synchronous and asynchronous disposal, while container-owned custom factories remain untouched.
1155+
- **Validation:** focused lifecycle tests observe cleanup on resolution failure, prompt tracked disposal for removed shards, exact owned-provider cleanup across eviction and root-shutdown races, and no disposal of externally supplied factories. Review regressions also force removal to finish while the shard builder is still active, invoke a shard-update callback captured before root shutdown only after shutdown completes, and verify async-only dependency cleanup on eviction, root disposal, and factory-resolution failure.
11561156

11571157
### PERS12. `DbWaitHint` singletons have the wrong runtime type
11581158

src/ActualLab.Fusion.EntityFramework.Npgsql/NpgsqlDbLogWatcher.cs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ IServiceProvider services
2525
protected override DbShardWatcher CreateShardWatcher(string shard)
2626
=> new ShardWatcher(this, shard);
2727

28+
// Private methods
29+
30+
private static string QuoteChannelName(string channelName)
31+
=> new NpgsqlCommandBuilder().QuoteIdentifier(channelName);
32+
2833
// Nested types
2934

3035
/// <summary>
@@ -47,14 +52,15 @@ public ShardWatcher(NpgsqlDbLogWatcher<TDbContext, TDbEntry> owner, string shard
4752
Owner = owner;
4853
var hostId = DbHub.HostId;
4954
var channelName = owner.Settings.ChannelNameFormatter.Invoke(shard, typeof(TDbEntry));
55+
var quotedChannelName = QuoteChannelName(channelName);
5056
QuotedNotifyPayload = hostId.Id
5157
#if NETSTANDARD2_0
5258
.Replace("'", "''");
5359
#else
5460
.Replace("'", "''", StringComparison.Ordinal);
5561
#endif
56-
ListenSql = $"LISTEN {channelName}";
57-
NotifySql = $"NOTIFY {channelName}, '{QuotedNotifyPayload}'";
62+
ListenSql = $"LISTEN {quotedChannelName}";
63+
NotifySql = $"NOTIFY {quotedChannelName}, '{QuotedNotifyPayload}'";
5864

5965
var watchChain = new AsyncChain($"Watch({shard})", async cancellationToken => {
6066
var dbContext = await DbHub.CreateDbContext(Shard, cancellationToken).ConfigureAwait(false);

src/ActualLab.Fusion.EntityFramework/DbContextExt.cs

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
using System.Runtime.CompilerServices;
21
using Microsoft.EntityFrameworkCore;
32
using Microsoft.EntityFrameworkCore.Internal;
43
using ActualLab.Fusion.EntityFramework.Internal;
@@ -14,7 +13,6 @@ namespace ActualLab.Fusion.EntityFramework;
1413
public static class DbContextExt
1514
{
1615
#if !NETSTANDARD2_0
17-
private static readonly ConditionalWeakTable<DbContext, SaveChangesGuardState> SaveChangesGuardStates = new();
1816
private static readonly EventHandler<SavingChangesEventArgs> FailOnSaveChanges =
1917
(sender, args) => throw Errors.DbContextIsReadOnly();
2018
#endif
@@ -67,13 +65,9 @@ public static TDbContext EnableSaveChanges<TDbContext>(this TDbContext dbContext
6765
where TDbContext : DbContext
6866
{
6967
#if !NETSTANDARD2_0
70-
var state = SaveChangesGuardStates.GetValue(dbContext, static _ => new SaveChangesGuardState());
71-
lock (state) {
72-
state.IsEnabled = mustEnable;
73-
dbContext.SavingChanges -= FailOnSaveChanges;
74-
if (!state.IsEnabled)
75-
dbContext.SavingChanges += FailOnSaveChanges;
76-
}
68+
dbContext.SavingChanges -= FailOnSaveChanges;
69+
if (!mustEnable)
70+
dbContext.SavingChanges += FailOnSaveChanges;
7771
#else
7872
// Do nothing. DbContext has no SavingChanges event in NETSTANDARD2_0
7973
#endif
@@ -93,11 +87,4 @@ public static TDbContext SuppressDispose<TDbContext>(this TDbContext dbContext)
9387
#endif
9488
return dbContext;
9589
}
96-
97-
#if !NETSTANDARD2_0
98-
private sealed class SaveChangesGuardState
99-
{
100-
public bool IsEnabled { get; set; } = true;
101-
}
102-
#endif
10390
}

src/ActualLab.Fusion.EntityFramework/Sharding/ShardDbContextBuilder.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -124,7 +124,7 @@ public ShardDbContextBuilder<TDbContext> AddShardDbContextFactory(
124124
return new ShardDbContextFactoryEntry<TDbContext>(dbContextFactory, serviceProvider);
125125
}
126126
catch {
127-
serviceProvider.Dispose();
127+
serviceProvider.DisposeAsync().AsTask().GetAwaiter().GetResult();
128128
throw;
129129
}
130130
});

0 commit comments

Comments
 (0)