cr-sqlite icon indicating copy to clipboard operation
cr-sqlite copied to clipboard

Automigration: improve error handling (clearer communication with the caller)

Open ikusteu opened this issue 2 months ago • 1 comments

TL;DR

The errors occurring in automigration function(s) aren't always communicated clearly to the caller. I see two points to this issue:

  • make sure we're using the correct DB interface (local_db / mem_db) when retrieving the error (db.errmsg()) and communicating it back to the caller
  • make sure errors in local_db context live long enough to be communicated back to the caller (I got OK with not an error when I knew for a fact --through alternative logging/tracing methods-- that an error occurred in local_db ctx)

The Issue

If an error happens during crsql_automigrate execution (and is propagated up using try shorthand ?), it is always handled as if actions on mem_db have caused the error:

// automigrate.rs:automigrate_impl

// It's possible that migrate_result = Err(rc) originating from local_db or mem_db
if let Err(_) = migrate_result {
    local_db.exec_safe("ROLLBACK")?;
   
    // The error gets communicated as if we know for certain
    // that it originated from mem_db
    let mem_db_err_msg = mem_db.errmsg()?;
    ctx.result_error(&mem_db_err_msg);
    ctx.result_error_code(mem_db.errcode());

    cleanup(mem_db)?;
    return Err(ResultCode::OK);
} 

I think this should be handled properly: by letting the wrapping code if let Err(_) = migrate_result { ... } know the source of the error so that it can pull errmsg() using the correct DB interface and communicate the error to the caller, something like:

// automigrate.rs:automigrate_imp

if let Err(err) = migrate_result {
    local_db.exec_safe("ROLLBACK")?;

    let errsrc = figure_out_error_source(err, local_db, mem_db);
    
    let errcode = errsrc.errcode();
    let errmsg = errsrc.errmsg()?;
    ctx.result_error(&errmsg);
    ctx.result_error_code(errcode);

    cleanup(mem_db)?;
    return Err(ResultCode::OK);
} 

Furthermore, errors having happened in local_db context are cleared before the execution is done: I've tried fixing the previous point and, even though migrate_result returned an error that an error occurred in local_db (and I knew for a fact -- through alternative logging/tracing), local_db.errcode() and local_db.errmsg() returned OK and not an error (respectively). This, I believe, has something to do with calling context (how deep into the call we are) but am still investigating.

The Symptom

If using a JS wrapper with wa-sqlite, wa-sqlite validates the return code and throws an error if != 0 (for intents and purposes), so in this case:

  • error happens in local_db
  • the if Err(_) ... block retrieves mem_db error code/message (which are OK) and communicates back to caller
  • wa-sqlite's validation sees 0 and treats this as a non error

ikusteu avatar Oct 29 '25 08:10 ikusteu