Skip to content

ShardName.TryParse silently mis-parses per-tenant HighWaterMark:{tenant} rows #618

Description

@jeremydmiller

ShardName.TryParse silently mis-parses the per-tenant high-water progression rows instead of rejecting them, returning a ShardName whose Identity doesn't match the input and whose tenant has been discarded.

Repro

ShardName.TryParse("HighWaterMark:acme", out var parsed);
// returns true
parsed.Name;       // "HighWaterMark"
parsed.ShardKey;   // "acme"   <- the tenant id, in the shard-key slot
parsed.TenantId;   // null     <- tenant lost
parsed.Identity;   // "HighWaterMark"  <- does NOT round-trip the input

Why

TryParse (src/JasperFx.Events/Projections/ShardName.cs:101) special-cases only the exact string ShardState.HighWaterMark. "HighWaterMark:acme" falls through to the generic two-segment branch and becomes new ShardName("HighWaterMark", "acme", 1, null). The constructor's own special case then fires (ShardName.cs:64-68):

if (name == ShardState.HighWaterMark) { Identity = ShardState.HighWaterMark; }

collapsing the identity to the bare store-global constant and dropping the shard key and tenant from it entirely.

Why it matters

HighWaterMark:{tenant} is a real, persisted row shape, not a hypothetical. Marten writes one per tenant on every vectorized high-water poll (HighWaterDetector.MarkHighWaterForTenantAsync, marten#4717) and on the bulk-append path, and those rows come back from AllProjectionProgress interleaved with the projection shard rows.

So a caller doing the obvious thing — enumerate AllProjectionProgress, TryParse each ShardState.ShardName, group by tenant — gets a ShardName that claims to be the store-global high-water mark for every tenant, with no error and no false return to signal it. Silent wrong answer, not a crash.

Marten already works around this by keeping a separate, non-ShardName helper for exactly these rows (src/Marten/Events/Daemon/HighWater/HighWaterShardIdentity.cs:31-44) and Marten's own consumers do the string surgery by hand rather than going through TryParse. That workaround shouldn't be Marten-private knowledge — marten#5170 is a user who reached for TryParse, and every future lag/status/readiness consumer will too.

Suggested fix

Either is defensible; the first is probably better:

  1. Round-trip it. Recognize the HighWaterMark:{tenant} shape in TryParse and preserve the tenant, and stop the constructor from flattening the identity when a tenant is present. Identity then round-trips, and TenantId is populated. Needs care: ShardName's equality is identity-only, so per-tenant high-water names becoming distinct values is a behavior change for anything keying on them.
  2. Reject it. Have TryParse return false for any HighWaterMark-prefixed string other than the bare constant, and promote a HighWaterShardIdentity equivalent (compose + recognize + extract tenant) into JasperFx alongside it, so there's a supported way to handle those rows.

Whichever way, the current behavior — true plus a wrong answer — is the one option that shouldn't stay.

Blocks the projection-lag API filed separately; a lag implementation that routes high-water rows through ShardName today would attribute every tenant's mark to the store-global one.

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