Skip to content

5092a4dc - Fix double-encoded jsonb writes that trap the nostr publish loop - #103

Draft
davidleomay wants to merge 9 commits into
21gifts:developfrom
davidleomay:fix/jsonb-double-encode-publish-loop
Draft

5092a4dc - Fix double-encoded jsonb writes that trap the nostr publish loop#103
davidleomay wants to merge 9 commits into
21gifts:developfrom
davidleomay:fix/jsonb-double-encode-publish-loop

Conversation

@davidleomay

@davidleomay davidleomay commented Sep 9, 2026

Copy link
Copy Markdown

EN:
Values bound to a Postgres ::jsonb parameter were passed through JSON.stringify first, so the Bun SQL driver encoded them a second time and the column stored a jsonb string scalar instead of an object. Every SQL path expression on those columns therefore evaluated to NULL, which made all three listSignedMissing* repair scans match unconditionally and reset already-published messages forever, re-signing and re-publishing the same rows in a loop that nothing could break. This removes the manual serialization at all seven ::jsonb bind sites, backfills the two affected nostr_event columns in place, and makes the retry self-limiting using the nostr_attempts column, which was declared but never incremented anywhere. Two related follow-ups are deliberately out of scope and named below.

DE:
Werte, die an einen Postgres-::jsonb-Parameter gebunden werden, liefen zuerst durch JSON.stringify; der Bun-SQL-Treiber kodierte sie daraufhin ein zweites Mal, sodass in der Spalte ein jsonb-String-Skalar statt eines Objekts landete. Jeder SQL-Pfadausdruck auf diesen Spalten ergab damit NULL, wodurch alle drei listSignedMissing*-Reparaturlaeufe bedingungslos zutrafen und bereits veroeffentlichte Nachrichten endlos zuruecksetzten — dieselben Zeilen wurden immer wieder neu signiert und veroeffentlicht, ohne dass irgendetwas die Schleife brechen konnte. Dieser PR entfernt die manuelle Serialisierung an allen sieben ::jsonb-Bindestellen, repariert die beiden betroffenen nostr_event-Spalten in place und begrenzt die Wiederholung ueber die Spalte nostr_attempts, die zwar deklariert, aber nirgends hochgezaehlt wurde. Zwei zugehoerige Folgearbeiten bleiben bewusst aussen vor und sind unten benannt.

Details

Root cause

updateSignedEvent (and six sibling call sites) bound JSON.stringify(value) to a
$n::jsonb placeholder. The driver JSON-encodes a JS string again before sending, so
the stored value is a jsonb string scalar:

stringified -> jsonb_typeof = 'string', nostr_event->>'content' = NULL
object      -> jsonb_typeof = 'object', nostr_event->>'content' = '…'

Application code never noticed, because normalizeSignedEvent in
src/lib/nostr/publish.ts parses and then parses again when the result is still a
string. SQL has no such fallback, so only the database-side predicates broke.

Why it looped

With nostr_event->>'content' NULL:

  • listSignedMissingPhoto / listSignedMissingVideo —
    COALESCE(nostr_event->>'content','') NOT LIKE '%…%' is always true
  • listSignedMissingHashtags — jsonb_typeof(nostr_event->'content') IS DISTINCT FROM 'string' is always true, and this scan has no media precondition, so it selects every
    published top-level message

resetSignedEvent then cleared event_id / nostr_event and set the row back to
pending; the worker re-signed and re-published it, and the next scan pass selected it
again. Publishing itself always succeeded — the relay acknowledged every attempt — so the
only visible log line was a successful publish, repeating. resetSignedEvent logs
nothing and signing has no success log, which is why the cycle left no trace.

Changes

  1. Write path — the manual JSON.stringify is removed at all seven ::jsonb bind
    sites: message.nostr_event (insert and updateSignedEvent),
    conversation_message.nostr_event (insert and updateSignedEvent),
    message_invoice.zap_request, message_invoice.lnurl_response, and
    nostr_zap_ingest.receipt.
  2. Backfill — an idempotent UPDATE … WHERE jsonb_typeof(col) = 'string' is appended
    to MESSAGE_SCHEMA_SQL and CONVERSATION_SCHEMA_SQL, repairing existing rows on the
    next migration boot. After the first run the predicate matches zero rows.
  3. Loop guard — resetSignedEvent now increments nostr_attempts and stamps
    nostr_first_attempt_at, and the three repair scans skip rows at or above
    MAX_PUBLISH_ATTEMPTS. The same rule is mirrored in InMemoryMessageStore so the test
    double cannot show a terminating loop where the durable store would still spin.

push-store.ts is deliberately untouched: its delivered_endpoints column is text,
not jsonb, so JSON.stringify is correct there and removing it would corrupt the value.

The db_change audit trigger is unchanged, and the backfill UPDATEs are ordinary
durable writes logged by that trigger — no gap is introduced.

Deliberately out of scope

  • No backfill for message_invoice.zap_request, message_invoice.lnurl_response or
    nostr_zap_ingest.receipt.     Their write paths are fixed here, so no new corrupt rows
    are produced, and all three are read through parseJsonObject, which accepts a string
    or an object — so old and new rows coexist safely. nostr_zap_ingest is large, and
    rewriting it would emit one audit row per record; that is a separate, scheduled change.
  • The double-parse fallback in normalizeSignedEvent is kept. It must stay until the
    backfill has run everywhere; removing it earlier would turn stale rows into hard
    failures. Removing it afterwards is a follow-up, and worth doing so a regression cannot
    hide again.

Verification

bun run typecheck, bun run lint, bun run handbook:check, bun run e2e:check,
bun run test:coverage, bun run build and bun run e2e.

