mirror of
https://github.com/0xrsydn/idx-cli.git
synced 2026-11-07 09:45:36 +00:00
fix(net): bounded request time, curl size and time caps, lock waits, sane prices
From the full-repo Codex audit: - MSN and Yahoo clients set connect and body timeouts but none for the response headers, so a server that stalled before answering hung the call; four such calls take every MCP call slot. Add a 30 s overall deadline. Every curl-impersonate run gets --max-time 600 and a 256 MiB --max-filesize, and output above that is rejected (curl cannot cap a response without a length); the Yahoo cookie fetch gets --max-time. - Ownership commands blocked on the database lock without limit, so a long sync or import, or a stuck process, held MCP call slots indefinitely. Wait up to 30 s, then fail with a "busy, try again" error. - A price of 1e20 saturated `as i64` and the change subtraction then overflowed. Prices that are not finite, negative, or above 1e12 IDR are treated as missing, and a quote's own price as an error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3syn2fN2LNo5pvTSqc6eh
This commit is contained in:
parent
94aec74b09
commit
66207d24b6
6 changed files with 144 additions and 34 deletions
|
|
@ -30,6 +30,9 @@ impl MsnClient {
|
|||
let agent: ureq::Agent = ureq::Agent::config_builder()
|
||||
.timeout_connect(Some(Duration::from_secs(5)))
|
||||
.timeout_recv_body(Some(Duration::from_secs(10)))
|
||||
// A server that stalls before sending headers must not hold a
|
||||
// call (and an MCP call slot) forever.
|
||||
.timeout_global(Some(Duration::from_secs(30)))
|
||||
.build()
|
||||
.into();
|
||||
|
||||
|
|
|
|||
|
|
@ -25,11 +25,15 @@ pub(super) fn parse_quote(symbol: &str, quotes: &[MsnQuote]) -> Result<Quote, Id
|
|||
}
|
||||
return Err(IdxError::SymbolNotFound(symbol.to_string()));
|
||||
};
|
||||
let prev_close = quote.price_previous_close.map(round_price);
|
||||
let price = round_price(raw_price);
|
||||
let prev_close = quote.price_previous_close.and_then(round_price);
|
||||
let price = round_price(raw_price).ok_or_else(|| {
|
||||
IdxError::ParseError(format!(
|
||||
"MSN sent an implausible price for {symbol}: {raw_price}"
|
||||
))
|
||||
})?;
|
||||
let change = prev_close
|
||||
.map(|previous| price - previous)
|
||||
.or_else(|| quote.price_change.map(round_price));
|
||||
.or_else(|| quote.price_change.and_then(round_price));
|
||||
|
||||
let ticker = quote
|
||||
.symbol
|
||||
|
|
@ -59,8 +63,8 @@ pub(super) fn parse_quote(symbol: &str, quotes: &[MsnQuote]) -> Result<Quote, Id
|
|||
change_pct: quote.price_change_percent,
|
||||
volume: round_u64(quote.accumulated_volume),
|
||||
market_cap: round_u64(quote.market_cap),
|
||||
week52_high: quote.price_52w_high.map(round_price),
|
||||
week52_low: quote.price_52w_low.map(round_price),
|
||||
week52_high: quote.price_52w_high.and_then(round_price),
|
||||
week52_low: quote.price_52w_low.and_then(round_price),
|
||||
week52_position,
|
||||
range_signal,
|
||||
prev_close,
|
||||
|
|
@ -196,8 +200,12 @@ fn sanitize_current_ratio(value: Option<f64>) -> Option<f64> {
|
|||
value.filter(|number| number.is_finite() && *number >= 0.01)
|
||||
}
|
||||
|
||||
fn round_price(value: f64) -> i64 {
|
||||
value.round() as i64
|
||||
/// Prices are IDR; anything non-finite, negative, or above this is not a
|
||||
/// real quote, and treating it as missing keeps arithmetic from overflowing.
|
||||
const MAX_PRICE: f64 = 1e12;
|
||||
|
||||
fn round_price(value: f64) -> Option<i64> {
|
||||
(value.is_finite() && (0.0..=MAX_PRICE).contains(&value)).then(|| value.round() as i64)
|
||||
}
|
||||
|
||||
fn round_u64(value: Option<f64>) -> Option<u64> {
|
||||
|
|
@ -667,11 +675,11 @@ pub(super) fn parse_screener_results(raw: &RawScreenerResponse) -> Result<Vec<Qu
|
|||
.iter()
|
||||
.filter_map(|q| {
|
||||
let raw_price = q.price.filter(|price| price.is_finite() && *price > 0.0)?;
|
||||
let price = round_price(raw_price);
|
||||
let prev_close = q.price_previous_close.map(round_price);
|
||||
let price = round_price(raw_price)?;
|
||||
let prev_close = q.price_previous_close.and_then(round_price);
|
||||
let change = prev_close
|
||||
.map(|pc| price - pc)
|
||||
.or_else(|| q.price_change.map(round_price));
|
||||
.or_else(|| q.price_change.and_then(round_price));
|
||||
let ticker = q
|
||||
.symbol
|
||||
.as_deref()
|
||||
|
|
@ -698,8 +706,8 @@ pub(super) fn parse_screener_results(raw: &RawScreenerResponse) -> Result<Vec<Qu
|
|||
change_pct: q.price_change_percent,
|
||||
volume: round_u64(q.accumulated_volume),
|
||||
market_cap: round_u64(q.market_cap),
|
||||
week52_high: q.price_52w_high.map(round_price),
|
||||
week52_low: q.price_52w_low.map(round_price),
|
||||
week52_high: q.price_52w_high.and_then(round_price),
|
||||
week52_low: q.price_52w_low.and_then(round_price),
|
||||
week52_position,
|
||||
range_signal,
|
||||
prev_close,
|
||||
|
|
@ -729,12 +737,11 @@ pub(super) fn parse_history(
|
|||
let Some(close_raw) = series.prices.get(idx).copied().flatten() else {
|
||||
continue;
|
||||
};
|
||||
if !close_raw.is_finite() {
|
||||
let Some(close) = round_price(close_raw) else {
|
||||
continue;
|
||||
}
|
||||
};
|
||||
let timestamp = DateTime::parse_from_rfc3339(raw_ts)
|
||||
.map_err(|e| IdxError::ParseError(format!("msn chart timestamp '{raw_ts}': {e}")))?;
|
||||
let close = round_price(close_raw);
|
||||
let open = series_price_at(&series.open_prices, idx).unwrap_or(close);
|
||||
let high = series_price_at(&series.prices_high, idx).unwrap_or(close);
|
||||
let low = series_price_at(&series.prices_low, idx).unwrap_or(close);
|
||||
|
|
@ -774,7 +781,7 @@ fn series_price_at(values: &[Option<f64>], idx: usize) -> Option<i64> {
|
|||
.copied()
|
||||
.flatten()
|
||||
.filter(|value| value.is_finite())
|
||||
.map(round_price)
|
||||
.and_then(round_price)
|
||||
}
|
||||
|
||||
fn series_volume_at(series: &RawChartSeries, idx: usize) -> u64 {
|
||||
|
|
@ -889,6 +896,22 @@ mod tests {
|
|||
assert_eq!(sentiment.statistics[0].neutral, 3);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_quote_rejects_implausible_prices_instead_of_overflowing() {
|
||||
let raw: Vec<MsnQuote> =
|
||||
serde_json::from_str(r#"[{"symbol":"BBCA","price":1e20,"pricePreviousClose":-1e20}]"#)
|
||||
.unwrap();
|
||||
assert!(parse_quote("BBCA.JK", &raw).is_err());
|
||||
|
||||
let raw: Vec<MsnQuote> = serde_json::from_str(
|
||||
r#"[{"symbol":"BBCA","price":6075.0,"pricePreviousClose":-1e20}]"#,
|
||||
)
|
||||
.unwrap();
|
||||
let quote = parse_quote("BBCA.JK", &raw).unwrap();
|
||||
assert_eq!(quote.price, 6075);
|
||||
assert_eq!(quote.prev_close, None);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parses_news_fixture_items() {
|
||||
let raw: RawNewsFeed =
|
||||
|
|
|
|||
|
|
@ -30,6 +30,9 @@ impl YahooClient {
|
|||
let agent: ureq::Agent = ureq::Agent::config_builder()
|
||||
.timeout_connect(Some(Duration::from_secs(5)))
|
||||
.timeout_recv_body(Some(Duration::from_secs(10)))
|
||||
// A server that stalls before sending headers must not hold a
|
||||
// call (and an MCP call slot) forever.
|
||||
.timeout_global(Some(Duration::from_secs(30)))
|
||||
.build()
|
||||
.into();
|
||||
|
||||
|
|
@ -141,7 +144,14 @@ impl YahooClient {
|
|||
// Step 1: fetch fc.yahoo.com to set A3 cookie (returns 404 but writes cookie jar).
|
||||
// We allow non-zero exit here since 404 still writes the cookie.
|
||||
let _ = Command::new(binary)
|
||||
.args(["--silent", "--cookie-jar", cookie_jar_str, COOKIE_FETCH_URL])
|
||||
.args([
|
||||
"--silent",
|
||||
"--max-time",
|
||||
"30",
|
||||
"--cookie-jar",
|
||||
cookie_jar_str,
|
||||
COOKIE_FETCH_URL,
|
||||
])
|
||||
.stdout(Stdio::null())
|
||||
.stderr(Stdio::null())
|
||||
.output();
|
||||
|
|
|
|||
|
|
@ -20,8 +20,12 @@ pub(super) fn parse_quote(symbol: &str, chart: &ChartResponse) -> Result<Quote,
|
|||
.ok_or_else(|| IdxError::SymbolNotFound(symbol.to_string()))?;
|
||||
let raw_prev_close = meta.previous_close.or(meta.chart_previous_close);
|
||||
|
||||
let price = round_price(raw_price);
|
||||
let prev_close = raw_prev_close.map(round_price);
|
||||
let price = round_price(raw_price).ok_or_else(|| {
|
||||
IdxError::ParseError(format!(
|
||||
"Yahoo sent an implausible price for {symbol}: {raw_price}"
|
||||
))
|
||||
})?;
|
||||
let prev_close = raw_prev_close.and_then(round_price);
|
||||
let change = prev_close.map(|p| price - p);
|
||||
let change_pct = raw_prev_close
|
||||
.filter(|p| *p != 0.0)
|
||||
|
|
@ -50,8 +54,8 @@ pub(super) fn parse_quote(symbol: &str, chart: &ChartResponse) -> Result<Quote,
|
|||
change_pct,
|
||||
volume: meta.regular_market_volume,
|
||||
market_cap: meta.market_cap,
|
||||
week52_high: meta.fifty_two_week_high.map(round_price),
|
||||
week52_low: meta.fifty_two_week_low.map(round_price),
|
||||
week52_high: meta.fifty_two_week_high.and_then(round_price),
|
||||
week52_low: meta.fifty_two_week_low.and_then(round_price),
|
||||
week52_position,
|
||||
range_signal,
|
||||
prev_close,
|
||||
|
|
@ -103,22 +107,22 @@ pub(super) fn parse_history(
|
|||
.open
|
||||
.as_ref()
|
||||
.and_then(|v| v.get(i).copied().flatten())
|
||||
.map(round_price);
|
||||
.and_then(round_price);
|
||||
let high = quote
|
||||
.high
|
||||
.as_ref()
|
||||
.and_then(|v| v.get(i).copied().flatten())
|
||||
.map(round_price);
|
||||
.and_then(round_price);
|
||||
let low = quote
|
||||
.low
|
||||
.as_ref()
|
||||
.and_then(|v| v.get(i).copied().flatten())
|
||||
.map(round_price);
|
||||
.and_then(round_price);
|
||||
let close = quote
|
||||
.close
|
||||
.as_ref()
|
||||
.and_then(|v| v.get(i).copied().flatten())
|
||||
.map(round_price);
|
||||
.and_then(round_price);
|
||||
let volume = quote
|
||||
.volume
|
||||
.as_ref()
|
||||
|
|
@ -202,8 +206,12 @@ pub(super) fn parse_fundamentals(
|
|||
})
|
||||
}
|
||||
|
||||
fn round_price(value: f64) -> i64 {
|
||||
value.round() as i64
|
||||
/// Prices are IDR; anything non-finite, negative, or above this is not a
|
||||
/// real quote, and treating it as missing keeps arithmetic from overflowing.
|
||||
const MAX_PRICE: f64 = 1e12;
|
||||
|
||||
fn round_price(value: f64) -> Option<i64> {
|
||||
(value.is_finite() && (0.0..=MAX_PRICE).contains(&value)).then(|| value.round() as i64)
|
||||
}
|
||||
|
||||
pub(super) fn map_yahoo_error(symbol: &str, endpoint: &str, err: &ChartError) -> IdxError {
|
||||
|
|
|
|||
|
|
@ -97,8 +97,19 @@ pub fn run_owned_with_fallback(
|
|||
Err(last_error.expect("at least one profile was tried"))
|
||||
}
|
||||
|
||||
/// Every curl run gives up after this long, so a stalled server cannot hang
|
||||
/// a command or an MCP call.
|
||||
const MAX_TIME_SECS: &str = "600";
|
||||
/// Largest response kept in memory (IDX report files are a few MB).
|
||||
const MAX_RESPONSE_BYTES: usize = 256 * 1024 * 1024;
|
||||
|
||||
fn run_binary(stage: &str, binary: &str, args: &[String]) -> Result<Output, IdxError> {
|
||||
let output = Command::new(binary).args(args).output().map_err(|e| {
|
||||
let max_filesize = MAX_RESPONSE_BYTES.to_string();
|
||||
let output = Command::new(binary)
|
||||
.args(["--max-time", MAX_TIME_SECS, "--max-filesize", max_filesize.as_str()])
|
||||
.args(args)
|
||||
.output()
|
||||
.map_err(|e| {
|
||||
if e.kind() == std::io::ErrorKind::NotFound {
|
||||
return IdxError::Http(format!(
|
||||
"curl-impersonate binary '{binary}' not found; set {OVERRIDE_ENV} or install nixpkgs#curl-impersonate-chrome"
|
||||
|
|
@ -107,6 +118,12 @@ fn run_binary(stage: &str, binary: &str, args: &[String]) -> Result<Output, IdxE
|
|||
IdxError::Http(format!("failed to run {binary} for {stage}: {e}"))
|
||||
})?;
|
||||
|
||||
// --max-filesize cannot stop a response without a length; check here too.
|
||||
if output.stdout.len() > MAX_RESPONSE_BYTES {
|
||||
return Err(IdxError::Http(format!(
|
||||
"{stage} response exceeds {MAX_RESPONSE_BYTES} bytes"
|
||||
)));
|
||||
}
|
||||
if output.status.success() {
|
||||
return Ok(output);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -484,6 +484,14 @@ fn lock_path(db_path: &Path) -> PathBuf {
|
|||
/// not need write access, so readers fall back to opening an existing lock
|
||||
/// file read-only (e.g. one created by another user).
|
||||
fn lock_at(db_path: &Path, exclusive: bool) -> Result<fs::File, IdxError> {
|
||||
lock_at_within(db_path, exclusive, LOCK_WAIT)
|
||||
}
|
||||
|
||||
fn lock_at_within(
|
||||
db_path: &Path,
|
||||
exclusive: bool,
|
||||
wait: std::time::Duration,
|
||||
) -> Result<fs::File, IdxError> {
|
||||
let path = lock_path(db_path);
|
||||
let opened = fs::OpenOptions::new()
|
||||
.create(true)
|
||||
|
|
@ -496,15 +504,38 @@ fn lock_at(db_path: &Path, exclusive: bool) -> Result<fs::File, IdxError> {
|
|||
.map_err(|e| IdxError::Io(format!("failed to open ownership lock: {e}")))?,
|
||||
Err(e) => return Err(IdxError::Io(format!("failed to open ownership lock: {e}"))),
|
||||
};
|
||||
if exclusive {
|
||||
file.lock()
|
||||
// Wait a bounded time: a blocking lock behind a long sync or import (or
|
||||
// a stuck process) would hold every MCP call slot indefinitely.
|
||||
let deadline = std::time::Instant::now() + wait;
|
||||
loop {
|
||||
let attempt = if exclusive {
|
||||
file.try_lock()
|
||||
} else {
|
||||
file.lock_shared()
|
||||
file.try_lock_shared()
|
||||
};
|
||||
match attempt {
|
||||
Ok(()) => return Ok(file),
|
||||
Err(fs::TryLockError::WouldBlock) if std::time::Instant::now() < deadline => {
|
||||
std::thread::sleep(std::time::Duration::from_millis(100));
|
||||
}
|
||||
Err(fs::TryLockError::WouldBlock) => {
|
||||
return Err(IdxError::Io(format!(
|
||||
"ownership database is busy (another idx is syncing or importing); try again in a moment ({})",
|
||||
path.display()
|
||||
)));
|
||||
}
|
||||
Err(fs::TryLockError::Error(e)) => {
|
||||
return Err(IdxError::Io(format!(
|
||||
"failed to lock ownership database: {e}"
|
||||
)));
|
||||
}
|
||||
}
|
||||
}
|
||||
.map_err(|e| IdxError::Io(format!("failed to lock ownership database: {e}")))?;
|
||||
Ok(file)
|
||||
}
|
||||
|
||||
/// How long ownership commands wait for another process's lock.
|
||||
const LOCK_WAIT: std::time::Duration = std::time::Duration::from_secs(30);
|
||||
|
||||
/// True when the lock file is absent and cannot be created.
|
||||
fn unlockable(db_path: &Path) -> bool {
|
||||
let path = lock_path(db_path);
|
||||
|
|
@ -780,3 +811,21 @@ mod tests {
|
|||
assert!(parse_report_family("above10").is_err());
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod lock_tests {
|
||||
use std::time::Duration;
|
||||
|
||||
use super::lock_at_within;
|
||||
|
||||
#[test]
|
||||
fn a_held_lock_times_out_with_a_busy_error() {
|
||||
let db = std::env::temp_dir().join(format!("idx-lock-{}.db", fastrand::u64(..)));
|
||||
let _held = lock_at_within(&db, true, Duration::ZERO).unwrap();
|
||||
|
||||
let err = lock_at_within(&db, false, Duration::from_millis(200)).unwrap_err();
|
||||
|
||||
assert!(err.to_string().contains("busy"), "{err}");
|
||||
let _ = std::fs::remove_file(super::lock_path(&db));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue