spetz commented on code in PR #4318:
URL: https://github.com/apache/iggy/pull/4318#discussion_r4138304848
##########
foreign/swift/Tests/IggyTests/Fixtures/golden.json:
##########
@@ -982,7 +983,7 @@
"response.raw_personal_access_token":
"167261772d7365637265742d746f6b656e2d76616c7565",
"response.send_messages":
"0200000001000000020000000300000004000000000000000100000002000000000000002a00000000000000",
"response.send_messages.empty": "00000000",
- "response.stats":
"d20400000000cc4100004842000000400000000000000000020000000000000001000000100e00000000000000e090663c13060040420f000000000020a107000000000080841e0000000000030000000a0000001e0000005a00000050c30000000000000500000002000000060000006e6f64652d31050000004c696e757803000000362e3105000000362e312e3006000000302e31312e30002c000001000000010000000100000000000000e80300000000000032000000000000003dcf733f1000000000000000190000000060253c77000000",
+ "response.stats":
"d20400000000cc4100004842000000400000000000000000020000000000000001000000100e00000000000000e090663c13060040420f000000000020a107000000000080841e0000000000030000000a0000001e0000005a00000050c30000000000000500000002000000060000006e6f64652d31050000004c696e757803000000362e3105000000362e312e3006000000302e31312e30002c000001000000010000000100000000000000e80300000000000032000000000000003dcf733f1000000000000000190000000060253c7700000080000000000000000028000000000000",
Review Comment:
`response.stats` gained the 16-byte tail here, but `Stats` in
`foreign/swift/Sources/Iggy/Models/Resources.swift:448` has no `openFilesCount`
/ `openFilesLimit`, and no Swift test consumes this vector. The PR body says
the CLI and all SDKs show the two fields. Either add them to the Swift model
now, while the vector is being regenerated, or drop Swift from that claim.
##########
core/server_common/src/fatal.rs:
##########
@@ -0,0 +1,193 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Stopping the process on an environmental failure that has no in-process
answer.
+
+use nix::errno::Errno;
+use nix::sys::resource::{Resource, getrlimit};
+use std::io::{self, Write};
+use std::sync::atomic::{AtomicBool, Ordering};
+
+/// Why the process is stopping. The discriminant is the exit status, one per
+/// condition; `1` stays the binary's generic startup failure.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[repr(u8)]
+pub enum FatalReason {
+ /// A prepare's WAL append failed and the op it had claimed could not be
handed
+ /// back. The durable log is intact up to the previous op, so recovery
re-derives
+ /// the frontier and restarting is the repair.
+ UnreconcilableLogFrontier = 2,
+ /// The superblock stayed unwritable past the configured fail-stop window.
+ /// The replica was already fenced quorum-invisible, so exiting hands the
+ /// wedge to a supervisor instead of a log reader.
+ SuperblockWedged = 3,
+ /// The server stopped on an error after a storage open for a write or a
+ /// sync failed because the process (`EMFILE`) or the host (`ENFILE`) had
no
+ /// free file descriptor. The exit comes after the ordinary shutdown, so
the
+ /// shards flushed what they could. See [`NoteDescriptorExhaustion`].
+ DescriptorsExhausted = 4,
+}
+
+impl FatalReason {
+ #[must_use]
+ pub const fn exit_status(self) -> u8 {
+ self as u8
+ }
+}
+
+/// The target that 0.9.0 shipped for the fatal log line, so existing log
+/// filters keep matching.
+const FATAL_LOG_TARGET: &str = "iggy.consensus.diag";
+
+/// Log `message` and terminate the process.
+///
+/// For an environmental failure where stopping IS the answer, rather than an
error
+/// threaded up a stack whose top knows less than this leaf does. Not for bugs
in
+/// this process, which are `assert!` / `panic!` and say so.
+///
+/// `exit`, not `panic!`: a panic unwinds one shard of a thread-per-core
runtime and
+/// leaves its siblings serving, which is the half-alive state this exists to
avoid.
+/// Skipping destructors is wanted here, since the reason for stopping is that
+/// further writes cannot be trusted.
+///
+/// The reason also goes straight to stderr. The tracing appenders are
+/// non-blocking workers, and `exit` stops them before they flush, so without
+/// this write a supervisor's journal never learns why the process stopped. The
+/// write result is ignored because `eprintln!` would panic on a broken stderr.
+pub fn fatal(reason: FatalReason, message: &str) -> ! {
Review Comment:
`fatal()` never consults `descriptors_exhausted()`, so the exit-4 contract
only holds on the shard-join path in `main.rs`. An `EMFILE` on the WAL `.tmp`
create in `PrepareJournal::truncate_from`
(`core/journal/src/prepare_journal.rs:851`) or `drain` (`:1030`) records the
note, poisons the journal, and `core/metadata/src/impls/metadata.rs:1054` then
calls `fatal(UnreconcilableLogFrontier)`, which exits 2. Same for
`SuperblockWedged` at exit 3. The PR body says descriptor exhaustion exits 4
because only a restart repairs it. Either have `fatal()` log the recorded
exhaustion alongside the reason it was given, or state in the body and in the
`FatalReason` docs that exits 2 and 3 can also be caused by a full descriptor
table.
##########
core/server/src/boot/fd_limit.rs:
##########
@@ -0,0 +1,191 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! The process's own `RLIMIT_NOFILE`, raised once at startup, and the client
+//! connection cap sized from it.
+
+use message_bus::ConnectionCap;
+use nix::errno::Errno;
+use nix::sys::resource::{Resource, getrlimit, setrlimit};
+use thiserror::Error;
+use tracing::{info, warn};
+
+/// `OPEN_MAX` from `<sys/syslimits.h>`, which `libc` does not export. The
+/// macOS hard limit is usually `RLIM_INFINITY`, and `setrlimit(2)` rejects
+/// that as a soft `RLIMIT_NOFILE` with `EINVAL`, so the man page's recipe is
+/// `min(OPEN_MAX, rlim_max)`.
+#[cfg(target_vendor = "apple")]
+const APPLE_OPEN_MAX: u64 = 10_240;
+
+/// `RLIMIT_NOFILE` around [`raise_open_file_limit`].
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct OpenFileLimit {
+ pub soft_before: u64,
+ pub soft: u64,
+ pub hard: u64,
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Error)]
+pub enum OpenFileLimitError {
+ #[error("cannot read RLIMIT_NOFILE: {0}")]
+ Read(Errno),
+ #[error(
+ "cannot raise the RLIMIT_NOFILE soft limit from {soft} to {target}
(hard {hard}): {errno}"
+ )]
+ Raise {
+ errno: Errno,
+ soft: u64,
+ hard: u64,
+ target: u64,
+ },
+}
+
+/// Raise the soft `RLIMIT_NOFILE` to the hard limit (clamped on macOS).
+/// A soft limit already at or above the target is left as it is.
+///
+/// # Errors
+///
+/// [`OpenFileLimitError::Read`] if the limit cannot be read, and
+/// [`OpenFileLimitError::Raise`] if `setrlimit` rejects the new soft limit.
+pub fn raise_open_file_limit() -> Result<OpenFileLimit, OpenFileLimitError> {
Review Comment:
The soft `RLIMIT_NOFILE` is raised to the hard limit unconditionally. An
operator who set a lower soft limit on purpose (bounding io_uring registered
files, or memory) loses it with one info line and no knob. Acceptable as the
default, but the only user-facing mention is one clause in the
`connections_max` comment at `core/server/config.toml:1136`. Say there, or in a
short `[server]` note, that the raise always happens and cannot be turned off.
##########
core/server/src/main.rs:
##########
@@ -99,6 +116,13 @@ fn main() -> Result<(), ServerError> {
if let Err(error) = &joined {
server::boot::systemd::notify_shutdown_failure(error);
}
+ if let Err(error) = &joined
+ && descriptors_exhausted()
Review Comment:
`DESCRIPTORS_EXHAUSTED` is set once and never cleared. A transient `EMFILE`
burst the server survives, followed hours later by an unrelated shutdown error
such as `ShardPumpDrainTimedOut`, still exits 4 here and points the operator at
the wrong cause. The `NoteDescriptorExhaustion` doc says the record lasts for
the life of the process but not that it can label an unrelated exit. Either say
so there, or include the time of the first noted `EMFILE` in the fatal stderr
line so the two events can be told apart.
##########
foreign/node/src/wire/system/get-stats.command.ts:
##########
@@ -91,6 +101,26 @@ const deserializeGetStats = (b: Buffer) => {
position + 4,
position + 4 + kernelVersionLength
).toString();
+ position += 4 + kernelVersionLength;
+
+ // iggy_server_version, iggy_server_semver
+ const iggyServerVersionLength = b.readUInt32LE(position);
Review Comment:
The `readUInt32LE` for `iggy_server_version` length (line 107) and
`cache_metrics_count` (line 110) run before the new `position > b.length` guard
on line 112, so a reply cut inside those eight bytes still throws a raw
`RangeError` rather than the `DeserializeError` the tail now gets. Same class
of defect as the tail fix, one guard higher.
##########
core/message_bus/src/client_listener/tcp.rs:
##########
@@ -77,6 +78,7 @@ pub async fn run(listener: TcpListener, token: ShutdownToken,
on_accepted: Accep
}
Err(e) => {
error!("Client listener (TCP) accept failed: {e}");
+ pause_after_accept_error(&e).await;
Review Comment:
The 1 s pause runs inside the `accept()` arm of the `select!` at line 73, so
the shutdown token is not polled while it sleeps. Same in the TCP-TLS, WS, WSS,
replica and HTTPS loops. A shutdown that lands during an `EMFILE` burst waits
up to one extra second per listener. Either race the sleep against the token
inside `pause_after_accept_error`, or note the added latency in its doc at
`core/message_bus/src/accept.rs:41`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
