fix: add Windows daemon support by maxplangg · Pull Request #163 · compozy/compozy

maxplangg · 2026-05-25T12:06:17Z

Fixes #148
Fixes #157
Fixes #170

Problem

Three independent failures prevented the daemon from running on Windows — all causing a silent exit with no error shown to the user:

Lock file sharing violation (daemon start fails on Windows: "another process has locked a portion of the file" with no other process holding the lock #148) — lock uses LockFileEx on Windows, which holds a byte-range lock that blocks all file handles, including those in the same process. Writing the PID back to the locked file returned ERROR_SHARING_VIOLATION.
Silent crash on startup (daemon start fails on Windows: "Error: wait for daemon readiness: read daemon info: daemon: read daemon info" #157) — syncDir called ile.Sync() on a directory opened read-only. On Windows, FlushFileBuffers requires a writable handle; it returns ERROR_ACCESS_DENIED, causing the daemon to exit before writing daemon.json.
SQLite URI rejected (daemon start fails on Windows: "Error: wait for daemon readiness: read daemon info: daemon: read daemon info" #157) — ilepath.ToSlash produces C:/path/to/db on Windows. Passed to url.URL, this serialises as ile://C:/... where the SQLite URI parser interprets C: as a network authority rather than a drive letter, failing to open the database.
Daemon inherited Ctrl+C — The daemon process inherited the parent console's process group, so any Ctrl+C sent to the CLI also terminated the daemon.

Changes

File	Change
internal/daemon/sync_dir_unix.go	Extracted existing syncDir implementation (non-Windows build)
internal/daemon/sync_dir_windows.go	No-op syncDir on Windows — NTFS journal guarantees durability after atomic rename
internal/daemon/lock_pid_other.go	lockPIDPath identity function for non-Windows (PID stored in lock file)
internal/daemon/lock_pid_windows.go	lockPIDPath returns path + .pid sidecar to avoid LockFileEx conflict
internal/daemon/lock.go	Use lockPIDPath() helper for all PID read/write calls
internal/daemon/info_lock_test.go	Update tests to use lockPIDPath() to reflect platform-correct path
internal/daemon/info.go	Remove syncDir (moved to platform-specific files)
internal/cli/daemon_launch_windows.go	SysProcAttr with CREATE_NEW_PROCESS_GROUP to detach daemon from parent console
internal/cli/daemon_launch_other.go	Add windows to the exclusion build constraint
internal/store/sqlite.go	Prepend / to drive-letter paths so url.URL produces valid ile:///C:/... URIs

Impact on Unix / Linux / macOS

No behavioral change. All Windows-specific code is behind //go:build windows build tags. The lockPIDPath non-Windows implementation is an identity function (returns the same path). The SQLite fix only triggers when the path does not start with /, which never happens on Unix. The syncDir move is a pure refactor with identical logic.

Verification

make verify

golangci-lint: 0 issues
Go race suite: all tests pass
Build: compiles cleanly for Windows target

Summary by CodeRabbit

Chores
- Improved cross-platform daemon launching and process detachment (Windows/unix behaviors)
- Refined lock/PID file handling for more reliable PID tracking and release
- Enhanced directory sync/durability handling per platform
- Normalized database file path handling for consistent connection URIs
Tests
- Expanded and hardened test coverage for daemon lifecycle, locking, SQLite behavior, and store utilities

- Add platform-specific daemon launcher using CREATE_NEW_PROCESS_GROUP to isolate the daemon from parent console Ctrl+C events - Split syncDir into unix/windows builds: no-op on Windows since FlushFileBuffers requires write access on directory handles - Introduce lockPIDPath helper with a .pid sidecar on Windows to avoid LockFileEx byte-range lock conflict when writing the PID - Fix SQLite DSN for Windows drive letters (ensure triple-slash URI form)

coderabbitai · 2026-05-25T12:06:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 150990c1-cd62-4cf5-85f8-612b20afb6d8

📥 Commits

Reviewing files that changed from the base of the PR and between a9070ab and 0de41f7.

📒 Files selected for processing (2)

internal/store/sqlite_windows_test.go
internal/store/values_test.go

Walkthrough

Platform-aware changes: Windows-specific daemon launch isolation and lock-PID sidecar handling, platform syncDir extraction, SQLite DSN path normalization, and expanded store tests (cross-platform and Windows-only cases).

Changes

Platform-specific daemon startup and locking

Layer / File(s)	Summary
Daemon process launch isolation `internal/cli/daemon_launch_windows.go`, `internal/cli/daemon_launch_other.go`	Windows now detaches the spawned daemon from the parent console process group using `CREATE_NEW_PROCESS_GROUP`; non-Windows build constraint updated.
Lock file PID storage abstraction `internal/daemon/lock_pid_windows.go`, `internal/daemon/lock_pid_other.go`, `internal/daemon/lock.go`, `internal/daemon/info_lock_test.go`	Introduces `lockPIDPath()` and updates lock acquisition/release and tests to read/write PID via the computed path. Windows uses a `.pid` sidecar to avoid LockFileEx byte-range conflicts; Unix persists PID at the lock path.
Directory sync abstraction `internal/daemon/sync_dir_windows.go`, `internal/daemon/sync_dir_unix.go`, `internal/daemon/info.go`	`syncDir()` moved into platform-specific files: Unix opens and syncs the directory handle; Windows is a documented no-op. `syncDir` removed from `info.go`.
Boot tests adjusted for lock PID path `internal/daemon/boot_test.go`	Boot/start tests updated to read/write the PID marker via `lockPIDPath(paths.LockPath)` in setups and assertions.
SQLite DSN path normalization and tests `internal/store/sqlite.go`, `internal/store/sqlite_test.go`, `internal/store/sqlite_windows_test.go`	`sqliteDSN()` now normalizes slashes and ensures a leading slash for absolute paths so `file:///...` URIs are constructed correctly; extensive SQLite tests added/expanded, including a Windows drive-letter DSN test.
Store value helper tests `internal/store/values_test.go`	Adds table-driven unit tests for timestamp formatting/parsing, nullable string helpers, and ID generation functions.

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.70% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main objective of the PR: adding Windows daemon support to fix critical platform-specific failures.
Linked Issues check	✅ Passed	The code changes comprehensively address all four root causes from issues `#148`, `#157`, and `#170`: lock file PID sidecar on Windows, syncDir as no-op on Windows, SQLite URI path normalization, and daemon process group detachment.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the four identified Windows daemon failures; comprehensive test coverage validates each fix without introducing unrelated functionality.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

pedronauck

@maxplangg

Findings

[P2] SQLite Database Path Handling

Issue: Relative SQLite database paths now resolve to filesystem-root paths.
- In internal/store/sqlite.go:99, a leading / is unconditionally prepended if the path does not start with /. This correctly addresses Windows drive-letter paths, but it also incorrectly modifies valid relative paths on Unix-like systems—converting them into absolute file:/// URIs, which point to the root instead of remaining relative.
- OpenSQLiteDatabase does not require absolute paths. Both globaldb.Open and rundb.Open accept and use caller-provided paths as-is, so this is a behavioral regression for any consumer using relative database paths.
- Recommendation: The path normalization should be more narrowly targeted. Only add a leading slash when handling Windows drive-letter paths, or convert the input path to an absolute path explicitly/document this contract.
- Action item: Add regression tests covering both relative.db and a Windows drive-letter path to ensure both scenarios are correctly handled.

[P2] Windows PID Sidecar Path Usage in Tests

Issue: The new Windows PID sidecar path is not used in current daemon startup lifecycle tests. As a result, Windows test runs could fail or miss the stale-PID scenario this PR intends to address.
- The PR introduces lockPIDPath for consistent PID file access; on Windows, internal/daemon/lock_pid_windows.go:10 returns daemon.lock.pid.
- However, existing lifecycle tests (e.g. internal/daemon/boot_test.go:33, boot_test.go:74, boot_test.go:339, boot_test.go:372) still assert or manipulate paths.LockPath directly.
- On Windows, Start will use .pid, while tests interact with daemon.lock, so Windows behavior isn't properly validated and could fail if executed.
- Recommendation: Update all relevant tests to use lockPIDPath(paths.LockPath) when checking the "persisted daemon PID" invariant.
- Action item: Add a Windows CI/test leg if the PR is intended to support and close Windows daemon-side support.

Merge Recommendation

Not ready to merge. The overall direction is reasonable, and Windows compile checks are passing, but:

The SQLite URI change is overly broad and regresses relative-path behavior.
Windows lifecycle test coverage for the new PID path is incomplete for the claimed feature.

Address these issues before merging.

Extends coverage for sqliteDSN (Windows drive-letter path handling), ShouldRecoverSQLite, OpenSQLiteDatabase, Checkpoint, recoverSQLiteDatabase, EnsureSchema, values helpers (FormatTimestamp, ParseTimestamp, NullableString, NullString, NewID) and CloseSQLiteDatabase edge cases; updates boot_test.go to use lockPIDPath helper introduced by the Windows daemon fix.

coderabbitai

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/store/sqlite_test.go (1)

87-440: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Use t.Run("Should ...") naming consistently for subtests.

Multiple new subtests use non-compliant names. Please rename them to the required Should ... pattern for consistency with test standards.

Suggested pattern

- t.Run("rejects empty path", func(t *testing.T) {
+ t.Run("Should reject empty path", func(t *testing.T) {

- t.Run("opens and initializes a fresh database", func(t *testing.T) {
+ t.Run("Should open and initialize a fresh database", func(t *testing.T) {

As per coding guidelines, "**/*_test.go: MUST use t.Run("Should...") pattern for ALL test cases".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/sqlite_test.go` around lines 87 - 440, Rename all subtest
t.Run names in this file to follow the "Should ..." pattern (e.g., change
"rejects empty path", "opens and initializes a fresh database", "opens without
initialize callback", "reopening an existing database succeeds", "recovers from
corrupt database file and returns fresh database", "returns error when
initialize callback fails", "nil db is a no-op", "checkpoints a real WAL
database", "nil db returns nil without panicking", "nil context returns error",
"nil db does not panic", "non-nil db gets closed", "renames main file and
existing companions", "succeeds when companion files are absent", "returns error
for invalid SQL statement") to start with "Should ..." so they comply with the
required naming convention; locate each t.Run call inside TestOpenSQLiteDatabase
(top-level tests), TestCheckpoint, TestCloseSQLiteDatabaseNilCases,
TestCloseQuietly, TestRecoverSQLiteDatabase, TestEnsureSchema and
TestCloseSQLiteDatabase and update the string literals to the "Should ..."
phrasing while keeping the rest of the description intact.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/store/sqlite_windows_test.go`:
- Around line 10-20: The test TestSQLiteDSNWindowsDriveLetter performs
assertions at the top level—wrap its assertions inside a t.Run subtest using the
"Should ..." naming convention (e.g., t.Run("Should convert Windows drive-letter
path to file URI", func(t *testing.T) { ... })), keeping the existing body
(path, dsn, wantPrefix and the strings.HasPrefix check) and preserving
t.Parallel() as appropriate; update the function so the call to sqliteDSN and
the t.Errorf assertion live inside that t.Run closure.

In `@internal/store/values_test.go`:
- Around line 36-37: Rename all subtest names in internal/store/values_test.go
to follow the required "Should..." pattern: update each t.Run(...) invocation
(including the ones around the table-driven cases and the specific occurrences
currently at the blocks starting with t.Run at lines shown in the review) so the
name string begins with "Should" and describes the expected behavior, e.g.,
change "valid case" to "Should handle valid case"; keep existing table-driven
tt.name values in sync or replace them with explicit "Should..." strings used in
t.Run calls for consistency across test functions like the ones hosting t.Run in
the values_test.go file.
- Around line 81-83: The test currently only checks presence of an error for
ParseTimestamp; update the test case table and assertions to verify specific
error content/type per invalid input by adding fields like wantErrContains
(string) and/or wantErrType (error or target type) to each tt entry, then
replace the generic (err != nil) check with targeted assertions using t.Fatalf
or testing/require helpers: use errors.As or errors.Is when checking types
(wantErrType) and check substring with strings.Contains(err.Error(),
wantErrContains) (or require.ErrorContains) for message content; apply this
change around the ParseTimestamp error check in values_test.go so each invalid
case asserts the expected error detail.

---

Outside diff comments:
In `@internal/store/sqlite_test.go`:
- Around line 87-440: Rename all subtest t.Run names in this file to follow the
"Should ..." pattern (e.g., change "rejects empty path", "opens and initializes
a fresh database", "opens without initialize callback", "reopening an existing
database succeeds", "recovers from corrupt database file and returns fresh
database", "returns error when initialize callback fails", "nil db is a no-op",
"checkpoints a real WAL database", "nil db returns nil without panicking", "nil
context returns error", "nil db does not panic", "non-nil db gets closed",
"renames main file and existing companions", "succeeds when companion files are
absent", "returns error for invalid SQL statement") to start with "Should ..."
so they comply with the required naming convention; locate each t.Run call
inside TestOpenSQLiteDatabase (top-level tests), TestCheckpoint,
TestCloseSQLiteDatabaseNilCases, TestCloseQuietly, TestRecoverSQLiteDatabase,
TestEnsureSchema and TestCloseSQLiteDatabase and update the string literals to
the "Should ..." phrasing while keeping the rest of the description intact.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 61fc9713-3626-4e3f-98b5-ad1b9047f397

📥 Commits

Reviewing files that changed from the base of the PR and between 306ae3a and a9070ab.

📒 Files selected for processing (5)

internal/daemon/boot_test.go
internal/store/sqlite.go
internal/store/sqlite_test.go
internal/store/sqlite_windows_test.go
internal/store/values_test.go

✅ Files skipped from review due to trivial changes (1)

internal/daemon/boot_test.go

…s assertion - Wrap TestSQLiteDSNWindowsDriveLetter body in t.Run("Should ...") subtest - Prefix all table-driven t.Run names with "Should " across values_test.go - Rename standalone NewID subtests to start with "Should" - Add errContains field to ParseTimestamp error cases to detect regressions

…274) * fix: restore Windows daemon support (PR #163 regression + new fixes) The v0.3 refactor to CompozyOS lost the Windows platform-specific files merged in PR #163 (fix: add Windows daemon support). Reproduces the original issues on Windows 11 with the v0.3.0-beta.2 release binary: Original PR #163 regressions re-fixed: - #148 daemon start fails: "another process has locked a portion of the file" (LockFileEx byte-range lock prevents writeLockPID) - #170 same, root-caused as LockFileEx conflict with in-process write - #157 daemon start fails: "invalid uri authority: C:" (sqliteDSN passes drive-letter path to url.URL which serialises as file://C:/... instead of file:///C:/...) - v0.3 also removed the sync_dir platform split — syncDir on info.go now fails on Windows because FlushFileBuffers requires a writable handle Additional Windows-only bugs discovered during validation: - CREATE_NEW_PROCESS_GROUP was missing from detached_windows.go spawnDetachedLoggedProcess. Unix uses Setpgid: true (detached_unix.go:58) but the Windows sibling forgot the equivalent, so the daemon inherits the parent CLI's console group and dies on parent exit. - procutil.StartedAt crashed with ACCESS_VIOLATION (0xc0000005) inside GetProcessTimes when passed nil pointers for exit/kernel/user times on some Windows builds. Pass valid Filetime pointers for all four outputs. Files: - internal/daemon/lock_pid_other.go (NEW): identity lockPIDPath for Unix - internal/daemon/lock_pid_windows.go (NEW): .pid sidecar for Windows - internal/daemon/sync_dir_unix.go (NEW): syncDir extracted from info.go - internal/daemon/sync_dir_windows.go (NEW): syncDir no-op for Windows - internal/daemon/lock.go: use lockPIDPath() at 3 call sites - internal/daemon/info.go: remove syncDir (moved to platform files) - internal/procutil/detached_windows.go: set CREATE_NEW_PROCESS_GROUP - internal/procutil/process_started_at_windows.go: pass valid Filetime pointers to GetProcessTimes, guard against invalid handles - internal/store/sqlite_config.go: add sqliteFilePath helper that prefixes drive-letter absolute paths with '/' so the URI serialises correctly - internal/store/sessiondb/read_only.go: same drive-letter normalisation inline (package boundary) Zero behaviour change on Unix / Linux / macOS — every fix is behind a //go:build windows tag or an identity function on non-Windows. Validated end-to-end on Windows 11 (build 26200) with go 1.26.5: - compozy daemon start succeeds - compozy daemon status reports "running" with PID + HTTP + UDS - compozy doctor -o json returns diagnostics via the daemon socket * refactor: export SqliteFilePath and reuse in sessiondb/read_only CodeRabbit review on PR #274 flagged the duplicate Windows drive-letter normalization in internal/store/sessiondb/read_only.go — the same logic was inlined that already lives in internal/store/sqlite_config.go under the unexported sqliteFilePath helper. Change: - Rename sqliteFilePath -> SqliteFilePath (exported) so sibling packages can reuse the canonical normalization. - Update internal call sites (sqliteReadOnlyDSN, sqliteDSNWithPragmas) to the new name. - In sessiondb/read_only.go, drop the inlined slash-prefix branch and call store.SqliteFilePath(path) instead. Also removes the now-unused path/filepath import. Behaviour is byte-identical: the exported helper contains exactly the same body as the previous unexported one. Windows drive-letter paths still serialize as file:///C:/... and non-absolute paths remain untouched.

coderabbitai Bot previously approved these changes May 25, 2026

View reviewed changes

pedronauck requested changes May 25, 2026

View reviewed changes

maxplangg dismissed coderabbitai[bot]’s stale review via a9070ab May 26, 2026 11:38

coderabbitai Bot requested changes May 26, 2026

View reviewed changes

Comment thread internal/store/sqlite_windows_test.go

Comment thread internal/store/values_test.go Outdated

Comment thread internal/store/values_test.go

coderabbitai Bot approved these changes May 26, 2026

View reviewed changes

pedronauck merged commit b0a0da4 into compozy:main May 26, 2026
7 checks passed

This was referenced May 26, 2026

release: Release v0.2.6 #171

Merged

release: Release v0.2.6 #174

Closed

lyonzin mentioned this pull request Jul 29, 2026

fix: restore Windows daemon support (PR #163 regression + new fixes) #274

Merged

This was referenced Jul 29, 2026

release: Release v0.3.0 #272

Merged

release: Release v0.3.0 #289

Merged

release: Release v0.3.0 #308

Merged

release: Release v0.3.0 #316

Merged

This was referenced Aug 14, 2026

build: release v0.3.0 #398

Merged

build: release v0.3.0 #402

Merged

build: release v0.3.0 #426

Merged

build: release v0.3.0 #429

Merged

build: release v0.3.0 #443

Merged

This was referenced Aug 21, 2026

build: release v0.3.0 #450

Merged

build: release v0.3.0 #458

Merged

build: release v0.3.0 #496

Merged

This was referenced Sep 1, 2026

build: release v0.3.0 #524

Merged

build: release v0.3.0 #558

Merged

This was referenced Sep 10, 2026

build: release v0.3.0 #584

Merged

build: release v0.3.0 #612

Merged

build: release v0.3.0 #642

Merged

build: release v0.3.0 #650

Merged

This was referenced Sep 27, 2026

build: release v0.3.0 #680

Merged

build: release v0.3.0 #688

Merged

This was referenced Oct 6, 2026

build: release v0.3.0 #700

Merged

build: release v0.3.0 #704

Merged

build: release v0.3.0 #712

Merged

build: release v0.3.0 #732

Open

Navigation Menu

Repository navigation

fix: add Windows daemon support - #163

fix: add Windows daemon support#163
pedronauck merged 3 commits into
compozy:mainfrom
maxplangg:fix/windows-daemon-support

maxplangg commented May 25, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 25, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

pedronauck left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

maxplangg commented May 25, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Changes

Impact on Unix / Linux / macOS

Verification

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

pedronauck left a comment

Choose a reason for hiding this comment

Findings

[P2] SQLite Database Path Handling

[P2] Windows PID Sidecar Path Usage in Tests

Merge Recommendation

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maxplangg commented May 25, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 25, 2026 •

edited

Loading