Basekick-Labs / Basekick-Labs/arc
A long but legal source key makes every backup fail permanently, because the destination key overruns the limit
- Dominant language
- Go
- Stars
- 677
- Forks
- 53
- Avg merge
- 9h 14m
- Merged PRs (30d)
- 164
Description
Found while working #756.
A source key that the storage contract fully accepts can make **every backup fail, permanently**, and leave partial data in the backup destination with no manifest.
`internal/backup/backup.go:288` builds the destination as:
```go
destPath := fmt.Sprintf("%s/data/%s", backupID, obj.Path)
```
`backupID` is 31 bytes (`manager.go:100-104`) and `/data/` is 6, so the destination is 37 bytes longer than the source key. `MaxUsableKeyLen` is 1019, so any source key over **982 bytes** produces a destination key the backup backend refuses.
That failure is not classified as skippable. `streamBackupFile` only wraps *source read* failures with `errBackupRead` (`backup.go:295-297`), and the write to backup storage is deliberately fatal, so the whole run aborts:
```
[source key len=984, ValidateKey=]
CreateBackup ERROR: failed to back up db/sss…/f.parquet: failed to write to backup storage:
invalid path: storage: invalid path: key is 1021 bytes, over the 1019-byte limit
```
Every subsequent backup fails the same way, since nothing about the source changes. The destination is left holding whatever was copied before the abort, with no manifest written.
This is the same lesson `MaxUsableKeyLen` exists for, applied one layer up and missed. #744 reserved headroom in the *key contract* because `LocalBackend` appends `.part` to every key it stages; the backup path appends a 37-byte prefix to every key it copies and reserves nothing. Its own comment at `backup.go:25-31` says the fatal classification exists so that "a failure mode added here later is fatal by default until someone marks it skippable", which is the right default and is exactly why this one is fatal.
Worth deciding explicitly rather than patching:
1. Treat a destination-key rejection as skippable, so the run completes and reports the file (it would then flow through the same accounting #756 adds for files that could not be copied). This keeps backups working and makes the gap visible.
2. Shorten the destination layout so the overhead is small and bounded, and document the reserved headroom the way `MaxUsableKeyLen` does.
3. Refuse such keys further upstream, at write time, so they cannot enter storage. This is the strictest and would need care: #741's review lesson was that a stricter validator is its own bug when the storage root legitimately holds names that never went through the create-time rule.
Option 1 plus a documented headroom constant looks right: backups should not stop working because one measurement has a very long name, and an operator needs to know which file is affected.
Related: #756 (backup silently omitting files it could not address, where the accounting this would reuse is added).
Contributor guide
Research direction
Read internal/backup/backup.go:25-31 and :288-297, manager.go:100-104, then review the accounting introduced by #756. Reproduce the 984-byte source-key case and trace how destination-key rejection is classified. Done means the chosen handling is explicit, affected files are reported without permanently aborting future backups, and any reserved headroom is documented.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100