fix(backend): keep daily summaries from dropping tail users - #12588
fix(backend): keep daily summaries from dropping tail users#12588aryanorastar wants to merge 6 commits into
Conversation
Failure-Class: FC-scheduled-batch-outlives-execution-contract
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Verification proofThis is the final evidence against current
Commands and resultsThe 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. |
There was a problem hiding this comment.
All reported issues were addressed across 15 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Failure-Class: FC-scheduled-batch-outlives-execution-contract
|
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:
Reproduced through the same runner CI requested: 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. |
|
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
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
Records and docs — the new Two decision points for maintainer sign-off before merge, beyond the normal human read:
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 |
Failure-Class: FC-scheduled-batch-outlives-execution-contract
Reviewer follow-up: all five findings addressedThe five findings from the Cubic review on
Fresh verification against current mainThe 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
CI follow-up: import-isolation fixture correctedThe only failing job on
Exact CI-runner reproduction: Combined changed and related suite: 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 |
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:
0 * * * *UTC Scheduler target, itsrun.invokerbinding, and one 3600-second/no-retry/one-task execution;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
*/1 * * * *, concurrent execution IDs, an OOM kill, andcoroutine 'day_summary_webhook' was never awaitedimmediately before process exit._send_bulk_summary_notificationcannot finish until that webhook is released.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.asyncio.runsubmit 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
gcp_notifications_job.ymlfor the protected environment with that exact SHA.0 * * * *,Etc/UTC,ENABLED,POST, the exact v2notifications-job:runURI, and the configured service account.