Repository navigation
initial implementation - #1
Merged
Merged
Conversation
Persistent humbug sidecar client per the v0.1 requirements: executable discovery, direct fork/exec with drained stderr and per-command timeouts, NDJSON transport with id/type checks, byte-exact headers (always base64) and bodies (absent vs empty), structured OpenAPI::Humbug::Error exceptions, and reliable cleanup. Includes fake-sidecar unit tests and opt-in integration tests (OPENAPI_HUMBUG_EXECUTABLE). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Split the line exchange into an internal OpenAPI::Humbug::Transport base class and a Transport::Command subclass that runs humbug as a child process, so other connectors (unix or tcp sockets) can be added later. The transport decides whether the session ends with shutdown. Rename the error kind 'process' to 'transport'. Protocol tests now run against an in-memory fake transport; the fake sidecar process tests cover transport behavior only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
plicease
commented
Oct 14, 2026
| # child: fds above $^F are close-on-exec, so only 0, 1 and 2 survive exec, | ||
| # and $status_w reports exec failure to the parent. | ||
| eval { | ||
| POSIX::dup2(fileno $child_in, 0) // die; |
Member
Author
There was a problem hiding this comment.
dup2 returns a file descriptor on success, including 0 for stdin, so using // is correct here. However, the exec-failure reporting pipe appears to rely on Perl's $^F close-on-exec behavior. I'd add a test specifically verifying that an executable which fails at exec reports a structured client.spawn_failed error and leaves no child behind.
A second area worth testing is close() after the subprocess has already died. The implementation is careful about reaping and escalation, but subprocess lifecycle edge cases are notoriously difficult to get right.
Member
Author
There was a problem hiding this comment.
Addressed in ee06b15.
Exec failure / close-on-exec
- Every pipe end is now marked
FD_CLOEXECexplicitly rather than relying on$^F, and the child copies its three ends above fd 2 (F_DUPFD) beforedup2-ing them onto 0–2, so one can't overwrite another if the caller has closed a standard handle. (For the record, the previous code turned out to be safe in practice because of the order the pipes are created in; this removes the dependency on that ordering.) - New tests in
t/openapi_humbug_transport_command.t: an executable that passes-f/-xbut fails at exec (#!/nonexistent/interpreter) givestransport/client.spawn_failedwith no child left behind (waitpid(-1, WNOHANG) == -1), both directly and throughOpenAPI::Humbug->new; construction returns while the child is still running (spawns are under a 5salarm, so a leaked status pipe fails instead of hanging — verified by temporarily breaking it); and spawning works with STDIN, STDIN+STDOUT, and all three standard handles closed.
close() after the child died
- Killed with SIGKILL:
closedoesn't throw, returns false, reaps, and doesn't wait out the grace period. - Exited 0 on its own:
closedoesn't throw and returns true. - Already reaped by someone else:
closedoesn't throw. - Destroyed after the child died: no error;
validateafter death is atransporterror (client.eof/client.broken_pipe), never a result.
…icitly Address review on PR #1: - Mark every pipe end close-on-exec explicitly instead of relying on $^F, and copy the child's ends above fd 2 before dup2 so one cannot overwrite another when the caller has closed a standard handle. - Test that an executable failing at exec reports client.spawn_failed and leaves no child, that construction does not block on the child, and that spawning works with closed standard handles. - Test close(), destroy and validate after the child was killed, exited on its own, or was reaped elsewhere. Co-Authored-By: Claude Opus 5.5 <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
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.
For the initial implemetnation.