Skip to content

fix(script): interrupt blocked socket writes on close - #1891

Open
TimotheusG wants to merge 3 commits into
storytold:mainfrom
TimotheusG:fix/socket-write-cancellation
Open

TimotheusG wants to merge 3 commits into
storytold:mainfrom
TimotheusG:fix/socket-write-cancellation

Conversation

@TimotheusG

@TimotheusG TimotheusG commented Oct 19, 2026 •

Copy link
Copy Markdown

Closing a socket on Windows did not reliably interrupt a blocked write_all: close returned, but the writer remained blocked. The existing regression fixture hid this because Windows could accept its entire 64 MiB write before any backpressure occurred.

Use a nonblocking write loop with one operation deadline and an explicit close flag, yielding for at most 5 ms while the send buffer is full. Restore blocking mode before returning so subsequent reads retain their behavior. The global table remains unlocked during I/O, and only the socket being written is locked.

The test peer now has bounded receive/send buffers and is filled until it actually reports backpressure. socket2 is a native-only dev dependency for configuring that fixture; the write payload shrinks to 64 KiB. The close test verifies the writer is still pending before closing. A further scenario checks complete 256 KiB transfers and delayed reads on both outgoing and accepted sockets. Related: #1248.

The corrected fixture reproduced the close failure before the production fix. All three socket tests then passed in 10 consecutive runs (30 checks). Companion fixes: Unix-only test-helper guard (#1889) and host-independent font tests (#1890).

Validation on Windows x64 (release, LTO disabled and 16 codegen units):

  • cargo xtask ci passed with all three maintenance fixes applied together: 3,657 tests passed, 32 existing opt-in tests ignored; format, strict Clippy, dependency/layer/asset audits and all 32 wasm crate checks passed.
  • The independent branches retain the failures addressed by their companion PRs; the full-CI result is for their combination.
  • An earlier run hit the existing intermittent theme::script_font_tests::simplified_chinese_interface_draws_simplified_hanzi_after_another_language failure. The identical failure was reproduced in the pre-change UI test binary (run 23 with --test-threads=64). Five complete UI runs with this patch passed, followed by the successful full CI rerun. This separate intermittent issue remains unresolved; no test was disabled to get the passing run.

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability when writing to network streams. Writes now honor a time limit and report when they time out, cannot make progress, or are interrupted by a socket closing.
    • Temporary interruptions can be retried, and successful writes work on both outgoing and accepted connections.
    • Closing a socket can interrupt a blocked write without preventing writes on other sockets.
    • Improved handling of socket mode-change failures and connection checks, including reporting when a write and its mode restoration both fail.

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 11572e00-2975-4c50-893b-ca3551fa73b4
📥 Commits

Reviewing files that changed from the base of the PR and between 4a84b4d and f667b6e.

📒 Files selected for processing: 1
  • crates/script/src/socket.rs

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

📝 Walkthrough

Native socket handling now tracks closure and retires streams when mode changes fail. Writes use deadline-bounded nonblocking sends and restore blocking mode. Tests cover backpressure, timeout, close interruption, mode failures, and complete transfers.

Changes: Socket writes

Layer / File(s) Summary
Socket close state and dispatch
crates/script/src/socket.rs
Socket entries track closure. Closing shuts down the wake stream, and dispatch checks closure after locking an entry.
Mode changes and bounded I/O
crates/script/src/socket.rs
Mode-change failures retire the stream. Writes use a deadline-bounded nonblocking loop and restore blocking mode. Connection checks use checked mode transitions.
Socket I/O test coverage
crates/script/Cargo.toml, crates/script/src/socket.rs
The native test dependency includes socket2. Tests cover backpressure, timeout, close interruption, mode failures, combined errors, and complete transfers.

Priority: ➖ Normal

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

Change: Bug fix

Suggested reviewers: echelon

Merge Risk: ⚪ Minimal · up to f667b

No actionable merge-blocking socket issue remains; the write deadline also applies when a peer continues to make slow progress.

Security architecture risk: 🔵 Low · up to f667b

The reviewed change preserves existing network permissions and socket reachability while improving cancellation and connection-level failure containment. Residual risk is concentrated in concurrent cleanup and rare operating-system failures, rather than expanded access or privileges.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A peer can still induce backpressure on an existing connection, but the changed lifecycle state is attached to that connection's entry. Outbound host selection, loopback listening, and process-global numeric handles predate this PR; the comparison establishes no new network destination, listener exposure, or handle authority.

Trust Boundaries and Controls

  • observed — Script-controlled operations, handles, and parameters still pass through native_sock's existing network-permission check before native dispatch. The PR does not move or bypass this control, and runtime registration and the script bridge remain unchanged.

Resilience and Maintainability Implications

  • observed — Mode-transition failures mark the entry unusable and attempt stream shutdown. Later operations reject it or report disconnected/EOF rather than reuse uncertain I/O state. This retirement does not itself remove the table entry; explicit close remains the removal path.
Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the primary change: interrupting blocked script socket writes when the socket closes.
Docstring Coverage Passed Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 1 files.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Warning

Some tools did not complete. Review the errors below.

🔧 Clippy (1.98.1)

Clippy execution timed out

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

@TimotheusG

Copy link
Copy Markdown
Author

Reproduced the recovery gap with a per-operation injected OS mode-restoration failure: the write reported an error but the stream still reported connected. Failed nonblocking/blocking transitions now mark the entry unusable and shut down the connection. Callers recheck that state after taking the socket lock, so subsequent or queued I/O cannot use the wrong mode; connected/eof return false/true, and close remains idempotent. The same protection covers the connected-status peek. If the write and restoration both fail, the returned error retains both causes.

Two new regression tests cover both mode transitions during write/connected, subsequent read/readln/write rejection, close, and combined failures. All five socket tests passed, including real loopback delivery, timeouts, and concurrent close. The failure is deliberately injected; no naturally occurring platform trigger is claimed.

Pushed in f667b6e. The final combined Windows cargo xtask ci passed: 3,685 tests passed, 0 failed, 33 unchanged ignored; formatting, strict Clippy, audits and 32-crate WASM checks passed. The standalone socket PR file is byte-identical to the file validated in the combined audio branch.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant