Skip to content

Commit 2057ccc

Browse files
author
Anonymous
committed
Fix a cache leak, a coarse watcher, and two claims that were not true
support/cache.rs never added a key to its LRU order — `touch` only moved keys it already had — so MAX_PER_PROVIDER evicted nothing and the map grew one entry per distinct query per provider for the life of the daemon. Five tests pin the bound now, and the leak is proven: without the fix the new bound test holds 48 entries where it should hold 16. oxyd/watch.rs compared mtimes in whole seconds, so a same-length edit saved twice inside one second looked like no change at all — a settings save that silently does nothing. Milliseconds now, in both signatures. provider/process.rs's `run_sync` and its `shell_command_sync` helper were dead (every probe goes through the async `check`), and `run_sync` would have deadlocked on a child writing more than a pipe buffer, since nothing drained stdout until the child had exited. Removed. Two comments claimed more than the code does: frecency's boost is added to the finished score, after the tier clamp, so a row within 9000 of its ceiling can cross into the next tier — reachable (a substring row at the top fuzzy band outranks a prefix row at local 0) and the script adds it the same way, so it is parity, not a port bug. Pins really are clamped, by contrast. Comments and the porting doc now say exactly that. Also: availability's recheck subtracted two wall-clock readings without saturating (a backwards clock jump panics a debug build), and logfile's rotation gained three tests — including the second rotation, which has to replace .old rather than fail.
1 parent 1dcf50e commit 2057ccc

8 files changed

Lines changed: 156 additions & 52 deletions

File tree

‎core/crates/oxy-core/src/provider/process.rs‎

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -82,17 +82,6 @@ fn shell_command(body: &str) -> Command {
8282
cmd
8383
}
8484

85-
/// The same environment for the blocking `std::process::Command` callers.
86-
fn shell_command_sync(body: &str) -> std::process::Command {
87-
let mut cmd = std::process::Command::new("bash");
88-
cmd.arg("-c").arg(body);
89-
if let Some(env) = &*LOGIN_ENV.read().unwrap() {
90-
cmd.envs(env.iter().map(|(k, v)| (k.as_str(), v.as_str())));
91-
}
92-
cmd.env("OXY_PLUGIN_ID", &*PLUGIN_ID);
93-
cmd
94-
}
95-
9685
/// What a run left behind: stdout, the exit code when the wait returned one,
9786
/// and whether the deadline killed it — the `code`/`timeout` fields the
9887
/// event log reports, which `Option<String>` could not carry.
@@ -183,35 +172,6 @@ pub async fn check(body: &str) -> bool {
183172
}
184173
}
185174

