<fix>[sftp]: add 60s timeout to SFTP backup storage connect syncJsonPost#4114
<fix>[sftp]: add 60s timeout to SFTP backup storage connect syncJsonPost#4114zstack-robot-2 wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 21 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
概述PR 为 SFTP 备份存储的连接请求添加了 60 秒超时，使用 变更说明SFTP 连接超时
序列图不适用于此变更。 🎯 2 (Simple) | ⏱️ ~10 分钟
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (1)
plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.java (1)
342-342: ⚡ Quick win将连接超时值提取为具名常量，避免魔法值。
这里直接写
60可读性和可维护性较弱，建议提取为类级常量（例如CONNECT_TIMEOUT_SECONDS），并使用60L与long timeout参数语义一致。♻️ 建议修改
public class SftpBackupStorage extends BackupStorageBase { private static final CLogger logger = Utils.getLogger(SftpBackupStorage.class); + private static final long CONNECT_TIMEOUT_SECONDS = 60L; ... - ConnectResponse rsp = restf.syncJsonPost(url, cmd, ConnectResponse.class, TimeUnit.SECONDS, 60); + ConnectResponse rsp = restf.syncJsonPost(url, cmd, ConnectResponse.class, TimeUnit.SECONDS, CONNECT_TIMEOUT_SECONDS);As per coding guidelines: “避免使用魔法值（Magic Value）”。
🤖 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 `@plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.java` at line 342, The restf.syncJsonPost call in SftpBackupStorage uses a magic literal 60 for the timeout; extract this into a class-level constant (e.g., CONNECT_TIMEOUT_SECONDS) and replace the literal with that constant (use 60L to match the long timeout parameter), updating the restf.syncJsonPost invocation to pass CONNECT_TIMEOUT_SECONDS so the timeout value is named and maintainable.
🤖 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
`@test/src/test/groovy/org/zstack/test/integration/storage/backup/sftp/SftpBackupStorageConnectTimeoutCase.groovy`:
- Around line 82-103: Add a negative test branch to testSftpConnectWithTimeout
that simulates a CONNECT_PATH delay exceeding the 60s timeout: use
env.simulator(SftpBackupStorageConstant.CONNECT_PATH) to introduce a long
sleep/delay and return no successful ConnectResponse, then call
addSftpBackupStorage and assert it fails within the expected timeout
(throws/returns error) and does not block further operations (for example,
immediately perform another lightweight addSftpBackupStorage or query and assert
it succeeds). Ensure assertions reference the test method name
testSftpConnectWithTimeout, the simulator hook
SftpBackupStorageConstant.CONNECT_PATH, and the
addSftpBackupStorage/BackupStorageInventory flow so the failure and non-blocking
behavior are validated.
---
Nitpick comments:
In
`@plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.java`:
- Line 342: The restf.syncJsonPost call in SftpBackupStorage uses a magic
literal 60 for the timeout; extract this into a class-level constant (e.g.,
CONNECT_TIMEOUT_SECONDS) and replace the literal with that constant (use 60L to
match the long timeout parameter), updating the restf.syncJsonPost invocation to
pass CONNECT_TIMEOUT_SECONDS so the timeout value is named and maintainable.
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: b6e3c009-3592-47e3-95af-69768cc1f3f7
📒 Files selected for processing (2)
plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.javatest/src/test/groovy/org/zstack/test/integration/storage/backup/sftp/SftpBackupStorageConnectTimeoutCase.groovy
| void testSftpConnectWithTimeout() { | ||
| env.simulator(SftpBackupStorageConstant.CONNECT_PATH) { | ||
| def rsp = new SftpBackupStorageCommands.ConnectResponse() | ||
| rsp.totalCapacity = SizeUnit.GIGABYTE.toByte(1000) | ||
| rsp.availableCapacity = SizeUnit.GIGABYTE.toByte(1000) | ||
| rsp.success = true | ||
| return rsp | ||
| } | ||
|
|
||
| BackupStorageInventory bs = addSftpBackupStorage { | ||
| name = "sftp-timeout-test" | ||
| description = "test SFTP connect with timeout" | ||
| username = "root" | ||
| password = "password" | ||
| hostname = "localhost" | ||
| url = "/data" | ||
| } | ||
|
|
||
| assert bs != null | ||
| assert bs.uuid != null | ||
| assert bs.status == "Connected" | ||
| } |
There was a problem hiding this comment.
当前用例未覆盖“超时触发失败并解除阻塞”的核心场景。
该测试只验证 CONNECT 正常返回时可连接成功；即使去掉超时参数，这个用例也会通过，无法证明 ZSTAC-67815 的关键修复生效。建议补充一个超时/不可达分支（例如让 CONNECT_PATH 延迟超过 60 秒），并断言请求在预期窗口内失败且不会造成后续操作长期阻塞。
🤖 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
`@test/src/test/groovy/org/zstack/test/integration/storage/backup/sftp/SftpBackupStorageConnectTimeoutCase.groovy`
around lines 82 - 103, Add a negative test branch to testSftpConnectWithTimeout
that simulates a CONNECT_PATH delay exceeding the 60s timeout: use
env.simulator(SftpBackupStorageConstant.CONNECT_PATH) to introduce a long
sleep/delay and return no successful ConnectResponse, then call
addSftpBackupStorage and assert it fails within the expected timeout
(throws/returns error) and does not block further operations (for example,
immediately perform another lightweight addSftpBackupStorage or query and assert
it succeeds). Ensure assertions reference the test method name
testSftpConnectWithTimeout, the simulator hook
SftpBackupStorageConstant.CONNECT_PATH, and the
addSftpBackupStorage/BackupStorageInventory flow so the failure and non-blocking
behavior are validated.
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-67815-clean
branch
2 times, most recently
from
baeb946 to
b9c2f6c
Compare
Resolves: ZSTAC-67815 Change-Id: I2d789c3f4561ab78e90cd23ef45a678901234567
MatheMatrix
force-pushed
the
sync/songtao.liu/fix/ZSTAC-67815-clean
branch
from
b9c2f6c to
c81ce08
Compare
2 participants
Root Cause
SftpBackupStorage.continueConnect() uses syncJsonPost(url, cmd, ConnectResponse.class) without timeout. When SFTP agent HTTP layer is unreachable, this blocks indefinitely in BackupStorageBase.chainSubmit context, permanently holding the per-BS queue slot.
Fix
Add TimeUnit.SECONDS, 60 timeout to syncJsonPost call, ensuring 60s failure → completion.fail() → chain.next() releases queue slot.
Resolves: ZSTAC-67815
sync from gitlab !10012