Skip to content

Commit c3b9ec8

Browse files
datlechindatlechin
committed
fix: address CodeRabbit review — hop relay safety, stale index, delete cleanup, duplicates
1 parent 1a2c348 commit c3b9ec8

7 files changed

Lines changed: 61 additions & 57 deletions

‎TablePro/Core/SSH/LibSSH2TunnelFactory.swift‎

Lines changed: 43 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -202,9 +202,13 @@ internal enum LibSSH2TunnelFactory {
202202
}
203203
}
204204

205-
// Clean up any jump hops that were created (reverse order)
205+
// Clean up any jump hops that were created (reverse order).
206+
// Shutdown sockets first to break relay loops, then free resources.
206207
for hop in jumpHops.reversed() {
207208
hop.relayTask?.cancel()
209+
shutdown(hop.socket, SHUT_RDWR)
210+
}
211+
for hop in jumpHops.reversed() {
208212
libssh2_channel_free(hop.channel)
209213
tablepro_libssh2_session_disconnect(hop.session, "Error")
210214
libssh2_session_free(hop.session)
@@ -452,17 +456,21 @@ internal enum LibSSH2TunnelFactory {
452456
}
453457

454458
/// Start a relay task that copies data between a channel and a socketpair fd.
455-
/// All libssh2 calls are dispatched onto the provided `sessionQueue` to serialize access.
459+
/// libssh2 calls use `sessionQueue.sync` for thread safety; I/O loop runs on a concurrent queue.
456460
private static func startChannelRelay(
457461
channel: OpaquePointer,
458462
socketFD: Int32,
459463
sshSocketFD: Int32,
460464
session: OpaquePointer,
461465
sessionQueue: DispatchQueue
462466
) -> Task<Void, Never> {
463-
Task.detached {
467+
let relayQueue = DispatchQueue(
468+
label: "com.TablePro.ssh.hop-relay",
469+
qos: .utility
470+
)
471+
return Task.detached {
464472
await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in
465-
sessionQueue.async {
473+
relayQueue.async {
466474
let bufferSize = 32_768
467475
let buffer = UnsafeMutablePointer<CChar>.allocate(capacity: bufferSize)
468476
defer {
@@ -471,7 +479,7 @@ internal enum LibSSH2TunnelFactory {
471479
continuation.resume()
472480
}
473481

474-
while true {
482+
while !Task.isCancelled {
475483
var pollFDs = [
476484
pollfd(fd: socketFD, events: Int16(POLLIN), revents: 0),
477485
pollfd(fd: sshSocketFD, events: Int16(POLLIN), revents: 0),
@@ -480,24 +488,29 @@ internal enum LibSSH2TunnelFactory {
480488
let pollResult = poll(&pollFDs, 2, 100)
481489
if pollResult < 0 { break }
482490

483-
// Channel -> socketpair
484-
let channelRead = tablepro_libssh2_channel_read(channel, buffer, bufferSize)
485-
if channelRead > 0 {
486-
var totalSent = 0
487-
while totalSent < Int(channelRead) {
488-
let sent = send(
489-
socketFD,
490-
buffer.advanced(by: totalSent),
491-
Int(channelRead) - totalSent,
492-
0
493-
)
494-
if sent <= 0 { return }
495-
totalSent += sent
491+
// Channel -> socketpair (serialized libssh2 call)
492+
if pollFDs[1].revents & Int16(POLLIN) != 0 || pollResult == 0 {
493+
let channelRead: Int = sessionQueue.sync {
494+
Int(tablepro_libssh2_channel_read(channel, buffer, bufferSize))
495+
}
496+
if channelRead > 0 {
497+
var totalSent = 0
498+
while totalSent < channelRead {
499+
let sent = send(
500+
socketFD,
501+
buffer.advanced(by: totalSent),
502+
channelRead - totalSent,
503+
0
504+
)
505+
if sent <= 0 { return }
506+
totalSent += sent
507+
}
508+
} else if channelRead == 0
509+
|| sessionQueue.sync({ libssh2_channel_eof(channel) }) != 0 {
510+
return
511+
} else if channelRead != Int(LIBSSH2_ERROR_EAGAIN) {
512+
return
496513
}
497-
} else if channelRead == 0 || libssh2_channel_eof(channel) != 0 {
498-
return
499-
} else if channelRead != Int(LIBSSH2_ERROR_EAGAIN) {
500-
return
501514
}
502515

503516
// Socketpair -> channel
@@ -507,15 +520,16 @@ internal enum LibSSH2TunnelFactory {
507520

508521
var totalWritten = 0
509522
while totalWritten < Int(socketRead) {
510-
let written = tablepro_libssh2_channel_write(
511-
channel,
512-
buffer.advanced(by: totalWritten),
513-
Int(socketRead) - totalWritten
514-
)
523+
let written: Int = sessionQueue.sync {
524+
Int(tablepro_libssh2_channel_write(
525+
channel,
526+
buffer.advanced(by: totalWritten),
527+
Int(socketRead) - totalWritten
528+
))
529+
}
515530
if written > 0 {
516-
totalWritten += Int(written)
531+
totalWritten += written
517532
} else if written == Int(LIBSSH2_ERROR_EAGAIN) {
518-
// Wait for socket readiness
519533
var writePollFD = pollfd(
520534
fd: sshSocketFD, events: Int16(POLLOUT), revents: 0
521535
)

‎TablePro/Views/Connection/ConnectionFormView.swift‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -837,6 +837,8 @@ struct ConnectionFormView: View { // swiftlint:disable:this type_body_length
837837
.onChange(of: sshEnabled) { _, _ in testSucceeded = false }
838838
.onChange(of: sshHost) { _, _ in testSucceeded = false }
839839
.onChange(of: sshPort) { _, _ in testSucceeded = false }
840+
.onChange(of: sshUsername) { _, _ in testSucceeded = false }
841+
.onChange(of: sshAuthMethod) { _, _ in testSucceeded = false }
840842
.onChange(of: sslMode) { _, _ in testSucceeded = false }
841843
}
842844

@@ -1061,14 +1063,9 @@ struct ConnectionFormView: View { // swiftlint:disable:this type_body_length
10611063
}
10621064

10631065
private func deleteConnection() {
1064-
guard let id = connectionId else { return }
1065-
var savedConnections = storage.loadConnections()
1066-
let hadConnection = savedConnections.contains { $0.id == id }
1067-
savedConnections.removeAll { $0.id == id }
1068-
storage.saveConnections(savedConnections)
1069-
if hadConnection {
1070-
SyncChangeTracker.shared.markDeleted(.connection, id: id.uuidString)
1071-
}
1066+
guard let id = connectionId,
1067+
let connection = storage.loadConnections().first(where: { $0.id == id }) else { return }
1068+
storage.deleteConnection(connection)
10721069
NSApplication.shared.closeWindows(withId: "connection-form")
10731070
NotificationCenter.default.post(name: .connectionUpdated, object: nil)
10741071
}

‎TablePro/Views/Main/Extensions/MainContentCoordinator+FKNavigation.swift‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,6 @@ extension MainContentCoordinator {
8181
AppState.shared.isCurrentTabEditable = tab.isEditable && !tab.isView && tab.tableName != nil
8282
toolbarState.isTableTab = tab.tabType == .table
8383
AppState.shared.isTableTab = tab.tabType == .table
84-
AppState.shared.isTableTab = tab.tabType == .table
8584
}
8685

8786
if needsQuery {

‎TablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swift‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,6 @@ extension MainContentCoordinator {
9494
AppState.shared.isCurrentTabEditable = !isView && tableName.isEmpty == false
9595
toolbarState.isTableTab = true
9696
AppState.shared.isTableTab = true
97-
AppState.shared.isTableTab = true
9897
}
9998
// In-place navigation needs selectRedisDatabaseAndQuery to ensure the correct
10099
// database is SELECTed and session state is updated before querying.
@@ -118,7 +117,6 @@ extension MainContentCoordinator {
118117
tabManager.tabs[tabIndex].pagination.reset()
119118
toolbarState.isTableTab = true
120119
AppState.shared.isTableTab = true
121-
AppState.shared.isTableTab = true
122120
}
123121
if let dbIndex = Int(currentDatabase) {
124122
selectRedisDatabaseAndQuery(dbIndex)
@@ -188,8 +186,6 @@ extension MainContentCoordinator {
188186
AppState.shared.isCurrentTabEditable = !isView && !tableName.isEmpty
189187
previewCoordinator.toolbarState.isTableTab = true
190188
AppState.shared.isTableTab = true
191-
AppState.shared.isTableTab = true
192-
AppState.shared.isTableTab = true
193189
}
194190
preview.window.makeKeyAndOrderFront(nil)
195191
previewCoordinator.runQuery()
@@ -216,7 +212,6 @@ extension MainContentCoordinator {
216212
AppState.shared.isCurrentTabEditable = !isView && !tableName.isEmpty
217213
toolbarState.isTableTab = true
218214
AppState.shared.isTableTab = true
219-
AppState.shared.isTableTab = true
220215
}
221216
runQuery()
222217
return

‎TablePro/Views/Main/Extensions/MainContentCoordinator+Pagination.swift‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,12 @@ extension MainContentCoordinator {
9090
tabIndex: Int,
9191
mutate: @escaping (inout PaginationState) -> Void
9292
) {
93+
let tabId = tabManager.tabs[tabIndex].id
9394
confirmDiscardChangesIfNeeded(action: .pagination) { [weak self] confirmed in
9495
guard let self, confirmed else { return }
95-
guard tabIndex < self.tabManager.tabs.count else { return }
96+
guard let idx = self.tabManager.tabs.firstIndex(where: { $0.id == tabId }) else { return }
9697

97-
mutate(&self.tabManager.tabs[tabIndex].pagination)
98+
mutate(&self.tabManager.tabs[idx].pagination)
9899
self.reloadCurrentPage()
99100
}
100101
}

‎TablePro/Views/Main/MainContentCommandActions.swift‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,6 @@ final class MainContentCommandActions {
338338
AppState.shared.isCurrentTabEditable = false
339339
coordinator?.toolbarState.isTableTab = false
340340
AppState.shared.isTableTab = false
341-
AppState.shared.isTableTab = false
342341
}
343342
}
344343

‎TablePro/Views/Main/MainContentCoordinator.swift‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1154,22 +1154,22 @@ final class MainContentCoordinator {
11541154
return
11551155
}
11561156

1157-
let capturedTabIndex = tabIndex
1157+
let tabId = tab.id
11581158
let capturedSort = currentSort
11591159
let capturedQuery = tab.query
11601160
let capturedColumns = tab.resultColumns
11611161
confirmDiscardChangesIfNeeded(action: .sort) { [weak self] confirmed in
1162-
guard let self, confirmed else { return }
1163-
guard capturedTabIndex < self.tabManager.tabs.count else { return }
1164-
self.tabManager.tabs[capturedTabIndex].sortState = capturedSort
1165-
self.tabManager.tabs[capturedTabIndex].hasUserInteraction = true
1166-
self.tabManager.tabs[capturedTabIndex].pagination.reset()
1162+
guard let self, confirmed,
1163+
let idx = self.tabManager.tabs.firstIndex(where: { $0.id == tabId }) else { return }
1164+
self.tabManager.tabs[idx].sortState = capturedSort
1165+
self.tabManager.tabs[idx].hasUserInteraction = true
1166+
self.tabManager.tabs[idx].pagination.reset()
11671167
let newQuery = self.queryBuilder.buildMultiSortQuery(
11681168
baseQuery: capturedQuery,
11691169
sortState: capturedSort,
11701170
columns: capturedColumns
11711171
)
1172-
self.tabManager.tabs[capturedTabIndex].query = newQuery
1172+
self.tabManager.tabs[idx].query = newQuery
11731173
self.runQuery()
11741174
}
11751175
}
@@ -1338,7 +1338,8 @@ private extension MainContentCoordinator {
13381338
tabManager.tabs[idx] = updatedTab
13391339
AppState.shared.isCurrentTabEditable = updatedTab.isEditable
13401340
&& !updatedTab.isView && updatedTab.tableName != nil
1341-
toolbarState.isTableTab = updatedTab.tabType == .table; AppState.shared.isTableTab = toolbarState.isTableTab
1341+
toolbarState.isTableTab = updatedTab.tabType == .table
1342+
AppState.shared.isTableTab = updatedTab.tabType == .table
13421343

13431344
let resolvedPK: String?
13441345
if let pk = metadata?.primaryKeyColumn {
@@ -1413,9 +1414,7 @@ private extension MainContentCoordinator {
14131414
}
14141415

14151416
// Phase 2b: Fetch enum/set values
1416-
let enumDriver = DatabaseManager.shared.driver(for: connectionId)
1417-
guard let enumDriver else { return }
1418-
1417+
guard let enumDriver = DatabaseManager.shared.driver(for: connectionId) else { return }
14191418
Task { [weak self] in
14201419
guard let self else { return }
14211420
try? await Task.sleep(nanoseconds: 200_000_000)

0 commit comments

Comments
 (0)