Fix concurrency and event emission across storage and task systems - #686
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
|
Code review (xhigh, --fix) — 10 finder angles + 5 adversarial verifiers. Fixes pushed in fc3c29a. Bugs confirmed and fixed:
Quality: the six identical Investigated and cleared: WhileTask does not have ReduceTask's x-stream leak (it overrides Verified: Generated by Claude Code |
…uard - Replace two raw NUL bytes in BaseTabularStorage.ts join separators with the backslash-u0000 escape so grep/ripgrep stop classifying the file as binary. - Route IndexedDbTabularStorage's eight remaining raw event emits through safeEmit: a throwing subscriber inside an IDB onsuccess callback unwound before resolve(), leaving the put/get/delete promise unsettled forever. - Emit the similaritySearch event declared on IVectorStorage from the Sqlite, SqliteAi, Postgres, and Supabase vector backends (previously only InMemory and IndexedDb fired it). - Give DuckDbTabularStorage the cross-instance connection guard SQLite and Postgres already have: connectionHandle() for a shared DuckDbDatabase and runOnConnection/runInTransactionOnConnection around writes and withTransaction, so a sibling instance's write can no longer interleave inside another instance's BEGIN/COMMIT. Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
…vents Every core queue storage's add() unconditionally stamped visible_at = now, clobbering the future timestamp the client computed for delaySeconds — a delayed send ran immediately on all five backends. Keep a caller-provided visible_at and assert the deferred visibility in the generic suite. Also forward errorCode on the client's job_error event; the server carries it and handleJobError received it, but the re-emit dropped it. Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
… wire dependency-JSON edges - TaskGraph.run() hand-copied nine config fields into runGraph, silently dropping enforceEntitlements and matchAllEmptyInputs; spread the config so every field forwards (runPreview already did). - A ReduceTask whose body ended in a streaming task copied the x-stream annotation onto its aggregate output schema, routed the run through IteratorTask.executeStream, and returned the input untouched without running an iteration. Strip x-stream from copied ports and make the iterator's executeStream throw a TaskConfigurationError instead of silently yielding its input. - createGraphFromDependencyJSON never read item.dependencies; JsonTask re-wired them at the top level, but graphs recursed through subtasks lost their edges silently (and toDependencyJSON did not round-trip). Wire the dataflows inside the factory and drop JsonTask's duplicate loop. - Document ABORTING as the real post-abort status in EXECUTION_MODEL.md (the ABORTED status it described does not exist). Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
AiChatWithKbTask duplicated AiChatTask's multi-turn loop but its copy never called gateOrThrow (violating the contract that both execute paths gate) and hand-rolled getJobInput without timeoutMs, so its provider calls always fell back to the 60-minute default. Gate in executeStream and delegate getJobInput to the base so timeoutMs, outputSchema, and future base fields stay populated. Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
The one-shot worker call path scrubbed absolute filesystem paths out of rehydrated error stacks, but the stream and run-fn paths rebuilt errors without scrubbing — the exact leak the scrubbing was added to stop. Extract rehydrateWorkerError() next to scrubStack and use it on all three paths. Also pair the ./worker export's default types with worker-node.d.ts; it pointed at worker-entry.d.ts, hiding the Worker.node exports that the worker-node.js runtime actually ships. Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
…ntitlement parity - Vector task and DateFormatTask input validation threw bare Error; use TaskInvalidInputError so the runner classifies the failures as non-retryable input errors like the scalar tasks do. - JavaScriptTask was never registered in TaskRegistry, so a serialized graph containing one could not rehydrate; export registerJavaScriptTasks(). The package also declared sideEffects: false while relying on a module-level Workflow.prototype patch — switch to an allowlist so bundlers keep the registration side effect. - McpListTask's HTTP entitlements omitted the optional CREDENTIAL entitlement its three siblings declare for the same auth path. Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
Every source file carries the Apache-2.0 SPDX header, but none of the 39 package.json files declared a license field, so npm listed the published packages as unlicensed. Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
client.getJob returns a Job instance with camelCase Date fields, not the raw storage row — the deferred-visibility assertion parsed snake_case string fields and compared NaN. Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
The ./worker export's browser, bun, and (as of the previous commit) default types conditions point at worker-browser.d.ts, worker-bun.d.ts, and worker-node.d.ts — but the tsconfig files list only type-built worker-entry.ts, so none of those declaration files were ever emitted and consumers of @workglow/util/worker failed to resolve its types. Add the three platform entries to the files list so the declarations the exports map names actually exist. Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
…d ai - TaskGraph.run: preserve an explicit outputCache:false (?? instead of ||) so per-run cache disabling reaches the runner; add coverage - DuckDbTabularStorage.guardedWrite: consult the connection chain even while inTransaction — a write deferred on the instance mutex could wake after COMMIT and interleave into a sibling instance's next transaction; add a deterministic scripted-stub regression test - IndexedDbQueueStorage.add: keep a caller-supplied fingerprint instead of always recomputing (parity with the other four backends); pin with a generic-suite test - genericJobQueueTests: assert delayed-send visibility against the client clock bracket [t0+200, t1+200] instead of created_at minus 50ms slack - WorkerManager: log the rehydrated (stack-scrubbed) worker error at both raw-payload log sites - AiChatWithKbTask.getJobInput: set sessionId after delegating to the base so it stays a top-level job field, out of the serialized taskInput - vector storages: extract the six identical emitSimilaritySearch blocks into one shared helper in @workglow/storage typed against VectorEventListeners - examples/web: drop the redundant dependency re-wiring now that createGraphFromDependencyJSON wires dataflows itself - TaskJSON: add tests for dependency wiring (top-level, nested subtasks, out-of-scope id error, toDependencyJSON round-trip) - util tsconfig: revert include/exclude formatting churn Claude-Session: https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm
fc3c29a to
bb3a081
Compare
Rebase onto current main hit conflicts in files also touched by #684 (queue-adapter deletions), #685/#686 (Usage seam, TaskInvalidInputError), and #641 (AiSessionContext). Per this PR's own conflict-resolution guidance: took main's side on every conflict, then re-ran `bun run format` to reapply the type-import conversion the autofix commit originally made to those files.
…e autofix (#683) * chore(eslint): enforce consistent-type-imports Adds @typescript-eslint/consistent-type-imports so type-only imports are written as `import type`. `disallowTypeAnnotations` is left off because inline `import()` type annotations are the established way optional peer dependencies are typed here without a static import. Claude-Session: https://claude.ai/code/session_01PwyJuFrJnibKvrrk8Fa4Fn * chore: apply consistent-type-imports autofix Mechanical output of `bun run format` (eslint --fix + prettier) after enabling @typescript-eslint/consistent-type-imports. No hand edits. Claude-Session: https://claude.ai/code/session_01PwyJuFrJnibKvrrk8Fa4Fn * chore: convert the remaining inline type specifiers to top-level import type Claude-Session: https://claude.ai/code/session_01PwyJuFrJnibKvrrk8Fa4Fn * chore: reapply consistent-type-imports autofix after rebase conflicts Rebase onto current main hit conflicts in files also touched by #684 (queue-adapter deletions), #685/#686 (Usage seam, TaskInvalidInputError), and #641 (AiSessionContext). Per this PR's own conflict-resolution guidance: took main's side on every conflict, then re-ran `bun run format` to reapply the type-import conversion the autofix commit originally made to those files. --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
This PR addresses several concurrency, event emission, and error handling issues across the storage, task-graph, and worker systems. Key fixes include proper cross-instance connection guarding in DuckDB storage, vector similarity search event emission, task graph dependency validation, and worker error rehydration.
Key Changes
Storage & Concurrency
connectionHandle()andguardedWrite()methods to properly serialize writes across multiple storage instances sharing the same database connection. This prevents interleaving ofBEGIN/COMMITblocks when sibling instances callputBulk()concurrently.withTransaction()to userunInTransactionOnConnection()for proper cross-instance transaction coordination.Vector Storage Events
emitSimilaritySearch()helper method to emit thesimilaritySearchevent on all vector search results (previously only the InMemory and IndexedDb backends emitted the event declared onIVectorStorage). The inheritedeventsemitter is widened to carry vector-specific events.Task Graph & Dependencies
run()now spreads the full run config instead of hand-copying a field whitelist —enforceEntitlementsandmatchAllEmptyInputswere silently dropped before.Dataflowcreation fromJsonTaskintocreateGraphFromDependencyJSON()so that all graph construction (including recursedsubtasks) properly wires dataflows and validates that dependency IDs exist in the graph.Task Execution & Validation
executeStream()to throw immediately if called (instead of yielding its input as the finish payload), since subclass output schemas excludex-streamand runs should never route here. Failing loudly prevents silent wrong results.x-streamannotation from child task output schemas when building the aggregate schema, ensuring the reduce output is never treated as a live stream (previously a reduce whose body ended in a streaming task returned its input without running an iteration).getJobInput()to base class to ensuretimeoutMs,outputSchema, and future base fields are always populated. Added explicitgateOrThrow()call inexecuteStream()since the override doesn't callsuper.executeStream().Errorthrows toTaskInvalidInputErrorfor proper error classification.TaskInvalidInputErrorfor consistent error classification.Worker Error Handling
rehydrateWorkerError()function inscrubStack.ts. All three error message handlers now use this function for consistent stack scrubbing and error reconstruction — previously the stream and run-fn paths skipped the stack scrubbing the call path applied, leaking absolute container paths.rehydrateWorkerError()function that rebuilds anErrorfrom worker message payloads with defense-in-depth stack scrubbing.Event Emission Safety
this.events.emit()calls withsafeEmit()— a throwing subscriber inside an IDBonsuccesscallback previously unwound beforeresolve(), leavingput()'s promise permanently unsettled.Job Queue
add()no longer clobbers a caller-set futurevisible_at, sosend(..., { delaySeconds })actually delays delivery instead of being a silent no-op.job_errorclient event now forwards theerrorCodethe server already carries.Package Configuration
sideEffectsto["./dist/task.js"](wasfalse) to preserve task registration side effects, and addedregisterJavaScriptTasks()so serialized graphs containing aJavaScriptTasknode can be rehydrated.McpListTasknow declares the optionalCREDENTIALentitlement on the HTTP branch, matching its three siblings../workerexport's default condition now pairsworker-node.d.tswithworker-node.js(types previously pointed atworker-entry.d.ts, hiding theWorker.nodeexports from TypeScript), and the tsconfig now emits theworker-node/browser/bun.d.tsdeclarations the exports map names — the browser/bun types conditions previously pointed at files that were never built."license": "Apache-2.0"field to package.json files for compliance.Documentation
ABORTINGstate transitions toFAILEDwhen the abort surfaces as an error.https://claude.ai/code/session_01TQFDxK7AKh4w4DCjRJUpQm