fix(shell): keep partial end marker across reads so jobs don't hang#277
Open
skipi wants to merge 2 commits into
Open
fix(shell): keep partial end marker across reads so jobs don't hang#277skipi wants to merge 2 commits into
skipi wants to merge 2 commits into
Conversation
The shell executor detects command completion by scanning PTY output for the end marker (\001 <endMark> <exitcode>). scan() entered buffering mode on the FIRST \001 in the buffer, but that byte can be a stray 0x01 from the command's own output (e.g. binary/protobuf printed to the log). When a real end marker arrived right after such a stray byte and had not yet been fully received, the memory-bound flush dumped the ENTIRE buffer - including the partial real marker - to the job output. The marker's tail then arrived in the next read with no leading \001, was flushed as plain output, and the complete marker never appeared whole in the buffer, so scan() looped forever and the job hung until the global time limit even though the command had already exited. Fix: in buffering mode, flush only up to the LAST \001 in the buffer, retaining a possible partially-received marker for the next read. The retained tail is capped at len(endMark)+10 (the longest a real marker can be), so a pathological stream of \001 bytes cannot grow the buffer without bound. Also make start-marker detection tolerant of a bare \n terminator (not just \r\n): a previous command can leave the PTY in -onlcr mode, in which case the next start marker arrives with \n and was never recognized, producing the same hang. The end-marker regex already tolerated [\r\n]+. Reproduced by three package-local tests that drive scan() over an os.Pipe with scripted read boundaries and a timeout, so a lost marker fails fast instead of hanging the suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review round 1 fixes for PR #277. Finding 1 (same-read output loss): scan() located the end marker by the first \001 then searched the whole buffer for the marker body, so when a stray \001 preceded the real marker in one read, everything between them was discarded. Match the complete marker (now including its \001 prefix) with FindStringSubmatchIndex and flush everything before the match start as ordinary output. The keep-tail buffering path is unchanged - it still retains from the last \001, which is the real marker's own prefix. Finding 2 (test timing + data race): replace the sleep-based pipe sequencing in the scan tests with a channel-driven read source, so each written chunk is returned as exactly one read() with deterministic boundaries and no goroutine races on inputBuffer. Adds a nil-defaulted readSource seam on Process for tests to drive reads directly. Also fixes a pre-existing data race the deterministic harness exposed: OutputBuffer.IsEmpty() read the shared byte slice without the mutex, and done was read/written without synchronization. Guard IsEmpty() with the lock and make done an atomic flag. Needed for the scan tests to pass under -race. Scan tests pass under `go test -race ./pkg/shell/ -count=5`. Co-Authored-By: Claude Fable 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
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.
Problem
The shell executor detects command completion by scanning PTY output for
\001 <token>-end <exitcode>. Two latent hangs inpkg/shell/process.go:Stray
0x01in command output swallows the real end marker.endMarkerHeaderIndex()locks onto the first\001in the buffer — which may be a stray byte from the command's own output (binary protobuf, tarballs, control-char logs). The scan loop enters buffering mode, and once the buffer exceedslen(endMark)+10without a regex match,flushInputAll()dumps the entire buffer to job output — including a partially-received real end marker sitting after the stray byte. The marker remainder arrives in the next read with no\001, gets flushed as plain output, and the complete marker never appears whole:scan()never returns and the job hangs until its time limit, even though the command exited. Observed in the wild: the job log's finalcmd_outputevent contains the leaked marker verbatim (^A <token>-end 0) and the agent idles.Start marker matched with a literal
\r\n. If a command leaves the PTY with output post-processing disabled (stty -opost/-onlcr), the next start marker arrives with a bare\nandwaitForStartMarker()never recognizes it — same hang, different marker. (The end-marker regex already tolerates[\r\n]+.)Fix
\001in the buffer, retaining everything from it onward as a possible partially-received marker. The retained tail is capped atlen(endMark)+10— the longest a real marker can be — so if the tail exceeds that, the leading\001provably isn't a partial marker and the whole buffer is flushed. Memory stays bounded even under a pathological stream of\001bytes.waitForStartMarker()now matchesstartMark + \r?\n(CR optional). Deliberately not the greedy[\r\n]+: that would also swallow a leading blank line of genuine command output.Tests
Three new deterministic
scan()tests drive the scanner over anos.Pipe-backed shell with scripted read boundaries and a 5s timeout, so a lost marker fails fast instead of hanging:Test__Scan_CompletesWhenStrayControlBytePrecedesEndMarker— the reported hang; failed before the fix, passes after.Test__Scan_CompletesWhenStartMarkerHasBareNewline— failed before the fix, passes after.Test__Scan_BoundsBufferOnRepeatedControlBytes— guards the memory bound of the new tail-retention path.Repro sketch for the original hang: any command whose final output writes a
0x01byte within ~len(endMark)+10bytes of command exit — e.g.printf 'some output \001 junk that pads the buffer past the threshold'; true.🤖 Generated with Claude Code