Skip to content

fix(relay): describe an error as text a database can store - #22

Open
johnnyhuirilef wants to merge 1 commit into
nestjs:masterfrom
johnnyhuirilef:fix/describe-error-unstorable-text
Open

johnnyhuirilef wants to merge 1 commit into
nestjs:masterfrom
johnnyhuirilef:fix/describe-error-unstorable-text

Conversation

@johnnyhuirilef

Copy link
Copy Markdown

PR Checklist

PR Type

  • Bugfix

What is the current behavior?

Issue Number: #21

Closes #21

Hi! 馃憢 With the PostgreSQL store, a message never reaches the dead-letter table when its handler throws an error whose text holds U+0000 or a lone UTF-16 surrogate. The relay runs the handler again each time the lease expires, with no limit.

describeError() keeps the text as it is and cuts it at 2000 code units. The cut can split an emoji, and V8 puts a lone \ud83d in the message of JSON.parse('馃榾 not json'). PostgreSQL refuses both in jsonb (and U+0000 in text). The store call throws, fail() returns 'lost', and attempts stays at 0.

I ran the repro on 0.1.1. The handler ran 7 times with a limit of 3, and the message was never dead-lettered:

--- bug: handler throws JSON.parse("馃榾 not json") ---
handler calls:   7
events:          none
stats:           pending=1 deadLetters=0
row attempts:    0
store log:       "Outbox store fail failed" logged 7 times
first one:       Outbox store fail failed: Error: Failed query: UPDATE "nest_outbox".messages
other errors:    0

RESULT: BUG: the message with the unstorable error was never retried or dead-lettered
        (7 handler calls, limit is 3; attempts stays 0)

What is the new behavior?

  • describeError() replaces U+0000 with U+FFFD. A reader can still see that something was there.
  • It calls toWellFormed() after the cut, because the cut can create the lone surrogate. This turns a lone surrogate into U+FFFD.
  • Both steps run on every kind of input: an error, a string, the errors of an aggregate and any other value.
  • The lib option of both tsconfig files adds ES2024.String, so TypeScript knows toWellFormed(). Node.js 20.19 and later has it.

The same repro against a local build of this branch:

--- bug: handler throws JSON.parse("馃榾 not json") ---
handler calls:   3
events:          retry-scheduled=2 dead-lettered=1
stats:           pending=0 deadLetters=1
row attempts:    row is gone
store log:       "Outbox store fail failed" logged 0 times
other errors:    0

RESULT: OK: both messages were dead-lettered

Does this PR introduce a breaking change?

  • Yes
  • No

The text of an error that was storable before stays the same.

Other information

The new specs cover these cases:

  • tests/unit.spec.ts: U+0000 in an error, in a thrown string and in the errors of an aggregate, also more than one in a text. A cut that splits a pair at index 1999. The lone surrogate in the message of a real JSON.parse error. A text that needs no change keeps its value and its cut.
  • tests/postgres/store.spec.ts: reschedule() and deadLetter() accept the description of these three errors. This runs on PGlite. The PostgreSQL target of the same suite is skipped when no server is available, and it was skipped on my machine. In CI, it runs on a PostgreSQL server.

I wrote each spec first and saw it fail. I also changed each line of describeError() and saw a spec fail.

I did not change fail(). It still returns 'lost' when the store throws for any other reason. A fallback there, such as a dead letter with a fixed error text, would stop a message from looping on an error that I did not think of. I am happy to open that as a separate PR if you want it. 馃檪

The payload has the same problem on add(). A U+0000 or a lone surrogate in a payload string makes add() throw inside the caller's transaction on the PostgreSQL store. The in-memory store accepts both. I ran this on PGlite, and issue #21 names the U+0000 case. I can open a separate issue if that helps.

PostgreSQL refuses U+0000 in text and jsonb, and a lone UTF-16 surrogate
in jsonb. describeError() let both through. A lone surrogate comes from the
message of JSON.parse('馃榾 not json') in V8, or from the cut at 2000 units
when it splits an emoji.

The store then threw, fail() returned 'lost', attempts stayed at 0 and the
message ran again with no limit. It never reached the dead-letter table.

describeError() now replaces U+0000 with U+FFFD, so a reader sees that
something was there. It calls toWellFormed() after the cut, because the cut
can create the lone surrogate. Both steps apply to every kind of input.

The lib option of both tsconfig files adds ES2024.String for toWellFormed().
Node.js 20.19 and later has it.

Closes nestjs#21
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.

Postgres store: a message runs again and again when the error text cannot be stored

1 participant