Skip to content

Harden boundary delivery and data convergence - #10646

Open
ZachL111 wants to merge 4 commits into
BasedHardware:mainfrom
ZachL111:zach/boundary-delivery-recovery
Open

Harden boundary delivery and data convergence#10646
ZachL111 wants to merge 4 commits into
BasedHardware:mainfrom
ZachL111:zach/boundary-delivery-recovery

Conversation

@ZachL111

@ZachL111 ZachL111 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Problem

Several boundary paths treated an attempted send or write as a complete outcome even when the remote effect was unknown or only partially persisted.

At origin/main, the pusher client returned after a failed finalization send without invalidating the dead socket:

        except Exception as e:
            logger.error(f"Failed to send process_conversation request: {e} {self.uid} {self.session_id}")
            return False

Legacy memory search requested only the desired result count before filtering missing, locked, rejected, invalid, or malformed records:

def _legacy_search_memories(uid: str, query: str, *, limit: int = 5) -> List[MemorySearchMatch]:
    capped_limit = max(1, min(limit, 20))
    matches = vector_db.find_similar_memories(uid, query, threshold=0.0, limit=capped_limit)

Todoist task creation converted an omitted provider identity into the string "None" and reported success:

            if response.status_code in [200, 201]:
                task_data = response.json()
                return {"success": True, "external_task_id": str(task_data.get('id'))}

The same boundary problem appeared in transcript and speaker delivery, webhook retries, vector projection counts, and legacy X-source acknowledgement after partial projection.

Reachability / failure scenario

  • backend/utils/listen_pusher_session.py::ListenPusherSession.request_conversation_processing, _transcript_flush, _audio_bytes_flush, and send_speaker_sample_request await socket sends during ordinary disconnect and cancellation paths. A failed socket could remain marked connected, while accepted work was discarded before _pusher_reconnect_loop could replay it. The fixed speaker path now delegates to _send_speaker_sample_request.
  • backend/routers/pusher.py::_websocket_util_trigger runs transcript and speaker effects in process_transcript_queue and process_speaker_sample_queue. If the connection closes after one effect completes, the client cannot distinguish completed work from work that must be replayed.
  • backend/utils/speaker_identification.py::extract_speaker_samples uploads through backend/utils/other/storage.py::upload_person_speech_sample_from_bytes before add_person_speech_sample commits the reference. A retryable database outcome previously created a new random object for the same logical delivery.
  • backend/utils/memory/memory_service.py::_legacy_search_memories requested only limit vector candidates, then hydrated and filtered them. Stale, locked, rejected, invalid, invalidated, or malformed leading hits could therefore hide eligible lower-ranked results.
  • backend/routers/memories.py::_purge_legacy_memories enumerated IDs through backend/database/memories.py::get_memories before calling delete_all_memories. Records excluded by that reader were deleted from Firestore without their vector IDs being available for cleanup.
  • backend/routers/task_integrations.py::create_task_via_integration delegates to backend/utils/task_integrations_ops.py::create_task_internal. A provider 2xx response without an ID became "None", while a connection failure after send was not distinguished from a safe pre-send failure.
  • backend/utils/x_connector.py::_extract_and_index persisted legacy memory documents and then called upsert_memory_vectors_batch. A partial legacy vector write could still let the source be counted as processed and acknowledged.
  • backend/utils/webhooks.py::_post_dev_webhook retried permanent and transient responses alike, and backend/utils/app_integrations.py::_async_trigger_realtime_integrations lacked stable retry identity. In addition, backend/routers/users.py::enable_user_webhook_endpoint recorded synthetic success without clearing the process-local circuit breaker.

