fix(compaction): configure cycle deadline and account for cancellation

[efegokdemir]

Summary

Fixes #915. Target release: 26.09.2.

Compaction could exhaust a hardcoded 30-minute cycle budget, continue draining cancelled work, and misreport unstarted batches and manifest checks as failures. Fielding a narrow manual run also required measurement-level scope.

• Add compaction.cycle_timeout / ARC_COMPACTION_CYCLE_TIMEOUT, default 30m, with positive Go-duration validation. Scheduled and manual cycles share the setting.
• Stop scheduling on cancellation, interrupt capacity waits, join active workers, and distinguish succeeded, failed, interrupted, and discovered-but-unstarted batches.
• Propagate recovery and eligibility errors while retaining normal delivery deferrals. Preserve recovery progress before returning cancellation.
• Apply manual database/measurement/tier scope to recovery as well as discovery, including spoke namespaces and empty selections.
• Retain unreadable manifests and fail closed instead of constructing a partial manifest cache. Protect outputs on cache hits as well as misses.
• Keep normal cancellation out of adaptive retry/failure logging; distinguish independent operation timeouts from expiration of the parent cycle.
• Document the settings, operational limits, and reproducible native acceptance procedure.

The original implementation is retained. Maintainer follow-up 37b912e addresses all four review findings and adds adversarial coverage.

Validation

• GitHub CI — passed, including the race suite and real MinIO object-storage tests; CLA and Enterprise chart checks also passed.

• go test -tags=duckdb_arrow -race ./... — passed across the repository.

• Cancellation, recovery, scope, and manifest-cache regressions — passed 20 repetitions with the race detector.

• go vet -tags=duckdb_arrow ./... — passed.

• Native Arc build with duckdb_arrow, formatting, and diff checks — passed.

• Actual local Arc process, HTTP APIs, real Parquet files, and real compaction subprocesses — passed the acceptance run below.

Recorded native acceptance run

Check	Result
Three completed hourly-tier cycles	Each reduced 14 files to 2; 7,000 rows per measurement preserved
Failed/interrupted/discovery counts in those three cycles	All zero
Deliberate 100 ms deadline during active work	1 interrupted batch, 5 unstarted batches, 0 failed batches
Retry with 30 s budget	6 successful batches; 42 files to 6; all 84,000 rows preserved
Post-upload recovery using real Parquet output and its raw inputs	All 3,500 rows preserved; 7 consumed raw files and manifest removed
Unrelated measurement during scoped recovery	Its manifest and raw inputs remained untouched
Data equality	Timestamps, tags, values, and multiplicity compared
Unexpected process exits	Zero; configuration restarts were planned and recorded separately
Observed local cgroup max/OOM/OOM-kill event increases	Zero

Memory settings remained 512 MB for the main database and compaction subprocess; threads 2, concurrency 1, batch size 7. Only the deadline changed for the interruption/retry comparison.

Run the native check with:

go build -tags=duckdb_arrow -o /tmp/arc-cycle-test ./cmd/arc
python3 scripts/compaction_cycle_acceptance.py \
  --arc /tmp/arc-cycle-test --output /tmp/arc-cycle-acceptance

The output directory must be new. See test procedure and adversarial matrix and UTC-stamped results.

This validation was performed natively in the development environment as requested by the maintainer, not in a container. The hourly tier was manually triggered; this was not three hours of cron execution. It does not establish production-scale throughput or behavior under a particular Kubernetes/cgroup memory limit. The post-upload recovery case reconstructs a durable interruption state from real Parquet; the deadline case separately interrupts a real running subprocess.

AI assistance was used for implementation, review, and test development. The commands and native validation above were executed locally.

[xe-nvdk]

Requesting changes on 00fd0aa4402c6d24649f7fe42860059487dfc877.

The configurable budget and main worker-cancellation logic are working in the checks below. However, all four findings below need to be addressed, with regression coverage, before we can merge this PR. The outstanding staging and real-workload recovery validation from #915 also needs to be completed and attached before merge.

