Skip to content

fix: C3 StatusUpdater batch upsert and per-block cap (COW-988)#88

Draft
lgahdl wants to merge 3 commits into
developfrom
luizhatem/cow-988-f1-c3-statusupdater-batch-upsert-and-per-block-cap
Draft

fix: C3 StatusUpdater batch upsert and per-block cap (COW-988)#88
lgahdl wants to merge 3 commits into
developfrom
luizhatem/cow-988-f1-c3-statusupdater-batch-upsert-and-per-block-cap

Conversation

@lgahdl
Copy link
Copy Markdown
Contributor

@lgahdl lgahdl commented Jun 1, 2026

Summary

  • Replace N sequential update() calls in C3 StatusUpdater with a single multi-row insert … onConflictDoUpdate, matching the pattern already used in C2 CandidateConfirmer
  • Add MAX_DISCRETE_ORDERS_PER_BLOCK constant (default 200) with per-chain env-var override (MAX_DISCRETE_ORDERS_PER_BLOCK_<chainId>) to cap the open-order SELECT and bound the /by_uids batch size
  • Extend the open-order SELECT to fetch all fields needed for the upsert (previously only orderUid was selected)

Test plan

  • pnpm typecheck passes (verified locally)
  • In live indexer: confirm C3 log line appears with updated=N after a settlement fills an open order
  • Confirm MAX_DISCRETE_ORDERS_PER_BLOCK_1=50 env var override limits the batch on mainnet

🤖 Generated with Claude Code

Replace N sequential per-order update() calls with a single multi-row
upsert, matching the C2 pattern. Add MAX_DISCRETE_ORDERS_PER_BLOCK cap
(default 200, env-var override per chainId) to bound /by_uids batch size
and keep block handler transactions short.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Jun 1, 2026

COW-988

Adds tests for DEFAULT_MAX_DISCRETE_ORDERS_PER_BLOCK (200) and all other
constants in src/constants.ts; also adds a pure-logic test for the
VALID_DISCRETE_STATUSES filter used by the C3 StatusUpdater batch upsert.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lgahdl lgahdl marked this pull request as draft June 2, 2026 13:50
@lgahdl lgahdl changed the base branch from main to develop June 2, 2026 13:52
@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

Indexer review — C3 StatusUpdater: batch upsert + per-block cap

The batch upsert is a meaningful improvement — N individual UPDATE round-trips → 1 insert. And the per-block cap + env-var override is the right pattern (mirrors what C1 does with MAX_GENERATORS_PER_BLOCK_<chainId>).

Missing ORDER BY on the SELECT (fairness / starvation risk):

.from(discreteOrder)
.where(and(eq(discreteOrder.chainId, chainId), eq(discreteOrder.status, "open")))
.limit(maxOrdersPerBlock)

Without an ORDER BY, the database is free to return any 200 rows, and in practice it tends to return the same physical pages each block. Orders inserted later (or living in different heap pages) may never surface within the cap window — they starve indefinitely while the same cohort of older rows gets re-queried each block without status change.

C1's equivalent select uses .orderBy(asc(conditionalOrderGenerator.lastCheckBlock)) to give priority to the stalest-checked row. A similar strategy here — e.g. .orderBy(asc(discreteOrder.creationDate)) or .orderBy(asc(discreteOrder.promotedAt)) — would ensure fairness. A composite index on (chainId, status, creationDate) would keep this fast.

@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

Missing docs: MAX_DISCRETE_ORDERS_PER_BLOCK_<chainId> is not in docs/deployment.md.

The Debug / Performance Flags table in docs/deployment.md already documents MAX_GENERATORS_PER_BLOCK_<chainId> (the analogous cap for OrderDiscoveryPoller/CancellationWatcher). The new MAX_DISCRETE_ORDERS_PER_BLOCK_<chainId> constant introduced in src/constants.ts belongs right next to it — an operator who hits a slow OrderStatusTracker block won't know this knob exists.

Suggested addition to the table (immediately after the MAX_GENERATORS_PER_BLOCK_<chainId> row):

| `MAX_DISCRETE_ORDERS_PER_BLOCK_<chainId>` | No | Per-block cap on how many open discrete orders `OrderStatusTracker` will check against the orderbook API on the given chain (e.g. `MAX_DISCRETE_ORDERS_PER_BLOCK_1=200`, `MAX_DISCRETE_ORDERS_PER_BLOCK_100=500`). Default is 200. Excess orders defer to the next block. |

