fix: keep proc liveness usable when a proc file is damaged

Merged
YO
3mo ago
+290-189

Summary

A quality audit of internal/persis/file/proc (chosen as the highest bug-density core package: test ratio 0.16, and it answers "is this DAG run alive?") turned up one production bug and one piece of dead configuration. Net −67 lines of production code; the only additions are two regression tests.

1. One damaged proc file could stop a queue and brick the CLI

Listing proc entries treated any undecodable file as fatal for the whole group:

entry, err := readProcEntryWithRetry(file, groupName, s.staleTime, now)
if err != nil {
    if errors.Is(err, os.ErrNotExist) { continue }
    return nil, err          // <-- one bad file fails the entire group
}

Measured before the fix — one healthy run plus one truncated .proc file dropped in the group directory:

BEFORE corruption: CountAlive=1 err=<nil>
AFTER  corruption: CountAlive=0 err=invalid proc file: ... shorter than the 8-byte heartbeat header
AFTER  corruption: IsRunAlive=false err=invalid proc file: ...
AFTER  corruption: LatestHeartbeat!=nil=true err=<nil>      <-- tolerant path already existed

Blast radius:

  • queue_dispatcher.go:553 — CountAlive failing aborts selectDispatchBatch and marks the queue unavailable, so the queue stops dispatching entirely
  • scheduler.go:216 — breaks IsRunning, blocking scheduled runs
  • internal/cmd/context.go:330 — NewContext validates the proc directory on every command, so one damaged file made the whole CLI unusable
  • runtime/manager.go (5 sites) and humantask/resume.go lose liveness; dagu ps/dagu rm fail

It could not self-heal: the zombie detector reaches RemoveIfStale only through the listing that was failing, so recovery meant deleting the file by hand.

LatestHeartbeat in the same file already skipped exactly this condition with a comment explaining why, so the fix adopts the behavior the package already demonstrated. Validate now means "the proc directory is usable" rather than "every file in it decodes" — which is what its one caller actually wants.

2. Clock skew caused the same group-wide failure, permanently

A heartbeat more than five minutes ahead of the reader was rejected as an invalid file. Since the heartbeat carries the writer's clock and is judged against the reader's, skew between hosts in distributed mode triggered finding 1 — and it never aged out, because the skewed writer kept rewriting the file. A future heartbeat is now treated as not fresh, so the entry stays stale and the zombie detector can reap it. This also makes a garbled timestamp self-healing instead of permanently poisoning the group.

3. proc.heartbeat_sync_interval did nothing

The setting was parsed, defaulted to 10s, aliased from the deprecated scheduler. key, exposed in the JSON schema and as DAGU_PROC_HEARTBEAT_SYNC_INTERVAL, and validated against proc.stale_threshold — while both proc store implementations accepted it as a no-op. An operator could be blocked from starting Dagu by a constraint on a value nothing reads.

Removed with its plumbing. Existing configs carrying the key keep loading (the main decoder ignores unknown keys — ErrorUnused is scoped only to auth.proxy); only the validation that rejected it is gone, which is strictly more permissive.

Cleanups (all subtractive)

  • Collapsed two byte-identical read-retry helpers into one
  • Stopped sleeping after the final retry attempt (wasted 300ms on every exhausted retry)
  • procMetaFromLegacyData → procMetaFromData; it decodes the current format, not just legacy
  • Dropped a local shadowing the builtin copy

Package average cyclomatic complexity 4.5 → 4.6 across 969 → 948 lines with 15 fewer functions.

Not addressed

RemoveIfStale has a check-then-delete window against a process that resumes heartbeating; closing it means holding the store's group lock across the re-read and removal. That adds code, so it is left out of a change whose goal was reduction. Worth a follow-up — it can delete a live run's proc file, making the run invisible to IsRunAlive.

Tests

Two behavior-level regression tests: a damaged file no longer hides a healthy run in its group (and Validate still passes), and a future heartbeat is stale and reapable.

make fmt clean including GOOS=windows. go test -race green on persis/..., cmn/config/..., service/scheduler/..., service/frontend/api/v1/..., cmd/..., and all of intg/... including the distributed and queue suites.

Summary by cubic

Makes proc liveness resilient and conservative: damaged .proc files no longer wedge queues/CLI, undercount alive runs, or mislead retries while files are still being written. Also removes the dead proc.heartbeat_sync_interval from loading/env while keeping it in the schema as deprecated and ignored, and bounds test waits by the test deadline.

  • Bug Fixes

    • internal/persis/file/proc listings skip undecodable .proc files only after they’re abandoned (past the stale threshold); files still being written surface as errors.
    • LatestHeartbeat filters to files that may belong to the requested run (by DAG dir and encoded IDs) and applies the same abandonment gate; unrelated damage no longer fails lookups, and damage that may belong returns an error to prevent duplicates.
    • Validate checks that the proc directory is readable, not that every file decodes.
    • Freshness prefers file write time, then heartbeat: future-dated heartbeats are stale unless the file was written recently.
    • Tests: bounded status and restart wait helpers to the test deadline.

  • Refactors

    • Removed WithHeartbeatSyncInterval, WithProcHeartbeatSyncInterval, and the proc.heartbeat_sync_interval loader/env aliases; the JSON schema now accepts the key as deprecated and ignored so existing configs still validate.
    • Consolidated read-retry helpers and removed the final sleep; cleaned up tests and E2E config.

Summary by CodeRabbit

  • Breaking Changes

    • Removed the deprecated heartbeat_sync_interval setting from process and scheduler configuration.
    • Configuration schemas, environment bindings, and legacy aliases no longer accept this setting.

  • Bug Fixes

    • Improved resilience when reading damaged process state files; valid entries remain accessible.
    • Future-dated heartbeats are now correctly treated as stale.
    • Improved retry behavior by avoiding unnecessary delays after the final attempt.