fix(morpc): join accepted server connections on close

[ck89119]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #26550

What this PR does / why we need it:

MORPC server shutdown previously stopped the goetty application and returned after disconnecting active sessions, but goetty did not join the admitted connection handlers. Under CI load, leak detection could therefore run while doConnection, server write-loop, and peer backend goroutines were still unwinding.

This change:

• tracks every connection admitted by the goetty application;
• runs the connection read loop through an MORPC-owned handler so its completion is observable;
• makes successful server shutdown wait until all admitted handlers have returned;
• preserves exactly-once completion when a session is rejected before its handler starts; and
• adds a deterministic regression test proving that Server.Close cannot return before an accepted connection completes cleanup.

Validation:

• mo-cgo-test -race -count=100 -run '^TestServerCloseWaitsForAcceptedConnections$' ./pkg/common/morpc
• mo-cgo-test -race -count=1 ./pkg/common/morpc
• mo-cgo-test -count=50 -run '^TestStatusInRollingRestartCN$' ./pkg/lockservice
• mo-cgo-test -race -count=13 -run '^TestStatusInRollingRestartCN$' ./pkg/lockservice
• mo-cgo-test -race -count=1 ./pkg/lockservice
• go build ./pkg/common/morpc/... ./pkg/lockservice/...
• go vet ./pkg/common/morpc/... ./pkg/lockservice/...

After merging the latest main, both focused race regressions were rerun successfully.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[XuPeng-SH]

Deep review finding — P1 / merge blocker

The connection tracker is admitted too late to provide the shutdown guarantee claimed by this PR. goetty first accepts the connection, calls addSession(rs), and launches the connection handler with go func() { handle(rs) }. However, MORPC only calls connections.begin() after that goroutine is scheduled and enters handleConnection.

This leaves a valid shutdown interleaving:

1. The connection is accepted and added to goetty's session set.
2. The handler goroutine is created but has not run yet.
3. server.Close() calls application.Stop().
4. sealAndWait() observes active == 0 and returns.
5. The already-created handler goroutine runs afterward; begin() sees the tracker sealed and returns.

So Close joins callbacks that have already started, not all connections already admitted by the server. The goetty handler goroutine from #26550 can therefore still outlive Close. Also, finish() runs inside the callback, before the outer goetty goroutine finishes its deferred session cleanup, so the tracker does not join the complete connection goroutine even on the started path.

The new test does not cover this boundary because it manually calls tracker.begin() before invoking Close(). I reproduced the missing state with a deterministic gated-handler test; the current implementation fails immediately because Close() returns before an already accepted handler is first scheduled. The existing test passes under -race -count=20, and the full morpc race suite passes, confirming that CI is exercising only the already-started path.

Please move the lifecycle accounting to the actual goetty admission/ownership boundary:

• reserve the connection before launching its goroutine;
• release it with a defer at the outermost goroutine boundary;
• in Stop(), stop and join accept loops first so no more reservations can occur;
• disconnect sessions to release blocked reads;
• then wait for all admitted connection goroutines to exit.

Please also add a deterministic regression test for the accepted-but-handler-not-yet-started state. This fixes the ownership protocol rather than relying on scheduling or a scenario-specific delay.

[XuPeng-SH]

Requesting changes for the P1 shutdown-ownership gap documented in the prior comment.

connections.begin() executes only after the goetty handler goroutine is scheduled, while connection admission happens before that goroutine is launched. Close() can therefore seal an empty tracker and return while an already accepted handler is still queued, leaving the original goroutine-leak failure possible.

Please move accounting to the goetty admission/outer-goroutine boundary and add a deterministic accepted-but-not-yet-started regression test.

[ck89119]

@XuPeng-SH Thanks for the detailed interleaving. The original change was intentionally scoped to MatrixOne: after stopping the goetty application, MORPC seals a tracker and waits for active MORPC handler callbacks to return. I also moved the tracking into the MORPC handler so that a caller-provided IOSessionAware is preserved.

I agree that this only accounts for callbacks after they start. It cannot cover a connection that goetty has already accepted and admitted but whose handler goroutine has not run yet, and the callback-level finish() also precedes goetty's outer session cleanup.

I investigated MatrixOne-only alternatives. A strict fix there would require either duplicating goetty's application/accept loop in MORPC or vendoring a patched goetty implementation, because the required admission and outer-goroutine ownership boundaries are currently internal to goetty. Neither looks like a good small fix.

Do you think we should modify matrixorigin/goetty itself to reserve before launching the handler goroutine, release after the outer session cleanup, and make Stop() wait for all admitted handler goroutines, with the deterministic accepted-but-not-started regression test there? MatrixOne could then bump the goetty dependency and remove the local tracker/custom handler logic.

[XuPeng-SH]

Re-review of f5dec32bf14f after the author response: P1 / REQUEST CHANGES remains.

Yes, fixing this in matrixorigin/goetty, then bumping MatrixOne's dependency and removing the MORPC-local tracker/custom read loop, is the correct ownership boundary. The current head is unchanged, however, and still does not establish the shutdown postcondition claimed by the PR.

The complete ownership/wait chain is:

1. goetty accepts a socket, constructs the IOSession, calls addSession, and only then launches the outer handler goroutine.
2. MORPC increments connections.active only after that goroutine enters handleConnection.
3. goetty.Stop closes listeners and waits only for accept loops, removes/disconnects sessions, and returns without joining handler goroutines.
4. MORPC then seals the callback-level tracker.

Therefore an admitted-but-not-yet-entered handler is absent from the tracker. I reran a deterministic gated admission test on this exact head: server.Close() returned <nil> in 0.00s before the admitted handler was allowed to run. The PR's test passes under -race -count=20 because it manually calls tracker.begin() before Close, so it covers only the already-started state.

There is also an outer-cleanup closure that the upstream fix must address. On Stop, goetty deletes each session and calls only Disconnect; afterward the handler defer calls deleteSession, which returns false when running == false, so rs.Close()/IOSessionAware.Closed is skipped. A real raw accepted connection that had not sent its first MORPC message reproduced this under -race: Close returned without the matching session Closed callback (deterministic failure in 0.21s). Thus waiting only for the callback would still not provide exactly-once session destruction.

Required fix shape in goetty:

• reserve handler ownership before launch, under the same admission state/lock that prevents new reservations after Stop begins;
• release at the outermost goroutine defer, after the admitted session has reached exactly-once final Close;
• close listeners and join accept loops, disconnect sessions to release blocked reads, then join all reserved handlers;
• continue teardown and waiting even if a listener close reports an error, returning the error only after cleanup;
• add deterministic upstream regressions for both admitted-before-handler-start and an idle accepted session's final Closed callback.

Then bump the goetty version here and remove the MORPC-local tracker/custom loop. Existing package validation is otherwise clean: build/vet pass, the full pkg/common/morpc race suite passes, and TestStatusInRollingRestartCN passes under -race -count=5; those successes do not exercise the missing ownership states above.