feat(taskferry): configurable maxResponseBytes ceiling (#506) - #540
jeremysball wants to merge 2 commits into
Conversation
| * @param {string[]} argv | ||
| * @returns {number|undefined} | ||
| */ | ||
| function parseMaxResponseBytesFlag(argv) { |
There was a problem hiding this comment.
we already parse flags at daemon's cli entrypoint? we should not be duplicating that by rolling our own argv parser. if you need a way to get at said parsed flags we can work on that. otherwise this is just bad
There was a problem hiding this comment.
Pushing back a bit here — main() is the arg parser right now, there's no other flag parsing at the daemon entrypoint to reuse. On main (src/daemon.js:1050) it's just:\n\njs\nasync function main() {\n const daemon = await startDaemon({ taskManagerOptions: { config: loadConfig() } });\n}\n\n\nNo commander/yargs, no shared CLI helper. So this PR's parseMaxResponseBytesFlag(process.argv.slice(2)) isn't duplicating — it's the first time the daemon has any CLI flag at all.\n\nI agree hand-rolling a second argv loop inside resolveDaemonOptions would be bad, but that's not what landed — main() parses argv once and threads maxResponseBytes into startDaemon() as an option, which is the shape we actually want (tests call startDaemon({ maxResponseBytes: ... }) without argv).\n\nIf you want a shared helper, I'd extract parseMaxResponseBytesFlag into src/cli.js / src/numbers.js and have both src/daemon.js and any future CLI entry import it, rather than pretending a daemon CLI framework already exists to piggyback on. Clean to do, but adding commander for one flag feels like overkill — keeping the tiny strict parser with explicit error messages (must be positive integer) is more honest. Happy to refactor if you have a specific shared parser in mind.
| const configForMax = /** @type {Record<string, unknown>|undefined} */ ( | ||
| /** @type {any} */ (merged.taskManagerOptions)?.config ?? /** @type {any} */ (merged).config | ||
| ); | ||
| const maxOutboundBytes = resolveMaxResponseBytes( |
There was a problem hiding this comment.
surely we can create an abstraction that does triplet (env var, config, flag) checks for us?
There was a problem hiding this comment.
Not quite 1:1 — they look similar but have different failure semantics, which is why I didn't just import it.\n\ntasks.js:4241 resolvePositiveIntOption is lenient:\njs\nfunction resolvePositiveIntOption(raw, env, config, def) {\n if (raw !== undefined) return raw;\n return positiveInteger(Number(env), positiveInteger(config, def));\n}\n\nInvalid env silently falls back to config/default via positiveInteger(..., fallback).\n\nDaemon's resolveMaxResponseBytes is strict — invalid TASKFERRY_MAX_RESPONSE_BYTES throws with error: ... must be positive integer / help: ... so a typo in env fails fast at daemon startup instead of silently ignoring the cap you thought you set. That's intentional for a wire-level limit where silent fallback hides misconfig.\n\nI agree we shouldn't maintain two copies. The fix is to make resolvePositiveIntOption take a { strictEnv: true } option (or extract a resolvePositiveIntOptionStrict into src/numbers.js) so both callers share the flag>env>config>default chain and the error-handling choice. Just aliasing one to the other would quietly change daemon startup from fail-fast to silent-fallback, which I think is the wrong tradeoff here. I'll wire that strict/shared helper on next push.
| // construction (512 KiB > 1 byte would throw). A raised production cap | ||
| // (>=1 MiB) is the only case where the manager needs to know the wire cap. | ||
| const taskManagerOptions = /** @type {Record<string, any>} */ (merged.taskManagerOptions); | ||
| if (maxOutboundBytes >= MAX_BUFFER_BYTES && taskManagerOptions.maxResponseBytes === undefined && taskManagerOptions.maxBufferBytes === undefined && taskManagerOptions.maxOutboundBytes === undefined) { |
There was a problem hiding this comment.
you say here that we shouldn't thread tiny test caps but I don't see logic to do that. unless you're saying implicitly that tests that do set the cap also set taskManagerOptions fields
There was a problem hiding this comment.
You're right the comment buries the lead — but the logic is there and it's load-bearing, worth walking through why it has to be this shape.\n\njs\n// src/daemon.js:923-926 (this PR)\nif (maxOutboundBytes >= MAX_BUFFER_BYTES && taskManagerOptions.maxResponseBytes === undefined ...) {\n taskManagerOptions.maxResponseBytes = maxOutboundBytes;\n}\n\nDowngrade tests spin up an in-process daemon with { maxOutboundBytes: 1 } / 200 / 512 to assert the wire degrades to RESPONSE_TOO_LARGE. If we threaded that 1-byte cap into the manager, its own constructor would immediately throw:\n\njs\n// src/tasks.js:4346\nif (maxOutputFileBytes (default 512 KiB) > maxResponseBytes (1 byte)) throw ...\n\n\nSo tiny caps must not propagate — only a raised production cap (>= 1 MiB, the default) should. The other half of the guard (taskManagerOptions.maxResponseBytes === undefined ...) respects your point: when a test explicitly sets taskManagerOptions it shouldn't be overwritten implicitly, even for large caps.\n\nThe current comment says "don't thread tiny test caps" but doesn't point to the >= MAX_BUFFER_BYTES line that actually implements it, so it reads as aspirational. I'll make it explicit: "only thread when >=1 MiB, otherwise downgrade tests with artificially tiny wire caps would fail manager construction" and cite the maxOutputFileBytes > maxResponseBytes check.
| // No static MAX_BUFFER_BYTES clamp here: the caller (tasks.js) validates | ||
| // maxBytes against the dynamic daemon response limit (TASKFERRY_MAX_RESPONSE_BYTES | ||
| // / maxResponseBytes flag/config/env, default 1 MiB). Clamping to the static | ||
| // 1 MiB here would silently undo a raised cap and break taskferry#506's |
There was a problem hiding this comment.
this comment not needed
There was a problem hiding this comment.
Disagree here — that comment is the only thing stopping a future re-introduction of the clamp.\n\nOn main (src/output-dir.js:505) we did:\njs\nif (maxBytes > MAX_BUFFER_BYTES) maxBytes = MAX_BUFFER_BYTES;\n\nThat silently re-capped any raised TASKFERRY_MAX_RESPONSE_BYTES back to 1 MiB. This PR removes it and adds:\njs\n// No static MAX_BUFFER_BYTES clamp here: the caller (tasks.js) validates\n// maxBytes against the dynamic daemon response limit ... Clamping to the\n// static 1 MiB here would silently undo a raised cap and break taskferry#506's\n// configurable ceiling.\n\nWithout that note, the next person seeing an unbounded maxBytes flowing into readTaskOutputFile will reasonably re-add the clamp as a "safety" fix. tasks.js:4346 does validate, but the reader of output-dir.js doesn't have that context — this file is imported in other paths too.\n\nI can shorten it to one line (// Caller validates against dynamic cap; don't re-clamp to static 1 MiB (see taskferry#506)) but I'd keep the intent. Want it trimmed vs removed?
- move --max-response-bytes argv parsing and flag>env>config>default chain into src/numbers.js (parsePositiveIntFlag, resolvePositiveIntOption with strictEnv) so daemon no longer carries a bespoke 40-line copy of the same logic - daemon now uses strictEnv throw-on-bad-env for TASKFERRY_MAX_RESPONSE_BYTES while tasks stays lenient via the same helper (default), preserving flag>env>config>default precedence and empty-string fallthrough used by all other numeric options Fixes review comments on #540: hand-rolled argv loop and duplicated positive-int chain.
Stacked on feat/extract-option-helpers stack. Closes #506 Makes 1 MiB response cap configurable: --max-response-bytes flag, TASKFERRY_MAX_RESPONSE_BYTES env, maxResponseBytes config (flag > env > config > 1 MiB). Threads cap into daemon/client/task manager via shared src/options.js helpers (parsePositiveIntFlag, resolvePositiveIntOption with strictEnv:true for daemon, lenient for tasks); maxOutputFileBytes validated against dynamic cap. On top of extraction/consumption stack so daemon no longer carries bespoke flag/env parsers — uses the shared strictEnv path.
cedf351 to
764f3b7
Compare
🤖 Automated review (taskferry)Effort: medium · Model(s): meta/muse-spark-1.3-contributor · 2026-09-03T19:30-0400 · Angle: line-by-line (A) · Verified: 6/6 CONFIRMED
|
🤖 Automated review (taskferry)Effort: medium · Model(s): meta/muse-spark-1.3-contributor · 2026-09-03T19:30-0400 · Angle: cross-file tracer (C) · Verified: 4/6 (2 REFUTED) Kept: |
🤖 Automated review (taskferry)Effort: medium · Model(s): meta/muse-spark-1.3-contributor · 2026-09-03T19:30-0400 · Angle: wrapper/proxy (E) · Verified: 1/3 (2 REFUTED) Kept: |
🤖 Automated review (taskferry)Effort: medium · Model(s): meta/muse-spark-1.3-contributor · 2026-09-03T19:30-0400 · Angles: reuse + simplification · Verified: 0 kept (all REFUTED) All duplication findings in these two angles were voted style-only or factually wrong on current inputs: triplicated fallback (identical expression, no divergent behavior), 4× ceiling formula (identical output everywhere today — future-edit DRY risk, not a bug), hand-rolled client chain (both sides throw identically on bad input), duplicate constants (actually an alias, |
🤖 Automated review (taskferry)Effort: medium · Model(s): meta/muse-spark-1.3-contributor · 2026-09-03T19:30-0400 · Angle: efficiency · Verified: 2 kept, 1 PLAUSIBLE, 3 REFUTED Kept: |
🤖 Automated review (taskferry)Effort: medium · Model(s): meta/muse-spark-1.3-contributor · 2026-09-03T19:30-0400 · Angles: altitude + asymmetry + conventions · Verified: 5 kept Kept: single knob drives inbound+outbound with 1MiB floor (CONFIRMED); stale static cap text |
There was a problem hiding this comment.
Ferry review, medium effort, 9 finder angles + 30 per-candidate verifier votes, all on meta/muse-spark-1.3-contributor. 19 survived; top 9 by severity (medium cap):
- src/daemon.js:840 (CONFIRMED): resolveDaemonOptions mutates shared DAEMON_DEFAULTS.taskManagerOptions — a raised cap leaks into later daemons in-process.
- src/tasks.js:4297 vs daemon.js:820 (CONFIRMED): alias precedence order differs between manager and wire caps, so the two ceilings can disagree.
- src/daemon.js:819 (CONFIRMED): 1MiB default shadows env/config — a raised maxResponseBytes via env/config never takes effect on the daemon.
- src/daemon.js:1004 (CONFIRMED): lowering below 1MiB splits the ceiling three ways (wire shrinks, inbound+manager stay).
- src/tasks.js:4296 (CONFIRMED): small explicit caps throw at construction against the default maxOutputFileBytes.
- src/client.js:640 (CONFIRMED): programmatic client raise never forwarded to auto-started daemon.
- src/daemon.js:292 (CONFIRMED): source-change self-restart drops a flag-only raise.
- src/tasks.js:6814 (CONFIRMED): safeCap goes negative for tiny caps, nonsense help text.
- src/options.js:13 (CONFIRMED): trailing bare flag silently ignored.
Cut by cap but kept on record in angle comments: client validation bypass, throwing-precondition change, per-connect loadConfig, double-encode measure, single-knob inbound/outbound, stale static cap text, CLI flag gap, missing bugs-that-aren't entry.
11 candidates refuted (documented per-angle). Stacking note: base is feat/extract-option-helpers-consume — merge waits on that branch regardless.
- daemon: read cap from raw options (default no longer shadows env/config) - daemon: clone taskManagerOptions (no shared-defaults mutation leak) - daemon: forward resolved cap to self-restart child env (flag-only raise survives) - client: forward programmatic raise to auto-started daemon via env copy - tasks: clamp safeCap at 0, unset output-file budget clamps instead of throwing - options: bare trailing flag throws with usage instead of silent ignore 1406/1406 tests pass.
1 participant
Closes #506
Makes 1 MiB response cap configurable: --max-response-bytes flag, TASKFERRY_MAX_RESPONSE_BYTES env, maxResponseBytes config (flag > env > config > 1 MiB). Threads cap into daemon/client/task manager; maxOutputFileBytes validated against dynamic cap.
Known check failure: tasks.sandbox.test.js UV_CACHE_DIR test (pre-existing, not this change).
Test: docs/config updated, daemon-server + tasks wired, 1405/1406 pass except that one.