feat: implement MSSQL cancelQuery and applyQueryTimeout#273
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds MSSQL query control: support for cancelling running queries via Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MSSQLPluginDriver
participant FreeTDSConnection
participant Database
rect rgba(100,150,200,0.5)
Note over Client,MSSQLPluginDriver: Timeout setup
Client->>MSSQLPluginDriver: applyQueryTimeout(seconds)
MSSQLPluginDriver->>Database: Execute "SET LOCK_TIMEOUT ..."
Database-->>MSSQLPluginDriver: OK
MSSQLPluginDriver-->>Client: Return
end
rect rgba(200,150,100,0.5)
Note over Client,FreeTDSConnection: Cancellation flow during query
Client->>MSSQLPluginDriver: Execute query
MSSQLPluginDriver->>FreeTDSConnection: Start query (reset _isCancelled)
FreeTDSConnection->>Database: Execute query
Database-->>FreeTDSConnection: Stream rows
par Concurrent
FreeTDSConnection->>FreeTDSConnection: Iterate rows, poll _isCancelled
Client->>MSSQLPluginDriver: cancelQuery()
MSSQLPluginDriver->>FreeTDSConnection: cancelCurrentQuery() (set _isCancelled, call dbcancel)
FreeTDSConnection->>Database: dbcancel
end
FreeTDSConnection->>FreeTDSConnection: Detect _isCancelled, throw "Query cancelled"
FreeTDSConnection-->>MSSQLPluginDriver: Propagate error
MSSQLPluginDriver-->>Client: Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift`:
- Around line 416-420: The current applyQueryTimeout(_:) uses "SET LOCK_TIMEOUT"
which only affects lock acquisition, not overall query execution; update the
implementation to either (A) document this limitation in applyQueryTimeout(_:)'s
method comment and in CHANGELOG so callers know it only controls lock waits, or
(B) implement application-level enforcement: store the timeout value and run
queries through a wrapper (e.g., executeWithTimeout or similar) that launches
the query in an async Task, uses Task.sleep for the timeout duration, and calls
cancelQuery() if the task hasn't completed—ensure you reference
applyQueryTimeout(_:), execute(query:), and cancelQuery() when adding the
wrapper and wire existing callers to use it.
- Around line 176-184: cancelCurrentQuery() currently captures dbproc under lock
but calls dbcancel() directly, racing with disconnect() which clears dbproc and
enqueues dbclose() asynchronously; instead, dispatch the dbcancel() call onto
the same serial queue used for dbclose() so dbcancel() and dbclose() are
serialized, and modify disconnect() to read/clear dbproc under the same lock (or
while on the serial queue) to avoid the data race on dbproc; reference the
cancelCurrentQuery(), disconnect(), dbcancel(), dbclose(), dbproc and lock
symbols when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 753ee17e-67a2-4cf7-a822-5e847a0891aa
📒 Files selected for processing (2)
CHANGELOG.mdPlugins/MSSQLDriverPlugin/MSSQLPlugin.swift
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift (1)
220-226: Consider extracting duplicated cancellation check logic.The cancellation check pattern (lock, read flag, clear if set, unlock, throw) appears twice: lines 220-226 and lines 258-264. Extracting this to a helper method would reduce duplication and make the cancellation behavior consistent.
♻️ Suggested helper extraction
private func checkCancellationSync() throws { lock.lock() let cancelled = _isCancelled if cancelled { _isCancelled = false } lock.unlock() if cancelled { throw MSSQLPluginError.queryFailed("Query cancelled") } }Then replace both occurrences with:
try checkCancellationSync()Also applies to: 258-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift` around lines 220 - 226, Extract the duplicated cancellation check into a private helper (e.g., checkCancellationSync) that acquires lock, reads _isCancelled, clears it if set, releases the lock, and throws MSSQLPluginError.queryFailed("Query cancelled") when cancelled; then replace both in-place blocks around lock/_isCancelled with a single call (try checkCancellationSync()) so the cancellation behavior is centralized and consistent.TableProTests/Core/Database/MSSQLDriverTests.swift (1)
40-43: MockapplyQueryTimeoutrecords both value and synthetic SQL string.The mock records the timeout value in
applyQueryTimeoutValuesand also appends a synthetic SQL string toexecutedQueries. SinceapplyQueryTimeoutin the mock doesn't callexecute(), this synthetic entry may mislead tests expecting to verify actual SQL execution. Consider either:
- Only recording the timeout value (simpler, clearer mock)
- Or documenting this behavior in a comment
♻️ Simplify mock to only record timeout values
func applyQueryTimeout(_ seconds: Int) async throws { applyQueryTimeoutValues.append(seconds) - executedQueries.append("SET LOCK_TIMEOUT \(seconds * 1_000)") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TableProTests/Core/Database/MSSQLDriverTests.swift` around lines 40 - 43, The mock applyQueryTimeout currently appends both the numeric timeout to applyQueryTimeoutValues and a synthetic SQL string to executedQueries; remove the synthetic SQL append so applyQueryTimeout only records the timeout value (leave applyQueryTimeoutValues.append(seconds) intact) to avoid misleading tests that inspect executedQueries; update the applyQueryTimeout implementation (and optionally add a brief comment in the mock near applyQueryTimeout) to reflect that it only records the timeout and does not execute SQL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Plugins/MSSQLDriverPlugin/MSSQLPlugin.swift`:
- Around line 220-226: Extract the duplicated cancellation check into a private
helper (e.g., checkCancellationSync) that acquires lock, reads _isCancelled,
clears it if set, releases the lock, and throws
MSSQLPluginError.queryFailed("Query cancelled") when cancelled; then replace
both in-place blocks around lock/_isCancelled with a single call (try
checkCancellationSync()) so the cancellation behavior is centralized and
consistent.
In `@TableProTests/Core/Database/MSSQLDriverTests.swift`:
- Around line 40-43: The mock applyQueryTimeout currently appends both the
numeric timeout to applyQueryTimeoutValues and a synthetic SQL string to
executedQueries; remove the synthetic SQL append so applyQueryTimeout only
records the timeout value (leave applyQueryTimeoutValues.append(seconds) intact)
to avoid misleading tests that inspect executedQueries; update the
applyQueryTimeout implementation (and optionally add a brief comment in the mock
near applyQueryTimeout) to reflect that it only records the timeout and does not
execute SQL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19a0c549-5382-4bed-886b-f3024c8a5553
📒 Files selected for processing (2)
Plugins/MSSQLDriverPlugin/MSSQLPlugin.swiftTableProTests/Core/Database/MSSQLDriverTests.swift
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
1 participant
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
cancelQuery()for MSSQL using FreeTDSdbcancel()— enables stopping long-running queries from another threadapplyQueryTimeout()usingSET LOCK_TIMEOUT— the closest SQL Server equivalent to session-level query timeout_isCancelledflag toFreeTDSConnectionwith proper lock synchronization and reset logic to avoid stale cancellationsTest plan
SET LOCK_TIMEOUTis applied when query timeout is configured in settingsSummary by CodeRabbit