Scope known_external_ids per-user in pipeline (bd-m6g) - #64
Merged
Conversation
The pipeline used to derive each scraper's known_external_ids from every row in flats, regardless of which user had actually had a matcher decision on them. Single-user mode tolerates this; Phase 5 multi-user does not — user A's scrape would mark a flat as "seen" and user B's paginating scraper could terminate early on flats user B has never been evaluated against. Change run_scrape_pass to filter the seen set via INNER JOIN matches on (flat_id, user_id). A brand-new user starts with an empty set so flats scraped by others still propagate through the full fetch/match path. user_id threads through from run_pipeline_once → run_pipeline_scrape → run_scrape_pass (kw-only, defaults to DEFAULT_USER_ID so single-user callers don't change). Index check: the auto-index for matches' UNIQUE(user_id, flat_id, profile_version_hash, decision) covers the (user_id, flat_id) prefix the join needs, and flats.id is the primary key — no new index required for current expected volumes.
There was a problem hiding this comment.
Pull request overview
This PR fixes a multi-user correctness issue in the scraping pipeline where a flat could be treated as “already seen” for the wrong user, causing paginating scrapers to terminate early. It scopes known_external_ids to the current (platform, user_id) by loading “seen” IDs from matches rather than all rows in flats.
Changes:
- Thread
user_idthroughrun_pipeline_once→run_pipeline_scrape→run_scrape_pass(kw-only, defaulting toDEFAULT_USER_ID). - Build per-platform
known_external_idsby joiningflatstomatchesand filtering bym.user_id. - Update/add tests to validate both the single-user default behavior and cross-user isolation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/flatpilot/pipeline.py |
Passes user_id through scrape orchestration and scopes known_external_ids via matches.user_id. |
tests/test_pipeline_known_ids.py |
Refactors seeding helpers and adds a new test proving other users’ decisions don’t leak into known_external_ids. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+166
to
+176
| # bd-m6g: scope "seen" to flats this user has already had a match | ||
| # decision on. A brand-new user starts with an empty set so flats | ||
| # other users scraped still get evaluated under this user's profile. | ||
| # Paginating scrapers may walk more pages on a new user's first | ||
| # pass — accepted trade-off for multi-user correctness. | ||
| known_external_ids = frozenset( | ||
| row[0] | ||
| for row in conn.execute( | ||
| "SELECT external_id FROM flats WHERE platform = ?", | ||
| (plat,), | ||
| "SELECT f.external_id FROM flats f " | ||
| "INNER JOIN matches m ON m.flat_id = f.id " | ||
| "WHERE f.platform = ? AND m.user_id = ?", |
Comment on lines
171
to
178
| known_external_ids = frozenset( | ||
| row[0] | ||
| for row in conn.execute( | ||
| "SELECT external_id FROM flats WHERE platform = ?", | ||
| (plat,), | ||
| "SELECT f.external_id FROM flats f " | ||
| "INNER JOIN matches m ON m.flat_id = f.id " | ||
| "WHERE f.platform = ? AND m.user_id = ?", | ||
| (plat, user_id), | ||
| ) |
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.
Summary
The scrape pass used to derive
known_external_idsfrom every row inflats, regardless of who had actually had a matcher decision on each flat. That's fine in single-user mode, but on Phase 5 multi-user, user A's scrape can mark a flat as "seen" viaINSERT OR IGNOREand user B's paginating scraper terminates early on flats user B has never been evaluated against.This PR scopes the seen set to
(platform, user_id):run_scrape_passnow joinsflatsagainstmatcheson(flat_id, user_id)so only flats the current user has personally had a decision on are considered "known."user_idthreads through fromrun_pipeline_once→run_pipeline_scrape→run_scrape_passas a kw-only arg defaulting toDEFAULT_USER_ID, so single-user CLI callers don't change.Indexes
The auto-index for
matches'UNIQUE(user_id, flat_id, profile_version_hash, decision)covers the leftmost(user_id, flat_id)prefix the join needs, andflats.idis the primary key. No new index required at current expected volumes; we can addmatches(user_id, flat_id)later if profiling shows pressure.Test plan
pytest tests/test_pipeline_known_ids.py— 2/2 pass.pytest— 566/566 pass, coverage 80%.ruff checkon touched files — clean.DEFAULT_USER_IDand assert the same per-platform sets it used to.run_scrape_pass(user_id=2), asserts user 2 sees only their own flat inknown_external_idsand not user 1's.