diff --git a/changelog.d/fixed-554-nps-relay-ip-slot.md b/changelog.d/fixed-554-nps-relay-ip-slot.md new file mode 100644 index 00000000..d99de57f --- /dev/null +++ b/changelog.d/fixed-554-nps-relay-ip-slot.md @@ -0,0 +1,2 @@ +- The NPS relay no longer consumes the per-IP rate slot when the per-install limit + refuses a submission (hivecommons/docs#554). diff --git a/netlify/nps-relay/relay.test.ts b/netlify/nps-relay/relay.test.ts index 4df9f767..5eacb181 100644 --- a/netlify/nps-relay/relay.test.ts +++ b/netlify/nps-relay/relay.test.ts @@ -19,6 +19,7 @@ import { HEADER_TIMESTAMP, INSTALL_PREFIX, INSTALL_RATE_PREFIX, + IP_RATE_PREFIX, MAX_ACK_BODY_BYTES, MAX_ENTRIES, MAX_FEEDBACK_CHARS, @@ -673,6 +674,53 @@ describe("POST /api/nps (submit)", () => { expect(store.entries()).toHaveLength(MAX_PER_INSTALL_PER_WINDOW); }); + it("releases the per-IP slot when the install limit refuses the submission", async () => { + // The pre-check read (plain get) lags one slot behind, so it passes and + // only the install reservation (conditional read via getWithMetadata) + // refuses; the IP slot taken just before must go back. + const stale = new (class extends MemoryStore { + async get(key: string): Promise { + const rec = await super.get(key); + if (!key.startsWith(INSTALL_RATE_PREFIX) || !rec) return rec; + const { times } = rec as { times: number[] }; + return { times: times.slice(1) }; + } + + async getWithMetadata( + key: string + ): Promise<{ data: unknown; etag?: string } | null> { + const data = await super.get(key); + if (data === null) return null; + return { data, etag: this.etags.get(key) }; + } + })(); + store = stale; + await registerOk(); + store.data.set(`${INSTALL_RATE_PREFIX}${hive.installId}`, { + times: Array.from({ length: MAX_PER_INSTALL_PER_WINDOW }, () => now - 1), + }); + const res = await handleRelayRequest(submit(GOOD), deps()); + expect(res.status).toBe(429); + const installRec = store.data.get( + `${INSTALL_RATE_PREFIX}${hive.installId}` + ) as { times: number[] }; + expect(installRec.times).toHaveLength(MAX_PER_INSTALL_PER_WINDOW); + const ipRecords = [...store.data.entries()].filter(([k]) => + k.startsWith(IP_RATE_PREFIX) + ); + for (const [, rec] of ipRecords) { + expect((rec as { times: number[] }).times).toEqual([]); + } + + const other = newHive(); + await registerOk(other, "198.18.0.2"); + const next = await handleRelayRequest( + submit(GOOD, { signer: other }), + deps() + ); + expect(next.status).toBe(201); + }); + it("omits feedback and dashboard_version from the entry when not supplied", async () => { const res = await handleRelayRequest( submit({ hive_id: "solo-hive", score: 2 }), diff --git a/netlify/nps-relay/relay.ts b/netlify/nps-relay/relay.ts index fa9b1103..3b503015 100644 --- a/netlify/nps-relay/relay.ts +++ b/netlify/nps-relay/relay.ts @@ -571,6 +571,20 @@ function reserveSlot( }); } +/** Gives back one slot taken by `reserveSlot` at `now`. */ +function releaseSlot( + store: RelayStore, + key: string, + now: number +): Promise { + return compareAndSwap(store, key, current => { + const times = liveTimes(current, now); + const i = times.indexOf(now); + if (i !== -1) times.splice(i, 1); + return { next: { times } satisfies RateRecord }; + }); +} + /** Whether a windowed rate record is already at `max`, without reserving. */ async function atLimit( store: RelayStore, @@ -835,8 +849,11 @@ async function handleSubmit( if (await atLimit(store, installKey, MAX_PER_INSTALL_PER_WINDOW, now)) return RATE_LIMITED(); - // Reserve both slots atomically before storing the entry, so concurrent - // submissions cannot all squeeze into the same remaining slot. + // Reserve each slot with its own conditional write before storing the + // entry, so concurrent submissions cannot all squeeze into the same + // remaining slot. The two writes are not atomic together: if the install + // reservation is refused, the IP slot is released so a rejected submission + // does not consume the independent IP quota. const ipDenied = await reserveSlot(store, ipKey, MAX_PER_IP_PER_WINDOW, now); if (ipDenied) return ipDenied; const installDenied = await reserveSlot( @@ -845,7 +862,10 @@ async function handleSubmit( MAX_PER_INSTALL_PER_WINDOW, now ); - if (installDenied) return installDenied; + if (installDenied) { + await releaseSlot(store, ipKey, now); + return installDenied; + } const entry: RelayEntry = { id: newEntryId(now),