🐛 fix(nps-relay): release the per-IP rate slot when the install limit refuses a submission

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).

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<unknown> {
+        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 }),

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<Response | null> {
+  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),