fix(api): apply eligible banked freeze during streak recalculation to close the hourly-job race#383
Conversation
… close the hourly-job race RecalculateAsync only treated a scheduled miss as covered when a StreakFreeze row already existed (freezeDateSet); it never consumed a banked freeze itself. So when a user missed their local "yesterday" but held a bankable freeze, the recurring-schedule walk (ComputeStreakAsOf) reset CurrentStreak to 0 and cleared LastActiveDate. That made the user fail the hourly StreakFreezeAutoActivationService candidate filter (CurrentStreak > 0 && LastActiveDate < utcYesterday), so the freeze that should have bridged the gap was dropped. Whichever path ran first with stale state won, and a real user's streak was lost. Recalculation now deterministically bridges the single most-recent scheduled miss (userToday - 1) with one banked freeze, mirroring the hourly job: it spends a freeze only when covering the day actually raises the computed streak (so an over-large gap is left to break), the user is freeze-eligible (Pro or the gamification-free-tier flag), and the per-month cap is not yet hit. Consumption persists a StreakFreeze row, which is the idempotency + race-safety anchor: a later recalc — or the hourly job — sees the frozen date in freezeDateSet and neither re-consumes a freeze nor inflates the streak, so the invariant holds regardless of which path runs first. Activation is logged at Information as a lifecycle event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #383 — fix(api): apply eligible banked freeze during streak recalculation to close the hourly-job race
Recommendation: Request changes
Summary
TryBridgeRecentGapWithBankedFreezeAsync is added to UserStreakService.RecalculateAsync so an on-demand streak recalculation can consume a banked streak freeze for the user's missed "yesterday," mirroring what the hourly StreakFreezeAutoActivationService already does. The gating logic (Pro/feature-flag gate, monthly cap, "only bridge if it actually raises the streak") is correct and well covered by 4 new unit tests. However, the fix introduces an unguarded DB failure mode on the exact race window it claims to close.
Findings
[CRITICAL] Concurrent freeze-bridge insert can throw an unhandled unique-constraint DbUpdateException instead of degrading gracefully
- Location:
src/Orbit.Infrastructure/Services/UserStreakService.cs:157(TryBridgeRecentGapWithBankedFreezeAsync→streakFreezeRepository.AddAsync), surfacing at everyRecalculateAsynccaller — e.g.src/Orbit.Application/Gamification/Queries/GetStreakInfoQuery.cs:46-49, which callsunitOfWork.SaveChangesAsyncimmediately after with no try/catch at all. - Issue:
AddAsync(confirmed inGenericRepository.cs:139-141) only stages the entity; the actual INSERT + unique-constraint check happens at the caller's laterSaveChangesAsync. TheStreakFreezetable has a unique index on(UserId, UsedOnDate)(OrbitDbContext.ConfigureStreakFreezeEntity, line 321). Both the new bridging path and the pre-existing hourlyStreakFreezeAutoActivationServiceindependently compute the identicalmissedDate = userToday.AddDays(-1)for the same eligible user (confirmed atStreakFreezeAutoActivationService.cs:126) and can target the exact same(UserId, UsedOnDate)row concurrently. Nothing in the new code or its 9 call sites catches the resultingDbUpdateException. - Risk: this is precisely the race the PR's description says it's closing (on-demand recalc racing the hourly job on the same missed day). The loser's
SaveChangesAsyncthrows a raw Postgres 23505 unique-violation wrapped inDbUpdateException. Confirmed this propagates uncaught:ConcurrencyRetryBehavior(src/Orbit.Application/Behaviors/ConcurrencyRetryBehavior.cs:33) only catchesDbUpdateConcurrencyException, notDbUpdateException; it also only applies to requests markedIConcurrencyRetryable, whichGetStreakInfoQueryis not. The exception reachesUnhandledExceptionHandler(src/Orbit.Api/Middleware/UnhandledExceptionHandler.cs), producing an HTTP 500 for the end user (viewing streak info, logging/unlogging/deleting a habit, chatting) plus a spurious Sentry issue — on the exact live-account scenario cited in the PR body. - Existing pattern not reused:
DbUniqueViolation.IsUniqueViolationexists in bothsrc/Orbit.Application/Common/DbUniqueViolation.csandsrc/Orbit.Infrastructure/Services/DbUniqueViolation.csspecifically to detect this SQLSTATE 23505 case.StreakFreezeAutoActivationService.TrySaveBatchAsync/TrySaveUserFreezeAsync(StreakFreezeAutoActivationService.cs:156-171,253-273) already catchDbUpdateException ex when (DbUniqueViolation.IsUniqueViolation(ex))and degrade to a no-op skip for exactly this conflict. The new bridging path was not given the matching guard. - Fix: wrap the save that persists the bridged freeze in a catch for
DbUpdateExceptionwhereDbUniqueViolation.IsUniqueViolation(ex), discarding the staged freeze and falling through to the no-bridge outcome on conflict — mirroringStreakFreezeAutoActivationService's existing pattern. Add a test exercising that conflict path. - Reference: CLAUDE.md "No workarounds / root-cause every bug"; existing pattern in
DbUniqueViolation.cs+StreakFreezeAutoActivationService.cs.
Verified directly by reading GenericRepository.AddAsync, OrbitDbContext.ConfigureStreakFreezeEntity, ConcurrencyRetryBehavior, GetStreakInfoQuery, and UnhandledExceptionHandler — no catch exists anywhere in the chain for this new insert path.
No High, Medium, or Low/Info findings survived the signal gate.
Security
No authz/tenant-isolation, data-exposure, or injection issues. New code scopes strictly to the caller's userId, logs only non-sensitive UserId/FrozenDate.
Cross-repo (not verifiable in CI)
Diff touches no DTO, Controller route, or contract-shaped surface — orbit-ui-mobile is not checked out in this job, but this dimension is moot for this diff regardless.
What's good
- Gating logic matches the hourly job's eligibility rules field-for-field (Pro/feature-flag gate, monthly cap, "only bridge if it actually raises the streak").
- Reuses
HabitScheduleService.ComputeStreakAsOfrather than duplicating streak math; correctly mutates the sharedfreezeDateSetso the subsequentComputeLongestStreakstays consistent. - Logging correctly gated to Information level with
IsEnabledcheck, matching CLAUDE.md's callout of streak-freeze activation as an Information-level event. - Solid test coverage of the sequential/logical paths (applies-and-preserves, idempotent-on-rerun, gap-too-large-breaks, no-freeze-breaks, monthly-cap-reached).
Recommendation
Add the DbUpdateException/DbUniqueViolation.IsUniqueViolation guard around the save path that commits the bridged StreakFreeze row, mirroring StreakFreezeAutoActivationService, so the loser of a genuine concurrent race degrades to "no bridge this round" instead of throwing a 500. Once fixed, this is ready to merge — the core logic is sound.
…unique-violation The streak-recalc bridge staged its StreakFreeze insert and left the INSERT to the caller's later SaveChangesAsync (e.g. GetStreakInfo, LogHabit). StreakFreeze has a UNIQUE index on (UserId, UsedOnDate), and the hourly StreakFreezeAutoActivationService independently inserts the same missedDate = userToday - 1 row for the same eligible user. On the exact race this feature closes, the loser's save threw a Postgres 23505 wrapped in DbUpdateException that no caller catches (ConcurrencyRetryBehavior only handles DbUpdateConcurrencyException), surfacing as HTTP 500 + a spurious Sentry issue for a user merely viewing their streak or logging a habit. The bridge now owns a guarded save: it consumes the banked freeze and stages the StreakFreeze insert, then flushes both in one SaveChangesAsync so they commit atomically. On a unique-violation it catches DbUpdateException via the existing DbUniqueViolation helper, drops the staged rows in-memory with DiscardChanges (the failed save already rolled back this consume, so exactly one freeze is spent overall — the concurrent winner's), and still treats the day as covered because the winner's row already bridges it. The catch does no further DB work, so it stays safe even when recalc runs inside IdempotencyBehavior's transaction. Net invariant: whichever path wins the race, the streak is preserved and exactly one freeze is consumed, with no unhandled 500. Adds a conflict-path test asserting the streak stays preserved, the freeze is consumed exactly once, DiscardChanges runs, and no exception propagates. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review: PR #383
Recommendation: APPROVE
Summary
This closes a real race between on-read streak recalculation and the hourly StreakFreezeAutoActivationService: previously only RecalculateAsync looked at existing StreakFreeze rows and never consumed a banked one itself, so a missed scheduled day could reset the streak before the hourly job got a chance to freeze it. The new TryBridgeRecentGapWithBankedFreezeAsync bridges the single most-recent scheduled miss when it strictly helps, is gated by the same Pro/free-tier-flag eligibility and monthly cap as the hourly job, and is race-safe against a concurrent activation via a guarded SaveChangesAsync + unique-violation catch that treats the day as covered either way. I traced the concurrency handling against the hourly job's identical existing DbUniqueViolation/DiscardChanges idiom and against EF Core's automatic-savepoint behavior for ambient transactions (Npgsql supports savepoints), and found the internal partial save safe even when RecalculateAsync runs inside BulkLogHabitsCommand/BulkDeleteHabitsCommand's ExecuteInTransactionAsync wrapper. No contract, authz, or logging-policy issues found; a dedicated security-reviewer pass also returned PASS.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Missing test: banked freeze available + bridgeable gap, but user is NOT freeze-eligible
- location:
tests/Orbit.Infrastructure.Tests/Services/UserStreakServiceTests.cs(new test block, ~line 284) - issue: The 6 new/changed tests exercise "freeze applied" (Pro/flag enabled), "no banked freeze", "gap too large", "concurrent conflict", and "monthly cap reached" — but none exercises the new eligibility gate itself (
!user.HasProAccess && !enabledFlags.Contains(FeatureFlagKeys.GamificationFreeTier)atUserStreakService.cs:157) with a freeze actually banked and a genuinely bridgeable gap. Every test that reaches that line callsEnableGamificationFreeTier()first; the one test that doesn't (RecentMissedDay_NoBankedFreeze_BreaksStreak) also setsStreakFreezesAccumulatedto 0, so it short-circuits at the very first guard and never reaches the eligibility check. - risk: A future change to the eligibility ordering (e.g. accidentally moving the flag check after
ConsumeStreakFreeze(), or inverting the boolean) would let a free-tier, non-flagged user silently auto-consume a banked freeze during recalculation, with no test to catch the regression — the same paygate-adjacent logic inStreakFreezeAutoActivationService.StageFreezeis already treated as test-worthy elsewhere. - fix: Add a test with
SetBankedFreezes(user, 1), no call toEnableGamificationFreeTier(),HasProAccessleft at its default (false), and a completions/date setup that would otherwise be a coverable one-day gap; assert the streak stays broken,user.StreakFreezesAccumulatedstays at 1, and_streakFreezeRepository.DidNotReceive().AddAsync(...). - reference: rubric.md dimension 13 ("every new … service has a unit test"); severity ladder Medium ("missing edge case, missing test").
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS |
| contract-aligner | N/A — no DTO, Controller route, or packages/shared type touched by this diff |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — skipped in CI; Build runs as a separate required check |
| Tests (dotnet) | N/A — skipped in CI; Unit Tests run as a separate required check |
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8) — N/A, no
apps/*UI files in this diff. - Parity (#9) — N/A, frontend-only dimension; no
apps/*mirror surface touched. - i18n (#10) — N/A, no new user-facing strings added.
- Contract drift + backward-compat (#11) — N/A, no DTO, Controller route, or Zod schema changed; no backward-compat risk to old mobile clients (purely internal recalculation logic, no wire-shape change).
- FEATURES.md parity (#14) — N/A, this is a bugfix closing an existing race in already-shipped streak-freeze behavior, not a new/changed/removed user-facing feature surface; also not checked out in this repo regardless.
- Cross-model second opinion (Phase 6.2) — UNAVAILABLE (no
opencodein this CI environment); moot anyway since no Critical finding survived. - All 3 changed files (
UserStreakService.cs,GetStreakHistoryQueryHandlerTests.cs,UserStreakServiceTests.cs) received a verdict — nothing left unexamined.
What's good
- Root-causes the actual bug (recalc never consumed a banked freeze) instead of papering over symptoms, and explicitly reuses the hourly job's own eligibility/cap/idempotency semantics rather than inventing a parallel rule set.
- The concurrency story is genuinely thought through: the "coverable-gap" test (
streakWithBridge <= currentStreak→ skip) avoids spending a freeze on an unrecoverable gap, and the unique-violation catch treats a concurrent hourly-job win as "day covered" rather than an error — with a dedicated test exercising exactly that race. - Idempotency across repeated recalculation is tested directly, and the monthly cap and "gap too large" boundaries each get their own test.
- Logging follows the project's structured
[LoggerMessage]convention at the right levels (Information for the business event, Debug for the internal race-resolution detail).
Recommendation
Approve as-is. The Medium finding (missing eligibility-gate test) is a good follow-up but not merge-blocking — it doesn't indicate a defect in the shipped logic, only a coverage gap in an already well-tested change.
1 participant
Permanent fix for #10 (the race a live account hit).
UserStreakService.RecalculateAsynconly honored existing StreakFreeze rows; it never CONSUMED a banked freeze, so a missed scheduled day reset CurrentStreak to 0 and cleared LastActiveDate — which then made the user fail the hourly StreakFreezeAutoActivationService filter, permanently dropping the freeze (whichever path ran first with stale state won).New
TryBridgeRecentGapWithBankedFreezeAsyncbridges the single most-recent scheduled miss (yesterday) only when ALL hold: a banked freeze is available; yesterday is a scheduled, un-completed, not-already-frozen day; monthly cap not hit; covering it strictly RAISES the recomputed streak (precise coverable-gap test); user is freeze-eligible (same gate as the hourly job). It consumes the freeze AND persists a StreakFreeze row — that row is the idempotency + race-safety anchor (a later recalc or the hourly job sees the date and neither re-consumes nor inflates). PR #355's notify-isolation untouched. The CalendarFallback (zero-recurring-habit) path is deliberately left unchanged (schedule-less gap is ambiguous; those users stay protected by the hourly job).5 tests incl. the without-fix-fails race case + idempotency + monthly-cap + gap-too-large + no-freeze. Build 0 errors; Infra 1982 / App 2773 / Domain 512 all pass.
Refs thomasluizon/orbit-ui-mobile#243
🤖 Generated with Claude Code