Summary
TempTableGuard's doc comment says it drops the merge temp table even if a per-format ingest "panics mid-load," but TempTableGuard::new is called after the load returns — so a panic inside the load is not covered.
The claim, twice:
// hyperdb-mcp/src/ingest.rs:162-168
/// RAII guard that ensures a temp table is dropped on **every** scope
/// exit — `Ok`, `Err`, *and* panic-unwind. Used by
/// [`merge_via_temp_table`] so an orphan `__hyperdb_merge_*` table
/// can't leak into the workspace, even if a per-format ingest path
/// panics mid-load.
// hyperdb-mcp/src/ingest.rs:260-262
/// 2. Load incoming data into a unique temp table via `replace_load`.
/// A `TempTableGuard` arms here and unwind-safely drops the temp
/// on every exit (`Ok` / `Err` / panic).
The construction site:
// hyperdb-mcp/src/ingest.rs:377-383
let tmp_result = replace_load(engine, &tmp_opts)?;
// Arm the cleanup guard immediately after the load so any later
// failure (or panic) drops the temp table on unwind. The guard
// tracks `target_db` so the DROP lands in the same DB the temp
// was created in.
let mut guard = TempTableGuard::new(engine, tmp.clone(), opts.target_db.clone());
The inline comment at :379-380 is accurate about the timing — "immediately after the load … so any later failure." The two doc comments above it are not. Everything from :383 onward genuinely is covered: the table_exists_in contract check, the column-metadata reads, the ALTER TABLE ADD COLUMN pass, and the DELETE/INSERT pair. The uncovered window is replace_load itself, which is exactly the window the doc names.
Impact
Latent. A replace_load that panics after creating the temp table but before returning leaves an orphan __hyperdb_merge_<target>_<pid>_<nanos>_<counter> table in the target database, with no cleanup and no log line — the guard's panic-path reporting at hyperdb-mcp/src/ingest.rs:216-227 was never constructed. Temp names are unique per call, so these accumulate rather than colliding: one per panicking merge, until someone notices them in a table listing. No data corruption and no wrong answers. The real cost is a doc comment that will convince the next reader the window is already closed.
Fix direction
This is not a one-line move. The guard borrows the engine immutably for its lifetime:
// hyperdb-mcp/src/ingest.rs:172-173
struct TempTableGuard<'a> {
engine: &'a Engine,while replace_load needs an exclusive borrow (F: FnOnce(&mut Engine, &IngestOptions), hyperdb-mcp/src/ingest.rs:304). Constructing the guard first would hold a shared borrow across the closure's exclusive one, which the borrow checker rejects — presumably why it sits where it does.
So the choice is either to restructure so the guard can span the load — have it hold something cheaply shareable instead of an &Engine, or register the temp name in a scope-owned list that a single outer guard drains — or to correct the two doc comments to promise only what the code delivers, namely coverage from the load's return onward. The doc fix is small and honest; the restructure closes the window but is worth weighing against how likely a per-format loader is to panic mid-load in the first place.
Summary
TempTableGuard's doc comment says it drops the merge temp table even if a per-format ingest "panics mid-load," butTempTableGuard::newis called after the load returns — so a panic inside the load is not covered.The claim, twice:
The construction site:
The inline comment at
:379-380is accurate about the timing — "immediately after the load … so any later failure." The two doc comments above it are not. Everything from:383onward genuinely is covered: thetable_exists_incontract check, the column-metadata reads, theALTER TABLE ADD COLUMNpass, and the DELETE/INSERT pair. The uncovered window isreplace_loaditself, which is exactly the window the doc names.Impact
Latent. A
replace_loadthat panics after creating the temp table but before returning leaves an orphan__hyperdb_merge_<target>_<pid>_<nanos>_<counter>table in the target database, with no cleanup and no log line — the guard's panic-path reporting athyperdb-mcp/src/ingest.rs:216-227was never constructed. Temp names are unique per call, so these accumulate rather than colliding: one per panicking merge, until someone notices them in a table listing. No data corruption and no wrong answers. The real cost is a doc comment that will convince the next reader the window is already closed.Fix direction
This is not a one-line move. The guard borrows the engine immutably for its lifetime:
while
replace_loadneeds an exclusive borrow (F: FnOnce(&mut Engine, &IngestOptions),hyperdb-mcp/src/ingest.rs:304). Constructing the guard first would hold a shared borrow across the closure's exclusive one, which the borrow checker rejects — presumably why it sits where it does.So the choice is either to restructure so the guard can span the load — have it hold something cheaply shareable instead of an
&Engine, or register the temp name in a scope-owned list that a single outer guard drains — or to correct the two doc comments to promise only what the code delivers, namely coverage from the load's return onward. The doc fix is small and honest; the restructure closes the window but is worth weighing against how likely a per-format loader is to panic mid-load in the first place.