refactor(data-common): PGlite init-retry helper follow-ups from #205 review - #358
Open
sharonyajain wants to merge 3 commits into
Open
refactor(data-common): PGlite init-retry helper follow-ups from #205 review#358sharonyajain wants to merge 3 commits into
sharonyajain wants to merge 3 commits into
Conversation
…elper - Narrow the WASM-trap classifier to specific signatures (RuntimeError: unreachable, wasm trap: unreachable, Emscripten Aborted()) instead of the bare word 'unreachable', so an unrelated probe failure that merely contains the word (assertUnreachable helpers, 'unreachable host') is not retried. - Classify retryability before consulting the attempt budget in initializePgliteWithRetry, making instance cleanup uniform and fixing the maxAttempts=1 edge that closed on any failure without classifying. - Mark the four init-retry exports @internal (shared only across engine packages) and document the PgliteLike methods; regenerate API.md. - Update/extend tests: trap-signature positives, a bare-word negative, and maxAttempts=1 cleanup regressions.
🦋 Changeset detectedLatest commit: 079bc35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
ikenyal
approved these changes
Aug 14, 2026
ikenyal
left a comment
ikenyal
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the classifier narrowing, the retry-budget ordering, and the generated API report. The new ordering correctly preserves a non-retryable instance when maxAttempts is 1 while still closing a diagnosed trap before giving up. The focused PGlite helper tests cover message, stack, cause-chain, negative, and exhausted-budget cases; I also ran all 18 of those tests locally. No blocking issues found.
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.
Follow-up to #205 addressing @sarayev's three review comments based on improvement on the shared PGlite init-retry helper (
data-common). No behavior change for the happy path or the recovery path shipped in #205 — this is polish + hardening.1. Narrow the WASM-trap classifier (
isPgliteUnreachableTrap)Previously matched the bare word
unreachableanywhere in the message/stack, which could misclassify an unrelated probe failure (anassertUnreachablehelper, an "unreachable host" message) as a retryable trap. Now matches specific signatures only:RuntimeError: unreachable,wasm trap: unreachable, and the EmscriptenAborted()form.2. Uniform cleanup in
initializePgliteWithRetryRetryability is now classified before consulting the attempt budget. A non-retryable error is always rethrown untouched (we never close an instance we didn't diagnose as a dead WASM trap), and this fixes the
maxAttempts === 1edge that previously closed the instance on any failure without classifying it.3. Mark the init-retry exports
@internalinitializePgliteWithRetry,isPgliteUnreachableTrap,PgliteLike, andPgliteInitRetryOptionsexist only so the two engine packages can share the helper — they were needlessly part ofdata-common's public API. Marked@internal(and documented thePgliteLikemethods);API.mdregenerated to match.Tests
RuntimeError: unreachable,wasm trap:,Aborted()), including stack-only and cause-chain paths.unreachablewithout a trap signature is not retried.maxAttempts=1regressions: a non-retryable error does not close the instance; a trap still closes it and does not recreate.