Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/ws-stream-writer-session.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@workflow/core': patch
'@workflow/world': minor
---

Add an optional stateful stream writer-session seam with stable writer identity and sequence tracking.
38 changes: 36 additions & 2 deletions packages/core/src/serialization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
WorkflowRuntimeError,
} from '@workflow/errors';
import { once } from '@workflow/utils';
import type { StreamWriteSession } from '@workflow/world';
import { envNumber } from '@workflow/world/env-config';
import { parse, stringify, unflatten } from 'devalue';
import { monotonicFactory } from 'ulid';
Expand Down Expand Up @@ -1310,6 +1311,20 @@ export class WorkflowServerWritableStream extends WritableStream<Uint8Array> {
}
};

// One stable identity and sequence space per in-memory sink. Start session
// construction as soon as the run-ready barrier permits, so transports may
// negotiate eagerly without allowing a write to overtake run creation.
// This eager chain cannot strand a new rejection: ensureRunReady absorbs its
// ordering-only failure, and every path that can observe worldPromise also
// awaits this session promise before it writes, closes, or disposes.
const writerId = `wrtr_${defaultUlid()}` as const;
const writeSessionPromise: Promise<StreamWriteSession | undefined> =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AI Review: Nit

I checked the unhandled-rejection question on this eagerly-constructed promise, since it is created for every writable whether or not a World implements the seam, and it comes out fine: ensureRunReady swallows its own rejection by design ("ordering barrier only"), and the only other rejection source, worldPromise, is already awaited on the same paths that await this. Noting it because the eager .then at construction time is the kind of thing worth a comment saying it cannot strand a rejection, given a stream that is created and then never written, closed, or aborted attaches no handler until it is.

The abort path using dispose() rather than close() is the right call, and the distinction between transport cleanup and semantic completion is well drawn.

ensureRunReady().then(async () => {
const world = await worldPromise;
return world.streams.createWriteSession?.(runId, name, { writerId });
});
let nextChunkSeq = 0;