@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

The batch upsert is a clear win — one round-trip instead of N sequential updates is the right shape for a block handler.

One thing that needs a comment in the code: the onConflictDoUpdate set clause deliberately omits promotedAt:

set: {
  status: sql`excluded.status`,
  executedSellAmount: sql`excluded.executed_sell_amount`,
  executedBuyAmount: sql`excluded.executed_buy_amount`,
  // promotedAt is intentionally absent
},

Without an explanation, this looks like an oversight — a reviewer or future maintainer will reasonably ask "why isn't promotedAt being updated?". Please add a brief inline comment, e.g.:

set: {
  status: sql`excluded.status`,
  executedSellAmount: sql`excluded.executed_sell_amount`,
  executedBuyAmount: sql`excluded.executed_buy_amount`,
  // promotedAt is intentionally NOT updated — we keep the original timestamp
  // from when the order was first promoted to discrete_order; overwriting it
  // here would make the field meaningless.
},

Also note that type DiscreteStatus is defined inline again at line ~583 (same as the issue called out in #86). If that type gets extracted to a shared location in parallel, make sure this PR imports from there instead of adding a fourth copy.

Everything else looks good — the per-chain env-var override, the .limit(maxOrdersPerBlock) cap, and selecting the full row upfront to avoid a second query are all sensible.

@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

Code review — C3 batch upsert + per-block cap (COW-988)

Missing ORDER BY on the capped SELECT — starvation risk

const openOrders = await context.db.sql
  .select({ orderUid: ..., ... })
  .from(discreteOrder)
  .where(and(eq(discreteOrder.chainId, chainId), eq(discreteOrder.status, "open")))
  .limit(maxOrdersPerBlock)  // ← no .orderBy(...)

Without an ORDER BY, PostgreSQL is free to return any 200 rows. In practice it will tend to return the same physical-order rows block after block (typically the most recently inserted rows, or whatever the heap scan returns first). Orders that happen to fall outside that 200-row heap window will never be checked as long as total open orders exceeds the cap — they will remain "open" indefinitely until either the generator is cancelled or validTo expires.

The fix is to add .orderBy(asc(discreteOrder.promotedAt)) (or creationDate). This ensures the oldest open orders are checked first, which is the same fairness policy already used in OrderDiscoveryPoller (orderBy(asc(lastCheckBlock))). Without this the cap creates a starvation bug rather than a scheduling policy.

onConflictDoUpdate does not guard against downgrading terminal rows

The upsert is:

.onConflictDoUpdate({
  target: [discreteOrder.chainId, discreteOrder.orderUid],
  set: {
    status: sql`excluded.status`,
    executedSellAmount: sql`excluded.executed_sell_amount`,
    executedBuyAmount: sql`excluded.executed_buy_amount`,
  },
})

Only rows with status = "open" are selected into rowsToUpdate (the SELECT filters eq(discreteOrder.status, "open")), so in the normal path the conflict target will always be an "open" row and overwriting it is correct.

The residual concern: a race where OrderStatusTracker and CandidateConfirmer both process the same UID in the same block (different Ponder handler invocations). If CandidateConfirmer promoted the row to "fulfilled" earlier in the same block, and the stale "open" select in OrderStatusTracker then fires an upsert with status = "fulfilled" from the API, the result is the same — no regression. If the API returns "open" for a row that was just promoted (cache miss window), the upsert would write status = "open" back onto a terminal row. This can't happen in the current code because VALID_DISCRETE_STATUSES excludes "open" — only terminal statuses pass the if (!info || !VALID_DISCRETE_STATUSES.has(info.status)) continue guard. So rowsToUpdate will only ever contain terminal statuses, and no downgrade of an already-terminal row is possible through this path.

However, a WHERE status = 'open' clause in the onConflictDoUpdate SET expression (e.g. set: { status: sql\CASE WHEN ${discreteOrder.status} = 'open' THEN excluded.status ELSE ${discreteOrder.status} END` }) would make the intent explicit and be a zero-cost safeguard. Optional but recommended.

Summary: The missing ORDER BY is a real starvation bug that must be fixed before this is deployed with a cap in place. The upsert correctness concern is a minor hardening recommendation, not a blocking bug.

@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

Documentation / CI review

MAX_DISCRETE_ORDERS_PER_BLOCK_<chainId> is not documented in docs/deployment.md or .env.example. Must fix before merge.

The analogous MAX_GENERATORS_PER_BLOCK_<chainId> is already in the Debug/Performance Flags table at line 35 of docs/deployment.md:

MAX_GENERATORS_PER_BLOCK_<chainId> — Per-block cap on how many generators C1 and C5 will touch…

The new MAX_DISCRETE_ORDERS_PER_BLOCK_<chainId> constant (default 200) should be added as the very next row in that table, directly below MAX_GENERATORS_PER_BLOCK_<chainId>. Suggested wording:

Variable Required Description
MAX_DISCRETE_ORDERS_PER_BLOCK_<chainId> No Per-block cap on how many open discrete orders OrderStatusTracker (C3) will check in a single block (e.g. MAX_DISCRETE_ORDERS_PER_BLOCK_1=200, MAX_DISCRETE_ORDERS_PER_BLOCK_100=500). Default is 200. Excess orders are picked up on the next block.

.env.example also missing the new var. The existing MAX_GENERATORS_PER_BLOCK_1 / MAX_GENERATORS_PER_BLOCK_100 example lines should be followed by equivalent commented-out MAX_DISCRETE_ORDERS_PER_BLOCK_1 / MAX_DISCRETE_ORDERS_PER_BLOCK_100 examples.

src/constants.ts comment — good. The JSDoc on DEFAULT_MAX_DISCRETE_ORDERS_PER_BLOCK clearly explains the override pattern. The docs/deployment.md entry will give ops teams the same visibility without reading source.

Batch upsert logic — no doc impact. The internal change from N sequential update() calls to a single insert … onConflictDoUpdate is an implementation detail; nothing in the docs describes that query pattern.

Overall: One actionable item before merge — add the new env var to docs/deployment.md (Debug/Performance Flags table) and .env.example.

@lgahdl
Copy link
Copy Markdown
Contributor Author

lgahdl commented Jun 2, 2026

Review: C3 batch upsert + per-block cap (COW-988)

promotedAt excluded from onConflictDoUpdate.set{} — intentional and correct, but needs a comment

promotedAt is included in rowsToUpdate values (carried from order.promotedAt) but is intentionally not in the set object. This is correct: promotedAt records when C2 originally promoted the row from candidateDiscreteOrder; C3 should never overwrite it. However, there is no comment explaining the intentional omission. A future reader will reasonably wonder if it was forgotten. Add a one-liner:

// promotedAt intentionally omitted — only set once by C2 at promotion time

type DiscreteStatus — third copy in the same file

This PR adds a third inline type DiscreteStatus = "open" | "fulfilled" | "unfilled" | "expired" | "cancelled" definition inside blockHandler.ts (PR #86 added one in the C2 orphan path, the existing code had one in the C2 stale path). All three copies are identical. This should be extracted to a shared module. Tracking alongside the same note left on PR #86.

sql import for onConflictDoUpdate.set{}

sql\excluded.status`etc. are used in thesetobject. Confirmsqlis already imported inblockHandler.tsfromponder` — it is (line ~31). No missing import.

Per-block cap ordering

The .limit(maxOrdersPerBlock) is applied to the SELECT of open orders but there is no ORDER BY. Without an ORDER BY, the DBMS (Postgres) may return a different subset each block, potentially starving some rows indefinitely. This is an existing limitation for any per-block cap pattern, but worth calling out — consider adding .orderBy(asc(discreteOrder.creationDate)) or similar to ensure older orders are prioritized over newer ones.

Number(process.env[...]) || DEFAULT

If the env var is set to "0" this evaluates to the default (falsy). Should be:

const envVal = Number(process.env[`MAX_DISCRETE_ORDERS_PER_BLOCK_${chainId}`]);
const maxOrdersPerBlock = Number.isFinite(envVal) && envVal > 0 ? envVal : DEFAULT_MAX_DISCRETE_ORDERS_PER_BLOCK;

Minor but avoids a surprising footgun for ops.

…cument per-block cap (COW-988)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.

1 participant