fix(cron): handle AuthClient error and cronjobs koanf key mismatch to prevent server crash - #584
Open
parziva-1 wants to merge 5 commits into
Open
fix(cron): handle AuthClient error and cronjobs koanf key mismatch to prevent server crash#584parziva-1 wants to merge 5 commits into
parziva-1 wants to merge 5 commits into
Conversation
added 5 commits
August 7, 2026 17:44
cleanFiles and cleanUploads in pkg/cron/cron.go discarded the error from tgc.AuthClient (`client, _ := tgc.AuthClient(...)`) and proceeded to call tgc.DeleteMessages with a nil *telegram.Client whenever auth failed (e.g. an invalid/expired Telegram session). DeleteMessages ends up calling client.Run(ctx, ...) on the nil client inside tgc.RunWithAuth, which panics with a nil pointer dereference. There is no recover() anywhere on the cron execution path, so the panic propagates and crashes the entire teldrive server process, not just the cron job. Fixes tgdrive#580. - cleanFiles/cleanUploads now check the AuthClient error, log it with channel/user context (no secrets), and skip that channel/session group instead of dereferencing a nil client. - Added CronService.recoverJob, a narrowly-scoped defer+recover wrapper applied to all four cron job bodies (cleanFiles, cleanUploads, updateFolderSize, cleanOldEvents) as defense-in-depth so any future unforeseen panic in a cron job is logged and skipped instead of taking down the server. - cmd/check.go already handles the AuthClient error correctly (`client, err := ...`) and needed no change.
StartCronJobs failing (e.g. gocron-gorm-lock 'worker is required', observed intermittently on cold start despite CronJobConfig.LockerInstance having a non-empty default) was fed into initErrCh, which triggers os.Exit(1) on the whole server. A background maintenance subsystem failing to start should never take down file serving. Retry up to 5 times with exponential backoff (matches the observed pattern of the job succeeding on a later attempt); if all retries fail, log and keep serving files without cron instead of crashing.
ServerCmdConfig.CronJobs had no explicit koanf tag, so its default
key was derived via toKebabCase("CronJobs") = "cron-jobs" (with a
hyphen), while the documented/used config.toml section is [cronjobs]
(no hyphen, one word) and every deployed config file in the wild uses
that spelling. This created two separate branches in the koanf tree:
defaults registered under cron-jobs.*, file values loaded under
cronjobs.*. mapstructure's custom MatchName strips hyphens before
comparing, so both "cronjobs" and "cron-jobs" satisfy the match for
the CronJobs struct field -- which one "wins" during decode depends
on Go's randomized map iteration order, which is reseeded on every
process run. This is the real, deterministic root cause of the
'worker is required' panic in tgdrive#580: CronJobConfig.LockerInstance (and
any other CronJobs field not explicitly set in config.toml, e.g.
CleanUploadsInterval/FolderSizeInterval) would end up as its Go zero
value roughly half the time, purely by chance of that run's map
iteration order, with no relation to the actual file content.
Fix: give the field an explicit koanf:"cronjobs" tag so both the
defaults-registration path and the file-loading path agree on the
same key. Verified deterministic (10/10 runs) with the real deployed
config.toml after the fix, versus previously-intermittent failures.
Complements the earlier nil-client and non-fatal-cron-init fixes:
this is the actual reason cron initialization was failing in the
first place; those two remain valuable defense-in-depth regardless.
Downgraded to Debug level -- these were instrumental in finding the real root cause of tgdrive#580 (the cronjobs koanf tag mismatch) and are cheap, valuable observability for anyone debugging a similar cron init issue in the future. Silent at default log level, same convention as the existing cron.init.completed log line.
Reproduces the exact scenario from the bug report: a config.toml with a [cronjobs] section that sets some fields (enable, clean-files-interval) and leaves the rest to their struct-tag defaults (LockerInstance, CleanUploadsInterval, FolderSizeInterval). Fails without the koanf:"cronjobs" tag on ServerCmdConfig.CronJobs (flakily, depending on map iteration order -- verified by removing the tag locally and re-running); passes deterministically with it.
Author
|
Hi @divyam234 — noticed |
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
1 participant
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.
Description
Fixes the crash from #580 and its actual root cause, found while tracking down why the fix for the reported panic didn't fully resolve cron reliability.
1.
cleanFiles/cleanUploadsnil-client panic (the reported crash)Both discarded the error from
tgc.AuthClient(...)(client, _ := ...) and proceeded to calltgc.DeleteMessages(ctx, client, ...)with a nil client whenever auth failed, causing an uncaught nil-pointer panic that took down the whole process. Now the error is logged and that channel/session group is skipped instead.2. Cron init failure was fatal to the whole server
In
cmd/run.go, any error fromcron.StartCronJobs(including the panic-turned-error above, or the root cause below) was fed intoinitErrCh, which callsos.Exit(1)— a background maintenance subsystem failing to start should never take down file serving. Now it retries up to 5 times with backoff, and if it still fails, logs and keeps serving files without cron instead of crashing.3. The actual root cause of the "worker is required" panic (not covered by the original report)
ServerCmdConfig.CronJobshad no explicitkoanftag, so its key was derived viatoKebabCase("CronJobs")="cron-jobs"(with a hyphen), while every realconfig.tomluses the documented section name[cronjobs](no hyphen). This created two separate branches in the koanf tree: defaults registered undercron-jobs.*, file values loaded undercronjobs.*. SinceMatchNamestrips hyphens before comparing, mapstructure treats both"cronjobs"and"cron-jobs"as valid matches for theCronJobsfield — which one "wins" depends on Go's randomized map iteration order, reseeded every process run. That's the actual, deterministic-root-cause explanation for whyCronJobConfig.LockerInstance(and any other field not explicitly set in the file) would end up empty roughly half the time: not truly random, just an unintended key collision resolved by Go's map iteration randomization.Verified against the reporter's exact scenario (a
config.tomlwith[cronjobs]setting onlyenable/clean-files-interval, everything else left to defaults): 10/10 local runs and 5/5 real container restarts were flaky/broken before this fix, deterministic after it. Also reproduced and fixed a live crash-loop on a production deployment while testing this.Added
TestConfigLoader_PartialCronJobsSectionas a regression test — fails without thekoanf:"cronjobs"tag, passes with it.4. Startup diagnostics (Debug level)
Added a few
Debug-level log lines inStartCronJobs(resolved locker instance, interval values, scheduler job count) — these were what actually surfaced the root cause above and should help anyone debugging a similar cron init issue in the future. Silent at default log level, same convention as the existingcron.init.completedline.Related issues
Fixes #580
Breaking changes
None.
koanf:"cronjobs"matches the section name every shippedconfig.toml/config.sample.tomlalready uses, so existing configs are unaffected either way; this only fixes which key path the defaults/decoder agree on internally.Testing
go test ./...— all unit tests pass (tests/integrationandtests/performancefail as before, unrelated, they require a local Postgres instance).task lint— no new issues in touched files (4 pre-existing issues remain in untouched files:internal/logging/logger.go,pkg/services/upload.go,cmd/check.go).task server— builds cleanly.