core: services: versionchooser: Retry delete after freeing reserve

[patrickelectric]

Docker cannot update overlay metadata when the disk is full.

Fix #1015

[github-actions]

Automated PR Review

0. Summary

• Verdict: MINOR SUGGESTIONS ✏️

Adds a full-disk recovery path to versionchooser.delete_version: on the first delete failure, the code unlinks a pre-allocated 1 GB reserve file (/usr/blueos/userdata/.disk_reserve) and retries the docker image deletion. The reserve itself is fallocated at container start by core/start-blueos-core when total disk > 7 GB and free > 1 GB. Targets #1015 where Docker can't free overlay metadata on a full disk.

1. Correctness & Implementation Bugs

• 1.1 [minor] core/services/versionchooser/utils/chooser.py:361 — DISK_RESERVE_PATH.unlink() has a TOCTOU race with the exists() check on line 360. If another process removes the reserve between the two calls (unlikely but possible), unlink() raises FileNotFoundError, which escapes delete_version uncaught and turns a recoverable case into a 500 with a stack trace in the client. Use DISK_RESERVE_PATH.unlink(missing_ok=True) and drop the explicit exists() guard, or wrap it in the same broad except Exception.
• 1.2 [minor] core/services/versionchooser/utils/chooser.py:352-369 — once the reserve is consumed, it is never re-created for the lifetime of the container. A user who deletes one image on a full disk and then tries to delete another (still on a full disk) will get the original opaque 500. Consider either recreating the reserve after a successful retry, or logging a clear one-shot notice so the user understands they need to reboot to re-arm the mechanism.
• 1.3 [nit] core/services/versionchooser/utils/chooser.py:358,366 — the pre-existing typo "Unable do delete image" is now duplicated on both error branches. Worth fixing to "Unable to delete image" while touching these lines.

3. Security

Nothing to flag. No new deps, no shell-injection surface, no obfuscation, no network calls, no changes to CI/Docker/nginx beyond the reserve-file allocation in start-blueos-core.

4. Performance

• 4.1 [nit] core/services/versionchooser/utils/chooser.py:360-361 — pathlib.Path.exists() and unlink() are blocking syscalls on the event loop. The cost is negligible for a single stat/unlink, but the codebase pattern is asyncio.to_thread for filesystem work; not worth changing on its own, just flagging.

6. Code Quality & Style

• 6.1 [minor] The reserve path is duplicated as a string literal in core/services/versionchooser/utils/chooser.py:22 and core/start-blueos-core:105. If either side changes, the recovery path breaks silently. Either export the path from a well-known location (e.g. a shared constant read by both) or add a comment on each side referencing the other so they stay in lock-step.
• 6.2 [nit] core/services/versionchooser/utils/chooser.py:355,362 — inconsistent exception-variable naming within the same function: error on the outer branch and exception on the inner. Pick one (the surrounding module uses error).
• 6.3 [nit] core/start-blueos-core:108 — echo "Allocating 1GB disk reserve" runs before fallocate, so the message is printed even when fallocate fails and the || rm -f cleanup fires. Minor log noise; consider moving the echo into the success branch (e.g. fallocate ... && echo "Allocated 1GB disk reserve" || rm -f ...).

8. Documentation

• 8.1 [nit] core/services/versionchooser/utils/chooser.py:352-369 — the retry-on-full-disk trick is non-obvious. A one-line comment above the try (something like # On a full disk, docker can't rewrite overlay metadata; free our reserve and retry once.) would save the next reader a trip to the linked issue.
• 8.2 [nit] core/start-blueos-core:104-113 — the TOTAL_SPACE_MB > 7 * 1024 threshold is a magic number without justification; a brief # skip on small (<7GB) media where 1GB is too large a slice would clarify intent.

7. Tests

• 7.1 [nit] No pytest coverage added for the retry-on-failure branch or the exists()/unlink() behavior. The path is inherently disk-full-dependent and hard to reach in CI, but a simple test that mocks client.images.delete to raise once then succeed, and asserts the reserve file is unlinked, would lock the behavior in.

Generated by PR Review Bot. This is advisory, a human reviewer must still approve.