Fix

  • Preserve finalization, transcript, audio, and speaker work across ambiguous outbound send failures and cancellation. On those delivery send failures, mark only the socket that failed as disconnected so a concurrent replacement socket is not cleared.
  • Negotiate effect acknowledgements for transcript and speaker deliveries, carry stable delivery IDs through downstream integrations, and acknowledge only after owned effects complete. Redis delivery leases and done markers suppress cross-socket duplicates and are bounded by expiry.
  • Reject new stable transcript and speaker effect deliveries instead of evicting accepted unacknowledged effect deliveries when those queues are full. Add a bounded graceful drain so audio metadata is flushed before dependent speaker extraction.
  • Derive one opaque speaker object name from the stable delivery identity. Legacy callers without a delivery identity retain random UUID names. The code does not delete on ambiguous database failure because the commit may already have succeeded.
  • Retry developer webhooks only for transport failures, 408, 425, 429, and 5xx, while reusing one receiver-visible idempotency key. Explicit re-enable or URL replacement resets persisted and process-local delivery health without recording synthetic success.
  • Progressively over-fetch legacy vector candidates, hydrate each ID once, preserve vector order, and stop at a fixed candidate cap. The origin/main sibling _legacy_search_memories_mcp already over-fetched min(limit * 3, 60) before filtering; this change makes both API and MCP paths progressively backfill instead of relying on one fixed over-fetch.
  • Use the exact IDs returned by the committed database delete for vector cleanup, chunk projection-repair writes below the Firestore batch limit, use one injected Firestore client for related reads and writes, and report actual vector write counts.
  • Leave a legacy X source retryable after partial vector projection instead of acknowledging incomplete convergence. Canonical X projection acknowledgement behavior is unchanged.
  • Reject provider success responses without a usable task identity. Classify failures as safe pre-send retries or ambiguous outcomes. For provider-create outcomes represented by CreateTaskResponse, preserve error_code, retryable, and ambiguous through the public response and generated clients. Existing authentication and token-refresh preflight HTTP errors remain unchanged. Ambiguous task creation is reported, not retried automatically.
  • Expand high-risk workflow contracts and document the listen-to-pusher delivery guarantees and limitations.

Product invariants affected: INV-MEM-1.

Tests

  • backend/tests/unit/utils/test_listen_pusher_session.py covers dead-socket invalidation, route-preserving replay, stable delivery IDs, cancellation, acknowledgement drain, and queue retention. Key cases include test_finalization_send_failure_disconnects_dead_socket_and_preserves_request, test_ack_capability_retains_and_replays_transcript_until_peer_completion, test_full_speaker_buffer_preserves_accepted_unacknowledged_delivery, and test_stale_socket_failure_does_not_disconnect_replacement.
  • backend/tests/unit/test_listen_finalization_cloud_tasks.py proves acknowledgement occurs only after owned effects complete, retryable speaker work remains unacknowledged, done markers suppress cross-socket duplicates, and a full stable queue does not evict accepted work.
  • backend/tests/unit/test_redis_db_cache_serialization.py covers lease ownership, done markers, owner-fenced release, bounded Redis timeouts, and fail-open behavior.
  • backend/tests/unit/test_speaker_identification_delivery.py covers retryable extraction outcomes, missing-person retry, passing a one-sample cap to the transactional append helper, and stable hashed object identity across retries.
  • backend/tests/unit/test_async_webhooks.py, test_async_app_integrations.py, test_async_http_infrastructure.py, test_webhook_auto_disable.py, and test_users_webhook_url_validation.py cover retry classification, stable idempotency keys, URL replacement, and explicit delivery-health reset without synthetic success.
  • backend/tests/unit/test_memory_service_parity.py, test_memories_batch_delete.py, test_memories_delete_batch_chunk.py, test_memory_ledger.py, and test_memories_batch.py cover progressive backfill, candidate caps, injected Firestore use, exact committed delete IDs, 499-write repair chunks, and reported vector write counts.
  • backend/tests/unit/test_x_memory_extraction_retry.py::test_pending_x_source_remains_unacknowledged_after_partial_legacy_projection proves partial legacy projection leaves the source retryable.
  • backend/tests/unit/test_task_integrations_ops.py covers missing identities, unexpected 2xx responses, rate limits, server failures, safe pre-send failures, and ambiguous post-send failures. test_task_integration_due_date_validation.py::test_provider_failure_metadata_is_preserved_in_public_response covers response propagation.
  • backend/testing/e2e/test_task_integrations.py verifies the expanded provider-create response at the HTTP boundary for success, provider 500, and connect timeout outcomes. backend/testing/e2e/test_webhooks.py explicitly disables delivery after URL replacement and verifies a disabled webhook neither calls the provider nor mutates its reset health snapshot.
  • backend/tests/unit/test_openapi_contract.py, test_app_client_ts_generator.py, and test_app_client_dart_generator.py verify the public schema and generated clients, including test_wrapped_task_integrations_wire_dart_is_generated_from_app_client_openapi.
  • backend/tests/unit/test_dev_api_lock_bypass.py models the committed delete count and verifies the route injects its Firestore client. test_developer_memory_adapter.py scopes the guard/read/delete ordering check to MemoryService.delete_external_memory. test_pusher_ghost_connections.py parses the actual speaker-age condition instead of relying on line wrapping. test_tools_router.py exercises the shared legacy-memory search boundary instead of stale vector internals.

