fix(shared-log): avoid unhandled rejection when entry.hash is missing#591
Open
Faolain wants to merge 1 commit intodao-xyz:masterfrom
Open
fix(shared-log): avoid unhandled rejection when entry.hash is missing#591Faolain wants to merge 1 commit intodao-xyz:masterfrom
Faolain wants to merge 1 commit intodao-xyz:masterfrom
Conversation
- Guard persistCoordinate when entry.hash is missing/invalid - Wrap _waitForReplicators() check() calls to avoid unhandled rejections - Add regression test covering the failure mode
Contributor
Author
|
I couldn't reproduce the I ran: pnpm run build
pnpm run test:ci:part-2And also re-ran the specific test 10x: pnpm --filter @peerbit/document test -- --grep "can search while keeping minimum amount of replicas"All passing locally. This looks like a flake (?) ; a rerun of the CI job would help confirm. |
Contributor
Author
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.


tl;dr Mainly saw this while testing, spinning up and tearing down quickly (in parallel), I know you have tests set up sequentially but I was curious what was actually happening and why this was the case. I figured churning nodes could also be a good usecase for real network topology, especially with the fanout tree + large network sims plan.
test failure seems unrelated and is a flake that has been see before?
Problem
In
@peerbit/shared-log,_waitForReplicators()invokes an asynccheck()from event listeners without awaiting/catching it. Ifcheck()rejects (e.g. becausefindLeaders()callspersistCoordinate()with an entry that is missing/invalidentry.hash), Node emits anunhandledRejection.This shows up as a flaky failure in highly-concurrent test runners (Vitest) and can also surface in production as noisy
unhandledrejectionevents (browser) or process failure depending on Node--unhandled-rejectionsmode.Observed stack trace
Captured by a minimal script that calls
_waitForReplicators()with an entry-like object whosehashisundefined:TypeError: Cannot read properties of undefined (reading 'multihash') at SharedLog.persistCoordinate (.../packages/programs/data/shared-log/dist/src/index.js:2597:85) at SharedLog.findLeaders (.../packages/programs/data/shared-log/dist/src/index.js:2678:37) at async check (.../packages/programs/data/shared-log/dist/src/index.js:2548:37)Fix
persistCoordinate()defensive: ifentry.hashis missing/empty/invalid (or ifcidifyString(hash)fails/returnsundefined), return early._waitForReplicators()'s internalcheck()calls in asafeCheck()that catches rejections so they don't become unhandled promise rejections when triggered from event listeners.Test
Adds
packages/programs/data/shared-log/test/unhandled-rejection.spec.tsto assert that_waitForReplicators()does not emitunhandledRejectionwhen the internalcheck()path fails.Run:
pnpm --filter @peerbit/shared-log test