audit-rotate: upload progress percent + ETA - #5
Merged
Conversation
A multi-GB audit backup logs speed but no total, so a watcher has no expectation.
audit-rotate now estimates the dump size from the archive table's on-disk size
(pg_table_size = heap + TOAST) and logs it up front ("archive_backup_starting",
estimated_size); the progress monitor takes an optional total and, when known,
adds percent_approx + eta_approx to each upload_in_progress line. Estimate is
approximate (compression), so percent is capped below 100 and labelled _approx;
0 = unknown keeps the old behaviour for pg backup / oci.
pipeline.RunWithRetry / progress.New gain a totalHint (0 for callers that can't
estimate). Unit test for the percent/ETA path (race-checked); smoke-validated the
estimate line end-to-end.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ReARM-Agent: 1420896f-adf5-4843-896f-d863cfcc6528
ReARM-Agentic-Session: 1403ccf5-4b2a-4718-850a-3a42d188532d
The --verify-restore re-download (re-fetch + decrypt + pg_restore -l) was silent -- for a multi-GB archive that's ~10 min of no output, which reads as a hang. Monitor it like the upload, and since the object size is known exactly from HeadObject, report a real (not estimated) percent + ETA. - progress.Monitor: configurable slog event (SetEvent) so a download logs "verify_download_in_progress" not "upload_in_progress"; SetPrecise() marks total as exact -> "percent"/"eta" labels (not "_approx") and no 99.9 cap. - verifyRestorable takes the exact size and tees the download into an atomic counter a Monitor reads; logs "archive_verify_restore_starting" with the download size up front. Both callers pass their HeadObject size. - Tests: precise-mode + custom-event assertions on the emitted log line. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> ReARM-Agent: 1420896f-adf5-4843-896f-d863cfcc6528 ReARM-Agentic-Session: 1403ccf5-4b2a-4718-850a-3a42d188532d
Independent multi-lens review of the progress work surfaced four items; fix them: - ETA int64 overflow: `time.Duration(secs) * time.Second` wraps to a garbage/ negative Duration when the remaining-time estimate exceeds int64-ns range (a near-stall on a multi-GB transfer: a few bytes over the 10s tick). Guard against maxETASeconds and OMIT the ETA instead of logging a wrapped value. Also compute via `time.Duration(secs * float64(time.Second))` to avoid integer-second trunc. - Stall warning is now event-aware: SetEvent takes (progressEvent, stallEvent) so a verify-restore DOWNLOAD stall logs "verify_download_stalled_or_waiting", not the hardcoded "upload_stalled_or_waiting". Defaults unchanged for the upload path. - verify-restore download goroutine now has a defer/recover (mirrors executeStream): guarantees mon.Stop() + pw.CloseWithError + dlErrCh send exactly once on every exit incl. a panic, so the reader never hangs on <-dlErrCh and the monitor never leaks. - Document the "call SetEvent/SetPrecise BEFORE Start()" ordering contract (the fields are read by the monitor goroutine without synchronization). Tests: custom-stall-event assertion + ETA-overflow-omitted assertion. All pass under -race; validated end-to-end (stage + --drop-pending) against Postgres+MinIO. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> ReARM-Agent: 1420896f-adf5-4843-896f-d863cfcc6528 ReARM-Agentic-Session: 1403ccf5-4b2a-4718-850a-3a42d188532d
Collaborator
Author
Independent multi-lens review + live stress testReviewed by 3 independent agents (Go concurrency/lifecycle, numeric edge-cases, regression/API) and stress-tested end-to-end against Postgres 17 + MinIO with a 2.4 GB incompressible audit table. Stress test (both paths, real artifact image):
Findings fixed (commit pushed):
Added unit tests for the custom stall event and the ETA-overflow-omitted path; all pass under Regression conclusion (confirmed): Not fixed (by design / noted):
🤖 Generated with Claude Code |
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.
Small observability follow-up. A multi-GB audit backup currently logs
streamed_so_far+speedbut no total, so someone tailing the logs has no idea how far along a large upload is.What
pg_table_size= heap + TOAST) and logs it up front:archive_backup_startingwithestimated_size.percent_approx+eta_approxto eachupload_in_progressline._approx.pipeline.RunWithRetry/progress.Newgain atotalHintparam;0(unknown) keeps the existing behaviour forpg backup/oci, which stream without a known size.Example
Validated
Unit test for the percent/ETA path (race-checked); smoke-tested the estimate line end-to-end against Postgres + MinIO (the per-line percent needs a >10s upload to tick, which the unit test covers and real multi-GB uploads exercise).
Motivated by a real demo-instance run (an ~1.3 GB+ audit upload with no size expectation in the logs).
🤖 Generated with Claude Code