SEC-007: Harden backend process recovery

[budzeg]

Summary

Berd’s backend process recovery and shutdown handling relied on mutable pathnames and process identifiers that could become stale. This could allow pathname substitution, unsafe record permissions, PID reuse, or cleanup races to cause Berd to signal an unrelated process or modify the wrong filesystem object.

• Add a bounded, owner-private recovery-record store with retained-directory operations, no-replace publication, verified reads, and exact-object deletion.
• Bind process signaling to captured identity, revalidate identity before escalation, and retain recovery evidence whenever exit or exact cleanup cannot be confirmed.
• Make backend startup, readiness-failure handling, singleton shutdown, and app-exit teardown bounded, asynchronous, and successor-safe.
• Fail closed on macOS by using deletion-only recovery records when stable process-bound executable identity is unavailable.
• Harden Windows storage with retained-root handle operations, owner-only protected DACL validation, reparse rejection, bounded directory parsing, handle-relative rename, and exact-handle deletion.
• Add lifecycle and adversarial regressions covering PID reuse, identity changes, publication and retention failures, cleanup timeouts, path substitution, hostile ACLs, root swaps, reparse children, and pathname successors.

This is a high-sensitivity change to Berd’s backend lifecycle and local recovery-record storage, with platform-specific behavior. No current architectural law directly governs backend process-record storage or teardown (LAWS/README.md, LAWS/AGENTS.md, LAWS/CHAT.md).

Full native Windows adversarial verification remains a merge blocker. GitHub’s native Windows/MSVC CI compiled the implementation and passed its 71 managed-service tests on exact head da966afa688779903e9b8a04b8913bf53890a7cd. That CI evidence does not exercise the complete Windows adversarial suite. Before final QA or merge, an authorized native Windows/MSVC host must still validate the adversarial ACL, retained-root enumeration, handle-relative no-replace rename, exact-handle deletion, root-swap, reparse-child, pathname-substitution, and pathname-successor scenarios.

Related issue

N/A — no public issue was opened because the underlying report is security-sensitive. No duplicate public issue or PR is cited.

Testing

Locally available macOS gates passed on the implementation snapshot:

• Rust formatting and check
• just tauri-check
• just clippy
• Focused recovery-store tests
• Backend lifecycle tests

Two broader backend tests still fail because of known unrelated default-provider fixtures.

GitHub’s native Windows/MSVC CI compiled the implementation and passed 71 managed-service tests on exact head da966afa688779903e9b8a04b8913bf53890a7cd; all GitHub checks on that head passed. Full native Windows adversarial execution remains outstanding and merge-blocking as described above.

Generated with Goose

[Copilot]

🟡 Changes recommended

There are verified security/correctness issues in the Unix record publication/cleanup path (name substitution + unbound deletion) and a concrete portability build break on non-Linux/macOS Unix targets.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens Berd’s backend (Goose serve) lifecycle recovery by replacing path/PID-based recovery handling with a verified, bounded process-record store and process-identity-based signaling decisions across platforms (including special “fail closed” handling on macOS and hardened handle-relative operations on Windows).

Changes:

• Introduces a new ProcessRecordStore that publishes and cleans up recovery records using owner-private, no-replace, successor-safe filesystem operations (Unix + Windows-specific implementations).
• Switches Goose serve startup/shutdown and stale-process cleanup to use verified record reads, exact-object deletion, and identity probes before signaling/escalation.
• Extends process identity plumbing on Unix (Linux + macOS) and updates shutdown flow to be asynchronous and bounded.

File summaries

File	Description
src-tauri/src/services/process.rs	Expands process identity support beyond Windows and adds Unix identity capture/probe helpers.
src-tauri/src/services/acp/process_record_store.rs	Adds the verified, bounded, platform-specific recovery record store and its adversarial tests.
src-tauri/src/services/acp/mod.rs	Wires the new process_record_store module into ACP services.
src-tauri/src/services/acp/goose_serve.rs	Migrates Goose serve lifecycle + stale recovery to the new store and identity-based signaling/retention behavior.
src-tauri/src/lib.rs	Updates app-exit teardown to await async singleton shutdown.
src-tauri/Cargo.toml	Adds required windows-sys feature flags for the new Windows ACL / NT API usage.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

It’s a high-sensitivity, platform-specific lifecycle/security change with outstanding native Windows adversarial verification, and it includes at least one correctness/robustness issue to address (stored PR comment).

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The Unix stale-process cleanup retains recovery records even when a follow-up identity probe could confirm the process is gone/mismatched after a SIGTERM/SIGKILL failure, which can leave behind stale records indefinitely.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src-tauri/src/services/acp/goose_serve.rs:889

