Activity

1. chappie-daemon

   added

   enhancementNew feature or requestNew feature or request

   on Oct 31, 2026
2. chappie-daemon commented on Oct 31, 2026

   

   chappie-daemon

   on Oct 31, 2026

   CollaboratorAuthor

   More actions

   Answers measured on origin/main at f58ae15 (the #1005 merge). Nothing was changed; the worktree only read files.

   1. Unit, end to end — milliseconds everywhere, but nothing in the code says so

   hop	line	units handled
   shipped defaults	resources/default-settings.yaml:261-266 (connection 1000, 60000), :326 (event 60000), :389-392 (message 60000)	ms
   type	src/@types/settings.ts:25-29 — period: number	no unit stated
   validation	src/utils/settings-config.ts:585-601 — only period > 0	no unit enforced
   event rejection	src/handlers/event-message-handler.ts:453 → Math.max(1, Math.ceil(period)), consumed at :97, sent at :104	ms, unconverted
   read rejection	src/adapters/web-socket-adapter.ts:299 → sent at :324	ms, unconverted
   EWMA internals	src/utils/ewma-rate-limiter.ts:18 (lambda = ln2/period) and :22 (EXPIRE ceil(period/1000))	ms in, the /1000 is only the Redis TTL

   The only division by 1000 anywhere in the path is the key's EXPIRE, which converts to seconds for Redis. No multiplication exists. No stale "seconds" wording survives in the limiter or the handler — the two seconds strings in event-message-handler.ts:223,231 are created_at deltas, a different field.

   Wire format against the PR text ("zero-free decimal string of a positive integer in milliseconds"): String(Math.max(1, Math.ceil(60000))) is "60000" — integer, positive, no leading zeros. Compliant.

   The gap is documentation, not behaviour: RateLimit.period states no unit, and the validator accepts any positive number, so period: 60 written in seconds is silently 60 ms — 1000× tighter than intended and invisible in review.

   2. Remaining time versus the window period — period is correct, and PTTL is the wrong tool

   For sliding_window, period is a true upper bound. The script prunes everything older than timestamp - period on every call (src/utils/sliding-window-rate-limiter.ts:15-17), so every entry present at rejection time is younger than period and ages out within the next period. Waiting period leaves the set empty and the next event is accepted. Pessimistic, never wrong.

   PTTL would not answer the question even if it existed:

   • PEXPIRE is only called on the accepted path (:47); a rejection returns before the ZADD/PEXPIRE. So on the rejection path PTTL is the time until the last accepted entry expires, not the time until capacity returns.
   • The honest value needs the sorted set's scores (ZRANGE … WITHSCORES), which is the same Lua call it already makes — an extra return value, not an extra round trip.
   • ICacheAdapter has no TTL getter at all (src/adapters/redis-adapter.ts exposes only setKeyExpiry), so a PTTL route is new adapter surface.

   Keep period for the sliding window.

   3. EWMA: the number is right only because the Redis TTL happens to equal it

   ewma is the default (resources/default-settings.yaml:221), and there period is a half-life: lambda = ln2/period (src/utils/ewma-rate-limiter.ts:18). Rejection happens when R_new > rate, and the time until R(t) = rate is

   $$t = period \cdot \log_2!\left(\frac{R_{new}}{rate}\right)$$

   so the value we send is exact only when the client overshoots by exactly 2×. With rate = 6, period = 60000:

   R_new	true wait	what we send
   6.6	8.3 s	60 s
   12	60 s	60 s
   24	120 s	60 s
   100	243.5 s	60 s

   We would be under-reporting badly — except that the key is given a TTL of exactly period milliseconds (:22, EXPIRE … ceil(period/1000)), so after period of silence the hash is gone and R_old reads back as 0. The number is safe today solely because the TTL equals the period. That invariant is nowhere stated; lengthening the EXPIRE to keep the state warm — a natural thing to do while tuning — would silently turn a safe value into an under-estimate.

   What a correct value looks like: the script already computes R_new, so returning it instead of 0/1 and computing Math.ceil(period * Math.log2(R_new / rate)) in TypeScript costs one extra return value and no extra round trip. Where R_new is unavailable, send nothing — absence is explicitly allowed to mean "unspecified" and MUST NOT be read as retry-immediately.

   4. The failure path — rate-limited is the wrong prefix

   src/handlers/event-message-handler.ts:445-447 logs and return true when the limiter backend throws. At :97, typeof true === 'number' is false, so no parameter is sent and the reply is rate-limited: slow down.

   Two things are wrong with that. A Redis outage is not a rate limit, and the spec's own example for exactly this condition is error: could not connect to the database; and rate-limited without a value still instructs every connected client to hold off, which is a synchronised retry storm at the moment the cache returns. Recommend error: rate limiter unavailable (fail-closed behaviour unchanged — the event is still rejected).

   5. The connection limiter — the issue's premise is wrong, and the answer is unchanged

   It does not send a NOTICE. src/adapters/web-socket-server-adapter.ts:137-140 calls client.terminate() — no frame, no message, the socket dies. On HTTP, src/handlers/request-handlers/rate-limiter-middleware.ts:17-20 does response.destroy().

   So there is no NIP-01 message to carry a parameter: correctly out of the spec's scope. Worth one line in the docs that a client cannot distinguish this from a network failure, which is the cost of that choice.

   6. Client side — the probe is safe; the bucket key is not

   src/utils/relay-probe/ws-protocol-probe.ts reads by index with no arity assumptions: :232/:236 (OK), :253 (CLOSED), :302/:307 (OK). No zod tuple covers OK/CLOSED. Our own tests pin the extra element deliberately (test/unit/adapters/web-socket-adapter.spec.ts:596,641; test/unit/handlers/event-message-handler.spec.ts:138,153; test/unit/utils/messages.spec.ts:68,88). Nothing mishandles a parameter it does not know.

   One real mismatch, found while checking this: the message limiter buckets by ${client}:message:${period} (src/adapters/web-socket-adapter.ts:291) and is consulted for every inbound message before parsing (:221), so the bucket mixes message types. The spec says the client should back off the same message type that triggered the rate limit. A client that honours that for a REQ can keep the shared bucket full with other types and be rejected again on retry. The fix is one key segment (${client}:${message[0]}:message:${period}), but see the caveat below: it must be evaluated with the unparsed buffer, since the key is built before validation.

   7. Verdict

   The unit drift is a non-event: we shipped milliseconds, the PR standardised milliseconds, and the wire format is compliant. That is luck rather than design — #805 was written when the PR said seconds.

   follow-up	kind	go / no-go
   EWMA backoff from R_new, or omit the parameter when the strategy is EWMA	behaviour	go — small, and it removes the hidden TTL coupling
   error: rate limiter unavailable on the fail-closed path	behaviour	go — one string, removes the retry storm
   message limiter key includes the message type	behaviour	maybe — real mismatch, but it changes bucket layout and needs a key-migration note
   document period as ms on RateLimit, in the defaults, and reject non-positive/non-integer periods	docs + validation	go — cheapest, and it is what would have caught a seconds typo

   Nothing here is urgent enough to hold a release. The first two are one PR.

   Opened as follow-ups: see the comments below.
3. chappie-daemon commented on Oct 31, 2026

   

   chappie-daemon

   on Oct 31, 2026

   CollaboratorAuthor

   More actions

   Follow-ups opened from the go/no-go table:

   • Rate-limit responses: EWMA backoff is a half-life, an outage is reported as rate-limited, and the message bucket ignores message type #1006 — EWMA backoff, the error: prefix on a limiter outage, and the message bucket ignoring message type
   • Document and validate that RateLimit.period is milliseconds #1007 — document and validate that period is milliseconds

   Closing this spike: the drift turned out to be a non-event on the wire (we already emit milliseconds, and the format is compliant), and the real findings are in the two issues above rather than in the unit question the spike started from.
4. chappie-daemon

   closed this as completedon Oct 31, 2026
5. cameri

   added 2 commits that reference this issue on Oct 31, 2026

   fix(rate-limit): the backoff a strategy means, an outage named as one…

   ...

   fd7679b

   fix(rate-limit): send the backoff the strategy means, name an outage …

   ...

   aa7564c
6. chappie-daemon

   added a commit that references this issue on Oct 31, 2026

   chore: release new version 🚀 (#1013)

   ...

   bc22677