Skip to content

fix: sequence invalidation, batch timing metadata, weak notification listeners - #881

Merged
Just-Bamford merged 3 commits into
Sorokit:mainfrom
Speedable2:fix/core-assigned-batch-chidalu
Oct 21, 2026
Merged

fix: sequence invalidation, batch timing metadata, weak notification listeners#881
Just-Bamford merged 3 commits into
Sorokit:mainfrom
Speedable2:fix/core-assigned-batch-chidalu

Conversation

@Speedable2

@Speedable2 Speedable2 commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Summary

This batch fixes three stale-state and observability gaps in the transaction/notification layer: a stale sequence cache after a failed submission, missing timing metadata on batch submission results, and strong-referenced notification channel listeners that prevented garbage collection.

closes #836
closes #839
closes #842

Changes

  • AccountSequenceTracker does not invalidate sequence on failed submissions — stale sequence is reused #836 - AccountSequenceTracker reuses stale sequence after failed submission: src/transaction/buildTransaction.ts's autoFetchSequence cache (used by payment/offer/trustline builders) was never invalidated when a submission failed with a sequence-related error, so a tx_bad_seq failure cascaded into further failures until the cache's 5s TTL expired. Added isSequenceError() in src/shared/horizonErrorMapper.ts, which detects sequence errors from Horizon's structured extras.result_codes (both transaction- and operation-level codes) with a message-based fallback. Added invalidateSequenceCacheEntry(publicKey) in buildTransaction.ts. submitTransaction.ts's failure path now calls this for every source account (including muxed-account resolution) in the transaction whenever the failure is sequence-related, forcing the next build to fetch a fresh sequence from Horizon instead of reusing the stale one.
  • BatchTransactionResult does not include timing metadata — cannot measure per-transaction latency or order #839 - BatchTransactionResult missing timing metadata: Added submittedAtMs, confirmedAtMs, and attemptCount to BatchTransactionStatus in src/transaction/batchSubmitter.ts. A new submitEntry() helper records the submit timestamp and increments the attempt count immediately before calling submitTransaction, and applyResult() records the confirmation timestamp once the result settles, on both success and failure. Wired into both sequential and parallel (Promise.allSettled) submission modes, including the rejected-promise branch in parallel mode.
  • NotificationChannel listeners are not weak references — event listeners prevent garbage collection of subscribers #842 - NotificationChannel listeners prevent garbage collection: src/transaction/paymentNotifications.ts's channelRegistry was a strong Set<PaymentNotificationChannel>, so a subscriber was kept alive forever once registered. Converted to a WeakSet (for dedup checks) plus a WeakRef-backed list (channelRefs) for enumeration, since WeakSet has no iteration protocol. Delivery now goes through liveChannels(), which dereferences each WeakRef and lazily prunes dead ones. Added getRegisteredChannelCount() as an observability/test helper. Required adding ES2021.WeakRef to tsconfig.json's lib array (target stays ES2020; this only adds the ambient WeakRef type declaration, available natively in the Node versions this package already targets).

#841 (ManageOfferManager allows identical buying/selling assets) was found already implemented on main - buildManageOfferTransaction in src/transaction/buildTransaction.ts (lines ~1906-1910) already rejects offers where sellingResult.data.equals(buyingResult.data), returning a TX_BUILD_FAILED error. No change needed; left out of this batch. Flagging so the issue can be closed/confirmed by a maintainer independent of this PR.

Test plan

  • Added src/shared/horizonErrorMapper.test.ts: unit tests for isSequenceError() against structured Horizon result codes (transaction- and operation-level), message fallback, and negative cases; confirms mapHorizonError still maps tx_bad_seq to INVALID_TRANSACTION.
  • Added src/tests/submit-sequence-invalidation.test.ts: integration-style test mocking Horizon.Server end-to-end with a real signed testnet XDR, proving a tx_bad_seq submission failure forces the next autoFetchSequence build for that account to re-fetch from Horizon, while an unrelated failure (e.g. tx_insufficient_fee) leaves the cache untouched.
  • Extended src/tests/transaction.test.ts's "sequence number auto-fetch cache" suite with a test for invalidateSequenceCacheEntry(), confirming it clears only the targeted account's entry and leaves others cached.
  • Extended src/transaction/batchSubmitter.test.ts with four new tests covering timing metadata on success, failure, sequential ordering, and parallel mode (including a rejected promise).
  • Extended src/tests/paymentNotifications.test.ts with a dedup test (registering the same channel instance twice only counts once) and a GC test guarded behind typeof global.gc === "function" so it never flakes the suite under a different Node/vitest flag configuration.
  • Ran the full suite locally: npm run typecheck, npm run lint, and npm run test all pass (192 test files, 3846 tests passed, 0 failed, 80 pre-existing skips).

Not run against a live Horizon network; the sequence-invalidation test mocks Horizon.Server with a real signed XDR fixture rather than hitting testnet.

@Speedable2
fix(transaction): use weak references for notification channel listen…
c6ae108 
…ers (Sorokit#842)

Convert the payment notification channel registry from a strong Set to a
WeakSet + WeakRef-tracked list, so a subscriber becomes eligible for
garbage collection as soon as no other reference to it remains, instead of
being pinned alive indefinitely by the module-level registry.
@Speedable2
feat(transaction): add timing metadata to batch transaction results (S…
b56c5ac 
…orokit#839)

Add submittedAtMs, confirmedAtMs, and attemptCount to BatchTransactionStatus
so callers can measure per-transaction latency, detect unusual delays, and
inspect retry/attempt counts after a submitBatch() call, in both sequential
and parallel mode.
@Speedable2
fix(transaction): invalidate cached sequence on tx_bad_seq submission…
7fcf2fc 
… failure (Sorokit#836)

A failed submission with a sequence-related error (e.g. tx_bad_seq) left
the build-time autoFetchSequence cache holding the stale, now-incorrect
sequence number, causing every subsequent submission for that account to
fail with tx_bad_seq until the cache's TTL expired.

Add isSequenceError() to detect sequence-related Horizon failures (via the
structured extras.result_codes, with a message-based fallback), and
invalidateSequenceCacheEntry() to clear a single account's cached
sequence. submitTransaction's failure path now invalidates the cache entry
for every source account in the transaction when the failure is
sequence-related, forcing the next build to fetch a fresh sequence.
@drips-wave

drips-wave Bot commented Oct 21, 2026

Copy link
Copy Markdown

@Speedable2 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Just-Bamford
Just-Bamford merged commit bbe0587 into Sorokit:main Oct 21, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment