chore(engine): server hygiene batch — workers tests, surname dedup, save pin (T-1063, T-1064)
- workers/pool.rs: 5 new tests; catch_unwind keeps worker threads alive on handler panic (in-flight request loss unchanged, pinned by test + #843 docs); stubs.rs no longer falsely claims the pool is tested - save/load: execute_save_load pinned .after(Storyteller) so the scheduler cannot legally save pre-Input state; exclusive-system exception recorded in tick_phases.rs rules - surname corpus extracted to bin/shared/surname_corpus.rs (both economy generators import it; byte-identical output verified on 23.6MB+1.45MB TOMLs); all three stamp/watch registries updated - generator_spike gated behind non-default 'generator-spike' feature - economy.rs: 11 new D-181 signal-derivation tests on the new econ_sim Simulation::from_economy in-memory constructor - perception exemption comments now state the consumer sort contract; unused bytemuck removed; rayon comment corrected; the 22 allow(dead_code) documented as serde schema enforcement Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Claude Fable 5
+208
-9
@@ -5,12 +5,17 @@
|
||||
//!
|
||||
//! ## Worker crash behavior
|
||||
//!
|
||||
//! TODO (#843): If a worker thread panics, in-flight requests on that thread
|
||||
//! are lost. The pool does NOT currently re-enqueue them. For Fallback-strategy
|
||||
//! workers (voice, LLM) this is acceptable — the tick loop uses the default.
|
||||
//! For ModalLock-strategy workers (chunk gen on gate transit), a lost request
|
||||
//! means the player waits forever. Future: add heartbeat monitoring and
|
||||
//! automatic re-enqueue on worker death.
|
||||
//! Handler panics are contained per-request (T-1063): the worker thread
|
||||
//! catches the unwind, logs an error, and keeps serving the queue — a
|
||||
//! poisoned request cannot permanently shrink the pool. The in-flight
|
||||
//! request is still LOST: no result is ever delivered for it.
|
||||
//!
|
||||
//! TODO (#843): re-enqueue lost requests (needs heartbeat/ack bookkeeping).
|
||||
//! For Fallback-strategy workers (voice, LLM) the loss is acceptable — the
|
||||
//! tick loop uses the default. For ModalLock-strategy workers (chunk gen on
|
||||
//! gate transit), a lost request means the player waits forever. Before any
|
||||
//! ModalLock consumer ships (Phase 5), implement re-enqueue or downgrade
|
||||
//! ModalLock to an error path.
|
||||
|
||||
use std::sync::atomic::{AtomicBool, Ordering};
|
||||
use std::sync::Arc;
|
||||
@@ -88,9 +93,27 @@ where
|
||||
match rx.recv_timeout(std::time::Duration::from_millis(100)) {
|
||||
Ok(work) => {
|
||||
let strategy = work.strategy;
|
||||
let result = handler(work.payload);
|
||||
if tx.send((result, strategy)).is_err() {
|
||||
break; // response channel closed
|
||||
// Contain handler panics (T-1063): the thread must
|
||||
// survive a poisoned request or the pool permanently
|
||||
// loses capacity. The in-flight request is still lost
|
||||
// (no result delivered) — re-enqueue is future work,
|
||||
// see the module-level TODO (#843).
|
||||
let result = std::panic::catch_unwind(
|
||||
std::panic::AssertUnwindSafe(|| handler(work.payload)),
|
||||
);
|
||||
match result {
|
||||
Ok(resp) => {
|
||||
if tx.send((resp, strategy)).is_err() {
|
||||
break; // response channel closed
|
||||
}
|
||||
}
|
||||
Err(_) => {
|
||||
tracing::error!(
|
||||
strategy = ?strategy,
|
||||
"worker handler panicked — in-flight request lost, \
|
||||
no result will be delivered (#843)"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
Err(crossbeam_channel::RecvTimeoutError::Timeout) => continue,
|
||||
@@ -159,3 +182,179 @@ impl<Req, Resp> Drop for BackgroundWorkerPool<Req, Resp> {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Tests (T-1063): spawn/push/poll/shutdown lifecycle + worker-panic behavior.
|
||||
//
|
||||
// Every test also exercises Drop implicitly — a pool whose threads fail to
|
||||
// exit would hang the test binary at scope end.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use std::time::{Duration, Instant};
|
||||
|
||||
const WAIT: Duration = Duration::from_secs(5);
|
||||
|
||||
/// Spin until `cond` holds, panicking after `WAIT`. Worker completion is
|
||||
/// timing-dependent (thread scheduling), so tests wait on observable
|
||||
/// state instead of sleeping fixed amounts.
|
||||
fn wait_for(what: &str, mut cond: impl FnMut() -> bool) {
|
||||
let start = Instant::now();
|
||||
while !cond() {
|
||||
if start.elapsed() > WAIT {
|
||||
panic!("timed out after {WAIT:?} waiting for: {what}");
|
||||
}
|
||||
thread::sleep(Duration::from_millis(5));
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn spawn_push_poll_round_trip() {
|
||||
let pool = BackgroundWorkerPool::spawn(2, |x: u32| x * 2);
|
||||
assert!(pool.poll().is_none(), "fresh pool has no results");
|
||||
assert!(!pool.has_results());
|
||||
assert_eq!(pool.pending_count(), 0);
|
||||
|
||||
pool.submit(WorkRequest {
|
||||
payload: 21,
|
||||
strategy: DeliveryStrategy::Fallback,
|
||||
});
|
||||
|
||||
wait_for("result ready", || pool.has_results());
|
||||
let (resp, strategy) = pool.poll().expect("has_results implies poll succeeds");
|
||||
assert_eq!(resp, 42);
|
||||
assert_eq!(strategy, DeliveryStrategy::Fallback);
|
||||
assert!(pool.poll().is_none(), "single request yields single result");
|
||||
assert_eq!(pool.pending_count(), 0);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn drain_collects_all_results_and_preserves_strategies() {
|
||||
let pool = BackgroundWorkerPool::spawn(2, |x: u32| x + 1);
|
||||
for i in 0..10u32 {
|
||||
pool.submit(WorkRequest {
|
||||
payload: i,
|
||||
strategy: if i % 2 == 0 {
|
||||
DeliveryStrategy::GracefulDegrade
|
||||
} else {
|
||||
DeliveryStrategy::ModalLock
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
let mut results: Vec<(u32, DeliveryStrategy)> = Vec::new();
|
||||
wait_for("all 10 results drained", || {
|
||||
results.extend(pool.drain());
|
||||
results.len() == 10
|
||||
});
|
||||
|
||||
// Two worker threads — completion order is not guaranteed. Each
|
||||
// request must come back exactly once, paired with its own strategy.
|
||||
results.sort_by_key(|(resp, _)| *resp);
|
||||
for (i, (resp, strategy)) in results.iter().enumerate() {
|
||||
assert_eq!(*resp, i as u32 + 1);
|
||||
let expected = if i % 2 == 0 {
|
||||
DeliveryStrategy::GracefulDegrade
|
||||
} else {
|
||||
DeliveryStrategy::ModalLock
|
||||
};
|
||||
assert_eq!(*strategy, expected, "strategy must travel with request {i}");
|
||||
}
|
||||
assert!(!pool.has_results(), "drain leaves the queue empty");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn shutdown_stops_workers_and_drops_later_submissions() {
|
||||
let pool = BackgroundWorkerPool::spawn(2, |x: u32| x);
|
||||
pool.shutdown();
|
||||
wait_for("all workers exited", || {
|
||||
pool.workers.iter().all(|h| h.is_finished())
|
||||
});
|
||||
|
||||
// Once every worker has exited, all Receiver clones are gone (spawn
|
||||
// drops the original), so the request channel is disconnected:
|
||||
// submit() silently discards the request — by design (`let _ =` on
|
||||
// send). Nothing is queued and no result ever appears.
|
||||
pool.submit(WorkRequest {
|
||||
payload: 7,
|
||||
strategy: DeliveryStrategy::Fallback,
|
||||
});
|
||||
assert_eq!(
|
||||
pool.pending_count(),
|
||||
0,
|
||||
"post-shutdown submissions are silently dropped, not queued"
|
||||
);
|
||||
assert!(pool.poll().is_none());
|
||||
assert!(!pool.has_results());
|
||||
}
|
||||
|
||||
// -- Worker-panic behavior (T-1063) ----------------------------------------
|
||||
|
||||
/// Handler that panics on payload 0 and echoes anything else.
|
||||
fn poison_handler(x: u32) -> u32 {
|
||||
if x == 0 {
|
||||
panic!("poisoned request");
|
||||
}
|
||||
x
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn handler_panic_does_not_kill_the_worker_thread() {
|
||||
// Single thread: if the panic killed it, the second request would
|
||||
// never be processed and this test would time out.
|
||||
let pool = BackgroundWorkerPool::spawn(1, poison_handler);
|
||||
pool.submit(WorkRequest {
|
||||
payload: 0, // poison
|
||||
strategy: DeliveryStrategy::ModalLock,
|
||||
});
|
||||
pool.submit(WorkRequest {
|
||||
payload: 5,
|
||||
strategy: DeliveryStrategy::Fallback,
|
||||
});
|
||||
|
||||
wait_for("post-panic result", || pool.has_results());
|
||||
let (resp, _) = pool.poll().expect("worker survived the panic");
|
||||
assert_eq!(resp, 5);
|
||||
assert!(
|
||||
!pool.workers[0].is_finished(),
|
||||
"worker thread must survive a handler panic"
|
||||
);
|
||||
}
|
||||
|
||||
/// KNOWN LIMITATION (#843): the panicked request is silently lost — no
|
||||
/// result is ever delivered for it and it is not re-enqueued. For a
|
||||
/// ModalLock consumer this means the player waits forever; heartbeat +
|
||||
/// re-enqueue (or downgrading ModalLock to an error path) must land
|
||||
/// before any ModalLock consumer ships (Phase 5). This test pins the
|
||||
/// current loss behavior so the eventual fix has to update it visibly.
|
||||
#[test]
|
||||
fn handler_panic_loses_the_inflight_request() {
|
||||
let pool = BackgroundWorkerPool::spawn(1, poison_handler);
|
||||
pool.submit(WorkRequest {
|
||||
payload: 0, // poison — would be the ModalLock request a player waits on
|
||||
strategy: DeliveryStrategy::ModalLock,
|
||||
});
|
||||
// Sentinel: once its result arrives, the poison request has
|
||||
// definitely been taken off the queue (single worker, FIFO channel).
|
||||
pool.submit(WorkRequest {
|
||||
payload: 99,
|
||||
strategy: DeliveryStrategy::Fallback,
|
||||
});
|
||||
|
||||
wait_for("sentinel result", || pool.has_results());
|
||||
let results = pool.drain();
|
||||
assert_eq!(
|
||||
results.len(),
|
||||
1,
|
||||
"only the sentinel completes — the poisoned request produced no result"
|
||||
);
|
||||
assert_eq!(results[0].0, 99);
|
||||
assert_eq!(
|
||||
pool.pending_count(),
|
||||
0,
|
||||
"the poisoned request is gone from the queue, not re-enqueued"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,7 +4,10 @@
|
||||
//! The actual computation logic is a no-op placeholder — implementations
|
||||
//! plug in when the phases that need them arrive (Phase 5+).
|
||||
//!
|
||||
//! The infrastructure (channels, threads, push/poll) is real and tested.
|
||||
//! The infrastructure (channels, threads, push/poll) is real — unit tests
|
||||
//! live in `pool.rs` (T-1063). Known limitation (#843): a handler panic
|
||||
//! loses the in-flight request (no result is ever delivered, no re-enqueue);
|
||||
//! see the crash-behavior notes in `pool.rs`.
|
||||
|
||||
use bevy_ecs::prelude::*;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user