fix(photon): persist send and reaction state - #100
Conversation
|
Review Complete Files Reviewed: 11 By Severity:
The PR adds Photon platform state persistence and deferred plugin loading. Three issues found: a schema version strict-equality check silently discards state on any version bump, two Files Reviewed (11 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🟡 Medium (48/100) — 2 medium findings, 1 low · 1142 LOC across 11 files
Summary
This PR introduces persistent state tracking for the Photon messaging platform adapter (PhotonStateStore) and adds deferred plugin loading in hermes_cli/plugins.py. The state store manages sent-message IDs, reaction handles, inbound trackers, and audit records that survive adapter restarts.
Findings
finding-001 (Medium): Schema version strict-equality check silently discards all state on any version bump
state.py:99-100 uses if payload.get('schema_version') != SCHEMA_VERSION: raise ValueError. The exception is caught and resets to _empty_state(), silently discarding all persisted correlation data. A future bump of the SCHEMA_VERSION constant from 1→2 would wipe every user's state file with no migration path.
finding-002 (Medium): Two PhotonStateStore instances race on shared state file
Both __init__ (line 267) and _standalone_send (line 1771) create separate PhotonStateStore() instances pointing to the same photon_state_path() file. Concurrent load-modify-write cycles can interleave, producing classic lost-update races that silently drop audit records, sent-message tracking, and reaction state.
finding-003 (Low): Dead code: write_error display in photon status never triggers
cli.py:333 checks health.get('write_error') but write_error is never persisted to the state file — it is only an instance attribute set during _persist() failures. The status command creates a fresh store and calls load(), so write_error is always None and the guard is dead code.
Assessment
No critical or high-severity issues. The two medium findings are real bugs that would manifest on version upgrades and under gateway concurrency. Recommended to fix before merging to avoid silent data loss paths.
| if payload.get("schema_version") != SCHEMA_VERSION: | ||
| raise ValueError("unsupported schema version") |
There was a problem hiding this comment.
🟡 Schema version strict-equality check silently discards all state on any version bump (bug)
PhotonStateStore.load() at state.py:99-100 uses if payload.get('schema_version') != SCHEMA_VERSION: raise ValueError('unsupported schema version'). The exception is caught at line 102, which logs a warning and resets to _empty_state() at line 109 — silently discarding all persistent correlation data. Because SCHEMA_VERSION is a module-level constant (line 21, currently 1), any future bump from 1→2 causes every existing user's state file to fail the check and be wiped. The adapter depends on this state for reaction rehydration (adapter.py:287-305 _hydrate_persistent_state), and the loss breaks cross-restart reaction correlation.
💡 Suggestion: Replace the strict-equality check with a version-range gate. At minimum, accept schema_version <= SCHEMA_VERSION (backward-compatible read). Better: add an _UPGRADERS dict mapping old→new version transformers, called in load() before _normalize(). For forward-compatibility (downgrade scenario), accept schema_version > SCHEMA_VERSION with a warning rather than data loss.
📋 Prompt for AI Agents
In plugins/platforms/photon/state.py, replace line 99 (if payload.get('schema_version') != SCHEMA_VERSION: raise ValueError('unsupported schema version')) with a version-range check. Accept schema versions 1 through SCHEMA_VERSION (i.e., >= 1 and <= SCHEMA_VERSION). If stored version < current, run a migration transformer before _normalize(). If stored version > current, log a warning but proceed without discarding state.
| state = PhotonStateStore() | ||
| state.load() |
There was a problem hiding this comment.
🟡 Two PhotonStateStore instances race on shared state file, risking lost updates (bug)
The adapter's __init__ creates self._photon_state = PhotonStateStore() at line 267, and _standalone_send creates its own state = PhotonStateStore() at line 1771. Both resolve to the same photon_state_path() file via get_hermes_home(). Each instance loads the file into its own in-memory dict, modifies it independently, and persists — the last writer silently overwrites the other's changes. In a gateway process, send_message (which invokes _standalone_send via the standalone_sender_fn registry) can interleave with adapter-driven state updates at await points, creating a lost-update race. Audit records, sent-message tracking, and reaction state can be lost.
💡 Suggestion: Share a single PhotonStateStore instance between the adapter and _standalone_send. Options: (a) pass the adapter's self._photon_state into the standalone send function via a closure or module-level reference; (b) make _standalone_send accept an optional state_store parameter; (c) add file-level advisory locking (fcntl.flock) around load-modify-write sequences.
📋 Prompt for AI Agents
In plugins/platforms/photon/adapter.py, refactor _standalone_send to use a shared PhotonStateStore rather than creating its own (line 1771). The simplest fix: accept an optional state_store parameter and pass self._photon_state from the adapter's standalone_sender_fn registration (around line 1924). Alternatively, add a module-level _shared_state_store singleton initialized on first use, and guard load-modify-write cycles with threading.Lock or fcntl.flock to serialize concurrent access.
| if health.get("write_error"): | ||
| emit(f" state write failure : {health['write_error']}") |
There was a problem hiding this comment.
🟢 Dead code: write_error display in photon status never triggers (bug)
_print_state_summary() in plugins/platforms/photon/cli.py (line 321) creates a fresh PhotonStateStore() at line 324, calls store.load(), and then checks health.get('write_error') at line 333. However, write_error is an instance attribute initialized to None (state.py:75) and only set during _persist() failures (state.py:266). It is never persisted to the state file and never restored in load(). Since _print_state_summary never triggers _persist(), write_error is always None, making the if health.get('write_error'): guard at cli.py:333 always False. The 'state write failure' message at cli.py:334 can never be emitted.
💡 Suggestion: Either persist write_error to the state file so load() can restore it (add to _empty_state(), _normalize(), and load()), or remove the dead write_error check if write errors are only meant to be surfaced within the active adapter instance's lifecycle.
📋 Prompt for AI Agents
In plugins/platforms/photon/state.py, modify _persist() (line 253) to store self.write_error into self._state['write_error'] before writing; update _empty_state() (line 78) to include 'write_error': None; update _normalize() (line 269) to preserve the 'write_error' key from the payload; and update load() (line 89) to restore self.write_error from the loaded state. This makes write errors visible to fresh store instances like the status command.
1 participant
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 NousResearch#43726, NousResearch#53451, and NousResearch#55105.
Type of Change
Changes Made
/unreact, including DM alias fallback and restart recovery.hermes photon status.How to Test
scripts/run_tests.sh tests/plugins/platforms/photon tests/hermes_cli/test_plugins.py::TestPluginDiscovery::test_deferred_bundled_platform_cli_loads_on_matching_command -qgit diff --checkpython -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.pynode --check plugins/platforms/photon/sidecar/index.mjsuv run hermes photon --helpChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & 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.
Mirror-of: NousResearch#56006
NousResearch#56006