Transfer backups and install logs #142
Conversation
WalkthroughAdds multipart streaming and verification for server transfers: archive, per-backup files, and install logs are streamed with on-the-fly SHA-256 checksums and verified after receipt. Router delete logic now ignores missing install-log files when removing them. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as router_transfer
participant Archive as Archive (server)
participant FS as Filesystem
Client->>Router: multipart POST (archive, checksum_archive, backup_*, checksum_backup_*, install_logs)
activate Router
alt archive part
Router->>Archive: stream archive (tee -> sha256)
Archive->>FS: write/archive to destination
Router->>Router: record calculated archive checksum
end
alt checksum_archive part
Client->>Router: send expected archive checksum
Router->>Router: record expected archive checksum
end
loop per backup
Router->>Archive: stream backup_<id> (write to disk + sha256)
Archive->>FS: write backup file under server dir
Router->>Router: record calculated backup checksum
Client->>Router: send checksum_backup_<id>
Router->>Router: record expected backup checksum
end
alt install_logs part
Router->>Archive: stream install_logs
Archive->>FS: write install log file (skip if missing)
end
Router->>Router: verify archive & each backup checksums
alt checksums match
Router->>Archive: CreateEnvironment()
Router->>Client: respond success
else mismatch
Router->>Client: respond error (checksum mismatch)
end
deactivate Router
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/transfer/source.go (2)
70-78: UnbufferederrChancan leak the streaming goroutine on HTTP errors or early returns
errChanis unbuffered and only read afterclient.Dosucceeds andres.StatusCode == http.StatusOK. Ifclient.Dofails, or you return early on a non-200 status,PushArchiveToTargetexits without ever reading fromerrChan, while the inner goroutine may still try to send an error, blocking forever.Since only a single error is ever sent, you can fix this safely by making the channel buffered:
- // Create a new goroutine to write the archive to the pipe used by the - // multipart writer. - errChan := make(chan error) + // Create a new goroutine to write the archive to the pipe used by the + // multipart writer. Buffered to avoid blocking if we return early. + errChan := make(chan error, 1)Optionally, you can also rely on the deferred
cancel()/cancel2()and explicitly document that the goroutine must never block on sending more than one value, but the buffered channel is the simplest, robust fix.Also applies to: 150-187
160-173: Multipart error handling logs and wraps the wrong error variableInside the
selectyou checkerr2 := <-errChan, but still log/wraperr(the HTTP request error, which is guaranteed to benilhere):if err != nil || err2 != nil { if err == context.Canceled { return nil, err } t.Log().WithError(err).Debug("failed to send archive to destination") return nil, fmt.Errorf("http error: %w, multipart error: %v", err, err2) }At this point
erris alwaysnil, so you lose the actual multipart error (err2) in logs and in the returned error.A more accurate handling would be:
- if err != nil || err2 != nil { - if err == context.Canceled { - return nil, err - } - - t.Log().WithError(err).Debug("failed to send archive to destination") - return nil, fmt.Errorf("http error: %w, multipart error: %v", err, err2) - } + if err2 != nil { + t.Log().WithError(err2).Debug("failed to send archive to destination") + return nil, fmt.Errorf("multipart error: %w", err2) + }This preserves the real failure cause and avoids misleading logs.
🧹 Nitpick comments (2)
server/transfer/archive.go (2)
37-120: Backup streaming logic is solid; consider optional context-aware cancellationThe backup streaming loop correctly:
- Filters
.tar.gzfiles,- Streams each backup via
io.Copywhile hashing withsha256,- Emits a per-backup checksum field, and
- Logs/send messages for progress.
Two optional improvements:
- Honor
ctxto allow early aborts during long backup transfers (e.g., checkctx.Err()between files and bail appropriately).- Use
defer file.Close()inside the per-backup loop for slightly clearer resource handling (current explicit closes are correct but more error-prone to maintain).Not blocking for merge, but worth considering for large backup sets.
122-162: Install log streaming correctly treated as best-effort; context support is optional
StreamInstallLogs:
- Uses the same log path convention as the rest of the codebase.
- Treats missing or unreadable install logs as non-fatal (logs a warning and returns
nil), which matches the intended “optional” semantics.- Properly closes the file and streams via
io.Copy.If you want to improve it further, you could:
- Check
ctx.Err()before starting the copy to avoid work after cancellation.- Optionally log an info message when logs are skipped due to missing file so operators can distinguish “no logs” vs “failure”.
Current behaviour is correct; these are nice-to-have refinements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
router/router_server.go(1 hunks)router/router_transfer.go(3 hunks)server/transfer/archive.go(3 hunks)server/transfer/source.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
router/router_transfer.go (5)
server/server.go (2)
Server(30-81)New(85-108)router/middleware/middleware.go (1)
CaptureAndAbort(60-63)server/filesystem/filesystem.go (2)
New(37-54)Filesystem(24-34)server/transfer/transfer.go (1)
New(59-69)config/config.go (1)
Get(421-429)
server/transfer/archive.go (4)
server/filesystem/archive.go (1)
Archive(58-78)config/config.go (1)
Get(421-429)server/filesystem/filesystem.go (1)
New(37-54)server/transfer/transfer.go (2)
New(59-69)Transfer(43-56)
server/transfer/source.go (4)
server/transfer/archive.go (1)
Archive(165-169)server/filesystem/archive.go (1)
Archive(58-78)internal/progress/progress.go (1)
Progress(13-21)server/transfer/transfer.go (1)
New(59-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
router/router_server.go (1)
250-255: Install log deletion now correctly treats missing file as non-fatalGuarding the warning with
!os.IsNotExist(err)makes server deletion idempotent and avoids noisy logs when the install log was already removed. No further changes needed here.router/router_transfer.go (2)
149-199: Archive streaming + checksum verification flow looks goodThe new
archive/checksum_archivehandling:
- Streams directly into
ExtractStreamUnsafewhile teeing into a SHA-256 hasher.- Stores the calculated checksum and later compares it to the separately streamed
checksum_archivefield.- Fails fast on missing checksum or mismatch, with useful logging.
This gives a clear, single-source-of-truth hash for the main archive and integrates well with the rest of the transfer flow.
Also applies to: 301-320
208-245: Install logs handling correctly treats failures as non-fatalThe new
install_logscase:
- Creates
<LogDirectory>/installif needed.- Writes logs to
<serverID>.log.- Logs warnings but does not abort the transfer on directory/file creation or write errors.
This aligns well with the “optional install logs” intent and keeps transfer success dependent only on core data (archive/backups), not ancillary logs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/router_transfer.go (1)
209-246: LGTM: Install logs handled gracefully.Non-fatal error handling ensures that install log failures don't abort the entire transfer. File handles are closed properly on error paths.
Note: If
io.Copyfails (line 233), a partial install log file remains on disk. This is likely acceptable for diagnostic logs, but you could optionallyos.Remove(installLogPath)after theinstallLogFile.Close()on line 234 if you prefer cleanup of incomplete artifacts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/router_transfer.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
router/router_transfer.go (4)
router/middleware/middleware.go (1)
CaptureAndAbort(60-63)server/filesystem/filesystem.go (2)
New(37-54)Filesystem(24-34)internal/ufs/filesystem.go (1)
Filesystem(11-168)config/config.go (1)
Get(421-429)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
🔇 Additional comments (7)
router/router_transfer.go (7)
14-14: LGTM: Imports added for new functionality.The
filepathandconfigimports support the new backup directory and install log path construction.Also applies to: 21-21
149-155: Excellent fix: Checksum verification bug resolved.The split maps (
backupChecksumsCalculatedvsbackupChecksumsReceived) correctly address the critical issue flagged in prior reviews where a single map caused self-comparison, preventing detection of backup corruption or tampering.
175-198: LGTM: Archive streaming with in-flight checksum calculation.The TeeReader pattern efficiently calculates the SHA256 checksum during extraction without requiring additional passes or buffering.
247-287: LGTM: Backup streaming with proper checksum tracking.The backup streaming correctly:
- Calculates checksums in-flight using TeeReader
- Stores calculated checksums in the dedicated map
- Closes file handles on both success and error paths
- Treats backup errors as fatal, appropriately aborting the transfer
The path traversal concern for
backupName(line 248) was previously discussed and dismissed since the multipart request originates only from trusted Wings instances.
199-208: LGTM: Checksum reception properly separated.Archive and backup checksums are correctly stored in dedicated "received" variables/maps, maintaining clear separation from calculated checksums for subsequent verification.
Also applies to: 288-298
302-341: Excellent: Checksum verification logic is now correct.The verification correctly:
- Compares archive calculated vs received checksums (lines 310-317)
- Iterates over calculated backup checksums and verifies each against its received counterpart (lines 323-340)
- Detects missing checksums for received backups (line 325)
- Aborts on any mismatch with detailed logging
This implementation properly fixes the critical bug and will now detect corruption or tampering in transferred backups.
343-365: LGTM: Completion logic ensures transfer integrity.The final checks ensure the archive was received (line 343), environment setup succeeds (line 353), and the success flag is set only when all verification passes. The deferred cleanup function properly handles both success and failure cases.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
server/transfer/archive.go (1)
138-145: EnsureServer.ID()cannot introduce path traversal in backup/log pathsBoth backup and install‑log paths are built via:
backupPath := filepath.Join(config.Get().System.BackupDirectory, a.transfer.Server.ID()) installLogPath := filepath.Join(config.Get().System.LogDirectory, "install", a.transfer.Server.ID()+".log")If
Server.ID()is ever derived from untrusted input and allowed to contain path separators or.., this could enable directory traversal outside the intended backup/log roots. If there isn’t already a central guarantee, consider normalizing/validating IDs (e.g., restricting to a safe character set) before using them in filesystem paths.
🧹 Nitpick comments (3)
server/transfer/source.go (2)
78-145: Archive + backup + install‑log streaming pipeline is functionally sound but end‑error handling can be simplifiedThe main archive pipe +
io.TeeReaderSHA‑256 hashing, followed by conditionalStreamBackupsandStreamInstallLogs, all look correct and match the intended multipart shape. A couple of minor cleanups would make this easier to reason about:
- At the bottom, once
client.Dohas succeeded,errwill always benil, so in theselectblock onlyerrChan’s value matters. You can simplify the combination oferr/err2and avoid logging a nil HTTP error.For example:
- select { - case <-ctx.Done(): - return nil, ctx.Err() - case err2 := <-errChan: - t.Log().Debug("stream completed") - if err != nil || err2 != nil { - if err == context.Canceled { - return nil, err - } - - t.Log().WithError(err).Debug("failed to send archive to destination") - return nil, fmt.Errorf("http error: %w, multipart error: %v", err, err2) - } + select { + case <-ctx.Done(): + return nil, ctx.Err() + case err2 := <-errChan: + t.Log().Debug("stream completed") + if err2 != nil { + t.Log().WithError(err2).Debug("failed to send archive to destination") + return nil, fmt.Errorf("multipart error: %w", err2) + } }This keeps responsibility clear: transport errors are handled immediately after
client.Do, while multipart/streaming errors flow exclusively througherrChan.
140-145:StreamInstallLogscall path is more defensive than needed
Archive.StreamInstallLogsalready treats all internal failures as non‑fatal and always returnsnil, so this check:if err := a.StreamInstallLogs(ctx, mp); err != nil { errChan <- fmt.Errorf("failed to stream install logs: %w", err) return }will never fire. You can either (a) drop the
if err != nilcheck and ignore the return value, or (b) haveStreamInstallLogsactually propagate errors you want to treat as fatal and keep this branch. Right now it reads as if install‑log failures abort the transfer, but they don’t in practice.server/transfer/archive.go (1)
37-132: Backup streaming behavior matches intent; consider minor robustness tweaksThe overall flow in
StreamBackupslooks good:
- Filters on
BackupUUIDsand matchesUUID.tar.gzon disk.- Skips cleanly when the directory is missing or no matches are found.
- Streams each file with a per‑backup SHA‑256 checksum field, and emits clear per‑backup progress messages.
A couple of small points you might consider:
ctxis currently unused. If you want transfers to react more promptly to cancellation, you could checkctx.Err()orselectonctx.Done()inside the loop and abort withctx.Err()when canceled.- Errors opening or copying a specific backup file are treated as fatal for the whole transfer, whereas missing or unreadable install logs are non‑fatal. That difference may be intentional; if backups are considered “optional” in some flows, you may want to mirror the log behavior (warn and continue) or at least log more context before returning.
None of these are blockers; the correctness of the current implementation is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/transfer/archive.go(3 hunks)server/transfer/source.go(3 hunks)server/transfer/transfer.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/transfer/archive.go (2)
config/config.go (1)
Get(421-429)server/transfer/transfer.go (1)
Transfer(43-60)
server/transfer/source.go (3)
server/transfer/archive.go (1)
Archive(177-181)internal/progress/progress.go (1)
Progress(13-21)server/transfer/transfer.go (1)
New(63-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
server/transfer/transfer.go (1)
57-59: BackupUUIDs field wiring looks consistent with backup streaming logicThe semantics (“empty means no backups”) are clear and align with the checks in
Archive.StreamBackups; no issues from this addition.server/transfer/source.go (1)
35-53: Periodic upload progress goroutine looks fineUsing
a.Progress()with a 5‑second ticker and guarding againstnilprogress is reasonable, andctx2/tc.Stop()correctly scope the goroutine to the transfer lifecycle.server/transfer/archive.go (2)
135-174: Install‑log streaming is correctly non‑fatal and aligns with cleanup behavior
StreamInstallLogs’s behavior—skip when the file doesn’t exist, and warn‑and‑continue on open/create/copy failures—matches the stated goal that install‑log issues should not abort a transfer. The log messages give good traceability, and keeping this logic centralized here works well with the caller’s simple invocation.
176-191: Newtransferfield on Archive is wired correctlyAdding
transfer *Transferand initializing it inNewArchivecleanly enablesArchivemethods to reuse logging, messages, and backup selection without changing external callers. No issues here.
2 participants
This pull request enhances the server transfer functionality by adding robust support for streaming backups and install logs, along with improved checksum verification for both archives and backups. The changes ensure data integrity during transfers and provide better error handling and progress reporting.
Backup and Install Log Streaming:
StreamBackupsandStreamInstallLogsto theArchivetype inserver/transfer/archive.go, enabling backups and install logs to be streamed as multipart form data during server transfers. Each backup is streamed with its own checksum for integrity verification, and install logs are included if present. [1] [2]Checksum Verification Improvements:
router/router_transfer.goto verify checksums for both the main archive and each backup file after streaming. The handler now aborts the transfer if any checksum does not match, ensuring data integrity.Transfer Workflow Enhancements:
server/transfer/source.goto stream the main archive, then backups and install logs, each with their own checksum. Progress messages for backup streaming and install logs are sent to the websocket for better visibility. [1] [2] [3]Error Handling and Logging:
Cleanup Logic:
router/router_server.goto only warn if the install log file removal fails for reasons other than the file not existing, reducing unnecessary warnings.Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.