No exported symbol is added, so no handbook section or e2e entry is required by this
change.

@davidleomayAI
Fix double-encoded jsonb writes that trap the nostr publish loop
84b18e4 
Values bound to a Postgres ::jsonb parameter were passed through JSON.stringify
first, so the driver encoded them a second time and the column stored a jsonb
string scalar instead of an object. Every SQL path expression on those columns
then evaluated to NULL, which made all three listSignedMissing* repair scans
match unconditionally and reset published messages forever.

Remove the manual serialization at all seven ::jsonb bind sites, backfill the
two affected nostr_event columns idempotently on migration, and cap the retry
via nostr_attempts, which was declared but never incremented. The same cap is
mirrored in InMemoryMessageStore so the test double cannot disagree with the
durable store.
@davidleomayAI
Document the one-time jsonb unwrap and the publish-attempt cap
91f8fff 
The schema arrays were still described as idempotent DDL although each now
carries a one-time UPDATE, and the attempt cap added with the loop guard
appeared in no TSDoc and no handbook section. Follow the db-change.ts
precedent: name the DML in the array TSDoc, describe it in the schema mirror
comment, and extend both handbook sections.
@davidleomayAI
Make the jsonb backfill boot-safe, audited and self-clearing
875257e 
The backfill cast could abort the whole migration - and with it the process
boot - on a single unparseable row, and it ran before migrateDbChangeSchema
attaches trg_db_change, so its writes could land without an audit entry.

Replace both statements with a DO block that returns early until the audit
trigger is attached, repairs each row inside its own exception handler so one
bad value is skipped with a warning instead of failing the boot, and clears
nostr_attempts for the rows it repairs - the moment the root cause for that row
actually disappears. The counter is deliberately not cleared on a successful
publish: in this failure mode publishing always succeeds, so that would reset
the cap every cycle and restore the unbounded loop.
@davidleomayAI
Match the backfill UPDATE against the live column, not the cursor sna…
8a98925 
…pshot

PL/pgSQL fixes the cursor's MVCC snapshot when the loop opens, so the
per-row UPDATE was writing a stale unwrapped value keyed only on id. A
concurrent writer - an old replica still running resetSignedEvent, which nulls
nostr_event - could have its newer state clobbered, and on message the forced
nostr_attempts = 0 would re-arm the retry cap this PR installs.

Select only the id, recompute the unwrap from the live column, and repeat the
type predicate in the UPDATE's own WHERE so a since-changed row matches zero
rows instead of being overwritten.
@davidleomayAI
Narrow the repair exception, index its predicate, anchor its test
76ffd9c 
EXCEPTION WHEN others swallowed every error class, so a permission or
constraint failure during the repair would have been reduced to a warning and
the boot would have continued as if nothing happened. Only the cast failure the
handler was written for is tolerated now; anything else propagates.

The repair predicate had no index, so every boot paid a sequential scan of
message and conversation_message to establish that nothing was left to repair.
A partial index over the same predicate is empty once the repair converges.

The test counted occurrences of the type predicate, which stayed green even if
both sat in the cursor query rather than one in the UPDATE's WHERE. It now
anchors the predicate to the UPDATE statement.
@davidleomayAI
Mirror the repair-predicate indexes into the schema docs
3ec882d 
The two partial indexes added with the boot repair existed only in the schema
arrays, while the array TSDoc and the handbook both still claimed the mirrors
matched. The mirror files carry every other index, and their existing
disclaimer is scoped to the repair statement alone, so it does not cover DDL.

Add both indexes to docs/schema and name them in the two migrate sections.
@davidleomayAI
Catch the whole data-exception class in the repair loop
ca7d548 
Narrowing the handler to invalid_text_representation caught SQLSTATE 22P02
only. A nostr_event value carrying a NUL escape or a Unicode escape for a
character absent from the database encoding raises 22P05 or 22021 instead -
both in the same class, neither caught - so it would escape the block, abort
the migration and abort the boot. The values come from arbitrary third-party
clients, so those codes are reachable.

Catch data_exception, the class category, so any bad value is skipped while
permission errors, constraint violations and operational failures still
propagate. The test now also asserts the single-code form is absent, so a
future edit cannot silently narrow it again.
@davidleomayAI
Anchor the handler assertion on its trailing THEN
d987c67 
toContain is a substring match, so EXCEPTION WHEN data_exception OR
unique_violation THEN would have satisfied both the positive and the negative
assertion and widened the handler undetected. Anchoring on the trailing THEN
rejects any appended condition.
@davidleomayAI
Wrap only the cast in the repair handler, not the UPDATE
adeebbf 
The handler wrapped the whole UPDATE, and the UPDATE fires the db_change audit
trigger - so it unavoidably covered the trigger too. That is why every attempt
at a condition list came out either too broad (WHEN others, swallowing
permission and operational failures) or too narrow (a single SQLSTATE, missing
sibling codes and aborting the boot).

Wrap only the cast. The trigger is no longer inside the handler, so an audit
insert that fails on disk_full, insufficient_privilege or a sequence limit now
propagates and aborts the boot, which is correct. data_exception on a bare
text::jsonb cast is exactly the condition being handled, plus
statement_too_complex, which is class 54 and reachable because tags: string[][]
is a compile-time type only and inbound third-party events reach this column.

The cast now reads the cursor snapshot, so the UPDATE carries a compare-and-swap
on nostr_event to keep the guarantee an earlier round established. The warning
is also truthful again: previously a trigger failure was reported as a value
that could not be unwrapped.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@davidleomay @davidleomayAI