Repository navigation
feat(cdp): reserve part of the ses bucket for a higher-priority lane - #127613
meikelmosby wants to merge 2 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Risk: Low · 1 low This change adds an optional Sentinel reviewed |
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/warehouse_sources/sources/folk/folk.py:144 |
products/warehouse_sources/sources/select_star/select_star.py:156 |
18 | 90 |
products/warehouse_sources/sources/ruddr/ruddr.py:127 |
products/warehouse_sources/sources/tempo/tempo.py:136 |
20 | 72 |
⚠️ Duplication (TypeScript) — 11 new duplicated blocks (worst 195 tokens)
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
packages/agent/packages/agent/src/adapters/claude/claude-agent.slash-command.test.ts:49 |
packages/agent/packages/agent/src/adapters/claude/claude-agent.task-notification.test.ts:53 |
38 | 195 |
products/replay_vision/frontend/components/QuillScanList.tsx:31 |
products/replay_vision/frontend/components/ScanList.tsx:30 |
34 | 191 |
products/replay_vision/frontend/components/ChapterFilmstrip.tsx:26 |
products/replay_vision/frontend/components/QuillChapterFilmstrip.tsx:27 |
31 | 174 |
products/replay_vision/frontend/components/QuillScanList.tsx:203 |
products/replay_vision/frontend/components/ScanList.tsx:179 |
15 | 172 |
frontend/src/scenes/session-recordings/player/sidebar/PlayerSidebarOverviewDeviceTab.tsx:11 |
frontend/src/scenes/session-recordings/player/watch/QuillOverviewDeviceCard.tsx:11 |
14 | 144 |
products/replay_vision/frontend/components/QuillScanList.tsx:87 |
products/replay_vision/frontend/components/ScanList.tsx:81 |
27 | 138 |
frontend/src/scenes/session-recordings/player/sidebar/PlayerSidebarOverviewSessionTab.tsx:14 |
frontend/src/scenes/session-recordings/player/watch/QuillOverviewSessionCard.tsx:14 |
12 | 102 |
products/replay_vision/frontend/components/QuillScanList.tsx:257 |
products/replay_vision/frontend/components/ScanList.tsx:231 |
24 | 101 |
products/replay_vision/frontend/components/ObservationProgressBar.tsx:12 |
products/replay_vision/frontend/components/QuillObservationProgressBar.tsx:10 |
19 | 87 |
frontend/src/scenes/session-recordings/player/sidebar/PlayerSidebarOverviewPersonTab.tsx:15 |
frontend/src/scenes/session-recordings/player/watch/QuillOverviewPersonCard.tsx:16 |
14 | 84 |
products/replay_vision/frontend/components/ObservationProgressBar.tsx:45 |
products/replay_vision/frontend/components/QuillObservationProgressBar.tsx:43 |
13 | 70 |
🚨 Comment density — 14% of added code lines are comments (13 of 92)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
nodejs/src/cdp/services/rate-limiter/rate-limiter.service.ts |
7 | 26 |
nodejs/src/cdp/services/job-queue/job-queue-rate-limited-postgres-v2.ts |
3 | 11 |
nodejs/src/cdp/services/rate-limiter/rate-limiter.service.test.ts |
3 | 53 |
This check does not block merging. It updates on every push and clears when the share drops.
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 1f61448: 1 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
| slotAt = tonumber(rawResv) + (requested / refillPerSecond) * 1000 | ||
| else | ||
| local deficit = requested - available | ||
| local deficit = requested - claimable |
There was a problem hiding this comment.
P2 · Apply the floor-aware shortfall when a reservation cursor exists
With an empty bucket refilling at 10/s, an unfloored one-token reservation sets resv 100ms ahead. A subsequent five-token claimOrReserve with reserveFloor: 20 takes the existing-cursor branch and reserves 600ms, when only six tokens exist, guaranteeing another denial even without competing claims. Bound both branches by the floor-aware refill deadline.
| -- Left negative on purpose when the pool is below the floor: the grant clamps | ||
| -- at zero just below, and the reservation needs the true shortfall to place the | ||
| -- slot after the floor has refilled, not just after this caller's own tokens. | ||
| local claimable = available - reserveFloor |
There was a problem hiding this comment.
Unvalidated reserveFloor lets a negative value overspend the SES bucket
The new reserveFloor is never validated or clamped, so a negative value inverts its meaning: claimable = available - reserveFloor becomes available + |floor|, and the grant is debited from the true pool (tokensAfter = available - granted, rate-limiter.service.ts:116), driving the pool negative. The module's own contract — concurrent callers "never overshoot the bucket capacity" (rate-limiter.service.ts:16-17) — is broken by a single caller, and the floored lane then sends above the configured SES rate until the pool refills, risking SES throttling and reputation damage. A floor greater than capacity is the mirror bug: claimable is always negative, so that lane is granted 0 forever and its emails never send.
How:
- An operator sets
CDP_SES_RATE_LIMIT_TRANSACTIONAL_RESERVE=-10(sign typo, or a mistaken belief that negative means a boost). overrideWithEnv(common/config/config.ts:48-49)parseInts it to -10 with no range check, andevalClaimforwards it verbatim (String(req.reserveFloor ?? 0), rate-limiter.service.ts:360).- With 5 tokens available and 15 rows ready,
claimableis 15, so the caller is granted 15 and the pool becomes -10 — 10 sends above budget.
Fix: Clamp the floor at the entry point — treat any value outside [0, capacity] as 0 (or fail startup) in evalClaim/claimBatchLimit, since the config loader does not validate it.
React with 👍 if useful or 👎 if not
A token-bucket claim can now carry a reserve floor: tokens the caller may not spend, so a caller that passes no floor always finds that much waiting. One bucket still refills at one rate, so the total send rate is unchanged. Inert on deploy — CDP_SES_RATE_LIMIT_TRANSACTIONAL_RESERVE defaults to 0 and nothing passes a floor yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1f61448 to
2196030
Compare
A floored caller denied behind another caller's slot took the next slot after it, even when its own tokens could not refill above the floor by then, which booked it a slot that was certain to be denied again. The slot is now never earlier than the floor-aware refill time. A rate-limited queue refuses to start with a negative floor, which would let its lane overspend the bucket, or with one at capacity, which would stop the lane from sending. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Closing while the transactional email work is replanned. The branch stays for reference. |
Problem
@posthog/ses/globalas peers, so a pod holding only marketing rows could drain it before a password reset claims.Changes
CDP_SES_RATE_LIMIT_TRANSACTIONAL_RESERVEdefaults to 0 and no caller passes a floor.How did you test this code?
Test rationale: Four cases on
RateLimiterService, which had no floor coverage: a floored caller draining the reserve, a double debit, and a denied caller waking before the floor refills, alone or behind an unfloored caller's slot.Ran locally: the rate-limiter and job-queue Jest suites.
Release status
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Opus 5; restacked and finished by Claude Code, Opus 5.5
Skills invoked:
/writing-tests,/writing-pr-descriptions,/stacking-prs.More context for AI agents
The floor started clamped at zero. The third test showed that a bucket below the floor then reported a shortfall of only the tokens requested, so a denied caller booked a slot before the floor could refill. The value stays negative now; the grant clamps separately.
Layer 2 of 4, on top of the table migration in #127619.