[review-stack 11/11] review-fixes - #16261
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c63ac9b6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| session.scalars( | ||
| sa.select(AssetContent).where( | ||
| AssetContent.is_missing.is_(False), | ||
| sa.or_(*(sql_path_under_prefix(AssetContent.path, prefix) for prefix in prefixes)), |
There was a problem hiding this comment.
On Windows, when a configured models/input/output root differs in capitalization from paths already stored in the catalog (for example, C:\Models versus c:\models), the previous Path.is_relative_to check matched them case-insensitively, whereas this binary SQL predicate omits every existing row. Consequently sync_root_safely neither reconciles those rows nor returns them in existing_paths, allowing the subsequent filesystem walk to insert duplicate live catalog entries for the same physical files. Use Windows-appropriate case comparison rather than changing this behavior as part of the SQL optimization.
AGENTS.md reference: AGENTS.md:L20-L21
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The stated harm doesn't hold: the duplicate-insert path exists identically before and after this change. The dedup gate is abs_p in existing_paths (scanner.py:276) — plain Python set membership, case-sensitive on every platform — and survivors carries os.path.abspath(content.path), which preserves stored casing (ntpath.normpath/abspath never case-fold; only normcase does, and nothing calls it). So under the old code on Windows, a case-different root meant is_relative_to matched and reconciled the row, but the walk still compared c:\models\... against {C:\Models\...}, missed, and inserted — and the live-path unique index is BINARY, so the differing-case strings don't collide. Duplicate either way; the old case-insensitive match never translated into case-insensitive dedup.
What this PR actually changes is narrower: with a case-different root, stale-cased rows are no longer visited by the reconcile loop (previously they were stat'd and kept current; now they sit live and stale). Neither version marks them missing.
Making sql_path_under_prefix case-insensitive would reintroduce the bug it exists to fix — its case-sensitivity is deliberate (see its docstring: LIKE's ASCII case-insensitivity previously let the temp wipe hard-delete rows under a case-different persistent directory) and matches its two existing production consumers. The root cause here is that the stored-path invariant (os.path.abspath at the write boundary) doesn't case-normalize on Windows; the durable fix is normcase at that boundary, which changes the invariant every path predicate rests on — out of scope for this layer. Filed as a follow-up covering the pre-existing duplicate-on-case-change behaviour together with the reconciliation delta noted above.
2 participants
Addressing @guill's feedback on #16081.
Only exit on a database file-lock timeout when assets are enabled (
main.py). The filelock message ("Could not acquire lock on database") had been folded into the same fatal branch as the driver message ("database is locked"), so a non-assets instance exited at startup whenever another instance held the DB file lock — previously it logged the guidance and kept running. The branches are now split: driver lock stays unconditionally fatal (master behavior), filelock logs the same guidance and exits only under--enable-assets. Three tests pin the matrix: non-assets filelock survives, assets filelock exits, driver lock always exits.Filter live contents in SQL (
scanner_changes.py).live_contents_under_prefixesselected every liveAssetContentrow and filtered by prefix in Python — a full-table load on the post-execution scan path. It now issues one query usingsql_path_under_prefix, the same component-bounded predicate the temp wipe and enrichment scan already use, and only materializes matching rows: on 20k live rows with 5k matching, 286 ms → 30 ms (in-memory SQLite, mean of 5). Eight tests were written against the Python implementation first and pass unchanged against the SQL version: prefix boundary (/a/bdoes not match/a/bc), case sensitivity, LIKE metacharacters treated literally,is_missingexclusion, multi-prefix OR, empty-prefix result, and a set-equivalence sweep againstis_path_under_prefixesover the shared path corpus. One divergence: the old check was case-insensitive on Windows (Path.is_relative_to); the SQL predicate is case-sensitive everywhere, matching the helper's existing consumers.Suite: 1365 passed, 1 skipped. Ruff clean.
main.pyapp/assets/scanner_changes.pytests-unit/app_test/test_db_init_locking.pytests-unit/assets_test/services/test_enrichment_predicate.pyStacked on #16218. Draft until the layers below land.