Verification

Commit 4ea54d07e5d3e3e03fb3a393fe06e0c1abc1b143 has parent eb60a5175e96682185bba21829ccfeb98ba38c04, the latest origin/main used for publication. The final upstream advance touched no PR paths. The resulting patch is clean, and the complete listen session unit file passed again with 27 passed.

The maintainer-requested socket-identity regression is parameterized across receive and heartbeat failure paths:

python -m pytest -q tests/unit/utils/test_listen_pusher_session.py::test_stale_socket_failure_does_not_disconnect_replacement

For byte-identical prove-fail, only backend/utils/listen_pusher_session.py was restored from origin/main, and git diff origin/main -- backend/utils/listen_pusher_session.py printed nothing before pytest ran. Both variants failed at the intended boundary with:

AssertionError: stale socket failure disconnected the healthy replacement

Restoring the committed product file passed both variants. That product file is byte-identical between the prove-fail base and the final publication base.

Current-base behavioral verification:

  • Conflict-sensitive grouped regression set: 108 passed.
  • Listen and pusher mapped unit workflow: 462 passed.
  • Listen and pusher end-to-end workflow: 18 passed.
  • Conversation lifecycle workflow: 197 passed.
  • Webhook delivery health workflow: 210 passed.
  • Legacy memory shared workflow: 135 passed.
  • Isolated lock-bypass workflow: 61 passed.
  • Account deletion workflow: 235 non-E2E passed and 5 E2E passed.
  • Task provider unit workflow: 19 passed.
  • Task and webhook E2E workflow: 13 passed.
  • OpenAPI and code generation tests: 49 passed.
  • App-client OpenAPI export is current under the CI-pinned Linux Python 3.11.15 environment. Compatibility, TypeScript generation, and Dart generation are current.

Static and repository gates:

  • Black 24.4.2 and CI-pinned Black 26.5.1 with --line-length 120 --skip-string-normalization: all changed Python files accepted.
  • Pyright: 0 errors in both changed-product groups.
  • Module-stub scan: 835 backend test files checked, 0 violations.
  • Async-blocker scan: 452 synchronous and 87 asynchronous definitions checked, 0 findings.
  • Import-side-effect scan: 744 product files checked, 0 violations.
  • Product-file line-count ratchet: no increase across 19 changed product files.
  • Workflow contracts and conversation lifecycle write guard: passed.
  • Preflight selected 55 files, identified INV-MEM-1, and required no failure-class declaration because no fix: commit is present.
  • git diff --check: clean.

The repository pre-push single-flight wrapper could not spawn its child process on Windows and exited with WinError 2. Its underlying checks were run directly with the pinned project environments before the guarded force-with-lease publication.

Failure-class protocol: no fix: commit was used, so no declaration is required.

