Skip to content

Commit ce8b3d3

Browse files
Sean SmithSean Smith
authored andcommitted
fix(core): disposition a lifetime whose last reserved extension is refused
`reserve` raises `pending` for a supplemental sequence; if that sequence then loses admission, the former `unreserve` returned `pending` alone. When the owner sequence had already settled successfully -- settle's `pending > 0` branch correctly keeping the lifetime alive -- the refusal dropped `pending` to 0 with nothing left running to call `settle` again. The lifetime sat `state: "armed"`, `status: "running"`, `pending: 0`, permanently. Three consequences, all reproduced before fixing: - A blocked SYNCHRONOUS caller hangs forever: `done` is resolved only in the disposition branch. - The owner's completed answer is silently DISCARDED: moving the buffered answer into the terminal Info is itself a disposition step. - No terminal ever reaches a caller parked on `waitHandle`. The task tool parks there to release a child session's attachment scope, so that child stays registered and every later resume dies on the coordinator's exclusive open -- this strand could permanently recreate the bug fixed two commits ago. Fix: the refusal settles as a sequence with NO outcome, `settle(id, token, Exit.succeed(undefined))`. `undefined` is already a legal SequenceOutcome, so nothing files, an already-successful owner answer still disposes through its inline slot, and the status is `completed`. Correct rather than convenient: the FOLLOW-UP was refused and its own caller learns so from its own result, while the run's own work demonstrably succeeded -- marking the job `error` would emit an error envelope for a lifetime that did not fail. No note is attached; the caller already learns of the refusal, and a second report would be a duplicate notice path. `settleAdmissibility` handles the hazards for free: a replaced lifetime is `foreign_token`, an already-terminal one `not_running`, and a concurrent second refusal `not_running` -- each a no-op. With other sequences still registered, settle's `pending > 0` branch keeps the lifetime alive exactly as the bare decrement did. `settle`'s `sequence` parameter was dead after the previous commit removed `ledger.announced`, and `fork`'s existed only to forward it; both removed. Not cosmetic: routing a sequence-LESS event through `settle` is correct only because `settle` ignores `sequence`, so removing it makes that safety structural rather than incidental. Blast radius: the fix creates no new state -- it routes an existing path into an already-handled state (terminal/completed) instead of an unreachable one (running at pending 0). No caller relies on `extended: false` leaving the lifetime alive. No reentrancy at `binder.terminal`: settle releases the registry lock before calling it and `bind` has fully returned. Tests: two guards, both verified to fail against the previous `unreserve` behaviour and pass with the fix. The first pins disposition, the preserved owner answer and the released synchronous caller in one combined assertion, so a failing run reports every consequence at once. The second pins the `waitHandle` waiter specifically, since that is the path the attachment-scope release depends on. Both use bounded races rather than unbounded waits, so a regression fails loudly instead of hanging the suite. background-job goes 22 -> 24. Ported from the ACF fork of opencode.
1 parent aeb6699 commit ce8b3d3

2 files changed

Lines changed: 164 additions & 21 deletions

File tree

‎packages/core/src/background-job.ts‎

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -591,7 +591,7 @@ export const makeWith = (binder: Binder) =>
591591
})
592592

