Skip to content

fix(health-check): cross-check content before flagging a compiled artifact stale - #2329

Merged
john-the-dev merged 9 commits into
sonichi:mainfrom
amkunte:fix/health-check-compiled-artifact-crosscheck
Jul 27, 2026
Merged

john-the-dev merged 9 commits into
sonichi:mainfrom
amkunte:fix/health-check-compiled-artifact-crosscheck

Conversation

@amkunte

@amkunte amkunte commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Problem

mark_stale_if_outdated in src/health-check.py makes three mtime comparisons. Two of them ask git whether the content actually changed before flagging — the process-start path (#253) and the bridges path (#255) — because git checkout, pull, and rebase bump mtime on files that are byte-identical. The compiled-artifact branch (binary_path is not None) arrived later in #529 and is the only one of the three without that cross-check, so any mtime bump older than the threshold reports "rebuild needed" regardless of whether the source changed.

Observed 2026-07-26 on a deployment host: upstream-sync fast-forwarded main and restored the deployment branch with a checkout. That restore restamped 116 byte-identical files, src/Sutando/main.swift among them, and health-check reported sutando-app: stale — rebuild needed. The binary was current — the source's last content change predated the build by six days. The sync runs daily, so the false alarm did too, which is the failure mode where a health channel stops being read.

Fix

Reuse the existing _file_unchanged_since helper, with the binary's build time as the reference point: if the source content at build time is byte-identical to the content now, the mtime bump was idempotent and the binary is current. This is the same treatment #253 and #255 already applied to the other two comparisons; the compiled-artifact branch is the last one missing it.

The helper already fails safe — it returns False on any git error — so an unresolvable cross-check still flags rather than hiding a genuinely stale binary.

Evidence — real binary, real repo

Simulating the checkout restamp on src/Sutando/main.swift (content unchanged since 2026-07-18; binary built 2026-07-24 11:57):

Before (src/health-check.py at the parent commit):

  ♻ sutando-app         stale   running, but binary is 2698 min older than source — rebuild needed
1 issue(s) found:
  - sutando-app: stale (running, but binary is 2698 min older than source — rebuild needed)

After (same file, same restamp, this PR applied):

  ✓ sutando-app         ok      running (hotkeys: ⌃⇧C/⌃S/⌃⇧R/⌃V/⌃M)

Guard rail — same host, but with a genuine post-build content edit to main.swift. It must still flag, and does:

  ♻ sutando-app         stale   running, but binary is 2699 min older than source — rebuild needed
1 issue(s) found:
  - sutando-app: stale (running, but binary is 2699 min older than source — rebuild needed)

So the change suppresses the mtime-only case and nothing else.

Tests

tests/health-check-compiled-artifact-stale.test.py — 5 cases over the compiled-artifact branch: binary newer than source; idempotent mtime bump (the regression); genuine content change; cross-check unresolvable (fail safe); binary absent.

The suite gates both ways:

$ git stash push src/health-check.py   # revert just the fix
$ python3 tests/health-check-compiled-artifact-stale.test.py
FAIL: test_idempotent_mtime_bump_is_not_stale
AssertionError: 'stale' != 'ok'
Ran 5 tests — FAILED (failures=1)

$ git stash pop
$ python3 tests/health-check-compiled-artifact-stale.test.py
Ran 5 tests — OK

One fixture note worth flagging for review: _file_unchanged_since resolves the tree via git reflog HEAD --before=@<ts>, so a fixture that commits "now" but dates the binary hours earlier has no reflog entry at-or-before the build — every case then fails safe and the suite passes for the wrong reason, unable to exercise the fix at all. The fixture backdates its commits via GIT_COMMITTER_DATE so the timeline matches reality (commit, then build). This bit the first draft of the test.

🤖 Generated with Claude Code

…ifact stale

`mark_stale_if_outdated` makes three mtime comparisons. The process-start
path (sonichi#253) and the bridges path (sonichi#255) both ask git whether the content
actually changed before flagging, because `git checkout`, `pull`, and
`rebase` bump mtime on files that are byte-identical. The compiled-artifact
branch arrived later (sonichi#529) and is the only one without that cross-check,
so any mtime bump older than the threshold reports "rebuild needed"
regardless of whether the source changed.

Observed 2026-07-26: `upstream-sync` fast-forwarded `main` and restored the
deployment branch with a checkout. That restore restamped 116 byte-identical
files, `src/Sutando/main.swift` among them, and health-check reported
`sutando-app: stale — binary is 2679 min older than source — rebuild
needed`. The binary was current; the source's last content change predated
the build by six days. Because the sync runs daily, so did the false alarm.

Reuses the existing `_file_unchanged_since` helper with the binary's build
time as the reference point: if the source content at build time matches the
content now, the bump was idempotent and the binary is current. The helper
already fails safe (returns False on any git error), so an unresolvable
cross-check still flags.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 65%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • src/health-check.py (100%)

Summary

  • Total: 10 lines
  • Missing: 0 lines
  • Coverage: 100%

…gate

The `--fix` path for sutando-app decides whether it may launch the existing
binary with a bare mtime comparison (`binary.st_mtime >= source.st_mtime`).
That is the same defect as the stale check this branch already fixes, in the
opposite direction: after a checkout restamps `main.swift`, the gate refuses
to launch a current binary and prints "needs manual rebuild + relaunch", so
a down app stays down until someone rebuilds what did not need rebuilding.

Extracts `_binary_is_current(binary, src)` — mtime ordering OR an idempotent
bump proven by the existing content cross-check — and uses it at both call
sites, so "is this binary current" has one definition. Fails safe on stat
error, matching `_file_unchanged_since`.

A genuine post-build content edit still returns False, so the auto-launch
gate does not become more permissive about actually-stale binaries.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@amkunte

amkunte commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a second commit (628bd614) covering the same defect at its other call site in this file.

--fix's sutando-app auto-launch gate decides binary freshness the same way the stale check did — a bare binary.st_mtime >= source.st_mtime. It fails in the opposite direction: after a checkout restamps main.swift, the gate refuses to launch a current binary and prints needs manual rebuild + relaunch, so a down app stays down until someone rebuilds what didn't need rebuilding.

Both call sites now go through one _binary_is_current(binary, src) predicate — mtime ordering, or an idempotent bump proven by the existing content cross-check. A genuine post-build edit still returns False, so the gate does not get more permissive about actually-stale binaries.

Two things I'd rather flag than let a reviewer discover:

  • The 4 new BinaryIsCurrentTests cases error, not fail, against the parent commit — the symbol they exercise doesn't exist there. An error is a weaker gate than an assertion, so test_idempotent_restamp_is_current additionally pins the delta inline: it asserts the old expression rejects the restamp and the new predicate accepts it. The first commit's regression case (test_idempotent_mtime_bump_is_not_stale) does fail properly at its parent.
  • The auto-launch change is covered at the predicate level only, not end-to-end. Exercising the real gate needs sutando-app in a not running state, which I'm not going to induce on a live host. The open call itself is unchanged; only the condition guarding it moved.

Happy to split this into its own PR if you'd rather keep the first commit single-concern — it's the same root cause and the same file, which is why I pushed here per CONTRIBUTING.

@amkunte

amkunte commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Housekeeping note for whoever reviews this, and a correction on my part.

Commit 1 of this PR duplicates work that already existed. A branch fix/health-check-content-check-on-main (6dfb9195) was prepared and tested yesterday with the same fix — same helper (_file_unchanged_since(src_file, bin_mtime)), same incident cited, 2 files / +177, test 6/6 on the upstream base. I re-derived it from scratch because I didn't read my own current-track.md before starting.

This PR is a superset, so it should be the one that lands: commit 2 fixes the same defect at its other call site — --fix's sutando-app auto-launch gate, which compares binary.st_mtime >= source.st_mtime directly and therefore refuses to launch a current binary after a restamp. 6dfb9195 does not cover that path. The older branch can be dropped.

No change to the diff here; flagging it so the duplication is visible in the record rather than discovered later.

@amkunte

amkunte commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Adversarial self-review of this diff, since it's the standing rule here and I hadn't done it. Three attacks; one real hole found, and it is pre-existing rather than introduced by this PR — disclosing rather than silently widening scope.

Held up:

  • Genuine post-build edit with mtime moving forward → False (correctly stale).
  • Checkout restamp, content identical to build time → True (the fix working).
  • Source reverted to an older revision after a newer build (binary has v2, source now v1) → False. The content compare is against the tree as of the binary's mtime, so a revert doesn't read as "current."

The hole — pre-existing, out of scope for this PR:

If a source is genuinely edited after the build but its mtime is pushed backward behind the binary's, the check reports current while the content differs:

binary built -20h from v1;  src edited to v2, mtime forced to -30h
_binary_is_current(binary, src)                  -> True   ← false-current
binary.st_mtime >= source.st_mtime  (the ORIGINAL) -> True   ← identical

Both spellings return True, so this PR neither introduces nor worsens it — the early bin_mtime >= src_mtime return preserves the original semantics exactly. Closing it means dropping that fast path and always content-checking, which is a behavioural change (and 2 extra git subprocesses on the common path) that doesn't belong in a fix framed as "stop the false positive." Happy to do it as a follow-up if you'd rather.

Worth flagging operationally: the interim remediation for this false alarm — restoring mtimes to last-commit time — creates exactly that backward-mtime shape. It's only safe because it's gated on git status --porcelain being empty, i.e. disk content == committed content, so no post-build edit can exist. That precondition isn't hygiene; it's what keeps the remediation from silencing a real staleness. Anyone reusing the recipe should keep the gate.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving per Chi's explicit go (one-by-one). Review-verified LGTM: adds git content cross-check to the compiled-artifact stale branch, kills the false rebuild-needed alarm; 5-case tests.

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the premise and the mechanism against the source. The diagnosis is correct and the fix is the consistent one. One scoping limitation worth recording, not a blocker.

Premise verified at src/health-check.py. mark_stale_if_outdated does have three comparisons and exactly one lacks the cross-check:

  • compiled-artifact branch, lines 916-926 — bare src_mtime - bin_mtime > threshold_sec, no _file_unchanged_since call
  • process-start path, line 971 — calls _file_unchanged_since
  • bridges path, line 2501 — calls _file_unchanged_since

So "the only one of the three without that cross-check" is accurate as stated.

Fail-open direction is preserved, which is the thing I most wanted to check. _file_unchanged_since returns False on any git error (docstring: "so real stale deploys aren't hidden"). At line 971 False means do-not-suppress → report stale. Reusing it with the binary's build time as the reference keeps that polarity: a git failure still reports stale rather than silently claiming the binary is current. A fix in this area that inverted that would be worse than the false alarm, and this one doesn't.

Limitation: on a deployment host that is not a git checkout, this fix silently does nothing. _file_unchanged_since shells to /usr/bin/git log -1 --format=%ct HEAD -- <src> with cwd=REPO_DIR. Where REPO_DIR has no .git, that returns non-zero, the helper returns False, and the branch reports stale exactly as before. Measured on the host I am on:

$ git rev-parse --is-inside-work-tree
fatal: not a git repository (or any of the parent directories): .git
$ /usr/bin/git log -1 --format=%ct HEAD -- src/Sutando/main.swift
fatal: not a git repository (or any of the parent directories): .git

This is an app-bundled engine tree (REPO_DIR = Path(__file__).parent.parent), not a clone, so every _file_unchanged_since consumer already degrades to no-suppression here. That makes it a pre-existing property of the helper rather than anything this PR introduces — #253 and #255 inherit it too — and your observed failure was on a host that does sync via git, where the fix will work. I raise it only so the PR is not read as "the daily false alarm is now fixed everywhere": on non-git deployment hosts it will still fire, and closing that would need a non-git fallback (e.g. content hash recorded at build time) rather than a git query.

Not approving, because I could not execute this branch — no git checkout on this host, so I could not run tests/health-check-compiled-artifact-stale.test.py or diff the head. Everything above is read from the parent-commit source and from live commands whose output is pasted verbatim. Someone with a real clone should confirm the test fails at the parent and passes at the head before merge.

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 3b8de4f. The bug exists on main: the compiled-artifact freshness path was the lone mtime-only branch. The shared content cross-check preserves fail-safe behavior on git errors and the predicate is reused by the repair gate. Independently ran all 9 focused cases, Python compile, diff check, and hardcoded-path scan; all pass. Live before/after evidence covers an idempotent checkout restamp and a genuine post-build edit. No blocking findings.

@john-the-dev
john-the-dev enabled auto-merge (squash) July 27, 2026 02:59
@john-the-dev
john-the-dev disabled auto-merge July 27, 2026 03:02
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev
john-the-dev merged commit f7a67fd into sonichi:main Jul 27, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants