Skip to content

fix(filedata): clean up archive temp dirs on error and reject env ID mismatches - #847

Open
kparkinson-ld wants to merge 2 commits into
v8from
devin/1788215156-offline-archive-hardening
Open

fix(filedata): clean up archive temp dirs on error and reject env ID mismatches#847
kparkinson-ld wants to merge 2 commits into
v8from
devin/1788215156-offline-archive-hardening

Conversation

@kparkinson-ld

@kparkinson-ld kparkinson-ld commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Hardens offline-mode archive loading in internal/filedata against temp-directory leaks and environment ID mismatches.

Closes SEC-9492, Closes SEC-9499.

  • Extraction temp directories are now removed on every archive-load failure, not just on success.
  • An unchanged, invalid archive is no longer re-extracted on every poll tick; a later change to the file still triggers a reload.
  • An archive environment file whose name-derived env ID differs from the embedded envID is skipped, mirroring the existing RAC-plane mismatch handling.
  • Environment teardown uses the identity the environment was registered under, so a previously loaded mismatched environment can still be removed.

Backend-only change with no UI impact, so no screenshots or staging preview apply.

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions
Implementation details

SEC-9492. newArchiveReader created a temp dir with os.MkdirTemp("", "ld-relay-") and then returned every subsequent error without removing it, so a malformed archive, missing checksum.md5, or checksum mismatch leaked the extracted contents. The post-mkdir work moved into expandArchive, and the caller cleans up on any error:

dirPath, err := os.MkdirTemp("", "ld-relay-")
ar, err := expandArchive(filePath, dirPath)
if err != nil { _ = os.RemoveAll(dirPath); return nil, err }

The monitor loop only advanced prevInfo after a successful load, so an unchanged invalid file was re-extracted (and re-leaked) on every tick. prevInfo is 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. updatedArchive keyed lastKnownEnvs by the filename-derived env ID but registered the environment with envMetadata.params.EnvID. A file named rogue.json carrying another environment's envID therefore created an environment that could never be deleted, since deletion used the map key. Mismatched entries are now skipped with a new logMsgEnvHasWrongID warning (same shape as the check in autoconfig/stream_manager.go), and teardown deletes with envData.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) and make test pass 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 leave ld-relay- temp directories behind, and environments whose filename-derived ID disagrees with the embedded envId are ignored instead of being registered under the wrong identity.

Archive reader. Extraction moved into expandArchive; newArchiveReader calls os.RemoveAll on the temp dir whenever expansion fails (malformed tar, missing/bad checksum, etc.).

Polling reload. The monitor now updates prevInfo as 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. updatedArchive skips entries where envMetadata.params.EnvID != envID and logs logMsgEnvHasWrongID (aligned with autoconfig’s mismatch handling). Removals call DeleteEnvironment with envData.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.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@devin-ai-integration devin-ai-integration Bot added the devin-pr Pull request created by Devin AI label Aug 31, 2026
@devin-ai-integration
devin-ai-integration Bot requested a review from a team August 31, 2026 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devin-pr Pull request created by Devin AI exempt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant