fix(stream/goals): prevent unbounded SSE polling load and preserve recurring goal history#1882
Open
Ridanshi wants to merge 16 commits into
Open
Conversation
Previously, any 403 or 429 response from GitHub was classified as a
rate-limit error, causing misclassification of permission failures,
invalid credentials, and insufficient-scope errors.
Root cause: GitHubRateLimitError was thrown on HTTP status alone
without inspecting X-RateLimit-Remaining, Retry-After, or response
body content.
Changes:
- Add extractRateLimitInfo() to parse X-RateLimit-Reset,
X-RateLimit-Remaining, and Retry-After headers
- Add isSecondaryRateLimitBody() to detect secondary rate-limit signals
in response body messages
- Add buildGitHubError() that classifies errors correctly:
429 => GitHubRateLimitError
403 + X-RateLimit-Remaining=0 => GitHubRateLimitError (primary)
403 + Retry-After present => GitHubRateLimitError (secondary)
403 + rate-limit body message => GitHubRateLimitError (secondary)
403 (all other cases) => GitHubApiError
- Enrich GitHubRateLimitError with retryAfter field (seconds)
- Add 25 new tests covering all classification paths for both
githubFetch and githubGraphQL
Recurring goals silently erased period data when the weekly/monthly
boundary was crossed. Before this fix, current was zeroed and
period_start advanced with no record of what was achieved.
Root cause: the reset branch in GET /api/goals called
supabase.update({ current: 0, period_start }) with no prior archival.
Changes:
- Upsert into goal_history (goal_id, period_start unique) before every
reset, capturing achieved_value, target, completed, and period dates.
ignoreDuplicates=true makes concurrent resets idempotent.
- Fetch the most recent history row per recurring goal and attach as
last_period in the GET response so the UI needs no extra request.
- GoalTracker shows check N/M or cross N/M last period for recurring goals.
- data-export includes full goal_history records and fixes goals column
list; goal_history is also purged on account deletion.
- Migration creates goal_history table with referential integrity,
cascade-on-delete, and indexes for efficient last-period lookup.
- 17 tests cover completed/incomplete resets, idempotency, ordering,
last_period attachment, weekly, monthly, and multi-goal scenarios.
|
@Ridanshi is attempting to deploy a commit to the PRIYANSHU DOSHI's projects Team on Vercel. A member of the Team first needs to authorize it. |
GSSoC Label Checklist 🏷️@Priyanshu-byte-coder — please apply the appropriate labels before merging: Difficulty (pick one):
Quality (optional):
Validation (required to score):
|
sentCount was incremented unconditionally after each loop iteration, regardless of whether the Resend API call succeeded, failed with a non-2xx response, or threw a network exception. The fetch() return value was completely discarded, making the reported count misleading and hiding delivery failures from operators. Changes to src/app/api/cron/weekly-digest/route.ts: - Capture the Resend fetch() response in a local variable - Check res.ok before incrementing sentCount - Add failedCount to track sends that received non-2xx responses - Wrap each per-user fetch in try/catch so a network error for one recipient does not abort delivery to subsequent recipients - Log failed sends with HTTP status and github_login (not email) - Include failedCount in the JSON response alongside sentCount Changes to test/weekly-digest-cron-auth.test.ts: - Update existing success test to assert failedCount: 0 - Update RESEND_API_KEY-absent test: sentCount is now 0 (not 1) since no delivery was actually attempted - Add 8 new tests covering: successful delivery, Resend 422/429/500 error responses, network exceptions, mixed success/failure batches, per-user failure isolation, and response shape
Closes Priyanshu-byte-coder#1878 ## Vulnerability analysis NextAuth session cookies are SameSite=Lax by default, which blocks naive cross-site form submissions but does not prevent attacks from same-registered-domain subdomains or certain browser edge cases. State-changing API routes authenticated only via cookies were therefore vulnerable to CSRF. ## Root cause No Origin/Referer validation existed for browser-authenticated mutations. API routes accepted POST/PUT/PATCH/DELETE requests from any origin as long as a valid session cookie was present. ## Implementation **Layer 1 — Origin validation (src/lib/security/csrf.ts)** - `buildAllowedOrigins()` reads NEXTAUTH_URL and ALLOWED_ORIGINS env vars to build the permitted-origin set. - `getRequestOrigin()` extracts the Origin header, falling back to the Referer origin when Origin is absent. - `isOriginAllowed()` performs exact, case-sensitive comparison. - `isCsrfExemptPath()` marks paths that authenticate out-of-band. **Layer 2 — Middleware enforcement (src/middleware.ts)** - Applies to all authenticated (token present) non-safe-method requests under /api/ that are not on the exempt list. - Requests carrying Authorization: Bearer are skipped — they are API clients, not browser sessions, and are not susceptible to CSRF. - Cross-origin requests are rejected with 403. **Exempt paths (authenticate via their own mechanism)** - /api/webhooks/github — x-hub-signature-256 HMAC - /api/webhooks/dispatch — WEBHOOK_DISPATCH_SECRET bearer - /api/cron/ — CRON_SECRET bearer - /api/auth/ — NextAuth internals **Allowed-origin configuration (.env.example)** - NEXTAUTH_URL is always included automatically. - ALLOWED_ORIGINS accepts additional comma-separated origins for staging/preview deployments. ## Protected routes (state-changing, session-authenticated) POST: /api/goals, /api/rooms, /api/notifications, /api/webhooks/custom, /api/daily-focus (via PUT) PUT: /api/daily-focus PATCH: /api/goals/[id], /api/user/settings, /api/notifications/[id] DELETE: /api/goals/[id], /api/streak/freeze, /api/daily-focus, /api/notifications/[id] ## Additional fixes - goals/[id] PATCH: require current as a non-negative integer (reject missing field and floats) - local-coding/stats: return 500 on DB error instead of empty stats - user/github-accounts: return 500 on DB error instead of empty list - discord.test.ts: update test URLs to valid Discord webhook format so they pass the URL validation added to discord.ts ## Tests added test/csrf.test.ts — 42 tests covering: - SAFE_METHODS set membership - isCsrfExemptPath for all exempt and non-exempt paths - buildAllowedOrigins with NEXTAUTH_URL, ALLOWED_ORIGINS, both, neither - getRequestOrigin: Origin header, Referer fallback, both present, malformed Referer, whitespace trimming - isOriginAllowed: known/unknown/scheme/port/case/empty - Middleware integration: valid origin, invalid origin, missing headers, unauthenticated bypass, safe methods, bearer bypass, webhook exemption, cron exemption, Referer fallback test/daily-focus.test.ts — 19 tests for the new /api/daily-focus route ## Verification pnpm test — 1169/1169 passed pnpm lint — no errors (warnings pre-existing) pnpm typecheck — clean
Closes Priyanshu-byte-coder#1878 Include remaining test and component files for the security branch.
Closes Priyanshu-byte-coder#1878 Update github auth error response code for consistency with client handling.
Closes Priyanshu-byte-coder#1878 Update component error handling and auth error test assertions.
Closes Priyanshu-byte-coder#1878 Update date utility and tests.
Closes Priyanshu-byte-coder#1878 Commit the remaining SSE stream connection improvements and date-utils re-export that were staged alongside the CSRF implementation. - src/app/api/stream/route.ts: add heartbeat keepalive, idle-timeout recycling, and max-age ceiling so zombie connections do not hold slots indefinitely; extract HEARTBEAT_INTERVAL_MS, IDLE_TIMEOUT_MS, MAX_AGE_MS as named exports for test coverage - src/lib/date-utils.ts: re-export date helpers from dateUtils.ts under the hyphenated alias expected by test/date-utils.test.ts
Extends the existing test file from 10 basic cases to 47 behavioral tests, grouped into three describe blocks. Functions covered: - privateCacheHeaders(maxAgeSeconds?, swrSeconds?) - publicCacheHeaders(maxAgeSeconds?, swrSeconds?) New coverage added: SWR default formula regression - Verifies stale-while-revalidate defaults to exactly 2× maxAgeSeconds for both functions, using non-round inputs (100, 150, 3600) that would expose an accidental multiplier change Directive presence / absence guards - privateCacheHeaders: always contains `private`, never `public` or `s-maxage` - publicCacheHeaders: always contains `public` and `s-maxage`, never `private` or standalone `max-age` Exact format and ordering - Regex asserts on full directive pattern (e.g. /^private, max-age=\d+, stale-while-revalidate=\d+$/) - Verifies `=` assignment, comma-space separators, and the full `stale-while-revalidate` keyword (not an abbreviation) - Ordering test: splits on `", "` and checks position of each part Return object shape - Both functions return an object with exactly one key: `Cache-Control` - Value is a string Large / boundary values - 86400 s (24 h) and explicit swrSeconds=0 override Headers constructor compatibility - Confirms the returned HeadersInit can be passed to `new Headers()` and yields the correct `Cache-Control` value via `.get()` Cross-function regression describe block - Two functions produce distinct values for the same input - max-age vs s-maxage mutual exclusion - private vs public mutual exclusion - Both expose stale-while-revalidate - Directive count is exactly 3 for both functions
Closes Priyanshu-byte-coder#1879 Root cause: src/lib/date-utils.ts (barrel re-export) was committed without corresponding regression tests, so a future rename of dateDiffDays or dateDiff in the barrel could silently break callers without any test catching it. Fix: extend test/dateDiffDays.test.ts with a dedicated describe block that imports both dateDiffDays and dateDiff from the @/lib/date-utils barrel and asserts: - both names are exported as functions - dateDiff and dateDiffDays return identical results - correct positive, zero, and negative day differences Tests added: - "exports dateDiffDays" — verifies named export exists - "exports dateDiff alias" — verifies alias export exists - "dateDiff and dateDiffDays produce identical results" - "barrel dateDiffDays computes correct day difference" - "barrel dateDiff computes correct day difference" - "barrel export returns 0 for same date" - "barrel export returns negative for reversed dates" Verification: next lint → warnings only (pre-existing), no errors tsc --noEmit → clean vitest run → 1255 tests across 86 files, all passed
Closes Priyanshu-byte-coder#1848 Root cause: Today's Focus goal was stored only in localStorage, making it inaccessible across devices, browsers, and incognito sessions. Any browser storage clear or device switch would lose the user's focus entry. Implementation summary: Database layer - Added migration 20260602000000_add_daily_focus.sql creating the daily_focus table with columns (id, user_id, focus_date, goal_text, created_at, updated_at), a unique (user_id, focus_date) constraint, composite index, FK to users with ON DELETE CASCADE, and RLS policies scoped to auth.uid()::text so each user can only access their own rows. API layer - Implemented GET, PUT (upsert), and DELETE handlers under src/app/api/daily-focus/route.ts following the same authentication, validation, and error-handling patterns used throughout the codebase. Input is validated and sanitised via validateTextInput(); date params are validated with a strict regex before use. Frontend - Refactored src/components/TodayFocusHero.tsx to treat the server as the source of truth. On mount the component fetches today's record; saves and deletes are sent to the API with optimistic updates and rollback on failure. If the server is unreachable the component falls back to localStorage and shows a sync warning. Unauthenticated users retain the localStorage-only experience unchanged. Migration path - On first load, if no server record exists but a localStorage entry does, the component silently migrates the value via PUT and removes the localStorage key only after a confirmed 200 response. No data is lost during the transition. Export support - src/app/api/user/data-export/route.ts now includes a dailyFocus section (last 365 records) in GET exports and explicitly deletes daily_focus rows during account deletion. Additional fixes included in this branch - Extended GitHub auth-error propagation to ci, inactive-repos, pinned-repos, productive-hours, and weekly-summary metric routes, and to the ci-analytics helper, using the shared GitHubAuthError / githubAuthErrorResponse pattern already established earlier on this branch. - Updated DiscussionsWidget and PRReviewTrendChart to handle token_expired responses from the API. Tests added - test/daily-focus.test.ts: 19 tests covering GET (authenticated, unauthenticated, missing record, existing record, DB error), PUT (create, upsert, validation errors, invalid JSON, DB error), and DELETE (success, idempotent delete, defaults, DB error). Verification commands next lint -- warnings only (pre-existing), no errors tsc --noEmit -- clean vitest run -- all tests pass
…ric routes Extends the GitHubAuthError / githubAuthErrorResponse pattern to coding-activity-insights, pr-breakdown, PRReviewTrendChart, and WeeklySummaryCard — completing the token-revocation error propagation pass started in earlier commits on this branch.
The SSE stream endpoint previously polled two database tables every 2 s per open connection with no cap on concurrent connections and no cleanup of zombie connections, creating O(N*queries) database load. The fix (already on branch in c3f4c6b) addressed this with: - Per-user connection cap of 4 (HTTP 429 with Retry-After when exceeded) - Poll interval increased from 2 s to 15 s (7.5x fewer queries) - Events only emitted when data actually changes (no client spam) - Heartbeat keepalive every 30 s to detect dead TCP streams early - Idle timeout: connections with no real data for 5 min are recycled - Max-age ceiling: connections forcibly recycled after 30 min - Single closeStream() path with a cleanedUp guard prevents slot double-decrement when abort, idle-timeout, and max-age race This commit adds dedicated test coverage for the lifetime-control mechanisms that were previously untested: - HEARTBEAT_INTERVAL_MS / IDLE_TIMEOUT_MS / MAX_AGE_MS constant values - Idle timeout releases the connection slot (fake-timer test) - Max-age releases the connection slot (fake-timer test) All 20 SSE stream tests pass.
Complete the remaining auth-error propagation gaps so all metrics routes and dashboard widgets behave consistently when a GitHub OAuth token is revoked. Routes: add TokenRevoked early-exit + GitHubAuthError catch to achievements, coding-activity-insights, contributions/daily, contributions/hourly, and repo-explorer. InactiveRepositoriesCard and ProductiveHoursWidget detect token_expired and render a reconnect prompt instead of a generic error state. Tests: new github-auth-metrics.test.ts covers all five newly-fixed routes for the TokenRevoked session path, GitHub 401 propagation, and non-auth failures remaining as 502. token-expired.test.ts extended with matching coverage for discussions, pinned-repos, pr-breakdown, and weekly-summary routes.
8377f98 to
d973cda
Compare
Owner
|
CI is failing because the Please run npm install
git add package-lock.json
git commit -m "chore: regenerate lockfile"
git push |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1687
Closes #1847
Issue #1687 — SSE stream unbounded polling load
Root cause
/api/streampolled two Supabase tables every 2 seconds per open connection with no concurrent connection limit and no mechanism to close zombie connections. Under concurrent usage this creates O(connections × queries/second) database load.Scalability analysis
Connection management changes
activeStreamConnectionsmap enforcesMAX_CONNECTIONS_PER_USER = 4. Excess connections return429withRetry-After: 30.releaseSlot()with acleanedUpguard prevents double-decrement when abort, idle-timeout, and max-age fire concurrently.closeStream()guards withisClosed, clears all four timers atomically, always callsreleaseSlot().IDLE_TIMEOUT_MS = 5 min): frees slots held by zombie connections.MAX_AGE_MS = 30 min): forcibly recycles long-lived connections.EventSourcereconnects within seconds.HEARTBEAT_INTERVAL_MS = 30 s):: heartbeat\n\ncomment keeps TCP alive; a failed enqueue triggerscloseStream()immediately.Files changed
src/app/api/stream/route.ts— heartbeat, idle-timeout, max-age, single close path, named exportstest/sse-stream-route.test.ts— 5 new tests (20 total): constant values + fake-timer idle/max-age lifecycleIssue #1847 — Recurring goals silently erase completion history on reset
Root cause
When a recurring goal's period boundary was crossed,
GET /api/goalscalledsupabase.update({ current: 0, period_start })directly with no prior archival step, permanently discarding the achieved value, target, and completion status for the expired period.Implementation
goal_history(unique ongoal_id, period_start) before every reset.ignoreDuplicates: truemakes concurrent resets idempotent.last_periodin the GET response.GoalTrackerrenders✓ N / M last periodor✗ N / M last periodfor recurring goals.data-exportincludes fullgoal_historyrecords;goal_historyis purged on account deletion.Schema changes
New table
goal_history:id,goal_id(FK→goals, CASCADE),user_id(FK→users, CASCADE),period_start,period_end,target,achieved_value,completed,created_at. Unique constraint on(goal_id, period_start). Two indexes for efficient lookups.Files changed
src/app/api/goals/route.ts— archive upsert before reset, fetch/attachlast_periodsrc/components/GoalTracker.tsx— display ✓/✗ last-period summarysrc/app/api/user/data-export/route.ts— includegoalHistoryin exports + purge on deletesupabase/migrations/20260602000000_add_goal_history.sql— new table migrationtest/goal-history.test.ts— 17 testsVerification