186-
/// Command for the availability probe at load and for `oxy test` preflight.
187-
pub fn run_sync(body: &str, timeout: Duration) -> Option<String> {
188-
let child = shell_command_sync(body)
189-
.stdin(Stdio::null())
190-
.stdout(Stdio::piped())
191-
.stderr(Stdio::null())
192-
.spawn()
193-
.ok()?;
194-
let start = std::time::Instant::now();
195-
let mut out = child;
196-
loop {
197-
match out.try_wait() {
198-
Ok(Some(_)) => {
199-
let mut buf = Vec::new();
200-
use std::io::Read;
201-
out.stdout.take()?.read_to_end(&mut buf).ok()?;
202-
return Some(String::from_utf8_lossy(&buf).into_owned());
203-
}
204-
Ok(None) if start.elapsed() < timeout => {
205-
std::thread::sleep(Duration::from_millis(5));
206-
}
207-
_ => {
208-
let _ = out.kill();
209-
return None;
210-
}
211-
}
212-
}
213-
}
214-
215175
/// Parse a script's stdout into rows: a JSON array, or one object per line.
216176
pub fn parse(text: &str) -> Vec<Value> {
217177
crate::model::row::parse_rows(text)

‎core/crates/oxy-core/src/state/frecency.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,14 @@ use crate::model::row::Row;
1212
/// Half-life in days: a launch is worth half as much a week after you made it.
1313
const HALF_LIFE_DAYS: f64 = 7.0;
1414
const DAY_MS: f64 = 86_400_000.0;
15-
/// The most a boost can move a row inside its tier. It never crosses one.
15+
/// The most a boost adds. Unlike a pin — which is clamped to the row's own
16+
/// tier in `pins_apply` — this is added to the *finished* score, after the
17+
/// tier clamp in `rank::score`. So a row already within 9000 of its tier's
18+
/// ceiling can edge past the next tier's floor: a substring row at the top
19+
/// fuzzy band (local 91 646) plus a full boost outranks a prefix row at
20+
/// local 0, and a used prefix row can outrank a `forced` one. The script's
21+
/// `Frecency.js` adds it exactly the same way, so this is parity — changing
22+
/// it means changing both builds, not fixing a port.
1623
const MAX_BOOST: i64 = 9000;
1724

1825
#[derive(Debug, Clone, Default, Serialize, Deserialize)]

‎core/crates/oxy-core/src/support/availability.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ impl Availability {
3939
match self.store.get(check) {
4040
None => None,
4141
Some(e) if e.ok => Some(true),
42-
Some(e) if now_ms() - e.at < RECHECK_MS => Some(false),
42+
Some(e) if now_ms().saturating_sub(e.at) < RECHECK_MS => Some(false),
4343
Some(_) => None,
4444
}
4545
}

‎core/crates/oxy-core/src/support/cache.rs‎

Lines changed: 81 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,18 @@ struct Bucket {
5252
}
5353

5454
impl Bucket {
55+
/// Move a key to the front of the LRU order, adding it if it is new. A
56+
/// key reaches `entries` only through `put`, so the first touch of a key
57+
/// is what puts it in the order — without that, eviction never sees it
58+
/// and the map grows past `MAX_PER_PROVIDER` for the life of the daemon.
5559
fn touch(&mut self, key: &str) {
56-
if let Some(at) = self.keys.iter().position(|k| k == key)
57-
&& at > 0
58-
{
59-
let key = self.keys.remove(at).unwrap();
60-
self.keys.push_front(key);
60+
match self.keys.iter().position(|k| k == key) {
61+
Some(0) => {}
62+
Some(at) => {
63+
let key = self.keys.remove(at).unwrap();
64+
self.keys.push_front(key);
65+
}
66+
None => self.keys.push_front(key.to_string()),
6167
}
6268
}
6369
}
@@ -148,3 +154,73 @@ impl Cache {
148154
}
149155
}
150156
}
157+
158+
#[cfg(test)]
159+
mod tests {
160+
use super::*;
161+
use std::sync::Arc;
162+
163+
fn rows(n: usize) -> SharedRows {
164+
Arc::new(
165+
(0..n)
166+
.map(|i| Arc::new(crate::model::row::Row::new(format!("k{i}"), "t")))
167+
.collect(),
168+
)
169+
}
170+
171+
/// The bound the module promises: a daemon up for days must not hold one
172+
/// entry per query it has ever seen.
173+
#[test]
174+
fn a_provider_holds_at_most_max_per_provider_answers() {
175+
let mut cache = Cache::default();
176+
for i in 0..(MAX_PER_PROVIDER * 3) {
177+
cache.put("gh", &format!("q{i}"), rows(1), 60_000, 7);
178+
}
179+
assert_eq!(cache.entries(), MAX_PER_PROVIDER);
180+
// The most recent ones are the ones kept: the oldest was evicted.
181+
assert!(cache.get("gh", "q0", 7).is_none());
182+
let newest = format!("q{}", MAX_PER_PROVIDER * 3 - 1);
183+
assert!(cache.get("gh", &newest, 7).is_some());
184+
}
185+
186+
#[test]
187+
fn a_hit_refreshes_the_lru_order() {
188+
let mut cache = Cache::default();
189+
for i in 0..MAX_PER_PROVIDER {
190+
cache.put("gh", &format!("q{i}"), rows(1), 60_000, 7);
191+
}
192+
// Touch the oldest, then add one: the touched key survives, the
193+
// second-oldest is the one evicted.
194+
assert!(cache.get("gh", "q0", 7).is_some());
195+
cache.put("gh", "fresh", rows(1), 60_000, 7);
196+
assert!(cache.get("gh", "q0", 7).is_some(), "q0 was refreshed");
197+
assert!(cache.get("gh", "q1", 7).is_none(), "q1 was the coldest");
198+
}
199+
200+
#[test]
201+
fn ttl_zero_stores_nothing() {
202+
let mut cache = Cache::default();
203+
cache.put("vol", "q", rows(2), 0, 7);
204+
assert_eq!(cache.entries(), 0);
205+
}
206+
207+
#[test]
208+
fn a_stamp_mismatch_is_a_miss() {
209+
let mut cache = Cache::default();
210+
cache.put("gh", "q", rows(1), 60_000, 7);
211+
assert!(cache.get("gh", "q", 7).is_some());
212+
assert!(cache.get("gh", "q", 8).is_none());
213+
// The stale read still serves it, which is what keeps a reload from
214+
// blanking the card while the new definition answers.
215+
assert!(cache.get_stale("gh", "q", 7).is_some());
216+
}
217+
218+
#[test]
219+
fn providers_do_not_share_keys() {
220+
let mut cache = Cache::default();
221+
cache.put("gh", "same", rows(3), 60_000, 7);
222+
cache.put("pr", "same", rows(1), 60_000, 7);
223+
assert_eq!(cache.get("gh", "same", 7).unwrap().len(), 3);
224+
assert_eq!(cache.get("pr", "same", 7).unwrap().len(), 1);
225+
}
226+
}