Impact

User-visible successful outcomes stay the same; negotiated delivery paths now carry acknowledgement and stable identity metadata. The affected failure cases occur during ordinary reconnects, transient downstream failures, stale vector hydration, partial legacy X projection, or malformed provider success responses. They are not expected on every request, but their previous outcomes included dropped or duplicated side effects, leaked speaker objects, false task success, incomplete memory search, stale deleted projections, and legacy X sources acknowledged before all projections converged.

Limitations remain explicit:

  • Effect-level acknowledgements require peer negotiation. Legacy peers continue through the compatibility path.
  • Stable transcript and speaker effect deliveries are bounded and retained across reconnects, but are not durable across a client process loss.
  • Finalization, transcript, audio, speaker, receive, and heartbeat failures are fenced to the socket that failed; stale failures from a replaced socket cannot disconnect the healthy replacement.
  • Redis delivery fencing fails open for availability, so cross-socket duplicate suppression is degraded while Redis is unavailable.
  • Webhook idempotency still depends on receivers honoring the stable idempotency key.
  • Progressive memory backfill has a fixed candidate cap, so a severely stale vector index can still return fewer than requested.
  • Cross-store projection is not transactional. Legacy X-source partial projection remains retryable; canonical X acknowledgement behavior is unchanged. Other Firestore/vector operations surface or log partial counts.
  • Ambiguous external task creation is surfaced to the caller but is not automatically reconciled with the provider.

@ZachL111
ZachL111 force-pushed the zach/boundary-delivery-recovery branch 3 times, most recently from 24b1a5e to a90a80c Compare July 26, 2026 11:07
@ZachL111
ZachL111 marked this pull request as ready for review July 26, 2026 11:25
@Git-on-my-level Git-on-my-level added needs-maintainer-review Needs a human maintainer to sign off before merge security-review Touches auth, provider routing, secrets, or security-sensitive surfaces labels Jul 26, 2026

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the very thorough write-up and the proof-fail cases. I reviewed this as a high-risk boundary/data-convergence change set.

Positive signal from this pass: the scope is cohesive around delivery idempotency, ambiguous boundary outcomes, and legacy projection convergence, and I did not find an obvious security/supply-chain concern in the changed surfaces. The approach of stable receiver-visible delivery IDs, bounded Redis fencing/done markers, not claiming synthetic webhook success, over-fetching stale vector hits, and preserving ambiguous task/provider outcomes looks directionally right. I also verified the current GitHub checks are green from the PR checks view, and git diff --check origin/main...HEAD is clean locally.

I’m intentionally not approving because this touches websocket delivery semantics, developer webhooks, user memory/vector convergence, task-provider error classification, and public generated API contracts across a large diff. Before merge, I’d like a human maintainer to review the delivery/ACK semantics and data-convergence tradeoffs in particular: Redis fail-open duplicate behavior, which local completions are acknowledged versus downstream delivery guarantees, and the public API change for retryable/ambiguous task creation results.

No specific code changes requested from this automated pass.


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

@tianmind-studio tianmind-studio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I found one actionable stale-socket race in the reconnect path that should be fixed before merge.

[P1] Fence receive failures by the socket that actually failed — backend/utils/listen_pusher_session.py:584

pusher_ws is captured before recv(), but the ConnectionClosed branch calls session-wide _mark_disconnected() without checking that the failing socket is still current. A concurrent send failure can mark the old socket down and install a replacement; if the old recv() is scheduled to raise afterward, this branch flips pusher_connected to False for the healthy replacement and starts another reconnect that closes it.

I reproduced this with a blocked old recv(), session.connect() installing a replacement, then releasing the old ConnectionClosed: the replacement remained in pusher_ws, but pusher_connected became false. Switching this call to _mark_failed_socket_disconnected(pusher_ws, auto_reconnect=True), as the send paths already do, made that regression plus the existing test_listen_pusher_session.py suite pass (26 tests total).

