Fix SFTP downloading and enhance stability#9
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
📝 WalkthroughWalkthrough增加 SFTP 单包长度上限（16MB）与异常安全处理，显式处理流的 onError/onDone；本地 TCP 套接字启用 TCP_NODELAY；SSH 通道文件格式化并修复 remoteChannelId getter。 变更SFTP 健壮性与套接字配置
概览SFTP 客户端的流订阅增加错误与完成处理，数据包解析加入大小校验与异常捕获；TCP 套接字连接启用 TCP_NODELAY 选项；SSH 通道代码进行格式化并修复 remoteChannelId getter。 变更（更新）SFTP 健壮性与套接字配置
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: 6
🧹 Nitpick comments (4)
lib/src/sftp/sftp_client.dart (4)
36-49: ⚡ Quick win流订阅的错误和完成处理存在潜在竞态条件。
_done.isCompleted检查与complete/completeError调用之间不是原子操作。如果多个事件（例如 onError 和 onDone）同时触发，可能会导致竞态条件。虽然 Dart 的事件循环通常会序列化这些调用，但依赖非原子检查并不是最佳实践。建议使用 try/catch 包裹 complete 调用，捕获
StateError异常以优雅处理重复完成：♻️ 推荐的修复方案
onError: (Object e, _) { print('[SFTP] stream onError: $e'); - if (!_done.isCompleted) { + try { _done.completeError(e); + } catch (_) { + // Already completed } }, onDone: () { - if (!_done.isCompleted) { + try { _done.complete(); + } catch (_) { + // Already completed } },🤖 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 `@lib/src/sftp/sftp_client.dart` around lines 36 - 49, The stream subscription's onError/onDone handlers currently check _done.isCompleted before calling _done.complete/_done.completeError, which can race; instead, remove the non-atomic isCompleted checks and call complete/completeError inside try/catch, catching StateError to ignore duplicate completions. Update the handlers registered on _channel.stream.listen (and keep using _handleData) so onError calls _done.completeError(e) wrapped in try/catch(StateError) and onDone calls _done.complete() wrapped in try/catch(StateError), ensuring any duplicate-completion StateError is swallowed and other exceptions are rethrown or logged.
509-510: 💤 Low value未知数据包类型的日志记录存在重复。
第 509 行的无条件
printDebug调用记录相同的未知数据包事件。这会导致：
- 日志输出重复
- 代码冗余
如果未知数据包类型确实是需要无条件记录的严重问题，建议移除
printDebug调用；否则，移除♻️ 可选的重构方案
方案 1：移除条件日志（如果未知数据包是严重问题）
default: print('[SFTP] UNKNOWN packet type=$type'); - printDebug?.call('SftpClient._handlePacket: unknown packet: $type');方案 2：移除无条件日志（如果仅用于调试）
default: - print('[SFTP] UNKNOWN packet type=$type'); printDebug?.call('SftpClient._handlePacket: unknown packet: $type');🤖 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 `@lib/src/sftp/sftp_client.dart` around lines 509 - 510, In SftpClient._handlePacket remove the unconditional print('[SFTP] UNKNOWN packet type=$type') to avoid duplicate logging and rely on the existing conditional debugger callback printDebug?.call('SftpClient._handlePacket: unknown packet: $type') so unknown packet events are logged only when a debug handler is present; if you want always-on logging instead, do the opposite (remove the printDebug?.call and keep the unconditional print) but do not keep both.
474-474: ⚡ Quick win将魔法数字提取为命名常量。
硬编码的
16 * 1024 * 1024应提取为文件顶部的命名常量，以提高可读性和可维护性：♻️ 推荐的重构方案
在文件顶部添加常量定义：
const _kMaxPacketSize = 16 * 1024 * 1024; // 16MB SFTP packet size limit然后在检查中使用：
- if (length > 16 * 1024 * 1024) { + if (length > _kMaxPacketSize) {🤖 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 `@lib/src/sftp/sftp_client.dart` at line 474, Extract the hard-coded 16 * 1024 * 1024 into a named top-level constant (e.g. const _kMaxPacketSize = 16 * 1024 * 1024;) and replace the inline literal in the length check (the conditional using length > 16 * 1024 * 1024) with that constant; add a short comment like "16MB SFTP packet size limit" next to the constant for clarity and update any other occurrences in the file to use _kMaxPacketSize so the intent is explicit and maintainable.
473-478: 16MB 限制不是 SFTP 协议定义的要求，而是实现级别的安全措施。根据 SFTP 和 SSH 协议规范：
- RFC 4253 要求 SSH 实现至少支持 35,000 字节的数据包
- SFTP 规范建议至少支持 34,000 字节以允许 32KB 的读写操作
- SFTP 协议本身并未定义硬性最大数据包大小限制，由实现决定
16MB 远大于协议最小要求，作为 DoS 保护措施合理。建议：
- 将硬编码值改为命名常量，以便维护和调整
- 在代码注释中明确说明这是安全限制而非协议限制
🤖 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 `@lib/src/sftp/sftp_client.dart` around lines 473 - 478, Replace the hard-coded 16 * 1024 * 1024 magic number in _handlePackets with a clearly named constant (e.g. kMaxSftpPayloadBytes or _maxPayloadSize) declared near the class/module top so it’s easy to find and adjust; update the conditional that checks length and the diagnostic print to reference that constant; add a comment above the constant stating this is an implementation-level DoS/safety limit (not an SFTP protocol requirement) and mention RFC 4253 / SFTP minimums for context so maintainers understand why the limit exists; keep the logic that clears _buffer and returns when length exceeds the constant.
🤖 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 `@lib/src/sftp/sftp_client.dart`:
- Around line 36-49: The SFTP client file has formatting issues around the
_channel.stream.listen block (handlers _handleData, onError and onDone) and
other nearby blocks; run "dart format lib/src/sftp/sftp_client.dart" to auto-fix
formatting, then stage the formatted file (ensure changes around
_channel.stream.listen, uses of _handleData, and _done completions are
preserved) and rerun CI; no code logic changes required—only apply the
formatter.
- Around line 483-486: The catch block in _handlePackets currently prints the
error, clears _buffer, and returns, which leaves pending requests hung; instead
invoke _closeError with the caught exception to terminate the connection and
fail outstanding requests properly (replace the _buffer.clear(); return; flow
with a call to _closeError(e) or otherwise pass the error into _closeError so
the connection and pending requests are closed/failed).
- Around line 475-477: In _handlePackets, do not drop potential valid data by
calling _buffer.clear() and returning on suspicious length; instead treat this
as a fatal protocol error and call _closeError (similar to _handleData's
exception handling) with a descriptive error object/message so the connection is
terminated and callers are notified; remove the _buffer.clear() + return path
and ensure the _closeError invocation includes context (length and
_buffer.length) to aid debugging.
- Around line 460-465: The catch in _handleData currently only prints the error
which can leave _buffer/_handlePackets state inconsistent; update the catch to
log the error and then signal failure and tear down the connection: if _done
exists and is not completed, complete it with the caught error (e.g.
_done.completeError(e)) and then close/destroy the underlying connection (e.g.
_socket?.destroy() or call your connection-close helper) so callers observe the
failure and the client stops processing further data.
In `@lib/src/ssh_channel.dart`:
- Around line 556-559: The constructor for SSHChannelDataSplitter accepts
maxSize but doesn't validate it, which can cause a divide-by-zero when computing
chunk.bytes.length ~/ maxSize; update SSHChannelDataSplitter (constructor and/or
field initialization) to enforce that maxSize is a positive integer (e.g.,
assert or throw ArgumentError.value when maxSize <= 0) so any instantiation with
non-positive values fails fast and prevents runtime exceptions where
chunk.bytes.length ~/ maxSize is used.
- Around line 456-458: The getter SSHChannel.remoteChannelId is returning the
wrong field: it currently returns _controller.localId; change it to return
_controller.remoteId so the public API exposes the actual remote channel ID
(update the getter remoteChannelId to return _controller.remoteId).
---
Nitpick comments:
In `@lib/src/sftp/sftp_client.dart`:
- Around line 36-49: The stream subscription's onError/onDone handlers currently
check _done.isCompleted before calling _done.complete/_done.completeError, which
can race; instead, remove the non-atomic isCompleted checks and call
complete/completeError inside try/catch, catching StateError to ignore duplicate
completions. Update the handlers registered on _channel.stream.listen (and keep
using _handleData) so onError calls _done.completeError(e) wrapped in
try/catch(StateError) and onDone calls _done.complete() wrapped in
try/catch(StateError), ensuring any duplicate-completion StateError is swallowed
and other exceptions are rethrown or logged.
- Around line 509-510: In SftpClient._handlePacket remove the unconditional
print('[SFTP] UNKNOWN packet type=$type') to avoid duplicate logging and rely on
the existing conditional debugger callback
printDebug?.call('SftpClient._handlePacket: unknown packet: $type') so unknown
packet events are logged only when a debug handler is present; if you want
always-on logging instead, do the opposite (remove the printDebug?.call and keep
the unconditional print) but do not keep both.
- Line 474: Extract the hard-coded 16 * 1024 * 1024 into a named top-level
constant (e.g. const _kMaxPacketSize = 16 * 1024 * 1024;) and replace the inline
literal in the length check (the conditional using length > 16 * 1024 * 1024)
with that constant; add a short comment like "16MB SFTP packet size limit" next
to the constant for clarity and update any other occurrences in the file to use
_kMaxPacketSize so the intent is explicit and maintainable.
- Around line 473-478: Replace the hard-coded 16 * 1024 * 1024 magic number in
_handlePackets with a clearly named constant (e.g. kMaxSftpPayloadBytes or
_maxPayloadSize) declared near the class/module top so it’s easy to find and
adjust; update the conditional that checks length and the diagnostic print to
reference that constant; add a comment above the constant stating this is an
implementation-level DoS/safety limit (not an SFTP protocol requirement) and
mention RFC 4253 / SFTP minimums for context so maintainers understand why the
limit exists; keep the logic that clears _buffer and returns when length exceeds
the constant.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f00fc0b6-1dc9-4b4c-bd4e-f54b975e41f5
📒 Files selected for processing (3)
lib/src/sftp/sftp_client.dartlib/src/socket/ssh_socket_io.dartlib/src/ssh_channel.dart
📜 Review details
🧰 Additional context used
🪛 GitHub Actions: dart analysis / 0_check.txt
lib/src/sftp/sftp_client.dart
[error] 1-1: dart format check failed because formatting changes are required. Step: "dart format --output=none --set-exit-if-changed ." (reported: 1 file would be changed).
🔇 Additional comments (1)
lib/src/socket/ssh_socket_io.dart (1)
7-15: LGTM!Also applies to: 17-45
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/sftp/sftp_client.dart (1)
259-268:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
_closeError未完成_handshake，握手前失败会导致后续 API 卡死。
_sendRequest()会先await handshake。如果在收到版本包前进入_closeError，handshake不会完成，调用方会一直等待。💡 建议改动
void _closeError(Object error, [StackTrace? stackTrace]) { stackTrace ??= StackTrace.current; + if (!_handshake.isCompleted) { + _handshake.completeError(error, stackTrace); + } for (var waiter in _replyWaiters.values) { waiter.completeError(error, stackTrace); } _replyWaiters.clear(); _buffer.clear(); if (!_done.isCompleted) { _done.completeError(error, stackTrace); } }🤖 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 `@lib/src/sftp/sftp_client.dart` around lines 259 - 268, _closeError currently completes _replyWaiters and _done but never completes the _handshake completer, so any callers awaiting handshake in _sendRequest will hang if the connection closes before version exchange; modify _closeError to also completeError the _handshake completer (check !_handshake.isCompleted) with the same error and stackTrace, keeping the existing behavior for _replyWaiters and _done so that both handshake waiters and regular reply waiters are unblocked.
🤖 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 `@lib/src/sftp/sftp_client.dart`:
- Around line 39-56: The stream error/done handlers currently only complete
_done, leaving any in-flight request Futures in _replyWaiters hanging; update
the onError and onDone callbacks on _channel.stream.listen (the same place that
calls _handleData) to iterate and fail/completeError every outstanding entry in
_replyWaiters (with the received error for onError and a suitable error like
StateError or StreamClosed for onDone) before completing _done, and guard
against double-completion as already done for _done.
---
Outside diff comments:
In `@lib/src/sftp/sftp_client.dart`:
- Around line 259-268: _closeError currently completes _replyWaiters and _done
but never completes the _handshake completer, so any callers awaiting handshake
in _sendRequest will hang if the connection closes before version exchange;
modify _closeError to also completeError the _handshake completer (check
!_handshake.isCompleted) with the same error and stackTrace, keeping the
existing behavior for _replyWaiters and _done so that both handshake waiters and
regular reply waiters are unblocked.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca92d749-022e-4f79-b2ed-5a1c2159b7dd
📒 Files selected for processing (2)
lib/src/sftp/sftp_client.dartlib/src/ssh_channel.dart
📜 Review details
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check
🔇 Additional comments (3)
lib/src/ssh_channel.dart (2)
456-458: LGTM!Also applies to: 554-560
506-517: LGTM!lib/src/sftp/sftp_client.dart (1)
26-28: LGTM!Also applies to: 470-475, 481-499
GT-610
changed the title
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 by CodeRabbit
发布说明