fix(worker): resolve #41-#43, #49, #52 - #55
Conversation
…dates, lifecycle tick test, off-market filters Fixes #41, #42, #43, #49, #52 - alerts.ts: keyset-batched watchlist fetch (ALERT_WATCHLIST_BATCH=2000) — bounded memory - oper-worker-alerts.service: MemoryMax 192M -> 384M - CANDIDATES_SQL predicates aligned to the idx_listings_last_seen partial index (subset match), comment names the index - lifecycle.test.ts: mock-pool orchestration test (step order, params, stats aggregation, null rowCount) - digest/rent-estimator/watchlist-alerts: lifecycle filters (active-only notifications; NOT IN for recompute) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ytTRmZoRpCSnhKA3xJURj
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe worker now bounds alert-tick watchlist loading, filters inactive listings from digest and alert queries, excludes off-market rent-estimator rows, increases alert service memory, and adds lifecycle orchestration tests. ChangesWorker processing controls
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/worker/src/alerts.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/worker/src/alerts.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. apps/worker/src/digest.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a batch limit on watchlists fetched per alert tick to bound memory and CPU usage, increases the systemd service memory limit to 384M, and updates various queries to filter by active listing status to align with partial indexes. Feedback highlights a critical SQL three-valued logic bug in rent-estimator.ts where using NOT IN on the nullable listing_status column will cause NULL rows to be permanently stuck in a pending state. Additionally, a false positive warning issue was identified in alerts.ts when the watchlist count exactly matches the batch size, which can be resolved by querying for one extra row to detect if more records remain.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| -- Lifecycle (#52): don't spend ML recompute on off-market rows. | ||
| -- NOT IN (not = 'active') keeps pending_verify scoreable. | ||
| AND listing_status NOT IN ('sold','stale','rental_misfiled') |
There was a problem hiding this comment.
SQL Three-Valued Logic Bug & Pending Row Leak
Using NOT IN on a nullable column like listing_status introduces a classic SQL three-valued logic issue. If listing_status is NULL (which is common for legacy rows or before the lifecycle migration fully populates all rows), the expression listing_status NOT IN ('sold','stale','rental_misfiled') evaluates to UNKNOWN rather than TRUE. As a result, any rentable listing with a NULL status will be silently excluded from this query.
Because these rows are also not handled by the settled or noGeo queries, they will be permanently stuck in rent_calc_status = 'pending' and leak/accumulate in the database forever, potentially degrading performance over time.
Recommendations:
- Fix the Correctness Bug: Update the predicate to safely handle
NULLvalues by usingIS DISTINCT FROMor an explicitOR listing_status IS NULLcheck. - Settle Off-Market Rows: In a future PR, consider adding a query at the start of
drainBatchto transition off-market pending rows directly todonewith arent_model_versionlike'off_market_skip'so they do not remainpendingindefinitely.
| -- Lifecycle (#52): don't spend ML recompute on off-market rows. | |
| -- NOT IN (not = 'active') keeps pending_verify scoreable. | |
| AND listing_status NOT IN ('sold','stale','rental_misfiled') | |
| -- Lifecycle (#52): don't spend ML recompute on off-market rows. | |
| -- NOT IN (not = 'active') keeps pending_verify scoreable. | |
| AND (listing_status IS NULL OR listing_status NOT IN ('sold','stale','rental_misfiled')) |
| const wlRes = await client.query( | ||
| `SELECT user_id, name, query_json FROM watchlists`, | ||
| `SELECT user_id, name, query_json FROM watchlists ORDER BY id LIMIT $1`, | ||
| [watchlistBatch], | ||
| ); | ||
| watchlists = wlRes.rows; | ||
| if (watchlists.length >= watchlistBatch) { | ||
| log.warn( | ||
| { cap: watchlistBatch, fetched: watchlists.length }, | ||
| 'watchlists fetch hit ALERT_WATCHLIST_BATCH cap — some watchlists were not evaluated this tick', | ||
| ); | ||
| } |
There was a problem hiding this comment.
False Positive Warning on Exact Batch Size Match
Currently, the query uses LIMIT $1 with watchlistBatch, and the warning is triggered if watchlists.length >= watchlistBatch. If the total number of watchlists in the database is exactly equal to watchlistBatch, this warning will fire even though no watchlists were actually skipped. This can lead to false positive warnings in production logs and cause alert fatigue.
Recommendation:
To accurately detect if there are more watchlists remaining, query for watchlistBatch + 1 rows. If the returned length is greater than watchlistBatch, log the warning and slice the array back to watchlistBatch for processing.
Note: If you apply this suggestion, please also update the corresponding test in apps/worker/src/alerts.test.ts (line 320) to expect [2] instead of [1] for the query parameters.
const wlRes = await client.query(
"SELECT user_id, name, query_json FROM watchlists ORDER BY id LIMIT $1",
[watchlistBatch + 1],
);
const hasMore = wlRes.rows.length > watchlistBatch;
watchlists = hasMore ? wlRes.rows.slice(0, watchlistBatch) : wlRes.rows;
if (hasMore) {
log.warn(
{ cap: watchlistBatch },
"watchlists fetch hit ALERT_WATCHLIST_BATCH cap — some watchlists were not evaluated this tick",
);
}![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/worker/src/digest.ts (1)
463-503: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid survivorship bias in historical statistics.
Filtering historical cohorts by the current
listing_status = 'active'introduces survivorship bias. Listings created in previous time windows (e.g., 7–14 days ago) that were highly desirable and sold quickly will have transitioned tosoldand will be excluded from themedian_prev,median_now, andnew_countcalculations. This leaves only homes that failed to sell quickly, artificially skewing your medians and counts.Use
listing_status IN ('active', 'sold', 'pending_verify', 'stale')(excluding justrental_misfiled) to include all legitimately created listings that existed in that historical cohort.📈 Proposed fix for accurate historical cohorts
SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY price) FILTER ( - WHERE listing_type = 'for_sale' AND listing_status = 'active' AND created_at > now() - interval '7 days' + WHERE listing_type = 'for_sale' AND listing_status IN ('active', 'sold', 'pending_verify', 'stale') AND created_at > now() - interval '7 days' ) AS median_now, percentile_cont(0.5) WITHIN GROUP (ORDER BY price) FILTER ( - WHERE listing_type = 'for_sale' AND listing_status = 'active' + WHERE listing_type = 'for_sale' AND listing_status IN ('active', 'sold', 'pending_verify', 'stale') AND created_at > now() - interval '14 days' AND created_at <= now() - interval '7 days' ) AS median_prev FROM listings WHERE zip_code = $1 `, [zip] ); const countsRes = await client.query( ` SELECT (SELECT count(*)::int FROM listings - WHERE zip_code = $1 AND listing_type = 'for_sale' AND listing_status = 'active' + WHERE zip_code = $1 AND listing_type = 'for_sale' AND listing_status IN ('active', 'sold', 'pending_verify', 'stale') AND created_at > now() - interval '7 days') AS new_count, (SELECT count(*)::int FROM sold_listings WHERE zip_code = $1 AND sold_date > now() - interval '7 days') AS sold_count `, [zip] ); // Rent $/sqft trend from listings (estimated_rent / sqft). h3_market_stats // is keyed by H3 hex with no H3 index available in Postgres, so it can't be // joined to a ZIP here — see report note. We fall back to the listings // surface, which is ZIP-keyed and real. const rentRes = await client.query( ` SELECT percentile_cont(0.5) WITHIN GROUP (ORDER BY estimated_rent / NULLIF(sqft, 0)) FILTER ( - WHERE listing_type = 'for_sale' AND listing_status = 'active' AND created_at > now() - interval '7 days' + WHERE listing_type = 'for_sale' AND listing_status IN ('active', 'sold', 'pending_verify', 'stale') AND created_at > now() - interval '7 days' AND sqft > 0 AND estimated_rent > 0 ) AS rent_now, percentile_cont(0.5) WITHIN GROUP (ORDER BY estimated_rent / NULLIF(sqft, 0)) FILTER ( - WHERE listing_type = 'for_sale' AND listing_status = 'active' + WHERE listing_type = 'for_sale' AND listing_status IN ('active', 'sold', 'pending_verify', 'stale') AND created_at > now() - interval '14 days' AND created_at <= now() - interval '7 days' AND sqft > 0 AND estimated_rent > 0 ) AS rent_prev🤖 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 `@apps/worker/src/digest.ts` around lines 463 - 503, Update the historical cohort filters in the median queries and new_count calculation to use listing_status IN ('active', 'sold', 'pending_verify', 'stale') instead of requiring listing_status = 'active'. Apply this to median_prev, median_now, and the listings subquery for new_count, while continuing to exclude rental_misfiled listings.
🤖 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.
Outside diff comments:
In `@apps/worker/src/digest.ts`:
- Around line 463-503: Update the historical cohort filters in the median
queries and new_count calculation to use listing_status IN ('active', 'sold',
'pending_verify', 'stale') instead of requiring listing_status = 'active'. Apply
this to median_prev, median_now, and the listings subquery for new_count, while
continuing to exclude rental_misfiled listings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57c59d00-2901-420a-8f1f-8191f516e5b4
📒 Files selected for processing (8)
apps/worker/src/alerts.test.tsapps/worker/src/alerts.tsapps/worker/src/digest.tsapps/worker/src/env.tsapps/worker/src/lifecycle.test.tsapps/worker/src/rent-estimator.tsapps/worker/src/watchlist-alerts.tsops/systemd/oper-worker-alerts.service
1 participant
Fixes #41, #42, #43, #49, #52 — alert cap, memory headroom, index alignment, tick orchestration test, off-market worker filters. Worker suite 56/56 + typecheck clean.
Summary by CodeRabbit