fix(datastore): stop nullable binds from wrapping the shared pg.NULL

What broke

To set a column to SQL NULL, these builders wrote pg.StringExp(pg.NULL). Two facts about that collide:

1. pg.NULL is one package-level value shared by the whole process (jet.NULL = newExpression(Keyword("NULL"))).
2. StringExp and TimestampzExp call setRoot on the value they wrap, which mutates it.

So it behaves like a single rubber stamp shared by the whole building, where stamping means re-inking it first. One caller at a time is fine. Two at once write the same memory, and test:race has been failing on merge requests that change nothing related (job 15973223966, tag v1.321.1).

It is a production race, not a test one

Issue #683 (closed) records this as test-only, reading it as "a request path calls this function on one goroutine". The object is process-global, so the second writer is any other in-flight request, not a parallel subtest.

Two of the affected builders sit on wired paths today:

Builder	Reached from
repositoryPatchAssignments	repository PATCH clearing description
upsertNpmRemoteFileStmt, upsertNpmRemoteMetadataFileStmt	npm remote cache fill when the upstream sends no ETag

Two concurrent description-clearing PATCHes are enough. For accuracy the other way: UpdateNpmRemoteRepository and UpdateContainerRemoteRepository have no non-test callers on main yet, so those four sites are latent. They are fixed here because the shape is identical and the settings PATCH handler is coming.

Sites fixed

Nine, not the two #683 (closed) names. Six wrap pg.NULL directly; three reach it through nullableStringExpr:

File	Lines
npm_remote_repositories_update.go	310, 333
container_remote_repositories_update.go	290, 291, 302, 305
repositories.go, npm_remote_files.go, npm_remote_metadata_files.go	869, 716, 785, all via nullableStringExpr returning the shared pg.NULL into a pg.StringExp

versionIDExpr and mavenRemoteVersionIDExpr also return pg.NULL, but only into unwrapped VALUES lists, which never call setRoot. Left alone.

The fix

Two changes, and the second is the one that matters.

1. Build the NULL per call, using the typed-CAST form credentialColumnExpr and the maven checksum helpers already use. pg.CAST stores its argument and AS_*() returns a fresh expression, so nothing shared is written.
2. nullableStringExpr now returns a pg.StringExpression. Callers previously wrapped it in pg.StringExp only to satisfy the compiler, which is exactly why all three live sites existed. With the right type returned, no wrap is needed, and reverting the helper is a compile error at every SET site rather than a silent race. Verified by reverting the signature.

Fixing only the nine call sites would leave the trap in place for the next nullable bind.

The SQL changes, the plan does not

The affected binds gain a cast: NULL becomes NULL::text or NULL::timestamp with time zone. PostgreSQL already coerces a bare NULL to the target column's type during parse analysis, so both forms reduce to the same node before planning. Evidence in the Database Review Evidence section below. Most of the test diff is statement-shape expectations moving with it.

One place a reviewer may want to push back: making nullableStringExpr safe by construction changes the SQL of the helper's other call sites, all VALUES binds that were never racy, in exchange for the invariant holding for every future caller. The narrower alternative is a second helper used only at SET sites, which leaves those expectations untouched but restores the "use the right one here" trap that produced all three live sites.

Testing

TestNullableBindsDoNotShareJetNULL runs all five fixed builders concurrently, each on two goroutines. The pairing is load-bearing: the fix leaves no builder writing the shared value, so a single reverted site would be the lone writer, and a race needs two concurrent accesses. One goroutine per builder caught only a simultaneous revert of every site, which is not the regression that is likely.

Non-vacuousness, proved per site rather than by reverting everything at once:

Reverted alone	DATA RACE reports
npm_remote_repositories_update.go	3
container_remote_repositories_update.go	2

• go test -race ./internal/datastore/ ./internal/managementapi/...: green
• golangci-lint run ./internal/datastore/: 0 issues
• golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 ./internal/datastore/: no findings in any changed file
• go vet -tags=integration ./internal/datastore/: compiles, and no integration test asserts the affected SQL

Review round

Five review agents ran over this branch. They found the fix correct at every call site and the cast right for every column, and found the guard defeated four ways, three of them silent:

Defect	Fixed in
git grep -E uses the platform engine; on BSD \s matches a literal s, so pg.StringExp( pg.NULL ) passed locally while the pattern worked on glibc	[[:space:]]
|| true collapsed no-match (1) and error (>1), so a run that never searched reported clean	keep the status; only 1 means clean
git grep is cwd-relative, so a run from a subdirectory passed on a violation above it	search from the repository root
The check ran only in pre-commit, which the docs claimed was CI	lint:jet-null-wrap

They also falsified several claims in the prose, each now corrected rather than softened: a revert is a compile error at the SET sites only (3 of 11 call sites), a typed return removes the motive to wrap but not the ability, pg.CAST is not one of the mutating wrappers, and the statement-shape tests do catch a revert at the two update builders, so the race detector is not the only reporter.

Each guard property was re-proved after the fixes and again after /simplify, by reverting one site at a time: guard exits 1 from the root and from a subdirectory, exits 2 outside a repository, and the race test reports.

Scenario catalogs

No change. This is a concurrency fix behind existing endpoints with no user-visible behaviour change and no new route, so no e2e scenario is added or affected.

Database Review Evidence

Queries

No migrations. No plan is affected: PostgreSQL already coerces a bare NULL to the target column's type during parse analysis, so both forms reduce to the same node before planning.

Check	Method	Result
Each cast matches its column	affected columns read from information_schema.columns	11 text columns take NULL::text; the two last_health_checked_at columns are timestamptz and take NULL::timestamp with time zone. No implicit coercion.
Plans equivalent	diff of EXPLAIN (VERBOSE, COSTS OFF) for both forms of three UPDATEs and the INSERT / ON CONFLICT DO UPDATE pair	identical, empty diff
Stored value	both forms run inside BEGIN / ROLLBACK on repositories, then read back	both store SQL NULL, both satisfy IS NULL, same digest, count(DISTINCT description IS NULL) = 1

The IS NULL check earns its place: the partial indexes on these tables are defined WHERE soft_deleted_at IS NULL, so a value that stopped satisfying IS NULL would change index eligibility rather than just a stored byte.

Collected on PostgreSQL 17.11, the canonical GL_PG_CURR_VERSION, against internal/datastore/migrations/structure.sql.

Collected by hand, because /db-review-prep query mode did not fire: its filter matches .QueryContext( / .ExecContext( in the changed file, and these files dispatch through instrumentQuery and instrumentExec. Repo-wide it sees 8 of the 65 query-dispatching files in internal/datastore. Tracked in #691.

Closes #683 (closed), #696 (closed), #711 (closed)

#696 (closed) and #711 (closed) report the same setRoot mutation as #683 (closed), filed independently against the same six call sites. All three close here.