fix(state): in-product recovery after deleted-WAL guard fires (#110054) - #110073
JoaoMarcos44 wants to merge 3 commits into
Conversation
…search#110054) - Add two-layer explainer for deleted_wal persistence failures: a friendly Layer 1 reassurance in chat explaining that saving is paused with no data lost and offering a direct 'Recover' / 'doctor --fix' action, followed by Layer 2 operator runbook with profile-aware recovery commands and guide links. - Enhance hermes doctor --fix to actively detect and safely terminate orphaned processes holding retired/unlinked WAL generations, reopen state.db, and surface forensic retired-wal capture artifacts for inspection. - Update large WAL doctor warning to avoid directing users to run doctor --fix while writers are active without clarifying that Desktop/gateway must be stopped. - Add fix parameter to POST /api/ops/doctor and Desktop runDoctor() API. - Wire in-app Desktop error notification banner with one-click 'Recover' action when session_persistence_failed:deleted_wal occurs, plus manual maintenance entry. - Add user-guide/session-storage-recovery.md and update developer docs and sidebars. Fixes NousResearch#110054
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head c3eaa188ec1d9fa3b6a621987df044ad5a25b73a against current main 5dea46d13deec9549bdc2ea703ae9201d733c28d, including the full 15-file diff, the existing deleted-WAL guard/capture semantics, the doctor action spawn path, gateway.status.terminate_pid, #110054 and its salvage graph, and exact-head Actions state.
There is a lot that is directionally right here. The PR fixes the circular large-WAL guidance, gives the failure a human-facing recovery surface, keeps the forensic runbook available, and routes Desktop through the existing background action machinery instead of doing database surgery in the renderer. The problem is that the destructive half of the recovery transaction is currently stronger than its proof half. For this failure class, that is the boundary that has to be exact.
Blocker 1 — recovery can destroy the only surviving committed WAL frames
_recover_retired_wal() enumerates holder PIDs and immediately starts terminating them. It does not first prove that each holder's exact retired WAL generation has been durably captured and is inspectable.
That violates the invariant already encoded in hermes_state_dbfile.capture_retired_wal_generation(): the unlinked WAL can contain committed frames that survive only while the owning descriptor remains open; capture failure deliberately raises without closing that descriptor so the bytes remain recoverable. Killing the holder is what finally drops that inode.
The current retired_dirs lookup is not a sufficient settlement receipt. It is just a directory glob, and after the kill loop the code reports the lexically latest artifact without proving that it belongs to each PID/generation being terminated, that its manifest is complete, or that its recorded WAL identity matches the holder's exact orphaned inode. A stale artifact from an earlier incident can therefore coexist with a new uncaptured holder and make the output look reassuring while doctor --fix destroys the new generation.
Required shape: make holder termination a capture-gated transaction. Before signalling a PID, bind the holder to a process/generation identity and require a durable manifest for that exact generation (including the WAL identity/digest and whatever main-image mode was actually captured). If capture is absent, incomplete, mismatched, or cannot be proven, fail closed and leave that holder alive. Add a regression where the only holder has uncaptured committed frames / capture fails and assert no signal is sent and the descriptor remains live. #109766 is complementary here: its self-heal path already treats capture failure as a hard stop before descriptor retirement; this operator recovery path needs the same ordering rather than bypassing it.
Blocker 2 — PID is used as mutation authority without an identity fence
The scan returns (pid, target), then _read_proc_argv(pid) is used only for display, and the code calls terminate_pid(pid) later. There is no process-start-time or equivalent immutable identity carried from observation to mutation. A holder can exit between the /proc scan and the signal; that PID can be recycled, after which the recovery command may SIGTERM/SIGKILL an unrelated process.
The repository already has the exact defense for this class in gateway.status.terminate_pid(..., expected_start_time=...) (#89614). This call site currently declines to use it, and on POSIX the forced kill remains allowed without a fingerprint.
Required shape: capture an identity witness while the fd/holder is observed, revalidate it immediately before TERM and again before any KILL escalation, and refuse if it changed or cannot be established. The test matrix needs at least: stable holder -> termination; holder exits -> no signal to replacement; PID recycled between TERM and KILL -> no KILL; identity unavailable -> fail closed/manual recovery.
Scope / interlock gate — do not close the recovery issue as platform-complete yet
#110054 explicitly listed the open salvage lanes rather than asking this PR to duplicate them. Current main's iter_deleted_sqlite_sidecar_holders() is Linux-only and returns [] elsewhere. #110023 is the active macOS holder-enumeration salvage (preserving #109758 authorship); until that lands or equivalent functionality is composed, this PR's new doctor recovery cannot actually discover the macOS deleted-WAL holders it claims to remediate. #109997 is adjacent and changes self-holder semantics; #109766 is complementary capture-first self-heal. Please make those relationships explicit in the PR/closure graph and test the composed behavior rather than silently treating them as solved here.
This does not mean #110073 must absorb those implementations. The clean ownership split is: detection stays with the holder-enumeration lane, in-process self-heal stays with #109766, and this PR owns the user-facing/operator recovery transaction. But Fixes #110054 is premature unless the composed recovery path exists on the platforms the issue covers.
Should-fix — header_only artifacts are advertised with a nonexistent state.db
The new explainer correctly says manifest.main.mode == header_only has no copied state.db, but _recover_retired_wal() always prints sessions recover --source <artifact>/state.db --inspect-only regardless of mode. For header_only, that command points at a file that intentionally does not exist. Branch the guidance on the manifest mode and only print an inspect command when the copied image is actually present; otherwise state that the artifact is forensic-only and name the header/manifest that exists.
Verification state
There were 0 submitted reviews and 0 inline threads on this head before this review. The PR is currently mergeable against main, but exact-head hosted verification has not executed: CI 34769424327, Docker 34769423959, and Nix 34769423949 all ended action_required. That is missing release evidence, not a code-failure verdict. The author's targeted 38 Python + 6 Desktop tests are useful local evidence, but they do not cover either destructive counterexample above.
Disposition: strong recovery UX direction, but not safe to merge yet. Gate every kill on exact-generation preservation and exact process identity, fix the header_only guidance, declare the open holder/self-heal interlocks, then prove the hostile recovery matrix plus exact-head CI/Docker/Nix. Once those are in place I would expect this to become a very useful closure path for #110054.
| argv = _read_proc_argv(pid) | ||
| cmd_desc = " ".join(argv)[:60] if argv else f"PID {pid}" | ||
| try: | ||
| terminate_pid(pid, force=False) |
There was a problem hiding this comment.
Blocking — preserve the exact WAL generation before terminating its owner. iter_deleted_sqlite_sidecar_holders() proves that this PID has an orphaned sidecar, but it does not prove those committed frames were durably captured. The state layer intentionally keeps the descriptor open when capture_retired_wal_generation() fails because process exit is what destroys the last copy. Before this signal, require a manifest/digest/identity receipt for this holder's exact generation; otherwise fail closed and leave it alive. A pre-existing retired-wal-* directory from another incident is not that proof.
| exited = True | ||
| break | ||
| if not exited: | ||
| terminate_pid(pid, force=True) |
There was a problem hiding this comment.
Blocking — the escalation is not process-identity qualified. Between the initial /proc holder scan and this KILL, the original holder can exit and its PID can be reused. terminate_pid already accepts expected_start_time specifically to prevent recycled-PID kills (#89614), but this caller supplies none; on POSIX the forced kill is therefore still permitted against only the integer PID. Carry an immutable process fingerprint from observation through TERM and KILL and revalidate before each signal; identity unavailable/mismatched should become a manual-recovery refusal.
| pass | ||
| check_info( | ||
| f"Retired WAL capture preserved at {latest.name} (mode: {mode}; " | ||
| f"inspect with 'hermes {profile_arg}sessions recover --source {latest / 'state.db'} --inspect-only')" |
There was a problem hiding this comment.
mode == "header_only" intentionally means there is no copied state.db in this artifact, but this always prints a sessions recover --source …/state.db command. The new chat explainer gets this distinction right. Mirror it here: only emit the inspect command for a verified copied main image; for header_only, report the manifest/header as forensic-only instead of pointing users at a nonexistent file.
… status in web router (NousResearch#110054) - Exclude doctor's own PID (os.getpid()) from unlinked WAL holder termination. - Optimize multi-holder termination from sequential sleep loop to bounded parallel SIGTERM and SIGKILL, reducing worst-case latency from O(N) to O(1) <= 3s. - Surface preserved message spool files in pending_messages/ after clean reopen. - Extend corrupt_store_as_status in web_routers to map DeletedWalGenerationError and StateDbReplacedError to structured 503 responses, preventing 500 error storms during dashboard polling. - Provide active toast feedback upon clicking Recover in Desktop message stream. - Add tests for PID exclusion, pending spool discovery, and 503 status mapping.
|
Additional field evidence from a related report (#110082), in case it helps scope the recovery path:
Not asking for a change in this PR — the desktop toast + |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Follow-up on new exact head 41e2b12a3e52875f44cb5695258eb69fac17378d (current main still 5dea46d13deec9549bdc2ea703ae9201d733c28d). The head advanced by one commit while I was submitting the prior review, so I re-read that entire delta rather than treating the old-head receipt as current.
The new commit improves responsiveness by terminating holders in parallel, excludes the doctor subprocess's own PID from the kill set, adds pending-spool reporting, extends dashboard 503 shaping for replacement errors, and adds tests around those changes. Those are useful changes. They do not close the two destructive blockers from the prior review:
-
Exact-generation preservation is still not a precondition for signalling a holder. The new head still calls
terminate_pid(pid, force=False)before establishing that this PID's exact orphaned WAL inode has a durable, identity-matching capture.retired_dirsremains only a later glob/reporting mechanism. Parallelizing the signals does not change the settlement ordering: an uncaptured retired WAL can still be the only surviving copy of committed frames when its owner is killed. -
Process identity is still not carried from observation to mutation. The holder scan still yields an integer PID; the new parallel loop sends TERM and later KILL using only that PID.
os.kill(pid, 0)proves only that some process currently owns the number, not that it is the process whose fd was observed.terminate_pid(..., expected_start_time=...)remains unused, so PID reuse between scan/TERM/KILL can still target a replacement process.
The self-PID exclusion is also not a substitute for the #109997 interlock. Current main intentionally includes self in iter_deleted_sqlite_sidecar_holders() because the guard uses that fact to refuse a fresh connection. This head filters self only inside doctor recovery; it does not change the guard, so a doctor process with a retired self-descriptor can still reach the underlying self-refusal path. #109997 owns that semantic decision and should remain explicit rather than being partially recreated here.
The header_only guidance issue is unchanged: the doctor report still prints sessions recover --source <artifact>/state.db --inspect-only for a mode whose contract says no copied state.db exists.
Exact-head hosted evidence is also still absent on this new SHA: CI 34769786270, Docker 34769785703, and Nix 34769785702 all ended action_required without executing jobs.
Current disposition remains blocking. Preserve each exact WAL generation before any process termination; bind TERM/KILL to a stable process fingerprint and revalidate it; keep #109997/#110023/#109766 as explicit adjacent/complementary owners; fix mode-aware artifact guidance; then run the hostile recovery matrix and exact-head CI. The new delta is an improvement, but it does not change the safety verdict.
alt-glitch
added
type/bug
…ty witness before signaling (NousResearch#110054) - Require durable capture of exact unlinked WAL generation before signaling any holder, supporting external capture from /proc/<pid>/fd descriptors when needed, and failing closed to manual recovery if capture cannot be established. - Capture process start-time identity witness while observing holder fds, revalidate witness immediately before SIGTERM and before SIGKILL escalation, refusing signals if identity changes or PID is recycled. - Add mode-aware guidance for retired WAL captures: only advertise sessions recover inspect commands for copied images, reporting forensic-only guidance for header_only. - Add test matrix for hostile recovery: stable holder termination, exited holder, recycled PID between TERM and KILL, unavailable identity, and uncaptured WAL.
|
Thank you for the thorough and incisive review @andrexibiza. All points have been addressed and landed in commit \e3d4d08979:
All 45 Python tests and 6 Desktop Vitest tests pass cleanly. |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Follow-up: preservation still does not cover the termination scope
Re-reviewed exact head e3d4d08979fda9a046f8879ff5693314a6178130 against the PR's recorded base 5dea46d13deec9549bdc2ea703ae9201d733c28d, including the 17-file diff, the recovery/capture implementations, existing reviews, added tests, Desktop request routing, the actual process-termination helper, and the open holder/self-heal interlocks. This follows the fix-completion comment; it is not a repetition of the unchanged earlier heads.
The mode-aware header_only branch, explicit issue ownership, and caller-side start-time checks are real improvements. The original reviews were right to block capture-before-kill and PID-only authority, but the new implementation does not yet close those classes. It also exposes two boundaries the earlier reviews did not spell out: a process can own more state than one selected descriptor, and the Recover action must remain bound to the failed chat's owner.
Disposition: four P1 findings remain. Do not treat the current capture checks or the reported test counts as proof that destructive recovery is safe.
P1 — A manifest claim still authorizes termination without a verified capture
_find_retired_wal_capture, lines 211–230 returns a directory on PID equality alone when wal_identity is None. The caller explicitly reaches that case when its detailed scan does not supply a WAL descriptor/identity. Even with an identity, it checks only the manifest's recorded tuple: no payload existence, length, digest, or completed-publication check. The glob also includes .partial directories.
These are executable counterexamples, not merely missing assertions:
- No detailed descriptor + a PID-only manifest + no WAL file still reaches
terminate_pid(..., force=False). - A matching
(st_dev, st_ino)in JSON + missing WAL payload also reaches termination. - A matching manifest in an unpublished
.partialdirectory is accepted without recapture. - Altering the captured WAL after its manifest digest was recorded does not prevent acceptance.
The new test_doctor_fix_stops_holders_and_reopens actually constructs the first unsafe receipt: {"pid": 11111, "main": {"mode": "copied"}}, with no captured WAL or recorded WAL identity, and expects a successful stop. Similar fixtures appear in the identity tests. Those tests must stop treating absent evidence as the positive control.
A minimal regression for the actual repository helper is:
def test_manifest_without_wal_is_not_a_preservation_receipt(tmp_path):
import json
from hermes_cli.doctor_state import _find_retired_wal_capture
db = tmp_path / "state.db"
artifact = tmp_path / "state.db.retired-wal-repro"
artifact.mkdir()
(artifact / "manifest.json").write_text(json.dumps({
"pid": 12345,
"wal": {"identity": [1, 2], "file": "state.db-wal",
"bytes": 4096, "sha256": "0" * 64},
"main": {"mode": "copied"},
}))
assert _find_retired_wal_capture(db, 12345, (1, 2)) is NoneRequired: unavailable descriptor identity must refuse, not downgrade to PID-only matching. Validate the published artifact and its actual bytes against the exact generation before granting termination authority. Missing, incomplete, changed, or mismatched evidence must either trigger a safe recapture or leave the holder alive. Also change the failure guidance: after preservation fails, the current manual_issues message says “stop them manually and reopen”. That directs the operator to perform the same potentially data-destroying step the guard just refused; preservation failure needs “keep the holder alive and preserve the generation,” not a manual kill instruction.
P1 — One descriptor per PID is not enough to authorize killing the process
Lines 308–328 collapse descriptor records into {r["pid"]: r}. A PID holding two distinct retired WAL inodes is reduced to whichever record appears last. Only that generation is captured, then the whole process becomes a termination target.
The recovery-function harness reproduced both orderings:
holder generations: [(1, 2), (1, 3)] -> captures: [(1, 3)] -> TERM
holder generations: [(1, 3), (1, 2)] -> captures: [(1, 2)] -> TERM
Terminating the process closes both generations, including the uncaptured one. This is broader than preserving one selected database: a multi-profile process can also hold retired descriptors for another profile, which this state_db_path-scoped scan never considers.
Required: retain a set of generation records, not a single record per PID. Deduplicate multiple descriptors for the same inode, preserve every affected generation, and establish that the preservation scope covers the process-wide effect. For a process serving additional stores, either preserve/coordinate that complete affected set or refuse process-wide termination in favor of an owned per-handle recovery protocol. A changed or unprovable descriptor set before termination must not inherit the earlier proof.
P1 — External capture can return success while omitting a committed transaction
capture_external_retired_wal_generation, lines 453–480 samples st_size once and copies exactly that many bytes. It does not establish that all writers to the inode are quiescent, detect growth during capture, or bind the final captured contents to the later termination. Descriptor enumeration establishes ownership, not that the owning SQLite connection has stopped committing.
I exercised the retrieved capture-function excerpt with a real disposable SQLite subprocess, an actually unlinked WAL, a real /proc/<pid>/fd/<fd> handle, and fsynced output. A second transaction was committed after the initial size sample and before the WAL copy. The function returned success:
SQLite: 3.46.1
initial WAL size: 4,152 bytes
live WAL after commit: 8,272 bytes
manifest/captured WAL: 4,152 bytes
live committed rows: before, during_capture
copied artifact rows: before
capture returned: success
The inode identity remained the same. This is why an identity match plus a digest of an old prefix is not a complete preservation receipt. The existing-capture fast path has the same temporal issue if that inode acquired more committed frames after the earlier capture.
Required: make external recovery a capture-and-retirement protocol over a stable committed state: coordinate/quiesce the affected writers and preserve the final generation before permitting its retirement, or refuse when that cannot be established. Keep preservation protection across the capture/termination boundary. Merely adding another size check while writers remain free to commit leaves another race. Add a real writer/capture interleaving test that proves every committed row survives in the artifact before allowing termination.
P1 — Recover can target a different connection/profile from the failed chat
The toast callback at lines 371–380 calls runDoctor(true) without retaining the failed session's owner. runDoctor supplies no profile or explicit connection pin. hermesApi adds the currently active connection when the request is made; it does not automatically add the active profile. The backend doctor route likewise uses _spawn_action, which calls spawn_profile_action(None, ...) rather than accepting the failed store's profile.
A delayed-click routing probe using those request-function excerpts produced:
failure/toast owner: gateway-A / research
user switches to: gateway-B / production
Recover request: {connectionId: "gateway-B",
path: "/api/ops/doctor",
method: "POST", body: {fix: true}}
profile field: absent
Even without a connection switch, the named chat profile is not carried into this destructive operation. The notification is also outside the isActiveEvent block, so it is not limited to the foreground chat.
Required: capture and carry the failed session's qualified (connectionId, profile) owner through the callback, API helper, router, and spawned CLI action. A stale/unknown owner should refuse; it must not fall back to the current selection. Test background-session failure, profile switching, and connection switching before clicking the existing notification, asserting that no other owner's repair endpoint is called.
Correction to my earlier review: the helper does not enforce the TERM fingerprint
My earlier description of terminate_pid(..., expected_start_time=...) was too broad. In the actual implementation, fingerprint enforcement is inside if force ...; force=False goes directly to os.kill(SIGTERM). A POSIX helper-excerpt probe with a stale supplied witness sent TERM without reading the current fingerprint, while the force/KILL control correctly refused.
The new outer check in doctor is real and narrows the race; I am not claiming it is absent. But passing the keyword does not add a second TERM fence or make lookup-plus-signal atomic. The regression must exercise the real termination boundary rather than mocking the helper away. The complete binding must cover the observed holder/process identity through the actual signal operation, not just a matching value earlier in the loop.
Should-fix — the runbook still overstates the result
The new guide passes .../state.db-wal to sessions recover, whereas this artifact contract and the corrected doctor guidance identify the copied .../state.db as the inspection source. Keep the header_only/missing-image distinction in the guide too. Its unconditional “Nothing is lost” / “Zero Data Loss” wording is not supported when capture or spooling cannot be proved.
The recovery branch also returns from _state_db_health after _recover_retired_wal; its success message uses _session_count, not the normal _db_opens_cleanly write-health probe. Consequently the guide's claims that this branch revalidates FTS/write readiness and resumes saving are stronger than the executed checks. Preserve distinct outcomes for capture verified, holders retired, database write health verified, and the serving runtime resumed; a readable session count or a spawned doctor PID is not all four.
Verification and interlocks
- Local verification performed: an extracted-function harness using the retrieved head's recovery bodies, real temporary manifests/payloads, and injected process-enumeration/signal seams: 9 scenarios, 6 failed safety assertions, 3 passing controls (
python -m pytest test_preservation.py -q --tb=short, exit 1). Failures were PID-only/missing-payload/.partial/altered-payload acceptance and both two-generation orderings. Controls covered refusal without a capture, refusal without a start-time witness, and the single-matching-capture route. - The additional live SQLite capture witness above used real subprocess/filesystem effects. The routing and TERM probes used extracted request/helper code. These are targeted counterexamples, not a claim that I ran the complete repository, Desktop, or packaged E2E suites. The author's 45 Python / 6 Desktop results remain separately attributed, not independently reproduced here.
- Exact-head hosted runs currently end
action_required: CI 34770156484, Docker 34770156262, Nix 34770156228. This is absent green verification, not evidence of a test failure. - #110023, #109997, and #109766 remain open. Preserve #109758's authorship through #110023. Their detection, self-holder-policy, and in-process-heal responsibilities are complementary, not superseded by this PR. The new detailed-descriptor interface is itself Linux-only, and external capture consumes
/proc; composing macOS detection alone does not automatically provide a macOS preservation transaction. Keep that platform contract explicit rather than declaring closure from the PR graph alone.
The class-level acceptance gate is: the exact owner must authorize the operation, and verified preservation must cover every generation and committed state that the operation can retire. The current head still admits counterexamples on both sides of that boundary.
| return d | ||
| captured_ident = tuple(m.get("wal", {}).get("identity") or ()) | ||
| if captured_ident == tuple(wal_identity): | ||
| return d |
There was a problem hiding this comment.
P1 — a manifest tuple is not a verified preservation receipt. This returns success without opening/checking the captured WAL, validating its length/digest, or rejecting .partial publication directories. The preceding wal_identity is None branch additionally accepts PID equality alone when the detailed scan produced no descriptor. In the targeted recovery-function harness, PID-only/missing-payload, matching-identity/missing-payload, .partial, and altered-payload cases all reached termination. Fail closed on missing live identity and require a complete, validated artifact for the actual generation; fix the positive fixtures that currently create only a manifest and expect a successful stop.
| # Precondition 2: Exact-Generation Preservation | ||
| # Ensure this PID's exact orphaned WAL inode has a durable capture BEFORE sending any signal. | ||
| holder_descriptors = { | ||
| r["pid"]: r for r in iter_deleted_sqlite_sidecar_holder_descriptors(state_db_path) |
There was a problem hiding this comment.
P1 — the preservation scope is narrower than the process-wide kill. Keying this dictionary solely by PID discards all but the last retired WAL generation held by that process. With two records for one PID, (1,2) then (1,3), the recovery-function harness captured only (1,3) and sent TERM; reversing enumeration captured only (1,2) and still sent TERM. Retain all distinct generations, deduplicate only identical inodes, and verify the full affected set before authorizing process-wide retirement. A process hosting other profiles also cannot be safely retired on proof covering only this state_db_path.
| raise RetiredGenerationCaptureError( | ||
| f"descriptor at {fd_path} identity {(st.st_dev, st.st_ino)} does not match expected {wal_identity}" | ||
| ) | ||
| wal_size = st.st_size |
There was a problem hiding this comment.
P1 — stable inode identity does not imply stable committed contents. This size is sampled once while the foreign writer remains live. A real SQLite subprocess committed between this sample and _copy_descriptor: the same unlinked WAL grew from 4,152 to 8,272 bytes, but capture returned success with a 4,152-byte manifest/artifact containing only the earlier row. The caller then considers the PID preserved. Establish a quiesced/final capture-and-retirement protocol across all writers, or refuse; an old prefix plus its digest must not authorize closing the last live copy.
| action: { | ||
| label: 'Recover', | ||
| onClick: () => { | ||
| void runDoctor(true) |
There was a problem hiding this comment.
P1 — pin recovery to the failed chat's owner, not the selection at click time. This closure retains neither connection nor profile. runDoctor omits profile scope, and hermesApi adds the ambient connection when invoked. A notification created for gateway-A/research, then clicked after switching to gateway-B, sends {connectionId: 'gateway-B', body: {fix: true}} with no profile. Carry the failure's explicit (connectionId, profile) through the helper/router/spawn boundary and refuse unresolved ownership. Cover background failures and switching before clicking the existing notification.
|
Complements #110097 well — the doctor --fix transaction layer is exactly what an operator needs when the guard fires. The in-product Recover button on the deleted_wal notification closes the loop end-to-end. |
alt-glitch
added
the
comp/dashboard
…rofile
The replaced / deleted_wal / default persistence explanations printed bare
`hermes gateway stop` and `hermes doctor`, while `{home}` in the same sentence
was already profile-aware. On a multi-profile backend (Desktop serve) the
session whose state.db failed is not the process default, and a bare `hermes`
follows the sticky active_profile — so the copy-pasteable command stops or
inspects the wrong profile's database. corrupt / fts_index were pinned by #105887;
this applies the same `{profile_arg}` substitution to every cause
instead of the two-cause tuple.
Spotted via #110073 (@JoaoMarcos44), whose explainer hunk added the selector
to the since-rewritten deleted_wal runbook.
|
FYI: the profile-pinning gap your explainer hunk touched (bare |
|
Item 3 of the maintainer's fix list on #110054 — the The recovery-transaction / holder-termination design in this PR is left as-is for @teknium1's call; the open review threads above are the reviewer's, not touched. |
…ever as "run --fix" `hermes doctor` (without --fix) warned "WAL file is large — run 'hermes doctor --fix' to checkpoint" without checking whether Desktop or the gateway held the database; the holder scan only ran inside --fix. A large WAL is normal for a live writer, and that nudge is how users in the #110054 threads became the second writer that the deleted-WAL guard then fired on. The holder scan now runs on the warn path too: held (or unprovable) reports the size as normal while Desktop/gateway run and orders "stop" before any --fix; no holder keeps the checkpoint suggestion, also stop-first. Refs #110054 (item 3 of the proposed fix), #110073.
|
Supersession check at Landed on
Not on Two small pieces would stand on their own if wanted as fresh commits: the 503 mapping for Recommendation for the maintainer: if no kill path is wanted, close as superseded (concerns 1/2/5 landed, 3/4 rejected by design). |
|
The 503 mapping from this PR landed via #121428 (merged as 4cc4072). Your commit is kept with authorship, and The kill-holders / Desktop "Recover" half isn't included: it conflicts with main's guard against running |
6 participants
What does this PR do?
Provides the user-facing and
hermes doctor --fixrecovery transaction layer for the deleted-WAL generation guard (#110054).Interlock Architecture & Scope
#110054 spans several distinct concerns:
lsoftracking).POST /api/ops/doctor--fixplumbing, and safehermes doctor --fixrecovery transaction. Full platform closure of Pain cluster: after the deleted-WAL guard fires there is no in-product recovery — Desktop users restart/ask-the-agent/run doctor --fix and make it worse (4 Discord threads, 13 issues this week) #110054 occurs when these composed lanes land.Root Cause & Pain Points Addressed
DeletedWalGenerationErrorbecause another process (updater, backup restore, or second instance) replacedstate.db, the chat outputted an operator-level forensics dump advising users to inspect manifests and stop services, with no clear user reassurance or in-product action.hermes doctor --fixwhile a background gateway or desktop worker held the retired WAL descriptor, doctor warned about a large WAL file and advised runningdoctor --fix, which then skipped the checkpoint because live writers held the database.Safety Invariants Implemented
_recover_retired_wal()verifies that the holder's exact unlinked WAL inode(st_dev, st_ino)has a durable, identity-matching capture instate.db.retired-wal-*/manifest.jsonBEFORE sending any signal./proc/<pid>/fd/<fd>(capture_external_retired_wal_generation).get_process_start_time(pid)) while observing holder descriptors; refuses if unavailable.expected_start_time). If the process exited or PID changed, no signal is sent to the replacement.os.getpid()) is strictly excluded.hermes sessions recover --source .../state.db --inspect-onlywhenmanifest.main.mode == "copied".header_onlyartifacts, explicitly reports that the artifact is forensic-only and directs the operator to inspectmanifest.json, never advertising nonexistentstate.dbfiles.hermes {profile_arg}doctor --fix."Run doctor --fixin Command Center Maintenance.DeletedWalGenerationErrorandStateDbReplacedErrorto structured HTTP 503 incorrupt_store_as_statusto prevent dashboard 500 polling crashes.Related Issue
Addresses #110054 (User-facing & doctor recovery transaction layer)
Type of Change
How to Test
Verified all 45 Python tests passed (including full hostile recovery matrix: stable holder termination, exited holder, PID recycling between TERM and KILL, unavailable identity, uncaptured WAL, header_only guidance, current PID exclusion, and 503 status mapping).
Desktop Vitest: 6/6 passed.
Fixes #110054