Repository navigation
Conversation
created: searches filtered created_at_unix against UTC-midnight bounds, so a photo stored with a real timezone offset (e.g. XMP CreateDate +09:00) landed on the UTC calendar day and was missed or misfiled by up to the offset amount. The metadata displays the local date, so the filter now compares created_at_unix + created_at_tz_offset*60 — the same value the created_at display column uses — both in the SQL filter and in the Go wildcard post-filter. Reproduced end-to-end with a photo stamped 2025-06-01 03:30+09:00 (2025-05-31 18:30 UTC): created:2025-06-01 missed it and created:2025-05-31 wrongly returned it. Covered by created_search_test.go.
dedup: only suppressed a row when it was similar to the immediately previously kept row, so two near-duplicates separated by an unrelated photo both survived — contradicting the documented guarantee to keep only photos less than the threshold similar to each other. Track the embeddings of every kept row and suppress any row similar to any of them. Covered by dedup_search_test.go (non-adjacent near-duplicates, synthetic CLIP embeddings).
… -similarity text, img: and face: queries are scored in Go, but the SQL LIMIT was applied before scoring, so threshold (t:) and dedup: filters only ever saw the first N rows in the SQL order — matches beyond the window were silently dropped, and 'img:5 t:0.8 limit 5' could return 0 rows. - image.listWithPrefixIds: disable the SQL LIMIT whenever an embedding is involved and enforce it in Go after threshold/dedup filtering - collection.Search: an explicit sort still wins, but embedding queries now default to -similarity (best matches first) instead of -date, which is what the search_photos description always promised - search_photos description: document the per-query-kind default sort - tests: pin the threshold/limit interaction at the DB level and the default-sort resolution at the collection level
…mit) and #200 (instant skew)
SmilyOrg
changed the title
… the pool while the writePendingInfosSqlite goroutine could still be flushing a batch, which reads dirUpdateFuncs (data race caught in TestListThresholdLimitSpansFullSet). Close now signals the writer by closing pending, waits for it to drain and exit, then touches the shared state.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The MCP description gives contradictory defaults for image and face queries.
1 open finding
What changed in this PR
Fixes timezone-aware date matching and embedding search ranking/limits in search_photos.
Changes:
- Matches
created:filters against local wall-clock dates. - Applies embedding limits after scoring and defaults embedding searches to similarity order.
- Adds regression tests and safe database-writer shutdown synchronization.
| File | Description |
|---|---|
created_search_test.go |
Tests timezone-aware date filtering. |
similarity_search_test.go |
Tests scoring before result limits. |
internal/image/database.go |
Fixes filtering, limiting, and writer shutdown. |
internal/collection/search.go |
Selects query-aware default ordering. |
internal/collection/search_test.go |
Tests default-order selection. |
internal/mcp/mcp.go |
Updates search tool guidance. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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
Two fixes for the
search_photosMCP tool, each found while testing end-to-end and verified with unit tests plus an e2e run:created:matched the UTC instant, not the stored local date (e67d817).Photos with a negative UTC offset were missed by date filters even though every display surface showed the local date. The Go post-filter in
listWithPrefixIdscompared against baretime.Unix(unix, 0)while the SQL pre-filter usedunix + tz_offset*60. Now both evaluate the same local wall-clock instant.Embedding queries: SQL
LIMITapplied before Go scoring (a7068aa).The similarity threshold and image/face scoring run in Go, but the SQL
LIMITclipped the row stream first, so only the firstlimitrows were ever scored and the best matches beyond that were silently discarded. NowgoScoresqueries disable the SQL limit and enforce it in Go after threshold/dedup filtering; embedding queries without an explicitsortdefault to-similarity(matching the tool description), and thesearch_photosdescription documents the per-query-kind default sort.Also
search_photosfilename:example, which claimed the wrong default order (eee5b01) — it now demonstratessort:+date.dedup:comparing against all kept rows instead of only the last one — into #202 for independent review (reverted here in 35a0547; theembeddingBytestest helper moves intosimilarity_search_test.go, which uses it). Merge this PR before fix: compare dedup: against all kept rows, not just the last one #202 to keep that diff clean.Remaining open issues (filed, not fixed here)
dedup:without embeddings + limit: same "limit before filter" defect; thegoScoresgate needs aDeduplicate.Presentterm (one-line extension + test).created_at_unixskew: same camera stored on two different instant bases (local-as-UTC vs true UTC); a scan-time normalization problem, deliberately out of scope of these query fixes.Test plan
created_search_test.go,similarity_search_test.go,internal/collection/search_test.go(TestPrimaryOrderFor).img:6 t:0.3with hand-seeded float16 embeddings — limit 2 returns the top two matches (sim 1.0, 0.9) instead of the two newest rows;sort:+datelimit 4 returns all four threshold-passing rows across the date range.go test . ./internal/...passes on the branch.