Skip to content

Commit 04c475d

Browse files
AlexStocksOmX
andcommitted
fix: prevent PR #403 failure paths from escaping verification
Bound queue admission and executor completion failures, then aligned CI gates and regression coverage so silent stalls and skipped targets block review before merge. Constraint: Preserve public APIs and exclude the Base-only RPOPLPUSH atomicity defect. Rejected: Remove the storage stats feature gate | it is required to keep fault injection out of production-only builds. Confidence: high Scope-risk: moderate Tested: WSL fmt; workspace all-targets all-features Clippy; net, executor, runtime, RESP, Raft, storage, and fault-injection storage stats tests. Not-tested: Fresh GitHub Actions on this commit. Co-authored-by: OmX <omx@oh-my-codex.dev>
1 parent 7d55fa8 commit 04c475d

12 files changed

Lines changed: 320 additions & 37 deletions

File tree

.github/workflows/ci.yml

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,9 +136,6 @@ jobs:
136136
137137
- run: make lint
138138

139-
- name: Lint runtime baseline tool targets
140-
run: cargo clippy -p runtime-baseline --all-targets -- -D warnings -D clippy::unwrap_used
141-
142139
build-and-test:
143140
strategy:
144141
fail-fast: false
@@ -223,6 +220,10 @@ jobs:
223220
max_attempts: 2
224221
command: bash -o pipefail -c "make test 2>&1 | tee cargo-test.log"
225222

223+
- name: Test storage stats with fault injection
224+
if: matrix.os == 'ubuntu-latest'
225+
run: cargo test -p storage --features test-fault-injection --test storage_stats_test
226+
226227
- name: Test runtime baseline tool
227228
run: cargo test -p runtime-baseline --all-targets
228229

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ fmt-check:
5151
@cargo fmt --manifest-path ./Cargo.toml --all -- --check
5252

5353
lint:
54-
@cargo clippy --manifest-path ./Cargo.toml --all-features --workspace -- -D warnings -D clippy::unwrap_used
54+
@cargo clippy --manifest-path ./Cargo.toml --all-features --workspace --all-targets -- -D warnings -D clippy::unwrap_used
5555

5656
standalone:
5757
@$(DEV) build

