fix(shared): make ApiManager refresh paths actually replace the stale api#1259
Merged
Conversation
… api populateApi() returns the cached instance when one exists, but it was also used as the refresh mechanism in getApi() (on spec version change) and executeApiCall() (on bad-signature retry). In both cases the cached ApiPromise was returned unchanged, so the refresh was a no-op and the stale connection kept being used. Add refreshApi(), which disconnects the cached instance and replaces it with a fresh connection, and use it on the spec-version-change path, the forceRefresh path, and the bad-signature retry. The retry also rebuilds the extrinsic against the refreshed api, since the original call is bound to the stale registry. populateApi/populateAllApis stay idempotent for normal startup. Add tests covering populateApi idempotency and both refresh paths; the refresh tests fail against the previous implementation.
✅ Deploy Preview for vortexfi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for vortex-sandbox ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a stale-cache bug in ApiManager where “refresh” code paths were reusing an already-cached ApiPromise, making refresh operations (spec-version change and bad-signature retry) effectively no-ops.
Changes:
- Introduces
refreshApi()to discard a cached API instance and create a new connection, and refactors connect/cache behavior intocreateAndCacheApi(). - Updates
getApi()to refresh on spec-version changes and whenforceRefreshis requested. - Updates the bad-signature retry path in
executeApiCall()to refresh and rebuild the extrinsic against the refreshed API registry; adds tests covering refresh semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/shared/src/services/pendulum/apiManager.ts | Adds explicit refresh behavior and updates refresh call sites (spec-version change + bad-signature retry). |
| packages/shared/src/services/pendulum/apiManager.test.ts | Adds unit tests asserting refresh replaces cached instances and disconnects stale ones. |
Comments suppressed due to low confidence (1)
packages/shared/src/services/pendulum/apiManager.ts:141
createAndCacheApistores the new API instance inapiInstancesbefore ensuring it is connected andisReady. If another request callspopulateApi/getApiconcurrently, it can observe the cached instance and start using it while it is still connecting. Caching should happen only after the API is confirmed ready (or the method should otherwise guard concurrent readers).
const instanceKey = this.generateInstanceKey(networkName, index);
const newApi = await this.connectApi(networkName, index);
this.apiInstances.set(instanceKey, newApi);
if (!newApi.api.isConnected) await newApi.api.connect();
await newApi.api.isReady;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback on refreshApi: the stale instance was removed and disconnected before the replacement connection existed, so a failed reconnect left the network without any cached api. Create the new instance first, then swap and best-effort disconnect the stale one. Also cache new instances only after connect/isReady so concurrent readers never observe a still-connecting api. Add a test asserting a failed refresh keeps the previous instance cached and undisconnected.
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.
Summary
populateApi()inpackages/shared/src/services/pendulum/apiManager.tsreturns the cached instance when one exists, but it was also used as the "refresh" mechanism in two places:getApi()when a spec version change is detected (logs "refreshing the api...")executeApiCall()on the "Transaction has a bad signature" retryIn both cases the cached instance was returned unchanged, so the refresh was a no-op — the stale
ApiPromisekept being used.Changes
refreshApi(), which removes the cached instance fromapiInstances, disconnects the oldApiPromise(best-effort with a warning log), and creates a fresh connection. The connect-and-cache logic is shared withpopulateApivia a privatecreateAndCacheApihelper, sopopulateApi/populateAllApisremain idempotent for normal startup.getApi()usesrefreshApi()on the spec-version-change path, and also forforceRefresh: true, which had the same no-op bug (no current callers pass it).executeApiCall()usesrefreshApi()and rebuilds the extrinsic viacreateCall(refreshedApi.api)— the original call object is bound to the stale api's registry, so re-signing it would defeat the refresh even with a new connection.Tests
New
ApiManager api refreshblock inapiManager.test.tsstubs the privateconnectApiwith fake instances whose reported spec version is controllable:populateApiidempotency (cached instance reused, no reconnect, no disconnect)apiInstancesforceRefreshpath: same replacement + disconnect semanticsBoth refresh tests were verified to fail against the previous implementation (
getApireturned the identical stale instance).Verified with
bun testinpackages/shared(19 pass),bun typecheck, andbun lint.