History

Accept the rare budget-yield launch reversal instead of suppressing yields

Rhys Sullivan committed 2 months ago

Commit b8edd4d1 parent 8c7833e2 files · +62 −67Browse files

Unified · Split

2 files
  1. packages/core/sdk/src/executor.test.ts +26 −29
  2. packages/core/sdk/src/executor.ts +36 −38
packages/core/sdk/src/executor.test.ts+26−29
@@ -980,31 +980,32 @@ const recordCredentialLaunch = (maxOps?: number) => · 979 unmodified lines
980980 return recorder;
981981 });
982982
983−/** Assert `start:<dominant>` is present and precedes every `start:<key>` in
984− * `speculative`, carrying the budget and full event log into any failure. */
985−const expectDominantFirst = (
983+/** Assert every armed read genuinely ran to completion — `start:` and
984+ * `end:` present for each key — carrying the budget and full event log into
985+ * any failure. Launch ORDER is deliberately not asserted here: under an
986+ * adversarial op budget the child's own cooperative yield can park the
987+ * dominant read's inline launch, letting a speculative read start first.
988+ * That reversal is accepted (see the launch-order comment in executor.ts —
989+ * it costs no wall time on any backend, and the only way to suppress it,
990+ * a fiber-lifetime `PreventSchedulerYield`, provably disables effect
991+ * timeouts across provider code). What must hold at EVERY budget is that
992+ * the call completes correctly and no read is lost or stranded. */
993+const expectAllReadsRan = (
986994 recorder: ReturnType<typeof makeReadOrderRecorder>,
987995 budget: number | undefined,
988− dominant: string,
989− speculative: readonly string[],
996+ keys: readonly string[],
990997 ) => {
991− const dominantStart = recorder.at(`start:${dominant}`);
992− const launchedFirst =
993− dominantStart >= 0 && speculative.every((key) => dominantStart < recorder.at(`start:${key}`));
994− expect({ budget, launchedFirst, log: recorder.log() }).toMatchObject({
995− budget,
996− launchedFirst: true,
997− });
998+ const ran = keys.every(
999+ (key) => recorder.at(`start:${key}`) >= 0 && recorder.at(`end:${key}`) >= 0,
1000+ );
1001+ expect({ budget, ran, log: recorder.log() }).toMatchObject({ budget, ran: true });
9981002 };
9991003
1000−// The reproduced defect landed the run loop's op-budget yield between the
1001−// speculative forks and the dominant read's launch, which parked the parent's
1002−// continuation BEHIND the already-scheduled children and reversed the launch
1003−// order. Sweeping low budgets lands that yield at every position in the
1004−// window; 6 and 8 are the two positions where runtime probes reproduced the
1005−// reversal against the pre-fix code. Budgets 1 and 2 are excluded because
1006−// they deadlock the effect run loop itself (a resumed fiber re-yields before
1007−// evaluating a single operation), independent of this code.
1004+// Sweeping low budgets lands the run loop's cooperative yield at every
1005+// position in the launch window (6 and 8 are where runtime probes reproduced
1006+// launch-order reversals). Budgets 1 and 2 are excluded because they deadlock
1007+// the effect run loop itself (a resumed fiber re-yields before evaluating a
1008+// single operation), independent of this code.
10081009 const ADVERSARIAL_BUDGETS: readonly number[] = [3, 4, 5, 6, 7, 8, 9, 10, 11, 12];
10091010
10101011 describe("execute read concurrency", () => {
@@ -1057,17 +1058,13 @@ describe("execute read concurrency", () => { · 46 unmodified lines
10571058 );
10581059
10591060 it.effect(
1060− "launches the tool-row read first at every adversarial scheduler budget",
1061+ "completes with every pre-approval read run at every adversarial scheduler budget",
10611062 () =>
10621063 Effect.gen(function* () {
1063− // Only the launch-order half of the contract holds under an
1064− // adversarial budget: with a handful of ops per tick the speculative
1065− // children legitimately cannot reach their reads within the one
1066− // deferred tick the recorder grants the dominant read, so the
1067− // overlap assertions belong to the default-budget test above.
10681064 for (const budget of ADVERSARIAL_BUDGETS) {
10691065 const recorder = yield* recordToolRowLaunch(budget);
1070− expectDominantFirst(recorder, budget, "tool.findFirst", [
1066+ expectAllReadsRan(recorder, budget, [
1067+ "tool.findFirst",
10711068 "tool_policy.findMany",
10721069 "connection.findFirst",
10731070 ]);
@@ -1077,12 +1074,12 @@ describe("execute read concurrency", () => { · 3 unmodified lines
10771074 );
10781075
10791076 it.effect(
1080− "starts credential resolution first at every adversarial scheduler budget",
1077+ "completes with the credential and integration reads run at every adversarial budget",
10811078 () =>
10821079 Effect.gen(function* () {
10831080 for (const budget of ADVERSARIAL_BUDGETS) {
10841081 const recorder = yield* recordCredentialLaunch(budget);
1085− expectDominantFirst(recorder, budget, "credential.get", ["integration.findFirst"]);
1082+ expectAllReadsRan(recorder, budget, ["credential.get", "integration.findFirst"]);
10861083 }
10871084 }),
10881085 { timeout: 60_000 },
packages/core/sdk/src/executor.ts+36−38
@@ -10,7 +10,6 @@ import { · 9 unmodified lines
1010 Option,
1111 Predicate,
1212 Ref,
13− Scheduler,
1413 Schema,
1514 Semaphore,
1615 } from "effect";
@@ -5817,28 +5816,23 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 5800 unmodified lines
58175816 // handler at construction), so an abandoned read cannot
58185817 // unhandled-reject; a fiber interrupted before it ever ran issues no
58195818 // read at all.
5820− // Launch order is STRUCTURAL, not scheduling-dependent. The dominant
5821− // tool-row read is forked FIRST with `startImmediately: true` under
5822− // `PreventSchedulerYield`: an immediate fork evaluates the child
5823− // INLINE (`forkUnsafe` calls `child.evaluate` on the spot), and with
5824− // the yield-prevention ref inherited at fork the child's run loop
5825− // cannot insert a cooperative `yieldNow` mid-launch, so the child
5826− // runs synchronously until its first genuine suspension — the
5827− // driver-promise await, i.e. the read is ISSUED before the fork
5828− // returns. The speculative reads are then forked plainly, which only
5829− // schedules them on the fiber's dispatcher for the next tick. That
5830− // makes dominant-before-speculative unconditional: without the
5831− // guard, the fiber's operation budget (`MaxOpsBeforeYield`) can
5832− // expire anywhere in this window, and the parked continuation queues
5833− // BEHIND already-scheduled children — plain-forking the dominant (or
5834− // forking it immediately but without `PreventSchedulerYield`) lets
5835− // the speculative reads launch first whenever that boundary lands
5836− // here. The cost of the ref is only that this one bounded read (one
5837− // projected row) never cooperatively yields; the speculative
5838− // children are forked OUTSIDE the provide, so their scheduling is
5839− // untouched, and the dispatcher runs them whether or not this fiber
5840− // ever suspends, so a synchronous parent path delays them by one
5841− // tick but cannot starve them.
5819+ // Launch order is dominant-first by construction, best-effort under
5820+ // the scheduler. The dominant tool-row read is forked FIRST with
5821+ // `startImmediately: true`: an immediate fork evaluates the child
5822+ // INLINE (`forkUnsafe` calls `child.evaluate` on the spot), so the
5823+ // read is normally issued before the speculative forks below are
5824+ // even scheduled (a plain fork only queues its child on the
5825+ // dispatcher for the next tick). The one exception is deliberate: if
5826+ // the child's own operation budget (`MaxOpsBeforeYield`) expires
5827+ // inside the launch window, the inline segment parks and a
5828+ // speculative read may launch first. That reversal is materially
5829+ // free, so it is NOT suppressed: on a synchronous driver the reads
5830+ // serialize whichever order they start (same wall time), and on an
5831+ // asynchronous driver every read suspends at its first await and
5832+ // they overlap regardless of order. Suppressing the yield (a
5833+ // fiber-lifetime `PreventSchedulerYield`) is NOT an option here —
5834+ // the ref is inherited by everything the child runs and provably
5835+ // keeps effect timeouts from firing across CPU-bound stretches.
58425836 const toolRowFiber = yield* Effect.forkChild(
58435837 core.findFirst("tool", {
58445838 where: (b: AnyCb) =>
@@ -5851,7 +5845,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 6 unmodified lines
58515845 select: TOOL_INVOCATION_COLUMNS,
58525846 }),
58535847 { startImmediately: true },
5854− ).pipe(Effect.provideService(Scheduler.PreventSchedulerYield, true));
5848+ );
58555849 const policyRulesFiber = yield* Effect.forkChild(listActivePolicyRuleSet());
58565850 const connectionRowFiber = yield* Effect.forkChild(
58575851 findConnectionRow({
@@ -5957,22 +5951,26 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 99 unmodified lines
59575951 // the two run concurrently. Both start only after
59585952 // `enforceApproval` above completes — a declined call must never
59595953 // trigger the token refresh credential resolution can perform.
5960− // Credential resolution is the dominant work here and must launch
5961− // first, so it uses the same structural launch-order shape as the
5962− // pre-approval forks above: an immediate fork under
5963− // `PreventSchedulerYield` issues its first I/O inline before the
5964− // plain integration-row fork is even scheduled, no matter where
5965− // the fiber's operation budget expires. The integration fork
5966− // is joined after `values`, so a credential resolution failure
5967− // keeps dominating a storage failure exactly as it did when the
5968− // reads were sequential (`Fiber.join` resumes with the credential
5969− // fiber's exact Exit); on that failure path the integration fork
5970− // is interrupted rather than joined, so a hung integration read
5971− // cannot gate the credential error and a failed one is deliberately
5972− // abandoned, never silently dropped as an unobserved value.
5954+ // Credential resolution is the dominant work here and launches
5955+ // first: an immediate fork evaluates it inline to its first
5956+ // suspension before the plain integration-row fork is even
5957+ // scheduled (best-effort — see the launch-order comment at the
5958+ // pre-approval forks for why the rare budget-yield reversal is
5959+ // accepted and why suppressing it is off the table: credential
5960+ // resolution runs extension-owned provider code, and a
5961+ // fiber-lifetime yield guard would disable the provider-call
5962+ // timeout across it and leak into the detached refresh fork). The
5963+ // integration fork is joined after `values`, so a credential
5964+ // resolution failure keeps dominating a storage failure exactly as
5965+ // it did when the reads were sequential (`Fiber.join` resumes with
5966+ // the credential fiber's exact Exit); on that failure path the
5967+ // integration fork is interrupted rather than joined, so a hung
5968+ // integration read cannot gate the credential error and a failed
5969+ // one is deliberately abandoned, never silently dropped as an
5970+ // unobserved value.
59735971 const valuesFiber = yield* Effect.forkChild(resolveConnectionValues(connectionRow), {
59745972 startImmediately: true,
5975− }).pipe(Effect.provideService(Scheduler.PreventSchedulerYield, true));
5973+ });
59765974 const integrationRowFiber = yield* Effect.forkChild(
59775975 findIntegrationRow(parsed.integration),
59785976 );