feat(ipc): add db_proxy_transaction for atomic gateway-orchestrated writes#29886
Merged
Conversation
…ated writes Companion to db_proxy. Lets the gateway commit multiple write statements to the assistant SQLite DB inside a single BEGIN IMMEDIATE transaction. All steps commit together; any throw — including a requireChanges constraint failure — rolls the entire batch back. Motivating use case: gateway-driven invite redemption (upsert contact channel + record invite use must be atomic). Stays useful as a cutover primitive even after individual tables migrate to the gateway DB. - New IPC route registered alongside db_proxy (intentionally not in ROUTES; private gateway↔assistant detail). - requireChanges per-step guard for stale-write detection (e.g. 'increment use_count only if status active and uses < max'). - Gateway helper assistantDbTransaction() in assistant-db-proxy.ts. - 6 unit tests covering commit / SQL-error rollback / requireChanges abort / requireChanges pass / empty steps / lastInsertRowid.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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: 49aff7d451
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+116
to
+120
| const result = await ipcCallAssistant("db_proxy_transaction", { steps }); | ||
| if (result === undefined) { | ||
| throw new Error( | ||
| "db_proxy_transaction IPC call failed — assistant may not be ready", | ||
| ); |
There was a problem hiding this comment.
assistantDbTransaction() treats any non-success response from ipcCallAssistant() as undefined and then throws "assistant may not be ready", but ipcCallAssistant() also returns undefined for handler-level errors (including SQL constraint/statement failures) in gateway/src/ipc/assistant-client.ts (msg.error path). As a result, real transaction errors are misclassified as assistant unavailability, which can drive incorrect retry/fallback behavior and makes debugging transactional failures much harder.
Useful? React with 👍 / 👎.
dvargasfuertes
approved these changes
May 7, 2026
dvargasfuertes
pushed a commit
that referenced
this pull request
May 8, 2026
…29890) * fix(db-proxy-transaction): preserve handler errors instead of masking as transport loss Codex review on #29886 caught that assistantDbTransaction() treats every non-success response from ipcCallAssistant() as a transport failure and throws 'assistant may not be ready'. But ipcCallAssistant() also returns undefined on handler-level errors (msg.error path) — including SQL constraint violations. Real transaction errors were being misclassified as assistant unavailability, masking the underlying SQL error and breaking debuggability + retry decisions. Fix: - assistant: db_proxy_transaction now wraps caught errors in RouteError with statusCode 500 + DB_PROXY_TRANSACTION_FAILED code, preserving the original SQL message. Empty-steps validation throws a 400 RouteError (INVALID_PARAMS) instead of a bare Error. - gateway: assistantDbTransaction() switches to ipcCallAssistantStrict so handler errors come through as IpcHandlerError (carrying the SQL message + statusCode) and transport errors as IpcTransportError. - Tests updated to assert RouteError shape + that the original SQL constraint message survives wrapping. * feat(gateway): mirror assistant_ingress_invites schema in gateway DB Add the assistant_ingress_invites table to the gateway DB so future PRs can migrate invite redemption (currently in invite-redemption-service.ts) into the gateway process. With this schema in place plus contacts / contact_channels already gateway-side, voice-invite redemption can upsert the contact channel and record invite use in a single LOCAL gateway-DB transaction instead of round-tripping the assistant DB. Schema mirrors assistant/src/memory/schema/contacts.ts:assistantIngressInvites exactly, with two additions: * contactId now has a real FK on contacts.id (ON DELETE CASCADE). The assistant-side schema only has the column without a reference because contacts live in a different DB on that side. * Four indexes covering the active query patterns from invite-store.ts: - idx_..._voice_lookup (source_channel, status, expected_external_user_id) for findActiveVoiceInvites - idx_..._code_lookup (invite_code_hash, source_channel) for findByInviteCodeHash - idx_..._token_hash (token_hash) for findByTokenHash - idx_..._contact (contact_id) for contact-scoped lookups No data migration: voice invites have a TTL of 24h, so aging out during the cutover window is acceptable. The table starts empty in the gateway DB; reads/writes will continue going to the assistant DB until follow-up PRs migrate the invite-creation and invite-redemption code paths. Schema test verifies columns, NOT NULL constraints, defaults (max_uses=1, use_count=0, status='active'), FK cascade behaviour, and index column composition. * address review feedback: simplify ingress_invites schema - Rename table assistant_ingress_invites -> ingress_invites; drop the 'assistant_' prefix on the gateway side. Indexes renamed to match. - Drop source_conversation_id (daemon-only concept; conversations don't exist in the gateway). - Drop voice-specific columns (expected_external_user_id, voice_code_hash, voice_code_digits) and the voice_lookup index. Make the table channel-agnostic — voice flows can locate the right invite via contact_channels.external_user_id + contact_id rather than embedded columns here. - Drop friend_name and guardian_name; rely on contacts.display_name. - Remove the schema test file. Schema tests against drizzle definitions re-state the schema in another shape and don't catch real bugs. Survives: id, source_channel, token_hash (URL token), invite_code_hash (optional short typeable code), note, max_uses/use_count, expires_at, status, redeemed_by_*, contact_id (FK CASCADE), created_at, updated_at. * feat(gateway): drop tokenHash and inline comments from ingressInvites Per review: ingressInvites only ever held tokenHash transiently before the redemption path moved to gateway-managed bindings. Now that the schema reflects gateway ownership, only inviteCodeHash is needed. - Remove tokenHash column and idx_ingress_invites_token_lookup - Strip header/inline comments on the ingressInvites table - Keep idx_ingress_invites_code_lookup on (inviteCodeHash, sourceChannel) and idx_ingress_invites_contact on contactId * refactor(gateway): make ipcCallAssistant always strict; collapse Strict variant Per review: the lenient ipcCallAssistant was a transitional shim. Every call site either (a) relied on the strict variant already, or (b) fired and forgot — neither case wants silent error swallowing. This replaces the lenient overload with a single strict implementation that throws either IpcHandlerError (preserving statusCode + code) or IpcTransportError, and migrates every caller in this PR. Caller migrations: - assistantDbTransaction now calls ipcCallAssistant directly (no Strict alias) - ipc-runtime-proxy uses ipcCallAssistant (was already effectively strict) - contacts-control-plane-proxy emit_event becomes void ipcCallAssistant(...).catch(() => {}) since fire-and-forget - handle-inbound touchContactChannelStats casts result and checks result.rows?.length; outer caller still fire-and-forget - voice-approval-sync follows the same shape - contact-prompt resolves errors through a new notifyDaemonResolveError helper that wraps the IPC in try/catch so resolve failures don't reject the HTTP response - post-assistant-ready waitForAssistant catches IpcTransportError during health polling but rethrows everything else Tests: - assistant-client.test.ts: replace 'returns undefined' tests with rejects.toBeInstanceOf(IpcTransportError); add IpcHandlerError test covering message/statusCode/code propagation; gate ASSISTANT_IPC_SOCKET_DIR in setup so tests don't hit the real daemon - ipc-runtime-proxy.test.ts: collapse dual mocks into a single ipcCallAssistantMock with a defaultIpcImpl Also fix lint import sort in assistant/src/ipc/routes/db-proxy-transaction.ts. --------- Co-authored-by: vellum-apollo-bot[bot] <242025090+vellum-apollo-bot[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion to
db_proxy. Lets the gateway commit multiple write statements against the assistant SQLite DB inside a singleBEGIN IMMEDIATEtransaction. All steps commit together; any throw — including arequireChangesconstraint failure — rolls the entire batch back.Motivation
Some gateway-orchestrated writes have to be all-or-nothing. The next concrete use case is voice invite redemption:
upsertContactChannelandrecordInviteUsecurrently sit inside one SQLite transaction ininvite-redemption-service.ts, and any migration of that to gateway control needs the same atomicity guarantee.This route stays useful as a cutover primitive even after individual tables migrate to the gateway DB — there will likely be transitional periods where a single logical operation has to write to both DBs (or to multiple assistant-DB tables that haven't fully moved yet).
What's in here
db_proxy_transactioninassistant/src/ipc/routes/db-proxy-transaction.ts. Registered directly inassistant-server.tsalongsidedb_proxy— intentionally NOT inROUTES(private gateway↔assistant detail, not part of the OpenAPI surface).requireChangesper-step guard — abort the transaction if a step affects fewer than N rows. Used for stale-write detection:UPDATE … WHERE status='active' AND uses < maxwithrequireChanges: 1cleanly aborts when the row no longer matches.assistantDbTransaction()ingateway/src/db/assistant-db-proxy.ts.requireChangesabort + rollback,requireChangeshappy path, empty-steps validation,lastInsertRowidreturn.Limitations (by design)
Read-modify-write across steps is not supported — the IPC is one-shot, later steps can't react to earlier results. Use SQL-level conditions (WHERE clauses, ON CONFLICT) plus
requireChangesfor stale-write detection. If you need true read-modify-write, do the read on the gateway side, then call this route with the resulting writes.Tests