Skip to content

feat(decdn_node): validate manual binaries are ELF + drain-safe stop timeout - #42

Merged
thiras merged 3 commits into
mainfrom
feat/manual-binary-validation-and-drain-timeout
Sep 9, 2026
Merged

thiras merged 3 commits into
mainfrom
feat/manual-binary-validation-and-drain-timeout

Conversation

@alpergundogdu

Copy link
Copy Markdown
Contributor

Two independent hardening changes to the decdn_node role, both driven by real operational pain deploying the internal fleet.

1. decdn_release_target_dir — validated manual-binary resolution

New manual-mode knob pointing at the Cargo target/release dir. The role:

  • derives both binary paths from it (replaces spelling out decdn_node_manual_bin_src + decdn_cli_manual_bin_src);
  • falls back to the cross / --target output dir target/{{ decdn_node_target }}/release when the plain dir lacks the binaries;
  • validates on the control machine (via file) that both binaries are ELF for decdn_node_target before copying them to the node.

Why: a host-native cargo build --release on a non-Linux control machine (macOS) silently produces a Mach-O the Linux node can't exec — today that surfaces as an opaque crash on first start. Now it fails at deploy time with an actionable message:

…/decdn-node is "Mach-O …" — not an ELF x86-64 binary for x86_64-unknown-linux-gnu. … Cross-compile: cross build --release --target x86_64-unknown-linux-gnu …

Explicit *_manual_bin_src paths remain the escape hatch — copied verbatim, not format-checked — so a test-harness stub (legitimately not an ELF for the node's arch) still works. Reuses the existing decdn_node_target for both the fallback triple and the expected arch; no new arch knob.

2. decdn_node_stop_timeout_sec (default 300)

Replaces the hardcoded TimeoutStopSec=30 in the unit. SIGTERM enters the same graceful teardown as decdn node drain — stop accepting work, await in-flight streams to zero, flush settlement + the receipt/audit tail — and the process exits the instant that finishes, so the timeout is only the cap. At 30 s a SIGKILL could truncate an in-flight drain, dropping paid deliveries and possibly the audit tail (a money-losing cut). 300 s is generous headroom; normal stops still exit immediately.

Tests

  • New molecule validation negative case (binary-format) proving the ELF assert rejects a non-ELF binary in dir-mode (added to the expected-rejection tally).
  • Logic verified locally across four cases: Mach-O rejection, empty-dir "not found", real ELF x86-64 accept, and cross-dir fallback resolution.

Sync note

Format validation + fallback apply only to decdn_release_target_dir mode. Existing consumers using explicit *_manual_bin_src are unaffected; the TimeoutStopSec default rises 30→300. The internal instance (decdn/internal-devops) adopts both in a companion PR.

🤖 Generated with Claude Code

…timeout

Two independent hardening changes to the decdn_node role.

1. decdn_release_target_dir — a manual-mode knob pointing at the Cargo
   target/release dir. The role derives both binary paths from it, falls
   back to the cross/`--target` output dir (target/<triple>/release), and
   validates on the control machine (via `file`) that both binaries are
   ELF for decdn_node_target BEFORE copying them to the node. This catches
   the common footgun of a host-native `cargo build --release` on macOS
   (a Mach-O the Linux node can't exec) with an actionable error instead
   of an opaque first-start crash. Explicit *_manual_bin_src paths remain
   the escape hatch (copied verbatim, not format-checked) so test stubs
   still work. Reuses the existing decdn_node_target for both the fallback
   triple and the expected arch — no new arch knob.

2. decdn_node_stop_timeout_sec (default 300) replaces the hardcoded
   TimeoutStopSec=30 in the unit. SIGTERM enters the same graceful drain
   as `decdn node drain` (await in-flight streams, flush settlement +
   receipt/audit tail); the process exits the instant that finishes, so
   the timeout is only the cap. 30s risked a SIGKILL truncating an
   in-flight drain — dropping paid deliveries and possibly the audit tail.

Adds a molecule `validation` negative case proving the ELF format assert
rejects a non-ELF binary in dir-mode. README updated.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 9, 2026 11:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new molecule negative test case will fail without creating the staged target directory, and the dir-selection logic can miss/fail on valid fallback layouts when only one of the two binaries exists in the first candidate directory.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens the ansible/roles/decdn_node role by (1) adding control-machine validation for “manual” locally-built binaries when sourced from a Cargo target/release directory, and (2) making the systemd stop timeout configurable (defaulting to a longer, drain-safe value).

Changes:

  • Add decdn_release_target_dir workflow for manual installs, including cross-dir fallback and ELF/arch validation on the control machine before copying.
  • Replace hardcoded TimeoutStopSec=30 with decdn_node_stop_timeout_sec (default 300) in the systemd unit.
  • Extend the molecule validation scenario with a negative test case to ensure non-ELF binaries are rejected in dir-mode.
File summaries
File Description
ansible/roles/decdn_node/templates/decdn-node.service.j2 Makes TimeoutStopSec configurable via decdn_node_stop_timeout_sec and documents drain behavior.
ansible/roles/decdn_node/tasks/main.yml Adds manual binary dir resolution + ELF/arch validation on the controller before copy/install.
ansible/roles/decdn_node/README.md Documents the new decdn_release_target_dir manual install option and behavior.
ansible/roles/decdn_node/defaults/main.yml Introduces defaults for decdn_release_target_dir and decdn_node_stop_timeout_sec.
ansible/molecule/validation/converge.yml Adds a negative molecule case (binary-format) to prove non-ELF binaries are rejected in dir-mode.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ansible/molecule/validation/converge.yml
Comment thread ansible/roles/decdn_node/tasks/main.yml Outdated
Comment thread ansible/roles/decdn_node/tasks/main.yml
alpergundogdu and others added 2 commits September 9, 2026 12:55
The role's derive-paths set_fact leaves decdn_node_manual_bin_src as a
host fact (outranking the play var) pointing at the removed badbin dir;
reset it in always so the later config-gate case (which reaches the copy
step and does not re-pass the path) resolves the stub again.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… file check

Address review feedback on the manual-binary resolution:
- Explicit *_manual_bin_src (when BOTH set) now take precedence over a
  fleet-wide decdn_release_target_dir, so a per-host override works
  without blanking the group var; a partial (one-of-two) explicit
  override is now rejected loud instead of silently ignored.
- Candidate-dir selection requires BOTH decdn-node AND decdn present, so
  a dir with only the daemon yields to the cross dir that has the pair.
- `file` format check uses argv + `--` (robust to spaces / leading dash).
- molecule: create the staging dir before copying the stub pair (copy
  does not create parent dirs — this was the failing binary-format case).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@alpergundogdu
alpergundogdu requested a review from thiras September 9, 2026 13:06
@thiras
thiras merged commit 8616e06 into main Sep 9, 2026
8 checks passed
@thiras
thiras deleted the feat/manual-binary-validation-and-drain-timeout branch September 9, 2026 23:04
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.

3 participants