P1 transcoder: ack message only after successful metadata update #50

Activity

1. SlavaKuntsov

   added

   phase:backlogBacklogBacklog

   area:transcoderarea:transcoderarea:transcoder

   phase:17Phase 17 hardening and refactors: shared Go module, pipeline fixesPhase 17 hardening and refactors: shared Go module, pipeline fixes

   and removed

   phase:backlogBacklogBacklog

   on Sep 13, 2026
2. SlavaKuntsov

   self-assigned this

   on Sep 14, 2026
3. SlavaKuntsov commented on Sep 18, 2026

   

   SlavaKuntsov

   on Sep 18, 2026

   OwnerAuthor

   More actions

   Agent review — iteration 1

   Findings

   • [Medium] services/transcoder/app/consumer.py:832-837 — metadata-only failure triggers a full re-transcode/re-upload on retry (renditions already in MinIO). Correct but wasteful; bounded by MAX_RETRIES=3. Mitigated with a code comment; follow-up ticket candidate for a ready-PATCH-only retry path.
   • [Medium] services/transcoder/app/consumer.py:915-924 — retry-publish-failure branch was untested (part of the requested retry/DLQ surface). Fixed: added test_handle_message_publish_failure_marks_failed_and_dlqs.
   • [Low] services/transcoder/app/consumer.py:669-671 — poison messages (invalid JSON) are acked and dropped, never dead-lettered. Pre-existing, out of scope.
   • [Low] Publish-then-ack window can duplicate a message on crash. Pre-existing, bounded by x-retry-count.
   • [Low] docs/services-pipeline.md / services/transcoder/README.md don't document ack semantics at this level — nothing factually wrong after this change.

   Verdict

   ready for PR

   Verification

   • uv run --project services/transcoder pytest services/transcoder/tests -q — 25 passed
   • uv run --project services/auth pytest services/auth/tests -q — 18 passed
   • black --check / flake8 / mypy (transcoder) — clean
4. SlavaKuntsov commented on Sep 18, 2026

   

   SlavaKuntsov

   on Sep 18, 2026

   OwnerAuthor

   More actions

   Ready for review

   Implemented

   • process_message raises when the final update_status(video_id, "ready", ...) fails, so the consumer no longer acks a video that would stay «processing» forever; raw-object cleanup still happens only after metadata confirms ready.
   • on_message closure extracted to module-level handle_message (behavior-identical): ack only after process_message succeeds; on failure republish to video.uploaded.retry with incremented x-retry-count (TTL dead-letters back to the main queue), and after MAX_RETRIES mark failed + basic_nack(requeue=False) → DLX → DLQ.
   • Tests: metadata ready update failed → raise (no raw delete); handle_message ack-after-success; metadata failure → retry republish + ack, not silent ack; incoming retry count respected; retries exhausted → failed + nack to DLQ; retry publish failure → failed + nack to DLQ.

   Acceptance criteria

   • ack только после успешного update_status("ready")
   • иначе retry-очередь / basic_nack(requeue=False) → DLX/DLQ
   • тест «metadata упал → ретрай, не ack»

   Verification

   • uv run --project services/transcoder pytest services/transcoder/tests -q — 25 passed
   • uv run --project services/auth pytest services/auth/tests -q — 18 passed
   • black --check, flake8, mypy (transcoder) — clean
   • make e2e — not run (requires full docker compose stack)

   Review cycle

   • Iterations: 1
   • Remaining risks: metadata-only failure re-runs the full pipeline on retry (bounded by MAX_RETRIES, idempotent re-uploads) — follow-up candidate

   Suggested PR title

   fix(transcoder): ack only after successful metadata ready update
5. SlavaKuntsov commented on Sep 18, 2026

   

   SlavaKuntsov

   on Sep 18, 2026

   OwnerAuthor

   More actions

   Pull request opened

   #78 — will auto-close via Closes #50 on merge.

   Project status: In review (synced after PR creation).

   Verification

   • uv run --project services/transcoder pytest services/transcoder/tests -q — 25 passed
   • uv run --project services/auth pytest services/auth/tests -q — 18 passed
   • black --check, flake8, mypy (transcoder) — clean

   Remaining risks

   • Metadata-only failure re-runs the full pipeline on retry (bounded by MAX_RETRIES=3, re-uploads idempotent) — follow-up candidate.
   • make e2e not run (requires full docker compose stack).
6. SlavaKuntsov

   closed this as completedin #78on Sep 18, 2026