feat: daemon connect-or-start with Unix socket IPC#67
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds a UNIX-domain socket daemon (server + client), socket-based health/shutdown endpoints, auto-start/spawn logic with marker caching, idle-timeout and migration from PID-file daemons, envelope freshness validation, new daemon CLI commands, and extensive tests for these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (status/doctor)
participant Client as DaemonClient
participant Spawn as Daemon Spawn
participant Server as SocketServer
participant Health as /health handler
CLI->>Client: EnsureDaemon(configPath)
Client->>Client: IsRunning() — dial socket
alt daemon running
Client->>Server: GET /health via unix socket
Server->>Health: handle request
Health-->>Client: {status,pid,uptime}
Client-->>CLI: report running
else daemon not running
Client->>Spawn: spawnDaemon() (jitter, setsid, redirect logs)
Spawn->>Server: SocketServer starts and binds socket
Client->>Client: poll IsRunning() until ready
Client->>Server: GET /health
Server-->>Client: {status,pid,uptime}
Client-->>CLI: report started
end
sequenceDiagram
participant CLI as CLI (status-line)
participant Bridge as Bridge
participant Client as DaemonClient
participant Server as SocketServer
CLI->>Bridge: feedData(envelope)
Bridge->>Bridge: IsEnvelopeFresh(envelope)
alt fresh
Bridge-->>CLI: feed data used
else stale/missing
Bridge-->>CLI: treat as nil
end
CLI->>Client: Health()
Client->>Server: GET /health via socket
alt responds
Server-->>Client: {status,pid,uptime}
Client-->>CLI: include daemon info
else no response
Client-->>CLI: daemon not running / suggest start
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/hookwise/cli_test.go`:
- Around line 1268-1403: The tests (TestDaemonCommandExists,
TestDaemonSubcommands, TestDaemonRunIsHidden, TestDaemonRunHasConfigFlag,
TestDaemonStopNotRunning, TestDoctorDaemonNotRunning,
TestEnsureDaemonWithCache_MarkerFileCaching) use stdlib t.Error/t.Fatalf;
replace these with testify's assert/require per project convention: add the
testify/assert and testify/require imports, use require.* (e.g.,
require.NoError, require.True, require.FileExists) for fatal assertions that
should stop the test and assert.* (e.g., assert.Equal, assert.Contains,
assert.True, assert.NoError) for non-fatal checks, and update checks like
client.IsRunning(), rootCmd.Find errors, executeCommand error, output contains
checks, and file existence checks to use the appropriate assert/require calls.
In `@cmd/hookwise/main.go`:
- Line 1658: The code is using the frozen core.DefaultSocketPath and
daemonAliveMarkerPath instead of resolving them from core.GetStateDir() at call
time; update every call that constructs a daemon client or checks the alive
marker (e.g., the feeds.NewDaemonClient(...) calls and any use of
daemonAliveMarkerPath around the areas mentioned) to compute the socket and
marker paths by calling core.GetStateDir() and joining the appropriate filenames
so they respect HOOKWISE_STATE_DIR at runtime (mirror how runDoctor() and cache
loading derive their paths). Ensure you replace direct references to
core.DefaultSocketPath and the frozen daemonAliveMarkerPath with freshly
computed paths derived from core.GetStateDir() wherever daemon start/stop,
status-line auto-start, or NewDaemonClient are used.
- Around line 1756-1760: The freshness check on daemonAliveMarkerPath currently
short-circuits EnsureDaemon() and can hide a stopped/crashed daemon for up to
60s; update the fast-path in the status-line logic to revalidate the actual
socket/daemon before returning: when os.Stat(daemonAliveMarkerPath) shows a
recent ModTime, attempt to connect to the daemon socket (or invoke
EnsureDaemon() / the existing connection-check routine) and only return early if
that connection succeeds; if the socket connection fails, fall through to the
normal EnsureDaemon()/start-or-reconnect flow so the daemon will be restarted if
needed.
In `@internal/feeds/client.go`:
- Around line 102-108: EnsureDaemon currently spawns the child daemon without
propagating c.socketPath so the child binds a different socket; modify the spawn
logic to pass the socket path to the child (e.g. add "--socket", c.socketPath to
args when non-empty) and update newDaemonRunCmd()/daemon run handling to accept
and use that flag to bind the same socket; locate the spawn site that builds
args (the args slice and exec.Command call) and the daemon run command parser to
add and handle the new socket flag so both parent dialing and child binding use
the same c.socketPath.
- Around line 135-150: The shutdown path in the method uses c.httpClient.Post
and always returns nil even if the HTTP response indicates failure or the daemon
remains running after the 5s poll; update the logic so that after calling
c.httpClient.Post("http://unix/shutdown", ...) you verify the HTTP response
status code (treat non-2xx as an error and return a wrapped fmt.Errorf including
status) and close the response body; then keep the polling loop that uses
c.IsRunning(), but if the deadline passes and c.IsRunning() is still true return
a non-nil error (e.g., fmt.Errorf("feeds: shutdown did not complete within
timeout")) so callers can distinguish a hung daemon from a clean stop.
In `@internal/feeds/daemon.go`:
- Around line 99-115: The daemon races and can panic by closing a nil or
already-closed stopCh from multiple shutdown paths; centralize shutdown by
adding a private sync.Once on the daemon (e.g., d.once) and a helper method
(e.g., d.shutdownOnce()) that calls once.Do to close d.stopCh (initializing
d.stopCh before server.Start() returns) and wraps the close in a deferred
recover to swallow double-close panics; update the NewSocketServer shutdown
callback, Stop(), startIdleMonitor, and installSignalHandler to call
d.shutdownOnce() instead of closing stopCh directly, and adjust tests to use
Stop() rather than direct server.Shutdown/removePIDFile so all cleanup goes
through the single helper.
In `@internal/feeds/feeds_test.go`:
- Around line 714-719: Replace the t.Fatal call in the select block that waits
for daemon shutdown with a testify/require call for consistency: change the
failing branch inside the select (the case triggered by time.After(5 *
time.Second)) to call require.FailNow(t, "daemon did not shut down within
timeout after idle timeout"). Ensure the test file already imports the
testify/require package (or add the import) so require.FailNow is available and
no other behavior changes.
- Around line 1511-1544: The test currently only collects Start() errors so the
goroutine whose Start() succeeds (the winning *Daemon) is never stopped; update
the goroutines/results to return the started *Daemon along with the error (e.g.
send a small struct containing err and d from the anonymous func that creates
NewDaemon/sets paths), capture the winning *Daemon after reading from results,
and ensure it is cleaned up with either t.Cleanup(func(){ _ =
winningDaemon.Stop() }) or by calling winningDaemon.Stop() before the test
returns; reference NewDaemon, (*Daemon).Start, and (*Daemon).Stop to locate and
change the logic around the results channel and the winningDaemon variable.
In `@internal/feeds/polling.go`:
- Around line 72-73: The idle timer reset is currently only called after
successful polls, so failures or panics still allow idle shutdown; move the call
to d.resetIdleTimer() into a defer at the start of runProducer() so it runs on
all exits (success, error, or panic). Specifically, open runProducer() and add a
single defer d.resetIdleTimer() near the top of that function so any early
return from Produce() or other error paths still count as activity; leave the
existing call or remove duplicates to avoid double-reset if desired.
In `@internal/feeds/socket.go`:
- Around line 93-99: The background goroutines that call
s.server.Serve(listener) and the shutdown goroutine (the other side-effecting
goroutine launched in this file) need panic boundaries: wrap their top-level
goroutine bodies with a defer func() { if r := recover(); r != nil {
fmt.Fprintf(os.Stderr, "hookwise: panic in socket goroutine: %v\n%s\n", r,
debug.Stack()) } }() so any panic is recovered and logged (use
runtime/debug.Stack() for stack trace) before running the existing Serve/
shutdown logic; apply this pattern to the goroutine that invokes
s.server.Serve(listener) and the shutdown goroutine to prevent the daemon from
crashing on panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2eb95c4-3737-4d38-b925-0dafd524f196
📒 Files selected for processing (12)
cmd/hookwise/cli_test.gocmd/hookwise/main.gointernal/bridge/bridge.gointernal/bridge/bridge_test.gointernal/core/constants.gointernal/feeds/client.gointernal/feeds/client_test.gointernal/feeds/daemon.gointernal/feeds/feeds_test.gointernal/feeds/polling.gointernal/feeds/socket.gointernal/feeds/socket_test.go
Comment on lines
+286
to
+348
| // migrateOldDaemon detects and shuts down an old PID-only daemon (MIGRATE-1). | ||
| // An old-style daemon is identified by: PID file exists AND socket file does NOT exist. | ||
| // This runs BEFORE socket bind, so it doesn't interfere with new startup. | ||
| func (d *Daemon) migrateOldDaemon() { | ||
| // Check if PID file exists. | ||
| _, pidErr := os.Stat(d.pidFile) | ||
| if pidErr != nil { | ||
| return // No PID file — nothing to migrate. | ||
| } | ||
|
|
||
| // Check if socket file exists. | ||
| _, sockErr := os.Stat(d.socketPath) | ||
| if sockErr == nil { | ||
| return // Socket exists — this is a new-style daemon, not an old one. | ||
| } | ||
|
|
||
| // Old-style daemon detected: PID file exists, socket does not. | ||
| core.Logger().Info("feeds: detected old-style daemon, attempting migration") | ||
|
|
||
| pid := d.readPID() | ||
| if pid <= 0 { | ||
| // Invalid PID file — just clean up. | ||
| os.Remove(d.pidFile) | ||
| return | ||
| } | ||
|
|
||
| // Verify the process is alive. | ||
| if !isProcessAliveByPID(pid) { | ||
| // Stale PID file — clean up. | ||
| core.Logger().Info("feeds: old daemon not running, removing stale PID file", "pid", pid) | ||
| os.Remove(d.pidFile) | ||
| return | ||
| } | ||
|
|
||
| // Send SIGTERM and wait for the old daemon to exit. | ||
| core.Logger().Info("feeds: sending SIGTERM to old daemon", "pid", pid) | ||
| proc, err := os.FindProcess(pid) | ||
| if err != nil { | ||
| os.Remove(d.pidFile) | ||
| return | ||
| } | ||
|
|
||
| if err := proc.Signal(syscall.SIGTERM); err != nil { | ||
| core.Logger().Error("feeds: failed to send SIGTERM to old daemon", "pid", pid, "error", err) | ||
| os.Remove(d.pidFile) | ||
| return | ||
| } | ||
|
|
||
| // Wait up to 3 seconds for the process to exit. | ||
| deadline := time.Now().Add(3 * time.Second) | ||
| for time.Now().Before(deadline) { | ||
| if !isProcessAliveByPID(pid) { | ||
| core.Logger().Info("feeds: old daemon exited", "pid", pid) | ||
| os.Remove(d.pidFile) | ||
| return | ||
| } | ||
| time.Sleep(100 * time.Millisecond) | ||
| } | ||
|
|
||
| // Process didn't exit in time — remove PID file anyway and proceed. | ||
| core.Logger().Error("feeds: old daemon did not exit within timeout", "pid", pid) | ||
| os.Remove(d.pidFile) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the daemon.go file around the relevant lines and look for readPID and PID file handling
head -400 internal/feeds/daemon.go | tail -150Repository: vishnujayvel/hookwise
Length of output: 4127
🏁 Script executed:
# Look for readPID function definition
rg "func.*readPID" -A 10 internal/feeds/Repository: vishnujayvel/hookwise
Length of output: 544
🏁 Script executed:
# Search for how the PID file is written/created to understand its format
rg "pidFile.*=" -B 2 -A 2 internal/feeds/daemon.goRepository: vishnujayvel/hookwise
Length of output: 424
🏁 Script executed:
# Look for isProcessAliveByPID implementation
rg "func.*isProcessAliveByPID" -A 15 internal/feeds/Repository: vishnujayvel/hookwise
Length of output: 935
🏁 Script executed:
# Check if there are any process identity verification patterns elsewhere in the code
rg "process.*name\|process.*command\|proc.*Args\|Cmdline" internal/ -t goRepository: vishnujayvel/hookwise
Length of output: 47
Don't auto-SIGTERM a PID without verifying process identity.
The PID file stores only a numeric PID—no process name, start time, or other identifier. Combined with stale files and PID reuse, migrateOldDaemon can send SIGTERM to an unrelated process. If that process survives the signal, line 347 removes the PID file anyway and startup continues, risking two daemons polling in parallel.
Currently, isProcessAliveByPID only confirms the PID exists; it doesn't verify the process is the old hookwise daemon. Without process identity verification (e.g., checking process name or command-line arguments), this migration is unsafe. Either verify the process identity before signaling, or fail startup and require manual intervention.
Owner
Author
There was a problem hiding this comment.
Known limitation, accepted risk for now. The migration code (migrateOldDaemon) only runs when the PID file exists AND the socket file does NOT exist AND isProcessAliveByPID returns true. This is a temporary migration path from PID-only to socket-based daemon management — full PID file removal is deferred to Phase 2 per scope cuts. The window for PID reuse collision is narrow (requires: old daemon crashed, PID recycled, no socket file present), and the consequence is a SIGTERM to an unrelated process which is non-destructive on most workloads.
There was a problem hiding this comment.
@vishnujayvel, understood — the narrow preconditions (PID file present + no socket + live PID) and the temporary nature of this migration path make the risk reasonable to accept for now. I'll note this as a known, accepted limitation for future reviews.
✏️ Learnings added
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 67
File: internal/feeds/daemon.go:279-341
Timestamp: 2026-03-10T00:43:30.109Z
Learning: In `internal/feeds/daemon.go` (vishnujayvel/hookwise), `migrateOldDaemon()` sends SIGTERM to the PID from the PID file without verifying process identity. This is a known, accepted risk: the window for PID reuse requires the old daemon to have crashed, the PID to be recycled by an unrelated process, and no socket file present — treated as a narrow edge case with non-destructive consequence (SIGTERM). This is a temporary migration path from PID-only to socket-based daemon management; full PID file removal and a safer migration approach are deferred to Phase 2.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: hooks/agent-tracker.sh:0-0
Timestamp: 2026-03-06T05:48:03.577Z
Learning: In `hooks/agent-tracker.sh` (vishnujayvel/hookwise), the read/modify/write on `active-agents.json` is intentionally not locked. Claude Code invokes hooks serially (one hook per event), so concurrent writes only occur with parallel Claude Code sessions sharing the same `HOOKWISE_STATE_DIR` — treated as an edge case. Adding `flock` is deferred to a follow-up hardening pass due to the added POSIX dependency. The atomic `mv` via temp file is sufficient for the single-session case.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tui/tests/test_snapshots.py:73-73
Timestamp: 2026-03-06T05:48:08.174Z
Learning: In `tui/tests/test_snapshots.py` (vishnujayvel/hookwise), mocking `HookwiseTUI._write_pid()` in snapshot tests is deferred to a follow-up PR. The current behavior (writing to `~/.hookwise/tui.pid` during tests) is considered safe in practice due to single-threaded execution and the PID guard in `_remove_pid`. The planned fix is to add a `monkeypatch` fixture in a `conftest.py` to no-op `_write_pid`, ensuring no real filesystem side effects during snapshot tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/hookwise/main.go (1)
408-416:⚠️ Potential issue | 🟠 MajorThe global daemon/cache key breaks repo-scoped feeds.
Everything here is keyed off
core.GetStateDir(), so every repo reuses the same socket andstate/*.json. That works for global feeds, but theprojectfeed is cwd-dependent; once repo A starts the daemon, repo B will keep rendering repo A's cached project data until the daemon is restarted. Consider namespacing the daemon/cache by project root, or excluding project-scoped feeds from the shared daemon.Also applies to: 1755-1778
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/hookwise/main.go` around lines 408 - 416, The daemon/socket and feed cache are currently global because code uses core.GetStateDir() (in the lines calling ensureDaemonWithCache(configPath) and bridge.CollectFeedCache(cacheDir)), which causes repo-scoped feeds (like the project feed derived from projectDir/project config) to leak between repositories; fix by namespacing the daemon and cache by project root when working with project-scoped feeds: compute a project-specific state directory (e.g., append a stable project identifier derived from projectDir or core.ProjectConfigFile — e.g., a short hash of projectDir) and use that for ensureDaemonWithCache and for cacheDir passed to bridge.CollectFeedCache, or alternatively detect and exclude project-scoped feeds from the shared daemon and load them directly from projectDir; update references to core.GetStateDir(), ensureDaemonWithCache(configPath), bridge.CollectFeedCache(cacheDir), and feedCache to use the project-scoped state dir or the direct-per-repo loading path.internal/feeds/daemon.go (1)
49-57:⚠️ Potential issue | 🟠 MajorOverride PIDFile and CacheDir at daemon startup to match the socket path when
HOOKWISE_STATE_DIRis set.
daemon runonly callsSetSocketPath(), leavingpidFileandcacheDirinitialized from the staticDefaultPIDPathandDefaultCachePath. WhenHOOKWISE_STATE_DIRis set, the socket correctly resolves to the override location, but the daemon still writesdaemon.pidand state files to~/.hookwise/, breaking state directory isolation. Both must be overridden at runtime alongside the socket, or the defaults must be computed lazily viaGetStateDir()rather than frozen at module init.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/feeds/daemon.go` around lines 49 - 57, NewDaemon currently initializes pidFile and cacheDir from DefaultPIDPath and DefaultCachePath at construction, so when SetSocketPath is later called (e.g., because HOOKWISE_STATE_DIR is set) only socketPath is updated and pidFile/cacheDir still point to the original defaults; update NewDaemon to compute pidFile and cacheDir lazily or call the same state-dir resolution used by SetSocketPath so they follow HOOKWISE_STATE_DIR—specifically ensure NewDaemon (or the constructor path used by daemon run) uses GetStateDir (or the same logic SetSocketPath uses) to derive pidFile and cacheDir instead of DefaultPIDPath/DefaultCachePath, or update SetSocketPath to also set pidFile and cacheDir when it applies the HOOKWISE_STATE_DIR override.
♻️ Duplicate comments (2)
internal/feeds/daemon.go (2)
305-340:⚠️ Potential issue | 🔴 CriticalDon't SIGTERM a PID-only migration target you can't identify.
A stale numeric PID can be reused by an unrelated process. Here we signal that PID without any identity check, and the timeout path at Lines 338-340 deletes the PID file and continues startup even if the process never exited. That can kill the wrong process or leave two daemons running side by side.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/feeds/daemon.go` around lines 305 - 340, The code currently sends SIGTERM to a numeric PID without verifying the process identity, risking signaling an unrelated reused PID; update the startup logic around isProcessAliveByPID, os.FindProcess, proc.Signal and d.pidFile to verify the target process before signaling (for example check process executable path, command line, or a stored start timestamp/unique marker referenced in the PID file) and only send SIGTERM when the identity matches; if verification fails, log a clear warning and remove the stale PID file without signaling, and ensure the timeout-path does not proceed to remove d.pidFile or start another daemon while a verified-but-stubborn old daemon still exists.
265-276:⚠️ Potential issue | 🟠 MajorAdd panic boundaries to the idle and signal goroutines.
Both goroutines call
triggerShutdown(), but neither has a top-levelrecover(), so an unexpected panic there still takes down the daemon.🛡️ Minimal change
go func() { + defer func() { + if r := recover(); r != nil { + core.Logger().Error("feeds: idle monitor panic recovered", "recovered", fmt.Sprintf("%v", r)) + } + }() defer d.wg.Done() select { @@ go func() { + defer func() { + if r := recover(); r != nil { + core.Logger().Error("feeds: signal handler panic recovered", "recovered", fmt.Sprintf("%v", r)) + } + }() defer d.wg.Done() select {As per coding guidelines,
internal/**/*.go: Side effects must be non-blocking with per-goroutine recover() (ARCH-7).Also applies to: 348-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/feeds/daemon.go` around lines 265 - 276, The idle goroutine (the anonymous function started after d.wg.Add(1) that selects on d.idleTimer.C and d.stopCh) needs a panic boundary: add a top-level deferred recover() inside that goroutine which logs the recovered panic via core.Logger().Error (including context like "feeds: idle goroutine panic") and prevents the panic from propagating, then continue to call d.triggerShutdown() as before; apply the same pattern to the other signal goroutine (the one around the select at the 348-359 region) so both goroutines catch panics, log them, and do not crash the daemon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/feeds/client.go`:
- Around line 90-110: The child daemon is being spawned with the caller's
working directory, so when a configPath is provided spawnDaemon should set the
child's working directory to the config file's directory; update the spawnDaemon
function to compute the directory from the configPath (e.g., using
filepath.Dir(configPath) after filepath.Abs) and assign it to the exec.Cmd's Dir
field (cmd.Dir) before starting the process so the daemon inherits the config's
directory and any cwd-dependent feeds (like project) operate correctly.
---
Outside diff comments:
In `@cmd/hookwise/main.go`:
- Around line 408-416: The daemon/socket and feed cache are currently global
because code uses core.GetStateDir() (in the lines calling
ensureDaemonWithCache(configPath) and bridge.CollectFeedCache(cacheDir)), which
causes repo-scoped feeds (like the project feed derived from projectDir/project
config) to leak between repositories; fix by namespacing the daemon and cache by
project root when working with project-scoped feeds: compute a project-specific
state directory (e.g., append a stable project identifier derived from
projectDir or core.ProjectConfigFile — e.g., a short hash of projectDir) and use
that for ensureDaemonWithCache and for cacheDir passed to
bridge.CollectFeedCache, or alternatively detect and exclude project-scoped
feeds from the shared daemon and load them directly from projectDir; update
references to core.GetStateDir(), ensureDaemonWithCache(configPath),
bridge.CollectFeedCache(cacheDir), and feedCache to use the project-scoped state
dir or the direct-per-repo loading path.
In `@internal/feeds/daemon.go`:
- Around line 49-57: NewDaemon currently initializes pidFile and cacheDir from
DefaultPIDPath and DefaultCachePath at construction, so when SetSocketPath is
later called (e.g., because HOOKWISE_STATE_DIR is set) only socketPath is
updated and pidFile/cacheDir still point to the original defaults; update
NewDaemon to compute pidFile and cacheDir lazily or call the same state-dir
resolution used by SetSocketPath so they follow HOOKWISE_STATE_DIR—specifically
ensure NewDaemon (or the constructor path used by daemon run) uses GetStateDir
(or the same logic SetSocketPath uses) to derive pidFile and cacheDir instead of
DefaultPIDPath/DefaultCachePath, or update SetSocketPath to also set pidFile and
cacheDir when it applies the HOOKWISE_STATE_DIR override.
---
Duplicate comments:
In `@internal/feeds/daemon.go`:
- Around line 305-340: The code currently sends SIGTERM to a numeric PID without
verifying the process identity, risking signaling an unrelated reused PID;
update the startup logic around isProcessAliveByPID, os.FindProcess, proc.Signal
and d.pidFile to verify the target process before signaling (for example check
process executable path, command line, or a stored start timestamp/unique marker
referenced in the PID file) and only send SIGTERM when the identity matches; if
verification fails, log a clear warning and remove the stale PID file without
signaling, and ensure the timeout-path does not proceed to remove d.pidFile or
start another daemon while a verified-but-stubborn old daemon still exists.
- Around line 265-276: The idle goroutine (the anonymous function started after
d.wg.Add(1) that selects on d.idleTimer.C and d.stopCh) needs a panic boundary:
add a top-level deferred recover() inside that goroutine which logs the
recovered panic via core.Logger().Error (including context like "feeds: idle
goroutine panic") and prevents the panic from propagating, then continue to call
d.triggerShutdown() as before; apply the same pattern to the other signal
goroutine (the one around the select at the 348-359 region) so both goroutines
catch panics, log them, and do not crash the daemon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7f81415-8e8c-4ad6-b0c6-708c2e9ae9c8
📒 Files selected for processing (8)
cmd/hookwise/cli_test.gocmd/hookwise/main.gointernal/feeds/client.gointernal/feeds/client_test.gointernal/feeds/daemon.gointernal/feeds/feeds_test.gointernal/feeds/polling.gointernal/feeds/socket.go
Comment on lines
+90
to
+110
| func (c *DaemonClient) spawnDaemon(configPath string) error { | ||
| // Ensure state directory exists (for daemon log and socket). | ||
| if err := core.EnsureDir(core.DefaultStateDir, core.DefaultDirMode); err != nil { | ||
| return fmt.Errorf("ensure state dir: %w", err) | ||
| } | ||
|
|
||
| // Find our own binary for re-exec. | ||
| self, err := os.Executable() | ||
| if err != nil { | ||
| return fmt.Errorf("find executable: %w", err) | ||
| } | ||
|
|
||
| args := []string{"daemon", "run"} | ||
| if configPath != "" { | ||
| args = append(args, "--config", configPath) | ||
| } | ||
| if c.socketPath != core.DefaultSocketPath { | ||
| args = append(args, "--socket", c.socketPath) | ||
| } | ||
|
|
||
| cmd := exec.Command(self, args...) |
There was a problem hiding this comment.
Start the child daemon in the config's directory.
Only --config is propagated today; the child still inherits the caller's cwd. status-line --project-dir /repo can therefore spawn the daemon in the wrong checkout, and cwd-dependent feeds like project will poll the wrong repo.
💡 Minimal fix
import (
"context"
"encoding/json"
"fmt"
"math/rand"
"net"
"net/http"
"os"
"os/exec"
+ "path/filepath"
"syscall"
"time"
@@
cmd := exec.Command(self, args...)
+ if configPath != "" {
+ cmd.Dir = filepath.Dir(configPath)
+ }
cmd.SysProcAttr = &syscall.SysProcAttr{Setsid: true} // SPAWN-2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/client.go` around lines 90 - 110, The child daemon is being
spawned with the caller's working directory, so when a configPath is provided
spawnDaemon should set the child's working directory to the config file's
directory; update the spawnDaemon function to compute the directory from the
configPath (e.g., using filepath.Dir(configPath) after filepath.Abs) and assign
it to the exec.Cmd's Dir field (cmd.Dir) before starting the process so the
daemon inherits the config's directory and any cwd-dependent feeds (like
project) operate correctly.
Owner
Author
There was a problem hiding this comment.
Good catch. Currently the daemon is only spawned from status-line, which always runs in the project directory context. The --project-dir flag sets the working directory before config resolution, so the inherited cwd is correct in practice. However, explicitly setting cmd.Dir would be more defensive — tracking this as a follow-up hardening item.
There was a problem hiding this comment.
@vishnujayvel, thanks for the context — that makes sense. Since status-line always inherits the correct project cwd today, the current behavior is safe within that usage constraint. Explicitly setting cmd.Dir would still be a good defensive hardening step, especially as new callers of spawnDaemon are added in the future (e.g., if daemon start is ever invoked from an arbitrary directory).
Would you like me to open a GitHub follow-up issue to track the cmd.Dir hardening item so it doesn't get lost?
✏️ Learnings added
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 67
File: internal/feeds/client.go:90-110
Timestamp: 2026-03-10T00:44:14.189Z
Learning: In `internal/feeds/client.go` (vishnujayvel/hookwise PR `#67`), `spawnDaemon()` does not explicitly set `cmd.Dir` on the spawned child process. This is intentional for now: the daemon is only spawned from `status-line`, which always runs in the correct project directory context (the `--project-dir` flag sets cwd before config resolution). Explicitly setting `cmd.Dir = filepath.Dir(configPath)` is deferred as a follow-up hardening item for when new callers of `spawnDaemon` are added.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 67
File: internal/feeds/daemon.go:279-341
Timestamp: 2026-03-10T00:43:30.109Z
Learning: In `internal/feeds/daemon.go` (vishnujayvel/hookwise), `migrateOldDaemon()` sends SIGTERM to the PID from the PID file without verifying process identity. This is a known, accepted risk: the window for PID reuse requires the old daemon to have crashed, the PID to be recycled by an unrelated process, and no socket file present — treated as a narrow edge case with non-destructive consequence (SIGTERM). This is a temporary migration path from PID-only to socket-based daemon management; full PID file removal and a safer migration approach are deferred to Phase 2.
Replace PID-file-only daemon lifecycle with a Unix domain socket connect-or-start pattern. CLI commands transparently start the daemon if not running, using socket bind as single-instance authority. - Add SocketServer with /health and /shutdown HTTP endpoints - Rewrite Daemon lifecycle: socket bind, idle timeout, migration - Add DaemonClient with connect-or-start, jitter, and readiness polling - Add daemon start/stop/run CLI subcommands - Wire auto-start into status-line with 60s marker file caching - Update doctor to use socket-based liveness check - Add IsEnvelopeFresh TTL checking for feed cache envelopes - 39 new tests across socket, client, daemon, and CLI packages Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centralize shutdown via sync.Once to prevent double-close panics, add panic recovery to socket goroutines (ARCH-7), surface shutdown errors in client, compute socket paths from GetStateDir() at call time, and convert CLI tests to testify assert/require. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
vishnujayvel
force-pushed
the
feat/daemon-connect-or-start
branch
from
327c33c to
8bc9b64
Compare
7 tasks
vishnujayvel
added a commit
that referenced
this pull request
Jul 8, 2026
The Nightly Validate workflow (dagger call validate — chaos + mutation +
snapshots) has been red every day for weeks. Per-PR CI only runs Tier 0/1
(dagger call test), which excludes the integration/chaos tier, so these
failures never gated a PR and accumulated silently.
Three stacked, test-only root causes in the daemon tests:
1. Socket not isolated. DefaultSocketPath is frozen at package init from
~/.hookwise, so the tests' t.Setenv("HOOKWISE_STATE_DIR", ...) never
moves it. Since #67 made socket-bind the single-instance authority, the
three daemon tests (integration x2 + chaos x1) all bound the shared real
~/.hookwise/daemon.sock and collided — with each other under parallel
`go test ./...`, or with a live local daemon. Fix: SetSocketPath into a
short /tmp dir (matching every feeds/bridge daemon test), short enough to
stay under the macOS 104-byte unix-socket limit.
2. Weather defaults to disabled. FeedEnabled("weather") reads
cfg.Weather.Enabled; the lifecycle test passed an empty FeedsConfig{}, so
the daemon skipped the weather producer and weather.json was never
written, failing the "both cache files" wait. Fix: enable weather in the
test config (as a real user must).
3. ttl_seconds type mismatch. After the JSON cache round-trip ttl_seconds is
a float64; the assertion compared it against the untyped-int
DefaultTTLSeconds. Value is correct (60s interval -> 180 -> floored 300);
fix: cast to float64, matching the sibling assertion.
No production code touched; changes are behind //go:build integration.
Test evidence: full integration + chaos suites green under
GOMEMLIMIT=4GiB go test -race -p 2 -tags integration, including run together
(the parallel-daemon collision scenario), with a real daemon holding the
default socket.
Claude-Session: https://claude.ai/code/session_01EjWqH4FMkdczkSagzXw3dQ
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
New Files
internal/feeds/socket.go— Unix domain socket HTTP server with/healthand/shutdownendpointsinternal/feeds/socket_test.go— 7 socket server tests (bind, health, shutdown, stale cleanup, permissions, double-start)internal/feeds/client.go— DaemonClient with connect-or-start pattern, spawn jitter, readiness pollinginternal/feeds/client_test.go— 10 client tests (IsRunning, Health, Shutdown, EnsureDaemon, timeouts)Modified Files
internal/core/constants.go— AddedDefaultSocketPathconstantinternal/feeds/daemon.go— Socket-based lifecycle (replaces PID-only), idle timeout, old daemon migration,StopCh()methodinternal/feeds/polling.go— Idle timer reset on producer poll completioninternal/feeds/feeds_test.go— 9 new daemon tests (socket start, double-start, shutdown, idle timeout, migration, concurrent start, socket release)cmd/hookwise/main.go— Daemon CLI commands (start/stop/run), status-line auto-start with 60s marker cache, socket-based doctor checkcmd/hookwise/cli_test.go— 8 new CLI tests (daemon subcommands, doctor socket check, marker file caching)internal/bridge/bridge.go— AddedIsEnvelopeFresh()TTL freshness check for feed cache envelopesinternal/bridge/bridge_test.go— TTL freshness testsArchitecture Constraints
Setsid: truedetaches daemon from parent--configflagTest plan
-raceflagHOOKWISE_STATE_DIRenv override for socket path🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests