fix(scale): single-flight the weather cache, index the crew outbox dr… - #601
Conversation
…ain, and close two write races
Weather cache stampede (confirmed). getWeatherForLocation was plain
cache-aside, read by two PUBLIC guest pages as well as both nudge crons — so
concurrent misses on one key are the normal case, not an edge one: the TTL
expires at a fixed point for every guest at a property. N misses were N calls
to Tomorrow.io, N times the rate-limit consumption, and N x WEATHER_TIMEOUT_MS
of blocked render/step time exactly when the provider is struggling.
The lock is now lib/cache/single-flight.ts rather than inline: two token-refresh
paths already had this open-coded and this would have been the third copy.
refresh-lock.ts delegates to the same SETNX, so there is one implementation to
reason about. Fails open — no Redis means produce, because failing closed turns
a cache outage into a total one.
Crew outbox drain (confirmed, and the obvious fix is a trap). The drain
materialised EVERY row — live and dead-lettered — then filtered in JS, on each
enqueue, each reconnect and the 30-second tick; dead letters live for 30 days,
so a device with a bad month re-loaded a growing table ~2,880 times a day.
Swapping to `.where('failed').equals(0)` alone would have BROKEN THE OUTBOX
SILENTLY. enqueueMutationTx only ever wrote `failed` on the held-back branch,
and IndexedDB omits a record from an index when the indexed property is
undefined — so every ordinary queued mutation would have become invisible to
the drain. Nothing would sync and nothing would say why. `failed: 0` is now
written on every row and schema v12 backfills the ones already queued; the fake
Dexie double models the strict-equality index semantics so the hazard cannot
come back untested.
Two write races:
- dismissSuggestion had no compare-and-swap where acceptSuggestion has one.
A dismiss landing after an accept overwrote suggestion_status
unconditionally: the accepted assignment stayed while the suggestion read
as dismissed, neither caller saw an error, and the negative training signal
was recorded against a suggestion that had been accepted. The precondition
is `suggestion_status = 'pending'` — NOT the SUGGESTION_ACCEPTABLE_STATUSES
list, which holds turnover_status values; matching one against the other
filters every row out and breaks dismissal entirely.
trackAssignmentAgainstSuggestions' override write had the same read-then-
write shape and gets the same guard.
- The purchase-order-from-count insert now treats 23505 as the constraint
working rather than a fault. Deliberately NOT "re-read and return the
winner's id": this file's own pre-check is emphatic that a header row alone
is not enough, so the race path lands back in the same repair logic. A
PO whose items insert died is a restock order listing nothing, permanently.
CPA export: severity does not hold as written. The trigger given was rows
"approaching the 200k fetchAllRows default" — an export is one row per active
asset per tax year, production has 160 active assets across 27 properties, and
CLAUDE.md's target user runs 10-50 properties. Reaching 200k needs ~33,000
properties in one org. The synchronous-CPU mechanism is real, so the route now
bounds itself with a head-only count and returns a clear 413 instead of being
killed mid-serialisation. Moving generation onto Inngest + Storage + polling is
a real option but it is a feature change on a premise off by three orders of
magnitude, so it is offered rather than built.
3473 tests / 345 files green. Lint 187/189, semgrep chokepoints exit 0, no
ratchet count increased. All five canaried by reverting each individually.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
smj1860 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds concurrency guards for suggestions and purchase orders, limits CPA exports, introduces Redis single-flight caching, and changes Dexie outbox draining to use a normalized indexed failure flag. Tests cover the new race, limit, cache, and replay behavior. ChangesSuggestion compare-and-swap
CPA export limit
Redis cache coordination
Indexed Dexie outbox draining
Purchase-order creation race handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WeatherCaller
participant singleFlight
participant Redis
participant TomorrowIO
WeatherCaller->>singleFlight: read rounded-location cache
singleFlight->>Redis: acquire NX lock
singleFlight->>TomorrowIO: fetch on lock winner
TomorrowIO-->>singleFlight: weather data
singleFlight->>Redis: cache data and release lock
singleFlight-->>WeatherCaller: return weather data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
lib/dexie/schema.ts (1)
433-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a v11-to-v12 migration test.
This upgrade normalizes absent
failedfields on legacy queued rows. Add a migration regression test that seeds version-11mutationsandpending_photo_uploadsrows withoutfailed, upgrades to version 12, and assertsfailed: 0for both outbox tables.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/dexie/schema.ts` around lines 433 - 439, The version(12) upgrade migration lacks regression coverage. Add a migration test that creates a version-11 database, seeds `mutations` and `pending_photo_uploads` rows without `failed`, upgrades through the schema to version 12, and asserts both rows contain `failed: 0` after migration.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/`(dashboard)/turnovers/actions.ts:
- Around line 74-77: Update the compare-and-swap update in the turnover outcome
flow to return the IDs of rows it actually changed, then filter the subsequent
priorSuggestionRows construction to those returned IDs instead of the stale
overridden list. Preserve the existing upsert behavior for matched rows and add
a race test covering a concurrent accept or dismiss where the update returns no
rows, ensuring no incorrect was_accepted: false record is written.
In `@lib/cache/single-flight.ts`:
- Line 55: Replace the key-bearing warning in the single-flight lock-unavailable
path with a non-sensitive lock category, ensuring the log never includes key or
userId values. Update the related Hospitable test assertion to expect the
category-based warning instead of the lock key.
- Around line 114-121: Update the lock-loser wait path in the single-flight
method to retry acquireLock() after each opts.read() while the lock remains
live, and do not call opts.produce() until the lock has expired or no valid
holder remains. Preserve returning settled values immediately, and update the
fallback test to model a slow winner whose lock remains valid.
- Line 1: Add the side-effect-only server-only import before existing imports in
lib/cache/single-flight.ts (line 1), lib/integrations/refresh-lock.ts (line 1),
and lib/weather/tomorrow.ts (line 3), ensuring all modules that access or
transitively use getRedisIfConfigured are explicitly marked server-only.
- Around line 45-70: Update acquireLock and releaseLock to use ownership leases:
generate and store a unique token with SET NX EX, return that token (or an
equivalent lease) only when the lock is acquired, and avoid granting a
releasable lease when Redis is unavailable. Change releaseLock to accept the
lease and use `@upstash/redis` eval with a compare-and-delete script so the key is
deleted only when its stored token matches; preserve the existing non-throwing
behavior.
In `@lib/inngest/functions/inventory-events.ts`:
- Around line 367-392: Make the 23505 recovery in the purchase-order creation
flow single-writer: do not treat an empty purchase_order_items list as proof
that no writer is active. Update the logic around the race re-read,
insertPoItems, and markPoSent to atomically claim completion or use a
conflict-safe database operation that creates the header and items together,
preserving idempotency and preventing the losing invocation from duplicating the
winner’s items.
In `@unit/inngest/inventory-events-po.test.ts`:
- Around line 206-226: The success-path tests around
handleInventoryCountSubmitted must not swallow handler failures with catch(() =>
{}). Stub the later email/downstream steps that are outside the test scope, then
await the handler directly and assert the purchase-order creation flow resolves
successfully, including the 23505 race handling and the insertPoItems/markPoSent
path.
In `@unit/turnovers/turnovers-actions.test.ts`:
- Around line 1013-1016: The dismissSuggestion test should verify the exact
NOTHING_UPDATED conflict response instead of only asserting that success is
absent. Update the assertion on result after dismissSuggestion('t_1') to match
the expected error state and preserve the assertion that assignment_outcomes is
not accessed.
---
Nitpick comments:
In `@lib/dexie/schema.ts`:
- Around line 433-439: The version(12) upgrade migration lacks regression
coverage. Add a migration test that creates a version-11 database, seeds
`mutations` and `pending_photo_uploads` rows without `failed`, upgrades through
the schema to version 12, and asserts both rows contain `failed: 0` after
migration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c3ddc28-0ffb-40be-91e4-dc7d2d4a538e
📒 Files selected for processing (22)
app/(dashboard)/turnovers/actions.tsapp/api/assets/cpa-export/route.tslib/cache/single-flight.tslib/dexie/schema.tslib/dexie/syncService.tslib/inngest/functions/inventory-events.tslib/integrations/refresh-lock.tslib/weather/tomorrow.tsunit/dexie/asset-scan-request-durability.test.tsunit/dexie/fake-dexie.tsunit/dexie/offline-write-durability.test.tsunit/dexie/outbox-drain-scheduling.test.tsunit/dexie/sync-outbox-backoff.test.tsunit/dexie/sync-outbox-durability.test.tsunit/dexie/sync-outbox-ordering.test.tsunit/dexie/sync-shadow-and-prune.test.tsunit/inngest/inventory-events-po.test.tsunit/lib/hospitable-token-lock.test.tsunit/lib/single-flight.test.tsunit/lib/weather-single-flight.test.tsunit/route-handlers/assets-cpa-export.test.tsunit/turnovers/turnovers-actions.test.ts
| const { error: overrideError } = await service.from('turnovers') | ||
| .update({ suggestion_status: 'overridden' }) | ||
| .eq('org_id', orgId) | ||
| .eq('suggestion_status', 'pending') |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use only rows changed by the compare-and-swap for outcome writes.
The update can match zero rows after a concurrent accept or dismiss. priorSuggestionRows still uses the stale overridden list. The subsequent upsert can then record was_accepted: false for a suggestion that was accepted.
Select the updated IDs. Build priorSuggestionRows only from turnovers whose IDs were returned. Add a race test for this path.
Proposed fix
- const { error: overrideError } = await service.from('turnovers')
+ const { data: overriddenRows, error: overrideError } = await service.from('turnovers')
.update({ suggestion_status: 'overridden' })
.eq('org_id', orgId)
.eq('suggestion_status', 'pending')
.in('id', overridden.map(t => t.id))
+ .select('id')
if (overrideError) {
console.error('[trackAssignmentAgainstSuggestions] override update failed', overrideError)
reportError(overrideError, { site: 'serverAction.turnovers.trackAssignmentAgainstSuggestions.override', orgId })
}
- const priorSuggestionRows = overridden.flatMap(t =>
+ const overriddenIds = new Set(overriddenRows?.map(row => row.id))
+ const priorSuggestionRows = overridden
+ .filter(t => overriddenIds.has(t.id))
+ .flatMap(t =>
(t.suggested_crew_ids ?? []).map(suggestedCrewId => ({
turnover_id: t.id,
org_id: orgId,
crew_member_id: suggestedCrewId,
was_accepted: false,
override_reason: `${crewName} assigned instead of the suggestion`,
}))
- )
+ )As per coding guidelines, use atomic database constraints or conditional updates for race-prone load-then-write operations, and provide cleanup or rollback for partial multi-step writes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { error: overrideError } = await service.from('turnovers') | |
| .update({ suggestion_status: 'overridden' }) | |
| .eq('org_id', orgId) | |
| .eq('suggestion_status', 'pending') | |
| const { data: overriddenRows, error: overrideError } = await service.from('turnovers') | |
| .update({ suggestion_status: 'overridden' }) | |
| .eq('org_id', orgId) | |
| .eq('suggestion_status', 'pending') | |
| .in('id', overridden.map(t => t.id)) | |
| .select('id') | |
| if (overrideError) { | |
| console.error('[trackAssignmentAgainstSuggestions] override update failed', overrideError) | |
| reportError(overrideError, { site: 'serverAction.turnovers.trackAssignmentAgainstSuggestions.override', orgId }) | |
| } | |
| const overriddenIds = new Set(overriddenRows?.map(row => row.id)) | |
| const priorSuggestionRows = overridden | |
| .filter(t => overriddenIds.has(t.id)) | |
| .flatMap(t => | |
| (t.suggested_crew_ids ?? []).map(suggestedCrewId => ({ | |
| turnover_id: t.id, | |
| org_id: orgId, | |
| crew_member_id: suggestedCrewId, | |
| was_accepted: false, | |
| override_reason: `${crewName} assigned instead of the suggestion`, | |
| })) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/`(dashboard)/turnovers/actions.ts around lines 74 - 77, Update the
compare-and-swap update in the turnover outcome flow to return the IDs of rows
it actually changed, then filter the subsequent priorSuggestionRows construction
to those returned IDs instead of the stale overridden list. Preserve the
existing upsert behavior for matched rows and add a race test covering a
concurrent accept or dismiss where the update returns no rows, ensuring no
incorrect was_accepted: false record is written.
Source: Coding guidelines
| @@ -0,0 +1,135 @@ | |||
| import { getRedisIfConfigured } from '@/lib/redis' | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
for f in lib/cache/single-flight.ts lib/integrations/refresh-lock.ts lib/weather/tomorrow.ts; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,120p' "$f"
else
echo "MISSING: $f"
fi
done
echo "== server-only usage in candidates =="
rg -n "import 'server-only'|refreshLock|singleFlight|getRedisIfConfigured|acquireLock|releaseLock|secret|tomorrow" lib/cache/single-flight.ts lib/integrations/refresh-lock.ts lib/weather/tomorrow.ts || true
echo "== ts config / client boundaries hints =="
fd -H '^(tsconfig.*\.json|package\.json)$' . -x sh -c 'echo "--- $1 ---"; sed -n "1,220p" "$1"' sh {}Repository: smj1860/fieldstay
Length of output: 19105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect exported usages from the three modules and imports into app/src packages,
# plus whether server-only marker appears near file top.
python3 - <<'PY'
import pathlib, re
files = [
pathlib.Path('lib/cache/single-flight.ts'),
pathlib.Path('lib/integrations/refresh-lock.ts'),
pathlib.Path('lib/weather/tomorrow.ts'),
]
for p in files:
text = p.read_text()
imports = re.findall(r"^import\s+(['\"][^'\"]+['\"])", text, flags=re.M)
marked = bool(re.search(r"^import\s+['\"]server-only['\"]", text, flags=re.M))
print(f"{p}: marked={marked}, line_count={len(text.splitlines())}, first_imports={imports[:5]}")
for i, line in enumerate(text.splitlines(), 1):
if "import 'server-only'" in line:
print(f" server-only marker line {i}: {line.strip()}")
PY
echo "== references =="
rg -n "from ['\"]`@/lib/cache/single-flight`|from ['\"]`@/lib/integrations/refresh-lock`|from ['\"]`@/lib/weather/tomorrow`|singleFlight|refreshLock|getRedisIfConfigured|acquireLock|releaseLock" -g '!node_modules' .Repository: smj1860/fieldstay
Length of output: 7387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lib/redis.ts server marker and relevant implementation =="
wc -l lib/redis.ts
sed -n '1,140p' lib/redis.ts
echo "== lib/weather/tomorrow.ts env/token/body excerpt =="
sed -n '120,229p' lib/weather/tomorrow.ts
echo "== server-only markers in related redis integrations =="
rg -n "import 'server-only'|getRedis\\(|getRedisIfConfigured\\(" lib/integrations lib/cache lib/weather lib -g '*.ts'
echo "== relevant exported imports into lib only =="
rg -n "from ['\"]`@/lib/`(cache/single-flight|integrations/refresh-lock)\"" lib -g '*.ts' || trueRepository: smj1860/fieldstay
Length of output: 11641
Mark the server-only modules explicitly.
lib/cache/single-flight.ts reads Upstash Redis config/client via getRedisIfConfigured(), and lib/integrations/refresh-lock.ts and lib/weather/tomorrow.ts transitively read the same server-only path. Add import 'server-only' before other imports in those modules.
📍 Affects 3 files
lib/cache/single-flight.ts#L1-L1(this comment)lib/integrations/refresh-lock.ts#L1-L1lib/weather/tomorrow.ts#L3-L3
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cache/single-flight.ts` at line 1, Add the side-effect-only server-only
import before existing imports in lib/cache/single-flight.ts (line 1),
lib/integrations/refresh-lock.ts (line 1), and lib/weather/tomorrow.ts (line 3),
ensuring all modules that access or transitively use getRedisIfConfigured are
explicitly marked server-only.
Source: Coding guidelines
| export async function acquireLock( | ||
| key: string, | ||
| ttlSeconds: number = DEFAULT_LOCK_TTL_SECONDS, | ||
| ): Promise<boolean> { | ||
| const redis = getRedisIfConfigured() | ||
| if (!redis) return true | ||
|
|
||
| try { | ||
| return (await redis.set(key, '1', { nx: true, ex: ttlSeconds })) === 'OK' | ||
| } catch (err) { | ||
| console.warn(`[single-flight] lock unavailable for ${key}, proceeding unlocked:`, err) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| /** Release early so the next caller doesn't wait out the TTL. Never throws. */ | ||
| export async function releaseLock(key: string): Promise<void> { | ||
| const redis = getRedisIfConfigured() | ||
| if (!redis) return | ||
|
|
||
| try { | ||
| await redis.del(key) | ||
| } catch { | ||
| // Non-fatal — the TTL expires it. | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)single-flight\.ts$|(^|/)package\.json$|pnpm-lock\.yaml$|package-lock\.json$|yarn\.lock$' || true
echo
echo "== single-flight outline =="
if [ -f lib/cache/single-flight.ts ]; then
ast-grep outline lib/cache/single-flight.ts || true
echo
echo "== single-flight lines =="
nl -ba lib/cache/single-flight.ts | sed -n '1,220p'
fi
echo
echo "== lock usages =="
rg -n "acquireLock|releaseLock|singleFlight|single-fl|getRedisIfConfigured|REDIS" -S . || trueRepository: smj1860/fieldstay
Length of output: 747
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== single-flight lines =="
awk '{printf "%6d\t%s\n",NR,$0}' lib/cache/single-flight.ts | sed -n '1,180p'
echo
echo "== package deps for redis =="
python3 - <<'PY'
import json
p=json.load(open('package.json'))
for sec in ['dependencies','devDependencies']:
if sec in p:
vals=p[sec]
print(sec, {k:v for k,v in vals.items() if 'redis' in k.lower() or 'upstash' in k.lower() or k in ['`@vercel/kv`','redis']})
PY
echo
echo "== lock usages =="
grep -RInE "acquireLock|releaseLock|singleFlight|single-fl|getRedisIfConfigured|REDIS" . || true
echo
echo "== pnpm relevant redis/upstash entries =="
grep -nE "`@upstash/redis`|redis" pnpm-lock.yaml | sed -n '1,120p'Repository: smj1860/fieldstay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== single-flight file size =="
wc -l lib/cache/single-flight.ts
echo
echo "== single-flight lines 1-180 =="
python3 - <<'PY'
from pathlib import Path
p=Path('lib/cache/single-flight.ts')
lines=p.read_text().splitlines()
for i,l in enumerate(lines[:180],1):
print(f"{i:6d}\t{l}")
PY
echo
echo "== package redis dependency =="
python3 - <<'PY'
import json
p=json.load(open('package.json'))
for sec in ['dependencies','devDependencies','peerDependencies']:
v=p.get(sec,{})
matches={k:vv for k,vv in v.items() if 'redis' in k.lower() or 'upstash' in k.lower() or k=='redis'}
if matches:
print(sec, matches)
PY
echo
echo "== lock usages (limited) =="
grep -RInE "acquireLock|releaseLock|singleFlight|single-fl|getRedisIfConfigured|REDIS" . || true
echo
echo "== lock-related file paths =="
git ls-files | grep -nE '(^|/)(cache|weather|openweather|single-flight|REDIS).*' || trueRepository: smj1860/fieldstay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== single-flight actual lines 1-180 =="
python3 <<'PY'
from pathlib import Path
for i, line in enumerate(Path('lib/cache/single-flight.ts').read_text().splitlines()[:180], 1):
print(f"{i:04d} | {line}")
PY
echo
echo "== relevant single-flight symbols =="
grep -n "acquireLock\\|releaseLock\\|singleFlight\\|DEFAULT_LOCK_TTL_SECONDS\\|crypto.randomUUID\\|return redis.set\\|await redis.del\\|eval\\|redis\\.eval\\|getRedisIfConfigured" lib/cache/single-flight.ts || true
echo
echo "== package redis dependency =="
python3 <<'PY'
import json
p=json.load(open('package.json'))
for sec in ['dependencies','devDependencies','peerDependencies']:
v=p.get(sec,{})
matches={k:vv for k,vv in v.items() if 'redis' in k.lower() or 'upstash' in k.lower() or k=='redis'}
if matches:
print(sec, matches)
PY
echo
echo "== lock usages =="
grep -RIn "acquireLock\\|releaseLock\\|singleFlight\\|single-fl\\|getRedisIfConfigured\\|REDIS" --exclude-dir=.git . || trueRepository: smj1860/fieldstay
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== single-flight first 220 lines, byte-range safe =="
awk '{printf "%04d | %s\n", NR, $0}' lib/cache/single-flight.ts | awk '{if(NR<=220) print}'
echo
echo "== single-flight target symbols =="
grep -n "acquireLock\\|releaseLock\\|singleFlight\\|DEFAULT_LOCK_TTL_SECONDS\\|DEFAULT_WAIT_MS\\|crypto.randomUUID\\|redis.set\\|redis.del\\|redis.eval\\|eval\\|getRedisIfConfigured" lib/cache/single-flight.ts || true
echo
echo "== package manifest redis-related deps =="
python3 <<'PY'
import json
p=json.load(open('package.json'))
for sec in ['dependencies','devDependencies','peerDependencies']:
v=p.get(sec,{})
matches={k:vv for k,vv in v.items() if 'redis' in k.lower() or 'upstash' in k.lower() or k=='redis'}
if matches: print(sec, json.dumps(matches, sort_keys=True, indent=2))
PY
echo
echo "== lock-related file candidates =="
git ls-files | grep -nE 'single-flight|cache|weather|REDIS|redis' || trueRepository: smj1860/fieldstay
Length of output: 7644
🌐 Web query:
@upstash/redis TypeScript redis.eval command usage
💡 Result:
The @upstash/redis TypeScript SDK provides the redis.eval command to execute Lua scripts server-side in an atomic operation [1][2]. Basic Usage The command signature is redis.eval(script, keys, args), where the SDK automatically handles the numkeys argument required by standard Redis [2]. await redis.eval( "return ARGV[1]", // Lua script [], // Keys array (accessible as KEYS in Lua) ["Hello World"] // Arguments array (accessible as ARGV in Lua)); Type Safety You can provide a generic type parameter to specify the expected return type of the script [3]: const result = await redis.eval( "return tonumber(ARGV[1])", [], ["42"]); Optimized Execution (Script Class) For frequently used scripts, it is recommended to use the Script class, which automates EVALSHA caching to reduce bandwidth by only sending the script content when necessary [1][3][4]. const script = redis.createScript(local current = redis.call('GET', KEYS[1]) or 0 local next = current + tonumber(ARGV[1]) redis.call('SET', KEYS[1], next) return next); // exec optimistically tries EVALSHA, falls back to EVAL if needed const result = await script.exec(["counter"], ["1"]); Key Differences: - redis.eval: Sends the full script to the server every time [1][4]. - redis.createScript.exec: Attempts to use EVALSHA (via cached SHA-1 hash), falling back to EVAL only if the script is not yet cached on the server [3][4]. - Read-Only Scripts: For read-only operations, use { readOnly: true } in createScript or the ScriptRO class to utilize EVAL_RO/EVALSHA_RO, which can be executed on read replicas [1][4].
Citations:
- 1: https://upstash-redis-js.mintlify.app/api/commands/eval
- 2: https://upstash.com/blog/lua-scripting-on-upstash-redis-atomic-operations-over-http
- 3: https://upstash-redis-js.mintlify.app/advanced/lua-scripts
- 4: https://upstash-redis-js.mintlify.app/api/script
Preserve lock ownership during release.
releaseLock() deletes by key only, so an expired owner can delete a newer owner’s valid lock. acquireLock() also returns true on Redis failure without a lease, which makes the same fail-over producer able to delete another caller’s lock after Redis recovers.
Store a unique token with SET NX EX, return that ownership lease from acquireLock(), and delete only when the stored token matches it in one atomic Redis script. Use @upstash/redis eval(script, keys, args) for this compare-and-delete operation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cache/single-flight.ts` around lines 45 - 70, Update acquireLock and
releaseLock to use ownership leases: generate and store a unique token with SET
NX EX, return that token (or an equivalent lease) only when the lock is
acquired, and avoid granting a releasable lease when Redis is unavailable.
Change releaseLock to accept the lease and use `@upstash/redis` eval with a
compare-and-delete script so the key is deleted only when its stored token
matches; preserve the existing non-throwing behavior.
| try { | ||
| return (await redis.set(key, '1', { nx: true, ex: ttlSeconds })) === 'OK' | ||
| } catch (err) { | ||
| console.warn(`[single-flight] lock unavailable for ${key}, proceeding unlocked:`, err) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log lock keys.
key can contain userId through lib/integrations/refresh-lock.ts Line 41. This warning logs that identifier when Redis is unavailable. Log a non-sensitive lock category instead. Update the Hospitable test to assert that category.
As per coding guidelines, “Never log PII, guest phone numbers, SMS bodies, actual financial costs, Stripe tokens, secrets, or API keys.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cache/single-flight.ts` at line 55, Replace the key-bearing warning in
the single-flight lock-unavailable path with a non-sensitive lock category,
ensuring the log never includes key or userId values. Update the related
Hospitable test assertion to expect the category-based warning instead of the
lock key.
Source: Coding guidelines
| for (let i = 0; i < maxWaits; i++) { | ||
| await new Promise((resolve) => setTimeout(resolve, waitMs)) | ||
| const settled = await opts.read() | ||
| if (settled !== null && settled !== undefined) return settled | ||
| } | ||
| // Winner died, or is slower than our patience. Produce rather than fail — | ||
| // but do NOT release a lock we never held. | ||
| return opts.produce() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not produce while a live lock still exists.
The default wait budget is 900 ms. The weather producer has an 8-second request timeout and a 13-second lock TTL. A normal provider response that takes longer than 900 ms causes every lock loser to call produce() while the winner still holds the lock.
Wait until the lock can expire, or retry acquireLock() after each cache re-read. Only fall back to production after no valid holder can remain. Update the fallback test to simulate a slow but live winner.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cache/single-flight.ts` around lines 114 - 121, Update the lock-loser
wait path in the single-flight method to retry acquireLock() after each
opts.read() while the lock remains live, and do not call opts.produce() until
the lock has expired or no valid holder remains. Preserve returning settled
values immediately, and update the fallback test to model a slow winner whose
lock remains valid.
| if (poError?.code === '23505') { | ||
| const raceRes = await supabase | ||
| .from('purchase_orders') | ||
| .select('id, purchase_order_items(id)') | ||
| .eq('source_count_id', count_id) | ||
| .eq('org_id', org_id) | ||
| .maybeSingle() | ||
|
|
||
| if (raceRes.error) { | ||
| throw new Error(`purchase_orders race re-read failed: ${raceRes.error.message}`) | ||
| } | ||
| // Gone again means the winner was rolled back — let the step retry | ||
| // rather than silently reporting a PO that does not exist. | ||
| if (!raceRes.data) throw new Error('purchase order vanished after a 23505 — retrying') | ||
|
|
||
| if ((raceRes.data.purchase_order_items ?? []).length > 0) { | ||
| return { purchaseOrderId: raceRes.data.id, alreadyExisted: true } | ||
| } | ||
|
|
||
| logger.warn( | ||
| `Count ${count_id}: lost the create race to ${raceRes.data.id}, which has zero line ` + | ||
| 'items — completing it rather than treating it as done.' | ||
| ) | ||
| await insertPoItems(supabase, raceRes.data.id, belowParItems) | ||
| await markPoSent(supabase, raceRes.data.id) | ||
| return { purchaseOrderId: raceRes.data.id, alreadyExisted: false } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make empty-header repair single-writer.
A 23505 proves that the header insert committed. It does not prove that the winning invocation stopped. The winner can still be between its header insert and insertPoItems().
The losing invocation can repair the empty header at Line 390. The original winner can then continue at Line 404 and insert the same rows. This can duplicate purchase-order items or cause a child unique-conflict retry.
Claim completion atomically, or create the header and item rows in one conflict-safe database operation. Do not use an empty item list as proof that no writer owns the purchase order.
As per coding guidelines, “Database-creating Inngest steps must be idempotent: check source_reference_id for owner transactions or use conflict-safe insertion for other records.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/inngest/functions/inventory-events.ts` around lines 367 - 392, Make the
23505 recovery in the purchase-order creation flow single-writer: do not treat
an empty purchase_order_items list as proof that no writer is active. Update the
logic around the race re-read, insertPoItems, and markPoSent to atomically claim
completion or use a conflict-safe database operation that creates the header and
items together, preserving idempotency and preventing the losing invocation from
duplicating the winner’s items.
Source: Coding guidelines
| it('treats a 23505 as already-handled when the winner wrote a COMPLETE purchase order', async () => { | ||
| const supabase = makeSupabase({ | ||
| existingPoQueue: [ | ||
| null, // pre-check: nothing yet | ||
| { id: 'po_winner', purchase_order_items: [{ id: 'x' }] },// race re-read: complete | ||
| ], | ||
| poInsertError: { code: '23505', message: 'duplicate key value violates unique constraint' }, | ||
| }) | ||
| ;(createServiceClient as ReturnType<typeof vi.fn>).mockReturnValue(supabase) | ||
|
|
||
| await invokeHandler(handleInventoryCountSubmitted, { | ||
| event, step: makeStep([belowParItem]), | ||
| logger: { info: vi.fn(), error: vi.fn(), warn: vi.fn() }, | ||
| }).catch(() => { /* later email steps are not under test */ }) | ||
|
|
||
| // No duplicate items, and no throw that would burn a retry. | ||
| expect( | ||
| supabase.calls.some((c) => c.table === 'purchase_order_items' && c.method === 'insert'), | ||
| 'the winner already wrote the items — re-inserting them would duplicate the restock order', | ||
| ).toBe(false) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not discard handler failures in success-path tests.
The catch(() => {}) calls at Lines 219 and 244 hide failures from the new race path. The first test can pass when the handler throws before completing the 23505 handling. The second test can pass when insertPoItems() succeeds but markPoSent() fails.
Stub downstream steps, then assert that the purchase-order creation path resolves successfully.
Also applies to: 228-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@unit/inngest/inventory-events-po.test.ts` around lines 206 - 226, The
success-path tests around handleInventoryCountSubmitted must not swallow handler
failures with catch(() => {}). Stub the later email/downstream steps that are
outside the test scope, then await the handler directly and assert the
purchase-order creation flow resolves successfully, including the 23505 race
handling and the insertPoItems/markPoSent path.
| const result = await dismissSuggestion('t_1') | ||
|
|
||
| expect(result).not.toEqual({ success: true }) | ||
| expect(supabase.calls.some((c) => c.table === 'assignment_outcomes')).toBe(false) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the exact NOTHING_UPDATED result.
The current assertion also passes for an unrelated database or operation error. Assert the expected error state so this test verifies the conflict response contract.
Proposed fix
- expect(result).not.toEqual({ success: true })
+ expect(result).toEqual({
+ error: 'You do not have permission to make this change, or the turnover no longer exists.',
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await dismissSuggestion('t_1') | |
| expect(result).not.toEqual({ success: true }) | |
| expect(supabase.calls.some((c) => c.table === 'assignment_outcomes')).toBe(false) | |
| const result = await dismissSuggestion('t_1') | |
| expect(result).toEqual({ | |
| error: 'You do not have permission to make this change, or the turnover no longer exists.', | |
| }) | |
| expect(supabase.calls.some((c) => c.table === 'assignment_outcomes')).toBe(false) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@unit/turnovers/turnovers-actions.test.ts` around lines 1013 - 1016, The
dismissSuggestion test should verify the exact NOTHING_UPDATED conflict response
instead of only asserting that success is absent. Update the assertion on result
after dismissSuggestion('t_1') to match the expected error state and preserve
the assertion that assignment_outcomes is not accessed.
2 participants
…ain, and close two write races
Weather cache stampede (confirmed). getWeatherForLocation was plain cache-aside, read by two PUBLIC guest pages as well as both nudge crons — so concurrent misses on one key are the normal case, not an edge one: the TTL expires at a fixed point for every guest at a property. N misses were N calls to Tomorrow.io, N times the rate-limit consumption, and N x WEATHER_TIMEOUT_MS of blocked render/step time exactly when the provider is struggling.
The lock is now lib/cache/single-flight.ts rather than inline: two token-refresh paths already had this open-coded and this would have been the third copy. refresh-lock.ts delegates to the same SETNX, so there is one implementation to reason about. Fails open — no Redis means produce, because failing closed turns a cache outage into a total one.
Crew outbox drain (confirmed, and the obvious fix is a trap). The drain materialised EVERY row — live and dead-lettered — then filtered in JS, on each enqueue, each reconnect and the 30-second tick; dead letters live for 30 days, so a device with a bad month re-loaded a growing table ~2,880 times a day.
Swapping to
.where('failed').equals(0)alone would have BROKEN THE OUTBOX SILENTLY. enqueueMutationTx only ever wrotefailedon the held-back branch, and IndexedDB omits a record from an index when the indexed property is undefined — so every ordinary queued mutation would have become invisible to the drain. Nothing would sync and nothing would say why.failed: 0is now written on every row and schema v12 backfills the ones already queued; the fake Dexie double models the strict-equality index semantics so the hazard cannot come back untested.Two write races:
suggestion_status = 'pending'— NOT the SUGGESTION_ACCEPTABLE_STATUSES list, which holds turnover_status values; matching one against the other filters every row out and breaks dismissal entirely. trackAssignmentAgainstSuggestions' override write had the same read-then- write shape and gets the same guard.CPA export: severity does not hold as written. The trigger given was rows "approaching the 200k fetchAllRows default" — an export is one row per active asset per tax year, production has 160 active assets across 27 properties, and CLAUDE.md's target user runs 10-50 properties. Reaching 200k needs ~33,000 properties in one org. The synchronous-CPU mechanism is real, so the route now bounds itself with a head-only count and returns a clear 413 instead of being killed mid-serialisation. Moving generation onto Inngest + Storage + polling is a real option but it is a feature change on a premise off by three orders of magnitude, so it is offered rather than built.
3473 tests / 345 files green. Lint 187/189, semgrep chokepoints exit 0, no ratchet count increased. All five canaried by reverting each individually.
Claude-Session: https://claude.ai/code/session_014Zk7eKd7UeNCoWxHSVxAFR
Summary by CodeRabbit
New Features
Bug Fixes
Performance