fix: increase timeout robustness#93
Merged
Merged
Conversation
Two configuration tweaks that independently reduce the chance of a bulletin upload failing on the polkadot-api WS heartbeat or hitting the default Node heap ceiling. No behavioural change to upload code. - packages/cli/src/bulletin/store.ts: Pass heartbeatTimeout=300_000 and timeout=10_000 to getWsProvider. The 40_000 ms ws-provider default is shorter than a single Bulletin chunk's worst-case best-block inclusion latency we measured (>50 s outliers under contention), so the transport was tearing down healthy WS connections while the chain was still acknowledging an in-flight extrinsic. - .github/actions/bulletin/action.yml: Set NODE_OPTIONS=--max-old-space-size=4096 in both the authorize and upload steps. ubuntu-latest runners have ≥16 GiB RAM; default ~2 GiB Node heap leaves no headroom if a transient leak window opens. Cheap safety net.
Move chunk.bytes = null into a finally so a failed submission also releases its 2 MiB. Today the buffer is only freed on success; on failure (stall, retry, abort) it stays alive via the closure, pinning memory for the lifetime of the upload. The payload is re-streamed from disk on the next attempt, so dropping the in-memory copy is always safe.
sevenmachines
requested review from
andrew-ifrita,
corey-hathaway,
filvecchiato,
sphamjoli and
waytwotall
as code owners
CI Summary
Release - PassedTest this PR Download artifact (GitHub CLI required): gh run download 24462300780 -n cli-release-0.0.0-pr.93 -R paritytech/dotns-sdkInstall globally: npm install -g ./parity-dotns-cli-0.0.0-pr.93.tgzVerify: dotns --helpDeploy UI — Passed
Deploy Example — Passed
Labelspkg: cli, scope: bulletin Test - Passed134 passed, 0 failed across 134 tests. |
| // success it is already persisted; on failure it must not pin | ||
| // memory while the caller decides whether to retry — the | ||
| // payload is re-streamed from disk on the next attempt. | ||
| chunk.bytes = null as unknown as Uint8Array; |
Collaborator
There was a problem hiding this comment.
Don't we want only null if there is success?
We could also restructure so in-wave failures reread the bytes from disk, otherwise that is only happening at the wave boundaries now if I am not mistaken (via runWaveWithRetries).
With this change as is, transient failures won't recover on retry
Contributor
Author
There was a problem hiding this comment.
happy to seperate this bit just now and focus on the timeouts. I took this bit out of a bigger error handling piece which might be needed for it to make sense. The timeout stuff should at least help.
Contributor
Author
There was a problem hiding this comment.
reverted it to focus on the timout orchestration in the first instance. Error handling/memory release I'll move to a seperate pr (if we want it)
This reverts commit 4e9b6a8.
andrew-ifrita
changed the title
Contributor
Author
|
lets see what the affect is. should be at least better |
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
2 participants
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.
Description
Type
Package
@parity/dotns-cliRelated Issues
Fixes
Checklist
Code
bun run lintpassesbun run formatpassesbun run typecheckpassesDocumentation
Breaking Changes
Breaking changes:
Testing
How to test:
Notes