diff --git a/backend/cpp/audio-cpp/audio_io.cpp b/backend/cpp/audio-cpp/audio_io.cpp index b6d1e6c63..709d1864e 100644 --- a/backend/cpp/audio-cpp/audio_io.cpp +++ b/backend/cpp/audio-cpp/audio_io.cpp @@ -97,7 +97,18 @@ void write_audio_file(const std::string &path, audio.channels > 0 ? audio.channels : 1, audio.samples); } catch (const std::exception &err) { - throw ConfigError("audio-cpp: cannot write " + path + ": " + err.what()); + // NOT a ConfigError, and the distinction is not cosmetic. The + // destination is chosen by LocalAI rather than by the caller: it is a + // unique name inside GeneratedContentDir. A failure to write it is a + // full disk, a permission fault on the server's own directory, or a bad + // mount, none of which the caller can fix or is to blame for. As a + // ConfigError this surfaced as INVALID_ARGUMENT, which tells a client + // its request was wrong and not to retry; a plain runtime_error maps to + // INTERNAL, which is both true and retryable. The empty path above + // stays INVALID_ARGUMENT, because that one really is a malformed + // request. + throw std::runtime_error("audio-cpp: cannot write " + path + ": " + + err.what()); } } diff --git a/backend/cpp/audio-cpp/audio_io.h b/backend/cpp/audio-cpp/audio_io.h index 6a882fdb0..be01734b6 100644 --- a/backend/cpp/audio-cpp/audio_io.h +++ b/backend/cpp/audio-cpp/audio_io.h @@ -44,8 +44,14 @@ namespace audiocpp_backend { engine::runtime::AudioBuffer read_audio_file(const std::string &path, int target_sample_rate); -// Writes 16-bit PCM WAV, creating parent directories. Throws ConfigError when -// the destination cannot be written. +// Writes 16-bit PCM WAV, creating parent directories. +// +// Throws ConfigError, i.e. INVALID_ARGUMENT, ONLY for an empty path, which is a +// malformed request. Every other failure throws a plain runtime_error, i.e. +// INTERNAL: the destination is LocalAI's own generated-content directory and +// not anything the caller named, so a full disk or a permission fault there is +// a server fault and is worth retrying, which is the opposite of what +// INVALID_ARGUMENT tells a client. void write_audio_file(const std::string &path, const engine::runtime::AudioBuffer &audio); diff --git a/backend/cpp/audio-cpp/audio_io_ctest.cpp b/backend/cpp/audio-cpp/audio_io_ctest.cpp index f967a65ae..8e6b1e1a6 100644 --- a/backend/cpp/audio-cpp/audio_io_ctest.cpp +++ b/backend/cpp/audio-cpp/audio_io_ctest.cpp @@ -152,6 +152,53 @@ static void test_unreadable_file_is_a_config_error() { check(threw_config_error, "a non-WAV input is INVALID_ARGUMENT, not INTERNAL"); } +// The write side of the same distinction. The destination is LocalAI's own +// generated-content directory, not a caller-supplied path, so a failure to +// write it is a server fault: INTERNAL, which a client may retry, and not +// INVALID_ARGUMENT, which tells it the request itself was wrong. +static void test_write_failure_is_not_a_config_error() { + // A regular file where a directory has to be. ENOTDIR defeats root as well + // as an ordinary user, unlike a chmod, which CI running as root would walk + // straight through. + const auto blocker = scratch_dir() / "blocking-file"; + { + FILE *file = fopen(blocker.string().c_str(), "wb"); + if (file != nullptr) { + fputs("not a directory", file); + fclose(file); + } + } + const auto path = blocker / "nested" / "out.wav"; + + bool threw_config_error = false; + bool threw_something = false; + try { + write_audio_file(path.string(), tone(16000, 1, 0.05f)); + } catch (const ConfigError &) { + threw_config_error = true; + threw_something = true; + } catch (const std::exception &) { + threw_something = true; + } + check(threw_something, "an unwritable destination is reported at all"); + check(!threw_config_error, + "a failed write is INTERNAL, not INVALID_ARGUMENT: the caller did not " + "choose the destination and cannot fix it"); + check(!std::filesystem::exists(path), "and nothing was written"); +} + +static void test_empty_output_path_is_a_config_error() { + // The one write failure that IS the caller's: no path at all. + bool threw_config_error = false; + try { + write_audio_file("", tone(16000, 1, 0.05f)); + } catch (const ConfigError &) { + threw_config_error = true; + } catch (const std::exception &) { + } + check(threw_config_error, "an empty output path stays INVALID_ARGUMENT"); +} + int main() { test_441k_stereo_is_read_as_16k_mono(); test_48k_is_read_as_16k(); @@ -159,6 +206,8 @@ int main() { test_zero_target_keeps_the_native_format(); test_missing_file_is_a_config_error(); test_unreadable_file_is_a_config_error(); + test_write_failure_is_not_a_config_error(); + test_empty_output_path_is_a_config_error(); if (failures) { fprintf(stderr, "%d check(s) failed\n", failures); return 1; diff --git a/backend/cpp/audio-cpp/grpc-server.cpp b/backend/cpp/audio-cpp/grpc-server.cpp index 8daefeeee..63db70f42 100644 --- a/backend/cpp/audio-cpp/grpc-server.cpp +++ b/backend/cpp/audio-cpp/grpc-server.cpp @@ -132,8 +132,14 @@ constexpr int kSpeechSampleRate = 16000; // // Separation forces it. htdemucs and mel_band_roformer refuse anything but // their own rate outright, from prepare(): "HTDemucs prepare() sample rate -// mismatch: expected 44100" (src/models/demucs/session.cpp) and the same shape -// in src/models/roformer/session.cpp. Passing kSpeechSampleRate here would +// mismatch: expected N" (src/models/demucs/session.cpp) and the same shape in +// src/models/roformer/session.cpp. That N is the CHECKPOINT'S declared rate, +// read from the packaged config ("samplerate" in demucs/assets.cpp, +// "sample_rate" in roformer/assets.cpp), not a constant: it is 44100 for every +// published checkpoint of both, which is why the messages below say 44100, but +// a checkpoint declaring something else would demand that instead, and only +// passing the file through unchanged can satisfy either. Passing +// kSpeechSampleRate here would // therefore turn every separation request into an INTERNAL, which is a loud // failure. The downmix half is the quiet one: both models declare two channels // and ACCEPT mono, by duplicating it across both, so a mono read would be taken @@ -147,27 +153,29 @@ constexpr int kSpeechSampleRate = 16000; // // seed_vc (vc, svc) seed_vc_prepare_audio_for_sample_rate takes the // buffer's rate and channel count and resamples to its own mel -// rate with resample_mono_torchaudio_sinc_hann. +// rate: soxr when it is available, falling back to +// resample_mono_torchaudio_sinc_hann (seed_vc/audio_features.cpp). // vevo2 (vc, s2s, svc) normalize_audio_to_24k_mono converts whatever -// it is given to 24 kHz mono before anything else runs. +// it is given to 24 kHz mono before anything else runs, linearly. // miocodec (vc, s2s) prepare_miocodec_mono_audio mixes down, then -// resamples to the model's rate, again with sinc-hann. +// resamples to the model's rate with sinc-hann. // chatterbox (vc) ChatterboxVcComponent::convert normalizes the source to -// 16 kHz and the reference to 24 kHz itself. +// 16 kHz and the reference to 24 kHz itself, linearly. // // So every one of them resamples internally, and passing the file through -// unchanged is not merely tolerated but strictly better: their outputs run at -// 22.05 to 44.1 kHz, and pre-folding the input to 16 kHz mono with -// read_audio_file's LINEAR resampler would band-limit at 8 kHz and alias, then -// hand the family a signal it would upsample again. Two of the four use a -// windowed-sinc resampler, which is exactly the quality this constant would -// throw away before they ever saw the samples. +// unchanged is better than folding it first. The reason is the DOUBLE +// conversion, not resampler quality: two of the four resample linearly, exactly +// as read_audio_file would. Their outputs run at 22.05 to 44.1 kHz, so +// pre-folding to 16 kHz mono with read_audio_file's LINEAR, unfiltered +// resampler would band-limit at 8 kHz and alias, and then the family would +// resample that damaged signal a second time on its way up. One conversion is +// the floor; this constant is what keeps it at one. // // The cost, stated plainly: a family that cannot handle the file's rate reports // it from inside the engine as a plain runtime_error, which to_status maps to // INTERNAL rather than INVALID_ARGUMENT. That is the accepted trade, since the -// only families that refuse are the separators and their requirement is -// 44.1 kHz stereo, i.e. an ordinary music file. +// only families that refuse are the separators and what they want is an +// ordinary music file at its own rate. constexpr int kTransformSampleRate = 0; // Parses ModelOptions.MainGPU into a device index. @@ -801,7 +809,8 @@ public: // behind somebody else's thirty second run, and it must not blame // the caller's paths for a decision that had nothing to do with // them. Same ordering as Diarize and AudioTranscription. - model->check_can_serve(audiocpp_backend::Rpc::AudioTransform, shape); + const audiocpp_backend::Route route = + model->check_can_serve(audiocpp_backend::Rpc::AudioTransform, shape); if (request->audio_path().empty() || request->dst().empty()) { throw audiocpp_backend::ConfigError( @@ -823,6 +832,23 @@ public: task.options[param.first] = param.second; } + // Refused from the ROUTE, before the file reads and before the run. + // Only source separation produces named stems, and the route says + // whether this is separation without running anything: the identical + // refusal below, taken from the empty named_audio_outputs list, + // cannot fire until a full conversion has been paid for (measured at + // 1.6 s on miocodec, far worse on seed_vc or vevo2). The one below + // stays as the backstop for a separation-routed family that returns + // no stems anyway. + if (!requested_stem.empty() && + route.task != audiocpp_backend::Task::SourceSeparation) { + throw audiocpp_backend::ConfigError( + "audio-cpp: family '" + model->family() + "' routes this " + + "request to " + audiocpp_backend::task_name(route.task) + + ", which produces a single output with no named stems, so " + "params[stem]='" + requested_stem + "' cannot be honoured"); + } + // Native rate, native channels, for both files. See // kTransformSampleRate: separation is destroyed by a downmix and // every conversion family resamples internally anyway. @@ -856,6 +882,13 @@ public: // unknown stem name is a refused request, and a refused request // must not leave four files in the caller's output directory // that it then reports nothing about. + // + // The guarantee is exactly that and no more: a request REFUSED + // ON ITS ARGUMENTS writes nothing. A write that FAILS partway + // through the loop below, on a full disk say, still leaves the + // siblings written before it, with no dst. Nothing is rolled + // back, because deleting files after a disk error is its own + // way to lose data, and the caller sees the failure. const auto choice = audiocpp_backend::select_named_output(names, requested_stem); if (!choice.error.empty()) { diff --git a/backend/cpp/audio-cpp/loaded_model.cpp b/backend/cpp/audio-cpp/loaded_model.cpp index bdcb376f9..f8888a055 100644 --- a/backend/cpp/audio-cpp/loaded_model.cpp +++ b/backend/cpp/audio-cpp/loaded_model.cpp @@ -337,11 +337,15 @@ LoadedModel::LoadedModel(const std::string &resolved_path, capabilities_ = to_capabilities(family, engine_caps); } -void LoadedModel::check_can_serve(Rpc rpc, const RequestShape &shape) const { +Route LoadedModel::check_can_serve(Rpc rpc, const RequestShape &shape) const { const Route route = resolve_route(rpc, shape, capabilities_); if (!route.ok) { throw CapabilityError(route.error); } + // Returned so a handler can act on the task before running it. It is the + // same route session_for will resolve, since both read the immutable + // capabilities_ from the same shape. + return route; } LoadedModel::Session LoadedModel::session_for(Rpc rpc, const RequestShape &shape, diff --git a/backend/cpp/audio-cpp/loaded_model.h b/backend/cpp/audio-cpp/loaded_model.h index f4f0a137a..a5e97a3e1 100644 --- a/backend/cpp/audio-cpp/loaded_model.h +++ b/backend/cpp/audio-cpp/loaded_model.h @@ -99,7 +99,15 @@ public: const std::string &pinned_task() const noexcept { return pinned_task_; } // Throws the same CapabilityError session_for would throw when this family - // cannot serve the RPC, and does nothing otherwise. + // cannot serve the RPC, and RETURNS THE RESOLVED ROUTE otherwise. + // + // The route is returned rather than computed and dropped because a handler + // often has to know which task it is about to run BEFORE running it. + // AudioTransform refuses params[stem] on any route but source separation, + // and reading that off the route costs microseconds where reading it off + // the result costs a whole inference first. A caller with no such need + // ignores the value, which is what the three transcription-shaped handlers + // do. // // It exists so a refusal does not have to buy a place in the queue first. // resolve_route is a pure function of capabilities_, which is fixed at @@ -115,7 +123,7 @@ public: // // Const and lane-free on purpose. If a future edit makes routing depend on // mutable state, this must grow the lane parameter its siblings carry. - void check_can_serve(Rpc rpc, const RequestShape &shape) const; + Route check_can_serve(Rpc rpc, const RequestShape &shape) const; // Routes the RPC and returns the cached session, creating it on first use. // Throws CapabilityError when this family cannot serve the RPC, and a plain diff --git a/backend/cpp/audio-cpp/stem_selection.cpp b/backend/cpp/audio-cpp/stem_selection.cpp index 11520b517..da944e6c2 100644 --- a/backend/cpp/audio-cpp/stem_selection.cpp +++ b/backend/cpp/audio-cpp/stem_selection.cpp @@ -34,6 +34,22 @@ bool name_is_writable(const std::string &name) { if (name.empty() || name == "." || name == "..") { return false; } + // Control bytes, NUL above all. GGUF strings are length prefixed and demucs + // reads its source names out of JSON, which can encode one, so a + // std::string holding an embedded NUL survives all the way here. Two such + // names differing only AFTER the NUL are distinct std::strings, so the + // duplicate check below waves them through, and then path::c_str() + // truncates both at the NUL and they open the same file: precisely the + // silent overwrite the duplicate check exists to prevent, with the ".wav" + // stripped off as well. The rest of the range goes with it, since a newline + // or an escape sequence in a file name is a terminal and log injection + // nuisance with no legitimate use. + for (const char byte : name) { + const auto value = static_cast(byte); + if (value < 0x20 || value == 0x7f) { + return false; + } + } // Both separators, not just the host's. A GGUF is a downloaded file and its // strings are not this host's to trust, so a name written on Windows must // not become a directory traversal wherever the check happens to run. diff --git a/backend/cpp/audio-cpp/stem_selection_test.cpp b/backend/cpp/audio-cpp/stem_selection_test.cpp index 6a857d5d8..1316e2aee 100644 --- a/backend/cpp/audio-cpp/stem_selection_test.cpp +++ b/backend/cpp/audio-cpp/stem_selection_test.cpp @@ -127,6 +127,43 @@ static void test_unwritable_names_are_refused() { check(late.index == -1 && !late.error.empty(), "an unwritable name after the selected one still refuses the whole request"); + // Control bytes, and the NUL case is why the whole range is refused. These + // two names are DIFFERENT std::strings, so the duplicate check does not + // fire, yet both truncate to "vocals" at path::c_str() and would open one + // file: the silent overwrite the duplicate check exists to prevent, with + // the ".wav" stripped off into the bargain. + const std::string nul_a("vocals\0drums", 12); + const std::string nul_b("vocals\0bass", 11); + check(nul_a != nul_b, "the two NUL names really are distinct std::strings"); + check(std::string(nul_a.c_str()) == "vocals" && + std::string(nul_b.c_str()) == "vocals", + "and both truncate to the same C string, which is the hazard"); + const auto nul_pair = select_named_output({nul_a, nul_b}, ""); + check(nul_pair.index == -1 && !nul_pair.error.empty(), + "two stem names differing only after an embedded NUL are refused"); + check(!select_named_output({"drums", std::string("vo\0cals", 7)}, "").error.empty(), + "a single embedded NUL is refused on its own"); + check(!select_named_output({"drums", "voc\nals"}, "").error.empty(), + "a newline in a stem name is refused"); + check(!select_named_output({"drums", "voc\tals"}, "").error.empty(), + "a tab in a stem name is refused"); + check(!select_named_output({"drums", "voc\033[31mals"}, "").error.empty(), + "an escape sequence in a stem name is refused"); + check(!select_named_output({"drums", "voc\177als"}, "").error.empty(), + "DEL in a stem name is refused"); + + // The boundary below the refused range is the space, which is an ordinary + // file name character and must stay usable, or this check would be + // refusing real stem names. + const auto spaced = select_named_output({"lead vocals", "drums"}, "lead vocals"); + check(spaced.index == 0 && spaced.error.empty(), + "a space is not a control character and stays usable"); + // And every byte above DEL: a UTF-8 stem name is ordinary, and signed char + // would make those bytes compare as negative. + const auto utf8 = select_named_output({"vocals", "b\xc3\xa4sse"}, "b\xc3\xa4sse"); + check(utf8.index == 1 && utf8.error.empty(), + "a UTF-8 stem name is not mistaken for a control character"); + // A leading dot is not a traversal and must stay usable. const auto dotted = select_named_output({".vocals", "drums"}, ".vocals"); check(dotted.index == 0 && dotted.error.empty(),