Skip to content

Flaky: string parameters can resolve to NpgsqlDbType.Unknown (add_first_parameter failed 3/4 CI jobs, green on re-run) #398

Description

@jeremydmiller

CommandExtensionsTests.add_first_parameter asserts that a string parameter resolves to NpgsqlDbType.Text. On the CI run for #397 it failed in 3 of the 4 Postgres jobs with:

Shouldly.ShouldAssertException : param.NpgsqlDbType
    should be
NpgsqlDbType.Text
    but was
NpgsqlDbType.Unknown

(net9.0 case-sensitive false, net9.0 case-sensitive true, net10.0 case-sensitive true; net10.0 case-sensitive false passed.) Re-running the identical commit with no changes turned all four green, so it is timing-dependent rather than a real regression in that PR. Not reproducible locally: 9 full runs of Weasel.Postgresql.Tests across net9.0 and net10.0, 785 tests, zero failures. Filing so it is not lost, since a silently wrong parameter type is worse than a flaky test.

Why Unknown is reachable at all

PostgresqlProvider.storeMappings() seeds DatabaseTypeMemo for string ("varchar") but not ParameterTypeMemo, so the NpgsqlDbType for string comes from the lazy path:

private NpgsqlDbType? ResolveNpgsqlDbType(Type type)
{
    var cached = ResolveParameterTypeFromMemo(type);
    if (cached != null) return cached;

    var value = GetTypeMapping(type)?.NpgsqlDbType;      // scans NpgsqlTypeMapper.Mappings
    ParameterTypeMemo.Swap(d => d.AddOrUpdate(type, value));
    return value;
}

GetTypeMapping does NpgsqlTypeMapper.Mappings.LastOrDefault(m => m.ClrTypes.Contains(type)) — a scan over a global, mutable Npgsql collection. When that returns null for string, determineParameterType falls through every branch (IsNullable, IsEnum, IsArray, the IEnumerable<T> branch, NpgsqlRange<>, DBNull, constructed generic) and ends at dbType = Unknown; return false. TryGetDbType then returns null and the parameter keeps Npgsql's default of Unknown — exactly what the test saw. So the failure mode is "the global type-mapper scan missed", not a bad assertion.

Candidate mechanisms (unverified)

  1. Concurrent mutation of NpgsqlTypeMapper.Mappings. Other test classes build data sources and open connections on parallel threads; if the global mapping collection is being populated or replaced while this scan enumerates it, a transient miss is plausible. This one would also affect production code, not just tests — any first-time resolution racing Npgsql's global mapper.
  2. A lost ParameterTypeMemo update. The memo is a Ref<ImHashMap<...>> mutated with Swap from several test classes at once (PostgresqlProviderTests calls RegisterMapping ~15 times). The provider ctor resolves and memoizes string up front, so a miss later implies that entry was not visible on the reading thread.

Worth noting the ctor calls ToParameterType(typeof(string)), which throws when resolution fails, so whatever happened here left the ctor succeeding and a later lookup missing.

Suggested direction

Seed ParameterTypeMemo for the common CLR types in storeMappings() alongside DatabaseTypeMemo, so string/int/Guid/bool never depend on a runtime scan of a global Npgsql collection. That removes the race for the types that matter without changing behaviour for anything else. If the global scan is genuinely racy, it is also worth deciding whether a failed resolution should be cached at all.

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