Skip to content

fix: FIFO partial drain under spawn argv budget (#3) - #9

Open
wized2 wants to merge 3 commits into
CL-BAF:mainfrom
wized2:fix/spawn-push-argv-limit
Open

wized2 wants to merge 3 commits into
CL-BAF:mainfrom
wized2:fix/spawn-push-argv-limit

Conversation

@wized2

@wized2 wized2 commented Sep 20, 2026

Copy link
Copy Markdown

Summary

Fixes #3 — spawn_push delivery stalled once the pending queue’s combined framed size exceeded the platform argv budget, even when the head of the queue would fit.

Behavior

Case Before After
Head message alone > budget Refuse whole queue Same (refuse, queue untouched, error once)
Prefix fits, later msgs don’t Refuse everything Drain fitting prefix (FIFO); rest stay pending
All fit Drain all Unchanged

Implementation

  1. drainForDelivery(state, id, opts?) — pass canDeliver through to drainQueue (already supported).
  2. deliverViaSpawn — running byte budget via canDeliver; head-only oversized still refuses without draining.

Tests

  • P2-2 rewritten: head message alone oversized → skip, no spawn, queue intact, error deduped.
  • P2-2b (new): three ~45k messages on POSIX → deliver 2, leave 1 pending, one spawn, no argv error.
npx tsx --test test/unit/core/spawn-delivery.test.ts
# 19 pass

spawn_push refused the entire pending queue whenever the *combined*
framed size exceeded the platform argv limit, even when the head of the
queue would fit. That left recipients stalled forever once the backlog
grew past the budget.

- drainForDelivery now accepts the same opts as drainQueue (canDeliver)
- deliverViaSpawn drains every message that still fits (FIFO), leaving
  the rest pending for the next cycle
- Only refuse delivery when the head message alone cannot fit; queue
  stays untouched and the operator is pointed at pull delivery
- Rewrite P2-2 for head-oversized; add P2-2b for partial drain (issue CL-BAF#3)
@CL-BAF

CL-BAF commented Sep 20, 2026

Copy link
Copy Markdown
Owner

Thanks for the PR. The overall approach looks right, but there are a few issues to fix before merge:

The current canDeliver logic can skip a message that does not fit and then deliver a later smaller message, which breaks strict FIFO ordering. Once the first pending message does not fit, later messages should remain queued too.
Please add a regression test with differently sized messages, for example ~80k, ~50k, and ~5k, and verify the 5k message does not leapfrog the 50k one.
Argv sizing should use UTF-8 byte length rather than JavaScript string length.
"\n\n---\n\n".length is 7, not 6.

Once those are fixed, I’ll review it again.

Once the first pending message does not fit, stop draining so a smaller
later message cannot leapfrog. Measure argv costs with UTF-8 byteLength.
Correct separator length for "\\n\\n---\\n\\n" (7). Add regression test.
@wized2

wized2 commented Oct 21, 2026

Copy link
Copy Markdown
Author

Thanks — addressed the review notes:

  1. Strict FIFO — when canDeliver rejects the head, we stop draining and leave all later messages queued (no leapfrog of a smaller message past a blocked larger one).
  2. UTF-8 bytes — argv cost uses Buffer.byteLength(..., "utf8") for content and fixed overhead.
  3. JOIN_SEP — "\n\n---\n\n" is 7 bytes.
  4. Regression test — P2-2b-strict with ~80k / ~50k / ~5k ordering.

Ready for another look.

@CL-BAF

CL-BAF commented Oct 22, 2026

Copy link
Copy Markdown
Owner

Thanks for following up and addressing the earlier review. I checked f5ed190; the FIFO change now behaves correctly, and all 20 spawn-delivery tests pass. There are still a few things to address before merge:

  1. First-message size accounting is inconsistent. The oversized-head check excludes JOIN_SEP, but canDeliver adds those seven bytes even for the first message. This creates a boundary where the head passes the initial check, nothing drains, and delivery returns "no pending messages" despite a pending message remaining. I reproduced this at the default 120,000-byte budget using "é".repeat(59723) + "x" with the existing test fixture. Please use consistent accounting, add separators only between messages, and cover this boundary with a regression test.
  2. The strict FIFO regression test does not catch the original bug. I temporarily restored the old continue behaviour in a disposable copy, and all 20 tests still passed—including P2-2b-strict. The assertions allow the 80k and 5k messages to be delivered while the 50k message stays queued. Please assert that only the first message is delivered, the remaining queue contains the exact second and third message IDs in order, and neither later message appears in the spawned payload.
  3. Updating onto current main introduces a channel-budget interaction. In the automatically merged engine, the sizing callback consumes argv capacity before the channel’s delivery-budget check runs. An 80k message from an exhausted channel can therefore consume capacity without being selected, blocking an otherwise eligible 50k message from another channel. Please check channel eligibility before consuming the batch’s argv allowance.
    When resolving the conflicts, please preserve current main’s actual framed-message sizing through spawnArgumentCost, recipient reservation before asynchronous work, framing inside the state lock, and persistence of queue cleanup even when nothing is delivered. In particular, ensure framed is still constructed—the merge preview can lose that assignment if the PR side of the conflicting block is accepted wholesale.

After updating, please rerun the typecheck and the spawn-delivery, engine, delivery-budgets, and linked-delivery-cleanup tests.

Thanks again for working on this. Once those points are addressed, I’ll take another look.

Merge upstream main. Charge JOIN only between framed messages; refuse
oversized head without draining. Channel lifetime budget is checked
before argv canDeliver so exhausted channels cannot block other mail.
Strengthen P2-2b-strict to assert payload and exact remaining queue ids.
@wized2

wized2 commented Oct 22, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed follow-up — addressed on the latest push:

  1. Consistent first-message accounting — head oversized check and canDeliver both use spawnArgumentCost(formatUntrustedMessage(...)). JOIN (\n\n---\n\n) is charged only between messages, never on the first. Boundary covered by P2-2 boundary: first-message JOIN_SEP must not be charged (UTF-8).

  2. Strict FIFO regression — P2-2b-strict now asserts: only the first message is delivered, spawned payload matches m0: only (not m1:/m2:), and the remaining queue is exactly [secondId, thirdId] in order.

  3. Channel eligibility before argv — in drainQueue, budgetRefusal(channel) runs before canDeliver, so an exhausted channel cannot consume the batch argv allowance and block another channel’s eligible mail. Argv rejection still stops the drain (strict FIFO).

Also merged current main and preserved framed construction inside the lock, spawnArgumentCost, recipient reservation via deliverViaSpawn wrapper, and queue persistence when cleanup runs with an empty drain.

Ready for another look when you have a moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix spawn_push delivery stalling when queued batch exceeds argv limit

2 participants