‎core/crates/oxy-core/src/support/rank.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@
44
//! The tier is the kind of match, and it always wins. `local` breaks ties
55
//! inside a tier and is where a provider's own bias lives, so a bias can
66
//! reorder equals but can never lift a weak match above a strong one.
7+
//!
8+
//! Two things add to the score after that clamp, and only one of them keeps
9+
//! the rule: a pin is clamped to the row's own tier (`pins_apply`), while
10+
//! frecency is added to the finished number (`frecency_apply`) and can carry
11+
//! a row within 9000 of its ceiling past the next tier's floor. The script
12+
//! does both the same way, so the difference is parity, not a port bug.
713
814
use std::sync::Arc;
915

‎core/crates/oxyd/src/logfile.rs‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,3 +60,55 @@ pub(crate) fn append_log(path: &Path, line: &str) {
6060
let _ = f.write_all(line.as_bytes());
6161
}
6262
}
63+
64+
#[cfg(test)]
65+
mod tests {
66+
use super::*;
67+
68+
/// Two generations, and the second rotation has to *replace* `.old`
69+
/// rather than fail: on Windows a rename onto an existing file is the
70+
/// case worth checking, and this runs on both platforms.
71+
#[test]
72+
fn rotation_keeps_two_generations() {
73+
let dir = std::env::temp_dir().join(format!("oxyd-log-test-{}", std::process::id()));
74+
std::fs::create_dir_all(&dir).unwrap();
75+
let path = dir.join("oxy.log");
76+
let old = dir.join("oxy.log.old");
77+
78+
std::fs::write(&path, vec![b'x'; 1_048_577]).unwrap();
79+
append_log(&path, "one\n");
80+
assert!(old.exists(), "the first rotation writes .old");
81+
assert_eq!(std::fs::read_to_string(&path).unwrap(), "one\n");
82+
83+
std::fs::write(&path, vec![b'y'; 1_048_577]).unwrap();
84+
append_log(&path, "two\n");
85+
assert_eq!(std::fs::read_to_string(&path).unwrap(), "two\n");
86+
let kept = std::fs::read_to_string(&old).unwrap();
87+
assert!(kept.starts_with('y'), "the second rotation replaced .old");
88+
89+
std::fs::remove_dir_all(&dir).ok();
90+
}
91+
92+
#[test]
93+
fn a_small_file_is_never_rotated() {
94+
let dir = std::env::temp_dir().join(format!("oxyd-log-small-{}", std::process::id()));
95+
std::fs::create_dir_all(&dir).unwrap();
96+
let path = dir.join("oxy.log");
97+
append_log(&path, "first\n");
98+
append_log(&path, "second\n");
99+
assert_eq!(std::fs::read_to_string(&path).unwrap(), "first\nsecond\n");
100+
assert!(!dir.join("oxy.log.old").exists());
101+
std::fs::remove_dir_all(&dir).ok();
102+
}
103+
104+
#[test]
105+
fn a_line_carries_ts_sid_and_ev() {
106+
let line = log_line("abc", "prov.done", &json!({"ms": 12}));
107+
assert!(line.ends_with('\n'));
108+
let v: Value = serde_json::from_str(line.trim()).unwrap();
109+
assert_eq!(v["sid"], "abc");
110+
assert_eq!(v["ev"], "prov.done");
111+
assert_eq!(v["ms"], 12);
112+
assert!(v["ts"].as_u64().unwrap() > 0);
113+
}
114+
}