docs/quality/quality-gates.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
```text
1717
license header / third-party license check
1818
cargo fmt --check
19-
cargo clippy --all-features --workspace -- -D warnings -D clippy::unwrap_used
19+
cargo clippy --all-features --workspace --all-targets -- -D warnings -D clippy::unwrap_used
2020
targeted unit tests
2121
affected compatibility manifest tests
2222
Cache OFF compatibility and recovery evidence

src/common/runtime/baseline.rs

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,6 +1015,45 @@ mod tests {
10151015
)
10161016
}
10171017

1018+
#[test]
1019+
fn attempt_state_decodes_every_valid_discriminant() {
1020+
let states = [
1021+
AttemptState::Offered,
1022+
AttemptState::ChannelQueued,
1023+
AttemptState::BatchQueued,
1024+
AttemptState::WaitingGate,
1025+
AttemptState::Running,
1026+
AttemptState::ExecutionFinished,
1027+
AttemptState::ShutdownRejectedAfterAccept,
1028+
AttemptState::Abandoned,
1029+
];
1030+
1031+
for state in states {
1032+
assert_eq!(AttemptState::from_u8(state as u8), Some(state));
1033+
}
1034+
}
1035+
1036+
#[test]
1037+
fn attempt_state_rejects_unknown_discriminant() {
1038+
assert_eq!(AttemptState::from_u8(8), None);
1039+
assert_eq!(AttemptState::from_u8(u8::MAX), None);
1040+
}
1041+
1042+
#[test]
1043+
fn attempt_state_accessor_falls_back_to_abandoned_for_unknown_raw_state() {
1044+
let observer = Arc::new(RecordingObserver::default());
1045+
let attempt = attempt(observer);
1046+
1047+
attempt.inner.state.store(u8::MAX, Ordering::Release);
1048+
assert_eq!(attempt.state(), AttemptState::Abandoned);
1049+
1050+
// Restore a valid pre-accept state so Drop follows the ordinary path.
1051+
attempt
1052+
.inner
1053+
.state
1054+
.store(AttemptState::Offered as u8, Ordering::Release);
1055+
}
1056+
10181057
#[test]
10191058
fn legal_transitions_record_previous_and_next_state() {
10201059
let observer = Arc::new(RecordingObserver::default());

src/executor/src/executor.rs

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ impl CmdExecutor {
8383
}
8484

8585
pub async fn execute(&self, exec: CmdExecution) {
86+
let client = Arc::clone(&exec.client);
8687
let (done_tx, done_rx) = oneshot::channel();
8788
let work = CmdExecutionWork {
8889
exec,
@@ -108,11 +109,15 @@ impl CmdExecutor {
108109
work.exec
109110
.client
110111
.set_reply(RespData::Error("ERR executor unavailable".into()));
112+
return;
111113
}
112114
}
113115

114116
// TODO(#400): add a timeout for waiting
115-
let _ = done_rx.await;
117+
if let Err(err) = done_rx.await {
118+
error!("executor worker exited before completing work: {err}");
119+
client.set_reply(RespData::Error("ERR executor unavailable".into()));
120+
}
116121
}
117122

118123
pub async fn close(&mut self) {
@@ -366,6 +371,66 @@ mod tests {
366371
executor.close().await;
367372
}
368373

374+
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
375+
async fn accepted_work_reports_unavailable_when_worker_exits() {
376+
let storage = Arc::new(Storage::new(1, 0));
377+
let exclusive = storage.acquire_exclusive_command_access().await;
378+
let attempts = Arc::new(AtomicUsize::new(0));
379+
let attempt_notify = Arc::new(tokio::sync::Notify::new());
380+
let executed = Arc::new(AtomicUsize::new(0));
381+
let command = Arc::new(WaitingSharedCmd::new(
382+
Arc::clone(&attempts),
383+
Arc::clone(&attempt_notify),
384+
Arc::clone(&executed),
385+
));
386+
let client = Arc::new(Client::new(Box::new(TestStream::new())));
387+
client.set_cmd_name(b"waiting-shared");
388+
client.set_argv(&[b"waiting-shared".to_vec()]);
389+
let executor = Arc::new(CmdExecutor::new(1, 1));
390+
391+
let executor_for_task = Arc::clone(&executor);
392+
let client_for_task = Arc::clone(&client);
393+
let storage_for_task = Arc::clone(&storage);
394+
let execute_task = tokio::spawn(async move {
395+
executor_for_task
396+
.execute(CmdExecution {
397+
cmd: command,
398+
client: client_for_task,
399+
storage: storage_for_task,
400+
})
401+
.await;
402+
});
403+
404+
tokio::time::timeout(Duration::from_secs(1), attempt_notify.notified())
405+
.await
406+
.expect("worker must dequeue accepted work");
407+
assert_eq!(attempts.load(Ordering::SeqCst), 1);
408+
409+
executor.workers[0].abort();
410+
tokio::time::timeout(Duration::from_secs(1), async {
411+
while !executor.workers[0].is_finished() {
412+
tokio::task::yield_now().await;
413+
}
414+
})
415+
.await
416+
.expect("aborted worker must finish");
417+
tokio::time::timeout(Duration::from_secs(1), execute_task)
418+
.await
419+
.expect("execute must observe the cancelled completion channel")
420+
.expect("execute task must not panic");
421+
422+
assert_eq!(executed.load(Ordering::SeqCst), 0);
423+
assert_eq!(
424+
client.take_reply(),
425+
RespData::Error("ERR executor unavailable".into())
426+
);
427+
428+
drop(exclusive);
429+
let mut executor = Arc::try_unwrap(executor)
430+
.unwrap_or_else(|_| panic!("test must release all executor references"));
431+
executor.close().await;
432+
}
433+
369434
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
370435
async fn executor_routes_commands_through_shared_storage_access() {
371436
let storage = Arc::new(Storage::new(1, 0));

src/net/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ raft = { path = "../raft" }
2828

2929
[dev-dependencies]
3030
env_logger = "0.11"
31+
tokio = { workspace = true, features = ["test-util"] }
3132

3233
[[bench]]
3334
name = "network_benchmark"

src/net/src/pipeline.rs

Lines changed: 117 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ use storage::storage::Storage;
2727
use tokio::sync::{Semaphore, mpsc, oneshot};
2828
use tokio::time::timeout;
2929

30+
const COMMAND_TIMEOUT: Duration = Duration::from_secs(30);
31+
3032
/// Configuration for pipeline processing
3133
#[derive(Debug, Clone)]
3234
pub struct PipelineConfig {
@@ -113,15 +115,16 @@ pub struct CommandPipeline {
113115

114116
impl CommandPipeline {
115117
pub fn new(
116-
config: PipelineConfig,
118+
mut config: PipelineConfig,
117119
storage: Arc<Storage>,
118120
cmd_table: Arc<CmdTable>,
119121
executor: Arc<CmdExecutor>,
120122
) -> Self {
121123
// Bounded channel enforces backpressure: when the queue is full,
122124
// senders await instead of growing the queue unboundedly.
123125
// Guard against 0 (rendezvous) to avoid unintentional stalls.
124-
let (command_tx, command_rx) = mpsc::channel(config.command_queue_size.max(1));
126+
config.command_queue_size = config.command_queue_size.max(1);
127+
let (command_tx, command_rx) = mpsc::channel(config.command_queue_size);
125128
let semaphore = Arc::new(Semaphore::new(config.max_concurrent_pipelines));
126129

127130
let pipeline = Self {
@@ -155,18 +158,7 @@ impl CommandPipeline {
155158
response_tx,
156159
};
157160

158-
// Send command to pipeline. On a full bounded channel this awaits
159-
// (backpressure) rather than growing the queue indefinitely.
160-
self.command_tx
161-
.send(command)
162-
.await
163-
.map_err(|_| PipelineError::ChannelClosed)?;
164-
165-
// Wait for response with timeout
166-
timeout(Duration::from_secs(30), response_rx)
167-
.await
168-
.map_err(|_| PipelineError::Timeout)?
169-
.map_err(|_| PipelineError::ResponseChannelClosed)
161+
send_and_receive(&self.command_tx, command, response_rx, COMMAND_TIMEOUT).await
170162
}
171163

172164
/// Start the background batch processor
@@ -396,12 +388,122 @@ pub enum PipelineError {
396388
Timeout,
397389
}
398390

391+
async fn send_and_receive(
392+
command_tx: &mpsc::Sender<PipelineCommand>,
393+
command: PipelineCommand,
394+
response_rx: oneshot::Receiver<RespData>,
395+
request_timeout: Duration,
396+
) -> Result<RespData, PipelineError> {
397+
timeout(request_timeout, async {
398+
command_tx
399+
.send(command)
400+
.await
401+
.map_err(|_| PipelineError::ChannelClosed)?;
402+
response_rx
403+
.await
404+
.map_err(|_| PipelineError::ResponseChannelClosed)
405+
})
406+
.await
407+
.map_err(|_| PipelineError::Timeout)?
408+
}
409+
399410
#[allow(clippy::unwrap_used)]
400411
#[cfg(test)]
401412
mod tests {
402413
use super::*;
403414
use std::time::Duration;
404-
use tokio::time::sleep;
415+
use tokio::time::{advance, sleep};
416+
417+
struct TestStream;
418+
419+
#[async_trait::async_trait]
420+
impl client::StreamTrait for TestStream {
421+
async fn read(&mut self, _buf: &mut [u8]) -> Result<usize, std::io::Error> {
422+
Ok(0)
423+
}
424+
425+
async fn write(&mut self, _data: &[u8]) -> Result<usize, std::io::Error> {
426+
Ok(0)
427+
}
428+
}
429+
430+
fn pipeline_command(response_tx: oneshot::Sender<RespData>) -> PipelineCommand {
431+
PipelineCommand {
432+
data: RespData::Array(Some(Vec::new())),
433+
client: Arc::new(Client::new(Box::new(TestStream))),
434+
received_at: Instant::now(),
435+
response_tx,
436+
}
437+
}
438+
439+
#[tokio::test(start_paused = true)]
440+
async fn queue_wait_and_response_share_single_deadline() {
441+
let (command_tx, mut command_rx) = mpsc::channel(1);
442+
let (filler_response_tx, _filler_response_rx) = oneshot::channel();
443+
command_tx
444+
.send(pipeline_command(filler_response_tx))
445+
.await
446+
.expect("filler command must fit in the queue");
447+
448+
let (target_response_tx, target_response_rx) = oneshot::channel();
449+
let target_command = pipeline_command(target_response_tx);
450+
let submit_task = tokio::spawn(async move {
451+
send_and_receive(
452+
&command_tx,
453+
target_command,
454+
target_response_rx,
455+
Duration::from_secs(30),
456+
)
457+
.await
458+
});
459+
tokio::task::yield_now().await;
460+
461+
advance(Duration::from_secs(20)).await;
462+
let filler_command = command_rx
463+
.recv()
464+
.await
465+
.expect("filler command must remain queued");
466+
drop(filler_command);
467+
tokio::task::yield_now().await;
468+
469+
advance(Duration::from_secs(11)).await;
470+
tokio::task::yield_now().await;
471+
assert!(
472+
submit_task.is_finished(),
473+
"queue wait and response wait must share one deadline"
474+
);
475+
let result = submit_task.await.expect("submission task must not panic");
476+
assert!(matches!(result, Err(PipelineError::Timeout)));
477+
478+
let target_command = command_rx
479+
.recv()
480+
.await
481+
.expect("target command must have entered the queue");
482+
assert!(
483+
target_command
484+
.response_tx
485+
.send(RespData::SimpleString("late".into()))
486+
.is_err(),
487+
"timing out must close the target response receiver"
488+
);
489+
}
490+
491+
#[tokio::test]
492+
async fn zero_command_queue_size_reports_effective_capacity() {
493+
let config = PipelineConfig {
494+
command_queue_size: 0,
495+
..Default::default()
496+
};
497+
let pipeline = CommandPipeline::new(
498+
config,
499+
Arc::new(Storage::new(1, 0)),
500+
Arc::new(CmdTable::new()),
501+
Arc::new(CmdExecutor::new(1, 1)),
502+
);
503+
504+
assert_eq!(pipeline.command_tx.max_capacity(), 1);
505+
assert_eq!(pipeline.stats().queue_capacity, 1);
506+
}
405507

406508
#[tokio::test]
407509
async fn test_pipeline_basic() {

src/raft/src/network.rs

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,3 +331,39 @@ impl RaftNetwork<KiwiTypeConfig> for KiwiNetwork {
331331
Err(rpc_failed_network_error(&self.target, last_error))
332332
}
333333
}
334+
335+
#[allow(clippy::unwrap_used)]
336+
#[cfg(test)]
337+
mod tests {
338+
use openraft::Vote;
339+
340+
use super::*;
341+
342+
#[tokio::test]
343+
async fn invalid_raft_endpoint_returns_network_error_without_panicking() {
344+
let invalid_target = "not a valid raft address";
345+
let mut factory = KiwiNetworkFactory::with_config(ConnectionConfig {
346+
max_retries: 0,
347+
..Default::default()
348+
});
349+
let node = KiwiNode {
350+
raft_addr: invalid_target.to_string(),
351+
resp_addr: String::new(),
352+
};
353+
354+
let mut network = factory.new_client(2, &node).await;
355+
let error = network
356+
.vote(
357+
VoteRequest::new(Vote::new(1, 1), None),
358+
RPCOption::new(Duration::from_millis(50)),
359+
)
360+
.await
361+
.expect_err("invalid endpoint must fail as a network error");
362+
363+
assert!(matches!(error, RPCErr::Network(_)));
364+
assert!(
365+
error.to_string().contains(invalid_target),
366+
"network error must identify the configured target: {error}"
367+
);
368+
}
369+
}

0 commit comments

Comments
 (0)