Skip to content

fix(sdk): report MigrationCooldownNotMet as a skip, not a failure (#725) - #737

Merged
collinsezedike merged 3 commits into
drydocs:mainfrom
Iker2522:fix/migration-cooldown-skip-not-failure
Sep 7, 2026
Merged

fix(sdk): report MigrationCooldownNotMet as a skip, not a failure (#725)#737
collinsezedike merged 3 commits into
drydocs:mainfrom
Iker2522:fix/migration-cooldown-skip-not-failure

Conversation

@Iker2522

@Iker2522 Iker2522 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Summary

#557 lengthened migrate_adapter's MIN_LEDGER_GAP cooldown from ~1 minute to ~1 day. The migration keeper didn't special-case the resulting MigrationCooldownNotMet rejection, so every hourly run during that ~1-day window produced an HTTP 500 and a failure log entry — undermining the timelock's purpose of giving observers a calm window to watch for a malicious begin_migration.

What changed

Added isMigrationCooldownError (packages/stellar-sdk-helpers/src/keeper-tx.ts), matching the contract's MigrationCooldownNotMet = 20 in the raw simulation error text — same message-text approach isStaleAdapterError already uses, since withKeeperRetry's wrapping loses the original error's type.
migration-keeper.ts now checks it before the existing isStaleAdapterError check and reports a skip instead of a failure.
api/v1/keepers/[action].ts needs no changes — its failures.length > 0 ? 500 : 200 status logic already does the right thing once the count is accurate.
Fixed a stale comment describing the bug as intended behavior, and updated apps/docs/operations/migration-keeper.md, which already documented this exact gap as a tracked follow-up.

Test plan

pnpm typecheck clean
75/75 tests pass (2 new: a unit test for isMigrationCooldownError, and an integration test asserting the full runMigrationKeeper path reports skipped/not failures/no retry for a #20 simulation rejection)
apps/docs vitepress build succeeds
prettier --check passes on every changed file

Closes #725

@vercel

vercel Bot commented Sep 7, 2026

Copy link
Copy Markdown

@Iker2522 is attempting to deploy a commit to the Collins' projects Team on Vercel.

A member of the Team first needs to authorize it.

@Iker2522

Iker2522 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Hi! This PR fixes issue #725 — MigrationCooldownNotMet now reports as a skip instead of a failure, so the keeper stops paging on expected behavior during the ~1-day cooldown window. All three acceptance criteria are covered: detection + skip handling, updated docs, and test coverage (unit + integration). Ready for review whenever you get a chance — let me know if anything needs adjusting!

@collinsezedike

Copy link
Copy Markdown
Collaborator

@Iker2522 the code here is correct, I checked it against the actual contract source and traced the fix through the loop, lease release, and both the unit and integration tests. Nothing to change there.

CI is failing on Commit Messages and PR Title because neither has a Conventional Commits type prefix. Please read CONTRIBUTING.md's "Commit Convention" section and update both to something like fix(sdk): report MigrationCooldownNotMet as a skip, not a failure.

Iker2522 and others added 2 commits September 7, 2026 18:07
drydocs#557 lengthened migrate_adapter's MIN_LEDGER_GAP cooldown from ~1 minute
to ~1 day. The migration keeper didn't special-case the resulting
MigrationCooldownNotMet rejection, so it fell into the generic failure
path: every hourly run during that ~1-day window produced an HTTP 500
and a failure log entry, undermining the timelock's purpose of giving
observers a calm window to watch for a malicious begin_migration.

Adds isMigrationCooldownError (packages/stellar-sdk-helpers/src/keeper-tx.ts),
matching the contract's MigrationCooldownNotMet = 20 (packages/contracts/vault/src/errors.rs)
in the raw simulation error text, the same message-text approach
isStaleAdapterError already uses (withKeeperRetry's wrapping loses the
original error's type). migration-keeper.ts now checks it before the
existing isStaleAdapterError check and reports a skip instead of a
failure; api/v1/keepers/[action].ts needs no changes, since its
'failures.length > 0 ? 500 : 200' status logic already does the right
thing once the count is accurate.

Also fixes a stale comment describing the bug as intended behavior
('comfortably fine given MIN_LEDGER_GAP is ~1 minute'), and updates
apps/docs/operations/migration-keeper.md, which already documented this
exact gap as a tracked follow-up.

Verified: pnpm typecheck clean, 75/75 tests pass (2 new: a unit test for
isMigrationCooldownError and an integration test asserting the full
runMigrationKeeper path reports skipped/not failures/no retry for a
drydocs#20 simulation rejection), apps/docs vitepress build succeeds, and
prettier --check passes on every changed file.
@Iker2522
Iker2522 force-pushed the fix/migration-cooldown-skip-not-failure branch from 518c7b8 to 3bec12d Compare September 7, 2026 18:12
@Iker2522 Iker2522 changed the title Report MigrationCooldownNotMet as a skip, not a failure (#725) fix(sdk): report MigrationCooldownNotMet as a skip, not a failure (#725) Sep 7, 2026

@collinsezedike collinsezedike left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Iker2522 Thank you for the contribution. Feel free to pick up another open issue whenever you're ready.

@collinsezedike
collinsezedike merged commit b8cdc1d into drydocs:main Sep 7, 2026
8 of 9 checks passed
@collinsezedike

Copy link
Copy Markdown
Collaborator

@Iker2522 If you found working on this useful, a star on the repo goes a long way in helping the project grow.

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.

[Bug] Migration keeper reports MigrationCooldownNotMet as a failure for the full ~1-day timelock

2 participants