Conversation
The `hermes serve` / `hermes dashboard` auto-archive ticker fires 90s after bind and, with `sessions.auto_archive: true`, opens a *writable* SessionDB for the profile and closes it. On Python < 3.12 SQLite's close-time checkpoint cannot be disabled, so that second connection ends the WAL generation and unlinks `state.db-wal`/`-shm` while the running gateway still holds those inodes — after which the deleted-WAL guard fails closed on every session write (`DeletedWalGenerationError`) until the gateway is restarted. Gate the sweep behind the same liveness check the in-process cron scheduler already applies (`_check_gateway_running`, plus `_served_by_running_multiplexer` for satellite profiles). Nothing is lost: the gateway runs its own `maybe_auto_archive` timer, and Desktop-only installs with no gateway keep sweeping as before. The gate fails closed — an unknown liveness answer skips the sweep rather than risking the tear. Fixes NousResearch#109727 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Ty |
alt-glitch
added
type/bug
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
I found three correctness gaps in the new ownership gate.
-
hermes_cli/web_server_sessions.py:226-231does not fail closed for the real liveness contract._check_gateway_running()returns only.runningand discardsGatewayLiveness.probe_error. The multiplexer helper also converts probe exceptions toFalse. A focused exact-head probe returnedrunning=False, probe_error=True, then observed a writable open, archive, and close. The new exception test mocks_check_gateway_running()itself to raise, so it does not exercise this production path. Please preserve the unknown state and stand down when ownership cannot be determined. -
hermes_cli/web_server_sessions.py:251-254checks ownership and opens the writer without a shared reservation. Gateway startup constructsGatewayRunner, which opens its writable session store, before it publishes the runtime lock and PID file. An exact-head interleaving probe changed ownership immediately after the gate returned false and confirmed that the dashboard still opened a second writer. Please fence this maintenance open with the same cross-process ownership primitive, or publish ownership before any gateway writer opens and revalidate atomically. -
hermes_cli/web_server_sessions.py:226-227disables the dashboard sweep for a satellite profile served by the default multiplexer, butgateway.run._housekeeping_auto_archive()loads and maintains only the launch profile store. It does not sweep served satellite homes. This means a satellite profile with auto-archive enabled can stop archiving entirely. Please make the multiplexer sweep each served profile with its own configuration, or retain a safe maintenance path for those stores.
The focused gate, production-path, ticker, and liveness tests passed locally. The two probes above expose behavior not covered by the added fixed-result mocks.
Addresses review points 1 and 3 on NousResearch#110405. 1. `_check_gateway_running()` returns only `GatewayLiveness.running` and drops `probe_error` — the field that exists to tell "down" from "unknown". A rung that raises degrades to the next and leaves `running=False`, so the gate read an unreadable PID file as "no gateway" and opened the second writer anyway. Resolve the liveness directly so the unknown state survives, and count it as owned. The gate now also fails closed on an unresolvable profile or a raising multiplexer probe. 2. `_housekeeping_auto_archive()` called `acquire()` with no argument, so the multiplexer only ever swept its launch home. Combined with the dashboard standing down for served satellites (which write no `gateway.pid` of their own), a satellite with `sessions.auto_archive` enabled would never archive. It now sweeps each served home under its own `HERMES_HOME` scope, so each is gated by its OWN `sessions` config, and one broken satellite cannot abort the rest of the sweep. Tests now drive the real `resolve_gateway_liveness` ladder instead of mocking the gate's own helper, so the `probe_error` contract is actually exercised. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks — all three reproduce. Points 1 and 3 are fixed in c323b9a; point 2 I've confirmed and want to argue for handling separately. 1. You're right that the test mocked the helper rather than the production path. The new 3. Satellite profiles would have stopped archiving — fixed. Confirmed: 2. The startup window is real, but it predates this PR. Reproduced from the source: Two things follow. First, this race exists on That's a change to gateway startup and to every writable-store open, which I don't think should ride along with a dashboard-side gate — it wants its own issue, its own reproduction, and its own review. Happy to open it and do the work if you agree; equally happy to do it here if you'd rather this PR not land until the window is closed. Your call. Not claiming a full green suite locally, incidentally — my checkout has unrelated pre-existing failures (identical with and without this change) and |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Reviewed exact head c323b9abcfcf80d739cc15e550b239c01e1fadf9. The revised commit fixes the prior unknown-liveness and normal-path satellite coverage findings. The ownership-check-to-writable-open race remains as already acknowledged, so I have not repeated it inline.
I found two additional P2 gaps in the new multiplexer sweep. Details are inline. The 10 changed tests passed on the exact head. A synthetic merge with current main at 5eb99eb2844b22ebb723711b8e6a0bbb80bb5f04 was conflict-free, and 57 focused and adjacent tests passed there. Ruff and git diff --check passed. GitHub reports no hosted checks for this head.
| only ``acquire()`` (the launch home) and a satellite with ``auto_archive`` enabled never archived. | ||
| Each home is scoped so it is gated by its OWN ``sessions`` config, not the launch profile's. | ||
| """ | ||
| _auto_archive_one_home() |
There was a problem hiding this comment.
P2: Please isolate the launch-home sweep too. If this config read or SessionDB acquire raises, _housekeeping_auto_archive() exits before visiting any satellite. GatewayRunner._init_session_db() explicitly tolerates primary-store initialization failure and continues, so a multiplexer can be running with an unavailable launch store and healthy satellite stores. In that state the dashboard now stands down for the satellites, which leaves their configured auto-archive disabled on every tick. I reproduced this by making the launch call raise and observed only the launch call, with no satellite attempt.
| home = _Path(_home) | ||
| if home == launch_home: | ||
| continue # already swept above as this process's own store | ||
| _token = set_hermes_home_override(str(home)) |
There was a problem hiding this comment.
P2: This installs the satellite home but not its secret scope. load_config() expands ${VAR} through agent.secret_scope, and without a scope _env_ref_lookup() falls back to the launch process environment. I reproduced a satellite config with auto_archive_days: ${ARCHIVE_DAYS}, satellite .env value 9, and launch environment value 3. The satellite sweep used 3.0. With no process value, conversion fails and the profile is silently skipped. Please run this through _profile_runtime_scope(home) or install the equivalent hydrated profile secret scope, and cover a per-profile ${VAR} value.
…rets Addresses the two P2 findings on NousResearch#110405. 1. The launch-home sweep was not isolated, so a raise there returned before any satellite was visited. `GatewayRunner._init_session_db()` deliberately tolerates a failed primary-store init and keeps running, so a multiplexer can serve healthy satellites while its own store is unavailable — and since the dashboard now stands down for served satellites, that left them with no sweeper at all. Every home, launch included, now runs through the same isolated helper. 2. Satellites ran under a bare HERMES_HOME override, which installs the home but not the profile's secret scope. `load_config()` expands `${VAR}` through `agent.secret_scope`, so `auto_archive_days: ${ARCHIVE_DAYS}` resolved against the LAUNCH process environment (or failed conversion and skipped the profile). Satellites now run under `_profile_runtime_scope(home)`, which installs the home, the hydrated profile secret scope and the terminal policy together. Both regressions are pinned: reverting the fix makes the new tests fail with exactly the reported symptoms (satellite unswept; `${ARCHIVE_DAYS}` resolving to the launch value 3 instead of the profile's own 9). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both P2s were real and are fixed in 4122407. Launch-home isolation. Correct — and the consequence is worse than a skipped tick, because the dashboard has already stood down for those satellites, so they had no sweeper left at all. Every home now goes through one isolated helper, launch included; a raise is logged and the loop continues. Secret scope. Also correct, and I'd missed that Both new tests fail on the pre-fix tree with those exact symptoms, so neither passes vacuously. On scoping-construction failure: Test state: 7 in Point 2 (the ownership-check-to-writable-open race) still stands as acknowledged; thanks for not re-litigating it inline. I'd still rather it landed as its own change against gateway startup, but say the word if you want it here. |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Reviewed exact head 412240706c2cbc3db48dfa8611fa3de77d3b6d4d. The revision fixes the two findings from the prior head, but I found three new P2 error-path gaps. Details are inline.
The 12 changed tests passed, and 93 focused and adjacent tests passed with 2 platform-specific skips. Three external regression tests failed on this head and reproduced the reported behavior through scripts/run_tests.sh. git diff --check passed. GitHub reports no hosted checks for this head.
I have not repeated the previously acknowledged ownership-check-to-writable-open startup race.
| launch_home = get_hermes_home() | ||
| for _name, _home in homes.items(): | ||
| try: | ||
| home = _Path(_home).resolve() |
There was a problem hiding this comment.
P2: Please include path resolution in the per-profile failure boundary. On Python 3.11, a cyclic profile symlink raises RuntimeError, not OSError, so this escapes _housekeeping_auto_archive() before the later per-profile try and prevents every following healthy satellite from being swept during that tick. I reproduced this with a real cyclic profile symlink followed by a healthy satellite. Move construction and resolution inside the isolated block and cover this error path.
| try: | ||
| with _profile_runtime_scope(home): | ||
| _sweep(f"profile {_name}", home / "state.db") | ||
| except Exception as exc: |
There was a problem hiding this comment.
P2: Catching scope-entry failure here can leave the housekeeping thread scoped to the failed satellite. _profile_runtime_scope() installs the home token before secret hydration, but its cleanup starts only after secret and terminal setup complete. A failure during that setup reaches this catch without resetting the token. A synchronized filesystem race reproduced this through the real secret hydration path, and the thread ended with the broken satellite as get_hermes_home(). Make the scope unwind each token even when later setup fails, then assert the launch home is restored after a construction failure.
| return True | ||
| # A served satellite writes no gateway.pid of its own; the live default | ||
| # multiplexer holds its writer and (since this change) sweeps it too. | ||
| return bool(name != "default" and _served_by_running_multiplexer(name)) |
There was a problem hiding this comment.
P2: This still fails open when default-multiplexer ownership is unknown. _served_by_running_multiplexer() and named_profile_served_by_running_multiplexer() catch probe failures and return False, so the outer exception handler never sees the failure. I held the default gateway lock while its PID and runtime records were malformed. The real probe returned false here, and _maybe_auto_archive_for_profile("work") opened, archived, and closed a writable satellite store. Please use a strict or tri-state multiplexer probe and treat unknown ownership as owned.
Addresses the three P2 findings on NousResearch#110405. 1. Path construction and resolution now sit inside the per-profile failure boundary. A cyclic profile symlink makes Path.resolve() raise RuntimeError, not OSError, on 3.11 — it escaped the tick and stranded every following healthy satellite. 2. `_profile_runtime_scope()` installed the HERMES_HOME token before secret hydration but only unwound it after secret and terminal setup had both succeeded, so a failure in between left the calling thread scoped to the failed profile. Every token now unwinds in one finally. This is a fix to the shared helper, not just to this caller. 3. The satellite branch still failed OPEN when multiplexer ownership was unknown: `_served_by_running_multiplexer()` converts probe failures into False, so a malformed gateway.pid / gateway_state.json under a LIVE default gateway read as "nobody serves this profile" and the dashboard opened a second writer. Replaced with a tri-state probe that returns False only when the default multiplexer is positively known not to own the store; an absent or unreadable served record under a live-or-unknown multiplexer counts as owned, and exceptions propagate to the caller's fail-closed handler. All four new regression tests fail on the pre-fix tree with the reported symptoms. Two further tests pin that fail-closed did not become fail-always: an authoritative served list that omits the profile is still a definite "not served", so that store keeps its dashboard sweep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
austinpickett
left a comment
austinpickett
left a comment
There was a problem hiding this comment.
Reviewed in a worktree against current main.
- The gap is real post-#108076:
hermes_cli/web_server_sessions.pystill opensread_only=Falsefrom the dashboard process on every archive tick, which is the last dashboard-side second writer onstate.db. #110544 made the resulting close non-destructive, so the symptom is contained, but the writer itself contradicts the single-writer-per-path design; standing down when a gateway owns the store is the right layer, and it mirrors what the cron ticker already does inweb_server.py. - Fail-closed on
running or probe_error, and thegateway/run.pycompanion is necessary — once the dashboard stands down for satellites, the multiplexer has to sweep them or they never archive. - Clean merge; the two new test files plus
test_memory_trim_housekeeping.pyrun 19/19 here. The three earlier review rounds are addressed at head.
One body edit before merge: #109727 is already closed, so this should read Refs #109727 / #100896 rather than Fixes. Approving; leaving the merge to @teknium1 since it sits inside the WAL work he is driving.
|
|
||
| try: | ||
| launch_home = get_hermes_home().resolve() | ||
| except OSError: |
There was a problem hiding this comment.
[Bug] (blocking, P2) Please handle RuntimeError while resolving the launch home. This branch catches only OSError, but Path.resolve() raises RuntimeError for a cyclic symlink. The exception occurs before the satellite loop, so one bad launch path prevents every healthy served profile from being auto-archived during that tick. The satellite branch below already handles the same exception type. I reproduced this on the exact head and on a clean merge with current main: a review-only test that makes launch-home resolution raise RuntimeError fails here before the satellite sweep is called. Please give launch-home resolution the same failure boundary and add a regression case.
Blocking P2 on NousResearch#110405. The satellite loop caught RuntimeError from Path.resolve() (cyclic symlink) but the launch-home resolution above it caught only OSError — and it runs BEFORE the loop, so one bad launch path stopped every healthy served profile being swept for that tick. launch_home is only used to skip a duplicate sweep, so it now degrades to None ("cannot dedupe") rather than aborting; _auto_archive_one_home is idempotent under the min_interval_hours gate. The regression test fails on the pre-fix tree with the reported RuntimeError. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Fixed in 49658d1 — you're right, and I'd fixed the satellite loop while leaving the identical gap on the line above it.
@austinpickett thanks — body updated to Note for whoever merges: |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Reviewed exact head 49658d19ef725ea7479012c0d0cfa7c0267801f7. The previous launch-home blocker is fixed. I found one new blocking fail-closed gap. Details are inline. I did not repeat the acknowledged ownership-check-to-writable-open race.
Verification: 19 changed tests passed on the exact head. A conflict-free synthetic merge with current main at 00570550f37e9082676955d50f65c7d9ba846cc9 passed 390 focused and adjacent tests with 4 platform-specific skips. Linux-only and Windows-only cases did not run on macOS. GitHub reports no hosted checks or commit statuses for this head.
| liveness = resolve_gateway_liveness( | ||
| profile_dir=home, use_cache=False, | ||
| pid_probe=lambda path: get_running_pid(path, cleanup_stale=False)) | ||
| return bool(liveness.running or liveness.probe_error) |
There was a problem hiding this comment.
[Bug] (blocking, P1) This gate still fails open for a held runtime lock when the identity metadata is malformed or unreadable. resolve_gateway_liveness() can only set probe_error when its injected function raises, but get_running_pid() calls non-strict helpers that normalize these production failures to None. I reproduced an active lock with malformed PID and lock records. The result was running=False, probe_error=False, and _gateway_owns_home(...) == False, so the dashboard can proceed to a writable SessionDB even though another process owns the store. The new test makes get_running_pid() raise directly and does not exercise this normalization path. Please use the strict identity and lock probe, or add a tri-state API that preserves unknown ownership, and add a regression using malformed or unreadable real metadata.
Conflict in gateway/run.py's housekeeping chore list. main has since grown
`profile_scoped_chore()` / `_for_each_served_profile()`, which runs a chore once
per served profile inside that profile's runtime scope — the general primitive
this branch had hand-rolled for auto-archive alone.
Resolved by keeping both intents through main's primitive:
- `_housekeeping_auto_archive()` goes back to a zero-arg chore that sweeps the
CURRENTLY SCOPED profile, and is registered as
`profile_scoped_chore(runner, _housekeeping_auto_archive)` alongside the
curator and skill-sync chores. main left auto-archive unwrapped because it only
ever swept the launch home; wrapping it is this branch's whole point, since the
dashboard now stands down for served satellites.
- The bespoke satellite loop, its launch/satellite path resolution and its
HERMES_HOME override are deleted. The primitive already supplies the home AND
the secret scope, so the `${VAR}`-against-the-launch-environment bug stays
fixed without this branch owning the scoping.
- `_auto_archive_one_home()` drops its db_path argument: `acquire()` resolves
`_default_db_path()` from `get_hermes_home()` at call time, which the scope has
already pointed at the right profile.
- The chore keeps its own try/except. `_for_each_served_profile` does NOT isolate
per profile, so without it one unreadable store still abandons every profile
after it in the same tick.
Tests rewritten to drive the real primitive rather than a hand-rolled loop, plus
a guard on the registration line itself — an unwrapped chore silently reverts
every served satellite to never archiving.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Reviewed exact head c62130f3ba487e0ba009a9e393e33488ffc28b96. The merge resolution preserves the prior profile-scope, launch-profile, satellite, secret-expansion, and per-profile error-isolation fixes.
This head remains blocked. The existing strict-metadata P1 is still open and already has an inline comment at this exact head, so I did not duplicate it. I found one additional P1 in the cross-container ownership path; details are inline. The previously acknowledged ownership-check-to-writable-open startup race also remains unchanged.
Verification: 142 focused and adjacent tests passed on the exact head with 2 Windows-only skips. A conflict-free synthetic merge with current upstream/main at 2388cab52cbad7a4d2352dbac3ffce3500c2dd72 passed 142 tests with 4 platform-specific skips. git diff --check passed. GitHub reports no hosted checks for this head.
| from gateway.status import get_running_pid, resolve_gateway_liveness | ||
|
|
||
| # cleanup_stale=False: a status probe for ANOTHER profile must never unlink its PID file. | ||
| liveness = resolve_gateway_liveness( |
There was a problem hiding this comment.
[Bug] (blocking, P1) This ownership check skips the configured cross-container health rung. In a split gateway/dashboard deployment, GATEWAY_HEALTH_URL can be the only evidence that the remote gateway is live while local PID and runtime files are absent. I reproduced the divergence at this head: resolve_gateway_liveness(..., health_probe=_probe_gateway_health) returned running=True with source health, while _gateway_owns_home() returned False for the same home. _maybe_auto_archive_for_profile() can then open and close a writable SessionDB on the shared store, recreating the WAL-generation hazard this gate is intended to prevent. Please include the configured health probe in this ownership decision. A configured probe that cannot establish absence should fail closed here. Add a regression where only remote health reports ownership.
`_maybe_auto_archive_for_profile` ran 90s after bind (and on every dashboard session-list request) and opened a WRITABLE SessionDB even when a gateway owned the store. The gateway's own housekeeping already runs that sweep, so the serve copy only added a second writer to a database another process is archiving. Return early when the profile's gateway runtime lock is held. Credit: @isair (#110405)
Pre-merge step only. main renormalized this file (CRLF->LF) in 3f1afe79; the stale CRLF blob here blocked every merge attempt with 'local changes would be overwritten'. After merging main the content is identical, so this adds nothing to the PR diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main has since implemented both halves of this PR directly: - `_housekeeping_state_db_maintenance()`, registered as `profile_scoped_chore(runner, ...)`, supersedes this branch's `_housekeeping_auto_archive()` and is a superset — it covers auto-prune and VACUUM as well, and carries the launch home's transcript-dir override. - `_maybe_auto_archive_for_profile()` now stands down when a gateway owns the store, citing NousResearch#110405. Both conflicts resolved toward main, so this branch no longer carries a duplicate implementation. Two things are kept, because main's version does not have them and both came out of review on this PR: 1. The stand-down predicate stays `_auto_archive_owned_by_gateway`, not `_check_gateway_running`. The latter exposes only `GatewayLiveness.running` and drops `probe_error`, and `_served_by_running_multiplexer` converts probe failures to False — so an unreadable gateway.pid or a malformed gateway_state.json under a LIVE gateway reads as "no owner" and the second writer is opened anyway. Both were reproduced in review; this gate resolves the liveness itself and treats unknown as owned. 2. Per-profile isolation in `_housekeeping_state_db_maintenance()`. `_for_each_served_profile` puts no boundary between profiles and `_housekeeping_chore` only catches at the tick level, so one unreadable store abandoned every profile after it in that tick — reachable, since `GatewayRunner._init_session_db()` tolerates a failed primary-store init and the dashboard has already stood down for served satellites. Tests retargeted from the deleted functions onto main's, and pinned against a sibling test re-pointing DEFAULT_DB_PATH (main now derives the profile home from it). Removing either kept change fails them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rung Addresses the two open blocking P1s on NousResearch#110405. 1. `resolve_gateway_liveness()` can only set `probe_error` when its injected probe RAISES, but `get_running_pid()` normalises malformed or unreadable identity metadata to None. An ACTIVE runtime lock with corrupt PID/lock records therefore reported running=False, probe_error=False, and the gate opened a second writer on a store another process owns. The probe is now `get_running_pid_identity_strict()`, which raises on ambiguous state and, like the previous non-strict call, never unlinks another profile's PID file. 2. The ownership decision skipped the configured cross-container health rung. In a split gateway/dashboard deployment GATEWAY_HEALTH_URL can be the only evidence the gateway is live, since local PID and runtime files are absent entirely. The health probe is now passed when one is configured. `_probe_gateway_health` collapses DNS, timeout, refused and non-200 into (False, None), which is indistinguishable from "the gateway is down", so it is wrapped: a configured endpoint that does not positively confirm liveness raises and is recorded as probe_error. With GATEWAY_HEALTH_URL set and the remote gateway genuinely down the dashboard stops sweeping — deliberate, and cheaper than the tear it prevents. The regressions use REAL unreadable metadata (a chmod 000 lock) rather than a mocked helper, so they exercise the normalisation path the review identified. They also pin that fail-closed did not become fail-always: with no health URL configured, an ordinary single-host install still resolves as unowned and keeps its sweep. All four fail on the pre-fix gate. Noted in passing: the non-strict `get_running_pid()` unlinks the runtime lock even with cleanup_stale=False, which is why the test asserts that branch last. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both P1s fixed in ca23056. Strict identity probe. Correct — The regression plants a real Cross-container health rung. Also correct, and it's the more dangerous of the two since a split deployment has no local PID files at all. The configured probe is now passed through. On your "a configured probe that cannot establish absence should fail closed": Four new tests, all failing on the pre-fix gate. Two of them pin that fail-closed didn't become fail-always: no health URL configured still resolves unowned and keeps its sweep. 17 passing across both test files; 643 in the gateway status/liveness/pid/lock set. Separately — worth a look before more review effort goes in: |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Reviewed exact head ca23056394930d4991ace3bb2548a902e9666f73. The new commit fixes the prior strict-metadata and cross-container-health P1 findings. I found one new blocking regression in the target-home handoff; details are inline. The previously acknowledged ownership-check-to-writable-open startup race remains unchanged and is not repeated here.
Verification: the 17 changed tests passed. In the adjacent production-path suite, 318 tests passed, 2 failed, and 4 platform-specific tests were skipped. Both failures pass on the current base 274bc7b8f613c299b4f59160bacf8a19010f7003. Ruff, Python compilation, and git diff --check passed. GitHub reports no hosted checks.
| # gateway reads as "no owner" and the second writer is opened anyway — both reproduced in | ||
| # review on #110405. This gate resolves the liveness itself, keeps the unknown state, and | ||
| # treats unknown as owned. | ||
| if _auto_archive_owned_by_gateway(profile): |
There was a problem hiding this comment.
[Bug] (blocking, P1) Use the already resolved profile_home for this ownership decision. The function resolves the target store at line 352, but this call passes only the profile label, so _auto_archive_owned_by_gateway() resolves the home again through get_hermes_home() or _cron_profile_home(). If the active DB path is redirected, or a named profile mapping changes between these reads, the gate can inspect one home and then open another home writable. Two existing production-path regressions expose this exact mismatch: test_web_server_auto_archive_gateway_lock.py opens the store while a real lock holder owns the selected path, and test_web_server_auto_archive_profile_config.py skips a valid named-profile sweep. Both pass on base and fail at this head. Please make the ownership check consume the same resolved target used for config and database access, and keep both regressions green.
Conflict in `_profile_runtime_scope`. main has independently fixed the token leak this branch carried (P2 on NousResearch#110405): a failure during secret hydration or the terminal-policy install left the HERMES_HOME token installed, stranding the calling thread on the failed profile. Resolved toward main, whose version is strictly better — it initialises both tokens to None before the try, so it also covers `set_hermes_home_override` itself raising, which this branch's version did not. main additionally passes `profile_home=` to `set_secret_scope`. This branch's remaining contributions are unaffected and verified present after the merge: the fail-closed ownership gate in `hermes_cli/web_server_sessions.py` (strict identity probe + cross-container health rung) and the per-profile isolation in `_housekeeping_state_db_maintenance`. 17 tests across both PR test files pass; 525 in the gateway scope/multiplex/ housekeeping set, with only the two long-standing pre-existing failures (`test_completion_preflight_runs_in_target_profile_scope` and the residue-parity test it pollutes). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Superseded by #120268. The core of this PR is already on Your review rounds found two things
I did not carry the fail-closed ownership gate. Serve under one-backend-per-host is a legitimate Thank you for sticking with this through eight review rounds. The broken-launch-store case is yours. |
isair
changed the title
|
Retitled and rewrote the description to match what this actually is now.
So this is now a hardening follow-up, not the feature. Two things remain:
@ehz0ah the startup race is now written up under "Known issues" in the body rather than left implicit, along with the health-probe fail-closed tradeoff, since that one is a judgment call worth someone else's eyes. |
5 participants
What does this PR do?
Hardening follow-up to the auto-archive WAL work.
mainhas since landed the original fix itself, in three separate pieces, so this PR no longer carries that feature:_housekeeping_state_db_maintenance(), registered viaprofile_scoped_chore(runner, ...), supersedes this branch's per-profile auto-archive sweep and is a superset of it (auto-prune and VACUUM too)._maybe_auto_archive_for_profile()now stands down when a gateway owns the store, citing this PR._profile_runtime_scope()now unwinds every contextvar token when a later setup step raises.Each of those conflicts was resolved toward
main. What remains is two thingsmain's versions don't have, both found during review here.1. The stand-down gate fails closed on unknown ownership
mainuses_check_gateway_running(profile_home). That exposes onlyGatewayLiveness.runningand discardsprobe_error— the field that exists precisely to tell "down" from "unknown" — and_served_by_running_multiplexer()converts probe failures toFalse. Three ways that opens a second writer on a store another process owns, all reproduced in review:get_running_pid()normalises a malformed or unreadable PID/lock record toNone, so an active runtime lock reportedrunning=False, probe_error=False. Now usesget_running_pid_identity_strict(), which raises on ambiguous state and never unlinks another profile's PID file.GATEWAY_HEALTH_URLcan be the only evidence a remote gateway is live, since local PID and runtime files are absent entirely. The configured health probe is now part of the ownership decision.Falseonly when the default multiplexer is positively known not to own the store._probe_gateway_healthcollapses DNS, timeout, refused and non-200 into(False, None), which is indistinguishable from "the gateway is down", so it is wrapped — a configured endpoint that doesn't positively confirm liveness is recorded asprobe_error. Consequence worth reviewing: withGATEWAY_HEALTH_URLset and the remote gateway genuinely down, the dashboard stops sweeping. A skipped archive interval versus a torn WAL generation; say the word if you'd rather that were narrower.2. Per-profile isolation in the maintenance chore
_for_each_served_profileputs no boundary between profiles, and_housekeeping_choreonly catches at the tick level, so one unreadable store abandoned every profile after it in that tick. Reachable in practice:GatewayRunner._init_session_db()deliberately tolerates a failed primary-store init and keeps running, and the dashboard has already stood down for served satellites — leaving the multiplexer as their only sweeper.Related Issue
Refs #109727 / #100896
Type of Change
How to Test
17 tests. Every regression here fails on the pre-fix tree with the reported symptom, and the identity-probe tests use real unreadable metadata (a
chmod 000lock) rather than a mocked helper, so they exercise the normalisation path rather than asserting around it.Three tests deliberately pin that fail-closed did not become fail-always: an authoritative served list omitting the profile is still a definite "not served", and an ordinary single-host install with no health URL still resolves as unowned and keeps its sweep.
Known issues
GatewayRunner(config)opens its writable store before_start_gateway_claim_pid_file()publishes ownership, so a sweep landing inside that window still races. It predates this PR and closing it means either reordering gateway startup or a cross-processflockfence (fts_rebuild_admission()is the template). Acknowledged by the reviewer as out of scope for this branch.test_completion_preflight_runs_in_target_profile_scope, andtest_stale_served_turn_never_recreates_archived_profile, which it pollutes.check-attributionfails on an unmapped author email, not on anything in this diff.🤖 Generated with Claude Code