From 703acab78956f0d4908f76b4449853ce893bae92 Mon Sep 17 00:00:00 2001 From: Julien Lucca Date: Sat, 12 Sep 2026 21:18:19 +0200 Subject: [PATCH 1/2] fix(actions): read effective_closed inside the write transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Objectives.complete_objective/2 in the backend bulk-sets actions.effective_closed=true for every action under an objective in one update_all, triggered off the first of a multi-action completion's N upsertactions to confirm. This function is typically still processing actions 2..N at that point. Reading the objective's is_completed at the top of the function (before the create path's chain-id-resolution await, or before anything else that runs earlier in a busy indexer) captured is_completed=false from before completion, and writing that into the row after the backend's update_all had already landed true clobbered it back to false — a lost update on the happy path of every multi-action objective completion, not a rare race. Move the read inside db.withTransaction, on tx's own connection, right before the write. Narrows the window from the whole function's duration down to one SELECT-to-write gap; full elimination would need a correlated-subquery UPDATE or explicit row locking, which doesn't fit this write's shape. What's left is exactly the shape the backend's new monitoring query (objectives.is_completed=true AND actions.effective_closed=false) watches for. See scripts/objective-closed-action-adr.md in the backend repo for full background. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HoKQkEipzS7KSQgRcrBpha --- src/updaters/community.js | 84 ++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 33 deletions(-) diff --git a/src/updaters/community.js b/src/updaters/community.js index 31e5782..83f9542 100644 --- a/src/updaters/community.js +++ b/src/updaters/community.js @@ -436,6 +436,8 @@ function upsertAction (db, payload, blockInfo, _context) { reward: rewardAmount, verifier_reward: verifierAmount, is_completed: false, + // effective_closed is set further down, inside db.withTransaction, right + // before the write — see the comment there for why it can't be read here. usages: payload.data.usages, usages_left: payload.data.usages, verifications: payload.data.verifications, @@ -517,43 +519,59 @@ function upsertAction (db, payload, blockInfo, _context) { } } - return db.withTransaction(tx => { + return db.withTransaction(async tx => { + // effective_closed is read HERE — inside the transaction, on tx (not db), + // immediately before the write — rather than from the `o` fetched at the + // top of this function. Backend's Objectives.complete_objective/2 bulk-sets + // effective_closed=true for every action under an objective in one + // update_all when the FIRST of a multi-action completion's upsertactions + // confirms; this function is still processing actions 2..N at that point. + // Reading `o.is_completed` up there (fetched before the create-path's + // chain-id-resolution await, or before whatever else runs earlier in a + // busy indexer) would capture is_completed=false from before completion, + // and writing that stale value here — after backend's update_all already + // landed true — would silently clobber it back to false: a lost update, + // not a theoretical one (see scripts/objective-closed-action-adr.md in the + // backend repo). Re-reading right before the write on tx's own connection + // (not db's separate pooled one — see the validators comment below for why + // that distinction matters) shrinks that window to as small as this + // function can make it. + const freshObjective = await tx.objectives.findOne({ id: payload.data.objective_id }) + data.effective_closed = freshObjective.is_completed + // Create path uses insert(), not save(): with an explicit id present save() // emits an UPDATE (matching nothing for a new id); insert() honors the id, and // without one the serial assigns it. The update path keeps save()'s upsert-by-id. - const writeAction = - payload.data.action_id > 0 ? tx.actions.save(data) : tx.actions.insert(data) - - return writeAction.then(savedAction => { - // On update, replace the validator set: delete the old rows then - // re-insert from validators_str. Both the delete and the inserts run - // inside `tx` and are awaited, so the transaction commits only after - // they complete. Previously the delete ran on `db` (a separate - // connection) and neither it nor the inserts were awaited, so the tx - // could commit before the inserts landed — or the out-of-tx delete - // could race and wipe them — leaving an action with zero validators - // and its claims permanently invisible to validators. Any failure now - // rolls back the whole action instead of being silently swallowed. - const replaceValidators = - payload.data.action_id > 0 - ? tx.validators.destroy({ action_id: payload.data.action_id }) - : Promise.resolve() - - return replaceValidators.then(() => - Promise.all( - validators.map(validator => - tx.validators.insert({ - action_id: savedAction.id, - validator_id: validator, - created_block: blockInfo.blockNumber, - created_tx: payload.transactionId, - created_eos_account: payload.authorization[0].actor, - created_at: toUTC(blockInfo.timestamp) - }) - ) - ) + const savedAction = + payload.data.action_id > 0 + ? await tx.actions.save(data) + : await tx.actions.insert(data) + + // On update, replace the validator set: delete the old rows then re-insert + // from validators_str. Both the delete and the inserts run inside `tx` and + // are awaited, so the transaction commits only after they complete. + // Previously the delete ran on `db` (a separate connection) and neither it + // nor the inserts were awaited, so the tx could commit before the inserts + // landed — or the out-of-tx delete could race and wipe them — leaving an + // action with zero validators and its claims permanently invisible to + // validators. Any failure now rolls back the whole action instead of being + // silently swallowed. + if (payload.data.action_id > 0) { + await tx.validators.destroy({ action_id: payload.data.action_id }) + } + + return Promise.all( + validators.map(validator => + tx.validators.insert({ + action_id: savedAction.id, + validator_id: validator, + created_block: blockInfo.blockNumber, + created_tx: payload.transactionId, + created_eos_account: payload.authorization[0].actor, + created_at: toUTC(blockInfo.timestamp) + }) ) - }) + ) }).catch(e => { // Log AND rethrow. Swallowing left the action (and its validators) unwritten // while `ledgered` kept the _processed_actions row, so the create was recorded From e849c9902047843b24d7a64520905bf4284078fb Mon Sep 17 00:00:00 2001 From: Julien Lucca Date: Sun, 13 Sep 2026 01:18:01 +0200 Subject: [PATCH 2/2] fix: guard against null objective in upsertAction transaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review response on PR #72 (feat/objective-closed-propagation): the in-transaction re-read of the objective (`freshObjective`) could come back null on the same chain<->DB id-drift class the audit found for actions 399-406 (objective row deleted, or never indexed). Reading `freshObjective.is_completed` unguarded would throw inside db.withTransaction, whose .catch logs AND rethrows by design — so a single drifted objective row would roll back the transaction and the indexer would retry the same block forever. Fall back to `o`, the objective already fetched and null-checked at the top of upsertAction, matching this file's established log-and-continue pattern for this exact class of missing-row risk (the objective-existence check at the top of upsertAction, and verifyClaim's three skip-and-log guards). Also corrects the comment above this read, which overstated that moving the read inside the transaction fully closes the lost-update race against the backend's Objectives.complete_objective/2 update_all. Under Postgres's default READ COMMITTED isolation this is a plain SELECT with no row lock, so the race is narrowed, not closed; what actually makes the residual race unreachable today is the backend's completion guardrail (refuses to mark an objective complete until every action already reads is_completed: true on chain), not this transaction scoping alone. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HoKQkEipzS7KSQgRcrBpha --- src/updaters/community.js | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/updaters/community.js b/src/updaters/community.js index 83f9542..ac2b515 100644 --- a/src/updaters/community.js +++ b/src/updaters/community.js @@ -532,12 +532,33 @@ function upsertAction (db, payload, blockInfo, _context) { // and writing that stale value here — after backend's update_all already // landed true — would silently clobber it back to false: a lost update, // not a theoretical one (see scripts/objective-closed-action-adr.md in the - // backend repo). Re-reading right before the write on tx's own connection - // (not db's separate pooled one — see the validators comment below for why - // that distinction matters) shrinks that window to as small as this - // function can make it. + // backend repo). + // + // Re-reading on tx's own connection (not db's separate pooled one — see + // the validators comment below for why that distinction matters) narrows + // that window as far as this function can, but does not close it: under + // Postgres's default READ COMMITTED isolation this is a plain SELECT + // taking no row lock, so backend's update_all can still commit between + // this read and this transaction's write. What makes that residual race + // unreachable today is a separate mechanism: complete_objective/2 refuses + // to mark an objective complete until every one of its actions already + // reads is_completed: true on chain, so by the time completion can + // succeed at all, event-source has already durably written every action's + // row once. Keep the re-read — it shrinks the window on its own merits — + // but relaxing that backend guardrail would silently reopen the race. const freshObjective = await tx.objectives.findOne({ id: payload.data.objective_id }) - data.effective_closed = freshObjective.is_completed + // freshObjective can legitimately come back null — the same chain<->DB + // id-drift class the audit found for actions 399-406 (objective row + // deleted, or never indexed). The backend guardrail above + // (complete_objective/2 refuses completion while any action is still + // chain-open) makes that very unlikely mid-transaction, but falling back + // to `o` — fetched and null-checked at the top of this function — keeps + // the failure mode consistent with the rest of the file: log-and-continue, + // never crash-loop the whole indexer over one drifted row. Without the + // fallback an uncaught TypeError would roll back this transaction inside + // a .catch that logs AND rethrows, and the indexer would retry the same + // block forever — turning a rare drift into a total outage. + data.effective_closed = freshObjective ? freshObjective.is_completed : o.is_completed // Create path uses insert(), not save(): with an explicit id present save() // emits an UPDATE (matching nothing for a new id); insert() honors the id, and