Skip to content

fix(backend): keep daily summaries from dropping tail users - #12588

Open
aryanorastar wants to merge 6 commits into
BasedHardware:mainfrom
aryanorastar:codex/12530-daily-summary-tail
Open

fix(backend): keep daily summaries from dropping tail users#12588
aryanorastar wants to merge 6 commits into
BasedHardware:mainfrom
aryanorastar:codex/12530-daily-summary-tail

Conversation

@aryanorastar

@aryanorastar aryanorastar commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

What changed and why

Fixes #12530.

Daily summaries were being scheduled every minute even though the worker is designed to own one hourly batch. That created overlapping Cloud Run executions, OOM/timeout pressure, and tail users who never reached completion. The same path also launched developer webhooks as unowned executor work and rendered an unlimited day of conversation history into one model request.

This PR makes the deployment and worker lifecycle agree:

  • the notifications deploy now owns and verifies the exact retained 0 * * * * UTC Scheduler target, its run.invoker binding, and one 3600-second/no-retry/one-task execution;
  • deployment accepts only the current merged-main SHA with a successful first-attempt Release Eligibility proof, then rechecks that SHA before publishing and deployment;
  • each selected user returns an explicit delivered/skipped/failed outcome and the run emits bounded aggregate accounting;
  • daily-summary webhooks are awaited by the cron event loop before the Cloud Run job can return;
  • rendered conversation history is deterministically capped at 120,000 tokens, leaving output/profile/memory headroom instead of letting one heavy account overflow the model context.

Product invariants affected

  • INV-DATA-1 — preserves the existing production data-plane authority; this is not a migration exception and does not change customer identity, Firebase, Firestore, or serving-plane selection.
  • INV-MEM-4 — preserves canonical memory authority. The generated runtime manifest changes only notifications-job execution flags; it does not add a memory intake, promotion, projection, or mutation path.

How it was verified

  • The read-only production evidence recorded on Daily summaries silently stop for tail-of-batch users: notifications-job 600s timeout + one-user context overflow aborts whole hour groups #12530 shows the prior Scheduler at */1 * * * *, concurrent execution IDs, an OOM kill, and coroutine 'day_summary_webhook' was never awaited immediately before process exit.
  • A behavioral regression test blocks the developer webhook and proves _send_bulk_summary_notification cannot finish until that webhook is released.
  • Mixed-batch coverage proves delivered, skipped, failed, and webhook-completed users remain independently accounted rather than aborting the batch.
  • Scheduler tests exercise existing-job update/resume, missing-job create, exact target/cadence validation, and a permission failure that must not be misclassified as “not found.”
  • Prompt-budget tests prove unchanged small inputs and deterministic bounded large inputs.
  • backend/.venv/bin/pytest -q ... over the related daily-summary, webhook, scheduler, runtime-env, and workflow suites: 208 passed.
  • PATH="$PWD/backend/.venv/bin:$PATH" backend/scripts/pre-deploy-check.sh: 226 passed, with dev/prod runtime validation and workflow contracts green.
  • python3 backend/scripts/check_workflow_contracts.py: passed.
  • python3 .github/scripts/check_failure_class_guard_ratchet.py: passed.
  • make preflight: passed against the final committed diff and this PR body.

No production mutation was performed from a development machine. After merge, the protected notifications deployment must be dispatched with the admitted merged-main SHA; that run applies and then describes the Scheduler, failing if cadence, state, method, URI, or service account differs. This is backend/infrastructure-only and requires no iOS, Android, pendant, or physical-device verification.

Tests

  • backend/tests/unit/test_other_notifications_async_boundaries.py — event-loop responsiveness, awaited webhook lifecycle, and mixed user accounting.
  • backend/tests/unit/test_provision_notifications_scheduler.py — exact Scheduler create/update/validation and deployment manifest ownership.
  • backend/tests/unit/test_daily_summary_zero_coordinate_locations.py — prompt history remains unchanged below the cap and deterministic above it.
  • Existing webhook wiring guards now reject the former fire-and-forget asyncio.run submit pattern.

