fix: use mutex for compute builders #504
Conversation
WalkthroughThis change introduces Redis-based distributed mutex locking for selector compute operations in the database package. It adds Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SelectorComputeMutex
participant Redis
participant Builder
Caller->>Builder: releaseTargets() / resourceSelectors()
Builder->>SelectorComputeMutex: createAndAcquireMutex(type, workspaceId)
SelectorComputeMutex->>Redis: Acquire lock
Redis-->>SelectorComputeMutex: Lock acquired
SelectorComputeMutex-->>Builder: Mutex instance
Builder->>Builder: Perform transactional compute logic
Builder->>SelectorComputeMutex: unlock()
SelectorComputeMutex->>Redis: Release lock
Redis-->>SelectorComputeMutex: Lock released
SelectorComputeMutex-->>Builder: Unlock complete
Builder-->>Caller: Promise resolved or error thrown
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/db/src/config.ts (1)
16-16: Consider providing a default value for REDIS_URLUnlike POSTGRES_URL, REDIS_URL doesn't have a default value, making it a required environment variable. While this ensures explicit configuration, it could cause issues in development environments if not properly set.
Consider adding a development default for easier local setup:
- REDIS_URL: z.string().url(), + REDIS_URL: z + .string() + .url() + .default("redis://localhost:6379"),packages/db/src/redis.ts (1)
9-10: Consider configuring retry options more specifically.The current configuration sets
maxRetriesPerRequesttonull, which means infinite retries. While this prevents operations from failing due to transient Redis issues, it could potentially cause unexpected behavior if Redis is unreachable for an extended period.Consider setting a reasonable finite retry limit and adding retry delay configuration:
-const config = { maxRetriesPerRequest: null }; +const config = { + maxRetriesPerRequest: 10, // Reasonable upper limit + retryStrategy: (times: number) => { + const delay = Math.min(times * 50, 2000); // Exponential backoff with 2s max + return delay; + } +};packages/db/src/selectors/compute/mutex.ts (1)
32-41: Consider documenting timeout implications.The mutex configuration uses a 30-second timeout for both lock timeout and acquire timeout. While this is reasonable, it would be helpful to document the implications of these timeout values.
Add a comment explaining the timeout choices and their implications:
this.mutex = new RedisMutex(redis, this.key, { + // Lock timeout: Maximum time a lock can be held before automatically releasing + // Acquire timeout: Maximum time to wait when trying to acquire a lock + // Both set to 30s to balance between preventing deadlocks and allowing sufficient + // time for compute operations to complete lockTimeout: ms("30s"), acquireTimeout: ms("30s"), onLockLost: (e) => log.warning("Lock lost for selector compute", { error: e, type, workspaceId, }), });packages/db/src/selectors/compute/resource-builder.ts (2)
7-7: Consider importing each entity separately.For better maintainability and clarity, consider importing each entity separately rather than importing from a namespace.
-import { createAndAcquireMutex, SelectorComputeType } from "./mutex.js"; +import { createAndAcquireMutex } from "./mutex.js"; +import { SelectorComputeType } from "./mutex.js";
126-140: Consider extracting transaction logic to a separate method.Since the transaction logic is duplicated between both classes, consider extracting it to a reusable private method to improve maintainability.
Extract the transaction logic to a separate method to reduce duplication:
private async executeReleaseTargetsTransaction(tx: Tx, resourceIds: string[]) { await this.deleteExistingReleaseTargets(tx, resourceIds); const vals = await this.findMatchingEnvironmentDeploymentPairs(tx, resourceIds); if (vals.length === 0) return []; const results = await tx .insert(SCHEMA.releaseTarget) .values(vals) .onConflictDoNothing() .returning(); await this.recomputePolicyReleaseTargets(tx); return results; }Then modify both methods to use this extracted method with appropriate parameters.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/event-worker/package.json(1 hunks)packages/db/package.json(1 hunks)packages/db/src/config.ts(1 hunks)packages/db/src/redis.ts(1 hunks)packages/db/src/selectors/compute/mutex.ts(1 hunks)packages/db/src/selectors/compute/resource-builder.ts(3 hunks)packages/events/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/redis.tspackages/db/src/config.tspackages/db/src/selectors/compute/mutex.tspackages/db/src/selectors/compute/resource-builder.ts
🧬 Code Graph Analysis (3)
packages/db/src/redis.ts (2)
packages/logger/src/index.ts (1)
logger(48-48)packages/db/src/config.ts (1)
env(5-20)
packages/db/src/selectors/compute/mutex.ts (2)
packages/logger/src/index.ts (1)
logger(48-48)packages/db/src/redis.ts (1)
redis(10-10)
packages/db/src/selectors/compute/resource-builder.ts (3)
packages/logger/src/index.ts (1)
logger(48-48)packages/db/src/selectors/compute/mutex.ts (1)
createAndAcquireMutex(17-24)packages/db/src/selectors/compute/compute.ts (1)
resources(51-53)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (12)
pnpm-workspace.yaml (1)
35-36: Appropriate Redis dependencies added to catalogThe addition of
redis-semaphoreandioredisto the package catalog supports the implementation of distributed mutex locking for compute operations. The versions specified follow the caret versioning pattern used by other dependencies in the catalog.packages/db/package.json (1)
49-49: Redis dependencies properly added to packageRedis dependencies are correctly added to support the implementation of mutex functionality, and they appropriately reference the versions defined in the workspace catalog.
Also applies to: 52-52
packages/events/package.json (1)
29-29: Good dependency management practiceChanging the
ioredisdependency to use the catalog reference improves consistency across packages and centralizes version management, which is a good practice for dependency management.packages/db/src/redis.ts (2)
12-13: LGTM! Good error handling with appropriate logging.The event listeners for connection and error events are well implemented with proper logging.
10-10:❓ Verification inconclusive
Ensure Redis URL is validated.
The code uses
env.REDIS_URLwithout any fallback mechanism. If the environment variable is not set correctly, this will cause runtime errors when the application starts.Based on the context from
packages/db/src/config.ts, it appears thatREDIS_URLis required in the environment configuration. Make sure this is documented in deployment instructions and verify that the application fails gracefully if the Redis URL is missing.
🏁 Script executed:
#!/bin/bash # Check if REDIS_URL is properly documented in README or environment setup files fd -t f "README.md|.env.example" -x grep -l "REDIS_URL"Length of output: 55
I've added a script to:
- Find and display
packages/db/src/config.tsto see how env vars are validated.- Search the entire repo for any mentions of
REDIS_URL.This will confirm if
REDIS_URLis already required/checked in code or documented in examples. Once we have the results, we can finalize the review comment.apps/event-worker/package.json (1)
46-46: Consistent dependency versioning.Updating these dependencies to use
"catalog:"aligns with the project's dependency management approach across packages. This change helps maintain consistency across the monorepo and simplifies version management.Also applies to: 50-51
packages/db/src/selectors/compute/mutex.ts (4)
10-15: LGTM! Well-structured enum for compute types.Clear enumeration of different selector compute types that provides type safety and readability.
17-24: LGTM! Helpful utility function for mutex acquisition.The
createAndAcquireMutexfunction provides a convenient way to create and immediately acquire a mutex in a single step.
44-47: LGTM! Good validation before lock acquisition.The code correctly checks if the mutex is already locked before attempting to acquire it, which prevents logical errors.
49-52: LGTM! Good validation before lock release.The code correctly checks if the mutex is actually locked before attempting to release it, which prevents logical errors.
packages/db/src/selectors/compute/resource-builder.ts (2)
119-149: LGTM! Properly implemented mutex pattern with error handling.The code correctly acquires a mutex before performing database operations and ensures it's released in a
finallyblock. The error handling is comprehensive with proper logging of the error context.
249-284: LGTM! Consistent implementation of mutex pattern in both classes.The same mutex pattern is consistently applied in the
WorkspaceResourceBuilderclass, maintaining the same error handling approach and ensuring the mutex is properly released.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/db/src/selectors/compute/environment-builder.ts (2)
49-51: Avoid executing the same query twice
getEnvironments()is called once to buildworkspaceIds(lines 72‑78) and again inside the transaction throughfindMatchingResourcesForEnvironments()(lines 49‑51).
You already have the first result set; passing it down eliminates an extra round‑trip and simplifies testing.Also applies to: 84-99
100-107: Protect againstmutex.unlock()throwing
redis‑semaphorecan propagate network errors onunlock(). An exception here would mask the original failure and leave the lock dangling.
Wrap it in its own try/catch or usefinally { void mutex.unlock().catch(err => log.warn(...)) }.- } finally { - await mutex.unlock(); - } + } finally { + try { + await mutex.unlock(); + } catch (unlockErr) { + log.warn("Failed to release mutex", { unlockErr, workspaceId }); + } + }packages/db/src/selectors/compute/deployment-builder.ts (1)
99-101: Guardunlock()against Redis/network errorsSame rationale as in
EnvironmentBuilder.- } finally { - await mutex.unlock(); - } + } finally { + try { + await mutex.unlock(); + } catch (unlockErr) { + log.warn("Failed to release mutex", { unlockErr, workspaceId }); + } + }packages/db/src/selectors/compute/policy-builder.ts (1)
118-119: Wrapmutex.unlock()in try/catchSame concern as the other builders – an unlock failure would overshadow the transaction result and potentially leave a stuck lock.
Also applies to: 221-222
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/db/src/selectors/compute/deployment-builder.ts(4 hunks)packages/db/src/selectors/compute/environment-builder.ts(4 hunks)packages/db/src/selectors/compute/policy-builder.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/selectors/compute/environment-builder.tspackages/db/src/selectors/compute/deployment-builder.tspackages/db/src/selectors/compute/policy-builder.ts
🧬 Code Graph Analysis (1)
packages/db/src/selectors/compute/environment-builder.ts (4)
packages/logger/src/index.ts (1)
logger(48-48)packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/compute/compute.ts (1)
environments(31-33)packages/db/src/selectors/query/builder.ts (1)
environments(38-43)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (1)
packages/db/src/selectors/compute/policy-builder.ts (1)
153-190: Null‑selector handling removed – may generate invalid SQL
WorkspacePolicyBuilder.findMatchingReleaseTargetsForTargetsno longer checks
resourceSelector/deploymentSelector/environmentSelectorfornullbefore passing them toQueryBuilder.where().
IfQueryBuilderdoes not internally coercenullinto a tautology, the generated SQL becomes invalid and the whole compute fails.Please either reinstate the early‑return guard used in
PolicyBuilderor confirm thatQueryBuilder.where(null)is a no‑op.
Summary by CodeRabbit
New Features
Bug Fixes
Chores