feat(owned-api): recognise network accept() factories (Socket/TcpListener)#155
Merged
Conversation
…ener) Next owned-API recall tranche, same approach as the ADO.NET one: a server that accepts a connection OWNS the returned client/socket and must dispose it; dropping it leaks the accepted connection (the handle is held until finalization) — the classic accept-loop server leak. `IsOwningFactory` now recognises the instance "accept" members, matched by the concrete BCL receiver type + method name (Socket / TcpListener are not subclassed in practice, like the File branch) with the result's IDisposable pinned, so the async variants (AcceptAsync / AcceptTcpClientAsync -> Task/ValueTask) are excluded: * Socket.Accept() -> a new connected Socket the caller must dispose * TcpListener.AcceptSocket() -> a new connected Socket the caller must dispose * TcpListener.AcceptTcpClient() -> a new TcpClient the caller must dispose Corpus fixture `tcplistener-accept-leak`: before.cs leaks the accepted client (OWN001), after.cs disposes it with `using` (clean), case.own reduces the pattern. Locally verified: case.own -> OWN001 (test_corpus 22/22), full suite green, mypy --strict, ruff. The C# branch + before/after end-to-end run are verified by CI (golden C# / leak extractor / corpus benchmark) — no dotnet locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughAdds an accept-loop ownership rule for ChangesTCP accept ownership detection
Sequence Diagram(s)sequenceDiagram
participant RoslynAnalysis
participant IsOwningFactory
participant ImplementsIDisposable
participant AnyDisposableArgument
RoslynAnalysis->>IsOwningFactory: inspect Socket.Accept / TcpListener.AcceptSocket / AcceptTcpClient
IsOwningFactory->>ImplementsIDisposable: check resolved return type
IsOwningFactory->>AnyDisposableArgument: check invocation arguments
IsOwningFactory-->>RoslynAnalysis: classify as owning or fall through
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
frontend/roslyn/OwnSharp.Extractor/Program.cs (1)
2336-2342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for the other two hard-coded accept branches.
This matcher now recognizes three specific members, but the new corpus in this PR only proves
TcpListener.AcceptTcpClient(). A typo or future regression inSocket.Accept()orTcpListener.AcceptSocket()would slip through without a dedicated fixture/assertion.🤖 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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs` around lines 2336 - 2342, The matcher in the disposable-return check now has three hard-coded accept paths, but only TcpListener.AcceptTcpClient() is currently covered. Add direct test coverage/fixtures for Socket.Accept() and TcpListener.AcceptSocket() alongside the existing AcceptTcpClient() case, using the same matcher path in Program.cs so regressions in those branches are caught.
🤖 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.
Nitpick comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2336-2342: The matcher in the disposable-return check now has
three hard-coded accept paths, but only TcpListener.AcceptTcpClient() is
currently covered. Add direct test coverage/fixtures for Socket.Accept() and
TcpListener.AcceptSocket() alongside the existing AcceptTcpClient() case, using
the same matcher path in Program.cs so regressions in those branches are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6aa79eba-f736-41b6-8c25-6753f53096b6
📒 Files selected for processing (6)
corpus/real-world/tcplistener-accept-leak/after.cscorpus/real-world/tcplistener-accept-leak/before.cscorpus/real-world/tcplistener-accept-leak/case.owncorpus/real-world/tcplistener-accept-leak/expected-diagnostics.txtcorpus/real-world/tcplistener-accept-leak/notes.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
…nches CodeRabbit: the accept matcher has three hard-coded branches but only AcceptTcpClient had an end-to-end fixture, so a typo/regression in the Socket.Accept or TcpListener.AcceptSocket branch would slip the corpus benchmark (its after.cs specificity check + recall visibility). Add a corpus fixture for each: socket-accept-leak (Socket.Accept) and tcplistener-acceptsocket-leak (TcpListener.AcceptSocket) — before.cs leaks the accepted socket (OWN001), after.cs disposes it with `using` (clean), case.own reduces the pattern. case.own -> OWN001 locally (test_corpus 24/24); before/after verified by the CI corpus benchmark. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
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
2 participants
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.
Что и зачем
Следующий транш owned-API recall тем же приёмом, что и ADO.NET (#154): сервер, принимающий соединение, владеет возвращённым client/socket и обязан его диспоузить — брошенный, он течёт (handle держится до финализации) — классическая утечка accept-цикла.
IsOwningFactoryтеперь распознаёт instance-методы "accept", матчируя по конкретному BCL-типу receiver'а + имени метода (Socket/TcpListener на практике не наследуют, как в ветке File) с пиннингом результата наIDisposable, так что async-варианты (AcceptAsync/AcceptTcpClientAsync→Task/ValueTask) исключены:Socket.Accept()→ новый connectedSocket,TcpListener.AcceptSocket()→ новый connectedSocket,TcpListener.AcceptTcpClient()→ новыйTcpClient.Тип изменения
Как проверено
python tests/run_tests.py— зелёный; новый corpus-кейсtcplistener-accept-leakчерезcase.ownдаёт OWN001 (corpus 22/22), bridge 208/208, остальные сюитыruff check .иmypy(--strict) — чистоВАЖНО: нет dotnet локально → C#-ветка
IsOwningFactoryи прогонbefore.cs/after.csчерез реальный экстрактор проверяются CI:golden C# compiles & runs,C# leak extractor (Roslyn), иcorpus benchmark(before пойман как OWN001, after чист, без регресса specificity). Локально верифицирована pure-Python половина (case.own→ OWN001).Связанные issue
Нет issue для закрытия. Refs: P-005/P-006 P1a recall ladder; продолжает owned-factory распознавание (#153 Xml/Json, #154 ADO.NET).
Чеклист
corpus/real-world/tcplistener-accept-leak/: before/after/case.own/expected/notes)IsOwningFactory+ notes.md)feat(owned-api): …)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
using var-scoped disposal for accepted clients/sockets.