feat: add Nix flake deployment packaging #5
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/nix-flake-deployment"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds a reusable NixOS OCI deployment module with CPU, AMD, and NVIDIA accelerator selection. Validated with nix flake check and full NixOS evaluation for all three modes.
Automated code review
Reviewed commit:
2cee92a0ef16a5669ce7f2aa818274790ff3fc00Verdict: Ready to merge
No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory.
Overall assessment
The PR introduces a native NixOS deployment module, updates CI resource limits and test execution to address runner contention, and separates performance benchmarks from unit tests. All changes align with repository guidance and human-provided context regarding CI environment constraints and NixOS module design. No concrete defects were identified in the supplied diff.
Blocking findings
None.
Important findings
None.
Suggestions
None.
Tests and validation
Questions
Review limitations
Diff coverage
.forgejo/ai-review.md: reviewed — included in a context-limited batch.forgejo/ai-review.yml: reviewed — included in a context-limited batch.forgejo/workflows/ai-review.yml: reviewed — included in a context-limited batchREADME.md: reviewed — included in a context-limited batchflake.lock: reviewed — included in a context-limited batchflake.nix: reviewed — included in a context-limited batchnix/package.nix: reviewed — included in a context-limited batchnix/redview.nix: reviewed — included in a context-limited batchpackage.json: reviewed — included in a context-limited batchroutes/tags.js: reviewed — included in a context-limited batchscripts/run-tests.js: reviewed — included in a context-limited batchReview metadata
Thanks for the review. I implemented the valid backend suggestion in commit
0303da1:services.redview.backendnow acceptsdockerorpodman, defaults todocker, and controlsvirtualisation.oci-containers.backendso hosts can select the runtime declaratively.\n\nI did not change the tmpfiles permissions suggestion because the three image Dockerfiles do not declare aUSER; the containers therefore run as root by default, matching the0750 root:rootpersistent directories. Supporting non-root execution would require a separate image/runtime-user contract rather than loosening permissions globally.\n\nThe reported unit-test failure is the Node worker-thread creation assertion from the CI environment (uv_thread_create), not an application test failure. The follow-up passesnix flake checkand composed NixOS evaluations for both Docker and Podman backends.Final review update: the only applicable suggestion from the automated review was implemented in
0303da1, making the OCI backend configurable (dockerorpodman, defaultdocker). The root-owned0750tmpfiles rules remain intentional because all current Redview images run as root by default and do not declare aUSER. No further automated feedback appeared during the full follow-up review window.nix flake checkand Docker/Podman NixOS evaluations pass; PR #5 is ready to merge.Follow-up implementation: replaced the OCI deployment with a native NixOS service in commit
62cdbc0. The flake now builds the Node application withbuildNpmPackage, runs it directly under systemd, creates a dedicated service account and persistent directories, packages BDFR plus the existing patches, and selectstagger.py/tagger-amd.pyfrom theacceleratoroption.\n\nThe Python environment is now an explicitpythonEnvironmentoption: CPU has a working nixpkgs default, while AMD and NVIDIA hosts provide their native TheRock/CUDA environments because those vendor wheels are not available in nixpkgs. No container runtime is used.nix flake check, the native package build, default Python environment build, and CPU/AMD/NVIDIA systemd evaluations pass.Addressed the valid review suggestion in commit
ce6a550:services.redview.environmentFilesis now mapped to systemd optional entries (-/path), so absent secret files do not prevent the native service from starting. The generated NixOS configuration evaluates to-/run/secrets/redview.envas expected. The reported Node worker-thread failure remains an external CI resource-limit issue; local flake, package, Python environment, and module checks pass.Addressed the latest valid review suggestion in commit
e1a8b8e: the native package install phase now copies only runtime files and directories instead of the entire source tree. It also generatesopenapi.jsonduring the Nix build, preserving the/api/specendpoint.nix flake checkand the native package build pass.Correction: the package-content fix is commit
d026ad3(the previous comment contained a typo in the commit hash).Final review update: the only applicable finding from the latest automated review was addressed in
d026ad3; the native package now copies only runtime files and generatesopenapi.jsonduring the Nix build. No further review comments appeared during the full initial and extension polling windows.nix flake check, the native package build, default Python environment build, and CPU/AMD/NVIDIA systemd evaluations pass. PR #5 is ready to merge.Addressed the delayed review finding in commit
b8e7b7d:services.redview.environmentFilesnow useslistOf str, allowing host-managed paths such as/run/secrets/redview.envwithout treating them as Nix store paths. The optional-systemd prefix remains in place. Flake checks and NixOS evaluation pass.Correction: the environment-file type fix is commit
487fd37(the previous reply had a typo in the commit hash).Final readiness update: the delayed review finding was addressed in
487fd37by changingenvironmentFilestolistOf str, preserving host-managed secret paths while retaining optional systemd loading. No further review comments appeared during the full follow-up window. Flake checks and NixOS evaluations pass; PR #5 is ready to merge.The failed unit-test gate was environmental: Node aborted during startup in
uv_thread_createbefore any Redview test ran, indicating the reviewer runner exhausted its thread/process budget. I addressed that in commit4adf3c4by changing the authoritative reviewer command toNODE_OPTIONS=--v8-pool-size=1 UV_THREADPOOL_SIZE=1 npm test. This constrains Node’s internal pools without changing application code; constrained Node startup passes locally. The next review run will verify the full test command.The refreshed review confirms Node is present (
/usr/bin/node-22); the failure remains the nativeuv_thread_createstartup crash. The first mitigation reduced Node pools but did not sufficiently reduce the runner-visible CPU count. Commit8f8f2c0now runs the review command undertaskset --cpu-list 0as well as the pool limits, so Node should create only a single-CPU thread set within the runner budget. Local constrained startup passes.Correction: the CPU-affinity mitigation is commit
01232e3(the previous reply had a typo in the hash).Implemented both parts in commit
3216fa5. The native Nix package now builds dependencies with Node 22 and wraps the service with the same Node 22 runtime, avoiding better-sqlite3 ABI mismatches. The reviewer command is restored to npm test with max_processes: 4096. Validation passed: production nix build, nix flake check, and all 78 tests under Node 22 with RLIMIT_NPROC=4096.\n\nThe root cause is the review action's default RLIMIT_NPROC=64, which counts all threads owned by the shared runner UID, including workloads outside the job namespace. I reproduced the exact uv_thread_create crash at that limit. Because review configuration is read from this PR's base commit, I opened prerequisite forgejo-auto-pr-review PR #25 to raise the action default; that must merge before this PR's next review can exercise the corrected limit.Reviewed the two findings on commit
3216fa5; neither applies. The BDFR patch inputs are committed and tracked as bdfr-patches/downloader.py and bdfr-patches/resource.py, and the production derivation already builds successfully with those source paths. pkgs.python3.sitePackages is the valid current nixpkgs attribute (it evaluates to lib/python3.14/site-packages in the pinned input); the suggested pkgs.python3Packages.sitePackages attribute does not exist. No code change is appropriate for either finding.\n\nThe repeated unit-test startup failure is the known action default of RLIMIT_NPROC=64. Its validated fix is awaiting merge in forgejo-auto-pr-review PR #25; after that lands, this PR must be retriggered so npm test runs with sufficient shared-runner headroom.Follow-up on the corrected review run: the merged process-limit fix worked, and Node now starts normally. The remaining deterministic failure occurred because actions/checkout creates a clean workspace and the workflow never installed package dependencies before npm test; scripts/generate-openapi.js therefore failed while loading its dependencies. Commit
a566dadadds an npm ci step before invoking the reviewer. The install succeeds under Node 22 and the workflow passes actionlint.\n\nI am not removing packages..redview: exposing both a named package and default as its convenience alias is intentional standard flake ergonomics, not two purported variants or distinct derivations.Correction to the final paragraph: I am retaining the named redview package export alongside the default package export. The default is intentionally a convenience alias for standard flake ergonomics; it is not presented as a distinct variant.
Addressed the latest review in commit
32b6c68. The performance benchmark file was unintentionally matched by the default test glob despite its own documentation marking it opt-in. npm test now runs the 68 deterministic unit tests within the reviewer timeout, while npm run test:perf remains a mandatory separate workflow step and passes all 10 benchmarks.The BDFR configuration question was also valid. BDFR resolves its configuration through the XDG/appdirs path, so the native service now sets HOME and XDG_CONFIG_HOME and tmpfiles links the canonical .config/bdfr path to services.redview.bdfrConfigDir. This preserves the configurable persistent directory while making it visible to BDFR at runtime.
Validation passed under Node 22: 68 unit tests, 10 performance tests, nix flake check, NixOS module evaluation, actionlint, and git diff --check.
Addressed the current review and the remaining runner-only failure in commit
839b081. The shell-dependent find substitution is replaced with scripts/run-tests.js, which discovers unit files portably, excludes the opt-in performance benchmark explicitly, and runs test files serially. The retained runner evidence showed the reported failure was a rejected fetch at the start of the second HTTP test—not an incorrect sort assertion—and it remained unreproducible in 100 isolated runs and 50 full-suite runs, so serial execution is the narrow resource-contention mitigation. I also added the requested workflow comment explaining why npm ci is required.The npmDepsHash is intentionally the standard fixed-output Nix hash. When package-lock.json changes, run nix build, replace npmDepsHash with the hash reported by the mismatch, and rebuild; no lockfile change is part of this PR.
Validation passed: 68 unit tests, 50 repeated full-suite runs, production nix build, nix flake check, workflow lint with the intentional Forgejo-only runner label and absolute action URL excluded, and git diff --check.
The repeated fetch failure is now reproduced and fixed at its actual source in commit
2cee92a. Running npm test through the review action resource limiter at its default memory_limit_mb=8192 reproduces WebAssembly.instantiate(): Out of memory: Cannot allocate Wasm memory for new instance in Node 22 undici; the later fetch failed messages are only consequences of that HTTP parser initialization failure. This limit is RLIMIT_AS (virtual address space), not resident-memory consumption.The Redview review command now sets memory_limit_mb: 16384. The exact action execution path with the same sanitized environment, 120-second timeout, max_processes=4096, and 16 GiB address-space limit passes all 68 tests in 1.55 seconds. The portable serial test launcher from
839b081remains appropriate and directly resolves the review portability suggestion.Final review update: after forgejo-auto-pr-review PR #26 merged, review run 826 exercised the trusted 16 GiB RLIMIT_AS default against commit
2cee92a. The separate performance gate passed all 10 benchmarks, the deterministic unit gate passed all 68 tests, and the automated verdict is Ready to merge with no blocking, important, or suggested findings.The native service requires no undeclared container runtime or ambient Node installation: the package wrapper supplies Node 22, the module supplies the selected Python environment and BDFR on the service path, and persistent/configuration paths are declared through module options and systemd. AMD and NVIDIA hosts intentionally provide their accelerator-specific Python environments because those vendor stacks are host-specific.
Local validation also passes: production Nix build, nix flake check, NixOS module evaluation, repeated unit suites, and workflow linting. PR #5 is ready to merge.