Skip to content

Keep per-run history and show running state for scheduled tasks - #554

Draft
nicdavidson wants to merge 1 commit into
developfrom
fix/df-scheduler-41
Draft

nicdavidson wants to merge 1 commit into
developfrom
fix/df-scheduler-41

Conversation

@nicdavidson

Copy link
Copy Markdown
Contributor

Refs dreamfactorysoftware/df-scheduler#41

What

Behavior change

  • Scheduled tasks now keep a record of every run, successful or not. Before, a successful run left no trace and only the last failure was kept.
  • The scheduler API and list show when a task last ran, its last success, its last failure, and whether it is running now.
  • Each task keeps its 100 most recent runs. Deleting a task deletes its history. A run that never finished is marked failed (500) on the task's next run, and stops showing as "Running" after 24 hours.
  • Successful runs now keep up to 16 KB of the scheduled call's response output, visible to admins in the Run history tab. Before, only failure output was kept. The output is collapsed by default, but it can contain whatever the scheduled call returns.
  • The new task_run migration runs on upgrade. The existing error log column and task_log_by_task_id behave as before.

Tests

  • df-scheduler: tests/TaskRunHistoryTest.php adds 10 tests. All 10 fail on develop and pass on this branch. They prove:

    • a successful and a failed run are each recorded, and task_log is untouched;
    • is_running is true while a run is open and false once it finishes;
    • a stale open run does not read as running;
    • last success and last failure are kept separately;
    • an abandoned run is closed on the next start;
    • the 100-run cap keeps the newest and leaves other tasks alone;
    • output is truncated;
    • invalid UTF-8 output is scrubbed so the task API still encodes;
    • the API returns run_status and the run history.

    Result: OK, but there were issues! | Tests: 16, Assertions: 38, PHPUnit Deprecations: 1. The deprecation is also on develop.

  • df-admin-interface: jest Tests: 313 passed, 313 total (was 307), including the new run-status.spec.ts. The build passes (92 files). Playwright e2e/smoke.spec.ts and e2e/scheduler-runs.spec.ts pass, 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

  • In:
    • df-scheduler: migration, TaskRun model, TaskScheduler start/finish hooks, run_status on SchedulerTask, table mapping, CHANGELOG.
    • df-admin-interface: Last run column, Run history tab, resolver related param, i18n keys, helper and specs.
    • Removed or rerouted flows: none. The existing log column and log tab stay.
  • Out:
    • A days-based or configurable retention (it is a fixed 100-run constant).
    • History for the "cron not running" branch, where no run happens.
    • Live polling of running state in the UI (it refreshes on page load).
    • A docs page (df-docs has no scheduler page).
    • Any change to task_log.
  • Repos changed: df-scheduler and df-admin-interface. The same body is on both PRs.

Related issues

Existing bugs found but not fixed

No bugs that already existed on develop were found. The review left these limits in the new code:

  • run_status costs 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.
  • An open run older than 24 hours reports is_running: false with a null status code, and the list cell reads "Failed , 2 days ago" with no code. The next run closes it as 500.
  • On PostgreSQL, output containing NUL bytes may make the final save fail. The run would then stay open until the next tick closes it as 500. This was read in review, not run.
  • If a run fails but its parsed status code is below 300, it is recorded as a success.
  • The edit page loads up to 100 runs with their output (about 1.6 MB worst case) even if the Run history tab is never opened.
  • Run output can contain API keys or response data, and the UI only collapses it. Masking is not done.
  • isOk is imported but unused in df-scheduler-details.component.ts.
  • This was not checked with an instance holding many tasks.

