@@ -227,6 +227,53 @@ void copy_first_rule(const std::filesystem::path& from, const std::filesystem::p
227227// The BMI equivalence check, which used to be a POSIX shell one-liner inside the
228228// generated ninja command — and was therefore skipped entirely on Windows.
229229// Having it here is what brings cascade suppression to every platform.
230+ // A BMI's IDENTITY, so publication can be detected without the file ever having
231+ // to be absent.
232+ //
233+ // ⚠️ THE OLD DESIGN CREATED THE HOLE IT WAS TRYING TO AVOID. Phase 1 used to
234+ // `rename(bmi, bmi.bak)` before spawning the compiler — the comment said it was
235+ // so "its mere presence can never be mistaken for the new one landing". That is
236+ // a real hazard, but the cure left the module with NO BMI ON DISK from that
237+ // rename until the compiler republished: measured at ~208 ms of a single
238+ // incremental rebuild. Any importer scheduled inside that window dies with
239+ //
240+ // error: failed to read compiled module: No such file or directory
241+ // note: imports must be built before being imported
242+ //
243+ // reproducible at `-j1`, on four of six scenarios of the generated fixture.
244+ //
245+ // The previous BMI is now COPIED aside instead, so the file is continuously
246+ // readable and GCC's own atomic rename is what replaces it. Publication is
247+ // detected by the identity below changing, which is exactly the question the
248+ // existence check was a poor proxy for.
249+ struct BmiIdentity {
250+ bool present{};
251+ std::uintmax_t size{};
252+ std::filesystem::file_time_type mtime{};
253+ bool operator ==(const BmiIdentity&) const = default ;
254+ };
255+
256+ BmiIdentity bmi_identity (const std::filesystem::path& p) {
257+ BmiIdentity id;
258+ if (p.empty ()) return id;
259+ std::error_code ec;
260+ // ⚠️ ASSIGN NOTHING BEFORE CHECKING `ec`. `file_size` returns
261+ // `static_cast<uintmax_t>(-1)` when it fails, so writing it into the struct
262+ // first makes "this file is missing" compare UNEQUAL to a default-built
263+ // identity — which is exactly the sentinel used for "there was no previous
264+ // BMI". Phase 1 then saw a difference on its very first poll and returned
265+ // before the compiler had produced anything, and every object edge failed
266+ // with `no compiler was started … phase 1 did not run`.
267+ const auto size = std::filesystem::file_size (p, ec);
268+ if (ec) return id;
269+ const auto mtime = std::filesystem::last_write_time (p, ec);
270+ if (ec) return id;
271+ id.present = true ;
272+ id.size = size;
273+ id.mtime = mtime;
274+ return id;
275+ }
276+
230277// Put the previous BMI back. Used when the compile failed: the unit still has
231278// the BMI it had before, and leaving it parked in `.bak` would strand every
232279// importer on a file that does not exist.
@@ -243,8 +290,11 @@ void settle_bmi(const std::filesystem::path& bmi) {
243290 const auto backup = suffixed (bmi, " .bak"
244291 if (!file_exists (backup)) return ;
245292 std::error_code ec;
293+ // Equivalent → put the PREVIOUS file back, so its mtime does not advance and
294+ // ninja's restat stops the cascade. The rename is atomic, so the BMI is
295+ // readable throughout: there is no moment at which importers see nothing.
246296 if (stage::bmi_equivalent (bmi, backup))
247- std::filesystem::rename (backup, bmi, ec); // keep the old mtime: no cascade
297+ std::filesystem::rename (backup, bmi, ec);
248298 else
249299 std::filesystem::remove (backup, ec);
250300}
@@ -369,12 +419,27 @@ int compile_release_at_bmi(const CompileRequest& req) {
369419 std::filesystem::remove (suffixed (req.slot , " .rc"
370420 std::filesystem::remove (suffixed (req.slot , " .rc.tmp"
371421
372- // Keep the previous BMI for the equivalence check AND get it out of the
373- // way, so its mere presence can never be mistaken for the new one landing.
422+ // Keep the previous BMI for the equivalence check, and LEAVE THE ORIGINAL
423+ // IN PLACE — see BmiIdentity for why moving it away is what broke importers.
424+ // Snapshot through the SAME function in both cases, so "no previous BMI"
425+ // and "the BMI as it is now" are directly comparable.
426+ const BmiIdentity before = bmi_identity (req.bmi );
374427 if (!req.bmi .empty ()) {
375428 const auto backup = suffixed (req.bmi , " .bak"
376429 std::filesystem::remove (backup, ec);
377- if (file_exists (req.bmi )) std::filesystem::rename (req.bmi , backup, ec);
430+ if (before.present ) {
431+ std::filesystem::copy_file (
432+ req.bmi , backup, std::filesystem::copy_options::overwrite_existing, ec);
433+ // ⚠️ AND CARRY THE MTIME ACROSS. `copy_file` stamps the copy with
434+ // the time of the copy, and `settle_bmi` restores this file when the
435+ // new BMI turns out equivalent — precisely so the mtime does NOT
436+ // advance and ninja's restat stops the cascade. Without this line
437+ // the restore moves the mtime forward instead, every importer is
438+ // rebuilt, and the optimisation is silently off: `touch-hub` came
439+ // back at 12.61s against a 12.37s cold build, with every cell
440+ // reporting `ok`. A status column cannot catch that; the number can.
441+ std::filesystem::last_write_time (backup, before.mtime , ec);
442+ }
378443 }
379444
380445 const auto token = acquire_token (req.semaphore , req.maxCompilers );
@@ -389,7 +454,9 @@ int compile_release_at_bmi(const CompileRequest& req) {
389454 if (!spawn_detached (sup)) return 2 ;
390455
391456 for (;;) {
392- if (!req.bmi .empty () && file_exists (req.bmi )) {
457+ // Published = the file's identity is no longer the one we snapshotted.
458+ // For a unit with no previous BMI that reduces to "it now exists".
459+ if (!req.bmi .empty () && bmi_identity (req.bmi ) != before) {
393460 settle_bmi (req.bmi );
394461 copy_first_rule (req.depFrom , req.depTo , req.bmi .string ());
395462 return 0 ; // importers may proceed
0 commit comments