fix(helper): don't let the accept deadline bound the whole transfer (#44)

[talmacschen-arch]

fix #44

---

Change logs

ConcurrentServer.serve() runs an unbounded Accept() loop that lives for the whole transfer. The listener's absolute 600 s deadline (set in ServerBase.Start()) therefore bounds the total transfer time of a single table, not just the accept phase: once all expected clients (--client-numbers) have connected, the loop blocks on Accept() again, and at start + 600 s that still-blocked Accept() returns i/o timeout → setError → the transfer is torn down. This is confined to ExtDestLtCopy + push (destination has fewer segments than source), i.e. any table whose data takes longer than ~10 minutes to move. See #44 for the full root-cause analysis.

Fix: make serve() accept-count aware — stop accepting once numConn connections are in, then Close() the listener so the in-flight data connections stream to completion with no listener deadline over them. A timeout/error before all expected clients have connected is still fatal (a client genuinely failed to connect), and the error path now returns instead of continue-ing, so a tripped deadline can no longer busy-spin flooding the log.

Scope: only ConcurrentServer (ExtDestLtCopy + push) is affected. OneTimeServer (CopyOnMaster / CopyOnSegment / ExtDestGeCopy and all pull-mode senders) closes its listener after a single Accept() and is unchanged — so --connection-mode=pull was, and remains, unaffected.

Validation

On a 16 → 8 segment setup (source 16 primaries, destination 8) copying a single large unpartitioned table (TPC-DS store_sales, 1 TB scale) in default push mode:

• The pre-fix helper reproduces Large-table copy aborts with SQLSTATE 22P04 after exactly 600s (ExtDestLtCopy + push): listener deadline caps total transfer time #44 at exactly start + 600 s — as an abort (could not write to COPY program: Broken pipe on the source; 22P04 / 25M01 on the destination) and, depending on timing, as an indefinite hang with no error surfaced to either side.
• After rebuilding just the receive-side helper with this change and re-running the same copy, it ran well past the 600 s cap and completed normally (successfully copied 1 tables, total elapsed 36m52s), with zero Listener.Accept() ... i/o timeout on the destination helpers.

The helper package currently has no unit tests; happy to add a focused serve() test (accept numConn → listener closed, Err()==nil, no timeout; fewer than numConn before the deadline → Err()!=nil, no busy-spin) if maintainers would like one in this PR.

Summary by CodeRabbit

• Bug Fixes
  • Server listeners now stop accepting connections after the expected number of clients connect.
  • Accept errors now terminate the connection-acceptance process promptly instead of being ignored.
  • Listeners close automatically once all expected connections are established, while existing connections remain active.

[coderabbitai]

📝 Walkthrough

Walkthrough

ServerBase.serve() now accepts only the expected number of client connections, exits immediately on accept errors, and closes the listener after the final expected connection is accepted.

Changes

Server acceptance lifecycle

Layer / File(s)	Summary
Bounded accept and listener shutdown helper/server.go	serve() tracks accepted connections, stops accepting at t.numConn, returns on accept errors, and closes the listener after all expected clients connect.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: preventing the accept deadline from capping the whole transfer.
Description check	✅ Passed	The description follows the required issue reference and change-log structure and provides substantial implementation and validation detail.
Linked Issues check	✅ Passed	The changes match #44 by stopping acceptance after numConn, closing the listener, and preserving fatal pre-connection errors.
Out of Scope Changes check	✅ Passed	The PR appears scoped to the listener/accept-loop fix in helper/server.go with no unrelated code changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@helper/server.go`:
- Line 159: Update the cleanup path containing t.listener.Close() to handle its
returned error instead of discarding it. Match the established approach in
helper/one_time_server.go by logging and surfacing listener shutdown failures
while preserving the existing cleanup flow.

🪄 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 Plus

Run ID: 58f05fb0-c7d5-47c5-be1d-d81274c7de4a

📥 Commits

Reviewing files that changed from the base of the PR and between 22d64a7 and 105c8c5.

📒 Files selected for processing (1)

• helper/server.go

[leaocx]

LGTM. Precise fix with solid validation.

• Bounded accept loop correctly prevents the listener deadline from capping the entire transfer
• continue → return on error path eliminates busy-spin
• Closing the listener does not affect already-established data connections
• Log-only (no setError) on listener-close failure is the right call — avoids reintroducing spurious teardown