fix(ipi): complete socket writes and bound paths

[njzjz-bot]

Fixes #5626

Summary

• reject /tmp/ipi_<host> names that cannot fit in sockaddr_un.sun_path instead of overflowing or truncating the address
• retry write() after EINTR and after partial progress until the full i-PI protocol buffer has been sent
• treat zero-progress writes as broken connections so the retry loop cannot spin forever
• add internal, documented helpers that keep the Fortran-compatible public API unchanged while allowing deterministic unit tests
• add a small CTest regression for exact path bounds, interrupted writes, repeated short writes, zero writes, and hard socket errors

Why existing tests missed this

The existing i-PI tests are ASE end-to-end tests configured with the short name localhost. Their force and virial messages are also small and sent through blocking local sockets, where a single write() normally accepts the whole buffer. They therefore never approach sun_path capacity and cannot force the kernel to return a deterministic short write or EINTR.

The new C regression injects the write behavior directly: its writer first returns EINTR and then accepts at most three bytes per call. As a negative control, temporarily restoring one-shot behavior made this test fail at the first complete-write assertion; restoring the loop makes the full payload comparison pass.

Validation

• cmake --build source/build --target test_ipi_sockets dp_ipi -j$(nproc --all)
• ctest --test-dir source/build -R ^test_ipi_sockets$ --output-on-failure
• pytest source/ipi/tests/test_driver.py::TestDPIPI::test_ase_unix -vv -s
• standalone C compilation with -Wall -Wextra -Werror
• standalone socket regression with ASAN and UBSAN
• ruff format .
• ruff check .
• clang-format applied to the changed C sources and header
• git diff --check

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

Summary by CodeRabbit

• Bug Fixes
  • Improved Unix-socket handling to safely construct /tmp/ipi_<host> paths and fail fast when the hostname is too long.
  • Strengthened socket write behavior to ensure the full payload is sent, with correct handling of interrupted/partial writes.
  • Added clearer validation for invalid write sizes and more consistent error reporting for stalled write scenarios.
• Tests
  • Added a new C regression test (test_ipi_sockets) covering Unix-socket path length enforcement and reliable full-buffer write behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 819c727e-af2e-4804-b6a3-cc07dd5285a4

📥 Commits

Reviewing files that changed from the base of the PR and between b70e643 and 69a8ff6.

📒 Files selected for processing (1)

• source/ipi/tests/test_sockets.c

---

📝 Walkthrough

Walkthrough

i-PI socket handling now bounds Unix socket paths, completes partial writes with retry and error handling, validates negative lengths, and adds C regression tests registered through CTest.

Changes

i-PI socket I/O hardening

Layer / File(s)	Summary
Socket helper contracts and implementations source/ipi/src/sockets_internal.h, source/ipi/src/sockets.c	Adds internal APIs for bounded Unix socket address construction and retry-aware complete writes, with errno-based validation and failure handling.
Socket operation integration source/ipi/src/sockets.c	Uses the address helper during Unix socket setup and routes writebuffer_ through complete-write handling while rejecting negative lengths.
Regression tests and CTest registration source/ipi/tests/test_sockets.c, source/ipi/CMakeLists.txt	Tests path-length bounds, interrupted and partial writes, zero-progress writes, propagated errors, overreporting writes, and negative lengths through a new CTest executable.

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

Sequence Diagram(s)

sequenceDiagram
  participant iPI as writebuffer_
  participant Helper as deepmd_write_all
  participant Socket as socket write callback
  iPI->>Helper: provide buffer and length
  Helper->>Socket: write remaining bytes
  Socket-->>Helper: partial count, EINTR, or error
  Helper->>Socket: retry until complete
  Helper-->>iPI: success or errno-based failure

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main fix: bounded Unix socket paths and complete socket writes.
Linked Issues check	✅ Passed	The PR addresses #5626 by bounding sun_path construction, retrying short/EINTR writes, and adding tests for both cases.
Out of Scope Changes check	✅ Passed	The added header, CMake wiring, and tests are all directly supporting the socket I/O fix and appear in scope.
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

---

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.}

[njzjz]

Possible reviewers based on changed lines, exact file history, and exact-file review history:

• @wanghan-iapcm — 3 commits on changed files (source/ipi/CMakeLists.txt, source/ipi/src/sockets.c).

No review request was made automatically.

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

[codecov]

Codecov Report

