r.vif: preserve active MASK when read_data() exits with an error

[Valyrian-Code]

Summary

Fixes #1702.

When r.vif is run with n= (random sampling), read_data() renames the active MASK to a temporary backup so a new random-sampling mask can be applied. The backup name was previously added to CLEAN_RAST via tmpname(), so the atexit cleanup() deleted it on any exit — including exceptions raised during r.stats or the numpy parse. The user's original mask was permanently lost, and the random-sample reclass left behind by r.mask raster=new_mask was left as the active MASK.

This is the same antipattern fixed in r.boxplot (#1696, PR #1701), adapted to r.vif's flow.

Fix

1. Track the backup mask name in a module-level mask_backup_name variable instead of adding it to CLEAN_RAST. Set the variable only after a successful rename.

2. Add a restore_mask_backup() helper that:

   • Returns silently if no backup is tracked.
   • Removes any pre-existing MASK first (at cleanup time any MASK present is one r.vif created internally, so this is safe).
   • Wraps the rename in try/except, emits gs.warning() with the backup name and a manual restore command on failure, and never raises.

3. Call restore_mask_backup() from cleanup() before the temp-map removal loop, so the user's MASK is restored on any exit path.

4. Wrap the r.stats / np.loadtxt block in try/finally so on the normal-exception path the internal mask is removed and the backup is restored locally.

Test plan

Verified manually in GRASS 8.5.0 with an active MASK and three correlated rasters. Controlled failure injected after r.mask raster=new_mask to simulate a crash in the protected window.

Before the fix:

• g.list pattern="rvifoldmask*" after crash → backup raster remained but
• g.list pattern=MASK after crash → MASK was the random-sample reclass, not the user's mask. Original MASK was deleted by cleanup().

After the fix:

• g.list pattern=MASK → MASK present
• r.info MASK → "Reclass of rvif_mask" — the user's original mask
• g.list pattern="rvifoldmask*" → empty (backup was renamed back, not orphaned)
• No warnings during cleanup

cc @ecodiv @wenzeslaus

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR improves how r.vif handles an existing GRASS MASK by backing it up, attempting best-effort restoration, and integrating restoration into cleanup() to reduce the chance of leaving users with a broken mask state after errors.

Changes:

• Introduces a tracked MASK backup (mask_backup_name) and a restore_mask_backup() helper.
• Updates cleanup() to restore the original MASK before removing temporary rasters.
• Wraps sampling (r.stats/np.loadtxt) with try/finally to remove internal masks and restore the backup when possible.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Valyrian-Code]

Thanks for the review. Addressed two of the three:

1. Widened try/finally to also cover gs.run_command("r.mask", raster=new_mask, ...). Even before this change the atexit cleanup() served as a backstop, but it is cleaner to localize the restoration inline.

2. Switched restore_mask_backup() from ["file"] to ["fullname"] for consistency with the rest of the function. Both keys work in gs.find_file output (both truthy on hit), but mixing reads as sloppy.

3. Did not change the "remove pre-existing MASK before restore" behavior. GRASS mapsets are locked during operation, so concurrent MASK creation by another process is not a supported scenario. At cleanup time any MASK present is one r.vif itself created (the random-sample reclass), and removing it before renaming the backup back is the right action. Renaming it to a temp would just create leftover state for a case that cannot happen.

Re-tested manually in GRASS 8.5.0 — normal run completes with MASK preserved, crash-injection test (raise after r.mask) leaves MASK intact via atexit cleanup.

[wenzeslaus]

In 8.5, we have MaskManager for this. I would consider duplicatating the code here for compatibility with 8.4.

[Valyrian-Code]

In 8.5, we have MaskManager for this. I would consider duplicatating the code here for compatibility with 8.4.

Good point. Just to confirm before refactoring: the r.vif use case is "replace MASK with a different one" (random-sample), which is what MaskManager handles cleanly via manager.mask_name and GRASS_MASK. (This is different from r.boxplot's "disable the existing MASK" case where MaskManager alone is not sufficient see #1701.)

Plan:

• On 8.5+: write the random-sample raster directly to manager.mask_name inside a MaskManager context. No manual rename, no try/finally needed locally, MaskManager handles cleanup.

• On 8.4: keep the current manual approach (g.rename + restore_mask_backup() backstop in cleanup).

• Version detection via try/import on grass.script.MaskManager.

Does that match what you had in mind?