Skip to content

Commit 161cb40

Browse files
committed
fix: strict marker-only on resolve path, no legacy adoption
Round-5 review fix: is_install_complete() is now strict marker-only. No more legacy heuristic fallback on the resolve/install path. Rationale: from directory layout alone we cannot distinguish a legacy-complete install (bin/ exists, full) from a half-extracted residue (bin/ exists, partial). Adopting the latter silently corrupts the user's toolchain. Strict semantics close this gap. Cost: upgrade users do a one-time reinstall per toolchain. In practice this hits the fast copy_xpkg_from_global() fallback that reuses ~/.xlings/, so it's rarely a real download. clean_all_incomplete() (mcpp self init) still preserves legacy packages (no marker + legacy layout) as user-visible assets — that's a separate concern from the resolve path's strictness. looks_complete_legacy() is now exported for explicit legacy-aware call sites (currently only clean_all_incomplete uses it).
1 parent d8ff4ca commit 161cb40

2 files changed

Lines changed: 39 additions & 21 deletions

File tree

src/fallback/install_integrity.cppm

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,19 @@ export namespace mcpp::fallback {
3333
// Marker file name written into xpkg directories after successful install.
3434
inline constexpr std::string_view kInstallMarker = ".mcpp_ok";
3535

36-
// Check whether an xpkg directory looks like a complete installation.
37-
// Returns true if:
38-
// - .mcpp_ok marker exists, OR
39-
// - (backward compat) bin/, lib/, or include/ exists
36+
// Check whether an xpkg directory has the .mcpp_ok marker.
37+
// STRICT marker-only — does not fall back to legacy heuristics.
4038
bool is_install_complete(const std::filesystem::path& xpkgDir);
4139

40+
// Heuristic check for pre-.mcpp_ok packages (upgrade compat).
41+
// Returns true if the directory looks like a complete legacy install
42+
// based on layout (top-level bin/lib/lib64/include/share, or a single
43+
// subdirectory containing src/ or mcpp.toml).
44+
// Use ONLY for one-time legacy adoption or to avoid deleting old packages;
45+
// do NOT use this to decide "skip install" on the active install path —
46+
// that's what is_install_complete()'s strict semantics protect against.
47+
bool looks_complete_legacy(const std::filesystem::path& xpkgDir);
48+
4249
// Write the .mcpp_ok marker into a directory, marking it as complete.
4350
void mark_install_complete(const std::filesystem::path& xpkgDir);
4451

@@ -60,11 +67,8 @@ int clean_all_incomplete(const std::filesystem::path& xpkgsBase);
6067

6168
namespace mcpp::fallback {
6269

63-
namespace {
64-
65-
// Heuristic: does this directory look like a complete xpkg?
66-
// Used ONLY for legacy migration (pre-.mcpp_ok packages).
6770
bool looks_complete_legacy(const std::filesystem::path& xpkgDir) {
71+
if (!std::filesystem::exists(xpkgDir)) return false;
6872
// xim toolchain/tool packages: top-level bin/lib/lib64/include/share
6973
for (auto dir : {"bin", "lib", "lib64", "include", "share"}) {
7074
if (std::filesystem::exists(xpkgDir / dir))
@@ -87,8 +91,6 @@ bool looks_complete_legacy(const std::filesystem::path& xpkgDir) {
8791
return false;
8892
}
8993

90-
} // namespace
91-
9294
// Strict: has .mcpp_ok marker (written only on verified success).
9395
bool has_marker(const std::filesystem::path& xpkgDir) {
9496
return std::filesystem::exists(xpkgDir / std::string(kInstallMarker));
@@ -97,14 +99,17 @@ bool has_marker(const std::filesystem::path& xpkgDir) {
9799
bool is_install_complete(const std::filesystem::path& xpkgDir) {
98100
if (!std::filesystem::exists(xpkgDir)) return false;
99101

100-
// Primary: .mcpp_ok marker — the only fully trusted signal.
101-
if (has_marker(xpkgDir)) return true;
102-
103-
// Read-only legacy fallback: recognize pre-upgrade packages so they
104-
// aren't treated as missing by resolve_xpkg_path(). Does NOT write
105-
// any marker. Does NOT prevent clean_incomplete_install() from
106-
// cleaning — that function uses marker-only semantics.
107-
return looks_complete_legacy(xpkgDir);
102+
// STRICT marker-only.
103+
// Used on the install/resolve path — half-extracted dirs with bin/
104+
// would otherwise be mistaken for complete packages.
105+
//
106+
// Legacy packages (installed before .mcpp_ok existed) will trigger
107+
// a one-time reinstall after upgrade. This is the cost of strict
108+
// semantics; the alternative (legacy heuristic) shields half-extracted
109+
// packages from cleanup and re-introduces the very bug we're fixing.
110+
// The reinstall is cheap because copy_xpkg_from_global() is the
111+
// typical fallback path — it reuses the existing ~/.xlings/ copy.
112+
return has_marker(xpkgDir);
108113
}
109114

110115
void mark_install_complete(const std::filesystem::path& xpkgDir) {

src/pm/package_fetcher.cppm

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -647,15 +647,28 @@ Fetcher::resolve_xpkg_path(std::string_view target,
647647
return payload;
648648
};
649649

650-
// ─── Resolution chain: check → clean residue → install → fallback ─
650+
// ─── Resolution chain: marker check → clean residue → install → fallback ─
651651

652-
// 1. Already installed and complete?
652+
// 1. Already installed and complete (has .mcpp_ok marker)?
653+
//
654+
// Strict marker-only. We do NOT do legacy heuristic adoption here
655+
// because we cannot distinguish a legacy-complete install (has bin/)
656+
// from a half-extracted residue (also has bin/). Adopting the latter
657+
// would silently corrupt the user's toolchain.
658+
//
659+
// Cost for users upgrading mcpp: a one-time reinstall per toolchain.
660+
// The install path normally hits copy_xpkg_from_global() as a fast
661+
// fallback (reuses ~/.xlings/ copy), so this is rarely a real download.
653662
if (mcpp::fallback::is_install_complete(verdir)) {
654663
mcpp::log::debug("fetcher", "install complete in sandbox");
655664
return make_payload();
656665
}
657666

658-
// 2. Directory exists but incomplete (Ctrl+C / interrupted) → clean up.
667+
// 2. Directory exists without marker → either interrupted install
668+
// or legacy package. Either way, the safe action is to clean and
669+
// re-resolve (install or copy fallback will produce a marked
670+
// installation). For legacy packages this is wasteful but correct;
671+
// for half-extracted residue it's required.
659672
mcpp::fallback::clean_incomplete_install(verdir);
660673

661674
// 3. Install via xlings (primary path).

0 commit comments

Comments
 (0)