I focused this pass on negotiated transcript/speaker delivery, reconnect replay and close ordering, webhook retry identity/reset behavior, provider outcome classification, and generated client parity. The other inspected boundaries were internally consistent, and the Windows TypeScript client matches the expanded OpenAPI task response.

ZachL111 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@tianmind-studio I’m coordinating review of this PR directly with the Omi maintainers. Please leave further reviews and change requests on my PRs to maintainers unless I’ve asked you for input, and focus your review activity on your own PRs. I’ll work with the maintainers on any changes they consider necessary here.

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Request changes: socket-identity fencing is incomplete

This PR correctly introduces _mark_failed_socket_disconnected(failed_ws, ...) for most outbound send failures, but two receive/liveness paths still use session-wide _mark_disconnected() after capturing a socket reference. That permits a stale failure from old socket A to mark a healthy replacement B disconnected after B has been installed. The result is an unnecessary reconnect cycle and can close/replace B.

Required implementation

  1. In ListenPusherSession.pusher_receive(), replace the ConnectionClosed handler's _mark_disconnected() with:
self._mark_failed_socket_disconnected(
    pusher_ws,
    auto_reconnect=True,
)
  1. Apply the same socket-identity fence to pusher_heartbeat(). It currently calls _mark_disconnected() when an old heartbeat send raises ConnectionClosed, which leaves the same stale-socket race.

  2. Add a deterministic hermetic regression test in backend/tests/unit/utils/test_listen_pusher_session.py. Use the existing injectable connect_to_pusher dependency and controllable fake websockets; do not use timing sleeps as the proof. For each of the receive and heartbeat variants:

    • begin a blocked operation on old socket A;
    • trigger reconnect and install healthy socket B;
    • release A so it raises ConnectionClosed;
    • assert session.pusher_ws is B, session.pusher_connected is True, B was not closed, and no redundant reconnect task was created.

The invariant should be explicit: a failure associated with socket A may mutate session connection state only when A is still session.pusher_ws.

This is a focused, hermetic proof of the reported race; the current green gauntlet does not cover this interleaving. Please rerun the focused session suite and the Listen/Pusher Stack Gauntlet on the new PR head after the fix.

Git-on-my-level added a commit that referenced this pull request Jul 27, 2026
## Problem

`GET /v1/dev/user/memories` authorized and pinned canonical accounts
correctly, but its `MemoryService.read()` call independently reran the
stricter rollout/control read decision. When those decisions diverged,
the canonical route silently read the legacy collection and returned
HTTP 200 with a stale pre-cutover prefix. The MCP list routes had the
same double-routing boundary.

## Fix

- Add a narrow `MemoryService.read_pinned()` seam that reads the backend
already selected by an authorized external list route.
- Use that seam for Developer API, MCP REST, and MCP SSE canonical list
reads.
- Preserve the existing default-read grant ordering, canonical
visibility filtering, public response projection, and legacy behavior
for legacy-pinned accounts.
- Add a behavioral Developer API regression with old- and new-schema
canonical rows, including predicate, subject attribution, evidence,
veracity, and uncertainty fields. It proves the current row remains
visible while raw provenance fields stay outside the Developer response.

PR #10646 changes legacy search and mutation/vector boundaries in
`memory_service.py`; this PR does not overlap those hunks or duplicate
its behavior.

## Product invariants affected

- INV-MEM-1

Failure-Class: none

This is an isolated double-routing defect rather than an instance of a
registered semantic failure class. The prevention artifact is the
Developer route regression at
`backend/tests/unit/test_dev_api_canonical_grant_ordering.py::test_get_memories_uses_authorized_canonical_pin_for_new_schema_rows`,
backed by the explicit pinned-read service seam.

## Verification

