fix: add recovery for invalid POI sync state on startup#3014
Conversation
When latestSyncedPoiHeight in metadata points to an invalid POI (created state without hash/parentHash), the node would fail to start. Now it searches backward to find the last valid synced POI and updates the metadata accordingly.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdded backward-search recovery and error handling to POI synchronization: new helper to find the last valid POI, updates to metadata and in-memory state when the metadata POI is invalid or missing, and changes to POI model exports and bulk upsert logic to avoid overwriting synced hash/parentHash when not present. Changes
Sequence Diagram(s)sequenceDiagram
participant SyncSvc as "POI Sync Service"
participant DB as "Poi Table (DB)"
participant Meta as "Metadata Store"
participant Mem as "In-Memory State"
SyncSvc->>Meta: read latestSyncedPoiHeight
alt metadata height present
SyncSvc->>DB: query Poi by id = metadata.height
DB-->>SyncSvc: poi (may be invalid/missing)
alt poi valid
SyncSvc->>Mem: set latestSyncedPoi
else poi invalid or missing
SyncSvc->>DB: query Poi where id <= startHeight ORDER BY id DESC LIMIT 1 (hash & parentHash not null)
DB-->>SyncSvc: lastValidPoi or none
alt found
SyncSvc->>Meta: update latestSyncedPoiHeight
SyncSvc->>Mem: set latestSyncedPoi
else not found
SyncSvc->>Meta: clear latestSyncedPoiHeight
SyncSvc->>Mem: clear latestSyncedPoi
end
end
else metadata missing
SyncSvc->>DB: similar backward search for valid POI...
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a21bf06e60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
|
|
||
| if (result) { | ||
| return result.toJSON<ProofOfIndex>(); |
There was a problem hiding this comment.
In CockroachDB, POI id can come back from Sequelize as a string (the existing PlainPoiModel methods normalize this via ensureProofOfIndexId in storeModelProvider/poi/poi.ts), but findLastValidSyncedPoi returns result.toJSON() directly without that normalization. When this new startup-recovery branch is hit, _latestSyncedPoi.id may be a string, and later arithmetic such as this.latestSyncedPoi.id + 1 in syncPoiJob can do string concatenation and break ordering/gap handling. Normalize the recovered row before storing metadata and calling setLatestSyncedPoi.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/node-core/src/indexer/poi/poiSync.service.ts (1)
248-290: Consider extracting the repeated recovery/update block into a helper.Both branches (invalid record vs missing record) share the same search/update/clear flow; a small helper would reduce drift and keep the method tighter.
♻️ Example refactor to centralize recovery
+ private async recoverLatestSyncedPoi(startHeight: number, reason: string): Promise<ProofOfIndex | undefined> { + logger.warn(`${reason} Searching for last valid synced POI...`); + const validPoi = await this.findLastValidSyncedPoi(startHeight); + if (validPoi) { + logger.info(`Found last valid synced POI at height ${validPoi.id}, updating metadata`); + const tx = await this.newTx(); + await this.updateMetadataSyncedPoi(validPoi.id, tx); + await tx.commit(); + return validPoi; + } + logger.warn(`No valid synced POI found below height ${startHeight}, clearing metadata`); + const tx = await this.newTx(); + await this.metadataRepo.destroy({where: {key: 'latestSyncedPoiHeight'}, transaction: tx}); + await tx.commit(); + return undefined; + }- const validPoi = await this.findLastValidSyncedPoi(latestSyncedPoiHeight); - if (validPoi) { - logger.info(`Found last valid synced POI at height ${validPoi.id}, updating metadata`); - const tx = await this.newTx(); - await this.updateMetadataSyncedPoi(validPoi.id, tx); - await tx.commit(); - this.setLatestSyncedPoi(validPoi); - } else { - // No valid synced POI found, clear metadata - logger.warn(`No valid synced POI found below height ${latestSyncedPoiHeight}, clearing metadata`); - const tx = await this.newTx(); - await this.metadataRepo.destroy({where: {key: 'latestSyncedPoiHeight'}, transaction: tx}); - await tx.commit(); - } + const validPoi = await this.recoverLatestSyncedPoi( + latestSyncedPoiHeight, + `Metadata latestSyncedPoiHeight ${latestSyncedPoiHeight} points to an invalid POI (created state, not synced).` + ); + if (validPoi) this.setLatestSyncedPoi(validPoi);- const validPoi = await this.findLastValidSyncedPoi(latestSyncedPoiHeight); - if (validPoi) { - logger.info(`Found last valid synced POI at height ${validPoi.id}, updating metadata`); - const tx = await this.newTx(); - await this.updateMetadataSyncedPoi(validPoi.id, tx); - await tx.commit(); - this.setLatestSyncedPoi(validPoi); - } else { - // No valid synced POI found, clear metadata - logger.warn(`No valid synced POI found below height ${latestSyncedPoiHeight}, clearing metadata`); - const tx = await this.newTx(); - await this.metadataRepo.destroy({where: {key: 'latestSyncedPoiHeight'}, transaction: tx}); - await tx.commit(); - } + const validPoi = await this.recoverLatestSyncedPoi( + latestSyncedPoiHeight, + `POI record not found at metadata height ${latestSyncedPoiHeight}.` + ); + if (validPoi) this.setLatestSyncedPoi(validPoi);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/node-core/src/indexer/poi/poiSync.service.ts` around lines 248 - 290, The two recovery branches duplicate the same "search for valid POI, update metadata and setLatestSyncedPoi, or clear metadata" logic; extract that into a helper method (e.g., recoverOrClearToLatestSyncedPoi) that takes latestSyncedPoiHeight and returns/sets the chosen POI or clears metadata. Move the shared calls to findLastValidSyncedPoi, updateMetadataSyncedPoi, newTx, tx.commit, setLatestSyncedPoi and metadataRepo.destroy into that helper and replace both branches with a single call to recoverOrClearToLatestSyncedPoi to remove duplication and keep syncLatestSyncedPoiFromDb concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/node-core/src/indexer/poi/poiSync.service.ts`:
- Around line 248-290: The two recovery branches duplicate the same "search for
valid POI, update metadata and setLatestSyncedPoi, or clear metadata" logic;
extract that into a helper method (e.g., recoverOrClearToLatestSyncedPoi) that
takes latestSyncedPoiHeight and returns/sets the chosen POI or clears metadata.
Move the shared calls to findLastValidSyncedPoi, updateMetadataSyncedPoi, newTx,
tx.commit, setLatestSyncedPoi and metadataRepo.destroy into that helper and
replace both branches with a single call to recoverOrClearToLatestSyncedPoi to
remove duplication and keep syncLatestSyncedPoiFromDb concise.
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
|---|---|---|---|
| 🟡 | Statements | 70.11% | 18556/26468 |
| 🟡 | Branches | 78.29% | 2099/2681 |
| 🟡 | Functions | 62.5% | 860/1376 |
| 🟡 | Lines | 70.11% | 18556/26468 |
Report generated by 🧪jest coverage report action from 27304f6
1 participant
When latestSyncedPoiHeight in metadata points to an invalid POI (created state without hash/parentHash), the node would fail to start. Now it searches backward to find the last valid synced POI and updates the metadata accordingly.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor