Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChanges reduce time boundary constants from 120 to 60 seconds and add lock extension functionality to the handler pool. When a WAIT_TIMEOUT error occurs during transaction submission, the system now extends the pool lock instead of releasing it, with corresponding test coverage for this behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Handler as Handler
participant Pool as ChannelPool
participant KV as Storage (KV)
Client->>Handler: Submit transaction
Handler->>Pool: acquire(lock)
Pool->>KV: Check and lock channel
KV-->>Pool: Lock acquired
Pool-->>Handler: poolLock returned
Handler->>Handler: Attempt submit
Note over Handler: Submit fails with WAIT_TIMEOUT
Handler->>Pool: extendLock(poolLock)
Pool->>KV: Read lock entry
KV-->>Pool: Lock entry (verify token)
alt Token matches (ownership confirmed)
Pool->>KV: Update lock with new timestamp & TTL
KV-->>Pool: Lock updated
else Token mismatch
Note over Pool: No-op, swallow error
end
Pool-->>Handler: extendLock complete
Note over Handler: Skip release, keep lock extended
Handler-->>Client: Return with extended lock
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements lock extension on transaction timeout to prevent race conditions where another request could acquire a channel while a previous transaction is still unresolved. The changes reduce transaction time bounds from 120s to 60s to better align with the timeout (25s) + lock extension (30s) pattern, totaling 55s which fits within the new 60s time bound.
Changes:
- Reduced time bound constants from 120s to 60s to match timeout and lock extension timing
- Added
extendLockmethod toChannelPoolthat refreshes lock TTL with token-based ownership verification - Modified error handling in
handler.tsto extend locks onWAIT_TIMEOUTinstead of releasing them
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugin/constants.ts | Reduced MAX_TIME_BOUND_OFFSET_SECONDS from 120s to 60s in TIME and SIMULATION constants |
| src/plugin/pool.ts | Added extendLock method with token-based ownership verification and silent error handling |
| src/plugin/handler.ts | Modified WAIT_TIMEOUT error handling to extend lock and skip release in finally block |
| test/pool.test.ts | Added comprehensive tests for extendLock functionality including token validation and error handling |
| test/handler.sequence-cache.test.ts | Added integration tests verifying lock extension behavior on WAIT_TIMEOUT vs. normal release on other errors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/pool.test.ts (1)
62-77: Consider resetting the spy beforeextendLockfor clarity.The test logic is correct—the spy is created after
acquire(), so filtering by the key works. However, callingsetSpy.mockClear()beforeextendLockwould make the assertion simpler and the intent clearer.♻️ Optional improvement
// Spy on kv.set to verify it's not called for the extend const setSpy = vi.spyOn(kv, 'set'); + setSpy.mockClear(); await pool.extendLock(wrongLock); - // set should not have been called (token mismatch) - const extendCalls = setSpy.mock.calls.filter((c) => c[0] === `testnet:channel:in-use:${lock.relayerId}`); - expect(extendCalls).toHaveLength(0); + // set should not have been called at all (token mismatch) + expect(setSpy).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/pool.test.ts` around lines 62 - 77, Reset the kv.set spy before calling pool.extendLock to ensure only calls caused by extendLock are observed: after creating the spy (vi.spyOn(kv, 'set')) call setSpy.mockClear() so prior set calls from pool.acquire() are cleared, then call pool.extendLock(wrongLock) and assert that no sets were made for the key `testnet:channel:in-use:${lock.relayerId}`; this involves the FakeKV instance, ChannelPool.acquire, ChannelPool.extendLock, and the setSpy mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/pool.test.ts`:
- Around line 62-77: Reset the kv.set spy before calling pool.extendLock to
ensure only calls caused by extendLock are observed: after creating the spy
(vi.spyOn(kv, 'set')) call setSpy.mockClear() so prior set calls from
pool.acquire() are cleared, then call pool.extendLock(wrongLock) and assert that
no sets were made for the key `testnet:channel:in-use:${lock.relayerId}`; this
involves the FakeKV instance, ChannelPool.acquire, ChannelPool.extendLock, and
the setSpy mock.
This PR will:
Dropping lock on timeout means that another request could pick it up while previous tx is still unresolved.
Summary by CodeRabbit
Bug Fixes
Configuration Updates
Tests