From d5f5d1b7cfe0fab5e18cf2e07664b293f2141385 Mon Sep 17 00:00:00 2001 From: aviezerl Date: Mon, 27 Jul 2026 17:20:17 +0300 Subject: [PATCH] Allow atomic in-place track overwrite via override emr_track.create and emr_track.import both refused to write a track that already existed in the target db, regardless of `override` - that flag only covered shadowing a track held in another db. Callers wanting to rebuild a track in place had no option but to emr_track.rm() it first, which left the track absent for the entire rebuild. Anything reading it in that window failed, which is what wiser.epi's cohort/computed-track rebuild hits. Let override mean "replace this track" in the same db too, and route every track write through a staging file that is renamed into place. rename(2) replaces the target atomically, so a reader sees either the complete previous track or the complete new one, never a missing or half-written file. emr_track.import already staged to a ".tmp" file, but unlinked the target before the move, reopening exactly the window the staging was meant to close; the unlink is unnecessary since rename replaces the target by itself. The staging path is also now suffixed with the pid so two writers of the same track cannot corrupt each other's staging file, and it is cleaned up if serialization or the rename throws. Claude-Session: https://claude.ai/code/session_01J4rGxe5SVxtXktoRHvWXsL --- DESCRIPTION | 2 +- NEWS.md | 5 +++ R/track.R | 10 ++++-- man/emr_track.create.Rd | 5 ++- man/emr_track.import.Rd | 5 ++- src/NRImport.cpp | 34 +++++++++++++------ src/NRTrackCreate.cpp | 29 +++++++++++++--- tests/testthat/test-track.overwrite.R | 48 +++++++++++++++++++++++++++ 8 files changed, 119 insertions(+), 19 deletions(-) create mode 100644 tests/testthat/test-track.overwrite.R diff --git a/DESCRIPTION b/DESCRIPTION index 17f53a8f..8ccfe6fc 100755 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Type: Package Package: naryn Title: Native Access Medical Record Retriever for High Yield Analytics -Version: 2.7.1 +Version: 2.7.2 Authors@R: c( person("Misha", "Hoichman", , "misha@hoichman.com", role = "aut"), person("Aviezer", "Lifshitz", , "aviezer.lifshitz@weizmann.ac.il", role = c("aut", "cre")), diff --git a/NEWS.md b/NEWS.md index c4ceaea6..209c2b03 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,8 @@ +# naryn 2.7.2 + +* `emr_track.create` and `emr_track.import` can now rewrite a track in its own db when `override = TRUE`. Previously `override` only covered shadowing a track from another db, and rewriting in place required `emr_track.rm()` first - which left the track missing for the whole rebuild, so anything reading it in that window failed. +* Track writes are now staged to a temporary file and renamed into place. `rename(2)` replaces the target atomically, so a concurrent reader always sees either the complete previous track or the complete new one. `emr_track.import` already staged, but unlinked the target before the move, which reopened the same window. + # naryn 2.7.1 * Merged the CRAN-portability fixes from 2.6.32-2.6.34 onto the new locking mechanism introduced in 2.7.0. diff --git a/R/track.R b/R/track.R index 187489ef..e5bbe228 100644 --- a/R/track.R +++ b/R/track.R @@ -195,7 +195,10 @@ emr_track.addto <- function(track, src, force = FALSE, remove_unknown = FALSE) { #' implicitly based on track expressions. See also 'iterator' section. #' @param keepref If 'TRUE' references are preserved in the iterator #' @param filter Iterator filter -#' @param override Boolean indicating whether the creation intends to override an existing track (default FALSE) +#' @param override Boolean indicating whether the write intends to replace an existing track (default FALSE). +#' Covers both shadowing a track that lives in another db and rewriting one in this db. The new track is +#' written to a staging file and renamed into place, so readers never observe the track missing or partial +#' - there is no need to \code{emr_track.rm()} it first. #' #' @return None. #' @@ -347,7 +350,10 @@ emr_track.ids <- function(track) { #' @param space db dir string (path), one of the paths supplied in emr_db.connect #' @param categorical if 'TRUE' track is marked as categorical #' @param src file name or data-frame containing the track records -#' @param override Boolean indicating whether the creation intends to override an existing track (default FALSE) +#' @param override Boolean indicating whether the write intends to replace an existing track (default FALSE). +#' Covers both shadowing a track that lives in another db and rewriting one in this db. The new track is +#' written to a staging file and renamed into place, so readers never observe the track missing or partial +#' - there is no need to \code{emr_track.rm()} it first. #' @param remove_unknown if 'TRUE', removes unknown ids (ids that are not present at 'patients.dob' track) from the data. Otherwise, an error is thrown. #' #' @return None. diff --git a/man/emr_track.create.Rd b/man/emr_track.create.Rd index f343aa1b..28d5a9fc 100644 --- a/man/emr_track.create.Rd +++ b/man/emr_track.create.Rd @@ -37,7 +37,10 @@ implicitly based on track expressions. See also 'iterator' section.} \item{filter}{Iterator filter} -\item{override}{Boolean indicating whether the creation intends to override an existing track (default FALSE)} +\item{override}{Boolean indicating whether the write intends to replace an existing track (default FALSE). +Covers both shadowing a track that lives in another db and rewriting one in this db. The new track is +written to a staging file and renamed into place, so readers never observe the track missing or partial +- there is no need to \code{emr_track.rm()} it first.} } \value{ None. diff --git a/man/emr_track.import.Rd b/man/emr_track.import.Rd index 8e5e4bdf..f751dae4 100644 --- a/man/emr_track.import.Rd +++ b/man/emr_track.import.Rd @@ -22,7 +22,10 @@ emr_track.import( \item{src}{file name or data-frame containing the track records} -\item{override}{Boolean indicating whether the creation intends to override an existing track (default FALSE)} +\item{override}{Boolean indicating whether the write intends to replace an existing track (default FALSE). +Covers both shadowing a track that lives in another db and rewriting one in this db. The new track is +written to a staging file and renamed into place, so readers never observe the track missing or partial +- there is no need to \code{emr_track.rm()} it first.} \item{remove_unknown}{if 'TRUE', removes unknown ids (ids that are not present at 'patients.dob' track) from the data. Otherwise, an error is thrown.} } diff --git a/src/NRImport.cpp b/src/NRImport.cpp index e998b918..ee11bd9e 100755 --- a/src/NRImport.cpp +++ b/src/NRImport.cpp @@ -1,6 +1,8 @@ #include #include +#include + #include "EMRDb.h" #include "EMRTrack.h" #include "FileUtils.h" @@ -57,9 +59,12 @@ SEXP emr_import(SEXP _track, SEXP _db_id, SEXP _categorical, SEXP _src, SEXP _ad verror("%s directory is not set", db_id.c_str()); } - //Error only if exists in the same db, otherwise try overriding - if (g_db->track(trackname) && (g_db->track_info(trackname)->db_id == db_id)) { - verror("Track %s already exists", trackname.c_str()); + // Rewriting a track in its own db needs override, same as shadowing one from + // another db. The write below is staged and renamed into place, so an in-place + // rewrite never leaves readers with a missing or half-written track - callers no + // longer have to emr_track.rm() first and expose that window themselves. + if (g_db->track(trackname) && (g_db->track_info(trackname)->db_id == db_id) && !toverride) { + verror("Track %s already exists, see override argument", trackname.c_str()); } //User must explicitly pass an overriding argument if (g_db->track(trackname) && (g_db->track_info(trackname)->db_id != db_id) && !toverride) { @@ -90,7 +95,9 @@ SEXP emr_import(SEXP _track, SEXP _db_id, SEXP _categorical, SEXP _src, SEXP _ad track_filename = db_id + string("/") + trackname + EMRDb::TRACK_FILE_EXT; - if (access(track_filename.c_str(), F_OK) != -1) + // A stray file with no track registered for it: still refuse to clobber it silently, + // but override is an explicit "rewrite this track", so let it through. + if (!toverride && access(track_filename.c_str(), F_OK) != -1) verror("File %s already exists", track_filename.c_str()); } @@ -174,15 +181,22 @@ SEXP emr_import(SEXP _track, SEXP _db_id, SEXP _categorical, SEXP _src, SEXP _ad } - if (access(track_filename.c_str(), F_OK) != -1) { - string tmp_filename = track_filename + ".tmp"; + // Stage next to the target (same directory, so same filesystem) and rename into place. + // rename(2) replaces an existing file atomically, so a concurrent reader sees either the + // complete previous track or the complete new one - never a partial or absent file. The + // unlink that used to precede the move reopened exactly that window, and is not needed: + // rename replaces the target by itself. The pid in the suffix keeps two writers of the + // same track from clobbering each other's staging file. + string tmp_filename = track_filename + ".tmp." + std::to_string(getpid()); + try { EMRTrack::serialize(tmp_filename.c_str(), categorical ? EMRTrack::IS_CATEGORICAL : 0, data); - unlink(track_filename.c_str()); FileUtils::move_file(tmp_filename.c_str(), track_filename.c_str()); - } else { - EMRTrack::serialize(track_filename.c_str(), categorical, data); + } catch (...) { + unlink(tmp_filename.c_str()); + throw; } - + + if (has_overlap){ g_db->unload_track(trackname.c_str(), true, true); } diff --git a/src/NRTrackCreate.cpp b/src/NRTrackCreate.cpp index e75f54bb..4185947f 100755 --- a/src/NRTrackCreate.cpp +++ b/src/NRTrackCreate.cpp @@ -11,8 +11,13 @@ #undef error #endif +#include + +#include + #include "EMRDb.h" #include "EMRTrack.h" +#include "FileUtils.h" #include "naryn.h" #include "NRTrackExpressionScanner.h" @@ -46,10 +51,14 @@ SEXP emr_track_create(SEXP _track, SEXP _db_id, SEXP _categorical, SEXP _expr, S string trackname = { CHAR(Rf_asChar(_track)) }; - if (g_db->track(trackname) && (g_db->track_info(trackname)->db_id == db_id)){ - verror("Track %s already exists", trackname.c_str()); + // Rewriting a track in its own db needs override, same as shadowing one from another + // db. The write below is staged and renamed into place, so an in-place rewrite never + // leaves readers with a missing or half-written track - callers no longer have to + // emr_track.rm() first and expose that window themselves. + if (g_db->track(trackname) && (g_db->track_info(trackname)->db_id == db_id) && !toverride){ + verror("Track %s already exists, see override argument", trackname.c_str()); } - + // User must explicitly pass an overriding argument if (g_db->track(trackname) && (g_db->track_info(trackname)->db_id != db_id) && !toverride){ verror("Track %s already exists in db %s, see override argument", trackname.c_str(), g_db->track_info(trackname)->db_id.c_str()); @@ -85,7 +94,19 @@ SEXP emr_track_create(SEXP _track, SEXP _db_id, SEXP _categorical, SEXP _expr, S g_naryn->verify_max_data_size(data.data.size(), "Result"); } - EMRTrack::serialize(track_filename.c_str(), categorical, data); + // Stage next to the target (same directory, so same filesystem) and rename into place. + // rename(2) replaces an existing file atomically, so a concurrent reader sees either the + // complete previous track or the complete new one - never a partial or absent file. The + // pid in the suffix keeps two writers of the same track from clobbering each other's + // staging file; the rename itself then just decides who wins. + string tmp_filename = track_filename + ".tmp." + std::to_string(getpid()); + try { + EMRTrack::serialize(tmp_filename.c_str(), categorical ? EMRTrack::IS_CATEGORICAL : 0, data); + FileUtils::move_file(tmp_filename.c_str(), track_filename.c_str()); + } catch (...) { + unlink(tmp_filename.c_str()); + throw; + } if (has_overlap) { g_db->unload_track(trackname.c_str(), true, true); diff --git a/tests/testthat/test-track.overwrite.R b/tests/testthat/test-track.overwrite.R new file mode 100644 index 00000000..84b821fd --- /dev/null +++ b/tests/testthat/test-track.overwrite.R @@ -0,0 +1,48 @@ +load_test_db() + +# Rewriting a track in its own db used to be impossible: both emr_track.import and +# emr_track.create errored with "Track already exists" regardless of `override`, which only +# covered shadowing a track from another db. Callers had to emr_track.rm() first, leaving the +# track absent for the whole rebuild - anything reading it in that window failed. + +test_that("emr_track.import can rewrite a track in the same db with override", { + a <- emr_extract("track1", keepref = TRUE, names = "value") + emr_track.import("ovr_track", "global", categorical = FALSE, src = a) + withr::defer(emr_track.rm("ovr_track", force = TRUE)) + + expect_error(emr_track.import("ovr_track", "global", categorical = FALSE, src = a)) + + b <- emr_extract("track2", keepref = TRUE, names = "value") + emr_track.import("ovr_track", "global", categorical = FALSE, src = b, override = TRUE) + + expect_true(emr_track.exists("ovr_track")) + got <- emr_extract("ovr_track", keepref = TRUE, names = "value") + expect_equal(got$value, b$value) +}) + +test_that("emr_track.create can rewrite a track in the same db with override", { + emr_track.create("ovr_ctrack", "global", categorical = FALSE, expr = "track1") + withr::defer(emr_track.rm("ovr_ctrack", force = TRUE)) + before <- emr_extract("ovr_ctrack", names = "value")$value + + expect_error(emr_track.create("ovr_ctrack", "global", categorical = FALSE, expr = "track1 + 1")) + + # Same expression shifted by one, so the comparison stays inside a single iterator shape + # and only tests that the rewrite actually replaced the stored data. + emr_track.create("ovr_ctrack", "global", categorical = FALSE, expr = "track1 + 1", override = TRUE) + + expect_true(emr_track.exists("ovr_ctrack")) + expect_equal(emr_extract("ovr_ctrack", names = "value")$value, before + 1) +}) + +test_that("overwriting leaves no staging files behind", { + a <- emr_extract("track1", keepref = TRUE, names = "value") + emr_track.import("ovr_tmp_track", "global", categorical = FALSE, src = a) + withr::defer(emr_track.rm("ovr_tmp_track", force = TRUE)) + emr_track.import("ovr_tmp_track", "global", categorical = FALSE, src = a, override = TRUE) + emr_track.create("ovr_tmp_ctrack", "global", categorical = FALSE, expr = "track1", override = TRUE) + withr::defer(emr_track.rm("ovr_tmp_ctrack", force = TRUE)) + + db_dir <- emr_db.ls()[1] + expect_length(list.files(db_dir, pattern = "\\.tmp\\."), 0) +})