Repository navigation
fix(relay): reject malformed percent-escapes on upgrade instead of throwing - #18547
Conversation
|
Too many files changed for review (149 files, 100 file limit). Bypass the limit by tagging |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 71dfa156a534c09f5a01089427bcc8e508e58a6c and f86328e9de58c0a06e0ec32f711b911d09905fe6. 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe relay server now safely decodes path segments in the Merge Risk: ⚪ Minimal · up to Malformed relay upgrade paths now receive a client-error response rather than terminating the relay process, while valid upgrades continue to succeed. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the bug, fix, impact, and test results. However, it omits most required template sections, including ELI5, What Changed, Why, Visual Proof or N/A, AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist items. It also does not provide a Linked Issue entry in the required format. Resolution Complete the required template sections. Add an ELI5 summary, explicit What Changed and Why sections, a Linked Issue entry such as Full details: Docstring CoverageExplanation Docstring coverage is 2.94% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 136 functions across 52 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
cloud/apps/relay/src/cell-heartbeat-client.test.ts (1)
55-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative branch for
regionalRehomeProtocol.The test name states "only when its dedicated trust boundary is configured", but the test asserts only the positive case. The
0branch incell-heartbeat-client.ts(Line 95) stays uncovered. A regression that always advertises1would still pass.Add one case that omits
rehomeAudienceandrehomeDirectorServiceAccountand assertsregionalRehomeProtocol: 0.♻️ Proposed additional case
+ it('advertises no per-host drain protocol without the paired trust boundary', async () => { + const requests: RequestInit[] = [] + const client = startCellHeartbeat(CONFIG, { + ready: async () => true, + observedRequests: () => 0, + connectionCounts: () => ({ + totalConnections: 0, + inFlightConnections: 0, + reservedConnectionUnits: 0, + enforcedConnectionUnits: 0 + }), + regionalRehomeSafety: () => ({ + observedAt: 120, + sqlFailures: 0, + reconnects: 0, + controlActivityRecoveryFailures: 0, + databasePoolWaiting: 0, + databasePoolWaitersMax: 0, + databasePoolWaitMsMax: 0 + }), + fetch: async (_input, init) => { + requests.push(init ?? {}) + return new Response('{}', { status: 200 }) + }, + identityToken: async () => 'secret-token', + now: () => 123, + incarnation: '11111111-1111-4111-8111-111111111111', + intervalMs: 60_000 + })! + await vi.waitFor(() => expect(requests).toHaveLength(2)) + client.stop() + + expect(JSON.parse(String(requests[1]!.body))).toMatchObject({ + regionalRehomeProtocol: 0 + }) + })Also applies to: 94-105
cloud/apps/relay/src/relay-connection-ledger.test.ts (1)
153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake this assertion discriminate the behavior it claims.
The test name states that a claimed reservation is not restored after the phone closes. Line 153 cannot prove that. At this point the ledger is empty, so
tryReserveHostDatareturns non-null on both paths: the restored-reservation path and thereserveUpgrade(this.normalAdmissionLimit)fallback inrelay-connection-ledger.ts(Line 146). A regression that wrongly restored the reservation would still pass.Assert the resulting counts instead. A fresh upgrade adds one in-flight unit and leaves
reservedConnectionUnitsat 0. A restored reservation would consume a reserved unit.♻️ Proposed assertion
- expect(ledger.tryReserveHostData('connection-1')).not.toBeNull() + expect(ledger.tryReserveHostData('connection-1')).not.toBeNull() + // A fresh upgrade, not a restored reservation: no reserved unit is consumed. + expect(ledger.counts()).toMatchObject({ + inFlightConnections: 1, + reservedConnectionUnits: 0, + enforcedConnectionUnits: 1 + })cloud/apps/relay/src/host-session-registry.test.ts (1)
144-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the real
activatemember type.
HostSessionRegistry.activateis a private method with the eight-parameter signature shown incloud/apps/relay/src/host-session-registry.ts:625-634. Theas unknown ascast disconnectsboundfrom that declaration, so later signature changes will not produce compile errors at its call sites. Access the member directly and suppress only the private-member diagnostic, or add a typed test seam.cloud/apps/relay/src/postgres-maintenance-sweep-plans.test.ts (1)
39-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the teardown resilient so the 20,000 seeded rows are always removed.
clientis assigned last inbeforeAll. If any earlier statement rejects,clientstaysundefinedandafterAllthrows onclient.end(). The row cleanup anddatabase.close()then never run, and the shared database keeps 20,000sweep-plan-rows that skew plans for the other PostgreSQL suites.Delete the owned rows first, and end the client only if it exists.
♻️ Proposed teardown order
afterAll(async () => { - await client.end() await database.query( `DELETE FROM relay_connection_bases WHERE basis_conn_id LIKE ?`, [`${OWNED_PREFIX}%`] ) + await client?.end() await database.close() })
clientneeds a nullable declaration for this:- let client: pg.Client + let client: pg.Client | undefinedcloud/apps/relay/src/relay-readiness.ts (1)
53-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRelease the JWKS response body after checking its status.
The default Node
fetchpath reads onlyresponse.ok. It does not consume or cancelresponse.bodybefore the SQL query or the non-OK exception. An unconsumed undici body can keep its connection unavailable for reuse on each 10-second probe. Cancel or consume the body in a cleanup path that also runs for non-OK responses.cloud/apps/relay/src/database.ts (2)
767-786: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep pool-level
queryLockedfrom being used for serialization
PostgresDatabase.queryLockedappendsFOR UPDATEand callsthis.query.PostgresDatabase.queryreleases the pooled client after the statement, so PostgreSQL releases the row locks before follow-up operations run. Use a transaction handle for lock-and-update sequences, or make pool-level use fail explicitly.
799-826: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDestroy the client when
ROLLBACKfails.Track whether
ROLLBACKfails, then callclient.release(true).pg.PoolClient.releaseuses a boolean destroy flag. Callingclient.release()can return an aborted client to the pool, where later statements may fail with25P02.cloud/apps/relay/src/cell-admission-migration-registration.ts (1)
278-282: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the per-cell verification queries.
The loop runs three awaited queries for each cell, and
normalizeMigrationCellspermits up to 128 cells. That is up to 384 sequential round trips while the transaction still holds therelay_cellsand selector locks from lines 53 to 56. The extended lock hold time blocks concurrent admission updates.Fetch the region, admission state, and connection limit for all cell ids in three
IN (...)queries before the loop, then compare in memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 62abbf78-fbf1-40ab-a07f-6a02060a511c
📥 Commits
Reviewing files that changed from the base of the PR and between e42c60e and b96d057eda454fb58e625682dcce90a7c181d773.
⛔ Files ignored due to path filters (1)
cloud/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (148)
.github/scripts/check-root-directory-entries.mjs.github/workflows/cloud-verify.yml.oxfmtrc.json.oxlintrc.jsonREADME.mdcloud/.dockerignorecloud/.editorconfigcloud/.gitignorecloud/.gitleaks.tomlcloud/.node-versioncloud/.npmrccloud/.trufflehog-exclude-paths.txtcloud/.trufflehog-include-paths.txtcloud/README.mdcloud/apps/relay/Dockerfilecloud/apps/relay/package.jsoncloud/apps/relay/src/admin-token-verifier.test.tscloud/apps/relay/src/admin-token-verifier.tscloud/apps/relay/src/app.tscloud/apps/relay/src/assignment-cleanup-steps.test.tscloud/apps/relay/src/assignment-cleanup-steps.tscloud/apps/relay/src/assignment-connection-headroom-postgres.test.tscloud/apps/relay/src/assignment-connection-headroom-query.tscloud/apps/relay/src/assignment-connection-headroom.test.tscloud/apps/relay/src/assignment-control-supersession-postgres.test.tscloud/apps/relay/src/assignment-deployment-status-postgres.test.tscloud/apps/relay/src/assignment-identity-queue.test.tscloud/apps/relay/src/assignment-identity-queue.tscloud/apps/relay/src/assignment-inventory-snapshot.test.tscloud/apps/relay/src/assignment-inventory-snapshot.tscloud/apps/relay/src/assignment-rejection-log-window.test.tscloud/apps/relay/src/assignment-rejection-log-window.tscloud/apps/relay/src/assignment-rejection-logging.test.tscloud/apps/relay/src/assignment-store-lock-order.test.tscloud/apps/relay/src/assignment-store.test.tscloud/apps/relay/src/assignment-store.tscloud/apps/relay/src/cell-admission-migration-registration.tscloud/apps/relay/src/cell-admission-selector.test.tscloud/apps/relay/src/cell-admission-selector.tscloud/apps/relay/src/cell-admission-startup-postgres.test.tscloud/apps/relay/src/cell-admission-startup.test.tscloud/apps/relay/src/cell-admission-startup.tscloud/apps/relay/src/cell-connection-hard-cap-consistency.test.tscloud/apps/relay/src/cell-fence-legacy-adoption-postgres.test.tscloud/apps/relay/src/cell-heartbeat-client.test.tscloud/apps/relay/src/cell-heartbeat-client.tscloud/apps/relay/src/config.test.tscloud/apps/relay/src/config.tscloud/apps/relay/src/connection-reservation-debt-retention.test.tscloud/apps/relay/src/control-lease-recovery-postgres.test.tscloud/apps/relay/src/control-renewal-postgres.test.tscloud/apps/relay/src/control-reservation-reconnect-claim.test.tscloud/apps/relay/src/credential-store.test.tscloud/apps/relay/src/credential-store.tscloud/apps/relay/src/database-postgres-timeout.test.tscloud/apps/relay/src/database-transient-error.test.tscloud/apps/relay/src/database.test.tscloud/apps/relay/src/database.tscloud/apps/relay/src/fault-injection-test-entry.tscloud/apps/relay/src/google-metadata-identity-token.tscloud/apps/relay/src/host-session-registry.test.tscloud/apps/relay/src/host-session-registry.tscloud/apps/relay/src/index.tscloud/apps/relay/src/migration-recovery-postgres.test.tscloud/apps/relay/src/observed-relay-database.tscloud/apps/relay/src/postgres-drain-send-locking.test.tscloud/apps/relay/src/postgres-idle-client-error.test.tscloud/apps/relay/src/postgres-maintenance-sweep-plans.test.tscloud/apps/relay/src/postgres-pool-pressure.test.tscloud/apps/relay/src/postgres-pool-pressure.tscloud/apps/relay/src/postgres-schema-concurrency-postgres.test.tscloud/apps/relay/src/postgres-schema-startup.tscloud/apps/relay/src/postgres-transaction-recovery.test.tscloud/apps/relay/src/public-assignment-admission.test.tscloud/apps/relay/src/public-assignment-admission.tscloud/apps/relay/src/public-assignment-circuit-breaker.test.tscloud/apps/relay/src/public-assignment-overload.test.tscloud/apps/relay/src/public-assignment-reconnect-lane.test.tscloud/apps/relay/src/regional-host-drain-app.test.tscloud/apps/relay/src/regional-rehome-postgres.test.tscloud/apps/relay/src/regional-rehome-safety.test.tscloud/apps/relay/src/regional-rehome-safety.tscloud/apps/relay/src/regional-rehome-store.test.tscloud/apps/relay/src/regional-rehome-target-selection.test.tscloud/apps/relay/src/regional-rehome-trust-probe.tscloud/apps/relay/src/regional-rehome-worker.test.tscloud/apps/relay/src/regional-rehome-worker.tscloud/apps/relay/src/registered-migration-abandonment.tscloud/apps/relay/src/registered-migration-inventory-postgres.test.tscloud/apps/relay/src/registered-migration-inventory.test.tscloud/apps/relay/src/registered-migration-inventory.tscloud/apps/relay/src/relay-background-operation.test.tscloud/apps/relay/src/relay-background-operation.tscloud/apps/relay/src/relay-connection-hard-cap.blackbox.test.tscloud/apps/relay/src/relay-connection-ledger.test.tscloud/apps/relay/src/relay-connection-ledger.tscloud/apps/relay/src/relay-first-frame-failure.blackbox.test.tscloud/apps/relay/src/relay-host-log-digest.tscloud/apps/relay/src/relay-host-proof-failure.blackbox.test.tscloud/apps/relay/src/relay-observability.test.tscloud/apps/relay/src/relay-observability.tscloud/apps/relay/src/relay-readiness.test.tscloud/apps/relay/src/relay-readiness.tscloud/apps/relay/src/relay-region-app.test.tscloud/apps/relay/src/relay-region-placement.test.tscloud/apps/relay/src/relay-server.tscloud/apps/relay/src/relay-token-verifier.tscloud/apps/relay/src/relay-upgrade-malformed-uri.blackbox.test.tscloud/apps/relay/src/relay-websocket-close.test.tscloud/apps/relay/src/relay-websocket-close.tscloud/apps/relay/src/relay.blackbox.test.tscloud/apps/relay/src/splice-forwarder.test.tscloud/apps/relay/src/splice-forwarder.tscloud/apps/relay/src/staging-asia-proof-admission.test.tscloud/apps/relay/src/staging-asia-proof-admission.tscloud/apps/relay/tsconfig.build.jsoncloud/apps/relay/tsconfig.jsoncloud/apps/relay/vitest.config.tscloud/package.jsoncloud/packages/relay-contract/package.jsoncloud/packages/relay-contract/src/admission-budgets.tscloud/packages/relay-contract/src/assignment-invariants.tscloud/packages/relay-contract/src/close-codes.tscloud/packages/relay-contract/src/contract.test.tscloud/packages/relay-contract/src/control-continuity.tscloud/packages/relay-contract/src/control-messages.tscloud/packages/relay-contract/src/credential-messages.tscloud/packages/relay-contract/src/director-messages.tscloud/packages/relay-contract/src/host-proof-transcript.tscloud/packages/relay-contract/src/index.tscloud/packages/relay-contract/src/persistence-invariants.test.tscloud/packages/relay-contract/src/persistence-invariants.tscloud/packages/relay-contract/src/protocol-limits.tscloud/packages/relay-contract/src/relay-regions.tscloud/packages/relay-contract/src/resume-confirmation-contract.tscloud/packages/relay-contract/src/splice-state-machine.tscloud/packages/relay-contract/src/wire-scalars.tscloud/packages/relay-contract/tsconfig.build.jsoncloud/packages/relay-contract/tsconfig.jsoncloud/pnpm-workspace.yamlcloud/tsconfig.base.jsonconfig/oxlint-react-doctor.jsonconfig/scripts/check-changed-code-quality.mjsconfig/scripts/check-changed-code-quality.test.mjsconfig/scripts/check-root-directory-entries.test.mjsconfig/scripts/pr-code-change-scope.mjsconfig/scripts/pr-code-change-scope.test.mjspackage.json
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| function encodeCells(cells: NormalizedMigrationCellRegistration[]): string { | ||
| return JSON.stringify( | ||
| cells.map((cell) => | ||
| cell.region === RELAY_DEFAULT_REGION | ||
| ? { | ||
| id: cell.id, | ||
| url: cell.url, | ||
| capacityRequests: cell.capacityRequests, | ||
| connectionHardCap: cell.connectionHardCap, | ||
| connectionUnobservedBound: cell.connectionUnobservedBound | ||
| } | ||
| : cell | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Canonicalize encodeCells for non-default regions.
normalizeMigrationCells preserves caller property order and extra properties with { ...cell, region: ... }. encodeCells passes non-default-region cells directly to JSON.stringify, so equivalent retries can produce different cells_json values and trigger admission_selector_attempt_mismatch. Emit the declared fields in a fixed order and append region only for non-default regions.
| } | ||
| const cells = input | ||
| .map((cell) => ({ ...cell, region: cell.region ?? RELAY_DEFAULT_REGION })) | ||
| .sort((left, right) => left.id.localeCompare(right.id)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a locale-independent order for validated cell IDs.
CellIdSchema accepts any non-empty string up to 128 characters, so Unicode IDs are valid. normalizeMigrationCells uses String.prototype.localeCompare before encodeCells persists the order in cells_json. On a replay of the same admin request, RelayMigrationCellRegistrar.add compares that persisted JSON with newly encoded JSON. A different default locale or ICU dataset can produce a different order and cause admission_selector_attempt_mismatch. Use a code-unit comparison with < and > instead.
| ORCA_RELAY_CELL_CONNECTION_HARD_CAP: z.coerce | ||
| .number() | ||
| .int() | ||
| .pipe(RelayCellConnectionHardCapSchema) | ||
| .optional(), | ||
| ORCA_RELAY_CELL_CONNECTION_UNOBSERVED_BOUND: z.coerce | ||
| .number() | ||
| .int() | ||
| .nonnegative() | ||
| .max(RELAY_MAX_CELL_CONNECTION_UNOBSERVED_BOUND) | ||
| .optional(), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Treat an empty string as unset for the optional numeric variables.
z.coerce.number() converts '' to 0. If a deployment template sets ORCA_RELAY_CELL_CONNECTION_HARD_CAP=, ORCA_RELAY_CELL_CONNECTION_UNOBSERVED_BOUND=, or ORCA_RELAY_DATABASE_POOL_MAX= with an empty value, the value becomes 0 instead of undefined. RelayCellConnectionHardCapSchema rejects 0, and .positive() rejects 0 on line 80, so the relay fails to start rather than using its default.
The optional service-account fields already handle this with z.preprocess on lines 29 to 32 and 71 to 74. Apply the same preprocessing to the optional numeric fields.
🔧 Proposed fix
+const OptionalNumberSchema = <T extends z.ZodTypeAny>(schema: T) =>
+ z.preprocess((value) => (value === '' ? undefined : value), schema.optional())
+- ORCA_RELAY_CELL_CONNECTION_HARD_CAP: z.coerce
- .number()
- .int()
- .pipe(RelayCellConnectionHardCapSchema)
- .optional(),
- ORCA_RELAY_CELL_CONNECTION_UNOBSERVED_BOUND: z.coerce
- .number()
- .int()
- .nonnegative()
- .max(RELAY_MAX_CELL_CONNECTION_UNOBSERVED_BOUND)
- .optional(),
+ ORCA_RELAY_CELL_CONNECTION_HARD_CAP: OptionalNumberSchema(
+ z.coerce.number().int().pipe(RelayCellConnectionHardCapSchema)
+ ),
+ ORCA_RELAY_CELL_CONNECTION_UNOBSERVED_BOUND: OptionalNumberSchema(
+ z.coerce
+ .number()
+ .int()
+ .nonnegative()
+ .max(RELAY_MAX_CELL_CONNECTION_UNOBSERVED_BOUND)
+ ),Apply the same wrapper to ORCA_RELAY_DATABASE_POOL_MAX on line 80.
Also applies to: 80-80
| const adminServiceAccounts = [ | ||
| parsed.ORCA_RELAY_DEPLOY_SERVICE_ACCOUNT, | ||
| parsed.ORCA_RELAY_CAPACITY_SERVICE_ACCOUNT, | ||
| parsed.ORCA_RELAY_ASIA_PROOF_SERVICE_ACCOUNT, | ||
| parsed.ORCA_RELAY_MONITOR_SERVICE_ACCOUNT, | ||
| parsed.ORCA_RELAY_FENCE_SERVICE_ACCOUNT, | ||
| parsed.ORCA_RELAY_FENCE_BROKER_SERVICE_ACCOUNT, | ||
| parsed.ORCA_RELAY_REHOME_DIRECTOR_SERVICE_ACCOUNT | ||
| ].filter((value): value is string => value !== undefined) | ||
| if (new Set(adminServiceAccounts).size !== adminServiceAccounts.length) { | ||
| throw new Error('relay admin service accounts must be distinct') | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check which admin roles each configured service account grants, and whether runtimeServiceAccount is used as a caller identity.
rg -n -C6 'runtimeServiceAccount|fenceServiceAccount|monitorServiceAccount' cloud/apps/relay/src --type=ts -g '!*.test.ts'
fd -t f 'admin-token-verifier.ts' cloud/apps/relay/src --exec cat -n {}Repository: stablyai/orca
Length of output: 15490
🏁 Script executed:
#!/bin/bash
sed -n '205,245p' cloud/apps/relay/src/config.ts
rg -n -C5 'verifyRuntimeToken|createRuntimeTokenVerifier|googleMetadataIdentityToken|runtimeServiceAccount|verifyReadOnlyAdminToken|verifyAdminToken' cloud/apps/relay/src --type=ts -g '!*.test.ts'Repository: stablyai/orca
Length of output: 43325
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/conventions
Length of output: 37475
Authorization Bypass (CWE-269): Improper Privilege Management
Reachability: Internal · Exploitability: Difficult
Include the runtime service account in the distinctness check.
When ORCA_RELAY_RUNTIME_SERVICE_ACCOUNT matches ORCA_RELAY_FENCE_SERVICE_ACCOUNT or ORCA_RELAY_MONITOR_SERVICE_ACCOUNT, the runtime identity also passes that admin verifier. Reject this explicit overlap, but preserve the existing fallback to ORCA_RELAY_DEPLOY_SERVICE_ACCOUNT.
| const configuredCells = z | ||
| .array(RelayCellConfigSchema) | ||
| .max(128) | ||
| .parse(JSON.parse(parsed.ORCA_RELAY_CELLS_JSON) as unknown) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrap the ORCA_RELAY_CELLS_JSON parse in a descriptive error.
JSON.parse on line 252 throws a raw SyntaxError for malformed JSON. The message names no environment variable. Every other invalid value in this loader produces a named error. An operator debugging startup has to guess which of the many variables is malformed.
🔧 Proposed fix
+ let cellsJson: unknown
+ try {
+ cellsJson = JSON.parse(parsed.ORCA_RELAY_CELLS_JSON)
+ } catch {
+ throw new Error('ORCA_RELAY_CELLS_JSON must be valid JSON')
+ }
const configuredCells = z
.array(RelayCellConfigSchema)
.max(128)
- .parse(JSON.parse(parsed.ORCA_RELAY_CELLS_JSON) as unknown)
+ .parse(cellsJson)
.map((cell) => ({ ...cell, url: canonicalOrigin(cell.url, `cell ${cell.id}`) }))| function wrap(transaction: RelayDatabase): RelayDatabase { | ||
| return { | ||
| query: async (sql, params) => { | ||
| if (pattern && !faulted && sql.includes(pattern)) { | ||
| faulted = true | ||
| throw new Error('injected SQL failure') | ||
| } | ||
| return await transaction.query(sql, params) | ||
| }, | ||
| queryLocked: async (sql, params, options) => | ||
| await transaction.queryLocked(sql, params, options), | ||
| transaction: async (operation) => | ||
| await transaction.transaction(async (nested) => await operation(wrap(nested))), | ||
| close: async () => await transaction.close() | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Find code that branches on RelayDatabase.dialect and on transaction options.
rg -nP --type=ts -C3 '\.dialect\b' cloud/apps/relay/src cloud/packages
rg -nP --type=ts -C3 'reportRetries' cloud/apps/relay/srcRepository: stablyai/orca
Length of output: 4119
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- fault-injection wrapper ---'
cat -n cloud/apps/relay/src/fault-injection-test-entry.ts | sed -n '1,90p'
printf '%s\n' '--- database contract and PostgreSQL transaction implementation ---'
cat -n cloud/apps/relay/src/database.ts | sed -n '1,45p'
cat -n cloud/apps/relay/src/database.ts | sed -n '770,825p'
printf '%s\n' '--- sibling wrapper ---'
cat -n cloud/apps/relay/src/observed-relay-database.ts | sed -n '20,55p'
printf '%s\n' '--- dialect-dependent consumer ---'
cat -n cloud/apps/relay/src/assignment-store.ts | sed -n '3265,3285p'Repository: stablyai/orca
Length of output: 8806
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings
Length of output: 22717
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- database factories and dialect assignments ---'
rg -n -C4 'openRelayDatabase|dialect:' cloud/apps/relay/src/database.ts
printf '%s\n' '--- transaction callers that pass options ---'
rg -n -C5 'database\.transaction\([^;\n]*\{|database\.transaction\(' cloud/apps/relay/src --type=ts
printf '%s\n' '--- relay server database flow ---'
rg -n -C3 'createRelayServer|RelayDatabase' cloud/apps/relay/src/relay-server.ts cloud/apps/relay/src/assignment-store.ts --type=ts | head -160Repository: stablyai/orca
Length of output: 50369
Preserve dialect and transaction options in wrap. When the wrapped database is PostgreSQL, AssignmentStore receives undefined for dialect and selects the non-PostgreSQL renewal path. The wrapper also drops options, so reportRetries does not reach PostgresDatabase.transaction.
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined) | ||
|
|
||
| await expect(completionStore.completeReadyEvacuations()).resolves.toBe(0) | ||
| await expect(legacyTransaction).resolves.toBeUndefined() | ||
| expect(warn).not.toHaveBeenCalledWith( | ||
| expect.stringContaining('orca_relay_postgres_transaction_retry') | ||
| ) | ||
| await expect(store.completeReadyEvacuations()).resolves.toBe(1) | ||
| }, 15_000) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore each console.warn spy before its test ends.
The relay Vitest configuration does not enable restoreMocks, and the file’s afterEach does not restore mocks. Add warn.mockRestore() to the tests at lines 956, 1176, and 1285. Otherwise, later tests in the same worker can have relay warnings suppressed.
| if ( | ||
| invite.state === INVITE_STATE.RESERVED && | ||
| (!invite.reservationId || !invite.reservationExpiresAt) | ||
| ) { | ||
| context.addIssue({ code: z.ZodIssueCode.custom, message: 'reserved invite requires a lease' }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Compare reservationExpiresAt with undefined, not falsiness.
!invite.reservationExpiresAt is also true when reservationExpiresAt is 0. A reserved invite whose lease expiry is epoch 0 then fails validation with "reserved invite requires a lease". Line 33 already uses === undefined for cooldownUntil, so the two refinements disagree.
🔧 Proposed fix
if (
invite.state === INVITE_STATE.RESERVED &&
- (!invite.reservationId || !invite.reservationExpiresAt)
+ (invite.reservationId === undefined || invite.reservationExpiresAt === undefined)
) {📝 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.
| if ( | |
| invite.state === INVITE_STATE.RESERVED && | |
| (!invite.reservationId || !invite.reservationExpiresAt) | |
| ) { | |
| context.addIssue({ code: z.ZodIssueCode.custom, message: 'reserved invite requires a lease' }) | |
| } | |
| if ( | |
| invite.state === INVITE_STATE.RESERVED && | |
| (invite.reservationId === undefined || invite.reservationExpiresAt === undefined) | |
| ) { | |
| context.addIssue({ code: z.ZodIssueCode.custom, message: 'reserved invite requires a lease' }) | |
| } |
| Configuration is read from environment variables validated in | ||
| `apps/relay/src/config.ts`. `ORCA_RELAY_ASSIGNMENT_SIGNING_KEY` (at least 32 | ||
| bytes) is the only required value; everything else has a local default. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the list of required environment variables.
EnvSchema in cloud/apps/relay/src/config.ts declares several variables with no default: ORCA_RELAY_PUBLIC_URL, ORCA_RELAY_CELL_URL, ORCA_RELAY_AUTH_ISSUER, ORCA_RELAY_JWKS_URL, ORCA_RELAY_ADMIN_AUDIENCE, and ORCA_RELAY_DEPLOY_SERVICE_ACCOUNT. A developer who sets only ORCA_RELAY_ASSIGNMENT_SIGNING_KEY gets a Zod validation error at startup.
📝 Proposed documentation fix
Configuration is read from environment variables validated in
-`apps/relay/src/config.ts`. `ORCA_RELAY_ASSIGNMENT_SIGNING_KEY` (at least 32
-bytes) is the only required value; everything else has a local default.
+`apps/relay/src/config.ts`. The required values are
+`ORCA_RELAY_ASSIGNMENT_SIGNING_KEY` (at least 32 bytes),
+`ORCA_RELAY_PUBLIC_URL`, `ORCA_RELAY_CELL_URL`, `ORCA_RELAY_AUTH_ISSUER`,
+`ORCA_RELAY_JWKS_URL`, `ORCA_RELAY_ADMIN_AUDIENCE`, and
+`ORCA_RELAY_DEPLOY_SERVICE_ACCOUNT`. Everything else has a local default.📝 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.
| Configuration is read from environment variables validated in | |
| `apps/relay/src/config.ts`. `ORCA_RELAY_ASSIGNMENT_SIGNING_KEY` (at least 32 | |
| bytes) is the only required value; everything else has a local default. | |
| Configuration is read from environment variables validated in | |
| `apps/relay/src/config.ts`. The required values are | |
| `ORCA_RELAY_ASSIGNMENT_SIGNING_KEY` (at least 32 bytes), | |
| `ORCA_RELAY_PUBLIC_URL`, `ORCA_RELAY_CELL_URL`, `ORCA_RELAY_AUTH_ISSUER`, | |
| `ORCA_RELAY_JWKS_URL`, `ORCA_RELAY_ADMIN_AUDIENCE`, and | |
| `ORCA_RELAY_DEPLOY_SERVICE_ACCOUNT`. Everything else has a local default. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The fix is correct and well-tested, but this PR's diff against main is dominated by the unrelated mobile-relay import, not the ~137-line fix. Confirm intent before merging.
Reviewed changes
decodePathSegmenthelper — wrapsdecodeURIComponentin a try/catch and returnsnullonURIError, so a malformed percent-escape becomes a 4xx reject instead of an uncaught throw out of theupgradelistener./v1/connect/site — now rejects onhostId === nullin addition to the existing/^[A-Za-z0-9_-]{16}$/check./v1/host/data/site — swapsdecodeURIComponentfordecodePathSegment; its pre-existing!connId || connId.length > 128already coversnull.relay-upgrade-malformed-uri.blackbox.test.ts— new blackbox test driving the real server over a raw TCP socket: four malformed targets (%,%E0%A4%A,%C0%AFon connect,%on host-data) assert nouncaughtException, every response is 4xx, and a well-formed connect still upgrades to 101. The assertions pin exact behavior (toEqual([])for uncaught,4\d\d/101status), so it genuinely fails when either decode site is reverted.
The fix itself is isolated, minimal, and idiomatic (the bare catch {} matches the surrounding firstPayload style, and 429-for-invalid-hostId preserves the existing anti-enumeration behavior). No code concerns.
⚠️ PR scope: the diff carries the full relay import, not just the fix
The branch is based on 373deb8209 ("chore(relay): import the Orca mobile relay"), which is the head of the now-closed PR #18264 (mergedAt: null). Merging this PR would therefore land the entire cloud/apps/relay/** and cloud/packages/relay-contract/** tree (~43.8k lines) alongside the fix, under a "fix" title and description that never mention it. Notably, the changed root guard script itself comments that the entry "stays listed until it lands on main" — confirming the import is expected to land through its own path, with this branch stacked on top of it.
Technical details
# Resolve PR scope before merging
## Affected sites
- Both `373deb8209` (import) and `b96d057ed` (fix) are ancestors of this branch but not of `main`.
- `main` does not contain `cloud/apps/relay/src/relay-server.ts` (it appears as a new 543-line file in the diff), so the import is genuinely part of this PR's merge surface.
## Required outcome
- If the import lands separately: retarget this branch (or rebase onto `main` once the import merges) so the PR diff is only the fix.
- If the import is meant to land here: retitle and describe the PR accordingly, and make sure the import content itself has been reviewed (its prior PR is closed, unmerged).
## Open questions for the human
- Why did #18264 close without merging, and does that closure state need to be resolved before the import lands here?ℹ️ Nitpicks
- The PR description attributes the buggy code to "#18413", but that merged PR ("add the relay fence broker, ops console, Terraform root, scripts...") does not contain
relay-server.ts(it is absent frommain). ThedecodeURIComponentupgrade listener was introduced by the import in #18264; consider correcting the cross-reference.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
b96d057 to
71dfa15
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
71dfa15 to
f86328e
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Rebased onto
main— the relay import landed inmainvia #18413, so this PR's diff is now a clean 3-file fix (134+/3-) instead of the ~43.8k-line import plus fix flagged in the prior review. cloud/.gitleaks.tomlallowlist — added an entry so the RFC 6455 §1.3 exampleSec-WebSocket-Keynonce the blackbox test sends is not flagged by the gitleaks generic-key rule.relay-upgrade-malformed-uri.blackbox.test.ts— added a comment pointing the nonce at the allowlist. ThedecodePathSegmentfix and all assertions are otherwise unchanged from the prior review, where the fix was already assessed as correct, minimal, and idiomatic.
The prior review's scope concern is resolved (the import is no longer part of this PR's merge surface), and the "#18413" cross-reference is now accurate — the import did land through #18413.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…grade tests Cloud Verify's Secret scan runs gitleaks over --all refs, so the raw-socket upgrade test on fix/relay-upgrade-malformed-uri (#18547) trips every cloud PR's scan until its allowlist reaches main. Land the allowlist here first.
…a startup (#18553) * fix(cloud): retry the committed-winner collision codes in relay schema startup `CREATE TABLE IF NOT EXISTS` only checks the name before the catalog inserts, so the loser of a concurrent CREATE fails in one of two ways depending on timing: on the catalog unique index (23505, which the startup retry already handled) or, when the winner has committed by the time the loser reaches TypeCreate / heap_create_with_catalog, on the name check those routines repeat (42710 duplicate type, 42P07 duplicate relation). The predicate treated the latter as fatal, so a director could fail startup on a table it was about to find present. This is what turned `postgres-schema-concurrency-postgres.test.ts` red on main and on every relay PR (CI's shared runner loses the race more often than a dev box): a throwaway diagnostic run in CI reported 42710 from TypeCreate and 42P07 from heap_create_with_catalog as the only rejection reasons. Treat 42710/42P07 as retryable for `CREATE TABLE IF NOT EXISTS` and 42P07 for `CREATE [UNIQUE] INDEX IF NOT EXISTS`; every other statement shape still fails fast. The concurrency test now runs ten rounds and reports the loser's SQLSTATE instead of a bare boolean. * chore(cloud): allowlist the RFC 6455 example Sec-WebSocket-Key for upgrade tests Cloud Verify's Secret scan runs gitleaks over --all refs, so the raw-socket upgrade test on fix/relay-upgrade-malformed-uri (#18547) trips every cloud PR's scan until its allowlist reaches main. Land the allowlist here first.
…rowing decodeURIComponent on the /v1/connect/ and /v1/host/data/ path segments threw URIError out of the http 'upgrade' listener, which is uncaught and kills the relay process. Any client that sends GET /v1/connect/% could take down a cell (and every connection on it) or a director instance. Pre-existing since the splice landed (orca-cloud #20); not introduced by the import. A malformed escape now takes the existing 4xx reject branch. The blackbox test sends three malformed connect targets and one host-data target to the real server and asserts no uncaughtException fires and a well-formed upgrade still gets 101 afterwards; reverting either site fails it.
f86328e to
08cc38c
Compare
…a startup (stablyai#18553) * fix(cloud): retry the committed-winner collision codes in relay schema startup `CREATE TABLE IF NOT EXISTS` only checks the name before the catalog inserts, so the loser of a concurrent CREATE fails in one of two ways depending on timing: on the catalog unique index (23505, which the startup retry already handled) or, when the winner has committed by the time the loser reaches TypeCreate / heap_create_with_catalog, on the name check those routines repeat (42710 duplicate type, 42P07 duplicate relation). The predicate treated the latter as fatal, so a director could fail startup on a table it was about to find present. This is what turned `postgres-schema-concurrency-postgres.test.ts` red on main and on every relay PR (CI's shared runner loses the race more often than a dev box): a throwaway diagnostic run in CI reported 42710 from TypeCreate and 42P07 from heap_create_with_catalog as the only rejection reasons. Treat 42710/42P07 as retryable for `CREATE TABLE IF NOT EXISTS` and 42P07 for `CREATE [UNIQUE] INDEX IF NOT EXISTS`; every other statement shape still fails fast. The concurrency test now runs ten rounds and reports the loser's SQLSTATE instead of a bare boolean. * chore(cloud): allowlist the RFC 6455 example Sec-WebSocket-Key for upgrade tests Cloud Verify's Secret scan runs gitleaks over --all refs, so the raw-socket upgrade test on fix/relay-upgrade-malformed-uri (stablyai#18547) trips every cloud PR's scan until its allowlist reaches main. Land the allowlist here first.
…rowing (stablyai#18547) decodeURIComponent on the /v1/connect/ and /v1/host/data/ path segments threw URIError out of the http 'upgrade' listener, which is uncaught and kills the relay process. Any client that sends GET /v1/connect/% could take down a cell (and every connection on it) or a director instance. Pre-existing since the splice landed (orca-cloud stablyai#20); not introduced by the import. A malformed escape now takes the existing 4xx reject branch. The blackbox test sends three malformed connect targets and one host-data target to the real server and asserts no uncaughtException fires and a well-formed upgrade still gets 101 afterwards; reverting either site fails it.
Bug
decodeURIComponenton the/v1/connect/<hostId>and/v1/host/data/<connId>path segments inrelay-server.ts'supgradelistener throwsURIError: URI malformedfor any bad percent-escape (%,%E0%A4%A,%C0%AF). Node'supgradeevent has no error boundary and the relay registers nouncaughtExceptionhandler, so the throw kills the process: a cell drops every connection it holds, a director instance restarts. Verified against the real server, not by reading: the new blackbox test captured three process-levelURIErrors from three requests before the fix.Pre-existing since the splice landed (orca-cloud #20, 2026-07-12); the import in #18413 carried it over unchanged. Flagged by an automated review.
Fix
A
decodePathSegmenthelper returnsnullonURIError, and both sites take the existing 4xx reject branch. No behavior change for well-formed targets.Test
relay-upgrade-malformed-uri.blackbox.test.tsstarts the real relay server, sends the malformed targets over a raw TCP socket, asserts nouncaughtException, every response is 4xx, and a well-formed/v1/connect/<16 chars>still gets 101 afterwards. Mutation-checked: reverting the connect site alone fails the test with the three capturedURIErrors.Full relay suite: 47 files / 423 tests pass (Postgres-gated suites skipped, unrelated). Typecheck clean.