Веб-сервер: прием WebSocket и повторный Запустить

[sfaqer]

ВебСокет.ПолучитьСтроку() и ПолучитьДвоичныеДанные() писали в результат весь буфер на 1024 байта, а не result.Count, и возвращали GetBuffer() — в конце любого сообщения был мусор/нули. Теперь только полученные байты.

Прием сообщений идет под блокировкой: WebSocket допускает одно ожидающее получение, и два задания, читающие один сокет, падали с InvalidOperationException; сообщение из нескольких частей читает один поток целиком.

Повторный ВебСервер.Запустить() (например, из фонового задания) подменял приложение: первый запуск потом освобождал чужое, а Остановить() первый запуск не останавливал. Теперь это ошибка «Веб-сервер уже запущен». Заодно обертки свойств контекста запроса создаются под блокировкой — контекст могут передать в задания.

Тесты WebServerConcurrencyTests в Core.Tests.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • WebSocket messages are now received as complete, separate messages when multiple reads occur concurrently.
  • Starting a server that is already running now returns a clear error. The server can be started again after it stops or encounters a failure.
  • Improved reliability when multiple operations access shared server data.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 54cf4330-055d-419b-b87f-ba3d9de93265

📥 Commits

Reviewing files that changed from the base of the PR and between 36e2ca9 and bfcc2c5.

📒 Files selected for processing (4)

• src/OneScript.Web.Server/PropertyWrappersCollection.cs
• src/OneScript.Web.Server/WebServer.cs
• src/OneScript.Web.Server/WebSockets/WebSocketWrapper.cs
• src/Tests/OneScript.Core.Tests/WebServerConcurrencyTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

The changes synchronize property-wrapper cache access, prevent overlapping server runs, and serialize WebSocket receive operations. Integration tests cover concurrent socket reads and attempts to start the same server twice.

Changes

Web server concurrency

Layer / File(s)	Summary
Property-wrapper cache access src/OneScript.Web.Server/PropertyWrappersCollection.cs	Get<T> now locks the cache during lookup, wrapper creation, insertion, and return.
Server run guard src/OneScript.Web.Server/WebServer.cs	Run rejects a start while another run is active and resets the running flag after completion or failure. The app reference is now volatile.
Complete WebSocket receives src/OneScript.Web.Server/WebSockets/WebSocketWrapper.cs	Receive operations now use a shared lock. Message fragments append only the bytes reported as received, and string and binary methods use the accumulated message bytes.
Concurrency integration tests src/Tests/OneScript.Core.Tests/WebServerConcurrencyTests.cs	Tests exercise concurrent reads on one socket and rejection of a second server start. Helpers set up the engine and exchange WebSocket frames.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to bfcc2

The changes protect concurrent server operations and return correctly sized WebSocket messages. The tests reject padded message results; no actionable merge-blocking issue was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to bfcc2

The changes improve concurrent server operation and message integrity. No introduced security-boundary expansion was established. Assurance remains limited for interrupted receives and concurrent shutdown or restart behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — A peer withholding the remainder of a message can keep a high-level receiver waiting and queue other receivers behind it on the same wrapper. The added monitor is wrapper-local, not a service-wide lock. Aggregate resource exposure depends on application task creation and connection limits, which were not established.

Security Findings and Attack Paths

• observed — Unbounded fragmented-message accumulation and receives without an explicit cancellation token existed in the base implementation. The head retains these limits while correcting byte-count handling and serialization; the comparison does not establish a newly introduced resource-exhaustion vulnerability.

Trust Boundaries and Controls

• observed — The inspected script-visible acceptance path wraps the socket returned by AcceptWebSocketAsync in a new WebSocketWrapper. Synchronization is therefore scoped to that wrapper; the public C# constructor does not itself enforce unique wrapping of an underlying socket.
• inferred — The lock prevents simultaneous underlying receives but does not assign logical message ownership across successive low-level Receive calls. A low-level call returning an incomplete fragment can be followed by a high-level call consuming the remainder. This possibility predates the PR and still requires caller coordination.

Resilience and Maintainability Implications

• inferred — Receive exceptions release the monitor, and Abort, Close and CloseOutput do not acquire it, so the new lock does not prevent entry to those terminal operations. Completion of concurrent close handshakes depends on the underlying socket implementation and was not verified.

Hardening Proposals

• proposed — Consider explicit message-size and receive-duration limits, with cancellation and cleanup semantics, to bound the inherited exposure to peers that send oversized or unfinished messages.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 21.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the two main changes: WebSocket message reception and repeated web-server startup handling.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[sfaqer]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[sfaqer]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[sfaqer]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.