The concerns are scope and error/progress reporting. This review did not establish a new data-loss defect.

1. P2: genuine recovery and eligibility failures can still report a completed cycle.

   Locations: internal/compaction/manager.go:905, :975, and :1151.

   The finalizer returns failure only when runErr, failed batches, or discovery errors are populated. Manifest recovery errors are logged without contributing to those values. Sync eligibility errors are reduced to a false eligibility result, indistinguishable at this call site from a healthy deferral.

   Two deterministic tests exercise the real manager, with failing dependency adapters. A failure listing recovery manifests followed by an empty target, and a failure querying sync eligibility for a discovered candidate, both return nil and publish:

   status=completed
   discovery_errors=0
   failed_batches=0
   discovered_batches=0
   

   This is an incomplete integration of pre-existing error handling into the new cycle outcome, not a claim that this PR introduced those underlying dependency failures. A completed cycle cannot be used as the acceptance signal proposed in #915 if work was prevented by an infrastructure failure.

   Requested fix: propagate and count recovery/eligibility errors separately from normal eligibility deferrals. Keep file filtering fail-closed. The recovery helper also currently logs and swallows individual manifest recovery errors; account for those as well. Add tests for both failures and for a healthy not-yet-eligible deferral.

2. P2: the new measurement scope does not scope manifest recovery.

   Locations: internal/compaction/manager.go:861 and :975.

   RunCompactionCycleForMeasurement still invokes global RecoverOrphanedManifests before applying the measurement filter. That helper enumerates all manifests and can complete input-file deletion for unrelated measurements/databases.

   A local-storage test seeded a valid recovery manifest for otherdb/othermeasurement, then ran a scoped cycle for db/cpu with no candidates. The scoped cycle deleted the unrelated manifest's input file through recovery. This is legitimate completion of the prior compaction, not evidence of data loss; the defect is scope and budget isolation. Unrelated recovery can consume the selected target's entire cycle budget before its discovery starts.

   The pre-existing database-scoped path also performs global recovery. The new measurement-scoped endpoint inherits that behavior, so it does not yet meet #915's literal acceptance criterion that measurement targeting processes only the requested database/measurement.

   Requested fix: make recovery honor explicit scope without weakening manifest safety. If global recovery is an intentional prerequisite, explicitly revise/document the contract and account for its work/budget separately rather than claiming complete isolation. The current scope tests set ManifestManager to nil and cannot expose this interaction.

3. P2: cancellation discards already completed recovery progress.

   Location: internal/compaction/manager.go:976-984.

   The newly added early return on ctx.Err() occurs before recording recovered. RecoverOrphanedManifests can return both a positive completed count and a cancellation error.

   A deterministic test seeded two valid manifests and cancelled after the first was recovered. One manifest remained, proving one recovery completed, but total_manifests_recover stayed at zero. This is a direct regression from placing the new cancellation return before the existing progress update.

   Requested fix: record the successfully recovered count before returning the cancellation cause. Keep the cycle status cancelled/timed_out. Add a partial-progress regression test.

4. P3: normal cancellation still produces misleading adaptive-failure logs.

   Location: internal/compaction/manager.go:782-819 (existing helper called by the changed worker path).

   A real parent subprocess-runner test paused a helper child after a simulated completed upload and cancelled the cycle. Batch/job counters correctly recorded interruption, but the adaptive runner emitted Compaction failed at minimum batch size, cannot split further at error level. Larger cancelled batches can take the analogous Splitting batch after recoverable failure path before the recursive call notices cancellation.

   Required fix: short-circuit known parent cancellation immediately after CompactPartition returns, before adaptive error classification or retry logging. This is inherited logging behavior that remains inconsistent with the new cancellation contract.