- Prove-fail: with the Developer route restored to
`MemoryService.read()`, the new regression returned only
`canonical-old-schema` and failed because `canonical-new-schema` was
absent.
- `backend/.venv/bin/python -m pytest -q
backend/tests/unit/test_dev_api_canonical_grant_ordering.py
backend/tests/unit/test_dev_api_memories_pagination.py
backend/tests/unit/test_mcp_data_endpoints.py
backend/tests/unit/test_ws_l_surface_routing.py
backend/tests/unit/test_memory_service_parity.py` — 82 passed.
- Diff-selected `backend/test.sh` lane — all 38 selected unit-test files
passed.
- Pyright on the four changed production modules with the pinned Python
3.11 environment — 0 errors, 0 warnings.
- Black 24.4.2 check on all six changed Python files — clean.
- `make preflight` — passed after the final commit.

Closes SCA-185


<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/BasedHardware/omi/pull/10696?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->

@Git-on-my-level Git-on-my-level left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update. I re-checked the socket-identity blocker from my previous review against head 87917e774ba2edbbf8dc236c021ec65e2b49d80a.

Positive signal: the requested stale-socket fencing is now present in both paths I called out: pusher_receive() calls _mark_failed_socket_disconnected(pusher_ws, auto_reconnect=True) on ConnectionClosed, and pusher_heartbeat() does the same for heartbeat send ConnectionClosed. I also found the focused regression coverage in test_stale_socket_failure_does_not_disconnect_replacement, parameterized over receive and heartbeat, asserting that a stale socket A failure does not disconnect replacement socket B or create a redundant reconnect task.

I’m not requesting further code changes from this pass, and I’m dismissing my stale CHANGES_REQUESTED review because its specific blocker appears resolved on the current head.

Still intentionally not approving: this remains a large, security-sensitive boundary/data-convergence change touching websocket delivery ACK semantics, Redis delivery fencing, developer webhooks, memory/vector convergence, task-provider ambiguity classification, and generated public API contracts. The PR already has security-review and needs-maintainer-review, which still look appropriate; a human maintainer should sign off on the delivery guarantees, fail-open duplicate behavior, and public task response contract before merge.

Validation note: I statically inspected the current head and compiled the changed pusher session source/test files. A focused pytest attempt in a fresh worktree was blocked by this local environment missing backend test dependencies (ModuleNotFoundError: No module named 'google'), so I’m relying on static verification plus the PR’s reported test evidence rather than claiming a local pytest pass.


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 dismissed their stale review July 27, 2026 15:10

Current head 87917e7 fences the previously blocking receive and heartbeat stale-socket ConnectionClosed paths, and includes focused regression coverage for both paths. Dismissing only this automation-authored stale review; human/other-actor reviews are preserved.

Copy link
Copy Markdown
Contributor Author

Addressed the maintainer-requested stale-socket race in both receive and heartbeat paths.

  • Each path now captures the websocket associated with the operation and calls _mark_failed_socket_disconnected(..., auto_reconnect=True), so a failure from old socket A can mutate connection state only if A is still current.
  • Added deterministic parameterized regression coverage for receive and heartbeat. The test blocks old socket A, installs replacement B, releases A to raise ConnectionClosed, then asserts B remains current and connected, is not closed, and no redundant reconnect task is created.

David re-checked head 87917e774ba2edbbf8dc236c021ec65e2b49d80a and confirmed that the requested implementation and regression are present. No Tianmind feedback was used for this update.

@ZachL111
ZachL111 force-pushed the zach/boundary-delivery-recovery branch 2 times, most recently from 1cfed1b to 285a4b9 Compare July 28, 2026 16:32

Copy link
Copy Markdown
Contributor Author

Rebased onto current main in 285a4b96a06d5c0eda077e0ae0c32c59e61c2409 and preserved the maintainer-requested receive and heartbeat socket-identity fences. A failure from the old socket can no longer clear or close a concurrently installed replacement socket.

