fix: guard against double-unsubscribe removing wrong subscriber in cache.ts#4252
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an internal cache subscription bug where calling the returned unsubscribe function more than once could remove the wrong subscriber (due to splice(-1, 1) when indexOf returns -1). This aligns the unsubscribe behavior with the existing subscribeCallback utility’s guarded swap-and-pop removal pattern.
Changes:
- Make
unsubscribeidempotent by guarding againstindexOfreturning-1. - Replace
spliceremoval with an O(1) swap-and-pop removal approach (consistent withsubscribe-key.ts).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@tomohiro86 Could you signed the commit ? |
5ff34d5 to
624fc73
Compare
|
@promer94 |
|
Sorry I did not give you the correct context. @tomohiro86 You can follow this documention https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits to sign the commit. |
|
@promer94 |
|
@tomohiro86 It’s required that you have to sign all your commits. You might need to rebase your current commits. Coding agent can help you with that. |
When the unsubscribe function returned by `subscribe` was called more than once, `subs.indexOf(callback)` returned -1, causing `subs.splice(-1, 1)` to silently remove the last subscriber in the array instead of doing nothing. Fix by checking `index >= 0` before mutating the array, using the same O(1) swap-and-pop pattern already used in `subscribe-key.ts`. Signed-off-by: tomohiro86 <supersonic.dps@gmail.com>
ad58814 to
6c37b79
Compare
|
Thanks for the heads-up! I've rebased and signed the commit — GitHub now shows it as verified. |
Summary
Fixes #4251
subscribeinsrc/_internal/utils/cache.tsreturned() => subs.splice(subs.indexOf(callback), 1)indexOfreturned-1andsplice(-1, 1)silently removed the last subscriber in the array instead of being a no-opindex >= 0guard and uses the same O(1) swap-and-pop pattern already present insrc/_internal/utils/subscribe-key.tsChanges
src/_internal/utils/cache.ts: align the unsubscribe logic withsubscribeCallbackinsubscribe-key.tsTest plan
pnpm test: 360 passed, 5 skipped)tsc --noEmit)pnpm build)