Repository navigation
fix(rate-limiter): namespace EWMA keys away from sliding-window keys - #945
chappie-daemon wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 5e994d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
EWMA and sliding_window store different Redis types under the same key name: a sliding window keeps a sorted set, EWMA keeps a hash. Both write `<identity>:<bucket>:<period>`, and 2.x shipped only the sliding window, so a cache volume carried across the upgrade holds sorted sets where the EWMA script then runs HGET - a WRONGTYPE error raised on every connection and every message until the old key expires. Switching `limits.rateLimiter.strategy` back and forth does the same thing to a live instance. EWMA now writes under an `ewma:` prefix. A prefix rather than a delete-if-wrong-type guard, so both strategies keep their own window and a switch back to sliding_window resumes it instead of starting from zero. Reproduced before the change on a real Redis 6.2 with a zset seeded at the shared key name: `@user_script:6: WRONGTYPE`, the same error and line the issue reports. After it, the same call returns normally, writes a hash under `ewma:`, and leaves the legacy zset untouched. The unit test pins the key the script receives. The integration scenario seeds a real sliding-window zset under the shared name and asserts the client is still rate limited, so a regression fails on the behaviour rather than on a string.
8ce1593 to
5e994d2
Compare
|
🔍 Site preview for this PR (includes all changes on this branch): https://nostream-web-pr-945.cameri.workers.dev/preview/945/ The https://ema.tnsor.network/preview/945/ form serves the same build once this PR merges — the production router that proxies /preview/* ships with it. Updates with every push to this PR; deleted when the PR closes. |
[Medium impact] The PR affects rate limiting and the container release and deployment workflow. Keep prereleases from replacing the production Findings
|
| # here is what makes `latest` something this repository promises. | ||
| tags: | | ||
| type=semver,pattern={{version}} | ||
| type=raw,value=latest |
There was a problem hiding this comment.
Prereleases replace the production image
Publishing a prerelease such as v3.7.0-beta.1 now overwrites latest. The workflow accepts every published v* release, and the new type=raw,value=latest rule also applies to prereleases. Both production stacks now use latest, so their next update can install a test release. Only assign latest when github.event.release.prerelease is false.
| type=raw,value=latest | |
| type=raw,value=latest,enable=${{ !github.event.release.prerelease }} |
| None. Developers and deployment users receive the same `main` and per-commit | ||
| container image tags automatically after CI succeeds. | ||
| Deployments tracking `ghcr.io/cameri/nostream:main` should be updated to track released versions, | ||
| such as `ghcr.io/cameri/nostream:latest` or pinned release tags (e.g. `ghcr.io/cameri/nostream:v3.6.2`). |
There was a problem hiding this comment.
| 1. `NOSTREAM_TAG` in `/opt/nostream/webhook/.env` set to `latest`. | ||
| 2. The GitHub webhook subscribed to **releases** as well as **push**, because | ||
| the receiver ignores anything that is not a push to the watched branch. | ||
|
|
||
| Both live in `containers/nostream-dev-webhook/` (the versioned source) with its | ||
| own README. Until both are done, deployments are manual: pull the tag by hand |
There was a problem hiding this comment.
Webhook migration steps are incomplete
If the receiver ignores non-push events as stated here, subscribing the GitHub webhook to releases will not make it accept them. Neither listed step changes that filter, so operators can finish both steps without restoring automatic image pulls. Document the receiver change as a required step and link its actual source; containers/nostream-dev-webhook/ is absent from this repository.
Coverage Report for CI Build 40029515750Warning No base build found for commit Coverage: 75.467%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats💛 - Coveralls |
Takes @cameri's call in the issue and @Perlover's diagnosis: namespace the EWMA keys. Closes #930.
The bug, reproduced
Seeded a real Redis 6.2 with a sliding-window sorted set at
1.2.3.4:connection:120000and ran the shipped EWMA script against it:Same error, same line as the report. Both strategies build their key the same way (
<identity>:<bucket>:<period>) but store different types — a sorted set versus a hash — so any key left bysliding_windowbreaks everyewmarequest that touches it.The fix
EWMARateLimiternow writes under anewma:prefix:A prefix rather than the self-healing
TYPE/DELguard the issue also suggested. Deleting the key would destroy the sliding window's state, so switchingstrategyback would silently start from a fresh window — and that switch is a normal operation, not an upgrade path.Affected call sites are unchanged; the prefix is applied once, inside the limiter, so nothing has to remember it.
Verification
hit()against a legacy zsetWRONGTYPE @user_script:6…:connection:120000(hash, collided)ewma:…:connection:120000(hash)zsetThe before/after used the built
EWMARateLimiterclass against real Redis 6.2, not a stub — a mocked adapter cannot produce aWRONGTYPE, which is exactly why the existing unit test never caught this.Tests. A unit test pins the key the script receives. An integration scenario seeds a real sliding-window zset under the shared name and still expects the client to be rate limited, so a regression fails on behaviour. The existing
@rate-limiterstep now seeds the namespaced key, since seeding the old name would leave the limiter on a fresh window and the original scenario would stop covering anything.Full unit suite: 2521 passing, 5 failing — the same
user-subscription-repositoryfailures at base, untouched by this change.tsc --noEmitandbiomeclean.Upgrade note
No manual Redis cleanup: old zsets stay where they are and simply stop being read. They expire on their own TTL, and the
ewma:keys start empty, so the first request after the upgrade is unrestricted — the same fresh-start a restart already gives a sliding window.