‎core/crates/oxyd/src/watch.rs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ pub(crate) fn spawn(
3434

3535
/// A single file's change signature — len + mtime, so an edit or a delete
3636
/// (None metadata → 0) both register. Same fixed-key hasher as `signature`.
37+
/// The mtime is in **milliseconds**: seconds would miss a same-length edit
38+
/// saved twice inside one second, which is a save that changes a setting
39+
/// and looks like nothing happened.
3740
fn file_signature(path: &Path) -> u64 {
3841
use std::hash::Hasher;
3942
let mut h = std::collections::hash_map::DefaultHasher::new();
@@ -44,7 +47,7 @@ fn file_signature(path: &Path) -> u64 {
4447
meta.modified()
4548
.ok()
4649
.and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok())
47-
.map(|d| d.as_secs())
50+
.map(|d| d.as_millis() as u64)
4851
.unwrap_or(0),
4952
);
5053
}
@@ -78,7 +81,7 @@ fn signature(dir: &Path) -> u64 {
7881
meta.modified()
7982
.ok()
8083
.and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok())
81-
.map(|d| d.as_secs())
84+
.map(|d| d.as_millis() as u64)
8285
.unwrap_or(0),
8386
);
8487
acc ^= h.finish();

‎docs/PORTING-BACKLOG.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ starts from a measured state:
4343
| fact | value |
4444
|---|---|
4545
| Rust files | 100 (37 before the restructure; +3 for `provider/llm/`, +8 for the batch A scaffold) |
46-
| total lines | 16 562 |
46+
| total lines | 16 666 |
4747
| largest file | 600 lines (`provider/worker/state.rs`); nothing above the 800 target |
48-
| tests | 62 passing (`cargo test --workspace`): 47 engine, 13 LLM, 2 for `util::on_path` |
48+
| tests | 70 passing (`cargo test --workspace`): 47 engine, 13 LLM, 2 `util::on_path`, 5 cache, 3 logfile |
4949
| lints | `cargo clippy --workspace --all-targets -- -D warnings` clean; `cargo fmt --check` clean |
5050
| guards | the file budget and the `model/` layering check in `tests/run.sh`; clippy in CI; `core/.loc-allow` names the three exempt data tables; `core/README.md` is the crate map |
5151
| extensions | 40 (15 native, 30 script-backed, 27 scripts) — the marketplace was removed |
@@ -1003,7 +1003,7 @@ header, and is the kind of thing a clean rewrite quietly loses.
10031003

10041004
| rule | where it comes from | what breaks if a port ignores it |
10051005
|---|---|---|
1006-
| A tier is never crossed: pins and frecency reorder *within* a tier only | `support/rank.rs` | a pinned substring outranks a name that starts with what you typed |
1006+
| A **pin** is clamped to its own tier; **frecency** is added to the finished score and is *not* — a heavily-used row near its tier's ceiling can cross into the next band (the script adds it the same way, so this is parity, not a port bug) | `state/pins.rs`, `state/frecency.rs`, `support/rank.rs` | a pinned substring never outranks a name that starts with what you typed; a frecency-boosted one can, in principle, outrank even a `forced` row |
10071007
| The web row is dropped whenever anything real matched; a scoped `?` keeps it | `support/rank.rs` (`merge`) | the fallback row buries the answer, or disappears from `web:` |
10081008
| An empty answer over rows already on screen keeps those rows | `worker/state.rs` (`keep_stale`) | a timeout blanks the card (looks like "no matches") |
10091009
| A nonzero exit that printed rows is not a failure — only a bad exit with *no* rows is | `prov.fail` | a script that warns on stderr stops answering |

0 commit comments

Comments
 (0)