593593
const settle = Effect.fn("BackgroundJob.settle")(
594-
function* (id: string, token: object, sequence: number, exit: Exit.Exit<SequenceOutcome, unknown>) {
594+
function* (id: string, token: object, exit: Exit.Exit<SequenceOutcome, unknown>) {
595595
const completed_at = yield* Clock.currentTimeMillis
596596
// Publishing swaps the ledger gate; the fresh gate is made before the lock so the committed
597597
// modification stays pure.
@@ -734,13 +734,12 @@ export const makeWith = (binder: Binder) =>
734734
scope: Scope.Scope,
735735
id: string,
736736
token: object,
737-
sequence: number,
738737
run: Effect.Effect<SequenceOutcome, unknown>,
739738
) {
740739
return yield* run.pipe(
741740
Effect.matchCauseEffect({
742-
onSuccess: (outcome) => settle(id, token, sequence, Exit.succeed(outcome)),
743-
onFailure: (cause) => settle(id, token, sequence, Exit.failCause(cause)),
741+
onSuccess: (outcome) => settle(id, token, Exit.succeed(outcome)),
742+
onFailure: (cause) => settle(id, token, Exit.failCause(cause)),
744743
}),
745744
Effect.asVoid,
746745
Effect.forkIn(scope, { startImmediately: true }),
@@ -786,19 +785,6 @@ export const makeWith = (binder: Binder) =>
786785
return result.info
787786
}, Effect.uninterruptible)
788787

789-
/**
790-
* Releases a reserved-but-unarmed extension coordinate. Pending is returned to its prior value
791-
* so the lifetime can still settle. The sequence number itself is spent - coordinates are
792-
* monotonic and gaps are legitimate.
793-
*/
794-
const unreserve = Effect.fn("BackgroundJob.unreserve")(function* (id: string, token: object) {
795-
yield* SynchronizedRef.update(state.jobs, (jobs) => {
796-
const job = jobs.get(id)
797-
if (!job || job.token !== token) return jobs
798-
return new Map(jobs).set(id, { ...job, pending: Math.max(0, job.pending - 1) })
799-
})
800-
})
801-
802788
const list: Interface["list"] = Effect.fn("BackgroundJob.list")(function* () {
803789
return Array.from((yield* SynchronizedRef.get(state.jobs)).values())
804790
.map(snapshot)
@@ -963,7 +949,7 @@ export const makeWith = (binder: Binder) =>
963949

964950
if (!armed) return yield* restore(Deferred.await(registration.arm))
965951

966-
yield* fork(registration.scope, id, registration.token, 0, restore(input.run))
952+
yield* fork(registration.scope, id, registration.token, restore(input.run))
967953
const outcome: ArmOutcome = { info: armed.info, lifetime, handle: armed.handle }
968954
yield* Deferred.succeed(registration.arm, outcome).pipe(Effect.ignore)
969955
return outcome
@@ -1036,7 +1022,25 @@ export const makeWith = (binder: Binder) =>
10361022
Effect.onExit((exit) =>
10371023
Exit.isSuccess(exit) && exit.value !== undefined
10381024
? Effect.void
1039-
: unreserve(input.lifetime.id, result.token).pipe(Effect.ignore),
1025+
: // This reserved coordinate lost admission, so nothing will ever run for it.
1026+
// Returning `pending` alone left the lifetime `armed`/`running` at `pending: 0`
1027+
// with nothing remaining to settle it whenever the owner sequence had already
1028+
// settled successfully — a permanent strand. It hung any blocked synchronous
1029+
// caller (`done` resolves only at disposition), discarded the owner's completed
1030+
// answer (moving it into the terminal Info is itself a disposition step), and
1031+
// withheld the terminal that releases a child Task session's attachment scope.
1032+
// The refusal therefore settles as a sequence with NO outcome: `undefined` is a
1033+
// legal SequenceOutcome, so nothing files, an already-successful owner answer
1034+
// still disposes through its inline slot, and the status is `completed` — the
1035+
// FOLLOW-UP was refused and its own caller learns that from its own result,
1036+
// while the run's own work demonstrably succeeded; marking the job `error` would
1037+
// emit an error envelope for a lifetime that did not fail.
1038+
// `settleAdmissibility` covers the hazards for free: a replaced lifetime is
1039+
// `foreign_token`, an already-terminal one `not_running`, and a concurrent second
1040+
// refusal `not_running` — each a no-op. With other sequences still registered,
1041+
// settle's `pending > 0` branch keeps the lifetime alive exactly as the bare
1042+
// decrement did.
1043+
settle(input.lifetime.id, result.token, Exit.succeed(undefined)).pipe(Effect.ignore),
10401044
),
10411045
)
10421046

@@ -1045,7 +1049,7 @@ export const makeWith = (binder: Binder) =>
10451049
// An accepted supplemental run registers and runs without waiting for any previous run's
10461050
// tail: the serial hold is gone, so several runs on one lifetime can be in flight at once
10471051
// and their answers are ordered by position at delivery rather than by execution.
1048-
yield* fork(result.scope, input.lifetime.id, result.token, result.sequence, restore(input.run))
1052+
yield* fork(result.scope, input.lifetime.id, result.token, restore(input.run))
10491053
return { extended: true as const, sequence: result.sequence, handle: accepted }
10501054
}),
10511055
)

