fix: record upload.completed only once the job has actually finished - #447
Conversation
record_activity(db, "upload", "completed", ...) fired straight after the indexing commit, but two steps still ran after it. The near-duplicate check has its own try/except and is safe; detect_and_store_faces does not. When face detection raises, the outer handler appends upload.failed for the same media, and the activity feed carries both a completed and a failed row for one job. The record now happens immediately before each successful return, once every remaining step has survived. There are two such exits -- the metadata-only early return when runtime.applied_mode is "disabled", and the normal one at the end -- so it goes in both, via a small local helper rather than a copied call. media.status is deliberately left alone. It is committed as "indexed" and then set to "failed" on this path, which is a legitimate transition of a mutable field; the bug is that the activity log is append-only history, where two contradictory rows cannot both be true. tests/test_upload_activity_record.py makes detect_and_store_faces raise and asserts exactly one upload.* row, plus two controls: a clean run and the disabled-mode return, each asserting a single "completed". Against jobs.py before this change the first fails with ['completed', 'failed'] == ['failed'], which is the reported symptom exactly. Backend: ruff check clean, ruff format clean, and 56 passed across test_upload_activity_record, test_activity, test_thumbnails, test_reprocess and test_runtime_profile. Closes Abhash-Chakraborty#410 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Context Summary
Suggested issue links
Use |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe upload worker records ChangesUpload activity lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
ApprovabilityVerdict: Needs human review Straightforward bug fix with good test coverage, but the author does not own any of the changed files. The designated code owner (Abhash-Chakraborty) should review these changes to backend worker code. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an activity-log correctness bug in the upload worker by ensuring upload.completed is recorded only after all remaining processing steps have finished successfully, preventing a single job from producing both upload.completed and upload.failed rows.
Changes:
- Move
record_activity(..., "upload", "completed", ...)to run immediately before each successful return path inanalyze_image. - Add a small local helper to avoid duplicating the success-recording call across the two success exits.
- Add focused regression tests covering success, disabled/metadata-only success, and face-detection failure.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/src/find_api/workers/jobs.py | Delays upload.completed activity recording until just before successful returns to avoid contradictory completed + failed history rows. |
| backend/tests/test_upload_activity_record.py | Adds regression tests asserting exactly one upload.* activity row is written per job across key success/failure paths. |
Suppressed comments (2)
backend/tests/test_upload_activity_record.py:150
- This test patches
generate_hybrid_embeddingeven though disabled mode won’t call it; returning the string "[]" is also not type-accurate and can cause confusing failures if the control flow changes. Preferreturn_value=Nonehere as well for consistency.
patch(
"find_api.workers.processors.generate_hybrid_embedding", return_value="[]"
),
backend/tests/test_upload_activity_record.py:115
generate_hybrid_embeddingis mocked to return the string "[]", which is the wrong type and can cause the near-duplicate pgvector query to run (and fail) under the SQLite test DB. Mock it asNoneto keep this test isolated from near-duplicate detection.
patch(
"find_api.workers.processors.generate_hybrid_embedding", return_value="[]"
),
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
From CodeRabbit's review of Abhash-Chakraborty#447. `generate_hybrid_embedding` was mocked to return "[]", a string rather than a vector, which left `media.vector` truthy and non-None. The near-duplicate branch guards on `is not None`, so each test was entering the pgvector path on SQLite and relying on its try/except to swallow the resulting error. None matches what the guard is actually testing for and skips that branch outright, so these tests exercise the activity-log behaviour they are named for and nothing else.
Abhash-Chakraborty
added
gssoc26
Abhash-Chakraborty
left a comment
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Approved. Verified the parts you flagged as untested.
The fix is correct and the test proves it. I reverted jobs.py to canary and re-ran your suite — test_face_detection_failure_records_only_upload_failed fails with exactly the symptom in the issue:
AssertionError: assert ['completed', 'failed'] == ['failed']
Confirmed independently that analyze_image has precisely the two success exits you identified, and both are covered. Also checked record_activity commits internally — worth knowing, because the row would otherwise never persist now that nothing commits after it.
On leaving media.status alone: agreed, and the reasoning is the right one. The failure path sets status = "failed" and commits, so the field ends on a single correct value. The activity log is append-only history, where two contradictory rows both survive. Those are genuinely different problems and only the second is this issue. No need to widen.
Ran what you could not. Full backend suite: 794 passed, 7 skipped. ruff check and ruff format --check clean.
One commit pushed, from CodeRabbit's note: the mocked generate_hybrid_embedding returned the string "[]", which is truthy and non-None, so every test was entering the near-duplicate branch on SQLite and relying on its try/except to swallow the error. None matches what if media.vector is not None actually tests for and keeps these tests on the activity-log behaviour they are named for.
Thanks for the honest 'Tested / not tested' section — it made this quick to verify rather than guess at.
fa4cb61
into
Abhash-Chakraborty:canary
Closes #410.
The problem
record_activity(db, "upload", "completed", ...)fired straight after the indexing commit, but two steps still ran after it:try/except, so safedetect_and_store_faces(...)— not wrappedWhen face detection raises, the outer
except Exceptionhandler appendsupload.failedfor the same media, and the feed carries both acompletedand afailedrow for one job.The change
The record now happens immediately before each successful return, once every remaining step has survived — which is what the issue suggests. There are two success exits, not one: the metadata-only early return when
runtime.applied_mode == "disabled", and the normal one at the end. Both get it, via a small local helper rather than a copied call.media.statusis deliberately left alone. It is committed asindexedand then set tofailedon this path, but that is a legitimate transition of a mutable field — the bug is specifically that the activity log is append-only history, where two contradictory rows cannot both be true. Happy to widen if you'd rather it also hold back theindexedcommit.Tests
backend/tests/test_upload_activity_record.py— three cases:detect_and_store_facesraises → exactly oneupload.*row, and it isfailedcompleted(the control; moving the record must not drop it)completed(the other success exit)Against
jobs.pybefore this change, the first fails with exactly the reported symptom:Tested / not tested
Tested:
ruff check .clean,ruff format --check .clean, and 56 passed acrosstest_upload_activity_record,test_activity,test_thumbnails,test_reprocess,test_runtime_profile.Not tested: I did not run the full backend suite or the Docker stack — several suites need services (Postgres/Redis/MinIO) I don't have up locally, and the ML extras aren't installed. No manual upload/gallery/clustering verification for the same reason. The change is confined to where one already-existing log row is written, with no change to upload or indexing behaviour.
Branched from and targeting
canary, per AGENTS.md.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests