Repository navigation
harden(pusher): persist playback projection rebuild debt - #24725
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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 17 files
Confidence score: 2/5
backend/routers/pusher.pyignores a failed handoff result, so projection debt can be lost when the marker write fails. Check the result and retain ownership for a later cleanup attempt.config/feature-flags.yamlwires the kill switch only to the repair consumer, so disabling it will not stop production Pusher handoffs. Add the flag to the Pusher runtime and chart declaration.backend/scripts/record_seal_repair_job.pycan swallow a rebuild crash and still exit 0, making a failed rebuild look successful. Report the failure in the exit status or a metric.backend/routers/pusher.pywaits for the marker write before recording the custody hold, which can stall the drain loop for up to 5 seconds during an outage. Record the hold before the write.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/tests/unit/test_pusher_audio_projection.py">
<violation number="1" location="backend/tests/unit/test_pusher_audio_projection.py:650">
P2: This test does not verify the debt-retention behavior named by the test, giving false confidence around the new durable handoff. Exercise `utils.record_projection_rebuild.run` with a failing rebuild and assert the marker is not deleted, or rename this case to describe only the no-write rebuild failure.</violation>
</file>
<file name="backend/testing/lifecycle_sim/test_scenarios.py">
<violation number="1" location="backend/testing/lifecycle_sim/test_scenarios.py:342">
P3: The `eviction` parameter does not simulate socket eviction; it only directly calls `persist_pending()`, so the scenario would remain green if the router stopped invoking handoff during cleanup. Drive the actual cleanup/eviction path or remove the misleading parameter and cover that integration separately.</violation>
</file>
<file name="backend/scripts/record_seal_repair_job.py">
<violation number="1" location="backend/scripts/record_seal_repair_job.py:50">
P2: A rebuild crash is swallowed and the job still exits 0, so a fully failing rebuild looks like a successful run. Consider recording the failure in the exit status or in a metric the way the repair path does, so debt cannot grow silently.</violation>
</file>
<file name="backend/utils/batch_pipeline/record_projection_rebuild.py">
<violation number="1" location="backend/utils/batch_pipeline/record_projection_rebuild.py:15">
P3: `projection_owner_gone` builds the Firestore document path directly in `utils/`. Use `conversations_db.get_conversation_raw_snapshot(..., field_paths=['deleted']) is None` instead; that helper already returns None for a missing document and keeps persistence in `database/`.</violation>
</file>
<file name="backend/routers/pusher.py">
<violation number="1" location="backend/routers/pusher.py:549">
P2: Call `record_barrier.fail_recording` before the marker write. Right now the custody hold waits on a Firestore write that can take up to 5 s during the outage this change targets, and the drain loop stalls with it.</violation>
<violation number="2" location="backend/routers/pusher.py:549">
P1: This handoff drops projection debt when the marker write fails because the returned `False` is ignored. Check the handoff result and retain the owner for a later cleanup attempt, including when the bounded close cache is full.</violation>
</file>
<file name="config/feature-flags.yaml">
<violation number="1" location="config/feature-flags.yaml:2">
P2: This ops kill switch is wired only to the repair consumer, not to production Pusher handoff. Add the flag to the Pusher runtime/chart declaration so the documented rollback can disable both sides; otherwise disabling repair leaves new markers accumulating.</violation>
</file>
<file name="backend/utils/record_projection_rebuild.py">
<violation number="1" location="backend/utils/record_projection_rebuild.py:22">
P2: This ordered query permanently skips malformed markers that lack `requested_at`, so they cannot be rotated or repaired despite the pass claiming to handle malformed rows. Add a bounded cleanup path that also discovers missing-field markers, or backfill their ordering field before relying on this query.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| logger.warning('Durable audio projection conflict; archive receipt retained') | ||
| except InvalidArgument as exc: | ||
| audio_projection.inventory.pop(conv_id, None) | ||
| await audio_projection.defer_rebuild(conv_id) |
There was a problem hiding this comment.
P1: This handoff drops projection debt when the marker write fails because the returned False is ignored. Check the handoff result and retain the owner for a later cleanup attempt, including when the bounded close cache is full.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/routers/pusher.py, line 549:
<comment>This handoff drops projection debt when the marker write fails because the returned `False` is ignored. Check the handoff result and retain the owner for a later cleanup attempt, including when the bounded close cache is full.</comment>
<file context>
@@ -545,6 +546,7 @@ async def flush_refs(conv_id: str) -> None:
logger.warning('Durable audio projection conflict; archive receipt retained')
except InvalidArgument as exc:
audio_projection.inventory.pop(conv_id, None)
+ await audio_projection.defer_rebuild(conv_id)
# Custody and its generation-pinned manifest remain durable.
# Retrying a deterministic projection rejection cannot repair
</file context>
| assert not world.lists and not world.writes | ||
|
|
||
|
|
||
| def test_rebuild_retains_debt_on_incomplete_raw_index(world, monkeypatch): |
There was a problem hiding this comment.
P2: This test does not verify the debt-retention behavior named by the test, giving false confidence around the new durable handoff. Exercise utils.record_projection_rebuild.run with a failing rebuild and assert the marker is not deleted, or rename this case to describe only the no-write rebuild failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/tests/unit/test_pusher_audio_projection.py, line 650:
<comment>This test does not verify the debt-retention behavior named by the test, giving false confidence around the new durable handoff. Exercise `utils.record_projection_rebuild.run` with a failing rebuild and assert the marker is not deleted, or rename this case to describe only the no-write rebuild failure.</comment>
<file context>
@@ -596,3 +600,82 @@ def test_closing_flush_counts_as_final_discovery_without_second_listing(world):
+ assert not world.lists and not world.writes
+
+
+def test_rebuild_retains_debt_on_incomplete_raw_index(world, monkeypatch):
+ world.add(100)
+ monkeypatch.setattr(full_projection, 'projection_owner_gone', lambda *_: False)
</file context>
| assert await cache.finish_pending(sim.cid, set(), set()) is False | ||
| assert await cache.finish_pending(sim.cid, set(), set()) is True | ||
| else: | ||
| await cache.persist_pending() |
There was a problem hiding this comment.
P3: The eviction parameter does not simulate socket eviction; it only directly calls persist_pending(), so the scenario would remain green if the router stopped invoking handoff during cleanup. Drive the actual cleanup/eviction path or remove the misleading parameter and cover that integration separately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/testing/lifecycle_sim/test_scenarios.py, line 342:
<comment>The `eviction` parameter does not simulate socket eviction; it only directly calls `persist_pending()`, so the scenario would remain green if the router stopped invoking handoff during cleanup. Drive the actual cleanup/eviction path or remove the misleading parameter and cover that integration separately.</comment>
<file context>
@@ -290,3 +291,100 @@ async def test_server_batch_completion_ordering(lifecycle_sim, closed):
+ assert await cache.finish_pending(sim.cid, set(), set()) is False
+ assert await cache.finish_pending(sim.cid, set(), set()) is True
+ else:
+ await cache.persist_pending()
+ assert marker.get().exists
+ tombstone = sim.ref.parent.parent.collection('deleted_conversations').document(sim.cid)
</file context>
| def projection_owner_gone(uid: str, conversation_id: str) -> bool: | ||
| """Deletion intent and hard absence; the cold commit rechecks privacy atomically.""" | ||
| if conversation_tombstones.is_deleted(uid, conversation_id): | ||
| return True | ||
| doc = ( | ||
| get_firestore_client() | ||
| .collection('users') | ||
| .document(uid) | ||
| .collection('conversations') | ||
| .document(conversation_id) | ||
| .get() | ||
| ) |
There was a problem hiding this comment.
P3: projection_owner_gone builds the Firestore document path directly in utils/. Use conversations_db.get_conversation_raw_snapshot(..., field_paths=['deleted']) is None instead; that helper already returns None for a missing document and keeps persistence in database/.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At backend/utils/batch_pipeline/record_projection_rebuild.py, line 15:
<comment>`projection_owner_gone` builds the Firestore document path directly in `utils/`. Use `conversations_db.get_conversation_raw_snapshot(..., field_paths=['deleted']) is None` instead; that helper already returns None for a missing document and keeps persistence in `database/`.</comment>
<file context>
@@ -0,0 +1,84 @@
+ return os.getenv('PUSHER_PROJECTION_DEBT_DURABLE_ENABLED', '1').strip().lower() not in {'0', 'false', 'off'}
+
+
+def projection_owner_gone(uid: str, conversation_id: str) -> bool:
+ """Deletion intent and hard absence; the cold commit rechecks privacy atomically."""
+ if conversation_tombstones.is_deleted(uid, conversation_id):
</file context>
| def projection_owner_gone(uid: str, conversation_id: str) -> bool: | |
| """Deletion intent and hard absence; the cold commit rechecks privacy atomically.""" | |
| if conversation_tombstones.is_deleted(uid, conversation_id): | |
| return True | |
| doc = ( | |
| get_firestore_client() | |
| .collection('users') | |
| .document(uid) | |
| .collection('conversations') | |
| .document(conversation_id) | |
| .get() | |
| ) | |
| def projection_owner_gone(uid: str, conversation_id: str) -> bool: | |
| if conversation_tombstones.is_deleted(uid, conversation_id): | |
| return True | |
| return conversations_db.get_conversation_raw_snapshot(uid, conversation_id, field_paths=['deleted']) is None |
![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: 907f3cde4d
ℹ️ 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".
| audio_projection.inventory.pop(conv_id, None) | ||
| # The close-debt cache may be full. Persist terminal projection debt | ||
| # by its exact owner rather than relying on that bounded inventory. | ||
| await audio_projection.defer_rebuild(conv_id) |
There was a problem hiding this comment.
If a terminal batch failure occurs before audio_projection.refresh() runs (for example, during PCM read-back or upload), the conversation has never been added to close_debt. This call ignores the False returned when marker persistence fails, then drops the batch's retry state. Because defer_rebuild() does not register an in-memory obligation either, socket cleanup's persist_pending() will not retry that conversation, even if Firestore has recovered. The same gap affects conversations excluded by the close-debt capacity limit. Preserve an exact-owner retry obligation or handle the failed handoff before retiring the batch so a temporary marker-write failure does not permanently strand playback metadata.
Useful? React with 👍 / 👎.
| except BatchPrivacyBlocked: | ||
| return False # Revocation clears only the maintenance job, never content. |
There was a problem hiding this comment.
When this pass overlaps a standard-to-enhanced protection migration, assert_batch_write_allowed() raises BatchPrivacyBlocked('data_protection_changed') because the user has a nonempty migration_status (database/batch_pipeline_privacy.py). Returning False here makes the consumer classify the owner as gone and delete its rebuild marker. However, users.finalize_migration() subsequently removes that temporary status, and migrate_conversations_level_batch() preserves the conversation's playback metadata and stored audio, so the outstanding rebuild remains necessary after migration. Distinguish temporary privacy holds from permanently revoked owners and retain the marker without writing playback metadata while the hold is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0514ef8524
ℹ️ 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".
|
|
||
| def apply_projection(uid, cid, payload, *, expected_audio_files, firestore_client=None): | ||
| """Cold repair only: deletion intent and projection CAS share the commit.""" | ||
| from database.conversations import AudioFilesChanged, audio_files_revision, retain_aliases |
There was a problem hiding this comment.
The import inside apply_projection() violates the explicit import rule in backend/AGENTS.md: all imports must be at module scope, never inside functions. Move this import to the module header; the newly added function-local imports in utils/record_projection_rebuild.py::run_job() and scripts/record_seal_repair_job.py::main() need the same correction.
AGENTS.md reference: backend/AGENTS.md:L166-L168
Useful? React with 👍 / 👎.
| image: "{{ required "image.repository required" .Values.image.repository }}@{{ required "image.digest required" .Values.image.digest }}" | ||
| command: [python, scripts/record_seal_repair_job.py] | ||
| env: | ||
| - {name: PUSHER_PROJECTION_DEBT_DURABLE_ENABLED, value: {{ .Values.recordSealRepair.projectionDebtDurableEnabled | quote }}} |
There was a problem hiding this comment.
When regional chunk routing is enabled, the new repair pass cannot discover those chunks: this CronJob has no BUCKET_AUDIO_DERIVED binding or envFrom, although the worker Deployment binds it from .Values.audioDerivedBucket. storage.list_audio_chunks() only lists the regional bucket when that environment variable is nonempty (backend/utils/other/storage.py:1205–1211). For legacy recordings, raw packs cannot compensate because raw_audio_read.list_packs() excludes arrival_estimate captures (backend/utils/other/raw_audio_read.py:64–68). Consequently, a conversation containing both legacy-bucket and regional chunks can have its playback metadata replaced with only the legacy-bucket subset, after which the marker is retired as successfully rebuilt. Bind the same derived bucket into the repair CronJob before enabling its new consumer.
AGENTS.md reference: backend/AGENTS.md:L158-L158
Useful? React with 👍 / 👎.
Problem
Pusher drops its in-memory playback projection close obligation when the bounded close rebuild retries are exhausted or the socket is evicted. The retained audio remains safe, but no durable owner retries the stale playback metadata after the socket disappears.
What changes
record_projection_rebuild_duemarkers inrecord_projection_rebuild_jobs, keyed by user/conversation, on exhausted retries and socket cleanup. No conversation marker and no additional ordinary-flush reads or writes.retired_no_objects. Retirement rereads the marker version and the conversation producer map in one Firestore transaction. Only an explicit empty map permits deletion; live, unknown or malformed producer fences retain and rotate debt, including under collection pressure. The pressure-blocked gauge and aggregate fenced count surface passes that cannot evict empty markers. No audio is deleted.audio_files. Keep preview read-only and retain debt on a broken raw-pack index.The runtime kill-switch parser lives in
config/batch_pipeline_projection_rebuild.py; metadata rebuild stages do not read process policy from the environment.Flag:
PUSHER_PROJECTION_DEBT_DURABLE_ENABLED, default ON.0,false, oroffdisables marker handoff and consumption. Rollback: setrecordSealRepair.projectionDebtDurableEnabled=falsein the repair runtime first, then disable pusher's flag; existing markers remain inert and can resume when reenabled. This PR does not unsuspend or activate a CronJob.Custody and privacy
Playback metadata only. No seal authority, producer-token mutation, heartbeat, entry clock, dead-letter, conversation status, PCM read, or audio-object creation/rewriting/deletion. Tombstones, hard/soft deletion, account deletion, privacy changes, sync opt-out, peer projection updates, and marker replacement are fenced. Scope is redesign phase 5C only. #24712/#24716 and #23850 were checked before editing; #24712 and #24716 merged during verification and their flag metadata is preserved in the feature branch; shared overlap is flag metadata, not projection-drain behavior or deployment configuration. No
.githubchanges.A persistent Firestore outage or process death before a marker is committed can still prevent handoff; this marker is not a WAL. Failed marker writes retain metadata-only retry inventory and emit sanitized
marker_write_failedoutcomes; bounded inventory saturation emitsmarker_retry_overflow. Cached close debt remains retryable even when overflow inventory is full.Verification
~/workspace/omi/bin/omi-heavywith the requested GCP credential/project variables unset.agentctl: Luna R1 and R2 findings fixed. The same required Grok reviewer rechecked transactional empty retirement, producer and marker races, pressure telemetry and regression coverage: no blockers. No additional reviewer agents.make preflight: 44 checks passed, including the registered Firestore guard.scripts/pr-preflightmetadata contracts passed with the documented six-line router exception; the final frozen run passed all 44 checks; the normal hook repeated them successfully on the published head.95a9ea47ae98eb03843909459a6604523534a98e: full backend type checking, selected backend units, registered Firestore guards, OpenAPI and pinned formatting. No bypass, cap override or force push; Firestore guard passed in 172.30 seconds without timeout or retry.Exact published head:
95a9ea47ae98eb03843909459a6604523534a98e. CI results: checks for this exact commit.Product invariants affected
none
Not measured
No GCP/prod access or tier used; no live IAM/permission test, deployment, CronJob activation, production backlog/cost/latency measurement, device playback acceptance, or A/B worker-death/dead-letter simulator changes. Existing strict xfails remain existing simulator/state-table debt.
Line-Count-Exception: backend/routers/pusher.py | 1851 -> 1857 | Phase 5C adds exact-owner terminal projection-debt and socket-cleanup handoffs at the existing boundaries; full discovery is extracted into a shared module, and splitting adjacent custody code would exceed this lane's named scope.