Let a declared unit carry a file-descriptor limit (#78)

CHANGELOG.md

@@ -8,6 +8,20 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ### Added
 
+- `module_spec::UnitTemplate::limit_nofile`, an optional `LimitNOFILE=` a
+  package declares against its own unit, so a service whose store outgrows the
+  soft descriptor limit systemd hands a unit says so itself instead of leaving
+  an operator to raise it on the host. The renderer emits the directive between
+  `RestartSec=` and the sandbox booleans. Absence, which is what every unit
+  rendered until now carries, inherits the host's soft limit; there is no
+  spelling for systemd's `infinity`, so leaving a service unbounded stays the
+  host's decision rather than a package's: a declared zero is refused as
+  `module_spec::ModuleSpecError::ZeroLimitNofile`, and `u64::MAX` — the
+  numeric value of Linux's `RLIM_INFINITY`, which systemd's own rlimit parser
+  refuses — as `module_spec::ModuleSpecError::InfiniteLimitNofile`. A producer
+  now stamps manifest format version 5 and this build still reads 3, so a
+  payload already published stays readable, installable, and byte-identical in
+  what it renders.
 - `module_spec::RenderVar::ManagerEndpoint`, with which a unit template names
   the manager endpoint its module is pointed at, and the
   `render::RenderContext::manager_endpoint` field the renderer resolves it

assets/test-fixtures/unsigned-container/manifest.json

@@ -1 +1 @@
-{"format_version":4,"archive_members":[{"name":"bin/app","length":17}],"artifacts":[{"component":"example","version":"1.0.0","commit":"0123456789abcdef0123456789abcdef01234567","target_arch":"x86_64","kind":"native-binary","dispositions":["install"],"archive_path":"bin/app","sha256":"bcf23e595c273e3202d8fc0fb47b5d6107a14fbc2d0a08c97ae4dd0262907d81"}]}
+{"format_version":5,"archive_members":[{"name":"bin/app","length":17}],"artifacts":[{"component":"example","version":"1.0.0","commit":"0123456789abcdef0123456789abcdef01234567","target_arch":"x86_64","kind":"native-binary","dispositions":["install"],"archive_path":"bin/app","sha256":"bcf23e595c273e3202d8fc0fb47b5d6107a14fbc2d0a08c97ae4dd0262907d81"}]}

src/manifest.rs

@@ -22,7 +22,7 @@ use crate::module_spec::{ModuleSpec, ModuleSpecError};
 /// Stamped unconditionally, never derived from a manifest's contents: a
 /// manifest's version is the producer's, not a function of which optional
 /// variables its templates happen to name.
-pub const MANIFEST_FORMAT_VERSION: u32 = 4;
+pub const MANIFEST_FORMAT_VERSION: u32 = 5;
 
 /// Inclusive floor of the `format_version` range this build accepts.
 ///
@@ -40,7 +40,7 @@ pub const MIN_MANIFEST_FORMAT_VERSION: u32 = 3;
 /// Tracks [`MANIFEST_FORMAT_VERSION`]: this build reads what it writes, and a
 /// manifest naming anything newer is refused for its version alone rather than
 /// failing as an opaque decode error somewhere inside it.
-pub const MAX_MANIFEST_FORMAT_VERSION: u32 = 4;
+pub const MAX_MANIFEST_FORMAT_VERSION: u32 = 5;
 
 /// Container footer version the pre-versioned baseline payloads were written
 /// at.
@@ -1037,6 +1037,7 @@ mod tests {
                 ],
                 restart: RestartPolicy::Always,
                 restart_sec: 5,
+                limit_nofile: Some(8000),
                 protect_home: true,
                 private_tmp: true,
                 no_new_privileges: true,
@@ -1249,17 +1250,18 @@ mod tests {
     }
 
     #[test]
-    fn the_producer_writes_the_ceiling_and_the_floor_is_no_higher() {
-        // The range has a floor as well as a ceiling, and both are written by
-        // repointing a constant rather than by searching for a literal. The
-        // ceiling tracks the producer exactly — this build reads what it
-        // writes — while the floor only ever trails it, because an additive
-        // schema bump leaves every manifest already published at the floor
-        // readable.
-        assert_eq!(MAX_MANIFEST_FORMAT_VERSION, MANIFEST_FORMAT_VERSION);
-        // In a `const` block, so a floor raised past the producer is a build
-        // failure rather than a failing test.
-        const { assert!(MIN_MANIFEST_FORMAT_VERSION <= MANIFEST_FORMAT_VERSION) };
+    fn the_accepted_range_is_open_below_the_producer_and_closed_at_it() {
+        // The producer writes at the ceiling — a build never stamps a version
+        // it would then refuse to read — and the floor sits strictly below it,
+        // so the window admits more than one version. Pinned rather than left
+        // implicit because a later bump that moved all three together would
+        // close the window again and silently strand every payload published
+        // at the older version.
+        //
+        // Both hold at compile time, so both are stated that way: a bump that
+        // closed the window fails the build rather than one test run.
+        const _: () = assert!(MAX_MANIFEST_FORMAT_VERSION == MANIFEST_FORMAT_VERSION);
+        const _: () = assert!(MIN_MANIFEST_FORMAT_VERSION < MANIFEST_FORMAT_VERSION);
     }
 
     #[test]
@@ -1820,21 +1822,22 @@ mod tests {
         assert_eq!(unit.environment, expected.environment);
         assert_eq!(unit.restart, expected.restart);
         assert_eq!(unit.restart_sec, expected.restart_sec);
+        assert_eq!(unit.limit_nofile, expected.limit_nofile);
         assert_eq!(unit.protect_home, expected.protect_home);
         assert_eq!(unit.private_tmp, expected.private_tmp);
         assert_eq!(unit.no_new_privileges, expected.no_new_privileges);
         assert_eq!(spec.registration, module_spec().registration);
         assert_eq!(spec.placement, PlacementClass::ModuleHosts);
     }
 
+    /// Names which spec rule a read-path rejection must report.
+    type RuleCheck = fn(&ModuleSpecError) -> bool;
+
     #[test]
     fn the_read_path_runs_the_spec_validator_rather_than_trusting_the_producer() {
         // Each of these decodes cleanly and is refused by the validator, so a
         // consumer that never links a producer still refuses it. The case is
         // named by the source variant, not by "some error occurred".
-        /// Names which spec rule a read-path rejection must report.
-        type RuleCheck = fn(&ModuleSpecError) -> bool;
-
         let cases: [(&str, RuleCheck); 4] = [
             (
                 r#""exec_start":[{"var":"artifact-path"},{"var":"main-pid"}]"#,
@@ -1871,6 +1874,44 @@ mod tests {
         }
     }
 
+    #[test]
+    fn the_read_path_refuses_a_unit_declaring_an_unloadable_limit() {
+        // Both limit rules reach a consumer the way every other unit rule
+        // does, through the read path, so a consumer that never links a
+        // producer refuses the manifest rather than rendering a directive that
+        // starves the service of every descriptor or one systemd's rlimit
+        // parser refuses outright.
+        let cases: [(&str, RuleCheck); 2] = [
+            ("0", |source| {
+                matches!(source, ModuleSpecError::ZeroLimitNofile)
+            }),
+            ("18446744073709551615", |source| {
+                matches!(source, ModuleSpecError::InfiniteLimitNofile)
+            }),
+        ];
+        for (limit, is_expected) in cases {
+            let unit = unit_json(VALID_EXEC_START).replace(
+                r#""restart_sec":5,"#,
+                &format!(r#""restart_sec":5,"limit_nofile":{limit},"#),
+            );
+            let entry = entry_json_with_spec("bin/c", Some(GIT_COMMIT), &spec_json(&unit));
+            let json = format!(
+                r#"{{"format_version":{MANIFEST_FORMAT_VERSION},{MEMBERS_JSON}"artifacts":[{entry}]}}"#
+            );
+            let error = PayloadManifest::parse(json.as_bytes(), LEGACY_UNVERSIONED_FOOTER_VERSION)
+                .expect_err("an unloadable limit must be refused on read");
+            let ManifestError::InvalidSpec {
+                ref archive_path,
+                ref source,
+            } = error
+            else {
+                panic!("limit {limit}: expected a spec rejection, got {error:?}");
+            };
+            assert_eq!(archive_path, "bin/c");
+            assert!(is_expected(source), "limit {limit}: got {source:?}");
+        }
+    }
+
     #[test]
     fn the_read_path_enforces_the_kind_conditional_unit_rule() {
         // A `native-binary` artifact declaring a spec with no unit.
@@ -2006,21 +2047,17 @@ mod tests {
     }
 
     #[test]
-    fn the_range_accepts_the_unmoved_floor_as_well_as_the_current_version() {
-        // Every version is written by interpolating a constant, so the next
-        // bump stays a repoint of the three rather than a search for a
-        // literal. A manifest at the floor is what an earlier build published:
-        // it is still accepted, and reports the version it was written at.
-        // Either side of the range is refused for the version alone, which
-        // `a_version_below_the_floor_is_rejected_with_the_same_variant` and
-        // `a_version_above_the_ceiling_is_rejected_for_its_version_alone` each
-        // pin against its own end.
-        for version in [MIN_MANIFEST_FORMAT_VERSION, MANIFEST_FORMAT_VERSION] {
+    fn every_version_in_the_window_is_accepted_including_the_floor() {
+        // The two refusals either side of the window have their own tests; what
+        // this one pins is that the window is not a point. Written by
+        // interpolating the constants, so the next bump stays a repoint of them
+        // rather than a search for a literal.
+        for version in MIN_MANIFEST_FORMAT_VERSION..=MAX_MANIFEST_FORMAT_VERSION {
             let manifest = PayloadManifest::parse(
                 versioned_json(version).as_bytes(),
                 LEGACY_UNVERSIONED_FOOTER_VERSION,
             )
-            .expect("a version inside the accepted range must parse");
+            .expect("a version inside the window must be accepted");
             assert_eq!(manifest.format_version(), Some(version));
         }
     }
@@ -2090,6 +2127,40 @@ mod tests {
         );
     }
 
+    #[test]
+    fn a_manifest_at_the_floor_version_still_decodes_validates_and_renders_without_a_limit() {
+        // The floor stayed put across the `limit_nofile` bump precisely so an
+        // already-published payload keeps working. That promise is about the
+        // whole read path, so this exercises it to the rendered bytes rather
+        // than stopping at the decode.
+        let entry = entry_json_with_spec(
+            "bin/c",
+            Some(GIT_COMMIT),
+            &spec_json(&unit_json(VALID_EXEC_START)),
+        );
+        let json = format!(
+            r#"{{"format_version":{MIN_MANIFEST_FORMAT_VERSION},{MEMBERS_JSON}"artifacts":[{entry}]}}"#
+        );
+        let manifest = PayloadManifest::parse(json.as_bytes(), LEGACY_UNVERSIONED_FOOTER_VERSION)
+            .expect("a manifest at the floor version must still parse");
+        assert_eq!(manifest.format_version(), Some(MIN_MANIFEST_FORMAT_VERSION));
+
+        let spec = manifest
+            .artifacts()
+            .first()
+            .expect("one artifact")
+            .spec
+            .as_ref()
+            .expect("the artifact declares a spec");
+        let unit = spec.unit.as_ref().expect("the spec declares a unit");
+        assert_eq!(unit.limit_nofile, None);
+
+        let rendered = render_unit(spec, &read_path_context(None))
+            .expect("a floor-version spec must render")
+            .expect("the spec declares a unit");
+        assert!(!rendered.text.contains("Limit"), "got: {}", rendered.text);
+    }
+
     #[test]
     fn a_manifest_at_the_current_version_renders_a_spec_naming_the_manager_endpoint() {
         // The mirror of the test above, and the read path the bump was cut