❌ Patch coverage is 85.00000% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.63%. Comparing base (1c5ff68) to head (69a8ff6).
⚠️ Report is 44 commits behind head on master.

Files with missing lines	Patch %	Lines
source/ipi/tests/test_sockets.c	85.24%	5 Missing and 4 partials ⚠️
source/ipi/src/sockets.c	84.61%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5831      +/-   ##
==========================================
+ Coverage   78.57%   78.63%   +0.05%     
==========================================
  Files        1049     1055       +6     
  Lines      120650   121850    +1200     
  Branches     4348     4421      +73     
==========================================
+ Hits        94806    95820    +1014     
- Misses      24285    24460     +175     
- Partials     1559     1570      +11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wanghan-iapcm]

The write-side and path fixes are correct and well-verified: the path arithmetic is byte-identical to the old code and to what the i-PI server constructs (the > bound reserves the NUL exactly — no off-by-one), and deepmd_write_all handles EINTR / partial / empty-buffer / signedness correctly. The injected-callback test genuinely fails on the pre-fix one-shot write(). One incomplete-fix note (the coverage note is inline below):

The symmetric read path is left with the mirror-image bug. readbuffer_ (

deepmd-kit/source/ipi/src/sockets.c

Lines 137 to 160 in b70e643

	int sockfd;
	struct hostent* server;
	struct sockaddr* psock;
	int ssock;
	if (*inet > 0) { // creates an internet socket
	struct sockaddr_in serv_addr;
	psock = (struct sockaddr*)&serv_addr;
	ssock = sizeof(serv_addr);
	sockfd = socket(AF_INET, SOCK_STREAM, 0);
	if (sockfd < 0) {
	error("Error opening socket");
	}
	server = gethostbyname(host);
	if (server == NULL) {
	fprintf(stderr, "Error opening socket: no such host %s \n", host);
	exit(-1);
	}
	bzero((char*)&serv_addr, sizeof(serv_addr));
	serv_addr.sin_family = AF_INET;
	bcopy((char*)server->h_addr, (char*)&serv_addr.sin_addr.s_addr,

) is untouched, yet it has the same class of defect this PR fixes for writes: it never retries on EINTR, and its only error guard is if (n == 0). So if the first read() returns -1 (EINTR or error), n == -1, the nr > 0 loop is skipped and the n == 0 check is false — the function returns having left data uninitialized (silent garbage forces/energies to the driver). A mid-transfer peer close (0 < n < len) is likewise silently accepted as a short buffer. Since the whole motivation here is EINTR/partial-syscall hardening, readbuffer_ deserves the same treatment (retry on EINTR, treat any final n < len as an error) — either in this PR or a follow-up. It's pre-existing and outside the diff, so non-blocking.

[wanghan-iapcm]

All four branches I listed are covered now: both deepmd_write_all preconditions, the EIO over-report guard via a writer that returns len + 1, both NULL arguments to deepmd_build_unix_socket_address, and a forked child that calls writebuffer_ with a negative length and checks the error text. That last one is the case nothing touched at all before, so it is good to see it reached directly rather than inferred.

I built and ran this rather than reading it. The suite compiles under gnu11 against sockets.c and passes. Since deepmd_write_all is new, it cannot be run against master to show it fails there, so I did the next best thing: I substituted a faithful one-shot implementation -- preconditions kept, the loop and the EINTR retry removed -- and the suite fails cleanly at

CHECK failed at tests/test_sockets.c:108: deepmd_write_all(123, payload, sizeof(payload) - 1, partial_writer) == 0

which is exactly the regression this PR exists to prevent. That works because of how partial_writer is built: EINTR on the first call, then at most three bytes per call, so passing requires both retrying without advancing the source pointer and looping until the buffer is drained. The comment explaining that choice is worth keeping -- it is the reason the test has teeth.

Two things I checked because they are easy to get wrong and often are. The test is registered with add_test(NAME test_ipi_sockets COMMAND test_ipi_sockets) inside if(CMAKE_TESTING_ENABLED), CI configures with BUILD_TESTING:BOOL=TRUE and runs ctest, and the job log confirms it actually executes:

    Start 4: test_ipi_sockets
4/4 Test #4: test_ipi_sockets .................   Passed    0.00 sec
100% tests passed out of 4

All four Test C++ jobs are green. And the rationale in the CMake comment is the right one -- deterministic short-write injection is not something an end-to-end socket test can ask the kernel for, so a small C regression beside the library is the correct shape for this rather than a driver-level test.

Approving.