fix(photon): persist send and reaction state - #56006
Conversation
|
Review follow-up pushed in Validation against the mirrored review comments from hashbender#100:
Latest local validation on this PR branch:
|
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tackling restart durability and for addressing the earlier state-store review findings. The persistence premise remains real on current main: Photon keeps sent IDs and inbound targets only in adapter memory (plugins/platforms/photon/adapter.py:329, 1215-1252).
Problems
plugins/platforms/photon/state.py:263writes the whole loaded JSON snapshot, while the PR's shared store is only module-local (plugins/platforms/photon/adapter.py:120-129). Current main documents_standalone_sendas an out-of-process cron path (plugins/platforms/photon/adapter.py:1657-1660), so a gateway process and cron process can load different snapshots and atomically overwrite each other's records. Atomic replacement prevents torn JSON, not lost updates.plugins/platforms/photon/state.py:310calls Unix-onlyos.fchmodunconditionally. The existing helper explicitly guards it for Windows (utils.py:175-179), with coverage intests/hermes_cli/test_atomic_json_write.py:136-149.
Suggested changes
- Make state mutations cross-process transactional and add a gateway/standalone multi-process lost-update regression.
- Guard
fchmodas the shared helper does and test marker persistence without it.
This is an automated hermes-sweeper review.
| self._state = self._normalize(self._state) | ||
| self._state["updated_at"] = _now_iso() | ||
| try: | ||
| atomic_json_write( |
There was a problem hiding this comment.
atomic_json_write protects against a torn file, but not a lost update: gateway and _standalone_send can run in separate processes, each retaining its own loaded _state snapshot. Reload-and-merge under an inter-process lock (or use a transactional store) before replacing this file, and add a multiprocess regression.
| marker.parent.mkdir(parents=True, exist_ok=True) | ||
| fd = os.open(marker, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | ||
| with os.fdopen(fd, "w", encoding="utf-8") as handle: | ||
| os.fchmod(handle.fileno(), 0o600) |
There was a problem hiding this comment.
os.fchmod is unavailable on Windows. Match utils.atomic_json_write by guarding it with hasattr(os, "fchmod"); otherwise the write-error marker cannot be recorded on that platform.
What does this PR do?
Photon can now survive adapter restarts without forgetting the messages it sent, the latest inbound target per chat, or active reaction IDs needed for later removal. This makes send/reaction handling durable across the sidecar and standalone send paths, and exposes a small
hermes photon statusstate summary so operators can see whether local state is healthy.The change also lets deferred bundled platform CLIs load when their matching top-level command is invoked, so
hermes photon ...can resolve the Photon plugin CLI without eagerly loading every bundled platform.Related Issue
Related to #43726, #53451, and #55105.
Type of Change
Changes Made
/unreact, including DM alias fallback and restart recovery.hermes photon status.Review Follow-up
1loads normally, invalid schema values still fail open, and newer schema versions warn while preserving known fields.PhotonStateStorekeyed byphoton_state_path(), preventing the stale adapter-store overwrite described in the review.hermes photon statuscan now observe the last write failure through a private siblingstate.json.write_errormarker, and a later successful persist clears that marker.How to Test
scripts/run_tests.sh "tests/plugins/platforms/photon/test_markdown.py" "tests/plugins/platforms/photon/test_state.py" "tests/plugins/platforms/photon/test_status.py" -q-> 24 passed.scripts/run_tests.sh "tests/plugins/platforms/photon" "tests/hermes_cli/test_plugins.py::TestPluginDiscovery::test_deferred_bundled_platform_cli_loads_on_matching_command" -q-> 131 passed on the PR branch.git diff --check "huntsyea-fork/fix/photon-persistent-state"..HEADpython -m py_compile plugins/platforms/photon/adapter.py plugins/platforms/photon/state.py plugins/platforms/photon/cli.py hermes_cli/plugins.py hermes_cli/main.pyscripts/run_tests.sh "tests/" -qcompleted locally with 36,581 passed and 47 failures in unrelated existing ACP/gateway/tool-approval/macOS-path areas; all Photon files in that run passed.Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) - or N/Acli-config.yaml.exampleif I added/changed config keys - or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows - or N/AScreenshots / Logs
Not applicable; this is Photon state/reaction persistence and CLI loading hardening.