diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c26c441..b801e10 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -93,6 +93,8 @@ jobs: runs-on: [self-hosted, node-b, linux, x64, publish, docker] env: GIT_AUTH_TOKEN: ${{ secrets.FORGEJO_CARGO_TOKEN }} + DOCKERHUB_TOKEN: ${{ secrets.DOCKERHUB_TOKEN }} + DOCKERHUB_USERNAME: ${{ secrets.DOCKERHUB_USERNAME }} IMAGE: rustnzb:ci-${{ github.sha }} RUSTNZB_BUILD_REF: ${{ github.sha }} steps: diff --git a/.github/workflows/container-canary.yml b/.github/workflows/container-canary.yml index 24be5a3..c398bbb 100644 --- a/.github/workflows/container-canary.yml +++ b/.github/workflows/container-canary.yml @@ -32,6 +32,12 @@ jobs: registry: ghcr.io username: ${{ github.actor }} password: ${{ secrets.GHCR_TOKEN }} + - name: Authenticate to Docker Hub (higher pull rate limit for base images) + if: ${{ secrets.DOCKERHUB_TOKEN != '' }} + uses: docker/login-action@v4 + with: + username: ${{ secrets.DOCKERHUB_USERNAME }} + password: ${{ secrets.DOCKERHUB_TOKEN }} - name: Build and publish immutable amd64 canary uses: docker/build-push-action@v7 with: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 72c1533..96805b8 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -47,6 +47,12 @@ jobs: registry: ghcr.io username: ${{ github.actor }} password: ${{ secrets.GHCR_TOKEN }} + - name: Authenticate to Docker Hub (higher pull rate limit for base images) + if: ${{ secrets.DOCKERHUB_TOKEN != '' }} + uses: docker/login-action@v4 + with: + username: ${{ secrets.DOCKERHUB_USERNAME }} + password: ${{ secrets.DOCKERHUB_TOKEN }} - name: Build and publish multi-architecture release image once uses: docker/build-push-action@v7 with: diff --git a/Cargo.lock b/Cargo.lock index 453c822..b37c37b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2932,6 +2932,7 @@ dependencies = [ "rustls", "serde", "serde_json", + "serial_test", "tempfile", "tokio", "tokio-util", diff --git a/README.md b/README.md index cc52dc6..a82ed84 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,13 @@ full reference. | `OTEL_EXPORTER_OTLP_ENDPOINT` | OTLP gRPC endpoint | | `OTEL_SERVICE_NAME` | Service name for telemetry | +> **Docker note:** setting `RUSTNZB_PORT` changes the port the app listens +> on *inside* the container (default `9090`). The image's `EXPOSE 9090` is +> static Dockerfile metadata and won't follow it, so remap the host port to +> match your chosen value yourself: `-p :$RUSTNZB_PORT` (don't +> rely on `-P`/auto-mapping, which only publishes the declared `EXPOSE` +> port). + ### Docker volumes | Path | Purpose | diff --git a/apps/rustnzb/Cargo.toml b/apps/rustnzb/Cargo.toml index 58b1870..0561383 100644 --- a/apps/rustnzb/Cargo.toml +++ b/apps/rustnzb/Cargo.toml @@ -83,3 +83,4 @@ arc-swap.workspace = true nzb-nntp = { workspace = true, features = ["test-support"] } crc32fast = { workspace = true } mock-nntp-server = { workspace = true } +serial_test = "3" diff --git a/apps/rustnzb/root/etc/s6-overlay/s6-rc.d/svc-rustnzb/run b/apps/rustnzb/root/etc/s6-overlay/s6-rc.d/svc-rustnzb/run index fe2950e..354cd23 100755 --- a/apps/rustnzb/root/etc/s6-overlay/s6-rc.d/svc-rustnzb/run +++ b/apps/rustnzb/root/etc/s6-overlay/s6-rc.d/svc-rustnzb/run @@ -1,6 +1,11 @@ #!/usr/bin/with-contenv sh +# No --port here: clap gives an explicit CLI flag precedence over the +# RUSTNZB_PORT env var, so hardcoding one here would make RUSTNZB_PORT (and +# config.toml's general.port) silently unconfigurable in the image. Falls +# back to 9090 (matching the Dockerfile's default EXPOSE) when neither is +# set. If you override RUSTNZB_PORT, remember to remap the host port to +# match (`-p host:$RUSTNZB_PORT`) since the image's EXPOSE stays 9090. exec s6-setuidgid abc rustnzb \ --config /config/config.toml \ - --data-dir /data \ - --port 9090 + --data-dir /data diff --git a/apps/rustnzb/src/main.rs b/apps/rustnzb/src/main.rs index 4518d2d..2dc037e 100644 --- a/apps/rustnzb/src/main.rs +++ b/apps/rustnzb/src/main.rs @@ -419,3 +419,54 @@ async fn main() -> anyhow::Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::Args; + use clap::Parser; + use serial_test::serial; + + // These tests mutate the real process environment, so they must run + // serialized against each other (see issue #62 follow-up: the Docker + // image used to hardcode `--port 9090`, which silently defeated + // RUSTNZB_PORT — this locks in the precedence the fix relies on). + + #[test] + #[serial(rustnzb_port_env)] + fn port_env_var_is_used_when_no_explicit_flag_is_given() { + unsafe { + std::env::set_var("RUSTNZB_PORT", "8123"); + } + let args = Args::parse_from(["rustnzb"]); + unsafe { + std::env::remove_var("RUSTNZB_PORT"); + } + + assert_eq!(args.port, Some(8123)); + } + + #[test] + #[serial(rustnzb_port_env)] + fn explicit_port_flag_still_wins_over_env_var() { + unsafe { + std::env::set_var("RUSTNZB_PORT", "8123"); + } + let args = Args::parse_from(["rustnzb", "--port", "1234"]); + unsafe { + std::env::remove_var("RUSTNZB_PORT"); + } + + assert_eq!(args.port, Some(1234)); + } + + #[test] + #[serial(rustnzb_port_env)] + fn port_is_none_when_neither_flag_nor_env_var_is_set() { + unsafe { + std::env::remove_var("RUSTNZB_PORT"); + } + let args = Args::parse_from(["rustnzb"]); + + assert_eq!(args.port, None); + } +} diff --git a/apps/rustnzb/src/server.rs b/apps/rustnzb/src/server.rs index 988b67f..ee32bd2 100644 --- a/apps/rustnzb/src/server.rs +++ b/apps/rustnzb/src/server.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use anyhow::Context; use axum::Router; use axum::extract::DefaultBodyLimit; use axum::middleware::Next; @@ -396,7 +397,9 @@ pub async fn run(state: Arc) -> anyhow::Result<()> { pub async fn serve(state: Arc, router: Router) -> anyhow::Result<()> { let config = state.config(); let addr = format!("{}:{}", config.general.listen_addr, config.general.port); - let listener = TcpListener::bind(&addr).await?; + let listener = TcpListener::bind(&addr) + .await + .with_context(|| format!("Failed to bind HTTP server to {addr}"))?; info!("HTTP server listening on http://{addr}"); info!("Web GUI: http://{addr}/"); diff --git a/apps/rustnzb/tests/server_bind_error_test.rs b/apps/rustnzb/tests/server_bind_error_test.rs new file mode 100644 index 0000000..6d261f0 --- /dev/null +++ b/apps/rustnzb/tests/server_bind_error_test.rs @@ -0,0 +1,119 @@ +//! Regression tests for issue #62: startup failures must surface actionable +//! context (the failing address/path) instead of a bare `os error 13`. + +use std::path::PathBuf; +use std::sync::Arc; + +use arc_swap::ArcSwap; +use nzb_web::auth::{CredentialStore, TokenStore}; +use nzb_web::nzb_core::config::AppConfig; +use nzb_web::nzb_core::db::Database; +use nzb_web::{AppState, QueueManager}; +use rustnzb::server::build_router; + +async fn build_state(config: AppConfig) -> Arc { + let db = Database::open_memory().expect("Failed to create in-memory database"); + let tmp_dir = tempfile::tempdir().expect("Failed to create temp dir"); + let incomplete_dir = tmp_dir.path().join("incomplete"); + let complete_dir = tmp_dir.path().join("complete"); + std::fs::create_dir_all(&incomplete_dir).expect("Failed to create incomplete dir"); + std::fs::create_dir_all(&complete_dir).expect("Failed to create complete dir"); + + let log_buffer = nzb_web::LogBuffer::new(); + let qm = QueueManager::new( + config.servers.clone(), + db, + incomplete_dir, + complete_dir, + log_buffer.clone(), + config.general.max_active_downloads, + config.categories.clone(), + config.general.min_free_space_bytes, + config.general.speed_limit_bps, + false, + config.general.max_nested_archive_depth, + config.general.abort_hopeless, + config.general.early_failure_check, + config.general.required_completion_pct, + config.general.article_timeout_secs, + ); + let token_store = Arc::new(TokenStore::new()); + let credential_store = Arc::new(CredentialStore::new(tmp_dir.path().to_path_buf())); + // Leak the tempdir so it outlives the returned state for the duration of the test. + std::mem::forget(tmp_dir); + + Arc::new(AppState::new( + Arc::new(ArcSwap::from_pointee(config)), + PathBuf::from("config.toml"), + qm, + log_buffer, + token_store, + credential_store, + )) +} + +/// Negative: binding to an address already occupied by another listener must +/// fail with an error whose message identifies the address, not a bare +/// `os error` string (see apps/rustnzb/src/server.rs `serve`). +#[tokio::test] +async fn serve_reports_context_on_bind_failure() { + // Occupy a free port first, keeping the listener alive for the test. + let occupied = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("failed to occupy a port for the test"); + let addr = occupied.local_addr().expect("failed to read local addr"); + + let mut config = AppConfig::default(); + config.general.listen_addr = addr.ip().to_string(); + config.general.port = addr.port(); + + let state = build_state(config).await; + let router = build_router(state.clone()); + #[cfg(feature = "webdav")] + let router = router.layer(axum::Extension(None::>)); + + let result = rustnzb::server::serve(state, router).await; + let err = match result { + Ok(()) => panic!( + "expected serve() to fail: port {} is already bound", + addr.port() + ), + Err(e) => e, + }; + + let debug_text = format!("{err:?}"); + assert!( + debug_text.contains(&addr.port().to_string()), + "error should mention the failing bind address, got: {debug_text}" + ); + assert!( + debug_text.to_lowercase().contains("bind"), + "error should mention it was a bind failure, got: {debug_text}" + ); + + drop(occupied); +} + +/// Positive: binding to a free port succeeds, and the server can be reached. +#[tokio::test] +async fn serve_succeeds_on_free_port() { + let mut config = AppConfig::default(); + config.general.listen_addr = "127.0.0.1".to_string(); + config.general.port = 0; // OS-assigned free port + + let state = build_state(config).await; + let router = build_router(state.clone()); + #[cfg(feature = "webdav")] + let router = router.layer(axum::Extension(None::>)); + + let handle = tokio::spawn(async move { rustnzb::server::serve(state, router).await }); + + // Give the server a moment to either bind successfully or fail fast. + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + assert!( + !handle.is_finished(), + "serve() should still be running (bind succeeded)" + ); + + handle.abort(); +} diff --git a/ci/tasks/build-image b/ci/tasks/build-image index 6af692d..fef3e8e 100755 --- a/ci/tasks/build-image +++ b/ci/tasks/build-image @@ -19,12 +19,19 @@ fi cleanup() { docker buildx rm "$builder" >/dev/null 2>&1 || true rm -rf "$docker_config" - unset GIT_AUTH_TOKEN + unset GIT_AUTH_TOKEN DOCKERHUB_TOKEN DOCKERHUB_USERNAME } trap cleanup EXIT export DOCKER_CONFIG=$docker_config printf '%s' "$GIT_AUTH_TOKEN" | docker login repo.indexarr.net \ --username x-access-token --password-stdin >/dev/null +# Authenticated Docker Hub pulls get a much higher rate limit than +# anonymous, IP-based ones (which the shared self-hosted runner exhausts +# easily). Optional: falls back to anonymous pulls if unset. +if [ -n "${DOCKERHUB_TOKEN:-}" ] && [ -n "${DOCKERHUB_USERNAME:-}" ]; then + printf '%s' "$DOCKERHUB_TOKEN" | docker login docker.io \ + --username "$DOCKERHUB_USERNAME" --password-stdin >/dev/null +fi docker buildx create --name "$builder" --driver docker-container --use >/dev/null set -- \ diff --git a/crates/nzb-web/src/startup.rs b/crates/nzb-web/src/startup.rs index 4d14afc..9947640 100644 --- a/crates/nzb-web/src/startup.rs +++ b/crates/nzb-web/src/startup.rs @@ -1,6 +1,7 @@ -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; +use anyhow::Context; use arc_swap::ArcSwap; use tracing::info; @@ -45,6 +46,20 @@ fn env_flag_enabled(name: &str) -> Option { }) } +/// Create a data directory (e.g. `data_dir`/`incomplete_dir`/`complete_dir`), attaching +/// the failing path and a permission hint to any error so failures are actionable +/// instead of a bare `Permission denied (os error 13)`. +fn create_data_dir(path: &Path) -> anyhow::Result<()> { + std::fs::create_dir_all(path).with_context(|| { + format!( + "Failed to create directory {}. \ + Check that the directory (and its parent) is writable by the current user. \ + If using Docker, ensure the volume is owned by the container's user.", + path.display() + ) + }) +} + /// Configuration for engine initialization. /// /// All fields except `config_path` are optional overrides — @@ -121,9 +136,9 @@ pub async fn initialize( } // Ensure directories exist - std::fs::create_dir_all(&config.general.data_dir)?; - std::fs::create_dir_all(&config.general.incomplete_dir)?; - std::fs::create_dir_all(&config.general.complete_dir)?; + create_data_dir(&config.general.data_dir)?; + create_data_dir(&config.general.incomplete_dir)?; + create_data_dir(&config.general.complete_dir)?; // Open database let db_path = config.general.data_dir.join("rustnzb.db"); @@ -223,10 +238,54 @@ pub async fn initialize( #[cfg(test)] mod tests { - use super::sanitize_loaded_config; + use super::{create_data_dir, sanitize_loaded_config}; use crate::nzb_core::config::AppConfig; use crate::nzb_core::config::ServerConfig; + #[test] + fn create_data_dir_creates_nested_directories() { + let tmp = tempfile::tempdir().unwrap(); + let nested = tmp.path().join("a").join("b").join("c"); + + create_data_dir(&nested).expect("nested directory creation should succeed"); + + assert!(nested.is_dir()); + } + + #[cfg(unix)] + #[test] + fn create_data_dir_wraps_permission_denied_with_context() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + let locked_parent = tmp.path().join("locked"); + std::fs::create_dir_all(&locked_parent).unwrap(); + std::fs::set_permissions(&locked_parent, std::fs::Permissions::from_mode(0o000)).unwrap(); + + let target = locked_parent.join("data"); + let result = create_data_dir(&target); + + // Restore permissions so the tempdir can be cleaned up. + std::fs::set_permissions(&locked_parent, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let err = match result { + Err(e) => e, + // Running as root (e.g. CI containers) bypasses the permission + // check entirely, so there's nothing to assert. + Ok(()) => return, + }; + let debug_text = format!("{err:?}"); + + assert!( + debug_text.contains(&target.display().to_string()), + "error should mention the failing path, got: {debug_text}" + ); + assert!( + debug_text.contains("Caused by"), + "error should retain the underlying io::Error in the chain, got: {debug_text}" + ); + } + #[test] fn sanitize_loaded_config_trims_server_fields() { let mut config = AppConfig::default(); diff --git a/crates/nzb-web/tests/startup_directory_errors.rs b/crates/nzb-web/tests/startup_directory_errors.rs new file mode 100644 index 0000000..00b2274 --- /dev/null +++ b/crates/nzb-web/tests/startup_directory_errors.rs @@ -0,0 +1,93 @@ +//! Regression tests for issue #62: rustnzb crashed on startup with a bare +//! `Permission denied (os error 13)` and no indication of which directory or +//! path caused it. `startup::initialize` must now attach the failing path +//! (and a permission hint) to any directory-creation error. + +use nzb_web::{StartupConfig, startup}; + +/// Positive: a custom data/incomplete/complete dir set (not matching the +/// three hardcoded defaults baked into the Docker image's init script) +/// succeeds as long as the parent is writable. +#[tokio::test] +async fn initialize_creates_custom_directories() { + let tmp = tempfile::tempdir().unwrap(); + let config_path = tmp.path().join("config.toml"); + let data_dir = tmp.path().join("custom-data"); + let incomplete_dir = tmp.path().join("custom-incomplete"); + let complete_dir = tmp.path().join("custom-complete"); + + let mut startup_cfg = StartupConfig { + config_path, + listen_addr: None, + port: None, + data_dir: Some(data_dir.clone()), + log_level: None, + }; + + // `initialize` only overrides data_dir via StartupConfig; incomplete/complete + // come from the config file, so write one first with our custom paths. + let mut config = nzb_web::nzb_core::config::AppConfig::default(); + config.general.data_dir = data_dir.clone(); + config.general.incomplete_dir = incomplete_dir.clone(); + config.general.complete_dir = complete_dir.clone(); + config.save(&startup_cfg.config_path).unwrap(); + startup_cfg.data_dir = None; // already set in the saved config + + let result = startup::initialize(startup_cfg, None).await; + assert!( + result.is_ok(), + "expected initialize to succeed, got: {:?}", + result.err().map(|e| format!("{e:?}")) + ); + + assert!(data_dir.is_dir()); + assert!(incomplete_dir.is_dir()); + assert!(complete_dir.is_dir()); +} + +/// Negative: a data_dir under a non-writable parent must fail with an error +/// that names the failing path — not a bare `os error 13`. +#[cfg(unix)] +#[tokio::test] +async fn initialize_reports_context_on_unwritable_data_dir() { + use std::os::unix::fs::PermissionsExt; + + let tmp = tempfile::tempdir().unwrap(); + let locked_parent = tmp.path().join("locked"); + std::fs::create_dir_all(&locked_parent).unwrap(); + std::fs::set_permissions(&locked_parent, std::fs::Permissions::from_mode(0o000)).unwrap(); + + let config_path = tmp.path().join("config.toml"); + let data_dir = locked_parent.join("data"); + + let startup_cfg = StartupConfig { + config_path, + listen_addr: None, + port: None, + data_dir: Some(data_dir.clone()), + log_level: None, + }; + + let result = startup::initialize(startup_cfg, None).await; + + // Restore permissions so the tempdir can be cleaned up. + std::fs::set_permissions(&locked_parent, std::fs::Permissions::from_mode(0o755)).unwrap(); + + let err = match result { + Err(e) => e, + // Running as root (e.g. CI containers) bypasses the permission + // check entirely, so there's nothing to assert. + Ok(_) => return, + }; + + let debug_text = format!("{err:?}"); + assert_ne!( + debug_text.trim(), + "Permission denied (os error 13)", + "regression: error must not be the bare os-error text from issue #62, got: {debug_text}" + ); + assert!( + debug_text.contains(&data_dir.display().to_string()), + "error should mention the failing path, got: {debug_text}" + ); +}