Repository navigation
Conversation
withTenant, withRootTenant and withEachTenant put the previous tenant back through setTenant, which dereferences it. Work outside an HTTP request starts with no tenant: a background task, a scheduled action, an EventBridge event. For those the restore threw a TypeError after the callback had already done its work, so the invocation failed even though it succeeded. The helpers now restore through a private restoreTenant that accepts null and skips the disabled-tenant check, which only matters when switching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…er HTTP The standalone server reached its per-request stack from outside a request by POSTing to itself: /background-task, /scheduled-action-run, /scheduled-action-recover and /empty-trash-bins, each guarded by a per-process token and assuming localhost:PORT was reachable. Background tasks also spawned a worker thread per task just to drive that POST loop. Now everything goes through an EventDispatcher that createServerHandler registers on the root container. It hands an event to the handler app, which builds a fresh request container for it, the way Lambda invocations work on AWS. Dispatch runs in an AsyncLocalStorage snapshot taken at boot, so identity and authorization overrides active in the caller don't leak in. - Background tasks: a root TaskLoop dispatches one BackgroundTaskEvent per iteration and waits as the runner asks. No worker thread. - Scheduler: Bree fires a ScheduledActionEvent, handled by the same handler AWS uses. Boot recovery is a ScheduledActionRecoverEvent, run in the background without waiting for listen(). - Empty trash: the timer dispatches an EmptyTrashBinsEvent. BackgroundTaskEventType and ScheduledActionEventType, with their handler abstractions, move from event-handler-aws to event-handler-core so both hosting types share them. api-scheduler no longer depends on event-handler-aws. The four routes, three internal tokens, the worker and the localhost self-callback helpers are gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… first 1000 Recovery read one page of 1000. Anything past it was lost on restart. It now pages through the list with the cursor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🚓 Slop Cop Coherent refactor matching its stated scope; a couple of minor style nits and already-disclosed console-logging tradeoffs, nothing indicating accidental content or integrity risk. 🚨 Should this be in the PR? 🟡 Low — Large deletions match stated refactor scope The diff deletes ~1515 lines (worker threads, HTTP self-callback routes, internal token abstractions, and their tests) but this matches the PR's explicit description of removing routes, tokens, and the worker-based task orchestrator in favor of in-process dispatch. No unrelated files or unexplained deletions were found; the footprint is coherent with the stated intent. 🟡 Low — console.error/console.log used in root singleton wiring schedulerServer.ts, bulkActionsServer.ts, and DispatchingTaskLoop.ts use console.error/console.log directly instead of the DI Logger (no-console-in-backend.md). The PR author explicitly flags this in 'Notes for review' as a known tracked issue (#5446) since root singletons have no DI Logger available, so this is a deliberate, disclosed tradeoff rather than an accidental leftover. 📏 Code-style rule checks 🟡 Low — Multiple named imports on one line packages/api-event-handler-standalone/src/scheduler/RecoverScheduledActionsHandler.ts imports 🟡 Low — Potential mixed-layer dependency in RecoverScheduledActionsHandler RecoverScheduledActionsHandlerImpl composes several use cases (GetRootTenantUseCase, ListScheduledActionsUseCase) and primitives (RawTenantId, RequestTenantLoader, IdentityContext) alongside SchedulerSingleton, a stateful service that isn't a use case or repository. This may be intentional (SchedulerSingleton is a domain service, similar to the allowed exception in no-mixed-layers-in-dependencies.md), so flagged as low-confidence only. Automated, non-blocking heads-up from an LLM. It can be wrong — use your judgment. Regenerates on every push. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (63)
💤 Files with no reviewable changes (19)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds event dispatch to the core and standalone event-handler packages. It replaces standalone worker and internal HTTP-route paths for background tasks, scheduled actions, and empty-trash triggers. It also changes tenant-context restoration when callbacks start without a tenant. ChangesStandalone event runtime
Tenant-context restoration
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~50 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant InProcessTaskService
participant TaskLoop
participant EventDispatcher
participant HandlerApp
participant InProcessBackgroundTaskHandler
InProcessTaskService->>TaskLoop: start task event
TaskLoop->>EventDispatcher: dispatch task event
EventDispatcher->>HandlerApp: handle task event
HandlerApp->>InProcessBackgroundTaskHandler: invoke matched handler
InProcessBackgroundTaskHandler-->>EventDispatcher: return task result
EventDispatcher-->>TaskLoop: return task result
Merge Risk: ⚪ Minimal · up to No confirmed issue remains that should block merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 39 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
This branch has not been deployed
1 participant
Summary
The standalone server reached its own per-request stack by sending HTTP requests to itself. Background tasks, scheduled actions, scheduler boot recovery and the empty-trash timer each POSTed to a route on
localhost:PORT. Each route was guarded by a token generated per process, and background tasks started a worker thread per task just to run that POST loop.This PR replaces all of that with in-process dispatch, the same shape AWS already uses for non-HTTP Lambda invocations.
How it works
EventDispatcher. A new abstraction inevent-handler-core.createServerHandlerregisters it on the root container.dispatch(event)callsHandlerApp.handle(event), which builds a fresh request container, matches the event type and runs its handler.AsyncLocalStorage.snapshot()taken at boot. Without this, a task triggered insidewithoutAuthorization(...)orwithIdentity(...)would carry that override into its own request. The HTTP hop gave this isolation by accident; now it's deliberate, and there's a test for it.InProcessTaskServicehands the task to a rootTaskLoop. The loop dispatches oneBackgroundTaskEventper iteration and waits as long as the runner asks. There is no worker thread any more.ScheduledActionEvent. The handler is the existing AWS one,ScheduledActionLambdaHandler, unchanged. Boot recovery is aScheduledActionRecoverEvent. It runs in the background, so it no longer needssetTimeout(1000)to wait forlisten().EmptyTrashBinsEvent.BackgroundTaskEventTypeandScheduledActionEventType, with their handler abstractions, moved fromevent-handler-awstoevent-handler-core. That leavesapi-schedulerwith no dependency onevent-handler-aws.Removed
/background-task,/scheduled-action-run,/scheduled-action-recoverand/empty-trash-bins;TaskOrchestrator,workerEntry);localhost:PORTself-callback helpers.sequenceDiagram participant A as Admin participant R as Request (main thread) participant L as TaskLoop (root singleton) participant D as EventDispatcher participant DB as Database A->>R: triggerTask(definition, input, delay) R->>DB: create task record (pending) R->>L: start(BackgroundTaskEvent) R-->>A: response loop until done, aborted or error L->>D: dispatch(BackgroundTaskEvent) Note over D: fresh request container, TaskRunner D->>DB: task progress / result D-->>L: { status, wait, delay } Note over L: continue: sleep `wait`, send back `delay` endBug fixes found on the way
TenantContextrestore (separate commit,api-core).withTenant,withRootTenantandwithEachTenantput the previous tenant back throughsetTenant, which dereferences it. Work outside an HTTP request starts with no tenant, so the restore threw aTypeErrorafter the callback had already done its work. I found it running a second API process with its console visible: the empty-trash trigger failed every time. This affects AWS too. EventBridge (empty trash) and EventBridge Scheduler (scheduled publish) invocations have no tenant loader either, so they most likely do their work and then fail the invocation.Tests
New:
DispatchingTaskLoop: loop until done, the delay hand-off,wait, aborted, error, a throwing iteration.InProcessTaskService.EventDispatcheron the Node server: a dispatched event reaches its handler, async context doesn't leak, an unknown event is rejected.TenantContextwith no tenant set (3 cases; all fail without the fix).Passing:
Live, against this branch on a local standalone server (SQLite):
triggerTask(testingRun)started 13 ms after it was created.delay: 10, it started 10.01 s after it was created.re-armed N, and the empty-trash trigger no longer fails.Notes for review
console.error. Root singletons have no DILogger; that's Self-hosted scheduler: revisit the boot-time consoleLogger shim #5446.RequestContainerin the task handler.InProcessBackgroundTaskHandlertakesRequestContainerto buildTaskRunner's legacy context object, the same asBackgroundTaskLambdaHandleron AWS.ScheduledActionLambdaHandlereven though it now also runs on standalone, to keep the AWS diff small.streaming.test.ts"stalled client" failed once in 16 runs, on the first run after a new test file was added. That looks like cold-start transform time eating its 3 s window. It's unrelated to this change, but I'm noting it in case CI hits it.Workspace: wby-next9 · audit standalone handlers
🤖 Generated with Claude Code
Summary by CodeRabbit