fix: normalize nil event metadata to an empty map#973
Merged
Conversation
`events.metadata` (and `attempts.event_metadata`) are `jsonb NOT NULL`, but an event published without metadata carries a nil `map[string]string`. Nothing exercised that path, so it regressed silently when a pgx bump changed nil-map encoding to SQL NULL. Add coverage at both layers: - apirouter: publishing without metadata must build an event with a non-nil (empty) metadata map. - logstore conformance (pg + ch): inserting an event with nil metadata must not error and must round-trip as a non-nil empty map. All three are red on current code: Postgres rejects the insert (NOT NULL); ClickHouse inserts but reads the metadata back as nil; the router passes the nil map straight through. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An event published without metadata carries a nil map. `events.metadata`
and `attempts.event_metadata` are `jsonb NOT NULL`, and under pgx v5.10 a
nil map encodes to SQL NULL, so the Postgres log store rejects the insert
and the event is never persisted. ClickHouse accepted it but read the
metadata back as nil, an inconsistency with Postgres.
Treat nil and empty metadata as equivalent ("no metadata") and enforce a
non-nil map at every boundary:
- apirouter: default missing metadata to {} when building the event.
- pglogstore: default nil to {} in the event and attempt insert arrays.
- chlogstore: default nil to {} before marshaling, so it round-trips as
an empty map like Postgres.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The shared logstore conformance suite also runs against memlogstore (in-memory, exercised in the -short CI unit job). cloneEvent already defaults a nil MatchedDestinationIDs to an empty slice but left Metadata nil, so the new "nil metadata round-trips as empty" contract failed there. Mirror the existing convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alexbouchardd
approved these changes
Jun 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #972.
Problem
An event published without
metadatacarries a nilmap[string]string.events.metadataandattempts.event_metadataarejsonb NOT NULL, and under pgx v5.10 (bumped in #940) a nil map encodes to SQLNULL— so the Postgres log store rejects the insert:The event is never persisted; the log worker retries forever. ClickHouse accepted the insert but read metadata back as
nil— a silent inconsistency with Postgres.Why it wasn't caught
EventFactory.Any(), which always sets a populated metadata map — the nil path was never exercised.testing.Short()), and CI's Go job runs-short, so they don't run on PRs. Only thespec-sdk-testse2e job hit it, surfacing late as a confusing timeout.Change
Treat nil and empty metadata as equivalent ("no metadata") and enforce a non-nil map at every boundary:
{}when building the event (the canonical, earliest boundary; runs in CI).{}in the event and attempt insert arrays (correctness;NOT NULL).{}before marshaling, so it round-trips as an empty map like Postgres (consistency).Commits
test:— failing coverage at both layers. Red on current code: Postgres rejects the insert, ClickHouse reads back nil, the router passes nil through.fix:— the normalization above. All green.Verification
Local, both backends (
make up/test,TESTINFRA=1):apirouterpublish tests — pass (runs in CI-short).pglogstore+chlogstoreconformance — pass.Note
The conformance tests guard the real backends but only run in the non-
-shortintegration runs, not on PRs. The new apirouter test does run in CI. Wiring the logstore integration tests into CI is a worthwhile follow-up but out of scope here.🤖 Generated with Claude Code