Skip to content

feat(config,server,cli,watch): support global general.callback_url (#1481) - #1482

Merged
Slach merged 11 commits into
Altinity:masterfrom
prafful1234:feature/general-callback-url
Aug 8, 2026
Merged

feat(config,server,cli,watch): support global general.callback_url (#1481)#1482
Slach merged 11 commits into
Altinity:masterfrom
prafful1234:feature/general-callback-url

Conversation

@prafful1234

Copy link
Copy Markdown
Contributor

Summary

Fixes #1481

This PR introduces general.callback_url and general.callback_timeout configuration options to enable backup completion webhooks across all execution paths: API, CLI, and Watch Mode.

Currently, completion callbacks are only available via the API query parameter (POST /backup/create?callback=...). This PR unifies callback handling into a shared pkg/status dispatcher while preserving strict backward compatibility for existing API consumers.

Changes

  • Config (pkg/config): Added general.callback_url and general.callback_timeout (default 5s), with support for CALLBACK_URL and CALLBACK_TIMEOUT environment variables.
  • Shared Dispatcher (pkg/status): Created SendCallback() supporting CallbackPayload (status, error, operation_id, and optional command / duration).
  • API Fallback (pkg/server): Updated API handler to fall back to general.callback_url if the ?callback= query parameter is missing or empty. Query params still take precedence when provided.
  • CLI Commands (cmd/clickhouse-backup): Added synchronous callback dispatch in CLI lifecycle hooks, reusing existing commandId / operation_id tracking. Errors during dispatch are logged and swallowed so CLI exit codes are never affected by callback failures.
  • Watch Mode (pkg/backup/watch.go): Added per-iteration callback dispatch and status.Current.Start() / Stop() tracking inside the watch loop, ensuring each individual watch cycle sends a clean callback with its own unique operation_id.
  • Docs: Updated configuration references in ReadMe.md and Manual.md, and added an entry to ChangeLog.md.

Key Constraints Respected

  1. API Backward Compatibility: CallbackPayload.Error intentionally retains no omitempty tag so API callback JSON payloads remain byte-identical ("error": "" is always present). command and duration use omitempty.
  2. Resilience: HTTP errors or timeouts during webhook dispatch are caught and logged; they never cause a backup operation or CLI command to fail.
  3. Watch Lifecycle: Callbacks fire per watch cycle (with individual start/stop status tracking), rather than once for the parent process lifetime.

Testing

Followed workflow across all modified packages:

  • pkg/config: Tested parsing from YAML, ENV overrides, and default timeout handling.
  • pkg/status: Tested payload serialization, HTTP timeout enforcement, and non-200 response handling.
  • pkg/server: Tested fallback logic when ?callback= is absent/empty vs explicitly set.
  • cmd/clickhouse-backup: Tested CLI callback dispatch on command success/failure and confirmed non-zero callback failures do not mutate command exit codes.
  • pkg/backup: Tested per-iteration callback firing and error recovery across multiple watch ticks.

All tests pass cleanly:

go test -v ./pkg/... ./cmd/...

prafful1234 and others added 7 commits July 26, 2026 16:20
Provide a shared callback dispatcher with config fallback so one-shot CLI
commands and each watch iteration can notify external systems without
changing the existing API callback JSON contract.
ValidateConfig now errors on durations <= 0 (e.g: 0s)
CallbackTimeoutDuration is always positive after a successful load
API server already notifies via pkg/server
dispatchCLICallback now returns early when --command-id is set
Also extract applyCLICallbacks add allowlist coverage, and drop unused GetOperationId
`startWatchIteration` no longer registers a `status.Current` row per cycle (AsyncStatus.commands is append-only) preventing unbounded memory growth in long-running watch processes
The iteration `operation_id` is now a bare UUID and finish is made idempotent via sync
Once the top-level watch commandId is passed to `CreateToRemote/Rebase` again
`parseCallback` now builds each POST from `context.WithoutCancel` plus `general.callback_timeout`
Canceled caller context cannot kill the notification and callbacks no longer hang without a deadline
Signed-off-by: slach <bloodjazman@gmail.com>
@prafful1234

Copy link
Copy Markdown
Contributor Author

@Slach the failing Test (21.8–26.3) jobs look fixed on master (object-disk streaming fallback).

please approve the CI? Thanks!

@Slach Slach added this to the 2.8.1 milestone Jul 29, 2026
Slach added 2 commits August 7, 2026 13:42
…eature/general-callback-url

# Conflicts:
#	test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot
@Slach

Slach commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the work — the feature itself is useful and the config part is fine. But the code structure needs rework before merge, so I will implement this myself in feature/general-callback-url following the plan below. This PR will be closed once that lands; the config surface (general.callback_url, general.callback_timeout) stays as you designed it.

1. Callback dispatch is duplicated in 3 places

Only the low-level HTTP POST was shared (pkg/status/callback.go, ~20 lines). The dispatch logic now exists three times:

  • pkg/server/callback.go + pkg/server/utils.go:71parseCallback, errorCallback, successCallback, own CallbackResponse type
  • cmd/clickhouse-backup/cli_callback.go:60dispatchCLICallback
  • pkg/backup/watch_callback.go:31dispatchWatchCallback, an almost exact copy of the CLI one

CallbackResponse and status.CallbackPayload are two structs for the same JSON. postCallback (pkg/server/callback.go:85) needs a 4-case type switch to convert between them, plus a legacy json.Marshal branch that the comment itself says is only reached by tests — dead code in production.

2. The CLI wiring is fragile

applyCLICallbacks patches the Action fields of an already-built app, driven by a hardcoded map of 13 command names:

  • a new command silently gets no callback unless someone remembers to update the map;
  • cmd.Action.(func(*cli.Context) error) silently skips on any signature mismatch;
  • Subcommands are not walked, but nested commands exist (server -> watch, main.go:934);
  • newCLIOperationId() returns a fresh UUID that matches nothing — not /backup/status, not the logs. It cannot be correlated by the receiver.

3. The approach I will take instead

status.Current already holds command name, operation id, start/finish and error, and already knows NotFromAPI (pkg/status/status.go:24). The callback should be sent from Stop() (pkg/status/status.go:135) — one place covering API, CLI and watch:

  • pkg/status: keep CallbackPayload and SendCallback; store callback URL + timeout on AsyncStatus (set once from config), keep per-row extra URLs coming from ?callback=. Build the payload from ActionRowStatusduration derives from Start/Finish, no extra timer needed. Filter by command prefix (create, create_remote, upload, download, restore, restore_remote, delete, rebase, rebalance, clean*) so read-only commands like list / tables / status never fire callbacks.
  • cmd/clickhouse-backup: delete cli_callback.go. Just call Start/Stop when command-id == NotFromAPI. No allowlist map, no Action patching.
  • pkg/server: drop CallbackResponse, errorCallback, successCallback and the type switch. parseCallback shrinks to "parse ?callback= into []string", stored on the status row. The 8 handlers stop passing two config values each.
  • pkg/backup: delete watch_callback.go, use the normal Start/Stop per iteration.

4. About the watch memory concern

b6cddb3c bypasses status.Current in watch to avoid unbounded growth of status.commands. That concern is real, but the fix belongs in AsyncStatus: trim finished rows to a bounded number (configurable, default ~1000). That also fixes /backup/status for long-running watch processes, not just callbacks.

5. Smaller points

  • The PR description still claims watch does status.Current.Start()/Stop(). That was removed in b6cddb3c.
  • The API accepts multiple ?callback= URLs, but the general.callback_url fallback supports only one. The asymmetry is not documented.

@Slach

Slach commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

i will fix review comments from my side

@Slach
Slach merged commit c58f576 into Altinity:master Aug 8, 2026
28 checks passed
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.

[FR] Support global general.callback_url for CLI commands and watch mode

2 participants