fix(insert-event): guard against empty INSERT RETURNING#780
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesEvent Insertion Robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/utils/insert-event.ts (1)
230-238:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove embedding upsert after confirming the existing row still exists.
upsertEmbedding(existing.id, ...)still runs before the reread. If the row is deleted in that window, this path can still fail with FK errors instead of falling through to fresh insert.Proposed fix
if (isSemanticallyEqual(existing, params)) { - await upsertEmbedding(existing.id, params.embedding, sql); const existingRows = await sql` SELECT id, entity_ids, origin_id, title, semantic_type, created_at FROM events WHERE id = ${existing.id} LIMIT 1 `; const existingRow = existingRows[0] as InsertedEvent | undefined; - if (existingRow) return existingRow; + if (existingRow) { + await upsertEmbedding(existing.id, params.embedding, sql); + return existingRow; + } // Race: the existing row was deleted/tombstoned between the🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/utils/insert-event.ts` around lines 230 - 238, The upsertEmbedding call runs before re-reading the event and can trigger FK errors if the event was deleted; move the upsertEmbedding(existing.id, ...) call so it only runs after the SELECT re-read confirms an existingRow is present (i.e., after const existingRow = existingRows[0] ... and after the if (existingRow) return existingRow), and for the fresh-insert path call upsertEmbedding with the newly inserted event id (use the id returned by the insert flow). Update calls to use the confirmed existingRow.id or the inserted id to avoid orphaned embedding FK failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/index.ts`:
- Around line 312-317: The no-auth Origin check currently treats any loopback
origin as same-origin; tighten it by comparing the incoming Origin exactly to
this server's actual origin instead of allowing any localhost/127/::1. Compute
the server origin (e.g., build serverOrigin from the server's hostname and port
or use the canonical origin value your server exposes), then change the
condition around sameOrigin to: sfs === 'same-origin' || sfs === 'none' ||
origin === serverOrigin; keep the trustedNative check (lobuClient) as-is and use
sameOrigin and trustedNative in the existing if (!sameOrigin && !trustedNative)
branch.
In `@packages/server/src/start-local.ts`:
- Around line 421-429: The current early return when otherUserCountRows > 0 can
hide a missing bootstrap user under LOBU_NO_AUTH, causing resolveAuth() to later
block; change the logic in start-local.ts around the BOOTSTRAP_USER_ID check to
explicitly verify the bootstrap user exists before returning, and if
LOBU_NO_AUTH (env var) is enabled and the bootstrap user is not present, fail
fast by throwing an error or exiting so the process won’t start in a broken
state; locate the checks using BOOTSTRAP_USER_ID and the surrounding bootstrap
PAT provisioning code and ensure the new branch triggers a hard failure when
no-auth is set and bootstrap-user is absent so resolveAuth() cannot deadlock.
In `@packages/server/src/workspace/multi-tenant.ts`:
- Around line 117-163: The module-level cache noAuthUserCache must be cleared by
the test teardown hook; update the existing test-clear routine
(clearMultiTenantCachesForTests) to reset noAuthUserCache = null (or add a call
in that function to clear this module's cache) so tests that swap bootstrap rows
or env won't reuse stale no-auth context—look for the
clearMultiTenantCachesForTests function and add logic to set noAuthUserCache to
null or expose a small clearNoAuthUserCache helper and call it from that clear
function.
---
Outside diff comments:
In `@packages/server/src/utils/insert-event.ts`:
- Around line 230-238: The upsertEmbedding call runs before re-reading the event
and can trigger FK errors if the event was deleted; move the
upsertEmbedding(existing.id, ...) call so it only runs after the SELECT re-read
confirms an existingRow is present (i.e., after const existingRow =
existingRows[0] ... and after the if (existingRow) return existingRow), and for
the fresh-insert path call upsertEmbedding with the newly inserted event id (use
the id returned by the insert flow). Update calls to use the confirmed
existingRow.id or the inserted id to avoid orphaned embedding FK failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: abc0c9ee-2de6-46bd-9a17-e34cdcf1ff6b
📒 Files selected for processing (12)
apps/mac/Lobu/AppState.swiftapps/mac/Lobu/ChromeBridgeHost.swiftapps/mac/Lobu/LobuClient.swiftapps/mac/Lobu/LocalLobuRunner.swiftapps/mac/Lobu/MenuBarContent.swiftapps/mac/Lobu/OAuthClient.swiftpackages/server/src/index.tspackages/server/src/server.tspackages/server/src/start-local.tspackages/server/src/utils/insert-event.tspackages/server/src/utils/loopback.tspackages/server/src/workspace/multi-tenant.ts
Comment on lines
+312
to
+317
| const sameOrigin = | ||
| sfs === 'same-origin' || | ||
| sfs === 'none' || | ||
| (origin !== undefined && /^https?:\/\/(?:127\.\d{1,3}\.\d{1,3}\.\d{1,3}|localhost|\[::1\])(?::\d+)?$/.test(origin)); | ||
| const trustedNative = lobuClient !== undefined && lobuClient.length > 0; | ||
| if (!sameOrigin && !trustedNative) { |
There was a problem hiding this comment.
Tighten the no-auth Origin check to the actual server origin.
This currently trusts any loopback Origin, so a page on http://localhost:3000 can mutate http://127.0.0.1:8787 in no-auth mode. That broadens the CSRF trust boundary from same-origin to any local web app.
🔧 Proposed fix
+ const requestOrigin = new URL(c.req.url).origin;
const sameOrigin =
sfs === 'same-origin' ||
sfs === 'none' ||
- (origin !== undefined && /^https?:\/\/(?:127\.\d{1,3}\.\d{1,3}\.\d{1,3}|localhost|\[::1\])(?::\d+)?$/.test(origin));
+ origin === requestOrigin;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const sameOrigin = | |
| sfs === 'same-origin' || | |
| sfs === 'none' || | |
| (origin !== undefined && /^https?:\/\/(?:127\.\d{1,3}\.\d{1,3}\.\d{1,3}|localhost|\[::1\])(?::\d+)?$/.test(origin)); | |
| const trustedNative = lobuClient !== undefined && lobuClient.length > 0; | |
| if (!sameOrigin && !trustedNative) { | |
| const requestOrigin = new URL(c.req.url).origin; | |
| const sameOrigin = | |
| sfs === 'same-origin' || | |
| sfs === 'none' || | |
| origin === requestOrigin; | |
| const trustedNative = lobuClient !== undefined && lobuClient.length > 0; | |
| if (!sameOrigin && !trustedNative) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/index.ts` around lines 312 - 317, The no-auth Origin
check currently treats any loopback origin as same-origin; tighten it by
comparing the incoming Origin exactly to this server's actual origin instead of
allowing any localhost/127/::1. Compute the server origin (e.g., build
serverOrigin from the server's hostname and port or use the canonical origin
value your server exposes), then change the condition around sameOrigin to: sfs
=== 'same-origin' || sfs === 'none' || origin === serverOrigin; keep the
trustedNative check (lobuClient) as-is and use sameOrigin and trustedNative in
the existing if (!sameOrigin && !trustedNative) branch.
Comment on lines
+421
to
429
| const otherUserCountRows = await sql<[{ count: number }]>` | ||
| SELECT count(*)::int AS count FROM "user" WHERE id <> ${BOOTSTRAP_USER_ID} | ||
| `; | ||
| if ((userCountRows[0]?.count ?? 0) > 0) { | ||
| if ((otherUserCountRows[0]?.count ?? 0) > 0) { | ||
| logger.debug( | ||
| { userCount: userCountRows[0]?.count }, | ||
| 'Skipping bootstrap PAT — deployment already has users' | ||
| { userCount: otherUserCountRows[0]?.count }, | ||
| 'Skipping bootstrap PAT — deployment already has non-bootstrap users' | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Fail fast here when no-auth cannot provision its bootstrap identity.
With LOBU_NO_AUTH=1, returning here on an existing local DB leaves bootstrap-user absent, but resolveAuth() only ever resolves that fixed user/org pair. The server still boots and every request gets stuck at 503 server_not_ready.
🔧 Proposed fix
if ((otherUserCountRows[0]?.count ?? 0) > 0) {
+ if (process.env.LOBU_NO_AUTH === '1') {
+ throw new Error(
+ 'LOBU_NO_AUTH=1 requires the bootstrap user/org, but non-bootstrap users already exist'
+ );
+ }
logger.debug(
{ userCount: otherUserCountRows[0]?.count },
'Skipping bootstrap PAT — deployment already has non-bootstrap users'
);
return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/start-local.ts` around lines 421 - 429, The current early
return when otherUserCountRows > 0 can hide a missing bootstrap user under
LOBU_NO_AUTH, causing resolveAuth() to later block; change the logic in
start-local.ts around the BOOTSTRAP_USER_ID check to explicitly verify the
bootstrap user exists before returning, and if LOBU_NO_AUTH (env var) is enabled
and the bootstrap user is not present, fail fast by throwing an error or exiting
so the process won’t start in a broken state; locate the checks using
BOOTSTRAP_USER_ID and the surrounding bootstrap PAT provisioning code and ensure
the new branch triggers a hard failure when no-auth is set and bootstrap-user is
absent so resolveAuth() cannot deadlock.
Comment on lines
+117
to
+163
| let noAuthUserCache: NoAuthUser | null = null; | ||
|
|
||
| interface NoAuthUser { | ||
| userId: string; | ||
| organizationId: string; | ||
| user: { id: string; email: string; name: string; username: string }; | ||
| } | ||
|
|
||
| /// Resolve the single local user attributed to every request in no-auth | ||
| /// mode. Pulls the bootstrap-user row + the bootstrap-org membership by | ||
| /// **id pair**, not "first admin LIMIT 1" — if the bootstrap user ever | ||
| /// has multiple memberships (e.g. someone manually added them to another | ||
| /// org for testing) we still want the personal org, deterministically. | ||
| /// Returns `null` only when the row truly doesn't exist (server boot race | ||
| /// between HTTP listen and ensureBootstrapPat's await — `start-local.ts` | ||
| /// now runs the bootstrap BEFORE listen() to close that race). | ||
| async function getNoAuthUser( | ||
| sql: ReturnType<typeof getDb> | ||
| ): Promise<NoAuthUser | null> { | ||
| if (noAuthUserCache) return noAuthUserCache; | ||
| const rows = await simpleQuery(sql` | ||
| SELECT | ||
| u.id AS user_id, | ||
| u.email AS email, | ||
| u.name AS name, | ||
| u.username AS username | ||
| FROM "user" u | ||
| JOIN "member" m ON m."userId" = u.id | ||
| WHERE u.id = ${NO_AUTH_USER_ID} | ||
| AND m."organizationId" = ${NO_AUTH_ORG_ID} | ||
| AND m.role IN ('owner', 'admin') | ||
| LIMIT 1 | ||
| `); | ||
| if (rows.length === 0) return null; | ||
| const row = rows[0]; | ||
| noAuthUserCache = { | ||
| userId: row.user_id as string, | ||
| organizationId: NO_AUTH_ORG_ID, | ||
| user: { | ||
| id: row.user_id as string, | ||
| email: (row.email as string) ?? '', | ||
| name: (row.name as string) ?? '', | ||
| username: (row.username as string) ?? '', | ||
| }, | ||
| }; | ||
| return noAuthUserCache; | ||
| } |
There was a problem hiding this comment.
Reset noAuthUserCache from the existing test clear hook.
This cache is module-local and never cleared by clearMultiTenantCachesForTests, so once one test resolves the bootstrap user, later cases can reuse stale no-auth auth context after swapping rows or env.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/workspace/multi-tenant.ts` around lines 117 - 163, The
module-level cache noAuthUserCache must be cleared by the test teardown hook;
update the existing test-clear routine (clearMultiTenantCachesForTests) to reset
noAuthUserCache = null (or add a call in that function to clear this module's
cache) so tests that swap bootstrap rows or env won't reuse stale no-auth
context—look for the clearMultiTenantCachesForTests function and add logic to
set noAuthUserCache to null or expose a small clearNoAuthUserCache helper and
call it from that clear function.
`insertEvent` assumed `INSERT ... RETURNING` always yields a row, then dereferenced `result[0].id` to feed `upsertEmbedding`. When the result came back empty — exposed by the menu bar's no-auth sync round-trip completing for the first time — the call crashed with the cryptic `TypeError: Cannot read properties of undefined (reading 'id')` and the worker stream returned 500. Two defensive guards: 1. After the main INSERT path, treat `result[0]` as possibly undefined. Log connector/feed/origin context and throw a real error explaining the row wasn't persisted. Callers (worker streamContent) catch the throw and surface a useful message instead of the stack-trace blob. 2. In the conflict-update early-return path, `existingRows[0]` could also be undefined if the found row was deleted/tombstoned between `findCurrentEventByOrigin` and the subsequent reread (race). Fall through to the fresh-insert path instead of crashing. Root cause of the original empty-result is still under investigation — likely a PGlite quirk with the new `search_tsv tsvector GENERATED ALWAYS AS (...) STORED` column from PR #765, or a transient state during bootstrap-org/connector wire-up. Either way, the right behavior is to fail loud with context rather than crash on `undefined.id`.
The conflict-update early-return path called upsertEmbedding(existing.id, ...) before the SELECT that confirms the row is still there. If the row got deleted/tombstoned in that window, upsertEmbedding would FK-fail with a confusing error instead of letting us fall through to a fresh insert. Reorder so the embedding upsert only fires when the reread confirms the event still exists. Fall-through behavior on the race is unchanged. The 3 other CodeRabbit comments on this PR were against code from PR #779 (now merged) — those will land as separate follow-ups since they're out of scope here.
buremba
force-pushed
the
fix/insert-event-empty-returning
branch
from
a32985c to
c8715e1
Compare
This was referenced May 17, 2026
buremba
added a commit
that referenced
this pull request
May 17, 2026
Three deferred items from CodeRabbit's review of the no-auth work that were outside that PR's diff but in scope for follow-up: 1. CSRF middleware (index.ts): Origin check was any-loopback-shape, which let `http://localhost:9999` (or any malicious tab loaded from another loopback port) pass as same-origin against our `:8787`. Tighten to an exact match against the canonical origin derived from the validated Host header. 2. Bootstrap (start-local.ts): if LOBU_NO_AUTH=1 is set but the bootstrap user doesn't exist because OTHER (non-bootstrap) users already populate the deployment, the previous code silently skipped bootstrap. resolveAuth() would then return 503 for every request — visible only at request time, with no clear recovery path. Fail loud at startup instead, with a message naming both the cause (env vs. existing users) and the fix (clear data dir or unset env). 3. multi-tenant.ts: `noAuthUserCache` is a module-level variable inside this file, not in `multi-tenant-caches.ts` where the shared test teardown lives. Wrap the shared cache-clearer with a local version that also resets noAuthUserCache so tests that swap bootstrap rows / env between cases don't see stale identity.
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
2 participants
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.
Summary
`insertEvent` assumed `INSERT ... RETURNING` always yields a row, then dereferenced `result[0].id`. When the result came back empty (exposed by the menu bar's no-auth sync round-trip completing for the first time in PR #779) the call crashed with the cryptic `TypeError: Cannot read properties of undefined (reading 'id')` and the worker stream handler returned 500.
This adds two defensive guards in `packages/server/src/utils/insert-event.ts`:
Main INSERT path — treat `result[0]` as possibly undefined, log connector/feed/origin context, throw a real error explaining the row wasn't persisted. Callers (worker `streamContent`) catch the throw and surface a useful message instead of the stack-trace blob.
Conflict-update early-return — `existingRows[0]` could also be undefined if the found row was deleted/tombstoned between `findCurrentEventByOrigin` and the subsequent reread (race). Fall through to the fresh-insert path instead of crashing.
Root cause of the empty result
Still under investigation. Most likely culprit: PGlite quirk with the new `search_tsv tsvector GENERATED ALWAYS AS (...) STORED` column from PR #765, or a transient state during bootstrap-org/connector wire-up. The right behavior either way is to fail loud with context rather than silently crash on `undefined.id`.
Test plan
Summary by CodeRabbit
Release Notes