fix(engine): stand down when the installation is updated underneath (#963) - #965
Merged
Conversation
…963) Updating an install rewrites scripts in place while an engine may be running. The engine used to find out only by executing a half-written driver script -- and only when a spawn was unlucky enough to hit the write window (observed live: a mid-rewrite storage-sync-driver.sh failed to parse and the fatal named a syntax error, not the update). The engine that missed the window kept running pre-update code indefinitely, which is the state watch.sh already refuses by name. The engine now runs watch.sh's check at every cycle boundary, before any driver can be spawned: anything under scripts/ whose mtime is newer than the engine's start means the code on disk is no longer the code in memory. It emits a stand-down event naming the update, the changed path, and the restart command, then returns cleanly (exit 0). The pidfile is left in place on purpose: status reports the engine as stale and its stale line already names 'remote.sh sync start <team>', so detection-then-restart is one road with no new stop command needed. The failure direction is pinned the same way watch.sh pins it: an observation failure is not evidence. An unreadable directory, a failed stat, a vanished tree are each skipped, and only a successfully read newer mtime proves an update -- the engine must not stand down on proof it failed to collect. Unit tests cover the stand-down (before any cycle, event fields, clean resolve), the keep-running-on- observation-failure rule, and the positive-proof contract of the detector itself.
Review supplied a counterexample to comparing mtimes against the engine's start clock: a file that already carried a future mtime when the engine started (clock skew, an archive with preserved timestamps, a clock stepped backwards) reads as 'written after start' forever and stands down every fresh engine at its first cycle -- including one already running the new code. That is exactly the stand-down-when-it- should-not path this change was required not to create. The detector now takes a complete baseline of the scripts tree at engine start (path -> mtime) and treats only a DIFFERENCE against it as proof: a file that appeared, or whose mtime changed in either direction (which also catches updates that preserve older mtimes -- a direction an ordering test cannot see). A pre-existing future mtime is simply what the tree looked like at start. The observation-failure rule now has two phases. Baseline phase: one unreadable directory or failed stat disables the detector entirely rather than arming part of it -- a partial baseline would recreate the false positive when an initially-unreadable pre-existing file becomes readable later. Check phase: unreadable entries are skipped and prove nothing. Tests pin the reviewer's positive control (a fresh engine over a future-mtime tree does not stand down), the preserved-older- mtime rewrite, the disarmed-detector path, and the added-file proof.
Second review counterexample: an mtime difference does not prove a content difference. A touch, a metadata-only correction, a same-content re-copy would have stood the engine down with the code in memory and on disk still identical -- and a stood-down sync engine does not come back by itself, so a false positive is a stopped sync, not a noisy log line. The baseline now records a sha256 per file alongside the mtime. A path absent from the baseline is proof on its own (a new install artifact). For a known path, an mtime difference only nominates the file for re-reading: matching bytes are a benign touch (remembered, so the file is not re-read every cycle), and only bytes that actually differ stand the engine down. Cost: one full read+hash of scripts/ (1.9MB, 123 files) at engine start, stat-only sweeps afterwards. Also updates the installer's post-update warning to the wording agreed for the release this lands in: the engine stands down when it can tell it was updated and keeps running pre-update code when it cannot -- replacing the '#964' text that becomes false once this ships. Tests pin the touch-with-unchanged-content positive control, the no-re-read memoization, the unreadable-reread yields-no-evidence rule, and the existing future-mtime and disarmed-detector contracts.
fujibee
force-pushed
the
fix/engine-update-standdown
branch
from
August 22, 2026 17:50
2e9d926 to
38b8dec
Compare
… check fails The stdout-channel case failed twice on CI macOS with '2 !== 1' and no way to tell from the log what the second line was -- the captured writes never reach the terminal. The count assertion now carries the captured lines, so the next failure names the leak instead of its size.
The diagnostic named it: the second 'line' in the bootstrap stdout check was the runner's serialized test:complete frame for the PRECEDING test -- under node --test the runner transports results over the same stdout the test patches, and on a slow machine the frame for the previous test flushes into the patched window. This branch made that likely by accident: runLoop tests that do not inject the new baseline dependency were paying a real read+hash of scripts/ (1.9MB), slowing the test just ahead of the window. Locally and on the ubuntu shard the frame lands before the patch; on the macOS runner it landed inside, twice. Two test-only changes. The nine runLoop cases that are not about the detector now inject a disarmed baseline, so no unit test walks the real scripts tree as a side effect. And the bootstrap case now judges by content -- exactly one pull_bootstrap_result line, no progress marker on stdout -- instead of counting raw lines, because the count also counts the runner's transport, which no real consumer of pullBootstrap ever sees (in production this code does not run under the test runner).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second half of #963 (the installer-warning half is #964). The sync engine now does what watch.sh already does: notice that the installation was updated underneath it and stand down deliberately, instead of either dying on a half-written script or running pre-update code forever.
The two endings this replaces
A running engine that overlaps an
install.sh --updatehad two endings, and the quiet one was worse: a driver spawned mid-rewrite reads a partial file and the engine dies on a parse error that names the wrong cause (observed live on 2026-08-22:storage-sync-driver.sh line 42: syntax error, on a file that parses clean afterwards); an engine that spawns nothing during the window survives and keeps executing the old code indefinitely, whilestatusreports it running — the exact state watch.sh refuses by name.What the engine does now
At every cycle boundary, before any driver can be spawned, the engine checks whether anything under
scripts/has an mtime newer than its own start — the same predicate watch.sh has run in production (find -neweragainst a start stamp). On detection it emits astand-downevent naming the update, the changed path, and the restart command, then returns cleanly with exit 0. The pidfile is intentionally left in place:statusthen reports the engine as stale, and the stale line already tells the operator to runremote.sh sync start <team>— so detection and recovery are one road, and no newsync stop/restartsurface is needed (the alternative considered and not taken, per review discussion on #964).The failure direction is pinned
An observation failure is not evidence, and the engine must not stand down on proof it failed to collect. An unreadable directory, a failed stat, a vanished tree are each skipped; the check's own exceptions are swallowed at the call site; only a successfully read mtime newer than the engine's start proves an update. watch.sh holds the same line (a missing stamp reads as "not changed", its find's errors are discarded), so the two readers now fail in the same safe direction. Misfire and miss both degrade to today's behavior: a stood-down engine is revived by
sync start, a missed update behaves exactly as before this change.Tests
Three new cases in
tests/remote_sync_engine.test.mjs(suite 108/108 green): the stand-down fires before any cycle and resolves rather than rejects, with the event naming the update and the per-team restart command; a throwing detector keeps the loop cycling (observation failure ≠ evidence); the detector itself returns null for pre-existing trees and unreadable roots and a path only for a genuinely newer file. The default detector was also exercised against a real scripts tree (null before a write, the file's path after).Scope:
scripts/internal/remote-sync.mjs(one helper + the cycle-boundary check) and the engine unit suite. No shell surfaces changed; no overlap with #960's codex-bridge files.