Skip to content

fix(postgresql): seed Npgsql's global type mapper before the test suite runs (weasel#398) - #402

Merged
jeremydmiller merged 1 commit into
masterfrom
fix/398-npgsql-parameter-type-inference-flake
Jul 30, 2026
Merged

fix(postgresql): seed Npgsql's global type mapper before the test suite runs (weasel#398)#402
jeremydmiller merged 1 commit into
masterfrom
fix/398-npgsql-parameter-type-inference-flake

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Closes #398.

TL;DR

The parameter type is not silently wrong. add_first_parameter asserts on Npgsql's inference, and that inference depends on process-global state that another test happens to initialize first. In a cold process it fails 100% of the time.

What actually happens

command.AddParameter("a") passes dbType: null, so DatabaseProvider.AddParameter never calls SetParameterType:

parameter.Value = value ?? DBNull.Value;
if (dbType.HasValue)   // <-- false
{
    SetParameterType(parameter, dbType.Value);
}

PostgresqlProvider.determineParameterType, ResolveNpgsqlDbType and ParameterTypeMemo are not on this path at all. So the fix suggested in the issue — seeding ParameterTypeMemo in storeMappings() — would not have changed this test's outcome. Neither would anything about NpgsqlTypeMapper.Mappings; that static Cache is only read by Weasel's own resolution, which never runs here.

_npgsqlDbType is left null, so the param.NpgsqlDbType getter infers from Value via Npgsql's process-global type mapper. That mapper stays empty — reporting Unknown for every value — until the process constructs its first NpgsqlDataSource or NpgsqlConnection with a connection string. Measured, one fresh process per row:

what ran first in the process new NpgsqlCommand().AddParameter("a").NpgsqlDbType
nothing Unknown
new NpgsqlConnection() (parameterless) Unknown
new NpgsqlConnectionStringBuilder(cs) Unknown
new NpgsqlConnection(cs) Text
new NpgsqlDataSourceBuilder(cs) / .Build() Text
new NpgsqlDataSourceBuilder() Text

So the test passes only when some other test in the same process built a data source first. Under xUnit v3's parallel collections that is a race — hence 3 of 4 jobs failing and a clean re-run going green. It did not reproduce locally because a local full-suite run almost always wins that race.

This is a test-visibility artifact, not a wire-level defect: with _npgsqlDbType unset, Npgsql resolves the type at execution time against the real connection, and a command cannot execute without a connection existing.

The fix

Seed the mapper once from the existing module initializer, before any test runs. new NpgsqlDataSourceBuilder() is enough — no sockets, no connection string.

Why not make AddParameter set the type

That was the tempting fix (AddNamedParameter already does it) and it is a regression. Weasel's mapping is per-type; Npgsql's inference is per-value. Across the common CLR types DateTime is the only divergence, and it is a breaking one:

value Npgsql infers Weasel would set
DateTime.UtcNow TimestampTz Timestamp
DateTime.Now Timestamp Timestamp

Verified against a live Postgres:

untyped (today's AddParameter behavior)     -> OK
NpgsqlDbType=Timestamp (Weasel's mapping)   -> ArgumentException: Cannot write DateTime
    with Kind=UTC to PostgreSQL type 'timestamp without time zone' ...

All other checked types agree exactly: string, char, int, long, short, double, float, decimal, bool, Guid, DateTimeOffset, TimeSpan, DateOnly, TimeOnly, byte[], string[], int[], List<int>, IPAddress.

Two new tests pin this so the trap is not walked into later:

  • add_parameter_honors_an_explicit_type — the path Weasel controls, independent of global mapper state.
  • add_parameter_without_a_type_defers_to_npgsql_rather_than_weasels_mapping — fails if AddParameter ever starts stamping Weasel's mapping onto untyped parameters.

Verification

Counterfactual, with the warm-up commented out, cold process:

Failed  CommandExtensionsTests.add_first_parameter
Failed  CommandExtensionsTests.add_parameter_without_a_type_defers_to_npgsql_rather_than_weasels_mapping

— the exact CI failure. With the warm-out restored, green, including 5 consecutive cold-process runs of that class alone.

Full suite, all four CI matrix legs (net9.0/net10.0 x USE_CASE_SENSITIVE_QUALIFIED_NAMES true/false): 786 passed, 0 failed, 3 skipped each.

No product code changed.

Follow-ups, deliberately not in this PR

  1. AddParameter vs AddNamedParameter disagree on untyped values — the former defers to the driver, the latter calls ToParameterType. Worth reconciling, but it needs the DateTime-Kind question answered first.
  2. AddNamedParameter(cmd, name, DateTime.UtcNow) already sets Timestamp and so already throws on write. Pre-existing, separate from this flake.
  3. NpgsqlTypeMapper.Mappings really is a mutable static Cache enumerated without synchronization, and PostgresqlProviderTests writes to it while other collections read. Not the cause here, but still a genuine hazard for anything that does go through Weasel's resolution.

🤖 Generated with Claude Code

…uns (weasel#398)

`CommandExtensionsTests.add_first_parameter` failed in 3 of 4 Postgres CI
jobs and went green on an unchanged re-run.

The cause is not a bad parameter type. `command.AddParameter("a")` passes
`dbType: null`, so `DatabaseProvider.AddParameter` never calls
`SetParameterType` and Weasel's `determineParameterType` /
`ParameterTypeMemo` are not on this path at all. The assertion is reading
Npgsql's *inference* from the value, and that answers out of Npgsql's
process-global type mapper, which stays empty — reporting
`NpgsqlDbType.Unknown` for everything — until the process constructs its
first `NpgsqlDataSource` or `NpgsqlConnection` with a connection string.

So the test only passed when some other test in the same process happened
to build a data source first. Under xUnit v3's parallel collections that
is a race. In a cold process it fails 100% of the time, which is how it
reproduces locally.

Seed the mapper once from the existing module initializer, before any test
runs. `new NpgsqlDataSourceBuilder()` is enough; it opens no sockets and
needs no connection string.

Also pin the surrounding contract so this cannot regress:

- `add_parameter_honors_an_explicit_type` covers the path Weasel actually
  controls, and holds regardless of global mapper state.
- `add_parameter_without_a_type_defers_to_npgsql_rather_than_weasels_mapping`
  guards against "fixing" this by having `AddParameter` stamp Weasel's own
  mapping onto untyped parameters. That would be a regression: Weasel maps
  `DateTime` to `timestamp without time zone` for every value, while Npgsql
  resolves per value, and writing a `Kind=Utc` DateTime as
  `timestamp without time zone` throws at execution time. Verified against a
  live Postgres. `DateTime` is the only divergence across the common CLR
  types.

No product code changes. Full suite green on all four CI matrix legs
(net9.0/net10.0 x case-sensitive true/false), 786 passed each.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant