test(config): wait for background PID file

[anandh8x]

Summary

• wait briefly for a background provider command PID file to become readable before checking process termination
• preserve the strict child-liveness assertion once the PID is available
• add a deterministic regression covering delayed PID-file visibility

Why

Windows Smoke can start and terminate the provider command correctly while the PID file is not yet readable when the test assertion runs. #800 handled the basic timeout fixture, but the strict background-child fixtures still performed an immediate read and remained flaky.

Validation

• make fmt-check
• go vet ./...
• go test ./...
• go test ./internal/config -run TestAssertProcessTerminatedWaitsForDelayedPIDFile -count=100
• Windows cross-compilation of ./internal/config tests
• go run ./cmd/zero-release build
• go run ./cmd/zero-release smoke
• govulncheck ./... (no vulnerabilities found)
• git diff HEAD --check
• pinned advisory golangci-lint (pre-existing repository findings only; none in the changed file)

Summary by CodeRabbit

• Bug Fixes

  • Improved process termination checks to correctly handle PID files that are initially missing and appear shortly after monitoring begins.
  • Added a short wait window for PID file availability, reducing false failures during delayed shutdown.

• Tests

  • Added coverage for the delayed PID-file scenario to ensure the new waiting behavior is exercised.

[anandh8x]

@kevincodex1 This is the focused follow-up for the remaining Windows background-child PID-file race exposed after #800 merged. Could you please review and merge it when the checks are green? #801 can then be updated onto main without carrying this unrelated test fix.

[coderabbitai]

Walkthrough

The process-termination test helper now polls for a PID file for up to three seconds before checking process liveness. A new test covers PID files created shortly after assertion begins.

Changes

PID File Termination Assertions

Layer / File(s)	Summary
Poll delayed PID files internal/config/command_test.go	assertProcessTerminatedAfterMissing retries PID-file reads and invokes an optional callback when the file is initially missing. A new test validates delayed PID-file creation before termination checking.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• Gitlawb/zero#690: Updates related PID-file termination tests and polling behavior.
• Gitlawb/zero#800: Modifies related PID-file termination assertion helpers and timing logic.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% 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
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly reflects the main change: adding a wait for background PID file availability before process termination checks.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/windows-provider-background-pid-race

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🤖 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/config/command_test.go`:
- Around line 154-162: Update TestAssertProcessTerminatedWaitsForDelayedPIDFile
and the associated assertProcessTerminated polling path to add a test-only hook
or barrier that signals after the first missing-file poll, then releases the
PID-file writer. Track and assert that this delayed-file path was exercised,
removing reliance on the fixed 50ms sleep.
- Around line 157-160: Update the PID-file setup in the goroutine around
assertProcessTerminated to publish complete contents atomically, using a
temporary file followed by a rename, or retry reads until the parsed PID is
stable before checking liveness. Ensure the test never evaluates an empty or
partially written PID value.

🪄 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 Plus

Run ID: 692064f4-c866-4a5e-b205-01cbdab5a91c

📥 Commits

Reviewing files that changed from the base of the PR and between 9d5c82d and ad0d513.

📒 Files selected for processing (1)

• internal/config/command_test.go

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: ef745b359779
Changed files (1): internal/config/command_test.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[anandh8x]

@coderabbitai review

Addressed both requested changes in ef745b3:

• replaced the fixed-delay regression with a deterministic first-missing-file barrier and asserted that path was exercised
• published the test PID file atomically via a temporary file plus rename, preventing partial reads

Focused stress test passes 100/100, the full suite passes with required host permissions, and Windows test cross-compilation passes.

[coderabbitai]

@anandh8x: Thanks—I'll review the updated PID-file race coverage and the atomic publication change.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[anandh8x]

@kevincodex1 Follow-up: both CodeRabbit findings are addressed, CodeRabbit has approved the latest commit, and the full CI matrix—including Windows Smoke—is green. This is ready for maintainer review/merge.

[gnanam1990]

Summary

Test-only change in internal/config/command_test.go. It refactors assertProcessTerminated to poll for the background child's PID file (up to a 3s deadline) rather than doing a single os.ReadFile, and extracts assertProcessTerminatedAfterMissing(pidFile, onMissing) so a new test can hook the missing-file path. This fixes a real ordering bug: if the child hadn't yet written its PID file when the assertion ran, the old code failed immediately with "read sleeper pid file". PR-introduced (base read the file exactly once — confirmed via the diff against abase).

Correctness

• The stated bug is actually addressed: the read is now retried on os.ErrNotExist until the deadline, with non-NotExist errors still failing fast (command_test.go:207-215). This is the fix the title claims, not just a description of it.
• The new TestAssertProcessTerminatedWaitsForDelayedPIDFile genuinely exercises the delayed-write path rather than racing past it: the writer goroutine blocks on firstMissing, which is only closed from the onMissing callback fired the first time the file is observed missing, and the test asserts missingObserved so a pre-existing file would fail loudly (command_test.go:154-178). Write is atomic (temp + os.Rename), so no torn read.
• Fake PID 2147483647 is not a live process, so the liveness loop returns promptly — verified: the test completes in seconds, not at the 3s timeout.

Tests

• gofmt clean, go vet clean, build ok.
• go test -race passes for the new test and the existing background-child termination test, including repeated runs with no flakes. Re-ran here: ok internal/config 4.318s. No PR-attributable failures; nothing env/pre-existing flagged for this package.

Nice touch synchronizing via the onMissing hook instead of a sleep — it keeps the timing test deterministic.

Merge is kevin's call per the program gate.

[Vasanthdev2004]

Approving. This fixes the flake rather than papering over it, which was my main worry with a "wait for it" change.

Checked the important properties: the wait is bounded (3s deadline), it fails the test on timeout rather than passing or skipping (t.Fatalf("read sleeper pid file before timeout")), a missing or unreadable PID file is never treated as success, and the strict liveness assertion afterwards is byte-identical to before, so the test can still fail when the child really is alive. That is the part that mattered.

Four small things, none blocking:

1. The loop breaks on the first os.ReadFile that returns no error without checking the content parses, so an empty or half-written PID file falls straight through to t.Fatalf("parse sleeper pid") instead of being retried. That is the exact race the wait exists for, just one step later. Worth moving the parse inside the loop and only breaking once it succeeds.
2. Only os.ErrNotExist is retried; any other transient read error fatals immediately with the deadline unspent. On Windows a sharing-violation style error is plausible there.
3. The new regression test asserts the wait happens but never asserts it is bounded, so deleting the deadline entirely would still leave it green. A case that drives the helper past its deadline would pin the property you actually added.
4. The 3s budget appears as two bare literals (the PID wait and the liveness poll) that now compose into one ~6s window against a 5s provider-command timeout plus a 1s WaitDelay. A named constant would stop them drifting apart.

Note on verification: Windows Smart App Control on my machine is currently blocking the unsigned go toolchain, so I could not run the suite locally for this review and read the code instead. CI is green across all three OSes, which covers the behaviour here.

LGTM.