framework: add schedule_deletion #9064
Conversation
WalkthroughAdds a deletion-scheduling facility and integrates it into library initialization; callers replace immediate rm calls with enqueueing via schedule_deletion(). A cleanup hook pre_umount_final_image__schedule_deletion_delete_now() performs queued removals during image finalization. One rm invocation was changed to use rm -fv. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
02
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
extensions/schedule_deletion.sh (1)
16-29: Improve the success message phrasing.The message at line 23 reads "file deleted $file" which is awkward phrasing. Consider reversing to "deleted file: $file" or "file $file deleted" for better readability.
Additionally, the logic correctly fails with
exit_with_errorif deletion fails (line 25), which aligns with the PR objective to "raise an error if the file to be deleted did not exist."Example improvement:
- display_alert "file deleted $file" "$message" "info" + display_alert "Deleted file" "$file: $message" "info"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
extensions/schedule_deletion.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.
📚 Learning: 2025-10-24T04:46:22.901Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.
Applied to files:
lib/functions/rootfs/rootfs-create.sh
🧬 Code graph analysis (3)
extensions/schedule_deletion.sh (2)
extensions/cleanup-space-final-image.sh (2)
pre_umount_final_image__999_show_space_usage(33-36)post_umount_final_image__200_zerofree(19-31)lib/functions/general/extensions.sh (1)
cleanup_handler_extensions(378-387)
lib/functions/rootfs/rootfs-create.sh (1)
extensions/schedule_deletion.sh (1)
schedule_deletion(7-14)
lib/functions/rootfs/create-cache.sh (2)
lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)lib/functions/rootfs/post-tweaks.sh (1)
post_debootstrap_tweaks(10-34)
🔇 Additional comments (4)
extensions/schedule_deletion.sh (2)
7-14: Provenance tracking is well-implemented.The use of
BASH_SOURCE[1],FUNCNAME[1], andBASH_LINENO[0]correctly captures which code scheduled the deletion, which aids debugging and satisfies the PR goal of controlling deletion order.
1-29: Both assumptions verified as correct; no actionable issues found.The deferred deletion design is sound. Verification confirms:
- The extension is sourced early via
lib/library-functions.sh:677, beforeschedule_deletion()is called atrootfs-create.sh:345.- The
pre_umount_final_image__schedule_deletion_delete_now()hook is explicitly invoked bycall_extension_method "pre_umount_final_image"inlib/functions/image/rootfs-to-image.sh:108. The hook naming convention follows the established framework pattern and will be automatically discovered and executed by the extension manager.The build lifecycle correctly orders extension loading →
schedule_deletion()calls → hook invocation during final image unmounting.lib/functions/rootfs/create-cache.sh (1)
125-125: Addition of-fflag is appropriate.The change from
rm -vtorm -fvallows the command to succeed silently if the file is missing, while still providing verbose output when present. This is consistent with the pattern used elsewhere in the codebase (e.g., post-tweaks.sh line 15 in relevant_code_snippets).lib/functions/rootfs/rootfs-create.sh (1)
339-345: Consider clarifying why two different strategies are used for machine-id handling.The code employs two separate approaches for the machine-id files:
- Line 344: empties
/etc/machine-idimmediately viaecho -n ">"- Line 345: schedules
/var/lib/dbus/machine-idfor deletion in the pre-umount phaseWhile this dual-strategy is intentional and correct—emptying
/etc/machine-idduring rootfs creation prevents systemd from auto-starting services on first boot, while scheduling/var/lib/dbus/machine-idfor deletion handles secondary references—the comment (lines 339–343) only explains the systemd behavior, not the rationale for the different treatment.Consider adding a note explaining why
/etc/machine-idis emptied immediately while/var/lib/dbus/machine-idis scheduled for later deletion.
tabrisnet
force-pushed
the
tabrisnet_schedule_deletion
branch
from
214d416 to
ee7cbbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/functions/general/schedule_deletion.sh (1)
5-14: Makeschedule_deletion()validate input and store data without fragile;serialization.
Today it’ll break if the path (or caller metadata) contains;, and it happily accepts an empty$1. Consider storing separate parallel arrays, or at least reject empty input.function schedule_deletion() { local file="$1" + [[ -n "$file" ]] || exit_with_error "schedule_deletion: empty file path" local source="${BASH_SOURCE[1]}" local func="${FUNCNAME[1]}" local line="${BASH_LINENO[0]}" schedule_deletion_files_to_delete+=("$file;$source;$func;$line") }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
lib/functions/general/schedule_deletion.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(1 hunks)lib/library-functions.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/functions/rootfs/create-cache.sh
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-24T04:46:22.901Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.
Applied to files:
lib/functions/rootfs/rootfs-create.sh
📚 Learning: 2025-09-05T12:31:03.067Z
Learnt from: Ayush1325
Repo: armbian/build PR: 8382
File: packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh:104-105
Timestamp: 2025-09-05T12:31:03.067Z
Learning: Scripts using `set -euo pipefail` provide comprehensive error handling that automatically exits on any command failure, making explicit error handling for function calls redundant.
Applied to files:
lib/library-functions.sh
📚 Learning: 2025-04-28T08:27:26.890Z
Learnt from: leggewie
Repo: armbian/build PR: 8133
File: extensions/apa.sh:1-2
Timestamp: 2025-04-28T08:27:26.890Z
Learning: In the Armbian build system, extension scripts in the `extensions/` directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or `set -euo pipefail`.
Applied to files:
lib/library-functions.sh
🧬 Code graph analysis (2)
lib/functions/rootfs/rootfs-create.sh (1)
lib/functions/general/schedule_deletion.sh (1)
schedule_deletion(7-14)
lib/functions/general/schedule_deletion.sh (5)
extensions/cleanup-space-final-image.sh (3)
add_host_dependencies__cleanup_space_final_image_zerofree(1-3)pre_umount_final_image__999_show_space_usage(33-36)post_umount_final_image__200_zerofree(19-31)extensions/fs-cryptroot-support.sh (1)
post_umount_final_image__750_cryptroot_cleanup(95-97)lib/functions/rootfs/trap-rootfs.sh (1)
trap_handler_cleanup_rootfs_and_image(53-82)extensions/lowmem.sh (1)
pre_umount_final_image__memory_optimize_defaults(65-74)extensions/lvm.sh (1)
post_umount_final_image__cleanup_lvm(77-79)
🔇 Additional comments (2)
lib/functions/rootfs/rootfs-create.sh (1)
339-346: Good switch to deferred deletion; verify the deletion hook always runs for this flow.
The callsite looks correct and keeps path semantics unchanged. Main thing to double-check is thatpre_umount_final_image__schedule_deletion_delete_now()is consistently invoked in the rootfs/image pipeline(s) wherecreate_new_rootfs_cache_via_debootstrap()is used, otherwise this becomes a no-op.lib/library-functions.sh (1)
1-3: Sourcingschedule_deletion.shhere is the right place (init-order safe).
This ensuresschedule_deletion()is available for later sourced modules without adding ad-hoc sourcing at callsites.Also applies to: 751-759, 1188-1188
…rootfs-create, then do it the same way in create-cache
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/functions/general/schedule_deletion.sh (1)
16-31: Don’t clobber the caller’serrexit(set -e) state.
set +e/ unconditionalset -einside the loop can turn-eon even if it was previously off, changing behavior of any code that runs afterward.function pre_umount_final_image__schedule_deletion_delete_now() { local file source func line message deletion_info + local had_errexit="no" + case $- in *e*) had_errexit="yes";; esac for deletion_info in "${schedule_deletion_files_to_delete[@]}"; do IFS=';' read -r file source func line <<< "$deletion_info" message="scheduled from $source $func $line" set +e # don't bail out, let us bail out ourselves more verbosely if [[ ! -e "$file" ]]; then exit_with_error "FILE DELETION FAILED (missing): '${file}'; ${message}" elif rm -- "$file"; then display_alert "deleted file '${file}'" "$message" "info" else exit_with_error "FILE DELETION FAILED: '${file}'; ${message}" fi - set -e # restore the previous behaviour + [[ "$had_errexit" == "yes" ]] && set -e done }
🧹 Nitpick comments (2)
lib/functions/general/schedule_deletion.sh (2)
5-14: Validate input + avoid “;”-delimited serialization (breaks on paths containing;/newlines).
Right now, any;in$filewill corrupt parsing (and an empty$1schedules a dangerous no-op / confusing error later). Consider either (a) validating$1, and (b) storing fields in separate arrays or using a NUL-delimited encoding.function schedule_deletion() { local file="$1" + [[ -n "$file" ]] || exit_with_error "schedule_deletion(): missing file argument" local source="${BASH_SOURCE[1]}" local func="${FUNCNAME[1]}" local line="${BASH_LINENO[0]}" schedule_deletion_files_to_delete+=("$file;$source;$func;$line") }
16-32: Consider explicitly handling the “no scheduled deletions” case for observability.
Right now, the hook silently does nothing if the queue is empty; a debug-level alert can make it easier to confirm the hook ran during image finalization.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/functions/general/schedule_deletion.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/functions/general/schedule_deletion.sh (2)
extensions/lowmem.sh (1)
pre_umount_final_image__memory_optimize_defaults(65-74)lib/functions/rootfs/trap-rootfs.sh (1)
trap_handler_cleanup_rootfs_and_image(53-82)
tabrisnet
force-pushed
the
tabrisnet_schedule_deletion
branch
from
738c7d3 to
e79f7ef
Compare
|
I gotta be missing something about those two files. I can't see how making a delayed deletion helps. |
github-actions
bot
added
size/medium
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/functions/rootfs/rootfs-create.sh (1)
339-345: Consider the root-cause concern raised in the PR review.The PR comments from rpardini question whether scheduling delayed deletion is the right approach: "if the file is missing it indicates the component that created the file is failing." The change moves from immediate deletion with built-in error handling (
rmfails if file missing) to deferred deletion with explicit error checking in the hook.This is a design choice: either fix why
/var/lib/dbus/machine-idexists when it shouldn't, or schedule its deletion for later. The PR objectives state the goal is to "ensure file deletion occurs regardless of explicit order," which suggests ordering/sequencing is the issue, not that the file may or may not exist.Clarify: Is the scheduling necessary because the file may be recreated by subsequent build steps, or because deletion order matters for other reasons?
If you'd like to document this design decision or consider whether fixing the root cause would be more robust, I can help with that analysis.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
lib/functions/general/schedule_deletion.sh(1 hunks)lib/functions/rootfs/create-cache.sh(1 hunks)lib/functions/rootfs/rootfs-create.sh(1 hunks)lib/library-functions.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/library-functions.sh
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.
📚 Learning: 2025-10-24T04:46:22.901Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-10-24T04:46:22.901Z
Learning: In lib/functions/rootfs/rootfs-create.sh, the FIXME comment about mmdebstrap usage with --aptopt is a future note related to PR #8785, which hasn't been merged yet.
Applied to files:
lib/functions/general/schedule_deletion.shlib/functions/rootfs/rootfs-create.sh
📚 Learning: 2025-09-05T12:31:03.067Z
Learnt from: Ayush1325
Repo: armbian/build PR: 8382
File: packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh:104-105
Timestamp: 2025-09-05T12:31:03.067Z
Learning: Scripts using `set -euo pipefail` provide comprehensive error handling that automatically exits on any command failure, making explicit error handling for function calls redundant.
Applied to files:
lib/functions/general/schedule_deletion.sh
📚 Learning: 2025-09-25T18:37:00.330Z
Learnt from: tabrisnet
Repo: armbian/build PR: 8661
File: lib/functions/compilation/armbian-kernel.sh:194-199
Timestamp: 2025-09-25T18:37:00.330Z
Learning: In PR armbian/build#8661, line 235 of lib/functions/compilation/armbian-kernel.sh already contains the corrected comment "BPF link support for netfilter hooks" for NETFILTER_BPF_LINK, not the misleading "BPF_SYSCALL" comment that was flagged during review.
Applied to files:
lib/functions/general/schedule_deletion.sh
📚 Learning: 2025-07-25T03:51:50.830Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Applied to files:
lib/functions/general/schedule_deletion.sh
📚 Learning: 2025-07-25T03:51:50.830Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Applied to files:
lib/functions/general/schedule_deletion.sh
📚 Learning: 2025-08-30T06:48:09.091Z
Learnt from: tabrisnet
Repo: armbian/build PR: 0
File: :0-0
Timestamp: 2025-08-30T06:48:09.091Z
Learning: In lib/functions/compilation/armbian-kernel.sh, the user prefers flexible grep patterns over anchored ones for BTRFS configuration checks, but agrees to use quiet grep (-q) to avoid polluting build logs.
Applied to files:
lib/functions/general/schedule_deletion.sh
📚 Learning: 2025-08-14T17:19:39.693Z
Learnt from: leggewie
Repo: armbian/build PR: 8502
File: config/desktop/trixie/environments/i3-wm/config_base/packages:44-44
Timestamp: 2025-08-14T17:19:39.693Z
Learning: When a PR author provides clear context about package transitions in the commit message and the changes are scoped to specific release pockets, focus the review on validating those specific changes rather than suggesting broader investigations across other releases or additional dependency verifications.
Applied to files:
lib/functions/general/schedule_deletion.sh
🧬 Code graph analysis (2)
lib/functions/rootfs/rootfs-create.sh (1)
lib/functions/general/schedule_deletion.sh (1)
schedule_deletion(7-14)
lib/functions/rootfs/create-cache.sh (1)
lib/functions/logging/runners.sh (1)
run_host_command_logged(204-206)
🔇 Additional comments (4)
lib/functions/rootfs/create-cache.sh (1)
125-126: Consider the implications of changing torm -fvon a file that should exist.Using the
-f(force) flag on/etc/resolv.confcausesrmto succeed silently if the file is missing, which contradicts the PR objective to "throw an error if the file did not exist to be deleted." If this file is guaranteed to exist after extraction (line 113), the originalrm -vsemantics—failing on a missing file—seems more aligned with catching unexpected extraction failures.Is this change intentional to be more lenient, or does it conflict with the stated PR goal?
lib/functions/general/schedule_deletion.sh (2)
7-14: Approve schedule_deletion() function implementation.The
schedule_deletion()function correctly captures caller context (BASH_SOURCE,FUNCNAME,BASH_LINENO) and encodes it for later processing. The use of a semicolon-delimited format and appending to the global array is straightforward and maintainable.
16-43: The deletion hook is properly wired and invoked—this concern is unfounded.The
pre_umount_final_image__schedule_deletion_delete_now()function is registered and called correctly. Atlib/functions/image/rootfs-to-image.sh:108,call_extension_method "pre_umount_final_image"explicitly invokes allpre_umount_final_image__*functions, including your deletion handler, before unmounting the rootfs. This occurs afterschedule_deletion()calls are made fromrootfs-create.sh:345, ensuring correct ordering.If files remain in the rootfs image, the issue lies elsewhere—either files aren't being scheduled for deletion in the first place, or there's a logic problem in the deletion function itself. The hook infrastructure is sound.
lib/functions/rootfs/rootfs-create.sh (1)
344-345: Confirm the scheduling mechanism is initialized before this call.Line 345 calls
schedule_deletion(), which depends on the global arrayschedule_deletion_files_to_deletebeing declared and the scheduling framework being loaded. Please verify:
- Is
lib/functions/general/schedule_deletion.shsourced beforerootfs-create.shis executed?- Is the sourcing order documented in
lib/library-functions.sh(mentioned in the AI summary)?
the proximate cause of |
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes. @tabrisnet, thank you for the clarification! That makes perfect sense now. The design rationale is sound:
This handles the variable timing elegantly—rather than trying to enforce strict ordering of package installations (which you noted is evolving), the scheduled deletion acts as a cleanup pass that runs at a well-defined point before image finalization. If the package hasn't been installed, the scheduled deletion will catch the missing file with an appropriate error message. This pattern is particularly useful for build systems where component ordering may vary but certain cleanup operations must always occur. ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @tabrisnet, thank you for the clarification! That makes perfect sense now. The scheduling approach is the right solution for this scenario:
This is a clean way to handle the non-deterministic ordering of package installations without having to track or control every point where the dbus package might be pulled in as a dependency. ✏️ Learnings added
🧠 Learnings used |
2 participants
Description
followup to #9060 and specifically #9060 (comment)
The goal is to be sure the deletion happened regardless of the explicit order of other actions within the framework, but also throw an error if the file wasn't there to be deleted.
Documentation summary for feature / change
Yes, documentation is probably needed.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.
--> (688) INFO: file deleted /armbian/.tmp/rootfs-6424fec5-d6f6-4f7c-9d79-540884d0449f/var/lib/dbus/machine-id [ scheduled from /armbian/lib/functions/rootfs/rootfs-create.sh create_new_rootfs_cache_via_debootstrap 345 ]tar --zstd -tvf cache/rootfs/rootfs-armhf-bookworm-cli_202512-11aa195f4c01-H7f4843-B018ebe.tar.zst |grep /var/lib/dbus/machine-idChecklist:
Please delete options that are not relevant.
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.