Never invent objective or action ids (finish what #61 started for claims) - #66
Merged
Merged
Conversation
#61 removed the serial fallback for claims. The same fallback was still in the objective and action create paths, and it is the same defect: when the chain read failed, the resolver substituted a DB serial that silently names a DIFFERENT row. For an action that is worse than for a claim — claimaction carries action_id, so later claims attach to the wrong action, and the action becomes un-editable. That is the shape of the phantom action ids 399-406 the audit found. Removing the fallback alone would not have been safe, because the reads underneath it were still truncatable: * actionsForObjective used the `byobjective` SECONDARY index. Secondary reads cannot be resumed — nodeos returns the secondary key as next_key — so a short read just looks like "this objective has fewer actions than it does", and the caller then hands the create an id that is already taken. It now pages the whole `action` table through the primary index and filters client-side: 400 rows / 5 calls on prod, on the create path only. * objectivesForCommunity took a single call at limit 2000 and trusted it. It now pages its scope until more=false. Both go through one pager, and assertComplete is gone with them: a lone call is never proof of a complete set at any table size, so there is nothing left for a throw-on-`more` guard to protect. Chain reads are now retried (3 attempts, backing off) before giving up. Without a fallback a failed read costs a skipped create until someone reindexes, and about 2 in 100 calls came back without a rows array while paging the action table on prod — the acceptance check below failed 2 of 105 calls before the retry and passes cleanly after. The node's own message is carried into the error, since Sentry is not running and the log line is the only diagnostic. The claim reader shares the retry for the same reason. Create-path inserts log AND rethrow instead of swallowing. A swallowed insert drops the row while ledgered still records the action as processed, so no reindex revisits it — the likeliest explanation for the two claims found missing on prod. Verified with scripts/verify-create-resolvers.js against the prod chain: replay each parent's creation history (first k ids known, ask for the next) and require the resolver to name id k+1 every time. 14 communities / 95 objective steps and 77 action steps across the busiest objectives all pass, and an exhausted parent throws instead of inventing an id. verify-claim-resolver.js still 636/636. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#61 removed the serial fallback for claims. The objective and action create paths still had it, and it is the same defect: when the chain read fails, the resolver substitutes a DB serial that silently names a different row.
For actions it is worse than for claims —
claimactioncarriesaction_id, so later claims attach to the wrong action, and the action becomes un-editable. That is the shape of the phantom action ids 399–406 the audit found.Removing the fallback alone would not have been safe
The reads underneath it were still truncatable:
actionsForObjectiveused thebyobjectiveSECONDARY index. Secondary reads cannot be resumed — nodeos returns the secondary key asnext_key(a query bounded to 389 returnsnext_key: 389) — so a short read just looks like this objective has fewer actions than it does, and the caller then hands the create an id that is already taken. It now pages the wholeactiontable through the primary index and filters client-side: 400 rows / 5 calls on prod, on the create path only.objectivesForCommunitytook a single call atlimit: 2000and trusted it. It now pages its scope untilmore: false.Both go through one pager, and
assertCompleteis gone with them — a lone call is never proof of a complete set at any table size, so there is nothing left for a throw-on-moreguard to protect.Retries
Chain reads are retried (3 attempts, backing off). Without a fallback a failed read costs a skipped create until someone reindexes, and about 2 in 100 calls came back without a
rowsarray while paging the action table on prod — the acceptance check below failed 2 of ~105 calls before the retry and passes cleanly after. The node's own message is carried into the error, since Sentry is not a paid account and the log line is the only diagnostic. The claim reader shares the retry for the same reason.Create-path inserts now log and rethrow instead of swallowing. A swallowed insert drops the row while
ledgeredstill records the action as processed, so no reindex revisits it — the likeliest explanation for the two claims found missing on prod.Verification
scripts/verify-create-resolvers.jsagainst the prod chain replays each parent's creation history (first k ids known, ask for the next) and requires the resolver to name id k+1 every time:An exhausted parent throws instead of inventing an id.
verify-claim-resolver.jsstill 636/636 after the reader refactor.