Failure class (fixes)

Failure-Class: new

Failure-class transition narrative (only when needed)

New class: FC-scheduled-batch-outlives-execution-contract.

The violated contract is that a scheduled batch worker's deployed cadence, deadline, concurrency, and owned async work must match the lifecycle assumed by its code. The canonical prevention is an exact deployment-owned and post-apply-verified Scheduler target, manifest-owned execution bounds, bounded provider input, awaited webhook completion, and explicit run accounting. The supporting guards are the scheduler contract, blocked-webhook lifecycle test, mixed-batch accounting test, and prompt-budget tests listed above.

New guards (only when adding a check or ratchet)

These guards would have caught #12530 before deployment. The exact Scheduler name/target/cadence is intentionally job-specific; the implementation still uses the repository's shared runtime-manifest renderer and async executor primitives rather than creating a second general deployment or concurrency framework.

Rollout and rollback

  • Rollout: merge, wait for Release Eligibility on the merged main SHA, then dispatch gcp_notifications_job.yml for the protected environment with that exact SHA.
  • Acceptance: the workflow's final Scheduler describe must report 0 * * * *, Etc/UTC, ENABLED, POST, the exact v2 notifications-job:run URI, and the configured service account.
  • Rollback: revert this PR and redeploy the last admitted main SHA. Do not restore the per-minute cadence or fire-and-forget webhook pattern as an operational workaround.

Review in cubic

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@aryanorastar

Copy link
Copy Markdown
Contributor Author

Verification proof

This is the final evidence against current main (d1e83d90b090) after integrating upstream changes.

Failure mode from #12530 Guard/proof now in this PR
Hourly code was invoked by a live */1 * * * * Scheduler test_existing_scheduler_is_updated_to_exact_hourly_contract_and_resumed asserts 0 * * * *; the provisioner describes the applied job and rejects any wrong cadence, state, method, URI, timezone, or service account.
Missing Scheduler and permission failure looked equivalent Create and update paths are covered separately; PERMISSION_DENIED raises and cannot fall through to create.
Cloud Run could exit with the summary webhook still queued test_bulk_summary_waits_for_owned_webhook_before_returning blocks the webhook, proves the batch task is still unfinished, then releases it and observes webhook_completed=1.
One user failure obscured the batch tail Mixed-batch coverage observes selected=3, delivered=1, skipped=1, failed=1, and one completed webhook without aborting the other users.
A heavy day could overflow the model context The history helper returns small input byte-for-byte unchanged and deterministically truncates oversized input within the 120,000-token budget.
Live runtime drifted from the source deadline/concurrency assumptions Runtime manifest and workflow contract pin --task-timeout=3600s --max-retries=0 --tasks=1 --parallelism=1; the generated dev/prod manifest is checked for exact composition.

Commands and results

Related daily-summary/webhook/scheduler/runtime suites: 208 passed
backend/scripts/pre-deploy-check.sh: 226 passed
make preflight: 34 repository checks passed
bounded pre-push gate: passed
runtime-image contracts: 13 registered images passed
pyright: 0 errors

The production observations remain the read-only evidence already attached to #12530: per-minute cadence, overlapping execution IDs, an OOM termination, and the un-awaited webhook warning immediately before container exit. I did not mutate live infrastructure from a development machine. The protected post-merge deployment is the acceptance step and fails closed unless the final Scheduler describe matches the retained contract exactly.

No iOS, Android, pendant, or physical-device testing is applicable to this backend/Cloud Run fix.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 15 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread backend/scripts/provision_notifications_scheduler.py Outdated
Comment thread backend/tests/unit/test_async_http_infrastructure.py Outdated
Comment thread backend/utils/other/notifications.py
Comment thread backend/utils/llm/external_integrations.py Outdated
Comment thread backend/utils/llm/external_integrations.py Outdated
Failure-Class: FC-scheduled-batch-outlives-execution-contract
@aryanorastar

