-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: read state with fresh expiry #938
Conversation
when polling for `read_state` requests. This prevents the request from exceeding the `maximum_ingress_expiry` when the replica is slow to respond.
cc @yvonneanne |
size-limit report 📦
|
packages/agent/src/polling/index.ts
Outdated
@@ -14,6 +14,9 @@ export type PollStrategy = ( | |||
) => Promise<void>; | |||
export type PollStrategyFactory = () => PollStrategy; | |||
|
|||
// Default delta for ingress expiry is 5 minutes. | |||
const DEFAULT_INGRESS_EXPIRY_DELTA_IN_MSECS = 5 * 60 * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rust agent seems to use 3min. https://github.com/dfinity/agent-rs/blob/main/ic-agent/src/agent/agent_config.rs#L43
Do we wanna use 3min as the default in the agent-js as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, a constant with the same name and the same value already exists in agent/http/index.ts
https://github.com/dfinity/agent-js/pull/938/files#diff-1deee5c6f68eb82e2be634c32d050e818f857292ed0fa9c9e19026e2ef60869cR57
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Expiry
class does additional rounding down, so it's closer to 4 minutes, generally. I would handle changing the default in a separate PR if we want to match agent-rs
.
I can pull the constant out into a constants
utility file, but I don't want to simply export it because that would put it into the @dfinity/agent
library and it doesn't seem necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, no need to squeeze in too many changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, let's merge and roll this out asap to alleviate user pain :)
Description
With increased load events on some subnets, we have started to see cases where the read_state requests eventually start to have
ingress_expiry
errors after exceptionally long polling. Fortunately, the network allows us to have a separate ingress_expiry that differs from the one used in the request id duringread_state
requests.This updates the expiry in
polling
andretry
flowsFixes # SDK-1855
How Has This Been Tested?
Using existing local and mainnet tests
Checklist: