Remove in-memory cache from EncryptedManager

[amirejaz]

Summary

The EncryptedManager loaded secrets from disk once at construction time and served all GetSecret reads from an in-memory syncmap.Map. This meant any write or delete performed by a separate process after construction was invisible to a long-running process — the in-memory snapshot never updated.

• Remove the syncmap.Map cache entirely; GetSecret now reads and decrypts the file on every call, consistent with all other Provider implementations (environment, keyring) which already read live on every access
• SetSecret and DeleteSecret already re-read the file inside the write lock; remove the now-unnecessary cache update calls after each write
• ListSecrets and Cleanup updated to go through readFileSecrets/writeFileSecrets instead of the removed map
• NewEncryptedManager validates the file is readable at startup but no longer populates a cache

Type of change

• Bug fix

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

Does this introduce a user-facing change?

No.

Special notes for reviewers

The syncmap.Map was introduced for performance (nanosecond in-memory reads vs. a file decrypt per call). For a secrets file that is a few KB, AES-GCM decryption is fast enough that the per-call overhead is negligible compared to the correctness benefit. All other Provider implementations already pay this cost on every read.

Generated with Claude Code

[reyortiz3]

are there any concerns with increased I/O now?

This meant any write or delete performed by a separate process after construction was invisible to a long-running process

So, it wasn't possible to update the cache on write/delete then?

[amirejaz]

Good question! Same-process writes and deletes did update the cache — SetSecret and DeleteSecret called e.secrets.Store/e.secrets.Delete after writing to disk, so within a single process the cache stayed consistent. The problem was strictly cross-process: when the CLI writes keySessionAccess to disk, the running server has no way to know — the syncmap only reflects writes that went through that same process instance.

On the I/O concern: the increase is real, but the impact is minimal in practice. The secrets file is tiny (a handful of keys, a few KB), so the OS page cache keeps it in memory after the first access — actual disk reads are rare. The cost is dominated by syscall overhead, roughly microseconds on macOS. For the session auth use case this is one file read per HTTP request to the local API server, which is acceptable.

Alternatives like file watching (inotify/kqueue) or mtime-based cache invalidation would preserve caching while fixing cross-process visibility, but add platform-specific complexity. Given the file is tiny and stays in the page cache, reading on every GetSecret is the simpler and correct choice — the correctness win for cross-process logout signals clearly outweighs the cost.

[codecov]

Codecov Report

❌ Patch coverage is 79.31034% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.89%. Comparing base (8c70343) to head (f1f4ab7).
⚠️ Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/secrets/encrypted.go	79.31%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4645      +/-   ##
==========================================
+ Coverage   68.87%   68.89%   +0.02%     
==========================================
  Files         505      505              
  Lines       52380    52424      +44     
==========================================
+ Hits        36076    36120      +44     
+ Misses      13516    13512       -4     
- Partials     2788     2792       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[reyortiz3]

Thanks for the reply! It makes sense, if any real issue is observed we can implement mtime-based cache invalidation later, but agree with this. Thanks