Skip to content

Commit 28cd8da

Browse files
NathanFlurryclaude
andcommitted
fix: plug twelve runtime memory leaks across sidecar, kernel, VFS, V8
A leak audit of the VM runtime found tracking maps/registries populated on create but released only on a happy-path teardown (skipped on error '?' or never), plus per-timer threads with no cancellation. Each fix releases on every termination path; each ships a fast safeguard-firing test. sidecar: - dispose_vm_internal removes per-VM tracking on every exit path (VM removed before the fallible teardown half; cleanup runs unconditionally, error surfaced after) and the dispose_session/remove_connection loops attempt every item and aggregate errors instead of '?'-ing out (H1). - extension_process_output_buffers cleared on VM disposal, not just on successful handoff (M6). - disposed sessions are now untracked from the stdio active_sessions set (M5). - loopback-TLS endpoints remove their own registry entry on Drop instead of relying on the lazy retain() sweep (L4). - new additive Extension::on_session_disposed hook, fired on DisposeReason::ConnectionClosed, so extensions (e.g. ACP) can free per-session state on client disconnect (enables agent-os H4). sidecar-browser: vms/contexts maps cleared on every dispose path (H1). execution: bridge/kernel timer threads are delay-capped and cancellable via a generation check + clear-on-teardown, so guest timers can't exhaust OS threads or outlive their session (H2, M3). v8-runtime: VM_CONTEXTS slots evicted on context finalize, not only on error, so reused isolates don't hit MAX_VM_CONTEXTS (M1); pending promise-resolver Globals reset before the isolate is dropped on every run_event_loop exit (Shutdown/abort), fixing the leak and a V8 lifetime-contract violation (M2). kernel: socket ids allocated only after the backlog check passes, so a full-backlog connect failure no longer consumes an id (M4). vfs: rename rolls back staged snapshot entries on copy/rename failure (L2); InMemoryMetadataStore gains delete_snapshot() and a real gc() that reclaims unreferenced blocks (L3). L1 (bounded, documented Box::into_raw isolate handle) intentionally left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 3d18ea0 commit 28cd8da

14 files changed

Lines changed: 1949 additions & 153 deletions

File tree

‎crates/execution/src/javascript.rs‎

Lines changed: 253 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -493,6 +493,23 @@ struct LocalBridgeState {
493493
module_reader: Option<Box<dyn ModuleFsReader + Send>>,
494494
}
495495

