fix: exclude grace period from first throughput window - #39
Merged
taoky merged 2 commits intoSep 16, 2026
Merged
Conversation
The relay-phase throughput floor anchored its first sampling window at the start of the relay (lastSampleTime := relayStartedAt) and only moved it forward once a measurement was taken. Because min_throughput_grace merely gated whether a measurement ran, and never touched lastSampleTime, the very first measurement compared the whole ramp-up period against min_throughput_bytes. That made the grace period ineffective in practice: a connection that was quiet during its ramp-up (for example rsync's initial file-list exchange for a very large module) but transferred well afterwards could be torn down by that first evaluation, even though the grace period was meant to exempt it. Anchor a fresh window when the grace period ends, so the first measurement only covers traffic after the grace period and the ramp-up is never counted. When min_throughput_grace is 0 the previous behaviour is preserved. Also document the effective floor (min_throughput_bytes / min_throughput_window) and recommend window = 300 (1 MiB over 5 minutes, ~3.4 KiB/s) instead of the previous window = 60 (~17.5 KiB/s), which was observed to terminate legitimate slow transfers, and correct the relay_max_duration description: rsync does not reconnect on its own.
There was a problem hiding this comment.
🟡 Changes recommended
The regression test timing is unreliable, and reconnect documentation remains inconsistent.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes relay throughput-floor sampling so grace-period traffic is excluded from the first measured window, with documentation and regression coverage updates.
Changes:
- Starts the first throughput window when the grace period ends.
- Updates throughput and relay-duration guidance.
- Adds regression coverage for post-grace throughput.
File summaries
| File | Summary |
|---|---|
README.md |
Updates documentation. Open nit (2 votes): reconnect wording remains inconsistent in assets/config.example.toml and pkg/server/config.go. |
pkg/server/server.go |
Implements grace-aware throughput sampling. |
pkg/server/server_test.go |
Adds regression coverage. Open moderate issue (1 vote): synchronize the test with relay start so it reliably catches the previous behavior. |
assets/config.example.toml |
Updates configuration guidance; reconnect wording still needs correction. |
Review details
Suppressed comments (1)
pkg/server/server_test.go:1864
- This sleep is measured from the client handshake, but
relayStartedAtis recorded only after the upstream connection setup. If setup takes more than the 100 ms margin, the pre-fix watcher can reach its first grace-boundary tick after the test starts writing 4000-byte payloads, so the old implementation may pass instead of being caught. Synchronize with the relay-start point (for example, the existing “starts requesting module” log) before waiting through the grace period.
// Wait until the grace period has elapsed.
time.Sleep(2100 * time.Millisecond)
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | --- | --- | --- | --- | --- | | ||
| | `relay_idle_timeout` | int 秒 | 0 | relay 阶段双向无 I/O 多久后关闭连接。语义同 rsyncd `timeout`。 | `600` | | ||
| | `relay_max_duration` | int 秒 | 0 | relay 阶段总时长上限。超时关闭,rsync 客户端通常会重连续传。 | `14400`(4h) | | ||
| | `relay_max_duration` | int 秒 | 0 | relay 阶段总时长上限。超时关闭;rsync 本身不会自动重连,是否续传取决于调用方(脚本/cron)。 | `14400`(4h) | |
taoky
approved these changes
Sep 16, 2026
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
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.
Problem
The relay-phase throughput floor anchors its sampling window at
relayStartedAtand only moves it forward after a measurement is taken:min_throughput_graceonly gates whether a measurement runs; it never toucheslastSampleTime. So the first measurement runs att ≈ minGraceand itsdeltacovers everything sincet0— i.e. the entire ramp-up period. The grace period is therefore ineffective: a connection that is quiet during its ramp-up (for example rsync's initial file-list exchange for a very large module) but transfers fine afterwards can be torn down by that first evaluation.Concretely, with
min_throughput_bytes = 1048576andmin_throughput_grace = 600, the first check requires 1 MiB of cumulative traffic inside the first 600s. A connection that is still building the file list of a very large module at t=600 is dropped even if it is transferring at full speed by then.Fix
Anchor a fresh window the moment the grace period ends, so the first measurement only covers traffic after the grace period:
When
min_throughput_grace <= 0,graceEndedstarts true andlastSampleTimeisrelayStartedAt, so the previous behaviour is preserved.Docs
min_throughput_bytes / min_throughput_window, and recommendwindow = 300(1 MiB over 5 minutes ≈ 3.4 KiB/s) instead of the previouswindow = 60(~17.5 KiB/s), which was observed to terminate legitimate slow transfers. The built-in default (60) is unchanged.relay_max_durationdescription: rsync does not reconnect on its own; whether a transfer is retried depends on how the client is invoked.Testing
TestThroughputFloorExcludesGracePeriodFromFirstWindow(pkg/server/server_test.go): the grace period transfers a few bytes, then the connection transfers above the floor; asserts the connection survives and nobelow throughput floorline is logged. It fails on the previous implementation.go vet ./... && go build ./... && go test -race -count=1 ./pkg/... ./cmd/...passes.golangci-lint run ./...: 0 issues.