From bed1d807c3223f05051549e66f35fb146970ee89 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Mon, 7 Sep 2026 20:05:29 +0200 Subject: [PATCH] fix(migration): cancel releases migration_readonly, pause deliberately does not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Step 4 of docs/plan/jobs-handling-recoverable-error.md — the sharp edge the plan flagged, and it was already a live trap independent of the retry work. `backend_migration` engages `migration_readonly`, which refuses writes ACROSS THE WHOLE APPLICATION until cutover. Cancelling it cleared nothing. The flag is persisted, so the state survived restarts — boot even logs a warning about coming up read-only — and the only escape was editing `admin_settings` by hand. Two paths reach a cancel, and only one of them ran any handler code: * a RUNNING row re-enters the handler, which now releases the gate at its next cancel poll when the intent is terminal; * a PAUSED row does NOT. `request_terminal_cancel` flips it straight to Cancelled in SQL with no handler in the loop. The second is the common case and the one that matters: a migration paused by an outage, holding the freeze, cancelled by an operator precisely to get writes back. Fixed in the cancel endpoint, which is the only place that sees it. Releasing on cancel is safe because cancel ENDS the run with no swap — the source is still the active backend, so nothing is left to protect, and a later retry starts fresh and rescans everything. **Pause deliberately keeps the gate**, per Ed's call: Ops cancels to release it. That is not conservatism for its own sake. The cursor is a position in a hash-ordered walk and stays valid only while nothing writes; release the gate on pause and a blob written afterwards whose hash sorts BELOW the cursor is never visited, so the run completes, flips the pointer, and reads for that hash 404 against a target that never received it. Releasing on pause becomes safe only once resume rescans from the start or a final catch-up pass runs under the freeze before the swap — the plan's follow-up, not this commit. Both release paths are best effort: a run that has already been cancelled should not become a hard failure because a DB blip prevented clearing a flag. The in-memory store happens regardless, so writes resume in this process; a loud warning names the DB copy needing attention. The endpoint check is gated on the job name AND on the flag currently being set, so it is a no-op for every other job — nothing else ever sets it. Co-Authored-By: Claude Opus 5 (1M context) --- src/infrastructure/scheduler/mod.rs | 6 +- .../services/backend_migration_service.rs | 77 +++++++++++++++++++ src/interfaces/api/handlers/admin_handler.rs | 77 ++++++++++++++++--- 3 files changed, 147 insertions(+), 13 deletions(-) diff --git a/src/infrastructure/scheduler/mod.rs b/src/infrastructure/scheduler/mod.rs index a78b9310..f1077050 100644 --- a/src/infrastructure/scheduler/mod.rs +++ b/src/infrastructure/scheduler/mod.rs @@ -33,9 +33,9 @@ pub use engine::SchedulerEngine; pub use handler::JobHandler; pub use pg_job_store::{PgJobStore, PgJobStoreProvider}; pub use recoverable::{ - Finding, JobStore, JobStoreProvider, OpenedRun, ProgressKind, RecoverableAdapter, - RecoverableJobHandler, RunOutcome, RunProgress, RunStatus, RunSummary, derive_progress, - record_or_log, run_or_resume, + CANCEL_INTENT_PARAM, CANCEL_INTENT_TERMINATE, Finding, JobStore, JobStoreProvider, OpenedRun, + ProgressKind, RecoverableAdapter, RecoverableJobHandler, RunOutcome, RunProgress, RunStatus, + RunSummary, derive_progress, record_or_log, run_or_resume, }; pub use registry::{ JobEntry, JobRegistry, JobSummary, PausedRunBrief, RegisterError, StartupTrigger, diff --git a/src/infrastructure/services/backend_migration_service.rs b/src/infrastructure/services/backend_migration_service.rs index aa6b4c15..286320b6 100644 --- a/src/infrastructure/services/backend_migration_service.rs +++ b/src/infrastructure/services/backend_migration_service.rs @@ -580,6 +580,23 @@ impl RecoverableJobHandler for BackendMigrationService { source_missing = source_missing_count, "backend_migration cancelled cooperatively, pausing" ); + // A TERMINAL cancel must give writes back. + // + // Cancel ends the run with no swap, so the source + // stays the active backend and there is nothing left + // to protect. Leaving the gate set stranded the whole + // application read-only with no way out: the flag is + // persisted, so a restart reloaded it rather than + // clearing it, and the only escape was editing + // `admin_settings` by hand. + // + // A plain PAUSE deliberately keeps the gate. The + // cursor stays valid only while nothing writes, so + // resuming after allowing writes could miss a blob + // written below the cursor — see the plan's + // "Why NOT to release the gate on pause". Cancel is + // the escape hatch, and it is the operator's call. + self.release_readonly_on_terminal_cancel(store).await; return RunOutcome::Paused { cursor: cursor .as_ref() @@ -834,6 +851,66 @@ impl RecoverableJobHandler for BackendMigrationService { } impl BackendMigrationService { + /// Clear `migration_readonly` when the cancel was TERMINAL. + /// + /// Cancel ends the run with no swap: the source is still the active + /// backend, so there is nothing left for the write freeze to + /// protect, and leaving it set locks the whole application out of + /// writes. The flag is persisted, so that state survived restarts — + /// the only escape was hand-editing `admin_settings`. + /// + /// **Pause is deliberately not this.** The cursor is a position in a + /// hash-ordered walk, and it stays valid only while nothing writes. + /// Release the gate on pause and a blob written afterwards whose + /// hash sorts BELOW the cursor is never visited, so the run + /// completes, flips the pointer, and reads for that hash 404 against + /// a target that never received it. Cancel is safe precisely because + /// it ENDS the run: a later retry starts fresh and rescans + /// everything. + /// + /// Distinguished by the same `cancel_intent` param the engine reads + /// to decide `Cancelled` vs `Paused`, so the two cannot disagree + /// about which kind of stop this was. + /// + /// Best effort, and deliberately so: a run that has already been + /// cancelled should not be turned into a hard failure by a DB blip + /// while releasing a flag. The in-memory store still happens, so + /// writes resume in THIS process even if the persist fails; the loud + /// warning is what tells an operator the DB copy needs attention. + async fn release_readonly_on_terminal_cancel(&self, store: &dyn JobStore) { + let terminal = store + .get_string_param(crate::infrastructure::scheduler::CANCEL_INTENT_PARAM) + .await + .ok() + .flatten() + .as_deref() + == Some(crate::infrastructure::scheduler::CANCEL_INTENT_TERMINATE); + if !terminal { + return; + } + + if let Err(e) = persist_migration_readonly(self.pool.as_ref(), false).await { + tracing::warn!( + target: "oxicloud::migration", + event = "storage.migration_readonly.release_persist_failed", + run_id = %store.run_id(), + error = %e, + "could not persist migration_readonly=false after a terminal cancel; writes \ + resume in this process but a restart will come up read-only until \ + admin_settings is corrected" + ); + } + self.migration_readonly.store(false, Ordering::Relaxed); + tracing::info!( + target: "audit", + event = "storage.migration_readonly.released", + reason = "migration_cancelled", + run_id = %store.run_id(), + "🚧 migration_readonly released after terminal cancel — writes resume, active \ + backend unchanged" + ); + } + /// Terminal successful path — reached from both Completed sites /// in the batch loop (empty-first-batch and short-batch). /// diff --git a/src/interfaces/api/handlers/admin_handler.rs b/src/interfaces/api/handlers/admin_handler.rs index e6c699ab..15de53a1 100644 --- a/src/interfaces/api/handlers/admin_handler.rs +++ b/src/interfaces/api/handlers/admin_handler.rs @@ -2879,16 +2879,73 @@ pub async fn cancel_job( .request_terminal_cancel(&name) .await { - Ok(Some(run_id)) => ( - StatusCode::OK, - Json(serde_json::json!({ - "cancelled": true, - "run_id": run_id.to_string(), - "note": "Running row → will land in Cancelled at next batch boundary; \ - Paused row → flipped to Cancelled immediately.", - })), - ) - .into_response(), + Ok(Some(run_id)) => { + // Cancelling a PAUSED migration has to give writes back + // here, because nothing else will. + // + // A Running row re-enters the handler, which releases the + // gate itself at its next cancel poll. A Paused row does + // not: `request_terminal_cancel` flips it straight to + // Cancelled in SQL with no handler in the loop. That is the + // common case — a migration paused by an outage, holding + // `migration_readonly`, which an operator cancels precisely + // TO get writes back. Without this the app stayed read-only + // forever: the flag is persisted, so even a restart reloaded + // it, and the only escape was editing admin_settings by + // hand. + // + // Safe because cancel ends the run with no swap — the source + // is still the active backend, so there is nothing left for + // the freeze to protect. Releasing on PAUSE would not be + // safe; see `release_readonly_on_terminal_cancel`. + // + // Idempotent and harmless for every other job: the flag is + // only ever set by backend_migration, so clearing it when it + // is already false is a no-op. + if name == crate::infrastructure::services::backend_migration_service::BACKEND_MIGRATION_JOB_NAME + && state + .migration_readonly + .load(std::sync::atomic::Ordering::Relaxed) + { + if let Some(pool) = state.db_pool.as_ref() + && let Err(e) = + crate::infrastructure::services::entry_backend::persist_migration_readonly( + pool.as_ref(), + false, + ) + .await + { + tracing::warn!( + target: "oxicloud::migration", + event = "storage.migration_readonly.release_persist_failed", + run_id = %run_id, + error = %e, + "could not persist migration_readonly=false after cancelling a paused \ + migration; writes resume now but a restart will come up read-only" + ); + } + state + .migration_readonly + .store(false, std::sync::atomic::Ordering::Relaxed); + tracing::info!( + target: "audit", + event = "storage.migration_readonly.released", + reason = "paused_migration_cancelled", + run_id = %run_id, + "🚧 migration_readonly released — writes resume, active backend unchanged" + ); + } + ( + StatusCode::OK, + Json(serde_json::json!({ + "cancelled": true, + "run_id": run_id.to_string(), + "note": "Running row → will land in Cancelled at next batch boundary; \ + Paused row → flipped to Cancelled immediately.", + })), + ) + .into_response() + } Ok(None) => ( StatusCode::OK, Json(serde_json::json!({