Factory gate

  • Result on the branch: OK, but there were issues! | Tests: 16, Assertions: 38, PHPUnit Deprecations: 1. | Tests: 313 passed, 313 total | OK build: 92 files | OK e2e: e2e/smoke.spec.ts e2e/scheduler-runs.spec.ts
  • New failures vs base: none (0 tests already fail on base and are excluded)
  • New tests that fail on base and pass here: DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testApiExposesRunStatusAndHistory, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testFailedRunIsRecordedAndTaskLogUnchanged, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testInvalidUtf8OutputIsSanitised, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testLastSuccessAndFailureKeptSeparately, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testNeverRunTask, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testOutputTruncated, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testRetentionCap, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testRunningStateExposed, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testScheduleWiresRunHooks, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testStartClosesAbandonedRun, DreamFactory\Core\Scheduler\Tests\TaskRunHistoryTest::testSuccessfulRunIsRecorded, FAILED: df-admin-interface gate

Open review notes (not blocking, from the factory's final review)

  1. df-scheduler src/Components/TaskScheduler.php:130-135: NUL bytes and a single save.
    • Problem: mb_scrub keeps \0, because NUL is valid UTF-8. PostgreSQL text rejects 0x00 ("invalid byte sequence for encoding UTF8: 0x00"). finishRun writes finished_at, status_code and output in one save(). So when the output is bad, the whole row update fails. The exception is caught and only logged.
    • Failing input: a PostgreSQL system DB, and a scheduled GET on a remote web service that returns binary content. print_r($result) writes the raw bytes into the log file.
    • Result: the run stays open and reads "Running". On the next tick, startRun closes it as 500 "Run did not finish". Every successful run of that task is recorded as a failure.
    • Fix: str_replace("\0", '', ...) in the same expression. Optionally, save finished_at and status_code first, then the output, so a bad output can never leave the run open.
    • I read this, I did not run it (the test env is SQLite).
  2. df-scheduler tests/TaskRunHistoryTest.php:166 testScheduleWiresRunHooks (carried over from round 1).
    • Problem: this is still a source-regex test. The production claim ("a successful run leaves a record") depends on the before/onSuccess/onFailure wiring, and that wiring is proven only by grep.
    • What I checked by reading: the wiring is correct against Laravel's Event::run. The mutex check runs first, then callBeforeCallbacks, then execute, then finish with exitCode. onSuccess and onFailure are gated on exitCode. $state is shared per event.
    • Fix: move the pgrep cron check behind a seam. Then pull the event from app(Schedule::class)->events() and drive callBeforeCallbacks and finish($app, 0|1).
  3. df-admin-interface e2e/scheduler-runs.spec.ts:38-49: the e2e can pass for the wrong reason.
    • "Never" shows when runStatus is missing entirely (describeLastRun(undefined) returns "Never").
    • "This task has not run yet" shows when the related param is broken (runs = []).
    • So the e2e does not prove the API-to-UI mapping. The jest spec covers only the pure helpers.
    • A non-empty case would need a seeded run. There is no API to create one, so this is acceptable for now.
  4. df-scheduler src/Models/SchedulerTask.php:66-87: N+1 on the list endpoint (round 1, still open). run_status costs 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.
  5. df-scheduler SchedulerTask.php:85 and df-admin-interface utils/run-status.ts:27: stale open run shows an empty failure code (round 1, still open). An open run older than 24 h gives is_running = false and status_code = null. The list cell then renders "Failed , 2 days ago" (empty code). In practice startRun closes such rows on the next tick, so the window is small.
  6. df-admin-interface 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 without output and fetching output on expand.
  7. df-admin-interface df-scheduler-details.component.ts:29,99: dead code. isOk is imported and assigned but never used in the template. Drop it.
  8. Data retention and demo safety (team decision, round 1).
    • Successful-run output was deleted before this change. It is now kept: up to 100 runs × 16 KB per task, and visible in the UI.
    • That output is the full response of the scheduled call, so it can contain API keys or PII. It is collapsed by default.
    • Mention this in the PR description so the team decides on purpose.
  9. Merge conflict with open PR df-admin-interface#553. It appends to the same spot in jest.config.ci.js. The conflict is trivial: keep both entries.

@nicdavidson

Copy link
Copy Markdown
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant