fix(engine): observer-token name conflict returns 500 on D1, not 409#232
Conversation
…not 500) POST /v1/observer-tokens re-minting an existing name returns an uncaught 500 in production instead of a clean 409 observer_token_name_conflict, because isObserverTokenNameConflict() only recognized better-sqlite3's error shape (.code === 'SQLITE_CONSTRAINT*', raw "UNIQUE constraint failed: ..." message). The hosted engine runs on Cloudflare D1 via drizzle-orm/d1, which prefixes the message with "D1_ERROR: " and which drizzle-orm may re-wrap under `.cause` instead of surfacing at the top level — this engine already confirmed and handled that exact shape in engine/agent.ts's isUniqueConstraintError (#193), for the identical class of bug. Widen the check to recurse into `.cause` and fall back to a broad, case-insensitive "unique constraint" message match, while keeping the existing index/column-specific substring checks, so a future driver error-format shift doesn't silently reopen this again. Adds a focused unit test (observerToken.conflictDetection.test.ts) exercising the better-sqlite3 shape, the D1 top-level shape, and the drizzle .cause-wrapped D1 shape directly against the exported detection function, plus negative cases (unrelated errors, circular .cause, non-object input). The existing end-to-end conformance test already covers the better-sqlite3 path through the real HTTP route and continues to pass unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors isObserverTokenNameConflict to robustly detect unique-constraint violations on the observer_tokens table across different SQLite drivers (such as better-sqlite3 and Cloudflare D1), including recursing into .cause wrappers, and adds a comprehensive test suite. The review feedback points out a potential stack overflow vulnerability with multi-step circular references in the error's .cause chain, and warns about false positives where other SQLite constraints might be incorrectly classified as name conflicts. It is recommended to use an iterative approach with a Set to track visited errors and to tighten the constraint checks, as well as adding a test case for multi-step circular references.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function isObserverTokenNameConflict(err: unknown): boolean { | ||
| if (!err || typeof err !== 'object') return false; | ||
| const candidate = err as { code?: string; message?: string; cause?: unknown }; | ||
| const message = candidate.message ?? ''; | ||
| return ( | ||
| const isNameIndexMessage = ( | ||
| message.includes('observer_tokens_workspace_name_unique') | ||
| || message.includes('observer_tokens.workspace_id, observer_tokens.name') | ||
| ); | ||
| const isGenericUniqueViolation = ( | ||
| candidate.code === 'SQLITE_CONSTRAINT' | ||
| || candidate.code === 'SQLITE_CONSTRAINT_UNIQUE' | ||
| || message.includes('observer_tokens_workspace_name_unique') | ||
| || message.includes('observer_tokens.workspace_id, observer_tokens.name') | ||
| || message.toLowerCase().includes('unique constraint failed') | ||
| ); | ||
| if (isNameIndexMessage || isGenericUniqueViolation) return true; | ||
| if (candidate.cause && candidate.cause !== err) return isObserverTokenNameConflict(candidate.cause); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
There are two issues with the current implementation of isObserverTokenNameConflict:
- Potential Stack Overflow / Infinite Recursion: The check
candidate.cause !== erronly guards against direct self-referential cycles of length 1 (e.g.,A -> A). If there is a multi-step circular reference (e.g.,A -> B -> A), it will bypass this check and cause aRangeError: Maximum call stack size exceededstack overflow. Using an iterative loop with aSetto track visited errors completely avoids recursion and safely handles any circular reference chain. - False Positives for Other Constraints: Checking
candidate.code === 'SQLITE_CONSTRAINT'without verifying that the message contains a unique constraint keyword will incorrectly classify other SQLite constraint violations (such asFOREIGN KEY constraint failedorNOT NULL constraint failed) as name conflicts. This would lead to a misleading409 observer_token_name_conflictinstead of letting the correct error propagate.
Here is a suggested refactoring that addresses both issues:
export function isObserverTokenNameConflict(err: unknown): boolean {
const visited = new Set<unknown>();
let current = err;
while (current && typeof current === 'object') {
if (visited.has(current)) {
break;
}
visited.add(current);
const candidate = current as { code?: string; message?: string; cause?: unknown };
const message = candidate.message ?? '';
const isNameIndexMessage = (
message.includes('observer_tokens_workspace_name_unique')
|| message.includes('observer_tokens.workspace_id, observer_tokens.name')
);
const isGenericUniqueViolation = (
candidate.code === 'SQLITE_CONSTRAINT_UNIQUE'
|| message.toLowerCase().includes('unique constraint failed')
|| (candidate.code === 'SQLITE_CONSTRAINT' && message.toLowerCase().includes('unique'))
);
if (isNameIndexMessage || isGenericUniqueViolation) {
return true;
}
current = candidate.cause;
}
return false;
}There was a problem hiding this comment.
Confirmed both points and fixed in cd103ad.
-
Stack overflow: rewrote
isObserverTokenNameConflictas an iterative loop tracking every visited error object in aWeakSet, breaking on any revisit (not justcause === err). Added a regression test for exactly theA -> B -> Acase you flagged — it asserts both that it doesn't throw and that it correctly resolves tofalse(not just that it terminates), plus a test that a real conflict earlier in the chain still resolvestrueeven when the tail of the chain cycles. -
Bare
SQLITE_CONSTRAINTfalse positives: verified this empirically against the installed better-sqlite3 version before deciding how far to tighten it — FK, NOT NULL, and CHECK violations each throw their own distinct extended code (SQLITE_CONSTRAINT_FOREIGNKEY/_NOTNULL/_CHECK), never the bareSQLITE_CONSTRAINT, and the D1 shape already confirmed inengine/agent.tsalso uses the extended_UNIQUEcode. So this wasn't an observed false positive on either driver this engine actually runs on today. I tightened it anyway per your suggestion, as defense-in-depth: the bare code now only counts as a match when the message also mentions "unique", same as your suggested implementation. Documented both the empirical check and the reasoning in the function's doc comment.
Full suite (typecheck/lint/test) green, 221/221 tests in packages/engine.
| it('does not recurse infinitely or throw on a self-referential .cause', () => { | ||
| const circular: { message: string; cause?: unknown } = { message: 'wrapped' }; | ||
| circular.cause = circular; | ||
| expect(() => isObserverTokenNameConflict(circular)).not.toThrow(); | ||
| expect(isObserverTokenNameConflict(circular)).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Add a test case to verify that multi-step circular references (e.g., A -> B -> A) do not cause infinite recursion or stack overflow.
it('does not recurse infinitely or throw on a self-referential .cause', () => {
const circular: { message: string; cause?: unknown } = { message: 'wrapped' };
circular.cause = circular;
expect(() => isObserverTokenNameConflict(circular)).not.toThrow();
expect(isObserverTokenNameConflict(circular)).toBe(false);
const a: any = { message: 'A' };
const b: any = { message: 'B' };
a.cause = b;
b.cause = a;
expect(() => isObserverTokenNameConflict(a)).not.toThrow();
expect(isObserverTokenNameConflict(a)).toBe(false);
});There was a problem hiding this comment.
Added, in cd103ad — "does not stack-overflow on a multi-step .cause cycle (A -> B -> A)". It asserts both that it doesn't throw and that the result is false (not just termination), and I added a companion test confirming a real conflict earlier in the chain still resolves true even when the tail of the chain cycles, so the cycle guard doesn't accidentally mask genuine conflicts either. Full suite green: 221/221 tests in packages/engine.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…SQLITE_CONSTRAINT match Addresses PR review from gemini-code-assist and cubic-dev-ai on #232. 1. isObserverTokenNameConflict's .cause walk was recursive and only guarded against a direct self-reference (cause === err). A multi-step cycle (A -> B -> A) bypasses that guard and blows the call stack with RangeError: Maximum call stack size exceeded — the exact check meant to turn an uncaught 500 into a clean 409 would itself crash into one. Rewritten as an iterative loop tracking every visited error object in a WeakSet, breaking on any revisit rather than only a direct self-reference. 2. Verified empirically (against the installed better-sqlite3 version) that FOREIGN KEY, NOT NULL, and CHECK constraint violations each throw their own distinct extended code (SQLITE_CONSTRAINT_FOREIGNKEY/_NOTNULL/_CHECK), never the bare SQLITE_CONSTRAINT — and the D1 shape already confirmed in engine/agent.ts also uses the extended _UNIQUE code. So the bare 'SQLITE_CONSTRAINT' match was not observed to cause any real false positive on the drivers this engine runs on. Tightened it anyway as defense-in-depth per review: now only matches when the message also mentions "unique", so an unrelated constraint failure on the same insert (e.g. workspace_id FK) can no longer be misreported as a name conflict. Test additions in observerToken.conflictDetection.test.ts: - multi-step .cause cycle (A -> B -> A): asserts no throw AND asserts the final result is `false` (not just that it terminates) - a real conflict preceding a multi-step cycle still resolves `true` - bare SQLITE_CONSTRAINT with a non-unique message (FK/NOT NULL) resolves `false` Full suite (typecheck/lint/test) green: 221/221 tests passing in packages/engine. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Bug
POST /v1/observer-tokensre-minting an existing name (e.g. Pear's broker callingRelaycastHttpClient::create_observer_tokenwith the fixed default namepear-dashboard-observermore than once for the same workspace) returns an uncaught500in production instead of a clean409 observer_token_name_conflict. Reproduced live: the first mint for a workspace succeeds; a later mint under the same name fails with a client-side "Max retries exceeded" (the Rust SDK retrying on 5xx and giving up).Root cause:
isObserverTokenNameConflict()inpackages/engine/src/engine/observerToken.tsonly recognized better-sqlite3's error shape (.code === 'SQLITE_CONSTRAINT*', raw"UNIQUE constraint failed: ..."message). Self-hosted engine deployments use better-sqlite3 (so the existing conformance test for this passes), but the hosted engine (relaycast-cloud) runs on Cloudflare D1 viadrizzle-orm/d1, which throws a differently-shaped error, so the conflict was never caught there and fell through as an uncaught exception → 500.D1 error shape — evidence
I didn't have to guess or spin up a D1 emulator: this exact class of bug was already hit and fixed once before in this codebase, in
engine/agent.ts'sisUniqueConstraintError(added in #193, "bounded-durable mailbox, location routing, §5 cleanup"). That commit's message documents the confirmed shape:.codeisSQLITE_CONSTRAINT_UNIQUE(same as better-sqlite3's code convention).messageis prefixed:D1_ERROR: UNIQUE constraint failed: <table>.<col1>, <table>.<col2>(not the raw better-sqlite3 message).causeinstead of surfacing it at the top levelobserverToken.ts's check predated that fix and was never updated to match, so it silently rotted for this one table's insert path whileagent.ts's equivalent check (foragent_already_exists) was already correct.Fix
Widened
isObserverTokenNameConflict(now exported for direct unit testing) to:.codechecks (unchanged)"unique constraint failed"message fallback.cause(guarded against self-referential cycles)This covers the D1 shape (including drizzle's
.causewrapping) without narrowing or replacing the self-hosted (better-sqlite3) behavior.Tests
packages/engine/src/engine/observerToken.conflictDetection.test.ts: unit-tests the exported detection function directly against the better-sqlite3 shape, the D1 top-level shape, the drizzle-.cause-wrapped D1 shape, and negative cases (unrelated errors, circular.cause, non-object/null input) — shapes that can't be produced by this package's better-sqlite3-backed test harness, so a real D1 emulator isn't needed to verify them.src/__tests__/conformance/observerToken.test.ts("returns a conflict for duplicate observer token names in one workspace") continues to pass unchanged — end-to-end coverage of the better-sqlite3 path through the real HTTP route, confirming no self-hosted regression.npm run typecheck,npm run lint,npm testall pass (218/218 tests,packages/engine).Design question: should re-minting under an existing name just work (upsert), instead of erroring?
Investigated the existing patterns in this codebase before deciding:
engine/agent.ts'sregisterAgenttreats a duplicate(workspace_id, name)the same way: a strict409 agent_already_exists, not an upsert.engine/reaction.ts'saddReactionis the one place in this codebase that does treat a unique-constraint hit as idempotent-success (same agent/message/emoji reacting twice is genuinely the same action, so it re-reads and returns the existing row).Observer tokens are not like reactions: minting one produces a genuinely new secret, and the plaintext token is only ever returned once (at create/rotate time) — the row's
tokenHashis all that persists. IfcreateObserverTokenreturned the existing row on a name conflict, the caller would get token metadata with no usable secret (defeats the purpose of calling create). If it silently rotated the existing row instead, that's a meaningful, surprising side effect on aPOST .../observer-tokens"create" call — it invalidates whatever consumer is currently using the old secret under that name, with no explicit opt-in.So I kept
POST /v1/observer-tokensstrict/create-only (409on conflict) — this matches theagent.tsprecedent for named-unique resources, is what the existing conformance test already asserts, and avoids a surprising implicit-invalidation side effect on a plain POST. I did not add new upsert/on_conflictAPI surface in this PR.The actual fix for "make this far less likely to occur" is caller-side:
list_observer_tokensandrotate_observer_tokenalready exist and fully support an idempotent-mint pattern — list first, and if a token with that name exists, callrotate_observer_token(id)to get a fresh usable secret instead of blindlycreate_observer_token-ing every time. This requires a change in therelayrepo (whereverRelaycastHttpClient::create_observer_tokenis called for Pear's dashboard, e.g. its broker), not in this engine — flagging as a manual follow-up since it's out of scope for this repo/PR.Additional finding (not fixed here, flagged for follow-up)
While verifying this, I confirmed (via a scratch test against the real HTTP route) that
observer_tokens_workspace_name_uniqueis an unconditional unique index on(workspace_id, name)— not scoped tostatus = 'active'. That means revoking a token does not free up its name for reuse; a later create under the same name still 409s forever, even on the already-passing better-sqlite3 path. Making that index partial (WHERE status = 'active') would need a schema migration on a table that's live on Cloudflare D1 in production — bigger surface than this bug-fix PR, so I'm calling it out here for a human to decide on separately rather than bundling it in.Scope note
No changes needed in
relaycast-cloud(the only place D1 is actually wired up) — it consumes@relaycast/engineas a plain npm dependency with no override of this function, so once this fix ships in a new@relaycast/enginerelease,relaycast-cloudneeds its dependency bumped to pick it up. I can't publish that npm release myself — a human needs to do the version bump/publish, then bump the dependency inrelaycast-cloud, as a manual follow-up after this PR merges.