The deterministic old-socket/replacement-socket regression fails against the byte-identical current-main product file with AssertionError: stale socket failure disconnected the healthy replacement, then passes in both receive and heartbeat variants with the committed implementation. The complete listen session unit file passes 27 passed; the mapped listen unit and E2E workflows pass 462 passed and 18 passed. Black 24.4.2, Pyright with 0 errors, repository scans, workflow contracts, lifecycle guard, line-count ratchet, OpenAPI/codegen checks, preflight, and git diff --check are clean. The PR is ready and currently mergeable.

@ZachL111
ZachL111 force-pushed the zach/boundary-delivery-recovery branch from 285a4b9 to 4ea54d0 Compare July 28, 2026 16:48

Copy link
Copy Markdown
Contributor Author

Follow-up CI corrections are in 4ea54d07e5d3e3e03fb3a393fe06e0c1abc1b143.

  • Refactored the Redis Lua registrations so both the contribution-rule Black 24.4.2 pin and the repository’s current CI Black 26.5.1 pin accept the file unchanged. The Redis serialization and rate-limit group passes 85 passed.
  • Regenerated the app-client OpenAPI contract in the same pinned Linux Python 3.11.15 environment used by CI, then regenerated all four TypeScript clients. The Linux export check is current, compatibility passes against eb60a5175e96682185bba21829ccfeb98ba38c04, the OpenAPI/codegen group passes 49 passed, and TypeScript and Dart generation checks are current.

The PR remains ready and mergeable; a fresh CI run is in progress.

@undivisible undivisible added human Human-authored pull request backend Backend Task (python) app flutter flutter work mobile desktop web javascript Pull requests that update javascript code docs-tooling Layer: Documentation, examples, dev tools labels Aug 10, 2026
@undivisible undivisible added the privacy-review Touches user-data persistence, permissions, or privacy-sensitive surfaces label Aug 10, 2026
@Git-on-my-level Git-on-my-level removed flutter flutter work desktop docs-tooling Layer: Documentation, examples, dev tools web javascript Pull requests that update javascript code labels Aug 11, 2026
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks for the continued work on this. I reviewed the current head (4ea54d07e5d3e3e03fb3a393fe06e0c1abc1b143) as a high-risk backend boundary/data-convergence change set.

Positive signal from this pass: I did not find a new blocking issue in the sampled high-impact paths, and the current CI/check set is green.

Specific observations:

  • backend/utils/listen_pusher_session.py now keeps stable transcript and speaker-sample delivery IDs pending until opcode 202 is received, and the close path sends the drain opcode before waiting for acknowledgements. The send-failure paths keep pending work replayable and fence failed sockets for reconnect.
  • backend/routers/pusher.py advertises delivery-ack support, rejects full transcript/speaker queues instead of silently evicting accepted stable frames, and uses Redis leases/done markers around transcript and speaker-sample effects before acknowledging them.
  • backend/database/redis_db.py bounds pusher delivery keys by hashing the caller-visible delivery ID and uses token-checked Lua completion/abandon scripts, which keeps the idempotency state bounded and prevents a different worker from completing another worker's lease.
  • backend/utils/memory/memory_service.py over-fetches legacy vector hits, filters missing/locked/rejected/invalidated/malformed docs, and preserves vector relevance order after Firestore hydration; this addresses the under-filled search-result failure mode without broadening the public limit.
  • backend/database/memories.py and backend/routers/memories.py now return the exact legacy delete snapshot and compare committed counts before vector cleanup, so delete-all/batch-delete convergence is easier to reason about.
  • backend/utils/task_integrations_ops.py stops treating missing provider task IDs as success and distinguishes ambiguous provider/transport outcomes; the generated Dart/TypeScript/OpenAPI response updates line up with the new error_code, retryable, and ambiguous fields.
  • backend/utils/webhooks.py, backend/utils/http_client.py, and backend/routers/users.py reset webhook failure/circuit-breaker state on explicit re-enable without recording a synthetic delivery success.
  • The added/updated tests cover the main risk surfaces I checked: pusher delivery replay/ack behavior, Redis delivery fencing, speaker-sample retryable outcomes, memory projection convergence, task integration ambiguity, and webhook retry/reset behavior.

