-
Notifications
You must be signed in to change notification settings - Fork 12
Stop unknown-device sync rejection storms and Windows EBUSY flakes #1120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
148 changes: 148 additions & 0 deletions
148
apps/ade-cli/src/services/sync/pairedDeviceRejectionLimiter.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { | ||
| applyPairedDeviceRejectionThrottle, | ||
| createPairedDeviceRejectionLimiter, | ||
| PAIRED_DEVICE_REJECTION_BASE_DELAY_MS, | ||
| PAIRED_DEVICE_REJECTION_DELAY_AFTER, | ||
| PAIRED_DEVICE_REJECTION_LOG_EVERY, | ||
| PAIRED_DEVICE_REJECTION_MAX_DELAY_MS, | ||
| PAIRED_DEVICE_REJECTION_WINDOW_MS, | ||
| } from "./pairedDeviceRejectionLimiter"; | ||
|
|
||
| function createClock(startMs = 1_000_000) { | ||
| let nowMs = startMs; | ||
| return { | ||
| now: () => nowMs, | ||
| advance(ms: number) { | ||
| nowMs += ms; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| describe("paired device rejection limiter", () => { | ||
| it("logs the first rejection and then every Nth, independent of rejection reason", () => { | ||
| const clock = createClock(); | ||
| const limiter = createPairedDeviceRejectionLimiter({ now: clock.now }); | ||
| const logged: number[] = []; | ||
|
|
||
| for (let i = 1; i <= PAIRED_DEVICE_REJECTION_LOG_EVERY * 2; i += 1) { | ||
| const action = limiter.record("phone-a"); | ||
| if (action.shouldLog) logged.push(i); | ||
| expect(action.countInWindow).toBe(i); | ||
| } | ||
|
|
||
| expect(logged).toEqual([ | ||
| 1, | ||
| PAIRED_DEVICE_REJECTION_LOG_EVERY, | ||
| PAIRED_DEVICE_REJECTION_LOG_EVERY * 2, | ||
| ]); | ||
| }); | ||
|
|
||
| it("does not start delaying until after the burst threshold, then caps", () => { | ||
| const clock = createClock(); | ||
| const limiter = createPairedDeviceRejectionLimiter({ now: clock.now }); | ||
|
|
||
| for (let i = 1; i <= PAIRED_DEVICE_REJECTION_DELAY_AFTER; i += 1) { | ||
| expect(limiter.record("phone-a").delayMs).toBe(0); | ||
| } | ||
|
|
||
| const firstDelayed = limiter.record("phone-a"); | ||
| expect(firstDelayed.delayMs).toBe(PAIRED_DEVICE_REJECTION_BASE_DELAY_MS); | ||
|
|
||
| const secondDelayed = limiter.record("phone-a"); | ||
| expect(secondDelayed.delayMs).toBe(PAIRED_DEVICE_REJECTION_BASE_DELAY_MS * 2); | ||
|
|
||
| for (let i = 0; i < 20; i += 1) { | ||
| limiter.record("phone-a"); | ||
| } | ||
| expect(limiter.record("phone-a").delayMs).toBe(PAIRED_DEVICE_REJECTION_MAX_DELAY_MS); | ||
| }); | ||
|
|
||
| it("isolates devices so one looping phone cannot delay another", () => { | ||
| const clock = createClock(); | ||
| const limiter = createPairedDeviceRejectionLimiter({ now: clock.now }); | ||
|
|
||
| for (let i = 0; i < 10; i += 1) { | ||
| limiter.record("phone-looping"); | ||
| } | ||
|
|
||
| const other = limiter.record("phone-ok"); | ||
| expect(other.countInWindow).toBe(1); | ||
| expect(other.shouldLog).toBe(true); | ||
| expect(other.delayMs).toBe(0); | ||
| }); | ||
|
|
||
| it("forgets hits that fall outside the window", () => { | ||
| const clock = createClock(); | ||
| const limiter = createPairedDeviceRejectionLimiter({ now: clock.now }); | ||
|
|
||
| for (let i = 0; i < 10; i += 1) { | ||
| limiter.record("phone-a"); | ||
| } | ||
| expect(limiter.record("phone-a").countInWindow).toBe(11); | ||
|
|
||
| clock.advance(PAIRED_DEVICE_REJECTION_WINDOW_MS + 1); | ||
| const other = limiter.record("phone-b"); | ||
| expect(other.countInWindow).toBe(1); | ||
| const afterWindow = limiter.record("phone-a"); | ||
| expect(afterWindow.countInWindow).toBe(1); | ||
| expect(afterWindow.shouldLog).toBe(true); | ||
| expect(afterWindow.delayMs).toBe(0); | ||
| }); | ||
|
|
||
| it("does not key empty device ids into a shared bucket", () => { | ||
| const limiter = createPairedDeviceRejectionLimiter(); | ||
| const first = limiter.record(" "); | ||
| const second = limiter.record(""); | ||
| expect(first.delayMs).toBe(0); | ||
| expect(second.delayMs).toBe(0); | ||
| expect(first.shouldLog).toBe(true); | ||
| expect(second.shouldLog).toBe(true); | ||
| }); | ||
|
|
||
| it("skips sleeping when delay is zero", async () => { | ||
| const started = Date.now(); | ||
| await applyPairedDeviceRejectionThrottle({ | ||
| countInWindow: 1, | ||
| shouldLog: true, | ||
| delayMs: 0, | ||
| }); | ||
| expect(Date.now() - started).toBeLessThan(50); | ||
| }); | ||
|
|
||
| it("keeps a large same-device burst as a bounded count, not a growing timestamp list", () => { | ||
| const clock = createClock(); | ||
| const limiter = createPairedDeviceRejectionLimiter({ now: clock.now }); | ||
| const burst = 10_000; | ||
|
|
||
| let last = limiter.record("phone-burst"); | ||
| for (let i = 1; i < burst; i += 1) { | ||
| last = limiter.record("phone-burst"); | ||
| } | ||
|
|
||
| expect(last.countInWindow).toBe(burst); | ||
| expect(last.delayMs).toBe(PAIRED_DEVICE_REJECTION_MAX_DELAY_MS); | ||
| const other = limiter.record("phone-ok"); | ||
| expect(other.countInWindow).toBe(1); | ||
| expect(other.delayMs).toBe(0); | ||
| }); | ||
|
|
||
| it("keeps backoff across the 60s boundary for a sustained retry loop", () => { | ||
| const clock = createClock(); | ||
| const limiter = createPairedDeviceRejectionLimiter({ now: clock.now }); | ||
|
|
||
| for (let i = 0; i < 8; i += 1) { | ||
| limiter.record("phone-loop"); | ||
| clock.advance(7_500); | ||
| } | ||
|
|
||
| const acrossBoundary = limiter.record("phone-loop"); | ||
| expect(acrossBoundary.countInWindow).toBeGreaterThan(PAIRED_DEVICE_REJECTION_DELAY_AFTER); | ||
| expect(acrossBoundary.delayMs).toBeGreaterThan(0); | ||
|
|
||
| clock.advance(PAIRED_DEVICE_REJECTION_WINDOW_MS + 1); | ||
| const afterQuiet = limiter.record("phone-loop"); | ||
| expect(afterQuiet.countInWindow).toBe(1); | ||
| expect(afterQuiet.delayMs).toBe(0); | ||
| }); | ||
| }); |
111 changes: 111 additions & 0 deletions
111
apps/ade-cli/src/services/sync/pairedDeviceRejectionLimiter.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| /** | ||
| * Host-side throttle for repeated paired-hello rejections from the same | ||
| * device id. | ||
| * | ||
| * A phone that kept a pairing secret after the host forgot the record will | ||
| * retry forever (LAN + Tailscale + Relay racing). Each hello is a full reject | ||
| * + close + warn log; left alone that is thousands of lines a day. | ||
| * | ||
| * The wire body stays identical for `unknown_device` and `secret_mismatch`. | ||
| * This limiter never sees the reason: applying a different delay or log | ||
| * cadence per reason would let an unauthenticated caller tell whether the | ||
| * device id exists. | ||
| */ | ||
|
|
||
| export const PAIRED_DEVICE_REJECTION_WINDOW_MS = 60_000; | ||
| export const PAIRED_DEVICE_REJECTION_LOG_EVERY = 8; | ||
| export const PAIRED_DEVICE_REJECTION_DELAY_AFTER = 3; | ||
| export const PAIRED_DEVICE_REJECTION_BASE_DELAY_MS = 250; | ||
| export const PAIRED_DEVICE_REJECTION_MAX_DELAY_MS = 8_000; | ||
| const PAIRED_DEVICE_REJECTION_MAX_TRACKED = 512; | ||
| /** Rolling buckets keep a sliding 60s count without a timestamp per hit. */ | ||
| const PAIRED_DEVICE_REJECTION_BUCKET_MS = 5_000; | ||
|
|
||
| export type PairedDeviceRejectionAction = { | ||
| countInWindow: number; | ||
| shouldLog: boolean; | ||
| delayMs: number; | ||
| }; | ||
|
|
||
| export type PairedDeviceRejectionLimiter = { | ||
| record(deviceId: string): PairedDeviceRejectionAction; | ||
| }; | ||
|
|
||
| type PairedDeviceRejectionLimiterOptions = { | ||
| now?: () => number; | ||
| windowMs?: number; | ||
| logEvery?: number; | ||
| delayAfter?: number; | ||
| baseDelayMs?: number; | ||
| maxDelayMs?: number; | ||
| }; | ||
|
|
||
| export function createPairedDeviceRejectionLimiter( | ||
| options: PairedDeviceRejectionLimiterOptions = {}, | ||
| ): PairedDeviceRejectionLimiter { | ||
| const now = options.now ?? Date.now; | ||
| const windowMs = options.windowMs ?? PAIRED_DEVICE_REJECTION_WINDOW_MS; | ||
| const logEvery = options.logEvery ?? PAIRED_DEVICE_REJECTION_LOG_EVERY; | ||
| const delayAfter = options.delayAfter ?? PAIRED_DEVICE_REJECTION_DELAY_AFTER; | ||
| const baseDelayMs = options.baseDelayMs ?? PAIRED_DEVICE_REJECTION_BASE_DELAY_MS; | ||
| const maxDelayMs = options.maxDelayMs ?? PAIRED_DEVICE_REJECTION_MAX_DELAY_MS; | ||
| const hits = new Map<string, Map<number, number>>(); | ||
|
|
||
| const pruneSlot = (slot: Map<number, number>, nowMs: number): number => { | ||
| const cutoff = nowMs - windowMs; | ||
| let count = 0; | ||
| for (const [bucketStartMs, bucketCount] of slot) { | ||
| if (bucketStartMs <= cutoff) slot.delete(bucketStartMs); | ||
| else count += bucketCount; | ||
| } | ||
| return count; | ||
| }; | ||
|
|
||
| const pruneExpired = (nowMs: number): void => { | ||
| for (const [id, slot] of hits) { | ||
| if (pruneSlot(slot, nowMs) === 0) hits.delete(id); | ||
| } | ||
| }; | ||
|
|
||
| return { | ||
| record(deviceId: string): PairedDeviceRejectionAction { | ||
| const key = deviceId.trim(); | ||
| if (!key) { | ||
| return { countInWindow: 1, shouldLog: true, delayMs: 0 }; | ||
| } | ||
| const nowMs = now(); | ||
| pruneExpired(nowMs); | ||
| if (hits.size >= PAIRED_DEVICE_REJECTION_MAX_TRACKED && !hits.has(key)) { | ||
| const oldest = hits.keys().next().value; | ||
| if (oldest) hits.delete(oldest); | ||
| } | ||
| let slot = hits.get(key); | ||
| if (!slot) { | ||
| slot = new Map(); | ||
| hits.set(key, slot); | ||
| } | ||
| const bucketStartMs = | ||
| Math.floor(nowMs / PAIRED_DEVICE_REJECTION_BUCKET_MS) * | ||
| PAIRED_DEVICE_REJECTION_BUCKET_MS; | ||
| slot.set(bucketStartMs, (slot.get(bucketStartMs) ?? 0) + 1); | ||
| const countInWindow = pruneSlot(slot, nowMs); | ||
| if (countInWindow === 0) hits.delete(key); | ||
| const shouldLog = countInWindow === 1 || countInWindow % logEvery === 0; | ||
| let delayMs = 0; | ||
| if (countInWindow > delayAfter) { | ||
| const exp = Math.min(countInWindow - delayAfter - 1, 16); | ||
| delayMs = Math.min(maxDelayMs, baseDelayMs * (2 ** exp)); | ||
| } | ||
| return { countInWindow, shouldLog, delayMs }; | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| export async function applyPairedDeviceRejectionThrottle( | ||
| action: PairedDeviceRejectionAction, | ||
| ): Promise<void> { | ||
| if (action.delayMs <= 0) return; | ||
| await new Promise<void>((resolve) => { | ||
| setTimeout(resolve, action.delayMs); | ||
| }); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep each rejection active for the full configured window.
pruneSlotexpires a bucket from its start time, not from each rejection time. A rejection at4,999 msis removed at60,000 ms, although it is only55,001 msold. This can resetcountInWindow, log cadence, anddelayMsalmost five seconds early.Use an exact bounded representation, or explicitly define and test a bucket-aligned window policy. Add a named regression test with four hits late in one bucket and a fifth hit after the bucket-start boundary. The fifth action must retain all five hits and apply backoff.
Also applies to: 87-91
🤖 Prompt for AI Agents
Source: Coding guidelines