Skip to content

fix(actions): read effective_closed inside the write transaction - #72

Merged
lucca65 merged 2 commits into
masterfrom
feat/objective-closed-propagation
Sep 12, 2026
Merged

lucca65 merged 2 commits into
masterfrom
feat/objective-closed-propagation

Conversation

@lucca65

@lucca65 lucca65 commented Sep 12, 2026 •

Copy link
Copy Markdown
Member

Summary

Companion to cambiatus/backend#443 (objective-closed-column). That PR adds
actions.effective_closed, written by both the backend's
Objectives.complete_objective/2 and this repo's action mapper — see its
scripts/objective-closed-action-adr.md for the full design and why the write is
split across two repos.

This PR is the event-source half of that dual writer, plus a fix for a lost-update
bug that dual writer created:

  • upsertAction now reads the objective's is_completed inside the write
    transaction, on the transaction's own connection, immediately before the write
    — not from the objective fetched earlier in the function.
  • Why: complete_objective/2 bulk-sets 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 is_completed
    earlier in the function captured false from before completion; writing that
    stale value after the backend's update_all had already landed true
    clobbered it back to false — on the happy path of every multi-action
    objective completion, not a rare edge case.
  • This narrows the race window from the whole function's duration down to one
    SELECT-to-write gap inside the transaction. It does not eliminate it outright —
    full elimination would need a correlated-subquery UPDATE or explicit row
    locking, which didn't fit this write's existing shape. What's left is exactly
    the drift shape the backend PR's new monitoring query watches for.

Test plan

  • node -c src/updaters/community.js — syntax valid
  • npx standard src/updaters/community.js — no new lint findings (pre-existing
    warnings on untouched lines only)
  • No runnable test suite exists in this repo for this path; verified by
    reading the diff directly, no automated test coverage added
  • Must deploy after cambiatus/backend#443's migrations, and this
    process must be restarted after that deploy — the DB layer introspects
    the schema once at boot and won't see the new column otherwise. See that
    PR's runbook update for the full sequencing.

🤖 Generated with Claude Code

lucca65 and others added 2 commits September 12, 2026 21:18
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HoKQkEipzS7KSQgRcrBpha
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HoKQkEipzS7KSQgRcrBpha
@lucca65

lucca65 commented Sep 12, 2026

Copy link
Copy Markdown
Member Author

Independent staff review round on this PR (and companion cambiatus/backend#443) found one real must-fix, addressed in a follow-up commit:

freshObjective (the in-transaction re-read added to narrow the lost-update race) could come back null on the same chain↔DB id-drift class the project's audit already found elsewhere — an unguarded .is_completed there would throw inside db.withTransaction, whose .catch deliberately rethrows, rolling back the block and retrying it forever. Fixed by falling back to o (the objective already fetched and null-checked earlier in the function), matching this file's own established log-and-continue pattern for this exact risk class.

Also corrected the comment above that read, which overstated that moving the read inside the transaction fully closes the race against the backend's complete_objective/2 bulk update. It narrows the window, not closes it — what actually makes the residual race unreachable today is backend#443's completion guardrail (refuses completion until every action already reads is_completed: true on chain). Noted so nobody relaxes that guardrail thinking this file alone covers it.

@lucca65
lucca65 merged commit f94c33d into master Sep 12, 2026
2 checks passed
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