I am leaving this for human maintainer review because it changes security/privacy-sensitive delivery, webhook, and user-memory persistence boundaries. The implementation looks coherent from this pass, but the merge decision should include maintainer sign-off on the new boundary semantics and rollout risk.

— Automated maintainer review (glm-5.2)


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

Resolves three weeks of drift. Kept the webhook delivery-health model, task-integrations
hardening, async http and storage infrastructure, and speaker identification delivery.
Dropped the memory-stack and pusher-side pieces that upstream superseded: the universal
memory and task authority convergence rewrote the memory paths this branch hardened, and
PR 10678 landed its own bounded delivery-failure handling for the pusher. Details in the
PR comment. Generated clients and the app-client spec regenerated from the merged backend.
@ZachL111

Copy link
Copy Markdown
Contributor Author

Rebased this onto current main by merging main in and resolving the conflicts. The PR is MERGEABLE again, and its scope is now narrower than when it was opened, because upstream landed overlapping work while it sat conflicting:

Kept (still applies to current main):

  • The webhook delivery-health model: auto-disable state machine in utils/webhooks.py, reset-on-enable via reset_user_webhook_delivery_health, the users router enable/disable behavior, database/webhook_health.py changes, and the unit plus hermetic e2e coverage.
  • Task-integrations hardening: routers/task_integrations.py, utils/task_integrations_ops.py, due-date validation, ops tests, e2e tests, and the regenerated wire and client files for the new response fields.
  • Async infrastructure: utils/http_client.py, utils/app_integrations.py, utils/other/storage.py, database/redis_db.py cache serialization, projection_repair, vector_db, and their tests.
  • Speaker identification delivery (utils/speaker_identification.py plus a new test file).

Dropped as superseded by upstream work that landed after this branch was cut:

  • The memory-stack hardening (memory_service legacy-search limits, vector-write verification, memories router batch changes): main's universal memory and task authority convergence (5724a10) rewrote those code paths, and the old dual-system branches this PR verified no longer exist.
  • The pusher delivery-ack protocol (begin/finish/abandon delivery in routers/pusher.py and the listen session ack client): fix(pusher): bound delivery and runtime failure handling #10678 landed upstream's own delivery-preservation and bounded-failure handling for the same problem and moved the protocol into utils/pusher_protocol.py and utils/pusher_finalization.py, so the ack machinery would have re-implemented a solved problem on top of a moved architecture.
  • The dev API memory lock-enforcement test class that main's convergence commit deliberately removed.

Where a dropped piece still has residual value (for example client-confirmed delivery acks on top of #10678's model), it should be a fresh PR against the current architecture rather than a rebase of this one.

Resolution notes: the merged webhook test suite was adapted to the delivery-health model (reset on enable writes a cleared health hash without claiming an HTTP success, so the e2e disable test asserts the health snapshot is unchanged and that a user disable is not the auto-disable flag). Generated clients and the app-client OpenAPI spec were regenerated from the merged backend with the repo scripts; the regenerated output is byte-identical to the merged state, so every remaining generated delta is backed by the surviving API surface.

…erification

The legacy_memory_projection_convergence workflow entry belonged to the
memory-stack half this PR dropped as superseded by the universal memory
authority: its sources pulled database/memories.py into the tuple-result scan
(flagging functions that live identically on main) and its test list referenced
the deleted mcp search suite. The surviving vector_db delta was the same
dropped surface's write-verification and returned a reported count where main's
contract returns the payload length, failing main's batch-upsert tests; the
file reverts to main wholesale.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app backend Backend Task (python) human Human-authored pull request mobile needs-maintainer-review Needs a human maintainer to sign off before merge privacy-review Touches user-data persistence, permissions, or privacy-sensitive surfaces security-review Touches auth, provider routing, secrets, or security-sensitive surfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants