[Fix] roomote upgrade prints spurious "tags_file: unbound variable" after completion

[roomote-roomote-v1]

Opened on behalf of Matt Rubens. Follow up by mentioning @openmote, in the web UI, or in Slack.

What changed

Fixed a spurious error printed after sudo roomote upgrade finishes:

/usr/local/bin/roomote: line 146: tags_file: unbound variable

The upgrade itself always succeeded — this line fired afterward, during image-retention cleanup. In prune_roomote_images, a nested cleanup_prune_roomote_images function was registered as a bash RETURN trap and also called explicitly at the end of the function. bash RETURN traps are dynamically scoped, so trap - RETURN issued from inside the nested cleanup helper did not clear the trap in prune_roomote_images's own scope. The trap re-fired when an outer caller (cmd_upgrade) returned, by which point the function's local variables (tags_file, keep_file, images_file) were already gone — and with set -u in effect, referencing them aborted with "unbound variable."

The nested cleanup function is replaced with a single self-clearing inline RETURN trap:

trap 'rm -f "$tags_file" "$images_file"; [ -n "$keep_file" ] && rm -f "$keep_file" "${keep_file}.dedup"; trap - RETURN' RETURN

The trailing trap - RETURN runs inside the trap handler (i.e. in prune_roomote_images's own frame at return time), so it clears the trap there and prevents it from propagating to outer callers. The redundant explicit cleanup call at the end of the function is removed; the RETURN trap now owns cleanup on both normal completion and early-return (|| return) paths.

The .dedup removal is guarded with [ -n "$keep_file" ] so that on the narrow early-return window where keep_file's mktemp fails right after tags_file succeeds, keep_file is still empty and the trap does not expand ${keep_file}.dedup to .dedup and delete an unrelated .dedup file in the current working directory. tags_file and images_file are removed unconditionally because rm -f "" is a harmless no-op (verified). This restores the if [ -n "$keep_file" ] guard that the original nested cleanup helper had.

Why this change was made

The trailing error made successful upgrades look failed, which is alarming for operators and could mask a real failure that follows it. The .dedup guard closes a review-flagged edge case where a failed mktemp on the early-return path could have deleted an unrelated file in the deployment working directory. The locals are pre-initialized to '' on their local declaration line, so the inline trap body is safe under set -u even on early-return paths where not every temp file has been created yet.

Impact

roomote upgrade no longer prints a spurious "unbound variable" error after completing, and a failed keep_file mktemp on the early-return path cannot delete an unrelated .dedup file in the current working directory. Temp-file cleanup behavior is preserved on both the normal-completion path and every || return early-exit path inside prune_roomote_images; the RETURN trap fires once on function return and removes tags_file, images_file, and (when keep_file is set) keep_file and ${keep_file}.dedup. No user-facing behavior changes beyond removing the misleading error and closing the edge case.

[roomote-roomote-v1]

No new code issues found, and the previously flagged edge case is resolved. See task

The new commit guards the .dedup removal with [ -n "$keep_file" ], so a failed keep_file mktemp on the early-return path no longer deletes a .dedup file in the current directory. I verified against the current code that the early-return path preserves a cwd .dedup, the guarded trap runs cleanly under set -euo pipefail (the failing guard test does not abort), and the normal-completion path still removes all temp files.

• 🟢 deploy/host/roomote:145 — The RETURN trap unconditionally ran rm -f "${keep_file}.dedup"; on the narrow early-return window where keep_file's mktemp fails right after tags_file succeeds, keep_file was still '' and rm -f ".dedup" could delete a .dedup file in the current working directory. Resolved in 83377ff by guarding the removal with [ -n "$keep_file" ] && rm -f "$keep_file" "${keep_file}.dedup".

[github-actions]

Thank you for your contribution! Before we can merge this pull request, we need you to sign our Contributor License Agreement. You can sign it by posting the comment below.

---

I have read the CLA Document and I hereby sign the CLA

---

_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

[roomote-roomote-v1]

Fixed the .dedup cwd-deletion hazard on the early-return path.

The RETURN trap now guards the keep_file-dependent removals with [ -n "$keep_file" ]:

trap 'rm -f "$tags_file" "$images_file"; [ -n "$keep_file" ] && rm -f "$keep_file" "${keep_file}.dedup"; trap - RETURN' RETURN

So when keep_file's mktemp fails on the early-return path, keep_file is still empty and ${keep_file}.dedup no longer expands to .dedup (which would have deleted an unrelated .dedup in the current working directory). tags_file and images_file are still removed unconditionally — rm -f "" is a harmless no-op (verified). This restores the if [ -n "$keep_file" ] guard the original nested cleanup helper had.

Verified: reproduced the hazard against the pre-guard trap (a planted .dedup in cwd was deleted), then confirmed the guarded trap leaves it intact on the same failing-mktemp path; the normal-completion path still cleans up all temp files.

Pushed in 83377ff. See task