Repository navigation
fix: flush sys.__stdout__ in PrintBuffer.flush() - #1167
Conversation
…ch the platform promptly Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1789662473-print-buffer-flush-stdout#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1789662473-print-buffer-flush-stdoutPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
🟡 Changes recommended
The new subprocess regression test embeds an indented python -c script that will raise IndentationError due to leading whitespace, causing the test to fail.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR fixes delayed connector/log output by ensuring PrintBuffer.flush() flushes the underlying sys.__stdout__ stream after writing buffered content, so bytes reach the stdout pipe promptly (important for platform heartbeats and log visibility).
Changes:
- Update
PrintBuffer.flush()to callsys.__stdout__.flush()afterwrite(). - Add unit/regression tests validating that
flush()both writes and flushes, including a real-pipe subprocess test.
File summaries
| File | Description |
|---|---|
airbyte_cdk/utils/print_buffer.py |
Flushes underlying real stdout after writing buffered output to avoid long delays when stdout is a pipe. |
unit_tests/utils/test_print_buffer.py |
Adds unit/regression coverage for the new flush-through behavior, including a real pipe subprocess scenario. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
sys.__stdout__ in PrintBuffer.flush() so messages reach the platform promptlysys.__stdout__ in PrintBuffer.flush()
Daryna Ishchenko (darynaishchenko)
left a comment
There was a problem hiding this comment.
The fix makes sense: without it, batches sit in the ~8 KiB sys.__stdout__ buffer on a pipe instead of reaching the platform every flush_interval. One side effect for logs is noted inline.
#1167 landed on main as a squash of the commit this branch already carries, so both files keep this branch's version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Overview
👉 TL;DR: Connector output written through the CDK's print buffer could sit inside the process for hours before the platform saw it, because the buffer wrote to the real stdout but never asked the interpreter to push those bytes into the pipe. This made healthy, progressing sources look silent to the platform heartbeat and to anyone reading the logs.
Specifically,
PrintBuffer.flush()now callssys.__stdout__.flush()aftersys.__stdout__.write().Related: airbytehq/oncall#12977 (internal,
source-stripe). Requested by Zane Hyatt (ZaneHyattAB).Changes
PrintBuffer.flush()flushes the underlyingsys.__stdout__TextIOWrapperafter writing the batched content.unit_tests/utils/test_print_buffer.py:flush()writes to and flushessys.__stdout__;PRINT_BUFFER(withPYTHONUNBUFFEREDunset) emits a RECORD and alogger.infoline, then sleeps; the parent asserts both lines arrive on the pipe while the child is still alive. Without the fix this test times out with zero bytes received.Review Spotlight
Reviewers with limited time, please review first:
print_buffer.py#L54-L64: the one-line fixtest_print_buffer.py#L27-L86: the real-pipe regression testWhy this is a correctness bug, not just a debuggability one
When stdout is a pipe (always, on the platform),
sys.__stdout__is a block-bufferedTextIOWrapperwith an ~8 KiB pending buffer.PrintBuffer.flush()wrote into that buffer and returned, so bytes only reached the pipe when cumulative output crossed 8 KiB or the process exited. Measured with the real CDK logger and a real pipe: 230 B writes arrive in 8,280 B bursts; a RECORD + twologger.infolines emitted at t+1 s reached the reader only at process exit (t+9 s); withPYTHONUNBUFFERED=1they arrived at t+1.5 s.The platform's source heartbeat counts messages it receives. For a low-volume or keepalive-only stream (e.g. one 213 B interim STATE per 600 s state-throttle window) filling 8 KiB takes ~36 messages ≈ 6 h, so with the default
heartbeat-max-seconds-between-messagesof 3 h a source that is progressing can be failed as silent. In the airbytehq/oncall#12977 investigation, one production job's observed 6h03m gap between two adjacentlogger.infocalls in_on_stream_is_doneis exactly this buffer filling (36 × 600 s), not a stall — and three independent analyses of that job reached wrong conclusions from the log because of it.Design notes: why flush in
PrintBuffer.flush()rather thanwrite_through/PYTHONUNBUFFEREDShow/Hide Content
Options considered:
sys.__stdout__.flush()insidePrintBuffer.flush()(chosen).PrintBufferis already the single choke point for stdout inlaunch(); its existing 0.1 s interval keeps batching for theprint()path, andlogging.StreamHandler.emitalready callsPrintBuffer.flush()after each log line. Adding the fd flush there makes "leftPrintBuffer" mean "reached the pipe", with syscall count still bounded by the interval (≤ 10/s for records) plus one per log line. Smallest, most contained change; nothing outside the entrypoint path is affected.sys.stdout.reconfigure(write_through=True)/ line buffering at the entrypoint. Equivalent syscall profile for thePrintBufferpath (eachPrintBuffer.flush()already writes one combined string), but it is process-global: it also affects any code that writes tosys.__stdout__directly, and it must be applied in every entrypoint (launch,source_declarative_manifest, connector builder). Rejected as less contained for no benefit.PYTHONUNBUFFERED=1in the connector base image. Correct and complementary, but outside this repository (base image /airbytemonorepo) and would not fix connectors pinned to older images. Suggested as a follow-up.Residual behaviour after this change: messages written via
print()within 0.1 s after the last flush stay inPrintBufferuntil the nextwrite()orloggercall (the existing design). A message written after ≥ 0.1 s of idleness is flushed immediately, so a sparse keepalive stream now reaches the platform on emission.Throughput and syscall measurements
Benchmark: child process with stdout = real pipe,
init_logger("airbyte")+with PRINT_BUFFER:,print()of 1,000,000 ~270 B RECORD lines plus onelogger.infoper 100k records; parent drains the pipe. Python 3.12, same machine, 3 runs each (child-side records/sec = records / time inside thewithblock).write()syscalls (strace -f -c, 1M records)Difference is within run-to-run noise; syscall count is unchanged (+1 per
loggerline, because those are now pushed through instead of coalescing with the next record batch). Per-line flushing was therefore not needed and the 0.1 s batching is preserved.Risks
logger.*call now costs onewrite()syscall (previously it could coalesce into the 8 KiB buffer). WithLOG_LEVEL=debug(request/response logging) this is one extra syscall per HTTP request — negligible against the request itself.write()already blocked there when the 8 KiB buffer spilled; the flush just moves the block to the same call.Follow-ups
PYTHONUNBUFFERED=1(or-u) in the Python connector base image so stderr and any directsys.__stdout__writers get the same guarantee (outside this repo).HttpClient(the other finding from airbytehq/oncall#12977).Test plan
poetry run pytest unit_tests/utils/test_print_buffer.py unit_tests/test_logger.py unit_tests/test_entrypoint.py -q→ 61 passedmain(times out after 5 s withb''received) and passes with this change.poetry run ruff check/ruff format --checkon changed files;poetry run mypy --config-file mypy.ini airbyte_cdk→ no issues.Suggested reviewers
Maxime Carbonneau-Leclerc (@maxi297), Artem Inzhyyants (@artem1205) (recent authors of
print_buffer.py/logger.py). Not assigned on purpose; Zane will assign.Link to Devin session: https://app.devin.ai/sessions/0e315592fd95478b8b4db2e122b927a8
Open in Devin Desktop: https://app.devin.ai/desktop/session/0e315592fd95478b8b4db2e122b927a8?variant=devin