Uh oh!
There was an error while loading. Please reload this page.
Always return a Promise from getOutputStreamByRef - #808
Merged
Conversation
`getOutputStreamByRef` and `getOutputStreamByRefForRun` were declared as a
tri-state union — `AsyncIterable<Uint8Array> | undefined | Promise<...>` — which
does not compose with `StreamPortCodec.materialize`, whose parameter is the
iterable alone. Every consumer had to decide for itself whether a given backing
needed awaiting, and the tree had grown two narrowings of the one interface in
opposite directions: the concrete repositories overrode it with the synchronous
half, while the streaming contract helper declared its own Promise-only version.
The union also made the await in `streamRefViaBacking` load-bearing by
convention rather than by type. That await is what turns an asynchronous
backing's dangling ref into `undefined` — a cache miss — instead of a truthy
Promise a caller reads as a live stream, and nothing in the signature required
it. Both production `materialize` call sites funnel through that helper, which
is why nothing was broken; the type just did not say so.
A backing that can answer synchronously still does — the cost is one microtask
per ref, not per chunk.
The test churn is the interface's own evidence. Un-awaited call sites failed
loudly (`bytes is not async iterable`, `expected Promise{…} to be undefined`),
except one: `expect(repo.getOutputStreamByRef!(ref)).toBeDefined()` passed
vacuously, because a Promise is always defined. That assertion would have held
for a dangling ref.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RFf49R1YHc5s8JAAEhRHyjCoverage Report
File Coverage
| ||||||||||||||||||||||||||||||||||||||||||||||||||
`turbo run build-types` does not typecheck task-graph's own `__tests__` — a separate `typecheck:tests` script does, one tsconfig.test.json per package — so ten test doubles kept the old synchronous `getOutputStreamByRef` signature and only CI caught them. `AsyncStreamReader`'s last case is rewritten rather than widened. It asserted that a synchronous backing hands back a non-thenable, and blanket-marking its double `async` turned the test into a contradiction of its own premise. The guarantee worth keeping is the runtime one: the interface now requires a Promise, but `await` is a no-op on a non-thenable, so a JavaScript backing (or one compiled against the older signature) returning a bare iterable still works. The cast is what lets the test reach that shape, and it is the subject rather than a convenience. Also restores the parameter on `ConditionalTask.getCachePolicy`. That override declared zero parameters, which satisfies `ITask` structurally but narrows the arity seen through the CONCRETE type, so a caller holding a `ConditionalTask` could not pass the inputs `TaskRunner` passes through the interface. Verified pre-existing: `typecheck:tests` fails on those two lines with this branch's task-graph source replaced by main's and dist rebuilt from it. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RFf49R1YHc5s8JAAEhRHyj
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TaskOutputRepository.getOutputStreamByRef(andgetOutputStreamByRefForRun) were declared as a tri-state union:StreamPortCodec.materializetakesAsyncIterable<Uint8Array>. The two do not compose, so every consumer had to decide for itself whether a given backing needed awaiting — and the tree had grown two narrowings of the one interface, in opposite directions: the concrete repositories (FsFolderTaskOutputRepository,StreamingMemoryRepo) overrode it with the synchronous half, whilestreamingTaskOutputRepositoryContract.tsdeclared its own Promise-only version.Both are now
Promise<AsyncIterable<Uint8Array> | undefined>.Why it mattered even though nothing was broken
Both production
materializecall sites —CacheCoordinator.tsandTaskRunner.ts— funnel throughstreamRefViaBacking(), which awaits internally. That await was doing double duty: collapsing the union and turning an asynchronous backing's dangling ref intoundefined(a cache miss) rather than a truthy Promise a caller reads as a live stream. Nothing in the signature required it. Now the type does.A backing that can answer synchronously still does; the cost is one microtask per ref, not per chunk.
The test churn is the interface's own evidence
Un-awaited call sites failed loudly once the union collapsed —
TypeError: bytes is not async iterablefromstreamCodec, andexpected Promise{…} to be undefinedfrom nine assertions.One did not fail, and is the point of the change:
A Promise is always defined, so that assertion passed vacuously — it would have held for a dangling ref. It now awaits and tests what it claims to.
Changes
TaskOutputRepository: both declarations collapsed to Promise; JSDoc rewritten (the "synchronous backings return the iterable directly" paragraph is gone).resolveRef.ts:CacheRefStreamResolverandRefStreamBacking.getOutputStreamByReffollow;streamRefViaBacking's explanatory comment about awaiting a maybe-sync value is no longer needed.FsFolderTaskOutputRepositoryandStreamingMemoryRepooverrides markedasync.TabularStreamingTaskOutputRepositoryalready was.CacheStreamOut,InputRefPortGating,TaskOutputRepositoryStream,RunPrivateClearRunCost,RunPrivateFsFolderStream,FsFolderStreamPort.Breaking change
An external implementor must mark the method
async; a caller relying on the synchronous return mustawait. Changelog entry added under## Unreleasedin@workglow/task-graphwith the migration note.Verification
https://claude.ai/code/session_01RFf49R1YHc5s8JAAEhRHyj
Generated by Claude Code