Skip to content

Fix review findings: stderr API, UDS perms, signal range, write loop,…#7

Merged
nyo16 merged 1 commit into
masterfrom
review-fixes-stderr-uds-daemon
Jun 6, 2026
Merged

Fix review findings: stderr API, UDS perms, signal range, write loop,…#7
nyo16 merged 1 commit into
masterfrom
review-fixes-stderr-uds-daemon

Conversation

@nyo16
Copy link
Copy Markdown
Owner

@nyo16 nyo16 commented Jun 6, 2026

Summary

Addresses findings from a parallel code review of the recent changes (commits #4#6). One blocker, four warnings, and several cleanups.

Verified: mix format --check-formatted, mix compile --warnings-as-errors (clean C rebuild), mix credo --strict (no issues), mix test140 passed / 2 excluded.

Blocker

  • Remove documented-but-unimplemented stderr: :redirect. Output was silently dropped, and a child emitting enough stderr could block on a full pipe. :stderr is now validated at spawn time (:consume /
    :disabled only); docs, typespec, and README updated.

Security / correctness

  • nif_kill: reject signals outside 1..31 (mirrors the shepherd's CMD_KILL validation).
  • UDS socket now lives in a per-spawn 0700 directory, blocking the cross-user SCM_RIGHTS FD-hijack vector. Also cleans up the dir on failure paths (previously leaked empty dirs in tmp).
    Peer-credential verification (SO_PEERCRED) deferred — platform-divergent.
  • Write loop: a zero-byte write on a non-empty buffer is now mapped to EAGAIN + enif_select inside the NIF, dropping the GenServer-blocking Process.sleep(1) retry.
  • Daemon drains via Task.Supervisor.async_nolink (added NetRunner.TaskSupervisor to the app tree) so a drain-task crash can't take the Daemon down.

Cleanups

  • Log shepherd errors in handle_uds_message instead of dropping them.
  • Operations: O(1) demonitor via an op_ref -> mref reverse index (was O(n) Enum.find).
  • Command.new/3: guard clauses instead of unless/raise.

Tests

  • process_test: poll instead of sleep(500) for async cleanup (CI flake risk).
  • net_runner_test: unique pgrep -f marker — no longer racy under async: true.
  • signal_test: assert_raise now matches the error message.
  • daemon_test: added a drain-task isolation test.

Deferred (intentional)

  • write_all_input error propagation — broken-pipe on stdin is normal for head-style commands; making it fatal would regress.
  • cgroup symlink note (root/Linux, trusted config) and minor test-hygiene nits.

… Daemon

Address findings from a parallel code review of recent changes.

Blocker:
- Remove documented-but-unimplemented `stderr: :redirect` (output was
  silently dropped and the child could block on a full stderr pipe).
  `:stderr` is now validated at spawn time (`:consume`/`:disabled`);
  docs, typespec, and README updated.

Security/correctness:
- nif_kill: reject signals outside 1..31 (mirrors shepherd CMD_KILL).
- UDS socket now lives in a per-spawn 0700 dir, blocking cross-user
  SCM_RIGHTS hijack; also cleans up the dir on failure paths (was
  leaking empty dirs in tmp).
- Map a zero-byte write on a non-empty buffer to EAGAIN+enif_select in
  the NIF and drop the GenServer-blocking Process.sleep(1) retry.
- Daemon drains via Task.Supervisor.async_nolink (added
  NetRunner.TaskSupervisor) so a drain crash can't take the Daemon down.

Cleanups:
- Log shepherd errors in handle_uds_message instead of dropping them.
- Operations: O(1) demonitor via an op_ref -> mref reverse index.
- Command.new/3: guard clauses instead of unless/raise.

Tests:
- process_test: poll instead of sleep(500) for async cleanup.
- net_runner_test: unique `pgrep -f` marker (no longer racy under async).
- signal_test: assert_raise message patterns.
- daemon_test: drain-task isolation test.
@nyo16 nyo16 merged commit c2bbea1 into master Jun 6, 2026
10 checks passed
@nyo16 nyo16 deleted the review-fixes-stderr-uds-daemon branch June 6, 2026 14:06
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