Skip to content

Route all ShardName / progression-row identity construction through ShardName.Compose (root cause of marten#4679) #419

Description

@jeremydmiller

Background

marten#4679 reports 23505 duplicate key value violates unique constraint pk_mt_event_progression from catchUpPerTenantAsync at JasperFxAsyncDaemon.cs:989 for store-global (:All) projection shards under UseTenantPartitionedEvents = true. The original #4665 fix to per-tenant catch-up regressed into this 23505 — ForceAllMartenDaemonActivityToCatchUpAsync() still cannot be used under per-tenant partitioning + multiple tenants.

An audit of every ShardName construction and every mt_event_progression identity producer turned the diagnosis from "patch the per-tenant loop" into "enforce a design principle."

Diagnosis

The per-tenant loop in catchUpPerTenantAsync (JasperFxAsyncDaemon.cs:983-988) is correct on its face:

var tenantShard = asyncShard with { Name = asyncShard.Name.ForTenant(tenantId) };
var state = progress.FirstOrDefault(x => x.ShardName == tenantShard.Name.Identity)
            ?? new ShardState(tenantShard.Name, 0);
var agent = buildAgentForShard(tenantShard);
await agent.CatchUpAsync(ceiling, state, cancellation).ConfigureAwait(false);

It calls asyncShard.Name.ForTenant(tenantId) and queries the progression table by .Identity. That's the canonical path.

The bug is downstream of buildAgentForShard(tenantShard). Somewhere in the agent build / catch-up path, a wrapper reconstructs a ShardName from the projection's bare Name + Versiondiscarding the tenant binding the loop carefully established. The proximate INSERT then hits 23505 because every per-tenant catch-up writes the same {Projection}:V{N}:All row.

Design principle

ShardName is the canonical producer of shard and progression-row identities.
Every code path that needs a shard identity goes through ShardName.Compose or ForTenant.
No new ShardName(name, key, version) outside test code.
No string concatenation that mimics the grammar.

ShardName.Compose(name, shardKey, tenantId, version) forces every call site to confront the tenant slot — wrappers that have no tenant id to pass become visibly suspect, which is exactly the bug shape #4679 surfaces.

Refactor target list — hand-rolled ShardName constructors

All of these reconstruct a store-global ShardName from projection metadata. Each is a potential tenant-binding loss site:

File Lines Note
src/JasperFx.Events/Projections/Composite/CompositeProjection.cs 52, 83, 117 Composite projection identities — high suspicion for #4679
src/JasperFx.Events/Projections/Composite/ProjectionStage.cs 39, 51 Composite member-stage identities — high suspicion for #4679
src/JasperFx.Events/Aggregation/JasperFxAggregationProjectionBase.cs 77, 372 AsyncShard construction
src/JasperFx.Events/Projections/ProjectionWrapper.cs 79, 94 Line 94 uses literal "All" not the constant
src/JasperFx.Events/Projections/JasperFxEventProjectionBase.cs 45, 57 Event projection wrapper
src/JasperFx.Events/Projections/ContainerScoped/ProjectionSourceWrapperBase.cs 61, 70 Scoped projection wrapper base
src/JasperFx.Events/Projections/ContainerScoped/ScopedProjectionWrapper.cs 77, 102 Scoped projection wrapper
src/JasperFx.Events/Subscriptions/ScopedSubscriptionServiceWrapper.cs 96, 102 Line 102 uses literal "All" not the constant
src/JasperFx.Events/Subscriptions/JasperFxSubscriptionBase.cs 47, 65 Subscription wrapper

Replace every new ShardName(name, ShardName.All, version) with ShardName.Compose(name, version: version). Where a tenant id is available on the calling path, pass it through.

Composite projection scope (call-out)

The composite projection sites (CompositeProjection.cs:52,83,117 and ProjectionStage.cs:39,51) need particular attention. A composite projection's identity AND each member stage's identity must both go through ShardName.Compose. When the parent composite is bound to a tenant during a per-tenant catch-up, that binding must propagate to every member stage's ShardName — currently the bare-constructor calls drop it, which is the simplest explanation for the #4679 stack.

Catch-up loop semantics fix

In catchUpPerTenantAsync (JasperFxAsyncDaemon.cs:945-992), gate the per-tenant iteration on ShardName.TenantId != null. Store-global shards (TenantId == null) have a single progression row by design and are caught up by the regular continuous catch-up path — they have no business being iterated per tenant.

// Skip store-global shards: they have a single progression row by design.
// The regular continuous catch-up handles them.
foreach (var asyncShard in shards.Where(s => s.Name.TenantId == null
    && /* ... existing filter ... */)) continue;

(Exact insertion point + filter shape TBD by implementer; the spec is "store-global shards bypass the per-tenant loop.")

Defense-in-depth

InsertProjectionProgress (marten/src/Marten/Events/Daemon/Progress/InsertProjectionProgress.cs:38-40) should use ON CONFLICT (name) DO NOTHING. Cheap insurance against future drift; protects against any path that might still try to double-insert a progression row.

Regression test

In /Users/jeremymiller/code/jasperfx/src/EventTests/ or the appropriate Marten test project, add a test matching the exact #4679 repro shape:

  • UseTenantPartitionedEvents = true
  • TenancyStyle.Conjoined
  • Multiple async SingleStream / MultiStream projections registered as store-global (:All)
  • Seed events for 3+ distinct tenants
  • Call ForceAllMartenDaemonActivityToCatchUpAsync()
  • Assert: no 23505. All projections reach the latest sequence.

A second test for the composite case:

  • Same configuration as above, but the projections are wrapped in a CompositeProjection with 2+ stages
  • Same expectation

Non-goals

  • Not changing the ShardName grammar — the existing four forms (Name:Key, Name:V{n}:Key, Name:Key:Tenant, Name:V{n}:Key:Tenant) stay.
  • Not forcing every projection to be tenant-scoped — store-global is a legitimate, supported configuration (admin / reporting views across all tenants).
  • Not touching the ShardName.TryParse path — already handles all four forms correctly.

Already correct (don't refactor)

  • marten/src/Marten/Events/Daemon/Progress/InsertProjectionProgress.cs:40 — uses _progress.ShardName.Identity
  • marten/src/Marten/Events/Daemon/Progress/UpdateProjectionProgress.cs:43 — uses Range.ShardName.Identity
  • JasperFxAsyncDaemon.cs:983-988 per-tenant loop body — uses ForTenant(tenantId) + .Identity

Companion issue

A small Marten-side companion issue covers HighWaterDetector.cs:199 hand-rolling per-tenant high-water identity in SQL string concat — separate hygiene fix, not the root of #4679 but in the same design-principle scope.

Effort

Medium. Mostly mechanical constructor rewrites (~14 sites). Composite path needs careful tracing to confirm the bug is where it appears to be. Defensive INSERT + per-tenant loop semantics filter + regression tests round it out. Estimate 2-3 days.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions