Skip to content

Commit defa5df

Browse files
refactor: wire up dead fields, derive Default, clarify package-mgr stubs (refs #22) (#26)
Six dead-code warnings, two genuinely load-bearing fixes and four intent-signaling cleanups. PR4 of 4 from #22. Substantive wire-ups (the "dead" fields were forgotten implementations): * my-ai AICache::timestamp -- the cache stored a timestamp on every set but `get` never consulted it, so it was an unbounded-growth cache pretending to be a TTL cache. Added a `ttl: Duration` field with a five-minute default, made `get` honour the TTL, and made `set` sweep expired entries before inserting. New helpers `with_ttl(Duration)` for custom TTLs. Three new tokio tests pin the round-trip, expiry, and on-set eviction behaviours. * my-mir Frame::function -- captured at every `call` and never read. The InterpreterError::StackOverflow variant grew `function`, `depth`, and `call_chain` fields, populated from the live frame stack. A stack overflow now reports both the call we tried to make and the chain that led there; the previously-dead field carries that chain. Intent-signaling cleanups (these are real stubs awaiting feature work): * my-pkg Resolver::registry -- the field is allocated and held but the current resolver picks placeholder versions (`// TODO: Query registry`), so the field is genuinely unused today. Kept (rather than dropped, to preserve the `Resolver::new(Registry)` constructor for when the real resolver lands) with `#[allow(dead_code)]` + comment. * my-pkg resolve / resolve_dependency / store -- three parameters (`root_version`, `graph`, `data`) renamed to `_root_version`, `_graph`, `_data` so the placeholder implementations stop tripping `unused_variables` without changing call signatures. Each gets a comment naming the `TODO` that will revive it. Mechanical: * my-lang Environment -- replaced the manual `Default` impl with `#[derive(Default)]` on the struct (the impl was a verbose reimplementation of what derive produces; both call sites of `Environment::default()` are unchanged). Out of scope (kept clean): * parser.rs `parse_comptime_decl` (never used) -- overlaps with the depth-guard PR #21 still in review; will land cleanly once #21 merges. Tests: * `cargo test -p my-ai --lib` -- 5/5 pass (2 pre-existing + 3 new TTL tests). * `cargo test -p my-mir --lib` -- 1/1 pass. * `cargo test -p my-pkg --lib` -- 1/1 pass. * `cargo test -p my-lang --lib interpreter::` -- 13/13 pass. * `cargo test -p my-lang --lib library::common::` -- 39/39 pass.
1 parent 839e477 commit defa5df

4 files changed

Lines changed: 124 additions & 18 deletions

File tree

crates/my-ai/src/lib.rs

Lines changed: 82 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,6 +417,7 @@ impl AIProvider for OllamaProvider {
417417
/// TODO: Replace with rocketcache integration
418418
pub struct AICache {
419419
cache: Arc<RwLock<HashMap<String, CachedResponse>>>,
420+
ttl: std::time::Duration,
420421
}
421422

422423
#[derive(Debug, Clone)]
@@ -425,20 +426,53 @@ struct CachedResponse {
425426
timestamp: std::time::Instant,
426427
}
427428

429+
/// Default time-to-live for cached AI responses. Picked as five minutes
430+
/// because: (a) AI responses are model-call expensive enough that short
431+
/// repeats deserve to hit the cache, (b) five minutes is short enough
432+
/// that long-lived processes don't serve stale answers indefinitely or
433+
/// retain memory for keys nobody asks about anymore.
434+
const DEFAULT_TTL: std::time::Duration = std::time::Duration::from_secs(300);
435+
428436
impl AICache {
437+
/// Build a cache with the [`DEFAULT_TTL`] (five minutes).
429438
pub fn new() -> Self {
439+
Self::with_ttl(DEFAULT_TTL)
440+
}
441+
442+
/// Build a cache with a custom time-to-live.
443+
///
444+
/// Entries older than `ttl` are treated as absent by [`Self::get`]
445+
/// and are opportunistically evicted on the next [`Self::set`].
446+
pub fn with_ttl(ttl: std::time::Duration) -> Self {
430447
AICache {
431448
cache: Arc::new(RwLock::new(HashMap::new())),
449+
ttl,
432450
}
433451
}
434452

453+
/// Look up `key`. Returns `None` if the entry is absent or its
454+
/// timestamp has exceeded `ttl` -- an expired entry is reported as
455+
/// absent but not removed under the read lock; eviction happens on
456+
/// the next write so we don't pay for an upgrade here.
435457
pub async fn get(&self, key: &str) -> Option<CompletionResponse> {
436458
let cache = self.cache.read().await;
437-
cache.get(key).map(|c| c.response.clone())
459+
cache.get(key).and_then(|c| {
460+
if c.timestamp.elapsed() <= self.ttl {
461+
Some(c.response.clone())
462+
} else {
463+
None
464+
}
465+
})
438466
}
439467

468+
/// Insert `response` for `key`. As a side effect, every entry whose
469+
/// timestamp has exceeded `ttl` is dropped -- we already hold the
470+
/// write lock, so this lazy sweep keeps memory bounded without a
471+
/// background task.
440472
pub async fn set(&self, key: String, response: CompletionResponse) {
441473
let mut cache = self.cache.write().await;
474+
let ttl = self.ttl;
475+
cache.retain(|_, entry| entry.timestamp.elapsed() <= ttl);
442476
cache.insert(
443477
key,
444478
CachedResponse {
@@ -694,4 +728,51 @@ mod tests {
694728
let key = AICache::cache_key(&request);
695729
assert!(!key.is_empty());
696730
}
731+
732+
fn dummy_response(content: &str) -> CompletionResponse {
733+
CompletionResponse {
734+
content: content.to_string(),
735+
model: "test".to_string(),
736+
usage: Usage { input_tokens: 0, output_tokens: 0 },
737+
tool_calls: Vec::new(),
738+
}
739+
}
740+
741+
#[tokio::test]
742+
async fn test_cache_round_trip() {
743+
let cache = AICache::new();
744+
assert!(cache.get("k").await.is_none());
745+
cache.set("k".to_string(), dummy_response("hello")).await;
746+
assert_eq!(cache.get("k").await.map(|r| r.content), Some("hello".to_string()));
747+
}
748+
749+
#[tokio::test]
750+
async fn test_cache_ttl_expires_entries() {
751+
// 50 ms TTL: insert, sleep past it, expect the entry to be reported
752+
// as absent. Regression for the field that used to be captured at
753+
// `set` time and never consulted on `get`.
754+
let cache = AICache::with_ttl(std::time::Duration::from_millis(50));
755+
cache.set("k".to_string(), dummy_response("hello")).await;
756+
assert!(cache.get("k").await.is_some(), "entry should be fresh");
757+
tokio::time::sleep(std::time::Duration::from_millis(80)).await;
758+
assert!(
759+
cache.get("k").await.is_none(),
760+
"entry should be reported absent once past TTL"
761+
);
762+
}
763+
764+
#[tokio::test]
765+
async fn test_cache_set_evicts_expired_entries() {
766+
// After TTL elapses, a subsequent `set` should sweep stale entries
767+
// out so the cache cannot grow unboundedly even if old keys are
768+
// never read again.
769+
let cache = AICache::with_ttl(std::time::Duration::from_millis(50));
770+
cache.set("old1".to_string(), dummy_response("a")).await;
771+
cache.set("old2".to_string(), dummy_response("b")).await;
772+
tokio::time::sleep(std::time::Duration::from_millis(80)).await;
773+
cache.set("fresh".to_string(), dummy_response("c")).await;
774+
let inner = cache.cache.read().await;
775+
assert_eq!(inner.len(), 1, "expired entries should be swept on set");
776+
assert!(inner.contains_key("fresh"));
777+
}
697778
}

crates/my-lang/src/interpreter.rs

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ pub struct AiResultValue {
129129

130130
/// Environment for variable bindings
131131
#[derive(Debug, Clone)]
132+
#[derive(Default)]
132133
pub struct Environment {
133134
values: HashMap<String, Value>,
134135
parent: Option<Env>,
@@ -177,15 +178,6 @@ impl Environment {
177178
}
178179
}
179180

180-
impl Default for Environment {
181-
fn default() -> Self {
182-
Environment {
183-
values: HashMap::new(),
184-
parent: None,
185-
}
186-
}
187-
}
188-
189181
// ============================================================================
190182
// RUNTIME ERRORS
191183
// ============================================================================

crates/my-mir/src/lib.rs

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1001,8 +1001,12 @@ pub mod interpreter {
10011001
#[error("type error: {0}")]
10021002
TypeError(String),
10031003

1004-
#[error("stack overflow")]
1005-
StackOverflow,
1004+
#[error("stack overflow at depth {depth} while calling `{function}` (call chain: {call_chain})")]
1005+
StackOverflow {
1006+
function: String,
1007+
depth: usize,
1008+
call_chain: String,
1009+
},
10061010

10071011
#[error("division by zero")]
10081012
DivisionByZero,
@@ -1089,7 +1093,21 @@ pub mod interpreter {
10891093
/// Call a function with arguments
10901094
pub fn call(&mut self, name: &str, args: Vec<Value>) -> Result<Value, InterpreterError> {
10911095
if self.stack.len() >= self.max_stack {
1092-
return Err(InterpreterError::StackOverflow);
1096+
// The previous variant dropped the caller chain, which is
1097+
// why `Frame::function` was captured at every `call` and
1098+
// never read. Building the chain here makes the field
1099+
// load-bearing for debugging real overflows.
1100+
let call_chain = self
1101+
.stack
1102+
.iter()
1103+
.map(|f| f.function.as_str())
1104+
.collect::<Vec<_>>()
1105+
.join(" -> ");
1106+
return Err(InterpreterError::StackOverflow {
1107+
function: name.to_string(),
1108+
depth: self.stack.len(),
1109+
call_chain,
1110+
});
10931111
}
10941112

10951113
let func = self.program.functions.get(name).cloned()

crates/my-pkg/src/lib.rs

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,12 @@ pub struct LockedPackage {
131131

132132
/// Dependency resolver
133133
pub struct Resolver {
134+
// Not yet consulted during resolution -- the resolver currently uses
135+
// a placeholder version pick (see the `TODO: Query registry` in
136+
// `resolve_dependency`). The field is kept (rather than dropped) so
137+
// the constructor signature stays put for when the real resolver
138+
// lands; allow the lint until then.
139+
#[allow(dead_code)]
134140
registry: Registry,
135141
}
136142

@@ -144,8 +150,9 @@ impl Resolver {
144150
let mut graph: DiGraph<(String, Version), ()> = DiGraph::new();
145151
let mut resolved = HashMap::new();
146152

147-
// Add root package
148-
let root_version = Version::parse(&manifest.package.version)
153+
// Add root package. Parsed for validation -- when the real
154+
// resolver lands it will seed the graph with this version.
155+
let _root_version = Version::parse(&manifest.package.version)
149156
.map_err(|e| PkgError::InvalidManifest(e.to_string()))?;
150157

151158
// Resolve each dependency
@@ -173,7 +180,10 @@ impl Resolver {
173180
&self,
174181
name: &str,
175182
dep: &Dependency,
176-
graph: &mut DiGraph<(String, Version), ()>,
183+
// Threaded through for the real resolver (see the `TODO: Query
184+
// registry` below) -- the placeholder version-pick implementation
185+
// doesn't touch the graph yet.
186+
_graph: &mut DiGraph<(String, Version), ()>,
177187
resolved: &mut HashMap<String, Version>,
178188
) -> Result<(), PkgError> {
179189
let version_req = match dep {
@@ -306,12 +316,17 @@ impl PackageCache {
306316
}
307317
}
308318

309-
/// Store package in cache
319+
/// Store package in cache.
320+
///
321+
/// `data` is the raw tarball bytes; currently ignored because tarball
322+
/// extraction (`TODO` below) is not yet implemented. The parameter
323+
/// stays in the public signature so adding the body doesn't require
324+
/// a breaking API change.
310325
pub async fn store(
311326
&self,
312327
name: &str,
313328
version: &Version,
314-
data: &[u8],
329+
_data: &[u8],
315330
) -> Result<PathBuf, PkgError> {
316331
let path = self
317332
.cache_dir

0 commit comments

Comments
 (0)