chore(openspec): complete and archive openspec proposal change#98
chore(openspec): complete and archive openspec proposal change#98
Conversation
- Mark remaining verification tasks as done in add-headless-mode tasks.md - Archive add-headless-mode, filter-chain-state-management, drop-wsi-proxy, and wayland-native-frame-delivery under changes/archive/2026-02-27-* - Promote headless-mode/spec.md to canonical spec; sync deltas into app-window and render-pipeline specs
📝 WalkthroughWalkthroughAdds comprehensive headless-mode support: new CLI flags and validation, surfaceless Vulkan backend and create_headless factory, headless render/readback/export loop with signal-based shutdown, posix_spawn signal-mask attributes, test additions for PNG output, and minor Vulkan-HPP API migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant SigFD as SignalFD
participant Comp as CompositorServer
participant Backend as VulkanBackend (Surfaceless)
participant PNG as PNG Export
App->>SigFD: create signalfd (SIGTERM/SIGINT)
App->>Backend: create_headless(RenderSettings)
Backend->>Backend: Init (no SDL, no swapchain, offscreen image)
loop frame 0..N-1
App->>Comp: get_presented_frame()
Comp-->>App: Presented frame (DMA-BUF fd, meta)
App->>Backend: import DMA-BUF / import semaphore fd
Backend->>Backend: render to offscreen image, wait fence
Backend-->>App: frame rendered
end
App->>PNG: readback offscreen -> staging buffer
PNG->>PNG: write PNG (stb_image_write_png)
par Monitor signals
SigFD->>App: poll/signalfd event
alt signal received
App->>Backend: request clean shutdown
App->>Comp: release surfaces/resources
end
end
App-->>App: exit (status)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/main.cpp`:
- Around line 118-124: Check the return values of posix_spawnattr_init,
posix_spawnattr_setsigmask, and posix_spawnattr_setflags before calling
posix_spawn; if any of these (called on attr) fail, log or propagate the error,
call posix_spawnattr_destroy(&attr) where appropriate, and avoid calling
posix_spawn so you don't spawn the child with an
uninitialized/partially-configured attribute object. Specifically, update the
block that uses posix_spawnattr_init(&attr), posix_spawnattr_setsigmask(&attr,
&empty_mask), and posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGMASK) to
check each rc, handle failures (cleanup attr and return/exit), and only call
posix_spawn(&pid, reaper_path.c_str(), nullptr, &attr, argv.data(), envp.data())
when all prior calls succeeded.
In `@src/render/backend/vulkan_backend.cpp`:
- Around line 1946-1952: vk::ImportSemaphoreFdInfoKHR import_info is
default-constructed without brace/value-initialization which can leave members
uninitialized when VULKAN_HPP_NO_EXCEPTIONS is set; replace the default
construction with brace/value-initialization (e.g., vk::ImportSemaphoreFdInfoKHR
import_info{} ) so all fields are zero-initialized before you set semaphore,
handleType, fd and flags used by m_device.importSemaphoreFdKHR; update the same
pattern wherever vk::ImportSemaphoreFdInfoKHR is constructed.
In `@tests/CMakeLists.txt`:
- Around line 228-231: The test hardcodes a machine-local vkcube path and a
fixed /tmp output file; update the test so it discovers the vkcube executable
and writes the artifact into the build/test output area. Use CMake's
find_program or an overridable cache variable (e.g. set(VKCUBE_EXEC CACHE
FILEPATH ...) / find_program(VKCUBE_EXEC NAMES vkcube)) and replace the literal
"/home/..." with ${VKCUBE_EXEC} in the goggles_headless_integration add_test
invocation; similarly replace "/tmp/goggles_headless_test.png" with a path under
the build tree (e.g. ${CMAKE_CURRENT_BINARY_DIR} or
${CMAKE_BINARY_DIR}/tests/goggles_headless_test.png) or a per-test variable, and
apply the same changes to the other test at lines 243-244 so tests are portable
and avoid artifact collisions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.gitignoreopenspec/changes/archive/2026-02-27-add-headless-mode/.openspec.yamlopenspec/changes/archive/2026-02-27-add-headless-mode/design.mdopenspec/changes/archive/2026-02-27-add-headless-mode/proposal.mdopenspec/changes/archive/2026-02-27-add-headless-mode/specs/app-window/spec.mdopenspec/changes/archive/2026-02-27-add-headless-mode/specs/headless-mode/spec.mdopenspec/changes/archive/2026-02-27-add-headless-mode/specs/render-pipeline/spec.mdopenspec/changes/archive/2026-02-27-add-headless-mode/tasks.mdopenspec/changes/archive/2026-02-27-drop-wsi-proxy-simplify-capture/proposal.mdopenspec/changes/archive/2026-02-27-drop-wsi-proxy-simplify-capture/tasks.mdopenspec/changes/archive/2026-02-27-update-filter-chain-state-management/.openspec.yamlopenspec/changes/archive/2026-02-27-update-filter-chain-state-management/design.mdopenspec/changes/archive/2026-02-27-update-filter-chain-state-management/proposal.mdopenspec/changes/archive/2026-02-27-update-filter-chain-state-management/specs/app-window/spec.mdopenspec/changes/archive/2026-02-27-update-filter-chain-state-management/specs/render-pipeline/spec.mdopenspec/changes/archive/2026-02-27-update-filter-chain-state-management/tasks.mdopenspec/changes/archive/2026-02-27-wayland-native-frame-delivery/proposal.mdopenspec/changes/archive/2026-02-27-wayland-native-frame-delivery/tasks.mdopenspec/specs/app-window/spec.mdopenspec/specs/headless-mode/spec.mdopenspec/specs/render-pipeline/spec.mdsrc/app/application.cppsrc/app/main.cppsrc/render/backend/vulkan_backend.cpptests/CMakeLists.txt
| #### Scenario: First discovery defaults ON for direct Vulkan sessions | ||
| - **GIVEN** a direct Vulkan capture session and a newly discovered active surface | ||
| - **WHEN** the surface appears in Surface List without user override | ||
| - **THEN** its per-surface filter toggle SHALL default to enabled | ||
|
|
There was a problem hiding this comment.
Surface defaulting rule conflicts with current runtime behavior.
This scenario states newly discovered direct-Vulkan surfaces default to enabled, but runtime logic currently initializes new surface filter state to disabled (src/app/application.cpp, Line 515). Please align spec or implementation to a single rule.
| ### Requirement: Surfaceless VulkanBackend Factory | ||
| `VulkanBackend` SHALL provide a `create_headless(RenderSettings) -> ResultPtr<VulkanBackend>` static factory that creates a Vulkan instance, selects a physical device, and creates a logical device and queue without requiring a `vk::SurfaceKHR`. This factory SHALL NOT create a swapchain, present semaphores, or frame-pacing resources. | ||
|
|
There was a problem hiding this comment.
Headless factory signature in spec is out of sync with the implemented API.
The spec requires create_headless(RenderSettings), but the current implementation takes additional parameters (enable_validation, shader_dir, cache_dir, settings). Please reconcile the canonical signature.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/main.cpp`:
- Around line 118-129: posix_spawnattr_* calls return error codes directly, so
change the error handling in the posix_spawnattr_init/setsigmask/setflags
sequence to capture each function's return value and use that value with
std::strerror() in the error message instead of reading errno; specifically, in
the posix_spawnattr_init() branch capture the int rc (from
posix_spawnattr_init), and in the two subsequent calls call
posix_spawnattr_setsigmask(&attr, &empty_mask) and
posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGMASK) into separate int
rc1/rc2 variables, check each independently, call posix_spawnattr_destroy(&attr)
on failure, and return
goggles::make_error<pid_t>(goggles::ErrorCode::unknown_error,
"posix_spawnattr_setsigmask() failed: " + std::string(std::strerror(rc1))) or
similarly for setflags using rc2 so the actual return codes are reported; update
references to attr and the existing make_error calls accordingly.
- Around line 430-440: The loop's waitpid call can return -1 and currently gets
ignored; update the child-watching logic around waitpid(child_pid,
&child_status, WNOHANG) to handle negative returns: if result == -1 and errno ==
EINTR, simply retry (continue) the wait loop; if result == -1 for any other
errno (including ECHILD), treat the child as exited by setting child_exited =
true and calling push_quit_event(); keep the existing handling when result ==
child_pid. Reference symbols: waitpid, child_exited, child_pid, child_status,
push_quit_event, errno, EINTR, ECHILD.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pixi.tomlsrc/app/main.cppsrc/render/backend/vulkan_backend.cpptests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/CMakeLists.txt
| if (posix_spawnattr_init(&attr) != 0) { | ||
| return goggles::make_error<pid_t>(goggles::ErrorCode::unknown_error, | ||
| std::string("posix_spawnattr_init() failed: ") + | ||
| std::strerror(errno)); | ||
| } | ||
| sigset_t empty_mask{}; | ||
| sigemptyset(&empty_mask); | ||
| if (posix_spawnattr_setsigmask(&attr, &empty_mask) != 0 || | ||
| posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGMASK) != 0) { | ||
| posix_spawnattr_destroy(&attr); | ||
| return goggles::make_error<pid_t>(goggles::ErrorCode::unknown_error, | ||
| "posix_spawnattr configuration failed"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
POSIX posix_spawnattr_init posix_spawnattr_setsigmask posix_spawnattr_setflags return error code directly or errno
💡 Result:
For these POSIX spawn-attribute functions:
posix_spawnattr_init()posix_spawnattr_setsigmask()posix_spawnattr_setflags()- (and the other
posix_spawnattr_set*()/get*()style routines)
they return the error number directly:
- Return
0on success - Return an error number (an
errno-style value such asEINVAL,ENOMEM, etc.) on failure
So you generally do not check -1 or rely on errno; you check the function’s return value and treat it as the error code. This is explicitly stated in the man pages (“return an error number from <errno.h>” / “otherwise, an error number shall be returned”). [1], [2]
Practical rule:
int rc = posix_spawnattr_setflags(&attr, flags);
if (rc != 0) {
// rc is the error code (errno value). You can do: strerror(rc)
}errno may or may not be modified by a given implementation, but POSIX only guarantees the returned error number for these interfaces. [1], [2]
Sources: Apple man page for posix_spawnattr_init(3) (also covering related setters) [1], and the POSIX/Linux man page text (POSIX wording) [2].
[1] (developer.apple.com)
[2] (unix.com)
Citations:
- 1: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/posix_spawnattr_init.3.html?utm_source=openai
- 2: https://www.unix.com/man-page/linux/3/posix_spawnattr_init?utm_source=openai
Use return values directly instead of errno for spawn-attr error messages.
The POSIX spawn-attribute functions (posix_spawnattr_init(), posix_spawnattr_setsigmask(), posix_spawnattr_setflags()) return error codes directly, not via errno. Line 121 incorrectly uses std::strerror(errno) instead of the function's return value, which can emit misleading diagnostics.
Additionally, the combined conditional at lines 125–126 masks which operation failed and omits actual error codes from the error message. Separate the checks and report the actual return value for each operation.
Suggested fix
- if (posix_spawnattr_init(&attr) != 0) {
+ const int init_rc = posix_spawnattr_init(&attr);
+ if (init_rc != 0) {
return goggles::make_error<pid_t>(goggles::ErrorCode::unknown_error,
std::string("posix_spawnattr_init() failed: ") +
- std::strerror(errno));
+ std::strerror(init_rc));
}
sigset_t empty_mask{};
sigemptyset(&empty_mask);
- if (posix_spawnattr_setsigmask(&attr, &empty_mask) != 0 ||
- posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGMASK) != 0) {
+ const int mask_rc = posix_spawnattr_setsigmask(&attr, &empty_mask);
+ if (mask_rc != 0) {
posix_spawnattr_destroy(&attr);
return goggles::make_error<pid_t>(goggles::ErrorCode::unknown_error,
- "posix_spawnattr configuration failed");
+ std::string("posix_spawnattr_setsigmask() failed: ") +
+ std::strerror(mask_rc));
+ }
+ const int flags_rc = posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGMASK);
+ if (flags_rc != 0) {
+ posix_spawnattr_destroy(&attr);
+ return goggles::make_error<pid_t>(goggles::ErrorCode::unknown_error,
+ std::string("posix_spawnattr_setflags() failed: ") +
+ std::strerror(flags_rc));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/main.cpp` around lines 118 - 129, posix_spawnattr_* calls return
error codes directly, so change the error handling in the
posix_spawnattr_init/setsigmask/setflags sequence to capture each function's
return value and use that value with std::strerror() in the error message
instead of reading errno; specifically, in the posix_spawnattr_init() branch
capture the int rc (from posix_spawnattr_init), and in the two subsequent calls
call posix_spawnattr_setsigmask(&attr, &empty_mask) and
posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGMASK) into separate int
rc1/rc2 variables, check each independently, call posix_spawnattr_destroy(&attr)
on failure, and return
goggles::make_error<pid_t>(goggles::ErrorCode::unknown_error,
"posix_spawnattr_setsigmask() failed: " + std::string(std::strerror(rc1))) or
similarly for setflags using rc2 so the actual return codes are reported; update
references to attr and the existing make_error calls accordingly.
| while (app.is_running()) { | ||
| app.process_event(); | ||
| app.tick_frame(); | ||
|
|
||
| if (!child_exited) { | ||
| const pid_t result = waitpid(child_pid, &child_status, WNOHANG); | ||
| if (result == child_pid) { | ||
| child_exited = true; | ||
| push_quit_event(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/app/main.cpp | sed -n '420,450p'Repository: K1ngst0m/Goggles
Length of output: 1263
🌐 Web query:
waitpid return -1 error handling EINTR WNOHANG recommended approach
💡 Result:
Recommended waitpid() error-handling pattern:
> 0: you reaped a child (PID returned).== 0withWNOHANG: no child status is available right now (not an error). (man7.org)== -1:errno == EINTR: interrupted by a signal → retry. (man7.org)errno == ECHILD: no (matching) unwaited-for children → you’re done. (man7.org)- otherwise: treat as a real error (e.g.,
EINVAL). (docs.oracle.com)
Common nonblocking “reap all exited children” loop (typical in a SIGCHLD-driven design):
for (;;) {
int status;
pid_t pid = waitpid(-1, &status, WNOHANG);
if (pid > 0) {
// handle this child's status
continue; // try to reap more
}
if (pid == 0) {
break; // none ready right now (WNOHANG)
}
// pid == -1
if (errno == EINTR) continue; // retry if interrupted
if (errno == ECHILD) break; // no more children
// other error
perror("waitpid");
break;
}Blocking wait (no WNOHANG): same idea—loop on EINTR until you get a PID or a real error. (man7.org)
Citations:
- 1: https://man7.org/linux/man-pages/man3/wait.3p.html?utm_source=openai
- 2: https://man7.org/linux/man-pages/man3/wait.3p.html?utm_source=openai
- 3: https://man7.org/linux/man-pages/man3/wait.3p.html?utm_source=openai
- 4: https://docs.oracle.com/cd/E88353_01/html/E37843/waitpid-3c.html?utm_source=openai
- 5: https://man7.org/linux/man-pages/man3/wait.3p.html?utm_source=openai
Handle waitpid negative returns to avoid a stuck windowed loop.
If waitpid(child_pid, &child_status, WNOHANG) returns -1, the loop currently ignores it and can keep running with child_exited == false. According to standard error handling for waitpid, -1 with errno == EINTR should be retried, while -1 with other errors (including ECHILD when the child is already gone) should trigger shutdown to prevent an infinite loop.
Suggested fix
if (!child_exited) {
const pid_t result = waitpid(child_pid, &child_status, WNOHANG);
if (result == child_pid) {
child_exited = true;
push_quit_event();
+ } else if (result < 0) {
+ if (errno == EINTR) {
+ continue;
+ }
+ GOGGLES_LOG_ERROR("waitpid() failed for pid {}: {}", child_pid, std::strerror(errno));
+ child_exited = true;
+ push_quit_event();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/main.cpp` around lines 430 - 440, The loop's waitpid call can return
-1 and currently gets ignored; update the child-watching logic around
waitpid(child_pid, &child_status, WNOHANG) to handle negative returns: if result
== -1 and errno == EINTR, simply retry (continue) the wait loop; if result == -1
for any other errno (including ECHILD), treat the child as exited by setting
child_exited = true and calling push_quit_event(); keep the existing handling
when result == child_pid. Reference symbols: waitpid, child_exited, child_pid,
child_status, push_quit_event, errno, EINTR, ECHILD.
1 participant
User description
PR Type
Bug fix, Enhancement
Description
Fix frame number tracking in headless render loop by moving assignment earlier
Refactor signal handling to block SIGTERM/SIGINT before thread spawning
Extract windowed mode logic into separate function for clarity
Modernize Vulkan semaphore import code to use C++ bindings
Mark all headless mode verification tasks as complete
Diagram Walkthrough
File Walkthrough
1 files
Move frame number update before validation checks2 files
Signal handling and windowed mode refactoringModernize Vulkan semaphore import to C++ bindings4 files
Mark remaining verification tasks as completeAdd headless mode requirements and filter chain control scopeCreate canonical headless mode specificationExpand filter chain routing and add headless backend requirements1 files
Add headless mode integration test with PNG verification16 files
Summary by CodeRabbit
New Features
Documentation
Tests
Refactor