496+
impl Drop for LocalBridgeState {
497+
/// Tear down all tracked timers when the bridge state is dropped (which
498+
/// happens when the event-bridge service loop exits on session termination —
499+
/// success, error, or shutdown). Clearing the shared `timers` map cancels both
500+
/// kernel and bridge timers: any in-flight timer thread that wakes afterwards
501+
/// finds its entry gone and suppresses its callback via `timer_should_fire`,
502+
/// so a destroyed session's timers do not fire and their thread stops touching
503+
/// the session. Without this, kernel-timer threads (M3) and bridge-timer
504+
/// threads (H2) would outlive the session, holding a session `Arc` and firing
505+
/// after the fact.
506+
fn drop(&mut self) {
507+
if let Ok(mut timers) = self.timers.lock() {
508+
timers.clear();
509+
}
510+
}
511+
}
512+
496513
#[derive(Debug, Default)]
497514
struct LocalKernelStdinBridge {
498515
state: Mutex<LocalKernelStdinState>,
@@ -572,6 +589,14 @@ enum LocalBridgeCallResult {
572589
Deferred,
573590
}
574591

592+
/// Upper bound on guest-supplied timer delays, matching the JS `TIMEOUT_MAX`
593+
/// ceiling (`2**31 - 1` ms, ~24.8 days). Guest code can pass a delay up to
594+
/// `u64::MAX` ms; without a cap each `_scheduleTimer` / `kernelTimerCreate` call
595+
/// spawns a thread that sleeps for an effectively unbounded duration while
596+
/// pinning a clone of the session `Arc`, blocking session cleanup. Clamping the
597+
/// delay bounds how long a timer thread can outlive its session.
598+
const MAX_TIMER_DELAY_MS: u64 = 2_147_483_647;
599+
575600
fn timer_delay_ms(value: Option<&Value>) -> u64 {
576601
let delay = match value {
577602
Some(Value::Number(number)) => number.as_f64().unwrap_or(0.0),
@@ -582,10 +607,40 @@ fn timer_delay_ms(value: Option<&Value>) -> u64 {
582607
if !delay.is_finite() || delay <= 0.0 {
583608
0
584609
} else {
585-
delay.floor().min(u64::MAX as f64) as u64
610+
delay.floor().min(MAX_TIMER_DELAY_MS as f64) as u64
586611
}
587612
}
588613

614+
/// Decide whether a woken timer thread should fire, and reclaim its tracking
615+
/// entry. Returns `false` (suppressing the callback) when the timer is gone from
616+
/// the map (cleared, or wiped on session teardown) or its generation no longer
617+
/// matches the one captured at scheduling time (re-armed/cancelled). A one-shot
618+
/// (`repeat == false`) timer that does fire is removed from the map so its id is
619+
/// reclaimed. Shared by the kernel-timer and bridge-timer paths so both honor the
620+
/// same cancellation semantics.
621+
fn timer_should_fire(
622+
timers: &Arc<Mutex<HashMap<u64, LocalTimerEntry>>>,
623+
timer_id: u64,
624+
generation: u64,
625+
) -> bool {
626+
timers
627+
.lock()
628+
.ok()
629+
.and_then(|mut timers| {
630+
let (current_generation, repeat) = timers
631+
.get(&timer_id)
632+
.map(|entry| (entry.generation, entry.repeat))?;
633+
if current_generation != generation {
634+
return Some(false);
635+
}
636+
if !repeat {
637+
timers.remove(&timer_id);
638+
}
639+
Some(true)
640+
})
641+
.unwrap_or(false)
642+
}
643+
589644
impl GuestPathTranslator {
590645
fn from_host_context(
591646
env: &BTreeMap<String, String>,
@@ -917,17 +972,17 @@ impl ModuleResolutionTestHarness {
917972
];
918973
sort_guest_path_mappings(&mut mappings);
919974

920-
Self {
921-
local_bridge: LocalBridgeState {
922-
translator: GuestPathTranslator {
923-
implicit_guest_cwd: String::from("/root"),
924-
implicit_host_cwd: host_root,
925-
sandbox_root: None,
926-
mappings,
927-
},
928-
..LocalBridgeState::default()
929-
},
930-
}
975+
// Build via default + in-place assignment rather than `..default()`:
976+
// LocalBridgeState implements Drop (to cancel timers on session teardown),
977+
// and functional-record-update would move fields out of a Drop type (E0509).
978+
let mut local_bridge = LocalBridgeState::default();
979+
local_bridge.translator = GuestPathTranslator {
980+
implicit_guest_cwd: String::from("/root"),
981+
implicit_host_cwd: host_root,
982+
sandbox_root: None,
983+
mappings,
984+
};
985+
Self { local_bridge }
931986
}
932987

933988
pub fn resolve_import(&mut self, specifier: &str, from_path: &str) -> Option<String> {
@@ -949,14 +1004,11 @@ pub fn handle_internal_bridge_call_from_host_context(
9491004
method: &str,
9501005
args: &[Value],
9511006
) -> Option<Value> {
952-
let mut local_bridge = LocalBridgeState {
953-
translator: GuestPathTranslator::from_host_context(
954-
env,
955-
host_cwd.to_path_buf(),
956-
guest_cwd.to_owned(),
957-
),
958-
..LocalBridgeState::default()
959-
};
1007+
// default + in-place assign: LocalBridgeState is Drop, so `..default()` (E0509)
1008+
// is not allowed.
1009+
let mut local_bridge = LocalBridgeState::default();
1010+
local_bridge.translator =
1011+
GuestPathTranslator::from_host_context(env, host_cwd.to_path_buf(), guest_cwd.to_owned());
9601012

9611013
match local_bridge.handle_internal_bridge_call(0, method, args) {
9621014
Some(LocalBridgeCallResult::Immediate(value)) => Some(value),
@@ -1828,19 +1880,20 @@ impl JavascriptExecutionEngine {
18281880
let pending_sync_rpc = Arc::new(Mutex::new(None));
18291881
let kernel_stdin = Arc::new(LocalKernelStdinBridge::default());
18301882
let standalone_translator = translator.clone();
1883+
// default + in-place assign: LocalBridgeState is Drop, so `..Default::default()`
1884+
// (E0509) is not allowed.
1885+
let mut local_bridge = LocalBridgeState::default();
1886+
local_bridge.translator = translator;
1887+
local_bridge.kernel_stdin = kernel_stdin.clone();
1888+
local_bridge.v8_session = Some(v8_session.clone());
1889+
local_bridge.module_reader = module_reader;
1890+
local_bridge.module_resolution = GuestModuleResolution::from_env(&request.env);
18311891
let events = spawn_v8_event_bridge(
18321892
frame_receiver,
18331893
pending_sync_rpc.clone(),
18341894
sync_rpc_timeout,
18351895
v8_session.clone(),
1836-
LocalBridgeState {
1837-
translator,
1838-
kernel_stdin: kernel_stdin.clone(),
1839-
v8_session: Some(v8_session.clone()),
1840-
module_reader,
1841-
module_resolution: GuestModuleResolution::from_env(&request.env),
1842-
..Default::default()
1843-
},
1896+
local_bridge,
18441897
);
18451898

18461899
// Install the direct module reader on the session thread BEFORE the Execute
@@ -3097,6 +3150,25 @@ impl LocalBridgeState {
30973150
self.next_timer_id
30983151
}
30993152

3153+
/// Allocate a fresh timer id and register a one-shot (`repeat == false`)
3154+
/// tracking entry at generation 0. Used by the bridge-timer path so the
3155+
/// spawned thread can be cancelled (its entry removed) on `clear`/teardown.
3156+
fn register_oneshot_timer(&mut self, delay_ms: u64) -> u64 {
3157+
self.next_timer_id += 1;
3158+
let timer_id = self.next_timer_id;
3159+
if let Ok(mut timers) = self.timers.lock() {
3160+
timers.insert(
3161+
timer_id,
3162+
LocalTimerEntry {
3163+
delay_ms,
3164+
generation: 0,
3165+
repeat: false,
3166+
},
3167+
);
3168+
}
3169+
timer_id
3170+
}
3171+
31003172
fn arm_kernel_timer(&self, timer_id: u64) {
31013173
let Some(session) = self.v8_session.clone() else {
31023174
return;
@@ -3117,23 +3189,7 @@ impl LocalBridgeState {
31173189
thread::sleep(Duration::from_millis(delay_ms));
31183190
}
31193191

3120-
let should_fire = timers
3121-
.lock()
3122-
.ok()
3123-
.and_then(|mut timers| {
3124-
let (current_generation, repeat) = timers
3125-
.get(&timer_id)
3126-
.map(|entry| (entry.generation, entry.repeat))?;
3127-
if current_generation != generation {
3128-
return Some(false);
3129-
}
3130-
if !repeat {
3131-
timers.remove(&timer_id);
3132-
}
3133-
Some(true)
3134-
})
3135-
.unwrap_or(false);
3136-
if !should_fire {
3192+
if !timer_should_fire(&timers, timer_id, generation) {
31373193
return;
31383194
}
31393195

@@ -3148,15 +3204,31 @@ impl LocalBridgeState {
31483204
}
31493205
}
31503206

3151-
fn schedule_bridge_timer_response(&self, call_id: u64, delay_ms: u64) {
3207+
fn schedule_bridge_timer_response(&mut self, call_id: u64, delay_ms: u64) {
31523208
let Some(session) = self.v8_session.clone() else {
31533209
return;
31543210
};
31553211

3212+
// Register the bridge timer in the shared `timers` map with a generation,
3213+
// mirroring the kernel-timer cancellation path. Previously this spawned a
3214+
// bare, untracked thread holding the session `Arc` with no cancellation and
3215+
// no generation check, so a cleared/destroyed session's timer would still
3216+
// fire (and pin the session) after the full delay. Tracking it means that
3217+
// when `LocalBridgeState` is dropped on session teardown (which clears the
3218+
// map) or the entry is otherwise removed, the woken thread observes the
3219+
// missing/mismatched generation via `timer_should_fire` and suppresses the
3220+
// response instead of touching the torn-down session.
3221+
let timer_id = self.register_oneshot_timer(delay_ms);
3222+
let generation = 0;
3223+
let timers = self.timers.clone();
3224+
31563225
thread::spawn(move || {
31573226
if delay_ms > 0 {
31583227
thread::sleep(Duration::from_millis(delay_ms));
31593228
}
3229+
if !timer_should_fire(&timers, timer_id, generation) {
3230+
return;
3231+
}
31603232
let _ = session.send_bridge_response(call_id, 0, Vec::new());
31613233
});
31623234
}
@@ -6908,4 +6980,139 @@ mod tests {
69086980
drop(receiver);
69096981
host.unregister_session(&session_id);
69106982
}
6983+
6984+
// --- Timer cancellation / cap regression tests (U4: H2 bridge timers, M3
6985+
// kernel timers). These assert the *safeguards firing* (delay clamped, timer
6986+
// entry reclaimed, callback suppressed) and never spawn unbounded threads. ---
6987+
6988+
#[test]
6989+
fn timer_delay_is_clamped_to_the_cap() {
6990+
// A guest can pass an arbitrarily large delay (up to u64::MAX ms); without
6991+
// a cap each scheduled timer spawns a thread that sleeps essentially
6992+
// forever while pinning the session Arc. The cap bounds that lifetime.
6993+
assert_eq!(
6994+
timer_delay_ms(Some(&json!(u64::MAX))),
6995+
MAX_TIMER_DELAY_MS,
6996+
"a u64::MAX delay must be clamped to MAX_TIMER_DELAY_MS"
6997+
);
6998+
assert_eq!(
6999+
timer_delay_ms(Some(&json!(1.0e308_f64))),
7000+
MAX_TIMER_DELAY_MS,
7001+
"an enormous float delay must be clamped to the cap"
7002+
);
7003+
assert_eq!(
7004+
timer_delay_ms(Some(&json!(MAX_TIMER_DELAY_MS + 1))),
7005+
MAX_TIMER_DELAY_MS,
7006+
"a delay one past the cap must clamp down to the cap"
7007+
);
7008+
// Below-cap values pass through unchanged so normal timers are unaffected.
7009+
assert_eq!(timer_delay_ms(Some(&json!(250))), 250);
7010+
assert_eq!(timer_delay_ms(Some(&json!(0))), 0);
7011+
}
7012+
7013+
#[test]
7014+
fn cleared_timer_is_suppressed_and_entry_reclaimed() {
7015+
// Mirrors what a woken bridge/kernel timer thread does after sleeping: it
7016+
// consults the shared map via `timer_should_fire`. When the entry has been
7017+
// removed (clear or session teardown), the callback must be suppressed.
7018+
let timers: Arc<Mutex<HashMap<u64, LocalTimerEntry>>> =
7019+
Arc::new(Mutex::new(HashMap::new()));
7020+
timers.lock().unwrap().insert(
7021+
7,
7022+
LocalTimerEntry {
7023+
delay_ms: 1_000,
7024+
generation: 0,
7025+
repeat: false,
7026+
},
7027+
);
7028+
7029+
// Simulate `kernelTimerClear` / teardown removing the entry before the
7030+
// thread wakes.
7031+
timers.lock().unwrap().remove(&7);
7032+
7033+
assert!(
7034+
!timer_should_fire(&timers, 7, 0),
7035+
"a cleared timer must not fire"
7036+
);
7037+
assert!(
7038+
timers.lock().unwrap().is_empty(),
7039+
"tracking map stays empty after a cleared timer is evaluated"
7040+
);
7041+
}
7042+
7043+
#[test]
7044+
fn rearmed_timer_generation_mismatch_suppresses_stale_thread() {
7045+
// The bridge/kernel timer thread captures the generation at schedule time.
7046+
// If the timer is re-armed (generation bumped) before the stale thread
7047+
// wakes, the stale thread must observe the mismatch and suppress, while the
7048+
// entry survives for the live generation.
7049+
let timers: Arc<Mutex<HashMap<u64, LocalTimerEntry>>> =
7050+
Arc::new(Mutex::new(HashMap::new()));
7051+
timers.lock().unwrap().insert(
7052+
3,
7053+
LocalTimerEntry {
7054+
delay_ms: 10,
7055+
generation: 1,
7056+
repeat: false,
7057+
},
7058+
);
7059+
7060+
// Stale thread captured generation 0; current entry is at generation 1.
7061+
assert!(
7062+
!timer_should_fire(&timers, 3, 0),
7063+
"a stale generation must be suppressed"
7064+
);
7065+
assert!(
7066+
timers.lock().unwrap().contains_key(&3),
7067+
"the live entry must survive a stale-generation evaluation"
7068+
);
7069+
7070+
// The matching (current) generation fires and reclaims the one-shot entry.
7071+
assert!(
7072+
timer_should_fire(&timers, 3, 1),
7073+
"the current generation must fire"
7074+
);
7075+
assert!(
7076+
timers.lock().unwrap().is_empty(),
7077+
"a fired one-shot timer must reclaim its id from the map"
7078+
);
7079+
}
7080+
7081+
#[test]
7082+
fn bridge_timer_registration_is_tracked_and_drop_clears_timers() {
7083+
// H2: the bridge-timer path must register its timer (so it is cancellable)
7084+
// rather than spawning a bare untracked thread, and session teardown
7085+
// (dropping LocalBridgeState) must wipe the tracking map so in-flight timer
7086+
// threads are cancelled.
7087+
let mut state = LocalBridgeState::default();
7088+
// Observe the same map the spawned threads would consult.
7089+
let timers = state.timers.clone();
7090+
7091+
let id_a = state.register_oneshot_timer(MAX_TIMER_DELAY_MS);
7092+
let id_b = state.register_oneshot_timer(500);
7093+
assert_ne!(id_a, id_b, "each bridge timer gets a fresh id");
7094+
assert_eq!(
7095+
timers.lock().unwrap().len(),
7096+
2,
7097+
"registered bridge timers are tracked in the shared map"
7098+
);
7099+
// A registered timer would fire for its captured generation (proving the
7100+
// entry is real and consultable) ...
7101+
assert!(timer_should_fire(&timers, id_a, 0));
7102+
// ... and seeding a still-pending one before teardown:
7103+
let id_c = state.register_oneshot_timer(1_000);
7104+
7105+
// Session teardown: dropping the bridge state must clear every timer so any
7106+
// sleeping thread wakes to a missing entry and suppresses its callback.
7107+
drop(state);
7108+
7109+
assert!(
7110+
timers.lock().unwrap().is_empty(),
7111+
"dropping LocalBridgeState must clear the timers map on teardown"
7112+
);
7113+
assert!(
7114+
!timer_should_fire(&timers, id_c, 0),
7115+
"a pending bridge timer is suppressed after teardown"
7116+
);
7117+
}
69117118
}

0 commit comments

Comments
 (0)