An additional unit-level hardening edge exists at manager.go:1213-1217: a typed context error returned by a batch while the parent context remains live increments interrupted, abandons remaining partition batches, and can still yield status=completed because the finalizer does not consider interrupted counts. The test hook reproduces it, but the current production subprocess boundary normally preserves typed cancellation only from the parent context. I have not established a production path for this case, so it is not included as a confirmed merge-blocking defect. Classification should nevertheless check parent cancellation rather than assume every context-shaped error means the whole cycle expired.

Validation performed:

• Original complete internal/config, internal/compaction, and internal/api suites passed. API tests were rerun with local networking enabled after an existing httptest listener was blocked by the sandbox.
• PR-specific Issue915 tests passed for all three packages, including 10 repetitions with the race detector.
• go vet passed for all three packages; git diff --check passed.
• Review regressions for recovery errors, eligibility errors, and scope failed consistently over five race-enabled repetitions, exposing the findings above.
• A real parent process/child process cancellation test passed over five race-enabled repetitions. It used actual local manifest storage and recovered a simulated post-upload/pre-input-deletion state on the next cycle, preserving its output and deleting consumed inputs and the manifest. The child fixture did not run DuckDB or produce real Parquet; this does not replace full compaction/storage integration or staging verification.
• The partial-recovery cancellation counter regression reproduced independently.
• PR CI was green at review time; PR remains draft and explicitly reports staging as not performed.

Remaining release validation: the requested three completed hourly staging cycles, no OOM/restarts/new memory-limit events, timestamped per-measurement file reduction with unchanged resource settings during timeout comparison, and real-workload recovery validation. Longer cycle budgets also warrant observing interaction with subsequent hourly/daily ticks; existing manager exclusion is preserved, but skipped scheduling is not fair/resumable scheduling.

Before requesting re-review, please provide:

• Fixes and regression tests for findings 1–4, including manifest-enabled measurement-scope tests.
• Updated test results for config, compaction, and API packages, including race checks.
• Staging evidence for the acceptance criteria above and real-workload interruption/recovery verification.
• A description of how any intentional scope-contract changes satisfy #915; a documentation change alone should not silently relax its targeting requirement.

The additional live-parent-context timeout case is identified above as a unit-level hardening concern, not a confirmed production defect.

[xe-nvdk]

I'm working on this changes that I requested, and I'm going to push to your branch.

Superseded by maintainer follow-up 37b912e. All four findings are addressed with regression coverage; the full local duckdb_arrow race suite, repository vet, 20 adversarial repetitions, and the recorded native Arc acceptance run passed. PR description and docs/testing contain the evidence and native-environment limits. Final GitHub CI is still running.

[xe-nvdk]

Pushed a second follow-up, 5ec3be2. My earlier commit 37b912e had two regressions, both found in review:

1. A manifest that reads but does not decode (typically a zero-length file left by a crash before the rename was durable) was retained by recovery and made GetFilesInManifests fail closed. Together that skipped every candidate on the node on every cycle, with nothing ever removing the file. ReadManifest now marks that case with ErrManifestUnparseable. Recovery parks such a manifest under the existing .quarantined suffix, since the path is the only remaining pointer to the partition an operator should inspect, and honors the manual database scope from the path. Candidate filtering skips it and logs once per path, and still fails closed on a transient read failure.
2. Recovery was scoped by tier. Each scheduler runs one tier, so the hourly cycle left a daily orphan for the daily tick, up to a day, or forever once that tier was disabled, while the orphan's output and inputs coexisted in the partition. Recovery now spans every tier and keeps the database/measurement scope, which is what fix(compaction): configure cycle deadline and handle cancellation accurately #915 asks for.

Also removed the dead recoverManifest helper, dropped the one-off local results JSON from docs/testing, and updated the release notes, arc.toml and the testing doc to match. A metric for the new park route is filed separately as #926. Full suite, race runs and 20 repetitions of the new tests pass locally.

Next step is CI on the new head, then merge for 26.09.2. If you want to look over the follow-up before that, the two regressions and their tests are all in internal/compaction/manifest.go and cycle_adversarial_test.go.