Copy link
Copy Markdown
Contributor Author

CI follow-up proof

The first full backend unit run exposed one import-isolation test double that had not been updated for the new token-count dependency:

  • failing file: tests/unit/test_high_priority_usage_tracking.py
  • failure: its hand-written utils.llm.clients stub did not expose num_tokens_from_string
  • production code was not failing; the stub no longer matched the real client-module contract
  • fix: added the deterministic token counter to that stub in b6e0fb212b

Reproduced through the same runner CI requested:

BACKEND_UNIT_TEST_FILE_LIST=/tmp/omi-backend-unit-failures-12530.txt bash test.sh
21 passed in 0.34s

The push then passed the complete bounded pre-push gate again, including 34 PR-preflight checks, pyright (0 errors), runtime-image closure for 13 images, workflow/deployment contracts, and all 8 selected backend test files. A fresh authoritative GitHub Actions run is now in progress.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Reviewed against #12530's production evidence — this is a well-executed fix for the tail-user drop, and the deployment contract now matches the worker's actual one-hourly-batch lifecycle. Notes by area:

Worker lifecycle — backend/utils/other/notifications.py

  • Every skip path in _send_summary_notification now returns a typed DailySummaryUserResult (skipped_lock, skipped_existing, skipped_no_conversations, skipped_no_unlocked_conversations, skipped_no_speech) instead of a bare return, and the new mixed-batch test observes selected=3/delivered=1/skipped=1/failed=1 without one user aborting the hour group — exactly the head-of-line poisoning in the issue.
  • send_daily_summary_notification replaces the single whole-loop try/except with per-hour-group isolation plus query_failures accounting, so one Firestore read failure no longer throws away the remaining timezone groups.
  • The fire-and-forget postprocess_executor.submit(asyncio.run, day_summary_webhook(...)) is gone; webhooks are gathered on the cron event loop per batch and counted (webhook_completed/webhook_failed). Since day_summary_webhook posts through the shared client (30s read / 2s connect timeouts, circuit breaker, 1/5/30s retry delays), awaiting it can't unboundedly stall the 3600s task budget. One behavioral note: a batch's webhooks now complete after that batch's pushes — slightly later than before, but owned and observable.

Context overflow — backend/utils/llm/external_integrations.py

Deployment contract — .github/workflows/gcp_notifications_job.yml, backend/scripts/provision_notifications_scheduler.py, runtime_env yamls

  • Replacing the free-form branch input with release_sha (must equal the current origin/main SHA, re-verified before image push and again before Cloud Run deploy, plus a first-attempt Release Eligibility proof) closes the deploy-any-branch hole — strictly a hardening.
  • The provisioner fails closed on PERMISSION_DENIED vs NOT_FOUND, pins 0 * * * * UTC with the exact run.googleapis.com URI and SA email, and re-validates final state after create/update/resume. Both paths plus identity rejection are unit-tested, and the cross-artifact test reads the workflow YAML and runtime manifest together so they can't drift.
  • --max-retries=0 --tasks=1 --parallelism=1 pinned across _base.yaml and both environments in runtime_env.yaml matches the one-task hourly design.

Records and docs — the new .github/failure-classes/FC-scheduled-batch-outlives-execution-contract.json accurately captures the violated contract and points at the right prevention tests, and the one-line backend/docs/subservice-internals.md update matches the code. The CI follow-up (num_tokens_from_string added to the hand-written clients stub in test_high_priority_usage_tracking.py) was a test-double refresh, not a production issue.

Two decision points for maintainer sign-off before merge, beyond the normal human read:

  1. The Scheduler invoker binding reuses memory-maintenance-scheduler@… for notifications-job. It works, but shared-vs-dedicated service-account identity is an infra call for a maintainer.
  2. The first manual deploy after merge re-provisions the live Scheduler to hourly and resumes it — dispatch is main-only and manual so merging alone touches nothing, but the first hourly run's daily_summary_run accounting line is the acceptance signal to watch.

