fix(workflow-executor): page through the segment when one padded page holds only known records - #1931
Conversation
3 new issues
|
|
Coverage Impact This PR will not change total coverage. Modified Files with Diff Coverage (2)
🛟 Help
|
|
|
||
| page | ||
| .filter(recordId => !knownSet.has(recordId)) | ||
| .slice(0, config.maxConcurrentRuns - candidates.size) |
There was a problem hiding this comment.
Test gap: cross-page candidate-cap truncation is never exercised.
This .slice(0, config.maxConcurrentRuns - candidates.size) runs on every page, but across the padded-paging suite the subtraction only ever executes as 20 - 0 — the cap is either satisfied entirely from page 1, or the multi-page tests finish well under the cap (finals of 2 and 7). No test accumulates candidates across pages so that a later page must be truncated by a non-zero running total.
That's the exact budget this line enforces, and the logic the recent "cap padded candidates at the run budget" commit was circling. A mutation replacing config.maxConcurrentRuns - candidates.size with a constant config.maxConcurrentRuns (over-slicing every page instead of just the remaining budget) would pass the whole suite.
Suggested case: page 1 yields 15 fresh records (below the 20 cap, so the loop continues), page 2 yields 15 more → assert sync receives exactly 20 candidates and page 2 contributed only 5.
There was a problem hiding this comment.
Valid, fixed in d092bf9. Added "should take from a later page only what the earlier pages left of the run budget": page 1 brings 15 fresh ids, page 2 brings 15 more, and sync must receive the 15 plus only the first 5 of page 2. Red with the mutation you describe (.slice(0, config.maxConcurrentRuns) sends 30).
| page | ||
| .filter(recordId => !knownSet.has(recordId)) | ||
| .slice(0, config.maxConcurrentRuns - candidates.size) | ||
| .forEach(id => candidates.add(id)); |
There was a problem hiding this comment.
The .slice() budget is maxConcurrentRuns - candidates.size, but the filter only removes ids in knownSet — not ids already collected in candidates from an earlier page. Reads are sequential, un-snapshotted offset pages (no cursor). If a record shifts across a page boundary between reads (a waiting-on-a-person record released, or a row inserted, between page 1 and page 2), a previously-seen id can reappear early in a later page: it passes the knownSet filter, consumes a slice slot, but candidates.add(id) is a no-op on the Set — so a genuinely new record past the cutoff on that page is dropped even though budget was still available.
Impact is bounded (the next iteration recomputes the budget, so a following page recovers the slack unless reachedEnd/MAX_PADDED_PAGES hits first — worst case one sweep under-fills the run budget), but it's a real gap and it's exactly the class of bug this PR guards against.
Suggested fix — also filter against what's already collected:
.filter(recordId => !knownSet.has(recordId) && !candidates.has(recordId))Note: the Set dedup is never exercised today because every test uses disjoint page ranges — the same tests would pass with a plain array + push.
There was a problem hiding this comment.
Valid, fixed in 657f62d. The filter now also drops ids already collected from an earlier page: !knownSet.has(recordId) && !candidates.has(recordId). Covered by "should not spend the run budget on a record an earlier page already brought": page 2 repeats fresh-14 from page 1, and sync must still receive the 15 of page 1 plus 5 genuinely new ids of page 2 (red without the filter, it sends 19).
| ); | ||
| }); | ||
|
|
||
| describe('when one padded page holds nothing but known records', () => { |
There was a problem hiding this comment.
Coverage gap: the multi-page loop is never exercised via the not-in-refused route. readPaddedCandidates is now reached from two call sites — the direct paddedPageReason branch and the not_in-refused catch block (which also threads the new logContext parameter). Every test in this block uses too-many-known-records or the composite-key reason; the existing not-in-refused tests (~lines 704-774) return a short single page, so the loop never advances past page 1 on that path.
A bug specific to that route — wrong paddedPageReason in the cap-warning log, or knownSet computed differently when entered via the catch block — wouldn't be caught. Suggest adding one not-in-refused-flavored multi-page (or 5-page-cap) test.
There was a problem hiding this comment.
Valid, added in 657f62d: "should page the same way when the fallback comes from a refused not_in". On that route the page size is maxConcurrentRuns + known, so a full page always holds a full budget and paging only kicks in once the 500 cap bites. The test uses maxConcurrentRuns: 480 with 30 known records: not_in is refused, page 1 brings 470 fresh ids, page 2 is read with pageNumber: 2 and sortByPrimaryKey: true, and the poll log carries paddedPageReason: "not-in-refused" and candidatePagesRead: 2.
03a2448 to
a87e99d
Compare
ShohanRahman
left a comment
There was a problem hiding this comment.
Both raised points (observability of a partial padded read on a persistent later-page failure, and the port not encoding the single-column-key paging invariant) were answered convincingly:
- The partial read is already machine-distinguishable: a distinct
A later padded candidate page failed…Warn carryinginboxId/pagesRead/erroronce per sweep, rate-alertable, withErrorcorrectly reserved for the no-progress case. Records aren't lost — re-read on the next sweep. Adding per-inbox failure state just for a log level isn't worth it. - The paging invariant lives in the sole caller (gated on a single-column key, sending
sortByPrimaryKey: pageable) and is pinned by the composite-key test. AsortByPrimaryKey: truetype wouldn't cover the composite case anyway. Fair to fold the fields together only when a second caller appears.
Core paging/budget/end-of-segment/error-handling logic reviewed as correct and well-covered. LGTM 👍
… holds only known records Records waiting on a person stay known for as long as nobody handles them. Past about 480 of them, the single padded page held none but them and the automation silently found nothing. The padded read now pages, sorted by the first key column, up to five pages of 500, and warns when that finds none. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…a later page fails Review follow-up: a timeout on page 2 no longer sends an empty sync, and two comments no longer overclaim. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ded a candidate A later page failing before any candidate no longer reads as a page cap reached with nothing new. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… composite keys on one page Review follow-ups (Macroscope): a 500-record page no longer sends 500 candidates, and a composite key, which agent-client can only sort on its first column, keeps the single unsorted page offset paging cannot walk. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A later page must only add what the earlier pages left of maxConcurrentRuns. No test accumulated candidates across pages, so slicing every page at the full budget passed the suite. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…rlier padded page brought Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
657f62d to
9b7663a
Compare
f53dfa9
into
feature/prd-1183-runtime-automation-poller
… holds only known records (#1931)

fixes PRD-1362
Targets the integration branch of #1906.
Why
Records waiting on a person keep their
doingrow in the automated inbox, and nothing bounds how many there are. That is validated behaviour. Past 150 known records, the poller replacedpk not_inwith one padded page of at most 500 records, then filtered known ids on its side. Past about 480 known records, that single page held nothing but known records: the automation silently found no new record while the fallback inbox kept filling.Change
maxConcurrentRunscandidates collected (never more are sent, the same ceiling as thenot_inpath);MAX_PADDED_PAGES = 5pages read.The padded candidate read found no new record within its page capfires when the cap is reached with no candidate while the segment went on. It carriesknown,pagesReadandpaddedPageReason.candidatePagesReadis added toAutomated inbox polled.pageNumberandsortByPrimaryKey, and agent-client already supports both.Unchanged:
not_inpath and its capability check (PRD-1279), so an inbox with 150 known records or fewer still costs one read;reconcileClosed;The server is not touched, and there is no contract change.
Load on the customer's agent
ORDER BYafter the segment scope). Datasources whose key is not sortable ignore it.Tests
maxConcurrentRunscandidates;not_in;page[number],sorton the first key column, and no sort by default.🤖 Generated with Claude Code
Note
Fix padded candidate reads to page through segments of known records
AutomationPoller.readPaddedCandidatesnow walks up toMAX_PADDED_PAGES(5) ordered pages when a padded page holds only known records, instead of stopping after one pageListSegmentRecordIdsQuerygained optional page-number and sort controls, andAgentClientSegmentReader.listRecordIdsserializes them into agent requestsreadPaddedCandidatesif that path needs coverageChanges since #1931 opened
AutomationPoller.cyclescheduling that verifies pagination budget handling across multiple pages [d092bf9]AutomationPollerto exclude IDs already present in thecandidatesset in addition to those inknownSet, preventing duplicate record IDs from being accumulated across multiple pages within the same sweep [657f62d]automation-poller.test.tsto verify that candidate selection does not waste run budget on duplicate records appearing in subsequent pages and that paging behavior remains consistent when falling back due to a refusednot_infilter [657f62d]Macroscope summarized 9b7663a.