‎packages/core/test/background-job.test.ts‎

Lines changed: 140 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,33 @@
11
import { describe, expect, test } from "bun:test"
22
import { BackgroundJob } from "@opencode-ai/core/background-job"
33
import { LayerNode } from "@opencode-ai/core/effect/layer-node"
4-
import { Deferred, Effect, Exit, Scope } from "effect"
4+
import { Deferred, Effect, Exit, Fiber, Scope } from "effect"
55
import { it } from "./lib/effect"
66

77
const jobsLayer = LayerNode.compile(BackgroundJob.node)
88

9+
const admission = { lease: "lease_synthetic_core", epoch: 0n }
10+
11+
/**
12+
* Arms sequence zero and refuses every later one, parking inside `bind` first so a test can hold a
13+
* supplemental sequence in the registered-but-unarmed window and release it deliberately.
14+
*/
15+
const refusingBinder = (input: {
16+
readonly entered: Deferred.Deferred<void>
17+
readonly release: Deferred.Deferred<void>
18+
}): BackgroundJob.Binder => ({
19+
bind: (request) =>
20+
request.sequence === 0
21+
? BackgroundJob.makePermit(request.lifetime, request.sequence).pipe(
22+
Effect.map((made) => ({ kind: "arm_allowed" as const, permit: made.permit })),
23+
)
24+
: Deferred.succeed(input.entered, undefined).pipe(
25+
Effect.andThen(Deferred.await(input.release)),
26+
Effect.as({ kind: "rejected" as const, reason: "refused after reserving" }),
27+
),
28+
terminal: () => Effect.void,
29+
})
30+
931
/**
1032
* A run reports the position it answered at plus an opaque payload. Tests use the position string
1133
* as the payload too, so an assertion names one value.
@@ -421,4 +443,121 @@ describe("BackgroundJob", () => {
421443
expect((yield* jobs.get(job.id))?.status).toBe("running")
422444
}),
423445
)
446+
447+
it.live("dispositions when a refused extension returns pending to zero", () =>
448+
Effect.scoped(
449+
Effect.gen(function* () {
450+
// THE STRAND. `reserve` raises `pending` for a supplement that then LOSES admission. The
451+
// owner sequence has already succeeded, so its settle saw `pending > 0` and correctly kept
452+
// the lifetime running. When the refusal returns `pending` to zero there is nothing left
453+
// running to call `settle` again — so without a disposition on this path the lifetime sits
454+
// `running` at `pending: 0` forever, and two things break: a blocked SYNCHRONOUS caller
455+
// never receives its answer (`done` is resolved only at disposition), and the owner's
456+
// completed answer is silently discarded, because moving it into the terminal Info is
457+
// itself a disposition step.
458+
const holdOwner = yield* Deferred.make<void>()
459+
const entered = yield* Deferred.make<void>()
460+
const release = yield* Deferred.make<void>()
461+
const jobs = yield* BackgroundJob.makeWith(refusingBinder({ entered, release }))
462+
const started = yield* jobs.startExact({
463+
admission,
464+
id: "job_unreserve_strand",
465+
type: "test",
466+
run: Deferred.await(holdOwner).pipe(Effect.as({ position: "p_a", at: 1, detected: "owner answer" })),
467+
})
468+
if (!started.lifetime || !started.handle) return yield* Effect.die("did not arm")
469+
470+
// A blocked synchronous caller — the shape a synchronously started task takes.
471+
const blocked = yield* jobs.wait({ id: "job_unreserve_strand" }).pipe(Effect.forkScoped)
472+
473+
// The supplement RESERVES (pending 2) and parks inside bind.
474+
const extension = yield* jobs
475+
.extendWithHandle({ id: "job_unreserve_strand", admission, run: Effect.succeed(undefined) })
476+
.pipe(Effect.forkScoped)
477+
yield* Deferred.await(entered)
478+
479+
// The owner settles successfully inside that window: pending 2 → 1, answer buffered,
480+
// correctly no disposition because a supplement is still registered.
481+
yield* Deferred.succeed(holdOwner, undefined)
482+
yield* Effect.yieldNow
483+
yield* Effect.yieldNow
484+
485+
// The parked bind now REFUSES: pending 1 → 0 with nothing left running.
486+
yield* Deferred.succeed(release, undefined)
487+
expect(yield* Fiber.join(extension)).toBeUndefined()
488+
489+
// The lifetime MUST dispose, and as `completed` rather than `error`/`cancelled`: the
490+
// FOLLOW-UP was refused — its own caller learns that from its own result — while the run's
491+
// own work demonstrably succeeded.
492+
const settled = yield* jobs.wait({ id: "job_unreserve_strand", timeout: 1000 })
493+
// Bounded-negative: before the fix this fiber parks forever.
494+
const delivered = yield* Effect.raceFirst(
495+
Fiber.join(blocked).pipe(Effect.as("released" as const)),
496+
Effect.sleep("500 millis").pipe(Effect.as("hung" as const)),
497+
)
498+
499+
// Gathered before any assertion, and asserted together, so one failing run reports every
500+
// consequence at once rather than stopping at the first.
501+
expect({
502+
status: settled.info?.status,
503+
output: settled.info?.output,
504+
blockedCaller: delivered,
505+
}).toEqual({
506+
status: "completed",
507+
output: "owner answer",
508+
blockedCaller: "released",
509+
})
510+
}),
511+
),
512+
)
513+
514+
it.live("releases the lifetime waiter when a refused extension ends a background job", () =>
515+
Effect.scoped(
516+
Effect.gen(function* () {
517+
// The consequence that motivated the fix. A caller can park on `waitHandle` — that is, on
518+
// the job's TERMINAL — to learn when the lifetime ends; the task tool does exactly this to
519+
// release a child session's attachment scope. While a refused extension could leave the
520+
// lifetime running at `pending: 0`, that terminal never arrived and the waiter was held
521+
// indefinitely.
522+
const holdOwner = yield* Deferred.make<void>()
523+
const entered = yield* Deferred.make<void>()
524+
const release = yield* Deferred.make<void>()
525+
const jobs = yield* BackgroundJob.makeWith(refusingBinder({ entered, release }))
526+
const started = yield* jobs.startExact({
527+
admission,
528+
id: "job_unreserve_observer",
529+
type: "test",
530+
metadata: { background: true },
531+
run: Deferred.await(holdOwner).pipe(Effect.as({ position: "p_a", at: 1, detected: "owner answer" })),
532+
})
533+
if (!started.lifetime || !started.handle) return yield* Effect.die("did not arm")
534+
535+
const waiter = yield* jobs.waitHandle({ handle: started.handle }).pipe(Effect.forkScoped)
536+
537+
const extension = yield* jobs
538+
.extendWithHandle({ id: "job_unreserve_observer", admission, run: Effect.succeed(undefined) })
539+
.pipe(Effect.forkScoped)
540+
yield* Deferred.await(entered)
541+
542+
yield* Deferred.succeed(holdOwner, undefined)
543+
yield* Effect.yieldNow
544+
yield* Effect.yieldNow
545+
546+
yield* Deferred.succeed(release, undefined)
547+
expect(yield* Fiber.join(extension)).toBeUndefined()
548+
549+
// Bounded-negative: before the fix this waiter is held forever.
550+
const released = yield* Effect.raceFirst(
551+
Fiber.join(waiter).pipe(Effect.as("released" as const)),
552+
Effect.sleep("500 millis").pipe(Effect.as("held" as const)),
553+
)
554+
expect(released).toBe("released")
555+
556+
// And the work that DID complete is still delivered: ending the lifetime is not the same as
557+
// discarding the answer the run already produced.
558+
const first = yield* jobs.waitAnswer({ handle: started.handle, after: 0 })
559+
expect(first.answer?.detected).toBe("owner answer")
560+
}),
561+
),
562+
)
424563
})

0 commit comments

Comments
 (0)