diff --git a/packages/indexeddb/src/job-queue/IndexedDbQueueStorage.ts b/packages/indexeddb/src/job-queue/IndexedDbQueueStorage.ts index aee4a69e6..16011b3ea 100644 --- a/packages/indexeddb/src/job-queue/IndexedDbQueueStorage.ts +++ b/packages/indexeddb/src/job-queue/IndexedDbQueueStorage.ts @@ -14,7 +14,7 @@ import type { } from "@workglow/job-queue"; import { JobStatus, validateLeaseMs } from "@workglow/job-queue"; import { HybridSubscriptionManager } from "@workglow/storage"; -import { createServiceToken, deepEqual, makeFingerprint, uuid4 } from "@workglow/util"; +import { createServiceToken, deepEqual, getLogger, makeFingerprint, uuid4 } from "@workglow/util"; import { IndexedDbMigrationRunner } from "../migrations/IndexedDbMigrationRunner"; import { indexedDbQueueMigrations } from "../migrations/indexedDbQueueMigrations"; import { openIdb } from "../storage/openIdb"; @@ -419,9 +419,10 @@ export class IndexedDbQueueStorage implements IQueueStorage & Record) | undefined; if (!existing || existing.queue !== this.queueName || !this.matchesPrefixes(existing)) { - reject( - new Error(`Job ${job.id} not found or does not belong to queue ${this.queueName}`) - ); + // Contract: complete() must silently no-op (not throw) when the row is + // missing or belongs to another queue. Lease-expiry races can legitimately + // make the target row disappear between claim and complete, so the + // transaction is allowed to commit with no write and resolve. return; } const currentAttempts = existing.attempts ?? 0; @@ -673,7 +674,12 @@ export class IndexedDbQueueStorage implements IQueueStorage | null ): Promise { const job = await this.get(id); - if (!job) throw new Error(`Job ${id} not found`); + if (!job) { + // Contract: progress updates for a missing job silently no-op — the job may + // have already completed/been removed, or a lease-expiry race removed it. + getLogger().warn("Job not found for progress update", { id }); + return; + } job.progress = progress; job.progress_message = message; diff --git a/packages/indexeddb/src/storage/IndexedDbVectorStorage.ts b/packages/indexeddb/src/storage/IndexedDbVectorStorage.ts index 83725ffd0..c0ddc1d2c 100644 --- a/packages/indexeddb/src/storage/IndexedDbVectorStorage.ts +++ b/packages/indexeddb/src/storage/IndexedDbVectorStorage.ts @@ -17,8 +17,10 @@ import { getMetadataProperty, getVectorProperty, matchesFilter, + safeEmit, validateVectorEntities, } from "@workglow/storage"; +import type { EventEmitter } from "@workglow/util"; import { createServiceToken } from "@workglow/util"; import type { DataPortSchemaObject, @@ -130,7 +132,10 @@ export class IndexedDbVectorStorage< options: VectorSearchOptions> = {} ) { assertVectorShape(query, this.vectorDimensions, "query"); - const { topK = 10, filter, scoreThreshold = 0 } = options; + // Default to no floor: cosine similarity ranges over [-1, 1], so a default + // of 0 would silently drop negatively-correlated hits and return fewer than + // `topK`. Callers opt into a relevance floor explicitly via `scoreThreshold`. + const { topK = 10, filter, scoreThreshold = -Infinity } = options; const results: Array = []; const allEntities = (await this.getAll()) || []; @@ -138,9 +143,10 @@ export class IndexedDbVectorStorage< for (const entity of allEntities) { // IndexedDB stores TypedArrays natively via structured clone const vector = entity[this.vectorPropertyName] as TypedArray; - const metadata = this.metadataPropertyName - ? (entity[this.metadataPropertyName] as Metadata) - : ({} as Metadata); + // A present-but-null metadata column value coalesces to `{}` so a filtered + // search treats the row as a non-match rather than dereferencing null. + const rawMetadata = this.metadataPropertyName ? entity[this.metadataPropertyName] : undefined; + const metadata = (rawMetadata ?? {}) as Metadata; if (filter && !matchesFilter(metadata, filter)) { continue; @@ -160,6 +166,19 @@ export class IndexedDbVectorStorage< results.sort((a, b) => b.score - a.score); const topResults = results.slice(0, topK); + // The inherited `events` emitter is typed for the tabular event surface; + // `similaritySearch` lives on the vector extension of that surface. The + // emitter instance is the same object, so widen the view to a record that + // carries the event so it can be emitted type-safely. + type SimilaritySearchEvents = { + similaritySearch: (query: TypedArray, results: (Entity & { score: number })[]) => void; + }; + safeEmit( + this.events as unknown as EventEmitter, + "similaritySearch", + query, + topResults + ); return topResults; } diff --git a/packages/job-queue/src/job/JobErrorRegistry.ts b/packages/job-queue/src/job/JobErrorRegistry.ts index b9f3a8845..06176104e 100644 --- a/packages/job-queue/src/job/JobErrorRegistry.ts +++ b/packages/job-queue/src/job/JobErrorRegistry.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { getLogger } from "@workglow/util"; import { JobError } from "./JobError"; /** @@ -57,9 +58,9 @@ export function registerErrorCodeReconstructor( return; // idempotent — same fn, same prefix } - console.warn( - `registerErrorCodeReconstructor: overwriting existing reconstructor for prefix "${prefix}"` - ); + getLogger().warn("registerErrorCodeReconstructor: overwriting existing reconstructor", { + prefix, + }); reconstructors[existingIdx] = { prefix, reconstructor }; return; } diff --git a/packages/job-queue/src/job/JobQueueClient.ts b/packages/job-queue/src/job/JobQueueClient.ts index 2ae48100e..039377a89 100644 --- a/packages/job-queue/src/job/JobQueueClient.ts +++ b/packages/job-queue/src/job/JobQueueClient.ts @@ -173,23 +173,7 @@ export class JobQueueClient { readonly timeoutSeconds?: number; } ): Promise> { - const job: JobStorageFormat = { - queue: this.queueName, - input, - job_run_id: options?.jobRunId, - fingerprint: options?.fingerprint, - max_attempts: options?.maxAttempts ?? 10, - visible_at: - options?.delaySeconds != null - ? new Date(Date.now() + options.delaySeconds * 1000).toISOString() - : new Date().toISOString(), - deadline_at: - options?.timeoutSeconds != null - ? new Date(Date.now() + options.timeoutSeconds * 1000).toISOString() - : null, - completed_at: null, - status: JobStatus.PENDING, - }; + const job = this.buildJobBody(input, options); const id = await this.messageQueue.send(job, { fingerprint: options?.fingerprint, @@ -208,21 +192,77 @@ export class JobQueueClient { } /** - * Send multiple jobs to the queue + * Send multiple jobs to the queue in a single batched insert. + * + * Delegates to {@link IMessageQueue.sendBatch} so N jobs become one storage + * round-trip and a single worker wake, instead of N inserts + N wakes. + * + * Per the {@link IMessageQueue.sendBatch} contract, a batch-wide + * `fingerprint` is intentionally NOT accepted (it would dedup every body + * against the first row); use {@link send} per job for fingerprinted dedup. + * The other options — `jobRunId`, `maxAttempts`, `delaySeconds`, + * `timeoutSeconds` — apply uniformly to every body and are forwarded (the + * previous per-item loop silently dropped `delaySeconds`/`timeoutSeconds`). */ public async sendBatch( inputs: readonly Input[], options?: { readonly jobRunId?: string; readonly maxAttempts?: number; + /** Delay in seconds before every job becomes visible for processing */ + readonly delaySeconds?: number; + /** Timeout in seconds after which every job's deadline is exceeded */ + readonly timeoutSeconds?: number; } ): Promise[]> { - const handles: JobHandle[] = []; - for (const input of inputs) { - const handle = await this.send(input, options); - handles.push(handle); + if (inputs.length === 0) return []; + + const bodies = inputs.map((input) => this.buildJobBody(input, options)); + const ids = await this.messageQueue.sendBatch(bodies, { + jobRunId: options?.jobRunId, + maxAttempts: options?.maxAttempts, + delaySeconds: options?.delaySeconds, + timeoutSeconds: options?.timeoutSeconds, + }); + + // Single wake for the whole batch — avoids the N-wake thundering herd the + // per-item loop caused. + this.server?.handleJobAdded(ids[ids.length - 1]); + + return ids.map((id) => this.createJobHandle(id)); + } + + /** + * Build a {@link JobStorageFormat} body from an input + send options. Shared + * by {@link send} and {@link sendBatch} so both produce identical rows. + */ + private buildJobBody( + input: Input, + options?: { + readonly jobRunId?: string; + readonly fingerprint?: string; + readonly maxAttempts?: number; + readonly delaySeconds?: number; + readonly timeoutSeconds?: number; } - return handles; + ): JobStorageFormat { + return { + queue: this.queueName, + input, + job_run_id: options?.jobRunId, + fingerprint: options?.fingerprint, + max_attempts: options?.maxAttempts ?? 10, + visible_at: + options?.delaySeconds != null + ? new Date(Date.now() + options.delaySeconds * 1000).toISOString() + : new Date().toISOString(), + deadline_at: + options?.timeoutSeconds != null + ? new Date(Date.now() + options.timeoutSeconds * 1000).toISOString() + : null, + completed_at: null, + status: JobStatus.PENDING, + }; } /** diff --git a/packages/job-queue/src/job/JobQueueServer.ts b/packages/job-queue/src/job/JobQueueServer.ts index 9f6e9fc59..09d4bc0ef 100644 --- a/packages/job-queue/src/job/JobQueueServer.ts +++ b/packages/job-queue/src/job/JobQueueServer.ts @@ -21,6 +21,14 @@ import { storageToClass } from "./JobStorageConverters"; * Statistics tracked for the job queue */ export interface JobQueueStats { + /** + * Total number of execution ATTEMPTS started, not distinct jobs. Incremented + * once per `job_start`, which the worker emits on every execution — so a job + * that retries N times contributes N to this counter. It therefore will not + * equal `completedJobs + failedJobs + disabledJobs` in any run with retries; + * use {@link retriedJobs} to reconcile. Renamed semantics, not the field, to + * preserve the public stats shape. + */ readonly totalJobs: number; readonly completedJobs: number; readonly failedJobs: number; @@ -39,7 +47,7 @@ export type JobQueueServerEventListeners = { server_stop: (queueName: string) => void; job_start: (queueName: string, jobId: unknown) => void; job_complete: (queueName: string, jobId: unknown, output: Output) => void; - job_error: (queueName: string, jobId: unknown, error: string) => void; + job_error: (queueName: string, jobId: unknown, error: string, errorCode?: string) => void; job_disabled: (queueName: string, jobId: unknown) => void; job_retry: (queueName: string, jobId: unknown, visibleAt: Date) => void; job_progress: ( @@ -414,6 +422,8 @@ export class JobQueueServer< // Forward worker events to server and clients worker.on("job_start", (jobId) => { + // job_start fires once per execution attempt, so totalJobs is + // attempt-weighted (see JobQueueStats.totalJobs docs). this.stats = { ...this.stats, totalJobs: this.stats.totalJobs + 1 }; this.events.emit("job_start", this.queueName, jobId); this.forwardToClients("handleJobStart", jobId); @@ -428,7 +438,11 @@ export class JobQueueServer< // Immediate deletion when configured if (this.deleteAfterCompletionMs === 0) { this.jobStore.delete(jobId).catch((err) => { - console.error("Error deleting job after completion:", err); + getLogger().error("Error deleting job after completion", { + error: err, + jobId, + queueName: this.queueName, + }); }); } @@ -438,13 +452,17 @@ export class JobQueueServer< worker.on("job_error", (jobId, error, errorCode) => { this.stats = { ...this.stats, failedJobs: this.stats.failedJobs + 1 }; - this.events.emit("job_error", this.queueName, jobId, error); + this.events.emit("job_error", this.queueName, jobId, error, errorCode); this.forwardToClients("handleJobError", jobId, error, errorCode); // Immediate deletion when configured if (this.deleteAfterFailureMs === 0) { this.jobStore.delete(jobId).catch((err) => { - console.error("Error deleting job after error:", err); + getLogger().error("Error deleting job after error", { + error: err, + jobId, + queueName: this.queueName, + }); }); } @@ -460,7 +478,11 @@ export class JobQueueServer< // Immediate deletion when configured if (this.deleteAfterDisabledMs === 0) { this.jobStore.delete(jobId).catch((err) => { - console.error("Error deleting job after disabling:", err); + getLogger().error("Error deleting job after disabling", { + error: err, + jobId, + queueName: this.queueName, + }); }); } @@ -562,7 +584,7 @@ export class JobQueueServer< await this.jobStore.deleteByStatusAndAge(JobStatus.DISABLED, this.deleteAfterDisabledMs); } } catch (error) { - console.error("Error in cleanup:", error); + getLogger().error("Error in cleanup", { error, queueName: this.queueName }); } } diff --git a/packages/job-queue/src/job/JobQueueWorker.ts b/packages/job-queue/src/job/JobQueueWorker.ts index dbca8015f..b695b71a3 100644 --- a/packages/job-queue/src/job/JobQueueWorker.ts +++ b/packages/job-queue/src/job/JobQueueWorker.ts @@ -41,6 +41,21 @@ import { storageToClass } from "./JobStorageConverters"; */ const MAX_LIMITER_WAKE_MS = 30_000; +/** + * Minimum interval between {@link JobQueueWorker.processJobs} loop-error logs. + * A persistent storage failure throws every iteration; without rate-limiting + * that would flood the log at the poll rate. We still log the first occurrence + * immediately so operators get a prompt signal. + */ +const LOOP_ERROR_LOG_INTERVAL_MS = 5_000; + +/** + * Size of the recent-window ring buffer feeding + * {@link JobQueueWorker.getAverageProcessingTime}. Bounds memory and keeps the + * reported average representative of recent throughput. + */ +const MAX_PROCESSING_TIME_SAMPLES = 1_000; + /** * Events emitted by JobQueueWorker */ @@ -179,9 +194,19 @@ export class JobQueueWorker< protected readonly activeJobAbortControllers: Map = new Map(); /** - * Processing times for statistics + * Recent per-job processing durations (ms) used for + * {@link getAverageProcessingTime}. Bounded to the most recent + * {@link MAX_PROCESSING_TIME_SAMPLES} entries so a long-lived worker doesn't + * accumulate one entry per distinct job id forever, and so the reported + * average reflects a recent window rather than all-time. */ - protected readonly processingTimes: Map = new Map(); + protected readonly processingTimes: number[] = []; + + /** + * Timestamp (ms) of the last loop-error log, used to rate-limit the + * {@link processJobs} catch-path logging. See {@link LOOP_ERROR_LOG_INTERVAL_MS}. + */ + private lastLoopErrorLogAt = 0; constructor(jobClass: JobClass, options: JobQueueWorkerOptions) { this.queueName = options.queueName; @@ -324,10 +349,13 @@ export class JobQueueWorker< } /** - * Get average processing time + * Average processing time over the most recent + * {@link MAX_PROCESSING_TIME_SAMPLES} completed jobs (a recent window, not + * the worker's all-time history). Returns undefined until at least one job + * has completed. */ public getAverageProcessingTime(): number | undefined { - const times = Array.from(this.processingTimes.values()); + const times = this.processingTimes; if (times.length === 0) return undefined; return times.reduce((a, b) => a + b, 0) / times.length; } @@ -558,8 +586,21 @@ export class JobQueueWorker< if (dispatched === 0 && limiterFull) { await this.waitForWakeOrTimeout(await this.getLimiterWakeDelay()); } - } catch { - // Don't let transient errors kill the loop + } catch (err) { + // Don't let transient errors kill the loop, but never swallow them + // silently — a persistent storage failure (lost connection, schema + // mismatch, serialization error) would otherwise sleep-loop forever + // producing no work and no diagnostics. Rate-limit the log so a hot + // failure doesn't flood while still surfacing the problem. + const now = Date.now(); + if (now - this.lastLoopErrorLogAt >= LOOP_ERROR_LOG_INTERVAL_MS) { + this.lastLoopErrorLogAt = now; + getLogger().error("JobQueueWorker processing loop error", { + error: err, + queueName: this.queueName, + workerId: this.workerId, + }); + } await sleep(this.pollIntervalMs); } } @@ -731,7 +772,10 @@ export class JobQueueWorker< await this.completeJob(job, output); const elapsed = Date.now() - startTime; - this.processingTimes.set(job.id, elapsed); + this.processingTimes.push(elapsed); + if (this.processingTimes.length > MAX_PROCESSING_TIME_SAMPLES) { + this.processingTimes.shift(); + } if (span) { span.setAttributes({ "workglow.job.duration_ms": elapsed }); diff --git a/packages/job-queue/src/limiter/ConcurrencyLimiter.ts b/packages/job-queue/src/limiter/ConcurrencyLimiter.ts index b4e4d343b..30786cb7c 100644 --- a/packages/job-queue/src/limiter/ConcurrencyLimiter.ts +++ b/packages/job-queue/src/limiter/ConcurrencyLimiter.ts @@ -46,7 +46,13 @@ export class ConcurrencyLimiter implements ILimiter { this.currentRunningJobs = Math.max(0, this.currentRunningJobs - 1); } - async complete(_token: unknown): Promise { + async complete(token: unknown): Promise { + // Guard on the sentinel exactly like release() does. Without this, a + // duplicate complete(), a complete() after a release(), or a complete() + // with a foreign/undefined token would decrement a slot this limiter never + // handed out, permanently over-admitting concurrency (floored at 0) until + // clear(). + if (token !== ConcurrencyLimiter.SENTINEL) return; this.currentRunningJobs = Math.max(0, this.currentRunningJobs - 1); } diff --git a/packages/job-queue/src/queue-storage/IQueueStorage.ts b/packages/job-queue/src/queue-storage/IQueueStorage.ts index d88453c28..1900c5d03 100644 --- a/packages/job-queue/src/queue-storage/IQueueStorage.ts +++ b/packages/job-queue/src/queue-storage/IQueueStorage.ts @@ -216,6 +216,11 @@ export interface IQueueStorage { * writes the terminal result fields WITHOUT touching `attempts` — a successful * ack must not consume a retry attempt that was already accounted for by * the lease-expiry reclaim or by the wrapper's retry path. + * + * Missing-id contract: if no row matches `job.id` (already deleted, foreign, + * or concurrently reclaimed), implementations MUST silently no-op rather than + * throw. The forgiving semantics are required because lease-expiry races can + * legitimately make the target row disappear between claim and complete. * @param job - The job to complete */ complete(job: JobStorageFormat): Promise; @@ -235,6 +240,16 @@ export interface IQueueStorage { * atomic `disable` path can release the lease and clear progress in the * same single write. * + * `finalize` (plus {@link markDisabled}) is the ONLY interface-level + * terminal-write primitive. The atomic convenience wrappers + * `completeWithResult` / `failWithError` (and `saveStatus` / `getMany`) live + * on the concrete `InMemoryQueueStorage` and on the `IJobStore` facade + * produced by {@link wrapQueueStorage}, NOT on this interface — callers + * holding an `IQueueStorage` should reach those via the wrapper's + * `IJobStore`, or compose the field bag and call `finalize` directly. Kept + * off the interface so cloud adapters aren't forced to reimplement sugar + * that the wrapper already provides on top of `finalize`. + * * @param id - The ID of the job to finalize * @param fields - Terminal fields to write */ @@ -321,7 +336,12 @@ export interface IQueueStorage { getByRunId(runId: string): Promise>>; /** - * Saves progress updates for a job in the queue storage + * Saves progress updates for a job in the queue storage. + * + * Missing-id contract: if no row matches `id` (already finalized, deleted, + * or foreign), implementations MUST silently no-op rather than throw — + * progress is best-effort and a finalized/lost job must not turn a progress + * report into an exception. Matches {@link complete}'s forgiving semantics. * @param id - The ID of the job to save the progress for * @param progress - The progress of the job * @param message - The message of the job diff --git a/packages/job-queue/src/queue-storage/InMemoryMessageQueue.ts b/packages/job-queue/src/queue-storage/InMemoryMessageQueue.ts index 5f6721c48..691709295 100644 --- a/packages/job-queue/src/queue-storage/InMemoryMessageQueue.ts +++ b/packages/job-queue/src/queue-storage/InMemoryMessageQueue.ts @@ -25,7 +25,11 @@ class InMemoryClaim implements IClaim { const current = (await this.core.get(this.id)) ?? this.body; - const output = result !== undefined ? result : (current.output ?? null); + // Do not fall back to current.output — that's the prior attempt's value + // and finalize() must overwrite it on every ack. Matches WrappedClaim.ack + // so the ack(undefined) contract is identical across every IQueueStorage + // backend (in-memory vs wrapped cloud/IndexedDB). + const output = result !== undefined ? result : null; await this.core.finalize(this.id, { output: output as Output | null, error: null, @@ -58,8 +62,11 @@ class InMemoryClaim implements IClaim { void opts?.permanent; const current = (await this.core.get(this.id)) ?? this.body; - const error = opts?.error !== undefined ? opts.error : (current.error ?? null); - const errorCode = opts?.errorCode !== undefined ? opts.errorCode : (current.error_code ?? null); + // Do not fall back to current.error / current.error_code — those are the + // prior attempt's values and finalize() must overwrite them on every fail. + // Matches WrappedClaim.fail so the contract is identical across backends. + const error = opts?.error !== undefined ? opts.error : null; + const errorCode = opts?.errorCode !== undefined ? opts.errorCode : null; const abortRequested = opts?.abortRequested === true; await this.core.finalize(this.id, { error, diff --git a/packages/job-queue/src/queue-storage/TelemetryQueueStorage.ts b/packages/job-queue/src/queue-storage/TelemetryQueueStorage.ts index 49ae9d611..c7a129e0c 100644 --- a/packages/job-queue/src/queue-storage/TelemetryQueueStorage.ts +++ b/packages/job-queue/src/queue-storage/TelemetryQueueStorage.ts @@ -22,12 +22,36 @@ export class TelemetryQueueStorage implements IQueueStorage - ) {} + ) { + // findActiveByFingerprint is an OPTIONAL native method. Only expose it when + // the inner storage actually implements it, so wrapQueueStorage's + // `typeof native === "function"` probe still falls through to the bounded + // scan fallback when the inner has no native impl. A telemetry decorator + // must be transparent: always defining the method would force the O(1) + // native path to be reported as present even for backends (in-memory, + // IndexedDB) that lack it, then throw when delegating to undefined. + if (typeof inner.findActiveByFingerprint === "function") { + this.findActiveByFingerprint = (fingerprint: string, queueName: string) => + traced("workglow.storage.queue.findActiveByFingerprint", this.storageName, () => + this.inner.findActiveByFingerprint!(fingerprint, queueName) + ); + } + } public get scope(): QueueStorageScope { return this.inner.scope; } + /** + * Conditionally assigned in the constructor — present only when the inner + * storage exposes a native implementation. See the constructor for why this + * mirrors the inner's presence rather than always defining the method. + */ + public readonly findActiveByFingerprint?: ( + fingerprint: string, + queueName: string + ) => Promise | undefined>; + add(job: JobStorageFormat): Promise { return traced("workglow.storage.queue.add", this.storageName, () => this.inner.add(job)); } diff --git a/packages/job-queue/src/queue-storage/wrapQueueStorage.ts b/packages/job-queue/src/queue-storage/wrapQueueStorage.ts index 909beb2d7..d8ca02ca0 100644 --- a/packages/job-queue/src/queue-storage/wrapQueueStorage.ts +++ b/packages/job-queue/src/queue-storage/wrapQueueStorage.ts @@ -27,12 +27,6 @@ import type { */ const MAX_FINGERPRINT_SCAN = 10_000; -/** - * One-shot warning gate for the bounded-scan exhaustion path so a hot - * queue doesn't flood logs with the same message every send. - */ -let __fingerprintScanExhaustedWarned = false; - class WrappedClaim implements IClaim> { constructor( private readonly storage: IQueueStorage, @@ -199,6 +193,15 @@ class WrappedMessageQueue implements IMessageQueue implements IJobStore { + /** + * Per-instance one-shot gate for the bounded-scan exhaustion warning, so a + * hot queue doesn't flood logs with the same message on every send. Scoped + * to this store (not module-global) so each affected queue emits the warning + * at least once — a process-wide flag would let the first queue to exhaust + * permanently silence the signal for every other queue. + */ + private fingerprintScanExhaustedWarned = false; + constructor(private readonly storage: IQueueStorage) {} get(id: MessageId): Promise | undefined> { @@ -283,8 +286,8 @@ class WrappedJobStore implements IJobStore { } } - if (scanned >= MAX_FINGERPRINT_SCAN && !__fingerprintScanExhaustedWarned) { - __fingerprintScanExhaustedWarned = true; + if (scanned >= MAX_FINGERPRINT_SCAN && !this.fingerprintScanExhaustedWarned) { + this.fingerprintScanExhaustedWarned = true; getLogger().warn( "WrappedJobStore.findActiveByFingerprint: scanned MAX_FINGERPRINT_SCAN rows without a match; dedup may be best-effort under load" ); diff --git a/packages/storage/src/events/safeEmit.ts b/packages/storage/src/events/safeEmit.ts index 934288c82..ca02dbc8b 100644 --- a/packages/storage/src/events/safeEmit.ts +++ b/packages/storage/src/events/safeEmit.ts @@ -11,11 +11,18 @@ import { getLogger } from "@workglow/util"; * Emit an event without letting a throwing listener crash the caller. * * The shared {@link EventEmitter.emit} rethrows listener errors - * synchronously (or as an `AggregateError`). Storage write paths use - * that signal to trigger rollback, so they must not be derailed by a - * misbehaving subscriber. Listener errors are surfaced via - * `getLogger().warn` so observability tooling still sees the bug; the - * synchronous caller keeps running. + * synchronously (or as an `AggregateError`). Storage backends emit their + * mutation and read events POST-commit — after the row is already in the + * Map / written to disk / persisted — so at the emit site the operation has + * already succeeded and there is nothing left to roll back. A throwing + * subscriber must therefore not be allowed to turn that already-committed + * write into a thrown error (which would make `put()`/`delete()` reject even + * though the data is durable). Routing every post-commit emit through this + * helper decouples the operation's success/failure signal from subscriber + * behavior, and keeps single-row and batch emit paths consistent. + * + * Listener errors are surfaced via `getLogger().warn` so observability + * tooling still sees the bug; the synchronous caller keeps running. * * The error is intentionally NOT re-thrown onto the unhandled-rejection * channel: Node's default mode (`--unhandled-rejections=throw`, the diff --git a/packages/storage/src/kv/FsFolderKvStorage.ts b/packages/storage/src/kv/FsFolderKvStorage.ts index a4c0c32d3..f6cd2b7e7 100644 --- a/packages/storage/src/kv/FsFolderKvStorage.ts +++ b/packages/storage/src/kv/FsFolderKvStorage.ts @@ -8,6 +8,8 @@ import { createServiceToken } from "@workglow/util"; import { JsonSchema } from "@workglow/util/schema"; import { mkdir, readFile, rm, unlink, writeFile } from "fs/promises"; import path from "path"; +import { safeEmit } from "../events/safeEmit"; +import { StorageUnsupportedError } from "../tabular/StorageError"; import { IKvStorage } from "./IKvStorage"; import { KvStorage } from "./KvStorage"; @@ -69,7 +71,9 @@ export class FsFolderKvStorage< await mkdir(path.dirname(localPath), { recursive: true }); await writeFile(localPath, content); - this.events.emit("put", key, value); + // Post-commit emit: the file is written, so a throwing subscriber must not + // turn a durable write into a thrown error. + safeEmit(this.events, "put", key, value); } public async putBulk(items: Array<{ key: Key; value: Value }>): Promise { @@ -88,7 +92,7 @@ export class FsFolderKvStorage< }) ); const combined = settled.filter((r) => r !== undefined) as Combined[]; - this.events.emit("getBulk", keys, combined); + safeEmit(this.events, "getBulk", keys, combined); return combined; } @@ -128,31 +132,37 @@ export class FsFolderKvStorage< value = content as unknown as Value; } - this.events.emit("get", key, value); + safeEmit(this.events, "get", key, value); return value; } catch (error) { - this.events.emit("get", key, undefined); + safeEmit(this.events, "get", key, undefined); return undefined; } } public async delete(key: Key): Promise { const localPath = path.join(this.folderPath, this.pathWriter(key).replaceAll("..", "_")); - await unlink(localPath); - this.events.emit("delete", key); + try { + await unlink(localPath); + } catch (error) { + // Deleting a key that was never written is a no-op, matching the + // idempotent delete of the in-memory and tabular FsFolder backends. + if ((error as { code?: string })?.code !== "ENOENT") throw error; + } + safeEmit(this.events, "delete", key); } public async getAll(): Promise { - throw new Error("Not implemented"); + throw new StorageUnsupportedError("getAll", "FsFolderKvStorage"); } public async deleteAll(): Promise { const localPath = path.join(this.folderPath); await rm(localPath, { recursive: true, force: true }); - this.events.emit("deleteall"); + safeEmit(this.events, "deleteall"); } public async size(): Promise { - throw new Error("Not implemented"); + throw new StorageUnsupportedError("size", "FsFolderKvStorage"); } } diff --git a/packages/storage/src/kv/IKvStorage.ts b/packages/storage/src/kv/IKvStorage.ts index d1003136f..3cda7ded4 100644 --- a/packages/storage/src/kv/IKvStorage.ts +++ b/packages/storage/src/kv/IKvStorage.ts @@ -24,6 +24,12 @@ export type KvEventListeners = { getBulk: (keys: readonly Key[], results: readonly Combined[]) => void; getAll: (results: Combined[] | undefined) => void; delete: (key: unknown) => void; + /** + * Fired by {@link IKvStorage.deleteAll}. NOTE: the tabular surface emits the + * same clear-the-store concept under a different name (`clearall`, see + * {@link TabularEventListeners}). Subscribing at an abstraction boundary that + * spans both KV and tabular stores must account for both identifiers. + */ deleteall: () => void; }; @@ -49,7 +55,7 @@ export type KvEventParameters = * @typeParam Combined - Combined type of Key & Value */ export interface IKvStorage< - Key extends string | number = string, + Key extends string = string, Value = any, Combined = { key: Key; value: Value }, > { diff --git a/packages/storage/src/kv/KvViaTabularStorage.ts b/packages/storage/src/kv/KvViaTabularStorage.ts index 831d31b11..d09e8ab98 100644 --- a/packages/storage/src/kv/KvViaTabularStorage.ts +++ b/packages/storage/src/kv/KvViaTabularStorage.ts @@ -4,6 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { getLogger } from "@workglow/util"; +import { safeEmit } from "../events/safeEmit"; import type { BaseTabularStorage } from "../tabular/BaseTabularStorage"; import { DefaultKeyValueKey, DefaultKeyValueSchema } from "./IKvStorage"; import { KvStorage } from "./KvStorage"; @@ -38,14 +40,21 @@ export abstract class KvViaTabularStorage< /** * Deserialize a stored value back to the runtime form. When the schema * isn't a primitive we round-trip through JSON; if the stored bytes - * aren't valid JSON we return them as-is so callers see what's on disk - * rather than an exception. + * aren't valid JSON we log a warning (the value is corrupt for a + * JSON-serialized store) and return them as-is so callers see what's on + * disk rather than an exception. The log makes the corruption observable + * instead of silently surfacing a raw string where a parsed value is + * expected. */ private deserialize(raw: unknown): Value { if (this.needsJsonSerialization && typeof raw === "string") { try { return JSON.parse(raw) as Value; - } catch { + } catch (error) { + getLogger().warn( + "KvViaTabularStorage: stored value failed JSON parse; returning raw string", + { error } + ); return raw as unknown as Value; } } @@ -60,7 +69,9 @@ export abstract class KvViaTabularStorage< public async put(key: Key, value: Value): Promise { const storedValue = this.needsJsonSerialization ? (JSON.stringify(value) as Value) : value; await this.tabularRepository.put({ key, value: storedValue }); - this.events.emit("put", key, value); + // Post-commit emit: route through safeEmit so a throwing subscriber cannot + // turn an already-committed write into a thrown error. + safeEmit(this.events, "put", key, value); } public async putBulk(items: Array<{ key: Key; value: Value }>): Promise { @@ -70,18 +81,18 @@ export abstract class KvViaTabularStorage< await this.tabularRepository.putBulk(entities); for (const { key, value } of items) { - this.events.emit("put", key, value); + safeEmit(this.events, "put", key, value); } } public async get(key: Key): Promise { const result = await this.tabularRepository.get({ key }); if (!result) { - this.events.emit("get", key, undefined); + safeEmit(this.events, "get", key, undefined); return undefined; } const value = this.deserialize(result.value); - this.events.emit("get", key, value); + safeEmit(this.events, "get", key, value); return value; } @@ -95,13 +106,13 @@ export abstract class KvViaTabularStorage< const combined = rows.map( (row) => ({ key: row.key as Key, value: this.deserialize(row.value) }) as Combined ); - this.events.emit("getBulk", keys, combined); + safeEmit(this.events, "getBulk", keys, combined); return combined; } public async delete(key: Key): Promise { await this.tabularRepository.delete({ key }); - this.events.emit("delete", key); + safeEmit(this.events, "delete", key); } public async getAll(): Promise { @@ -109,13 +120,13 @@ export abstract class KvViaTabularStorage< const results = values ? values.map((row) => ({ key: row.key, value: this.deserialize(row.value) }) as Combined) : undefined; - this.events.emit("getAll", results); + safeEmit(this.events, "getAll", results); return results; } public async deleteAll(): Promise { await this.tabularRepository.deleteAll(); - this.events.emit("deleteall"); + safeEmit(this.events, "deleteall"); } public async size(): Promise { @@ -124,5 +135,9 @@ export abstract class KvViaTabularStorage< destroy(): void { this.tabularRepository.destroy(); + // Release KV-emitter subscriptions so long-lived subscribers (cache + // invalidators, telemetry) don't leak across store lifecycles. The wrapped + // tabular repo tears down its own emitter in its destroy(). + this.events.removeAllListeners(); } } diff --git a/packages/storage/src/kv/TelemetryKvStorage.ts b/packages/storage/src/kv/TelemetryKvStorage.ts index 68b47b899..9f8d25779 100644 --- a/packages/storage/src/kv/TelemetryKvStorage.ts +++ b/packages/storage/src/kv/TelemetryKvStorage.ts @@ -13,7 +13,7 @@ import type { IKvStorage, KvEventListener, KvEventName, KvEventParameters } from * Creates spans for all storage operations. */ export class TelemetryKvStorage< - Key extends string | number = string, + Key extends string = string, Value = any, Combined = { key: Key; value: Value }, > implements IKvStorage { diff --git a/packages/storage/src/sql/mapPostgresType.ts b/packages/storage/src/sql/mapPostgresType.ts index 529c3ecda..d659dcc9f 100644 --- a/packages/storage/src/sql/mapPostgresType.ts +++ b/packages/storage/src/sql/mapPostgresType.ts @@ -65,19 +65,29 @@ export function mapPostgresType(typeDef: JsonSchema, options: PostgresTypeMapOpt case "integer": // Handle integer vs floating point if (actualType.multipleOf === 1 || actualType.type === "integer") { - // Use PostgreSQL's numeric range types based on min/max values - if (typeof actualType.minimum === "number") { - if (actualType.minimum >= 0) { - // For unsigned integers - if (typeof actualType.maximum === "number") { - if (actualType.maximum <= 32767) return "SMALLINT"; - if (actualType.maximum <= 2147483647) return "INTEGER"; - } - return "BIGINT"; + // Use PostgreSQL's numeric range types based on min/max values. + if (typeof actualType.minimum === "number" && actualType.minimum >= 0) { + // For unsigned integers. + if (typeof actualType.maximum === "number") { + if (actualType.maximum <= 32767) return "SMALLINT"; + if (actualType.maximum <= 2147483647) return "INTEGER"; } + return "BIGINT"; } - // Default integer type + // Signed (or unbounded-below) integers: a large positive maximum still + // needs a wide enough column. Without this, a schema like + // `{ type: "integer", minimum: -1, maximum: 9999999999 }` would map to + // INTEGER (max 2147483647) and reject schema-valid writes at runtime. + // A minimum below INTEGER's lower bound also forces BIGINT. + if (typeof actualType.maximum === "number" && actualType.maximum > 2147483647) { + return "BIGINT"; + } + if (typeof actualType.minimum === "number" && actualType.minimum < -2147483648) { + return "BIGINT"; + } + + // Default integer type. return "INTEGER"; } diff --git a/packages/storage/src/sql/typedArrayCtors.ts b/packages/storage/src/sql/typedArrayCtors.ts index 1c4ff1094..b47e9292b 100644 --- a/packages/storage/src/sql/typedArrayCtors.ts +++ b/packages/storage/src/sql/typedArrayCtors.ts @@ -9,6 +9,14 @@ * back to its constructor, used when decoding stored vector bytes into a typed * array. Shared by the SQL tabular backends so the supported set stays * consistent across SQLite and Postgres. + * + * `Float16Array` is included to match the documented quantized-vector set. On + * runtimes that predate native `Float16Array`, `@workglow/util`'s schema layer + * installs a `globalThis.Float16Array` polyfill at module-load time, so the + * reference here resolves whenever a vector column was actually stored as + * Float16. The entry is registered conditionally so loading this module before + * the polyfill (or on a runtime that genuinely lacks it) cannot throw a + * `ReferenceError`. */ export const TYPED_ARRAY_CTORS: Record ArrayBufferView> = { Float32Array, @@ -17,4 +25,7 @@ export const TYPED_ARRAY_CTORS: Record ArrayBuff Uint8Array, Int16Array, Uint16Array, + ...(typeof globalThis.Float16Array !== "undefined" + ? { Float16Array: globalThis.Float16Array as new (data: number[]) => ArrayBufferView } + : {}), }; diff --git a/packages/storage/src/tabular/BaseTabularStorage.ts b/packages/storage/src/tabular/BaseTabularStorage.ts index 0043a6333..eb1c074c4 100644 --- a/packages/storage/src/tabular/BaseTabularStorage.ts +++ b/packages/storage/src/tabular/BaseTabularStorage.ts @@ -6,6 +6,7 @@ import { createServiceToken, EventEmitter, makeFingerprint } from "@workglow/util"; import { DataPortSchemaObject, FromSchema, TypedArraySchemaOptions } from "@workglow/util/schema"; +import { safeEmit } from "../events/safeEmit"; import { ITabularMigration, ITabularMigrationApplier, @@ -399,7 +400,7 @@ export abstract class BaseTabularStorage< if (keys.length === 0) return []; const results = await Promise.all(keys.map((k) => this.get(k))); const found = results.filter((r) => r !== undefined) as Entity[]; - this.events.emit("getBulk", keys, found); + safeEmit(this.events, "getBulk", keys, found); return found; } diff --git a/packages/storage/src/tabular/CachedTabularStorage.ts b/packages/storage/src/tabular/CachedTabularStorage.ts index 1e14a90c1..34376bc7c 100644 --- a/packages/storage/src/tabular/CachedTabularStorage.ts +++ b/packages/storage/src/tabular/CachedTabularStorage.ts @@ -6,6 +6,7 @@ import { createServiceToken, getLogger } from "@workglow/util"; import { DataPortSchemaObject, FromSchema, TypedArraySchemaOptions } from "@workglow/util/schema"; +import { safeEmit } from "../events/safeEmit"; import type { ITabularMigrationApplier } from "../migrations"; import { BaseTabularStorage, ClientProvidedKeysOption } from "./BaseTabularStorage"; import { InMemoryTabularStorage } from "./InMemoryTabularStorage"; @@ -83,20 +84,22 @@ export class CachedTabularStorage< } private setupEventForwarding(): void { + // Forwarded events are post-commit (the cache already mutated), so a + // throwing subscriber must not derail the write — route through safeEmit. this.cache.on("put", (entity) => { - this.events.emit("put", entity); + safeEmit(this.events, "put", entity); }); this.cache.on("get", (key, entity) => { - this.events.emit("get", key, entity); + safeEmit(this.events, "get", key, entity); }); this.cache.on("query", (key, entities) => { - this.events.emit("query", key, entities); + safeEmit(this.events, "query", key, entities); }); this.cache.on("delete", (key) => { - this.events.emit("delete", key); + safeEmit(this.events, "delete", key); }); this.cache.on("clearall", () => { - this.events.emit("clearall"); + safeEmit(this.events, "clearall"); }); } @@ -116,8 +119,13 @@ export class CachedTabularStorage< } this.cacheInitialized = true; } catch (error) { + // Surface the warm-up failure instead of swallowing it: query() and + // queryIndex() read ONLY the cache, so a silently-failed warm-up would + // make them return an empty result set as if the table were empty — + // a data-correctness bug callers cannot distinguish from "no rows". + // cacheInitialized stays false so a later access retries the warm-up. getLogger().warn("Failed to initialize cache from durable repository:", { error }); - // Don't mark as initialized on error — allow retry on next access. + throw error; } finally { this.cacheInitPromise = null; } @@ -273,23 +281,38 @@ export class CachedTabularStorage< /** * Subscribes to durable changes (including external ones) and keeps the * cache in step before forwarding each change to `callback`. + * + * The durable backend emits changes synchronously and does not await the + * subscriber, so the awaited cache mutation below cannot run inline. Instead, + * each change is appended to a per-subscription promise chain so the cache + * mutations apply in the same order the durable emitted them (no races + * between two rapid events) and any error in a cache mutation or in + * `callback` is logged rather than becoming a floating unhandled rejection. */ override subscribeToChanges( callback: (change: any) => void, options?: TabularSubscribeOptions ): () => void { - return this.durable.subscribeToChanges(async (change) => { - if (change.type === "INSERT" || change.type === "UPDATE") { - if (change.new) { - await this.cache.put(change.new); - } - } else if (change.type === "DELETE") { - if (change.old) { - await this.cache.delete(change.old); - } - } - - callback(change); + let chain: Promise = Promise.resolve(); + return this.durable.subscribeToChanges((change) => { + chain = chain + .then(async () => { + if (change.type === "INSERT" || change.type === "UPDATE") { + if (change.new) { + await this.cache.put(change.new); + } + } else if (change.type === "DELETE") { + if (change.old) { + await this.cache.delete(change.old); + } + } + callback(change); + }) + .catch((error) => { + getLogger().warn("CachedTabularStorage: failed to apply subscribed change to cache", { + error, + }); + }); }, options); } diff --git a/packages/storage/src/tabular/FsFolderTabularStorage.ts b/packages/storage/src/tabular/FsFolderTabularStorage.ts index 6fad5776b..1b4f49e4a 100644 --- a/packages/storage/src/tabular/FsFolderTabularStorage.ts +++ b/packages/storage/src/tabular/FsFolderTabularStorage.ts @@ -15,6 +15,7 @@ import { import { DataPortSchemaObject, FromSchema, TypedArraySchemaOptions } from "@workglow/util/schema"; import { mkdir, readdir, readFile, rm, writeFile } from "node:fs/promises"; import path from "node:path"; +import { safeEmit } from "../events/safeEmit"; import { type ITabularMigration, type ITabularMigrationApplier } from "../migrations"; import { PollingSubscriptionManager } from "../util/PollingSubscriptionManager"; import { @@ -189,7 +190,9 @@ export class FsFolderTabularStorage< ); } } - this.events.emit("put", entityToStore); + // Post-commit emit (the file is already written): a throwing subscriber + // must not turn a durable write into a thrown error. + safeEmit(this.events, "put", entityToStore); return entityToStore; } @@ -205,10 +208,10 @@ export class FsFolderTabularStorage< const buf = await readFile(filePath); const data = buf.toString("utf8"); const entity = JSON.parse(data) as Entity; - this.events.emit("get", key, entity); + safeEmit(this.events, "get", key, entity); return entity; } catch (error) { - this.events.emit("get", key, undefined); + safeEmit(this.events, "get", key, undefined); return undefined; } } @@ -220,9 +223,12 @@ export class FsFolderTabularStorage< try { await rm(filePath); } catch (error) { - console.error("Error deleting file", filePath, error); + // Missing file is an idempotent no-op; surface other errors for diagnosis. + if ((error as { code?: string })?.code !== "ENOENT") { + getLogger().error("Error deleting file", { filePath, error }); + } } - this.events.emit("delete", key as Partial); + safeEmit(this.events, "delete", key as Partial); } async getAll(): Promise { @@ -249,7 +255,7 @@ export class FsFolderTabularStorage< return values.length > 0 ? values : undefined; } catch (error) { - console.error("Error in getAll:", error); + getLogger().error("Error in getAll:", { error }); throw error; } } @@ -259,10 +265,10 @@ export class FsFolderTabularStorage< try { await rm(this.folderPath, { recursive: true, force: true }); } catch (error) { - console.error("Error deleting folder", this.folderPath, error); + getLogger().error("Error deleting folder", { folderPath: this.folderPath, error }); await rm(this.folderPath, { recursive: true, force: true }); } - this.events.emit("clearall"); + safeEmit(this.events, "clearall"); } async size(): Promise { @@ -274,6 +280,10 @@ export class FsFolderTabularStorage< } async getOffsetPage(offset: number, limit: number): Promise { + // Match the validation the query paths already enforce (and that the + // IndexedDb backend applies) so a negative offset or non-positive limit + // fails the same way across backends instead of silently slicing. + this.validateGetAllOptions({ offset, limit }); await this.setupDirectory(); const files = await readdir(this.folderPath); // Exclude internal bookkeeping files (prefixed with "_"). diff --git a/packages/storage/src/tabular/HttpTabularProxyStorage.ts b/packages/storage/src/tabular/HttpTabularProxyStorage.ts index c33d146f0..855cb81a9 100644 --- a/packages/storage/src/tabular/HttpTabularProxyStorage.ts +++ b/packages/storage/src/tabular/HttpTabularProxyStorage.ts @@ -6,6 +6,7 @@ import { createServiceToken, deepEqual, makeFingerprint } from "@workglow/util"; import { DataPortSchemaObject, FromSchema, TypedArraySchemaOptions } from "@workglow/util/schema"; +import { safeEmit } from "../events/safeEmit"; import { PollingSubscriptionManager } from "../util/PollingSubscriptionManager"; import { BaseTabularStorage, ClientProvidedKeysOption } from "./BaseTabularStorage"; import type { @@ -125,34 +126,34 @@ export class HttpTabularProxyStorage< async put(value: InsertType): Promise { const { entity } = await this.call<{ entity: Entity }>("put", { value }); - this.events.emit("put", entity); + safeEmit(this.events, "put", entity); return entity; } async putBulk(values: InsertType[]): Promise { if (values.length === 0) return []; const { entities } = await this.call<{ entities: Entity[] }>("putBulk", { values }); - for (const entity of entities) this.events.emit("put", entity); + for (const entity of entities) safeEmit(this.events, "put", entity); return entities; } async get(key: PrimaryKey): Promise { const { entity } = await this.call<{ entity: Entity | null }>("get", { key }); const result = entity ?? undefined; - this.events.emit("get", key, result); + safeEmit(this.events, "get", key, result); return result; } async delete(key: PrimaryKey | Entity): Promise { await this.call<{ ok: true }>("delete", { key }); const { key: normalizedKey } = this.separateKeyValueFromCombined(key as Entity); - this.events.emit("delete", normalizedKey as Partial); + safeEmit(this.events, "delete", normalizedKey as Partial); } override async getBulk(keys: readonly PrimaryKey[]): Promise { if (keys.length === 0) return []; const { entities } = await this.call<{ entities: Entity[] }>("getBulk", { keys }); - this.events.emit("getBulk", keys, entities); + safeEmit(this.events, "getBulk", keys, entities); return entities; } @@ -166,7 +167,7 @@ export class HttpTabularProxyStorage< options, }); const result = entities ?? undefined; - this.events.emit("query", criteria as Partial, result); + safeEmit(this.events, "query", criteria as Partial, result); return result; } @@ -188,7 +189,7 @@ export class HttpTabularProxyStorage< async deleteAll(): Promise { await this.call<{ ok: true }>("deleteAll", {}); - this.events.emit("clearall"); + safeEmit(this.events, "clearall"); } async deleteSearch(criteria: DeleteSearchCriteria): Promise { diff --git a/packages/storage/src/tabular/HuggingFaceTabularStorage.ts b/packages/storage/src/tabular/HuggingFaceTabularStorage.ts index e750a06dd..0f704f256 100644 --- a/packages/storage/src/tabular/HuggingFaceTabularStorage.ts +++ b/packages/storage/src/tabular/HuggingFaceTabularStorage.ts @@ -6,6 +6,7 @@ import { createServiceToken } from "@workglow/util"; import { DataPortSchemaObject, FromSchema, TypedArraySchemaOptions } from "@workglow/util/schema"; +import { safeEmit } from "../events/safeEmit"; import { type ITabularMigration, type ITabularMigrationApplier } from "../migrations"; import { BaseTabularStorage } from "./BaseTabularStorage"; import { decodeCursor, encodeCursor, PageCursor } from "./Cursor"; @@ -275,11 +276,11 @@ export class HuggingFaceTabularStorage< if (data.rows.length > 0) { const entity = this.rowToEntity(data.rows[0]); - this.events.emit("get", key, entity); + safeEmit(this.events, "get", key, entity); return entity; } - this.events.emit("get", key, undefined); + safeEmit(this.events, "get", key, undefined); return undefined; } @@ -509,10 +510,10 @@ export class HuggingFaceTabularStorage< } if (results.length > 0) { - this.events.emit("query", criteria as Partial, results); + safeEmit(this.events, "query", criteria as Partial, results); return results; } else { - this.events.emit("query", criteria as Partial, undefined); + safeEmit(this.events, "query", criteria as Partial, undefined); return undefined; } } diff --git a/packages/storage/src/tabular/ITabularStorage.ts b/packages/storage/src/tabular/ITabularStorage.ts index 5bd079162..8d77f4fe7 100644 --- a/packages/storage/src/tabular/ITabularStorage.ts +++ b/packages/storage/src/tabular/ITabularStorage.ts @@ -15,6 +15,14 @@ export type ValueOptionType = string | number | bigint | boolean | null | Uint8A export type TabularEventListeners = { put: (entity: Entity) => void; get: (key: PrimaryKey, entity: Entity | undefined) => void; + /** + * NOTE: backends that implement `getBulk` as a fan-out over per-key `get` + * (the default {@link BaseTabularStorage.getBulk}, used by InMemory) ALSO + * emit one `get` event per key in addition to this single `getBulk`. + * Push-down backends (SQL `WHERE pk IN (...)`, and the KV-over-tabular path) + * emit only `getBulk`. Instrumentation that counts reads via `get` events + * must account for this backend-dependent fan-out. + */ getBulk: (keys: readonly PrimaryKey[], entities: readonly Entity[]) => void; query: (key: Partial, entities: Entity[] | undefined) => void; /** @@ -24,12 +32,28 @@ export type TabularEventListeners = { * can filter and caches can invalidate without the backend reading the row. */ delete: (key: Partial) => void; + /** + * Fired by {@link ITabularStorage.deleteAll}. NOTE: the KV surface emits the + * same clear-the-store concept under a different name (`deleteall`, see + * {@link KvEventListeners}). Subscribing at an abstraction boundary that spans + * both tabular and KV stores must account for both identifiers. + */ clearall: () => void; /** - * Emitted when an atomic batch op (e.g. vector `putBulk`) detects a mid-batch - * failure and restores prior state. Subscribers should treat any uncommitted - * `put` events from the failed batch as superseded and reconcile against the - * post-rollback state. + * Emitted when an atomic batch op (`putBulk`) detects a mid-batch failure and + * restores prior state. Subscribers should treat any uncommitted `put` events + * from the failed batch as superseded and reconcile against the post-rollback + * state. + * + * **Support is backend-dependent.** Backends that can cheaply snapshot and + * restore their mutable state honor an all-or-nothing `putBulk` and emit this + * event on failure: the in-memory family (`InMemoryTabularStorage`, its vector + * overlay, and `SharedInMemoryTabularStorage`, which delegates to an inner + * in-memory repo) and the IndexedDB transactional batch. Backends whose batch + * is a non-atomic fan-out — notably `FsFolderTabularStorage` (per-file writes + * with no snapshot) — do NOT emit `rollback`; a mid-batch failure there leaves + * earlier rows committed. Subscribers that rely on rollback for correctness + * must confirm the concrete backend supports it. * * `ids` carries the primary keys of rows that were observably committed (via * a per-row `put` event or backend-level write) before the failure, in the diff --git a/packages/storage/src/tabular/InMemoryTabularStorage.ts b/packages/storage/src/tabular/InMemoryTabularStorage.ts index aef9a4fda..36b466e8c 100644 --- a/packages/storage/src/tabular/InMemoryTabularStorage.ts +++ b/packages/storage/src/tabular/InMemoryTabularStorage.ts @@ -6,6 +6,7 @@ import { createServiceToken, makeFingerprint, uuid4 } from "@workglow/util"; import { DataPortSchemaObject, FromSchema, TypedArraySchemaOptions } from "@workglow/util/schema"; +import { safeEmit } from "../events/safeEmit"; import { type ITabularMigration, type ITabularMigrationApplier } from "../migrations"; import { BaseTabularStorage, @@ -144,12 +145,65 @@ export class InMemoryTabularStorage< this._lastPutWasInsert = !this.values.has(id); this.values.set(id, entityToStore); - this.events.emit("put", entityToStore); + // Post-commit emit: the row is already in the Map, so a throwing subscriber + // must not turn a successful write into a thrown error. Route through + // safeEmit (consistent with the batch `rollback` emit and the per-row paths + // below) so the mutation's success/failure signal is never derailed. + safeEmit(this.events, "put", entityToStore); return entityToStore; } + /** + * Atomic batch put: snapshot mutable state, write rows one at a time, and + * roll back to the snapshot if any single `put` throws. The inherited + * `Promise.all` shape used to leave the Map (and autoincrement counter) in a + * half-applied state when a later `put` rejected after earlier ones already + * mutated the Map. This guarantees either every row in the batch is visible + * after a successful return, or none of the batch's rows are — so subscribers + * written against the `rollback` event contract behave correctly here, not + * just on the vector overlay. + */ async putBulk(values: InsertType[]): Promise { - return await Promise.all(values.map(async (value) => this.put(value))); + if (values.length === 0) return []; + return this.atomicPutBulk(values); + } + + /** + * Shared all-or-nothing batch implementation used by {@link putBulk} and the + * vector overlay's `putBulk`. Snapshots the Map + counter, writes serially, + * and on a mid-batch throw restores the snapshot and emits a `rollback` event + * (through {@link safeEmit}) carrying the PKs that committed before the + * failure. + * + * `writeRow` defaults to {@link put}. The vector overlay passes its own + * base-`put` writer so the per-row writes do NOT re-enter the overlay's + * `put` (which re-acquires the overlay mutex this batch already holds, and + * re-runs validation already performed up front). + */ + protected async atomicPutBulk( + values: InsertType[], + writeRow: (value: InsertType) => Promise = (value) => this.put(value) + ): Promise { + const snapshot = this.snapshotMutableState(); + const results: Entity[] = []; + // PKs of rows that actually committed before a throw, so the rollback event + // lets subscribers surgically invalidate caches instead of re-reading the + // whole table. Typed via `any` because the public PK type is computed from + // the schema; the value still flows through the {@link TabularEventListeners} + // payload at the emit site. + const committedIds: any[] = []; + try { + for (const value of values) { + const committed = await writeRow(value); + results.push(committed); + committedIds.push(this.separateKeyValueFromCombined(committed).key); + } + return results; + } catch (error) { + this.restoreMutableState(snapshot); + safeEmit(this.events, "rollback", { op: "putBulk", error, ids: committedIds }); + throw error; + } } /** @@ -217,7 +271,7 @@ export class InMemoryTabularStorage< async get(key: PrimaryKey): Promise { const id = await makeFingerprint(key); const out = this.values.get(id); - this.events.emit("get", key, out); + safeEmit(this.events, "get", key, out); return out; } @@ -228,12 +282,12 @@ export class InMemoryTabularStorage< // A keyed delete emits only the key. Bulk deleteSearch carries the matched // row (it already has it in hand), and scoped callers delete via // deleteSearch so the owner columns reach subscribers without a read-back. - this.events.emit("delete", key as Partial); + safeEmit(this.events, "delete", key as Partial); } async deleteAll(): Promise { this.values.clear(); - this.events.emit("clearall"); + safeEmit(this.events, "clearall"); } async getAll(options?: QueryOptions): Promise { @@ -271,6 +325,10 @@ export class InMemoryTabularStorage< } async getOffsetPage(offset: number, limit: number): Promise { + // Match the validation the query paths already enforce (and that the + // IndexedDb backend applies) so a negative offset or non-positive limit + // fails the same way across backends instead of silently slicing. + this.validateGetAllOptions({ offset, limit }); const all = Array.from(this.values.values()); // Sort by primary key for deterministic pagination order. @@ -336,7 +394,7 @@ export class InMemoryTabularStorage< this.values.delete(id); // Emit the matched row as the deleted identity (it carries the owner // columns); InMemory already has it in hand, so this is not a read-back. - this.events.emit("delete", entity); + safeEmit(this.events, "delete", entity); } } @@ -407,7 +465,7 @@ export class InMemoryTabularStorage< } const result = results.length > 0 ? results : undefined; - this.events.emit("query", criteria as Partial, result); + safeEmit(this.events, "query", criteria as Partial, result); return result; } diff --git a/packages/storage/src/tabular/SharedInMemoryTabularStorage.ts b/packages/storage/src/tabular/SharedInMemoryTabularStorage.ts index 2f9bc607b..cab0c05cc 100644 --- a/packages/storage/src/tabular/SharedInMemoryTabularStorage.ts +++ b/packages/storage/src/tabular/SharedInMemoryTabularStorage.ts @@ -4,8 +4,9 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { createServiceToken } from "@workglow/util"; +import { createServiceToken, getLogger } from "@workglow/util"; import { DataPortSchemaObject, FromSchema, TypedArraySchemaOptions } from "@workglow/util/schema"; +import { safeEmit } from "../events/safeEmit"; import { type ITabularMigration, type ITabularMigrationApplier } from "../migrations"; import { BaseTabularStorage, ClientProvidedKeysOption } from "./BaseTabularStorage"; import { @@ -99,7 +100,7 @@ export class SharedInMemoryTabularStorage< private initializeBroadcastChannel(): void { if (!this.isBroadcastChannelAvailable()) { - console.warn("BroadcastChannel is not available. Tab synchronization will not work."); + getLogger().warn("BroadcastChannel is not available. Tab synchronization will not work."); return; } @@ -111,25 +112,27 @@ export class SharedInMemoryTabularStorage< this.syncFromOtherTabs(); } catch (error) { - console.error("Failed to initialize BroadcastChannel:", error); + getLogger().error("Failed to initialize BroadcastChannel:", { error }); } } private setupEventForwarding(): void { + // Forwarded events are post-commit (the inner repo already mutated), so a + // throwing subscriber must not derail the write — route through safeEmit. this.inMemoryRepo.on("put", (entity) => { - this.events.emit("put", entity); + safeEmit(this.events, "put", entity); }); this.inMemoryRepo.on("get", (key, entity) => { - this.events.emit("get", key, entity); + safeEmit(this.events, "get", key, entity); }); this.inMemoryRepo.on("query", (key, entities) => { - this.events.emit("query", key, entities); + safeEmit(this.events, "query", key, entities); }); this.inMemoryRepo.on("delete", (key) => { - this.events.emit("delete", key); + safeEmit(this.events, "delete", key); }); this.inMemoryRepo.on("clearall", () => { - this.events.emit("clearall"); + safeEmit(this.events, "clearall"); }); } @@ -215,7 +218,7 @@ export class SharedInMemoryTabularStorage< this.syncInProgress = false; void this.drainPendingMessages() .catch((error) => { - console.error("Failed to drain pending messages after sync timeout", error); + getLogger().error("Failed to drain pending messages after sync timeout", { error }); }) .finally(() => { this.resolveSyncSettled(); diff --git a/packages/storage/src/util/HybridSubscriptionManager.ts b/packages/storage/src/util/HybridSubscriptionManager.ts index a263dd9eb..c3e10c445 100644 --- a/packages/storage/src/util/HybridSubscriptionManager.ts +++ b/packages/storage/src/util/HybridSubscriptionManager.ts @@ -4,6 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { getLogger } from "@workglow/util"; + export interface HybridManagerOptions { /** Default polling interval in milliseconds for backup polling */ readonly defaultIntervalMs?: number; @@ -87,7 +89,7 @@ export class HybridSubscriptionManager { this.handleBroadcastMessage(event.data); }; } catch (error) { - console.error("Failed to initialize BroadcastChannel:", error); + getLogger().error("Failed to initialize BroadcastChannel:", { error }); this.channel = null; } } diff --git a/packages/storage/src/vector/InMemoryVectorStorage.ts b/packages/storage/src/vector/InMemoryVectorStorage.ts index eb5e968c8..61812b0cc 100644 --- a/packages/storage/src/vector/InMemoryVectorStorage.ts +++ b/packages/storage/src/vector/InMemoryVectorStorage.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type { EventEmitter } from "@workglow/util"; import type { DataPortSchemaObject, FromSchema, @@ -37,6 +38,12 @@ export class InMemoryVectorStorage< private vectorPropertyName: keyof Entity; private metadataPropertyName: keyof Entity | undefined; + /** + * @param _vectorCtor Accepted for call-site parity with the cloud vector + * backends (and `createKnowledgeBase`), but unused here: the in-memory + * store always reconstructs vectors from whatever typed array the schema / + * decoder produced, so it does not honor a custom constructor. + */ constructor( schema: Schema, primaryKeyNames: PrimaryKeyNames, @@ -117,31 +124,14 @@ export class InMemoryVectorStorage< this.vectorDimensions ); if (values.length === 0) return []; - return this.mutex(async () => { - const snapshot = this.snapshotMutableState(); - const results: Entity[] = []; - // Track the PKs of rows that actually committed before the throw, so the - // rollback event lets subscribers surgically invalidate caches instead of - // re-reading the whole table. Typed via `any` because the public PK type - // is computed from the schema and BaseTabularStorage holds its own - // PrimaryKey type parameter — the value still flows through the - // {@link TabularEventListeners} payload at the emit site below. - const committedIds: any[] = []; - try { - for (const value of values) { - const committed = await super.put(value); - results.push(committed); - // Extract the PK from the committed entity (handles auto-generated - // keys assigned during put). - committedIds.push(this.separateKeyValueFromCombined(committed).key); - } - return results; - } catch (error) { - this.restoreMutableState(snapshot); - safeEmit(this.events, "rollback", { op: "putBulk", error, ids: committedIds }); - throw error; - } - }); + // Reuse the base atomic batch (snapshot / serial write / restore + + // `rollback` emit), wrapped in this instance's mutex so a concurrent + // single-row `put` cannot slip into the Map between the snapshot and a + // rollback and get wiped along with the failed batch. Rows are written via + // the base `put` (validation already ran up front, and the overlay mutex is + // already held) — calling the overlay `put` here would deadlock on the + // mutex and re-validate every row. + return this.mutex(() => this.atomicPutBulk(values, (value) => super.put(value))); } async similaritySearch( @@ -149,16 +139,20 @@ export class InMemoryVectorStorage< options: VectorSearchOptions> = {} ) { assertVectorShape(query, this.vectorDimensions, "query"); - const { topK = 10, filter, scoreThreshold = 0 } = options; + // Default to no floor: cosine similarity ranges over [-1, 1], so a default + // of 0 would silently drop negatively-correlated hits and return fewer than + // `topK`. Callers opt into a relevance floor explicitly via `scoreThreshold`. + const { topK = 10, filter, scoreThreshold = -Infinity } = options; const results: Array = []; const allEntities = (await this.getAll()) || []; for (const entity of allEntities) { const vector = entity[this.vectorPropertyName] as TypedArray; - const metadata = this.metadataPropertyName - ? (entity[this.metadataPropertyName] as Metadata) - : ({} as Metadata); + // A present-but-null metadata column value coalesces to `{}` so a filtered + // search treats the row as a non-match rather than dereferencing null. + const rawMetadata = this.metadataPropertyName ? entity[this.metadataPropertyName] : undefined; + const metadata = (rawMetadata ?? {}) as Metadata; if (filter && !matchesFilter(metadata, filter)) { continue; @@ -177,6 +171,20 @@ export class InMemoryVectorStorage< } results.sort((a, b) => b.score - a.score); - return results.slice(0, topK); + const topResults = results.slice(0, topK); + // The inherited `events` emitter is typed for the tabular event surface; + // `similaritySearch` lives on the vector extension of that surface. The + // emitter instance is the same object, so widen the view to a record that + // carries the event so it can be emitted type-safely. + type SimilaritySearchEvents = { + similaritySearch: (query: TypedArray, results: (Entity & { score: number })[]) => void; + }; + safeEmit( + this.events as unknown as EventEmitter, + "similaritySearch", + query, + topResults + ); + return topResults; } } diff --git a/packages/storage/src/vector/README.md b/packages/storage/src/vector/README.md index 33b5cf657..96cfbea69 100644 --- a/packages/storage/src/vector/README.md +++ b/packages/storage/src/vector/README.md @@ -278,7 +278,7 @@ destroy(): void; interface VectorSearchOptions> { readonly topK?: number; // Number of results (default: 10) readonly filter?: Partial; // Filter by metadata fields - readonly scoreThreshold?: number; // Minimum score 0-1 (default: 0) + readonly scoreThreshold?: number; // Minimum cosine score in [-1, 1]; default -Infinity (no floor, pure topK) } interface HybridSearchOptions extends VectorSearchOptions { diff --git a/packages/task-graph/README.md b/packages/task-graph/README.md index 7ea829ce9..21d6c2b0b 100644 --- a/packages/task-graph/README.md +++ b/packages/task-graph/README.md @@ -757,6 +757,9 @@ Both slots are optional. A missing slot is a silent no-op — the task still run import { CACHE_REGISTRY, DefaultCacheRegistry, + RunPrivateTaskOutputPrimaryKeyNames, + RunPrivateTaskOutputRepository, + RunPrivateTaskOutputSchema, tabularTaskOutputStorage, TaskOutputPrimaryKeyNames, TaskOutputSchema, @@ -779,15 +782,16 @@ const deterministic = new TaskOutputTabularRepository({ ), }); -const privateBacking = new TaskOutputTabularRepository({ - storage: tabularTaskOutputStorage( - new SqliteTabularStorage( - "./cache.sqlite", - "task_outputs_private", - TaskOutputSchema, - TaskOutputPrimaryKeyNames, - ["createdAt"] - ) +// The private slot must be a RunPrivateTaskOutputRepository: its own table with +// a runId column and a runId-leading primary key, so run-scoped cleanup is an +// indexed delete. (It takes the ITabularStorage directly — no tabular adapter.) +const privateBacking = new RunPrivateTaskOutputRepository({ + storage: new SqliteTabularStorage( + "./cache.sqlite", + "task_outputs_private", + RunPrivateTaskOutputSchema, + RunPrivateTaskOutputPrimaryKeyNames, + ["createdAt"] ), }); @@ -801,7 +805,7 @@ registry.registerInstance( await graph.run({}, { registry, runId: "run-" + crypto.randomUUID() }); ``` -The runner constructs a per-run `RunPrivateCacheRepo` wrapper over the `private` slot, namespaced by `runId`. The wrapper exists only for the duration of the run; the rows it writes survive in the backing store until either explicit cleanup (on successful completion) or the TTL janitor sweeps them (after a crashed run is abandoned). +The runner constructs a per-run `RunPrivateCacheRepo` wrapper over the `private` slot, scoping every entry to `runId` (stored as a first-class column in the run-private table, not a key prefix). The wrapper exists only for the duration of the run; the rows it writes survive in the backing store until either explicit cleanup (on successful completion) or the TTL janitor sweeps them (after a crashed run is abandoned). ### Run identity and durable execution @@ -833,7 +837,7 @@ const janitor = new CacheJanitor({ privateBacking }); await janitor.sweepStaleRunPrivate(24 * 60 * 60 * 1000); ``` -The janitor only touches rows with the `__run:` prefix that `RunPrivateCacheRepo` writes; deterministic-tier rows are never affected. +The run-private cache is its own dedicated table, so the janitor sweeps every entry older than the cutoff; the deterministic tier (a separate repository/table) is never touched. Pass the raw `RunPrivateTaskOutputRepository` here — not a per-run `RunPrivateCacheRepo` wrapper, whose `clearOlderThan` is scoped to a single run. #### Durability warning diff --git a/packages/task-graph/src/EXECUTION_MODEL.md b/packages/task-graph/src/EXECUTION_MODEL.md index 5ee415189..b9dbf1746 100644 --- a/packages/task-graph/src/EXECUTION_MODEL.md +++ b/packages/task-graph/src/EXECUTION_MODEL.md @@ -245,7 +245,7 @@ interface CacheRegistry { `TaskGraphRunner` resolves the registry from the per-run `ServiceRegistry` and dispatches each task's read/write to the slot named by its policy. Both slots are independently optional. A missing slot is a silent no-op: the task runs uncached, no error. -For the `private` slot, the runner constructs a per-run `RunPrivateCacheRepo` wrapper that prefixes every key with `__run:${runId}::${taskId}`. Two runs with different `runId`s never see each other's rows; the same `runId` (a restart) does. Two task nodes of the same class in one graph never share private entries because each keys by its instance id, not its type. +For the `private` slot, the runner constructs a per-run `RunPrivateCacheRepo` wrapper that threads `runId` into the backing `RunPrivateTaskOutputRepository`, whose rows carry `runId` as a first-class column under a runId-leading primary key `(runId, key, taskId)`. Two runs with different `runId`s never see each other's rows; the same `runId` (a restart) does. Two task nodes of the same class in one graph never share private entries because each keys by its instance id, not its type. ### Run identity (`runId`) @@ -269,7 +269,7 @@ The caller owns generation. The contract: key = sha256(taskType + getCacheVersion() + fingerprint(inputs)) ``` -`fingerprint(inputs)` reuses the `PortCodec` normalization in `CacheCoordinator` — ports with `format` annotations hash by their canonical wire representation. Scope namespacing (the `runId` prefix for the private tier) is handled by the repo wrapper, not the key function. +`fingerprint(inputs)` reuses the `PortCodec` normalization in `CacheCoordinator` — ports with `format` annotations hash by their canonical wire representation. Run scoping (the `runId` column for the private tier) is handled by the repo wrapper, not the key function. `getCacheVersion()` walks the prototype chain and combines each ancestor's static `version` (default `1`). Bump `version` on a task when its semantics change — every prior cached entry becomes a miss. diff --git a/packages/task-graph/src/cache/CacheJanitor.ts b/packages/task-graph/src/cache/CacheJanitor.ts index b2264dd99..211972b7a 100644 --- a/packages/task-graph/src/cache/CacheJanitor.ts +++ b/packages/task-graph/src/cache/CacheJanitor.ts @@ -4,32 +4,37 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { TaskOutputRepository } from "../storage/TaskOutputRepository"; +import type { RunPrivateTaskOutputRepository } from "../storage/RunPrivateTaskOutputRepository"; export interface CacheJanitorOptions { - privateBacking: TaskOutputRepository; + /** + * The dedicated run-private backing (NOT a per-run {@link RunPrivateCacheRepo} + * wrapper, whose `clearOlderThan` is scoped to a single run — that would leave + * other runs' stale rows un-swept). + */ + privateBacking: RunPrivateTaskOutputRepository; } /** * Periodic cleanup helper for run-private cache entries left behind by runs * that crashed and were never restarted. * - * Run-private rows are namespaced by `RunPrivateCacheRepo` with the - * `__run:${runId}::${taskId}` prefix. This janitor sweeps those rows when they are older than - * `olderThanMs`. Entries lacking the prefix (deterministic cache, shared tier) - * are not touched. + * The run-private cache is its own dedicated table, so any row older than + * `olderThanMs` is a stale orphan — a successful run deletes its own rows via + * {@link RunPrivateCacheRepo.clearRun}. The sweep is an indexed + * `deleteSearch({ createdAt: { "<" } })`, not a table scan. * * Apps schedule the sweep themselves (cron, periodic worker, on startup) — * libs does not run it automatically. */ export class CacheJanitor { - private readonly privateBacking: TaskOutputRepository; + private readonly privateBacking: RunPrivateTaskOutputRepository; constructor({ privateBacking }: CacheJanitorOptions) { this.privateBacking = privateBacking; } async sweepStaleRunPrivate(olderThanMs: number): Promise { - await this.privateBacking.clearOlderThanWithTaskTypePrefix("__run:", olderThanMs); + await this.privateBacking.clearOlderThan(olderThanMs); } } diff --git a/packages/task-graph/src/cache/RunPrivateCacheRepo.ts b/packages/task-graph/src/cache/RunPrivateCacheRepo.ts index 9b0e137b9..4122cb5c9 100644 --- a/packages/task-graph/src/cache/RunPrivateCacheRepo.ts +++ b/packages/task-graph/src/cache/RunPrivateCacheRepo.ts @@ -15,10 +15,13 @@ export interface RunPrivateCacheRepoOptions { /** * Wraps a TaskOutputRepository so that all entries are namespaced by `runId`. * - * Namespacing happens at the repository's `taskType` axis: {@link CacheCoordinator} - * passes each task's instance `id` for private-policy entries, so rows are stored - * as `__run:${runId}::${taskId}` in the backing store. The input fingerprint is - * unchanged for resume lookups within the same node. + * Run scoping is delegated to the backing repository's run-scoped methods + * ({@link TaskOutputRepository.saveOutputForRun} etc.): {@link CacheCoordinator} + * passes each task's instance `id` as the entry's `taskType`, and this wrapper + * threads its `runId` so the backing stores it as a first-class column under a + * runId-leading primary key. The input fingerprint is unchanged for resume + * lookups within the same node. The backing must be a run-private repository + * (e.g. `RunPrivateTaskOutputRepository`). * * - Two wrappers with the same `runId` (e.g., a restart after a crash) see each * other's writes via the backing store — that's the restart-survival contract. @@ -66,30 +69,26 @@ export class RunPrivateCacheRepo extends TaskOutputRepository { return this.observedFallback; } - private ns(cacheIdentity: string): string { - return `__run:${this.runId}::${cacheIdentity}`; - } - public async saveOutput( cacheIdentity: string, inputs: TaskInput, output: TaskOutput, createdAt?: Date ): Promise { - await this.backing.saveOutput(this.ns(cacheIdentity), inputs, output, createdAt); + await this.backing.saveOutputForRun(this.runId, cacheIdentity, inputs, output, createdAt); } public async getOutput( cacheIdentity: string, inputs: TaskInput ): Promise { - return this.backing.getOutput(this.ns(cacheIdentity), inputs); + return this.backing.getOutputForRun(this.runId, cacheIdentity, inputs); } /** - * Override of `TaskOutputRepository.clear()` that only deletes entries - * namespaced under THIS wrapper's `runId`. Entries from other runs are not - * touched. Use the backing repository directly if you need a global clear. + * Override of `TaskOutputRepository.clear()` that only deletes entries for + * THIS wrapper's `runId`. Entries from other runs are not touched. Use the + * backing repository directly if you need a global clear. */ public async clear(): Promise { await this.clearRun(); @@ -97,29 +96,28 @@ export class RunPrivateCacheRepo extends TaskOutputRepository { /** * Delete every entry written through this wrapper's `runId`. Called by the - * graph runner after a successful run, and by the janitor for stale runs. - * Requires the backing repository to implement `deleteByTaskTypePrefix`. + * graph runner after a successful run. An indexed `deleteSearch({ runId })` + * on the backing's runId-leading primary key — not a table scan. */ public async clearRun(): Promise { - await this.backing.deleteByTaskTypePrefix(`__run:${this.runId}::`); + await this.backing.deleteRun(this.runId); } /** - * Returns the count of entries namespaced under THIS wrapper's `runId`. - * Consistent with `saveOutput`/`getOutput`/`clear()` being run-scoped. + * Returns the count of entries for THIS wrapper's `runId`. Consistent with + * `saveOutput`/`getOutput`/`clear()` being run-scoped. */ public async size(): Promise { - return this.backing.sizeByTaskTypePrefix(`__run:${this.runId}::`); + return this.backing.sizeForRun(this.runId); } /** * Override of `TaskOutputRepository.clearOlderThan()` scoped to THIS - * wrapper's `runId`. Without the scope override, the wrapper would - * accidentally prune the entire backing store (including deterministic - * cache entries and other runs' private rows). + * wrapper's `runId`. Without the scope, the wrapper would prune other runs' + * private rows. An indexed `deleteSearch({ runId, createdAt: { "<" } })`. */ public async clearOlderThan(olderThanInMs: number): Promise { - await this.backing.clearOlderThanWithTaskTypePrefix(`__run:${this.runId}::`, olderThanInMs); + await this.backing.deleteRunOlderThan(this.runId, olderThanInMs); } public isDurable(): boolean { diff --git a/packages/task-graph/src/common.ts b/packages/task-graph/src/common.ts index b995546c3..49f334d35 100644 --- a/packages/task-graph/src/common.ts +++ b/packages/task-graph/src/common.ts @@ -50,6 +50,8 @@ export * from "./storage/ITaskOutputStorage"; export * from "./storage/TabularTaskOutputStorage"; export * from "./storage/TaskGraphRepository"; export * from "./storage/TaskGraphTabularRepository"; +export * from "./storage/RunPrivateTaskOutputRepository"; +export * from "./storage/RunPrivateTaskOutputSchema"; export * from "./storage/TaskOutputRepository"; export * from "./storage/TaskOutputStorageSchema"; export * from "./storage/TaskOutputTabularRepository"; diff --git a/packages/task-graph/src/storage/ITaskOutputStorage.ts b/packages/task-graph/src/storage/ITaskOutputStorage.ts index b0e022d35..95a501124 100644 --- a/packages/task-graph/src/storage/ITaskOutputStorage.ts +++ b/packages/task-graph/src/storage/ITaskOutputStorage.ts @@ -10,7 +10,15 @@ import type { SearchCondition } from "@workglow/storage"; export interface TaskOutputRow { readonly taskType: string; readonly key: string; - /** Serialized (optionally compressed) output payload. */ + /** + * Serialized output payload. NOTE: although typed `string`, the compression + * path actually stores raw bytes here (a Uint8Array/Buffer written through an + * `as unknown as string` cast in TaskOutputTabularRepository.saveOutput). The + * declared type is kept `string` to match the blob column's schema-derived + * tabular type; a backing store may round-trip those bytes as a Uint8Array, a + * number[] array, or a numeric-keyed object, and the reader in + * {@link TaskOutputTabularRepository.getOutput} normalizes all three shapes. + */ readonly value: string; readonly createdAt: string; } diff --git a/packages/task-graph/src/storage/RunPrivateTaskOutputRepository.ts b/packages/task-graph/src/storage/RunPrivateTaskOutputRepository.ts new file mode 100644 index 000000000..2f509ade3 --- /dev/null +++ b/packages/task-graph/src/storage/RunPrivateTaskOutputRepository.ts @@ -0,0 +1,142 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ITabularStorage } from "@workglow/storage"; +import { makeFingerprint } from "@workglow/util"; +import type { TaskInput, TaskOutput } from "../task/TaskTypes"; +import { + RunPrivateTaskOutputPrimaryKeyNames, + RunPrivateTaskOutputSchema, +} from "./RunPrivateTaskOutputSchema"; +import { decodeTaskOutput, encodeTaskOutput } from "./taskOutputCodec"; +import { TaskOutputRepository } from "./TaskOutputRepository"; + +/** Backing tabular table type for {@link RunPrivateTaskOutputRepository}. */ +export type RunPrivateTaskOutputBacking = ITabularStorage< + typeof RunPrivateTaskOutputSchema, + typeof RunPrivateTaskOutputPrimaryKeyNames +>; + +export type RunPrivateTaskOutputRepositoryOptions = { + storage: RunPrivateTaskOutputBacking; + outputCompression?: boolean; +}; + +/** + * Dedicated backing repository for the run-private output cache. Rows carry a + * first-class `runId` column with a runId-leading primary key, so run-scoped + * cleanup (`deleteRun`, `deleteRunOlderThan`) and the all-runs janitor sweep + * (`clearOlderThan`) are indexed deletes rather than full-table scans, and two + * runs writing the same `(taskType, inputs)` do not collide. + * + * The run-agnostic `saveOutput`/`getOutput` are intentionally unsupported here: + * every private write/read carries a `runId` and goes through the `*ForRun` + * methods (driven by {@link RunPrivateCacheRepo}). + */ +export class RunPrivateTaskOutputRepository extends TaskOutputRepository { + readonly storage: RunPrivateTaskOutputBacking; + + constructor({ storage, outputCompression }: RunPrivateTaskOutputRepositoryOptions) { + super({ outputCompression }); + this.storage = storage; + this.outputCompression = outputCompression ?? true; + } + + public isDurable(): boolean { + return this.storage.isDurable?.() ?? true; + } + + async setupDatabase(): Promise { + await this.storage.setupDatabase?.(); + } + + public async keyFromInputs(inputs: TaskInput): Promise { + return await makeFingerprint(inputs); + } + + override async saveOutputForRun( + runId: string, + taskType: string, + inputs: TaskInput, + output: TaskOutput, + createdAt = new Date() + ): Promise { + const key = await this.keyFromInputs(inputs); + const value = await encodeTaskOutput(output, this.outputCompression); + await this.storage.put({ + runId, + taskType, + key, + // Blob column: raw bytes stored under a string-typed schema (see + // TaskOutputRow.value); getOutputForRun normalizes the round-tripped shape. + value: value as unknown as string, + createdAt: createdAt.toISOString(), + }); + this.emit("output_saved", taskType); + } + + override async getOutputForRun( + runId: string, + taskType: string, + inputs: TaskInput + ): Promise { + const key = await this.keyFromInputs(inputs); + const row = await this.storage.get({ runId, key, taskType }); + if (!row?.value) return undefined; + // Emit only on an actual hit so hit-rate metrics keyed off this event are + // not inflated by misses. + this.emit("output_retrieved", taskType); + return await decodeTaskOutput(row.value, this.outputCompression); + } + + override async deleteRun(runId: string): Promise { + await this.storage.deleteSearch({ runId }); + this.emit("output_pruned"); + } + + override async deleteRunOlderThan(runId: string, olderThanInMs: number): Promise { + const cutoff = new Date(Date.now() - olderThanInMs).toISOString(); + await this.storage.deleteSearch({ runId, createdAt: { value: cutoff, operator: "<" } }); + this.emit("output_pruned"); + } + + override async sizeForRun(runId: string): Promise { + // count() tallies matching rows without loading (and decoding) every blob. + return await this.storage.count({ runId }); + } + + /** + * All-runs age sweep for the janitor. The private table holds only run-private + * rows, so deleting everything older than the cutoff is the correct stale-row + * reap — served by the `createdAt` index. + */ + async clearOlderThan(olderThanInMs: number): Promise { + const cutoff = new Date(Date.now() - olderThanInMs).toISOString(); + await this.storage.deleteSearch({ createdAt: { value: cutoff, operator: "<" } }); + this.emit("output_pruned"); + } + + async clear(): Promise { + await this.storage.deleteAll(); + this.emit("output_cleared"); + } + + async size(): Promise { + return await this.storage.size(); + } + + async saveOutput(): Promise { + throw new Error( + "RunPrivateTaskOutputRepository requires a runId — use saveOutputForRun (via RunPrivateCacheRepo)." + ); + } + + async getOutput(): Promise { + throw new Error( + "RunPrivateTaskOutputRepository requires a runId — use getOutputForRun (via RunPrivateCacheRepo)." + ); + } +} diff --git a/packages/task-graph/src/storage/RunPrivateTaskOutputSchema.ts b/packages/task-graph/src/storage/RunPrivateTaskOutputSchema.ts new file mode 100644 index 000000000..bce7573cf --- /dev/null +++ b/packages/task-graph/src/storage/RunPrivateTaskOutputSchema.ts @@ -0,0 +1,35 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { DataPortSchemaObject } from "@workglow/util/schema"; + +export type RunPrivateTaskOutputPrimaryKey = { + runId: string; + key: string; + taskType: string; +}; + +/** + * Storage shape for the run-private output cache. Distinct from + * {@link TaskOutputSchema}: it carries a first-class `runId` column and a + * runId-leading primary key. That makes run-scoped cleanup (`clearRun`, the + * janitor sweep) an indexed delete rather than a full-table scan, and keeps two + * runs that execute the same task with identical inputs from colliding on + * `(key, taskType)`. + */ +export const RunPrivateTaskOutputSchema = { + type: "object", + properties: { + runId: { type: "string" }, + key: { type: "string" }, + taskType: { type: "string" }, + value: { type: "string", contentEncoding: "blob" }, + createdAt: { type: "string", format: "date-time" }, + }, + additionalProperties: false, +} satisfies DataPortSchemaObject; + +export const RunPrivateTaskOutputPrimaryKeyNames = ["runId", "key", "taskType"] as const; diff --git a/packages/task-graph/src/storage/TaskOutputRepository.ts b/packages/task-graph/src/storage/TaskOutputRepository.ts index 1a3b8ed5e..1111160bd 100644 --- a/packages/task-graph/src/storage/TaskOutputRepository.ts +++ b/packages/task-graph/src/storage/TaskOutputRepository.ts @@ -97,42 +97,59 @@ export abstract class TaskOutputRepository { abstract isDurable(): boolean; /** - * Delete every entry whose `taskType` starts with `prefix`. Used by - * `RunPrivateCacheRepo.clearRun()` to delete entries for a specific `runId`. + * Run-scoped write for the private cache: persist an entry under a specific + * `runId`. Used by {@link RunPrivateCacheRepo} so the run id is a first-class + * column (indexed) rather than a `taskType` prefix. * - * Default implementation throws — backing repositories that support run-private - * caching MUST override this. + * Default implementation throws — only run-private backing repositories + * (e.g. `RunPrivateTaskOutputRepository`) implement it. */ - async deleteByTaskTypePrefix(_prefix: string): Promise { + async saveOutputForRun( + _runId: string, + _taskType: string, + _inputs: TaskInput, + _output: TaskOutput, + _createdAt?: Date + ): Promise { throw new Error( - `${this.constructor.name}: deleteByTaskTypePrefix is not supported by this repository.` + `${this.constructor.name}: saveOutputForRun is not supported by this repository.` ); } - /** - * Delete entries whose `taskType` starts with `prefix` and were created more - * than `olderThanMs` ago. Used by `CacheJanitor.sweepStaleRunPrivate()`. - * - * Default implementation throws — backing repositories that support periodic - * janitor sweeps of run-private rows MUST override this. - */ - async clearOlderThanWithTaskTypePrefix(_prefix: string, _olderThanMs: number): Promise { + /** Run-scoped read counterpart to {@link saveOutputForRun}. Default throws. */ + async getOutputForRun( + _runId: string, + _taskType: string, + _inputs: TaskInput + ): Promise { throw new Error( - `${this.constructor.name}: clearOlderThanWithTaskTypePrefix is not supported by this repository.` + `${this.constructor.name}: getOutputForRun is not supported by this repository.` ); } /** - * Count entries whose `taskType` starts with `prefix`. Used by - * `RunPrivateCacheRepo.size()` so the wrapper's count reflects only its own - * namespaced view rather than the entire backing store. - * - * Default implementation throws — backing repositories that support run-private - * caching MUST override this. + * Delete every entry for `runId`. Used by `RunPrivateCacheRepo.clearRun()`. + * Indexed on the run-private schema's runId-leading primary key. Default throws. + */ + async deleteRun(_runId: string): Promise { + throw new Error(`${this.constructor.name}: deleteRun is not supported by this repository.`); + } + + /** + * Delete entries for `runId` created more than `olderThanMs` ago. Used by + * `RunPrivateCacheRepo.clearOlderThan()`. Default throws. */ - async sizeByTaskTypePrefix(_prefix: string): Promise { + async deleteRunOlderThan(_runId: string, _olderThanMs: number): Promise { throw new Error( - `${this.constructor.name}: sizeByTaskTypePrefix is not supported by this repository.` + `${this.constructor.name}: deleteRunOlderThan is not supported by this repository.` ); } + + /** + * Count entries for `runId`. Used by `RunPrivateCacheRepo.size()` so the + * wrapper's count reflects only its own run. Default throws. + */ + async sizeForRun(_runId: string): Promise { + throw new Error(`${this.constructor.name}: sizeForRun is not supported by this repository.`); + } } diff --git a/packages/task-graph/src/storage/TaskOutputTabularRepository.ts b/packages/task-graph/src/storage/TaskOutputTabularRepository.ts index 036e5fd56..58ed468a2 100644 --- a/packages/task-graph/src/storage/TaskOutputTabularRepository.ts +++ b/packages/task-graph/src/storage/TaskOutputTabularRepository.ts @@ -5,10 +5,10 @@ */ import { makeFingerprint } from "@workglow/util"; -import { compress, decompress } from "@workglow/util/compress"; import { TaskInput, TaskOutput } from "../task/TaskTypes"; import type { ITaskOutputStorage } from "./ITaskOutputStorage"; import type { TaskOutputTabularBacking } from "./TabularTaskOutputStorage"; +import { decodeTaskOutput, encodeTaskOutput } from "./taskOutputCodec"; import { TaskOutputRepository } from "./TaskOutputRepository"; export { TaskOutputPrimaryKeyNames, TaskOutputSchema } from "./TaskOutputStorageSchema"; @@ -53,58 +53,26 @@ export class TaskOutputTabularRepository extends TaskOutputRepository { createdAt = new Date() ): Promise { const key = await this.keyFromInputs(inputs); - const value = JSON.stringify(output); - if (this.outputCompression) { - const compressedValue = await compress(value); - await this.storage.put({ - taskType, - key, - value: compressedValue as unknown as string, - createdAt: createdAt.toISOString(), - }); - } else { - const valueBuffer = Buffer.from(value); - await this.storage.put({ - taskType, - key, - value: valueBuffer as unknown as string, - createdAt: createdAt.toISOString(), - }); - } + const value = await encodeTaskOutput(output, this.outputCompression); + await this.storage.put({ + taskType, + key, + // Blob column: raw bytes stored under a string-typed schema (see + // TaskOutputRow.value); getOutput normalizes the round-tripped shape. + value: value as unknown as string, + createdAt: createdAt.toISOString(), + }); this.emit("output_saved", taskType); } async getOutput(taskType: string, inputs: TaskInput): Promise { const key = await this.keyFromInputs(inputs); const output = await this.storage.get({ key, taskType }); + if (!output?.value) return undefined; + // Emit only on an actual hit so hit-rate metrics keyed off this event are + // not inflated by misses. this.emit("output_retrieved", taskType); - if (output?.value) { - if (this.outputCompression) { - const raw: unknown = output.value as unknown; - const bytes: Uint8Array = - raw instanceof Uint8Array - ? raw - : Array.isArray(raw) - ? new Uint8Array(raw as number[]) - : raw && typeof raw === "object" - ? new Uint8Array( - Object.keys(raw as Record) - .filter((k) => /^\d+$/.test(k)) - .sort((a, b) => Number(a) - Number(b)) - .map((k) => (raw as Record)[k]) - ) - : new Uint8Array(); - const decompressedValue = await decompress(bytes); - const value = JSON.parse(decompressedValue) as TaskOutput; - return value as TaskOutput; - } else { - const stringValue = output.value.toString(); - const value = JSON.parse(stringValue) as TaskOutput; - return value as TaskOutput; - } - } else { - return undefined; - } + return await decodeTaskOutput(output.value, this.outputCompression); } async clear(): Promise { @@ -121,37 +89,4 @@ export class TaskOutputTabularRepository extends TaskOutputRepository { await this.storage.deleteSearch({ createdAt: { value: date, operator: "<" } }); this.emit("output_pruned"); } - - override async deleteByTaskTypePrefix(prefix: string): Promise { - for await (const row of this.storage.records()) { - if (typeof row.taskType === "string" && row.taskType.startsWith(prefix)) { - await this.storage.delete({ key: row.key, taskType: row.taskType }); - } - } - } - - override async clearOlderThanWithTaskTypePrefix( - prefix: string, - olderThanInMs: number - ): Promise { - const cutoff = Date.now() - olderThanInMs; - for await (const row of this.storage.records()) { - if (typeof row.taskType === "string" && row.taskType.startsWith(prefix)) { - const ts = typeof row.createdAt === "string" ? new Date(row.createdAt).getTime() : NaN; - if (!isNaN(ts) && ts < cutoff) { - await this.storage.delete({ key: row.key, taskType: row.taskType }); - } - } - } - } - - override async sizeByTaskTypePrefix(prefix: string): Promise { - let count = 0; - for await (const row of this.storage.records()) { - if (typeof row.taskType === "string" && row.taskType.startsWith(prefix)) { - count++; - } - } - return count; - } } diff --git a/packages/task-graph/src/storage/taskOutputCodec.ts b/packages/task-graph/src/storage/taskOutputCodec.ts new file mode 100644 index 000000000..c1fa29a53 --- /dev/null +++ b/packages/task-graph/src/storage/taskOutputCodec.ts @@ -0,0 +1,62 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import { compress, decompress } from "@workglow/util/compress"; +import type { TaskOutput } from "../task/TaskTypes"; + +/** + * Encodes a task output to the bytes stored in a cache row's `value` column. + * The blob column is declared `string` in the schema, so callers store the + * returned bytes through an `as unknown as string` cast (see TaskOutputRow.value). + */ +export async function encodeTaskOutput( + output: TaskOutput, + outputCompression: boolean +): Promise { + const value = JSON.stringify(output); + // TextEncoder (not Buffer.from) so the uncompressed path works in browser + // builds, where Buffer is not defined. + return outputCompression ? await compress(value) : new TextEncoder().encode(value); +} + +/** + * Normalizes the `value` a backing store round-tripped into bytes. A backend may + * hand the blob back as a Uint8Array, a number[], or a numeric-keyed object + * (structured-clone / JSON round-trips), so all three shapes are handled. + */ +function toBytes(rawValue: unknown): Uint8Array { + if (rawValue instanceof Uint8Array) return rawValue; + if (Array.isArray(rawValue)) return new Uint8Array(rawValue as number[]); + if (rawValue && typeof rawValue === "object") { + return new Uint8Array( + Object.keys(rawValue as Record) + .filter((k) => /^\d+$/.test(k)) + .sort((a, b) => Number(a) - Number(b)) + .map((k) => (rawValue as Record)[k]) + ); + } + return new Uint8Array(); +} + +/** + * Decodes the `value` a backing store round-tripped back into a task output. + * Mirrors {@link encodeTaskOutput}: gzip when compressed, otherwise UTF-8 JSON. + */ +export async function decodeTaskOutput( + rawValue: unknown, + outputCompression: boolean +): Promise { + if (outputCompression) { + const decompressed = await decompress(toBytes(rawValue)); + return JSON.parse(decompressed) as TaskOutput; + } + // Uncompressed: durable backends (SQL/IndexedDB) hand the blob back as bytes, + // not a string, so decode the bytes as UTF-8 rather than calling `.toString()` + // on a Uint8Array (which would yield comma-separated byte numbers). + const json = + typeof rawValue === "string" ? rawValue : new TextDecoder().decode(toBytes(rawValue)); + return JSON.parse(json) as TaskOutput; +} diff --git a/packages/task-graph/src/task-graph/ConditionalBuilder.ts b/packages/task-graph/src/task-graph/ConditionalBuilder.ts index 21c541039..1e2f2ba07 100644 --- a/packages/task-graph/src/task-graph/ConditionalBuilder.ts +++ b/packages/task-graph/src/task-graph/ConditionalBuilder.ts @@ -10,8 +10,9 @@ import { ConditionalTask } from "../task/ConditionalTask"; import type { ITask, ITaskConstructor } from "../task/ITask"; import { WorkflowError } from "../task/TaskError"; import type { DataPorts, TaskConfig, TaskInput } from "../task/TaskTypes"; -import { Dataflow } from "./Dataflow"; +import { Dataflow, DATAFLOW_ALL_PORTS } from "./Dataflow"; import type { Workflow } from "./Workflow"; +import { getLastTask } from "./WorkflowPipe"; /** * Fluent builder for constructing a {@link ConditionalTask} with a @@ -74,6 +75,12 @@ export class ConditionalBuilder { throw new WorkflowError(".endIf() called without a prior .then(...) call"); } + // Capture the immediately-preceding workflow task BEFORE the conditional is + // added so its output can be wired INTO the conditional's input. Without + // this, the ConditionalTask runs against empty input and the predicate sees + // `{}` rather than the upstream task's output. + const priorTask = getLastTask(this.workflow); + const thenPort = "then"; const elsePort = "else"; @@ -105,6 +112,16 @@ export class ConditionalBuilder { }); this.workflow.graph.addTask(conditionalTask); + // Wire the prior task's output into the conditional's input so the predicate + // evaluates against real upstream data. ConditionalTask accepts any input + // (additionalProperties: true), so an all-ports edge passes the full output + // through, mirroring the all-ports wiring used by connect()/onError(). + if (priorTask && priorTask.id !== conditionalTask.id) { + this.workflow.graph.addDataflow( + new Dataflow(priorTask.id, DATAFLOW_ALL_PORTS, conditionalTask.id, DATAFLOW_ALL_PORTS) + ); + } + const thenTask = instantiate(this.thenSpec); this.workflow.graph.addTask(thenTask); this.workflow.graph.addDataflow(new Dataflow(conditionalTask.id, thenPort, thenTask.id, "*")); @@ -115,6 +132,13 @@ export class ConditionalBuilder { this.workflow.graph.addDataflow(new Dataflow(conditionalTask.id, elsePort, elseTask.id, "*")); } + // Record the branch terminus so a continuation can be validated. A two-arm + // conditional leaves two mutually-exclusive leaf nodes with no join, so the + // next .addTask(...) cannot pick a single predecessor and must throw. A + // then-only conditional leaves a single leaf (the then task), which is a + // safe predecessor to continue from. + this.workflow.builder.markBranchTerminus(this.elseSpec ? 2 : 1); + return this.workflow; } } diff --git a/packages/task-graph/src/task-graph/Dataflow.ts b/packages/task-graph/src/task-graph/Dataflow.ts index 74193a969..043ceec8a 100644 --- a/packages/task-graph/src/task-graph/Dataflow.ts +++ b/packages/task-graph/src/task-graph/Dataflow.ts @@ -474,7 +474,11 @@ export class Dataflow { */ export class DataflowArrow extends Dataflow { constructor(dataflow: DataflowIdType) { - const pattern = /^([a-z0-9-]+)\[([a-z0-9-]+)\] ==> ([a-z0-9-]+)\[([a-z0-9-]+)\]$/i; + // Match the id grammar `id[port] ==> id[port]` produced by Dataflow.createId. + // ids/ports may contain any character EXCEPT the structural `[` and `]` + // delimiters (underscores, camelCase, and the `*` all-ports sentinel are all + // valid), so accept exactly that — the parser is the inverse of createId. + const pattern = /^([^[\]]+)\[([^[\]]*)\] ==> ([^[\]]+)\[([^[\]]*)\]$/; const match = dataflow.match(pattern); if (!match) { diff --git a/packages/task-graph/src/task-graph/LoopBuilderContext.ts b/packages/task-graph/src/task-graph/LoopBuilderContext.ts index 94c965cb8..5e4c8dba8 100644 --- a/packages/task-graph/src/task-graph/LoopBuilderContext.ts +++ b/packages/task-graph/src/task-graph/LoopBuilderContext.ts @@ -74,10 +74,19 @@ export class LoopBuilderContext { /** * Promotes a populated child template graph into the iterator task's - * subGraph. No-op on empty graphs. + * subGraph. No-op on empty graphs (an empty loop body is almost certainly a + * mistake — the iterator keeps its default/empty subGraph and the failure + * surfaces later at run as iteration-over-nothing — so warn to make it + * diagnosable at the point of the mistake). */ public finalizeTemplate(childGraph: TaskGraph): void { - if (childGraph.getTasks().length === 0) return; + if (childGraph.getTasks().length === 0) { + getLogger().warn( + `Loop body for iterator task ${this.iteratorTask.config.id} is empty; ` + + "the loop has no tasks to iterate. Add at least one task inside the loop builder." + ); + return; + } this.iteratorTask.subGraph = childGraph; this.iteratorTask.validateAcyclic(); } diff --git a/packages/task-graph/src/task-graph/RunContext.ts b/packages/task-graph/src/task-graph/RunContext.ts index b57e88f44..8ba691300 100644 --- a/packages/task-graph/src/task-graph/RunContext.ts +++ b/packages/task-graph/src/task-graph/RunContext.ts @@ -35,8 +35,13 @@ export class RunContext { // constructor when parentSignal is provided; called from dispose(). private parentSignalCleanup?: () => void; - constructor(parentSignal?: AbortSignal) { - this.runId = uuid4(); + constructor(parentSignal?: AbortSignal, runId?: string) { + // Derive from the caller-supplied runId when present so the same identifier + // flows through to per-task runnerId (and thus queued-job jobRunId). This + // lets a caller holding the graph's runId reliably target the run's queued + // jobs via the queue's runId-keyed operations (e.g. abortJobRun(runId)). + // Falls back to a fresh uuid when no runId was provided. + this.runId = runId ?? uuid4(); this.abortController = new AbortController(); if (parentSignal) { // Listen first, then check — addEventListener on an already-aborted signal diff --git a/packages/task-graph/src/task-graph/RunScheduler.ts b/packages/task-graph/src/task-graph/RunScheduler.ts index dfca68805..540979c7c 100644 --- a/packages/task-graph/src/task-graph/RunScheduler.ts +++ b/packages/task-graph/src/task-graph/RunScheduler.ts @@ -7,7 +7,7 @@ import { getLogger } from "@workglow/util"; import { ConditionalTask } from "../task/ConditionalTask"; import type { ITask } from "../task/ITask"; -import { TaskError, TaskGraphTimeoutError } from "../task/TaskError"; +import { TaskError, TaskFailedError, TaskGraphTimeoutError } from "../task/TaskError"; import type { TaskInput, TaskOutput } from "../task/TaskTypes"; import { TaskStatus } from "../task/TaskTypes"; import type { EdgeMaterializer } from "./EdgeMaterializer"; @@ -17,6 +17,14 @@ import type { GraphResultArray, GraphSingleTaskResult, TaskGraphRunner } from ". import { taskPrototypeHasOwnExecute } from "./TaskGraphRunner"; import type { ITaskGraphScheduler } from "./TaskGraphScheduler"; +/** + * Key used to record a scheduler-iterator-level failure in + * `ctx.failedTaskErrors` (as opposed to a per-task failure, which is keyed by + * task id). Lets the runGraph epilogue treat a scheduler throw as a graph + * failure rather than completing on partial results. + */ +const SCHEDULER_FAILURE_KEY = Symbol("scheduler-failure"); + /** * @internal * Run-loop coordinator. Drives task selection via processScheduler, @@ -345,6 +353,15 @@ export class RunScheduler { } } catch (err) { getLogger().error("Error running graph", { error: err }); + // A throw from the scheduler iterator itself (not an individual task) must + // propagate as a graph failure, not be logged-and-dropped on the success + // path. Record it so the runGraph epilogue throws instead of calling + // handleComplete and emitting "complete" on a partial run. + const schedulerError = + err instanceof TaskError + ? err + : new TaskFailedError(err instanceof Error ? err.message : String(err)); + ctx.failedTaskErrors.set(SCHEDULER_FAILURE_KEY, schedulerError); } // Wait for all tasks to complete since we did not await runAsync()/this.runTaskWithProvenance() diff --git a/packages/task-graph/src/task-graph/StreamPump.ts b/packages/task-graph/src/task-graph/StreamPump.ts index b53515cc4..8e47b80bd 100644 --- a/packages/task-graph/src/task-graph/StreamPump.ts +++ b/packages/task-graph/src/task-graph/StreamPump.ts @@ -5,6 +5,7 @@ */ import type { ResourceScope, ServiceRegistry } from "@workglow/util"; +import { getLogger } from "@workglow/util"; import type { TaskOutputRepository } from "../storage/TaskOutputRepository"; import type { ITask } from "../task/ITask"; import type { StreamEvent, StreamMode } from "../task/StreamTypes"; @@ -166,16 +167,32 @@ export class StreamPump { } }; + // Guarded so a throwing listener on the (possibly bridged, cross-graph) + // stream events can't propagate back into the source task's stream loop and + // abort an otherwise-healthy run. Mirrors the guarded task_progress/ + // task_complete emits in RunScheduler. const onStreamStart = () => { - this.graph.emit("task_stream_start", task.id); + try { + this.graph.emit("task_stream_start", task.id); + } catch (err) { + getLogger().error("task_stream_start listener threw", { error: err }); + } }; const onStreamChunk = (event: StreamEvent) => { - this.graph.emit("task_stream_chunk", task.id, event); + try { + this.graph.emit("task_stream_chunk", task.id, event); + } catch (err) { + getLogger().error("task_stream_chunk listener threw", { error: err }); + } }; const onStreamEnd = (output: Record) => { - this.graph.emit("task_stream_end", task.id, output); + try { + this.graph.emit("task_stream_end", task.id, output); + } catch (err) { + getLogger().error("task_stream_end listener threw", { error: err }); + } }; task.on("status", onStatus); @@ -279,11 +296,36 @@ export class StreamPump { ): ReadableStream { return new ReadableStream({ start: (controller) => { + // Single teardown path: closes the controller and detaches every + // listener this stream added. Invoked on normal stream_end AND on a + // terminal task status (FAILED/COMPLETED), because StreamProcessor does + // not emit stream_end on the error/abort path — without the status + // fallback the controller and listeners would leak (and a downstream + // consumer awaiting `done` would hang) on failed/aborted source tasks. + let closed = false; + const cleanup = () => { + if (closed) return; + closed = true; + try { + controller.close(); + } catch { + // Stream may already be closed + } + task.off("stream_chunk", onChunk); + task.off("stream_end", onEnd); + task.off("status", onStatus); + }; const onChunk = (event: StreamEvent) => { try { if (portId !== undefined && StreamPump.isPortDelta(event) && event.port !== portId) { return; } + // Phase events are not accumulated into dataflow edges per the + // StreamTypes contract, so they must not be enqueued onto edge + // streams; drop them here. + if (event.type === "phase") { + return; + } // Tap: on snapshot events, write per-port data into each edge's // latestSnapshot slot. if (event.type === "snapshot") { @@ -304,16 +346,18 @@ export class StreamPump { } }; const onEnd = () => { - try { - controller.close(); - } catch { - // Stream may already be closed + cleanup(); + }; + const onStatus = (status: TaskStatus) => { + // Terminal statuses with no stream_end (error/abort -> FAILED, or a + // completion that bypassed stream_end) must still release the stream. + if (status === TaskStatus.FAILED || status === TaskStatus.COMPLETED) { + cleanup(); } - task.off("stream_chunk", onChunk); - task.off("stream_end", onEnd); }; task.on("stream_chunk", onChunk); task.on("stream_end", onEnd); + task.on("status", onStatus); }, }); } diff --git a/packages/task-graph/src/task-graph/TaskGraph.ts b/packages/task-graph/src/task-graph/TaskGraph.ts index 74b8b7b6c..87924b2cf 100644 --- a/packages/task-graph/src/task-graph/TaskGraph.ts +++ b/packages/task-graph/src/task-graph/TaskGraph.ts @@ -16,7 +16,7 @@ import type { TaskIdType, TaskInput, TaskOutput, TaskStatus } from "../task/Task import type { PipeFunction } from "./Conversions"; import { ensureTask } from "./Conversions"; import type { DataflowIdType } from "./Dataflow"; -import { Dataflow } from "./Dataflow"; +import { Dataflow, DataflowArrow } from "./Dataflow"; import { computeGraphEntitlements } from "./GraphEntitlementUtils"; import { addBoundaryNodesToDependencyJson, addBoundaryNodesToGraphJson } from "./GraphSchemaUtils"; import type { ITaskGraph } from "./ITaskGraph"; @@ -289,6 +289,23 @@ export class TaskGraph implements ITaskGraph { * @returns The data flow with the given id, or undefined if not found */ public getDataflow(id: DataflowIdType): Dataflow | undefined { + // The id encodes the target task, so scan only that node's in-edges instead + // of rebuilding the entire adjacency matrix via getEdges(). Falls back to a + // full scan if the id cannot be parsed or the target task is not present. + let targetTaskId: TaskIdType | undefined; + try { + targetTaskId = new DataflowArrow(id).targetTaskId; + } catch { + targetTaskId = undefined; + } + if (targetTaskId !== undefined && this.getTask(targetTaskId) !== undefined) { + for (const [, , edge] of this._dag.inEdges(targetTaskId)) { + if (edge.id === id) { + return edge; + } + } + return undefined; + } for (const [, , edge] of this._dag.getEdges()) { if (edge.id === id) { return edge; @@ -759,7 +776,7 @@ function serialGraphEdges( ): Dataflow[] { const edges: Dataflow[] = []; for (let i = 0; i < tasks.length - 1; i++) { - edges.push(new Dataflow(tasks[i].id, inputHandle, tasks[i + 1].id, outputHandle)); + edges.push(new Dataflow(tasks[i].id, outputHandle, tasks[i + 1].id, inputHandle)); } return edges; } diff --git a/packages/task-graph/src/task-graph/TaskGraphEvents.ts b/packages/task-graph/src/task-graph/TaskGraphEvents.ts index 68c3f33f9..5e18f29b8 100644 --- a/packages/task-graph/src/task-graph/TaskGraphEvents.ts +++ b/packages/task-graph/src/task-graph/TaskGraphEvents.ts @@ -32,6 +32,14 @@ export type TaskGraphStatusListeners = { * output. Mirrors `task_stream_end` but fires for every task — streaming or * not — so consumers can observe authoritative outputs incrementally rather * than only at graph completion. + * + * NOTE: the same `taskId` may legitimately fire more than once across a run. + * While/Fallback/GraphAsTask reuse the same subGraph (and thus stable task + * ids) across loop iterations, so each iteration re-emits task_complete for + * the identical id. There is no iteration discriminator on this event; + * consumers keying state by taskId should treat a later emit as the + * authoritative overwrite for that task. (Iterator clones mint fresh ids per + * iteration, so they are unaffected.) */ task_complete: (taskId: TaskIdType, output: Record) => void; /** diff --git a/packages/task-graph/src/task-graph/TaskGraphRunner.ts b/packages/task-graph/src/task-graph/TaskGraphRunner.ts index fd2f47812..33cab0ce6 100644 --- a/packages/task-graph/src/task-graph/TaskGraphRunner.ts +++ b/packages/task-graph/src/task-graph/TaskGraphRunner.ts @@ -147,6 +147,14 @@ export class TaskGraphRunner { protected currentRunPrivate?: RunPrivateCacheRepo; protected baseRegistryForRun?: ServiceRegistry; + /** + * Registry to restore after a preview run completes. Captured in + * handleStartPreview when a preview installs its own registry, and restored in + * handleCompletePreview/handleErrorPreview/handleAbortPreview so a preview's + * registry never leaks into a subsequent run() that omits an explicit registry. + */ + protected basePreviewRegistry?: ServiceRegistry; + protected readonly edgeMaterializer: EdgeMaterializer; protected readonly streamPump: StreamPump; @@ -708,8 +716,11 @@ export class TaskGraphRunner { this.running = true; - // Build per-run context (handles abortController + parentSignal wiring) - const ctx = new RunContext(config?.parentSignal); + // Build per-run context (handles abortController + parentSignal wiring). + // Thread the caller-supplied runId so ctx.runId — which becomes each task's + // runnerId via resetGraph — matches the id the caller uses for run-scoped + // queue operations. Falls back to a fresh uuid when no runId was provided. + const ctx = new RunContext(config?.parentSignal, this.runId); this.currentCtx = ctx; ctx.abortController.signal.addEventListener("abort", () => { @@ -786,13 +797,19 @@ export class TaskGraphRunner { } protected async handleStartPreview(config?: TaskGraphRunConfig): Promise { - if (this.previewRunning) { + // Reject if a real run() is in flight: preview and run drive the same shared + // graph/edge state (Dataflow.value/status, task.runInputData), so overlap + // produces torn reads and lets a preview clobber a live run's outputs. + if (this.running || this.previewRunning) { throw new TaskConfigurationError("Graph is already running in preview"); } // Use explicit registry if provided; otherwise keep the existing one // (which is either globalServiceRegistry by default, or whatever handleStart set). + // Save the prior registry so it can be restored after the preview, preventing + // a preview-supplied registry from leaking into later run() calls. if (config?.registry !== undefined) { + this.basePreviewRegistry = this.registry; this.registry = config.registry; } @@ -840,9 +857,21 @@ export class TaskGraphRunner { } protected async handleCompletePreview(): Promise { + this.restorePreviewRegistry(); this.previewRunning = false; } + /** + * Restores the registry captured before a preview installed its own, so the + * preview's registry does not persist into subsequent run() calls. + */ + protected restorePreviewRegistry(): void { + if (this.basePreviewRegistry !== undefined) { + this.registry = this.basePreviewRegistry; + this.basePreviewRegistry = undefined; + } + } + protected async handleError(error: TaskError): Promise { this.clearGraphTimeout(); await Promise.allSettled( @@ -873,6 +902,7 @@ export class TaskGraphRunner { } protected async handleErrorPreview(): Promise { + this.restorePreviewRegistry(); this.previewRunning = false; } @@ -915,6 +945,7 @@ export class TaskGraphRunner { } protected async handleAbortPreview(): Promise { + this.restorePreviewRegistry(); this.previewRunning = false; } diff --git a/packages/task-graph/src/task-graph/Workflow.ts b/packages/task-graph/src/task-graph/Workflow.ts index 3b5b404b5..9dfd2018d 100644 --- a/packages/task-graph/src/task-graph/Workflow.ts +++ b/packages/task-graph/src/task-graph/Workflow.ts @@ -13,7 +13,7 @@ import type { IRunConfig, ITask, ITaskConstructor } from "../task/ITask"; import type { StreamEvent } from "../task/StreamTypes"; import { Task } from "../task/Task"; import type { TaskEntitlements } from "../task/TaskEntitlements"; -import { WorkflowError } from "../task/TaskError"; +import { TaskAbortedError, WorkflowError } from "../task/TaskError"; import type { JsonTaskItem, TaskGraphJson, TaskGraphJsonOptions } from "../task/TaskJSON"; import type { DataPorts, TaskConfig, TaskIdType, TaskInput } from "../task/TaskTypes"; import { autoConnect } from "./autoConnect"; @@ -48,9 +48,22 @@ export interface RenameOptions { export type WorkflowEventListeners = { changed: (id: unknown) => void; reset: () => void; + /** + * Fired when a run fails. Carries a STRINGIFIED error (via `String(error)`), + * unlike the underlying TaskGraph 'error' event which carries the real Error. + * The stringification is intentional for this facade, but it is lossy (stack, + * error class, structured fields are dropped). To inspect the Error object + * (e.g. instanceof checks, .stack), catch the rejection from `run()` — it + * re-throws the original Error. + */ error: (error: string) => void; start: () => void; complete: () => void; + /** + * Fired when a run is cancelled (the rejection from `run()` is a + * TaskAbortedError). Carries the stringified error; see `error` above for the + * same lossiness caveat. Catch `run()`'s rejection for the typed Error. + */ abort: (error: string) => void; /** Fired when a task in the workflow starts streaming */ stream_start: (taskId: TaskIdType) => void; @@ -240,7 +253,14 @@ export class Workflow< this.events.emit("complete"); return results; } catch (error) { - this.events.emit("error", String(error)); + // Surface cancellation on the dedicated 'abort' event (declared on the + // facade) so consumers can distinguish a cancelled run from a failure. + // Other errors continue to fire 'error'. + if (error instanceof TaskAbortedError) { + this.events.emit("abort", String(error)); + } else { + this.events.emit("error", String(error)); + } throw error; } finally { ctx.dispose(); @@ -250,6 +270,13 @@ export class Workflow< } } + /** + * Signals the in-flight run to abort. This only TRIPS the abort signal; it + * does NOT wait for the run to finish unwinding. The returned promise resolves + * on the next microtask, before run()'s finally (dispose/teardown) has run. + * To observe the run fully stopped, await the promise returned by `run()` + * (which rejects with {@link TaskAbortedError} on abort), not this method. + */ public async abort(): Promise { const loopContext = this._builder.loopContext; if (loopContext) { @@ -481,12 +508,13 @@ export class Workflow< config?: Partial, runConfig?: Partial ): Workflow { - return this._builder.addTaskWithAutoConnect( - taskClass, - input, - config, - runConfig - ) as Workflow; + // The public fluent addTask throws on auto-connect failure (consistent with + // connect/rename/onError) so a failed add does not silently no-op and leave + // the chain wired against a missing predecessor. The createWorkflow helper + // path keeps the legacy swallow-into-.error behavior. + return this._builder.addTaskWithAutoConnect(taskClass, input, config, runConfig, { + throwOnAutoConnectError: true, + }) as Workflow; } // ======================================================================== diff --git a/packages/task-graph/src/task-graph/WorkflowBuilder.ts b/packages/task-graph/src/task-graph/WorkflowBuilder.ts index 44f6ce21b..a1ec4f797 100644 --- a/packages/task-graph/src/task-graph/WorkflowBuilder.ts +++ b/packages/task-graph/src/task-graph/WorkflowBuilder.ts @@ -41,6 +41,13 @@ import { getLastTask } from "./WorkflowPipe"; export interface IWorkflowBuilderHandle { readonly loopContext: LoopBuilderContext | undefined; setError(message: string): void; + /** + * Records that the chain currently terminates at a multi-arm conditional + * (then + else) whose branches are mutually-exclusive leaf nodes. A + * subsequent `.addTask(...)` has no unambiguous single predecessor to wire + * from, so the next add throws instead of silently wiring off one arm. + */ + markBranchTerminus(branchCount: number): void; } export class WorkflowBuilder implements IWorkflowBuilderHandle { @@ -49,6 +56,12 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { private _error: string = ""; private readonly _registry?: ServiceRegistry; private readonly _loopContext?: LoopBuilderContext; + /** + * Number of leaf arms the chain currently terminates at after a two-arm + * `.endIf()`. >1 means continuation is ambiguous (no single join node), so + * the next add is rejected. Reset to 0 by any successful add. + */ + private _branchTerminusArms: number = 0; constructor( facade: Workflow, @@ -81,10 +94,60 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { this._error = message; } + public markBranchTerminus(branchCount: number): void { + this._branchTerminusArms = branchCount; + } + + /** + * Throws if the chain terminates at a multi-arm conditional, where there is + * no single, unambiguous predecessor to auto-connect a continuation from. + * Callers must explicitly merge the branch outputs (e.g. via `connect()`) + * before continuing the chain. + */ + private assertNoAmbiguousBranchTerminus(): void { + if (this._branchTerminusArms > 1) { + throw new WorkflowError( + "Cannot add a task after a two-arm .if().then().else().endIf(): the then/else arms are " + + "separate leaf nodes with no join, so auto-connect cannot pick a single predecessor. " + + "Merge the branch outputs explicitly with connect() before continuing the chain." + ); + } + } + /** Clears pending state. Called by the facade's graph setter and reset(). */ public resetState(): void { this._dataFlows = []; this._error = ""; + this._branchTerminusArms = 0; + } + + /** + * Drains pending dataflows queued by `.rename(...)` onto the newly-added task: + * validates each against the task's input schema (boolean-schema handling, + * additionalProperties, and the DATAFLOW_ALL_PORTS sentinel), sets the error + * on mismatch, fills in the target task id, and adds the dataflow. Shared by + * the regular and loop add paths so the validation predicate lives in one place. + */ + private drainPendingDataflows(task: ITask): void { + if (this._dataFlows.length === 0) return; + this._dataFlows.forEach((dataflow) => { + const taskSchema = task.inputSchema(); + if ( + (typeof taskSchema !== "boolean" && + taskSchema.properties?.[dataflow.targetTaskPortId] === undefined && + taskSchema.additionalProperties !== true) || + (taskSchema === true && dataflow.targetTaskPortId !== DATAFLOW_ALL_PORTS) + ) { + this._error = `Input ${dataflow.targetTaskPortId} not found on task ${task.id}`; + getLogger().error(this._error); + return; + } + + dataflow.targetTaskId = task.id; + this._facade.graph.addDataflow(dataflow); + }); + + this._dataFlows = []; } /** @@ -124,8 +187,11 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { taskClass: ITaskConstructor, input: Partial = {}, config: Partial = {}, - runConfig?: Partial + runConfig?: Partial, + options?: { readonly throwOnAutoConnectError?: boolean } ): Workflow { + this.assertNoAmbiguousBranchTerminus(); + this._branchTerminusArms = 0; this._error = ""; const parent = getLastTask(this._facade); @@ -141,26 +207,7 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { ); // Process any pending data flows - if (this._dataFlows.length > 0) { - this._dataFlows.forEach((dataflow) => { - const taskSchema = task.inputSchema(); - if ( - (typeof taskSchema !== "boolean" && - taskSchema.properties?.[dataflow.targetTaskPortId] === undefined && - taskSchema.additionalProperties !== true) || - (taskSchema === true && dataflow.targetTaskPortId !== DATAFLOW_ALL_PORTS) - ) { - this._error = `Input ${dataflow.targetTaskPortId} not found on task ${task.id}`; - getLogger().error(this._error); - return; - } - - dataflow.targetTaskId = task.id; - this._facade.graph.addDataflow(dataflow); - }); - - this._dataFlows = []; - } + this.drainPendingDataflows(task); // Auto-connect to parent if needed if (parent) { @@ -188,7 +235,9 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { if (result.error) { // In loop builder mode, don't remove the task - allow manual connection - // In normal mode, remove the task since auto-connect is required + // In normal mode, remove the task since auto-connect is required, and + // throw so the failure surfaces at the call site instead of being + // silently swallowed into `.error` (consistent with connect/rename/onError). if (this._loopContext !== undefined) { this._error = result.error; getLogger().warn(this._error); @@ -196,6 +245,9 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { this._error = result.error + " Task not added."; getLogger().error(this._error); this._facade.graph.removeTask(task.id); + if (options?.throwOnAutoConnectError) { + throw new WorkflowError(this._error); + } } } } @@ -222,6 +274,8 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { config: Partial = {}, runConfig?: Partial ): Workflow { + this.assertNoAmbiguousBranchTerminus(); + this._branchTerminusArms = 0; this._error = ""; const parent = getLastTask(this._facade); @@ -250,26 +304,7 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { ); // Process any pending data flows (same as addTaskWithAutoConnect) - if (this._dataFlows.length > 0) { - this._dataFlows.forEach((dataflow) => { - const taskSchema = task.inputSchema(); - if ( - (typeof taskSchema !== "boolean" && - taskSchema.properties?.[dataflow.targetTaskPortId] === undefined && - taskSchema.additionalProperties !== true) || - (taskSchema === true && dataflow.targetTaskPortId !== DATAFLOW_ALL_PORTS) - ) { - this._error = `Input ${dataflow.targetTaskPortId} not found on task ${task.id}`; - getLogger().error(this._error); - return; - } - - dataflow.targetTaskId = task.id; - this._facade.graph.addDataflow(dataflow); - }); - - this._dataFlows = []; - } + this.drainPendingDataflows(task); // Defer auto-connect until endMap/endReduce/endWhile, when the iterator task // has its template graph populated and its dynamic inputSchema is available. @@ -343,6 +378,9 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { const dataflow = new Dataflow(sourceTaskId, sourceTaskPortId, targetTaskId, targetTaskPortId); this._facade.graph.addDataflow(dataflow); + // An explicit connect resolves the ambiguity left by a two-arm endIf: + // the caller has now wired the branch outputs to a join of their choosing. + this._branchTerminusArms = 0; } /** @@ -358,14 +396,20 @@ export class WorkflowBuilder implements IWorkflowBuilderHandle { const transforms = typeof indexOrOptions === "number" ? undefined : indexOrOptions.transforms; const nodes = this._facade.graph.getTasks(); - if (-index > nodes.length) { - const errorMsg = `Back index greater than number of tasks`; + // `index` is a back-offset from the end (default -1 = last task). Validate + // the resolved position fully: the old `-index > nodes.length` guard only + // caught the too-negative side, so index 0 / any non-negative index fell + // through to `nodes[nodes.length + index]` === undefined and crashed with a + // cryptic TypeError instead of a WorkflowError. + const resolvedIndex = nodes.length + index; + if (index >= 0 || resolvedIndex < 0 || resolvedIndex >= nodes.length) { + const errorMsg = `rename() index ${index} out of range; use a negative back-offset from the end (e.g. -1 for the last task) within [-${nodes.length}, -1]`; this._error = errorMsg; getLogger().error(this._error); throw new WorkflowError(errorMsg); } - const lastNode = nodes[nodes.length + index]; + const lastNode = nodes[resolvedIndex]; const outputSchema = lastNode.outputSchema(); // Handle boolean schemas diff --git a/packages/task-graph/src/task-graph/WorkflowEventBridge.ts b/packages/task-graph/src/task-graph/WorkflowEventBridge.ts index ed09a7c74..f2dc274fa 100644 --- a/packages/task-graph/src/task-graph/WorkflowEventBridge.ts +++ b/packages/task-graph/src/task-graph/WorkflowEventBridge.ts @@ -14,8 +14,10 @@ import type { WorkflowEventListeners } from "./Workflow"; * events to a Workflow's EventEmitter. Owns subscription lifecycle: * attach(graph) → subscribe to mutation + entitlement events * detach() → unsubscribe (called on graph swap and reset) - * beginRun() → subscribe to streaming events for the current run - * endRun() → unsubscribe streaming + * beginRun() → subscribe to streaming events for the current run and + * return an unsubscribe token the caller MUST hold and + * invoke when the run ends (there is no endRun() method; + * teardown is the returned token, kept on the run context). * * The bridge does NOT own the events emitter — the facade Workflow does. The * bridge holds a reference and emits through it. diff --git a/packages/task-graph/src/task/CacheCoordinator.ts b/packages/task-graph/src/task/CacheCoordinator.ts index 187c9ce63..948263c42 100644 --- a/packages/task-graph/src/task/CacheCoordinator.ts +++ b/packages/task-graph/src/task/CacheCoordinator.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { getPortCodec } from "@workglow/util"; +import { getLogger, getPortCodec } from "@workglow/util"; import { type CachePolicy, isPolicyCached, isPolicyPrivate } from "../cache/CachePolicy"; import type { CacheRegistry } from "../cache/CacheRegistry"; import { RunPrivateCacheRepo } from "../cache/RunPrivateCacheRepo"; @@ -39,6 +39,13 @@ export class CacheCoordinator { if (!outputCache || !this.task.cacheable) return undefined; - const cached = await outputCache.getOutput( - this.cacheIdentityKey(policy, outputCache), - keyInputs - ); - if (cached === undefined) return undefined; - - const outputSchema = (this.task.constructor as typeof Task).outputSchema(); - const outputs = (await CacheCoordinator.deserializeOutputPorts( - cached as Record, - outputSchema as unknown as SchemaProperties - )) as Output; + // A corrupt stored value or a codec/schema drift can make getOutput or + // deserializeOutputPorts throw. Caching is a transparent optimization: a bad + // entry should cost at most a recompute, never convert a runnable task into a + // hard failure. Degrade any decode failure to a cache miss (the deterministic + // path is safe to recompute by definition). + let cached: unknown; + let outputs: Output; + try { + cached = await outputCache.getOutput(this.cacheIdentityKey(policy, outputCache), keyInputs); + if (cached === undefined) return undefined; + + const outputSchema = (this.task.constructor as typeof Task).outputSchema(); + outputs = (await CacheCoordinator.deserializeOutputPorts( + cached as Record, + outputSchema as unknown as SchemaProperties + )) as Output; + } catch (err) { + getLogger().warn( + `CacheCoordinator: failed to read/decode cached output for ${this.task.type}; treating as cache miss`, + { error: err } + ); + return undefined; + } ctx.telemetrySpan?.addEvent("workglow.task.cache_hit"); diff --git a/packages/task-graph/src/task/ConditionUtils.ts b/packages/task-graph/src/task/ConditionUtils.ts index 3479825ca..f32ebc134 100644 --- a/packages/task-graph/src/task/ConditionUtils.ts +++ b/packages/task-graph/src/task/ConditionUtils.ts @@ -4,6 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { getLogger } from "@workglow/util"; + /** * Comparison operators supported by the UI condition builder. */ @@ -90,16 +92,25 @@ export function evaluateCondition( return strValue !== compareValue; case "greater_than": - return numValue > Number(compareValue); - case "greater_or_equal": - return numValue >= Number(compareValue); - case "less_than": - return numValue < Number(compareValue); - - case "less_or_equal": - return numValue <= Number(compareValue); + case "less_or_equal": { + // Ordering operators require numeric operands. If either side is + // non-numeric (Number(...) is NaN) the comparison can never be true and + // would silently never fire; surface it as a clear diagnostic and return + // false rather than failing quietly forever. + const compareNum = Number(compareValue); + if (isNaN(numValue) || isNaN(compareNum)) { + getLogger().warn( + `Non-numeric operand in '${operator}' comparison (field=${JSON.stringify(fieldValue)}, compare=${JSON.stringify(compareValue)}); evaluating as false` + ); + return false; + } + if (operator === "greater_than") return numValue > compareNum; + if (operator === "greater_or_equal") return numValue >= compareNum; + if (operator === "less_than") return numValue < compareNum; + return numValue <= compareNum; + } case "contains": return strValue.toLowerCase().includes(compareValue.toLowerCase()); diff --git a/packages/task-graph/src/task/ConditionalTask.ts b/packages/task-graph/src/task/ConditionalTask.ts index 8a56952da..2394f8660 100644 --- a/packages/task-graph/src/task/ConditionalTask.ts +++ b/packages/task-graph/src/task/ConditionalTask.ts @@ -60,6 +60,16 @@ export type ConditionalTaskConfig = TaskConfig & { * match activates. In multi-path mode, all matching branches activate simultaneously. * Inactive branches DISABLE their outgoing dataflows, cascading to downstream tasks * with no other active inputs. + * + * TWO OUTPUT SHAPES (selected by how branches are supplied): + * - Function branches (config.branches with ConditionFn) -> {@link buildOutput}: + * `{ _activeBranches: string[], [outputPort]: { ...input } }`. This is the shape + * the instance `outputSchema()` describes. + * - Serialized `conditionConfig` (from input or config, no function branches) -> + * {@link buildConditionConfigOutput}: UI-style `key_` / `key_else` suffixed + * keys with NO `_activeBranches`. The declared outputSchema does NOT describe + * this shape, so consumers wiring dataflows off a conditionConfig-driven + * ConditionalTask must account for the suffixed keys explicitly. */ export class ConditionalTask< Input extends TaskInput = TaskInput, diff --git a/packages/task-graph/src/task/FallbackTask.ts b/packages/task-graph/src/task/FallbackTask.ts index 583930d41..6a24e2d71 100644 --- a/packages/task-graph/src/task/FallbackTask.ts +++ b/packages/task-graph/src/task/FallbackTask.ts @@ -10,6 +10,7 @@ import { CreateEndLoopWorkflow, CreateLoopWorkflow } from "../task-graph/Workflo import { FallbackTaskRunner } from "./FallbackTaskRunner"; import type { GraphAsTaskConfig } from "./GraphAsTask"; import { GraphAsTask, graphAsTaskConfigSchema } from "./GraphAsTask"; +import type { TaskGraphJsonOptions } from "./TaskJSON"; import type { TaskInput, TaskOutput, TaskTypeName } from "./TaskTypes"; /** @@ -177,8 +178,8 @@ export class FallbackTask< // Serialization // ======================================================================== - public override toJSON() { - const json = super.toJSON(); + public override toJSON(options?: TaskGraphJsonOptions) { + const json = super.toJSON(options); return { ...json, config: { diff --git a/packages/task-graph/src/task/FallbackTaskRunner.ts b/packages/task-graph/src/task/FallbackTaskRunner.ts index 2dbba7569..58de64dc6 100644 --- a/packages/task-graph/src/task/FallbackTaskRunner.ts +++ b/packages/task-graph/src/task/FallbackTaskRunner.ts @@ -57,6 +57,13 @@ export class FallbackTaskRunner< /** * Tries each task in the subgraph sequentially. Returns the first * successful result. If all fail, throws with collected errors. + * + * NOTE: alternatives are run via `alternativeTask.run(input)` directly, not + * through the subGraph runner, so the `bridgeSubGraphTaskEvents` bridge used in + * data mode does not apply here — there is no subgraph run to bridge from. + * Consequently inner alternatives' task_progress/task_complete/task_stream_* + * events do not bubble to the top-level run stream in task mode (data mode does + * surface them). Coarse attempt progress is forwarded via handleProgress below. */ private async executeTaskFallback(input: Input): Promise { const tasks = this.task.subGraph.getTasks(); diff --git a/packages/task-graph/src/task/IteratorTaskRunner.ts b/packages/task-graph/src/task/IteratorTaskRunner.ts index 1f0514c8c..4508b1958 100644 --- a/packages/task-graph/src/task/IteratorTaskRunner.ts +++ b/packages/task-graph/src/task/IteratorTaskRunner.ts @@ -16,6 +16,7 @@ import { type IteratorTask, type IteratorTaskConfig, } from "./IteratorTask"; +import { TaskAbortedError } from "./TaskError"; import type { TaskRunContext } from "./TaskRunContext"; import type { TaskInput, TaskOutput } from "./TaskTypes"; @@ -100,7 +101,9 @@ export class IteratorTaskRunner< try { for (let batchStart = 0; batchStart < iterationCount; batchStart += batchSize) { if (this.currentCtx?.abortController.signal.aborted) { - break; + // Honor cancellation as a failure: returning the partial map result + // would report a truncated array as a COMPLETED success. + throw new TaskAbortedError("Iterator aborted during iteration"); } const batchEnd = Math.min(batchStart + batchSize, iterationCount); @@ -168,7 +171,9 @@ export class IteratorTaskRunner< for (let index = 0; index < iterationCount; index++) { if (this.currentCtx?.abortController.signal.aborted) { - break; + // Honor cancellation as a failure: returning the partial accumulator + // would report an incomplete reduction as a COMPLETED success. + throw new TaskAbortedError("Iterator aborted during iteration"); } const iterationInput = this.task.buildIterationRunInput(analysis, index, iterationCount, { diff --git a/packages/task-graph/src/task/Task.ts b/packages/task-graph/src/task/Task.ts index 32f7cd8ae..701bd4e4b 100644 --- a/packages/task-graph/src/task/Task.ts +++ b/packages/task-graph/src/task/Task.ts @@ -5,7 +5,7 @@ */ import type { ServiceRegistry } from "@workglow/util"; -import { deepEqual, EventEmitter, uuid4 } from "@workglow/util"; +import { deepEqual, EventEmitter, getLogger, uuid4 } from "@workglow/util"; import type { DataPortSchema, SchemaNode } from "@workglow/util/schema"; import { compileSchema } from "@workglow/util/schema"; import { type CachePolicy, DEFAULT_CACHE_POLICY } from "../cache/CachePolicy"; @@ -519,9 +519,9 @@ export class Task< }); return (defaultData || {}) as Partial; } catch (error) { - console.warn( + getLogger().warn( `Failed to compile input schema for ${this.type}, falling back to manual extraction:`, - error + { error } ); // Fallback to manual extraction if compilation fails return Object.entries(schema.properties || {}).reduce>( @@ -699,6 +699,10 @@ export class Task< this.runInputData = { ...this.runInputData, ...overrides }; changed = true; } else { + // `undefined` is treated as "no value provided" (absent), not as an + // explicit clear: a dataflow yielding undefined does NOT reset a + // previously-set port. There is intentionally no way to unset a port via + // this merge path; the previous value persists. if (overrides[inputId] === undefined) continue; const isArray = (prop as any)?.type === "array" || @@ -839,7 +843,7 @@ export class Task< enumerable: false, }); } catch (error) { - console.warn(`Failed to compile config schema for ${this.type}:`, error); + getLogger().warn(`Failed to compile config schema for ${this.type}:`, { error }); return undefined; } } @@ -899,9 +903,9 @@ export class Task< } catch (error) { // If compilation fails, fall back to accepting any object structure // This is a safety net for schemas that json-schema-library can't compile - console.warn( + getLogger().warn( `Failed to compile input schema for ${this.type}, falling back to permissive validation:`, - error + { error } ); Object.defineProperty(this, "__compiledInputSchema", { value: compileSchema({}), diff --git a/packages/task-graph/src/task/TaskRunner.ts b/packages/task-graph/src/task/TaskRunner.ts index 9140fff67..7807f0335 100644 --- a/packages/task-graph/src/task/TaskRunner.ts +++ b/packages/task-graph/src/task/TaskRunner.ts @@ -122,6 +122,13 @@ export class TaskRunner< * Tracks task types that have already received the "private policy without * runId" downgrade warning, so the warning fires only once per task type * across the process lifetime. + * + * Process-global and never reset: in long-lived processes that dynamically + * generate task-type names this set can grow unbounded. It is also + * test-order-dependent — a prior test that triggers the warning suppresses a + * later test's expected warning. Tests that assert on this warning should + * clear it (or use a unique task type) in setup. Bounding/eviction is left as + * a future improvement; the per-type cardinality is small in practice. */ private static __privateWithoutRunIdWarned = new Set(); diff --git a/packages/task-graph/src/task/WhileTask.ts b/packages/task-graph/src/task/WhileTask.ts index 87b467c18..237b73992 100644 --- a/packages/task-graph/src/task/WhileTask.ts +++ b/packages/task-graph/src/task/WhileTask.ts @@ -13,7 +13,7 @@ import { GraphAsTask, GraphAsTaskConfig, graphAsTaskConfigSchema } from "./Graph import type { IExecuteContext, IRunConfig } from "./ITask"; import { resolveIterationBound, type IterationBound } from "./IteratorTask"; import type { StreamEvent, StreamFinish } from "./StreamTypes"; -import { TaskConfigurationError, TaskFailedError } from "./TaskError"; +import { TaskAbortedError, TaskConfigurationError, TaskFailedError } from "./TaskError"; import type { TaskInput, TaskOutput, TaskTypeName } from "./TaskTypes"; import { WhileTaskRunner } from "./WhileTaskRunner"; @@ -316,6 +316,31 @@ export class WhileTask< return iterInput as Input; } + /** + * Normalizes an error thrown by the user-supplied condition function. + * A pre-typed {@link TaskFailedError} is rethrown unchanged so its type, + * message, and stack survive; any other error is wrapped in a TaskFailedError + * with the original stack chained on. Shared by execute() and executeStream() + * so both paths preserve error type and stack identically. + */ + private wrapConditionError(err: unknown): TaskFailedError { + if (err instanceof TaskFailedError) { + return err; + } + const message = `${this.type}: Condition function threw at iteration ${this._currentIteration}: ${ + err instanceof Error ? err.message : String(err) + }`; + const wrappedError = new TaskFailedError(message); + if (err instanceof Error && err.stack) { + if (wrappedError.stack) { + wrappedError.stack += `\nCaused by original error:\n${err.stack}`; + } else { + wrappedError.stack = err.stack; + } + } + return wrappedError; + } + public override async execute( input: Input, context: IExecuteContext @@ -380,7 +405,9 @@ export class WhileTask< // Execute iterations until condition returns false or max iterations reached while (this._currentIteration < effectiveMax) { if (context.signal?.aborted) { - break; + // Honor cancellation as a failure rather than returning the partial + // currentOutput as a COMPLETED success. + throw new TaskAbortedError(`${this.type}: aborted during iteration`); } // Build the input for this iteration @@ -415,23 +442,7 @@ export class WhileTask< try { shouldContinue = condition(currentOutput, this._currentIteration); } catch (err) { - // Rethrow TaskFailedError instances unchanged so their type, message, - // and stack are preserved for the caller. - if (err instanceof TaskFailedError) { - throw err; - } - const message = `${this.type}: Condition function threw at iteration ${this._currentIteration}: ${ - err instanceof Error ? err.message : String(err) - }`; - const wrappedError = new TaskFailedError(message); - if (err instanceof Error && err.stack) { - if (wrappedError.stack) { - wrappedError.stack += `\nCaused by original error:\n${err.stack}`; - } else { - wrappedError.stack = err.stack; - } - } - throw wrappedError; + throw this.wrapConditionError(err); } if (!shouldContinue) { break; @@ -516,7 +527,9 @@ export class WhileTask< try { while (this._currentIteration < effectiveMax) { - if (context.signal?.aborted) break; + if (context.signal?.aborted) { + throw new TaskAbortedError(`${this.type}: aborted during iteration`); + } let iterationInput: Input; if (arrayAnalysis) { @@ -546,9 +559,7 @@ export class WhileTask< try { shouldContinue = condition(currentOutput, this._currentIteration); } catch (err) { - throw new TaskFailedError( - `${this.type}: Condition function threw at iteration ${this._currentIteration}: ${err instanceof Error ? err.message : String(err)}` - ); + throw this.wrapConditionError(err); } if (!shouldContinue) { // This was the final iteration -- but we already ran it non-streaming. diff --git a/packages/task-graph/src/task/iterationSchema.ts b/packages/task-graph/src/task/iterationSchema.ts index aad929ee2..a2d937f47 100644 --- a/packages/task-graph/src/task/iterationSchema.ts +++ b/packages/task-graph/src/task/iterationSchema.ts @@ -179,13 +179,6 @@ export function extractIterationProperties(schema?: DataPortSchema): DataPortSch return { type: "object", properties: iterProps }; } -/** - * Remove iteration properties from a schema (alias for filterIterationProperties). - */ -export function removeIterationProperties(schema?: DataPortSchema): DataPortSchema | undefined { - return filterIterationProperties(schema); -} - /** * Merge chained output properties into input schema; marks output properties with "x-ui-iteration": true. */ diff --git a/packages/test/src/binding/RunPrivateInMemoryTaskOutputRepository.ts b/packages/test/src/binding/RunPrivateInMemoryTaskOutputRepository.ts new file mode 100644 index 000000000..d81ae9d77 --- /dev/null +++ b/packages/test/src/binding/RunPrivateInMemoryTaskOutputRepository.ts @@ -0,0 +1,29 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import { InMemoryTabularStorage } from "@workglow/storage"; +import { + RunPrivateTaskOutputPrimaryKeyNames, + RunPrivateTaskOutputRepository, + RunPrivateTaskOutputSchema, +} from "@workglow/task-graph"; + +/** + * In-memory backing for the run-private output cache. A dedicated table whose + * runId-leading primary key makes run-scoped cleanup an indexed delete. The + * `createdAt` index serves the janitor's age sweep. + */ +export class RunPrivateInMemoryTaskOutputRepository extends RunPrivateTaskOutputRepository { + constructor() { + super({ + storage: new InMemoryTabularStorage( + RunPrivateTaskOutputSchema, + RunPrivateTaskOutputPrimaryKeyNames, + ["createdAt"] + ), + }); + } +} diff --git a/packages/test/src/test/job-queue/InMemoryJobQueue.test.ts b/packages/test/src/test/job-queue/InMemoryJobQueue.test.ts index 71b47e96d..c02ac6d34 100644 --- a/packages/test/src/test/job-queue/InMemoryJobQueue.test.ts +++ b/packages/test/src/test/job-queue/InMemoryJobQueue.test.ts @@ -4,9 +4,17 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { DeadLetter, IJobExecuteContext } from "@workglow/job-queue"; +import type { + DeadLetter, + IJobExecuteContext, + IMessageQueue, + JobStorageFormat, + MessageId, + SendOptions, +} from "@workglow/job-queue"; import { ConcurrencyLimiter, + InMemoryMessageQueue, InMemoryQueueStorage, InMemoryRateLimiterStorage, Job, @@ -163,6 +171,49 @@ describe("InMemoryQueueStorage — abort_requested_at & lease expiry", () => { await expect(storage.extendLease(id, "worker-y", 5000)).rejects.toThrow(/extendLease failed/); }); + it("InMemoryClaim.ack(undefined) overwrites a stale output with null (matches WrappedClaim)", async () => { + // Seed a row that already carries an output from a prior attempt. + const id = await storage.add({ + input: { data: "stale" }, + output: { result: "prior-attempt" } as TO, + visible_at: null, + completed_at: null, + }); + + const mq = new InMemoryMessageQueue(storage); + const claims = await mq.receive({ workerId: "w-ack", leaseMs: 30_000, max: 1 }); + expect(claims).toHaveLength(1); + + // ack with no result must NOT fall back to current.output — finalize() + // overwrites it with null, identical to WrappedClaim.ack. + await claims[0]!.ack(); + + const after = await storage.get(id); + expect(after?.status).toBe(JobStatus.COMPLETED); + expect(after?.output).toBeNull(); + }); + + it("InMemoryClaim.fail() with no error overwrites stale error fields with null (matches WrappedClaim)", async () => { + const id = await storage.add({ + input: { data: "stale-err" }, + error: "prior error", + error_code: "PriorCode", + visible_at: null, + completed_at: null, + }); + + const mq = new InMemoryMessageQueue(storage); + const claims = await mq.receive({ workerId: "w-fail", leaseMs: 30_000, max: 1 }); + expect(claims).toHaveLength(1); + + await claims[0]!.fail(); + + const after = await storage.get(id); + expect(after?.status).toBe(JobStatus.FAILED); + expect(after?.error).toBeNull(); + expect(after?.error_code).toBeNull(); + }); + it("abort PROCESSING worker observes abort_requested_at via checkForAbortingJobs", async () => { const { messageQueue, jobStore } = wrapQueueStorage(storage); const server = new JobQueueServer(SimpleTestJob, { @@ -210,7 +261,7 @@ describe("InMemoryQueueStorage — abort_requested_at & lease expiry", () => { // Minimal in-memory IMessageQueue for DLQ testing // --------------------------------------------------------------------------- -import type { IClaim, IMessageQueue, MessageId } from "@workglow/job-queue"; +import type { IClaim } from "@workglow/job-queue"; class CollectingQueue implements IMessageQueue { public readonly scope = "process" as const; @@ -338,6 +389,97 @@ describe("InMemoryJobQueue — dead-letter queue (PR 5)", () => { }); }); +// --------------------------------------------------------------------------- +// Client sendBatch tests +// --------------------------------------------------------------------------- + +describe("JobQueueClient.sendBatch", () => { + let storage: InMemoryQueueStorage; + let queueName: string; + + beforeEach(async () => { + queueName = `test-sendbatch-${uuid4()}`; + storage = new InMemoryQueueStorage(queueName); + await storage.migrate(); + }); + + afterEach(async () => { + await storage.deleteAll(); + }); + + it("inserts every body and returns a handle per input", async () => { + const { messageQueue, jobStore } = wrapQueueStorage(storage); + const client = new JobQueueClient({ messageQueue, jobStore, queueName }); + + const handles = await client.sendBatch([{ data: "a" }, { data: "b" }, { data: "c" }]); + expect(handles).toHaveLength(3); + expect(await client.size()).toBe(3); + }); + + it("delegates to messageQueue.sendBatch once and forwards delaySeconds/timeoutSeconds (was silently dropped)", async () => { + // Capturing message queue: proves the client now (a) makes a single + // batched call instead of N per-item send()s, and (b) forwards the full + // option set including delaySeconds/timeoutSeconds, which the old + // per-item loop dropped. + const { jobStore } = wrapQueueStorage(storage); + const sendCalls: unknown[] = []; + const batchCalls: { count: number; opts: SendOptions | undefined }[] = []; + + const capturing: IMessageQueue> = { + scope: "process", + async send(body, opts) { + sendCalls.push({ body, opts }); + return await storage.add(body); + }, + async sendBatch(bodies, opts) { + batchCalls.push({ count: bodies.length, opts }); + const ids: MessageId[] = []; + for (const body of bodies) ids.push(await storage.add(body)); + return ids; + }, + async receive() { + return []; + }, + async releaseClaim() {}, + async migrate() {}, + getMigrations() { + return []; + }, + }; + + const client = new JobQueueClient({ + messageQueue: capturing, + jobStore, + queueName, + }); + + await client.sendBatch([{ data: "x" }, { data: "y" }], { + delaySeconds: 60, + timeoutSeconds: 120, + maxAttempts: 3, + }); + + // Exactly one batched call, never per-item send(). + expect(batchCalls).toHaveLength(1); + expect(sendCalls).toHaveLength(0); + expect(batchCalls[0]!.count).toBe(2); + expect(batchCalls[0]!.opts?.delaySeconds).toBe(60); + expect(batchCalls[0]!.opts?.timeoutSeconds).toBe(120); + expect(batchCalls[0]!.opts?.maxAttempts).toBe(3); + // Batch-wide fingerprint must NOT be forwarded (would dedup all bodies). + expect(batchCalls[0]!.opts?.fingerprint).toBeUndefined(); + }); + + it("returns an empty array for an empty input list without touching the queue", async () => { + const { messageQueue, jobStore } = wrapQueueStorage(storage); + const client = new JobQueueClient({ messageQueue, jobStore, queueName }); + + const handles = await client.sendBatch([]); + expect(handles).toEqual([]); + expect(await client.size()).toBe(0); + }); +}); + // --------------------------------------------------------------------------- // Prefetch tests (PR 5) // --------------------------------------------------------------------------- diff --git a/packages/test/src/test/job-queue/JobQueueWorker.test.ts b/packages/test/src/test/job-queue/JobQueueWorker.test.ts index 9d7069a46..4925e5c27 100644 --- a/packages/test/src/test/job-queue/JobQueueWorker.test.ts +++ b/packages/test/src/test/job-queue/JobQueueWorker.test.ts @@ -14,6 +14,7 @@ import { JobQueueServer, JobQueueWorker, JobStatus, + PermanentJobError, RateLimiter, wrapQueueStorage, } from "@workglow/job-queue"; @@ -262,4 +263,46 @@ describe("JobQueueWorker — PR #511 follow-up regressions", () => { expect(final?.abort_requested_at).toBeTruthy(); expect(TJob.executeCalls).toBe(0); }); + + it("server job_error event delivers the errorCode the worker carries", async () => { + // The worker emits job_error(jobId, error, errorCode); the server forwards + // errorCode to clients but historically dropped it from its OWN job_error + // re-emit. A consumer subscribing to the server directly must still see the + // machine-readable code. + class FailingJob extends Job { + public override async execute(): Promise { + throw new PermanentJobError("boom"); + } + } + + const { messageQueue, jobStore } = wrapQueueStorage(storage); + const server = new JobQueueServer(FailingJob, { + messageQueue, + jobStore, + queueName, + pollIntervalMs: 5, + stopTimeoutMs: 0, + }); + const client = new JobQueueClient({ messageQueue, jobStore, queueName }); + client.attach(server); + + const seen: { error: string; errorCode?: string }[] = []; + server.on("job_error", (_queueName, _jobId, error, errorCode) => { + seen.push({ error, errorCode }); + }); + + await server.start(); + const handle = await client.send({ taskType: "fail", data: "x" }, { maxAttempts: 1 }); + + const failed = await waitUntil(async () => { + const j = await storage.get(handle.id); + return j?.status === JobStatus.FAILED; + }); + expect(failed).toBe(true); + + await server.stop(); + + expect(seen.length).toBeGreaterThanOrEqual(1); + expect(seen[0]!.errorCode).toBe("PermanentJobError"); + }); }); diff --git a/packages/test/src/test/job-queue/Limiters.test.ts b/packages/test/src/test/job-queue/Limiters.test.ts index 0aa1368e6..b874796db 100644 --- a/packages/test/src/test/job-queue/Limiters.test.ts +++ b/packages/test/src/test/job-queue/Limiters.test.ts @@ -93,6 +93,30 @@ describe("ConcurrencyLimiter", () => { await limiter.setNextAvailableTime(future); expect(await limiter.tryAcquire()).toBeNull(); }); + + it("complete should free exactly one slot for a valid token", async () => { + const t1 = await limiter.tryAcquire(); + await limiter.tryAcquire(); + expect(await limiter.tryAcquire()).toBeNull(); + await limiter.complete(t1); + expect(await limiter.tryAcquire()).not.toBeNull(); + }); + + it("complete with an invalid/foreign token must not corrupt the counter", async () => { + // Saturate to the limit (2). + await limiter.tryAcquire(); + await limiter.tryAcquire(); + expect(await limiter.tryAcquire()).toBeNull(); + + // A duplicate/foreign complete used to decrement a slot this limiter never + // handed out, permanently over-admitting concurrency. With the sentinel + // guard it is a no-op, so the limit stays enforced. + await limiter.complete(undefined); + await limiter.complete(Symbol("not-the-sentinel")); + await limiter.complete("garbage"); + + expect(await limiter.tryAcquire()).toBeNull(); + }); }); describe("DelayLimiter", () => { diff --git a/packages/test/src/test/job-queue/TelemetryQueueStorage.test.ts b/packages/test/src/test/job-queue/TelemetryQueueStorage.test.ts index e1b639a28..3b1981812 100644 --- a/packages/test/src/test/job-queue/TelemetryQueueStorage.test.ts +++ b/packages/test/src/test/job-queue/TelemetryQueueStorage.test.ts @@ -93,4 +93,39 @@ describe("TelemetryQueueStorage", () => { expect(jobs).toEqual([]); expect(startSpanSpy).toHaveBeenCalledWith("workglow.storage.queue.peek", expect.anything()); }); + + it("exposes findActiveByFingerprint and delegates when the inner implements it", async () => { + // InMemoryQueueStorage has a native findActiveByFingerprint, so the + // telemetry wrapper must expose a delegating, traced passthrough — not + // drop it (which would silently degrade dedup to the O(N) scan fallback + // once wrapped in front of a real DB backend). + expect(typeof wrapped.findActiveByFingerprint).toBe("function"); + + const id = await inner.add({ + input: { data: "fp-test" }, + fingerprint: "fp-123", + visible_at: null, + completed_at: null, + }); + expect(id).toBeDefined(); + + const found = await wrapped.findActiveByFingerprint!("fp-123", "test-queue"); + expect(found?.fingerprint).toBe("fp-123"); + expect(startSpanSpy).toHaveBeenCalledWith( + "workglow.storage.queue.findActiveByFingerprint", + expect.anything() + ); + }); + + it("leaves findActiveByFingerprint undefined when the inner lacks it", () => { + // Mirror the optional-method semantics: if the inner storage has no native + // implementation, the wrapper must stay undefined so wrapQueueStorage's + // `typeof === 'function'` probe falls through to the bounded scan fallback. + const innerWithout = { + ...inner, + findActiveByFingerprint: undefined, + } as unknown as InMemoryQueueStorage<{ data: string }, { result: string }>; + const wrappedWithout = new TelemetryQueueStorage("no-native", innerWithout); + expect(wrappedWithout.findActiveByFingerprint).toBeUndefined(); + }); }); diff --git a/packages/test/src/test/storage-kv/FsFolderKvRepository.integration.test.ts b/packages/test/src/test/storage-kv/FsFolderKvRepository.integration.test.ts index ec52728d2..e24a89154 100644 --- a/packages/test/src/test/storage-kv/FsFolderKvRepository.integration.test.ts +++ b/packages/test/src/test/storage-kv/FsFolderKvRepository.integration.test.ts @@ -4,10 +4,10 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { FsFolderKvStorage } from "@workglow/storage"; +import { FsFolderKvStorage, StorageUnsupportedError } from "@workglow/storage"; import { setLogger } from "@workglow/util"; import { mkdirSync, rmSync } from "fs"; -import { afterEach, beforeEach, describe } from "vitest"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { getTestingLogger } from "../../binding/TestingLogger"; import { runGenericKvRepositoryTests } from "./genericKvRepositoryTests"; @@ -41,4 +41,34 @@ describe("FsFolderKvStorage", () => { valueType ); }); + + describe("contract symmetry", () => { + const makeStore = () => new FsFolderKvStorage(testDir, (key) => `${String(key)}.data`); + + it("delete of a never-written key is an idempotent no-op (swallows ENOENT)", async () => { + const store = makeStore(); + // Must not reject the way unlink() would on a missing path. + await expect(store.delete("never-written")).resolves.toBeUndefined(); + }); + + it("delete still emits a delete event for a missing key", async () => { + const store = makeStore(); + let emitted: unknown; + store.on("delete", (key) => { + emitted = key; + }); + await store.delete("missing"); + expect(emitted).toBe("missing"); + }); + + it("getAll throws the typed StorageUnsupportedError", async () => { + const store = makeStore(); + await expect(store.getAll()).rejects.toBeInstanceOf(StorageUnsupportedError); + }); + + it("size throws the typed StorageUnsupportedError", async () => { + const store = makeStore(); + await expect(store.size()).rejects.toBeInstanceOf(StorageUnsupportedError); + }); + }); }); diff --git a/packages/test/src/test/storage-tabular/CachedTabularStorage.integration.test.ts b/packages/test/src/test/storage-tabular/CachedTabularStorage.integration.test.ts index 535a1d64e..bf25c801e 100644 --- a/packages/test/src/test/storage-tabular/CachedTabularStorage.integration.test.ts +++ b/packages/test/src/test/storage-tabular/CachedTabularStorage.integration.test.ts @@ -431,8 +431,11 @@ describe("CachedTabularStorage", () => { expect(cacheResults?.length).toBe(2); }); - it("should handle cache initialization errors gracefully", async () => { - // Create a mock durable that throws on getAll + it("surfaces a cache warm-up failure instead of presenting an empty result", async () => { + // A transient durable read failure during warm-up must NOT be swallowed: + // query()/queryIndex() read only the cache, so a silently-failed warm-up + // would return an empty result set as if the table were empty. The read + // must fail loudly, and the warm-up must remain retryable. const errorDurable = new InMemoryTabularStorage< typeof CompoundSchema, typeof CompoundPrimaryKeyNames @@ -443,12 +446,24 @@ describe("CachedTabularStorage", () => { typeof CompoundPrimaryKeyNames >(errorDurable, undefined, CompoundSchema, CompoundPrimaryKeyNames); - // Mock getAll to throw - spyOn(errorDurable, "getAll").mockRejectedValueOnce(new Error("Test error")); + // Seed a row so a successful warm-up would return it. + await errorDurable.put({ name: "key1", type: "string1", option: "value1", success: true }); - // Should not throw, but cache initialization should fail gracefully + // First warm-up read throws. + const getAllSpy = spyOn(errorDurable, "getAll").mockRejectedValueOnce( + new Error("Test error") + ); + + // The error is surfaced, not masked as "no rows". + await expect(cachedWithError.get({ name: "key1", type: "string1" })).rejects.toThrow( + "Test error" + ); + + // The failed warm-up is retryable: the spy only rejected once, so a + // subsequent access re-runs init and returns the seeded row. + getAllSpy.mockRestore(); const result = await cachedWithError.get({ name: "key1", type: "string1" }); - expect(result).toBeUndefined(); + expect(result?.option).toEqual("value1"); cachedWithError.destroy(); }); diff --git a/packages/test/src/test/storage-tabular/InMemoryTabularStorage.test.ts b/packages/test/src/test/storage-tabular/InMemoryTabularStorage.test.ts index 787d4bf01..321a26e39 100644 --- a/packages/test/src/test/storage-tabular/InMemoryTabularStorage.test.ts +++ b/packages/test/src/test/storage-tabular/InMemoryTabularStorage.test.ts @@ -4,10 +4,15 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { InMemoryTabularStorage, type TabularChangePayload } from "@workglow/storage"; +import { + InMemoryTabularStorage, + StorageInvalidLimitError, + StorageValidationError, + type TabularChangePayload, +} from "@workglow/storage"; import { setLogger } from "@workglow/util"; import type { FromSchema } from "@workglow/util/schema"; -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { getTestingLogger } from "../../binding/TestingLogger"; import { runTabularStorageContract, @@ -221,3 +226,105 @@ describe("InMemoryTabularStorage delete change payloads", () => { expect(identity).toEqual({ type: "t1" }); }); }); + +describe("InMemoryTabularStorage putBulk atomicity", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("rolls back all rows and emits a rollback event when a mid-batch put throws", async () => { + const storage = new InMemoryTabularStorage< + typeof CompoundSchema, + typeof CompoundPrimaryKeyNames + >(CompoundSchema, CompoundPrimaryKeyNames); + + await storage.put({ name: "keep", type: "t0", option: "v0", success: true }); + + const originalPut = InMemoryTabularStorage.prototype.put; + let call = 0; + vi.spyOn(InMemoryTabularStorage.prototype as any, "put").mockImplementation(async function ( + this: InMemoryTabularStorage, + v: any + ) { + call += 1; + if (call === 2) throw new Error("mid-batch boom"); + return await (originalPut as any).call(this, v); + } as any); + + const rollbacks: Array<{ op: string; error: unknown; ids: readonly any[] }> = []; + storage.on("rollback" as any, (reason: any) => rollbacks.push(reason)); + + await expect( + storage.putBulk([ + { name: "a", type: "t1", option: "v1", success: true }, + { name: "b", type: "t2", option: "v2", success: true }, + { name: "c", type: "t3", option: "v3", success: true }, + ]) + ).rejects.toThrow("mid-batch boom"); + + vi.restoreAllMocks(); + + // Only the pre-existing row survives; no row from the failed batch remains. + expect(await storage.size()).toBe(1); + expect(await storage.get({ name: "keep", type: "t0" })).toBeDefined(); + expect(await storage.get({ name: "a", type: "t1" })).toBeUndefined(); + expect(await storage.get({ name: "b", type: "t2" })).toBeUndefined(); + + // Rollback event names exactly the row that committed before the throw. + expect(rollbacks).toHaveLength(1); + expect(rollbacks[0].op).toBe("putBulk"); + expect(rollbacks[0].ids).toHaveLength(1); + expect((rollbacks[0].ids[0] as { name: string }).name).toBe("a"); + }); + + it("commits every row when nothing throws", async () => { + const storage = new InMemoryTabularStorage< + typeof CompoundSchema, + typeof CompoundPrimaryKeyNames + >(CompoundSchema, CompoundPrimaryKeyNames); + const results = await storage.putBulk([ + { name: "a", type: "t1", option: "v1", success: true }, + { name: "b", type: "t2", option: "v2", success: true }, + ]); + expect(results).toHaveLength(2); + expect(await storage.size()).toBe(2); + }); + + it("a throwing put listener does not turn a committed write into a rejection", async () => { + const storage = new InMemoryTabularStorage< + typeof CompoundSchema, + typeof CompoundPrimaryKeyNames + >(CompoundSchema, CompoundPrimaryKeyNames); + + // Suppress the warning the safeEmit path logs for the thrown listener. + vi.spyOn(console, "warn").mockImplementation(() => {}); + storage.on("put", () => { + throw new Error("listener boom"); + }); + + // The write is post-commit, so the row persists and put() resolves. + await expect( + storage.put({ name: "a", type: "t1", option: "v1", success: true }) + ).resolves.toBeDefined(); + expect(await storage.get({ name: "a", type: "t1" })).toBeDefined(); + }); +}); + +describe("InMemoryTabularStorage getOffsetPage validation", () => { + it("rejects a non-positive limit", async () => { + const storage = new InMemoryTabularStorage< + typeof CompoundSchema, + typeof CompoundPrimaryKeyNames + >(CompoundSchema, CompoundPrimaryKeyNames); + await expect(storage.getOffsetPage(0, 0)).rejects.toBeInstanceOf(StorageInvalidLimitError); + await expect(storage.getOffsetPage(0, -1)).rejects.toBeInstanceOf(StorageInvalidLimitError); + }); + + it("rejects a negative offset", async () => { + const storage = new InMemoryTabularStorage< + typeof CompoundSchema, + typeof CompoundPrimaryKeyNames + >(CompoundSchema, CompoundPrimaryKeyNames); + await expect(storage.getOffsetPage(-1, 10)).rejects.toBeInstanceOf(StorageValidationError); + }); +}); diff --git a/packages/test/src/test/storage-util/sqlTypeMapping.test.ts b/packages/test/src/test/storage-util/sqlTypeMapping.test.ts new file mode 100644 index 000000000..9871b6e55 --- /dev/null +++ b/packages/test/src/test/storage-util/sqlTypeMapping.test.ts @@ -0,0 +1,77 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import { mapPostgresType, TYPED_ARRAY_CTORS } from "@workglow/storage"; +import type { JsonSchema } from "@workglow/util/schema"; +import { describe, expect, it } from "vitest"; + +// The real backends pass their null-stripping helper; for these schemas there +// is no nullable wrapper so an identity function is correct. +const options = { getNonNullType: (t: JsonSchema) => t }; + +describe("mapPostgresType integer range selection", () => { + it("uses BIGINT for an unsigned maximum above INTEGER's range", () => { + expect(mapPostgresType({ type: "integer", minimum: 0, maximum: 9999999999 }, options)).toBe( + "BIGINT" + ); + }); + + it("uses BIGINT for a signed schema whose maximum exceeds INTEGER's range", () => { + // Regression: a negative/absent minimum used to skip the maximum-based + // range selection and fall through to INTEGER, overflowing at runtime. + expect(mapPostgresType({ type: "integer", minimum: -1, maximum: 9999999999 }, options)).toBe( + "BIGINT" + ); + }); + + it("uses BIGINT when only a large maximum is present (no minimum)", () => { + expect(mapPostgresType({ type: "integer", maximum: 9999999999 }, options)).toBe("BIGINT"); + }); + + it("uses BIGINT when the minimum is below INTEGER's lower bound", () => { + expect(mapPostgresType({ type: "integer", minimum: -3000000000 }, options)).toBe("BIGINT"); + }); + + it("keeps INTEGER for in-range signed schemas", () => { + expect(mapPostgresType({ type: "integer", minimum: -100, maximum: 100 }, options)).toBe( + "INTEGER" + ); + }); + + it("keeps SMALLINT / INTEGER for in-range unsigned schemas", () => { + expect(mapPostgresType({ type: "integer", minimum: 0, maximum: 100 }, options)).toBe( + "SMALLINT" + ); + expect(mapPostgresType({ type: "integer", minimum: 0, maximum: 100000 }, options)).toBe( + "INTEGER" + ); + }); +}); + +describe("TYPED_ARRAY_CTORS", () => { + it("includes Float16Array so documented quantized vectors decode", () => { + // The util schema layer polyfills globalThis.Float16Array on older runtimes, + // so this entry should be present in this test process. + expect(TYPED_ARRAY_CTORS.Float16Array).toBeDefined(); + const arr = new TYPED_ARRAY_CTORS.Float16Array([1, 2, 3]) as ArrayBufferView & { + length: number; + }; + expect(arr.length).toBe(3); + }); + + it("includes the documented float/int constructors", () => { + for (const name of [ + "Float32Array", + "Float64Array", + "Int8Array", + "Uint8Array", + "Int16Array", + "Uint16Array", + ]) { + expect(TYPED_ARRAY_CTORS[name]).toBeDefined(); + } + }); +}); diff --git a/packages/test/src/test/task-graph-cache/AbstractRepoCapabilities.test.ts b/packages/test/src/test/task-graph-cache/AbstractRepoCapabilities.test.ts index b2443f533..dbb7ad371 100644 --- a/packages/test/src/test/task-graph-cache/AbstractRepoCapabilities.test.ts +++ b/packages/test/src/test/task-graph-cache/AbstractRepoCapabilities.test.ts @@ -9,7 +9,8 @@ import { describe, expect, it, vi } from "vitest"; /** * Minimal concrete subclass that implements only the truly-abstract methods. - * Leaves the three default-throwing prefix-aware methods as their base defaults. + * Leaves the run-scoped methods as their base defaults, which throw — only a + * run-private backing (RunPrivateTaskOutputRepository) implements them. */ class MinimalRepo extends TaskOutputRepository { constructor() { @@ -26,21 +27,31 @@ class MinimalRepo extends TaskOutputRepository { } } -describe("TaskOutputRepository default-throwing prefix-aware methods", () => { - it("deleteByTaskTypePrefix throws by default", async () => { +describe("TaskOutputRepository default-throwing run-scoped methods", () => { + it("saveOutputForRun throws by default", async () => { const r = new MinimalRepo(); - await expect(r.deleteByTaskTypePrefix("some-prefix::")).rejects.toThrow(/not supported/); + await expect(r.saveOutputForRun("run", "T", { x: 1 }, { r: 1 })).rejects.toThrow( + /not supported/ + ); }); - it("clearOlderThanWithTaskTypePrefix throws by default", async () => { + it("getOutputForRun throws by default", async () => { const r = new MinimalRepo(); - await expect(r.clearOlderThanWithTaskTypePrefix("some-prefix::", 86400_000)).rejects.toThrow( - /not supported/ - ); + await expect(r.getOutputForRun("run", "T", { x: 1 })).rejects.toThrow(/not supported/); + }); + + it("deleteRun throws by default", async () => { + const r = new MinimalRepo(); + await expect(r.deleteRun("run")).rejects.toThrow(/not supported/); + }); + + it("deleteRunOlderThan throws by default", async () => { + const r = new MinimalRepo(); + await expect(r.deleteRunOlderThan("run", 86400_000)).rejects.toThrow(/not supported/); }); - it("sizeByTaskTypePrefix throws by default", async () => { + it("sizeForRun throws by default", async () => { const r = new MinimalRepo(); - await expect(r.sizeByTaskTypePrefix("some-prefix::")).rejects.toThrow(/not supported/); + await expect(r.sizeForRun("run")).rejects.toThrow(/not supported/); }); }); diff --git a/packages/test/src/test/task-graph-cache/CacheJanitor.test.ts b/packages/test/src/test/task-graph-cache/CacheJanitor.test.ts index d041506b2..aca8399b4 100644 --- a/packages/test/src/test/task-graph-cache/CacheJanitor.test.ts +++ b/packages/test/src/test/task-graph-cache/CacheJanitor.test.ts @@ -6,49 +6,50 @@ import { CacheJanitor } from "@workglow/task-graph"; import { describe, expect, it } from "vitest"; -import { InMemoryTaskOutputRepository } from "../../binding/InMemoryTaskOutputRepository"; +import { RunPrivateInMemoryTaskOutputRepository } from "../../binding/RunPrivateInMemoryTaskOutputRepository"; describe("CacheJanitor", () => { it("sweepStaleRunPrivate prunes only entries older than the cutoff", async () => { - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const now = Date.now(); - await backing.saveOutput("__run:rA::T", { x: 1 }, { ok: 1 }, new Date(now - 8 * 24 * 3600_000)); - await backing.saveOutput("__run:rB::T", { x: 1 }, { ok: 2 }, new Date(now - 1 * 24 * 3600_000)); + await backing.saveOutputForRun( + "rA", + "T", + { x: 1 }, + { ok: 1 }, + new Date(now - 8 * 24 * 3600_000) + ); + await backing.saveOutputForRun( + "rB", + "T", + { x: 1 }, + { ok: 2 }, + new Date(now - 1 * 24 * 3600_000) + ); expect(await backing.size()).toBe(2); const janitor = new CacheJanitor({ privateBacking: backing }); await janitor.sweepStaleRunPrivate(7 * 24 * 3600_000); expect(await backing.size()).toBe(1); - expect(await backing.getOutput("__run:rB::T", { x: 1 })).toEqual({ ok: 2 }); + expect(await backing.getOutputForRun("rB", "T", { x: 1 })).toEqual({ ok: 2 }); }); - it("does not touch entries lacking the __run: prefix", async () => { - const backing = new InMemoryTaskOutputRepository(); + it("sweeps stale rows across every run (the private table is dedicated)", async () => { + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const now = Date.now(); - // Shared (deterministic) entry with no run prefix — old but should not be swept. - await backing.saveOutput( - "SharedTaskType", - { x: 1 }, - { ok: "shared" }, - new Date(now - 30 * 24 * 3600_000) - ); - // Run-private entry — old, should be swept. - await backing.saveOutput( - "__run:rX::T", - { x: 1 }, - { ok: "private" }, - new Date(now - 30 * 24 * 3600_000) - ); + const old = new Date(now - 30 * 24 * 3600_000); + await backing.saveOutputForRun("rX", "T", { x: 1 }, { ok: "x" }, old); + await backing.saveOutputForRun("rY", "T", { x: 1 }, { ok: "y" }, old); const janitor = new CacheJanitor({ privateBacking: backing }); await janitor.sweepStaleRunPrivate(7 * 24 * 3600_000); - expect(await backing.getOutput("SharedTaskType", { x: 1 })).toEqual({ ok: "shared" }); - expect(await backing.getOutput("__run:rX::T", { x: 1 })).toBeUndefined(); + // Every row is run-private; all are older than the cutoff, so all are reaped. + expect(await backing.size()).toBe(0); }); }); diff --git a/packages/test/src/test/task-graph-cache/PrivateRequiresRunId.test.ts b/packages/test/src/test/task-graph-cache/PrivateRequiresRunId.test.ts index 0aaf3b4fb..07f81427b 100644 --- a/packages/test/src/test/task-graph-cache/PrivateRequiresRunId.test.ts +++ b/packages/test/src/test/task-graph-cache/PrivateRequiresRunId.test.ts @@ -16,7 +16,7 @@ import { import { Container, ResourceScope, ServiceRegistry, getLogger, setLogger } from "@workglow/util"; import type { DataPortSchema } from "@workglow/util/schema"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { InMemoryTaskOutputRepository } from "../../binding/InMemoryTaskOutputRepository"; +import { RunPrivateInMemoryTaskOutputRepository } from "../../binding/RunPrivateInMemoryTaskOutputRepository"; class PrivatePolicyTask extends Task<{ q: string }, { r: string }> { public static override type = "PrivateRunIdTask"; @@ -79,13 +79,13 @@ class DeterministicTask extends Task<{ q: string }, { r: string }> { } } -async function freshPrivateRepo(): Promise { - const r = new InMemoryTaskOutputRepository(); +async function freshPrivateRepo(): Promise { + const r = new RunPrivateInMemoryTaskOutputRepository(); await (r as any).setupDatabase?.(); return r; } -function freshServices(privateRepo: InMemoryTaskOutputRepository): ServiceRegistry { +function freshServices(privateRepo: RunPrivateInMemoryTaskOutputRepository): ServiceRegistry { const services = new ServiceRegistry(new Container()); services.registerInstance(CACHE_REGISTRY, new DefaultCacheRegistry({ private: privateRepo })); return services; @@ -124,8 +124,7 @@ describe("private cache policy requires a runId", () => { // The private repo must be untouched — without a runId the task should // skip caching entirely rather than colliding in the shared namespace. - const stored = await backing.getOutput(PrivatePolicyTask.type, { q: "hello", __cv: "1" }); - expect(stored).toBeUndefined(); + expect(await backing.size()).toBe(0); const matches = warnings.filter( (w) => w.includes("private cache policy") && w.includes(PrivatePolicyTask.type) @@ -180,9 +179,12 @@ describe("private cache policy requires a runId", () => { ); expect(matchingWarnings.length).toBe(0); - // Private cache keys by task instance id, namespaced under __run::: - const prefixedType = `__run:${runId}::${task.id}`; - const stored = await rawBacking.getOutput(prefixedType, { q: "bypass-test", __cv: "1" }); + // Private cache keys by task instance id under the wrapper's runId (a + // first-class column, not a taskType prefix). + const stored = await rawBacking.getOutputForRun(runId, String(task.id), { + q: "bypass-test", + __cv: "1", + }); expect(stored).toEqual({ r: "done:bypass-test" }); }); }); diff --git a/packages/test/src/test/task-graph-cache/RunPrivateCacheKeyFallback.test.ts b/packages/test/src/test/task-graph-cache/RunPrivateCacheKeyFallback.test.ts index 30f181091..ef91b7f57 100644 --- a/packages/test/src/test/task-graph-cache/RunPrivateCacheKeyFallback.test.ts +++ b/packages/test/src/test/task-graph-cache/RunPrivateCacheKeyFallback.test.ts @@ -15,7 +15,7 @@ import { import { Container, ServiceRegistry } from "@workglow/util"; import type { DataPortSchema } from "@workglow/util/schema"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { InMemoryTaskOutputRepository } from "../../binding/InMemoryTaskOutputRepository"; +import { RunPrivateInMemoryTaskOutputRepository } from "../../binding/RunPrivateInMemoryTaskOutputRepository"; class CountingPrivateTask extends Task<{ q: string }, { r: string }> { public static override type = "CountingPrivateTask"; @@ -50,14 +50,14 @@ class CountingPrivateTask extends Task<{ q: string }, { r: string }> { } } -async function freshRepo(): Promise { - const r = new InMemoryTaskOutputRepository(); +async function freshRepo(): Promise { + const r = new RunPrivateInMemoryTaskOutputRepository(); await (r as any).setupDatabase?.(); return r; } function freshServices( - privateRepo: InMemoryTaskOutputRepository | RunPrivateCacheRepo + privateRepo: RunPrivateInMemoryTaskOutputRepository | RunPrivateCacheRepo ): ServiceRegistry { const services = new ServiceRegistry(new Container()); services.registerInstance( diff --git a/packages/test/src/test/task-graph-cache/RunPrivateCacheRepo.test.ts b/packages/test/src/test/task-graph-cache/RunPrivateCacheRepo.test.ts index 87ae308b6..bd8531bc4 100644 --- a/packages/test/src/test/task-graph-cache/RunPrivateCacheRepo.test.ts +++ b/packages/test/src/test/task-graph-cache/RunPrivateCacheRepo.test.ts @@ -6,13 +6,13 @@ import { RunPrivateCacheRepo } from "@workglow/task-graph"; import { beforeEach, describe, expect, it } from "vitest"; -import { InMemoryTaskOutputRepository } from "../../binding/InMemoryTaskOutputRepository"; +import { RunPrivateInMemoryTaskOutputRepository } from "../../binding/RunPrivateInMemoryTaskOutputRepository"; describe("RunPrivateCacheRepo", () => { - let backing: InMemoryTaskOutputRepository; + let backing: RunPrivateInMemoryTaskOutputRepository; beforeEach(async () => { - backing = new InMemoryTaskOutputRepository(); + backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); }); @@ -78,10 +78,6 @@ describe("RunPrivateCacheRepo", () => { // repoB: one old await repoB.saveOutput("T", { x: "old" }, { v: "B-old" }, oldDate); - // Also save a deterministic (non-prefixed) entry directly in the backing store - // with an old timestamp — clearOlderThan on repoA must NOT touch it. - await backing.saveOutput("DeterministicTask", { x: "det" }, { v: "det-old" }, oldDate); - // Prune entries in repoA older than 7 days. await repoA.clearOlderThan(7 * 24 * 3600_000); @@ -89,9 +85,7 @@ describe("RunPrivateCacheRepo", () => { expect(await repoA.getOutput("T", { x: "old" })).toBeUndefined(); // repoA's fresh entry should survive. expect(await repoA.getOutput("T", { x: "new" })).toEqual({ v: "A-new" }); - // repoB's old entry should survive (different runId namespace). + // repoB's old entry should survive (different runId, not touched by repoA's prune). expect(await repoB.getOutput("T", { x: "old" })).toEqual({ v: "B-old" }); - // The deterministic (non-prefixed) entry should survive. - expect(await backing.getOutput("DeterministicTask", { x: "det" })).toEqual({ v: "det-old" }); }); }); diff --git a/packages/test/src/test/task-graph-cache/RunPrivateOutputCodec.test.ts b/packages/test/src/test/task-graph-cache/RunPrivateOutputCodec.test.ts new file mode 100644 index 000000000..44126b587 --- /dev/null +++ b/packages/test/src/test/task-graph-cache/RunPrivateOutputCodec.test.ts @@ -0,0 +1,61 @@ +/** + * @license + * Copyright 2026 Steven Roussey + * SPDX-License-Identifier: Apache-2.0 + */ + +import { InMemoryTabularStorage } from "@workglow/storage"; +import { + RunPrivateTaskOutputPrimaryKeyNames, + RunPrivateTaskOutputRepository, + RunPrivateTaskOutputSchema, +} from "@workglow/task-graph"; +import { describe, expect, it } from "vitest"; + +function freshRepo(outputCompression: boolean): RunPrivateTaskOutputRepository { + return new RunPrivateTaskOutputRepository({ + storage: new InMemoryTabularStorage( + RunPrivateTaskOutputSchema, + RunPrivateTaskOutputPrimaryKeyNames, + ["createdAt"] + ), + outputCompression, + }); +} + +describe("RunPrivateTaskOutputRepository output codec", () => { + for (const outputCompression of [true, false]) { + it(`round-trips output (outputCompression=${outputCompression})`, async () => { + const repo = freshRepo(outputCompression); + await repo.saveOutputForRun("rT", "T", { x: 1 }, { nested: { ok: true }, n: 42 }); + expect(await repo.getOutputForRun("rT", "T", { x: 1 })).toEqual({ + nested: { ok: true }, + n: 42, + }); + }); + } + + it("decodes an uncompressed value handed back as a plain Uint8Array (durable backend shape)", async () => { + // SQL/IndexedDB backends return blob columns as a Uint8Array, not a Buffer; + // calling `.toString()` on that would yield comma-separated byte numbers and + // crash JSON.parse. Simulate that round-trip shape directly. + const storage = new InMemoryTabularStorage( + RunPrivateTaskOutputSchema, + RunPrivateTaskOutputPrimaryKeyNames, + ["createdAt"] + ); + const repo = new RunPrivateTaskOutputRepository({ storage, outputCompression: false }); + + const key = await repo.keyFromInputs({ x: 1 }); + const bytes = new Uint8Array(Buffer.from(JSON.stringify({ ok: "z" }))); + await storage.put({ + runId: "rZ", + key, + taskType: "T", + value: bytes as unknown as string, + createdAt: new Date().toISOString(), + }); + + expect(await repo.getOutputForRun("rZ", "T", { x: 1 })).toEqual({ ok: "z" }); + }); +}); diff --git a/packages/test/src/test/task-graph-cache/TaskGraphRunnerCleanup.test.ts b/packages/test/src/test/task-graph-cache/TaskGraphRunnerCleanup.test.ts index 66754394a..1613d5fd1 100644 --- a/packages/test/src/test/task-graph-cache/TaskGraphRunnerCleanup.test.ts +++ b/packages/test/src/test/task-graph-cache/TaskGraphRunnerCleanup.test.ts @@ -15,7 +15,7 @@ import { import { Container, ResourceScope, ServiceRegistry } from "@workglow/util"; import type { DataPortSchema } from "@workglow/util/schema"; import { describe, expect, it } from "vitest"; -import { InMemoryTaskOutputRepository } from "../../binding/InMemoryTaskOutputRepository"; +import { RunPrivateInMemoryTaskOutputRepository } from "../../binding/RunPrivateInMemoryTaskOutputRepository"; class PrivTask extends Task<{ q: string }, { r: string }> { public static override type = "PrivTask_Cleanup"; @@ -79,7 +79,7 @@ class FailingTask extends Task<{ q: string }, { r: string }> { } } -function freshServices(privateRepo: InMemoryTaskOutputRepository): ServiceRegistry { +function freshServices(privateRepo: RunPrivateInMemoryTaskOutputRepository): ServiceRegistry { const services = new ServiceRegistry(new Container()); services.registerInstance(CACHE_REGISTRY, new DefaultCacheRegistry({ private: privateRepo })); return services; @@ -87,7 +87,7 @@ function freshServices(privateRepo: InMemoryTaskOutputRepository): ServiceRegist describe("TaskGraphRunner cleanup on success", () => { it("private cache entries for the runId are cleared after a successful run", async () => { - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const services = freshServices(backing); @@ -107,7 +107,7 @@ describe("TaskGraphRunner cleanup on success", () => { }); it("entries survive a failed run (left for restart/TTL)", async () => { - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const services = freshServices(backing); diff --git a/packages/test/src/test/task-graph-cache/TaskGraphRunnerDurabilityWarning.test.ts b/packages/test/src/test/task-graph-cache/TaskGraphRunnerDurabilityWarning.test.ts index 6ff1c6669..c2af90d85 100644 --- a/packages/test/src/test/task-graph-cache/TaskGraphRunnerDurabilityWarning.test.ts +++ b/packages/test/src/test/task-graph-cache/TaskGraphRunnerDurabilityWarning.test.ts @@ -9,7 +9,7 @@ import { CACHE_REGISTRY, DefaultCacheRegistry, Task, TaskGraph } from "@workglow import { Container, ResourceScope, ServiceRegistry, getLogger, setLogger } from "@workglow/util"; import type { DataPortSchema } from "@workglow/util/schema"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { InMemoryTaskOutputRepository } from "../../binding/InMemoryTaskOutputRepository"; +import { RunPrivateInMemoryTaskOutputRepository } from "../../binding/RunPrivateInMemoryTaskOutputRepository"; class PrivTask extends Task<{ q: string }, { r: string }> { public static override type = "DurabilityWarnTask"; @@ -62,7 +62,7 @@ describe("TaskGraphRunner durability warning", () => { }); it("warns when a private-policy task is wired to a non-durable repo", async () => { - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const services = new ServiceRegistry(new Container()); @@ -79,7 +79,7 @@ describe("TaskGraphRunner durability warning", () => { }); it("does NOT warn when private repo is durable", async () => { - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); (backing as any).isDurable = () => true; await (backing as any).setupDatabase?.(); @@ -125,7 +125,7 @@ describe("TaskGraphRunner durability warning", () => { } } - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const services = new ServiceRegistry(new Container()); @@ -140,7 +140,7 @@ describe("TaskGraphRunner durability warning", () => { }); it("emits durability warning only once per private repo across multiple runGraph calls", async () => { - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const services = new ServiceRegistry(new Container()); @@ -163,7 +163,7 @@ describe("TaskGraphRunner durability warning", () => { it("emits a fresh warning for a different repo instance", async () => { // First repo — should warn once. - const backingA = new InMemoryTaskOutputRepository(); + const backingA = new RunPrivateInMemoryTaskOutputRepository(); await (backingA as any).setupDatabase?.(); const servicesA = new ServiceRegistry(new Container()); servicesA.registerInstance(CACHE_REGISTRY, new DefaultCacheRegistry({ private: backingA })); @@ -172,7 +172,7 @@ describe("TaskGraphRunner durability warning", () => { await gA.run({}, { runId: "ra", registry: servicesA, resourceScope: new ResourceScope() }); // Second, separate repo instance — must warn again (WeakSet keys on repo). - const backingB = new InMemoryTaskOutputRepository(); + const backingB = new RunPrivateInMemoryTaskOutputRepository(); await (backingB as any).setupDatabase?.(); const servicesB = new ServiceRegistry(new Container()); servicesB.registerInstance(CACHE_REGISTRY, new DefaultCacheRegistry({ private: backingB })); @@ -185,7 +185,7 @@ describe("TaskGraphRunner durability warning", () => { }); it("restores registry between runs to avoid nested RunPrivateCacheRepo wrappers", async () => { - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const services = new ServiceRegistry(new Container()); services.registerInstance(CACHE_REGISTRY, new DefaultCacheRegistry({ private: backing })); @@ -238,7 +238,7 @@ describe("TaskGraphRunner durability warning", () => { } } - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const services = new ServiceRegistry(new Container()); services.registerInstance(CACHE_REGISTRY, new DefaultCacheRegistry({ private: backing })); @@ -289,7 +289,7 @@ describe("TaskGraphRunner durability warning", () => { } } - const backing = new InMemoryTaskOutputRepository(); + const backing = new RunPrivateInMemoryTaskOutputRepository(); await (backing as any).setupDatabase?.(); const services = new ServiceRegistry(new Container()); services.registerInstance(CACHE_REGISTRY, new DefaultCacheRegistry({ private: backing })); diff --git a/packages/test/src/test/task-graph-cache/TaskGraphRunnerPrivate.test.ts b/packages/test/src/test/task-graph-cache/TaskGraphRunnerPrivate.test.ts index 0160e147a..620e6e497 100644 --- a/packages/test/src/test/task-graph-cache/TaskGraphRunnerPrivate.test.ts +++ b/packages/test/src/test/task-graph-cache/TaskGraphRunnerPrivate.test.ts @@ -15,7 +15,7 @@ import { import { Container, ServiceRegistry } from "@workglow/util"; import type { DataPortSchema } from "@workglow/util/schema"; import { describe, expect, it } from "vitest"; -import { InMemoryTaskOutputRepository } from "../../binding/InMemoryTaskOutputRepository"; +import { RunPrivateInMemoryTaskOutputRepository } from "../../binding/RunPrivateInMemoryTaskOutputRepository"; class FlakyTask extends Task<{ q: string }, { r: string }> { public static override type = "FlakyTask"; @@ -52,14 +52,14 @@ class FlakyTask extends Task<{ q: string }, { r: string }> { // Attach the static `runs` array properly (FlakyTask as any).runs = []; -function freshServices(privateRepo: InMemoryTaskOutputRepository): ServiceRegistry { +function freshServices(privateRepo: RunPrivateInMemoryTaskOutputRepository): ServiceRegistry { const services = new ServiceRegistry(new Container()); services.registerInstance(CACHE_REGISTRY, new DefaultCacheRegistry({ private: privateRepo })); return services; } -async function freshRepo(): Promise { - const r = new InMemoryTaskOutputRepository(); +async function freshRepo(): Promise { + const r = new RunPrivateInMemoryTaskOutputRepository(); await (r as any).setupDatabase?.(); return r; } diff --git a/packages/test/src/test/task-graph/ConditionalBuilder.test.ts b/packages/test/src/test/task-graph/ConditionalBuilder.test.ts index 8b5c10ed6..7673fcd5c 100644 --- a/packages/test/src/test/task-graph/ConditionalBuilder.test.ts +++ b/packages/test/src/test/task-graph/ConditionalBuilder.test.ts @@ -4,13 +4,56 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { ConditionalTask, Workflow, WorkflowError } from "@workglow/task-graph"; +import { ConditionalTask, Task, Workflow, WorkflowError } from "@workglow/task-graph"; import { setLogger } from "@workglow/util"; +import type { DataPortSchema } from "@workglow/util/schema"; import { describe, expect, it } from "vitest"; import { getTestingLogger } from "../../binding/TestingLogger"; import { DoubleToDoubledTask, HalveTask } from "../task/TestTasks"; +/** Echoes its `value` input back out, used to drive a conditional's predicate from upstream. */ +class ValueSourceTask extends Task<{ value: number }, { value: number }> { + static override type = "ValueSourceTask"; + static override category = "Test"; + static override inputSchema(): DataPortSchema { + return { + type: "object", + properties: { value: { type: "number" } }, + } as const satisfies DataPortSchema; + } + static override outputSchema(): DataPortSchema { + return { + type: "object", + properties: { value: { type: "number" } }, + } as const satisfies DataPortSchema; + } + override async execute(input: { value: number }): Promise<{ value: number }> { + return { value: input.value }; + } +} + +/** Consumes a `doubled` input, so it auto-connects from {@link DoubleToDoubledTask}'s output. */ +class DoubledConsumerTask extends Task<{ doubled: number }, { result: number }> { + static override type = "DoubledConsumerTask"; + static override category = "Test"; + static override inputSchema(): DataPortSchema { + return { + type: "object", + properties: { doubled: { type: "number" } }, + } as const satisfies DataPortSchema; + } + static override outputSchema(): DataPortSchema { + return { + type: "object", + properties: { result: { type: "number" } }, + } as const satisfies DataPortSchema; + } + override async execute(input: { doubled: number }): Promise<{ result: number }> { + return { result: input.doubled }; + } +} + describe("ConditionalBuilder (Workflow.if)", () => { let logger = getTestingLogger(); setLogger(logger); @@ -151,4 +194,63 @@ describe("ConditionalBuilder (Workflow.if)", () => { expect(conditional.isBranchActive("then")).toBe(false); expect(conditional.getActiveBranches().size).toBe(0); }); + + it("wires the prior task's output INTO the conditional input (mid-chain .if)", async () => { + const workflow = new Workflow(); + workflow + .addTask(ValueSourceTask, { value: 10 }) + .if((input: any) => input.value > 5) + .then(DoubleToDoubledTask) + .else(HalveTask) + .endIf(); + + const tasks = workflow.graph.getTasks(); + const source = tasks.find((t) => t instanceof ValueSourceTask)!; + const conditional = tasks.find((t) => t instanceof ConditionalTask) as ConditionalTask; + + // An A -> conditional dataflow must exist so the predicate sees A's output. + const dataflows = workflow.graph.getDataflows(); + const inEdge = dataflows.find( + (df) => df.sourceTaskId === source.id && df.targetTaskId === conditional.id + ); + expect(inEdge).toBeDefined(); + + // The predicate must observe the upstream value (10 > 5 -> then active). + let observed: unknown; + const branches = conditional.config.branches!; + const original = branches[0].condition; + (branches[0] as any).condition = (input: any) => { + observed = input?.value; + return original(input); + }; + + await workflow.run({ value: 10 }); + + expect(observed).toBe(10); + expect(conditional.isBranchActive("then")).toBe(true); + expect(conditional.isBranchActive("else")).toBe(false); + }); + + it("throws on .addTask after a two-arm .endIf() (ambiguous join)", () => { + const workflow = new Workflow(); + const continued = workflow + .if((input: any) => input.value > 5) + .then(DoubleToDoubledTask) + .else(HalveTask) + .endIf(); + + expect(() => continued.addTask(ValueSourceTask)).toThrow(WorkflowError); + }); + + it("allows .addTask after a then-only .endIf() (single leaf)", () => { + const workflow = new Workflow(); + const continued = workflow + .if((input: any) => input.value > 5) + .then(DoubleToDoubledTask) + .endIf(); + + // The single then-arm is an unambiguous predecessor, and DoubledConsumerTask + // auto-connects from its `doubled` output — so continuation must not throw. + expect(() => continued.addTask(DoubledConsumerTask)).not.toThrow(); + }); }); diff --git a/packages/test/src/test/task-graph/ResourceScope.test.ts b/packages/test/src/test/task-graph/ResourceScope.test.ts index 6ebfb8669..cfaf8b80f 100644 --- a/packages/test/src/test/task-graph/ResourceScope.test.ts +++ b/packages/test/src/test/task-graph/ResourceScope.test.ts @@ -97,6 +97,31 @@ describe("ResourceScope threading", () => { }); }); +describe("ResourceScope.disposeAll error handling", () => { + it("runs every disposer and does not throw when one rejects", async () => { + const scope = new ResourceScope(); + const ran: string[] = []; + + scope.register("ok-1", async () => { + ran.push("ok-1"); + }); + scope.register("boom", async () => { + ran.push("boom"); + throw new Error("disposer failed"); + }); + scope.register("ok-2", async () => { + ran.push("ok-2"); + }); + + // Best-effort contract: must not throw even though "boom" rejects. + await expect(scope.disposeAll()).resolves.toBeUndefined(); + + // All disposers ran despite the failure, and the scope is cleared. + expect(ran.sort()).toEqual(["boom", "ok-1", "ok-2"]); + expect(scope.size).toBe(0); + }); +}); + describe("ResourceScope browser pattern", () => { it("BrowserSessionTask-style task registers a disposer keyed by session ID", async () => { const scope = new ResourceScope(); diff --git a/packages/test/src/test/task-graph/TaskGraph.test.ts b/packages/test/src/test/task-graph/TaskGraph.test.ts index 0eae5b59f..658b1d4fd 100644 --- a/packages/test/src/test/task-graph/TaskGraph.test.ts +++ b/packages/test/src/test/task-graph/TaskGraph.test.ts @@ -55,15 +55,19 @@ describe("TaskGraph", () => { const inputHandle = "input"; const outputHandle = "output"; + // A serial chain task[i] -> task[i+1] reads from the upstream task's + // OUTPUT port and writes to the downstream task's INPUT port. const expectedDataflows: Dataflow[] = [ - new Dataflow("task1", inputHandle, "task2", outputHandle), - new Dataflow("task2", inputHandle, "task3", outputHandle), + new Dataflow("task1", outputHandle, "task2", inputHandle), + new Dataflow("task2", outputHandle, "task3", inputHandle), ]; const result = serialGraph(tasks, inputHandle, outputHandle); expect(result).toBeInstanceOf(TaskGraph); expect(result.getDataflows()).toEqual(expectedDataflows); + expect(result.getDataflow("task1[output] ==> task2[input]")).toBeDefined(); + expect(result.getDataflow("task2[output] ==> task3[input]")).toBeDefined(); }); describe("subscribeToTaskStatus", () => { diff --git a/packages/test/src/test/task-graph/Workflow.test.ts b/packages/test/src/test/task-graph/Workflow.test.ts index 49936b0b9..f5841ab92 100644 --- a/packages/test/src/test/task-graph/Workflow.test.ts +++ b/packages/test/src/test/task-graph/Workflow.test.ts @@ -444,6 +444,16 @@ describe("Workflow", () => { expect(workflow.graph.getTasks()).toHaveLength(1); // Second task not added }); + it("public addTask throws (not just sets .error) when auto-connect fails", () => { + workflow.addTask(StringTask, { input: "test" }); + + // NumberTask cannot auto-connect to StringTask (string output vs number + // input). The public fluent addTask must throw, not silently no-op. + expect(() => workflow.addTask(NumberTask)).toThrow(WorkflowError); + // The failed task was removed; only the first task remains. + expect(workflow.graph.getTasks()).toHaveLength(1); + }); + it("should auto-connect TypedArray ports with different names by format", () => { // VectorOutputTask outputs 'vector', VectorsInputTask expects 'vectors' // They should match because both have format: "TypedArray" @@ -1300,16 +1310,21 @@ describe("Workflow — refactor regression net", () => { expect(events).toEqual(["start", "complete"]); }); - it("abort() during run causes run to reject and emits error exactly once", async () => { + it("abort() during run causes run to reject and emits abort (not error) exactly once", async () => { const w = new Workflow(); w.addTask(LongRunningTask, {}); const errors: string[] = []; + const aborts: string[] = []; w.on("error", (msg) => errors.push(msg)); + w.on("abort", (msg) => aborts.push(msg)); const runPromise = w.run(); setTimeout(() => void w.abort(), 10); await expect(runPromise).rejects.toBeDefined(); - expect(errors).toHaveLength(1); + // Cancellation surfaces on the dedicated 'abort' event, NOT 'error', + // so consumers can distinguish a cancelled run from a failure. + expect(aborts).toHaveLength(1); + expect(errors).toHaveLength(0); }); }); diff --git a/packages/test/src/test/task/ConditionalTaskCondition.test.ts b/packages/test/src/test/task/ConditionalTaskCondition.test.ts index 3385eddbb..8f94a00da 100644 --- a/packages/test/src/test/task/ConditionalTaskCondition.test.ts +++ b/packages/test/src/test/task/ConditionalTaskCondition.test.ts @@ -37,6 +37,21 @@ describe("ConditionalTask with serialized conditionConfig", () => { expect(task.isBranchActive("low")).toBe(false); }); + it("treats a non-numeric field as a non-match for ordering operators (no silent NaN)", async () => { + const task = new ConditionalTask({ branches: [] }); + + const conditionConfig: UIConditionConfig = { + branches: [{ id: "high", field: "value", operator: "greater_than", value: "100" }], + exclusive: true, + }; + + // `value` is a non-numeric string; greater_than must evaluate to false + // (and not silently treat NaN as a never-firing comparison). + await task.run({ value: "not-a-number", conditionConfig }); + + expect(task.isBranchActive("high")).toBe(false); + }); + it("should route to second branch when first does not match", async () => { const task = new ConditionalTask({ branches: [], diff --git a/packages/test/src/test/task/IteratorTask.test.ts b/packages/test/src/test/task/IteratorTask.test.ts index 3526c6a5e..e26a71116 100644 --- a/packages/test/src/test/task/IteratorTask.test.ts +++ b/packages/test/src/test/task/IteratorTask.test.ts @@ -843,6 +843,45 @@ describe("IteratorTask", () => { expect(result.processed).toEqual([8, 10, 12]); }); + test("aborting a MapTask mid-flight throws (not a partial COMPLETED result)", async () => { + class SlowItemTask extends Task<{ item: number }, { processed: number }> { + public static override type = "SlowItemTask"; + public static override inputSchema(): DataPortSchema { + return { + type: "object", + properties: { item: { type: "number" } }, + required: ["item"], + } as const satisfies DataPortSchema; + } + public static override outputSchema(): DataPortSchema { + return { + type: "object", + properties: { processed: { type: "number" } }, + } as const satisfies DataPortSchema; + } + override async execute(input: { item: number }): Promise<{ processed: number }> { + await sleep(20); + return { processed: input.item * 2 }; + } + } + + const mapTask = new MapTask({ + maxIterations: "unbounded", + batchSize: 1, + concurrencyLimit: 1, + }); + const subGraph = new TaskGraph(); + subGraph.addTask(new SlowItemTask({ id: "slow", defaults: { item: 0 } })); + mapTask.subGraph = subGraph; + + const runPromise = mapTask.run({ item: [1, 2, 3, 4, 5] } as TaskInput); + // Abort after the first batch begins but before all batches finish. + await sleep(10); + mapTask.abort(); + + await expect(runPromise).rejects.toThrow(); + }); + test("map preserveOrder=true should return outputs aligned to input index", async () => { class DelayedEchoTask extends Task<{ item: number }, { item: number }> { public static override type = "DelayedEchoTask"; diff --git a/packages/test/src/test/util/BaseError.test.ts b/packages/test/src/test/util/BaseError.test.ts index a3b15a457..df1a3b917 100644 --- a/packages/test/src/test/util/BaseError.test.ts +++ b/packages/test/src/test/util/BaseError.test.ts @@ -42,4 +42,31 @@ describe("BaseError", () => { expect(err.name).toBe("CustomError"); expect(err.toString()).toBe("CustomError: custom"); }); + + it("should be an instance of the native Error", () => { + const err = new BaseError("boom"); + expect(err).toBeInstanceOf(Error); + expect(err).toBeInstanceOf(BaseError); + }); + + it("should preserve instanceof Error and BaseError for subclasses", () => { + class CustomError extends BaseError { + public static override type = "CustomError"; + } + const err = new CustomError("custom"); + expect(err).toBeInstanceOf(Error); + expect(err).toBeInstanceOf(BaseError); + expect(err).toBeInstanceOf(CustomError); + }); + + it("should propagate an optional cause", () => { + const root = new Error("root cause"); + const err = new BaseError("wrapper", { cause: root }); + expect(err.cause).toBe(root); + }); + + it("should leave cause undefined when not provided", () => { + const err = new BaseError("no cause"); + expect(err.cause).toBeUndefined(); + }); }); diff --git a/packages/test/src/test/util/Container.test.ts b/packages/test/src/test/util/Container.test.ts index 47fb82b26..46b26781f 100644 --- a/packages/test/src/test/util/Container.test.ts +++ b/packages/test/src/test/util/Container.test.ts @@ -49,6 +49,36 @@ describe("Container", () => { it("should throw for unregistered service", () => { expect(() => container.get("unknown")).toThrow("Service not registered: unknown"); }); + + it("should honor re-registration after a singleton was instantiated", () => { + container.register("svc", () => ({ value: "first" })); + // Force instantiation so the instance is cached in services. + expect(container.get<{ value: string }>("svc").value).toBe("first"); + + // Re-register with a new factory; the new factory must take effect. + container.register("svc", () => ({ value: "second" })); + expect(container.get<{ value: string }>("svc").value).toBe("second"); + }); + + it("should drop the singleton flag when re-registering as transient", () => { + container.register("svc", () => ({ value: "first" })); + container.get("svc"); + + let callCount = 0; + container.register( + "svc", + () => { + callCount++; + return { value: callCount }; + }, + false + ); + + const a = container.get<{ value: number }>("svc"); + const b = container.get<{ value: number }>("svc"); + expect(a).not.toBe(b); + expect(callCount).toBe(2); + }); }); describe("registerInstance", () => { @@ -180,5 +210,51 @@ describe("Container", () => { expect(child.has("childOnly")).toBe(true); expect(container.has("childOnly")).toBe(false); }); + + it("should not dispose parent-owned singleton instances on child disposal", async () => { + let disposed = 0; + const instance = { + dispose() { + disposed++; + }, + }; + container.registerInstance("shared", instance); + + const child = container.createChildContainer(); + // Child shares the parent's instance. + expect(child.get("shared")).toBe(instance); + + await child.dispose(); + + // The inherited instance must NOT be disposed by the child... + expect(disposed).toBe(0); + // ...and must still be usable from the parent. + expect(container.get("shared")).toBe(instance); + + // The parent still owns it and disposes it when the parent is disposed. + await container.dispose(); + expect(disposed).toBe(1); + }); + + it("should dispose a child's own instances even when it inherits from parent", async () => { + const parentDisposed = { count: 0 }; + container.registerInstance("shared", { + dispose() { + parentDisposed.count++; + }, + }); + + const child = container.createChildContainer(); + let childDisposed = 0; + child.registerInstance("childOwned", { + dispose() { + childDisposed++; + }, + }); + + await child.dispose(); + expect(childDisposed).toBe(1); + expect(parentDisposed.count).toBe(0); + }); }); }); diff --git a/packages/test/src/test/util/EventEmitter.test.ts b/packages/test/src/test/util/EventEmitter.test.ts index 9a016439a..a8287870a 100644 --- a/packages/test/src/test/util/EventEmitter.test.ts +++ b/packages/test/src/test/util/EventEmitter.test.ts @@ -121,6 +121,23 @@ describe("EventEmitter", () => { expect(listener1).toHaveBeenCalledTimes(1); expect(listener2).toHaveBeenCalledTimes(1); }); + + it("should not drop a once listener added re-entrantly during emit", () => { + const added = mock((_value: string) => {}); + emitter.once("test", () => { + // Re-subscribe a once listener while dispatching the same event. + emitter.once("test", added); + }); + + emitter.emit("test", "first"); + // The re-entrantly added listener must not fire during the first emit... + expect(added).toHaveBeenCalledTimes(0); + + emitter.emit("test", "second"); + // ...but must survive and fire exactly once on the next emit. + expect(added).toHaveBeenCalledTimes(1); + expect(added).toHaveBeenCalledWith("second"); + }); }); describe("removeAllListeners", () => { @@ -140,6 +157,21 @@ describe("EventEmitter", () => { expect(multipleArgsListener).toHaveBeenCalledTimes(1); }); + it("should not resurrect an event cleared by a listener during emit", () => { + const onceListener = mock((_value: string) => {}); + emitter.once("test", () => { + emitter.removeAllListeners("test"); + }); + emitter.once("test", onceListener); + + emitter.emit("test", "hello"); + + // The clearing listener tore down the event mid-dispatch; the event must + // stay empty rather than being resurrected by once-listener cleanup. + expect(emitter.listenerCount("test")).toBe(0); + expect(emitter.eventNames()).not.toContain("test"); + }); + it("should remove all listeners for all events when no event is specified", () => { const testListener = mock((value: string) => {}); const multipleArgsListener = mock((arg1: string, arg2: number, arg3: boolean) => {}); diff --git a/packages/test/src/test/util/SchemaUtils.test.ts b/packages/test/src/test/util/SchemaUtils.test.ts index 1438d166b..f47c95ab5 100644 --- a/packages/test/src/test/util/SchemaUtils.test.ts +++ b/packages/test/src/test/util/SchemaUtils.test.ts @@ -300,6 +300,40 @@ describe("SchemaUtils", () => { expect(areSemanticallyCompatible(source, target)).toBe("static"); }); + + it("should reject an optional target property present on source with an incompatible type", () => { + const source: JsonSchema = { + type: "object", + properties: { + count: { type: "string" }, + }, + }; + const target: JsonSchema = { + type: "object", + properties: { + count: { type: "number" }, // optional (not required) but present on source + }, + }; + + expect(areSemanticallyCompatible(source, target)).toBe("incompatible"); + }); + + it("should mark runtime when an optional shared property narrows via format", () => { + const source: JsonSchema = { + type: "object", + properties: { + model: { type: "string", format: "model" }, + }, + }; + const target: JsonSchema = { + type: "object", + properties: { + model: { type: "string", format: "model:EmbeddingTask" }, // optional + }, + }; + + expect(areSemanticallyCompatible(source, target)).toBe("runtime"); + }); }); describe("array schemas", () => { @@ -349,7 +383,9 @@ describe("SchemaUtils", () => { expect(areSemanticallyCompatible(source, target)).toBe("incompatible"); }); - it("should handle tuple types (array items)", () => { + it("should require a uniform source to satisfy every tuple position", () => { + // A uniform string[] source cannot satisfy a [string, number] tuple + // target: every element must fit every position, and string ≠ number. const source: JsonSchema = { type: "array", items: { type: "string" }, @@ -359,9 +395,38 @@ describe("SchemaUtils", () => { items: [{ type: "string" }, { type: "number" }], }; + expect(areSemanticallyCompatible(source, target)).toBe("incompatible"); + }); + + it("should accept a uniform source matching every tuple position statically", () => { + const source: JsonSchema = { + type: "array", + items: { type: "string" }, + }; + const target: JsonSchema = { + type: "array", + items: [{ type: "string" }, { type: "string" }], + }; + expect(areSemanticallyCompatible(source, target)).toBe("static"); }); + it("should mark a uniform source as runtime when a tuple position narrows", () => { + const source: JsonSchema = { + type: "array", + items: { type: "string", format: "model" }, + }; + const target: JsonSchema = { + type: "array", + items: [ + { type: "string", format: "model" }, + { type: "string", format: "model:EmbeddingTask" }, + ], + }; + + expect(areSemanticallyCompatible(source, target)).toBe("runtime"); + }); + it("should return incompatible when array items are incompatible", () => { const source: JsonSchema = { type: "array", @@ -696,6 +761,34 @@ describe("SchemaUtils", () => { expect(areSemanticallyCompatible(source, target)).toBe("incompatible"); }); + it("should compare the full multi-segment narrowing tail (incompatible)", () => { + // Both share name `points` and first narrowing segment `3d`, but differ + // in the third segment. The comparator must not collapse to `3d`. + const source: JsonSchema = { + type: "string", + format: "points:3d:relative", + }; + const target: JsonSchema = { + type: "string", + format: "points:3d:meters", + }; + + expect(areSemanticallyCompatible(source, target)).toBe("incompatible"); + }); + + it("should treat identical multi-segment narrowing as static", () => { + const source: JsonSchema = { + type: "string", + format: "points:3d:meters", + }; + const target: JsonSchema = { + type: "string", + format: "points:3d:meters", + }; + + expect(areSemanticallyCompatible(source, target)).toBe("static"); + }); + it("should check format compatibility for all types", () => { const source: JsonSchema = { type: "number", diff --git a/packages/test/src/test/util/SchemaValidation.test.ts b/packages/test/src/test/util/SchemaValidation.test.ts index f07d61587..873711e78 100644 --- a/packages/test/src/test/util/SchemaValidation.test.ts +++ b/packages/test/src/test/util/SchemaValidation.test.ts @@ -194,6 +194,23 @@ describe("SchemaValidation", () => { ).toBe(true); }); + it("should report array property values as errors", () => { + const schema = { + type: "object", + properties: { + field: [], + }, + } as unknown as DataPortSchema; + + const result = validateDataPortSchema(schema); + expect(result.valid).toBe(false); + expect( + result.errors.some( + (e) => e.path === "/properties/field" && e.message.includes("Expected schema object") + ) + ).toBe(true); + }); + it("should detect unknown types in deeply nested schemas", () => { const schema = { type: "object", diff --git a/packages/test/src/test/util/WorkerServerBase.race.test.ts b/packages/test/src/test/util/WorkerServerBase.race.test.ts index be5aa058a..6ad84cbd6 100644 --- a/packages/test/src/test/util/WorkerServerBase.race.test.ts +++ b/packages/test/src/test/util/WorkerServerBase.race.test.ts @@ -102,6 +102,47 @@ describe("WorkerServerBase abort-before-call race", () => { expect(errorReplies.length).toBe(1); }); + it("does not post a second terminal reply when a non-cooperative call arrives after the dedupe window", async () => { + vi.useFakeTimers(); + try { + const server = new WorkerServerBase(); + // A non-cooperative run-fn: ignores the abort signal and returns normally. + server.registerRunFunction("ignoresAbort", async () => { + // intentionally does not check signal.aborted + }); + + // Abort arrives first; handleAbort posts the terminal error and schedules + // cleanup of the completed id. + await server.handleMessage({ type: "message", data: { type: "abort", id: "req-dup" } }); + + // Advance past the OLD 5s completed-request cleanup window but stay within + // the 30s pending-abort TTL — this is the window where a duplicate used + // to slip through. + await vi.advanceTimersByTimeAsync(6000); + + await server.handleMessage({ + type: "message", + data: { + type: "call", + id: "req-dup", + functionName: "ignoresAbort", + run: true, + args: [{}, undefined, undefined, undefined], + }, + }); + + const errorReplies = stub.captured.filter((m) => m.id === "req-dup" && m.type === "error"); + const completeReplies = stub.captured.filter( + (m) => m.id === "req-dup" && m.type === "complete" + ); + // Exactly one terminal reply (the abort error), never a second response. + expect(errorReplies.length).toBe(1); + expect(completeReplies.length).toBe(0); + } finally { + vi.useRealTimers(); + } + }); + it("aborts a stream call whose abort arrived before the call", async () => { const server = new WorkerServerBase(); const seenAborted: boolean[] = []; diff --git a/packages/test/src/test/util/directedAcyclicGraph.test.ts b/packages/test/src/test/util/directedAcyclicGraph.test.ts index 94c7e193f..36331a8ae 100644 --- a/packages/test/src/test/util/directedAcyclicGraph.test.ts +++ b/packages/test/src/test/util/directedAcyclicGraph.test.ts @@ -47,6 +47,35 @@ describe("Directed Acyclic Graph", () => { expect(() => DirectedAcyclicGraph.fromDirectedGraph(graph)).toThrow(CycleError); }); + it("produces a DAG independent of the source graph (no shared backing state)", () => { + interface NodeType { + name: string; + } + const graph = new DirectedGraph((n: NodeType) => n.name, edgeIdentity); + + graph.insert({ name: "A" }); + graph.insert({ name: "B" }); + graph.addEdge("A", "B"); + + const dag = DirectedAcyclicGraph.fromDirectedGraph(graph); + + // Mutating the source graph after conversion must not change the DAG. + graph.insert({ name: "C" }); + graph.addEdge("B", "C"); + + expect( + dag + .getNodes() + .map((n) => n.name) + .sort() + ).toEqual(["A", "B"]); + expect(dag.canReachFrom("A", "C" as unknown as string)).toBe(false); + + // And mutating the DAG must not corrupt the source graph. + dag.insert({ name: "D" }); + expect(graph.getNodes().map((n) => n.name)).not.toContain("D"); + }); + it("can add an edge only if it wouldn't create a cycle", () => { interface NodeType { name: string; diff --git a/packages/test/src/test/util/directedGraph.test.ts b/packages/test/src/test/util/directedGraph.test.ts index eec95da6f..85b67cd94 100644 --- a/packages/test/src/test/util/directedGraph.test.ts +++ b/packages/test/src/test/util/directedGraph.test.ts @@ -233,6 +233,49 @@ describe("Directed Graph", () => { expect(graph.canReachFrom("nonexistent" as unknown as string, "A")).toBe(false); }); + it("canReachFrom terminates on a cyclic graph (no stack overflow)", () => { + interface NodeType { + name: string; + } + const graph = new DirectedGraph((n: NodeType) => n.name, edgeIdentity); + + graph.insert({ name: "A" }); + graph.insert({ name: "B" }); + graph.insert({ name: "C" }); + graph.insert({ name: "D" }); + + // Build a cycle A -> B -> C -> A reachable from A, with D unreachable. + graph.addEdge("A", "B"); + graph.addEdge("B", "C"); + graph.addEdge("C", "A"); + + // Without a visited set this recurses forever and overflows the stack. + expect(graph.canReachFrom("A", "C")).toBe(true); + expect(graph.canReachFrom("A", "D")).toBe(false); + expect(graph.canReachFrom("B", "A")).toBe(true); + }); + + it("canReachFrom handles diamond graphs without exponential re-walking", () => { + interface NodeType { + name: string; + } + const graph = new DirectedGraph((n: NodeType) => n.name, edgeIdentity); + + graph.insert({ name: "A" }); + graph.insert({ name: "B" }); + graph.insert({ name: "C" }); + graph.insert({ name: "D" }); + + // Diamond: A -> B, A -> C, B -> D, C -> D (D shares two paths). + graph.addEdge("A", "B"); + graph.addEdge("A", "C"); + graph.addEdge("B", "D"); + graph.addEdge("C", "D"); + + expect(graph.canReachFrom("A", "D")).toBe(true); + expect(graph.canReachFrom("D", "A")).toBe(false); + }); + it("can return a subgraph based on walking from a start node", () => { interface NodeType { name: string; diff --git a/packages/test/src/test/util/graph.test.ts b/packages/test/src/test/util/graph.test.ts index 141b1271c..4dcb82708 100644 --- a/packages/test/src/test/util/graph.test.ts +++ b/packages/test/src/test/util/graph.test.ts @@ -364,6 +364,55 @@ describe("Graph", () => { expect((graph as any).adjacency[0][1]).toBeFalsy(); }); + it("does not emit edge-removed when no edge matched", () => { + interface NodeType { + a: number; + b: string; + } + interface EdgeType { + c: string; + } + const graph = new Graph((n: NodeType) => n.a.toFixed(2), edgeIdentity); + + graph.insert({ a: 1, b: "b" }); + graph.insert({ a: 2, b: "b" }); + graph.addEdge("1.00", "2.00", { c: "c1" }); + + const removed: unknown[] = []; + graph.on("edge-removed", (id) => removed.push(id)); + + // No edge with this identity exists between the pair → no event. + graph.removeEdge("1.00", "2.00", "does-not-exist" as any); + expect(removed).toEqual([]); + expect(graph.getEdges().length).toBe(1); + }); + + it("emits edge-removed with real ids when removing all edges of a pair", () => { + interface NodeType { + a: number; + b: string; + } + interface EdgeType { + c: string; + } + const graph = new Graph((n: NodeType) => n.a.toFixed(2), edgeIdentity); + + graph.insert({ a: 1, b: "b" }); + graph.insert({ a: 2, b: "b" }); + const id1 = graph.addEdge("1.00", "2.00", { c: "c1" }); + const id2 = graph.addEdge("1.00", "2.00", { c: "c2" }); + + const removed: unknown[] = []; + graph.on("edge-removed", (id) => removed.push(id)); + + // Remove-all (no edgeIdentity): one event per removed edge, with real ids + // (never `undefined` cast to EdgeId). + graph.removeEdge("1.00", "2.00"); + expect(removed.sort()).toEqual([id1, id2].sort()); + expect(removed).not.toContain(undefined); + expect(graph.getEdges().length).toBe(0); + }); + it("can return the nodes", () => { interface NodeType { a: number; diff --git a/packages/test/src/test/util/parsePartialJson.test.ts b/packages/test/src/test/util/parsePartialJson.test.ts index e5a93fc98..e1a197961 100644 --- a/packages/test/src/test/util/parsePartialJson.test.ts +++ b/packages/test/src/test/util/parsePartialJson.test.ts @@ -70,6 +70,22 @@ describe("parsePartialJson", () => { const result = parsePartialJson('{"tags":["a","b"'); expect(result).toEqual({ tags: ["a", "b"] }); }); + + it("should yield a partial object when the only property has an incomplete keyword value", () => { + // First-property streaming: single key, value still arriving. + expect(parsePartialJson('{"a": tru')).toEqual({}); + expect(parsePartialJson('{"flag": fal')).toEqual({}); + expect(parsePartialJson('{"x": nul')).toEqual({}); + }); + + it("should yield a partial object when the only property has an incomplete number value", () => { + // A bare `1.` is not valid JSON yet; drop the incomplete pair. + expect(parsePartialJson('{"n": 1.')).toEqual({}); + }); + + it("should keep a single property whose number value is already complete", () => { + expect(parsePartialJson('{"n": 30')).toEqual({ n: 30 }); + }); }); describe("progressive parsing", () => { diff --git a/packages/test/src/test/vector/InMemoryVectorStorage.validation.test.ts b/packages/test/src/test/vector/InMemoryVectorStorage.validation.test.ts index 318b475b8..93e3c898e 100644 --- a/packages/test/src/test/vector/InMemoryVectorStorage.validation.test.ts +++ b/packages/test/src/test/vector/InMemoryVectorStorage.validation.test.ts @@ -120,4 +120,73 @@ describe("InMemoryVectorStorage validation", () => { ); }); }); + + describe("similaritySearch result behavior", () => { + it("returns negatively-correlated hits by default (no implicit score floor)", async () => { + // Store a vector that is the exact opposite of the query so cosine + // similarity is -1. The default scoreThreshold must not drop it. + await storage.put({ id: "opposite", vector: new Float32Array([-1, 0, 0, 0]), metadata: {} }); + await storage.put({ id: "same", vector: new Float32Array([1, 0, 0, 0]), metadata: {} }); + + const results = await storage.similaritySearch(new Float32Array([1, 0, 0, 0])); + + // Both rows come back; the negatively-correlated one is not silently + // filtered out (topK is honored regardless of sign). + expect(results).toHaveLength(2); + const opposite = results.find((r) => r.id === "opposite"); + expect(opposite).toBeDefined(); + expect(opposite!.score).toBeCloseTo(-1, 5); + }); + + it("still honors an explicit scoreThreshold floor", async () => { + await storage.put({ id: "opposite", vector: new Float32Array([-1, 0, 0, 0]), metadata: {} }); + await storage.put({ id: "same", vector: new Float32Array([1, 0, 0, 0]), metadata: {} }); + + const results = await storage.similaritySearch(new Float32Array([1, 0, 0, 0]), { + scoreThreshold: 0, + }); + + expect(results).toHaveLength(1); + expect(results[0].id).toBe("same"); + }); + + it("treats a present-but-null metadata value as a non-match under a filter (no throw)", async () => { + // A row whose metadata column is null must not crash a filtered search; + // it is coalesced to {} and excluded by the filter rather than matched. + await storage.put({ + id: "null-meta", + vector: new Float32Array([1, 0, 0, 0]), + metadata: null as unknown as Record, + }); + await storage.put({ + id: "tagged", + vector: new Float32Array([1, 0, 0, 0]), + metadata: { tag: "keep" }, + }); + + const results = await storage.similaritySearch(new Float32Array([1, 0, 0, 0]), { + filter: { tag: "keep" }, + }); + + expect(results).toHaveLength(1); + expect(results[0].id).toBe("tagged"); + }); + + it("emits a similaritySearch event with the returned results", async () => { + await storage.put({ id: "a", vector: new Float32Array([1, 0, 0, 0]), metadata: {} }); + + const seen: Array<{ query: unknown; results: unknown[] }> = []; + // The event lives on the vector extension of the tabular event surface; + // the public `on` is typed to the tabular names, so cast at the call site. + ( + storage as unknown as { + on: (name: string, fn: (query: unknown, results: unknown[]) => void) => void; + } + ).on("similaritySearch", (query, results) => seen.push({ query, results })); + + const out = await storage.similaritySearch(new Float32Array([1, 0, 0, 0])); + expect(seen).toHaveLength(1); + expect(seen[0].results).toBe(out); + }); + }); }); diff --git a/packages/test/src/test/vector/IndexedDbVectorStorage.validation.test.ts b/packages/test/src/test/vector/IndexedDbVectorStorage.validation.test.ts index d96547683..a758d8cea 100644 --- a/packages/test/src/test/vector/IndexedDbVectorStorage.validation.test.ts +++ b/packages/test/src/test/vector/IndexedDbVectorStorage.validation.test.ts @@ -130,4 +130,69 @@ describe("IndexedDbVectorStorage validation", () => { ); }); }); + + describe("similaritySearch result behavior", () => { + it("returns negatively-correlated hits by default (no implicit score floor)", async () => { + // A vector opposite to the query has cosine similarity -1; the default + // (no floor) must not silently drop it. + await storage.put({ id: "opposite", vector: new Float32Array([-1, 0, 0, 0]), metadata: {} }); + await storage.put({ id: "same", vector: new Float32Array([1, 0, 0, 0]), metadata: {} }); + + const results = await storage.similaritySearch(new Float32Array([1, 0, 0, 0])); + + expect(results).toHaveLength(2); + const opposite = results.find((r) => r.id === "opposite"); + expect(opposite).toBeDefined(); + expect(opposite!.score).toBeCloseTo(-1, 5); + }); + + it("still honors an explicit scoreThreshold floor", async () => { + await storage.put({ id: "opposite", vector: new Float32Array([-1, 0, 0, 0]), metadata: {} }); + await storage.put({ id: "same", vector: new Float32Array([1, 0, 0, 0]), metadata: {} }); + + const results = await storage.similaritySearch(new Float32Array([1, 0, 0, 0]), { + scoreThreshold: 0, + }); + + expect(results).toHaveLength(1); + expect(results[0].id).toBe("same"); + }); + + it("treats a present-but-null metadata value as a non-match under a filter (no throw)", async () => { + await storage.put({ + id: "null-meta", + vector: new Float32Array([1, 0, 0, 0]), + metadata: null as unknown as Record, + }); + await storage.put({ + id: "tagged", + vector: new Float32Array([1, 0, 0, 0]), + metadata: { tag: "keep" }, + }); + + const results = await storage.similaritySearch(new Float32Array([1, 0, 0, 0]), { + filter: { tag: "keep" }, + }); + + expect(results).toHaveLength(1); + expect(results[0].id).toBe("tagged"); + }); + + it("emits a similaritySearch event with the returned results", async () => { + await storage.put({ id: "a", vector: new Float32Array([1, 0, 0, 0]), metadata: {} }); + + const seen: Array<{ query: unknown; results: unknown[] }> = []; + // The event lives on the vector extension of the tabular event surface; + // the public `on` is typed to the tabular names, so cast at the call site. + ( + storage as unknown as { + on: (name: string, fn: (query: unknown, results: unknown[]) => void) => void; + } + ).on("similaritySearch", (query, results) => seen.push({ query, results })); + + const out = await storage.similaritySearch(new Float32Array([1, 0, 0, 0])); + expect(seen).toHaveLength(1); + expect(seen[0].results).toBe(out); + }); + }); }); diff --git a/packages/util/src/di/Container.ts b/packages/util/src/di/Container.ts index 550457853..f3edf522f 100644 --- a/packages/util/src/di/Container.ts +++ b/packages/util/src/di/Container.ts @@ -12,6 +12,12 @@ export class Container { private factories: Map any> = new Map(); private singletons: Set = new Set(); private resolving: string[] = []; + /** + * Tokens whose cached instance was inherited from a parent container (via + * {@link createChildContainer}). These instances are owned by the parent, so + * disposing this container must NOT dispose them. + */ + private inheritedServices: Set = new Set(); /** * Register a service factory @@ -21,8 +27,15 @@ export class Container { */ register(token: string, factory: () => T, singleton = true): void { this.factories.set(token, factory); + // Evict any previously instantiated singleton so the new factory actually + // takes effect on the next get(). Otherwise get() would keep returning the + // stale cached instance and the re-registration would be silently dead. + this.services.delete(token); + this.inheritedServices.delete(token); if (singleton) { this.singletons.add(token); + } else { + this.singletons.delete(token); } } @@ -48,6 +61,8 @@ export class Container { registerInstance(token: string, instance: T): void { this.services.set(token, instance); this.singletons.add(token); + // An explicitly registered instance is owned by this container. + this.inheritedServices.delete(token); } /** @@ -101,6 +116,7 @@ export class Container { this.services.delete(token); this.factories.delete(token); this.singletons.delete(token); + this.inheritedServices.delete(token); } /** @@ -110,8 +126,11 @@ export class Container { async dispose(): Promise { const errors: unknown[] = []; try { - for (const service of this.services.values()) { + for (const [token, service] of this.services) { if (service == null) continue; + // Instances inherited from a parent container are owned by the parent; + // disposing them here would leave the parent holding a disposed object. + if (this.inheritedServices.has(token)) continue; try { if (typeof service[Symbol.asyncDispose] === "function") { await service[Symbol.asyncDispose](); @@ -128,6 +147,7 @@ export class Container { this.services.clear(); this.factories.clear(); this.singletons.clear(); + this.inheritedServices.clear(); } if (errors.length > 0) { throw new AggregateError(errors, "One or more services failed to dispose"); @@ -156,6 +176,9 @@ export class Container { if (this.singletons.has(token)) { child.services.set(token, service); child.singletons.add(token); + // Mark the shared instance as parent-owned so child.dispose() does not + // dispose an instance the parent still hands out. + child.inheritedServices.add(token); } }); diff --git a/packages/util/src/di/ServiceRegistry.ts b/packages/util/src/di/ServiceRegistry.ts index 3eaa26274..20aa1f3df 100644 --- a/packages/util/src/di/ServiceRegistry.ts +++ b/packages/util/src/di/ServiceRegistry.ts @@ -27,7 +27,7 @@ export function createServiceToken(id: string): ServiceToken { * Service registry for managing and accessing services */ export class ServiceRegistry { - public container: Container; + public readonly container: Container; /** * Create a new service registry diff --git a/packages/util/src/events/EventEmitter.ts b/packages/util/src/events/EventEmitter.ts index b9f8a3011..6566b87be 100644 --- a/packages/util/src/events/EventEmitter.ts +++ b/packages/util/src/events/EventEmitter.ts @@ -184,22 +184,46 @@ export class EventEmitter | undefined = this.listeners[event]; if (listeners) { - // Snapshot the listener array to avoid issues with concurrent modification + // Snapshot the listener entries to avoid issues with concurrent + // modification (a listener may add/remove listeners during dispatch). const snapshot = [...listeners]; const errors: unknown[] = []; - for (const { listener } of snapshot) { + // Track the exact `once` entries we invoke so we can remove only those, + // by reference, after dispatch. Filtering the live array instead would + // (a) strip `once` listeners added re-entrantly before they ever fire and + // (b) resurrect the event if a listener called removeAllListeners(). + const invokedOnce: Array<{ + listener: EventListener; + once?: boolean; + }> = []; + for (const entry of snapshot) { + if (entry.once) { + invokedOnce.push(entry); + } try { - listener(...args); + entry.listener(...args); } catch (e) { errors.push(e); } } - // Remove once listeners we just called - this.listeners[event] = listeners.filter((l) => !l.once); + // Remove only the `once` listeners we actually invoked, by reference, + // from whatever the live array is now — but only if the event still + // exists (a listener may have torn it down via removeAllListeners()). + if (invokedOnce.length > 0) { + const live = this.listeners[event]; + if (live) { + for (const entry of invokedOnce) { + const index = live.indexOf(entry); + if (index >= 0) { + live.splice(index, 1); + } + } + } + } if (this.maxListeners > 0 && (this.listeners[event]?.length ?? 0) <= this.maxListeners) { this.warnedEvents.delete(event); } - // Re-throw errors after all listeners have been called + // Re-throw errors after all listeners have been called. if (errors.length > 1) { throw new AggregateError( errors, diff --git a/packages/util/src/graph/directedAcyclicGraph.ts b/packages/util/src/graph/directedAcyclicGraph.ts index b27c7a5ae..e8eef2c5b 100644 --- a/packages/util/src/graph/directedAcyclicGraph.ts +++ b/packages/util/src/graph/directedAcyclicGraph.ts @@ -3,7 +3,7 @@ // license: MIT import { DirectedGraph } from "./directedGraph"; -import { CycleError } from "./errors"; +import { CycleError, GraphInvariantError } from "./errors"; /** * # DirectedAcyclicGraph @@ -41,9 +41,19 @@ export class DirectedAcyclicGraph< graph.edgeIdentity ); - toRet.nodes = (graph as any).nodes; - toRet.adjacency = (graph as any).adjacency; - toRet.nodeIndexMap = (graph as any).nodeIndexMap; + // Deep-copy the backing state so the resulting DAG is an independent value. + // Aliasing the source graph's Maps/arrays by reference would let a later + // mutation of either graph silently corrupt the other (and break the DAG's + // acyclicity invariant from the outside). + const sourceNodes = (graph as any).nodes as Map; + const sourceAdjacency = (graph as any).adjacency as Array | null>>; + const sourceNodeIndexMap = (graph as any).nodeIndexMap as Map; + + toRet.nodes = new Map(sourceNodes); + toRet.nodeIndexMap = new Map(sourceNodeIndexMap); + toRet.adjacency = sourceAdjacency.map((row) => + row.map((cell) => (cell === null ? null : [...cell])) + ); return toRet; } @@ -71,7 +81,8 @@ export class DirectedAcyclicGraph< // Invalidate cache of toposorted nodes this._topologicallySortedNodes = undefined; - return super.addEdge(sourceNodeIdentity, targetNodeIdentity, edge, true); + // Acyclicity was just validated above; skip the redundant cycle check. + return this.addEdgeMaintainingCyclicality(sourceNodeIdentity, targetNodeIdentity, edge, true); } /** @@ -121,11 +132,15 @@ export class DirectedAcyclicGraph< while (toSearch.length > 0) { const n = toSearch.pop(); if (n === undefined) { - throw new Error("Unexpected empty array"); + throw new GraphInvariantError( + "Kahn's algorithm popped an empty search frontier; toSearch desynced from node set" + ); } const curNode = this.nodes.get(n[0]); if (curNode == null) { - throw new Error("This should never happen"); + throw new GraphInvariantError( + `Zero-indegree node ${String(n[0])} is missing from the node map; node/index bookkeeping desynced` + ); } toReturn.push(curNode); @@ -140,7 +155,9 @@ export class DirectedAcyclicGraph< toSearch.push([nodeIndices[index], 0]); } } else { - throw new Error("This should never happen"); + throw new GraphInvariantError( + `Edge target ${String(nodeIndices[index])} has no recorded indegree; adjacency/indegree maps desynced` + ); } } }); diff --git a/packages/util/src/graph/directedGraph.ts b/packages/util/src/graph/directedGraph.ts index 9b4d3732f..127efd84b 100644 --- a/packages/util/src/graph/directedGraph.ts +++ b/packages/util/src/graph/directedGraph.ts @@ -97,14 +97,29 @@ export class DirectedGraph()); + } + + /** + * Internal DFS that threads a visited set so cyclic graphs (which + * {@link DirectedGraph} explicitly permits) terminate instead of recursing + * forever, and shared descendants are not re-walked exponentially. + */ + private canReachFromInternal(startNode: NodeId, endNode: NodeId, visited: Set): boolean { const startNodeIndex = this.getNodeIndex(startNode); const endNodeIndex = this.getNodeIndex(endNode); @@ -138,16 +161,22 @@ export class DirectedGraph((carry, edge, index) => { if (carry || edge === null) { return carry; } - return this.canReachFrom(nodeKeys[index], endNode); + return this.canReachFromInternal(nodeKeys[index], endNode, visited); }, false); } diff --git a/packages/util/src/graph/errors.ts b/packages/util/src/graph/errors.ts index 2903d328b..900916ffc 100644 --- a/packages/util/src/graph/errors.ts +++ b/packages/util/src/graph/errors.ts @@ -53,6 +53,28 @@ export class NodeDoesntExistError extends BaseError { } } +/** + * # GraphInvariantError + * + * Thrown when an internal graph structural invariant is violated — for example + * when the node map, adjacency matrix, and positional index bookkeeping have + * desynced. This indicates corruption (e.g. mutating shared backing state) and + * names which structure was inconsistent so the failure is debuggable rather + * than a bare "this should never happen". + * + * @category Errors + */ +export class GraphInvariantError extends BaseError { + public static override type: string = "GraphInvariantError"; + constructor(message: string) { + super(message); + this.name = "GraphInvariantError"; + + // This bs is due to a limitation of Typescript: https://github.com/facebook/jest/issues/8279 + Object.setPrototypeOf(this, GraphInvariantError.prototype); + } +} + /** * # CycleError * diff --git a/packages/util/src/graph/graph.ts b/packages/util/src/graph/graph.ts index cfb6b99b8..a256cee9b 100644 --- a/packages/util/src/graph/graph.ts +++ b/packages/util/src/graph/graph.ts @@ -183,7 +183,10 @@ export class Graph { this.nodeIndexMap.set(id, this.adjacency.length); this.nodes.set(id, node); - this.adjacency.map((adj) => adj.push(null)); + // Append a new column (the new node) to every existing row, then push the + // new node's own row. forEach signals the side-effect intent and avoids the + // throwaway array that Array.prototype.map would allocate. + this.adjacency.forEach((adj) => adj.push(null)); this.adjacency.push(new Array>(this.adjacency.length + 1).fill(null)); this.emit("node-added", id); @@ -227,7 +230,7 @@ export class Graph { if (!isOverwrite) { this.nodeIndexMap.set(id, this.adjacency.length); - this.adjacency.map((adj) => adj.push(null)); + this.adjacency.forEach((adj) => adj.push(null)); this.adjacency.push(new Array>(this.adjacency.length + 1).fill(null)); this.emit("node-added", id); } else { @@ -263,15 +266,24 @@ export class Graph { const node1Index = this.getNodeIndex(node1Identity); const node2Index = this.getNodeIndex(node2Identity); + const id = this.edgeIdentity(edge, node1Identity, node2Identity); + + // De-duplicate by edgeIdentity (matching removeEdge), not raw value + // equality. Using includes() would conflate two edges that are distinct by + // identity but reference/value-equal, and would prevent a multigraph for + // the default `true` edge type since every edge value is identical. if (this.adjacency[node1Index][node2Index] === null) { this.adjacency[node1Index][node2Index] = [edge]; } else { - if (!this.adjacency[node1Index][node2Index]!.includes(edge)) { - this.adjacency[node1Index][node2Index]!.push(edge); + const existing = this.adjacency[node1Index][node2Index]!; + const alreadyPresent = existing.some( + (e) => this.edgeIdentity(e, node1Identity, node2Identity) === id + ); + if (!alreadyPresent) { + existing.push(edge); } } - const id = this.edgeIdentity(edge, node1Identity, node2Identity); this.emit("edge-added", id); return id; @@ -414,9 +426,19 @@ export class Graph { const node2Index = this.getNodeIndex(node2Identity); if (edgeIdentity === undefined) { + // Remove all edges between the pair. Emit one event per actually-removed + // edge carrying its real EdgeId, rather than a single event with + // `undefined` cast to EdgeId (which violates the listener contract). + const edgeList = this.adjacency[node1Index][node2Index]; this.adjacency[node1Index][node2Index] = null; + if (edgeList !== null) { + for (const edge of edgeList) { + this.emit("edge-removed", this.edgeIdentity(edge, node1Identity, node2Identity)); + } + } } else { - // Remove the specific edge matching edgeIdentity from this node pair + // Remove the specific edge matching edgeIdentity from this node pair. + // Only emit edge-removed if an edge was actually removed. const edgeList = this.adjacency[node1Index][node2Index]; if (edgeList !== null) { for (let edgeIndex = 0; edgeIndex < edgeList.length; edgeIndex++) { @@ -424,15 +446,15 @@ export class Graph { this.edgeIdentity(edgeList[edgeIndex], node1Identity, node2Identity) === edgeIdentity ) { edgeList.splice(edgeIndex, 1); + if (edgeList.length === 0) { + this.adjacency[node1Index][node2Index] = null; + } + this.emit("edge-removed", edgeIdentity); break; } } - if (edgeList.length === 0) { - this.adjacency[node1Index][node2Index] = null; - } } } - this.emit("edge-removed", edgeIdentity as EdgeId); } /** diff --git a/packages/util/src/json-schema/SchemaUtils.ts b/packages/util/src/json-schema/SchemaUtils.ts index cf0ec76ed..8805b86fe 100644 --- a/packages/util/src/json-schema/SchemaUtils.ts +++ b/packages/util/src/json-schema/SchemaUtils.ts @@ -48,7 +48,9 @@ import { FORMAT_PATTERN } from "./SchemaValidation"; /** * Checks if two format strings are compatible. - * Format: /\w+(:\w+)?/ where first part is the "name" and optional second part narrows the type. + * Format: a base "name" optionally followed by a narrowing tail after the first + * colon (e.g. `model`, `model:EmbeddingTask`, `points:3d:meters`). The narrowing + * tail is everything after the first colon and is compared in full. * - Same name without narrowing: static compatible * - Source name matches target narrowed name: runtime compatible * - Different names or incompatible narrowing: incompatible @@ -61,8 +63,16 @@ function areFormatStringsCompatible( return "incompatible"; } - const [sourceName, sourceNarrow] = sourceFormat.split(":"); - const [targetName, targetNarrow] = targetFormat.split(":"); + // Split on the FIRST colon only: everything after the name is the narrowing + // tail. FORMAT_PATTERN permits multiple colon segments (e.g. + // `points:3d:meters`), so we must compare the full tail rather than only the + // first segment — otherwise `a:b:c` and `a:b:d` would collapse to equal. + const sourceColon = sourceFormat.indexOf(":"); + const targetColon = targetFormat.indexOf(":"); + const sourceName = sourceColon === -1 ? sourceFormat : sourceFormat.slice(0, sourceColon); + const sourceNarrow = sourceColon === -1 ? undefined : sourceFormat.slice(sourceColon + 1); + const targetName = targetColon === -1 ? targetFormat : targetFormat.slice(0, targetColon); + const targetNarrow = targetColon === -1 ? undefined : targetFormat.slice(targetColon + 1); // Different base names are incompatible if (sourceName !== targetName) { @@ -398,6 +408,7 @@ export function areSemanticallyCompatible( // Check if all required target properties are present and compatible in source const targetRequired = targetSchema.required || []; + const requiredSet = new Set(targetRequired); let hasRuntime = false; for (const propName of targetRequired) { @@ -420,6 +431,24 @@ export function areSemanticallyCompatible( } } + // Optional target properties that ALSO exist on the source must still have + // compatible types — a present-but-optional property feeding an + // incompatible value would otherwise pass the gate and fail at runtime. + for (const [propName, targetProp] of Object.entries( + targetProperties as Record + )) { + if (requiredSet.has(propName)) continue; + const sourceProp = (sourceProperties as Record)?.[propName]; + // Property absent on the source is fine (it is optional on the target). + if (!sourceProp || !targetProp) continue; + const propCompatibility = areSemanticallyCompatible(sourceProp, targetProp); + if (propCompatibility === "incompatible") { + return "incompatible"; + } else if (propCompatibility === "runtime") { + hasRuntime = true; + } + } + // Check if target allows additional properties if (targetSchema.additionalProperties === false) { // Target doesn't allow additional properties, so source can't have extra properties @@ -495,9 +524,28 @@ export function areSemanticallyCompatible( return "incompatible"; } - // If target items is an array (tuple), check if source is compatible with any item + // If target items is an array (tuple), the target describes positional + // element types. The source here has a single uniform item schema (this + // branch is only reached when sourceItems is NOT an array), so every source + // element must satisfy EVERY tuple position — not merely match one of them. if (Array.isArray(targetItems)) { - return isCompatibleWithUnion(sourceItems as JsonSchema, targetItems as JsonSchema[]); + const tuple = targetItems as JsonSchema[]; + if (tuple.length === 0) { + return "static"; + } + let tupleRuntime = false; + for (const positionSchema of tuple) { + const positionCompatibility = areSemanticallyCompatible( + sourceItems as JsonSchema, + positionSchema + ); + if (positionCompatibility === "incompatible") { + return "incompatible"; + } else if (positionCompatibility === "runtime") { + tupleRuntime = true; + } + } + return tupleRuntime ? "runtime" : "static"; } // Fallback to static if we can't determine @@ -562,8 +610,9 @@ export function areSemanticallyCompatible( } /** - * Checks if two object schemas are semantically compatible. - * This is a helper function for checking object-level schema compatibility. + * Backward-compatible alias for {@link areSemanticallyCompatible}. It applies no + * object-specific handling — the underlying function already dispatches on the + * schema's type. Prefer {@link areSemanticallyCompatible} directly. */ export function areObjectSchemasSemanticallyCompatible( sourceSchema: JsonSchema, diff --git a/packages/util/src/json-schema/SchemaValidation.ts b/packages/util/src/json-schema/SchemaValidation.ts index 93ae9f821..4c0f353f2 100644 --- a/packages/util/src/json-schema/SchemaValidation.ts +++ b/packages/util/src/json-schema/SchemaValidation.ts @@ -31,7 +31,9 @@ const VALID_RESULT: SchemaValidationResult = Object.freeze({ /** * Pattern for format annotations used in dataflow compatibility checking. - * Format: /\w+(:\w+)?/ where first part is the "name" and optional second part narrows the type. + * A base "name" (letter-led) optionally followed by one or more colon-separated + * narrowing segments (each may start with a digit), e.g. `model`, + * `model:EmbeddingTask`, `points:3d:meters`. * Reused from SchemaUtils.ts areFormatStringsCompatible(). */ export const FORMAT_PATTERN = /^[a-z][\w-]*(?::[a-z0-9][\w-]*)*$/i; @@ -122,6 +124,12 @@ function collectJsonSchemaErrors( return; } + // Arrays are `typeof === "object"` but are not valid schema objects. + if (Array.isArray(schema)) { + errors.push({ path, message: `Expected schema object, got array` }); + return; + } + if (schema.type !== undefined) { if (typeof schema.type === "string") { if (!VALID_JSON_SCHEMA_TYPES.has(schema.type)) { @@ -179,8 +187,11 @@ function collectJsonSchemaErrors( /** * Validates that all `format` annotations in a schema match the expected pattern. * - * Format annotations use the pattern `/^[a-zA-Z][a-zA-Z0-9_-]*(:[a-zA-Z][a-zA-Z0-9_-]*)?$/` - * (e.g., `"model"`, `"model:EmbeddingTask"`, `"storage:tabular"`). + * Format annotations are validated against {@link FORMAT_PATTERN} + * (`/^[a-z][\w-]*(?::[a-z0-9][\w-]*)*$/i`): a letter-led base name optionally + * followed by one or more colon-separated narrowing segments, where a narrowing + * segment may begin with a digit (e.g., `"model"`, `"model:EmbeddingTask"`, + * `"storage:tabular"`, `"points:3d:meters"`). * * Standard JSON Schema formats (e.g., `"date-time"`, `"uri"`, `"email"`) also pass * since they match the pattern. diff --git a/packages/util/src/json-schema/parsePartialJson.ts b/packages/util/src/json-schema/parsePartialJson.ts index 7dcdfe362..0521191d3 100644 --- a/packages/util/src/json-schema/parsePartialJson.ts +++ b/packages/util/src/json-schema/parsePartialJson.ts @@ -197,9 +197,12 @@ function cleanTrailing(text: string): string { } // Trailing incomplete value after a colon (e.g., `"key": tru` or `"key": 12`) - // Check if there's an incomplete bare token at the end + // Check if there's an incomplete bare token at the end. The pair may be + // preceded by a comma (subsequent property) OR an opening brace (the very + // first property of an object) — both must be detected so the first + // streamed property degrades to a partial `{}` instead of failing entirely. const bareTokenMatch = trimmed.match( - /,\s*"[^"]*"\s*:\s*(?:tru|fal|nul|true|false|null|[\d.eE+-]+)$/ + /(,|\{)\s*"[^"]*"\s*:\s*(?:tru|fal|nul|true|false|null|[\d.eE+-]+)$/ ); if (bareTokenMatch) { // Check if the bare value is complete @@ -208,9 +211,13 @@ function cleanTrailing(text: string): string { JSON.parse(valueStr); // Value is complete, keep it } catch { - // Value is incomplete, remove the whole key-value pair - s = trimmed.slice(0, bareTokenMatch.index!).trimEnd(); - if (s.endsWith(",")) s = s.slice(0, -1); + // Value is incomplete, remove the whole key-value pair. Keep the leading + // boundary character (comma or brace) so the surrounding structure + // (and the closeStack pass) can still close the object/array correctly. + const boundary = bareTokenMatch[1]; + const cut = trimmed.slice(0, bareTokenMatch.index!).trimEnd(); + s = boundary === "{" ? `${cut}{` : cut; + if (boundary !== "{" && s.endsWith(",")) s = s.slice(0, -1); changed = true; continue; } diff --git a/packages/util/src/resource/ResourceScope.ts b/packages/util/src/resource/ResourceScope.ts index 6cfa89789..6633a17e9 100644 --- a/packages/util/src/resource/ResourceScope.ts +++ b/packages/util/src/resource/ResourceScope.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { getLogger } from "../logging"; import { DisposeStrategy, type IDisposeStrategy } from "./DisposeStrategy"; export interface ResourceScopeOptions { @@ -64,13 +65,23 @@ export class ResourceScope { /** * Call all disposers via Promise.allSettled (best-effort), then clear - * (escape hatch — bypasses the strategy). Individual disposer errors are - * silently swallowed. + * (escape hatch — bypasses the strategy). This intentionally does NOT throw, + * so it is safe in teardown paths (`[Symbol.asyncDispose]`, `await using`) + * where one failing disposer must not prevent the rest from running. Failures + * are no longer silent: each rejected disposer is logged so cleanup failures + * (e.g. a connection that errors on close) are visible to operators. */ async disposeAll(): Promise { const fns = [...this.disposers.values()]; this.disposers.clear(); - await Promise.allSettled(fns.map((fn) => fn())); + const results = await Promise.allSettled(fns.map((fn) => fn())); + for (const result of results) { + if (result.status === "rejected") { + getLogger().warn("ResourceScope.disposeAll: a disposer failed", { + error: result.reason, + }); + } + } } /** diff --git a/packages/util/src/utilities/BaseError.ts b/packages/util/src/utilities/BaseError.ts index 399f50fa3..a25c83cc0 100644 --- a/packages/util/src/utilities/BaseError.ts +++ b/packages/util/src/utilities/BaseError.ts @@ -4,34 +4,47 @@ * SPDX-License-Identifier: Apache-2.0 */ -export class BaseError { +/** + * Base class for all library errors. Extends the native {@link Error} so that + * the near-universal `err instanceof Error` idiom (and the diagnostics / + * telemetry utilities built on it) correctly classifies every domain error + * derived from this class. + * + * Subclasses set their human-readable `name` from the static `type` property + * (falling back to the runtime constructor name). An optional `cause` may be + * supplied via the second constructor argument and is propagated to the native + * `Error` cause chain. + */ +export class BaseError extends Error { public static type: string = "BaseError"; - public message: string; - public name: string; - public stack?: string; - constructor(message: string = "") { - this.message = message; - const constructor = this.constructor as any; - this.name = constructor.type ?? this.constructor.name; + constructor(message: string = "", options?: { cause?: unknown }) { + super(message, options); + + const constructor = this.constructor as typeof BaseError; + // Use the subclass's OWN `static type` when it declares one; otherwise fall + // back to the runtime constructor name. (`BaseError.type` is inherited, so a + // plain `constructor.type` would mask every subclass that forgets to set it.) + this.name = Object.prototype.hasOwnProperty.call(constructor, "type") + ? constructor.type + : this.constructor.name; + + // Some runtimes (or super() in older targets) do not forward `cause`. + if (options && "cause" in options && this.cause === undefined) { + this.cause = options.cause; + } + + // Restore the prototype chain so `instanceof` works across transpilation + // targets that break the chain when extending built-ins. + Object.setPrototypeOf(this, new.target.prototype); - // Capture stack trace if available + // Capture a clean stack trace pointing at the construction site (V8 only). if (typeof Error !== "undefined" && Error.captureStackTrace) { - const temp = { stack: "" }; - Error.captureStackTrace(temp, this.constructor); - this.stack = temp.stack; - } else { - try { - throw new Error(message); - } catch (err) { - if (err instanceof Error) { - this.stack = err.stack; - } - } + Error.captureStackTrace(this, new.target); } } - toString(): string { + override toString(): string { return `${this.name}: ${this.message}`; } } diff --git a/packages/util/src/worker/Worker.browser.ts b/packages/util/src/worker/Worker.browser.ts index 81f11584a..4e8c999d2 100644 --- a/packages/util/src/worker/Worker.browser.ts +++ b/packages/util/src/worker/Worker.browser.ts @@ -3,10 +3,11 @@ const parentPort = self; export { parentPort, Worker }; import { globalServiceRegistry } from "../di"; +import type { WorkerServerBaseOptions } from "./WorkerServerBase"; import { WORKER_SERVER, WorkerServerBase } from "./WorkerServerBase"; export { WORKER_SERVER }; export class WorkerServer extends WorkerServerBase { - constructor() { + constructor(options?: WorkerServerBaseOptions) { parentPort?.addEventListener("message", async (event) => { const msg = { type: event.type, @@ -14,7 +15,7 @@ export class WorkerServer extends WorkerServerBase { }; await this.handleMessage(msg); }); - super(); + super(options); } } diff --git a/packages/util/src/worker/Worker.bun.ts b/packages/util/src/worker/Worker.bun.ts index 81f11584a..4e8c999d2 100644 --- a/packages/util/src/worker/Worker.bun.ts +++ b/packages/util/src/worker/Worker.bun.ts @@ -3,10 +3,11 @@ const parentPort = self; export { parentPort, Worker }; import { globalServiceRegistry } from "../di"; +import type { WorkerServerBaseOptions } from "./WorkerServerBase"; import { WORKER_SERVER, WorkerServerBase } from "./WorkerServerBase"; export { WORKER_SERVER }; export class WorkerServer extends WorkerServerBase { - constructor() { + constructor(options?: WorkerServerBaseOptions) { parentPort?.addEventListener("message", async (event) => { const msg = { type: event.type, @@ -14,7 +15,7 @@ export class WorkerServer extends WorkerServerBase { }; await this.handleMessage(msg); }); - super(); + super(options); } } diff --git a/packages/util/src/worker/Worker.node.ts b/packages/util/src/worker/Worker.node.ts index 935a05991..58bec1250 100644 --- a/packages/util/src/worker/Worker.node.ts +++ b/packages/util/src/worker/Worker.node.ts @@ -24,10 +24,11 @@ const Worker = isMainThread ? WorkerPolyfill : parentPort; export { Worker, parentPort }; import { globalServiceRegistry } from "../di"; +import type { WorkerServerBaseOptions } from "./WorkerServerBase"; import { WORKER_SERVER, WorkerServerBase } from "./WorkerServerBase"; export { WORKER_SERVER }; export class WorkerServer extends WorkerServerBase { - constructor() { + constructor(options?: WorkerServerBaseOptions) { parentPort?.addEventListener("message", async (event) => { const msg = { type: event.type, @@ -36,7 +37,7 @@ export class WorkerServer extends WorkerServerBase { }; await this.handleMessage(msg); }); - super(); + super(options); } } diff --git a/packages/util/src/worker/WorkerServerBase.ts b/packages/util/src/worker/WorkerServerBase.ts index 9a5455a92..6506e5921 100644 --- a/packages/util/src/worker/WorkerServerBase.ts +++ b/packages/util/src/worker/WorkerServerBase.ts @@ -96,13 +96,11 @@ function extractTransferables(obj: any) { * All fields are optional and safe to omit — defaults match the previous * hard-coded values. * - * Reachable only via `new WorkerServerBase(...)` directly (used by tests - * that exercise eviction-path behaviour with a tiny cap). The platform- - * specific `WorkerServer` subclasses (`Worker.bun.ts`, `Worker.node.ts`, - * `Worker.browser.ts`) currently have zero-arg constructors and use the - * defaults; `new WorkerServer({ pendingAbortHardCap: ... })` will NOT - * compile against those subclasses. To override in production either - * bypass the subclass or extend it with a forwarding constructor. + * The platform-specific `WorkerServer` subclasses (`Worker.bun.ts`, + * `Worker.node.ts`, `Worker.browser.ts`) forward an optional + * {@link WorkerServerBaseOptions} argument to this base constructor, so + * `new WorkerServer({ pendingAbortHardCap: ... })` is configurable in + * production as well as in tests. */ export interface WorkerServerBaseOptions { /** @@ -111,10 +109,10 @@ export interface WorkerServerBaseOptions { * evicted in one pass — a memory safety-net for pathological abort bursts * that outrun the per-id TTL timers. Defaults to `10_000`. * - * Exposed as an option primarily as a test seam: tests that exercise the - * overflow eviction path can use a tiny cap (e.g. `100`) and insert ~110 - * entries in milliseconds instead of pushing 10,010 entries through - * vitest's fake-timer heap. + * Configurable on the platform `WorkerServer` subclasses as well; it also + * serves as a test seam, letting tests exercise the overflow eviction path + * with a tiny cap (e.g. `100`) and ~110 entries instead of pushing 10,010 + * entries through vitest's fake-timer heap. */ readonly pendingAbortHardCap?: number; } @@ -251,6 +249,18 @@ export class WorkerServerBase { }); } + /** + * Register a callable worker function. + * + * Cancellation is cooperative: an `abort` only fires the `AbortSignal` passed + * to `fn` (the 4th argument). The worker does NOT interrupt or terminate a + * running function, so `fn` MUST poll `signal.aborted` (or listen for the + * `abort` event) and return early to stop consuming CPU/GPU. A function that + * ignores the signal runs to completion even after the caller has aborted. + * + * @param name - The function name (e.g., task type identifier) + * @param fn - Async function: (input, model, postProgress, signal) => Promise + */ registerFunction(name: string, fn: (...args: any[]) => Promise) { this.functions[name] = fn; } @@ -273,6 +283,11 @@ export class WorkerServerBase { * iterates the generator and sends each yielded value as a `stream_chunk` * message, followed by a `complete` message when the generator finishes. * + * Cancellation is cooperative: an `abort` fires the `signal` but does not + * terminate the generator. The generator MUST check `signal.aborted` and + * return promptly; otherwise the underlying compute keeps running even though + * the server stops forwarding chunks once the request is marked complete. + * * @param name - The function name (e.g., task type identifier) * @param fn - Async generator function: (input, model, signal) => AsyncIterable */ @@ -285,6 +300,12 @@ export class WorkerServerBase { * receives an `emit` callback that posts events as `stream_chunk` messages on * the worker port and resolves with no value when complete. Use this in place * of {@link registerStreamFunction} for newly-ported provider run-fns. + * + * Cancellation is cooperative: an `abort` fires the supplied `signal` but the + * worker neither interrupts nor terminates the run-fn. The run-fn MUST honor + * `signal.aborted` (e.g. stop generating tokens / abort the network request) + * to actually release CPU/GPU; ignoring it lets the work run to completion + * after the caller has moved on. */ registerRunFunction( name: string, @@ -320,6 +341,15 @@ export class WorkerServerBase { } } + /** + * Handle an `abort` message for a request id. This only fires the request's + * {@link AbortController} (so a cooperative fn can observe `signal.aborted`) + * and posts the terminal "Operation aborted" error to the caller. It does NOT + * interrupt or terminate an in-flight function: non-cooperative fns that + * ignore the signal continue running to completion. If the abort beats the + * call to the worker, the id is recorded as a pending abort so the imminent + * handler aborts its controller as soon as it is constructed. + */ async handleAbort(id: string) { if (this.requestControllers.has(id)) { const controller = this.requestControllers.get(id); @@ -571,16 +601,24 @@ export class WorkerServerBase { } /** - * Schedule cleanup of a completed request ID. Uses a 5-second delay to - * handle late-arriving abort messages, and caps the completed set size at - * {@link COMPLETED_REQUESTS_HARD_CAP} entries to prevent unbounded growth. As in - * {@link recordPendingAbort}, the eviction list is snapshotted via - * `Array.from` before deletion to avoid iterating-while-deleting. + * Schedule cleanup of a completed request ID. The delay matches + * {@link PENDING_ABORT_TTL_MS} so a completed/aborted id stays in + * {@link completedRequests} at least as long as a pending-abort marker can + * live. Otherwise a `call` that arrives between the old 5s cleanup and the + * 30s abort TTL would be aborted by `consumePendingAbort` yet still post a + * SECOND terminal response (the dedupe guard in {@link postResult} / + * {@link postError} had already forgotten the id) — violating the + * "at most one terminal message per requestId" wire invariant. + * + * Caps the completed set size at {@link COMPLETED_REQUESTS_HARD_CAP} entries + * to prevent unbounded growth. As in {@link recordPendingAbort}, the eviction + * list is snapshotted via `Array.from` before deletion to avoid + * iterating-while-deleting. */ private scheduleCompletedRequestCleanup(id: string): void { setTimeout(() => { this.completedRequests.delete(id); - }, 5000); + }, WorkerServerBase.PENDING_ABORT_TTL_MS); // Safety cap: if the set grows too large, clear the oldest entries (FIFO). if (this.completedRequests.size > COMPLETED_REQUESTS_HARD_CAP) { diff --git a/providers/postgres/src/storage/PostgresTabularStorage.ts b/providers/postgres/src/storage/PostgresTabularStorage.ts index 7f268eff9..7e0f35c78 100644 --- a/providers/postgres/src/storage/PostgresTabularStorage.ts +++ b/providers/postgres/src/storage/PostgresTabularStorage.ts @@ -23,6 +23,7 @@ import { pickCoveringIndex, PostgresDialect, QueryOptions, + safeEmit, SearchCriteria, SimplifyPrimaryKey, SqlTabularMigrationApplier, @@ -770,7 +771,7 @@ export class PostgresTabularStorage< * rows that are about to roll back. */ protected emitPut(entity: Entity): void { - this.events.emit("put", entity); + safeEmit(this.events, "put", entity); } /** @@ -852,6 +853,10 @@ export class PostgresTabularStorage< } catch { // prefer the original error if rollback fails } + // The transaction is all-or-nothing: on ROLLBACK no row persisted and + // no `put` event fired (those are deferred to after COMMIT below), so + // `ids` is empty. + safeEmit(this.events, "rollback", { op: "putBulk", error: err, ids: [] }); throw err; } } finally { @@ -1001,7 +1006,7 @@ export class PostgresTabularStorage< throw err; } // Flush deferred events only on commit success. - for (const entity of deferredPutEvents) this.events.emit("put", entity); + for (const entity of deferredPutEvents) safeEmit(this.events, "put", entity); return result; } @@ -1037,7 +1042,7 @@ export class PostgresTabularStorage< } else { val = undefined; } - this.events.emit("get", key, val); + safeEmit(this.events, "get", key, val); return val; } @@ -1066,7 +1071,7 @@ export class PostgresTabularStorage< rows.push(...chunkRows); } } - this.events.emit("getBulk", keys, rows); + safeEmit(this.events, "getBulk", keys, rows); return rows; } @@ -1126,7 +1131,7 @@ export class PostgresTabularStorage< const params = this.getPrimaryKeyAsOrderedArray(key); await db.query(`DELETE FROM "${this.table}" WHERE ${whereClauses}`, params); - this.events.emit("delete", key as Partial); + safeEmit(this.events, "delete", key as Partial); } /** @@ -1185,7 +1190,7 @@ export class PostgresTabularStorage< private async _deleteAllInternal(): Promise { const db = this.db; await db.query(`DELETE FROM "${this.table}"`); - this.events.emit("clearall"); + safeEmit(this.events, "clearall"); } /** @@ -1383,7 +1388,7 @@ export class PostgresTabularStorage< const db = this.db; const { whereClause, params } = this.buildDeleteSearchWhere(criteria); await db.query(`DELETE FROM "${this.table}" WHERE ${whereClause}`, params); - this.events.emit("delete", this.deleteIdentity(criteria)); + safeEmit(this.events, "delete", this.deleteIdentity(criteria)); } /** @@ -1435,10 +1440,10 @@ export class PostgresTabularStorage< record[k] = this.sqlToJsValue(k, record[k] as ValueOptionType); } } - this.events.emit("query", criteria as Partial, result.rows as Entity[]); + safeEmit(this.events, "query", criteria as Partial, result.rows as Entity[]); return result.rows as Entity[]; } - this.events.emit("query", criteria as Partial, undefined); + safeEmit(this.events, "query", criteria as Partial, undefined); return undefined; } diff --git a/providers/sqlite/src/storage/SqliteTabularStorage.ts b/providers/sqlite/src/storage/SqliteTabularStorage.ts index 159d0cc3c..372ec1ece 100644 --- a/providers/sqlite/src/storage/SqliteTabularStorage.ts +++ b/providers/sqlite/src/storage/SqliteTabularStorage.ts @@ -22,6 +22,7 @@ import { PageRequest, pickCoveringIndex, QueryOptions, + safeEmit, SearchCriteria, SimplifyPrimaryKey, SqliteDialect, @@ -712,7 +713,7 @@ export class SqliteTabularStorage< * that are about to roll back. */ protected emitPut(entity: Entity): void { - this.events.emit("put", entity); + safeEmit(this.events, "put", entity); } /** @@ -754,7 +755,16 @@ export class SqliteTabularStorage< updatedEntities.push(this.executePutSync(item, false)); } }); - transaction(entities); + try { + transaction(entities); + } catch (error) { + // better-sqlite3's `db.transaction(...)` is all-or-nothing: a throw + // inside the wrapped body rolls the whole transaction back, so no row + // persists. Emit `rollback` with empty `ids` so subscribers treat the + // batch as fully reverted, then rethrow the original failure. + safeEmit(this.events, "rollback", { op: "putBulk", error, ids: [] }); + throw error; + } for (const entity of updatedEntities) this.emitPut(entity); return updatedEntities; @@ -891,7 +901,7 @@ export class SqliteTabularStorage< throw err; } // Flush deferred events only on commit success. - for (const entity of deferredPutEvents) this.events.emit("put", entity); + for (const entity of deferredPutEvents) safeEmit(this.events, "put", entity); return result; } finally { this.inTransaction = false; @@ -926,10 +936,10 @@ export class SqliteTabularStorage< for (const k in this.schema.properties) { row[k] = this.sqlToJsValue(k, row[k] as ValueOptionType); } - this.events.emit("get", key, value); + safeEmit(this.events, "get", key, value); return value; } else { - this.events.emit("get", key, undefined); + safeEmit(this.events, "get", key, undefined); return undefined; } } @@ -960,7 +970,7 @@ export class SqliteTabularStorage< rows.push(...chunkRows); } } - this.events.emit("getBulk", keys, rows); + safeEmit(this.events, "getBulk", keys, rows); return rows; } @@ -1014,7 +1024,7 @@ export class SqliteTabularStorage< const params = this.getPrimaryKeyAsOrderedArray(key); const stmt = db.prepare(`DELETE FROM \`${this.table}\` WHERE ${whereClauses}`); stmt.run(...(params as ValueOptionType[])); - this.events.emit("delete", key as Partial); + safeEmit(this.events, "delete", key as Partial); } /** @@ -1074,7 +1084,7 @@ export class SqliteTabularStorage< private async _deleteAllInternal(): Promise { const db = this.db; db.exec(`DELETE FROM \`${this.table}\``); - this.events.emit("clearall"); + safeEmit(this.events, "clearall"); } /** @@ -1254,7 +1264,7 @@ export class SqliteTabularStorage< const { whereClause, params } = this.buildDeleteSearchWhere(criteria); const stmt = db.prepare(`DELETE FROM \`${this.table}\` WHERE ${whereClause}`); stmt.run(...params); - this.events.emit("delete", this.deleteIdentity(criteria)); + safeEmit(this.events, "delete", this.deleteIdentity(criteria)); } /** @@ -1310,10 +1320,10 @@ export class SqliteTabularStorage< record[k] = this.sqlToJsValue(k, record[k] as ValueOptionType); } } - this.events.emit("query", criteria as Partial, result); + safeEmit(this.events, "query", criteria as Partial, result); return result; } - this.events.emit("query", criteria as Partial, undefined); + safeEmit(this.events, "query", criteria as Partial, undefined); return undefined; } diff --git a/providers/supabase/src/storage/SupabaseTabularStorage.ts b/providers/supabase/src/storage/SupabaseTabularStorage.ts index 7c4e001bc..ae6618217 100644 --- a/providers/supabase/src/storage/SupabaseTabularStorage.ts +++ b/providers/supabase/src/storage/SupabaseTabularStorage.ts @@ -21,6 +21,7 @@ import { PageRequest, pickCoveringIndex, QueryOptions, + safeEmit, SearchCriteria, SearchOperator, SimplifyPrimaryKey, @@ -432,7 +433,7 @@ export class SupabaseTabularStorage< if (error) throw error; const updatedEntity = this.hydrateRow(data); - this.events.emit("put", updatedEntity); + safeEmit(this.events, "put", updatedEntity); return updatedEntity; } @@ -463,7 +464,13 @@ export class SupabaseTabularStorage< .upsert(normalizedEntities, { onConflict: this.primaryKeyColumnList() }) .select(); - if (error) throw error; + if (error) { + // The upsert is a single PostgREST request whose INSERT ... ON CONFLICT + // runs inside one server-side transaction, so a failure commits no rows. + // `ids` is empty because nothing landed that a subscriber needs to undo. + safeEmit(this.events, "rollback", { op: "putBulk", error, ids: [] }); + throw error; + } if (!data) return []; const returnedRows = (data as unknown[]).map((row) => this.hydrateRow(row)); @@ -475,7 +482,7 @@ export class SupabaseTabularStorage< const orderedEntities = this.alignBulkResponseToInputOrder(normalizedEntities, returnedRows); for (const entity of orderedEntities) { - this.events.emit("put", entity); + safeEmit(this.events, "put", entity); } return orderedEntities; } @@ -540,7 +547,7 @@ export class SupabaseTabularStorage< if (error) { if (error.code === "PGRST116") { // Not found - this.events.emit("get", key, undefined); + safeEmit(this.events, "get", key, undefined); return undefined; } throw error; @@ -554,7 +561,7 @@ export class SupabaseTabularStorage< valRecord[key] = this.sqlToJsValue(key, valRecord[key] as ValueOptionType); } } - this.events.emit("get", key, val); + safeEmit(this.events, "get", key, val); return val; } @@ -578,7 +585,7 @@ export class SupabaseTabularStorage< const { error } = await query; if (error) throw error; - this.events.emit("delete", key as Partial); + safeEmit(this.events, "delete", key as Partial); } /** @@ -632,7 +639,7 @@ export class SupabaseTabularStorage< const { error } = await this.client.from(this.table).delete().neq(String(firstPkColumn), null); // Delete all rows by using a condition that's always true if (error) throw error; - this.events.emit("clearall"); + safeEmit(this.events, "clearall"); } /** @@ -795,7 +802,7 @@ export class SupabaseTabularStorage< const { error } = await query; if (error) throw error; - this.events.emit("delete", this.deleteIdentity(criteria)); + safeEmit(this.events, "delete", this.deleteIdentity(criteria)); } /** @@ -841,10 +848,10 @@ export class SupabaseTabularStorage< record[key] = this.sqlToJsValue(key, record[key] as ValueOptionType); } } - this.events.emit("query", criteria as Partial, data as Entity[]); + safeEmit(this.events, "query", criteria as Partial, data as Entity[]); return data as Entity[]; } - this.events.emit("query", criteria as Partial, undefined); + safeEmit(this.events, "query", criteria as Partial, undefined); return undefined; }