improve(gasless): drop malformed messages, retry operational failures - #3745
improve(gasless): drop malformed messages, retry operational failures#3745droplet-rl wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d543e0196f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const messageFilter = (deposit: AnyGaslessDepositMessage): boolean => { | ||
| try { | ||
| return _messageFilter(deposit); | ||
| } catch (err) { | ||
| this._dropMessage(deposit, err); |
There was a problem hiding this comment.
When an unprocessable message is present in the first API response, this guard never runs: initialize() passes the same messages through updateObservedCctpDeposits() and _markFilledFromInitialObservation() (src/gasless/GaslessRelayer.ts:278-280), whose token parsing can throw before polling starts (:585-586). For example, the invalid inputToken covered by the new test causes every process restart to fail initialization, so healthy sibling deposits are never reached; apply equivalent per-message isolation to the initialization path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct — initialize() runs the same messages through updateObserved(), updateObservedCctpDeposits() and _markFilledFromInitialObservation(), and that last one calls extractGaslessDepositFields() unguarded, so a message with an unparseable inputToken fails every restart and the healthy siblings never get indexed. _filterDepositsByAddress() has the same exposure via getGaslessAuthorizerAddress().
Rather than duplicate a boundary in each of those, 6818b97 moves the check upstream of all of them: _queryGaslessApi() now filters through _isProcessable(), which probes getGaslessAuthorizerAddress(), getGaslessPermitNonce(), _getDepositKeyFromMessage() (i.e. extractGaslessDepositFields) and, for swaps, the signed swapToken that updateObservedCctpDeposits() keys on. Both callers of _queryGaslessApi() — initialize() and the poll — inherit it, so nothing downstream ever sees a message whose derivations throw.
New test asserts _isProcessable rejects the bad message and drops it, and pins the reason the guard has to live at ingestion: _markFilledFromInitialObservation([bad]) still throws when called directly, [good] does not.
| try { | ||
| await _processDepositMessage(depositMessage); | ||
| } catch (err) { | ||
| this._dropMessage(depositMessage, err); |
There was a problem hiding this comment.
_processDepositMessage() includes live RPC operations such as willSucceed(), _findDeposit(), and _getDestinationFillStatus(), so a temporary provider failure reaches this catch just like malformed input. Because the catch then allows forEachAsync to continue through the batch while _dropMessage() records each request ID and sets its deposit state to ERROR, a shared outage can permanently exclude otherwise valid sibling deposits from every later poll in this process; only deterministic parsing/validation failures should be dropped, while operational failures should abort or remain retryable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and this was the most important of the three. _processDepositMessage() spans willSucceed(), _findDeposit(), _getDestinationFillStatus() and the submission calls, so the catch treated a provider blip identically to malformed input — and since an outage hits every message in the batch at once, one bad minute could have retired a batch of perfectly valid deposits for the lifetime of the process.
Split by failure class in 6818b97:
- Malformed — every derivation from the message payload is now probed at ingestion (
_isProcessable, called from_queryGaslessApi). Deterministic by construction: no provider is touched, so a failure there means the message can never be processed. That is the only path that reaches_dropMessage(), and it runs before the message is claimed, so the drop no longer needs to release a lock or writeERROR. - Operational — anything escaping the state machine now goes to
_deferMessage(): hands back the fill lock, logs once per failure streak (cleared when the message next completes), and leaves the message retryable. It rewinds to unclaimed only while nothing can have been submitted on origin —INITIAL/DEPOSIT_SUBMIT, which precedeinitiateDeposit()with no await in between, so a throw cannot interleave after a broadcast. Past that the state stays where it stopped rather than replayingDEPOSIT_SUBMITand risking a double submission.
The mid-flight test now asserts the inverse of what it did before: req-bad is not in droppedMessages, no drop log is emitted, its state stays at DEPOSIT_CONFIRM, and the sibling sharing the depositor's fill lock still reaches FILLED. A new test covers the pre-submission case end-to-end: fails, rewinds to unclaimed, logs once across two failing polls, then completes on the third once the fault clears.
| // Filtering runs before any message is claimed, so a malformed message (e.g. an unparseable | ||
| // inputToken) would otherwise throw out of Array.prototype.filter() and abort the whole poll. |
There was a problem hiding this comment.
This filter only receives messages after _queryGaslessApi() has restructured the entire response. restructureGaslessDeposits() processes the raw array in one flatMap and performs throwable operations such as BigNumber.from(depositId) and nested witness destructuring (src/utils/GaslessUtils.ts:272-300), so one malformed raw record rejects the query and aborts every steady-state poll before this per-message guard can run, again starving healthy records; isolate and drop failures at raw-record ingestion as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right — the guard sits downstream of restructuring, and restructureGaslessDeposits() walks the raw array in one flatMap, so BigNumber.from(depositId) on a non-numeric id or a witness that is neither BridgeWitness nor BridgeAndSwapWitness rejects the whole query. Nothing per-message can help there, and _queryGaslessApi() is shared with initialize(), so it takes down restarts too.
Fixed at the raw-record level in 6818b97: the per-record body is extracted into restructureGaslessDeposit() and the flatMap isolates each call. A record that will not restructure is skipped with a warn carrying its requestId, which is the same shape as the existing unsupported-permit-type skip a few lines up. Kept in GaslessUtils rather than wrapped by the relayer so the function keeps its own invariant (one bad record never rejects the batch) for any future caller. Test in test/GaslessUtils.ts pairs a malformed record with a healthy one and asserts the healthy one still comes through.
droplet-rl
changed the title
|
Addressed all three Codex findings in 6818b97 (replies posted on each inline thread). All three were real; the middle one was a regression I introduced. Transient failures were being classified as unprocessable (P1). Initialization path was unguarded. Raw records could reject the whole query. Net effect: No further iteration needed from my side — this was the one automated round, so a human review is the right next gate. |
…sages Codex review on #3745 flagged three gaps in the per-message boundary. _processDepositMessage() spans live RPC work, so the catch around it saw provider failures the same way it saw malformed input, and dropped both. An outage hits every message at once, so that could permanently exclude a batch of valid deposits. Split the two: an operational failure now defers the message instead of dropping it -- the fill lock is handed back, and the message rewinds to unclaimed when nothing can have been submitted on origin (still at INITIAL/DEPOSIT_SUBMIT, which precedes initiateDeposit() with no await in between). Past that point the state stays where it stopped, since re-running from INITIAL could submit the deposit twice. That leaves the drop path for malformed messages only, so it has to run before the message reaches anything: _isProcessable() now probes every payload-derived value at ingestion in _queryGaslessApi(). initialize() shares that path, and its observation pass (updateObservedCctpDeposits, _markFilledFromInitialObservation) has no boundary of its own -- a message whose inputToken won't parse used to fail every restart, so healthy siblings were never reached. One level lower, restructureGaslessDeposits() processed the raw response in a single flatMap: a record that won't restructure (unparseable depositId, unexpected witness shape) rejected the whole query and starved every poll before any per-message guard could run. Isolate each record. Co-Authored-By: Claude <noreply@anthropic.com>
evaluateApiSignatures() has no per-message error boundary, so one bad message takes down the whole poll. The API re-supplies it next tick, so it does so once per tick indefinitely. Add the boundary, and split the two failure classes it catches, because they want opposite handling: Malformed -- a derivation from the message payload throws. Deterministic, so the message is dropped permanently and logged once per requestId. _isProcessable() checks these at ingestion in _queryGaslessApi(), which is also the only boundary initialize()'s observation pass gets: a message whose inputToken won't parse used to fail every restart before any healthy sibling was reached. Operational -- an RPC or submission failure inside the state machine. Never dropped; an outage hits every message at once, so treating one as unprocessable would exclude healthy deposits with it. The message is deferred: the fill lock is handed back, or later deposits from the same depositor strand on an owner that is gone, and the state rewinds to unclaimed only while nothing can be on chain yet. Past DEPOSIT_SUBMIT it stays put, since re-running from INITIAL could submit the deposit twice. restructureGaslessDeposits() needed the same isolation one level lower; that is #3747, submitted separately. Co-Authored-By: Claude <noreply@anthropic.com>
6818b97 to
9f67f7b
Compare
droplet-rl
changed the title
1 participant
Rewritten to cut the diff down: 372 insertions → 222, with
restructureGaslessDeposits()split out into #3747 and the redundantmessageFilterbackstop removed (_isProcessableat ingestion already guarantees those derivations parse).evaluateApiSignatures()has no per-message error boundary, so one bad message takes down the whole poll — and since the API re-supplies it next tick, it does so once per tick indefinitely.Adds the boundary, and splits the two failure classes it catches, because they want opposite handling:
Malformed — a derivation from the message payload throws. Deterministic, so the message is dropped permanently and logged once per
requestId._isProcessable()checks these at ingestion in_queryGaslessApi(), which is also the only boundaryinitialize()'s observation pass gets: a message whoseinputTokenwon't parse used to fail every restart before any healthy sibling was reached.Operational — an RPC or submission failure inside the state machine. Never dropped; an outage hits every message at once, so treating one as unprocessable would exclude healthy deposits with it. The message is deferred instead: the fill lock is handed back (otherwise later deposits from the same depositor strand on an owner that's gone), and the state rewinds to unclaimed only while nothing can be on chain yet. Past
DEPOSIT_SUBMITit stays put, since re-running fromINITIALcould submit the deposit twice.Three tests, one per behaviour: malformed dropped at ingestion, operational failure after submission (deferred, lock returned, state preserved), operational failure before submission (rewound and retried).
Prerequisite for #3721, which hoists per-message work out of the state machine and needs these primitives.
🤖 Generated with Claude Code