Skip to content

engine: unguarded panics on dangling module model_name in legacy from_salsa path (module_deps, topo_sort) #806

Description

@bpowers

Problem

Two latent unguarded-panic sites on the legacy Project::from_salsa dependency-resolution path crash with an "internal compiler error" / HashMap-index panic on user-controllable input (a module with an empty or dangling model_name, e.g. a freshly-drawn module or a reference to a deleted model).

1. model.rs:173 — unguarded HashMap index in module_deps

In the is_initial branch of module_deps (src/simlin-engine/src/model.rs):

if ctx.is_initial {
    let model = ctx.models[model_name];   // <- panics if model_name not in ctx.models

This is an unguarded Index on ctx.models. If a Variable::Module references a model_name that is not present in ctx.models, this panics with "no entry found for key" / index panic.

The sibling at model.rs:241-244 (module_output_deps) already guards this correctly:

if !ctx.models.contains_key(model_name) {
    return model_err!(BadModelName, model_name.to_string());
}
let model = ctx.models[model_name];

Line 173 should do the same — use .get() and return BadModelName (or skip gracefully) rather than indexing unconditionally. The fix is a near-mechanical mirror of the guarded twin 70 lines below it.

2. common.rs:2659topo_sort panics on an unknown ident (lower priority, same class)

panic!("internal compiler error: unknown ident {}", ident.as_str());

topo_sort panics when a runlist ident has no dependencies entry. This is reachable from the same legacy from_salsa path with a dangling module reference. Lower priority but the same class of latent crash on bad input — should surface as a proper error rather than a panic!.

Why it matters

  • Unguarded panic on user-controllable input: an empty/dangling model_name is something the UI can produce (a module drawn but not yet wired up, or a reference to a model that was deleted). A panic here is an "internal compiler error" crash rather than a clean diagnostic.
  • Inconsistency: model.rs:173 is the unguarded twin of an already-guarded sibling (model.rs:241-244) in the same file. The guard pattern is established; line 173 simply was not updated to match.
  • WASM: under panic = "abort" (WASM release builds) a panic aborts the entire runtime.

Currently latent (why it is tracked, not urgently fixed)

These sites are only reached via ModelStage1::set_dependencies -> Project::from_salsa, which the engine CLAUDE.md documents as a test-only / legacy path. Production uses db::compile_project_incremental, whose parallel logic at src/simlin-engine/src/db.rs:687 already uses .get(...)?:

let sub_model = project_models.get(sub_model_name.as_ref())?;

So the production incremental path is safe; only the legacy monolithic path has the gap.

Components affected

  • src/simlin-engine/src/model.rs (module_deps, line 173)
  • src/simlin-engine/src/common.rs (topo_sort, line 2659)

Possible approaches

  1. model.rs:173: mirror the guarded twin — contains_key/.get() check returning BadModelName, or .get() and skip the module's deps gracefully.
  2. common.rs:2659: return a Result::Err (e.g. a Generic/BadModelName compilation error) instead of panic!, so a dangling runlist ident degrades to a diagnostic.

Context / how discovered

Found while fixing a WASM panic with the same root cause in src/simlin-engine/src/units_infer.rs (an empty/dangling model_name caused self.models[model_name] to panic with "no entry found for key"). These two sibling sites share that root cause but were left unfixed because they are on the test-only/legacy from_salsa path. Filed for tracking; not fixed as part of the units_infer work.

Metadata

Metadata

Assignees

No one assigned

    Labels

    engineIssues with the rust-based simulation engine

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions