Prevent SQL runner lock refreshes from hanging#6516
Open
tim-smart wants to merge 10 commits into
Open
Conversation
- Simulate a partitioned SQL connection and verify refresh completes before the shard lock expires
- Detach refresh work and bound fiber joins by the shard lock refresh interval
🦋 Changeset detectedLatest commit: f542e4c The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughShard locking now uses lease-safe deadlines and explicit health tracking. Unresponsive SQL connections trigger bounded rebuild handling; unhealthy storage interrupts affected entities, pauses acquisition, releases uncertain ownership, and probes for recovery before normal reacquisition. Tests cover SQL blackholes, failover, recovery, and graceful reassignment. ChangesShard lock failover
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Sharding
participant RunnerStorage
participant SqlConnection
participant EntityManager
Sharding->>RunnerStorage: refresh shard locks
RunnerStorage->>SqlConnection: execute bounded lock SQL
SqlConnection-->>RunnerStorage: timeout or failure
RunnerStorage-->>Sharding: lock storage unhealthy
Sharding->>EntityManager: force interrupt affected shards
Sharding->>RunnerStorage: release uncertain shards
Sharding->>RunnerStorage: probe and reacquire after recovery
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
Bundle Size Analysis
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/platform-node/test/cluster/SqlRunnerStorage.test.ts`:
- Line 49: Update the TestClock.adjust call in the relevant SqlRunnerStorage
test to advance only just beyond the 100ms refresh interval, such as 101ms,
while remaining below the 1000ms lock expiration boundary.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 89399661-80d2-4c72-b172-a5982e6075a7
📒 Files selected for processing (3)
.changeset/slow-spiders-refresh.mdpackages/effect/src/unstable/cluster/SqlRunnerStorage.tspackages/platform-node/test/cluster/SqlRunnerStorage.test.ts
github-project-automation
Bot
moved this from Discussion Ongoing
to Waiting on Author
in PR Backlog
- Bound SQL shard-lock operations and rebuild stalled connections - Interrupt entities when lock ownership is uncertain - Reacquire assigned shards with fresh entities after storage recovers
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/effect/src/unstable/cluster/SqlRunnerStorage.ts (1)
590-608: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
Effect.fnUntracedfor this Effect-returning helper.
withLockOperationDeadlineis a function whose body is only anEffect.gen. Per the repo's Effect conventions this should be defined withEffect.fnUntraced.Based on coding guidelines: "prefer `Effect.fnUntraced` over functions that only return `Effect.gen`".♻️ Suggested conversion
- const withLockOperationDeadline = <A, E, R>(operation: Effect.Effect<A, E, R>) => - Effect.gen(function*() { - const fiber = yield* Effect.forkIn(operation, layerScope, { startImmediately: true }) - return yield* Fiber.join(fiber).pipe( + const withLockOperationDeadline = Effect.fnUntraced(function*<A, E, R>(operation: Effect.Effect<A, E, R>) { + const fiber = yield* Effect.forkIn(operation, layerScope, { startImmediately: true }) + return yield* Fiber.join(fiber).pipe( Effect.timeout(lockOperationInterval), Effect.ensuring( Fiber.interrupt(fiber).pipe( Effect.forkIn(layerScope, { startImmediately: true }), Effect.asVoid ) ), Effect.tap(() => Effect.sync(() => { lockConnRebuildNeeded = true }) ), Effect.onError(rebuildLockConn) ) - }) + })🤖 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/effect/src/unstable/cluster/SqlRunnerStorage.ts` around lines 590 - 608, Convert the withLockOperationDeadline helper to use Effect.fnUntraced, wrapping its existing Effect.gen implementation without changing the operation timeout, interruption, rebuild, or error-handling behavior.Source: Coding guidelines
packages/effect/test/cluster/Sharding.test.ts (1)
680-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
assertfrom@effect/vitestinstead of Vitest'sexpect.Every other assertion in these tests uses
assert.*; this line is the loneexpect(...).Based on coding guidelines: "do not use `Effect.runSync` or Vitest's `expect`; use `assert` from `@effect/vitest`".♻️ Suggested change
- expect(yield* client.GetUserVolatile({ id: 2 })).toEqual(new User({ id: 2, name: "User 2" })) + assert.deepStrictEqual(yield* client.GetUserVolatile({ id: 2 }), new User({ id: 2, name: "User 2" }))🤖 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/effect/test/cluster/Sharding.test.ts` at line 680, Replace the lone Vitest expect assertion in the GetUserVolatile test with the corresponding assert API from `@effect/vitest`, preserving the existing expected User value and assertion behavior. Ensure this test continues to follow the surrounding assert.* convention and does not use Vitest’s expect.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/effect/src/unstable/cluster/SqlRunnerStorage.ts`:
- Around line 590-608: Convert the withLockOperationDeadline helper to use
Effect.fnUntraced, wrapping its existing Effect.gen implementation without
changing the operation timeout, interruption, rebuild, or error-handling
behavior.
In `@packages/effect/test/cluster/Sharding.test.ts`:
- Line 680: Replace the lone Vitest expect assertion in the GetUserVolatile test
with the corresponding assert API from `@effect/vitest`, preserving the existing
expected User value and assertion behavior. Ensure this test continues to follow
the surrounding assert.* convention and does not use Vitest’s expect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8c3d7150-df19-4cdd-a864-cbe604709739
📒 Files selected for processing (8)
.changeset/slow-spiders-refresh.mdpackages/effect/src/unstable/cluster/Sharding.tspackages/effect/src/unstable/cluster/ShardingConfig.tspackages/effect/src/unstable/cluster/SqlRunnerStorage.tspackages/effect/src/unstable/cluster/internal/entityManager.tspackages/effect/src/unstable/cluster/internal/shardLock.tspackages/effect/test/cluster/Sharding.test.tspackages/platform-node/test/cluster/SqlRunnerStorage.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/slow-spiders-refresh.md
- packages/platform-node/test/cluster/SqlRunnerStorage.test.ts
- Define the lock deadline helper with Effect.fnUntraced - Replace the remaining Vitest expectation with an Effect assertion
- Clarify that the deadline applies while joining cleanup fibers
- Reuse latch state and retry logic across shard release paths - Avoid interrupting completed SQL lock operations
5 tasks
- Skip shard acquisition while a forced releaseAll is pending - Cover reacquisition during a pending forced release Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebuilds were only re-armed by a successful lock operation, so once a rebuild succeeded the runner could not rebuild again while every operation kept failing - exactly the unresponsive-connection case. Track a connection generation instead: a failing operation schedules a rebuild unless the connection it ran on has already been replaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebuilding the reserved lock connection wrapped `rebuildUnsafe` in a plain `Effect.timeout`, which waits for the timed-out effect to finish interrupting. The rebuild starts by closing the previous scope, so a driver that stalls releasing the unresponsive connection kept the rebuild fiber alive past its deadline, left `lockConnRebuilding` set forever, and disabled every later rebuild. Reuse the fork-and-timeout-the-join pattern already used for lock operations, so the stalled release finishes detached and the flag is always reset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Testing
SqlRunnerStoragetest usingTestClockto verify refresh completes before the shard lock expires.Summary by CodeRabbit