fix(filedata): clean up archive temp dirs on error and reject env ID mismatches - #847
Open
kparkinson-ld wants to merge 2 commits into
Open
fix(filedata): clean up archive temp dirs on error and reject env ID mismatches#847kparkinson-ld wants to merge 2 commits into
kparkinson-ld wants to merge 2 commits into
Conversation
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
3 tasks
…e-archive-hardening
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.
Hardens offline-mode archive loading in
internal/filedataagainst temp-directory leaks and environment ID mismatches.Closes SEC-9492, Closes SEC-9499.
envIDis skipped, mirroring the existing RAC-plane mismatch handling.Backend-only change with no UI impact, so no screenshots or staging preview apply.
Requirements
Implementation details
SEC-9492.
newArchiveReadercreated a temp dir withos.MkdirTemp("", "ld-relay-")and then returned every subsequent error without removing it, so a malformed archive, missingchecksum.md5, or checksum mismatch leaked the extracted contents. The post-mkdir work moved intoexpandArchive, and the caller cleans up on any error:The monitor loop only advanced
prevInfoafter a successful load, so an unchanged invalid file was re-extracted (and re-leaked) on every tick.prevInfois now advanced before the load attempt, so a failed load is not retried until size/mtime change again — normal retry-after-change behavior is preserved.SEC-9499.
updatedArchivekeyedlastKnownEnvsby the filename-derived env ID but registered the environment withenvMetadata.params.EnvID. A file namedrogue.jsoncarrying another environment'senvIDtherefore created an environment that could never be deleted, since deletion used the map key. Mismatched entries are now skipped with a newlogMsgEnvHasWrongIDwarning (same shape as the check inautoconfig/stream_manager.go), and teardown deletes withenvData.params.EnvID, which also cleans up environments registered before this change.Out of scope. Aggregate byte/entry extraction limits (SEC-9491) are deliberately not addressed here.
How to test.
go test ./internal/filedata/... -count=1. New tests: temp-dir cleanup for malformed archive / missing checksum / bad checksum; no re-extraction of an unchanged invalid file followed by a successful reload once it changes; env ID mismatch at startup and on reload (asserting the correct delete identity). Each new test was confirmed to fail without the corresponding fix.make lint(0 issues) andmake testpass locally.Link to Devin session: https://app.devin.ai/sessions/da8ab445fc3149dc973227fecb7360e1
Open in Devin Desktop: https://app.devin.ai/desktop/session/da8ab445fc3149dc973227fecb7360e1?variant=devin
Requested by: @kparkinson-ld
Note
Overview
Hardens offline archive loading in
internal/filedata: failed extractions no longer leaveld-relay-temp directories behind, and environments whose filename-derived ID disagrees with the embeddedenvIdare ignored instead of being registered under the wrong identity.Archive reader. Extraction moved into
expandArchive;newArchiveReadercallsos.RemoveAllon the temp dir whenever expansion fails (malformed tar, missing/bad checksum, etc.).Polling reload. The monitor now updates
prevInfoas soon as size/mtime are observed, before attempting a reload. An unchanged invalid file therefore stops triggering repeated extract attempts (and repeated leak warnings) until the file changes again; a later valid update still reloads normally.Environment sync.
updatedArchiveskips entries whereenvMetadata.params.EnvID != envIDand logslogMsgEnvHasWrongID(aligned with autoconfig’s mismatch handling). Removals callDeleteEnvironmentwithenvData.params.EnvID, so teardown matches how the environment was registered—including cleaning up after a prior mismatch bug.New tests cover temp-dir cleanup on load errors, stable invalid-file polling behavior, and env-ID mismatch at startup and on reload.
Reviewed by Cursor Bugbot for commit b931656. Bugbot is set up for automated code reviews on this repo. Configure here.