feat(sdks): taskito executor subcommand for Python, Node and Java - #595
Merged
kartikeya-27 merged 46 commits intoAug 1, 2026
Merged
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds a shared scheduler-attachment executor to the Rust core and exposes it through Node.js, Python, and Java SDKs. It adds detached storage and cancellation handling, lifecycle controls, CLIs, Java handler discovery, protocol conversion, and end-to-end tests. ChangesCore executor attachment
Node.js executor
Python executor
Java executor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pratyush618
added
deployment
Contributor
Author
stromanni
force-pushed
the
feat/546-executor-attach-sdks
branch
2 times, most recently
from
August 1, 2026 04:16
9c5d725 to
7cdfec9
Compare
The attach protocol only had scheduler-side conversions. An executor needs the other direction: a Job rebuilt from a dispatch frame, and a result frame built from a JobResult.
ExecutorClient is Worker::spawn with storage swapped for a socket: it dials a scheduler, then feeds the same WorkerDispatcher every SDK already implements. Needs no Storage, so an executor image carries app code and no database credentials. Draining announces zero capacity by heartbeat before disconnecting, which the scheduler already honours, so in-flight work reports instead of waiting for a reap.
Every SDK executor needs to turn TASKITO_ATTACH into a connection, and the grammar has to match what the listener binds. One parser here rather than three in three languages, where unix: support would drift.
A worker reads the storage cancel flag; an attached executor has no storage and learns from the scheduler's cancel frame instead. Both answer one question, so both ask it in one place.
Slots are prefork children, so the timeout watchdog, least-loaded dispatch and restart-on-crash all come from shipped code and child.py is untouched. start/wait/stop rather than one blocking run: a Python signal handler only runs when the main thread holds the GIL, so a blocking call would make the process deaf to SIGTERM.
Reads TASKITO_ATTACH and TASKITO_SLOTS, the contract shared with the other SDKs. The token is env-only: in argv it would show up in ps and shell history.
A socket speaking the frame protocol stands in for the scheduler so these run without a Rust build; the same assertions run against the real taskito-server when TASKITO_SERVER_BIN points at one.
ExecutorHandle::wait consumes the handle, which a shell that must also hold it to shut down cannot do. A cloneable session view lets an async runtime watch for the scheduler ending the session.
The executor runs the same task bodies as the worker, so the middleware, codec and resource-scope handling is shared rather than copied.
run and executor both resolve a Queue from a module path.
Attaching is async: dialling and the handshake block, so on the JS thread they would freeze the event loop until the scheduler answers. The dispatcher now takes its cancels from the shared source, so a detached executor can learn of one without reading storage.
The inverse of runWorker: the scheduler holds the database connection and dispatches over a socket. Same middleware, codecs and resources — only the transport differs.
Reads TASKITO_ATTACH and TASKITO_SLOTS, the contract shared with the other SDKs. The token is env-only: in argv it would show up in ps.
A socket speaking the frame protocol stands in for the scheduler so these run without a Rust build; the same assertions run against the real taskito-server when TASKITO_SERVER_BIN points at one.
stop() cannot unpark the frame reader, which is blocked on a read only the scheduler could satisfy. A shell that stopped and then waited would hang instead of draining.
The processor now generates a ServiceLoader provider per annotated class and lists it in META-INF/services, so an executor finds handlers with no user main. ServiceLoader cannot load HandlerRegistry itself: on the classpath a listed class must be a subtype with a public no-arg constructor, and the static provider() form only applies to modules. Hence the new interface. Classes with no accessible no-arg constructor are skipped with a note — only their own code knows how to build them.
pratyush618
force-pushed
the
feat/546-executor-attach-sdks
branch
from
August 1, 2026 05:20
7cdfec9 to
39126be
Compare
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (14)
sdks/node/test/worker/executorAttach.test.ts (1)
467-469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead environment assignment.
process.env.TASKITO_ATTACH = undefinedstores the string"undefined", because Node coerces environment values to strings. Line 469 deletes the key immediately, so the assignment has no effect and misstates the intent.🧹 Proposed fix
const previous = process.env.TASKITO_ATTACH; - process.env.TASKITO_ATTACH = undefined; delete process.env.TASKITO_ATTACH;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/test/worker/executorAttach.test.ts` around lines 467 - 469, Remove the redundant process.env.TASKITO_ATTACH = undefined assignment in the test setup, leaving delete process.env.TASKITO_ATTACH as the sole operation that clears the environment variable while preserving the previous value capture.sdks/node/src/queue.ts (1)
1665-1670: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching the disable lookup instead of reading it per job.
middlewareForruns once per dispatched job, anddisables.getFor(taskName)performs a settings read each time. On a storage-backed queue handle that is a synchronous storage hit on the job path. A small per-task cache, or a snapshot taken at attach time, removes it.Note that the executor path normally runs detached, where the read is a no-op, so this only matters for a storage-backed handle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/src/queue.ts` around lines 1665 - 1670, Update the middlewareFor callback to cache the result of disables.getFor(taskName) per task name, avoiding repeated settings reads for dispatched jobs while preserving the existing filtering behavior for disabled and enabled task middleware.sdks/node/src/executor.ts (1)
176-187: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueValidate
run.slotsthe same way as the environment value.
envIntrejects a non-positive or non-integerTASKITO_SLOTS. A programmaticslotsvalue bypasses that check and crosses to native unvalidated. Reject it here so both paths report the same error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/src/executor.ts` around lines 176 - 187, Validate the programmatic run.slots value with the same positive-integer rules implemented by envInt before passing it to native code. Reject non-integer or values below 1 with the same error behavior and message format, while preserving handling for omitted slots.sdks/node/test/worker/executorAttachServer.test.ts (1)
38-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce two flakiness sources in the harness.
freePortcloses the probe socket before the server binds, so another process can take the port in that window. The failure then looks like a server startup failure.
await sleep(1000)also encodes a fixed startup delay. A slow machine can enqueue before the scheduler is ready.waitForalready exists; poll for the attached executor instead, for example throughqueue.listWorkers().Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/test/worker/executorAttachServer.test.ts` around lines 38 - 48, Update freePort and the executor startup test to remove both race conditions: reserve/select a port without closing the probe before the server binds, and replace the fixed await sleep(1000) delay with waitFor polling that uses queue.listWorkers() to confirm the attached executor is ready before enqueueing work.sdks/node/src/detached.ts (1)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the warning label with the method name.
updateProgresswarns as"setProgress"andwriteTaskLogwarns as"publish". The public API names are helpful, butwriteTaskLogalso backsjob.log(...), not onlypublish. A user who callsjob.log(...)sees a warning aboutpublish.Consider a label that covers both, for example
"log/publish".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/src/detached.ts` around lines 57 - 62, Update the warning labels in updateProgress and writeTaskLog to reflect their public API methods; use the method name for updateProgress and a shared label covering both log and publish for writeTaskLog, such as “log/publish”.sdks/python/tests/worker/executor_apps/attach_app.py (1)
22-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixed default database path in the fixture app is written by the test process.
attach_app.pyfalls back to a predictable/tmpfile whenTASKITO_EXECUTOR_TEST_DBis absent, and the test interpreter imports the same module without that variable set, so the suite creates a shared database outsidetmp_path.
sdks/python/tests/worker/executor_apps/attach_app.py#L22-L28: replace the/tmp/taskito-executor.dbdefault with atempfile.gettempdir()-based per-run name, and derivedb_urlfrom the backend value rather than from the presence ofTASKITO_EXECUTOR_TEST_BACKEND.sdks/python/tests/worker/test_executor_attach.py#L241-L257: setTASKITO_EXECUTOR_TEST_DBto atmp_pathvalue in the test process beforeattach_appis imported bypayload_forand the_app_importablefixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/tests/worker/executor_apps/attach_app.py` around lines 22 - 28, Update sdks/python/tests/worker/executor_apps/attach_app.py lines 22-28 to use a tempfile.gettempdir()-based per-run database filename instead of the fixed /tmp fallback, and derive db_url from the resolved backend value. Update sdks/python/tests/worker/test_executor_attach.py lines 241-257 to set TASKITO_EXECUTOR_TEST_DB to a tmp_path-based value in the test process before payload_for and _app_importable import attach_app.Source: Linters/SAST tools
crates/taskito-python/src/executor.rs (1)
142-148: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
waitholds the handle lock while it blocks.
with_handlekeeps theMutexguard for the whole closure, so the guard is held across the blockingwait_timeout. A concurrentstop(),is_running(), orshutdown()call from another thread blocks until the timeout expires. The CLI polls with 200 ms, so the delay is bounded, and the signal handler runs on the main thread afterwaitreturns. Consider cloning the needed handle state (or wrapping it in anArc) so lifecycle calls never wait on a network timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-python/src/executor.rs` around lines 142 - 148, Update wait so with_handle does not retain the mutex guard while blocking in handle.wait_timeout. Clone or otherwise share the required handle state before entering the timeout wait, then release the lock so stop, is_running, and shutdown can proceed immediately; preserve the existing timeout result and unwrap_or(true) behavior.sdks/python/taskito/cli.py (1)
342-406: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueInstall the signal handlers before work can arrive.
The scheduler can dispatch jobs as soon as
_Executor(...)completes the handshake. TheSIGTERMandSIGINThandlers are installed several statements later. In that window aSIGTERMterminates the process with the default action, so in-flight jobs are stranded for the reaper instead of drained. Installrequest_stopbefore the attach, or move theAlso consider wrapping the poll loop in
try/finallysoexecutor.shutdown()still runs if the loop raises.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/taskito/cli.py` around lines 342 - 406, Register SIGTERM and SIGINT handlers in run_executor before constructing _Executor, with request_stop safely stopping the attached executor once available; this must cover the handshake window where jobs may arrive. Keep the existing signal behavior and move the attachment status print after handler registration. Wrap the polling and shutdown flow in try/finally so executor.shutdown() always runs when the loop or wait raises.sdks/python/tests/worker/test_executor_attach.py (1)
129-136: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe drain heartbeat assertion also matches a normal busy heartbeat.
next_heartbeat(free_slots=0)accepts any heartbeat that reports zero free slots. Withslots=1and one running job, the executor already reportsfree_slots == 0before theSIGTERM. The test can therefore pass on a pre-signal heartbeat and prove nothing about the drain announcement. Assert on a drain-specific field if the frame carries one, or read heartbeats until one arrives after the signal was sent and assert the field the drain sets.Also applies to: 369-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/tests/worker/test_executor_attach.py` around lines 129 - 136, The next_heartbeat helper currently accepts a normal busy heartbeat when free_slots is zero, so the drain assertion can occur before SIGTERM takes effect. Update the drain test flow around next_heartbeat and the SIGTERM send to consume or distinguish pre-signal heartbeats, then assert the drain-specific heartbeat field or otherwise require a heartbeat emitted after the signal.sdks/python/tests/worker/test_executor_attach_server.py (1)
135-137: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed
time.sleep(1.0)attach waits with a readiness poll.Each test waits one second for the executor to attach before it enqueues. Under a loaded CI runner the attach can take longer, and the enqueued job is then dispatched late or the assertion times out. Poll for a deterministic signal instead, for example the executor's stdout attach line, or the presence of a registered worker in storage.
♻️ Sketch of a readiness helper
def wait_attached(process: subprocess.Popen[str], timeout: float = SETTLE) -> None: """Block until the executor prints its attach line.""" assert process.stdout is not None deadline = time.monotonic() + timeout while time.monotonic() < deadline: line = process.stdout.readline() if "attached to" in line: return if process.poll() is not None: raise AssertionError(f"the executor exited early: {read_stderr(process)}") raise AssertionError("the executor never attached")Also applies to: 148-150, 175-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/python/tests/worker/test_executor_attach_server.py` around lines 135 - 137, Replace the fixed time.sleep(1.0) waits in the affected executor attach tests with a shared readiness helper, such as wait_attached, that polls the executor process output for its attach confirmation until the existing timeout and reports early exit or timeout failures. Call this helper before enqueueing jobs in each affected test, preserving the current test flow while eliminating timing-dependent waits.sdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.java (2)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
resultsis never written, andreadFramecan block pastSETTLE_MS.Two test-hygiene points in
nextResult:
- Nothing ever adds to the
resultsdeque, so the!results.isEmpty()branch at lines 190-192 is dead. Remove the field and the branch.readFrame()blocks instream.read()with no socket timeout, so thedeadlineloop cannot enforceSETTLE_MS. When a result never arrives, the@Timeout(60)fires instead of the"no result frame arrived"assertion, which gives a less useful failure message. Set a read timeout on the accepted socket.♻️ Proposed fix
try { Socket client = server.accept(); + // Bounds every readFrame, so nextResult's deadline is real. + client.setSoTimeout((int) SETTLE_MS); socket.set(client);- private final Deque<JsonNode> results = new ArrayDeque<>();JsonNode nextResult() throws IOException { long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(SETTLE_MS); while (System.nanoTime() < deadline) { - if (!results.isEmpty()) { - return results.removeFirst(); - } JsonNode frame = readFrame();Drop the now-unused
ArrayDequeandDequeimports at lines 19-20.Also applies to: 186-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.java` at line 65, Remove the unused results deque field and its ArrayDeque/Deque imports, then delete the dead results branch from nextResult. Configure a read timeout on the accepted socket used by readFrame so its deadline loop can enforce SETTLE_MS and produce the intended no-result assertion.
229-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
discover()and for acancelframe.The suite covers explicit
register(...)only. Two gaps remain against the PR objectives:
- Classpath discovery through
META-INF/servicesis the headline Java objective, and no test exercisesExecutor.Builder.discover(ClassLoader)or the generatedProvider. A test can pass aClassLoaderthat serves a syntheticMETA-INF/services/org.byteveda.taskito.worker.HandlerRegistryProviderresource, so it needs no annotation-processor run.- No test sends a
cancelframe.JavaDispatcher::notify_canceland the failure-to-cancelled reclassification atcrates/taskito-java/src/dispatcher.rsline 162 are therefore untested from Java.I can draft both tests. Do you want me to open an issue to track them?
Also applies to: 349-356
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.java` around lines 229 - 247, Add tests in ExecutorAttachTest covering Executor.Builder.discover(ClassLoader) with a synthetic META-INF/services/org.byteveda.taskito.worker.HandlerRegistryProvider resource and verifying the discovered handler is advertised. Also add a cancel-frame test exercising JavaDispatcher cancellation, including reclassification of a failed task as cancelled; reuse existing FakeScheduler and attachment helpers where applicable.sdks/java/src/main/java/org/byteveda/taskito/worker/Executor.java (1)
55-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
closedis not volatile, so the documented idempotence does not hold across threads.
close()reads and writesclosedwithout synchronization. Two threads can both pass the check at line 115 and both run the drain andcontrol.close().JniExecutorControl.close()is itself guarded, so a double native free cannot happen, buthandlerPool.shutdown(),resources.teardownWorker(), and theWORKER_STOPPEDevent all run twice.A shutdown hook that calls
close()while the main thread is inside try-with-resources reaches this. Make the fieldvolatileand set it with a compare-and-set, or synchronize the method.♻️ Proposed fix
- private boolean closed; + private final java.util.concurrent.atomic.AtomicBoolean closed = + new java.util.concurrent.atomic.AtomicBoolean();`@Override` public void close() { - if (closed) { + if (!closed.compareAndSet(false, true)) { return; } - closed = true; control.stop();Also applies to: 113-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/main/java/org/byteveda/taskito/worker/Executor.java` at line 55, Update the closed-state handling in Executor.close() and its closed field so shutdown is atomic across threads: make the field volatile and use a compare-and-set guard, or synchronize close(). Ensure only one caller performs handlerPool.shutdown(), resources.teardownWorker(), and the WORKER_STOPPED event while preserving idempotent subsequent close() calls.sdks/java/processor/src/main/java/org/byteveda/taskito/processor/TaskHandlerProcessor.java (1)
40-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
AGGREGATINGfor the service-file processor.
writeServiceFile()collects providers inprocessorsand writes exactly that set toMETA-INF/services/<SERVICE>duringprocessingOver(). Since the processor is not declared incremental, Gradle currently forces full annotation processing; add@IncrementalAnnotationProcessor(AGGREGATING)so the accumulating service-output contract stays explicit and later isolating incremental builds do not lose providers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/processor/src/main/java/org/byteveda/taskito/processor/TaskHandlerProcessor.java` around lines 40 - 41, Annotate TaskHandlerProcessor with `@IncrementalAnnotationProcessor`(AGGREGATING) alongside its existing processor annotations. Keep writeServiceFile() and the processors accumulation behavior unchanged so the service output continues to include all providers during processingOver().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/taskito-core/src/worker/dial.rs`:
- Around line 91-103: Update the Tcp branch of connect to iterate through every
address returned by to_socket_addrs() in order, attempting
TcpStream::connect_timeout and constructing the TcpTransport for each candidate.
Return immediately on the first successful connection, retain the last
connection or transport error, and return it after all candidates fail; preserve
the existing invalid-host and no-address errors.
In `@crates/taskito-core/src/worker/executor.rs`:
- Around line 384-412: Bound Executor::teardown so it does not unconditionally
join threads after the shutdown-drain deadline: join them only when
results_flushed is set, otherwise detach the remaining threads. In
crates/taskito-core/src/worker/executor.rs lines 384-412, preserve the existing
drain warning and shutdown behavior. In
crates/taskito-core/src/worker/executor_tests.rs lines 908-930, replace
Behaviour::Block with a behavior that parks indefinitely and assert that
shutdown() returns within a multiple of shutdown_drain.
- Around line 468-489: Update ExecutorHandle::begin_drain so local shutdown
state is applied before the potentially blocking Heartbeat send: drop the job
sender and set session_over before calling self.send. Preserve the existing
idempotence guard and heartbeat announcement, while ensuring stop’s documented
non-waiting behavior is maintained even when the scheduler is no longer reading.
In `@crates/taskito-python/src/lib.rs`:
- Line 5: Gate the prefork module declaration in lib.rs and the PreforkPool
construction in executor::PyExecutor with the same supported-target or feature
condition. Ensure unsupported targets such as Windows avoid both the module and
instantiation while preserving the existing prefork behavior on supported
targets.
In
`@sdks/java/processor/src/main/java/org/byteveda/taskito/processor/TaskHandlerProcessor.java`:
- Around line 93-96: Update the Javadoc above the generated companion listing to
reference HandlerRegistryProvider instead of HandlerRegistry, matching the
service type loaded by Executor.Builder.discover(ClassLoader).
- Around line 128-132: Update isServiceLoadable() to reject every nested
TypeElement, including static nested classes, before ServiceLoader generation;
retain the existing abstract-class rejection. Ensure generate() only creates
providers for package-level owners so Provider.registry() cannot reference an
invalid simple nested type.
In `@sdks/java/src/main/java/org/byteveda/taskito/cli/Cli.java`:
- Around line 328-343: Update Cli.resolveSlots and its call path to reject
parsed TASKITO_SLOTS values less than one, and throw ParameterException using
the injected CommandSpec’s commandLine() rather than a new CommandLine instance.
Resolve the slot count once in call(), then reuse that value for both the
Executor.Builder configuration and the banner so they remain consistent.
- Around line 309-323: Update the shutdown-hook setup in Cli.main to use a
shared flag indicating whether the session is still active, and have the hook
return without calling executor.stop() or joining main once the normal path has
completed. Clear the flag and remove the registered hook before leaving the
try-with-resources block, while preserving stop-and-join behavior for
signal-triggered shutdowns.
In
`@sdks/java/src/main/java/org/byteveda/taskito/internal/JniExecutorControl.java`:
- Around line 88-100: Update awaitSession to acquire the read lock only long
enough to validate that the handle is open and capture the handle, then release
it before calling NativeExecutor.awaitSession. Do not wrap the blocking native
wait in withOpenHandle; preserve closed-state checking while allowing stop() and
close() to acquire their respective locks.
In `@sdks/java/src/main/java/org/byteveda/taskito/worker/Executor.java`:
- Around line 302-315: Extend the try/catch in the Executor setup flow to
include JniExecutorControl creation, bridge.bind, emitter.emit,
resources.acquireWorker, and Executor construction after NativeExecutor.attach
succeeds. If any post-attach operation throws, call NativeExecutor.close(handle)
before rethrowing, while preserving pool shutdown and successful setup behavior.
In `@sdks/node/src/cli/commands/executor.ts`:
- Around line 52-57: Update the signal handler around executor.stop() to catch
rejected promises, report the stop failure, and exit with an intentional
non-zero status; preserve the successful path’s clean exit(0) behavior for both
SIGINT and SIGTERM handlers.
In `@sdks/node/src/executor.ts`:
- Around line 114-120: Update the post-attach setup in the start flow around
resources.acquireWorker() and emitter.emit("worker.started", ...) so any thrown
error triggers cleanup of the successfully attached native executor before
propagating the error. Ensure the attached session is released or stopped and
the original rejection is preserved, while keeping normal Executor creation
unchanged.
In `@sdks/node/src/queue.ts`:
- Around line 1659-1675: Update Queue.runExecutor and Queue.shutdown so
executors created by runExecutor are tracked alongside liveWorkers and stopped
during shutdown, ensuring queue shutdown closes attached scheduler sessions;
otherwise explicitly document in both methods that callers own the executor
lifecycle.
In `@sdks/node/test/worker/executorAttachServer.test.ts`:
- Line 71: Update the server spawn setup in the executor attach test so its
stdout and stderr are either discarded or continuously consumed by the test
logger. Replace the undrained stdio pipes in the spawn call while preserving the
existing SERVER_BIN, env, and process lifecycle behavior.
---
Nitpick comments:
In `@crates/taskito-python/src/executor.rs`:
- Around line 142-148: Update wait so with_handle does not retain the mutex
guard while blocking in handle.wait_timeout. Clone or otherwise share the
required handle state before entering the timeout wait, then release the lock so
stop, is_running, and shutdown can proceed immediately; preserve the existing
timeout result and unwrap_or(true) behavior.
In
`@sdks/java/processor/src/main/java/org/byteveda/taskito/processor/TaskHandlerProcessor.java`:
- Around line 40-41: Annotate TaskHandlerProcessor with
`@IncrementalAnnotationProcessor`(AGGREGATING) alongside its existing processor
annotations. Keep writeServiceFile() and the processors accumulation behavior
unchanged so the service output continues to include all providers during
processingOver().
In `@sdks/java/src/main/java/org/byteveda/taskito/worker/Executor.java`:
- Line 55: Update the closed-state handling in Executor.close() and its closed
field so shutdown is atomic across threads: make the field volatile and use a
compare-and-set guard, or synchronize close(). Ensure only one caller performs
handlerPool.shutdown(), resources.teardownWorker(), and the WORKER_STOPPED event
while preserving idempotent subsequent close() calls.
In `@sdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.java`:
- Line 65: Remove the unused results deque field and its ArrayDeque/Deque
imports, then delete the dead results branch from nextResult. Configure a read
timeout on the accepted socket used by readFrame so its deadline loop can
enforce SETTLE_MS and produce the intended no-result assertion.
- Around line 229-247: Add tests in ExecutorAttachTest covering
Executor.Builder.discover(ClassLoader) with a synthetic
META-INF/services/org.byteveda.taskito.worker.HandlerRegistryProvider resource
and verifying the discovered handler is advertised. Also add a cancel-frame test
exercising JavaDispatcher cancellation, including reclassification of a failed
task as cancelled; reuse existing FakeScheduler and attachment helpers where
applicable.
In `@sdks/node/src/detached.ts`:
- Around line 57-62: Update the warning labels in updateProgress and
writeTaskLog to reflect their public API methods; use the method name for
updateProgress and a shared label covering both log and publish for
writeTaskLog, such as “log/publish”.
In `@sdks/node/src/executor.ts`:
- Around line 176-187: Validate the programmatic run.slots value with the same
positive-integer rules implemented by envInt before passing it to native code.
Reject non-integer or values below 1 with the same error behavior and message
format, while preserving handling for omitted slots.
In `@sdks/node/src/queue.ts`:
- Around line 1665-1670: Update the middlewareFor callback to cache the result
of disables.getFor(taskName) per task name, avoiding repeated settings reads for
dispatched jobs while preserving the existing filtering behavior for disabled
and enabled task middleware.
In `@sdks/node/test/worker/executorAttach.test.ts`:
- Around line 467-469: Remove the redundant process.env.TASKITO_ATTACH =
undefined assignment in the test setup, leaving delete
process.env.TASKITO_ATTACH as the sole operation that clears the environment
variable while preserving the previous value capture.
In `@sdks/node/test/worker/executorAttachServer.test.ts`:
- Around line 38-48: Update freePort and the executor startup test to remove
both race conditions: reserve/select a port without closing the probe before the
server binds, and replace the fixed await sleep(1000) delay with waitFor polling
that uses queue.listWorkers() to confirm the attached executor is ready before
enqueueing work.
In `@sdks/python/taskito/cli.py`:
- Around line 342-406: Register SIGTERM and SIGINT handlers in run_executor
before constructing _Executor, with request_stop safely stopping the attached
executor once available; this must cover the handshake window where jobs may
arrive. Keep the existing signal behavior and move the attachment status print
after handler registration. Wrap the polling and shutdown flow in try/finally so
executor.shutdown() always runs when the loop or wait raises.
In `@sdks/python/tests/worker/executor_apps/attach_app.py`:
- Around line 22-28: Update sdks/python/tests/worker/executor_apps/attach_app.py
lines 22-28 to use a tempfile.gettempdir()-based per-run database filename
instead of the fixed /tmp fallback, and derive db_url from the resolved backend
value. Update sdks/python/tests/worker/test_executor_attach.py lines 241-257 to
set TASKITO_EXECUTOR_TEST_DB to a tmp_path-based value in the test process
before payload_for and _app_importable import attach_app.
In `@sdks/python/tests/worker/test_executor_attach_server.py`:
- Around line 135-137: Replace the fixed time.sleep(1.0) waits in the affected
executor attach tests with a shared readiness helper, such as wait_attached,
that polls the executor process output for its attach confirmation until the
existing timeout and reports early exit or timeout failures. Call this helper
before enqueueing jobs in each affected test, preserving the current test flow
while eliminating timing-dependent waits.
In `@sdks/python/tests/worker/test_executor_attach.py`:
- Around line 129-136: The next_heartbeat helper currently accepts a normal busy
heartbeat when free_slots is zero, so the drain assertion can occur before
SIGTERM takes effect. Update the drain test flow around next_heartbeat and the
SIGTERM send to consume or distinguish pre-signal heartbeats, then assert the
drain-specific heartbeat field or otherwise require a heartbeat emitted after
the signal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad8de900-f2ef-4bc8-b7f5-bffad9e29091
📒 Files selected for processing (46)
crates/taskito-core/src/lib.rscrates/taskito-core/src/worker/cancel.rscrates/taskito-core/src/worker/dial.rscrates/taskito-core/src/worker/executor.rscrates/taskito-core/src/worker/executor_tests.rscrates/taskito-core/src/worker/mod.rscrates/taskito-core/src/worker/protocol.rscrates/taskito-java/src/dispatcher.rscrates/taskito-java/src/executor.rscrates/taskito-java/src/lib.rscrates/taskito-node/src/dispatcher.rscrates/taskito-node/src/executor.rscrates/taskito-node/src/lib.rscrates/taskito-python/src/executor.rscrates/taskito-python/src/lib.rssdks/java/processor/src/main/java/org/byteveda/taskito/processor/TaskHandlerProcessor.javasdks/java/src/main/java/org/byteveda/taskito/cli/Cli.javasdks/java/src/main/java/org/byteveda/taskito/internal/JniExecutorControl.javasdks/java/src/main/java/org/byteveda/taskito/internal/MiddlewareDisables.javasdks/java/src/main/java/org/byteveda/taskito/internal/NativeExecutor.javasdks/java/src/main/java/org/byteveda/taskito/worker/Executor.javasdks/java/src/main/java/org/byteveda/taskito/worker/HandlerRegistryProvider.javasdks/java/src/main/java/org/byteveda/taskito/worker/WorkerDispatchBridge.javasdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.javasdks/node/src/cli/commands/executor.tssdks/node/src/cli/commands/index.tssdks/node/src/cli/commands/run.tssdks/node/src/cli/index.tssdks/node/src/cli/load-app.tssdks/node/src/detached.tssdks/node/src/executor.tssdks/node/src/index.tssdks/node/src/native.tssdks/node/src/queue.tssdks/node/src/task-callback.tssdks/node/src/worker.tssdks/node/test/worker/executorAttach.test.tssdks/node/test/worker/executorAttachServer.test.tssdks/python/taskito/_taskito.pyisdks/python/taskito/app.pysdks/python/taskito/cli.pysdks/python/taskito/detached.pysdks/python/tests/worker/executor_apps/__init__.pysdks/python/tests/worker/executor_apps/attach_app.pysdks/python/tests/worker/test_executor_attach.pysdks/python/tests/worker/test_executor_attach_server.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
stop() is documented as safe on a signal-handling path, but begin_drain wrote the zero-capacity heartbeat first — a scheduler that stopped reading blocked that write for the whole write timeout.
teardown joined unconditionally after the drain budget, so a task that ignores its cancel kept the pool's run alive and the process never exited. The test could not catch it: its blocking behaviour released itself.
The companion is written at package level and names the handler by simple name, so an Outer.Inner provider would not compile. Also corrects the service type in the Javadoc.
System.exit runs hooks while main is still inside Runtime.exit, so a registered hook waited out the whole drain budget for a join that could not complete, then called stop() on an already-closed control.
A throwaway CommandLine routed a bad TASKITO_SLOTS through the execution-exception handler, so the user saw a stack trace rather than a usage message. Zero and negative counts also passed, and the builder clamped them behind the banner.
awaitSession held the read lock for the whole park. A queued close() then blocked a concurrent stop() — the only call that could have ended the session. Waiters are counted instead, and close drains them before freeing.
Nothing awaits a signal listener's return value, so a rejected stop() surfaced as an unhandled rejection and aborted the process mid-drain.
runExecutor tracked nothing, so queue.shutdown() left an attached executor running and its scheduler session open.
Nothing read the pipes, so a scheduler that logs per job fills the buffer and blocks on its next write.
Contributor
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
crates/taskito-core/tests/rust/executor_tests.rs (1)
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the pool observed
shutdown().
TestPool::shutdownrecords intoself.shutdown, but no test reads that field.ExecutorHandle::stopcallsself.dispatcher.shutdown(), andspawn_readercalls it again when the session ends. Neither call is covered.Add an accessor and assert it in the drain test, so a regression that stops signalling the pool is caught.
💚 Proposed test addition
fn cancelled(&self) -> Vec<String> { self.cancels.lock().expect("cancels").clone() } + + /// Whether the executor asked the pool to stop accepting work. + fn was_shut_down(&self) -> bool { + self.shutdown.load(Ordering::SeqCst) + }Then in
a_drain_announces_zero_capacity_before_anything_else, or instop_finishes_in_flight_work_before_disconnectingafter the result arrives:attached.handle.stop(); + wait_until( + || attached.pool.was_shut_down(), + "stop() must tell the pool to stop accepting work", + );Also applies to: 161-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-core/tests/rust/executor_tests.rs` around lines 59 - 60, Add a `TestPool` accessor for its recorded `shutdown` flag, then update the drain-related test (`a_drain_announces_zero_capacity_before_anything_else` or `stop_finishes_in_flight_work_before_disconnecting`) to assert the accessor is true after shutdown completes, covering the `ExecutorHandle::stop`/`spawn_reader` signaling paths.sdks/java/src/main/java/org/byteveda/taskito/worker/Executor.java (1)
113-138: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake
close()idempotent across threads.
closedis a plain field andclose()is not synchronized.Executoris public, so a caller can close it from a shutdown hook while the main thread also closes it. Both threads can then pass the guard, callresources.teardownWorker()twice, and emitWORKER_STOPPEDtwice.synchronizedonclose()plus avolatileread keeps the documented idempotence.♻️ Proposed refactor
- private boolean closed; + private volatile boolean closed;`@Override` - public void close() { + public synchronized void close() { if (closed) { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/main/java/org/byteveda/taskito/worker/Executor.java` around lines 113 - 138, Make Executor.close() thread-safe and idempotent by synchronizing the close() method and declaring the closed field volatile. Preserve the existing early-return guard so concurrent callers allow only one shutdown sequence, including resources.teardownWorker() and WORKER_STOPPED emission.sdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.java (1)
65-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
resultsdeque.Nothing adds to
results.readFrame()returns each frame directly, so the!results.isEmpty()branch at lines 190-192 is unreachable. Deleting the field and the branch makes the polling loop easier to read.♻️ Proposed cleanup
- private final Deque<JsonNode> results = new ArrayDeque<>();JsonNode nextResult() throws IOException { long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(SETTLE_MS); while (System.nanoTime() < deadline) { - if (!results.isEmpty()) { - return results.removeFirst(); - } JsonNode frame = readFrame();Drop the now-unused
ArrayDequeandDequeimports.Also applies to: 186-202
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.java` at line 65, Remove the unused results field from ExecutorAttachTest, delete the unreachable results check in the polling loop within readFrame-related logic, and remove the now-unused ArrayDeque and Deque imports while preserving direct frame handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/taskito-core/src/worker/protocol.rs`:
- Around line 244-293: Update the SchedulerMessage::Job dispatch representation
and SchedulerMessage::into_job to carry and reconstruct created_at,
scheduled_at, priority, metadata, unique_key, and notes instead of defaulting
them. Populate these fields when converting a Job into a dispatch message, and
ensure the Node and Python converters expose the reconstructed values
consistently for detached and attached workers.
In `@crates/taskito-python/src/executor.rs`:
- Around line 142-154: Update Executor::stop and its contract to acknowledge
that concurrent calls may block behind wait’s with_handle mutex for up to
timeout_ms, or otherwise ensure stop uses a separately shared primitive that
does not depend on the polling handle lock. Keep the existing wait behavior and
CLI signal-handling flow unchanged.
In
`@sdks/java/processor/src/main/java/org/byteveda/taskito/processor/TaskHandlerProcessor.java`:
- Around line 35-38: Update the class Javadoc near the generated companion
description to state that an accessible no-arg constructor causes generation of
a nested Provider class implementing HandlerRegistryProvider, which is
registered through META-INF/services; remove the inaccurate provider() method
wording and preserve the explanation of classpath discovery.
In `@sdks/java/src/main/java/org/byteveda/taskito/cli/Cli.java`:
- Around line 294-296: Update the token assignment in Cli.Executor to pass
TASKITO_ATTACH_TOKEN through a blankToNull helper before calling token, treating
null and blank values as absent while preserving nonblank tokens. Add the helper
as a private static nullable method alongside the existing Executor helpers,
matching the address normalization behavior.
In `@sdks/python/tests/worker/test_executor_attach_server.py`:
- Around line 134-138: Replace the fixed one-second attach waits with a shared
polling readiness helper that confirms the executor is attached before
enqueueing. Update sdks/python/tests/worker/test_executor_attach_server.py lines
134-138, 146-150, and 175-178 to call the helper before enqueueing ECHO, BOOM,
and SLOW respectively; preserve each test’s existing enqueue and status
assertions.
---
Nitpick comments:
In `@crates/taskito-core/tests/rust/executor_tests.rs`:
- Around line 59-60: Add a `TestPool` accessor for its recorded `shutdown` flag,
then update the drain-related test
(`a_drain_announces_zero_capacity_before_anything_else` or
`stop_finishes_in_flight_work_before_disconnecting`) to assert the accessor is
true after shutdown completes, covering the
`ExecutorHandle::stop`/`spawn_reader` signaling paths.
In `@sdks/java/src/main/java/org/byteveda/taskito/worker/Executor.java`:
- Around line 113-138: Make Executor.close() thread-safe and idempotent by
synchronizing the close() method and declaring the closed field volatile.
Preserve the existing early-return guard so concurrent callers allow only one
shutdown sequence, including resources.teardownWorker() and WORKER_STOPPED
emission.
In `@sdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.java`:
- Line 65: Remove the unused results field from ExecutorAttachTest, delete the
unreachable results check in the polling loop within readFrame-related logic,
and remove the now-unused ArrayDeque and Deque imports while preserving direct
frame handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e32abdfc-cc4f-4a80-bcfe-8c0a07073426
📒 Files selected for processing (49)
crates/taskito-core/src/lib.rscrates/taskito-core/src/worker/cancel.rscrates/taskito-core/src/worker/dial.rscrates/taskito-core/src/worker/executor.rscrates/taskito-core/src/worker/mod.rscrates/taskito-core/src/worker/protocol.rscrates/taskito-core/tests/rust.rscrates/taskito-core/tests/rust/executor_tests.rscrates/taskito-core/tests/rust/remote_tests.rscrates/taskito-core/tests/rust/worker_tests.rscrates/taskito-java/src/dispatcher.rscrates/taskito-java/src/executor.rscrates/taskito-java/src/lib.rscrates/taskito-node/src/dispatcher.rscrates/taskito-node/src/executor.rscrates/taskito-node/src/lib.rscrates/taskito-python/src/executor.rscrates/taskito-python/src/lib.rssdks/java/processor/src/main/java/org/byteveda/taskito/processor/TaskHandlerProcessor.javasdks/java/src/main/java/org/byteveda/taskito/cli/Cli.javasdks/java/src/main/java/org/byteveda/taskito/internal/JniExecutorControl.javasdks/java/src/main/java/org/byteveda/taskito/internal/MiddlewareDisables.javasdks/java/src/main/java/org/byteveda/taskito/internal/NativeExecutor.javasdks/java/src/main/java/org/byteveda/taskito/worker/Executor.javasdks/java/src/main/java/org/byteveda/taskito/worker/HandlerRegistryProvider.javasdks/java/src/main/java/org/byteveda/taskito/worker/WorkerDispatchBridge.javasdks/java/src/test/java/org/byteveda/taskito/worker/ExecutorAttachTest.javasdks/node/src/cli/commands/executor.tssdks/node/src/cli/commands/index.tssdks/node/src/cli/commands/run.tssdks/node/src/cli/index.tssdks/node/src/cli/load-app.tssdks/node/src/detached.tssdks/node/src/executor.tssdks/node/src/index.tssdks/node/src/native.tssdks/node/src/queue.tssdks/node/src/task-callback.tssdks/node/src/worker.tssdks/node/test/worker/executorAttach.test.tssdks/node/test/worker/executorAttachServer.test.tssdks/python/taskito/_taskito.pyisdks/python/taskito/app.pysdks/python/taskito/cli.pysdks/python/taskito/detached.pysdks/python/tests/worker/executor_apps/__init__.pysdks/python/tests/worker/executor_apps/attach_app.pysdks/python/tests/worker/test_executor_attach.pysdks/python/tests/worker/test_executor_attach_server.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
kartikeya-27
approved these changes
Aug 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the three SDK executor subcommands — the S5–S7 phases of #546.
Closes #551
Closes #552
Closes #553
One client in core, three thin CLIs
tasks/executor-attach.mdsays "only the concrete stream changes", and that turned out to be literally true, so it sets the shape:WorkerDispatcherwas already the seam every SDK pool implements, and its signature —run(job_rx, result_tx)+shutdown()+notify_cancel()— is exactly what an executor needs.Worker::dispatcher()even documented the mirror case. SoExecutorClientisWorker::spawnwith storage swapped for a socket, and each SDK's subcommand is a wrapper over the pool it already had.Writing that client three times would have tripled the subtle parts — handshake ordering, exactly-once result accounting, drain sequencing, cancel routing — with nothing gained. The JSON-header wire is unchanged, so a third-party SDK can still write an executor with its standard library alone.
The executor opens no storage
An executor imports the user's app to find its handlers, and that import used to construct a
Queue— which connected eagerly, putting the database credentials straight back into the app image that #546 exists to keep them out of.TASKITO_DETACHED_EXECUTOR=1is now set by each CLI before it imports the app, and the native queue is replaced by a stand-in. Python's prefork children inherit the variable, so they open nothing either; Java already took noQueueBackend. One rule decides every method:None/nullis what a queue with no such row returns anyway, and callers already handle it. Not optional:Queue.__init__reads settings (webhook subscriptions), so a stand-in that refused them could not build aQueueat all.set_progressmust not fail because it happens to be running detached.What that costs — progress, task logs, published partials, dashboard middleware toggles — is tracked in #589 with per-layer sub-issues; closing it needs new protocol frames.
Four things the tests caught that reading the docs did not
provider()form only applies to modules on the module path. The first attempt generatedprovider()and died withServiceConfigurationError: demo.GreeterTasks not a subtype. A newHandlerRegistryProviderinterface is now the service, and the processor generates a nested<Class>Tasks$Providerimplementing it. Owners with no accessible no-arg constructor are skipped with a compiler note rather than silently — only their own code knows how to build them.startExecutorblocked the Node event loop for the whole handshake timeout. Now async. Found because the in-process test deadlocked against its own fake scheduler.stop()hung forever.stop()cannot unpark the frame reader, which is parked on a read only the scheduler could satisfy, so the session never ended and any shell that stopped and then waited deadlocked. Fixed in core with a regression test.AbortSignala cooperative task watches is driven by a storage poll.JsExecutor.isCancelRequestednow exposes the state the cancel frames land in. The test asserts the result frame comes backcancelled, notfailure— proving both that the handler aborted and that the native side reclassified the throw.Verification
cargo test --workspaceclean,clippy --all-targets -D warningsclean,--features postgresand--features redischecktaskito-serverbinary; ruff + mypy cleanMETA-INF/services, with no usermainEach SDK has two test layers: a hermetic scheduler speaking the wire (so they run with no Rust build, in every CI job) and the same assertions against the real
taskito-serverbehindTASKITO_SERVER_BIN. That pairing is what keeps the fakes honest.Two environment notes, both reproduced on a clean tree and unrelated to this branch: Java's
:compileTestJavais skipped here, so its tests were run through the JUnit platform launcher directly; and 10test/dashboard/**Node files fail on a missing SPA build.Reviewing
The first three commits (
feat(server): authenticate the attach listener handshakeand the two storage fixes) are the #584 review follow-ups this branch was cut from, not part of this work — they are unmerged and unreviewed, so say the word if you would rather they went up as their own PR and this one rebased onto master.The remaining 25 split cleanly by layer: core (7), Python (5), Node (7), Java (6). S5/S6/S7 were meant to be separate PRs, so this can be split per SDK if you prefer that shape.
Summary by CodeRabbit
New Features
Bug Fixes