feat(spider-tdl)!: Add support for an optional init hook run once when a TDL package is loaded. - #409
Conversation
WalkthroughAdds optional TDL package initialization hooks. Registered packages export an init symbol, the executor runs it during loading, init errors become ChangesTDL package initialization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TdlPackageManager
participant TdlPackageLoad
participant PackageInitSymbol
participant PackageInitFunction
TdlPackageManager->>TdlPackageLoad: load package
TdlPackageLoad->>PackageInitSymbol: resolve optional init symbol
PackageInitSymbol-->>TdlPackageLoad: init function or missing symbol
TdlPackageLoad->>PackageInitFunction: invoke init once
PackageInitFunction-->>TdlPackageLoad: success or encoded TdlError
TdlPackageLoad-->>TdlPackageManager: loaded package or PackageInitError
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/huntsman/tdl-integration/tests/init.rs (1)
68-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a RAII drop guard to ensure the environment variable is cleaned up on panic.
If
manager.load()unexpectedly succeeds,.expect_err()will panic beforeremove_varis executed. While you have documented thatcargo nextestprocess isolation mitigates the fallout, implementing a standard drop guard makes the test intrinsically panic-safe regardless of the test runner being used.🛠️ Proposed refactor for panic safety
// The init hook failing with this var set is the behavior under test. unsafe { std::env::set_var(ENV_SPIDER_TEST_TDL_INIT_SHOULD_FAIL, "1") }; + + struct EnvGuard(&'static str); + impl Drop for EnvGuard { + fn drop(&mut self) { + unsafe { std::env::remove_var(self.0) }; + } + } + let _guard = EnvGuard(ENV_SPIDER_TEST_TDL_INIT_SHOULD_FAIL); + let path = package_path(); let mut manager = TdlPackageManager::new(); let err = manager .load(&path) .expect_err("load should fail when init errors"); - // SAFETY: see above. - unsafe { std::env::remove_var(ENV_SPIDER_TEST_TDL_INIT_SHOULD_FAIL) }; let ExecutorError::PackageInitError(TdlError::ExecutionError(msg)) = &err else {🤖 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 `@tests/huntsman/tdl-integration/tests/init.rs` around lines 68 - 75, Make the environment-variable setup in the test using ENV_SPIDER_TEST_TDL_INIT_SHOULD_FAIL panic-safe by introducing a scoped RAII drop guard that removes the variable during unwinding. Update the cleanup around manager.load and expect_err so normal execution and panic paths both remove the variable, while preserving the existing failure assertion.
🤖 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.
Nitpick comments:
In `@tests/huntsman/tdl-integration/tests/init.rs`:
- Around line 68-75: Make the environment-variable setup in the test using
ENV_SPIDER_TEST_TDL_INIT_SHOULD_FAIL panic-safe by introducing a scoped RAII
drop guard that removes the variable during unwinding. Update the cleanup around
manager.load and expect_err so normal execution and panic paths both remove the
variable, while preserving the existing failure assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 71bfc203-3bd3-4cee-a8ed-4743bc6be4a0
📒 Files selected for processing (6)
components/spider-task-executor/src/error.rscomponents/spider-task-executor/src/manager.rscomponents/spider-tdl/src/register.rstests/huntsman/integration-test-tasks/src/lib.rstests/huntsman/tdl-integration/Cargo.tomltests/huntsman/tdl-integration/tests/init.rs
Description
This PR lets a TDL package author register an optional initialization function that the task executor runs once, when the package is loaded. The hook is meant for one-time package-level setup and takes no arguments, returning
Result<(), TdlError>.Registration macro (
spider-tdl)register_tdl_package!gains an optionalinitfield, placed betweenpackage_nameandtasks:The macro always emits a new
extern "C"entry point,__spider_tdl_package_init, alongside the existingget_version/get_name/executesymbols. Wheninitis omitted, the emitted function defaults to a no-op that returnsOk(()), so the executor can dispatch to the symbol uniformly without probing whether the author supplied a hook. The emitted function returns aTaskExecutionResultcarrying an empty success buffer, or the msgpack-encodedTdlErrorwhen the hook fails.Executor (
spider-task-executor)TdlPackage::loadresolves__spider_tdl_package_initand, if present, calls it once during load. A returned error aborts the load and surfaces as the newExecutorError::PackageInitError(TdlError). The lookup is tolerant: a package that does not export the symbol at all (e.g. one built against an olderspider-tdl) is loaded without running any init, so existing packages continue to load unchanged.Note
BREAKING CHANGE:
The TDL package FFI contract now defines a fourth entry point,
__spider_tdl_package_init. Every package built withregister_tdl_package!exports it, and any tooling or hand-rolled package that reproduces or enumerates the TDL entry-point set must account for it. The executor tolerates the symbol's absence, so packages built against the previousspider-tdlstill load.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Tests