fix(api): re-read notified rows until they carry the notified block - #195
Conversation
There was a problem hiding this comment.
Pull request overview
Adds bounded replica-aware retries for socket notification row reads.
Changes:
- Introduces block-based freshness checks with retry backoff and throttled warnings.
- Integrates retries into seven notification handlers.
- Adds unit tests and release notes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/api/notification-read.ts |
Implements notification-aware row retries. |
src/api/notification-read.test.ts |
Tests freshness, retry, and warning behavior. |
src/api/namespaces/atomicmarket/routes/template-buyoffers.ts |
Adds retries for template buyoffers. |
src/api/namespaces/atomicmarket/routes/sales.ts |
Adds retries for sales. |
src/api/namespaces/atomicmarket/routes/buyoffers.ts |
Adds retries for buyoffers. |
src/api/namespaces/atomicmarket/routes/auctions.ts |
Adds retries for auctions. |
src/api/namespaces/atomicassets/routes/transfers.ts |
Adds retries for transfers. |
src/api/namespaces/atomicassets/routes/offers.ts |
Adds retries for offers. |
src/api/namespaces/atomicassets/routes/assets.ts |
Adds retries for assets. |
CHANGELOG.md |
Documents the fix and operational impact. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| expectedBlockById: extractNotificationBlocks(notifications, notification => { | ||
| if (notification.type === 'trace' && notification.data.trace?.act.name === 'logbackasset') { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Fixed in 6d36243: the assets read now selects max(updated_at_block) of the asset's backed-token rows as backed_at_block (a correlated subselect on the table's primary-key prefix), blockOf takes the greater of the two columns, logbackasset stays in the expectations, and the extra column is stripped before the formatter so the payload shape is unchanged. Executed read-only against a production wax database: two ids, nested-loop index plans. One unit test pins the two-column freshness.
| // A row whose block column reads as no number counts as behind, | ||
| // which the negated comparison gives and `<` does not. | ||
| return !(Number(blockOf(row)) >= expected); |
There was a problem hiding this comment.
Agreed on the mechanism and documented in 6d36243 rather than changed: a height does not identify a branch, so a fork that rewrites a row at the same height reads as fresh, and the replica's replay of the rollback closes that window, the same window the unmodified read had. Rows carry no block id and a canonical marker would be a schema change out of proportion to the fork rate; the helper comment states the limit.
dbf3624 to
6d36243
Compare
| sql: 'SELECT * FROM ' + this.transferView + ' WHERE contract = $1 AND transfer_id = ANY($2)', | ||
| params: [this.core.args.atomicassets_account, transferIDs], | ||
| ids: transferIDs, | ||
| expectedBlockById: extractNotificationBlocks(notifications, notification => notification.data.trace?.global_sequence), |
There was a problem hiding this comment.
Fixed in f886697: a logtransfer whose asset_ids is empty raises no expectation, so it keeps the single read. This is the post-review amend and merges without a further round.
…o socket payloads are not the pre-action row A socket consumer received a purchased sale whose row still said listed, with the listing's updated_at and no buyer, and a new sale with no row at all. The filler publishes a notification right after it commits on the primary, and a server that reads through an async replica queries the master view before the replica has replayed that commit, so the row it embeds is the one from before the action. Each handler now reads through readNotifiedRows, which judges every notified identifier fresh only when its row exists and its block column has reached the notification's block number, and re-queries with a short bounded backoff until that holds, five reads at most about a hundred milliseconds apart. When the budget is spent the rows are emitted as read, as before, and one warn names the channel and the identifiers, at most once a minute per channel with the count of batches suppressed in between. An action that does not advance the row's block column, such as logbackasset on the assets channel, is excluded from the expectation, or the check could never pass for it.
6d36243 to
f886697
Compare
Cause
The API's seven socket notification handlers query the
*_masterviews the instant a notification arrives. The filler publishes after it commits on the primary, and a server that reads through an async replica reaches the view before the replica has replayed that commit, so the embedded row is the pre-action row: a purchased sale stillLISTEDwith the listing'supdated_at_timeand no buyer, or no row at all for a sale created in the notified block. Observed on a production server whosePGHOSTis the read-replica pooler: sale 173987680 was emitted with the listing's time (12:43:19 UTC) while the primary held the purchase at 15:00:57 UTC, and a capturednew_saleevent carried nosalekey.Fix
readNotifiedRowswraps each handler's query. It builds the expected block per notified identifier from the notification's ownblock.block_num, judges a row fresh when it exists and its block column (updated_at_block, orcreated_at_blockfor the insert-only transfers) has reached that block, and re-queries with a bounded backoff until every identifier is fresh: five reads at most, about 100 ms apart. Spent budget emits the rows as read, as before, and warns at most once per channel per minute with the count of suppressed batches.logbackassetis excluded from the assets expectation because it never advances the asset row's block column. Emit code is unchanged.Impact
Consumers keep receiving one socket event per notification with the same payload shape; the row inside is at least as fresh as the notified block whenever the replica replays within about 400 ms. A server reading its primary performs one read per action as before. A transfer notification published while
store_transfersis off costs the full read budget on any server and warns at the rate limit.Validation
pnpm test404 to 418 passing (14 new propositions onreadNotifiedRowsandextractNotificationBlockswith a scripted fake database, an injected sleep and clock, and the rate limit proven red without its guard),pnpm lint,pnpm check-types,pnpm buildandscripts/release-notes.test.sh12 of 12 green. Cold code review took one pass with no blocking finding. Not verified against a lagging replica; the freshness rule was checked against every filler writer's block column.