fix(update): keep the in-flight update receipt across the stale-module purge - #101691
LordMelkor wants to merge 1 commit into
Conversation
…e purge `_purge_stale_hermes_modules()` runs right before the gateway-restart phase and evicted `hermes_cli.update_receipt` along with everything else under the Hermes prefixes. That module holds the open receipt as a module-level singleton (`_current`, set by `begin_update_receipt()` at the top of the run). Every later lazy `from hermes_cli.update_receipt import finalize_update_receipt` — the success path and the command-boundary safety net in main.py — bound a fresh module with no open receipt, so `finalize_update_receipt()` returned None and a successful `hermes update` wrote nothing to logs/update_receipts/. Only runs that failed before the purge (dependency install, fetch, preflight) left receipts, which hid the gap. Add `hermes_cli.update_receipt` to `_STALE_PURGE_PROTECTED`: it carries this run's process state, the same category as the executing modules the set already protects, and the running frames keep the module object alive regardless. Regression test: begin a receipt, purge, re-import (same object, receipt intact), finalize → file written under HERMES_HOME. The test file's autouse sys.modules restore now also re-points parent-package submodule attributes, which a post-purge import rebinds to fresh module objects; pytest's monkeypatch resolves dotted targets through those attributes, so a stale binding made later tests patch a module object no import uses. Field symptom: the Desktop's managed SSH update waits for a correlated receipt after the exit marker and reports "Update failed" even though the update completed (refs NousResearch#101516). Co-authored-by: Claude Code <noreply@anthropic.com> Ai-assisted: true (cherry picked from commit e5afa28c6073ac60f135012917d099e05c70fb80)
Duplicate of #92934. Both patches preserve hermes_cli.update_receipt through the stale-module purge so the active receipt can be finalized after a successful update. |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head d622a38368217f1f772cc9ca280530288401ec52 against live main@6064668c8fd2dbbb232ea073b32c9d06d932fa56, including the full two-file diff, the full stale-module regression file, update_receipt.py's _current lifecycle/finalization semantics, exact-head CI/Docker/Nix, the existing review/comment history, the duplicate lineage, the managed-SSH correlation sibling, and the active updater decomposition.
The actual mechanism is sound. hermes_cli.update_receipt is not ordinary cached implementation code here; it owns transaction state begun before the checkout mutation and consumed after _purge_stale_hermes_modules(). Evicting that module strands _current in the old module object, and the later lazy finalizer sees a new module with _current is None. Protecting it is therefore the right class of fix. The new test is particularly good because it pins the mechanism (reimported is ur) as well as the durable postcondition (a successful receipt is written), so a future accidental "green by some other path" cannot satisfy it.
Exact-head hosted verification is also green for this object: CI run https://github.com/NousResearch/hermes-agent/actions/runs/33691288926, Docker run https://github.com/NousResearch/hermes-agent/actions/runs/33691287968, and Nix run https://github.com/NousResearch/hermes-agent/actions/runs/33691287973 all completed successfully on d622a383…. I do not see a defect in the receipt-preservation implementation itself.
I would still hold this exact object from merge for three repository-topology blockers:
1. This is an existing fix lineage, not a new independent implementation.
#92934 (bluefateludi, opened Aug 23) already carries the same production hunk — adding hermes_cli.update_receipt to _STALE_PURGE_PROTECTED — and the same begin → purge → re-import → finalize regression shape. It fixes the earlier #92535, which documents this exact root cause. #92934 also accumulated independent macOS/Linux/Windows validation, including real updater receipts. Its earlier review even requested the explicit same-module identity assertion that this PR now includes.
#101690 is now duplicate-labeled, and this PR's own timeline already has the duplicate pointer to #92934. Because #92934 is currently stale/unmergeable, a current-main salvage/restack can absolutely be the right landing route; the missing piece is attribution/interlock. Please make that route explicit: preserve #92934 / #92535 as the predecessor lineage and bluefateludi's contribution rather than silently replacing it, and reconcile the closing reference accordingly. Duplicate, superseding, and credited salvage are different states and the PR should say which one this is.
2. The reviewed head is no longer current-main-composed.
Live comparison of main@6064668c8fd2dbbb232ea073b32c9d06d932fa56 to d622a383… reports diverged, with this head 1 ahead / 11 behind and merge base 6441d3a916d74d781860e996cb70bc2d60094ab9.
That means the green runs above prove the exact reviewed head, but not the final composition with today's main. Rebase/restack onto current main, resolve the updater-file collision deliberately, then rerun every required CI/Docker/Nix gate on the new exact head. The previous green receipts should remain historical evidence, not be transferred to the rebased object.
3. Merge order is coupled to the live update_cmd.py decomposition in #97634.
This PR modifies hermes_cli/update_cmd.py, which still has live content beyond line 3000 on this head. #97634 is the active decomposition owner for that file: it moves post-source module refresh into hermes_cli/update_runtime_refresh.py and leaves a small compatibility facade. Its current patch has its own _STALE_PURGE_PROTECTED set containing the extracted updater graph — but, because that branch predates this bug fix, it currently does not include hermes_cli.update_receipt.
So there is a real two-sided merge hazard, not merely a textual conflict. If this PR lands first, #97634 must absorb the new transaction-state protection and this regression when it rebases; otherwise the decomposition can reintroduce the successful-update receipt loss. If #97634 lands first, this fix belongs in the extracted runtime-refresh owner instead of regrowing the facade. Please interlock those two objects explicitly; do not create another competing extraction.
The adjacent #101516 managed-SSH failure is complementary, not superseded: preserving a receipt makes the durable object exist; the separately owned correlation_id work makes Desktop select the receipt for the exact update transaction. Both are required for that end-to-end path, and keeping those scopes separate is the right coordination choice.
Once the predecessor credit, current-main restack, and #97634 merge-order interlock are repaired — and the resulting exact head is fully green — I see this as a clean, narrowly correct fix. The underlying diagnosis and regression are strong work. 🚀
|
🤖 Closing as a duplicate of #92934, which has carried the same protected-module change and regression test since Aug 23. The one piece not in that PR is the |
Summary
Successful
hermes updateruns write no update receipt._purge_stale_hermes_modules()runs right before the gateway-restart phase and evictshermes_cli.update_receiptfromsys.modules, but that module holds the in-flight receipt as a module-level singleton (_current). Every later lazyfrom hermes_cli.update_receipt import finalize_update_receipt(the success path and the command-boundary safety net inmain.py) binds a fresh module with no open receipt, sofinalize_update_receipt()returnsNoneand nothing reacheslogs/update_receipts/.Runs that fail before the purge (dependency install, fetch, preflight) still write receipts, which is why the gap went unnoticed.
Fixes #101690. Related: #101516 (the Desktop's managed SSH update needs a receipt that carries its
correlation_id; with this bug the receipt does not exist at all, so that fix alone cannot make the Desktop report success).Change
hermes_cli/update_cmd.py: addhermes_cli.update_receiptto_STALE_PURGE_PROTECTED, with the rationale in the comment above the set. It carries this run's process state, the same category as the executing modules already protected; the running frames keep the module object alive regardless, so evicting it buys nothing.tests/hermes_cli/test_update_stale_module_purge.py:test_purge_keeps_in_flight_update_receipt: begin a receipt, run the purge, re-importhermes_cli.update_receipt(same object,_currentintact), finalize → receipt file written underHERMES_HOME._restore_sys_modulesfixture now also restores parent-package submodule attributes. A post-purge import rebinds e.g.hermes_cli.configon thehermes_clipackage to the fresh module object; pytest'smonkeypatch.setattr("hermes_cli.config.get_hermes_home", …)in later tests resolves through that attribute and would otherwise patch a module object no import statement uses (test_update_receipt.pyfailed this way when run after the new test).Verification
pytest tests/hermes_cli/test_update_stale_module_purge.py tests/hermes_cli/test_update_receipt.py→ 31 passed.pytest tests/hermes_cli/test_update*.py→ 18 failed / 670 passed with this change vs 19 failed / 670 passed on the pristine tree in the same environment (macOS; order-dependent launchd, lock and wedged-gateway tests). No failure is introduced by this change.✓ Update complete!, gateway handed back to its supervisor, fleet check up to date, exit code 0, left no receipt andlatest.jsonstill pointing at an earlier failed run. The in-process reproduction from the issue returnsNoneonmain; with this change the re-import is the same module object and the receipt is written.