Checks are green on this head, including the hermetic backend gates. No split needed — this is one cohesive lifecycle fix. Labels applied: backend, positive-signal, workflow-review, needs-maintainer-review.


by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

@Git-on-my-level Git-on-my-level added backend Backend Task (python) positive-signal Good PR — positive signal, not a formal approval workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior needs-maintainer-review Needs a human maintainer to sign off before merge labels Sep 2, 2026
@aryanorastar

Copy link
Copy Markdown
Contributor Author

Reviewer follow-up: all five findings addressed

The five findings from the Cubic review on d43280a025 were valid and are fixed in 3c0763b7a9; the branch is now pushed at d49b6f22a2 after a clean merge of current origin/main (0f0f679e5f).

Finding Resolution Regression proof
P1: enabled Scheduler was always resumed Parse initial state, accept only ENABLED/PAUSED, and resume only an initially paused job Enabled/no-resume, paused/resume, and invalid-state fail-closed tests
P2: webhook await assertion was non-binding AST-locate the exact day_summary_webhook call and require it to be inside awaited asyncio.gather Structural test rejects bare/unowned webhook calls
P2: skipped/failed webhooks counted as completed Typed DaySummaryWebhookResult; separate delivered, skipped, and failed counters Outcome tests plus mixed accounting coverage
P2: truncation could split records and retain invalid mappings Render the largest budget-fitting prefix of complete conversations; build numbering/map only from retained records; keep aggregate stats over all records Complete-record prefix and retained-ID mapping tests
P3: marker could exceed a tiny token budget Return empty bounded history when even the marker cannot fit Dedicated marker-budget boundary test

Fresh verification against current main

Focused reviewer suite: 103 passed
Combined affected backend suites: 183 passed
backend/scripts/pre-deploy-check.sh: 226 passed
Python 3.11 pyright wrapper: 0 errors
make preflight: 34/34 checks passed
pre-push runtime-image contracts: 13/13 images passed
pre-push selected backend suites: all passed
black: 12 changed Python files unchanged
working tree: clean

The full push gate completed successfully before the new head was published. Fresh GitHub Actions runs are now executing. This remains a backend/Cloud Run lifecycle fix; no iOS, Android, pendant, or physical-device validation is applicable.

Failure-Class: FC-scheduled-batch-outlives-execution-contract
@aryanorastar

Copy link
Copy Markdown
Contributor Author

CI follow-up: import-isolation fixture corrected

The only failing job on d49b6f22a2 was the full backend unit suite. It identified one more hand-written module stub that had not been updated for the typed webhook result introduced by the reviewer fix:

  • failing file: tests/unit/test_daily_summary_race_condition.py
  • failure: its isolated utils.webhooks replacement exposed day_summary_webhook but not DaySummaryWebhookResult
  • scope: test-fixture compatibility only; the production module loaded normally and every runtime/contract job passed
  • fix: add the result type to that isolated stub in 0e262093a2

Exact CI-runner reproduction:

BACKEND_UNIT_TEST_FILE_LIST=/tmp/omi-backend-unit-failures-12530-review.txt bash test.sh
16 passed

Combined changed and related suite:

9 files, 201 passed

The full pre-push gate then passed again: 34/34 PR checks, Python 3.11 pyright with 0 errors, 13/13 runtime-image contracts, all 9 selected backend test files, module-isolation, workflow/deployment contracts, and formatting. A fresh GitHub Actions run is now authoritative for head 0e262093a2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Backend Task (python) needs-maintainer-review Needs a human maintainer to sign off before merge positive-signal Good PR — positive signal, not a formal approval workflow-review Needs maintainer review for workflow, automation, hooks, or CI behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Daily summaries silently stop for tail-of-batch users: notifications-job 600s timeout + one-user context overflow aborts whole hour groups

2 participants