Skip to content

Report unreachable local daemon errors - #266

Merged
wesm merged 6 commits into
kenn-io:mainfrom
codyw912:fix-unreachable-local-daemon
Aug 13, 2026
Merged

Report unreachable local daemon errors#266
wesm merged 6 commits into
kenn-io:mainfrom
codyw912:fix-unreachable-local-daemon

Conversation

@codyw912

Copy link
Copy Markdown
Contributor

Summary

  • preserve local daemon probe errors when a live runtime PID exists
  • avoid starting a redundant daemon when the recorded endpoint is inaccessible
  • report the PID, endpoint, and underlying connection failure to the caller

Motivation

A sandboxed client can sometimes read the daemon runtime metadata while being
unable to access its Unix socket. Previously, health checks reported that no
daemon was running and normal commands could attempt another start, eventually
reporting a startup timeout.

This distinguishes an absent daemon from a live but unreachable one while
preserving the existing daemon_unavailable CLI error category.

@roborev-ci

roborev-ci Bot commented Aug 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (d307167)

The change has one medium-severity readiness regression; no security issues were identified.

Medium

  • internal/client/ensure.go:290autoStart aborts on the first ErrLocalDaemonUnreachable. Because the daemon publishes its runtime record before ServeListeners begins, a valid startup may briefly expose a live PID and endpoint that is not yet responsive. This bypasses the intended 30-second readiness window and can cause spurious failures on slower startups.

    Suggested fix: Continue polling when ErrLocalDaemonUnreachable occurs during auto-start, optionally retaining it for the final timeout error. Add a test where discovery initially returns this error and later succeeds.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 3m15s

@roborev-ci

roborev-ci Bot commented Aug 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (2461995)

One medium-severity issue: stale PID reuse can permanently block daemon auto-start and offline commands.

Medium

  • internal/client/client.go:98, internal/client/ensure.go:192ProcessAlive only establishes that some process owns the recorded PID. If a daemon crashes and its PID is reused, the stale runtime record is treated as an unreachable daemon, preventing auto-start even though no Kata daemon exists. Record and verify process-instance identity—such as process start metadata or a held lock—and ignore stale records whose PID belongs to another process.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 4m27s

@roborev-ci

roborev-ci Bot commented Aug 12, 2026

Copy link
Copy Markdown

roborev: Combined Review (0f48e24)

Code review passed with no Medium, High, or Critical findings.


Reviewers: 2 done | Synthesis: codex, 7s | Total: 4m28s

@wesm

wesm commented Aug 13, 2026

Copy link
Copy Markdown
Member

looking at this, I think I just fixed this bug in kit (Go library we share between a bunch of projects) but I might be wrong

Kit 0.20.0 records stronger process identities on Linux and preserves legacy identities for compatibility. Kata must compare the complete runtime record so a reused PID does not block daemon discovery or auto-start.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 13, 2026

Copy link
Copy Markdown

roborev: Combined Review (ff500c7)

Code review found one medium-severity issue involving mixed daemon runtime records.

Medium

  • internal/client/ensure.go:217 — When records contain both an unreachable live daemon and a reachable incompatible daemon, staleURL takes precedence over unreachable. After replacing the incompatible daemon, the wait loop discards discovery errors, potentially allowing another daemon to auto-start while the unreachable process is still running. Prioritize unreachable before staleURL, preserve errors while waiting for explicitly signaled daemons to stop, and add coverage for both record types appearing together.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 5m24s

A reachable old daemon must not hide another live daemon whose endpoint is inaccessible. Preserve that discovery error before replacement and during the post-signal wait so Kata cannot start a competing daemon.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 13, 2026

Copy link
Copy Markdown

roborev: Combined Review (42b9f42)

Code review found one medium-severity issue in runtime process-identity validation.

Medium

  • internal/client/client.go:120 — Process-identity validation is limited to discovery. daemon start, status, stop, restart, and reload still rely solely on ProcessAlive, so a stale runtime record with a reused PID can block startup or cause commands to signal an unrelated process. Apply CompareRuntimeProcessIdentity consistently before any runtime-record consumer reports, selects, or signals the PID.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 4m46s

A stale runtime file can name a PID that the operating system has assigned to an unrelated process. Require every runtime-record consumer to reject a definite process-identity mismatch before it reports, selects, or signals that PID.

Generated with Codex
Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 13, 2026

Copy link
Copy Markdown

roborev: Combined Review (d21f7cc)

Code changes are clean with no Medium, High, or Critical findings.


Reviewers: 2 done | Synthesis: codex, 5s | Total: 4m50s

@wesm

wesm commented Aug 13, 2026

Copy link
Copy Markdown
Member

will merge pending CI

@wesm
wesm merged commit e62bb67 into kenn-io:main Aug 13, 2026
14 checks passed
wesm pushed a commit to kenn-io/kit that referenced this pull request Aug 24, 2026
A sandboxed client can read a live daemon's runtime record while still being unable to reach its endpoint. Discovery previously collapsed that probe failure into absence, so `Manager.Ensure` could start a competing daemon against the same state.

PID-aware discovery now skips definite process-identity mismatches, keeps scanning after a failed probe, and returns a typed `UnreachableError` with the record, endpoint, and underlying probe failure when no later record succeeds. The manager already stops on discovery errors, so this prevents an unsafe start without adding restart policy to the shared package.

The repository hooks had a stale whole-repo lint, NilAway, Testify-helper, and Git-fixture baseline that blocked the daemon change. Most of the resulting test churn is mechanical helper conversion. The fixture changes isolate global and system Git config and strip repository-local variables inherited by hooks, which makes the shuffled suite deterministic under the real commit environment.

Sources of inspiration:

- [Vite Plus v0.2.9](https://github.com/voidzero-dev/vite-plus/releases/tag/v0.2.9), with the sandbox IPC work in [Vite Task #569](voidzero-dev/vite-task#569) and [#576](voidzero-dev/vite-task#576)
- [Kata #266](kenn-io/kata#266), which first kept a live daemon's failed probe distinct from absence
- [RoboRev #1021](kenn-io/roborev#1021), which applied the same boundary to daemon access errors and lifecycle decisions


Co-authored-by: Marius van Niekerk <mariusvniekerk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants