Repository navigation
Keep per-run history and show running state for scheduled tasks - #554
Draft
nicdavidson wants to merge 1 commit into
Draft
nicdavidson wants to merge 1 commit into
nicdavidson wants to merge 1 commit into
Conversation
Contributor
Author
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
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.
Refs dreamfactorysoftware/df-scheduler#41
What
task_runtable. Each run records start, finish, status code and output, capped at 16 KB. The newest 100 runs per task are kept.GET system/scheduler[/{id}]now returnsrun_status(is_running,last_run,last_success,last_failure). Full history is available through?related=task_run_by_task_id.task_logis unchanged.Behavior change
task_runmigration runs on upgrade. The existing error log column andtask_log_by_task_idbehave as before.Tests
df-scheduler:
tests/TaskRunHistoryTest.phpadds 10 tests. All 10 fail ondevelopand pass on this branch. They prove:task_logis untouched;is_runningis true while a run is open and false once it finishes;run_statusand the run history.Result:
OK, but there were issues! | Tests: 16, Assertions: 38, PHPUnit Deprecations: 1. The deprecation is also ondevelop.df-admin-interface: jest
Tests: 313 passed, 313 total(was 307), including the newrun-status.spec.ts. The build passes (92 files). Playwrighte2e/smoke.spec.tsande2e/scheduler-runs.spec.tspass, with 0 failures already on base.Limits: the schedule wiring (
before/onSuccess/onFailure) is checked by a source-level test, not by driving a real scheduled event. The e2e does not seed a run, so it shows the "Never" and empty states and not a populated history.Scope
TaskRunmodel,TaskSchedulerstart/finish hooks,run_statusonSchedulerTask, table mapping, CHANGELOG.relatedparam, i18n keys, helper and specs.task_log.Related issues
composer.json. No overlap, not fixed here.jest.config.ci.js. The conflict is trivial (keep both entries).Existing bugs found but not fixed
No bugs that already existed on
developwere found. The review left these limits in the new code:run_statuscosts about 4 small indexed queries per task, so the list endpoint runs one set per row. This is fine at normal task counts. Batch it if lists reach hundreds.is_running: falsewith a null status code, and the list cell reads "Failed , 2 days ago" with no code. The next run closes it as 500.isOkis imported but unused indf-scheduler-details.component.ts.Factory gate
Open review notes (not blocking, from the factory's final review)
src/Components/TaskScheduler.php:130-135: NUL bytes and a single save.mb_scrubkeeps\0, because NUL is valid UTF-8. PostgreSQLtextrejects 0x00 ("invalid byte sequence for encoding UTF8: 0x00").finishRunwritesfinished_at,status_codeandoutputin onesave(). So when the output is bad, the whole row update fails. The exception is caught and only logged.print_r($result)writes the raw bytes into the log file.startRuncloses it as500 "Run did not finish". Every successful run of that task is recorded as a failure.str_replace("\0", '', ...)in the same expression. Optionally, savefinished_atandstatus_codefirst, then the output, so a bad output can never leave the run open.tests/TaskRunHistoryTest.php:166testScheduleWiresRunHooks(carried over from round 1).before/onSuccess/onFailurewiring, and that wiring is proven only by grep.Event::run. The mutex check runs first, thencallBeforeCallbacks, thenexecute, thenfinishwithexitCode.onSuccessandonFailureare gated onexitCode.$stateis shared per event.pgrep croncheck behind a seam. Then pull the event fromapp(Schedule::class)->events()and drivecallBeforeCallbacksandfinish($app, 0|1).e2e/scheduler-runs.spec.ts:38-49: the e2e can pass for the wrong reason.runStatusis missing entirely (describeLastRun(undefined)returns "Never").relatedparam is broken (runs = []).src/Models/SchedulerTask.php:66-87: N+1 on the list endpoint (round 1, still open).run_statuscosts 4 queries per task. With a 50-row page that is about 200 small indexed queries. It is fine at normal task counts. Batch it if lists grow.SchedulerTask.php:85and df-admin-interfaceutils/run-status.ts:27: stale open run shows an empty failure code (round 1, still open). An open run older than 24 h givesis_running = falseandstatus_code = null. The list cell then renders "Failed , 2 days ago" (empty code). In practicestartRuncloses such rows on the next tick, so the window is small.adf-scheduler/resolvers/scheduler.resolver.ts:23: large payload on every edit-page load. The edit page loads up to 100 runs × 16 KB of output (about 1.6 MB) on every open, even if the Run history tab is never clicked. Consider loading it lazily on tab open, or requesting runs withoutoutputand fetching output on expand.df-scheduler-details.component.ts:29,99: dead code.isOkis imported and assigned but never used in the template. Drop it.jest.config.ci.js. The conflict is trivial: keep both entries.