// ------------------------------------------------------------------
// Group-commit buffering.
//
Expand Down Expand Up @@ -1427,15 +1442,24 @@ export class WorkflowServerWritableStream extends WritableStream<Uint8Array> {
): Promise<void> => {
await ensureRunReady();
const world = await worldPromise;
const session = await writeSessionPromise;
const dispatchAt = Date.now();
if (typeof world.streams.writeMulti === 'function' && group.length > 1) {
if (session) {
await session.write(nextChunkSeq, group);
} else if (
typeof world.streams.writeMulti === 'function' &&
group.length > 1
) {
await world.streams.writeMulti(runId, name, group);
} else {
// Fall back to sequential writes
for (const chunk of group) {
await world.streams.write(runId, name, chunk);
}
}
// `inFlight` admits only one dispatch loop, so no second group can read
// this sequence space until the current group has advanced it.
nextChunkSeq += group.length;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AI Review: Note

nextChunkSeq is read before an await and mutated after it, so its correctness rests entirely on dispatchGroup never running concurrently. That invariant does hold, and it is stated further up in this file ("Exactly one loop runs at a time (inFlight), so groups reach the server in write order"), but it is stated about ordering, several hundred lines away, and nothing near here records that a second dispatch loop would hand two groups the same starting sequence.

Since the whole point of the sequence space is that a transport can rely on it to reconstruct ordering, a one-line comment tying this increment to the inFlight guarantee is worth having. A future change that parallelises dispatch for throughput would silently produce duplicate chunkSeq values rather than fail a test.

if (groupT0 !== undefined) {
recordStreamWriteFlush(
groupT0,
Expand Down Expand Up @@ -1635,8 +1659,13 @@ export class WorkflowServerWritableStream extends WritableStream<Uint8Array> {
await ensureRunReady();

const world = await worldPromise;
const session = await writeSessionPromise;
const closeStart = Date.now();
await world.streams.close(runId, name);
if (session) {
await session.close();
} else {
await world.streams.close(runId, name);
}
recordStreamClose(closeStart, runId, name);
},
async abort(reason) {
Expand Down Expand Up @@ -1671,6 +1700,11 @@ export class WorkflowServerWritableStream extends WritableStream<Uint8Array> {
// Reject blocked writers and drain waiters so nothing leaks or
// hangs on a promise whose timer was just cleared.
rejectWaiters(sinkError);
// Abort is transport cleanup, not semantic stream completion. A
// stateful World releases its socket without sending close; stateless
// Worlds keep the existing no-op behavior.
const session = await writeSessionPromise.catch(() => undefined);
await session?.dispose?.();
},
});

Expand Down
99 changes: 99 additions & 0 deletions packages/core/src/writable-stream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ async function waitFor(

describe('WorkflowServerWritableStream', () => {
let mockStreams: {
createWriteSession?: ReturnType<typeof vi.fn>;
write: ReturnType<typeof vi.fn>;
writeMulti: ReturnType<typeof vi.fn>;
close: ReturnType<typeof vi.fn>;
Expand Down Expand Up @@ -516,6 +517,82 @@ describe('WorkflowServerWritableStream', () => {
});
});

describe('stateful writer sessions', () => {
it('uses one stable writer id and writer-local sequence across groups', async () => {
const session = {
write: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
};
mockStreams.createWriteSession = vi.fn(() => session);

const stream = new WorkflowServerWritableStream('run-123', 'test-stream');
const writer = stream.getWriter();
await writer.write(new Uint8Array([1]));
await waitFor(() => expect(session.write).toHaveBeenCalledTimes(1));
await writer.write(new Uint8Array([2]));
await waitFor(() => expect(session.write).toHaveBeenCalledTimes(2));
await writer.close();

expect(mockStreams.createWriteSession).toHaveBeenCalledTimes(1);
expect(mockStreams.createWriteSession).toHaveBeenCalledWith(
'run-123',
'test-stream',
{
writerId: expect.stringMatching(
/^wrtr_[0123456789ABCDEFGHJKMNPQRSTVWXYZ]{26}$/
),
}
);
expect(session.write).toHaveBeenNthCalledWith(1, 0, [
new Uint8Array([1]),
]);
expect(session.write).toHaveBeenNthCalledWith(2, 1, [
new Uint8Array([2]),
]);
expect(session.close).toHaveBeenCalledTimes(1);
expect(mockStreams.write).not.toHaveBeenCalled();
expect(mockStreams.writeMulti).not.toHaveBeenCalled();
expect(mockStreams.close).not.toHaveBeenCalled();
});

it('creates a new writer id for each in-memory sink lifetime', async () => {
const writerIds: string[] = [];
mockStreams.createWriteSession = vi.fn((_runId, _name, options) => {
writerIds.push(options.writerId);
return {
write: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
};
});

new WorkflowServerWritableStream('run-123', 'test-stream');
new WorkflowServerWritableStream('run-123', 'test-stream');
await waitFor(() => expect(writerIds).toHaveLength(2));
expect(new Set(writerIds).size).toBe(2);
});
});

describe('abort cleanup', () => {
it('drains accepted chunks then disposes without semantic close', async () => {
const session = {
write: vi.fn().mockResolvedValue(undefined),
close: vi.fn().mockResolvedValue(undefined),
dispose: vi.fn().mockResolvedValue(undefined),
};
mockStreams.createWriteSession = vi.fn(() => session);

const stream = new WorkflowServerWritableStream('run-123', 'test-stream');
const writer = stream.getWriter();
await writer.write(new Uint8Array([1]));
await writer.abort(new Error('producer failed'));

expect(session.write).toHaveBeenCalledWith(0, [new Uint8Array([1])]);
expect(session.dispose).toHaveBeenCalledTimes(1);
expect(session.close).not.toHaveBeenCalled();
expect(mockStreams.close).not.toHaveBeenCalled();
});
});

describe('error handling', () => {
it('surfaces a dispatch failure at the durability barrier (close) and poisons later writes', async () => {
mockStreams.write.mockRejectedValueOnce(new Error('write error'));
Expand Down Expand Up @@ -549,6 +626,28 @@ describe('WorkflowServerWritableStream', () => {
expect(mockStreams.write).toHaveBeenCalledTimes(1);
});

it('keeps a session failure sticky without advancing its sequence', async () => {
const session = {
write: vi.fn().mockRejectedValueOnce(new Error('session failed')),
close: vi.fn().mockResolvedValue(undefined),
};
mockStreams.createWriteSession = vi.fn(() => session);

const writer = new WorkflowServerWritableStream(
'run-123',
'test-stream'
).getWriter();
await writer.write(new Uint8Array([1]));
await waitFor(() => expect(session.write).toHaveBeenCalledTimes(1));
await expect(writer.write(new Uint8Array([2]))).rejects.toThrow(
'session failed'
);
await expect(writer.close()).rejects.toThrow();
expect(session.write).toHaveBeenCalledTimes(1);
expect(session.write).toHaveBeenCalledWith(0, [new Uint8Array([1])]);
expect(session.close).not.toHaveBeenCalled();
});

it('should propagate close errors', async () => {
mockStreams.close.mockRejectedValueOnce(new Error('close error'));

Expand Down
30 changes: 30 additions & 0 deletions packages/world/src/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,25 @@ import type {
StepWithoutData,
} from './steps.js';

export interface StreamWriteSession {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AI Review: Nit

The changeset marks @workflow/world as patch, but this adds two newly exported interfaces to the package's public surface. Adding an optional member to Streamer.streams is backward compatible for implementors, so nothing breaks, but new exported types are conventionally a minor. Worth bumping so the surface addition is visible in the changelog rather than folded into a patch line.

/**
* Write one ordered group from this in-memory writer lifetime.
* `chunkSeq` is writer-local and identifies the first chunk in `chunks`.
*/
write(chunkSeq: number, chunks: (string | Uint8Array)[]): Promise<void>;

/** Close this writer lifetime after all prior writes are durable. */
close(): Promise<void>;

/** Release transport resources without semantically closing the stream. */
dispose?(): Promise<void> | void;
}

export interface CreateStreamWriteSessionOptions {
/** Stable observational id for this in-memory writer lifetime. */
writerId: `wrtr_${string}`;
}

export interface Streamer {
/**
* Number of milliseconds a stream waits for additional chunks to arrive
Expand All @@ -57,6 +76,17 @@ export interface Streamer {
streamFlushIntervalMs?: number;

streams: {
/**
* Optionally create a stateful writer session. Core creates at most one
* session per in-memory WritableStream and otherwise uses the stateless
* write/writeMulti/close methods below unchanged.
*/
createWriteSession?(
runId: string,
name: string,
options: CreateStreamWriteSessionOptions
): StreamWriteSession;

write(
runId: string,
name: string,
Expand Down
Loading