fix: move sessions to user-specific temp dir#178
fix: move sessions to user-specific temp dir#178MartinKolarik merged 2 commits intojsdelivr:masterfrom
Conversation
WalkthroughAdds per-user session storage and a migration to relocate existing sessions from a shared temp path to user-scoped directories. Introduces LocalStorage.MoveSessionsToUserDir(), a getUserID helper (prefers UID, falls back to username, returns "unknown" on failure), and uses a user-suffixed temp directory for sessions/current session paths. Integrates the migration into storage initialization. Updates tests to expect LastMigration = 2. No other public API signatures changed. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![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
🧹 Nitpick comments (2)
storage/migrations.go (1)
60-101: Tighten error handling and partial-move behavior inMoveSessionsToUserDirTwo things here can make failures hard to detect/debug:
- Line 98:
os.RemoveAll(oldSessionsDir)has its error ignored, so we can end up with a stale old sessions tree while the migration is still considered completed.- Lines 91–96: when a per-entry
os.Renamefails, we log and continue, but then still try to remove the wholeoldSessionsDir. IfRemoveAllsucceeds, any sessions that failed to move are silently dropped.Given this is a migration, it would be safer to at least surface/removeAll errors and consider whether a hard failure (or skipping
RemoveAllwhen any rename failed) is preferable to quietly losing those directories.storage/config_test.go (1)
19-23: Avoid hard‑codingLastMigrationcount in testsBoth assertions pin
LastMigrationto2, which will force touching this test every time a new migration is added (or removed). SinceLastMigrationis effectivelylen(_storage.migrations)for a freshly initialized config, you can make the tests less brittle by deriving the expectation:- LastMigration: 2, + LastMigration: len(_storage.migrations),(and same for the second assertion).
This keeps the test aligned with the migration pipeline without manual updates on each new migration.
Also applies to: 49-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
storage/config_test.go(2 hunks)storage/migrations.go(1 hunks)storage/storage.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
storage/migrations.go (1)
storage/storage.go (1)
LocalStorage(16-27)
🔍 Remote MCP DeepWiki
The DeepWiki repository documentation for globalping-cli is not currently indexed. Let me try searching for the repository directly and then search for common storage and migration best practices:Now let me search for information about migration patterns in Go and permission handling for shared directories:Based on my research, I now have sufficient context to provide relevant information for reviewing the PR. Let me compile the findings:
Summary of Additional Context for PR Review
Storage Architecture & Migration Patterns
The globalping-cli storage package includes a Migrate() method and uses MigrationFunc type for migrations, establishing a structured migration pattern already in place. The Config struct includes a LastMigration integer field to track migration state, which is consistent with the test expectations being updated to LastMigration: 2 in the PR.
Multi-User Permission Issues & Rationale
The permission error cited in issue #176 is a well-documented problem in multi-user systems. /tmp is a sticky folder by default, meaning only the file owner can modify it. This explains the original access conflicts: when one user creates a session directory under /tmp/.globalping-cli/sessions/, other users cannot write to that user-owned directory.
The PR's solution of segregating sessions by user-specific paths aligns with Unix best practices. A better solution is creating a new folder in /tmp without touching the permissions on /tmp, which is what the getUserID() helper approach implements—user-scoped subdirectories under the application's temp directory.
Go Temporary Storage Best Practices
Go's standard library provides utilities for creating and managing temporary files and directories in a secure, cross-platform way, and it's important to clean up entire directories using os.RemoveAll(). The MoveSessionsToUserDir() migration appears to follow this pattern by removing the old sessions directory after migration.
Migration Implementation Pattern
The migration approach in the PR is appropriate: The storage package defines a MigrationFunc type and Migrate() method, and adding a new migration step (MoveSessionsToUserDir) to the initialization pipeline maintains consistency with existing patterns.
|
Do we still do cleanup of old files? |
|
if you're referring to #148 then yes |
3 participants
Sessions are now stored in a separate dir for each user.
Old path:
tmp/.globalping-cli/sessions/...New path:
tmp/.globalping-cli-[user id/username]/sessions/...Closes #176