Merged
Conversation
2c7d0a5 to
7445efc
Compare
081d663 to
88e8089
Compare
tonyxiao
reviewed
Apr 23, 2026
| }, | ||
| { | ||
| "in": "query", | ||
| "name": "soft_time_limit", |
Collaborator
There was a problem hiding this comment.
why is this a query param i thought it is calculated automatically?
| }, | ||
| "additionalProperties": {} | ||
| }, | ||
| "soft_limit_fraction": { |
Collaborator
There was a problem hiding this comment.
should make a note this only applies to destinations?
| async function* gate(): AsyncIterable<T> { | ||
| for await (const msg of takeLimits<T>(opts)(source)) { | ||
| if (msg.type === 'eof') { | ||
| state.stopped = (msg as EofMessage).eof.has_more |
Collaborator
There was a problem hiding this comment.
i think you should just call this hasMore for ease of use
| const elapsedSec = Math.round((Date.now() - flushStartedAt) / 1000) | ||
| log.info(`flushing to Sheets (in progress, ${elapsedSec}s)`) | ||
| yield { | ||
| type: 'log' as const, |
Collaborator
There was a problem hiding this comment.
don't yeid log. use logger.debug
tonyxiao
approved these changes
Apr 23, 2026
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.
Summary
Turns
soft_time_limitinto a useful tool to trigger destination flush instead of a fixed ±1s buffer aroundtime_limit, and rebuilds the Sheets destination around it so we never checkpoint state past data we haven't written.The old model
takeLimitstook a singletime_limitand silently split it into a soft cutoff atdeadline − 1sand a hard cutoff atdeadline + 1s. It was wrapped around the destination's output, so when the soft deadline fired the destination gotreturn()'d in the middle of its flush. The Sheets destination defended against that with afinally { await flushAll() }— which mostly worked but had two sharp edges:source_statemessages got yielded mid-stream, before the corresponding rows were actually in the sheet. A time-limit cut could leave the engine with a cursor ahead of the data.flushAll(wide catalogs take tens of seconds) could be killed by the sametime_limitthat was supposed to let it finish.The new model
soft_time_limitis a cooperative source-side stop. It's a separate, explicit option onSourceReadOptionsand a separate/pipeline_syncquery param. When it fires, we close the source iterator withreturn(), thefor awaitloop in the destination seesdone, and the destination's own post-loop code runs — flush, emit state, yield final logs — bounded only by the hardtime_limit.takeLimitsstops inventing ±1s buffers; soft and hard are just two independent deadlines and whichever fires first wins.soft_time_limitis dynamic per destination.ConnectorSpecificationnow acceptssoft_limit_fraction(0–1). When a caller doesn't passsoft_time_limitexplicitly, the engine derives it from the destination's spec:time_limit × fraction, falling back totime_limit − 1for destinations that don't declare one (preserves the old behaviour for Postgres and friends). Google Sheets declares0.5because its flush tail can easily eat half the budget; a fast destination can stay at the default. This means the source-read window automatically shrinks for destinations that need more flush time — no caller tuning required.The soft limit is the destination's flush cue. The Sheets destination no longer flushes in a
finally. It now:source_statefor the entirewrite()call alongside the record buffers,$stdinclosing (either natural EOF orlimitSource.return()from a soft hit) as the one and only "time to drain" signal,flushAllafter the drain,connection_status: failedinstead.If the hard
time_limitinterrupts the flush itself, the iterator'sreturn()skips the state yield — which is exactly what we want. We'd rather re-sync the remainder next run than advance a cursor past rows that never landed.New
limitSourcehelper on the engine side. It wrapstakeLimitsaround the source stream but hides the synthetic terminaleofand exposes astoppedflag instead. That's what lets the destination'swrite()keep running after a soft cut — the destination sees a normaldone, not a fake eof. The engine readsgate.stoppedwhen it builds its own outgoingeofsohas_more=truestill reflects "we stopped because of a limit, come back for more."Flush heartbeats. While
flushAllis in flight,uploadToSheetyields aflushing to Sheets (in progress, Ns)debug log every 20s (override viaflushHeartbeatMs). Two reasons: Temporal stops treating the activity as idle, and humans tailing logs see that we're actually waiting on Sheets, not hung.Supporting changes
applyBatch. We now add upcurrent grid + append payload(and the projected post-expansion grid, to catch column growth) and throw locally with a readable message instead of waiting for Sheets to return an opaque API error.MAX_CELLS_PER_SPREADSHEETis exported so tests can assert against it.drainMessagesheartbeat switched to 15s wall-clock (was every 50 messages). Decouples heartbeat cadence from source throughput — a slow source no longer goes silent, and a fast source doesn't heartbeat-storm.timeLimitbumped 30 → 300 in the service backfill loop, the backfill workflow, and the/pipelines/:id/backfillroute. With the soft/hard split doing real work now, 30s didn't give destinations a fair flush window.getSpreadsheetMetasnapshot was reused acrossensureSheets+ensureIntroSheet+protectSheets; now we refetch afterensureSheetsso the "Sheet1 renamed" state is visible downstream. (Unrelated to the drain work, but it shook out of testing wide-catalog setup.)log.warncalls inwriter.tsthat were just informational are nowlog.debug.Behavioural shape, in one line
Soft limit = "source, wrap it up." Hard limit = "everyone, stop." Destinations declare how much slack they need between the two. State checkpoints never get ahead of durable writes.
How to test
pipeline_sync() graceful closesuite inengine.test.tsandtakeLimits/limitSourcesuites inpipeline.test.tscover natural completion, soft-cut drains, hard-cut forces, and the combined case.state is re-emitted after flush, not mid-stream,state messages are suppressed when flushAll fails, andemits heartbeat log messages while flushAll is in flightinindex.test.ts.applyBatch cell-count limitsuite covers single-batch overflow, projected-grid overflow, the update-only path, and end-to-end failure surfacing throughdest.write().Quickest smoke test: run a Sheets backfill against a wide catalog with
time_limit=60, confirm the response iseof.has_more=truewith anending_statethat matches what's actually in the sheet (not ahead of it), and watch for theflushing to Sheets (in progress, Ns)log during the final drain.