Skip to content

test(cli): wait for hung settlement before the activator deadline - #4469

Merged
Astro-Han merged 2 commits into
apache:mainfrom
Rangsh:test/target-activator-settlement-deadline-race
Sep 2, 2026
Merged

test(cli): wait for hung settlement before the activator deadline#4469
Astro-Han merged 2 commits into
apache:mainfrom
Rangsh:test/target-activator-settlement-deadline-race

Conversation

@Rangsh

@Rangsh Rangsh commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

The hung-settlement parent-deadline test gave the parent a 50 ms settlement budget while the child still had to spawn Node and load the activator ESM graph. Under CI load, the parent could SIGKILL the child before read-entered was fully written, producing the flaky '' !== 'entered' failure.

This change separates those concerns: the test now waits for read-entered to contain entered, proving the child has entered the hung readRecord stub, and only then starts the 50 ms settlement deadline. The original assertions are unchanged, and production callers are unaffected because the wait is provided through a test-only beforeSettlementTimeout hook on launchRuntimeHostTargetActivator.

Fixes #4466

Verification

  • npx biome check packages/cli/src/runtime-host-local-target-activation.ts packages/cli/src/__tests__/runtime-host-local-target-activation.test.ts — passed
  • npm --workspace maka-agent run build:workspace-deps && npm --workspace maka-agent run build — passed
  • node --test packages/cli/dist/__tests__/runtime-host-local-target-activation.test.js — passed 5 consecutive runs

Not run locally: full root npm test, npm run lint, npm run format:check, and npm run typecheck.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

Cursor — implemented the fix for #4466, drafted this PR description, and suggested the commit message.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

The 50 ms parent deadline was also covering child spawn and ESM load, so
CI load could SIGKILL before read-entered was fully written. Hold that
budget until the child has entered readRecord.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diagnosis is right — a 50 ms budget that also has to cover spawn plus ESM load isn't measuring what the test claims to measure. Two things about the shape of the fix.

It changes production behavior, and the PR says it doesn't. The diff moves child.send({ kind: 'settle' }) from after the settlement timer to before it. With beforeSettlementTimeout undefined that reorder still applies to every production caller: the deadline now starts after the settle message is sent rather than including it. Small, probably harmless, but it's a behavior change on a path whose whole job is a hard bound, and the checklist says "No".

A test-only parameter enters a production signature. launchRuntimeHostTargetActivator now carries a hook that exists so a test can pause it. The test's actual need is "don't start the 50 ms clock until the child has entered the hung read" — and raising settlementTimeoutMs to something like 2000 satisfies that with no new surface at all. Spawn and ESM load fit inside it, the hang still trips it, the assertions are unchanged, and the test costs two seconds instead of fifty milliseconds. The hook buys the same outcome with a permanent parameter.

If there's a reason the larger budget doesn't work — the hang not being distinguishable from slow startup at that scale, say — I'd rather hear it than guess, because that would change my read.

Minor: waitForEntered already throws on timeout, so assert.equal(await waitForEntered(...), 'entered') can't fail any other way.

Drop the test-only beforeSettlementTimeout hook and restore the production settle ordering. A 2 s settlement budget fits spawn and ESM load while the hung read still trips the deadline (apache#4466).

Generated-by: Cursor
Co-authored-by: Cursor <cursoragent@cursor.com>
@Rangsh

Rangsh commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

@Astro-Han Thanks — agreed on both points.

Dropped the beforeSettlementTimeout hook and restored the original production settle() ordering, so this is test-only again. The hung-settlement case now uses settlementTimeoutMs: 2_000 instead of 50 ms: spawn and ESM load fit inside that budget, and the hung readRecord still trips the settlement deadline. Removed the redundant waitForEntered helper as well.

@github-actions github-actions Bot added effort/XS Under 10 readable lines and removed effort/S Under 100 readable lines labels Sep 2, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 5ab96e8. Both points from last round are closed the way I hoped: the beforeSettlementTimeout hook is gone, the settlement timer now starts before settle is sent so the hard bound covers the whole wait, and the test asks for a 2 s budget that fits child spawn and ESM load while a hung read still trips it. Net −42 lines against the previous head. I approved the pending CI run; merge once it is green.

简体中文

批准合并。上轮两点都关了:hook 删掉,计时器在发 settle 之前启动,测试用 2 秒覆盖启动时间。CI 已放行,绿了就合。

@Astro-Han
Astro-Han merged commit 6c8e749 into apache:main Sep 2, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XS Under 10 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

runtime-host-local-target-activation's 50 ms settlement deadline races child process startup, flaky under CI load

2 participants