mirror of
https://github.com/johnkerl/miller.git
synced 2026-07-17 16:38:54 +00:00
Fix mid-stream error-loss race (flaky exit codes on transformer errors) (#2211)
When a transformer fails mid-stream (e.g. join -s with a malformed left file), the error could be lost, yielding exit 0 with no stderr message: - runSingleTransformerBatch forwarded the end-of-stream marker downstream before runSingleTransformer sent the error to dataProcessingErrorChannel, so the record-writer could finish and signal done-writing while the error was still unsent. - Even with the error buffered, stream.Stream's select loop chooses among simultaneously-ready channels at random, and exiting on the done-writing signal dropped the buffered error. Send the error before forwarding the end-of-stream marker (so it is always buffered before the writer can finish), and drain the error channels after the select loop exits. Observed as a one-off Windows CI failure of test/cases/verb-join/left-file-malformed-sorted, where the same case passed on automatic rerun within the same job. Reproduced locally by widening the deschedule window with a sleep between the end-of-stream forward and the error send: 5/5 runs exited 0 with empty stderr; with this fix, 200/200 runs exit 1 with the expected message even with adversarial delays injected on both sides of the end-of-stream forward. Follow-up to the os.Exit-removal refactor (plans/exit.md, #2204/#2205). Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
1a20b32bb5
commit
5ede886106
2 changed files with 42 additions and 13 deletions
|
|
@ -104,6 +104,26 @@ func Stream(
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// An error and the done-writing signal can be ready simultaneously, and
|
||||||
|
// select chooses among ready channels at random -- so an error may still
|
||||||
|
// be sitting in a buffer when the loop above exits. Senders guarantee the
|
||||||
|
// error is buffered before the end-of-stream marker that lets the writer
|
||||||
|
// finish, so a final non-blocking drain is sufficient to pick it up.
|
||||||
|
if retval == nil {
|
||||||
|
select {
|
||||||
|
case ierr := <-inputErrorChannel:
|
||||||
|
retval = ierr
|
||||||
|
default:
|
||||||
|
}
|
||||||
|
}
|
||||||
|
if retval == nil {
|
||||||
|
select {
|
||||||
|
case derr := <-dataProcessingErrorChannel:
|
||||||
|
retval = derr
|
||||||
|
default:
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if err := bufferedOutputStream.Flush(); err != nil && retval == nil {
|
if err := bufferedOutputStream.Flush(); err != nil && retval == nil {
|
||||||
retval = err
|
retval = err
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -213,24 +213,21 @@ func runSingleTransformer(
|
||||||
outputRecordChannel,
|
outputRecordChannel,
|
||||||
inputDownstreamDoneChannel,
|
inputDownstreamDoneChannel,
|
||||||
outputDownstreamDoneChannel,
|
outputDownstreamDoneChannel,
|
||||||
|
dataProcessingErrorChannel,
|
||||||
options,
|
options,
|
||||||
)
|
)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
// Surface the error to stream.Stream's select loop. Non-blocking
|
// runSingleTransformerBatch has already sent the error to
|
||||||
// send: if another goroutine errored first, that error wins.
|
// dataProcessingErrorChannel and then forwarded an end-of-stream
|
||||||
select {
|
// marker downstream, so the record-writer drains and finishes,
|
||||||
case dataProcessingErrorChannel <- err:
|
// upon which stream.Stream returns the error.
|
||||||
default:
|
//
|
||||||
}
|
|
||||||
// Tell upstream (transformers and ultimately the record-reader,
|
// Tell upstream (transformers and ultimately the record-reader,
|
||||||
// via the mlr-head mechanism) that we'll ignore further input.
|
// via the mlr-head mechanism) that we'll ignore further input.
|
||||||
select {
|
select {
|
||||||
case outputDownstreamDoneChannel <- true:
|
case outputDownstreamDoneChannel <- true:
|
||||||
default:
|
default:
|
||||||
}
|
}
|
||||||
// runSingleTransformerBatch has already forwarded an end-of-stream
|
|
||||||
// marker downstream, so the record-writer drains and finishes,
|
|
||||||
// upon which stream.Stream returns the error we sent above.
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
@ -238,10 +235,10 @@ func runSingleTransformer(
|
||||||
|
|
||||||
// runSingleTransformerBatch passes one batch of records through the
|
// runSingleTransformerBatch passes one batch of records through the
|
||||||
// transformer. The boolean return is true on end of record stream. A non-nil
|
// transformer. The boolean return is true on end of record stream. A non-nil
|
||||||
// error is a mid-stream transformer failure: any output produced before the
|
// error is a mid-stream transformer failure: the error has already been sent
|
||||||
// failure, plus an end-of-stream marker, has already been forwarded
|
// to dataProcessingErrorChannel, and any output produced before the failure,
|
||||||
// downstream so the rest of the chain and the record-writer can drain and
|
// plus an end-of-stream marker, has already been forwarded downstream so the
|
||||||
// finish cleanly.
|
// rest of the chain and the record-writer can drain and finish cleanly.
|
||||||
func runSingleTransformerBatch(
|
func runSingleTransformerBatch(
|
||||||
inputRecordsAndContexts []*types.RecordAndContext, // list of types.RecordAndContext
|
inputRecordsAndContexts []*types.RecordAndContext, // list of types.RecordAndContext
|
||||||
recordTransformer RecordTransformer,
|
recordTransformer RecordTransformer,
|
||||||
|
|
@ -249,6 +246,7 @@ func runSingleTransformerBatch(
|
||||||
outputRecordChannel chan<- []*types.RecordAndContext, // list of *types.RecordAndContext
|
outputRecordChannel chan<- []*types.RecordAndContext, // list of *types.RecordAndContext
|
||||||
inputDownstreamDoneChannel <-chan bool,
|
inputDownstreamDoneChannel <-chan bool,
|
||||||
outputDownstreamDoneChannel chan<- bool,
|
outputDownstreamDoneChannel chan<- bool,
|
||||||
|
dataProcessingErrorChannel chan<- error,
|
||||||
options *cli.TOptions,
|
options *cli.TOptions,
|
||||||
) (bool, error) {
|
) (bool, error) {
|
||||||
outputRecordsAndContexts := make([]*types.RecordAndContext, 0, len(inputRecordsAndContexts))
|
outputRecordsAndContexts := make([]*types.RecordAndContext, 0, len(inputRecordsAndContexts))
|
||||||
|
|
@ -292,6 +290,17 @@ func runSingleTransformerBatch(
|
||||||
outputDownstreamDoneChannel,
|
outputDownstreamDoneChannel,
|
||||||
)
|
)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
// Surface the error to stream.Stream's select loop.
|
||||||
|
// Non-blocking send: if another goroutine errored first, that
|
||||||
|
// error wins. This must happen before the end-of-stream
|
||||||
|
// marker is forwarded below: the marker lets the
|
||||||
|
// record-writer finish and signal done-writing, and the error
|
||||||
|
// must already be buffered by then or stream.Stream could
|
||||||
|
// return nil, losing the nonzero exit code.
|
||||||
|
select {
|
||||||
|
case dataProcessingErrorChannel <- err:
|
||||||
|
default:
|
||||||
|
}
|
||||||
// Forward what was produced before the failure, plus an
|
// Forward what was produced before the failure, plus an
|
||||||
// end-of-stream marker so downstream drains and finishes.
|
// end-of-stream marker so downstream drains and finishes.
|
||||||
outputRecordsAndContexts = append(outputRecordsAndContexts,
|
outputRecordsAndContexts = append(outputRecordsAndContexts,
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue