fix(migration): cancel releases migration_readonly, pause deliberately does not

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) <noreply@anthropic.com>
This commit is contained in:
Edouard Vanbelle
2026-09-07 20:05:29 +02:00
parent 303a0421c2
commit bed1d807c3
3 changed files with 147 additions and 13 deletions
+3 -3
View File
@@ -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,
@@ -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).
///