fix(sync): terminalize cloud_tasks jobs whose dispatch was lost before any attempt#10098
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Thanks for the careful write-up and tests here. This is a positive signal from automation: the change is narrow, preserves the inline-queued and retry-backoff contracts, reuses the existing lease-owned stale finalizer, and adds focused coverage for the never-dispatched I’m not formally approving because this touches sync job terminalization/retry recovery for retained user audio and changes the timing/semantics of when a queued Cloud Tasks job becomes terminal. A human maintainer should still sign off on the operational threshold and recovery behavior before merge. No supply-chain or obvious malicious behavior found in the diff. The relevant backend/Cloud Tasks checks are passing on this head. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
kodjima33
left a comment
There was a problem hiding this comment.
Reviewed by automated PR reviewer — approving; holding merge (see notes).
|
@Git-on-my-level @undivisible This PR is fully approved. I've pushed a fix for the pre-existing Backend unit suite timebomb that was failing CI (the short-term lifecycle tests hardcoded June 20, 2026 and hit +30 days expiry validation failures today). CI should be green shortly. Could you please merge when you have a chance? |
|
Thanks for the review pass. Backend unit suite is green on this head; ready for a maintainer's final call on the queued-dispatch stale threshold (30m) whenever there's bandwidth. |
|
@kodjima33 approved and green — all checks pass (Backend unit suite, Hermetic E2E, Sync Cloud Tasks gauntlet). Just needs a maintainer sign-off on the 30-minute queued-dispatch stale threshold, which is a single constant. Ready to merge whenever you lift the hold. Thanks! |
|
Rechecked the current head after the follow-up test-only commit. Still a positive signal from automation: the sync change stays narrow, keeps inline queued jobs and Cloud Tasks retry backoff out of poll-terminalization, uses the existing lease-owned stale finalizer, and now has focused coverage for the I’m still not formally approving because this is a security/user-data recovery surface: it changes when retained-audio sync jobs become terminal and depends on the 30-minute lost-dispatch threshold being the right operational tradeoff. Human maintainer sign-off should remain required before merge. No supply-chain or obvious malicious behavior found in the current diff; relevant backend/Cloud Tasks checks are green. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
|
@Git-on-my-level need human response — requesting David's merge sign-off. Approved by @kodjima33, all checks green (Backend unit suite, Hermetic E2E, Sync Cloud Tasks gauntlet), no conflicts. The only open item your reviews flagged is the operational call on the 30-minute lost-dispatch stale threshold — it's a single constant ( |
…10119) * fix(sync): never merge synced audio into a soft-deleted conversation get_closest_conversation_to_timestamps matched any conversation in its ±2-minute window — including deleted: True tombstones — so offline audio recorded near a conversation the user later deleted merged its segments into the tombstone and vanished: uploads reported success, the job completed, and no visible conversation ever appeared (#10033's never-creates-a-conversation symptom). The auto-sync target-attach path had the same hole through its direct get_conversation fetch. The closest-match choice is now a pure selector (select_closest_conversation) over eligible merge targets, with one shared predicate (eligible_merge_target) excluding soft-deleted rows; the target-attach path consults the same predicate and falls back to the filtered timestamp lookup. A window containing only tombstones now creates a fresh conversation instead of feeding the dead one. Discarded rows stay eligible — the merge path already reprocesses and revives them. Regression tests (fail on old code — the selector did not exist and the inline loop accepted tombstones): deleted-only window yields None, a closer deleted row loses to a live one, boundary-distance choice and the predicate matrix, plus a labeled source tripwire that the target-attach path gates through the shared predicate. Fixes the never-creates-a-conversation symptom of #10033 (server side); the stuck-queue symptom is #10098. Failure-Class: none Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(sync): behavior test for target attach fallback & fix test timebomb * test(sync): allowlist the target-attach behavior test for the fast-unit duration guard The behavior test added for the #10119 review (drives the real process_segment target-attach fallback) is the sole heavy test in its file, so it amortizes the sync-pipeline import graph into its call phase and exceeds the 0.12s fast-unit CPU budget — the same file-isolation structural cost the allowlist already grandfathers for test_sync_silent_failure.py::TestProcessSegmentReal. Add its node id alongside, per the allowlist's documented purpose. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ee1ad88 to
5c5bba9
Compare
|
@Git-on-my-level need human response — this PR is approved, rebased onto latest |
…e any attempt
An uploaded batch creates the Redis sync job ('queued') and then enqueues
the named Cloud Task. When that dispatch is lost — both enqueue attempts
fail (the enqueue_uncertain path deliberately returns 202 and never
starts inline work), or the queue is purged — nothing ever transitions
the job again: is_sync_job_stale only considered 'processing', so the
poll-route stale finalizer never fired and the client polled
'Uploaded · processing on Omi' until the 24h JOB_TTL expiry (BasedHardware#10033).
A cloud_tasks job still 'queued' with no attempt ever started
(started_at/attempt unset) now goes stale after 30 minutes and the
existing owned stale finalizer terminalizes it with a distinct
'sync_dispatch_lost' code, releasing the content claim. The client's
existing failed-job handling then reverts the WALs to miss and
re-uploads from the retained local files.
The two neighboring contracts are preserved exactly: inline queued jobs
are never stale at any age (BasedHardware#7469 pool saturation), and a worker
re-queue between Cloud Tasks retries (mark_job_queued_for_retry, which
sets attempt/started_at) is never poll-terminalized however long its
backoff. A task that fires after terminalization already acks and drops
via the worker's terminal-status guard.
Regression tests: the staleness matrix (never-dispatched stale;
under-threshold, inline, and pending-retry not stale) fails on the old
code, and a route-level test proves the queued finalization publishes
'sync_dispatch_lost'.
Fixes the stuck-queue symptom of BasedHardware#10033 (server side).
Failure-Class: none
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…spatch runbook The enqueue-uncertainty runbook described the queued job as pollable with only TTL-expiry recovery; the new 30-minute sync_dispatch_lost terminalization changes what an operator observes and should check. Docs move with the code (DoD rule 6) — belongs in this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5c5bba9 to
ae938fc
Compare
|
@Git-on-my-level need human response — batching these so they don't need chasing one at a time. Six PRs are approved, green, and mergeable right now (all verified
Regular merge, no squash, per the contributing guide. #10060 and #10270 are also approved; both were just rebased onto latest No urgency on any of these — posting once as a batch rather than pinging each thread. Happy to rebase any that go stale. |
Summary
cloud_taskssync job whose named-task dispatch was lost before any worker attempt now goes stale after 30 minutes and is terminalized by the existing owned stale finalizer with a distinctsync_dispatch_lostcode, instead of sittingqueueduntil the 24hJOB_TTLexpiry while the app showsUploaded · processing on Omithe whole time.missand re-upload from the retained local files on the next sync pass — no app change needed.Uploaded · processing on Omiitems match this envelope exactly (stuck < 24h TTL, then eventual 404 recovery).Root cause
The upload route creates the Redis job (
status='queued',dispatch_mode='cloud_tasks') before enqueueing the named Cloud Task (deliberately, so an ambiguous enqueue can never start an unfenced inline twin). When the dispatch is lost — both enqueue attempts fail (theenqueue_uncertainpath returns 202 and preserves all durable inputs, correctly refusing inline work), or the task is purged — nothing ever transitions the job again:is_sync_job_staleonly consideredstatus == 'processing'("queued jobs are intentionally never stale"), so the poll-route stale finalizer never fired.notFoundrecovery only triggers at job-TTL expiry, 24 hours later.The stuck job is a zombie the whole window: client polls every pass, server truthfully answers
queued, nobody owns the exit.Failure class
Failure-Class: none
Closest is FC-session-scoped-durable-work (durable work must not inherit dispatcher lifetime), but that class covers work cancelled with its session; this is durable work whose dispatch signal was lost with no owner for the never-started case. Not stretching the definition.
What is deliberately preserved
dispatch_mode == 'cloud_tasks'.mark_job_queued_for_retryre-queues between Cloud Tasks attempts and relies on queued jobs being poll-safe. Those jobs carryattempt/started_at; the new staleness requires both unset (never-attempted), so a pending retry is never flipped terminal however long its backoff (GCP default max backoff is 1h — a naive age check would have broken this).try_acquire_sync_job_run_lock→ re-read →finalize_sync_job_failure_now);fenced_mark_job_failedalready allowsqueued → failed, and a polling read still never publishes a terminal state without the lease.Product invariants affected
none (verified with
scripts/pr-preflight --suggest)Validation
sync_dispatch_lostfinalization test → 2 failed; with the fix → both pass.attemptset) at any age → not.tests/unit/test_sync_v2.py+tests/unit/test_sync_cloud_tasks.py→ 82 passed.make preflight(local lane, this PR body) → all selected checks pass.blackclean on all changed files.backend/routers/sync.pyfrozen-size baseline raised 2024 → 2026 (+2 for the queued-dispatch-lost finalization path) with justification, per the ratchet's documented raise path.Not exercised live: a real Cloud Tasks queue purge against production (no live services here). The enqueue-uncertain code path that produces this state is deterministic in source; the tests drive the staleness decision and the route finalization directly.
Out of scope, named
record_fallbacktelemetry for the terminalization:finalize_sync_job_failure_nowalready emits the structured failure log + outcome metric; adding a fallback counter on top would be a duplicate one-off.