• If SIGKILL fails, we unconditionally retain the record. When the failure is due to the process already being gone (or the PID having already moved to a mismatched identity), a follow-up identity probe would allow us to remove the exact verified record and avoid leaving stale recovery evidence indefinitely.

    if !kill(pid) {
        log::warn!(
            "SIGKILL failed for exact goose serve {}; keeping {}",
            identity.pid,
            path.display()

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

This is a high-sensitivity, cross-platform lifecycle/security change with at least one confirmed correctness issue in identity stability that can undermine orphan cleanup behavior.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The Windows implementation has a compile-time issue (STATUS_OBJECT_NAME_NOT_FOUND is referenced but not imported/defined), which must be fixed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It’s a high-sensitivity, platform-specific security hardening change with an explicit merge-blocking requirement for native Windows adversarial verification still outstanding.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Unix record publication can leave a hardlinked record in an undeletable/invalid state if temp-name unlink fails after a successful publish, breaking cleanup guarantees.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It is a high-sensitivity cross-platform security change and the PR description explicitly notes outstanding native Windows adversarial verification as merge-blocking.

Review details

Suppressed comments (7)

src-tauri/src/services/acp/goose_serve.rs:761

• remove_verified() errors are ignored here; if exact deletion fails, the record will persist without any log signal. Logging the error will help diagnose recovery-record retention and potential directory pollution.

            let _ = store.remove_verified(path, verified);

src-tauri/src/services/acp/goose_serve.rs:820

• remove_verified() failures are silently ignored here; if exact deletion fails, stale or invalid records can accumulate without any diagnostic breadcrumb. Prefer logging the error (but still continuing) to keep startup recovery transparent.

        let _ = store.remove_verified(path, verified);

src-tauri/src/services/acp/goose_serve.rs:825

• This exact-record deletion is best-effort, but errors are dropped. If identity-bound deletion fails, logging helps explain why a record continues to reappear on subsequent startups.

            let _ = store.remove_verified(path, verified);

src-tauri/src/services/acp/goose_serve.rs:857

• When SIGTERM fails but a follow-up probe confirms Gone/Mismatch, the record cleanup still ignores remove_verified() errors. Please log failures so you can distinguish 'record intentionally retained' from 'cleanup failed'.

                let _ = store.remove_verified(path, verified);

src-tauri/src/services/acp/goose_serve.rs:870

• Exact cleanup is attempted here, but any remove_verified() error is discarded; that can silently leave behind a record even after a Gone/Mismatch probe. Logging failures would make this path diagnosable.

            let _ = store.remove_verified(path, verified);

src-tauri/src/services/acp/goose_serve.rs:893

• If SIGKILL fails but the follow-up probe confirms Gone/Mismatch, cleanup ignores any remove_verified() error. Please log deletion failures to avoid silently retaining records due to substitution/permission races.

                let _ = store.remove_verified(path, verified);

src-tauri/src/services/acp/goose_serve.rs:906

• This final best-effort cleanup drops remove_verified() errors; if the record can't be deleted exactly, it will remain with no log. Logging the error keeps recovery-record retention explainable in the field.

            let _ = store.remove_verified(path, verified);

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Security-sensitive, cross-platform process/FS hardening changes still need manual validation (including the stated native Windows adversarial verification) and there are correctness issues to address.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

High-sensitivity, cross-platform process/FS hardening with a confirmed Unix enumeration error-handling gap plus outstanding native Windows adversarial verification called out as merge-blocking in the PR description.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces high-sensitivity, cross-platform process/FS security changes and still depends on outstanding native Windows adversarial verification before merge.

Review details

• Files reviewed: 6/7 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Unresolved critical recovery races remain, and full native Windows adversarial verification is still merge-blocking.

Review details

Suppressed comments (4)

src-tauri/src/services/acp/goose_serve.rs:733

• [P1] If startup fails after publishing a record but before GOOSE_SERVE is initialized (for example, retention or teardown times out), a retry runs in the same app process with this owner identity still matching. This branch leaves the record without checking serve_identity, so a still-running failed child is never recovered and the record persists across retries. Treat a matching owner in an uninitialized singleton as a failed-startup record and run serve cleanup.

    match crate::services::process::probe_process_identity(owner_identity) {
        IdentityProbe::Matches => {
            log::debug!(
                "Process record {} is still owned by live exact process {}; leaving it alone",
                path.display(),
                record.owner_pid
            );
            return;

src-tauri/src/services/acp/process_record_store.rs:627

• [Security] Unix remove_verified ultimately reopens the pathname and performs fstatat followed by unlinkat; the identity check can race with a same-user rename-and-replace between those syscalls. In that window unlinkat deletes the successor at path, not the retained object, so this is not exact-object deletion on Unix. Use an fd-bound deletion approach where available or fail closed instead of relying on this check-then-unlink sequence.

    pub(super) fn remove_verified(
        root_path: &Path,
        root: &RootHandle,
        path: &Path,
        verified: &VerifiedRecord,

src-tauri/src/services/acp/process_record_store.rs:447

• After linkat succeeds, both cleanup calls still resolve names rather than the retained source object. A hostile same-user process can replace from before unlink(from), which deletes the successor and leaves the published object with two links; if rollback runs, unlink(to) has the same pathname race. That can delete an unrelated object or permanently fail the later nlink == 1 validation. Bind rollback/unlink to object identity or use an atomic publication primitive.

        if let Err(temp_error) = unlink(root, &from) {
            // Publication created `to` as a second name for the retained source.
            // Roll it back before the caller cleans the identity-bound temp name,
            // otherwise both names retain nlink == 2 and fail metadata validation.
            let rollback_error = unlink(root, &to).err();

src-tauri/src/services/acp/process_record_store.rs:1216

• When NextEntryOffset is nonzero it must point to another entry inside the returned buffer. Allowing next == remaining.len() advances exactly to EOF and returns success even though no next entry exists, silently accepting a truncated directory result and potentially skipping stale records during recovery.

                if next < minimum_next || !next.is_multiple_of(8) || next > remaining.len() {

• Files reviewed: 6/7 changed files
• Comments generated: 7
• Review effort level: Lite

[budzeg]

🤖 Implemented the consolidated recovery fixes in d8f97b1 on the existing PR branch.

• Publication returns its original retained file handle; startup no longer reopens the published path.
• Linux probes and signals one retained process descriptor. There is no numeric-PID fallback for stale recovery.
• Unix publication uses exclusive creation and a completion marker. Unix record deletion deliberately retains evidence because a check followed by pathname unlink cannot guarantee the same object. This also removes the link/unlink rollback race.
• Windows deletion revalidates both retained record and root ACLs. Directory parsing rejects a nonterminal offset at EOF.
• Already-reaped children count as exited. Failed startup retains any unreaped child and retries teardown before allowing a replacement backend.
• Recovery streams through a bounded buffer with a two-second budget and continues beyond its first 256 entries.
• The private app-data location and the decision not to read or migrate shared-temp JSON records remain intentional.

Validation on this commit: 36 focused macOS tests, formatting, all just tauri-check configurations, and all just clippy configurations passed. Windows storage/process modules and their tests also passed a focused cross-compilation check. One independent review identified scan-prefix starvation; the streaming scan and regression test address it. The two previously documented provider-fixture failures remain outside the focused recovery suite.

The existing CI gates now run the recovery, record-store, and process suites on native Linux and Windows. Their results on this commit remain required before merge; earlier managed-service results do not substitute for them. The PR remains a draft.

The current platform contract, including Unix evidence retention and the macOS no-stale-signal fallback, is documented in docs/process-recovery.md. This supersedes the description's older references to universal exact-object deletion and macOS deletion-only records.

[morgmart]

🤖 Automated code review

REQUEST_CHANGES: Linux recovery passes a proc-directory descriptor to pidfd_send_signal instead of a pidfd, so verified stale backends cannot be signaled. This is backend-only work with no graphical UI surface. Supplied GitHub check evidence was inspected separately and does not alter the blocking code finding.

Deterministic publication result: 1 blocking and 0 non-blocking inline finding(s) publishable; 0 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.

[morgmart]

🤖 Automated code review

COMMENT: No new publishable implementation findings remain. The prior Linux signaling concern is protected by a resolved thread with a substantive human reply and supporting exact-head CI evidence. Morgan should explicitly decide whether the documented Unix/macOS recovery-record retention and manual-cleanup lifecycle is acceptable for release. All supplied GitHub checks for the exact head are complete and successful, while required checks remain the merge-readiness authority.

Deterministic publication result: 0 blocking and 0 non-blocking inline finding(s) publishable; 1 duplicate(s) suppressed; 0 blocking screenshot-evidence requirement(s) in this review body.